diff --git a/CHANGELOG.md b/CHANGELOG.md index 843b98829..18c84131c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **Cross-actor subgraph cycle detection reads actor_ref field** (#1431): Fixed + `_detect_subgraph_cycles()`, `_map_node()`, and the `compile_actor()` main loop + in `src/cleveragents/actor/compiler.py` to read `actor_ref` from the top-level + `NodeDefinition.actor_ref` field instead of `node.config.get("actor_ref", "")`. + Because `actor_ref` is a typed, validated Pydantic field (not a key inside the + untyped `config` dict), the old code always returned an empty string, causing + cross-actor cycle detection to silently fail and leaving the system vulnerable to + infinite recursion at runtime. Added Behave regression tests + (`features/actor_subgraph_cycle_detection.feature`) and a Robot Framework + integration test (`robot/actor_compiler.robot`) to prevent regressions. + ### Changed - **`agents session list` now displays full 26-character session ULIDs** (#10970): The Rich table diff --git a/features/actor_subgraph_cycle_detection.feature b/features/actor_subgraph_cycle_detection.feature new file mode 100644 index 000000000..a5c577224 --- /dev/null +++ b/features/actor_subgraph_cycle_detection.feature @@ -0,0 +1,38 @@ +Feature: Cross-actor subgraph cycle detection reads actor_ref field + As a CleverAgents developer + I want the actor compiler to correctly detect cross-actor subgraph cycles + So that mutually-referencing actors raise SubgraphCycleError at compile time + + Background: + Given the actor compiler is available + + # ──────────────────────────────────────────────────────────── + # Bug fix: actor_ref is a top-level NodeDefinition field + # ──────────────────────────────────────────────────────────── + + @tdd_issue @tdd_issue_1431 + Scenario: Mutually-referencing actors raise SubgraphCycleError + Given actor "test/actor-a" has a subgraph node with actor_ref "test/actor-b" + And actor "test/actor-b" has a subgraph node with actor_ref "test/actor-a" + And a registry containing both actors + When I compile actor "test/actor-a" with the registry resolver + Then the compilation should raise SubgraphCycleError + And the cycle error message should mention "cycle" + + @tdd_issue @tdd_issue_1431 + Scenario: Non-cyclic subgraph reference compiles successfully + Given actor "test/actor-x" has a subgraph node with actor_ref "test/actor-y" + And actor "test/actor-y" has no subgraph nodes + And a registry containing both actors + When I compile actor "test/actor-x" with the registry resolver + Then the actor compilation should succeed + And the actor subgraph_refs should map "sub" to "test/actor-y" + + @tdd_issue @tdd_issue_1431 + Scenario: Subgraph node actor_ref is reflected in compiled metadata + Given actor "test/actor-p" has a subgraph node with actor_ref "test/actor-q" + And actor "test/actor-q" has no subgraph nodes + And a registry containing both actors + When I compile actor "test/actor-p" with the registry resolver + Then the actor compilation should succeed + And the compiled node "sub" should have subgraph set to "test/actor-q" diff --git a/features/steps/actor_compiler_coverage_steps.py b/features/steps/actor_compiler_coverage_steps.py index b9515284f..6573be81a 100644 --- a/features/steps/actor_compiler_coverage_steps.py +++ b/features/steps/actor_compiler_coverage_steps.py @@ -135,7 +135,8 @@ def _subgraph_node(node_id: str, actor_ref: str = "") -> NodeDefinition: type=NodeType.SUBGRAPH, name=node_id.title(), description=f"Subgraph {node_id}", - config={"actor_ref": actor_ref}, + config={}, + actor_ref=actor_ref if actor_ref else None, ) diff --git a/features/steps/actor_compiler_steps.py b/features/steps/actor_compiler_steps.py index 6a9dbed19..10c09f0af 100644 --- a/features/steps/actor_compiler_steps.py +++ b/features/steps/actor_compiler_steps.py @@ -186,7 +186,8 @@ def step_given_subgraph_ref(context: Context, ref_name: str) -> None: type=NodeType.SUBGRAPH, name="Sub", description="Subgraph", - config={"actor_ref": ref_name}, + config={}, + actor_ref=ref_name, ), ] edges = [EdgeDefinition(from_node="main", to_node="sub")] @@ -241,7 +242,8 @@ def step_given_outer_referencing_inner( type=NodeType.SUBGRAPH, name="Sub", description="Subgraph", - config={"actor_ref": inner_name}, + config={}, + actor_ref=inner_name, ), ] edges = [EdgeDefinition(from_node="main", to_node="sub")] @@ -260,7 +262,8 @@ def step_given_resolver_cycle(context: Context, inner_name: str, back_ref: str) type=NodeType.SUBGRAPH, name="Child", description="Back-ref", - config={"actor_ref": back_ref}, + config={}, + actor_ref=back_ref, ), ] inner = _build_graph_config(inner_name, inner_nodes, [], "child", ["child"]) diff --git a/features/steps/actor_subgraph_cycle_detection_steps.py b/features/steps/actor_subgraph_cycle_detection_steps.py new file mode 100644 index 000000000..1e3160c96 --- /dev/null +++ b/features/steps/actor_subgraph_cycle_detection_steps.py @@ -0,0 +1,201 @@ +"""Step definitions for cross-actor subgraph cycle detection tests. + +Tests for features/actor_subgraph_cycle_detection.feature — validates that +the actor compiler correctly reads actor_ref from the top-level +NodeDefinition field (not from node.config) when detecting cross-actor +subgraph cycles. + +This is the regression test for issue #1431: _detect_subgraph_cycles() +was reading node.config.get("actor_ref", "") instead of node.actor_ref, +causing cycle detection to always return an empty string and never detect +cross-actor cycles. +""" + +from __future__ import annotations + +from behave import given, then, when # type: ignore[import-untyped] +from behave.runner import Context # type: ignore[import-untyped] + +from cleveragents.actor.compiler import ( + SubgraphCycleError, + compile_actor, +) +from cleveragents.actor.schema import ( + ActorConfigSchema, + ActorType, + EdgeDefinition, + NodeDefinition, + NodeType, + RouteDefinition, +) + + +# ──────────────────────────────────────────────────────────── +# Helpers +# ──────────────────────────────────────────────────────────── + + +def _make_graph_actor( + name: str, + nodes: list[NodeDefinition], + edges: list[EdgeDefinition], + entry_node: str, + exit_nodes: list[str], +) -> ActorConfigSchema: + """Build a minimal valid GRAPH actor for testing.""" + route = RouteDefinition( + nodes=nodes, + edges=edges, + entry_node=entry_node, + exit_nodes=exit_nodes, + ) + return ActorConfigSchema( + name=name, + type=ActorType.GRAPH, + description=f"Test actor {name}", + provider="openai", + model="gpt-4", + route=route, + ) + + +def _make_agent_node(node_id: str) -> NodeDefinition: + """Build a minimal AGENT node.""" + return NodeDefinition( + id=node_id, + type=NodeType.AGENT, + name=node_id.title(), + description=f"Agent {node_id}", + config={"agent": "default"}, + ) + + +def _make_subgraph_node(node_id: str, actor_ref: str) -> NodeDefinition: + """Build a SUBGRAPH node using the top-level actor_ref field. + + The actor_ref is set as a first-class field on NodeDefinition, NOT + inside config. This is the correct way to reference a subgraph actor. + """ + return NodeDefinition( + id=node_id, + type=NodeType.SUBGRAPH, + name=node_id.title(), + description=f"Subgraph {node_id}", + config={}, + actor_ref=actor_ref, + ) + + +# ──────────────────────────────────────────────────────────── +# Given steps +# ──────────────────────────────────────────────────────────── + + +@given("the actor compiler is available") +def step_compiler_available(context: Context) -> None: + """Ensure the actor compiler module is importable.""" + context.actor_registry: dict[str, ActorConfigSchema] = {} + + +@given('actor "{name}" has a subgraph node with actor_ref "{ref}"') +def step_actor_has_subgraph_ref(context: Context, name: str, ref: str) -> None: + """Create a GRAPH actor with one agent node and one subgraph node.""" + agent = _make_agent_node("start") + sub = _make_subgraph_node("sub", actor_ref=ref) + actor = _make_graph_actor( + name, + nodes=[agent, sub], + edges=[EdgeDefinition(from_node="start", to_node="sub")], + entry_node="start", + exit_nodes=["sub"], + ) + context.actor_registry[name] = actor + context.actor_to_compile = name + + +@given('actor "{name}" has no subgraph nodes') +def step_actor_has_no_subgraph(context: Context, name: str) -> None: + """Create a GRAPH actor with only an agent node (no subgraph references).""" + agent = _make_agent_node("leaf") + actor = _make_graph_actor( + name, + nodes=[agent], + edges=[], + entry_node="leaf", + exit_nodes=["leaf"], + ) + context.actor_registry[name] = actor + + +@given("a registry containing both actors") +def step_registry_contains_both(context: Context) -> None: + """Set up the resolver from the accumulated registry.""" + registry = context.actor_registry + + def resolver(actor_name: str) -> ActorConfigSchema | None: + return registry.get(actor_name) + + context.resolver = resolver + + +# ──────────────────────────────────────────────────────────── +# When steps +# ──────────────────────────────────────────────────────────── + + +@when('I compile actor "{name}" with the registry resolver') +def step_compile_actor(context: Context, name: str) -> None: + """Compile the named actor using the registry resolver.""" + context.compile_error = None + context.compiled = None + actor = context.actor_registry[name] + try: + context.compiled = compile_actor(actor, actor_resolver=context.resolver) + except Exception as exc: + context.compile_error = exc + + +# ──────────────────────────────────────────────────────────── +# Then steps +# ──────────────────────────────────────────────────────────── + + +@then("the compilation should raise SubgraphCycleError") +def step_raises_cycle_error(context: Context) -> None: + """Assert that compilation raised SubgraphCycleError.""" + assert context.compile_error is not None, ( + "Expected SubgraphCycleError but compilation succeeded" + ) + assert isinstance(context.compile_error, SubgraphCycleError), ( + f"Expected SubgraphCycleError, got {type(context.compile_error).__name__}: " + f"{context.compile_error}" + ) + + +@then("the actor compilation should succeed") +def step_actor_compilation_succeeds(context: Context) -> None: + """Assert that compilation succeeded without errors.""" + assert context.compile_error is None, ( + f"Expected compilation to succeed but got: {context.compile_error}" + ) + assert context.compiled is not None + + +@then('the cycle error message should mention "{text}"') +def step_cycle_error_mentions(context: Context, text: str) -> None: + """Assert that the cycle error message contains the given text.""" + assert context.compile_error is not None + msg = str(context.compile_error) + assert text.lower() in msg.lower(), ( + f"Expected '{text}' in error message, got: {msg}" + ) + + +@then('the actor subgraph_refs should map "{node}" to "{ref}"') +def step_actor_subgraph_refs_map(context: Context, node: str, ref: str) -> None: + """Assert that the compiled metadata maps the node to the expected actor ref.""" + assert context.compiled is not None + refs = context.compiled.metadata.subgraph_refs + assert refs.get(node) == ref, ( + f"Expected subgraph_refs[{node!r}] == {ref!r}, got {refs}" + ) diff --git a/robot/actor_compiler.robot b/robot/actor_compiler.robot index 2052e00d7..7605a6485 100644 --- a/robot/actor_compiler.robot +++ b/robot/actor_compiler.robot @@ -42,3 +42,15 @@ Reject LLM Actor Compilation Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} actor-compiler-expected-fail Should Contain ${result.stdout} GRAPH + +Detect Cross-Actor Subgraph Cycle Via actor_ref Field + [Documentation] Verify that mutually-referencing actors raise SubgraphCycleError. + ... This is the regression test for issue #1431: _detect_subgraph_cycles() + ... was reading node.config.get("actor_ref") instead of node.actor_ref, + ... causing cycle detection to silently fail. + [Tags] slow + ${result}= Run Process ${PYTHON} ${HELPER} cycle-detect dummy cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} actor-compiler-cycle-detected diff --git a/robot/helper_actor_compiler.py b/robot/helper_actor_compiler.py index 21a767224..fad35e8c1 100644 --- a/robot/helper_actor_compiler.py +++ b/robot/helper_actor_compiler.py @@ -66,6 +66,82 @@ def main() -> int: print(f"actor-compiler-fail: {exc}") return 1 + if command == "cycle-detect": + # Test cross-actor cycle detection using actor_ref top-level field. + # Creates two mutually-referencing actors and verifies SubgraphCycleError + # is raised, confirming the fix for issue #1431. + from cleveragents.actor.compiler import SubgraphCycleError + from cleveragents.actor.schema import ( + ActorType, + EdgeDefinition, + NodeDefinition, + NodeType, + RouteDefinition, + ) + + def _make_actor( + name: str, subgraph_ref: str | None = None + ) -> ActorConfigSchema: + if subgraph_ref is not None: + nodes = [ + NodeDefinition( + id="start", + type=NodeType.AGENT, + name="Start", + description="Start", + config={"agent": "default"}, + ), + NodeDefinition( + id="sub", + type=NodeType.SUBGRAPH, + name="Sub", + description="Subgraph", + config={}, + actor_ref=subgraph_ref, + ), + ] + edges = [EdgeDefinition(from_node="start", to_node="sub")] + entry, exits = "start", ["sub"] + else: + nodes = [ + NodeDefinition( + id="leaf", + type=NodeType.AGENT, + name="Leaf", + description="Leaf", + config={"agent": "default"}, + ) + ] + edges = [] + entry, exits = "leaf", ["leaf"] + route = RouteDefinition( + nodes=nodes, edges=edges, entry_node=entry, exit_nodes=exits + ) + return ActorConfigSchema( + name=name, + type=ActorType.GRAPH, + description=f"Test actor {name}", + model="gpt-4", + route=route, + ) + + actor_a = _make_actor("test/actor-a", subgraph_ref="test/actor-b") + actor_b = _make_actor("test/actor-b", subgraph_ref="test/actor-a") + registry = {"test/actor-a": actor_a, "test/actor-b": actor_b} + + try: + compile_actor(actor_a, actor_resolver=registry.get) + print( + "actor-compiler-cycle-not-detected: FAIL — expected SubgraphCycleError" + ) + return 1 + except SubgraphCycleError as exc: + print(f"actor-compiler-cycle-detected: {exc}") + return 0 + except Exception as exc: + print(f"actor-compiler-unexpected-error: {exc}") + return 1 + print(f"Unknown command: {command}") return 1 diff --git a/src/cleveragents/actor/compiler.py b/src/cleveragents/actor/compiler.py index 5abc0439b..7c381552d 100644 --- a/src/cleveragents/actor/compiler.py +++ b/src/cleveragents/actor/compiler.py @@ -137,7 +137,7 @@ def _map_node(node: NodeDefinition) -> lg_nodes.NodeConfig: config.get("function") if node.type == NodeType.CONDITIONAL else None ), tools=config.get("tools", []) if node.type == NodeType.TOOL else [], - subgraph=(config.get("actor_ref") if node.type == NodeType.SUBGRAPH else None), + subgraph=(node.actor_ref if node.type == NodeType.SUBGRAPH else None), metadata=dict(config), ) @@ -187,6 +187,11 @@ def _detect_subgraph_cycles( """Detect cycles in subgraph references across actors. Returns list of actor names forming a cycle, or empty list. + + The actor reference is read from the top-level actor_ref field on + NodeDefinition, not from node.config. Reading from config + would always return an empty string because actor_ref is stored as a + first-class field on the schema model. """ if resolver is None: return [] @@ -194,7 +199,7 @@ def _detect_subgraph_cycles( for node in route_nodes: if node.type != NodeType.SUBGRAPH: continue - ref_name = node.config.get("actor_ref", "") + ref_name = node.actor_ref or "" if not ref_name: continue if ref_name in visited: @@ -290,7 +295,7 @@ def compile_actor( all_lsp_bindings.extend(bindings) if node_def.type == NodeType.SUBGRAPH: - ref = node_def.config.get("actor_ref", "") + ref = node_def.actor_ref or "" if ref: subgraph_refs[node_def.id] = ref