Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| a7fb57ea9c | |||
| a1093430bd |
@@ -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
|
||||
|
||||
- **Diagnostics spec examples expanded to all 9 providers** (#5320): Updated the
|
||||
|
||||
@@ -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"
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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"])
|
||||
|
||||
@@ -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
|
||||
from behave.runner import Context
|
||||
|
||||
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}"
|
||||
)
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user