fix(graph): reject edges targeting unknown nodes during validation #104
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
Reference
cleveragents/cleveractors-core!104
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/m1-graph-edge-target-validation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the fix for issue #89 (companion TDD-capture issue #92, PR !102): a graph route edge whose
sourceortargetnamed a node absent fromnodeswas accepted silently at load time instead of raisingConfigurationError, violating Actor Configuration Standard §11 preamble, §11.3.4, and §6.11.5.create_executor()/Executor.execute()— the router-facing API exercised by the TDD regression test and by the Robot integration tests — never called into the existingcleveractors.validationpackage at all, and that package's actor-level path (_actor.py_validate_graph_actor) never checked edge endpoints either (the spec-level path already did, but that's a different, unexercised config shape). Rather than wiring the fullvalidate_dict()/validate_actor_config()dispatcher intoExecutor.__init__(which would newly start enforcing llm/tool/multi_actor structural checks never exercised viacreate_executor()before — an unrelated regression risk), the fix adds a narrowly-scoped_validate_graph_edge_endpoints()helper inruntime_dispatch.py, called from both_execute_graph()and_execute_graph_stream()right afterpg_nodes/pg_edgesare built and before the agent-creation loop, so a malformed graph never instantiates an agent or processes a message (§12.1 lifecycle ordering).start/end/START/ENDare always accepted regardless of explicit declaration, matchingPureLangGraph's auto-injection/normalization (§6.2.1, §6.4).The existing (unwired) spec-level edge check in
cleveractors.validationis left as-is — out of scope for this actor-level runtime bug, preserved rather than removed.Changes
src/cleveractors/runtime_dispatch.py: new_validate_graph_edge_endpoints(), wired into_execute_graph()/_execute_graph_stream().features/pure_graph_edge_target_validation.feature/features/steps/pure_graph_edge_target_validation_steps.py: removed@tdd_expected_failfrom the two issue #91 regression scenarios (now pass unconditionally;@tdd_issue/@tdd_issue_91retained as permanent regression guards), added 3 new scenarios (END-target normalization, START-source normalization, fully valid graph).robot/email_graph_negative_tests.robot: 3 new integration test cases reusing the existingEmailGraphLib.CHANGELOG.md: newFixedentry.Test plan
nox -s lintgreennox -s format -- --checkgreennox -s typecheckgreennox -s security_scangreennox -s dead_codegreennox -s complexitygreennox -s unit_testsgreen (2910/2910 scenarios)nox -s integration_testsgreennox -s coverage_report— 96.7% (>= 96.5% threshold)Closes #89
PR Review: !104 (Ticket #89)
Verdict: Approve
The implementation correctly addresses the core bug: graph routes with edges referencing undeclared nodes now raise
ConfigurationErrorbefore any agent is instantiated or any message is processed. The fix is narrowly scoped to the runtime dispatch path, preserves the existing (unwired) spec-level validator, and includes Behave regression tests plus Robot integration tests. Code quality, type safety, and error messaging are all satisfactory.Critical Issues
None.
Major Issues
None.
Minor Issues
features/androbot/_validate_graph_edge_endpoints()is added to both_execute_graph()and_execute_graph_stream(), but the new tests only exercise the non-streamingexecute()path. The streaming path’s error branch is therefore not directly verified, even though it has distinct billing-integrity wrapping.executor.execute_stream()(orexecute_email_graph_stream) with a dangling edge and asserts aConfigurationErroris raised.Nits
None.
Summary
PR !104 is a focused, well-explained bugfix. The
_validate_graph_edge_endpoints()helper is clean, correctly accepts the auto-injected/normalizedstart/end/START/ENDsentinels, and is placed after edge parsing but before agent creation in both graph execution paths. Tests remove the@tdd_expected_failguard and add positive cases for valid graphs and sentinel normalization. The only notable gap is the lack of a streaming-path negative test; everything else meets the acceptance criteria without introducing regressions.25371bf89b8ecc4405ddAddressed the Minor finding from @hurui200320's review: added a Behave scenario (
features/pure_graph_edge_target_validation.feature) that drivesExecutor.execute_stream()against the same dangling-edge-target config, asserting theConfigurationErrorpropagates before any token is yielded and thatexecutor.last_resultgets the billing-integrity placeholder. Confirmed vianox -s coverage_report(scoped to this feature file) that the previously-uncovered_execute_graph_stream()exception branch is now exercised. Fullnox -s unit_testsis green at 2911/2911 scenarios (was 2910). Amended the existing commit (no new commit) and force-pushed to this branch. Re-requesting review.A graph route edge whose source or target named a node absent from nodes was accepted silently instead of raising ConfigurationError, violating the load-time validation required by the Actor Configuration Standard §11 preamble, §11.3.4 (graph edge source/ target must reference existing nodes), and §6.11.5 (edge validation). The actor would execute every node up to a dangling target and exit silently with partial output; a dangling source can never be traversed at runtime, so it instead let the graph complete normally while silently ignoring the structurally invalid edge. The cleveractors.validation package already implements this check for spec-level configs (agents + routes at the top level, _routes.py _validate_graph_route), but create_executor()/Executor.execute() — the router-facing API this project's TDD regression test and Robot integration tests exercise — never calls into cleveractors.validation at all. That package's actor-level path (_actor.py _validate_graph_actor) also never checked edge endpoints. Rather than wiring the full validate_dict()/validate_actor_config() dispatcher into Executor.__init__ (which would additionally start enforcing llm/tool/multi_actor structural checks never exercised via create_executor() before, an unrelated behavior change/regression risk), the fix adds a narrowly-scoped _validate_graph_edge_endpoints() helper in runtime_dispatch.py, called from both _execute_graph() and _execute_graph_stream() immediately after pg_nodes/pg_edges are built from either the legacy route={} or v2.0 routes={main:{}} config shape — and before the agent-creation loop, so a malformed graph never instantiates an agent or processes a message (§12.1 lifecycle ordering). start/end/START/END are always accepted as valid endpoints regardless of whether they appear in the declared nodes mapping, since PureLangGraph._initialize_nodes() auto-injects them and _analyze_graph() normalizes the uppercase spellings (§6.2.1, §6.4). The cleveractors.validation package's existing (unwired) spec-level edge check is left as-is per its own scope — extending it further is unrelated to this actor-level runtime bug and is preserved rather than removed. Removes @tdd_expected_fail from the two issue #91 regression scenarios now that the fix makes them pass unconditionally (leaving @tdd_issue/ @tdd_issue_91 as permanent regression guards), and adds four new scenarios: an edge targeting the auto-injected END node, an edge sourced from the auto-injected START node, a fully valid graph, and (per hurui200320's PR review) a streaming-path counterpart of the dangling-target scenario — all per issue #89's acceptance criteria. Adds three Robot integration test cases to email_graph_negative_tests.robot (reusing the existing EmailGraphLib rather than a new library) covering the same dangling-target, dangling-source, and END-normalization behavior end-to-end through create_executor(). Addresses the sole (Minor) finding from hurui200320's review of this PR: _validate_graph_edge_endpoints() is called from both _execute_graph() and _execute_graph_stream(), but the original tests only drove the non-streaming execute() path, leaving _execute_graph_stream()'s distinct billing-integrity wrapper (the try/except around config normalization that populates executor.last_result with a <no_llm> placeholder before re-raising ConfigurationError) unverified. The new scenario drives Executor.execute_stream() to exhaustion against the same dangling-edge-target config, asserting the ConfigurationError propagates before any token is yielded and that executor.last_result is populated with the billing-integrity placeholder. ISSUES CLOSED: #89 Refs: #91, #928ecc4405dd67972f0dc4