added bulk_create_next #236
Conversation
|
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. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds async GraphTemplate getters with polling and node-lookup caching; appends NEXT_CREATED_ERROR to StateStatusEnum; replaces per-state create_next_state with a batched create_next_states task that resolves dependencies, bulk-inserts next states, and updates statuses; updates executed_state controller and expands many unit tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Worker
participant Controller as executed_state
participant Scheduler as BackgroundTasks
participant Task as create_next_states
participant GT as GraphTemplate
participant Reg as RegisteredNode/Schema
participant DB as State Collection
Worker->>Controller: state execution completes
Controller->>Controller: collect next_state_ids
Controller->>Scheduler: add_task(create_next_states, next_state_ids, identifier, namespace, graph_name, parents)
Scheduler->>Task: invoke create_next_states(...)
Task->>GT: get_valid(namespace, graph_name)
alt template valid
GT-->>Task: GraphTemplate
else missing / timeout
GT-->>Task: raise ValueError
end
Task->>Reg: load RegisteredNode(s) & input schemas (cached)
loop per current state id
Task->>DB: load current state and parents
Task->>Task: parse dependents & resolve placeholders
alt dependencies satisfied
Task->>DB: prepare next-state documents (status=CREATED)
else unsatisfied / error
Task->>DB: mark affected states NEXT_CREATED_ERROR
Task-->>Task: record & raise
end
end
Task->>DB: insert_many(next-state docs)
Task->>DB: update original states -> SUCCESS
Task-->>Worker: completion / errors reported
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NiveditJain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the state management system by adding the capability for bulk creation of 'next' states. It leverages graph templates to define state transitions and intelligently resolves input dependencies from preceding states. The changes also include improvements to the GraphTemplate model for better validation and retrieval, and a new error status to handle failures during the state creation process, ensuring more robust and efficient workflow management.
Highlights
- Bulk State Creation: A new asynchronous task,
create_next_states, has been implemented to efficiently generate subsequent states in a graph-based workflow, allowing for bulk processing of state transitions. - Graph Template Utilities: The
GraphTemplatemodel now includes utility methods likeis_valid(),is_validating(),get(), andget_valid(). Theget_valid()method provides a robust way to retrieve a graph template, including polling until it reaches a valid state. - Dependency Resolution for State Inputs: New logic has been developed to parse and resolve input dependencies for newly created states. This system intelligently extracts required values from the outputs of current and parent states, ensuring correct data flow.
- Enhanced Error Handling: A new
NEXT_CREATED_ERRORstatus has been added toStateStatusEnumto specifically track and indicate failures that occur during the bulk creation of next states, improving error visibility and debugging.
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. ↩
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@cursoragent review and post your review comments here |
There was a problem hiding this comment.
Code Review
This pull request introduces a new task for bulk-creating the next states in a workflow, which is a significant and useful addition. The changes also include helpful additions to the GraphTemplate model for fetching and validating templates. The new create_next_states task is well-structured, utilizing caching and bulk database operations for efficiency. However, I've identified a critical logic error in the check_unites_satisfied function that must be addressed. Additionally, there are several medium-severity issues related to exception handling, an unused variable, and opportunities for code simplification that would improve the overall quality and robustness of the implementation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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/models/db/graph_template_model.py (1)
32-37: Docstring claims O(1) but implementation is O(n)Either adjust the docstring or add a lookup cache. Minimal fix below corrects the docstring.
- def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None: - """Get a node by its identifier using O(1) dictionary lookup.""" + def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None: + """Get a node by its identifier by iterating the nodes list.""" for node in self.nodes: if node.identifier == identifier: return node return None
📜 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(2 hunks)state-manager/app/models/state_status_enum.py(1 hunks)state-manager/app/tasks/create_next_states.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
state-manager/app/models/db/graph_template_model.py (4)
state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/tasks/verify_graph.py (1)
verify_graph(234-265)state-manager/app/controller/upsert_graph_template.py (1)
upsert_graph_template(12-62)
state-manager/app/tasks/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-109)get(92-96)get_valid(99-109)get_node_by_identifier(32-37)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
state-manager/app/models/state_status_enum.py (3)
state-manager/app/models/errored_models.py (1)
ErroredResponseModel(9-10)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)state-manager/app/models/executed_models.py (1)
ExecutedResponseModel(9-10)
🔇 Additional comments (2)
state-manager/app/models/state_status_enum.py (1)
14-16: NEXT_CREATED_ERROR integrated—please verify downstream handlingThe new
NEXT_CREATED_ERRORstatus is correctly set in
app/tasks/create_next_states.py:188However, no other code paths explicitly reference it. Before merging, ensure:
- API responses and controllers will propagate
NEXT_CREATED_ERRORunchanged- Any UI filters, dashboards or metrics consuming status values include the new enum
- Unit and integration tests are updated to cover error flows that surface
NEXT_CREATED_ERRORstate-manager/app/tasks/create_next_states.py (1)
184-191: Error propagation path looks goodOn failure, input states are marked NEXT_CREATED_ERROR with the error message, and the exception is re-raised. Matches the new enum semantics and allows upstream retries/alerts.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state-manager/app/models/db/graph_template_model.py (1)
33-37: Fix misleading docstring (claims O(1) but implementation is O(n))Either cache an index or update the docstring to reflect linear search.
- def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None: - """Get a node by its identifier using O(1) dictionary lookup.""" + def get_node_by_identifier(self, identifier: str) -> NodeTemplate | None: + """Get a node by its identifier by scanning the node list (O(n)).""" for node in self.nodes: if node.identifier == identifier: return node return None
♻️ Duplicate comments (11)
state-manager/app/models/db/graph_template_model.py (1)
98-109: Use monotonic clock and validate polling_interval/timeouttime.time() can jump; monotonic avoids clock changes. Also guard non-positive polling_interval/timeout. This also keeps retry cadence sane.
@staticmethod async def get_valid(namespace: str, graph_name: str, polling_interval: int = 1, timeout: int = 300) -> "GraphTemplate": - start_time = time.time() - while time.time() - start_time < timeout: + if polling_interval <= 0: + polling_interval = 1 + if timeout <= 0: + raise ValueError("timeout must be positive") + start_time = time.monotonic() + while time.monotonic() - start_time < timeout: graph_template = await GraphTemplate.get(namespace, graph_name) if graph_template.is_valid(): return graph_template if graph_template.is_validating(): await asyncio.sleep(polling_interval) else: raise ValueError(f"Graph template is not validating for namespace: {namespace} and graph name: {graph_name}") raise ValueError(f"Graph template is not valid for namespace: {namespace} and graph name: {graph_name} after {timeout} seconds")state-manager/app/tasks/create_next_states.py (10)
25-33: Iterate dependents deterministically and coerce to str; avoid range(len(...))range(len(...)) assumes contiguous integer keys; also, outputs may be non-str. Iterate sorted keys and coerce to str in concatenation.
def generate_string(self) -> str: base = self.head - for key in range(len(self.dependents)): - dependent = self.dependents[key] - if not dependent.value: - raise ValueError(f"Dependent value is not set for: {dependent}") - base += dependent.value + dependent.tail + for key in sorted(self.dependents): + dependent = self.dependents[key] + if dependent.value is None: + raise ValueError(f"Dependent value is not set for: {dependent}") + base += str(dependent.value) + dependent.tail return base
42-66: Critical: unites dependency check is invertedIf there are pending dependencies (non-SUCCESS), we must block creation. Current code blocks when pending_count == 0. Also simplify with early returns.
async def check_unites_satisfied(namespace: str, graph_name: str, node_template: NodeTemplate, parents: dict[str, PydanticObjectId]) -> bool: - satisfied = True - - if node_template.unites is None or len(node_template.unites) == 0: - return satisfied + if not node_template.unites: + return True for unit in node_template.unites: unites_id = parents.get(unit.identifier) if not unites_id: raise ValueError(f"Unit identifier not found in parents: {unit.identifier}") - else: - pending_count = await State.find( - State.identifier == unit.identifier, - State.namespace_name == namespace, - State.graph_name == graph_name, - NE(State.status, StateStatusEnum.SUCCESS), - { - f"parents.{unit.identifier}": unites_id - } - ).count() - if pending_count == 0: - satisfied = False - break - - return satisfied + pending_count = await State.find( + State.identifier == unit.identifier, + State.namespace_name == namespace, + State.graph_name == graph_name, + NE(State.status, StateStatusEnum.SUCCESS), + {f"parents.{unit.identifier}": unites_id}, + ).count() + if pending_count > 0: + return False + + return True
68-89: Fix E713 and placeholder parsing edge cases; avoid trimming content; use specific exceptions
- E713: use
"}}" not in splitinstead ofnot "}}" in split.str.split()always returns at least 1; uselen(splits) == 1for “no placeholders”.- Use ValueError, fix “placefolder” typo.
- Preserve head/tail whitespace; callers may rely on it.
def get_dependents(syntax_string: str) -> DependentString: splits = syntax_string.split("${{") - if len(splits) == 0: + if len(splits) == 1: return DependentString(head=syntax_string, dependents={}) - dependent_string = DependentString(head=splits[0].strip(), dependents={}) + dependent_string = DependentString(head=splits[0], dependents={}) order = 0 for split in splits[1:]: - if not "}}" in split: - raise Exception(f"Invalid syntax string placefolder {split} for: {syntax_string}"+"${{ not closed") + if "}}" not in split: + raise ValueError(f"Invalid syntax string placeholder '{split}' for: {syntax_string} — '${{' not closed") placeholder_content, tail = split.split("}}") parts = [p.strip() for p in placeholder_content.split(".")] if len(parts) != 3 or parts[1] != "outputs": - raise Exception(f"Invalid syntax string placefolder {placeholder_content} for: {syntax_string}") + raise ValueError(f"Invalid syntax string placeholder '{placeholder_content}' for: {syntax_string}") - dependent_string.dependents[order] = Dependent(identifier=parts[0], field=parts[2], tail=tail.strip()) + dependent_string.dependents[order] = Dependent(identifier=parts[0], field=parts[2], tail=tail) order += 1 return dependent_string
104-104: Remove unused variable to fix Ruff F841cached_states is never used.
- cached_states = {}
105-125: Fix typos: registerd → registered; keep cache naming consistentConsistent naming improves readability and avoids confusion.
- cached_registerd_nodes = {} + cached_registered_nodes = {} cached_input_models = {} @@ - async def get_registerd_node(node_template: NodeTemplate) -> RegisteredNode: - if node_template.node_name not in cached_registerd_nodes: - registerd_node = await RegisteredNode.find_one( + async def get_registered_node(node_template: NodeTemplate) -> RegisteredNode: + if node_template.node_name not in cached_registered_nodes: + registered_node = await RegisteredNode.find_one( RegisteredNode.name == node_template.node_name, RegisteredNode.namespace == node_template.namespace, ) - if not registerd_node: + if not registered_node: raise ValueError(f"Registered node not found for node name: {node_template.node_name} and namespace: {node_template.namespace}") - cached_registerd_nodes[node_template.node_name] = registerd_node - return cached_registerd_nodes[node_template.node_name] + cached_registered_nodes[node_template.node_name] = registered_node + return cached_registered_nodes[node_template.node_name] @@ - async def get_input_model(node_template: NodeTemplate) -> Type[BaseModel]: - if node_template.node_name not in cached_input_models: - cached_input_models[node_template.node_name] = create_model((await get_registerd_node(node_template)).inputs_schema) - return cached_input_models[node_template.node_name] + async def get_input_model(node_template: NodeTemplate) -> Type[BaseModel]: + if node_template.node_name not in cached_input_models: + cached_input_models[node_template.node_name] = create_model( + (await get_registered_node(node_template)).inputs_schema + ) + return cached_input_models[node_template.node_name]
126-133: Guard empty parents_ids to avoid invalid IN queryAvoid making an IN [] query; some ORMs/DBs treat it poorly.
- parent_states = await State.find( - In(State.id, parents_ids.values()) - ).to_list() + if parents_ids: + parent_states = await State.find(In(State.id, list(parents_ids.values()))).to_list() + else: + parent_states = []
150-159: Validate presence of template inputs and iterate dependents safely
- Validate that the input field exists in the template (clear error if missing).
- Iterate over dependents deterministically; avoid assuming contiguous integer keys.
- Optional: use .get() to surface missing outputs via generate_string’s check.
- for field_name, _ in next_state_input_model.model_fields.items(): - dependency_string = get_dependents(next_state_node_template.inputs[field_name]) + for field_name, _ in next_state_input_model.model_fields.items(): + if field_name not in next_state_node_template.inputs: + raise ValueError( + f"Missing input '{field_name}' in template for node '{next_state_node_template.identifier}'" + ) + dependency_string = get_dependents(next_state_node_template.inputs[field_name]) - for key in range(len(dependency_string.dependents)): - if dependency_string.dependents[key].identifier == identifier: - dependency_string.dependents[key].value = current_state.outputs[dependency_string.dependents[key].field] - else: - dependency_string.dependents[key].value = parents[dependency_string.dependents[key].identifier].outputs[dependency_string.dependents[key].field] + for key in sorted(dependency_string.dependents): + dep = dependency_string.dependents[key] + if dep.identifier == identifier: + dep.value = current_state.outputs.get(dep.field) + else: + parent_state = parents.get(dep.identifier) + if not parent_state: + raise KeyError( + f"Parent identifier '{dep.identifier}' not found for node '{next_state_node_template.identifier}'" + ) + dep.value = parent_state.outputs.get(dep.field)
181-183: Avoid insert_many([]) when no new states were createdBeanie/Motor can error on empty inserts; short-circuit safely.
- await State.insert_many(new_states) - await mark_success_states(state_ids) + if new_states: + await State.insert_many(new_states) + await mark_success_states(state_ids)
21-24: Consider switching dependents to a list instead of dict[int, Dependent]A list preserves order without relying on integer keys and avoids sorted(...) overhead in hot paths; update get_dependents/generate_string accordingly.
147-159: Pre-validate placeholders and referenced outputs to fail fast with clear errorsBefore populating dependency_string values, ensure:
- Each dependent identifier is either the current node or in parents.
- Each referenced output field exists on the resolved state.
This reduces late failures and improves error messaging.I can add a small validator helper and unit tests for get_dependents and input resolution. Want me to draft that?
📜 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(2 hunks)state-manager/app/tasks/create_next_states.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
state-manager/app/models/db/graph_template_model.py (5)
state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/tests/unit/controller/test_get_graph_template.py (2)
test_get_graph_template_with_pending_validation(148-185)test_get_graph_template_with_validation_errors(108-145)state-manager/app/tasks/verify_graph.py (1)
verify_graph(234-265)state-manager/app/controller/upsert_graph_template.py (1)
upsert_graph_template(12-62)
state-manager/app/tasks/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-109)get(92-96)get_valid(99-109)get_node_by_identifier(32-37)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/app/tasks/create_next_states.py
[error] 77-77: E713: Test for membership should be 'not in'. Command: 'ruff check state-manager/app/models/db/graph_template_model.py state-manager/app/models/state_status_enum.py state-manager/app/tasks/create_next_states.py'
[error] 104-104: F841: Local variable 'cached_states' is assigned to but never used.
🔇 Additional comments (3)
state-manager/app/models/db/graph_template_model.py (3)
85-87: Helper reads wellis_valid is concise and correct.
88-90: Readable membership checkUsing membership for ONGOING/PENDING is clear. No further changes needed.
91-97: Good: explicit not-found handling in get()Raising ValueError on not found is appropriate for callers.
|
@cursoragent review |
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the creation of subsequent states to be a bulk operation, which is a great improvement for performance. A new background task create_next_states is introduced, along with helper methods on the GraphTemplate model to poll for a valid template. The changes look solid. I've added a few suggestions to improve code clarity, efficiency, and error handling:
- Refactoring a function in
create_next_states.pyto be more efficient by merging loops. - Improving an error message in
graph_template_model.pyto be more specific. - Fixing a typo in an error message.
- Adding a docstring to the new complex
create_next_statesfunction for better maintainability.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance improvement by batching the creation of next states. Instead of creating a separate background task for each next state, it now collects all relevant state IDs and triggers a single create_next_states task. This new task is well-structured with a try-except block for error handling and caching for database objects. The addition of GraphTemplate.get_valid with polling logic is also a great improvement for ensuring graph templates are ready before use.
My review focuses on further performance optimizations within the new create_next_states task. I've identified a couple of areas where redundant computations and function calls can be avoided, especially when processing a large number of states in a batch.
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/models/db/graph_template_model.py (1)
33-33: Docstring claims O(1) but implementation is O(n)Either adjust the docstring or introduce an index/cache for identifiers. Minimal fix below updates the docstring.
- """Get a node by its identifier using O(1) dictionary lookup.""" + """Get a node by its identifier."""state-manager/app/controller/executed_state.py (1)
2-2: Fix Ruff F401: remove unused In importThis import is unused and breaks the linter.
-from beanie.operators import In
♻️ Duplicate comments (2)
state-manager/app/tasks/create_next_states.py (2)
218-219: Avoid insert_many([]) on empty new_statesBeanie/Motor can error on empty inserts. Short-circuit safely.
- await State.insert_many(new_states) - await mark_success_states(state_ids) + if new_states: + await State.insert_many(new_states) + await mark_success_states(state_ids)
160-162: Guard against potential driver/ORM quirks with In(values())Some drivers expect a concrete list. Casting to list avoids surprises.
- parent_states = await State.find( - In(State.id, parents_ids.values()) - ).to_list() + parent_states = await State.find( + In(State.id, list(parents_ids.values())) + ).to_list()
📜 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/executed_state.py(3 hunks)state-manager/app/models/db/graph_template_model.py(2 hunks)state-manager/app/tasks/create_next_states.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
state-manager/app/models/db/graph_template_model.py (3)
state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/tasks/verify_graph.py (1)
verify_graph(234-265)
state-manager/app/tasks/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-119)get(92-96)get_valid(99-119)get_node_by_identifier(32-37)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
state-manager/app/controller/executed_state.py (2)
state-manager/app/tasks/create_next_states.py (1)
create_next_states(115-228)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/app/controller/executed_state.py
[error] 2-2: Ruff lint failed during 'ruff check' (exit code 1): F401 [*] beanie.operators.In imported but unused
🪛 GitHub Actions: State Manager Unit Tests
state-manager/app/controller/executed_state.py
[error] 1-1: pytest failed with AttributeError: module 'app.controller.executed_state' does not have the attribute 'create_next_state' during patching in tests/unit/controller/test_executed_state.py. Command 'uv run pytest tests/unit/ --cov=app --cov-report=xml --cov-report=term-missing -v --junitxml=pytest-report.xml' exited with code 1.
🔇 Additional comments (10)
state-manager/app/models/db/graph_template_model.py (6)
2-3: Good addition: monotonic timing and asyncio supportThe imports align with the new async polling and timeout logic.
83-83: No-op change around get_secretNothing to flag here.
85-87: is_valid helper is clear and correctStraightforward and readable.
88-90: is_validating helper reads wellMembership check keeps this easy to extend.
91-97: Async GraphTemplate.get looks goodClear error when not found; using model fields directly is fine.
98-119: Robust get_valid: input validation + monotonic pollingNice use of time.monotonic() and guarding invalid intervals. Polling loop logic is sound.
state-manager/app/tasks/create_next_states.py (2)
42-62: Unites check: logic is now correct and efficientEarly return on missing unites, clear False when pending_count > 0. Good.
184-197: Type semantics of placeholder-only fieldsIf an input field is entirely a single placeholder (no head/tail), you likely want to pass the raw typed value (e.g., int, bool) instead of a stringified value. Current logic always emits strings.
Would you like me to add detection for the “single placeholder only” case and keep the raw type? This improves schema compliance when inputs expect non-strings.
state-manager/app/controller/executed_state.py (2)
56-59: Aggregation looks goodAppending inserted_ids to next_state_ids correctly feeds the bulk task.
1-66: Action required: Migrate legacycreate_next_statereferences and clean up unused importsPlease update the codebase to fully adopt the new bulk API (
create_next_states) and eliminate leftover references to the old single‐state helper. Specifically:
Remove the unused import in the controller:
File:state-manager/app/controller/executed_state.py
• Deletefrom beanie.operators import In(Ruff flagged it as F401).Update controller tests to patch the new bulk helper, not the old one:
Files understate-manager/tests/unit/controller/test_executed_state.py
• Change all@patch('app.controller.executed_state.create_next_state')→@patch('app.controller.executed_state.create_next_states').Decide on the legacy helper in
app/tasks/create_next_state.pyand its tests:
• If you’re deprecating it, removestate-manager/app/tasks/create_next_state.pyand its unit tests (state-manager/tests/unit/test_create_next_state.py).
• Otherwise, ensure it remains documented and supported alongside the new bulk API.After these updates, rerun Ruff (
ruff check) and your test suite to confirm there are no remaining references or lint failures.
Co-authored-by: nivedit <nivedit@aikin.club>
…fdf3' into 234-support-bulk-operation-in-createnextstate-taking-too-long
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ates.py for improved dependency validation. Adjusted checks for dependent values and ensured identifiers are validated against parents.
…o-long' of https://github.com/NiveditJain/exospherehost into 234-support-bulk-operation-in-createnextstate-taking-too-long
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
state-manager/app/tasks/create_next_states.py (7)
86-113: Consolidate validation loops to reduce redundant workYou iterate model_fields three times and call get_dependents twice per field. Combine into one pass.
Apply this diff:
-def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: - """Validate that all dependencies exist before processing them.""" - # 1) Confirm each model field is present 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}'") - - # 2) For each placeholder, verify the identifier is either current or present in parents - for field_name in next_state_input_model.model_fields.keys(): - dependency_string = get_dependents(next_state_node_template.inputs[field_name]) - - for dependent in dependency_string.dependents.values(): - if dependent.identifier != identifier and dependent.identifier not in parents: - raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") - - # 3) For each dependent, verify the target output field exists on the resolved state - for field_name in next_state_input_model.model_fields.keys(): - dependency_string = get_dependents(next_state_node_template.inputs[field_name]) - - for dependent in dependency_string.dependents.values(): - if dependent.identifier == identifier: - # This will be resolved to current_state later, skip validation here - continue - else: - parent_state = parents[dependent.identifier] - if dependent.field not in parent_state.outputs: - raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'") +def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: + """Validate that all dependencies exist before processing them.""" + for field_name in next_state_input_model.model_fields.keys(): + # 1) Confirm field exists in template inputs + 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 = get_dependents(next_state_node_template.inputs[field_name]) + for dependent in dependency_string.dependents.values(): + is_current = dependent.identifier == identifier + # 2) Placeholder identifier must be current or in parents + if not is_current and dependent.identifier not in parents: + raise KeyError( + f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'" + ) + # 3) Referenced output must exist on the resolved state (for parent deps) + if not is_current: + parent_state = parents[dependent.identifier] + if dependent.field not in parent_state.outputs: + raise AttributeError( + f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'" + )
168-176: Optimize: avoid recomputing per current_statecheck_unites_satisfied and get_input_model/validate_dependencies depend on the next_state_identifier and parents, not on current_state. Flip the loops to iterate next_state_identifiers outermost, compute once, then iterate current_states.
Would you like a concrete diff for the loop restructuring?
218-219: Avoid insert_many([]) when no new states were createdBeanie/Motor may error on empty inserts.
Apply this diff:
- await State.insert_many(new_states) - await mark_success_states(state_ids) + if new_states: + await State.insert_many(new_states) + await mark_success_states(state_ids)
21-24: Consider list[Dependent] instead of dict[int, Dependent] for order and simplicityA list preserves insertion order naturally and avoids assumptions about contiguous numeric keys.
If you’d like, I can provide a full diff converting dependents to list[Dependent] and updating usage sites.
115-216: Idempotency guard to prevent duplicate CREATED states on retriesRepeated scheduling or partial failures can create duplicate next states for the same (run_id, identifier, parents). Add a deterministic “next-state key” and enforce uniqueness or check-before-insert.
I can propose:
- A canonical parents signature (sorted items → hash).
- A unique index on (run_id, graph_name, identifier, parents_signature).
- An existence check before appending to new_states.
Want a concrete diff?
21-33: Fix falsy-value handling and string concatenation in generate_stringEmpty strings or 0 are valid outputs but currently treated as missing. Also, direct concatenation fails for non-strings.
Apply this diff:
def generate_string(self) -> str: base = self.head for key in sorted(self.dependents.keys()): dependent = self.dependents[key] - if not dependent.value: - raise ValueError(f"Dependent value is not set for: {dependent}") - base += dependent.value + dependent.tail + if dependent.value is None: + raise ValueError(f"Dependent value is not set for: {dependent}") + base += str(dependent.value) + dependent.tail return base
64-85: Tighten dependent parser and fix typo in error message
- Use split('}}', 1) to avoid over-splitting when tail contains '}}'.
- Correct “placefolder” → “placeholder”.
- Quote placeholder in error for clarity.
Apply this diff:
for split in splits[1:]: - if "}}" not in split: - raise ValueError(f"Invalid syntax string placeholder {split} for: {syntax_string} '${{' not closed") - placeholder_content, tail = split.split("}}") + if "}}" not in split: + raise ValueError( + f"Invalid syntax string placeholder '{split}' for: {syntax_string} '${{' not closed" + ) + placeholder_content, tail = split.split("}}", 1) @@ - if len(parts) != 3 or parts[1] != "outputs": - raise ValueError(f"Invalid syntax string placefolder {placeholder_content} for: {syntax_string}") + if len(parts) != 3 or parts[1] != "outputs": + raise ValueError( + f"Invalid syntax string placeholder '{placeholder_content}' for: {syntax_string}" + )
📜 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(2 hunks)state-manager/app/tasks/create_next_states.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
state-manager/app/models/db/graph_template_model.py (2)
state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)
state-manager/app/tasks/create_next_states.py (5)
state-manager/app/models/db/graph_template_model.py (3)
GraphTemplate(15-119)get(92-96)get_valid(99-119)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)
🔇 Additional comments (4)
state-manager/app/models/db/graph_template_model.py (3)
2-3: Good choice: monotonic clock + asyncio for pollingUsing time.monotonic() and asyncio fits the async polling use case and avoids wall-clock jumps.
88-90: Readable is_validating checkMembership check over explicit ORs improves clarity and extensibility.
91-97: Static get: clear error on missing templateLookup + explicit ValueError on not found is appropriate for callers.
state-manager/app/tasks/create_next_states.py (1)
157-163: Good: guarded parent lookup when parents_ids is emptyAvoids issuing an IN [] query and keeps behavior explicit.
…ve unused List imports from test files
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…o-long' of https://github.com/NiveditJain/exospherehost into 234-support-bulk-operation-in-createnextstate-taking-too-long
There was a problem hiding this comment.
Actionable comments posted: 24
♻️ Duplicate comments (6)
state-manager/app/tasks/create_next_states.py (6)
11-12: Fix type/concat bug in dependent rendering (support non-string outputs, avoid falsy-misinterpreted values)Concatenating non-string outputs raises TypeError and empty strings/0 are wrongly treated as missing. Use Any, check for None, and cast to str when rendering.
-from typing import Type +from typing import Type, Any @@ class Dependent(BaseModel): identifier: str field: str tail: str - value: str | None = None + value: Any | None = None @@ def generate_string(self) -> str: base = self.head for key in sorted(self.dependents.keys()): dependent = self.dependents[key] - if not dependent.value: + if dependent.value is None: raise ValueError(f"Dependent value is not set for: {dependent}") - base += dependent.value + dependent.tail + base += str(dependent.value) + dependent.tail return baseAlso applies to: 15-20, 25-32
72-79: Tighten placeholder parsing and fix typos
- Use maxsplit=1 to avoid over-splitting on '}}'
- Correct “placefolder” → “placeholder”
- for split in splits[1:]: - if "}}" not in split: - raise ValueError(f"Invalid syntax string placefolder {split} for: {syntax_string} '${{' not closed") - placeholder_content, tail = split.split("}}") + for split in splits[1:]: + if "}}" not in split: + raise ValueError(f"Invalid syntax string placeholder '{split}' for: {syntax_string} — '${{' not closed") + placeholder_content, tail = split.split("}}", 1) @@ - if len(parts) != 3 or parts[1] != "outputs": - raise ValueError(f"Invalid syntax string placefolder {placeholder_content} for: {syntax_string}") + if len(parts) != 3 or parts[1] != "outputs": + raise ValueError(f"Invalid syntax string placeholder '{placeholder_content}' for: {syntax_string}")
46-62: Simplify control flow in unites check (remove unnecessary else)No functional change; improves readability.
for unit in node_template.unites: unites_id = parents.get(unit.identifier) if not unites_id: raise ValueError(f"Unit identifier not found in parents: {unit.identifier}") - else: - pending_count = await State.find( - State.identifier == unit.identifier, - State.namespace_name == namespace, - State.graph_name == graph_name, - NE(State.status, StateStatusEnum.SUCCESS), - { - f"parents.{unit.identifier}": unites_id - } - ).count() - if pending_count > 0: - return False + pending_count = await State.find( + State.identifier == unit.identifier, + State.namespace_name == namespace, + State.graph_name == graph_name, + NE(State.status, StateStatusEnum.SUCCESS), + {f"parents.{unit.identifier}": unites_id} + ).count() + if pending_count > 0: + return False
86-113: Combine triple-pass validation into a single pass to reduce overheadCurrent implementation loops three times over the same fields and re-parses dependents. Merge into one pass.
-def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: - """Validate that all dependencies exist before processing them.""" - # 1) Confirm each model field is present 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}'") - - # 2) For each placeholder, verify the identifier is either current or present in parents - for field_name in next_state_input_model.model_fields.keys(): - dependency_string = get_dependents(next_state_node_template.inputs[field_name]) - - for dependent in dependency_string.dependents.values(): - if dependent.identifier != identifier and dependent.identifier not in parents: - raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") - - # 3) For each dependent, verify the target output field exists on the resolved state - for field_name in next_state_input_model.model_fields.keys(): - dependency_string = get_dependents(next_state_node_template.inputs[field_name]) - - for dependent in dependency_string.dependents.values(): - if dependent.identifier == identifier: - # This will be resolved to current_state later, skip validation here - continue - else: - parent_state = parents[dependent.identifier] - if dependent.field not in parent_state.outputs: - raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'") +def validate_dependencies(next_state_node_template: NodeTemplate, next_state_input_model: Type[BaseModel], identifier: str, parents: dict[str, State]) -> None: + """Validate that all dependencies exist before processing them.""" + 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 = get_dependents(next_state_node_template.inputs[field_name]) + for dependent in dependency_string.dependents.values(): + is_current = dependent.identifier == identifier + if not is_current and dependent.identifier not in parents: + raise KeyError(f"Identifier '{dependent.identifier}' not found in parents for template '{next_state_node_template.identifier}'") + if not is_current: + parent_state = parents[dependent.identifier] + if dependent.field not in parent_state.outputs: + raise AttributeError(f"Output field '{dependent.field}' not found on state '{dependent.identifier}' for template '{next_state_node_template.identifier}'")
218-219: Avoid insert_many([]) when no next states were createdinsert_many([]) can error on some ODMs.
- await State.insert_many(new_states) + if new_states: + await State.insert_many(new_states) await mark_success_states(state_ids)
221-228: Log full traceback and re-raise preserving original stackUse logger.exception for diagnostics and bare raise.
- except Exception as e: - await State.find( + except Exception as e: + logger.exception( + "Failed to create next states (identifier=%s, namespace=%s, graph=%s)", + identifier, namespace, graph_name, + ) + await State.find( In(State.id, state_ids) ).set({ "status": StateStatusEnum.NEXT_CREATED_ERROR, "error": str(e) }) # type: ignore - raise e + raise
📜 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 (10)
state-manager/app/tasks/create_next_states.py(1 hunks)state-manager/tests/unit/middlewares/__init__.py(1 hunks)state-manager/tests/unit/middlewares/test_request_id_middleware.py(1 hunks)state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py(1 hunks)state-manager/tests/unit/singletons/__init__.py(1 hunks)state-manager/tests/unit/singletons/test_singleton_decorator.py(1 hunks)state-manager/tests/unit/test_main.py(1 hunks)state-manager/tests/unit/utils/__init__.py(1 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)state-manager/tests/unit/utils/test_encrypter.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
state-manager/tests/unit/test_main.py (5)
state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/db/namespace.py (1)
Namespace(6-8)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-119)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)api-server/app/main.py (1)
lifespan(35-55)
state-manager/tests/unit/singletons/test_singleton_decorator.py (4)
api-server/tests/unit/auth/singletons/test_redis_manager.py (1)
setUp(23-55)api-server/tests/integration/auth/singletons/test_redis_manager_integration.py (1)
redis_manager_fixture(35-89)api-server/app/singletons/SingletonDecorator.py (1)
singleton(4-12)state-manager/app/singletons/SingletonDecorator.py (1)
singleton(4-12)
state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py (2)
state-manager/app/middlewares/unhandled_exceptions_middleware.py (1)
UnhandledExceptionsMiddleware(10-30)api-server/tests/test_unhandled_exceptions_middleware.py (5)
test_runtime_error_returns_expected_json(36-39)fail_key_error(15-16)client(22-29)test_key_error_returns_expected_json(46-49)test_value_error_returns_expected_json(41-44)
state-manager/tests/unit/utils/test_check_secret.py (1)
state-manager/app/utils/check_secret.py (1)
check_api_key(15-19)
state-manager/tests/unit/middlewares/test_request_id_middleware.py (1)
state-manager/app/middlewares/request_id_middleware.py (2)
RequestIdMiddleware(10-54)dispatch(11-54)
state-manager/app/tasks/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-119)get(92-96)get_valid(99-119)get_node_by_identifier(32-37)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
state-manager/tests/unit/utils/test_encrypter.py (1)
state-manager/app/utils/encrypter.py (6)
Encrypter(5-32)get_encrypter(36-49)generate_key(8-9)encrypt(23-26)decrypt(28-32)__init__(11-21)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/tests/unit/test_main.py
[error] 100-100: AssertionError: UnhandledExceptionsMiddleware index 0 is not greater than RequestIdMiddleware index 1 in TestMainApp.test_middleware_order. Command: uv run pytest tests/unit/ --cov=app --cov-report=xml --cov-report=term-missing -v --junitxml=pytest-report.xml.
state-manager/tests/unit/singletons/test_singleton_decorator.py
[error] 185-185: TypeError: function() argument 'code' must be code, not str during test_singleton_decorator_with_inheritance; applies @singleton to a subclass.
state-manager/tests/unit/utils/test_check_secret.py
[error] 181-181: AttributeError: 'APIKey' object has no attribute 'alias' while asserting api_key_header.model.alias; test expected alias to be 'x-api-key'. Command: uv run pytest tests/unit/ --cov=app --cov-report=xml --cov-report=term-missing -v --junitxml=pytest-report.xml.
state-manager/tests/unit/middlewares/test_request_id_middleware.py
[error] 193-193: TestRequestIdMiddleware.test_dispatch_response_time_calculation failed: expected response_time 100.0 ms but got 100.00000000002274 ms (floating-point precision issue). Command: uv run pytest tests/unit/ --cov=app --cov-report=xml --cov-report=term-missing -v --junitxml=pytest-report.xml.
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/singletons/test_singleton_decorator.py
[error] 2-2: F401: 'MagicMock' imported but unused.
state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py
[error] 2-2: F401: 'traceback' imported but unused.
state-manager/tests/unit/utils/test_check_secret.py
[error] 3-3: F401: 'AsyncMock' imported but unused.
[error] 8-8: F401: 'check_api_key' imported but unused.
[error] 8-8: F401: 'API_KEY' imported but unused.
[error] 19-19: F711: Redefinition of unused check_api_key from line 8
[error] 25-25: F811: Redefinition of unused check_api_key from line 19
[error] 38-38: F811: Redefinition of unused check_api_key from line 8
[error] 53-53: F811: Redefinition of unused check_api_key from line 8
[error] 68-68: F811: Redefinition of unused check_api_key from line 8
[error] 83-83: F811: Redefinition of unused check_api_key from line 8
[error] 98-98: F811: Redefinition of unused check_api_key from line 8
[error] 113-113: F811: Redefinition of unused check_api_key from line 8
[error] 125-125: F811: Redefinition of unused check_api_key from line 8
[error] 137-137: F811: Redefinition of unused check_api_key from line 8
[error] 151-151: F811: Redefinition of unused check_api_key from line 8
[error] 164-164: F811: Redefinition of unused check_api_key from line 8
[error] 214-214: F811: Redefinition of unused check_api_key from line 8
[error] 227-227: F811: Redefinition of unused check_api_key from line 8
state-manager/tests/unit/middlewares/test_request_id_middleware.py
[error] 2-2: F401: 'time' imported but unused.
[error] 116-116: F841: Local variable 'mock_logger' assigned to but never used.
[error] 117-117: F841: Local variable 'result' assigned to but never used.
🔇 Additional comments (4)
state-manager/tests/unit/utils/test_encrypter.py (3)
13-21: Good isolation of the singleton between testsResetting the module-scoped singleton in setup/teardown prevents inter-test coupling. This keeps the tests deterministic.
23-33: Key generation tests are correct and meaningfulValidating base64 decodability and raw length (32 bytes) directly targets correctness of the 256-bit key generation.
229-244: Solid verification of singleton semanticsMocking Encrypter and asserting single instantiation + identity across calls accurately tests the caching behavior of get_encrypter().
state-manager/tests/unit/singletons/__init__.py (1)
1-1: Package marker looks goodCreating the package marker file is appropriate to ensure tests under this directory import correctly.
state-manager/tests/unit/middlewares/test_request_id_middleware.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (22)
state-manager/tests/unit/singletons/test_singleton_decorator.py (5)
2-2: Remove unused MagicMock import (ruff F401).This import is not used anywhere in the test module and breaks the pipeline.
-from unittest.mock import MagicMock
145-148: Avoid locking the test comment to a specific decorator implementation.Reword to assert behavior, not that the decorator “returns a function.”
- # The returned function should still reference the original class - # Note: The decorator returns a function, not a class, but the instance - # should still be of the original class type + # Regardless of decorator implementation, the instance should be of the original class type
150-175: Make the thread-safety test actually concurrent.This is single-threaded; use threads + barrier to exercise races.
- def test_singleton_decorator_thread_safety_simulation(self): - """Test singleton decorator behavior under simulated concurrent access""" + def test_singleton_decorator_thread_safety(self): + """Test singleton decorator behavior under concurrent access""" @@ - # Simulate multiple "concurrent" calls - instances = [] - for _ in range(10): - instances.append(CountedClass()) + # Create multiple concurrent calls + import threading + instances = [] + barrier = threading.Barrier(10) + + def create(): + barrier.wait() + instances.append(CountedClass()) + + threads = [threading.Thread(target=create) for _ in range(10)] + for t in threads: + t.start() + for t in threads: + t.join()
225-245: Strengthen: verify failures aren’t cached and a later success works.Add an assertion that a subsequent successful construction returns a valid instance after failures.
# Second call with failure (should try to create again since first failed) with pytest.raises(ValueError, match="Constructor failed"): FailingClass() - # The singleton pattern should handle constructor failures gracefully - # After failure, the class should still not be in instances dict + # After failure, a successful call should still construct a fresh instance + success_instance = FailingClass(should_fail=False) + assert getattr(success_instance, "success", False) is True
4-4: Consider adding a supported reset API for tests.Instead of poking into decorator internals, expose and import a reset_singleton helper alongside the decorator and use it in tests/fixtures.
I can draft the reset_singleton helper and update tests to use it.
state-manager/tests/unit/middlewares/test_request_id_middleware.py (3)
2-2: Remove unused import (ruff F401).The time module is only accessed via patch('time.time'); the direct import is unused.
-import time
115-118: Eliminate unused variables (ruff F841).Avoid assigning to mock_logger and result when they aren’t used.
- with patch('time.time', side_effect=[3000.0, 3001.0]): # 1000ms duration - with patch('app.middlewares.request_id_middleware.logger') as mock_logger: - result = await self.middleware.dispatch(mock_request, mock_call_next) + with patch('time.time', side_effect=[3000.0, 3001.0]): # 1000ms duration + with patch('app.middlewares.request_id_middleware.logger'): + await self.middleware.dispatch(mock_request, mock_call_next)
191-194: Prefer pytest.approx for float comparisons.Reduces flakiness and is clearer than manual tolerances.
- second_call_args = mock_logger.info.call_args_list[1] - assert abs(second_call_args[1]["response_time"] - expected_ms) < 0.1 + second_call_args = mock_logger.info.call_args_list[1] + assert second_call_args[1]["response_time"] == pytest.approx(expected_ms, rel=1e-9, abs=1e-6)state-manager/tests/unit/utils/test_check_secret.py (10)
3-3: Remove unused AsyncMock import (ruff F401).-from unittest.mock import patch, AsyncMock +from unittest.mock import patch
8-8: Trim unused top-level imports (ruff F401) to avoid later F811 redefinitions.Do not import check_api_key and API_KEY at module scope.
-from app.utils.check_secret import check_api_key, api_key_header, API_KEY, API_KEY_NAME +from app.utils.check_secret import api_key_header, API_KEY_NAME
18-27: Fix duplicate import within the same test (ruff F811) and simplify reload pattern.Import the module, reload, then call the function from the module.
- # Import here to get the updated environment variable - from app.utils.check_secret import check_api_key - - # Reload the module to pick up the new environment variable - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - result = await check_api_key('test-secret-key') + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) + + result = await check_secret.check_api_key('test-secret-key')
30-39: Reduce duplication: add a tiny helper/fixture for module reloads.A helper that reloads and returns the module removes repeated import/reload boilerplate and avoids redefinition warnings.
Example helper (outside these ranges):
import importlib import app.utils.check_secret as check_secret def reload_check_secret(): return importlib.reload(check_secret)Usage:
mod = reload_check_secret() result = await mod.check_api_key('value')Also applies to: 46-69, 76-105, 106-117, 118-129, 130-143, 144-156, 207-217, 219-227
110-117: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - result = await check_api_key('special-chars-!@#$%^&*()') + result = await check_secret.check_api_key('special-chars-!@#$%^&*()')
80-90: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('CASE-SENSITIVE-KEY') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('CASE-SENSITIVE-KEY')
65-75: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('')
147-155: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - result = await check_api_key(long_key) + result = await check_secret.check_api_key(long_key)
95-105: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key(' whitespace-key ') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key(' whitespace-key ')
135-142: Same reimport pattern issue inside this test.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - result = await check_api_key('') + result = await check_secret.check_api_key('')state-manager/tests/unit/test_main.py (1)
65-65: Make Content-Type assertion robust to charset suffix.Servers may append a charset; assert prefix instead of exact equality.
- assert response.headers["content-type"] == "application/json" + assert response.headers["content-type"].startswith("application/json")state-manager/tests/unit/tasks/test_create_next_states.py (3)
64-69: Broaden invalid placeholder coverage.Add cases for unterminated placeholder, wrong delimiter order, internal whitespace, and adjacent placeholders.
Would you like me to push a parametrized test covering these malformed inputs?
144-163: Also add a no-op path test for missing IDs in mark_success_states.Ensure it handles missing states gracefully (should already, via bulk set).
I can add a variant that simulates an empty result set and asserts the function completes without errors.
1-163: Create happy-path and failure-path tests for create_next_states orchestration.You have great helper coverage; add end-to-end checks for:
- inputs rendering and bulk insert on success,
- dependency validation failures setting NEXT_CREATED_ERROR,
- queueing behavior where applicable.
I can draft parametrized tests using your DummyState/DummyQuery scaffolding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
state-manager/tests/unit/middlewares/test_request_id_middleware.py(1 hunks)state-manager/tests/unit/singletons/test_singleton_decorator.py(1 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(1 hunks)state-manager/tests/unit/test_main.py(1 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
state-manager/tests/unit/test_main.py (5)
state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/db/namespace.py (1)
Namespace(6-8)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-119)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)api-server/app/main.py (2)
lifespan(35-55)health(82-83)
state-manager/tests/unit/singletons/test_singleton_decorator.py (1)
state-manager/app/singletons/SingletonDecorator.py (2)
singleton(4-12)get_instance(7-10)
state-manager/tests/unit/utils/test_check_secret.py (1)
state-manager/app/utils/check_secret.py (1)
check_api_key(15-19)
state-manager/tests/unit/middlewares/test_request_id_middleware.py (1)
state-manager/app/middlewares/request_id_middleware.py (2)
RequestIdMiddleware(10-54)dispatch(11-54)
state-manager/tests/unit/tasks/test_create_next_states.py (1)
state-manager/app/tasks/create_next_states.py (5)
create_next_states(115-228)get_dependents(64-84)validate_dependencies(86-112)check_unites_satisfied(42-62)mark_success_states(34-39)
🪛 GitHub Actions: Ruff check on changed files only
state-manager/tests/unit/singletons/test_singleton_decorator.py
[error] 2-2: F401 [*] unittest.mock.MagicMock imported but unused
state-manager/tests/unit/utils/test_check_secret.py
[error] 3-3: F401 [*] unittest.mock.AsyncMock imported but unused
[error] 8-8: F401 [*] app.utils.check_secret.check_api_key imported but unused
[error] 8-8: F401 [*] app.utils.check_secret.API_KEY imported but unused
[error] 19-19: F811 [*] Redefinition of unused check_api_key from line 8
[error] 25-25: F811 [*] Redefinition of unused check_api_key from line 8
[error] 38-38: F811 [*] Redefinition of unused check_api_key from line 8
[error] 53-53: F811 [*] Redefinition of unused check_api_key from line 8
[error] 68-68: F811 [*] Redefinition of unused check_api_key from line 8
[error] 83-83: F811 [*] Redefinition of unused check_api_key from line 8
[error] 98-98: F811 [*] Redefinition of unused check_api_key from line 8
[error] 113-113: F811 [*] Redefinition of unused check_api_key from line 8
[error] 125-125: F811 [*] Redefinition of unused check_api_key from line 8
[error] 137-137: F811 [*] Redefinition of unused check_api_key from line 8
[error] 150-150: F811 [*] Redefinition of unused check_api_key from line 8
[error] 190-190: F811 [*] Redefinition of unused check_api_key from line 8
[error] 203-203: F811 [*] Redefinition of unused check_api_key from line 8
state-manager/tests/unit/middlewares/test_request_id_middleware.py
[error] 2-2: F401 [*] time imported but unused
[error] 116-116: F841 Local variable mock_logger assigned to but never used
[error] 117-117: F841 Local variable result assigned to but never used
state-manager/tests/unit/tasks/test_create_next_states.py
[error] 9-9: F401 [*] app.models.graph_template_validation_status.GraphTemplateValidationStatus imported but unused
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (12)
state-manager/tests/unit/singletons/test_singleton_decorator.py (4)
143-146: Don’t lock comments to a specific wrapper shapeThis comment bakes in that the decorator returns a function. Reword to focus on behavior (type/name preservation) so it stays valid if the decorator later returns a class.
- # The returned function should still reference the original class - # Note: The decorator returns a function, not a class, but the instance - # should still be of the original class type + # Regardless of decorator implementation, the instance should be of the original class type/name
148-173: Make this a real concurrent test (and xfail until the decorator is thread-safe)This “simulation” is single-threaded and won’t catch races. Convert it to a true multithreaded test using a barrier to start threads together. Mark it xfail until the decorator is upgraded to a thread-safe class-based impl.
- def test_singleton_decorator_thread_safety_simulation(self): - """Test singleton decorator behavior under simulated concurrent access""" + @pytest.mark.xfail(reason="Current singleton decorator is not thread-safe; upgrade to a locked, class-based impl.", strict=False) + def test_singleton_decorator_thread_safety(self): + """Test singleton decorator behavior under concurrent access""" @@ - # Simulate multiple "concurrent" calls - instances = [] - for _ in range(10): - instances.append(CountedClass()) + # Create multiple concurrent calls + import threading + instances = [] + barrier = threading.Barrier(10) + + def create(): + barrier.wait() + instances.append(CountedClass()) + + threads = [threading.Thread(target=create) for _ in range(10)] + for t in threads: + t.start() + for t in threads: + t.join()
223-243: Strengthen: verify failures aren’t cached by succeeding afterwardsRight now you only assert repeated failures. Add a successful call and verify it’s cached and reused, proving the failure wasn’t cached.
# Second call with failure (should try to create again since first failed) with pytest.raises(ValueError, match="Constructor failed"): FailingClass() - # The singleton pattern should handle constructor failures gracefully - # After failure, the class should still not be in instances dict + # After failures, a successful call should construct and cache the instance + success_instance = FailingClass(should_fail=False) + assert getattr(success_instance, "success", False) is True + # Subsequent calls should return the same instance without re-invoking __init__ + assert FailingClass() is success_instance
6-6: Make the suite description implementation-agnosticAvoid asserting the decorator is a “function” in the suite docstring. Keep tests resilient if the decorator is upgraded to return a class.
- """Test cases for singleton decorator function""" + """Test cases for the singleton decorator (behavior-focused, implementation-agnostic)"""state-manager/tests/unit/tasks/test_create_next_states.py (2)
63-67: Parametrize malformed placeholder cases to broaden parser coverageExpand this test to cover multiple invalid shapes in one go and keep the intent focused.
-def test_get_dependents_invalid_format(): - # Missing the mandatory ``.outputs.`` part should error out. - with pytest.raises(ValueError): - cns.get_dependents("Broken ${{parent.outputs_missing}} snippet") +@pytest.mark.parametrize("src", [ + # Missing mandatory ".outputs." + "Broken ${{parent.outputs_missing}} snippet", + # Unterminated placeholder + "Unterminated ${{parent.outputs.msg", + # Too many segments + "Too many parts ${{parent.outputs.msg.extra}}", + # Wrong mid token + "Missing outputs ${{parent.output.msg}}", + # No dot structure at all + "No dots ${{parent}}", +]) +def test_get_dependents_invalid_format(src): + with pytest.raises(ValueError): + cns.get_dependents(src)Optional: Add a positive test to lock in that whitespace inside placeholders is accepted (parser trims parts):
def test_get_dependents_allows_whitespace_inside_placeholder(): src = "Hello ${{ parent.outputs.msg }}!" result = cns.get_dependents(src) d0 = result.dependents[0] assert (d0.identifier, d0.field) == ("parent", "msg")
143-162: Update test to match bulk update implementation of mark_success_statesImplementation now performs a bulk set via State.find(...).set(...) rather than fetching each state and saving. The current test expects per-document save and will fail.
Apply this diff:
@pytest.mark.asyncio async def test_mark_success_states_updates_status(): - state_ids = ["sid-1", "sid-2"] - created = {} - - async def _get(sid): - created[sid] = DummyState(sid) - return created[sid] - - with patch.object(cns, "State") as mock_state: - # Provide *get* and *find* replacements. - mock_state.get = AsyncMock(side_effect=_get) - mock_state.find.return_value = DummyQuery() - - # Execute. - await cns.mark_success_states(state_ids) - - for st in created.values(): - assert st.status == StateStatusEnum.SUCCESS - st.save.assert_awaited() + state_ids = ["sid-1", "sid-2"] + dummy_query = DummyQuery() + + with patch.object(cns, "State") as mock_state: + mock_state.find.return_value = dummy_query + + await cns.mark_success_states(state_ids) + + # verify bulk update invoked + dummy_query.set.assert_awaited_once() + args, kwargs = dummy_query.set.call_args + assert args[0] == {"status": StateStatusEnum.SUCCESS}state-manager/tests/unit/utils/test_check_secret.py (4)
14-26: Avoid pre-reload from-import; use module import + reload to ensure fresh symbolsYou import check_api_key before reloading the module, then reload. The from-import keeps a stale reference, making the reload redundant and potentially misleading. Import the module, reload it, then call through the module.
Apply this diff:
- # Import here to get the updated environment variable - from app.utils.check_secret import check_api_key - - # Reload the module to pick up the new environment variable - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - - result = await check_api_key('test-secret-key') + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) + + result = await check_secret.check_api_key('test-secret-key')
33-41: Prefer module import + reload over from-import after reloadRe-importing the symbol after reload is verbose and easy to get wrong. Import the module as an alias, reload it, and call via the module to keep a single, clear reference.
- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - with pytest.raises(HTTPException) as exc_info: - await check_api_key('wrong-key') + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) + + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('wrong-key')
47-56: Consolidate repeated importlib.reload + from-import patterns across testsThese blocks repeat the same import/reload/from-import sequence. Use a single, consistent pattern (module import as alias, reload once, call via module). This reduces linter noise, duplication, and future F811 risks if top-level imports change.
Example refactor (apply to each block similarly):
- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - with pytest.raises(HTTPException) as exc_info: - await check_api_key('') + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) + + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('')Optional: create a small helper or pytest fixture to DRY this:
# conftest.py import importlib import app.utils.check_secret as check_secret def reload_check_secret(): return importlib.reload(check_secret)Usage in tests:
mod = reload_check_secret() with pytest.raises(HTTPException): await mod.check_api_key('wrong-key')Also applies to: 62-71, 77-86, 92-101, 107-116, 119-128, 131-141, 144-153, 186-193, 199-207
159-168: Add assertion for header location to fully validate APIKeyHeader configurationYou already assert name and auto_error. Also assert the location is header for completeness.
assert isinstance(api_key_header, APIKeyHeader) assert api_key_header.model.name == "x-api-key" + assert api_key_header.model.in_ == "header" assert api_key_header.auto_error is Falsestate-manager/tests/unit/middlewares/test_request_id_middleware.py (1)
190-193: Use pytest.approx for float equality to avoid flakinessFloating-point math can introduce tiny deltas; asserting exact/near-exact equality with a hard threshold is brittle across platforms and Python versions.
- second_call_args = mock_logger.info.call_args_list[1] - assert abs(second_call_args[1]["response_time"] - expected_ms) < 0.1 + second_call_args = mock_logger.info.call_args_list[1] + assert second_call_args[1]["response_time"] == pytest.approx(expected_ms, rel=1e-9, abs=1e-6)state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py (1)
339-342: Fix contradictory comment (assert expects propagation, not JSONResponse)The comment claims a JSONResponse should be returned when logging fails, but the test asserts the exception is propagated.
- # The middleware should still return a JSONResponse even if logging fails - # This tests the robustness of error handling + # When logger.error itself fails, the exception is propagated. + # This ensures failures in logging are surfaced to callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
state-manager/tests/unit/middlewares/test_request_id_middleware.py(1 hunks)state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py(1 hunks)state-manager/tests/unit/singletons/test_singleton_decorator.py(1 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(1 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
state-manager/tests/unit/tasks/test_create_next_states.py (3)
state-manager/app/tasks/create_next_states.py (5)
create_next_states(115-228)get_dependents(64-84)validate_dependencies(86-112)check_unites_satisfied(42-62)mark_success_states(34-39)state-manager/app/models/node_template_model.py (2)
NodeTemplate(9-15)Unites(5-6)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)
state-manager/tests/unit/singletons/test_singleton_decorator.py (2)
api-server/tests/integration/auth/singletons/test_redis_manager_integration.py (1)
redis_manager_fixture(35-89)state-manager/app/singletons/SingletonDecorator.py (2)
singleton(4-12)get_instance(7-10)
state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py (1)
state-manager/app/middlewares/unhandled_exceptions_middleware.py (2)
UnhandledExceptionsMiddleware(10-30)dispatch(11-30)
state-manager/tests/unit/utils/test_check_secret.py (1)
state-manager/app/utils/check_secret.py (1)
check_api_key(15-19)
state-manager/tests/unit/middlewares/test_request_id_middleware.py (1)
state-manager/app/middlewares/request_id_middleware.py (2)
RequestIdMiddleware(10-54)dispatch(11-54)
🔇 Additional comments (12)
state-manager/tests/unit/singletons/test_singleton_decorator.py (8)
8-25: Solid coverage: single-instance guaranteeThis test is clear and correct.
26-56: Preservation of behavior and state is well coveredGood checks for initial args sticking and shared state.
57-71: No-arg constructor path looks good
72-102: Multi-arg constructor path looks good
103-133: Per-class isolation validated correctly
174-222: Methods/properties/staticmethod/classmethod preserved — good coverage
244-271: Instance isolation across separate decorated classes — looks good
287-320: Complex initialization path validated wellGood checks for idempotence and preserved initial config.
state-manager/tests/unit/utils/test_check_secret.py (1)
211-216: LGTM: clear, focused assertion on APIKeyHeader behaviorThis test accurately captures the intent of auto_error=False without conflating FastAPI’s runtime behavior. No changes needed.
state-manager/tests/unit/middlewares/test_request_id_middleware.py (1)
10-377: Thorough, behavior-focused coverage — nice workCovers happy paths, malformed IDs, header case-insensitivity, timing, preservation of response headers, log field assertions, and exception propagation. Test doubles are well-isolated with precise patching.
state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py (2)
9-381: Comprehensive exception-path testing with precise logging assertionsCovers multiple exception types, presence/absence of request ID in state, varied methods/paths, mocked and real traceback flows, and ensures 500 JSONResponse on errors. Test structure is clear and maintainable.
186-189: Avoid deleting a non-existent MagicMock attributeRely on getattr(..., None) behavior in the middleware; explicit del on MagicMock may be brittle and is unnecessary.
- # Mock state without x_exosphere_request_id attribute - mock_request.state = MagicMock() - del mock_request.state.x_exosphere_request_id # Simulate missing attribute + # Mock state without x_exosphere_request_id attribute + mock_request.state = MagicMock()Likely an incorrect or invalid review comment.
state-manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py
Show resolved
Hide resolved
|
/gemini review but ignore tests |
|
@coderabbitai review but ignore tests |
🧩 Analysis chainI'll review the production code changes, focusing on the bulk operation implementation while ignoring test files as requested. @NiveditJain Here's my review of the production code changes, excluding tests as requested: 🔍 Overall AssessmentThis is a well-architected refactor that transforms individual state processing into efficient bulk operations. The changes are substantial but logically sound, addressing performance issues in state creation workflows. ✅ Strengths
🚨 Critical Issues1. Potential Race Condition in GraphTemplate.get_valid()async def get_valid(namespace: str, graph_name: str, polling_interval: float = 1.0, timeout: float = 300.0) -> "GraphTemplate":
# ... validation logic
start_time = time.monotonic()
while time.monotonic() - start_time < timeout:
template = await cls.get(namespace, graph_name)
if template.is_valid():
return template
if not template.is_validating():
raise ValueError(f"GraphTemplate validation failed for {namespace}/{graph_name}")
await asyncio.sleep(max(0.1, polling_interval))Issue: The polling logic doesn't handle the case where validation status changes between checks, potentially causing infinite loops or missed state transitions. 2. Memory Usage in create_next_states()The function loads all current states and parents into memory simultaneously: current_states = await State.get(state_ids)
if parents_ids:
parent_states = await State.get(list(parents_ids.values()))Risk: For large bulk operations, this could cause memory issues. Consider processing in batches.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the creation of next states to be a bulk operation, which is a great improvement for performance and efficiency. The introduction of create_next_states and the removal of the one-by-one creation logic is a solid change. I've included a few suggestions to further improve code maintainability and fix an incorrect test case. Overall, this is a well-structured refactoring with good test coverage.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (19)
state-manager/tests/unit/test_main.py (1)
67-67: Make Content-Type assertion robust to charset suffixServers commonly append a charset (e.g., "application/json; charset=utf-8"). Use startswith to avoid brittle failures.
- assert response.headers["content-type"] == "application/json" + assert response.headers["content-type"].startswith("application/json")state-manager/app/models/db/graph_template_model.py (1)
105-126: Enrich get_valid errors with current/last status and validation detailsImprove debuggability on non-validating state and timeout by surfacing the template’s status and any validation_errors.
@@ - start_time = time.monotonic() + start_time = time.monotonic() while time.monotonic() - start_time < timeout: graph_template = await GraphTemplate.get(namespace, graph_name) if graph_template.is_valid(): return graph_template if graph_template.is_validating(): await asyncio.sleep(polling_interval) else: - raise ValueError(f"Graph template is in a non-validating state: {graph_template.validation_status.value} for namespace: {namespace} and graph name: {graph_name}") - raise ValueError(f"Graph template is not valid for namespace: {namespace} and graph name: {graph_name} after {timeout} seconds") + err_details = f" errors={graph_template.validation_errors}" if graph_template.validation_errors else "" + raise ValueError( + f"Graph template is in a non-validating state: {graph_template.validation_status.value} " + f"for namespace: {namespace} and graph name: {graph_name}{err_details}" + ) + # On timeout, surface last-known status/errors + last = await GraphTemplate.get(namespace, graph_name) + last_details = ( + f" (status={last.validation_status.value}, errors={last.validation_errors})" + if last + else "" + ) + raise ValueError( + f"Graph template is not valid for namespace: {namespace} and graph name: {graph_name} " + f"after {timeout} seconds{last_details}" + )state-manager/tests/unit/tasks/test_create_next_states.py (2)
143-162: Test is asserting the old implementation; update to assert bulk update via find().set()mark_success_states now does a filtered bulk update with State.find(In(...)).set(...). Adjust the test to assert the bulk update call rather than per-object save.
@pytest.mark.asyncio async def test_mark_success_states_updates_status(): state_ids = ["sid-1", "sid-2"] - created = {} - - async def _get(sid): - created[sid] = DummyState(sid) - return created[sid] - with patch.object(cns, "State") as mock_state: - # Provide *get* and *find* replacements. - mock_state.get = AsyncMock(side_effect=_get) - mock_state.find.return_value = DummyQuery() + mock_query = DummyQuery() + mock_state.find.return_value = mock_query # Execute. - await cns.mark_success_states(state_ids) # type: ignore + await cns.mark_success_states(state_ids) # type: ignore - for st in created.values(): - assert st.status == StateStatusEnum.SUCCESS - st.save.assert_awaited() + # Verify filtered bulk update invoked with SUCCESS + mock_state.find.assert_called_once() + args, kwargs = mock_state.find.call_args + # First positional arg should be an In(...) filter including our IDs + assert hasattr(args[0], "operator") and args[0].operator == "$in" + assert state_ids == args[0].values + mock_state.find.return_value.set.assert_awaited_once_with({"status": StateStatusEnum.SUCCESS})
129-137: Broaden unites tests: missing parent raises, and no-unites fast-path returns TrueCovering these paths hardens behavior and prevents future regressions.
Add tests (outside this hunk):
@pytest.mark.asyncio async def test_check_unites_satisfied_missing_parent_raises(): unit = Unites(identifier="parent") node_tpl = NodeTemplate(node_name="node", namespace="ns", identifier="id", inputs={}, next_nodes=[], unites=[unit]) with patch.object(cns, "State") as mock_state: mock_state.find.return_value = DummyQuery(count_value=0) with pytest.raises(ValueError): await cns.check_unites_satisfied("ns", "graph", node_tpl, {}) # missing 'parent' @pytest.mark.asyncio async def test_check_unites_satisfied_no_unites_returns_true(): node_tpl = NodeTemplate(node_name="node", namespace="ns", identifier="id", inputs={}, next_nodes=[], unites=None) assert await cns.check_unites_satisfied("ns", "graph", node_tpl, {}) is Truestate-manager/app/tasks/create_next_states.py (4)
218-219: Avoid insert_many([]) when no new states were createdBeanie/Motor can error on empty inserts.
- await State.insert_many(new_states) - await mark_success_states(state_ids) + if new_states: + await State.insert_many(new_states) + await mark_success_states(state_ids)
221-228: Log failure with traceback and re-raise preserving original stackUse logger.exception and bare raise.
except Exception as e: - await State.find( + logger.exception( + "Failed to create next states (identifier=%s, namespace=%s, graph=%s)", + identifier, + namespace, + graph_name, + ) + await State.find( In(State.id, state_ids) ).set({ "status": StateStatusEnum.NEXT_CREATED_ERROR, "error": str(e) }) # type: ignore - raise e + raise
72-83: Fix over-splitting and typos in get_dependents; handle adjacent placeholdersUse maxsplit=1 to avoid “too many values to unpack” and correct “placefolder” typo.
for split in splits[1:]: - if "}}" not in split: - raise ValueError(f"Invalid syntax string placefolder {split} for: {syntax_string} '${{' not closed") - placeholder_content, tail = split.split("}}") + if "}}" not in split: + raise ValueError(f"Invalid syntax string placeholder '{split}' for: {syntax_string} — '${{' not closed") + placeholder_content, tail = split.split("}}", 1) parts = [p.strip() for p in placeholder_content.split(".")] if len(parts) != 3 or parts[1] != "outputs": - raise ValueError(f"Invalid syntax string placefolder {placeholder_content} for: {syntax_string}") + raise ValueError(f"Invalid syntax string placeholder '{placeholder_content}' for: {syntax_string}")
41-63: Minor: simplify control flow in unites checkDrop the unnecessary else; early raise already exits the iteration path.
for unit in node_template.unites: unites_id = parents.get(unit.identifier) if not unites_id: raise ValueError(f"Unit identifier not found in parents: {unit.identifier}") - else: - pending_count = await State.find( - State.identifier == unit.identifier, - State.namespace_name == namespace, - State.graph_name == graph_name, - NE(State.status, StateStatusEnum.SUCCESS), - { - f"parents.{unit.identifier}": unites_id - } - ).count() - if pending_count > 0: - return False + pending_count = await State.find( + State.identifier == unit.identifier, + State.namespace_name == namespace, + State.graph_name == graph_name, + NE(State.status, StateStatusEnum.SUCCESS), + {f"parents.{unit.identifier}": unites_id}, + ).count() + if pending_count > 0: + return Falsestate-manager/tests/unit/utils/test_check_secret.py (11)
32-43: Unify reload pattern: avoid from-import after reload; call via moduleUse
import app.utils.check_secret as check_secret+importlib.reload(check_secret)and callcheck_secret.check_api_key(...)to avoid redefinition noise and stale refs on future changes.- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('wrong-key') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('wrong-key')
47-59: Same reload/import pattern improvement herePrefer module import + reload, then call via module attribute.
- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key(None) # type: ignore + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key(None) # type: ignore
62-74: Same reload/import pattern improvement here- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('')
77-89: Same reload/import pattern improvement here- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('CASE-SENSITIVE-KEY') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('CASE-SENSITIVE-KEY')
92-104: Same reload/import pattern improvement here- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key(' whitespace-key ') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key(' whitespace-key ')
182-194: Unify reload pattern in integration test (success path)Import module + reload, then call via module to avoid stale refs.
- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - result = await check_api_key('integration-test-key') + result = await check_secret.check_api_key('integration-test-key')
195-210: Unify reload pattern in integration test (failure path)Same adjustment as the success path for consistency and future-proofing.
- import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) @@ - with pytest.raises(HTTPException) as exc_info: - await check_api_key('wrong-integration-key') + with pytest.raises(HTTPException) as exc_info: + await check_secret.check_api_key('wrong-integration-key')
163-168: Also assert header location for completenessFastAPI’s APIKey model exposes where the key is expected. Minor but helpful.
def test_api_key_header_configuration(self): """Test api_key_header is configured correctly""" assert isinstance(api_key_header, APIKeyHeader) assert api_key_header.model.name == "x-api-key" assert api_key_header.auto_error is False + assert api_key_header.model.in_ == "header"
18-27: Fix stale import: importing function before reload uses the pre-reload moduleYou import
check_api_keybefore reloading the module, so you end up calling a stale reference bound to the pre-reload globals. Import the module, reload it, and call via the reloaded module.- # Import here to get the updated environment variable - from app.utils.check_secret import check_api_key - - # Reload the module to pick up the new environment variable - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - - result = await check_api_key('test-secret-key') + # Reload the module to pick up the new environment variable and call via the reloaded module + import importlib + import app.utils.check_secret as check_secret + importlib.reload(check_secret) + + result = await check_secret.check_api_key('test-secret-key')
142-153: Delete duplicated “very long key” test (covered by the new parametrized test)This block becomes redundant once parametrized. Remove it.
- @patch.dict(os.environ, {'STATE_MANAGER_SECRET': 'very-long-key-with-many-characters-1234567890-abcdefghijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ'}) - @pytest.mark.asyncio - async def test_check_api_key_with_very_long_key(self): - """Test check_api_key works with very long keys""" - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - long_key = 'very-long-key-with-many-characters-1234567890-abcdefghijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ' - result = await check_api_key(long_key) - assert result == long_key
105-128: Parametrize success cases to remove duplication and simplify env handlingConsolidate the “special characters” and “unicode” (and include “very long key”) tests into a single parametrized async test using
monkeypatch.setenvand module reload.- @patch.dict(os.environ, {'STATE_MANAGER_SECRET': 'special-chars-!@#$%^&*()'}) - @pytest.mark.asyncio - async def test_check_api_key_with_special_characters(self): - """Test check_api_key works with special characters""" - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - result = await check_api_key('special-chars-!@#$%^&*()') - assert result == 'special-chars-!@#$%^&*()' - - @patch.dict(os.environ, {'STATE_MANAGER_SECRET': 'unicode-key-你好'}) - @pytest.mark.asyncio - async def test_check_api_key_with_unicode_characters(self): - """Test check_api_key works with unicode characters""" - import importlib - import app.utils.check_secret - importlib.reload(app.utils.check_secret) - from app.utils.check_secret import check_api_key - - result = await check_api_key('unicode-key-你好') - assert result == 'unicode-key-你好' + @pytest.mark.asyncio + @pytest.mark.parametrize("secret_and_key", [ + "special-chars-!@#$%^&*()", + "unicode-key-你好", + "very-long-key-with-many-characters-1234567890-abcdefghijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ", + ]) + async def test_check_api_key_accepts_various_valid_keys(self, monkeypatch, secret_and_key): + """Parametrized success cases for valid API keys (special chars, unicode, very long)""" + import importlib + import app.utils.check_secret as check_secret + monkeypatch.setenv("STATE_MANAGER_SECRET", secret_and_key) + importlib.reload(check_secret) + + result = await check_secret.check_api_key(secret_and_key) + assert result == secret_and_key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
state-manager/app/models/db/graph_template_model.py(3 hunks)state-manager/app/tasks/create_next_states.py(1 hunks)state-manager/tests/unit/tasks/test_create_next_states.py(1 hunks)state-manager/tests/unit/test_main.py(1 hunks)state-manager/tests/unit/utils/test_check_secret.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
state-manager/tests/unit/utils/test_check_secret.py (1)
state-manager/app/utils/check_secret.py (1)
check_api_key(15-19)
state-manager/tests/unit/test_main.py (5)
state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/db/namespace.py (1)
Namespace(6-8)state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(15-126)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)api-server/app/main.py (2)
lifespan(35-55)health(82-83)
state-manager/tests/unit/tasks/test_create_next_states.py (3)
state-manager/app/tasks/create_next_states.py (5)
create_next_states(115-228)get_dependents(64-84)validate_dependencies(86-112)check_unites_satisfied(42-62)mark_success_states(34-39)state-manager/app/models/node_template_model.py (2)
NodeTemplate(9-15)Unites(5-6)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)
state-manager/app/tasks/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-126)get(99-103)get_valid(106-126)get_node_by_identifier(39-44)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
state-manager/app/models/db/graph_template_model.py (5)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/tasks/verify_graph.py (1)
verify_graph(234-265)state-manager/app/controller/upsert_graph_template.py (1)
upsert_graph_template(12-62)
🔇 Additional comments (4)
state-manager/tests/unit/test_main.py (1)
90-102: Middleware order assertion is now correctFastAPI stores middlewares in reverse order; asserting UnhandledExceptions before RequestId is correct. Nice fix.
state-manager/app/models/db/graph_template_model.py (1)
32-38: Good addition: O(1) node lookup cacheLazy-building the identifier map is a solid perf improvement for repeated lookups during bulk processing.
state-manager/tests/unit/utils/test_check_secret.py (2)
169-178: LGTM: API_KEY reload behavior validatedThis correctly verifies that reloading the module picks up the environment value for API_KEY.
211-216: LGTM: Configuration check is clear and usefulAsserts the intended
auto_error=Falsebehavior on the header dependency.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
state-manager/app/models/db/graph_template_model.py (1)
105-126: Expose last-seen status and validation errors in get_valid errors (improves operability)On non-validating state and on timeout, include validation_errors and last-seen status to accelerate debugging. Current messages lack those details.
@staticmethod async def get_valid(namespace: str, graph_name: str, polling_interval: float = 1.0, timeout: float = 300.0) -> "GraphTemplate": @@ - while time.monotonic() - start_time < timeout: + while time.monotonic() - start_time < timeout: graph_template = await GraphTemplate.get(namespace, graph_name) if graph_template.is_valid(): return graph_template if graph_template.is_validating(): await asyncio.sleep(polling_interval) else: - raise ValueError(f"Graph template is in a non-validating state: {graph_template.validation_status.value} for namespace: {namespace} and graph name: {graph_name}") - raise ValueError(f"Graph template is not valid for namespace: {namespace} and graph name: {graph_name} after {timeout} seconds") + err_details = f" errors={graph_template.validation_errors}" if graph_template.validation_errors else "" + raise ValueError( + f"Graph template is in a non-validating state: {graph_template.validation_status.value} " + f"for namespace: {namespace} and graph name: {graph_name}{err_details}" + ) + # On timeout, surface last known status/errors + last = await GraphTemplate.get(namespace, graph_name) + last_details = ( + f" (status={last.validation_status.value}, errors={last.validation_errors})" + if last and last.validation_status else "" + ) + raise ValueError( + f"Graph template is not valid for namespace: {namespace} and graph name: {graph_name} " + f"after {timeout} seconds{last_details}" + )state-manager/app/tasks/create_next_states.py (5)
195-208: Idempotency/duplicate guard on retriesIf this task runs twice for the same batch, duplicates can be created. Enforce a uniqueness constraint and/or check-before-insert using a deterministic “next-state key”.
If you want to handle in this PR, I can propose a parents-signature helper and a uniqueness index on (run_id, graph_name, identifier, parents_signature). Otherwise, relying on issue #246 is fine.
210-211: Guard insert_many on empty listBeanie/Motor can error on insert_many([]). Short-circuit when no new states were generated.
- await State.insert_many(new_states) + if new_states: + await State.insert_many(new_states)
213-220: Log exception with traceback and preserve original stack on re-raiseAdd logger.exception for diagnostics. Use bare raise (already present) to keep traceback.
except Exception as e: + logger.exception( + "Failed to create next states (identifier=%s, namespace=%s, graph_name=%s)", + identifier, + namespace, + graph_name, + ) await State.find( In(State.id, state_ids) ).set({ "status": StateStatusEnum.NEXT_CREATED_ERROR, "error": str(e) }) # type: ignore - raise + raise
72-76: Fix potential “too many values to unpack” in placeholder splitIf tail contains additional "}}", split without a max limit can explode. Split once.
- placeholder_content, tail = split.split("}}") + placeholder_content, tail = split.split("}}", 1)
50-62: Simplify control flow in unites checkThe else is unnecessary and increases indentation. Early-raise, then proceed.
if not unites_id: raise ValueError(f"Unit identifier not found in parents: {unit.identifier}") - else: - pending_count = await State.find( - State.identifier == unit.identifier, - State.namespace_name == namespace, - State.graph_name == graph_name, - NE(State.status, StateStatusEnum.SUCCESS), - { - f"parents.{unit.identifier}": unites_id - } - ).count() - if pending_count > 0: - return False + pending_count = await State.find( + State.identifier == unit.identifier, + State.namespace_name == namespace, + State.graph_name == graph_name, + NE(State.status, StateStatusEnum.SUCCESS), + {f"parents.{unit.identifier}": unites_id}, + ).count() + if pending_count > 0: + return False
📜 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(4 hunks)state-manager/app/tasks/create_next_states.py(1 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/create_next_states.py (6)
state-manager/app/models/db/graph_template_model.py (4)
GraphTemplate(15-126)get(99-103)get_valid(106-126)get_node_by_identifier(39-44)state-manager/app/models/db/state.py (1)
State(8-18)state-manager/app/models/state_status_enum.py (1)
StateStatusEnum(4-15)state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/models/db/registered_node.py (1)
RegisteredNode(6-13)state-manager/app/tasks/create_next_state.py (1)
create_next_state(16-156)
state-manager/app/models/db/graph_template_model.py (5)
state-manager/app/models/node_template_model.py (1)
NodeTemplate(9-15)state-manager/app/utils/encrypter.py (2)
get_encrypter(36-49)decrypt(28-32)state-manager/app/models/graph_template_validation_status.py (1)
GraphTemplateValidationStatus(4-8)state-manager/app/tasks/verify_graph.py (1)
verify_graph(234-265)state-manager/app/controller/upsert_graph_template.py (1)
upsert_graph_template(12-62)
🔇 Additional comments (2)
state-manager/app/tasks/create_next_states.py (2)
147-160: Batch parent fetch is good; map key correctnessMapping parents by identifier assumes identifiers are unique within the graph, which is typically true. If that’s guaranteed by design, all good. Otherwise, consider scoping by (identifier, run_id) for safety.
Would you confirm that State.identifier is unique within a graph template and run scope? If not, we should key parents by a composite, or store a mapping per identifier to the specific parent State dictated by parents_ids.
168-173: Good: move invariant checks outside current_state loopUnites check and dependency validation now run once per next-state identifier. This prevents N×M redundant work.
2 participants
No description provided.