test(langgraph): capture parallel dispatch node-flag regression (#97) #109
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
#98 TDD: Pure-graph parallel dispatch runs all next-nodes concurrently whenever len > 1, ignoring each node's
parallel flag
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!109
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tdd/m1-pure-graph-parallel-node-gate"
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 #97:
PureLangGraph's three concurrent-dispatch sites (pure_graph.pylines 1078, 1860, 1965) fire every candidate next-node concurrently viaasyncio.gather/asyncio.create_taskwhenever the graph-levelparallel_execution: true(the default) and there are 2+ candidates, without ever consulting each node's ownparallelflag (NodeConfig.parallel/can_execute_parallel()). This violates Actor Configuration Standard §6.7 and §12.3.The scenario builds a pure-graph route where a trigger node has two unconditional edges to sibling agent nodes, neither marked
parallel: true, and drives both through a sharedTimingRecorderAgenttest double (features/mocks/) that records start/end timestamps. The Then step asserts the two agents' execution intervals never overlap; this assertion currently fails (both start concurrently) while the bug is present.Tagged
@tdd_issue,@tdd_issue_97, and@tdd_expected_failper the project's TDD issue-capture workflow, sonox -s unit_testsstays green via the existingTddExpectedFailPolicyinversion hook. The actual fix is delivered separately by issue #97 on abugfix/branch.Closes #98
Test plan
nox -s lint— greennox -s format -- --check— greennox -s typecheck— green (Pyright strict, 0 errors)nox -s security_scan— greennox -s dead_code— greennox -s unit_tests— green (assertion fails while bug present, inverted to pass via@tdd_expected_fail)nox -s coverage_report— 96.7% (≥ 96.5% threshold)nox -s integration_tests— greenparallelflagPR Review: !109 (Ticket #98)
Verdict: Approve
This PR delivers the requested failing-first Behave regression capture for issue #97. The scenario is correctly tagged with
@tdd_issue @tdd_issue_97 @tdd_expected_fail, signals the bug exclusively viaAssertionError, and exercises theexecute()concurrent-dispatch site inpure_graph.py. The newTimingRecorderAgentmock is a clean, reusable test double. No critical or major correctness issues were found in the changed code. The few observations below are minor hygiene/robustness notes and a project-level coverage-threshold inconsistency that is not introduced by this PR.Critical Issues
None
Major Issues
None
Minor Issues
Coverage threshold mismatch (
noxfile.py/CONTRIBUTING.md)The PR test plan reports
coverage_reportat 96.7% and passes becausenoxfile.pyusesCOVERAGE_THRESHOLD = 96.5. However,CONTRIBUTING.mdstates the project’s enforced merge gate is 97%, and the CI workflow comment also says “fail-under 97%”. The ticket acceptance criteria likewise ask to verify>= 97%. This is a pre-existing project-level inconsistency, and the PR does not regress coverage (it only adds test/mock files, which are excluded from the slipcover source paths), but the documented gate is not met. Recommend reconcilingnoxfile.py/CONTRIBUTING.md/CI wording, or addressing coverage separately.Thenstep could fail withKeyErrorinstead ofAssertionError(features/steps/pure_graph_parallel_node_gate_steps.py:74-94)If a regression ever caused one of the two agents not to run at all,
intervals[left]orintervals[right]would raiseKeyError. The@tdd_expected_failhook only invertsAssertionError, so the scenario would fail CI with the wrong exception type. Consider adding an explicit guard such as:before the overlap assertion.
Out-of-scope note for the eventual fix (#97)
The scenario intentionally covers only the
execute()dispatch site; the twoexecute_stream()branches mentioned in issue #97 are correctly left for the bugfix PR. This is not a defect, just a scope marker.Nits
Unused context attributes (
features/steps/pure_graph_parallel_node_gate_steps.py:59-60)context.left_agent_nameandcontext.right_agent_nameare assigned but never referenced. They can be removed to reduce noise.Empty background step (
features/steps/pure_graph_parallel_node_gate_steps.py:25-27)The
Given a fresh pure graph test context (pg)step is a no-op. It is harmless, but since Behave provides a freshcontextper scenario anyway, the Background could be omitted entirely.Summary
A focused, well-documented TDD capture PR that matches the issue metadata, branch name, commit message, and tagging requirements. The test correctly reproduces the per-node
parallelflag violation fromdocs/index.md§6.7/§12.3, and the@tdd_expected_failinversion path is compliant with the project policy infeatures/tdd_expected_fail.py. Once the minor robustness guard is added (optional) and the project-level coverage threshold is reconciled elsewhere, this is ready for merge.Minor issues number 1 - coverage, is not relevant, we have a noxfile threshold at 96.5% while having a 97% in the specification, but the 97% is the rounded value, which is also obtainable from a 96.5% real coverage.
Minor issues number 3 - is actually very relevant and must be addressed, it is indeed part of this ticket to also address the streaming, The scenario covers only the execute() dispatch site; the two execute_stream() branches mentioned in issue #97 are incorrectly left for the bugfix PR. They must be addressed too.
9e2aebd3a72b2e0fbcf52b2e0fbcf5bce43626f8Thanks for the review, @hurui200320. Went through each point against
docs/index.md§6.7/§12.3 and issues #97/#98 before acting on it. Here's the disposition, item by item.Addressed
Minor —
Thenstep could fail withKeyErrorinstead ofAssertionError(pure_graph_parallel_node_gate_steps.py)Agreed, and added essentially the guard you proposed, before the overlap comparison:
Confirmed via
nox -s unit_teststhat both agents still record timing normally on every scenario, so this only changes behavior if a future regression stops one sibling from running at all — it now fails CI with a clear message instead of a bareKeyError.Minor — out-of-scope note on the two
execute_stream()branchesYou read the PR as submitted correctly — the single
execute()scenario does satisfy issue #98's acceptance criteria on its own, so this wasn't a defect in what was there. I'm overriding my own original scoping from the reply earlier in this thread, though: issue #98's background frames the bug as manifesting identically at all three dispatch sites inpure_graph.py, and@tdd_issue/@tdd_issue_97are meant to stand as permanent regression guards per the project's TDD workflow. Leaving two of the three sites uncovered would mean a future regression at eitherexecute_stream()branch has no permanent test catching it — only whatever the bugfix PR happens to add. So I've brought both into this PR's scope:execute_stream()'s non-AGENT branch (pure_graph.py:1965) — reuses the existing FUNCTION-trigger fixture, driven through the streaming entrypoint instead ofexecute().execute_stream()'s intermediate-AGENT branch (pure_graph.py:1860) — new fixture where the trigger node is itself an AGENT (a secondTimingRecorderAgent), since that branch is only reachable when the entry node is an intermediate (non-terminal) AGENT.Both new scenarios reproduce the bug (the assertion genuinely fails — verified the raw failure message before the
@tdd_expected_failinversion — then passes CI via the existing hook), same as the original scenario.Nit — unused
context.left_agent_name/context.right_agent_nameRemoved — they were dead weight, never read anywhere.
Nit — empty Background step
Removed the
Background:block and the corresponding no-opstep_fresh_contextstep definition, per your point that Behave already gives every scenario a freshcontext.Not changed
Minor — coverage threshold mismatch (
noxfile.py96.5% vsCONTRIBUTING.md/CI wording at 97%)Leaving this alone in this PR, per my earlier reply on this thread: it's a pre-existing project-level inconsistency that predates this PR and isn't regressed by it, and you reached the same conclusion independently ("recommend reconciling elsewhere / addressing coverage separately"). Reconciling
noxfile.pyagainstCONTRIBUTING.md/CI wording is a config/doc change orthogonal to a TDD issue-capture test and deserves its own issue rather than riding along here.Verification after these changes
nox -s lint,nox -s format -- --check,nox -s typecheck,nox -s dead_code,nox -s security_scan— all greennox -s unit_tests(full suite) — 146 features / 2914 scenarios / 13650 steps, 0 failuresnox -s coverage_report— 96.7%, unchanged from the original submission (≥ 96.5% threshold;pure_graph.pyitself at 98%, with all three dispatch-site lines now exercised)Pushed as an amendment to the same commit rather than a new one (
9e2aebd→2b2e0fb), so the issue link, tags, and everything else on this PR are unchanged. Ready for another look whenever you have time.