Adding Store and cleaning Trigger APIs and Create State APIs #331
Conversation
- Introduced a new Store model to manage runtime key-value pairs for graph executions, improving state management. - Updated GraphTemplate and NodeTemplate models to integrate store configuration, allowing for dynamic input handling during graph execution. - Refactored trigger graph functionality to validate required store keys and construct inputs based on node templates. - Removed deprecated create_states functionality and replaced it with a more streamlined approach in the trigger_graph controller. - Enhanced validation logic in NodeTemplate to prevent reserved keywords as identifiers, ensuring better data integrity. - Updated various models and routes to accommodate the new store functionality, improving overall architecture and maintainability.
|
Note Other AI code review bot(s) detectedCodeRabbit 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 0 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the legacy create_states flow with a store-aware trigger flow: introduces Store and StoreConfig models, extends graph/node validation, registers Store with Beanie, refactors next-state generation to resolve store values asynchronously with caching and batching, removes old create_states endpoint and related models, and updates SDK and docs for the new trigger/store API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Routes
participant Controller as trigger_graph
participant GT as GraphTemplate(DB)
participant StoreDB as Store(DB)
participant StateDB as State(DB)
Client->>Routes: POST /namespaces/{ns}/graph/{g}/trigger {store, inputs}
Routes->>Controller: trigger_graph(ns, g, body, req_id)
Controller->>GT: GraphTemplate.get(ns, g)
alt not found / invalid
Controller-->>Routes: HTTP 404/400
Routes-->>Client: Error
else valid
Controller->>Controller: check_required_store_keys(template, body.store)
Controller->>StoreDB: insert_many(store entries for run_id)
Controller->>StateDB: insert(root State with merged inputs)
Controller-->>Routes: {run_id, status=CREATED}
Routes-->>Client: TriggerGraphResponseModel
end
sequenceDiagram
autonumber
participant Task as create_next_states
participant DepStr as DependentString
participant StoreDB as Store(DB)
participant GT as GraphTemplate.store_config
participant Parents as ParentStates
participant Curr as CurrentState
Task->>Task: iterate next nodes
Task->>DepStr: parse dependency token
alt dependency is store.key
Task->>StoreDB: get_value(run_id, ns, graph, key)
alt found
Task-->>Task: use store value
else not found
Task->>GT: check default_values[key]
alt default exists
Task-->>Task: use default
else
Task-->>Task: raise ValueError (missing store key)
end
end
else dependency is current or parent
Task->>Curr: current.outputs[field] or Parents: outputs[field]
end
Task->>Task: build inputs and return State(status=CREATED)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 introduces a fundamental change to how runtime data is managed and accessed within graph executions. It centralizes key-value pair storage through a new 'Store' model and significantly refactors the graph triggering mechanism to leverage this new capability. The changes aim to improve the flexibility, maintainability, and data integrity of graph state management.
Highlights
- New Store Model Introduced: A new 'Store' model has been added to manage runtime key-value pairs for graph executions, significantly improving state management capabilities.
- Refactored Trigger Graph Functionality: The trigger graph functionality has been completely refactored. It now validates required store keys and constructs inputs dynamically based on node templates, replacing the older 'create_states' approach.
- Enhanced Graph and Node Template Integration: GraphTemplate and NodeTemplate models have been updated to integrate store configuration, allowing for more dynamic input handling during graph execution. This includes new validation to prevent 'store' from being used as a reserved keyword for node identifiers.
- Deprecated Create States Functionality Removed: The 'create_states' functionality and its associated models have been removed, streamlining the API and codebase by consolidating state creation into the new trigger graph controller.
- Dependency Resolution Updated: The logic for resolving input dependencies has been updated to support fetching values from the new 'Store' model, ensuring that graph execution can leverage runtime key-value pairs.
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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable architectural improvement by adding a Store model for managing runtime key-value pairs during graph executions. The refactoring of the graph trigger mechanism to use this new store is well-executed, and the removal of the old create_states API cleans up the codebase. The added validation for node identifiers and store dependencies enhances data integrity.
I've provided a few suggestions to improve the implementation further. These include a critical fix for a bug in the store key validation, along with several recommendations to make the code more concise, idiomatic, and efficient. Overall, these are great changes that improve the maintainability and functionality of the state manager.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
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/main.py (1)
8-8: Import bug: wrong async client symbol; app will fail at startup.Use PyMongo’s asyncio client module.
-from pymongo import AsyncMongoClient +from pymongo.asyncio import MongoClient as AsyncMongoClientstate-manager/app/routes.py (1)
76-94: Consider 201 Created for trigger; also dedupe API key checks.Triggering creates a new run_id; 201 fits better. Repeated API key logging/branching can be centralized in a dependency.
state-manager/app/tasks/create_next_states.py (2)
71-75: validate_dependencies falsely rejects 'store' placeholdersWith store support, this validation must allow identifier=="store" and skip the outputs check for it.
- for dependent in dependency_string.dependents.values(): - # 2) For each placeholder, verify the identifier is either current or present in parents - if dependent.identifier != identifier and dependent.identifier not in parents: - raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") + for dependent in dependency_string.dependents.values(): + # 2) Allow special "store" identifier; others must be current or present in parents + if dependent.identifier == "store": + continue + if dependent.identifier != identifier and dependent.identifier not in parents: + raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") ... - else: - parent_state = parents[dependent.identifier] - if dependent.field not in parent_state.outputs: - raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'") + else: + # Skip output-field validation for "store"; it is resolved at runtime + if dependent.identifier != "store": + parent_state = parents[dependent.identifier] + if dependent.field not in parent_state.outputs: + raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'")Also applies to: 76-84
209-211: Optional: handle duplicate inserts for the non-unites batch tooMirror the unites path’s DuplicateKeyError/BulkWriteError handling to avoid noisy failures during races.
- if tasks: - new_states = await asyncio.gather(*tasks) - await State.insert_many(new_states) + if tasks: + new_states = await asyncio.gather(*tasks) + try: + await State.insert_many(new_states) + except (DuplicateKeyError, BulkWriteError): + logger.warning( + f"Caught duplicate key error for new states in namespace={namespace}, " + f"graph={graph_name}. Attempted to insert {len(new_states)} states" + )
📜 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.
📒 Files selected for processing (13)
state-manager/app/controller/create_states.py(0 hunks)state-manager/app/controller/trigger_graph.py(1 hunks)state-manager/app/main.py(2 hunks)state-manager/app/models/create_models.py(0 hunks)state-manager/app/models/db/graph_template_model.py(3 hunks)state-manager/app/models/db/store.py(1 hunks)state-manager/app/models/dependent_string.py(1 hunks)state-manager/app/models/graph_models.py(1 hunks)state-manager/app/models/node_template_model.py(1 hunks)state-manager/app/models/store_config_model.py(1 hunks)state-manager/app/models/trigger_model.py(1 hunks)state-manager/app/routes.py(1 hunks)state-manager/app/tasks/create_next_states.py(3 hunks)
💤 Files with no reviewable changes (2)
- state-manager/app/models/create_models.py
- state-manager/app/controller/create_states.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T17:49:27.930Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#173
File: state-manager/app/tasks/verify_graph.py:57-74
Timestamp: 2025-08-09T17:49:27.930Z
Learning: In the GraphTemplate model in state-manager/app/models/db/graph_template_model.py, the `secrets` field is defined as `Dict[str, str] = Field(default_factory=dict)`, which ensures it's always initialized to at least an empty dictionary and can never be None.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (8)
state-manager/app/models/trigger_model.py (1)
state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-13)
state-manager/app/controller/trigger_graph.py (6)
state-manager/app/models/trigger_model.py (2)
TriggerGraphRequestModel(4-6)TriggerGraphResponseModel(8-10)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-13)state-manager/app/models/db/state.py (2)
State(13-97)insert_many(55-61)state-manager/app/models/db/store.py (1)
Store(5-36)state-manager/app/models/db/graph_template_model.py (3)
GraphTemplate(17-340)get(313-317)get_root_node(281-285)state-manager/app/models/node_template_model.py (1)
NodeTemplate(17-76)
state-manager/app/models/db/graph_template_model.py (2)
state-manager/app/models/store_config_model.py (1)
StoreConfig(3-20)state-manager/app/models/dependent_string.py (1)
get_identifier_field(63-65)
state-manager/app/models/db/store.py (1)
state-manager/app/models/db/graph_template_model.py (1)
Settings(32-39)
state-manager/app/main.py (1)
state-manager/app/models/db/store.py (1)
Store(5-36)
state-manager/app/models/graph_models.py (1)
state-manager/app/models/store_config_model.py (1)
StoreConfig(3-20)
state-manager/app/tasks/create_next_states.py (3)
state-manager/app/models/db/store.py (2)
Store(5-36)get_value(27-36)state-manager/app/models/node_template_model.py (1)
NodeTemplate(17-76)state-manager/app/models/dependent_string.py (5)
DependentString(9-65)create_dependent_string(24-45)get_identifier_field(63-65)set_value(57-61)generate_string(14-21)
state-manager/app/routes.py (2)
state-manager/app/models/trigger_model.py (2)
TriggerGraphRequestModel(4-6)TriggerGraphResponseModel(8-10)state-manager/app/controller/trigger_graph.py (1)
trigger_graph(41-85)
🔇 Additional comments (8)
state-manager/app/models/graph_models.py (1)
7-7: LGTM: store_config added with safe defaults.Import and new fields look correct; default_factory ensures backward compatibility for existing clients.
Also applies to: 14-15, 21-21
state-manager/app/models/db/graph_template_model.py (1)
25-25: LGTM: persisted store_config with default_factory.Keeps templates backward compatible while enabling store-key validation.
state-manager/app/main.py (1)
45-46: Beanie registration with Store looks correct.No issues spotted here.
state-manager/app/models/trigger_model.py (1)
8-10: Response model LGTM.state-manager/app/routes.py (1)
12-14: Import rewiring to trigger flow looks good.state-manager/app/controller/trigger_graph.py (2)
12-12: Logger initialization OK.
65-76: State creation looks correct.state-manager/app/tasks/create_next_states.py (1)
123-136: Remove unnecessary None-guards for store_config and default_values
GraphTemplate.store_config is declared withField(default_factory=StoreConfig)and StoreConfig.default_values withField(default_factory=dict), so neither can ever be None—these extra guards are redundant.Likely an incorrect or invalid review comment.
- Streamlined the `check_required_store_keys` function to improve validation of required keys in store configurations. - Enhanced input construction in `construct_inputs` using dictionary comprehensions for better readability. - Improved error handling in `trigger_graph` to log and raise appropriate HTTP exceptions for missing graph templates. - Updated `NodeTemplate` and `GraphTemplate` models to include trimming of whitespace in validation, ensuring cleaner input handling. - Added validation for required keys and default values in `StoreConfig`, enhancing data integrity and error reporting. - Refactored `create_next_states` to utilize coroutines for state generation, improving performance and clarity in state management.
|
/gemini review |
- Updated the HTTPException detail message in the trigger_graph function to use a consistent string format, enhancing clarity in error reporting.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable architectural improvement by adding a Store model for managing runtime key-value pairs, which enhances state management during graph executions. The refactoring of the trigger API, removing the old create_states functionality, results in a cleaner and more streamlined interface. The added validation for identifiers and input stripping are also great additions for data integrity and robustness. My review focuses on improving code clarity, removing redundant checks, and making error handling more robust. The introduction of asyncio.gather for concurrent state creation is a commendable performance enhancement.
- Simplified the validation logic in the GraphTemplate model by removing the check for the "store" identifier in the dependency validation loop, enhancing clarity and reducing unnecessary checks. - Improved error reporting for dependencies that are not parents of the current node, ensuring more accurate feedback during validation.
There was a problem hiding this comment.
Actionable comments posted: 6
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/tasks/create_next_states.py (1)
203-205: Fix validate_dependencies to allow 'store' placeholders (prevents KeyError).Store references aren’t in parents; current validation will raise for
${{store.*}}.-def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: +def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: @@ - for dependent in dependency_string.dependents.values(): - # 2) For each placeholder, verify the identifier is either current or present in parents - if dependent.identifier != identifier and dependent.identifier not in parents: + for dependent in dependency_string.dependents.values(): + # Allow store placeholders; they are resolved via Store and not in parents. + if dependent.identifier == "store": + continue + # 2) For each placeholder, verify the identifier is either current or present in parents + if dependent.identifier != identifier and dependent.identifier not in parents: raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") @@ - else: - parent_state = parents[dependent.identifier] - if dependent.field not in parent_state.outputs: - raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'") + else: + parent_state = parents[dependent.identifier] + if dependent.field not in parent_state.outputs: + raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'")Also applies to: 223-225
♻️ Duplicate comments (14)
state-manager/app/models/node_template_model.py (2)
28-31: Simplify empty check (None is redundant).Use truthiness; Pydantic guarantees non-None for str.
- if trimmed_v == "" or trimmed_v is None: + if not trimmed_v: raise ValueError("Node name cannot be empty")
36-41: Harden reserved identifier check (case-insensitive) and simplify emptiness check.Prevents "Store"/"STORE" slipping through; cleaner condition.
- if trimmed_v == "" or trimmed_v is None: + if not trimmed_v: raise ValueError("Node identifier cannot be empty") - elif trimmed_v == "store": - raise ValueError("Node identifier cannot be reserved word 'store'") + elif trimmed_v.lower() == "store": + raise ValueError("Node identifier cannot be a reserved keyword 'store'") return trimmed_vstate-manager/app/models/db/graph_template_model.py (3)
124-128: Simplify empty check for name.- if trimmed_v == "" or trimmed_v is None: + if not trimmed_v: raise ValueError("Name cannot be empty")
132-135: Simplify empty check for namespace.- if trimmed_v == "" or trimmed_v is None: + if not trimmed_v: raise ValueError("Namespace cannot be empty")
242-263: Make 'store' checks case-insensitive, remove dead branch, and avoid repeated parent lookups.Also clarifies error message for undeclared store keys.
- dependent_identifiers = set() - store_fields = set() - - for key, field in dependent_string.get_identifier_field(): - if key == "store": - store_fields.add(field) - else: - dependent_identifiers.add(key) - - for identifier in dependent_identifiers: - if identifier == "store": - continue - - elif identifier not in self.get_parents_by_identifier(node.identifier): - errors.append(f"Input {input_value} depends on {identifier} but {identifier} is not a parent of {node.identifier}") - - for field in store_fields: - if field not in self.store_config.required_keys and field not in self.store_config.default_values: - errors.append(f"Input {input_value} depends on {field} but {field} is not a required key or a default value") + ident_fields = dependent_string.get_identifier_field() + dependent_identifiers = {identifier for identifier, _ in ident_fields if str(identifier).lower() != "store"} + store_fields = {field for identifier, field in ident_fields if str(identifier).lower() == "store"} + + parents = self.get_parents_by_identifier(node.identifier) + for identifier in dependent_identifiers: + if identifier not in parents: + errors.append(f"Input {input_value} depends on {identifier} but {identifier} is not a parent of {node.identifier}") + + for field in store_fields: + if field not in self.store_config.required_keys and field not in self.store_config.default_values: + errors.append(f"Input {input_value} depends on store.{field} but '{field}' is not declared in store_config.required_keys or default_values")state-manager/app/controller/trigger_graph.py (6)
15-21: Return structured 400 for missing store keys (avoid raw set in message).Improves client handling and error readability.
- missing_keys = required_keys - provided_keys - if missing_keys: - raise HTTPException(status_code=400, detail=f"Missing store keys: {missing_keys}") + missing_keys = sorted(required_keys - provided_keys) + if missing_keys: + raise HTTPException( + status_code=400, + detail=[f"Missing store key: {k}" for k in missing_keys], + )
23-25: Align input types with NodeTemplate (Any).NodeTemplate.inputs values are Any; keep merging logic unchanged but fix typing.
-def construct_inputs(node: NodeTemplate, inputs: dict[str, str]) -> dict[str, str]: +def construct_inputs(node: NodeTemplate, inputs: dict[str, Any]) -> dict[str, Any]: return {key: inputs.get(key, value) for key, value in node.inputs.items()}Add import (outside this hunk):
from typing import Any
32-39: Don’t string-match exception messages; map not-found to 404 directly.Log with stack trace; return clear 404.
- try: - graph_template = await GraphTemplate.get(namespace_name, graph_name) - except ValueError as e: - logger.error(f"Graph template not found for namespace {namespace_name} and graph {graph_name}", x_exosphere_request_id=x_exosphere_request_id) - if "Graph template not found" in str(e): - raise HTTPException(status_code=404, detail=f"Graph template not found for namespace {namespace_name} and graph {graph_name}") - else: - raise e + try: + graph_template = await GraphTemplate.get(namespace_name, graph_name) + except ValueError as e: + logger.error(str(e), x_exosphere_request_id=x_exosphere_request_id, exc_info=True) + raise HTTPException(status_code=404, detail=str(e)) from e
56-57: Harden store persistence: catch DB errors and avoid leaking internals.Wrap insert; log with stacktrace; return sanitized 400/500 as appropriate.
- await Store.insert_many(new_stores) + try: + await Store.insert_many(new_stores) + # consider: ordered=False if supported for throughput + except Exception as e: + logger.error("Failed inserting store keys", x_exosphere_request_id=x_exosphere_request_id, exc_info=True) + raise HTTPException(status_code=400, detail="Failed to persist store entries") from e
56-56: Optional: unordered bulk insert for throughput.If supported by Beanie/Motor, pass ordered=False.
- try: - await Store.insert_many(new_stores) + try: + await Store.insert_many(new_stores) # or: await Store.insert_many(new_stores, ordered=False)
78-80: Avoid re-raising raw exceptions (leaks internals); sanitize to 500.Preserve HTTPException as-is; log stack traces.
- except Exception as e: - logger.error(f"Error triggering graph {graph_name} for namespace {namespace_name}", x_exosphere_request_id=x_exosphere_request_id) - raise e + except HTTPException: + raise + except Exception as e: + logger.error(f"Error triggering graph {graph_name} for namespace {namespace_name}", x_exosphere_request_id=x_exosphere_request_id, exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") from estate-manager/app/tasks/create_next_states.py (3)
206-211: Good fix: coroutines are gathered before insert.The previous bug is resolved; inserts now receive concrete states.
214-215: Good: unites path also gathers and guards duplicates.Async batching + duplicate handling is appropriate here.
Also applies to: 231-234
143-156: Rename loop var and add defensive parent-output check.Avoids shadowing and provides clearer error on missing parent/field.
- for identifier, field in dependency_string.get_identifier_field(): + for dep_identifier, field in dependency_string.get_identifier_field(): - if identifier == "store": - dependency_string.set_value(identifier, field, await get_store_value(current_state.run_id, field)) + if dep_identifier == "store": + dependency_string.set_value(dep_identifier, field, await get_store_value(current_state.run_id, field)) - elif identifier == current_state.identifier: + elif dep_identifier == current_state.identifier: if field not in current_state.outputs: raise AttributeError(f"Output field '{field}' not found on current state '{current_state.identifier}' for template '{next_state_node_template.identifier}'") - dependency_string.set_value(identifier, field, current_state.outputs[field]) + dependency_string.set_value(dep_identifier, field, current_state.outputs[field]) else: - dependency_string.set_value(identifier, field, parents[identifier].outputs[field]) + parent = parents.get(dep_identifier) + if parent is None or field not in parent.outputs: + raise AttributeError( + f"Output field '{field}' not found on parent state '{dep_identifier}' for template '{next_state_node_template.identifier}'" + ) + dependency_string.set_value(dep_identifier, field, parent.outputs[field])
📜 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.
📒 Files selected for processing (5)
state-manager/app/controller/trigger_graph.py(1 hunks)state-manager/app/models/db/graph_template_model.py(4 hunks)state-manager/app/models/node_template_model.py(1 hunks)state-manager/app/models/store_config_model.py(1 hunks)state-manager/app/tasks/create_next_states.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T17:49:27.930Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#173
File: state-manager/app/tasks/verify_graph.py:57-74
Timestamp: 2025-08-09T17:49:27.930Z
Learning: In the GraphTemplate model in state-manager/app/models/db/graph_template_model.py, the `secrets` field is defined as `Dict[str, str] = Field(default_factory=dict)`, which ensures it's always initialized to at least an empty dictionary and can never be None.
Applied to files:
state-manager/app/controller/trigger_graph.pystate-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (3)
state-manager/app/controller/trigger_graph.py (6)
state-manager/app/models/trigger_model.py (2)
TriggerGraphRequestModel(4-6)TriggerGraphResponseModel(8-10)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-13)state-manager/app/models/db/state.py (2)
State(13-97)insert_many(55-61)state-manager/app/models/db/store.py (1)
Store(5-36)state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(17-348)get(321-325)is_valid(286-287)get_root_node(289-293)state-manager/app/models/node_template_model.py (1)
NodeTemplate(17-84)
state-manager/app/models/db/graph_template_model.py (2)
state-manager/app/models/store_config_model.py (1)
StoreConfig(3-61)state-manager/app/models/dependent_string.py (1)
get_identifier_field(63-65)
state-manager/app/tasks/create_next_states.py (5)
state-manager/app/models/db/store.py (2)
Store(5-36)get_value(27-36)state-manager/app/models/dependent_string.py (5)
DependentString(9-65)create_dependent_string(24-45)get_identifier_field(63-65)set_value(57-61)generate_string(14-21)state-manager/app/models/node_template_model.py (2)
UnitesStrategyEnum(7-9)NodeTemplate(17-84)state-manager/app/models/db/graph_template_model.py (1)
get(321-325)state-manager/app/models/db/state.py (2)
State(13-97)insert_many(55-61)
🔇 Additional comments (1)
state-manager/app/models/store_config_model.py (1)
7-61: StoreConfig validators look solid.Trim, duplicate, and '.' checks are correct; normalized returns are appropriate.
- Simplified the secrets validation logic by removing unnecessary whitespace trimming, ensuring that the original input is returned after validation. - Enhanced clarity in the validation process by directly iterating over the input dictionary without modification, while maintaining the integrity of the validation checks.
- Deleted the .coverage file as part of cleanup. - Updated unit tests to reflect the removal of the create_states route and adjusted assertions accordingly. - Enhanced TriggerGraphRequestModel validation by incorporating store handling and inputs. - Refactored test cases to ensure compatibility with the new structure and improved error handling for missing store keys in trigger_graph functionality.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
state-manager/tests/unit/models/test_store.py (1)
46-65: Parametrize and tighten assertions; avoid re-patching the class each loop.Improves readability and ensures each case awaits find_one.
- @pytest.mark.asyncio - async def test_get_value_with_different_parameters(self): - """Test get_value method with various parameter combinations""" - test_cases = [ - ("run1", "ns1", "graph1", "key1", "value1"), - ("run2", "ns2", "graph2", "key2", "value2"), - ("", "", "", "", ""), # Edge case with empty strings - ] - - for run_id, namespace, graph_name, key, expected_value in test_cases: - mock_store = MagicMock() - mock_store.value = expected_value - - with patch('app.models.db.store.Store') as mock_store_class: - mock_store_class.find_one = AsyncMock(return_value=mock_store) - - result = await Store.get_value(run_id, namespace, graph_name, key) - - assert result == expected_value + @pytest.mark.asyncio + @pytest.mark.parametrize( + "run_id,namespace,graph_name,key,expected_value", + [ + ("run1", "ns1", "graph1", "key1", "value1"), + ("run2", "ns2", "graph2", "key2", "value2"), + ("", "", "", "", ""), # Edge case with empty strings + ], + ) + async def test_get_value_with_different_parameters(self, run_id, namespace, graph_name, key, expected_value): + """Test get_value method with various parameter combinations""" + mock_store = MagicMock() + mock_store.value = expected_value + with patch.object(Store, "find_one", new_callable=AsyncMock) as find_one: + find_one.return_value = mock_store + result = await Store.get_value(run_id, namespace, graph_name, key) + assert result == expected_value + find_one.assert_awaited()state-manager/tests/unit/test_routes.py (3)
12-14: Use AsyncMock for async controller patchesIf route handlers await controller functions, import AsyncMock so patched callables are awaitable.
-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, AsyncMock, patch
414-429: Make patched trigger_graph awaitable and exercise non-empty payloadPatch with AsyncMock and send a non-empty body to better cover the pass-through.
- @patch('app.routes.trigger_graph') + @patch('app.routes.trigger_graph', new_callable=AsyncMock) async def test_trigger_graph_route_with_valid_api_key(self, mock_trigger_graph, mock_request): @@ - mock_trigger_graph.return_value = MagicMock() - body = TriggerGraphRequestModel() + mock_trigger_graph.return_value = MagicMock() + body = TriggerGraphRequestModel(store={"s1": "v1"}, inputs={"i1": "v2"})
430-445: Mirror AsyncMock + payload for invalid API key pathKeeps behavior consistent and avoids non-awaitable mocks.
- @patch('app.routes.trigger_graph') + @patch('app.routes.trigger_graph', new_callable=AsyncMock) async def test_trigger_graph_route_with_invalid_api_key(self, mock_trigger_graph, mock_request): @@ - body = TriggerGraphRequestModel() + body = TriggerGraphRequestModel(store={"s1": "v1"}, inputs={"i1": "v2"})state-manager/tests/unit/models/test_dependent_string.py (1)
71-86: LGTM; add parsing edge cases (whitespace, multi-placeholders, outputs-form).
This will better exercise create_dependent_string.Append:
+ @pytest.mark.parametrize("syntax, ident, field", [ + ("x_${{ store.config_key }}_y", "store", "config_key"), # whitespace trimmed + ("a_${{store.k1}}_b_${{store.k2}}_c", "store", "k2"), # multi-placeholders + ]) + def test_create_dependent_string_parsing_whitespace_and_multiple(self, syntax, ident, field): + ds = DependentString.create_dependent_string(syntax) + assert ds.dependents # not empty + # last dependent should be accessible and correctly parsed + last_idx = max(ds.dependents.keys()) + d = ds.dependents[last_idx] + assert (d.identifier, d.field) == (ident, field) + + def test_create_dependent_string_outputs_form(self): + ds = DependentString.create_dependent_string("p_${{node1.outputs.result}}_q") + d = ds.dependents[0] + assert (d.identifier, d.field, d.tail) == ("node1", "result", "_q") + + @pytest.mark.parametrize("bad", [ + "p_${{store.key", # not closed + "p_${{node.outputs}}_q", # missing field + "p_${{foo.bar.baz.qux}}_q", # too many parts + ]) + def test_create_dependent_string_invalid_placeholders(self, bad): + with pytest.raises(ValueError): + DependentString.create_dependent_string(bad) + + def test_generate_string_with_multiple_placeholders_ordered(self): + ds = DependentString.create_dependent_string("A_${{store.k1}}_B_${{store.k2}}_C") + ds.set_value("store", "k1", "X") + ds.set_value("store", "k2", "Y") + assert ds.generate_string() == "A_X_B_Y_C"state-manager/app/models/db/graph_template_model.py (1)
15-25: StoreConfig integration: OK; add a lightweight backfill plan.Field with default_factory is safe for new docs; consider a migration to populate store_config on existing documents to avoid surprises during projections/queries.
♻️ Duplicate comments (3)
state-manager/app/models/db/graph_template_model.py (3)
121-128: Simplify None check in name validator.Apply:
- trimmed_v = v.strip() - if trimmed_v == "" or trimmed_v is None: + trimmed_v = v.strip() + if not trimmed_v: raise ValueError("Name cannot be empty") return trimmed_v
129-136: Simplify None check in namespace validator.Apply:
- trimmed_v = v.strip() - if trimmed_v == "" or trimmed_v is None: + trimmed_v = v.strip() + if not trimmed_v: raise ValueError("Namespace cannot be empty") return trimmed_v
241-258: Minor clarity/perf tweaks and clearer store error message.Apply:
- dependent_identifiers = set() - store_fields = set() - - for key, field in dependent_string.get_identifier_field(): - if key == "store": - store_fields.add(field) - else: - dependent_identifiers.add(key) + ident_fields = dependent_string.get_identifier_field() + dependent_identifiers = {k for k, _ in ident_fields if str(k).lower() != "store"} + store_fields = {f for k, f in ident_fields if str(k).lower() == "store"} + parents = self.get_parents_by_identifier(node.identifier) - for identifier in dependent_identifiers: - if identifier not in self.get_parents_by_identifier(node.identifier): + for identifier in dependent_identifiers: + if identifier not in parents: errors.append(f"Input {input_value} depends on {identifier} but {identifier} is not a parent of {node.identifier}") - for field in store_fields: - if field not in self.store_config.required_keys and field not in self.store_config.default_values: - errors.append(f"Input {input_value} depends on {field} but {field} is not a required key or a default value") + for field in store_fields: + if field not in self.store_config.required_keys and field not in self.store_config.default_values: + errors.append(f"Input {input_value} depends on store.{field} but '{field}' is not declared in store_config.required_keys or default_values")
📜 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.
📒 Files selected for processing (11)
state-manager/app/models/db/graph_template_model.py(4 hunks)state-manager/tests/unit/controller/test_create_states.py(0 hunks)state-manager/tests/unit/controller/test_trigger_graph.py(1 hunks)state-manager/tests/unit/models/test_dependent_string.py(1 hunks)state-manager/tests/unit/models/test_graph_template_model.py(2 hunks)state-manager/tests/unit/models/test_node_template_model.py(1 hunks)state-manager/tests/unit/models/test_store.py(1 hunks)state-manager/tests/unit/models/test_store_config_model.py(1 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(1 hunks)state-manager/tests/unit/test_main.py(1 hunks)state-manager/tests/unit/test_routes.py(6 hunks)
💤 Files with no reviewable changes (1)
- state-manager/tests/unit/controller/test_create_states.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-09T17:49:27.930Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#173
File: state-manager/app/tasks/verify_graph.py:57-74
Timestamp: 2025-08-09T17:49:27.930Z
Learning: In the GraphTemplate model in state-manager/app/models/db/graph_template_model.py, the `secrets` field is defined as `Dict[str, str] = Field(default_factory=dict)`, which ensures it's always initialized to at least an empty dictionary and can never be None.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (9)
state-manager/tests/unit/test_main.py (1)
state-manager/app/models/db/store.py (1)
Store(5-36)
state-manager/tests/unit/models/test_store.py (1)
state-manager/app/models/db/store.py (1)
Store(5-36)
state-manager/tests/unit/models/test_node_template_model.py (2)
state-manager/app/models/node_template_model.py (3)
Unites(12-14)UnitesStrategyEnum(7-9)get_dependent_strings(78-84)state-manager/app/models/dependent_string.py (1)
DependentString(9-65)
state-manager/tests/unit/models/test_graph_template_model.py (1)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(17-344)get_valid(324-344)is_valid(282-283)is_validating(291-292)
state-manager/tests/unit/models/test_store_config_model.py (1)
state-manager/app/models/store_config_model.py (3)
StoreConfig(3-61)validate_required_keys(8-33)validate_default_values(36-61)
state-manager/app/models/db/graph_template_model.py (2)
state-manager/app/models/store_config_model.py (1)
StoreConfig(3-61)state-manager/app/models/dependent_string.py (1)
get_identifier_field(63-65)
state-manager/tests/unit/models/test_dependent_string.py (1)
state-manager/app/models/dependent_string.py (7)
DependentString(9-65)Dependent(3-7)generate_string(14-21)_build_mapping_key_to_dependent(47-55)set_value(57-61)get_identifier_field(63-65)create_dependent_string(24-45)
state-manager/tests/unit/controller/test_trigger_graph.py (5)
state-manager/app/controller/trigger_graph.py (1)
trigger_graph(27-80)state-manager/app/models/trigger_model.py (1)
TriggerGraphRequestModel(4-6)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-13)state-manager/app/models/db/graph_template_model.py (2)
is_valid(282-283)get_root_node(285-289)state-manager/app/models/db/state.py (1)
insert_many(55-61)
state-manager/tests/unit/test_routes.py (1)
state-manager/app/models/trigger_model.py (1)
TriggerGraphRequestModel(4-6)
🔇 Additional comments (22)
state-manager/tests/unit/test_main.py (1)
210-210: Good: Store included in Beanie model imports.Matches the new Store Document registration in app.main.
state-manager/tests/unit/test_routes.py (2)
3-3: Switch to new TriggerGraphRequestModel import — LGTMImport now targets app.models.trigger_model, matching the refactor.
29-29: Create-states route assertion removed — LGTMKeeps tests aligned with the deprecated endpoint removal.
state-manager/tests/unit/models/test_dependent_string.py (3)
8-19: LGTM: robust failure-path check.
Good use of match to avoid brittle exact-message coupling.
52-69: LGTM: covers uniqueness of identifier-field pairs.
Assertions are clear and correct.
1-2: Addstate-managerto PYTHONPATH in CI
Ensure tests importapp.models.dependent_stringcorrectly by exportingPYTHONPATH=.:state-manager(or equivalent) before running pytest.state-manager/tests/unit/tasks/test_create_next_states.py (1)
636-636: No-op test change is fine.Trailing newline only; no behavioral impact.
state-manager/tests/unit/models/test_node_template_model.py (4)
9-20: Consider case-insensitive check for reserved identifier "store".If NodeTemplate validation is case-sensitive, add tests (and impl) for "Store"/"STORE".
35-53: LGTM: get_dependent_strings happy-path coverage.
58-69: LGTM: Unites defaults and explicit strategy tested.
71-78: LGTM: Enum string values asserted.state-manager/tests/unit/models/test_graph_template_model.py (3)
170-193: LGTM: validates polling_interval/timeout constraints.
228-242: LGTM: non-validating state raises with clear message.
243-259: LGTM: timeout path covered; patching time/sleep is appropriate.state-manager/tests/unit/models/test_store_config_model.py (4)
8-22: LGTM: default and explicit construction validated.
23-70: LGTM: required_keys validator edge cases comprehensively covered.
82-134: LGTM: default_values validator edge cases comprehensively covered.
135-151: LGTM: integration test ensures end-to-end validation.state-manager/app/models/db/graph_template_model.py (1)
331-334: LGTM: sensible minimum for polling_interval.Matches new async tests; prevents tight loops.
state-manager/tests/unit/controller/test_trigger_graph.py (3)
2-2: LGTM: asyncio mocking import is correct.
5-6: LGTM: imports reflect the new trigger_graph API surface.
13-15: LGTM: fixture aligns with TriggerGraphRequestModel (store + inputs).
- Updated `create_graph_template` and `trigger_graph` functions to include optional store configurations for persisting key-value pairs during execution. - Revised documentation to reflect the new store functionality and its parameters. - Improved `StateManager` methods to support the new store feature, ensuring better state management across graph executions.
- Introduced a new test suite for StateManager, covering trigger functionality with inputs and validation checks. - Added tests for Runtime to ensure proper handling of signals, including PruneSignal and ReQueueAfterSignal. - Implemented mock sessions for testing HTTP interactions and error handling in signal sending. - Updated existing tests to replace state parameters with inputs for consistency across the codebase. - Removed outdated test cases from the StateManager test suite to streamline the testing process.
- Added a warning filter to suppress RuntimeWarnings related to coroutines not being awaited in `test_runtime_validation.py`. - Included a warning filter for Pytest unraisable exceptions in `test_signals_and_runtime_functions.py` to improve test output clarity.
- Introduced a new test class, TestGetStoreValue, to validate the behavior of the get_store_value function. - Implemented multiple test cases to cover scenarios including cache retrieval, default value fallback, and error handling for missing store values. - Ensured that the tests verify correct interactions with the Store and proper handling of different run_id and field combinations for cache isolation.
|
/gemini review |
|
@coderabbitai review this PR |
|
@cursoragent review this pr |
|
@NiveditJain I'll conduct a comprehensive review of this PR. This appears to be a significant architectural change introducing the Store model and refactoring the trigger graph functionality. Let me analyze all the changes thoroughly. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature: the Graph Store, which allows persisting key-value pairs across nodes in a graph run. The refactoring of the trigger API to use inputs and store dictionaries instead of the TriggerState object simplifies the SDK and aligns it with the new backend capabilities. The changes are well-implemented across the backend, SDK, documentation, and tests. The new models like Store and StoreConfig are robust with solid validation, and the asynchronous task handling in create_next_states has been nicely optimized using asyncio.gather. The addition of comprehensive unit tests for the new functionality is also a great contribution to the project's quality.
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
state-manager/tests/unit/tasks/test_create_next_states.py (1)
640-1246: DRY up repeated boilerplate with pytest fixtures/factories.Almost identical setup (GraphTemplate, NodeTemplates, RegisteredNode, create_model, State) is repeated across tests. Introduce fixtures to improve maintainability and readability.
Example fixtures to add near the top of this file:
import types import pytest from unittest.mock import AsyncMock, MagicMock @pytest.fixture def registered_node_fixture(): rn = MagicMock() rn.inputs_schema = {} return rn @pytest.fixture def input_model_fixture(): m = MagicMock() m.model_fields = {} return m @pytest.fixture def state_query_fixture(): find = AsyncMock() state_cls = MagicMock() state_cls.id = "id" state_cls.find.return_value = find state_cls.insert_many = AsyncMock() return state_cls, findThen reuse these fixtures across tests to cut duplication.
python-sdk/tests/test_runtime_validation.py (1)
61-69: Localize warning suppression and restrict by moduleSame here—limit suppression to the specific call sites.
Apply:
-@pytest.mark.filterwarnings("ignore:.*coroutine.*was never awaited.*:RuntimeWarning") def test_runtime_invalid_params_raises(monkeypatch): monkeypatch.setenv("EXOSPHERE_STATE_MANAGER_URI", "http://sm") monkeypatch.setenv("EXOSPHERE_API_KEY", "k") - with pytest.raises(ValueError): - Runtime(namespace="ns", name="rt", nodes=[GoodNode], batch_size=0) - with pytest.raises(ValueError): - Runtime(namespace="ns", name="rt", nodes=[GoodNode], workers=0) + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", + message=".*coroutine.*was never awaited.*", + category=RuntimeWarning, + module=r"exospherehost\..*" + ) + with pytest.raises(ValueError): + Runtime(namespace="ns", name="rt", nodes=[GoodNode], batch_size=0) + with pytest.raises(ValueError): + Runtime(namespace="ns", name="rt", nodes=[GoodNode], workers=0)python-sdk/tests/test_signals_and_runtime_functions.py (1)
719-729: Test doesn’t actually assert basicConfig usage; patch the correct targetCurrently asserting on mock_root_logger.basicConfig can’t catch regressions because logging.basicConfig is module-level. Patch logging.basicConfig and assert not called.
Apply:
-# Mock the root logger to have handlers -with patch('logging.getLogger') as mock_get_logger: - mock_root_logger = MagicMock() - mock_root_logger.handlers = [handler] - mock_get_logger.return_value = mock_root_logger - - # This should return early and not configure logging - _setup_default_logging() - - # Verify no basic config was called - mock_root_logger.basicConfig = MagicMock() - assert not mock_root_logger.basicConfig.called +with patch('logging.getLogger') as mock_get_logger, \ + patch('logging.basicConfig') as mock_basic_config: + mock_root_logger = MagicMock() + mock_root_logger.handlers = [handler] + mock_get_logger.return_value = mock_root_logger + _setup_default_logging() + mock_basic_config.assert_not_called()docs/docs/exosphere/create-graph.md (1)
178-193: Document full store_config surface (beta) and clarify TTL unitsReflect required_keys/default_values and keep TTL explicit to prevent user confusion.
Apply:
- # Create or update the graph template (with optional store, beta) + # Create or update the graph template (with optional store, beta) result = await state_manager.upsert_graph( @@ - }, - store_config={ # beta - "ttl": 7200 # seconds to keep key/values - } + }, + store_config={ # beta + "ttl": 7200, # seconds to retain key/values + "required_keys": ["user_id"], # execution-time keys that must be present + "default_values": { "region": "us-east-1" } # optional defaults + }Additionally, consider adding a bullet in “Graph Validation” noting “Identifiers cannot use reserved keywords” to align with NodeTemplate validation.
docs/docs/exosphere/trigger-graph.md (3)
9-11: Remove stale TriggerState importThe new API no longer uses TriggerState in this example; drop it to avoid confusion.
- from exospherehost import StateManager, TriggerState + from exospherehost import StateManager
3-3: Update introductory text to reflect inputs/store trigger APIReplace “sending trigger states to the state manager” with the new call pattern.
-Once you have created a graph template, you can trigger its execution by sending trigger states to the state manager. This guide shows you how to trigger graphs and monitor their execution. +Once you have created a graph template, you can trigger its execution via StateManager.trigger(...) with inputs and an optional store. This guide shows you how to trigger graphs and monitor their execution.
47-49: Fix typos in “Next Steps”Capitalize Architecture and fix “unites”.
-- **[ARchitecture](./architecture.md)** - Learn about fanout, unites +- **[Architecture](./architecture.md)** - Learn about fanout, unitspython-sdk/README.md (1)
233-244: Remove unused TriggerState from the exampleThe example creates a TriggerState but does not use it. Drop it and the import.
-from exospherehost import StateManager, TriggerState +from exospherehost import StateManager @@ -# Create a single trigger state -trigger_state = TriggerState( - identifier="user-login", - inputs={ - "user_id": "12345", - "session_token": "abc123def456", - "timestamp": "2024-01-15T10:30:00Z" - } -) +# Directly trigger with inputs/store (no TriggerState needed)python-sdk/exospherehost/statemanager.py (2)
124-158: Mirror timeout/handling in get_graphConsistent network behavior across methods.
- async with aiohttp.ClientSession() as session: - async with session.get(endpoint, headers=headers) as response: # type: ignore + timeout = aiohttp.ClientTimeout(total=15) + try: + async with aiohttp.ClientSession(timeout=timeout) as session: + async with session.get(endpoint, headers=headers) as response: # type: ignore if response.status != 200: raise Exception(f"Failed to get graph: {response.status} {await response.text()}") return await response.json() + except aiohttp.ClientError as e: + raise Exception(f"Failed to get graph due to network error: {e}") from e
10-40: Consider deprecating TriggerState in-codeIf kept for BC, mark as deprecated in the docstring to steer users to trigger(...).
class TriggerState(BaseModel): """ - Represents a trigger state for graph execution. + Deprecated: Use StateManager.trigger(graph_name, inputs, store) instead. + + Represents a trigger state for graph execution.
📜 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.
📒 Files selected for processing (11)
docs/docs/exosphere/create-graph.md(2 hunks)docs/docs/exosphere/trigger-graph.md(1 hunks)python-sdk/README.md(2 hunks)python-sdk/exospherehost/statemanager.py(2 hunks)python-sdk/tests/test_coverage_additions.py(1 hunks)python-sdk/tests/test_integration.py(2 hunks)python-sdk/tests/test_runtime_validation.py(3 hunks)python-sdk/tests/test_signals_and_runtime_functions.py(3 hunks)python-sdk/tests/test_state_manager.py(0 hunks)python-sdk/tests/test_statemanager_comprehensive.py(3 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(2 hunks)
💤 Files with no reviewable changes (1)
- python-sdk/tests/test_state_manager.py
🧰 Additional context used
🪛 LanguageTool
python-sdk/README.md
[grammar] ~80-~80: Use correct spacing
Context: ...e pairs across nodes within a single run ## Architecture The SDK is built around tw...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~258-~258: Use correct spacing
Context: ... nodes (beta) } ) ``` Parameters: - graph_name (str): Name of the graph to execute - `i...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~259-~259: Use correct spacing
Context: ...ame(str): Name of the graph to execute -inputs` (dict[str, str] | None): Key/value input...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~260-~260: Use correct spacing
Context: ...inputs for the first node (strings only) - store (dict[str, str] | None): Graph-level key...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~261-~261: Use correct spacing
Context: ...alue store (beta) persisted across nodes Returns: - dict: JSON payload from t...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~263-~263: There might be a mistake here.
Context: ...beta) persisted across nodes Returns: - dict: JSON payload from the state manager *...
(QB_NEW_EN)
[grammar] ~264-~264: Use correct spacing
Context: ...ct: JSON payload from the state manager **Raises:** - Exception`: If the HTTP req...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 Gitleaks (8.27.2)
python-sdk/README.md
250-250: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (10)
state-manager/tests/unit/tasks/test_create_next_states.py (2)
13-13: LGTM: StoreConfig import aligns tests with new Store-aware flow.
640-1246: Remove Store.get_value verification note. Store.get_value is declared as a @staticmethod in state-manager/app/models/db/store.py and is invoked on the class, so tests patching Store.get_value are valid.Likely an incorrect or invalid review comment.
python-sdk/tests/test_integration.py (2)
222-224: Good migration to new trigger signatureUsing inputs=trigger_state.inputs matches the new API. No further changes needed.
454-455: LGTM: failure path aligned with new trigger signatureThe assertion remains correct and uses the updated inputs parameter.
python-sdk/tests/test_statemanager_comprehensive.py (1)
129-131: LGTM: trigger single-state via inputsMatches the simplified API; assertions remain valid.
python-sdk/tests/test_coverage_additions.py (5)
31-47: Nice coverage for trigger defaultsAsserts empty inputs/store payload; aligns with SDK behavior.
60-77: Good enqueue loop cancellation patternUses CancelledError to stop the task and verifies queue population.
119-136: Prune signal handling test is solidPatches send and validates dispatch.
138-155: Requeue-after signal handling test is solidValidates delayed requeue path dispatch.
157-173: Start routine task creation covered wellCancels cleanly after task creation.
- Revised the README to clarify the Graph Store's functionality, specifying it as a strings-only key-value store with per-run scope. - Modified the StateManager to conditionally include retry policy and store configuration in the request body, improving flexibility in handling graph execution parameters.
1 participant
Pending items: