Skip to content

Conversation

@NiveditJain
Copy link
Member

No description provided.

…prove error handling

- Added store_config to the GraphTemplate creation process, ensuring comprehensive data handling.
- Improved error handling by refining the structure of the upsert_graph_template function, enhancing robustness and clarity.
- Included debug print statements for graph_template to aid in troubleshooting during development.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 3, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Warning

Rate limit exceeded

@NiveditJain has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 712235b and c5d3408.

📒 Files selected for processing (2)
  • state-manager/tests/unit/tasks/test_create_next_states.py (1 hunks)
  • state-manager/tests/unit/tasks/test_verify_graph.py (1 hunks)
📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Support configurable store defaults in graph templates and dynamic input resolution using store placeholders.
    • Validation improvements: store-dependent inputs are skipped during dependency checks.
  • Bug Fixes

    • API routes changed from /states/{id}/ to /state/{id}/; SDK endpoints updated accordingly.
    • Prune signal requests now wrap payloads under a top-level “data” key.
  • Tests

    • Expanded unit tests for graph triggering, dependency validation, next-state creation, and routes.
  • Chores

    • Ignore root-level temp files; SDK version bump.

Walkthrough

Adds root-level temp ignore rules; persists a new store_config on GraphTemplate; skips validation for "store" dependents; resolves store-driven inputs at trigger time using defaults; renames per-state endpoints from /states/... to /state/...; updates SDK endpoints and PruneSignal payload shape.

Changes

