test(graph): capture unvalidated edge target regression (#91) #102
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!102
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tdd/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
Adds a failing-first Behave regression test proving issue #91: a graph route whose edge
target(orsource) references a non-existent node is accepted at load time and only fails by silently truncating execution — violating §11 (load-time validation,ConfigurationError), §11.3.4 (edge source/target must reference existing nodes), and §6.11.5 (edge validation) of the Actor Configuration Standard.The scenario drives a graph actor whose only edge targets an undefined node through the public
create_executor()/execute()API and asserts aConfigurationErrornaming the missing node is raised before any node runs. Today it isn't: the actor executes every node up to the dangling edge and returns partial output silently instead.The scenario is tagged
@tdd_issue,@tdd_issue_91, and@tdd_expected_failper the TDD issue-capture workflow.The actual fix lands separately on
bugfix/m1-graph-edge-target-validationper issue #91.Test plan
nox -s lintgreennox -s format -- --checkgreennox -s typecheckgreennox -s security_scangreennox -s dead_codegreennox -s unit_testsgreen (2900 scenarios, including the new tagged scenario) — confirms the assertion fails viaAssertionErrorwithout the tag, and the hook inverts it to a pass with the tag presentnox -s coverage_report— 96.6% (>= 96.5% threshold)nox -s integration_tests— 329 Robot tests passednox -s complexity,nox -s benchmark,nox -s benchmark_regression,nox -s docs,nox -s buildall greenCloses #92
PR Review: !102 (Ticket #92)
Verdict: Approve
The PR delivers a well-structured TDD regression test that captures the runtime gap described in issue #91/#92: graph configs with dangling edge targets are accepted by
create_executor()/execute()and execute partially instead of raisingConfigurationErrorbefore any node runs. The scenario uses the mandatory@tdd_issue @tdd_issue_91 @tdd_expected_failtag set, signals the bug viaAssertionError, and the step definitions are fully implemented. No critical or major issues were found.Critical Issues
None
Major Issues
None
Minor Issues
None
Nits
features/pure_graph_edge_target_validation.feature:24: The scenario title and feature mention validating both edgetargetandsource, but only the dangling-target case is exercised. Consider adding a second@tdd_issue_91scenario for an undefinedsourcenode, or ensure the companion bugfix PR covers it (issue #91 AC explicitly requires both).features/steps/pure_graph_edge_target_validation_steps.py:67-78: The Then step text promises theConfigurationErrorwas raised "before any node ran", but the assertions only check the error type and message content. A stronger assertion thatcontext.executor_result is Nonewould make the "before any node ran" guarantee explicit and guard against a future fix that raises the error after partial execution.Summary
A clean, focused TDD issue-capture PR. It correctly exercises the public runtime API, follows the project's
@tdd_expected_failworkflow, and leaves the codebase in a mergeable state with the bug behavior documented and guarded. The two nits above are optional improvements and do not block approval.ba1a74793625ab52b204Thanks for the review, @hurui200320! Addressing both nits from your review (approved, no blockers):
Nit 1 — Test coverage completeness (undefined
sourcecase): not applied here, intentionallyChecked this against the ticket metadata before deciding:
targetnames an undefined node"). It never mentionssource.target, invalidsource,start/endnormalization ... and a valid graph loading cleanly" on the companionbugfix/m1-graph-edge-target-validationbranch — i.e. thesourcecase is already owned and tracked there, not here.I agree that §11.3.4/§6.11.5 state the source/target rule as one combined normative requirement, so testing both together isn't wrong in the abstract. But extending this scenario here would push issue #92 past its own declared single-responsibility scope and duplicate work #89 already commits to delivering. So I left this PR as-is — the
sourcescenario will land with the fix onbugfix/m1-graph-edge-target-validation, not deferred vaguely but explicitly tracked in #89's subtasks.Nit 2 — Assertion vs. step wording (
executor_result is None): appliedfeatures/steps/pure_graph_edge_target_validation_steps.py'sThenstep now also assertscontext.executor_result is None, in addition to the existingConfigurationErrortype/message checks. This makes the "before any node ran" guarantee explicit and would catch a future (incorrect) fix that raises the error only after partial execution instead of before it, exactly as you flagged.Verified via
nox -s unit_tests(targeted at this feature file): the scenario still correctly fails viaAssertionErroron the new assertion when@tdd_expected_failis removed, and passes with the tag present (hook inversion) — so the regression-capture behavior is unchanged, just strengthened.Also re-ran
nox -s lint,nox -s typecheck, andnox -s format -- --check— all green. No coverage_report re-run was needed: the change only touchesfeatures/steps/..., which is fully excluded from Slipcover's--omitpatterns, so there's no coverage delta from this change.Since this branch only had a single commit, I amended it in place (commit
25ab52b) rather than adding a follow-up commit, per project commit-hygiene rules, and force-pushed.25ab52b2047d0491976dUpdate on nit 1 (undefined
sourcecase): on reconsideration, added it to this PR after all. Since this is still the same TDD-capture PR/branch and the addition is purely additive test coverage for the same underlying regression (issue #91), it doesn't push the fix itself out of scope — only strengthens the capture test, so the earlier scope objection doesn't hold up as a reason to withhold it here.Added a second scenario + Given/When/Then step set in
pure_graph_edge_target_validation.feature/pure_graph_edge_target_validation_steps.py, tagged with the same@tdd_issue @tdd_issue_91 @tdd_expected_failset.Note the runtime symptom differs from the target case, so the wording/assertions differ slightly: a dangling edge source can never actually be traversed (nothing ever "is" the missing node to transition from), so instead of the graph running partially and exiting silently (the target case), it executes to completion normally, silently ignoring the structurally invalid edge. Both cases still violate §11.3.4/§6.11.5's requirement to reject such edges at load time, and both are asserted via
ConfigurationError+executor_result is None(per your nit 2, applied to both scenarios now).Verified via targeted
nox -s unit_tests(2 scenarios / 6 steps passing, correctly reproducing the bug with@tdd_expected_failpresent), and re-rannox -s lint,nox -s typecheck,nox -s format -- --check— all green. Commit amended again (7d04919) and force-pushed.7d0491976dda37a3db3a