Fixing validation_process for GraphTemplate #292
Conversation
- Updated verification functions to return lists of errors instead of modifying an external list, improving function clarity and usability. - Introduced asyncio.gather for concurrent execution of verification tasks in verify_graph, enhancing performance. - Adjusted function signatures to reflect the new return types, ensuring consistency across the module. These changes streamline the error handling process and optimize the verification workflow.
- Introduced field validators for various attributes in NodeTemplate to ensure non-empty values and uniqueness for node names, identifiers, next nodes, and unites. - Added validation for name and namespace in GraphTemplate to prevent empty values. - Implemented model-level validation to ensure node identifiers are unique and that next node identifiers exist within the graph. - Removed outdated verification functions from verify_graph.py to streamline the validation process. These changes improve data integrity and validation consistency across the models.
|
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 8 minutes and 29 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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReworks GraphTemplate to compute and cache root, parents and paths; adds node- and graph-level validators and secret accessors. Introduces DependentString model for input templates and delegates dependency parsing to it. Parallelizes verification tasks, centralizes settings/secrets, updates create_next_states to use DependentString, and expands CI/tests for DB-backed scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,230,255,0.3)
participant Controller
participant BG as verify_graph (background)
participant Reg as RegisteredNode.list_nodes_by_templates
participant V1 as verify_node_exists
participant V2 as verify_secrets
participant V3 as verify_inputs
participant Graph as GraphTemplate
end
Controller->>BG: schedule verify_graph(graph_template)
BG->>Reg: list_nodes_by_templates(graph_template.nodes)
Reg-->>BG: registered_nodes
par parallel checks
BG->>V1: verify_node_exists(graph_template, registered_nodes)
BG->>V2: verify_secrets(graph_template, registered_nodes)
BG->>V3: verify_inputs(graph_template, registered_nodes)
and
V1-->>BG: errors1
and
V2-->>BG: errors2
and
V3-->>BG: errors3
end
BG->>Graph: set validation_status / validation_errors
Graph-->>BG: saved
sequenceDiagram
autonumber
participant Creator as create_next_states
participant DS as DependentString
participant DB as State DB
Creator->>DS: DependentString.create_dependent_string(syntax)
DS-->>Creator: DependentString(head, dependents)
loop for each (identifier, field)
Creator->>DS: set_value(identifier, field, resolved_value)
end
Creator->>DS: generate_string()
DS-->>Creator: final_string
Creator->>DB: insert new state with final_string
DB-->>Creator: inserted/state_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 significantly refactors and enhances the validation process for GraphTemplate and NodeTemplate models. The core change involves shifting numerous validation checks from the verify_graph task directly into the Pydantic models using field_validator and model_validator. This approach centralizes data integrity checks within the model definitions, ensuring that GraphTemplate and NodeTemplate instances are valid upon creation or modification. The verify_graph task is consequently streamlined, relying on the models for initial data integrity and focusing on higher-level graph topology and dependency validations, leading to a more robust and efficient overall validation system.
Highlights
- Centralized Data Validation: Many validation checks previously performed in the
verify_graphtask have been migrated directly into theGraphTemplateandNodeTemplatePydantic models. This centralizes data integrity rules within the model definitions themselves. - Enhanced Pydantic Model Validation: New Pydantic
field_validatorandmodel_validatordecorators are used withinGraphTemplateto enforce critical constraints, such as ensuring unique node identifiers, validating thatnext_nodesreferences point to existing nodes, and confirming that node namespaces align with the graph's namespace. - Refactored Graph Verification Workflow: The
verify_graph.pymodule has been streamlined by removing redundant validation functions. The remaining asynchronous verification steps now return errors directly, allowing for more efficient error collection usingasyncio.gatherand a cleaner overall verification workflow.
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 refactors the validation logic for GraphTemplate by moving many checks into Pydantic model validators. This is a great improvement for maintainability and clarity. I've found a few issues, including a critical bug related to unreachable code, a high-severity issue with how concurrent task results are handled, and a potential logic regression in namespace validation. I've also included several medium-severity suggestions to improve code clarity, consistency, and efficiency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (6)
state-manager/app/tasks/verify_graph.py (6)
22-50: Remove unreachable legacy block after early returnEverything below the first
return errorsis dead code. It also duplicates validations now handled in models.- return errors - errors = [] - identifier_to_nodes = {} - # First pass: collect all nodes by identifier - for node in nodes: - if node.identifier is None or node.identifier == "": - errors.append(f"Node {node.node_name} in namespace {node.namespace} has no identifier") - continue - - if node.identifier not in identifier_to_nodes: - identifier_to_nodes[node.identifier] = [] - identifier_to_nodes[node.identifier].append(node) - # Check for duplicates and report all nodes sharing the same identifier - for identifier, nodes_with_identifier in identifier_to_nodes.items(): - if len(nodes_with_identifier) > 1: - node_list = ", ".join([f"{node.node_name} in namespace {node.namespace}" for node in nodes_with_identifier]) - errors.append(f"Duplicate identifier '{identifier}' found in nodes: {node_list}") - # Check next_nodes references using the valid identifiers - valid_identifiers = set(identifier_to_nodes.keys()) - for node in nodes: - if node.next_nodes is None: - continue - for next_node in node.next_nodes: - if next_node not in valid_identifiers: - errors.append(f"Node {node.node_name} in namespace {node.namespace} has a next node {next_node} that does not exist in the graph") - return errors + return errors
73-89: Skip DB queries when name lists are emptyAvoid unnecessary round-trips for empty
INfilters.async def get_database_nodes(nodes: list[NodeTemplate], graph_namespace: str) -> list[RegisteredNode]: graph_namespace_node_names = [ node.node_name for node in nodes if node.namespace == graph_namespace ] - graph_namespace_database_nodes = await RegisteredNode.find( - In(RegisteredNode.name, graph_namespace_node_names), - RegisteredNode.namespace == graph_namespace - ).to_list() + graph_namespace_database_nodes: list[RegisteredNode] = [] + if graph_namespace_node_names: + graph_namespace_database_nodes = await RegisteredNode.find( + In(RegisteredNode.name, graph_namespace_node_names), + RegisteredNode.namespace == graph_namespace + ).to_list() exospherehost_node_names = [ node.node_name for node in nodes if node.namespace == "exospherehost" ] - exospherehost_database_nodes = await RegisteredNode.find( - In(RegisteredNode.name, exospherehost_node_names), - RegisteredNode.namespace == "exospherehost" - ).to_list() + exospherehost_database_nodes: list[RegisteredNode] = [] + if exospherehost_node_names: + exospherehost_database_nodes = await RegisteredNode.find( + In(RegisteredNode.name, exospherehost_node_names), + RegisteredNode.namespace == "exospherehost" + ).to_list() return graph_namespace_database_nodes + exospherehost_database_nodes
91-149: Type annotation mismatch for dependency_graph
verify_topologybuilds sets for dependencies. Update annotation todict[str, set[str]](orCollection[str]) for accuracy.-async def verify_inputs(graph_nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], dependency_graph: dict[str, list[str]]) -> list[str]: +async def verify_inputs(graph_nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], dependency_graph: dict[str, set[str]]) -> list[str]:
150-159: Dead code: build_dependencies_graph is unusedRemove or wire it in; current pipeline doesn’t use it.
192-206: Fix mutable default argument in DFS
current_path=[]is shared across calls. UseNoneand initialize per call.- def dfs_visit(current_node: str, parent_node: str | None = None, current_path: list[str] = []): + def dfs_visit(current_node: str, parent_node: str | None = None, current_path: list[str] | None = None): + if current_path is None: + current_path = []
233-266: Add deprecated async shims for missing verify_nodes_ helpers to unblock CI*To satisfy the existing tests in state-manager/tests/unit/tasks/test_verify_graph.py (which import and
awaitthe following functions), append these no-op async shims at the end of state-manager/app/tasks/verify_graph.py:+# Deprecated shims for backward compatibility with existing tests +async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str]) -> None: # pragma: no cover + """ + No-op shim: node-name validation is now enforced via Pydantic validators + in the main verification flow. + """ + return + +async def verify_nodes_namespace( + nodes: list[NodeTemplate], namespace: str, errors: list[str] +) -> None: # pragma: no cover + """ + No-op shim: namespace validation is now enforced via Pydantic validators + in the main verification flow. + """ + return + +async def verify_node_identifiers(nodes: list[NodeTemplate], errors: list[str]) -> None: # pragma: no cover + """ + No-op shim: identifier validation (including duplicates) is now handled + in verify_node_exists and verify_topology. + """ + returnThese three async definitions exactly match the test signatures for:
await verify_nodes_names(nodes, errors)await verify_nodes_namespace(nodes, "test", errors)await verify_node_identifiers(nodes, 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.
📒 Files selected for processing (3)
state-manager/app/models/db/graph_template_model.py(3 hunks)state-manager/app/models/node_template_model.py(2 hunks)state-manager/app/tasks/verify_graph.py(7 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/models/db/graph_template_model.py
🧬 Code graph analysis (2)
state-manager/app/models/db/graph_template_model.py (1)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-60)
state-manager/app/tasks/verify_graph.py (3)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-60)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-174)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/app/tasks/verify_graph.py
[error] 1-1: Step 'uv run pytest tests/unit/ --cov=app --cov-report=xml --cov-report=term-missing -v --junitxml=pytest-report.xml' failed with ImportError: cannot import name 'verify_nodes_names' from 'app.tasks.verify_graph'.
🔇 Additional comments (7)
state-manager/app/models/node_template_model.py (1)
54-60: LGTM: unites identifier non-empty checkValidation is appropriate and consistent with other fields.
state-manager/app/models/db/graph_template_model.py (5)
6-7: LGTM: Pydantic v2 imports
model_validator,field_validator, andSelfusage is correct for Python 3.11.
44-50: LGTM: non-empty name validator
51-57: LGTM: non-empty namespace validator
79-91: LGTM: per-graph unique node identifiersAggregating all duplicates before raising is good.
92-108: LGTM: next_nodes must reference existing identifiersClear and complete.
state-manager/app/tasks/verify_graph.py (1)
12-21: LGTM: existence check against registered nodesSet-diff approach is fine and returns specific errors.
- Introduced private attributes for tracking the root node and parent nodes by identifier in the GraphTemplate model. - Implemented methods to build the root node and parents by identifier, ensuring a single root node and proper parent-child relationships. - Added model-level validation to check for graph connectivity and acyclic structure, enhancing data integrity. - Updated the get_root_node and get_parents_by_identifier methods for improved access to these attributes. These changes improve the structural validation of graphs and ensure that the graph's integrity is maintained during operations.
- Reintroduced validation methods for nodes, ensuring namespace consistency and connectivity to the root node. - Added checks for acyclic structure and existence of unit identifiers within nodes. - Moved the get_node_by_identifier and get_parents_by_identifier methods to the end of the class for better organization. - Removed outdated validation methods to streamline the model. These changes improve the integrity and validation of the GraphTemplate model, ensuring robust graph structure management.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
state-manager/app/tasks/verify_graph.py (2)
73-89: Query both namespaces concurrentlyMinor latency win; avoids two sequential DB calls.
async def get_database_nodes(nodes: list[NodeTemplate], graph_namespace: str) -> list[RegisteredNode]: @@ - graph_namespace_database_nodes = await RegisteredNode.find( - In(RegisteredNode.name, graph_namespace_node_names), - RegisteredNode.namespace == graph_namespace - ).to_list() + graph_ns_coro = RegisteredNode.find( + In(RegisteredNode.name, graph_namespace_node_names), + RegisteredNode.namespace == graph_namespace + ).to_list() @@ - exospherehost_database_nodes = await RegisteredNode.find( - In(RegisteredNode.name, exospherehost_node_names), - RegisteredNode.namespace == "exospherehost" - ).to_list() - return graph_namespace_database_nodes + exospherehost_database_nodes + exo_ns_coro = RegisteredNode.find( + In(RegisteredNode.name, exospherehost_node_names), + RegisteredNode.namespace == "exospherehost" + ).to_list() + graph_namespace_database_nodes, exospherehost_database_nodes = await asyncio.gather(graph_ns_coro, exo_ns_coro) + return graph_namespace_database_nodes + exospherehost_database_nodes
171-174: Honor validation errors before marking graph VALIDCurrently always sets VALID even when errors exist.
- graph_template.validation_status = GraphTemplateValidationStatus.VALID - graph_template.validation_errors = None - await graph_template.save() + if errors: + graph_template.validation_status = GraphTemplateValidationStatus.INVALID + graph_template.validation_errors = errors + await graph_template.save() + return + else: + graph_template.validation_status = GraphTemplateValidationStatus.VALID + graph_template.validation_errors = None + await graph_template.save()
♻️ Duplicate comments (6)
state-manager/app/models/db/graph_template_model.py (3)
133-139: Allow universal “exospherehost” namespace to preserve compatibilityStrict equality blocks valid graphs using shared nodes under “exospherehost”, contradicting get_database_nodes. Relax per prior behavior.
def validate_nodes(self) -> Self: for node in self.nodes: - if node.namespace != self.namespace: + if node.namespace != self.namespace and node.namespace != "exospherehost": raise ValueError(f"Node namespace {node.namespace} does not match graph namespace {self.namespace}") return self
105-118: Use truthiness check for emptiesPydantic ensures non-None for required str; simplify to
if not v.def validate_name(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") return v @@ def validate_namespace(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty") return v
177-205: Combine identifier uniqueness and next_nodes existence into one passReduces iterations and centralizes node-reference errors.
- @field_validator('nodes') - @classmethod - def validate_unique_identifiers(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - errors = [] - for node in v: - if node.identifier in identifiers: - errors.append(f"Node identifier {node.identifier} is not unique") - identifiers.add(node.identifier) - if errors: - raise ValueError("\n".join(errors)) - return v - - @field_validator('nodes') - @classmethod - def validate_next_nodes_identifiers_exist(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - for node in v: - identifiers.add(node.identifier) - - errors = [] - for node in v: - if node.next_nodes: - for next_node in node.next_nodes: - if next_node not in identifiers: - errors.append(f"Node identifier {next_node} does not exist in the graph") - if errors: - raise ValueError("\n".join(errors)) - return v + @field_validator('nodes') + @classmethod + def validate_nodes_references(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: + identifiers: set[str] = set() + errors: list[str] = [] + # pass 1: collect + check duplicates + for node in v: + if node.identifier in identifiers: + errors.append(f"Node identifier {node.identifier} is not unique") + identifiers.add(node.identifier) + # pass 2: verify next_nodes exist + for node in v: + if node.next_nodes: + for next_node in node.next_nodes: + if next_node not in identifiers: + errors.append(f"Node identifier {next_node} does not exist in the graph") + if errors: + raise ValueError("\n".join(errors)) + return vstate-manager/app/tasks/verify_graph.py (3)
22-50: Remove unreachable legacy code after early returnEverything after the first return is dead and duplicates model-level validators.
return errors - errors = [] - identifier_to_nodes = {} - # First pass: collect all nodes by identifier - for node in nodes: - if node.identifier is None or node.identifier == "": - errors.append(f"Node {node.node_name} in namespace {node.namespace} has no identifier") - continue - - if node.identifier not in identifier_to_nodes: - identifier_to_nodes[node.identifier] = [] - identifier_to_nodes[node.identifier].append(node) - # Check for duplicates and report all nodes sharing the same identifier - for identifier, nodes_with_identifier in identifier_to_nodes.items(): - if len(nodes_with_identifier) > 1: - node_list = ", ".join([f"{node.node_name} in namespace {node.namespace}" for node in nodes_with_identifier]) - errors.append(f"Duplicate identifier '{identifier}' found in nodes: {node_list}") - # Check next_nodes references using the valid identifiers - valid_identifiers = set(identifier_to_nodes.keys()) - for node in nodes: - if node.next_nodes is None: - continue - for next_node in node.next_nodes: - if next_node not in valid_identifiers: - errors.append(f"Node {node.node_name} in namespace {node.namespace} has a next node {next_node} that does not exist in the graph") - return errors
3-3: Fix incorrect NodeTemplate importImport NodeTemplate from its module.
-from app.models.db.graph_template_model import GraphTemplate, NodeTemplate +from app.models.db.graph_template_model import GraphTemplate +from app.models.node_template_model import NodeTemplate
155-160: Flatten gather results; current code builds a nested listExtend with strings, not lists.
- errors.extend(await asyncio.gather(*basic_verify_tasks)) + results = await asyncio.gather(*basic_verify_tasks) + for errs in results: + errors.extend(errs)
📜 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 (2)
state-manager/app/models/db/graph_template_model.py(5 hunks)state-manager/app/tasks/verify_graph.py(6 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/models/db/graph_template_model.py
🧬 Code graph analysis (2)
state-manager/app/tasks/verify_graph.py (3)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-60)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-278)
state-manager/app/models/db/graph_template_model.py (1)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-60)
- Rearranged import statements for better readability and structure. - Added missing import for Dict to support type annotations. - Ensured consistent import order by placing BaseDatabaseModel import after other dependencies. These changes enhance the maintainability of the GraphTemplate model by improving the organization of its imports.
…mplate - Introduced a new DependentString model to manage dependent values and their relationships. - Updated NodeTemplate to include a method for generating dependent strings from input values. - Enhanced GraphTemplate with validation for input dependencies using the new DependentString model. - Refactored existing code to utilize the new model, improving clarity and maintainability. These changes enhance the handling of dependent values within templates, ensuring better validation and structure in the graph management process.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
state-manager/app/tasks/create_next_states.py (2)
45-67: Validate dependencies only for string-typed inputs.Avoid calling parser on non-strings.
- for field_name in next_state_input_model.model_fields.keys(): - if field_name not in next_state_node_template.inputs: + for field_name in next_state_input_model.model_fields.keys(): + if field_name not in next_state_node_template.inputs: raise ValueError(f"Field '{field_name}' not found in inputs for template '{next_state_node_template.identifier}'") - - dependency_string = DependentString.create_dependent_string(next_state_node_template.inputs[field_name]) + raw_value = next_state_node_template.inputs[field_name] + if not isinstance(raw_value, str): + continue + dependency_string = DependentString.create_dependent_string(raw_value)
1-1: Ensureget_dependentsis re-exported to satisfy existing testsThe unit tests in
state-manager/tests/unit/tasks/test_create_next_states.pystill import and invokeget_dependents, butapp/tasks/create_next_states.pyno longer defines or re-exports it. You’ll need to re-introduce an alias for the helper in one of the following ways:• In
app/tasks/create_next_states.py, add a re-export forcreate_dependent_stringasget_dependentsfrom beanie import PydanticObjectId … + from app.models.dependent_string import create_dependent_string as get_dependents• Or, update the tests to call
DependentString.create_dependent_string(...)instead ofget_dependents(...)Affected files:
- app/tasks/create_next_states.py
- state-manager/tests/unit/tasks/test_create_next_states.py
Without this alias, imports of
get_dependentswill fail and break the test suite.
♻️ Duplicate comments (8)
state-manager/app/models/node_template_model.py (2)
25-31: Remove duplicate node_name validator.Redundant with validate_node_name; keep one.
- @field_validator('node_name') - @classmethod - def validate_node_name_unique(cls, v: str) -> str: - if v == "" or v is None: - raise ValueError("Node name cannot be empty") - return v
18-24: Simplify emptiness checks and fix next_nodes de-dup logic (avoid adding empties).Use
if not v:and short-circuit empty next_node entries to prevent spurious “not unique” errors.def validate_node_name(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Node name cannot be empty") return v @@ - @field_validator('identifier') + @field_validator('identifier') @classmethod - def validate_identifier_unique(cls, v: str) -> str: - if v == "" or v is None: + def validate_identifier(cls, v: str) -> str: + if not v: raise ValueError("Node identifier cannot be empty") return v @@ def validate_next_nodes(cls, v: Optional[List[str]]) -> Optional[List[str]]: identifiers = set() errors = [] if v is not None: for next_node_identifier in v: - if next_node_identifier == "" or next_node_identifier is None: - errors.append("Next node identifier cannot be empty") - elif next_node_identifier in identifiers: - errors.append(f"Next node identifier {next_node_identifier} is not unique") - identifiers.add(next_node_identifier) + if not next_node_identifier: + errors.append("Next node identifier cannot be empty") + continue + if next_node_identifier in identifiers: + errors.append(f"Next node identifier {next_node_identifier} is not unique") + else: + identifiers.add(next_node_identifier) if errors: raise ValueError("\n".join(errors)) return v @@ def validate_unites(cls, v: Optional[Unites]) -> Optional[Unites]: - if v is not None: - if v.identifier == "" or v.identifier is None: + if v is not None: + if not v.identifier: raise ValueError("Unites identifier cannot be empty") return vAlso applies to: 32-38, 39-53, 55-61
state-manager/app/models/db/graph_template_model.py (6)
49-55: Wrong in-degree accounting for unites; breaks root detection.Increment the unite target’s in-degree, not the current node’s. Also guard missing targets.
- if node.unites is None: - continue - in_degree[node.identifier] += 1 + if node.unites is not None: + uid = node.unites.identifier + if uid not in in_degree: + raise ValueError(f"Unites target {uid} does not exist in the graph") + in_degree[uid] += 1
77-86: DFS returns early; unite edges on leaves are never processed.Process unites before checking next_nodes, and recurse through the unite edge to propagate ancestry.
- if node.next_nodes is None: - return - if node.unites is not None: - self._parents_by_identifier[node.unites.identifier].add(node_identifier) - for next_node_identifier in node.next_nodes: - dfs(next_node_identifier, parents | {node_identifier}) + if node.unites is not None: + uid = node.unites.identifier + if uid not in self._parents_by_identifier: + raise ValueError(f"Unites target {uid} does not exist in the graph") + # add direct + transitive parents of current node + self._parents_by_identifier[uid].update(parents | {node_identifier}) + # traverse unite edge so descendants inherit ancestry + dfs(uid, parents | {node_identifier}) + + if node.next_nodes: + for next_node_identifier in node.next_nodes: + dfs(next_node_identifier, parents | {node_identifier}) + return
92-104: Simplify emptiness checks for name/namespace.Use
if not v:.def validate_name(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") return v @@ def validate_namespace(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty") return v
165-171: Namespace strict-equality likely too restrictive; allow universal namespace.Permit nodes from "exospherehost" (or align upstream query logic).
def validate_nodes(self) -> Self: for node in self.nodes: - if node.namespace != self.namespace: + if node.namespace != self.namespace and node.namespace != "exospherehost": raise ValueError(f"Node namespace {node.namespace} does not match graph namespace {self.namespace}") return self
172-181: Skip root in connectivity check.Root won’t list itself as parent; current code falsely flags it.
def validate_graph_is_connected(self) -> Self: errors = [] root_node_identifier = self.get_root_node().identifier for node in self.nodes: - if root_node_identifier not in self.get_parents_by_identifier(node.identifier): + if node.identifier != root_node_identifier and root_node_identifier not in self.get_parents_by_identifier(node.identifier): errors.append(f"Node {node.identifier} is not connected to the root node") if errors: raise ValueError("\n".join(errors)) return self
270-276: Defensive access to parents mapping.Avoid KeyError; return empty set for unknown ids.
def get_parents_by_identifier(self, identifier: str) -> set[str]: if self._parents_by_identifier is None: self._build_parents_by_identifier() assert self._parents_by_identifier is not None - return self._parents_by_identifier[identifier] + return self._parents_by_identifier.get(identifier, set())
📜 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 (4)
state-manager/app/models/db/graph_template_model.py(7 hunks)state-manager/app/models/dependent_string.py(1 hunks)state-manager/app/models/node_template_model.py(2 hunks)state-manager/app/tasks/create_next_states.py(3 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/models/db/graph_template_model.py
🧬 Code graph analysis (3)
state-manager/app/tasks/create_next_states.py (1)
state-manager/app/models/dependent_string.py (4)
DependentString(9-64)create_dependent_string(24-44)get_identifier_field(62-64)set_value(56-60)
state-manager/app/models/db/graph_template_model.py (4)
state-manager/app/models/db/base.py (1)
BaseDatabaseModel(7-15)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/dependent_string.py (3)
DependentString(9-64)create_dependent_string(24-44)get_identifier_field(62-64)
state-manager/app/models/node_template_model.py (1)
state-manager/app/models/dependent_string.py (2)
DependentString(9-64)create_dependent_string(24-44)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/app/tasks/create_next_states.py
[error] 1-1: ImportError: cannot import name 'get_dependents' from 'app.tasks.create_next_states'.
🔇 Additional comments (2)
state-manager/app/tasks/create_next_states.py (1)
198-207: Good defensive handling of duplicate inserts.Catching DuplicateKeyError/BulkWriteError and logging a concise diagnostic is appropriate.
state-manager/app/models/db/graph_template_model.py (1)
285-306: LGTM: sensible polling/timeout validation in get_valid.Positive checks and minimum polling interval protect against tight loops.
- Added a unique index on the combination of name and namespace to the RegisteredNode model for improved database performance. - Introduced static methods for retrieving nodes by name and namespace, and for listing nodes based on templates, enhancing data retrieval capabilities. - Updated verification functions in verify_graph.py to utilize the new RegisteredNode methods, improving the overall validation process. These changes improve the efficiency and clarity of node management within the application.
There was a problem hiding this comment.
Actionable comments posted: 5
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/verify_graph.py (4)
23-42: Handle optional secrets and simplify set buildingIf graph_template.secrets can be absent/None, this will raise. Also, sets can be built directly.
- required_secrets_set = set() - for node in registered_nodes: - if node.secrets is None: - continue - for secret in node.secrets: - required_secrets_set.add(secret) + required_secrets_set = {s for rn in registered_nodes for s in (rn.secrets or [])} @@ - present_secrets_set = set() - for secret_name in graph_template.secrets.keys(): - present_secrets_set.add(secret_name) + present_secrets_set = set((graph_template.secrets or {}).keys())
94-114: ** Optional: return errors for callers/tests**If API consumers need the errors, consider returning them (while still persisting status), making this easier to unit test.
1-114: Add focused tests for validation outcomes
- Empty templates: list_nodes_by_templates should short-circuit and not query.
- Missing node/secret/input scenarios: verify aggregated errors and INVALID status.
- Optional[str] input field acceptance.
I can generate pytest cases targeting verify_node_exists/verify_secrets/verify_inputs and verify_graph end-to-end. Want me to add them?
3-7: Fix remaining nestedasyncio.gatherusage and remove unused imports in verify_graph.pyThe verification script shows the following issues are still present in
state-manager/app/tasks/verify_graph.py:
Nested gather in
errors.extend(line 104)errors.extend(await asyncio.gather(*basic_verify_tasks))This still nests
gatherinsideextend, which can obscure exceptions and complicate tracebacks. Refactor to separate the gather call, for example:- errors.extend(await asyncio.gather(*basic_verify_tasks)) + results = await asyncio.gather(*basic_verify_tasks) + errors.extend(results)Unused
Inimport (line 7)from beanie.operators import InThe
Inoperator isn’t referenced anywhere in this module. Please remove this import.Unnecessary
NodeTemplateimport (line 3)from app.models.db.graph_template_model import GraphTemplate, NodeTemplateIf
NodeTemplateisn’t used here, drop it from the import. Otherwise, import it from its proper location (e.g.,app.models.node_template) to keep imports organized.Please address these before merging.
♻️ Duplicate comments (2)
state-manager/app/tasks/verify_graph.py (2)
3-7: Fix imports to pass Ruff and correct NodeTemplate sourceNodeTemplate isn’t used here and was imported from the wrong module; In is unused. Remove both. If NodeTemplate is needed later, import from app.models.node_template_model.
-from app.models.db.graph_template_model import GraphTemplate, NodeTemplate +from app.models.db.graph_template_model import GraphTemplate @@ -from beanie.operators import In
99-109: Flatten gather results and set validation status based on errorsCurrent code appends a list of lists and always marks VALID. Flatten and conditionally set status. This mirrors prior feedback.
- errors.extend(await asyncio.gather(*basic_verify_tasks)) - - graph_template.validation_status = GraphTemplateValidationStatus.VALID - graph_template.validation_errors = None + results = await asyncio.gather(*basic_verify_tasks) + for err_list in results: + errors.extend(err_list) + + if errors: + graph_template.validation_status = GraphTemplateValidationStatus.INVALID + graph_template.validation_errors = sorted(set(errors)) + else: + graph_template.validation_status = GraphTemplateValidationStatus.VALID + graph_template.validation_errors = None await graph_template.save()
📜 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 (2)
state-manager/app/models/db/registered_node.py(2 hunks)state-manager/app/tasks/verify_graph.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
state-manager/app/models/db/registered_node.py (2)
state-manager/app/models/db/graph_template_model.py (1)
Settings(28-35)state-manager/app/models/db/state.py (1)
Settings(57-69)
state-manager/app/tasks/verify_graph.py (3)
state-manager/app/models/db/graph_template_model.py (2)
GraphTemplate(16-305)get_node_by_identifier(262-268)state-manager/app/models/db/registered_node.py (2)
RegisteredNode(8-42)list_nodes_by_templates(34-42)state-manager/app/models/dependent_string.py (1)
get_identifier_field(62-64)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/app/tasks/verify_graph.py
[error] 3-3: Ruff: Unused import 'NodeTemplate' from app.models.db.graph_template_model in verify_graph.py (F401).
[error] 7-7: Ruff: Unused import 'In' from beanie.operators in verify_graph.py (F401).
🔇 Additional comments (3)
state-manager/app/models/db/registered_node.py (2)
17-24: LGTM: Proper compound unique index on (name, namespace)This ensures fast lookups and prevents duplicates per namespace.
26-31: LGTM: Targeted lookup by compound keyQuery aligns with the index; returns None or a single document as expected.
state-manager/app/tasks/verify_graph.py (1)
12-22: LGTM: Set-diff correctly detects missing nodesEfficient and clear.
…tionality - Updated the DependentString model to check for an empty dictionary instead of None when building mapping keys. - Refactored import statements in verify_graph.py for clarity and removed unused imports. - Enhanced unit tests for DependentString to cover various scenarios, including placeholder creation and error handling. - Improved verification functions to ensure better validation of graph structures and dependencies. These changes enhance the robustness and maintainability of the dependent string handling and graph verification processes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 9
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/verify_graph.py (1)
32-41: Guard against None graph_template.secretsIf secrets can be None, .keys() will fail. Make the code resilient.
Apply this diff:
- present_secrets_set = set() - for secret_name in graph_template.secrets.keys(): + present_secrets_set = set() + for secret_name in (graph_template.secrets or {}).keys(): present_secrets_set.add(secret_name)
♻️ Duplicate comments (8)
state-manager/app/tasks/verify_graph.py (5)
43-49: Use accurate lookup key (RegisteredNode.name) and clearer naming; avoid shadowing “node”RegisteredNode doesn’t have node_name. The current dict comp will fail against real objects. Also rename for clarity.
Apply this diff:
- look_up_table = { - (node.node_name, node.namespace): node - for node in registered_nodes - } + registered_node_lookup = { + (rn.name, rn.namespace): rn + for rn in registered_nodes + }
54-60: Index lookup safely; fix variable typo; handle invalid input schemasDirect indexing risks KeyError, and create_model can raise. Also fix registerd_node_input_model typo.
Apply this diff:
- if (node.node_name, node.namespace) not in look_up_table: + if (node.node_name, node.namespace) not in registered_node_lookup: errors.append(f"Node {node.node_name} in namespace {node.namespace} does not exist") continue - - registered_node = look_up_table[(node.node_name, node.namespace)] - registerd_node_input_model = create_model(registered_node.inputs_schema) + registered_node = registered_node_lookup[(node.node_name, node.namespace)] + try: + registered_node_input_model = create_model(registered_node.inputs_schema) + except Exception as e: + errors.append(f"Invalid inputs schema for node {node.node_name} in namespace {node.namespace}: {e}") + continue
61-69: Accept Optional[str]/Union[str,…] and flag extra template inputs not in schemaStrict is str rejects Optional[str] and similar. Also report extras present in template but not defined in schema.
Apply this diff:
- for input_name, input_info in registerd_node_input_model.model_fields.items(): - if input_info.annotation is not str: + from typing import get_origin, get_args, Union + def _is_str_like(ann) -> bool: + return (ann is str) or (get_origin(ann) is Union and str in get_args(ann)) + + for input_name, input_info in registered_node_input_model.model_fields.items(): + if not _is_str_like(input_info.annotation): errors.append(f"Input {input_name} in node {node.node_name} in namespace {node.namespace} is not a string") continue - - if input_name not in node.inputs.keys(): + if input_name not in node.inputs: errors.append(f"Input {input_name} in node {node.node_name} in namespace {node.namespace} is not present in the graph template") continue + extra_inputs = set(node.inputs.keys()) - set(registered_node_input_model.model_fields.keys()) + for extra in extra_inputs: + errors.append(f"Input {extra} in node {node.node_name} in namespace {node.namespace} not defined by registered node schema")
70-91: Remove assert; use safe lookups and catch output schema errorsAsserting aborts validation and hides multiple issues. Continue collecting errors instead.
Apply this diff:
- dependent_strings = node.get_dependent_strings() + dependent_strings = node.get_dependent_strings() for dependent_string in dependent_strings: identifier_field_pairs = dependent_string.get_identifier_field() for identifier, field in identifier_field_pairs: - temp_node = graph_template.get_node_by_identifier(identifier) - assert temp_node is not None + temp_node = graph_template.get_node_by_identifier(identifier) + if temp_node is None: + errors.append(f"Identifier {identifier} referenced by node {node.node_name} in namespace {node.namespace} does not exist") + continue - registered_node = look_up_table[(temp_node.node_name, temp_node.namespace)] - if registered_node is None: + registered_node = registered_node_lookup.get((temp_node.node_name, temp_node.namespace)) + if registered_node is None: errors.append(f"Node {temp_node.node_name} in namespace {temp_node.namespace} does not exist") continue - output_model = create_model(registered_node.outputs_schema) + try: + output_model = create_model(registered_node.outputs_schema) + except Exception as e: + errors.append(f"Invalid outputs schema for node {temp_node.node_name} in namespace {temp_node.namespace}: {e}") + continue if field not in output_model.model_fields.keys(): errors.append(f"Field {field} in node {temp_node.node_name} in namespace {temp_node.namespace} does not exist") continue - if output_model.model_fields[field].annotation is not str: + from typing import get_origin, get_args, Union + def _is_str_like(ann) -> bool: + return (ann is str) or (get_origin(ann) is Union and str in get_args(ann)) + if not _is_str_like(output_model.model_fields[field].annotation): errors.append(f"Field {field} in node {temp_node.node_name} in namespace {temp_node.namespace} is not a string")
98-104: Flatten asyncio.gather results and set INVALID when any errors occurCurrent code builds errors as a list of lists and ignores them, marking VALID regardless. Flatten and set status/errors appropriately.
Apply this diff:
- basic_verify_tasks = [ + basic_verify_tasks = [ verify_node_exists(graph_template, registered_nodes), verify_secrets(graph_template, registered_nodes), verify_inputs(graph_template, registered_nodes) ] - errors.extend(await asyncio.gather(*basic_verify_tasks)) - - graph_template.validation_status = GraphTemplateValidationStatus.VALID - graph_template.validation_errors = None + results = await asyncio.gather(*basic_verify_tasks) + for errs in results: + errors.extend(errs) + + if errors: + graph_template.validation_status = GraphTemplateValidationStatus.INVALID + graph_template.validation_errors = errors + else: + graph_template.validation_status = GraphTemplateValidationStatus.VALID + graph_template.validation_errors = None await graph_template.save()state-manager/app/models/dependent_string.py (3)
35-35: Guard against mis-splitting when tail contains '}}'.Split only once to avoid chopping the tail if it includes '}}'.
- placeholder_content, tail = split.split("}}") + placeholder_content, tail = split.split("}}", 1)
12-12: Make mapping cache robust: use None sentinel and build once.Cleaner and avoids edge cases when the dict is legitimately empty.
- _mapping_key_to_dependent: dict[tuple[str, str], list[Dependent]] = PrivateAttr(default_factory=dict) + _mapping_key_to_dependent: dict[tuple[str, str], list[Dependent]] | None = PrivateAttr(default=None) @@ def _build_mapping_key_to_dependent(self): - if self._mapping_key_to_dependent != {}: - return - - for dependent in self.dependents.values(): - mapping_key = (dependent.identifier, dependent.field) - if mapping_key not in self._mapping_key_to_dependent: - self._mapping_key_to_dependent[mapping_key] = [] - self._mapping_key_to_dependent[mapping_key].append(dependent) + if self._mapping_key_to_dependent is not None: + return + self._mapping_key_to_dependent = {} + for d in self.dependents.values(): + k = (d.identifier, d.field) + self._mapping_key_to_dependent.setdefault(k, []).append(d) @@ def get_identifier_field(self) -> list[tuple[str, str]]: self._build_mapping_key_to_dependent() - return list(self._mapping_key_to_dependent.keys()) + assert self._mapping_key_to_dependent is not None + return list(self._mapping_key_to_dependent.keys())Also applies to: 46-55, 62-64
56-61: Harden set_value: handle unknown keys and coerce to str.Prevents KeyError from dict lookup and ensures safe concatenation in generate_string.
def set_value(self, identifier: str, field: str, value: str): self._build_mapping_key_to_dependent() mapping_key = (identifier, field) - for dependent in self._mapping_key_to_dependent[mapping_key]: - dependent.value = value + mapping = self._mapping_key_to_dependent or {} + if mapping_key not in mapping: + raise KeyError(f"No dependent found for identifier={identifier}, field={field}") + for dependent in mapping[mapping_key]: + dependent.value = str(value)
📜 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 (4)
state-manager/app/models/dependent_string.py(1 hunks)state-manager/app/tasks/verify_graph.py(2 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(3 hunks)state-manager/tests/unit/tasks/test_verify_graph.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
state-manager/tests/unit/tasks/test_verify_graph.py (4)
state-manager/app/tasks/verify_graph.py (4)
verify_inputs(43-91)verify_graph(93-113)verify_node_exists(11-20)verify_secrets(22-41)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/db/graph_template_model.py (1)
get_node_by_identifier(262-268)
state-manager/app/tasks/verify_graph.py (4)
state-manager/app/models/db/graph_template_model.py (2)
GraphTemplate(16-305)get_node_by_identifier(262-268)state-manager/app/models/db/registered_node.py (2)
RegisteredNode(8-42)list_nodes_by_templates(34-42)state-manager/app/models/node_template_model.py (1)
get_dependent_strings(63-67)state-manager/app/models/dependent_string.py (1)
get_identifier_field(62-64)
state-manager/tests/unit/tasks/test_create_next_states.py (6)
state-manager/app/models/dependent_string.py (5)
Dependent(3-7)DependentString(9-64)create_dependent_string(24-44)set_value(56-60)get_identifier_field(62-64)state-manager/app/tasks/create_next_states.py (4)
mark_success_states(17-22)check_unites_satisfied(25-42)validate_dependencies(45-66)create_next_states(105-215)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (2)
NodeTemplate(10-67)Unites(6-7)state-manager/app/models/db/graph_template_model.py (2)
get_node_by_identifier(262-268)get_valid(285-305)state-manager/app/models/db/state.py (1)
insert_many(49-55)
|
/gemini review but ignore tests |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the graph template validation logic. The validation process is moved into the GraphTemplate model using Pydantic validators, which is a great improvement for encapsulation and maintainability. A new DependentString model is also introduced to handle input dependencies, which cleans up the code in create_next_states. While the refactoring is a positive change, I've identified several critical issues in the new validation logic within GraphTemplate and verify_graph.py that could lead to incorrect graph validation outcomes. There are also some medium-severity issues related to code duplication, naming, and efficiency that should be addressed.
|
…d clarity - Updated the DependentString model to utilize enumeration for order in dependent placeholders, enhancing readability. - Refactored validation methods in NodeTemplate and GraphTemplate to ensure non-empty inputs and unique identifiers. - Improved error handling in verification functions to check for string types in inputs and ensure proper validation of graph structures. - Streamlined the RegisteredNode model's list_nodes_by_templates method to handle empty templates gracefully. These changes enhance the robustness and maintainability of the models and their validation processes.
|
/gemini review but ignore tests |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that moves graph validation logic into the GraphTemplate Pydantic model. This centralizes validation and improves the overall structure. The introduction of the DependentString model is also a great abstraction for handling input dependencies. My review focuses on improving maintainability and robustness of the new validation logic, and correcting an issue in the tests. I've identified a few areas for improvement: adding a clarifying comment for some cryptic logic, replacing a risky assert with a proper check, and fixing an incorrect test case that was not properly asserting the invalid state of a graph.
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/verify_graph.py (1)
26-41: Simplify secrets handling; align with default empty listWith RegisteredNode.secrets defaulting to [], the None-check is unnecessary.
- for node in registered_nodes: - if node.secrets is None: - continue - for secret in node.secrets: - required_secrets_set.add(secret) + for node in registered_nodes: + for secret in node.secrets: + required_secrets_set.add(secret)
♻️ Duplicate comments (12)
state-manager/app/models/db/registered_node.py (1)
4-5: Avoid runtime import cycle; type-only import NodeTemplate and dedupe the $or queryImport NodeTemplate under TYPE_CHECKING and accept Sequence; also dedupe (name, namespace) pairs to shrink the $or.
-from pymongo import IndexModel -from ..node_template_model import NodeTemplate +from pymongo import IndexModel +from typing import TYPE_CHECKING, Sequence +if TYPE_CHECKING: + from ..node_template_model import NodeTemplate @@ - async def list_nodes_by_templates(templates: list[NodeTemplate]) -> list["RegisteredNode"]: - if len(templates) == 0: - return [] - - query = { - "$or": [ - {"name": node.node_name, "namespace": node.namespace} - for node in templates - ] - } - return await RegisteredNode.find(query).to_list() + async def list_nodes_by_templates(templates: "Sequence[NodeTemplate]") -> list["RegisteredNode"]: + if not templates: + return [] + pairs = {(t.node_name, t.namespace) for t in templates} + query = {"$or": [{"name": n, "namespace": ns} for (n, ns) in pairs]} + return await RegisteredNode.find(query).to_list()Also applies to: 33-44
state-manager/app/models/node_template_model.py (3)
18-23: Simplify non-empty checkRely on Pydantic’s type guarantees; use truthiness.
- if v == "" or v is None: + if not v: raise ValueError("Node name cannot be empty")
25-30: Simplify non-empty identifier checkSame rationale as above.
- if v == "" or v is None: + if not v: raise ValueError("Node identifier cannot be empty")
61-67: Don’t raise on non-string inputs; only parse stringsInputs are Any; schema may include non-strings. Parse placeholders only for str values.
- def get_dependent_strings(self) -> list[DependentString]: - dependent_strings = [] - for input_value in self.inputs.values(): - if not isinstance(input_value, str): - raise ValueError(f"Input {input_value} is not a string") - dependent_strings.append(DependentString.create_dependent_string(input_value)) - return dependent_strings + def get_dependent_strings(self) -> list[DependentString]: + return [ + DependentString.create_dependent_string(v) + for v in self.inputs.values() + if isinstance(v, str) + ]state-manager/app/models/dependent_string.py (2)
14-21: Coerce values to str during assemblyPrevents TypeError if a non-str sneaks in.
- base += dependent.value + dependent.tail + base += str(dependent.value) + dependent.tail
54-59: Harden set_value: explicit KeyError and str-coercionAvoids silent KeyError and guarantees string concatenation.
def set_value(self, identifier: str, field: str, value: str): self._build_mapping_key_to_dependent() mapping_key = (identifier, field) - for dependent in self._mapping_key_to_dependent[mapping_key]: - dependent.value = value + mapping = self._mapping_key_to_dependent or {} + if mapping_key not in mapping: + raise KeyError(f"No dependent found for identifier={identifier}, field={field}") + for dependent in mapping[mapping_key]: + dependent.value = str(value)state-manager/app/tasks/verify_graph.py (3)
11-20: Normalize error message punctuation (drop trailing dot)Matches existing tests and keeps messages consistent.
- errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.") + errors.append(f"Node {node[0]} in namespace {node[1]} does not exist")
75-90: Don’t assert; collect errors for missing identifiers and unknown registered nodesAvoid aborting validation; keep gathering actionable errors.
- temp_node = graph_template.get_node_by_identifier(identifier) - assert temp_node is not None + temp_node = graph_template.get_node_by_identifier(identifier) + if temp_node is None: + errors.append(f"Identifier {identifier} referenced by node {node.node_name} in namespace {node.namespace} does not exist") + continue @@ - if registered_node is None: + if registered_node is None: errors.append(f"Node {temp_node.node_name} in namespace {temp_node.namespace} does not exist") continue
61-68: Accept Optional[str] (and Union[…] including str) for inputsjson_schema_to_pydantic may emit Union with str; broaden the check locally.
- for input_name, input_info in registered_node_input_model.model_fields.items(): - if input_info.annotation is not str: + from typing import get_origin, get_args, Union + def _is_str_like(ann) -> bool: + return (ann is str) or (get_origin(ann) is Union and str in get_args(ann)) + for input_name, input_info in registered_node_input_model.model_fields.items(): + if not _is_str_like(input_info.annotation): errors.append(f"Input {input_name} in node {node.node_name} in namespace {node.namespace} is not a string") continuestate-manager/tests/unit/tasks/test_create_next_states.py (1)
270-279: Update placeholder syntax to parser-supported form${{…}}Tests should reflect the
${{…}}placeholder the parser recognizes.- inputs={"input1": "{{parent1.outputs.field1}}"}, + inputs={"input1": "${{parent1.outputs.field1}}"},state-manager/app/models/db/graph_template_model.py (2)
92-105: Simplify empty-string checks for name/namespacePydantic ensures required str fields aren’t None; use truthiness.
- if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") @@ - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty")
120-149: Reduce passes over nodes: merge identifier and next-nodes checksCombine both validators to avoid extra iterations and centralize errors.
- @field_validator('nodes') - @classmethod - def validate_unique_identifiers(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - errors = [] - for node in v: - if node.identifier in identifiers: - errors.append(f"Node identifier {node.identifier} is not unique") - identifiers.add(node.identifier) - if errors: - raise ValueError("\n".join(errors)) - return v - - @field_validator('nodes') - @classmethod - def validate_next_nodes_identifiers_exist(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - for node in v: - identifiers.add(node.identifier) - - errors = [] - for node in v: - if node.next_nodes: - for next_node in node.next_nodes: - if next_node not in identifiers: - errors.append(f"Node identifier {next_node} does not exist in the graph") - if errors: - raise ValueError("\n".join(errors)) - return v + @field_validator('nodes') + @classmethod + def validate_nodes_references(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: + identifiers: set[str] = set() + errors: list[str] = [] + + # pass 1: uniqueness + for node in v: + if node.identifier in identifiers: + errors.append(f"Node identifier {node.identifier} is not unique") + identifiers.add(node.identifier) + + # pass 2: next_nodes exist + for node in v: + if node.next_nodes: + for nxt in node.next_nodes: + if nxt not in identifiers: + errors.append(f"Node identifier {nxt} does not exist in the graph") + + if errors: + raise ValueError("\n".join(errors)) + return v
📜 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 (6)
state-manager/app/models/db/graph_template_model.py(7 hunks)state-manager/app/models/db/registered_node.py(2 hunks)state-manager/app/models/dependent_string.py(1 hunks)state-manager/app/models/node_template_model.py(2 hunks)state-manager/app/tasks/verify_graph.py(2 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/app/models/db/graph_template_model.py
📚 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 (4)
state-manager/app/tasks/verify_graph.py (4)
state-manager/app/models/db/graph_template_model.py (2)
get(277-281)get_node_by_identifier(261-267)state-manager/app/models/db/registered_node.py (2)
RegisteredNode(8-44)list_nodes_by_templates(34-44)state-manager/app/models/node_template_model.py (1)
get_dependent_strings(61-67)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
state-manager/app/models/node_template_model.py (1)
state-manager/app/models/dependent_string.py (2)
DependentString(9-62)create_dependent_string(24-42)
state-manager/app/models/db/registered_node.py (3)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/db/graph_template_model.py (1)
Settings(28-35)state-manager/app/models/db/state.py (1)
Settings(57-69)
state-manager/app/models/db/graph_template_model.py (4)
state-manager/app/models/db/base.py (1)
BaseDatabaseModel(7-15)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/dependent_string.py (3)
DependentString(9-62)create_dependent_string(24-42)get_identifier_field(60-62)
🔇 Additional comments (2)
state-manager/app/models/db/graph_template_model.py (2)
165-176: Connectivity validator: root-skip is correctSkipping the root in connectivity checks avoids false positives. No change needed.
269-275: Defensive parents lookup is goodReturning an empty set for unknown identifiers avoids KeyErrors downstream.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
state-manager/tests/unit/models/test_graph_template_model.py (6)
18-21: Patch the function where it’s used, not the stdlib symbol.If GraphTemplate imports base64 or urlsafe_b64decode in-module, patch that path to ensure the mock applies.
- with patch("base64.urlsafe_b64decode", return_value=b"x" * 20): + with patch("app.models.db.graph_template_model.base64.urlsafe_b64decode", return_value=b"x" * 20): result = GraphTemplate.validate_secrets(valid_secrets)Apply the same path correction to other tests that patch urlsafe_b64decode.
51-60: Fix invalid base64 padding in “valid” case.Padding the encoded string with extra characters can break decoding. Generate 24 bytes so the urlsafe base64 is exactly 32 chars and decodes cleanly without hacks.
- # Create a valid base64 string that decodes to at least 12 bytes and is long enough - valid_bytes = b"x" * 20 - valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() - # Pad to make it at least 32 characters - padded_base64 = valid_base64 + "x" * (32 - len(valid_base64)) - - # Should not raise any exception - GraphTemplate._validate_secret_value(padded_base64) + # 24 bytes -> 32 urlsafe base64 chars, decodes without '=' padding + valid_bytes = b"x" * 24 + valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() + GraphTemplate._validate_secret_value(valid_base64)
98-107: Same base64 length issue here.Use 24 bytes to produce a 32-char urlsafe base64 string; avoid manual padding.
- # Create a valid base64 string that decodes to exactly 12 bytes - valid_bytes = b"x" * 12 # Exactly 12 bytes - valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() - # Pad to make it at least 32 characters - padded_base64 = valid_base64 + "x" * (32 - len(valid_base64)) - - # Should not raise any exception - GraphTemplate._validate_secret_value(padded_base64) + # 24 bytes -> 32 base64 chars; decodes successfully + valid_bytes = b"x" * 24 + valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() + GraphTemplate._validate_secret_value(valid_base64)
51-61: Avoid testing private methods directly.Prefer validating via validate_secrets to keep tests resilient to refactors.
Example:
- GraphTemplate._validate_secret_value(valid_base64) + GraphTemplate.validate_secrets({"s": valid_base64})Also applies to: 75-83
117-168: Remove no-op assertions on name.These don’t validate behavior and add noise. Either implement real behavior checks or delete.
- def test_is_valid_valid_status(self): - """Test is_valid method with valid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_valid.__name__ == "is_valid" - - def test_is_valid_invalid_status(self): - """Test is_valid method with invalid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_valid.__name__ == "is_valid" - - def test_is_validating_ongoing_status(self): - """Test is_validating method with ongoing status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" - - def test_is_validating_pending_status(self): - """Test is_validating method with pending status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" - - def test_is_validating_invalid_status(self): - """Test is_validating method with invalid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" - - def test_get_valid_success(self): - """Test get_valid method with successful validation""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.get_valid.__name__ == "get_valid" - - def test_get_valid_ongoing_then_valid(self): - """Test get_valid method with ongoing then valid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.get_valid.__name__ == "get_valid" - - def test_get_valid_invalid_status(self): - """Test get_valid method with invalid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.get_valid.__name__ == "get_valid" - - def test_get_valid_timeout(self): - """Test get_valid method with timeout""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.get_valid.__name__ == "get_valid" - - def test_get_valid_exception_handling(self): - """Test get_valid method exception handling""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.get_valid.__name__ == "get_valid" + # Removed name-based placeholder tests; replace with behavior-driven tests in DB-backed suite.
109-113: Delete placeholder test or mark xfail with rationale.A pass statement hides an intended case; either implement, parametrize, or remove to keep the suite signal high.
.github/workflows/publish-state-mangaer.yml (1)
1-1: Typo in workflow filename impairs discoverability.Consider renaming the file to
publish-state-manager.yml.state-manager/tests/unit/utils/test_check_secret.py (1)
14-28: Tests can drop repeated module reloads if settings are cached with an explicit reload hook.If you adopt
reload_settings(), call it afterpatch.dictand remove importlib gymnastics to speed up tests and reduce flakiness.Also applies to: 29-44, 45-59, 60-74, 75-89, 90-104, 105-116, 117-128, 129-141, 142-154
state-manager/app/utils/encrypter.py (3)
15-23: Harden key handling: explicit “not set” guard and whitespace trim.Fail fast if the key is empty/missing; trim accidental whitespace to reduce decoding surprises.
def __init__(self): - key_b64 = settings.secrets_encryption_key + key_b64 = get_settings().secrets_encryption_key + if not key_b64: + raise ValueError("secrets_encryption_key is not set") + key_b64 = key_b64.strip() try: self._key = base64.urlsafe_b64decode(key_b64) except Exception as exc: raise ValueError("Key must be URL-safe base64 (44 chars for 32-byte key)") from exc if len(self._key) != 32: raise ValueError("Key must be 32 raw bytes (256 bits)")
30-34: More robust decrypt error reporting.Wrap decode/decrypt with a clear ValueError to aid callers and avoid leaking low-level exceptions.
def decrypt(self, encrypted_secret: str) -> str: - encrypted_secret_bytes = base64.urlsafe_b64decode(encrypted_secret) - nonce = encrypted_secret_bytes[:12] - ciphertext = encrypted_secret_bytes[12:] - return self._aesgcm.decrypt(nonce, ciphertext, None).decode() + try: + encrypted_secret_bytes = base64.urlsafe_b64decode(encrypted_secret) + nonce = encrypted_secret_bytes[:12] + ciphertext = encrypted_secret_bytes[12:] + return self._aesgcm.decrypt(nonce, ciphertext, None).decode() + except Exception as exc: + raise ValueError("Invalid encrypted secret format or authentication failed") from exc
39-47: Docstring refers to old env var; update to settings-based config.Reflect the new source of truth to avoid confusion.
- ValueError: If SECRETS_ENCRYPTION_KEY is not set or invalid. + ValueError: If the configured secrets_encryption_key is not set or invalid.
♻️ Duplicate comments (7)
state-manager/tests/unit/tasks/test_verify_graph.py (3)
10-10: Fix incorrect import: NodeTemplate comes from node_template_modelImporting NodeTemplate from graph_template_model will break these tests.
-from app.models.db.graph_template_model import NodeTemplate +from app.models.node_template_model import NodeTemplate
211-219: Mocks shouldn’t use non-existent RegisteredNode.node_name; spec to RegisteredNodeRegisteredNode exposes name/namespace, not node_name. Spec your mocks and drop node_name to avoid masking bugs.
- mock_node1 = MagicMock() - mock_node1.node_name = "node1" + from app.models.db.registered_node import RegisteredNode + mock_node1 = MagicMock(spec=RegisteredNode) mock_node1.name = "node1" mock_node1.namespace = "test"Apply similarly to the other occurrences in these blocks.
Also applies to: 255-263, 291-300, 373-381, 409-417
348-356: Don’t catch AssertionError; assert on reported validation errorverify_inputs now reports missing identifiers as errors rather than asserting. Update the test to assert on the error string.
- # 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 + )state-manager/app/tasks/verify_graph.py (1)
11-21: Error message punctuation consistencyDrop the trailing period to align with test substrings and avoid brittleness.
- errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.") + errors.append(f"Node {node[0]} in namespace {node[1]} does not exist")state-manager/app/models/db/graph_template_model.py (3)
48-56: Clarify ‘unites’ semantics and improve diagnostics
- Use “unites target” in comments.
- Show identifiers, not object reprs, in the root error.
- if node.unites is not None: - # If the node has a unit, it should have an in-degree of 1 - # As unites.node.identifier acts as the parent of the node + if node.unites is not None: + # A 'unites' node is a synchronization point: it runs after its unites target, + # so it must have at least one predecessor and cannot be a root. in_degree[node.identifier] += 1 - zero_in_degree_nodes = [node for node in self.nodes if in_degree[node.identifier] == 0] - if len(zero_in_degree_nodes) != 1: - raise ValueError("There should be exactly one root node in the graph but found " + str(len(zero_in_degree_nodes)) + " nodes with zero in-degree: " + str(zero_in_degree_nodes)) + zero_in_degree_nodes = [node for node in self.nodes if in_degree[node.identifier] == 0] + if len(zero_in_degree_nodes) != 1: + zero_ids = [n.identifier for n in zero_in_degree_nodes] + raise ValueError(f"There should be exactly one root node in the graph but found {len(zero_in_degree_nodes)} nodes with zero in-degree: {zero_ids}")
115-127: Minor: simplify empty-string checksPydantic ensures str not None; prefer truthiness checks.
- if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty")Apply similarly to validate_namespace.
227-247: Improve input dependency validation messages and robustnessLeverage NodeTemplate.get_dependent_strings, include node context, and avoid raising on non-strings.
- def verify_input_dependencies(self) -> Self: - errors = [] - - for node in self.nodes: - for input_value in node.inputs.values(): - try: - if not isinstance(input_value, str): - errors.append(f"Input {input_value} is not a string") - continue - - dependent_string = DependentString.create_dependent_string(input_value) - dependent_identifiers = set([identifier for identifier, _ in dependent_string.get_identifier_field()]) - - for identifier in dependent_identifiers: - if 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}") - - except Exception as e: - errors.append(f"Error creating dependent string for input {input_value}: {e}") + def verify_input_dependencies(self) -> Self: + errors = [] + for node in self.nodes: + try: + for dependent_string in node.get_dependent_strings(): + dependent_identifiers = {i for i, _ in dependent_string.get_identifier_field()} + allowed = self.get_parents_by_identifier(node.identifier) + for identifier in dependent_identifiers: + if identifier not in allowed: + errors.append( + f"Node {node.identifier} input depends on {identifier} but it is not a parent of {node.identifier}" + ) + except Exception as e: + errors.append(f"Node {node.identifier} has invalid inputs: {e}") if errors: raise ValueError("\n".join(errors)) return self
📜 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 ignored due to path filters (1)
state-manager/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/publish-state-mangaer.yml(1 hunks).github/workflows/release-state-manager.yml(2 hunks).github/workflows/test-state-manager.yml(2 hunks)state-manager/.dockerignore(1 hunks)state-manager/app/config/settings.py(1 hunks)state-manager/app/main.py(2 hunks)state-manager/app/models/db/graph_template_model.py(6 hunks)state-manager/app/tasks/verify_graph.py(2 hunks)state-manager/app/utils/check_secret.py(1 hunks)state-manager/app/utils/encrypter.py(1 hunks)state-manager/pyproject.toml(1 hunks)state-manager/pytest.ini(1 hunks)state-manager/tests/integration/integration_full_workflow_integration.py(0 hunks)state-manager/tests/integration/peinding_test_full_workflow_integration.py(0 hunks)state-manager/tests/unit/models/test_base.py(0 hunks)state-manager/tests/unit/models/test_graph_template_model.py(1 hunks)state-manager/tests/unit/tasks/test_verify_graph.py(2 hunks)state-manager/tests/unit/test_main.py(0 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)state-manager/tests/unit/utils/test_encrypter.py(0 hunks)state-manager/tests/unit/with_database/conftest.py(1 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)state-manager/tests/unit/with_database/test_health_api.py(1 hunks)
💤 Files with no reviewable changes (5)
- state-manager/tests/unit/models/test_base.py
- state-manager/tests/unit/test_main.py
- state-manager/tests/unit/utils/test_encrypter.py
- state-manager/tests/integration/integration_full_workflow_integration.py
- state-manager/tests/integration/peinding_test_full_workflow_integration.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (3)
state-manager/tests/unit/with_database/test_graph_template.py (2)
state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/tests/unit/with_database/conftest.py (1)
app_started(23-26)
state-manager/tests/unit/with_database/test_health_api.py (1)
state-manager/tests/unit/with_database/conftest.py (1)
app_fixture(29-33)
state-manager/tests/unit/tasks/test_verify_graph.py (2)
state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)
🪛 Gitleaks (8.27.2)
.github/workflows/release-state-manager.yml
58-58: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.github/workflows/test-state-manager.yml
56-56: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.github/workflows/publish-state-mangaer.yml
60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Checkov (3.2.334)
.github/workflows/release-state-manager.yml
[MEDIUM] 55-56: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 58-59: Base64 High Entropy String
(CKV_SECRET_6)
.github/workflows/test-state-manager.yml
[MEDIUM] 53-54: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 56-57: Base64 High Entropy String
(CKV_SECRET_6)
.github/workflows/publish-state-mangaer.yml
[MEDIUM] 57-58: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 60-61: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.37.1)
.github/workflows/release-state-manager.yml
[error] 51-51: trailing spaces
(trailing-spaces)
.github/workflows/test-state-manager.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
.github/workflows/release-state-manager.yml (2)
60-61: Step name and coverage run LGTM.Full suite with coverage and JUnit output is appropriate.
63-71: Confirm Codecov configurationVerify the CI pipeline produces
state-manager/coverage.xmlat runtime and that your Codecov project settings include the “unit-tests” flag..github/workflows/publish-state-mangaer.yml (1)
76-76: Good gating: publishing waits on tests.The
needs: testdependency properly prevents image publishing on failing tests.state-manager/tests/unit/with_database/test_graph_template.py (1)
17-18: Confirm None is an allowed value for unites.If validators expect a list, prefer [] to None.
Would you like me to patch the test to pass an empty list if None is rejected by the current validators?
state-manager/tests/unit/with_database/conftest.py (1)
22-27: LGTM on lifespan management.The LifespanManager usage is correct and aligns with async startup/shutdown.
state-manager/app/tasks/verify_graph.py (1)
100-118: verify_graph orchestration + flattening looks correctParallelization, error flattening, and status persistence are sound and match tests.
state-manager/app/models/db/graph_template_model.py (1)
288-294: Defensive access for parents mapping looks goodReturning an empty set by default is safe and avoids KeyError.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
.github/workflows/publish-state-mangaer.yml (1)
1-1: Typo in filename (“mangaer”) — consider renaming to publish-state-manager.yml.state-manager/app/utils/encrypter.py (2)
36-51: Make singleton creation thread-safe.Current
get_encrypter()can race under concurrent first access.-_encrypter_instance = None +_encrypter_instance = None +from threading import Lock +_encrypter_lock = Lock() @@ -def get_encrypter() -> Encrypter: +def get_encrypter() -> Encrypter: @@ - global _encrypter_instance - if _encrypter_instance is None: - _encrypter_instance = Encrypter() - return _encrypter_instance + global _encrypter_instance + if _encrypter_instance is not None: + return _encrypter_instance + with _encrypter_lock: + if _encrypter_instance is None: + _encrypter_instance = Encrypter() + return _encrypter_instance
20-22: Improve error messages with actual lengths.Add the observed decoded length to aid debugging.
- if len(self._key) != 32: - raise ValueError("Key must be 32 raw bytes (256 bits)") + if len(self._key) != 32: + raise ValueError(f"Key must be 32 raw bytes (256 bits); got {len(self._key)}")state-manager/tests/unit/models/test_graph_template_model.py (3)
117-141: These tests assert function names only; they don’t validate behavior. Replace with behavioral tests or drop.- def test_is_valid_valid_status(self): - """Test is_valid method with valid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_valid.__name__ == "is_valid" - - def test_is_valid_invalid_status(self): - """Test is_valid method with invalid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_valid.__name__ == "is_valid" - - def test_is_validating_ongoing_status(self): - """Test is_validating method with ongoing status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" - - def test_is_validating_pending_status(self): - """Test is_validating method with pending status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" - - def test_is_validating_invalid_status(self): - """Test is_validating method with invalid status""" - # This test doesn't require GraphTemplate instantiation - assert GraphTemplate.is_validating.__name__ == "is_validating" + # Consider adding parameterized tests over GraphTemplateValidationStatus to validate is_valid/is_validating logic.
109-113: Don’t leave placeholder tests.Either implement the <12 bytes case or remove/xfail with reason.
- def test_validate_secret_value_decoded_less_than_12_bytes(self): - """Test validation with decoded value less than 12 bytes""" - # This test was removed due to regex pattern mismatch issues - pass + @pytest.mark.xfail(reason="Behavior undefined for <12 decoded bytes; define and implement before enabling") + def test_validate_secret_value_decoded_less_than_12_bytes(self): + ...
145-168: Replace name-only assertions inget_validtests with async behavioral tests
In state-manager/tests/unit/models/test_graph_template_model.py (around lines 145–168), convert the existing sync tests that only assertGraphTemplate.get_valid.__name__into fully async tests using@pytest.mark.anyioand monkeypatchingGraphTemplate.getto simulate each validation scenario:
- Immediate valid status → assert returned instance’s
is_valid()is True- Ongoing then valid → first call
is_validating(), thenis_valid()→ assert finalis_valid()is True- Invalid status (non-validating) → expect
ValueError("non-validating state: INVALID")- Timeout (always validating) → expect
ValueError("after X seconds")- Exception in
get()→ expect the original exception to bubble upThis ensures
get_validbehavior is exercised rather than only checking the method name.state-manager/app/tasks/verify_graph.py (1)
22-41: LGTM on secrets check; minor cleanup optionalLogic is correct. Optionally simplify set construction.
- present_secrets_set = set() - for secret_name in graph_template.secrets.keys(): - present_secrets_set.add(secret_name) + present_secrets_set = set(graph_template.secrets.keys())
♻️ Duplicate comments (30)
state-manager/.dockerignore (1)
26-32: Add htmlcov/ and .env. to the ignore list.*Keeps coverage HTML and local env variants out of the Docker build context.
Dockerfile tests/ pytest.ini .pytest_cache/ .coverage .coverage.* coverage.xml +htmlcov/ +.env.*state-manager/pyproject.toml (1)
13-13: Move pytest-cov to dev dependencies (duplicate of prior feedback).pytest-cov is a tooling dep; keep it out of runtime deps.
[project] dependencies = [ "beanie>=2.0.0", "cryptography>=45.0.5", "fastapi>=0.116.1", "httpx>=0.28.1", "json-schema-to-pydantic>=0.4.1", - "pytest-cov>=6.2.1", "python-dotenv>=1.1.1", "structlog>=25.4.0", "uvicorn>=0.35.0", ] [dependency-groups] dev = [ "ruff>=0.12.5", "pytest>=8.0.0", "pytest-asyncio>=0.24.0", + "pytest-cov>=6.2.1", "asgi-lifespan>=2.1.0", ].github/workflows/test-state-manager.yml (5)
26-29: Hardcoded Mongo admin creds in CI (use GitHub Secrets).Replace inline username/password; rotate leaked creds.
- env: - MONGO_INITDB_ROOT_USERNAME: admin - MONGO_INITDB_ROOT_PASSWORD: password - MONGO_INITDB_DATABASE: test_db + env: + MONGO_INITDB_ROOT_USERNAME: ${{ secrets.CI_MONGO_USERNAME }} + MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.CI_MONGO_PASSWORD }} + MONGO_INITDB_DATABASE: test_db
49-49: Remove trailing spaces (yamllint).- +
52-56: Secrets and DB URI must not be inlined; add authSource=admin.Use secrets for URI/keys; include authSource for root user.
- env: - MONGO_URI: mongodb://admin:password@localhost:27017 - MONGO_DATABASE_NAME: test_exosphere_state_manager - STATE_MANAGER_SECRET: test-secret-key - SECRETS_ENCRYPTION_KEY: YTzpUlBGLSwm-3yKJRJTZnb0_aQuQQHyz64s8qAERVU= + env: + MONGO_URI: ${{ format('mongodb://{0}:{1}@localhost:27017/?authSource=admin', secrets.CI_MONGO_USERNAME, secrets.CI_MONGO_PASSWORD) }} + MONGO_DATABASE_NAME: test_exosphere_state_manager + STATE_MANAGER_SECRET: ${{ secrets.STATE_MANAGER_SECRET }} + SECRETS_ENCRYPTION_KEY: ${{ secrets.STATE_MANAGER_ENCRYPTION_KEY }}Optionally prefer service host:
- MONGO_URI: ...@localhost:27017/?authSource=admin + MONGO_URI: ...@mongodb:27017/?authSource=admin
50-58: Generate ephemeral SECRETS_ENCRYPTION_KEY at runtime (avoid committing keys).Add a prior step:
- name: Generate ephemeral encryption key run: | python - <<'PY' import os, base64 print('SECRETS_ENCRYPTION_KEY=' + base64.urlsafe_b64encode(os.urandom(32)).decode()) PY >> $GITHUB_ENVThen remove the hardcoded key from env (as shown above).
68-68: Add trailing newline at EOF (yamllint)..github/workflows/release-state-manager.yml (3)
29-33: Hardcoded Mongo admin creds in CI (use GitHub Secrets).- --health-retries 10 - env: - MONGO_INITDB_ROOT_USERNAME: admin - MONGO_INITDB_ROOT_PASSWORD: password - MONGO_INITDB_DATABASE: test_db + --health-retries 10 + env: + MONGO_INITDB_ROOT_USERNAME: ${{ secrets.CI_MONGO_USERNAME }} + MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.CI_MONGO_PASSWORD }} + MONGO_INITDB_DATABASE: test_db
51-51: Remove trailing spaces (yamllint).- +
54-58: Inline secrets and missing authSource in test env.Move to secrets; add authSource=admin; rotate any exposed keys.
- env: - MONGO_URI: mongodb://admin:password@localhost:27017 - MONGO_DATABASE_NAME: test_exosphere_state_manager - STATE_MANAGER_SECRET: test-secret-key - SECRETS_ENCRYPTION_KEY: YTzpUlBGLSwm-3yKJRJTZnb0_aQuQQHyz64s8qAERVU= + env: + MONGO_URI: ${{ format('mongodb://{0}:{1}@localhost:27017/?authSource=admin', secrets.CI_MONGO_USERNAME, secrets.CI_MONGO_PASSWORD) }} + MONGO_DATABASE_NAME: test_exosphere_state_manager + STATE_MANAGER_SECRET: ${{ secrets.STATE_MANAGER_SECRET }} + SECRETS_ENCRYPTION_KEY: ${{ secrets.STATE_MANAGER_ENCRYPTION_KEY }}.github/workflows/publish-state-mangaer.yml (3)
31-34: Hardcoded Mongo admin creds in CI (use GitHub Secrets).- env: - MONGO_INITDB_ROOT_USERNAME: admin - MONGO_INITDB_ROOT_PASSWORD: password - MONGO_INITDB_DATABASE: test_db + env: + MONGO_INITDB_ROOT_USERNAME: ${{ secrets.CI_MONGO_USERNAME }} + MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.CI_MONGO_PASSWORD }} + MONGO_INITDB_DATABASE: test_db
56-61: Inline secrets in test env; prefer service host and authSource.- env: - MONGO_URI: mongodb://admin:password@localhost:27017 - MONGO_DATABASE_NAME: test_exosphere_state_manager - STATE_MANAGER_SECRET: test-secret-key - SECRETS_ENCRYPTION_KEY: YTzpUlBGLSwm-3yKJRJTZnb0_aQuQQHyz64s8qAERVU= + env: + MONGO_URI: ${{ format('mongodb://{0}:{1}@mongodb:27017/?authSource=admin', secrets.CI_MONGO_USERNAME, secrets.CI_MONGO_PASSWORD) }} + MONGO_DATABASE_NAME: test_exosphere_state_manager + STATE_MANAGER_SECRET: ${{ secrets.STATE_MANAGER_SECRET }} + SECRETS_ENCRYPTION_KEY: ${{ secrets.STATE_MANAGER_ENCRYPTION_KEY }}
60-61: Generate SECRETS_ENCRYPTION_KEY at runtime; don’t commit keys.Add pre-test step (same as suggested for test-state-manager.yml) and drop the hardcoded key from env.
state-manager/app/config/settings.py (1)
26-38: Stop re-parsing settings on every call; remove module-level singleton to avoid import-time failures.
get_settings()recreates the model each call andsettings = get_settings()at import time can crash the process if env is missing and makes tests brittle. Cache once and exposereload_settings()for tests.+from functools import lru_cache @@ -# Global settings instance - will be updated when get_settings() is called -_settings = None - - -def get_settings() -> Settings: - """Get the global settings instance, reloading from environment if needed.""" - global _settings - _settings = Settings.from_env() - return _settings +@lru_cache(maxsize=1) +def get_settings() -> Settings: + """Get cached settings.""" + return Settings.from_env() + +def reload_settings() -> None: + """Clear settings cache (useful in tests).""" + get_settings.cache_clear() # type: ignore[attr-defined] @@ -# Initialize settings -settings = get_settings() +# Avoid exporting a module-level singleton; import and call get_settings() where needed.state-manager/app/utils/encrypter.py (2)
5-8: Don’t cache settings at import time.Resolve settings in init to respect test-time env overrides and the new cached settings.
-from app.config.settings import get_settings - -settings = get_settings() +from app.config.settings import get_settings
15-16: Fetch the key from settings at instantiation (not module import).- def __init__(self): - key_b64 = settings.secrets_encryption_key + def __init__(self): + key_b64 = get_settings().secrets_encryption_keystate-manager/tests/unit/with_database/test_health_api.py (1)
6-11: Use TestClient as a context manager to ensure lifespan hooks run and sockets close.- client = TestClient(app_fixture) - response = client.get("/health") - assert response.status_code == 200 - assert response.json() == {"message": "OK"} + with TestClient(app_fixture) as client: + response = client.get("/health") + assert response.status_code == 200 + assert response.json() == {"message": "OK"}state-manager/app/utils/check_secret.py (1)
11-16: Harden API-key check: constant-time compare, Optional typing, avoid shadowing.Use compare_digest, accept Optional[str] from Depends when auto_error=False, and don’t shadow the api_key_header dependency.
@@ -from fastapi import Depends, HTTPException -from fastapi.security.api_key import APIKeyHeader -from starlette.status import HTTP_401_UNAUTHORIZED +from fastapi import Depends, HTTPException +from fastapi.security.api_key import APIKeyHeader +from starlette.status import HTTP_401_UNAUTHORIZED +from typing import Optional +from hmac import compare_digest @@ -async def check_api_key(api_key_header: str = Depends(api_key_header)): - settings = get_settings() - if api_key_header == settings.state_manager_secret: - return api_key_header +async def check_api_key(provided_key: Optional[str] = Depends(api_key_header)): + settings = get_settings() + if isinstance(provided_key, str) and compare_digest(provided_key, settings.state_manager_secret): + return provided_key else: raise HTTPException(status_code=HTTP_401_UNAUTHORIZED, detail="Invalid API key")Also applies to: 1-3
state-manager/app/main.py (1)
8-8: Use Motor’s AsyncIOMotorClient with proper shutdown; fix typo.Beanie expects Motor, not pymongo.AsyncMongoClient. Also close the client in lifespan and fix the comment typo.
-from pymongo import AsyncMongoClient +from motor.motor_asyncio import AsyncIOMotorClient @@ - # begaining of the server + # beginning of the server @@ - client = AsyncMongoClient(settings.mongo_uri) + client = AsyncIOMotorClient(settings.mongo_uri) db = client[settings.mongo_database_name] await init_beanie(db, document_models=[State, Namespace, GraphTemplate, RegisteredNode]) logger.info("beanie dbs initialized") @@ - # main logic of the server - yield - - # end of the server - logger.info("server shutting down") + try: + # main logic of the server + yield + finally: + client.close() + # end of the server + logger.info("server shutting down")Also applies to: 35-35, 43-57
state-manager/tests/unit/utils/test_check_secret.py (1)
169-178: Add missing asyncio marker; this async test will be skipped/fail.@patch.dict(os.environ, {'STATE_MANAGER_SECRET': 'test-constant-key'}) - async def test_api_key_loads_from_environment(self): + @pytest.mark.asyncio + async def test_api_key_loads_from_environment(self):state-manager/tests/unit/with_database/conftest.py (1)
6-13: Fix Ruff F401 (unused imports) and set loop as current for reliability.Remove unused imports and ensure the created loop is set as current during tests.
-import sys -from pathlib import Path @@ -# Add the project root to Python path to ensure proper imports -# project_root = Path(__file__).parent.parent.parent.parent -# sys.path.insert(0, str(project_root))@pytest.fixture(scope="session") def event_loop(): """Create an event loop for the tests.""" - loop = asyncio.new_event_loop() - yield loop - loop.close() + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + try: + yield loop + finally: + loop.close()Also applies to: 16-21
state-manager/tests/unit/tasks/test_verify_graph.py (2)
10-11: Fix incorrect import for NodeTemplate.Import from node_template_model.
-from app.models.db.graph_template_model import NodeTemplate +from app.models.node_template_model import NodeTemplate
348-356: Don’t catch AssertionError; assert on reported validation error.verify_inputs returns an error when identifier is missing; assert on that instead of swallowing exceptions.
- # 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 err + for err in errors + )state-manager/app/tasks/verify_graph.py (2)
11-20: Drop trailing period in error message to match tests and existing styleAligns with prior feedback; prevents brittle assertions.
- errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.") + errors.append(f"Node {node[0]} in namespace {node[1]} does not exist")
43-93: Harden input validation: schema parsing errors, Optional[str] support, non-string inputs, extra keys, safe outputs parsingCurrent logic can crash on invalid JSON Schemas, rejects Optional[str], and doesn’t flag extra inputs. It may also raise on non-string template inputs.
-async def verify_inputs(graph_template: GraphTemplate, registered_nodes: list[RegisteredNode]) -> list[str]: +async def verify_inputs(graph_template: GraphTemplate, registered_nodes: list[RegisteredNode]) -> list[str]: errors = [] - look_up_table = { - (rn.name, rn.namespace): rn - for rn in registered_nodes - } + registered_node_lookup = { (rn.name, rn.namespace): rn for rn in registered_nodes } for node in graph_template.nodes: if node.inputs is None: continue - - registered_node = look_up_table.get((node.node_name, node.namespace)) + + registered_node = registered_node_lookup.get((node.node_name, node.namespace)) if registered_node is None: errors.append(f"Node {node.node_name} in namespace {node.namespace} does not exist") continue - - registered_node_input_model = create_model(registered_node.inputs_schema) + + # Build inputs model; catch invalid schemas + try: + registered_node_input_model = create_model(registered_node.inputs_schema) + except Exception as e: + errors.append(f"Invalid inputs schema for node {node.node_name} in namespace {node.namespace}: {e}") + continue - for input_name, input_info in registered_node_input_model.model_fields.items(): - if input_info.annotation is not str: + # Accept str or Optional[str]/Union[..., str, ...] + from typing import Union, get_origin, get_args + def _is_str_like(ann) -> bool: + return (ann is str) or (get_origin(ann) is Union and str in get_args(ann)) + + for input_name, input_info in registered_node_input_model.model_fields.items(): + if not _is_str_like(input_info.annotation): errors.append(f"Input {input_name} in node {node.node_name} in namespace {node.namespace} is not a string") continue - if input_name not in node.inputs.keys(): + if input_name not in node.inputs: errors.append(f"Input {input_name} in node {node.node_name} in namespace {node.namespace} is not present in the graph template") continue - - dependent_strings = node.get_dependent_strings() + + # Extra inputs present in template but not declared by schema + extra_inputs = set(node.inputs.keys()) - set(registered_node_input_model.model_fields.keys()) + for extra in extra_inputs: + errors.append(f"Input {extra} in node {node.node_name} in namespace {node.namespace} not defined by registered node schema") + + # Parse dependent strings; guard non-strings + try: + dependent_strings = node.get_dependent_strings() + except Exception as e: + errors.append(f"Error processing inputs for node {node.node_name} in namespace {node.namespace}: {e}") + continue for dependent_string in dependent_strings: identifier_field_pairs = dependent_string.get_identifier_field() for identifier, field in identifier_field_pairs: temp_node = graph_template.get_node_by_identifier(identifier) if temp_node is None: errors.append(f"Node {identifier} does not exist in the graph template") continue - registered_node = look_up_table.get((temp_node.node_name, temp_node.namespace)) + registered_node = registered_node_lookup.get((temp_node.node_name, temp_node.namespace)) if registered_node is None: errors.append(f"Node {temp_node.node_name} in namespace {temp_node.namespace} does not exist") continue - output_model = create_model(registered_node.outputs_schema) + # Build outputs model; catch invalid schemas + try: + output_model = create_model(registered_node.outputs_schema) + except Exception as e: + errors.append(f"Invalid outputs schema for node {temp_node.node_name} in namespace {temp_node.namespace}: {e}") + continue if field not in output_model.model_fields.keys(): errors.append(f"Field {field} in node {temp_node.node_name} in namespace {temp_node.namespace} does not exist") continue - if output_model.model_fields[field].annotation is not str: + if not _is_str_like(output_model.model_fields[field].annotation): errors.append(f"Field {field} in node {temp_node.node_name} in namespace {temp_node.namespace} is not a string")state-manager/app/models/db/graph_template_model.py (5)
49-56: Clarify ‘unites’ comment and improve root error diagnosticsUse precise terminology and list identifiers, not object reprs.
- if node.unites is not None: - # If the node has a unit, it should have an in-degree of 1 - # As unites.node.identifier acts as the parent of the node - in_degree[node.identifier] += 1 + if node.unites is not None: + # A 'unites' node runs after its unites target (synchronization point), + # so it must have at least one predecessor and cannot be a root. + in_degree[node.identifier] += 1 @@ - if len(zero_in_degree_nodes) != 1: - raise ValueError("There should be exactly one root node in the graph but found " + str(len(zero_in_degree_nodes)) + " nodes with zero in-degree: " + str(zero_in_degree_nodes)) + if len(zero_in_degree_nodes) != 1: + zero_ids = [n.identifier for n in zero_in_degree_nodes] + raise ValueError(f"There should be exactly one root node in the graph but found {len(zero_in_degree_nodes)} nodes with zero in-degree: {zero_ids}")
115-127: Simplify non-empty validatorsPydantic already ensures non-None; prefer truthiness check.
def validate_name(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") return v @@ def validate_namespace(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty") return v
228-247: Leverage NodeTemplate.get_dependent_strings and improve messagesAvoid duplicate parsing; include node context in errors.
- def verify_input_dependencies(self) -> Self: + def verify_input_dependencies(self) -> Self: errors = [] - for node in self.nodes: - for input_value in node.inputs.values(): - try: - if not isinstance(input_value, str): - errors.append(f"Input {input_value} is not a string") - continue - - dependent_string = DependentString.create_dependent_string(input_value) - dependent_identifiers = set([identifier for identifier, _ in dependent_string.get_identifier_field()]) - - for identifier in dependent_identifiers: - if 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}") - - except Exception as e: - errors.append(f"Error creating dependent string for input {input_value}: {e}") + for node in self.nodes: + try: + for dependent_string in node.get_dependent_strings(): + dependent_identifiers = {identifier for identifier, _ in dependent_string.get_identifier_field()} + allowed_parents = self.get_parents_by_identifier(node.identifier) + for identifier in dependent_identifiers: + if identifier not in allowed_parents: + errors.append( + f"Node {node.identifier} input depends on {identifier} " + f"but it is not a parent of {node.identifier}" + ) + except Exception as e: + errors.append(f"Node {node.identifier} has invalid inputs: {e}") if errors: raise ValueError("\n".join(errors)) return self
112-114: Avoid broad exception capture; preserve original tracebackCatching Exception and re-wrapping as ValueError obscures root causes.
- except Exception as e: - raise ValueError(f"Error building dependency graph: {e}") + except ValueError: + raise + except Exception as e: + # Let unexpected errors bubble with original type; or log and re-raise + raise
84-90: Fix unites-visited branch: don’t overwrite parents; include current node for descendantsOverwriting “parents” drops existing ancestry and excludes the current node from children’s parents.
- elif visited[node.unites.identifier]: - parents = self._parents_by_identifier[node.unites.identifier] - self._parents_by_identifier[node.identifier] = parents | {node.unites.identifier} - parents_for_children = parents | {node.unites.identifier} + elif visited[node.unites.identifier]: + unites_parents = self._parents_by_identifier[node.unites.identifier] | {node.unites.identifier} + combined_parents = parents | unites_parents + # Merge into current node's parents (preserve prior unions) + self._parents_by_identifier[node.identifier] = ( + self._parents_by_identifier[node.identifier] | combined_parents + ) + # Children inherit combined ancestry plus the current node + parents_for_children = combined_parents | {node_identifier}
📜 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 ignored due to path filters (1)
state-manager/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/publish-state-mangaer.yml(1 hunks).github/workflows/release-state-manager.yml(2 hunks).github/workflows/test-state-manager.yml(2 hunks)state-manager/.dockerignore(1 hunks)state-manager/app/config/settings.py(1 hunks)state-manager/app/main.py(2 hunks)state-manager/app/models/db/graph_template_model.py(6 hunks)state-manager/app/tasks/verify_graph.py(2 hunks)state-manager/app/utils/check_secret.py(1 hunks)state-manager/app/utils/encrypter.py(1 hunks)state-manager/pyproject.toml(1 hunks)state-manager/pytest.ini(1 hunks)state-manager/tests/integration/integration_full_workflow_integration.py(0 hunks)state-manager/tests/integration/peinding_test_full_workflow_integration.py(0 hunks)state-manager/tests/unit/models/test_base.py(0 hunks)state-manager/tests/unit/models/test_graph_template_model.py(1 hunks)state-manager/tests/unit/tasks/test_verify_graph.py(2 hunks)state-manager/tests/unit/test_main.py(0 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)state-manager/tests/unit/utils/test_encrypter.py(0 hunks)state-manager/tests/unit/with_database/conftest.py(1 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)state-manager/tests/unit/with_database/test_health_api.py(1 hunks)
💤 Files with no reviewable changes (5)
- state-manager/tests/unit/utils/test_encrypter.py
- state-manager/tests/unit/models/test_base.py
- state-manager/tests/unit/test_main.py
- state-manager/tests/integration/integration_full_workflow_integration.py
- state-manager/tests/integration/peinding_test_full_workflow_integration.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (10)
state-manager/tests/unit/with_database/test_graph_template.py (3)
state-manager/app/models/db/graph_template_model.py (3)
GraphTemplate(16-323)get_root_node(271-275)get_parents_by_identifier(288-293)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/tests/unit/with_database/conftest.py (1)
app_started(23-26)
state-manager/tests/unit/with_database/test_health_api.py (1)
state-manager/tests/unit/with_database/conftest.py (1)
app_fixture(29-33)
state-manager/app/main.py (1)
state-manager/app/config/settings.py (1)
get_settings(30-34)
state-manager/tests/unit/models/test_graph_template_model.py (1)
state-manager/app/models/db/graph_template_model.py (2)
GraphTemplate(16-323)get_valid(303-323)
state-manager/app/utils/encrypter.py (1)
state-manager/app/config/settings.py (1)
get_settings(30-34)
state-manager/tests/unit/utils/test_check_secret.py (1)
state-manager/app/utils/check_secret.py (1)
check_api_key(11-16)
state-manager/app/utils/check_secret.py (1)
state-manager/app/config/settings.py (1)
get_settings(30-34)
state-manager/app/tasks/verify_graph.py (4)
state-manager/app/models/db/graph_template_model.py (3)
GraphTemplate(16-323)get(296-300)get_node_by_identifier(280-286)state-manager/app/models/db/registered_node.py (2)
RegisteredNode(8-44)list_nodes_by_templates(34-44)state-manager/app/models/node_template_model.py (1)
get_dependent_strings(61-67)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
state-manager/tests/unit/tasks/test_verify_graph.py (3)
state-manager/app/tasks/verify_graph.py (4)
verify_inputs(43-93)verify_graph(95-124)verify_node_exists(11-20)verify_secrets(22-41)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/db/graph_template_model.py (1)
get_node_by_identifier(280-286)
state-manager/app/models/db/graph_template_model.py (3)
state-manager/app/models/db/base.py (1)
BaseDatabaseModel(7-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/dependent_string.py (3)
DependentString(9-62)create_dependent_string(24-42)get_identifier_field(60-62)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/with_database/conftest.py
[error] 6-6: Ruff: F401: 'sys' imported but unused. Command: 'ruff check $FILES_ARGS'.
[error] 7-7: Ruff: F401: 'Path' imported but unused. Command: 'ruff check $FILES_ARGS'.
🪛 Gitleaks (8.27.2)
.github/workflows/release-state-manager.yml
58-58: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.github/workflows/test-state-manager.yml
56-56: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.github/workflows/publish-state-mangaer.yml
60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Checkov (3.2.334)
.github/workflows/release-state-manager.yml
[MEDIUM] 55-56: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 58-59: Base64 High Entropy String
(CKV_SECRET_6)
.github/workflows/test-state-manager.yml
[MEDIUM] 53-54: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 56-57: Base64 High Entropy String
(CKV_SECRET_6)
.github/workflows/publish-state-mangaer.yml
[MEDIUM] 57-58: Basic Auth Credentials
(CKV_SECRET_4)
[LOW] 60-61: Base64 High Entropy String
(CKV_SECRET_6)
🪛 YAMLlint (1.37.1)
.github/workflows/release-state-manager.yml
[error] 51-51: trailing spaces
(trailing-spaces)
.github/workflows/test-state-manager.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
state-manager/pyproject.toml (1)
21-25: Dev dep addition LGTM.asgi-lifespan in dev group is appropriate.
state-manager/app/tasks/verify_graph.py (1)
98-118: LGTM: parallelization + proper flatteningThe asyncio.gather + flattening loop is correct; error status handling is restored.
state-manager/app/models/db/graph_template_model.py (1)
288-294: LGTM: safe parents lookupReturning an empty set for missing identifiers avoids KeyError paths.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
state-manager/tests/unit/with_database/conftest.py (1)
8-13: Set the created loop as current and reset it after teardown.Without setting it current, some plugins/utilities that call asyncio.get_event_loop() may misbehave; also reset after close to avoid “Event loop is closed” leakage.
@pytest.fixture(scope="session") def event_loop(): """Create an event loop for the tests.""" - loop = asyncio.new_event_loop() - yield loop - loop.close() + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + try: + yield loop + finally: + asyncio.set_event_loop(None) + loop.close()
📜 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 (1)
state-manager/tests/unit/with_database/conftest.py(1 hunks)
🔇 Additional comments (2)
state-manager/tests/unit/with_database/conftest.py (2)
15-20: LGTM: lifespan manager usage is correct.This reliably exercises startup/shutdown around the app for DB-backed tests.
8-26: Add pytest config for thewith_databasemarker and session-scoped asyncio support.
No pytest.ini (or equivalent) was found defining custom markers orasyncio_mode; please verify your test config and add:[pytest] markers = with_database: mark tests requiring a database asyncio_mode = auto
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
state-manager/tests/unit/with_database/conftest.py (4)
10-13: Remove sys.path mutation; risks import shadowing.Rely on repo layout/runner instead of inserting a path (which may resolve to tests/ and shadow app code).
-# Add the project root directory to the Python path -project_root = str(pathlib.Path(__file__).parent.parent.parent.parent) -sys.path.insert(0, project_root)
14-20: Set the new event loop as current and clear it after close.Some plugins call asyncio.get_event_loop(); without setting, they can fail.
@pytest.fixture(scope="session") def event_loop(): """Create an event loop for the tests.""" - loop = asyncio.new_event_loop() + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) yield loop loop.close() + asyncio.set_event_loop(None)
21-33: Reduce fixture scope to avoid shared mutated state across tests.Sharing app across session can cause flakes if routes/dependencies mutate.
-@pytest.fixture(scope="session") +@pytest.fixture(scope="function") async def app_started(app_fixture): @@ -@pytest.fixture(scope="session") +@pytest.fixture(scope="function") def app_fixture():
34-35: Align comment with marker name.-# Mark all tests in this directory as integration tests +# Mark all tests in this directory as database-backed tests pytestmark = pytest.mark.with_databasestate-manager/tests/unit/with_database/test_graph_template.py (5)
7-9: Prefer anyio marker at module level; drop per-test asyncio decorators.This avoids decorator noise and integrates better with ASGI lifespan helpers.
-import pytest +import pytest +pytestmark = pytest.mark.anyio @@ -@pytest.mark.asyncio async def test_graph_template_basic(app_started): @@ -@pytest.mark.asyncio -async def test_liner_graph_template(app_started): +async def test_linear_graph_template(app_started): @@ -@pytest.mark.asyncio -async def test_graph_template_invalid_liner_graph_template(app_started): +async def test_graph_template_invalid_linear_graph_template(app_started): @@ -@pytest.mark.asyncio -async def test_self_unites_validation(app_started): +async def test_self_unites_validation(app_started):Also applies to: 27-29, 74-76, 86-88
29-33: Typo: liner → linear (function name and docstring).-async def test_liner_graph_template(app_started): - """Test liner graph template creation""" +async def test_linear_graph_template(app_started): + """Test linear graph template creation"""
59-63: Prefer [] over None for next_nodes to match list schema.Unless Optional[List[str]] is intended, use empty list for clarity.
- next_nodes=None, + next_nodes=[], @@ - next_nodes=None, + next_nodes=[],Also applies to: 99-103
25-25: Strengthen basic test to exercise root and lookup APIs.- assert graph_template_model.name == "test_graph_template" + assert graph_template_model.name == "test_graph_template" + assert graph_template_model.get_root_node().identifier == "test_identifier" + node = graph_template_model.get_node_by_identifier("test_identifier") + assert node is not None and node.identifier == "test_identifier"
75-83: Rename invalid test for consistency (liner → linear).-async def test_graph_template_invalid_liner_graph_template(app_started): - """Test invalid liner graph template creation""" +async def test_graph_template_invalid_linear_graph_template(app_started): + """Test invalid linear graph template creation"""
📜 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 (3)
state-manager/app/controller/upsert_graph_template.py(2 hunks)state-manager/tests/unit/with_database/conftest.py(1 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T16:46:04.030Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#132
File: state-manager/app/controller/upsert_graph_template.py:11-27
Timestamp: 2025-08-03T16:46:04.030Z
Learning: In the exospherehost codebase, for upsert operations on graph templates, the team prioritizes API idempotency over avoiding race conditions in the database layer implementation. The approach of separate find and insert/update operations is acceptable when the API behavior remains consistent.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
🧬 Code graph analysis (2)
state-manager/app/controller/upsert_graph_template.py (2)
state-manager/app/models/db/graph_template_model.py (2)
set_secrets(252-254)GraphTemplate(16-323)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)
state-manager/tests/unit/with_database/test_graph_template.py (4)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(16-323)get_root_node(271-275)get_parents_by_identifier(288-293)get_node_by_identifier(280-286)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
Unites(6-7)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)
- Added `_path_by_identifier` to track paths in the GraphTemplate model. - Refactored `_build_parents_by_identifier` to `_build_parents_path_by_identifier` for clarity. - Updated depth-first search (DFS) logic to populate the new path tracking. - Introduced new tests for validating graph structures, including acyclic checks and parent propagation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
state-manager/tests/unit/with_database/test_graph_template.py (2)
29-33: Typo: “liner” → “linear” in test name and docstring.Rename for clarity and consistency.
-async def test_liner_graph_template(app_started): - """Test liner graph template creation""" +async def test_linear_graph_template(app_started): + """Test linear graph template creation"""
68-72: Remove type: ignore; assert non-None before deref.Avoid suppressing type checks; validate and then assert identifiers.
- assert graph_template_model.get_node_by_identifier("node1").identifier == "node1" # type: ignore - assert graph_template_model.get_node_by_identifier("node2").identifier == "node2" # type: ignore + node1 = graph_template_model.get_node_by_identifier("node1") + node2 = graph_template_model.get_node_by_identifier("node2") + assert node1 is not None and node1.identifier == "node1" + assert node2 is not None and node2.identifier == "node2"state-manager/app/models/db/graph_template_model.py (4)
49-57: Clarify unites comment and improve root error diagnostics.Use accurate terminology and list identifiers instead of object reprs.
- if node.unites is not None: - # If the node has a unit, it should have an in-degree of 1 - # As unites.node.identifier acts as the parent of the node - in_degree[node.identifier] += 1 + if node.unites is not None: + # A 'unites' node runs after its unites target; it must have a predecessor and cannot be a root. + in_degree[node.identifier] += 1 @@ - zero_in_degree_nodes = [node for node in self.nodes if in_degree[node.identifier] == 0] - if len(zero_in_degree_nodes) != 1: - raise ValueError("There should be exactly one root node in the graph but found " + str(len(zero_in_degree_nodes)) + " nodes with zero in-degree: " + str(zero_in_degree_nodes)) - self._root_node = zero_in_degree_nodes[0] + zero_in_degree_nodes = [node for node in self.nodes if in_degree[node.identifier] == 0] + if len(zero_in_degree_nodes) != 1: + zero_ids = [n.identifier for n in zero_in_degree_nodes] + raise ValueError(f"There should be exactly one root node in the graph but found {len(zero_in_degree_nodes)} nodes with zero in-degree: {zero_ids}") + self._root_node = zero_in_degree_nodes[0]
121-130: Simplify empty-field validators.Pydantic ensures required str fields aren’t None; use truthiness checks.
- if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") @@ - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty")
231-249: Skip non-string inputs during dependency parsing.Treat non-strings as literals; don’t emit errors for them.
for node in self.nodes: for input_value in node.inputs.values(): try: - if not isinstance(input_value, str): - errors.append(f"Input {input_value} is not a string") - continue + if not isinstance(input_value, str): + continue
89-93: Bug: unites-visited branch drops parents and omits current node for children.When the unites target is already visited, descendants lose the current node as a parent and prior parents aren’t merged. Fix by merging sets and including the current node in
parents_for_children.- elif visited[node.unites.identifier]: - parents = self._parents_by_identifier[node.unites.identifier] - self._parents_by_identifier[node.identifier] = parents | {node.unites.identifier} - parents_for_children = parents | {node.unites.identifier} + elif visited[node.unites.identifier]: + unites_parents = self._parents_by_identifier[node.unites.identifier] | {node.unites.identifier} + combined_parents = (parents | unites_parents) + self._parents_by_identifier[node_identifier] = combined_parents | self._parents_by_identifier[node_identifier] + parents_for_children = combined_parents | {node_identifier}
📜 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 (2)
state-manager/app/models/db/graph_template_model.py(6 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (1)
state-manager/tests/unit/with_database/test_graph_template.py (3)
state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
Unites(6-7)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/with_database/test_graph_template.py
[error] 1-1: Ruff lint error: F401 imported but unused: 'pydantic.NonNegativeInt' in this file. Remove unused import or run 'ruff --fix'.
🪛 GitHub Actions: Check spells before PR merge
state-manager/tests/unit/with_database/test_graph_template.py
[error] 443-443: codespell detected a misspelling: 'fliped' should be 'flipped'.
- Removed redundant type checks for secret names and values in GraphTemplate model. - Added tests to validate secret handling and graph input dependencies. - Simplified health API test by directly calling the health function instead of using TestClient.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
state-manager/tests/unit/with_database/test_health_api.py (1)
1-7: Convert to HTTP-level test with context-managed TestClient; remove unused imports.Exercise routing and lifespan, and fix the Ruff F401 by dropping the unused
pytestimport.-import pytest -from app.main import health +from fastapi.testclient import TestClient @@ -def test_health_api(): - """Test the health API endpoint function.""" - response = health() - assert response == {"message": "OK"} +def test_health_api(app_fixture): + """Exercise the HTTP /health endpoint with app lifespan.""" + with TestClient(app_fixture) as client: + response = client.get("/health") + assert response.status_code == 200 + assert response.json() == {"message": "OK"}state-manager/app/models/db/graph_template_model.py (6)
118-131: Simplify empty checks for name/namespaceUse truthiness; Pydantic ensures type. This repeats earlier feedback.
def validate_name(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty") return v @@ def validate_namespace(cls, v: str) -> str: - if v == "" or v is None: + if not v: raise ValueError("Namespace cannot be empty") return v
142-171: Combine node-identifier validators to reduce passesMerge duplicate-identifier and next-nodes existence checks into one @field_validator for clarity and performance. Previously suggested.
- @field_validator('nodes') - @classmethod - def validate_unique_identifiers(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - errors = [] - for node in v: - if node.identifier in identifiers: - errors.append(f"Node identifier {node.identifier} is not unique") - identifiers.add(node.identifier) - if errors: - raise ValueError("\n".join(errors)) - return v - - @field_validator('nodes') - @classmethod - def validate_next_nodes_identifiers_exist(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - for node in v: - identifiers.add(node.identifier) - - errors = [] - for node in v: - if node.next_nodes: - for next_node in node.next_nodes: - if next_node not in identifiers: - errors.append(f"Node identifier {next_node} does not exist in the graph") - if errors: - raise ValueError("\n".join(errors)) - return v + @field_validator('nodes') + @classmethod + def validate_nodes_references(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: + identifiers: set[str] = set() + errors: list[str] = [] + for node in v: + if node.identifier in identifiers: + errors.append(f"Node identifier {node.identifier} is not unique") + identifiers.add(node.identifier) + if errors: + raise ValueError("\n".join(errors)) + for node in v: + if node.next_nodes: + for nxt in node.next_nodes: + if nxt not in identifiers: + errors.append(f"Node identifier {nxt} does not exist in the graph") + if errors: + raise ValueError("\n".join(errors)) + return v
226-249: Leverage NodeTemplate.get_dependent_strings and add node context to errorsAvoid duplicate parsing and make messages clearer. Previously suggested.
- for node in self.nodes: - for input_value in node.inputs.values(): - try: - if not isinstance(input_value, str): - errors.append(f"Input {input_value} is not a string") - continue - - dependent_string = DependentString.create_dependent_string(input_value) - dependent_identifiers = set([identifier for identifier, _ in dependent_string.get_identifier_field()]) - - for identifier in dependent_identifiers: - if 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}") - - except Exception as e: - errors.append(f"Error creating dependent string for input {input_value}: {e}") + for node in self.nodes: + try: + for ds in node.get_dependent_strings(): + for identifier, _ in ds.get_identifier_field(): + if identifier not in self.get_parents_by_identifier(node.identifier): + errors.append( + f"Input {ds.head + '${{...}}'} depends on {identifier} but {identifier} is not a parent of {node.identifier}" + ) + except Exception as e: + errors.append(f"Node {node.identifier} has invalid inputs: {e}")
49-57: Clarify unites comment and improve root error diagnosticsRename “unit” to “unites” and list identifiers in the error for faster debugging.
if node.unites is not None: - # If the node has a unit, it should have an in-degree of 1 - # As unites.node.identifier acts as the parent of the node + # A 'unites' node runs after its unites target and therefore cannot be a root. in_degree[node.identifier] += 1 zero_in_degree_nodes = [node for node in self.nodes if in_degree[node.identifier] == 0] - if len(zero_in_degree_nodes) != 1: - raise ValueError("There should be exactly one root node in the graph but found " + str(len(zero_in_degree_nodes)) + " nodes with zero in-degree: " + str(zero_in_degree_nodes)) + if len(zero_in_degree_nodes) != 1: + zero_ids = [n.identifier for n in zero_in_degree_nodes] + raise ValueError(f"There should be exactly one root node in the graph but found {len(zero_in_degree_nodes)} nodes with zero in-degree: {zero_ids}") self._root_node = zero_in_degree_nodes[0]
115-116: Preserve original exceptions; avoid overbroad wrappingCatching all Exceptions obscures root causes. Let ValueError pass and only wrap unexpected errors with chaining.
- except Exception as e: - raise ValueError(f"Error building dependency graph: {e}") + except ValueError: + # Preserve validation errors as-is. + raise + except Exception as e: + raise ValueError("Error building dependency graph") from e
87-93: Children of a unites node miss the direct parent in ancestryIn the visited-unites branch, descendants don’t inherit the current node as a parent, which can incorrectly block children from referencing the current node’s outputs. Include the current node in parents_for_children.
- elif visited[node.unites.identifier]: - parents = self._parents_by_identifier[node.unites.identifier] - self._parents_by_identifier[node.identifier] = parents | {node.unites.identifier} - parents_for_children = parents | {node.unites.identifier} + elif visited[node.unites.identifier]: + unites_parents = self._parents_by_identifier[node.unites.identifier] | {node.unites.identifier} + # Node’s own parents are those of the unites target (design choice), but + # its children must still include the current node as a direct ancestor. + self._parents_by_identifier[node.identifier] = unites_parents + parents_for_children = unites_parents | {node_identifier}state-manager/tests/unit/with_database/test_graph_template.py (3)
29-33: Typo: “liner” → “linear” (name and docstring)Keeps test names readable.
-async def test_liner_graph_template(app_started): - """Test liner graph template creation""" +async def test_linear_graph_template(app_started): + """Test linear graph template creation""" - name="test_liner_graph_template", + name="test_linear_graph_template",
71-72: Avoid type: ignore; assert non-None before derefSafer and keeps type checkers happy. Previously suggested.
- assert graph_template_model.get_node_by_identifier("node1").identifier == "node1" # type: ignore - assert graph_template_model.get_node_by_identifier("node2").identifier == "node2" # type: ignore + node1 = graph_template_model.get_node_by_identifier("node1") + node2 = graph_template_model.get_node_by_identifier("node2") + assert node1 is not None and node1.identifier == "node1" + assert node2 is not None and node2.identifier == "node2"
1-1: Fix Ruff F401: remove unused importThis breaks CI; delete the unused NonNegativeInt import.
-from pydantic import NonNegativeInt
📜 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 (3)
state-manager/app/models/db/graph_template_model.py(5 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)state-manager/tests/unit/with_database/test_health_api.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/tests/unit/with_database/test_graph_template.pystate-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (2)
state-manager/tests/unit/with_database/test_graph_template.py (5)
state-manager/app/models/db/graph_template_model.py (5)
GraphTemplate(16-329)get_root_node(270-274)get_parents_by_identifier(287-292)get_node_by_identifier(279-285)get_path_by_identifier(294-299)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (2)
Unites(6-7)get_dependent_strings(61-67)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
state-manager/app/models/db/graph_template_model.py (4)
state-manager/app/models/db/base.py (1)
BaseDatabaseModel(7-15)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/dependent_string.py (3)
DependentString(9-62)create_dependent_string(24-42)get_identifier_field(60-62)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/with_database/test_health_api.py
[error] 1-1: Ruff: F401 'pytest' imported but unused. Remove unused import.
state-manager/tests/unit/with_database/test_graph_template.py
[error] 1-1: Ruff: F401 'NonNegativeInt' imported but unused. Remove unused import.
🔇 Additional comments (2)
state-manager/app/models/db/graph_template_model.py (2)
99-103: Path propagation on awaited children: confirm intentYou pass the child’s previously stored path instead of the parent-augmented path, which may under-report cycles via awaited unites edges. Consider passing path | {node_identifier}.
- for awaiting_identifier in awaiting_parent[node_identifier]: - dfs(awaiting_identifier, parents_for_children, self._path_by_identifier[awaiting_identifier]) + for awaiting_identifier in awaiting_parent[node_identifier]: + dfs(awaiting_identifier, parents_for_children, path | {node_identifier})Would you like a follow-up test asserting cycle detection across awaited unites paths?
301-330: LGTM: get_valid polling contractInput validation, minimum interval coercion, and timeout handling look solid.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
state-manager/tests/unit/with_database/test_health_api.py (1)
1-6: Exercise the ASGI stack; use TestClient with lifespan.Directly calling
health()bypasses routing, middleware, and startup/shutdown hooks. Test via HTTP and use a context manager so lifespan runs and sockets close deterministically. (Similar to a prior suggestion.)-from app.main import health +from fastapi.testclient import TestClient +from app.main import app -def test_health_api(): - """Test the health API endpoint function.""" - response = health() - assert response == {"message": "OK"} +def test_health_endpoint_returns_ok(): + """Test the /health endpoint over HTTP.""" + with TestClient(app) as client: + resp = client.get("/health") + assert resp.status_code == 200 + assert resp.json() == {"message": "OK"}state-manager/tests/unit/with_database/test_graph_template.py (3)
28-30: Typo: “liner” → “linear” (function name and docstring).Rename for clarity and consistency.
-@pytest.mark.asyncio -async def test_liner_graph_template(app_started): - """Test liner graph template creation""" +@pytest.mark.asyncio +async def test_linear_graph_template(app_started): + """Test linear graph template creation"""
70-71: Avoid# type: ignore; assert non-None before deref.- assert graph_template_model.get_node_by_identifier("node1").identifier == "node1" # type: ignore - assert graph_template_model.get_node_by_identifier("node2").identifier == "node2" # type: ignore + node1 = graph_template_model.get_node_by_identifier("node1") + node2 = graph_template_model.get_node_by_identifier("node2") + assert node1 is not None and node1.identifier == "node1" + assert node2 is not None and node2.identifier == "node2"
654-656: Same here: assert non-None before deref.- dependent_strings = graph_template_model.get_node_by_identifier("node2").get_dependent_strings() + node2 = graph_template_model.get_node_by_identifier("node2") + assert node2 is not None + dependent_strings = node2.get_dependent_strings()state-manager/app/models/db/graph_template_model.py (7)
49-52: Comment wording: “unit” → “unites target”.Minor clarity fix.
- # If the node has a unit, it should have an in-degree of 1 - # As unites.node.identifier acts as the parent of the node + # If the node has an unites target, it must have an incoming edge: + # the unites target acts as the parent of the node.
54-57: Improve root error diagnostics (identifiers, not object repr).Safer and clearer; keeps tests matching “[]” for zero roots.
- if len(zero_in_degree_nodes) != 1: - raise ValueError("There should be exactly one root node in the graph but found " + str(len(zero_in_degree_nodes)) + " nodes with zero in-degree: " + str(zero_in_degree_nodes)) + if len(zero_in_degree_nodes) != 1: + zero_ids = [n.identifier for n in zero_in_degree_nodes] + raise ValueError(f"There should be exactly one root node in the graph but found {len(zero_in_degree_nodes)} nodes with zero in-degree: {zero_ids}")
63-73: Simplify visited bookkeeping: use a set.Less churn, same behavior.
- visited = {node.identifier: False for node in self.nodes} + visited: set[str] = set() @@ - visited[node.identifier] = False + # initialized via membership
78-82: Follow-up to visited set change.- if visited[node_identifier]: + if node_identifier in visited: return - - visited[node_identifier] = True + visited.add(node_identifier)
115-116: Overbroad exception wrapping hides root cause.Preserve specific validation errors; only re-wrap when necessary.
- except Exception as e: - raise ValueError(f"Error building dependency graph: {e}") + except ValueError: + # Preserve specific validation errors + raise
121-129: Simplify empty-string checks.Pydantic ensures non-None for required str fields.
- if v == "" or v is None: + if not v: raise ValueError("Name cannot be empty")Apply similarly to
validate_namespace.
142-170: Combine duplicate node validators to reduce passes.Merge uniqueness and next-node existence checks.
- @field_validator('nodes') - @classmethod - def validate_unique_identifiers(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - errors = [] - for node in v: - if node.identifier in identifiers: - errors.append(f"Node identifier {node.identifier} is not unique") - identifiers.add(node.identifier) - if errors: - raise ValueError("\n".join(errors)) - return v - - @field_validator('nodes') - @classmethod - def validate_next_nodes_identifiers_exist(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: - identifiers = set() - for node in v: - identifiers.add(node.identifier) - - errors = [] - for node in v: - if node.next_nodes: - for next_node in node.next_nodes: - if next_node not in identifiers: - errors.append(f"Node identifier {next_node} does not exist in the graph") - if errors: - raise ValueError("\n".join(errors)) - return v + @field_validator('nodes') + @classmethod + def validate_nodes_structure(cls, v: List[NodeTemplate]) -> List[NodeTemplate]: + identifiers: set[str] = set() + errors: list[str] = [] + for node in v: + if node.identifier in identifiers: + errors.append(f"Node identifier {node.identifier} is not unique") + identifiers.add(node.identifier) + if errors: + raise ValueError("\n".join(errors)) + for node in v: + if node.next_nodes: + for next_node in node.next_nodes: + if next_node not in identifiers: + errors.append(f"Node identifier {next_node} does not exist in the graph") + if errors: + raise ValueError("\n".join(errors)) + return v
📜 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 (3)
state-manager/app/models/db/graph_template_model.py(5 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)state-manager/tests/unit/with_database/test_health_api.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T18:00:23.851Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#292
File: state-manager/app/models/db/graph_template_model.py:0-0
Timestamp: 2025-08-27T18:00:23.851Z
Learning: In the GraphTemplate model, when a node has a "unites" relationship (node.unites is not None), it means the current node will run after all branches of the unites target are complete. This makes the current node a child of the unites target in the dependency graph. Therefore, when calculating in-degrees for root node detection, the current node's in-degree should be incremented, not the unites target's in-degree.
Applied to files:
state-manager/app/models/db/graph_template_model.py
🧬 Code graph analysis (2)
state-manager/tests/unit/with_database/test_graph_template.py (5)
state-manager/app/models/db/graph_template_model.py (5)
GraphTemplate(16-329)get_root_node(270-274)get_parents_by_identifier(287-292)get_node_by_identifier(279-285)get_path_by_identifier(294-299)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (2)
Unites(6-7)get_dependent_strings(61-67)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
state-manager/app/models/db/graph_template_model.py (3)
state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (1)
NodeTemplate(10-67)state-manager/app/models/dependent_string.py (3)
DependentString(9-62)create_dependent_string(24-42)get_identifier_field(60-62)
🔇 Additional comments (2)
state-manager/tests/unit/with_database/test_health_api.py (1)
5-5: Remove async guard—healthis defined as a synchronous function with no DI, so direct invocation in the test is correct.state-manager/app/models/db/graph_template_model.py (1)
87-93: Verifyparents_for_childrenlogic forunitesbranch includes the current node
In state-manager/app/models/db/graph_template_model.py (lines 87–93), whennode.unitesis visited,parents_for_childrenis set to the ancestors ofnode.unitesplusnode.unites.identifier, but omitsnode_identifier, dropping the current node from its descendants’ ancestry. If child nodes must still depend on this node, update to union innode_identifier; otherwise confirm this override is intentional.
- Implemented a new test to verify that a validation error during template creation raises an HTTPException with the correct status code and error message. - Updated existing tests to suppress type checking warnings for dependent strings in the GraphTemplate model.
- Introduced tests to validate error handling for empty node names, identifiers, and non-unique next node identifiers in the NodeTemplate model. - Added tests to ensure correct retrieval of dependent strings from node inputs, including validation for non-string inputs.
- Created tests to validate error handling for empty fields and non-unique identifiers in the NodeTemplate model. - Added tests for retrieving dependent strings from node inputs, ensuring correct handling of non-string input types.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state-manager/tests/unit/controller/test_upsert_graph_template.py (1)
108-108: Remove sleep(1); it’s unnecessary and slows tests
background_tasksis a MagicMock andverify_graphis patched—there’s nothing asynchronous to wait for here. Drop these sleeps to speed up the suite.- sleep(1) # wait for the background task to completeAlso applies to: 241-241, 285-285
♻️ Duplicate comments (6)
state-manager/tests/unit/with_database/test_graph_template.py (6)
25-26: Strengthen basics: assert root and lookup.Cover get_root_node and get_node_by_identifier to catch regressions.
- assert graph_template_model.name == "test_graph_template" + assert graph_template_model.name == "test_graph_template" + assert graph_template_model.get_root_node().identifier == "test_identifier" + assert graph_template_model.get_node_by_identifier("test_identifier") is not None
28-30: Typo: “liner” → “linear”.Rename the test and fix the docstring.
-@pytest.mark.asyncio -async def test_liner_graph_template(app_started): - """Test liner graph template creation""" +@pytest.mark.asyncio +async def test_linear_graph_template(app_started): + """Test linear graph template creation"""
613-615: Defensive check before calling get_dependent_strings().Same pattern: remove type ignore and assert existence.
- dependent_strings = graph_template_model.get_node_by_identifier("node2").get_dependent_strings() # type: ignore - assert len(dependent_strings) == 2 + node2 = graph_template_model.get_node_by_identifier("node2") + assert node2 is not None + dependent_strings = node2.get_dependent_strings() + assert len(dependent_strings) == 2
654-656: Repeat: remove type ignore and assert non-None before deref.- dependent_strings = graph_template_model.get_node_by_identifier("node2").get_dependent_strings() # type: ignore - assert len(dependent_strings) == 2 + node2 = graph_template_model.get_node_by_identifier("node2") + assert node2 is not None + dependent_strings = node2.get_dependent_strings() + assert len(dependent_strings) == 2
70-71: Avoid type: ignore; assert non-None before deref.Prevent potential None deref and remove the ignore.
- assert graph_template_model.get_node_by_identifier("node1").identifier == "node1" # type: ignore - assert graph_template_model.get_node_by_identifier("node2").identifier == "node2" # type: ignore + node1 = graph_template_model.get_node_by_identifier("node1") + node2 = graph_template_model.get_node_by_identifier("node2") + assert node1 is not None and node1.identifier == "node1" + assert node2 is not None and node2.identifier == "node2"
1-8: Optional: standardize async test runner.If your project prefers anyio, add a module-level marker and drop per-test @pytest.mark.asyncio to reduce noise. If pytest-asyncio is standard here, ignore.
import pytest - +pytestmark = pytest.mark.anyio
📜 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 (3)
state-manager/tests/unit/controller/test_upsert_graph_template.py(1 hunks)state-manager/tests/unit/with_database/test_graph_template copy.py(1 hunks)state-manager/tests/unit/with_database/test_graph_template.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
state-manager/tests/unit/controller/test_upsert_graph_template.py (3)
state-manager/tests/unit/controller/test_get_graph_template.py (3)
mock_namespace(19-20)mock_graph_name(23-24)mock_request_id(15-16)state-manager/tests/unit/controller/test_executed_state.py (3)
mock_namespace(19-20)mock_background_tasks(27-28)mock_request_id(15-16)state-manager/app/routes.py (1)
upsert_graph_template(155-164)
state-manager/tests/unit/with_database/test_graph_template copy.py (3)
state-manager/app/models/node_template_model.py (2)
Unites(6-7)get_dependent_strings(61-67)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
state-manager/tests/unit/with_database/test_graph_template.py (5)
state-manager/app/models/db/graph_template_model.py (5)
GraphTemplate(16-329)get_root_node(270-274)get_parents_by_identifier(287-292)get_node_by_identifier(279-285)get_path_by_identifier(294-299)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/models/node_template_model.py (2)
Unites(6-7)get_dependent_strings(61-67)state-manager/tests/unit/with_database/conftest.py (1)
app_started(22-25)state-manager/app/models/dependent_string.py (1)
get_identifier_field(60-62)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/with_database/test_graph_template copy.py
[error] 1-1: Command: 'ruff check $FILES_ARGS'. Error: E902 No such file or directory (os error 2) for 'copy.py'. This may be caused by a file path containing spaces (e.g., 'state-manager/tests/unit/with_database/test_graph_template copy.py') causing FILES_ARGS to split incorrectly.
[error] 1-1: Command: 'ruff check $FILES_ARGS'. Error: E902 No such file or directory (os error 2) for 'state-manager/tests/unit/with_database/test_graph_template'. This path may be invalid or missing an extension due to incorrect FILES_ARGS construction.
state-manager/tests/unit/with_database/test_graph_template.py
[error] 1-1: Command: 'ruff check $FILES_ARGS'. Error: E902 No such file or directory (os error 2) for 'state-manager/tests/unit/with_database/test_graph_template'. This path may be invalid or missing an extension due to incorrect FILES_ARGS construction.
1 participant
Pending items: