Skip to content

[ONGOING, DO NOT MERGE] Fixing issues in verify_graph task #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
NiveditJain wants to merge 1 commit into from
Closed

[ONGOING, DO NOT MERGE] Fixing issues in verify_graph task #290

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions state-manager/app/tasks/verify_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import asyncio

from app.models.db.graph_template_model import GraphTemplate, NodeTemplate
from app.models.graph_template_validation_status import GraphTemplateValidationStatus
from app.models.db.registered_node import RegisteredNode
Expand All @@ -7,26 +9,33 @@

logger = LogsManager().get_logger()

async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str]):
async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]:
errors = []
for node in nodes:
if node.node_name is None or node.node_name == "":
errors.append(f"Node {node.identifier} has no name")
errors.append(f"Node {node.identifier} has no name")

@gemini-code-assist gemini-code-assist Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This line has trailing whitespace, which should be removed to adhere to the PEP 8 style guide.

Suggested change
errors.append(f"Node {node.identifier} has no name")
errors.append(f"Node {node.identifier} has no name")

return errors
Comment on lines +12 to +17

@coderabbitai coderabbitai Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Add backward-compatible optional errors parameter and avoid shadowing

Tests are still calling the old signature with (nodes, errors). Accept an optional errors param and, if provided, extend it for backward compatibility. Also avoid shadowing by using a different local variable name.

-async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]:
-    errors = []
+async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str] | None = None) -> list[str]:
+    errs: list[str] = []
     for node in nodes:
-        if node.node_name is None or node.node_name == "":
-            errors.append(f"Node {node.identifier} has no name")    
-    return errors
+        if not node.node_name:
+            errs.append(f"Node {node.identifier} has no name")
+    if errors is not None:
+        errors.extend(errs)
+    return errs

Run this to confirm all call-sites now work with the optional param (both styles supported):

🏁 Script executed:

#!/bin/bash
# Find invocations providing an 'errors' arg (legacy signature)
rg -nP '\bverify_nodes_names\s*\(\s*[^,]+,\s*[^)]+\)'

Length of output: 364

Backward Compatibility for verify_nodes_names

The tests in state-manager/tests/unit/tasks/test_verify_graph.py invoke:

  • await verify_nodes_names(nodes, errors) at lines 34 and 47
  • await verify_nodes_names(nodes, errors) again at line 88

These calls rely on two behaviors of the original signature:

  1. Passing in an existing errors list and having it populated.
  2. Receiving the same errors list back as the return value.

The proposed diff instead:

  • Introduces a new local errs list.
  • Extends the passed-in errors list (if non-null) but returns only errs.

This breaks callers expecting:

errors = []
result = await verify_nodes_names(nodes, errors)
# result is errors, and errors has been mutated

With the new code, result is a distinct list (errs), so result is not errors.

To restore full backward compatibility:

  • Use the passed-in errors list as the working list when provided.
  • Only allocate a new list when errors is None.
  • Return the very list that callers passed in (or the new one when none was passed).

Suggested replacement:

- async def verify_nodes_names(nodes: list[NodeTemplate], errors: list[str] | None = None) -> list[str]:
-     errs: list[str] = []
-     for node in nodes:
-         if not node.node_name:
-             errs.append(f"Node {node.identifier} has no name")
-     if errors is not None:
-         errors.extend(errs)
-     return errs
+ async def verify_nodes_names(
+     nodes: list[NodeTemplate],
+     errors: list[str] | None = None,
+ ) -> list[str]:
+     # Use provided list for backward compatibility, or create one if omitted
+     error_list = errors if errors is not None else []
+
+     for node in nodes:
+         if not node.node_name:
+             error_list.append(f"Node {node.identifier} has no name")
+
+     return error_list

This ensures:

  • If callers pass errors, it's both mutated and returned.
  • If callers omit errors, a new list is returned.

Please update the implementation accordingly to fully satisfy existing tests.

🤖 Prompt for AI Agents 
In state-manager/app/tasks/verify_graph.py around lines 12 to 17, the function
now creates and returns a new local list instead of using and returning the
caller-provided errors list, breaking callers who expect the same list to be
mutated and returned; change the implementation to accept an optional errors
parameter, set errors = errors or [] (i.e. only allocate when None), append
validation messages to that errors list, and return that same errors list so
callers receive and observe mutations to the original list.

💡 Verification agent

🧩 Analysis chain

Resolve pipeline signature mismatches with a temporary compatibility shim

Given the CI failures, the optional errors parameter pattern above should make both legacy tests and the new return-based style pass. Recommend adding a deprecation note and removing the param after tests are migrated.

Run this to find and update tests calling the legacy signatures:

I can submit a follow-up patch to update the tests to the new return-based API and then remove the shims.

Also applies to: 19-25, 26-36, 37-67, 68-88

🏁 Script executed:

#!/bin/bash
# Search legacy-style invocations that pass an explicit 'errors' list
rg -nP '\b(verify_nodes_names|verify_nodes_namespace|verify_node_exists|verify_node_identifiers|verify_secrets)\s*\(\s*[^,]+,\s*[^)]+\)' -C2

Length of output: 9126

Add optional errors parameter shim to all verify_* functions

The unit tests in state-manager/tests/unit/tasks/test_verify_graph.py still call the legacy signatures—passing in an explicit errors list—so we need to update each verify_* function in state-manager/app/tasks/verify_graph.py to accept an optional errors parameter and mutate it (while still returning the list for the new style). Please add a short deprecation note (and/or a warnings.warn(..., DeprecationWarning)) and remove this shim once all tests are migrated.

• Functions to update (signatures currently lack an errors param):

  • verify_nodes_names(nodes: list[NodeTemplate])
  • verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str)
  • verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode])
  • verify_node_identifiers(nodes: list[NodeTemplate])
  • verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode])

• Tests invoking legacy style (examples):

  • await verify_nodes_names(nodes, errors) at lines 34, 47, 88
  • await verify_nodes_namespace(nodes, "test", errors) at lines 106, 119, 134
  • await verify_node_exists(nodes, database_nodes, errors) at lines 164, 183, 204
  • await verify_node_identifiers(nodes, errors) at lines 223, 236, 283, 297
  • await verify_secrets(graph_template, database_nodes, errors) at lines 323, 341, 360, 381

• Example diff for verify_nodes_names:

--- a/state-manager/app/tasks/verify_graph.py
+++ b/state-manager/app/tasks/verify_graph.py
@@ -12,7 +12,10 @@
 async def verify_nodes_names(nodes: list[NodeTemplate]) -> list[str]:
-    errors = []
+async def verify_nodes_names(
+    nodes: list[NodeTemplate],
+    errors: list[str] | None = None,  # DEPRECATED: remove after tests migrate
+) -> list[str]:
+    if errors is None:
+        errors = []
     for node in nodes:
         if node.node_name is None or node.node_name == "":
             errors.append(f"Node {node.identifier} has no name")
     return errors

Apply the same pattern (optional errors defaulting to None, in-place mutation, return) to the other four functions.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In state-manager/app/tasks/verify_graph.py around lines 12 to 17, the function
verify_nodes_names currently accepts only nodes and returns a new errors list;
update its signature to accept an optional errors: Optional[list[str]] = None
(and add "import warnings" at the top if missing), then if errors is None set
errors = [] else call warnings.warn("legacy errors parameter is deprecated",
DeprecationWarning) so the function mutates the provided list in-place while
still returning it; apply the same pattern (optional errors param defaulting to
None, deprecation warning when provided, in-place appends, and returning errors)
to verify_nodes_namespace, verify_node_exists, verify_node_identifiers, and
verify_secrets.


async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str, errors: list[str]):
async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str) -> list[str]:
errors = []
for node in nodes:
if node.namespace != graph_namespace and node.namespace != "exospherehost":
errors.append(f"Node {node.identifier} has invalid namespace '{node.namespace}'. Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'")
return errors

Comment on lines +19 to 25

@coderabbitai coderabbitai Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

⚠️ Potential issue

Namespace check: add optional errors param for test compatibility

Mirror the compatibility shim and keep return-based semantics.

-async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str) -> list[str]:
-    errors = []
+async def verify_nodes_namespace(nodes: list[NodeTemplate], graph_namespace: str, errors: list[str] | None = None) -> list[str]:
+    errs: list[str] = []
     for node in nodes:
         if node.namespace != graph_namespace and node.namespace != "exospherehost":
-            errors.append(f"Node {node.identifier} has invalid namespace '{node.namespace}'. Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'")
-    return errors
+            errs.append(
+                f"Node {node.identifier} has invalid namespace '{node.namespace}'. "
+                f"Must match graph namespace '{graph_namespace}' or use universal namespace 'exospherehost'"
+            )
+    if errors is not None:
+        errors.extend(errs)
+    return errs

Optional clean-up (outside this hunk): define a constant to avoid string duplication.

# near the imports
UNIVERSAL_NAMESPACE = "exospherehost"
🤖 Prompt for AI Agents 
In state-manager/app/tasks/verify_graph.py around lines 19 to 25, the function
verify_nodes_namespace should accept an optional errors parameter for test
compatibility while retaining return-based semantics; update the signature to
accept errors: Optional[list[str]] = None, initialize it to a new list if None,
append namespace validation messages to that list, and still return the list at
the end. Also replace the literal "exospherehost" with a UNIVERSAL_NAMESPACE
constant (define it near imports) to avoid string duplication.

async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], errors: list[str]):
async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]:
errors = []
template_nodes_set = set([(node.node_name, node.namespace) for node in nodes])
database_nodes_set = set([(node.name, node.namespace) for node in database_nodes])

nodes_not_found = template_nodes_set - database_nodes_set

@gemini-code-assist gemini-code-assist Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This line contains only whitespace and creates an unnecessary blank line. According to the PEP 8 style guide (E303), excessive blank lines should be avoided. Please remove this line.

for node in nodes_not_found:
errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
return errors

Comment on lines +26 to 36

@coderabbitai coderabbitai Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Existence check: add optional errors param for compatibility

Keep the new return style while supporting old callers.

-async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]:
-    errors = []
+async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], errors: list[str] | None = None) -> list[str]:
+    errs: list[str] = []
@@
-    for node in nodes_not_found:
-        errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
-    return errors
+    for node in nodes_not_found:
+        errs.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
+    if errors is not None:
+        errors.extend(errs)
+    return errs
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def verify_node_exists(nodes: list[NodeTemplate], database_nodes: list[RegisteredNode]) -> list[str]:
errors = []
template_nodes_set = set([(node.node_name, node.namespace) for node in nodes])
database_nodes_set = set([(node.name, node.namespace) for node in database_nodes])
nodes_not_found = template_nodes_set - database_nodes_set
for node in nodes_not_found:
errors.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
return errors
async def verify_node_exists(
nodes: list[NodeTemplate],
database_nodes: list[RegisteredNode],
errors: list[str] | None = None
) -> list[str]:
errs: list[str] = []
template_nodes_set = set((node.node_name, node.namespace) for node in nodes)
database_nodes_set = set((node.name, node.namespace) for node in database_nodes)
nodes_not_found = template_nodes_set - database_nodes_set
for node in nodes_not_found:
errs.append(f"Node {node[0]} in namespace {node[1]} does not exist.")
if errors is not None:
errors.extend(errs)
return errs
🤖 Prompt for AI Agents 
In state-manager/app/tasks/verify_graph.py around lines 26 to 36, the function
should accept an optional errors parameter to maintain backwards compatibility;
change the signature to accept errors: Optional[list[str]] = None (import
Optional from typing), initialize local_errors = errors if errors is not None
else [] and append missing node messages to local_errors, then return
local_errors so callers using the old pattern still receive the aggregated list
while new callers get the same returned list.

async def verify_node_identifiers(nodes: list[NodeTemplate], errors: list[str]):
async def verify_node_identifiers(nodes: list[NodeTemplate]) -> list[str]:
errors = []
identifier_to_nodes = {}

# First pass: collect all nodes by identifier
Expand Down Expand Up @@ -54,7 +63,10 @@ async def verify_node_identifiers(nodes: list[NodeTemplate], errors: list[str]):
if next_node not in valid_identifiers:
errors.append(f"Node {node.node_name} in namespace {node.namespace} has a next node {next_node} that does not exist in the graph")

async def verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode], errors: list[str]):
return errors

async def verify_secrets(graph_template: GraphTemplate, database_nodes: list[RegisteredNode]) -> list[str]:
errors = []
required_secrets_set = set()

for node in database_nodes:
Expand All @@ -71,9 +83,10 @@ async def verify_secrets(graph_template: GraphTemplate, database_nodes: list[Reg

for secret_name in missing_secrets_set:
errors.append(f"Secret {secret_name} is required but not present in the graph template")


return errors

async def get_database_nodes(nodes: list[NodeTemplate], graph_namespace: str):
async def get_database_nodes(nodes: list[NodeTemplate], graph_namespace: str) -> list[RegisteredNode]:
graph_namespace_node_names = [
node.node_name for node in nodes if node.namespace == graph_namespace
]
Expand All @@ -91,7 +104,8 @@ async def get_database_nodes(nodes: list[NodeTemplate], graph_namespace: str):
return graph_namespace_database_nodes + exospherehost_database_nodes


async def verify_inputs(graph_nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], dependency_graph: dict[str, list[str]], errors: list[str]):
async def verify_inputs(graph_nodes: list[NodeTemplate], database_nodes: list[RegisteredNode], dependency_graph: dict[str, list[str]]) -> list[str]:
errors = []
look_up_table = {}
for node in graph_nodes:
look_up_table[node.identifier] = {"graph_node": node}
Expand Down Expand Up @@ -146,8 +160,10 @@ async def verify_inputs(graph_nodes: list[NodeTemplate], database_nodes: list[Re

except Exception as e:
errors.append(f"Error creating input model for node {node.identifier}: {str(e)}")

return errors

async def build_dependencies_graph(graph_nodes: list[NodeTemplate]):
async def build_dependencies_graph(graph_nodes: list[NodeTemplate]) -> dict[str, set[str]]:
dependency_graph = {}
for node in graph_nodes:
dependency_graph[node.identifier] = set()
Expand Down Expand Up @@ -230,21 +246,25 @@ async def verify_unites(graph_nodes: list[NodeTemplate], dependency_graph: dict
if node.unites.identifier not in dependency_graph[node.identifier]:
errors.append(f"Node {node.identifier} depends on {node.unites.identifier} which is not a dependency of {node.identifier}")


async def verify_graph(graph_template: GraphTemplate):
try:
errors = []
database_nodes = await get_database_nodes(graph_template.nodes, graph_template.namespace)

await verify_nodes_names(graph_template.nodes, errors)
await verify_nodes_namespace(graph_template.nodes, graph_template.namespace, errors)
await verify_node_exists(graph_template.nodes, database_nodes, errors)
await verify_node_identifiers(graph_template.nodes, errors)
await verify_secrets(graph_template, database_nodes, errors)
basic_verify_tasks = [
verify_nodes_names(graph_template.nodes),
verify_nodes_namespace(graph_template.nodes, graph_template.namespace),
verify_node_exists(graph_template.nodes, database_nodes),
verify_node_identifiers(graph_template.nodes),
verify_secrets(graph_template, database_nodes)
]
errors.extend(await asyncio.gather(*basic_verify_tasks))

@gemini-code-assist gemini-code-assist Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The result of asyncio.gather(*basic_verify_tasks) is a list of lists of strings (e.g., [['error1'], [], ['error2']]), as each verification task returns a list[str]. Using errors.extend() here will create a nested list of errors, which is not the expected flat list[str] for graph_template.validation_errors. This will cause incorrect error reporting.

You should flatten the list of lists into a single list of error strings.

Suggested change
errors.extend(await asyncio.gather(*basic_verify_tasks))
errors.extend([error for error_list in await asyncio.gather(*basic_verify_tasks) for error in error_list])


Comment on lines +254 to +262

@coderabbitai coderabbitai Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Flatten asyncio.gather results instead of extending with a list of lists

errors.extend(await asyncio.gather(...)) appends each returned list as a single element. This inflates errors length even when all lists are empty and will incorrectly skip later checks. Flatten before extending.

Apply this diff:

-        errors.extend(await asyncio.gather(*basic_verify_tasks))
+        basic_results = await asyncio.gather(*basic_verify_tasks, return_exceptions=False)
+        for errs in basic_results:
+            if errs:
+                errors.extend(errs)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
basic_verify_tasks = [
verify_nodes_names(graph_template.nodes),
verify_nodes_namespace(graph_template.nodes, graph_template.namespace),
verify_node_exists(graph_template.nodes, database_nodes),
verify_node_identifiers(graph_template.nodes),
verify_secrets(graph_template, database_nodes)
]
errors.extend(await asyncio.gather(*basic_verify_tasks))
basic_verify_tasks = [
verify_nodes_names(graph_template.nodes),
verify_nodes_namespace(graph_template.nodes, graph_template.namespace),
verify_node_exists(graph_template.nodes, database_nodes),
verify_node_identifiers(graph_template.nodes),
verify_secrets(graph_template, database_nodes)
]
basic_results = await asyncio.gather(*basic_verify_tasks, return_exceptions=False)
for errs in basic_results:
if errs:
errors.extend(errs)
🤖 Prompt for AI Agents 
In state-manager/app/tasks/verify_graph.py around lines 254 to 262, the code
does errors.extend(await asyncio.gather(...)) which appends each returned list
as a single element; change it to await the gather into a variable and then
flatten the list-of-lists before extending errors (e.g., collect results = await
asyncio.gather(...), then extend errors with each inner item by either iterating
results and extending by each sublist or using a list comprehension to flatten:
flattened = [item for sub in results for item in sub];
errors.extend(flattened)).

dependency_graph = await verify_topology(graph_template.nodes, errors)

if dependency_graph is not None and len(errors) == 0:
await verify_inputs(graph_template.nodes, database_nodes, dependency_graph, errors)
inputs_errors = await verify_inputs(graph_template.nodes, database_nodes, dependency_graph)
errors.extend(inputs_errors)

await verify_unites(graph_template.nodes, dependency_graph, errors)
Comment on lines 263 to 269

@gemini-code-assist gemini-code-assist Bot Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The refactoring to make verification functions pure is incomplete. The verify_topology and verify_unites functions still modify the errors list passed as an argument, which is inconsistent with the new pattern adopted for other verification functions in this pull request.

For consistency and to fully realize the benefits of this refactoring, these functions should also be updated to return a list of errors instead of modifying the list in-place.


Expand Down
Loading