Cohort / File(s) Summary of changes
Repo housekeeping
\.gitignore
Add comment, root-anchored ignore /temp*, re-include !/temp/.gitkeep.
Graph template model & upsert
state-manager/app/models/db/graph_template_model.py, state-manager/app/controller/upsert_graph_template.py
Add store_config field to GraphTemplate model and constructor; persist store_config on create and update.
Trigger input resolution
state-manager/app/controller/trigger_graph.py
Use DependentString to detect store dependents, read required store keys or fallback to graph_template.store_config.default_values, replace inputs with rendered values, wrap resolution in 400-on-error handling, and create State with resolved inputs.
Graph verification & dependency validation
state-manager/app/tasks/verify_graph.py, state-manager/app/tasks/create_next_states.py
Skip dependency validation for dependents with identifier "store" to avoid node/field lookups and missing-parent errors.
State endpoints (routes)
state-manager/app/routes.py
Rename POST routes: `/states/{state_id}/executed
SDK: runtime endpoints
python-sdk/exospherehost/runtime.py
Update executed/errored notification endpoints to use singular /state/{state_id}/....
SDK: signals payload
python-sdk/exospherehost/signals.py
Wrap PruneSignal payload as {"data": ...} when sending JSON.
Tests
python-sdk/tests/*, state-manager/tests/unit/*
Update tests to expect singular /state/... endpoints and new prune payload shape; add/expand tests for trigger_graph, verify_graph, create_next_states, and retry-policy tolerance for zero delays.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant API as trigger_graph
  participant DepStr as DependentString
  participant Store as Store (DB)
  participant Persister as Run/State Persister

  rect rgb(250,255,245)
    note right of API: Resolve inputs before persisting
    Client->>API: POST /trigger { inputs, graph_id, ... }
    loop per input
      API->>DepStr: construct( input_value )
      DepStr-->>API: dependents list
      alt dependent identifier == "store"
        API->>Store: read store keys/fields
        Store-->>API: store values (or missing)
        API->>API: use store value or graph_template.store_config.default_values
        API->>DepStr: inject values, generate_string()
      else
        API->>API: standard validation (unchanged)
      end
    end
    API->>Persister: persist Run, (optional) Store, create State with resolved inputs
    Persister-->>Client: 201 Created (Run/State)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • nk-ag

Poem

A hop, a nibble, I tweak the store,
I skip the checks and fetch values more.
Defaults fill gaps, inputs render bright —
I persist the run, then bound out of sight. 🥕🐇

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @NiveditJain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request primarily focuses on enhancing the graph template management system by introducing a new configuration field and includes initial setup for development by adding temporary file ignore rules. The changes aim to extend the capabilities of graph template definitions and streamline the development workflow.

Highlights

  • Git Ignore Rules: New rules have been added to the .gitignore file to prevent temporary directories and files (temp/ and temp.*) from being tracked by Git.
  • Graph Template Management Enhancement: The upsert_graph_template function now supports a new 'store_config' field for both updating existing graph templates and creating new ones. Additionally, temporary print statements were introduced for debugging the graph_template object.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for store_config when creating or updating a graph template, and also adds entries to the .gitignore file. While the logic for handling store_config seems correct, there are some debugging print statements that have been left in the code and should be removed before this is merged.

@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
state-manager/app/tasks/create_next_states.py 50.00% 1 Missing ⚠️
state-manager/app/tasks/verify_graph.py 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

- Added a condition to the verify_inputs function in verify_graph.py to continue the loop when the identifier is 'store', preventing unnecessary processing for this case.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
state-manager/app/controller/upsert_graph_template.py (5)

26-34: Reload document after update to avoid returning/staging stale data

Beanie’s update() does not refresh the in-memory document. Without a reload, you may return stale fields and enqueue verify_graph against an outdated template.

                 await graph_template.set_secrets(body.secrets).update(
                     Set({
                         GraphTemplate.nodes: body.nodes, # type: ignore
                         GraphTemplate.validation_status: GraphTemplateValidationStatus.PENDING, # type: ignore
                         GraphTemplate.validation_errors: [], # type: ignore
                         GraphTemplate.retry_policy: body.retry_policy, # type: ignore
                         GraphTemplate.store_config: body.store_config # type: ignore
                     })
                 )
+                await graph_template.reload()

21-24: Don’t log full graph_template; log only identifiers to avoid leaking secrets/PII

Dumping the full document risks exposing secrets or sensitive config. Log namespace, graph name, and doc id instead.

-                logger.info(
-                    "Graph template already exists in namespace", graph_template=graph_template,
-                    namespace_name=namespace_name,
-                    x_exosphere_request_id=x_exosphere_request_id)
+                logger.info(
+                    "Graph template already exists in namespace",
+                    namespace_name=namespace_name,
+                    graph_name=graph_name,
+                    graph_template_id=str(graph_template.id) if graph_template else None,
+                    x_exosphere_request_id=x_exosphere_request_id)

54-56: Message says “validating” but no explicit validation is performed here

Either call GraphTemplate.verify_input_dependencies() before enqueuing, or reword to “Error upserting graph template.”

-            logger.error("Error validating graph template", error=e, x_exosphere_request_id=x_exosphere_request_id)
-            raise HTTPException(status_code=400, detail=f"Error validating graph template: {str(e)}")
+            logger.error("Error upserting graph template", error=e, x_exosphere_request_id=x_exosphere_request_id)
+            raise HTTPException(status_code=400, detail=f"Error upserting graph template: {str(e)}")

Optionally validate immediately after update/insert:

-        except ValueError as e:
+            graph_template.verify_input_dependencies()
+        except ValueError as e:

58-59: Pass only the identifier to background task to ensure fresh read and reduce payload

Prefer passing graph_template.id and having verify_graph load the latest document from DB. This avoids accidental use of stale/mutated in-memory instances.

-        background_tasks.add_task(verify_graph, graph_template)
+        background_tasks.add_task(verify_graph, graph_template.id)

Confirm verify_graph accepts an id and fetches the doc; if not, I can provide a patch.


60-68: Include store_config in the response
Add store_config=graph_template.store_config to UpsertGraphTemplateResponse to match its model.

         return UpsertGraphTemplateResponse(
             nodes=graph_template.nodes,
             validation_status=graph_template.validation_status,
             validation_errors=graph_template.validation_errors,
             secrets={secret_name: True for secret_name in graph_template.get_secrets().keys()},
             retry_policy=graph_template.retry_policy,
+            store_config=graph_template.store_config,
             created_at=graph_template.created_at,
             updated_at=graph_template.updated_at
         )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0d64939 and 54003b3.

📒 Files selected for processing (2)
  • .gitignore (1 hunks)
  • state-manager/app/controller/upsert_graph_template.py (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/app/controller/upsert_graph_template.py (1)
state-manager/app/models/db/graph_template_model.py (1)
  • GraphTemplate (17-344)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
state-manager/app/tasks/verify_graph.py (3)

61-65: Input type check is too strict; handles only bare str.

Pydantic field annotations may be str | None or Annotated[str, ...]. Your is not str check will misclassify valid cases.

Suggested helper (outside this hunk) and usage:

# near top of file
from typing import get_origin, get_args, Union

def _is_str_like(tp) -> bool:
    if tp is str:
        return True
    origin = get_origin(tp)
    if origin is Union:
        return all(t in (str, type(None)) for t in get_args(tp))
    return False

# replace the check
if not _is_str_like(input_info.annotation):
    errors.append(...)

Please confirm Pydantic v2 is in use (model_fields API suggests it is).


33-41: Potential None access on graph_template.secrets.

If graph_template.secrets can be None, iterating .keys() will raise. Guard or default to {}.

Minimal fix (outside this hunk):

for secret_name in (graph_template.secrets or {}).keys():
    present_secrets_set.add(secret_name)

If the model guarantees {} by default, disregard.


98-107: Implement store validation in verify_inputs and add corresponding tests

  • In state-manager/app/tasks/verify_graph.py, within verify_inputs, remove the if identifier == "store": continue guard and instead:
    • Verify field against graph_template.store_config.required_keys and default_values; on missing → error "Store key '{field}' is not configured".
    • Retrieve the runtime store value and ensure it’s a string; on non-string → error "Store key '{field}' value must be a string".
  • Add unit tests in tests/unit/tasks/test_verify_graph.py covering ${store.foo} behavior:
    • Missing key → INVALID with clear error.
    • Existing non-string value → INVALID.
    • Existing string value → VALID.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 54003b3 and 8315cf8.

📒 Files selected for processing (1)
  • state-manager/app/tasks/verify_graph.py (1 hunks)

- Added a condition in the validate_dependencies function to continue the loop when the dependent identifier is 'store', preventing unnecessary validation checks for this case.
- Added logic to validate dependent strings in the trigger_graph function, ensuring that only valid identifiers are processed and that dependencies exist in the provided store.
- Improved error handling to provide clearer feedback when inputs are invalid, enhancing robustness and user experience.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
state-manager/app/tasks/create_next_states.py (3)

126-137: Harden get_store_value against missing store_config and non-string values; align with trigger flow.

If graph_template.store_config or default_values is None, this raises AttributeError. Also, ensure cached values are strings for DependentString.

Apply:

 async def get_store_value(run_id: str, field: str) -> str:
   key = (run_id, field)
   if key not in cached_store_values:
     store_value = await Store.get_value(run_id, namespace, graph_name, field)
-    
-    if store_value is None:
-      store_value = graph_template.store_config.default_values.get(field)
-      if store_value is None:
-        raise ValueError(f"Store value not found for field '{field}' in namespace '{namespace}' and graph '{graph_name}'")
+    defaults = getattr(getattr(graph_template, "store_config", None), "default_values", {}) or {}
+    if store_value is None:
+      store_value = defaults.get(field)
+    if store_value is None:
+      raise ValueError(f"Store value not found for field '{field}' in namespace '{namespace}' and graph '{graph_name}'")
 
-    cached_store_values[key] = store_value
+    cached_store_values[key] = str(store_value)
   return cached_store_values[key]

211-214: Handle duplicate inserts for non-unites path to mirror unites handling.

Insert-many can race and throw DuplicateKeyError/BulkWriteError; currently this would fail the whole batch.

Apply:

-        if len(new_states_coroutines) > 0:
-            await State.insert_many(await asyncio.gather(*new_states_coroutines))
+        if len(new_states_coroutines) > 0:
+            try:
+                await State.insert_many(await asyncio.gather(*new_states_coroutines))
+            except (DuplicateKeyError, BulkWriteError):
+                logger.warning(
+                    f"Caught duplicate key error for new states in namespace={namespace}, "
+                    f"graph={graph_name}, likely due to a race condition. "
+                    f"Attempted to insert {len(new_states_coroutines)} states"
+                )
         await mark_success_states(state_ids)

228-231: Avoid KeyError when resolving unites parent.

If parents is out of sync with parents_ids, this raises KeyError with little context.

Apply:

-            assert next_state_node_template.unites is not None
-            parent_state = parents[next_state_node_template.unites.identifier]
+            assert next_state_node_template.unites is not None
+            parent_state = parents.get(next_state_node_template.unites.identifier)
+            if parent_state is None:
+                raise ValueError(
+                    f"Unites parent '{next_state_node_template.unites.identifier}' not found in parents "
+                    f"for namespace={namespace}, graph={graph_name}"
+                )
state-manager/app/controller/trigger_graph.py (1)

16-23: Treat defaults as satisfying required store keys.

Otherwise trigger_graph rejects runs that create_next_states would accept via default fallback.

Apply:

 def check_required_store_keys(graph_template: GraphTemplate, store: dict[str, str]) -> None:
-    required_keys = set(graph_template.store_config.required_keys)
+    required_keys = set(getattr(getattr(graph_template, "store_config", None), "required_keys", []) or [])
+    defaults = getattr(getattr(graph_template, "store_config", None), "default_values", {}) or {}
     provided_keys = set(store.keys())
 
-    missing_keys = required_keys - provided_keys
+    missing_keys = {k for k in required_keys if k not in provided_keys and k not in defaults}
     if missing_keys:
         raise HTTPException(status_code=400, detail=f"Missing store keys: {missing_keys}")
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8315cf8 and 1c740b5.

📒 Files selected for processing (2)
  • state-manager/app/controller/trigger_graph.py (3 hunks)
  • state-manager/app/tasks/create_next_states.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/app/controller/trigger_graph.py (2)
state-manager/app/models/dependent_string.py (4)
  • DependentString (9-65)
  • create_dependent_string (24-45)
  • set_value (57-61)
  • generate_string (14-21)
state-manager/app/models/db/graph_template_model.py (1)
  • get_root_node (285-289)
🔇 Additional comments (2)
state-manager/app/tasks/create_next_states.py (1)

73-74: Correct to skip validating "store" placeholders here.

Prevents false negatives since store values are injected later. LGTM.

state-manager/app/controller/trigger_graph.py (1)

96-96: Using the resolved inputs for the new State is correct.

Ensures placeholders are materialized before persistence. LGTM.

…instead of plural 'states'

- Updated the endpoint construction in the Runtime class to reflect the change from '/states/{state_id}/executed' to '/state/{state_id}/executed' and similarly for the errored endpoint.
- Modified the route definitions in the state-manager to align with the new singular endpoint structure for executed, errored, and prune state routes.
…ng HTTP request

- Updated the PruneSignal class to wrap the data in a dictionary when making the POST request, improving the structure of the request payload.
…racked

- Enhanced .gitignore to ignore all temporary files and directories while ensuring that the .gitkeep file in the temp directory is not ignored, maintaining the directory structure in the repository.
- Improved clarity of the ignore rules for better maintainability.
- Updated test assertions in `test_runtime_comprehensive.py` and `test_signals_and_runtime_functions.py` to reflect the change from '/states/{state_id}/...' to '/state/{state_id}/...' for executed and errored endpoints.
- Ensured that the data structure in the POST request for prune operations is correctly encapsulated in a dictionary.
…endpoint structure

- Modified assertions in the TestRouteStructure class to replace deprecated plural 'states' routes with singular 'state' routes for executed, errored, prune, and re-enqueue-after endpoints.
- Ensured consistency with recent refactoring of endpoint URLs across the codebase.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
state-manager/app/routes.py (1)

95-101: Add backward-compatible alias routes (plural) to avoid breaking older SDKs

These path changes are fine, but they break clients pinned to plural "/states/{id}/...". Consider adding alias routes with include_in_schema=False that forward to the same handlers.

Example:

 @router.post(
-    "/state/{state_id}/executed",
+    "/state/{state_id}/executed",
     ...
 )
 async def executed_state_route(...):
     ...

+# Back-compat alias (hidden from OpenAPI)
+@router.post("/states/{state_id}/executed", include_in_schema=False, tags=["state"])
+async def executed_state_route_alias(namespace_name: str, state_id: str, body: ExecutedRequestModel, request: Request, background_tasks: BackgroundTasks, api_key: str = Depends(check_api_key)):
+    return await executed_state_route(namespace_name, state_id, body, request, background_tasks, api_key)

Repeat similarly for errored, prune, and re-enqueue-after.

Also applies to: 115-121, 135-141, 154-160

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1c740b5 and eb5d705.

📒 Files selected for processing (5)
  • .gitignore (1 hunks)
  • python-sdk/exospherehost/runtime.py (1 hunks)
  • python-sdk/exospherehost/signals.py (1 hunks)
  • state-manager/app/controller/trigger_graph.py (3 hunks)
  • state-manager/app/routes.py (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
python-sdk/exospherehost/signals.py (1)
python-sdk/tests/test_signals_and_runtime_functions.py (2)
  • post (123-124)
  • post (214-215)
state-manager/app/controller/trigger_graph.py (2)
state-manager/app/models/dependent_string.py (4)
  • DependentString (9-65)
  • create_dependent_string (24-45)
  • set_value (57-61)
  • generate_string (14-21)
state-manager/app/models/db/graph_template_model.py (1)
  • get_root_node (285-289)
🪛 GitHub Actions: Python SDK Tests
python-sdk/exospherehost/signals.py

[error] 1-1: Command: pytest run. PruneSignal.send uses incorrect JSON payload: json={'data': {'reason': '...'}} instead of json={'reason': '...'}.

python-sdk/exospherehost/runtime.py

[error] 1-1: Command: pytest run. Runtime endpoint bug: _get_executed_endpoint uses '/state/{state_id}/executed' instead of '/states/{state_id}/executed'.


[error] 1-1: Command: pytest run. Runtime endpoint bug: _get_errored_endpoint uses '/state/{state_id}/errored' instead of '/states/{state_id}/errored'.

🔇 Additional comments (4)
.gitignore (1)

1-3: LGTM: root-anchored ignore with kept placeholder

Addresses the earlier nit: root-anchored pattern and explicit .gitkeep allowlist. No further changes needed.

state-manager/app/controller/trigger_graph.py (3)

11-11: LGTM: DependentString import

Import aligns with new input resolution logic.


46-48: LGTM: resolve root and construct base inputs

Using construct_inputs before dependent resolution is appropriate.


99-99: LGTM: persist resolved inputs on the State

Using the resolved inputs avoids recomputation downstream.

…nctionality

- Incremented the version in _version.py to 0.0.2b4.
- Added multiple unit tests for the trigger_graph function to cover various scenarios, including handling dependent strings, validation errors, and edge cases with empty or invalid inputs.
- Improved assertions in existing tests to ensure comprehensive coverage and robustness of the trigger_graph functionality.
@coderabbitai coderabbitai bot added the enhancement New feature or request label Sep 3, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
python-sdk/tests/test_runtime_comprehensive.py (1)

185-203: Remove leftover /states/{state_id}/executed references in dashboard

  • dashboard/README.md:213: update or remove the POST /v0/namespace/{namespace}/states/{state_id}/executed entry
  • dashboard/src/services/api.ts:140: replace /v0/namespace/${namespace}/states/${stateId}/executed with the intended endpoint
state-manager/tests/unit/tasks/test_create_next_states.py (1)

231-247: Consider adding a concrete test for DuplicateKeyError/BulkWriteError handling in unit-state insertion.

To lock the new warning path, patch State.insert_many in the unites branch to raise DuplicateKeyError and assert logger.warning, ensuring exceptions are swallowed. I can draft a focused test if helpful.

state-manager/tests/unit/tasks/test_verify_graph.py (2)

10-10: Wrong import source for NodeTemplate (ImportError risk).

NodeTemplate resides in app.models.node_template_model, not in db.graph_template_model.

-from app.models.db.graph_template_model import NodeTemplate
+from app.models.node_template_model import NodeTemplate

348-356: Assert the documented error message instead of catching an AssertionError.

verify_inputs appends an error when a dependent node is missing; it does not assert. Make the expectation explicit.

-            # The function should raise an AssertionError when get_node_by_identifier returns None
-            # Since we can't change the code, we'll catch the AssertionError and verify it's the expected one
-            try:
-                errors = await verify_inputs(graph_template, registered_nodes) # type: ignore
-                # If no AssertionError is raised, that's also acceptable
-                assert isinstance(errors, list)
-            except AssertionError:
-                # The AssertionError is expected when the node is not found
-                pass
+            errors = await verify_inputs(graph_template, registered_nodes)  # type: ignore
+            assert any(
+                "Node missing_node does not exist in the graph template" in e
+                for e in errors
+            )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between eb5d705 and 712235b.

📒 Files selected for processing (8)
  • python-sdk/exospherehost/_version.py (1 hunks)
  • python-sdk/tests/test_runtime_comprehensive.py (1 hunks)
  • python-sdk/tests/test_signals_and_runtime_functions.py (2 hunks)
  • state-manager/tests/unit/controller/test_trigger_graph.py (1 hunks)
  • state-manager/tests/unit/models/test_retry_policy_model_extended.py (1 hunks)
  • state-manager/tests/unit/tasks/test_create_next_states.py (2 hunks)
  • state-manager/tests/unit/tasks/test_verify_graph.py (1 hunks)
  • state-manager/tests/unit/test_routes.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
state-manager/tests/unit/tasks/test_create_next_states.py (2)
state-manager/app/models/db/graph_template_model.py (2)
  • get_valid (324-344)
  • get_node_by_identifier (294-300)
state-manager/app/tasks/create_next_states.py (1)
  • create_next_states (89-250)
state-manager/tests/unit/tasks/test_verify_graph.py (6)
state-manager/app/models/db/registered_node.py (1)
  • list_nodes_by_templates (34-44)
state-manager/app/tasks/verify_graph.py (2)
  • verify_graph (98-127)
  • verify_inputs (43-96)
state-manager/app/models/graph_template_validation_status.py (1)
  • GraphTemplateValidationStatus (4-8)
state-manager/app/models/dependent_string.py (1)
  • get_identifier_field (63-65)
state-manager/app/models/node_template_model.py (2)
  • get_dependent_strings (78-84)
  • NodeTemplate (17-84)
state-manager/app/models/db/graph_template_model.py (1)
  • get_node_by_identifier (294-300)
python-sdk/tests/test_runtime_comprehensive.py (1)
python-sdk/exospherehost/runtime.py (1)
  • _get_errored_endpoint (146-150)
state-manager/tests/unit/controller/test_trigger_graph.py (5)
state-manager/app/models/trigger_model.py (1)
  • TriggerGraphRequestModel (4-6)
state-manager/app/models/db/graph_template_model.py (3)
  • is_valid (282-283)
  • get_root_node (285-289)
  • get (317-321)
state-manager/app/models/dependent_string.py (3)
  • generate_string (14-21)
  • create_dependent_string (24-45)
  • set_value (57-61)
state-manager/app/controller/trigger_graph.py (1)
  • trigger_graph (29-112)
state-manager/app/models/state_status_enum.py (1)
  • StateStatusEnum (4-20)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/tasks/test_create_next_states.py

[error] 15-15: Ruff: F401 'pymongo.errors.DuplicateKeyError' imported but unused.


[error] 15-15: Ruff: F401 'pymongo.errors.BulkWriteError' imported but unused.

state-manager/tests/unit/tasks/test_verify_graph.py

[error] 467-467: Ruff: F841 Local variable 'mock_logger' assigned to but never used.

🔇 Additional comments (5)
state-manager/tests/unit/models/test_retry_policy_model_extended.py (1)

55-55: Allowing zero delay for first retry is correct.

Change aligns with strategies that can legitimately return 0 on first attempt.

python-sdk/tests/test_runtime_comprehensive.py (2)

195-196: Singular executed endpoint expectation is correct.

Matches runtime builder change to /state/{id}/executed.


201-202: Singular errored endpoint expectation is correct.

Matches runtime builder change to /state/{id}/errored.

python-sdk/tests/test_signals_and_runtime_functions.py (2)

96-100: PruneSignal payload wrapper (“data”) matches server model.

Request body shape update looks correct.


271-275: Direct send uses wrapped payload as expected.

Aligned with PruneRequestModel({"data": ...}).

- Updated the test_create_next_states.py to improve the test for handling empty state_ids, ensuring that a ValueError is raised and the exception handler is verified.
- Expanded test_verify_graph.py with new tests for validation errors, valid graphs, and various input scenarios, ensuring comprehensive coverage of the verify_graph functionality.
- Improved assertions to validate the behavior of the graph template under different conditions, enhancing the robustness of the tests.
- Removed unused imports in test_create_next_states.py to streamline the test file.
- Updated the logger patch in test_verify_graph.py for improved readability and consistency in mocking.
@NiveditJain NiveditJain merged commit bff9cd6 into exospherehost:main Sep 3, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant