test(langgraph): capture parallel dispatch node-flag regression (#97) #109

Merged
CoreRasurae merged 1 commit from tdd/m1-pure-graph-parallel-node-gate into master 2026-08-05 23:21:24 +00:00
Member

Summary

Adds a failing-first Behave regression test proving issue #97: PureLangGraph's three concurrent-dispatch sites (pure_graph.py lines 1078, 1860, 1965) fire every candidate next-node concurrently via asyncio.gather/asyncio.create_task whenever the graph-level parallel_execution: true (the default) and there are 2+ candidates, without ever consulting each node's own parallel flag (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 shared TimingRecorderAgent test 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_fail per the project's TDD issue-capture workflow, so nox -s unit_tests stays green via the existing TddExpectedFailPolicy inversion hook. The actual fix is delivered separately by issue #97 on a bugfix/ branch.

Closes #98

Test plan

  • nox -s lint — green
  • nox -s format -- --check — green
  • nox -s typecheck — green (Pyright strict, 0 errors)
  • nox -s security_scan — green
  • nox -s dead_code — green
  • nox -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 — green
## Summary Adds a failing-first Behave regression test proving issue #97: `PureLangGraph`'s three concurrent-dispatch sites (`pure_graph.py` lines 1078, 1860, 1965) fire every candidate next-node concurrently via `asyncio.gather`/`asyncio.create_task` whenever the graph-level `parallel_execution: true` (the default) and there are 2+ candidates, without ever consulting each node's own `parallel` flag (`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 shared `TimingRecorderAgent` test 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_fail` per the project's TDD issue-capture workflow, so `nox -s unit_tests` stays green via the existing `TddExpectedFailPolicy` inversion hook. The actual fix is delivered separately by issue #97 on a `bugfix/` branch. Closes #98 ## Test plan - [x] `nox -s lint` — green - [x] `nox -s format -- --check` — green - [x] `nox -s typecheck` — green (Pyright strict, 0 errors) - [x] `nox -s security_scan` — green - [x] `nox -s dead_code` — green - [x] `nox -s unit_tests` — green (assertion fails while bug present, inverted to pass via `@tdd_expected_fail`) - [x] `nox -s coverage_report` — 96.7% (≥ 96.5% threshold) - [x] `nox -s integration_tests` — green
CoreRasurae added this to the v2.1.0 milestone 2026-08-04 20:11:47 +00:00
hurui200320 left a comment

PR 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 via AssertionError, and exercises the execute() concurrent-dispatch site in pure_graph.py. The new TimingRecorderAgent mock 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_report at 96.7% and passes because noxfile.py uses COVERAGE_THRESHOLD = 96.5. However, CONTRIBUTING.md states 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 reconciling noxfile.py/CONTRIBUTING.md/CI wording, or addressing coverage separately.

  • Then step could fail with KeyError instead of AssertionError (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] or intervals[right] would raise KeyError. The @tdd_expected_fail hook only inverts AssertionError, so the scenario would fail CI with the wrong exception type. Consider adding an explicit guard such as:

    assert left in intervals and right in intervals, "Expected both agents to execute"
    

    before the overlap assertion.

  • Out-of-scope note for the eventual fix (#97)
    The scenario intentionally covers only the execute() dispatch site; the two execute_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_name and context.right_agent_name are 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 fresh context per 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 parallel flag violation from docs/index.md §6.7/§12.3, and the @tdd_expected_fail inversion path is compliant with the project policy in features/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.

## PR 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 via `AssertionError`, and exercises the `execute()` concurrent-dispatch site in `pure_graph.py`. The new `TimingRecorderAgent` mock 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_report` at 96.7% and passes because `noxfile.py` uses `COVERAGE_THRESHOLD = 96.5`. However, `CONTRIBUTING.md` states 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 reconciling `noxfile.py`/`CONTRIBUTING.md`/CI wording, or addressing coverage separately. - **`Then` step could fail with `KeyError` instead of `AssertionError`** (`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]` or `intervals[right]` would raise `KeyError`. The `@tdd_expected_fail` hook only inverts `AssertionError`, so the scenario would fail CI with the wrong exception type. Consider adding an explicit guard such as: ```python assert left in intervals and right in intervals, "Expected both agents to execute" ``` before the overlap assertion. - **Out-of-scope note for the eventual fix (#97)** The scenario intentionally covers only the `execute()` dispatch site; the two `execute_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_name` and `context.right_agent_name` are 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 fresh `context` per 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 `parallel` flag violation from `docs/index.md` §6.7/§12.3, and the `@tdd_expected_fail` inversion path is compliant with the project policy in `features/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.
Author
Member

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.

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](https://git.cleverthis.com/cleveragents/cleveractors-core/issues/97) are incorrectly left for the bugfix PR. They must be addressed too.
CoreRasurae force-pushed tdd/m1-pure-graph-parallel-node-gate from 9e2aebd3a7
Some checks failed
CI / lint (pull_request) Successful in 1m1s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m27s
CI / quality (pull_request) Successful in 48s
CI / build (pull_request) Successful in 1m53s
CI / integration_tests (pull_request) Successful in 3m55s
CI / unit_tests (pull_request) Successful in 5m3s
CI / coverage (pull_request) Successful in 4m13s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Failing after 27m42s
to 2b2e0fbcf5
Some checks failed
CI / lint (pull_request) Successful in 1m1s
CI / typecheck (pull_request) Successful in 1m42s
CI / quality (pull_request) Successful in 1m23s
CI / security (pull_request) Successful in 1m54s
CI / build (pull_request) Successful in 36s
CI / coverage (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / unit_tests (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
2026-08-05 22:53:39 +00:00
Compare
test(langgraph): capture parallel dispatch node-flag regression (#97)
Some checks failed
CI / lint (pull_request) Successful in 45s
CI / quality (pull_request) Successful in 1m47s
CI / security (pull_request) Successful in 1m54s
CI / build (pull_request) Successful in 2m20s
CI / typecheck (pull_request) Successful in 2m54s
CI / integration_tests (pull_request) Successful in 4m57s
CI / unit_tests (pull_request) Successful in 5m23s
CI / benchmark (pull_request) Has been cancelled
CI / coverage (pull_request) Successful in 5m0s
CI / status-check (pull_request) Successful in 12s
CI / lint (push) Successful in 55s
CI / typecheck (push) Successful in 1m40s
CI / security (push) Successful in 1m38s
CI / quality (push) Successful in 1m50s
CI / build (push) Successful in 1m8s
CI / integration_tests (push) Successful in 4m24s
CI / unit_tests (push) Successful in 6m11s
CI / coverage (push) Successful in 5m8s
CI / status-check (push) Successful in 9s
CI / benchmark (push) Failing after 18m30s
bce43626f8
Adds a failing-first Behave regression test proving issue #97:
PureLangGraph fires every candidate next-node concurrently via
asyncio.gather/asyncio.create_task whenever the graph-level
parallel_execution flag is true (the default) and there are 2+
candidates, without ever consulting each node's own `parallel` flag
(NodeConfig.parallel / can_execute_parallel()). This violates Actor
Configuration Standard §6.7 and §12.3, which require that only the
subset of next-nodes with parallel: true run concurrently while the
rest run sequentially.

Three scenarios cover all three concurrent-dispatch sites named in
issue #97, each driving a shared TimingRecorderAgent test double
(features/mocks/) that records start/end timestamps and asserting
the two sibling agents' execution intervals never overlap:

- execute() (line 1078), via a non-agent FUNCTION trigger node.
- execute_stream()'s non-AGENT branch (line 1965), via the same
  FUNCTION trigger node, driven through the streaming entrypoint.
- execute_stream()'s intermediate-AGENT branch (line 1860), via an
  AGENT trigger node with two further sibling AGENT edges.

Each assertion currently fails (both siblings start concurrently)
while the bug is present. Tagged @tdd_issue, @tdd_issue_97, and
@tdd_expected_fail per the project's TDD issue-capture workflow, so
nox -s unit_tests stays green via the existing TddExpectedFailPolicy
inversion hook. The fix itself is delivered separately by issue #97
on a bugfix/ branch.

Also hardens the shared Then step to fail with a clear AssertionError
(rather than a raw KeyError) if either sibling agent recorded no
timing at all, and drops two unused test-only attributes and a no-op
Background step.

ISSUES CLOSED: #98
CoreRasurae force-pushed tdd/m1-pure-graph-parallel-node-gate from 2b2e0fbcf5
Some checks failed
CI / lint (pull_request) Successful in 1m1s
CI / typecheck (pull_request) Successful in 1m42s
CI / quality (pull_request) Successful in 1m23s
CI / security (pull_request) Successful in 1m54s
CI / build (pull_request) Successful in 36s
CI / coverage (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / unit_tests (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
to bce43626f8
Some checks failed
CI / lint (pull_request) Successful in 45s
CI / quality (pull_request) Successful in 1m47s
CI / security (pull_request) Successful in 1m54s
CI / build (pull_request) Successful in 2m20s
CI / typecheck (pull_request) Successful in 2m54s
CI / integration_tests (pull_request) Successful in 4m57s
CI / unit_tests (pull_request) Successful in 5m23s
CI / benchmark (pull_request) Has been cancelled
CI / coverage (pull_request) Successful in 5m0s
CI / status-check (pull_request) Successful in 12s
CI / lint (push) Successful in 55s
CI / typecheck (push) Successful in 1m40s
CI / security (push) Successful in 1m38s
CI / quality (push) Successful in 1m50s
CI / build (push) Successful in 1m8s
CI / integration_tests (push) Successful in 4m24s
CI / unit_tests (push) Successful in 6m11s
CI / coverage (push) Successful in 5m8s
CI / status-check (push) Successful in 9s
CI / benchmark (push) Failing after 18m30s
2026-08-05 22:58:46 +00:00
Compare
Author
Member

Thanks 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 — Then step could fail with KeyError instead of AssertionError (pure_graph_parallel_node_gate_steps.py)

Agreed, and added essentially the guard you proposed, before the overlap comparison:

assert left in intervals and right in intervals, (
    f"Expected both {left!r} and {right!r} to have executed and recorded "
    f"timing, but only recorded intervals for: {sorted(intervals)}"
)

Confirmed via nox -s unit_tests that 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 bare KeyError.

Minor — out-of-scope note on the two execute_stream() branches

You 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 in pure_graph.py, and @tdd_issue/@tdd_issue_97 are 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 either execute_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 of execute().
  • execute_stream()'s intermediate-AGENT branch (pure_graph.py:1860) — new fixture where the trigger node is itself an AGENT (a second TimingRecorderAgent), 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_fail inversion — then passes CI via the existing hook), same as the original scenario.

Nit — unused context.left_agent_name / context.right_agent_name

Removed — they were dead weight, never read anywhere.

Nit — empty Background step

Removed the Background: block and the corresponding no-op step_fresh_context step definition, per your point that Behave already gives every scenario a fresh context.

Not changed

Minor — coverage threshold mismatch (noxfile.py 96.5% vs CONTRIBUTING.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.py against CONTRIBUTING.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 green
  • nox -s unit_tests (full suite) — 146 features / 2914 scenarios / 13650 steps, 0 failures
  • nox -s coverage_report — 96.7%, unchanged from the original submission (≥ 96.5% threshold; pure_graph.py itself at 98%, with all three dispatch-site lines now exercised)

Pushed as an amendment to the same commit rather than a new one (9e2aebd2b2e0fb), so the issue link, tags, and everything else on this PR are unchanged. Ready for another look whenever you have time.

Thanks 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 — `Then` step could fail with `KeyError` instead of `AssertionError`** (`pure_graph_parallel_node_gate_steps.py`) Agreed, and added essentially the guard you proposed, before the overlap comparison: ```python assert left in intervals and right in intervals, ( f"Expected both {left!r} and {right!r} to have executed and recorded " f"timing, but only recorded intervals for: {sorted(intervals)}" ) ``` Confirmed via `nox -s unit_tests` that 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 bare `KeyError`. **Minor — out-of-scope note on the two `execute_stream()` branches** You 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 in `pure_graph.py`, and `@tdd_issue`/`@tdd_issue_97` are 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 either `execute_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 of `execute()`. - `execute_stream()`'s intermediate-AGENT branch (`pure_graph.py:1860`) — new fixture where the trigger node is itself an AGENT (a second `TimingRecorderAgent`), 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_fail` inversion — then passes CI via the existing hook), same as the original scenario. **Nit — unused `context.left_agent_name` / `context.right_agent_name`** Removed — they were dead weight, never read anywhere. **Nit — empty Background step** Removed the `Background:` block and the corresponding no-op `step_fresh_context` step definition, per your point that Behave already gives every scenario a fresh `context`. ### Not changed **Minor — coverage threshold mismatch (`noxfile.py` 96.5% vs `CONTRIBUTING.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.py` against `CONTRIBUTING.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 green - `nox -s unit_tests` (full suite) — 146 features / 2914 scenarios / 13650 steps, 0 failures - `nox -s coverage_report` — 96.7%, unchanged from the original submission (≥ 96.5% threshold; `pure_graph.py` itself 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.
CoreRasurae deleted branch tdd/m1-pure-graph-parallel-node-gate 2026-08-05 23:21:35 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!109
No description provided.