test(langgraph): capture content_not_contains edge condition regression (#95) #99

Merged
CoreRasurae merged 1 commits from tdd/m1-pure-graph-content-not-contains into master 2026-08-04 10:07:51 +00:00
Member

Summary

Adds a failing-first Behave regression test proving issue #95: PureLangGraph._evaluate_edge_condition() has no branch for content_not_contains (Actor Configuration Standard §5.4), so it falls into the "unknown condition type" catch-all and always returns True. When a classifier node has sibling content_contains/content_not_contains edges (the classic if/else idiom), both fire whenever the configured text is present, so _get_next_nodes() returns two targets instead of one.

The scenario is tagged @tdd_issue, @tdd_issue_95, and @tdd_expected_fail per the TDD issue-capture workflow.

No runtime hook previously existed in this repo to make @tdd_expected_fail actually invert pass/fail, so this PR also adds TddExpectedFailPolicy (features/tdd_expected_fail.py) wired into features/environment.py's after_step/after_scenario hooks:

  • Flips an @tdd_expected_fail scenario's AssertionError failures to passing (proving the bug is still present).
  • Fails the scenario if it ever passes outright without an inversion (catches a stale tag left after the bug was actually fixed).
  • Never inverts non-AssertionError failures.

The actual fix lands separately on bugfix/m1-pure-graph-content-not-contains per issue #95.

Test plan

  • nox -s lint green
  • nox -s format -- --check green
  • nox -s typecheck green
  • nox -s security_scan green
  • nox -s dead_code green
  • nox -s unit_tests green (2790 scenarios, including the new tagged scenario) — confirms the hook inverts the expected AssertionError failure
  • nox -s coverage_report — 96.8% (>= 96.5% threshold)

Closes #96

## Summary Adds a failing-first Behave regression test proving issue #95: `PureLangGraph._evaluate_edge_condition()` has no branch for `content_not_contains` (Actor Configuration Standard §5.4), so it falls into the "unknown condition type" catch-all and always returns `True`. When a classifier node has sibling `content_contains`/`content_not_contains` edges (the classic if/else idiom), both fire whenever the configured text is present, so `_get_next_nodes()` returns two targets instead of one. The scenario is tagged `@tdd_issue`, `@tdd_issue_95`, and `@tdd_expected_fail` per the TDD issue-capture workflow. No runtime hook previously existed in this repo to make `@tdd_expected_fail` actually invert pass/fail, so this PR also adds `TddExpectedFailPolicy` (`features/tdd_expected_fail.py`) wired into `features/environment.py`'s `after_step`/`after_scenario` hooks: - Flips an `@tdd_expected_fail` scenario's `AssertionError` failures to passing (proving the bug is still present). - Fails the scenario if it ever passes outright without an inversion (catches a stale tag left after the bug was actually fixed). - Never inverts non-`AssertionError` failures. The actual fix lands separately on `bugfix/m1-pure-graph-content-not-contains` per issue #95. ## Test plan - [x] `nox -s lint` green - [x] `nox -s format -- --check` green - [x] `nox -s typecheck` green - [x] `nox -s security_scan` green - [x] `nox -s dead_code` green - [x] `nox -s unit_tests` green (2790 scenarios, including the new tagged scenario) — confirms the hook inverts the expected AssertionError failure - [x] `nox -s coverage_report` — 96.8% (>= 96.5% threshold) Closes #96
CoreRasurae added this to the v2.1.0 milestone 2026-08-03 22:26:43 +00:00
CoreRasurae added the
Type
Testing
label 2026-08-03 22:28:16 +00:00
CoreRasurae closed this pull request 2026-08-03 22:35:14 +00:00
CoreRasurae reopened this pull request 2026-08-03 22:35:22 +00:00
Author
Member

LGTM — self-approving per project policy (1 approval required, self-approval permitted). All quality gates verified locally: nox -s lint, nox -s format -- --check, nox -s typecheck, nox -s security_scan, nox -s dead_code, nox -s unit_tests (2790/2790 scenarios green, including the new @tdd_issue_95/@tdd_expected_fail regression scenario), and nox -s coverage_report (96.8%, above the 96.5% threshold) all pass. Forgejo CI is currently broken with systemic infra failures (install-step failures across unrelated commits/branches) unrelated to this change — proceeding per maintainer confirmation to disregard CI for this session.

LGTM ✅ — self-approving per project policy (1 approval required, self-approval permitted). All quality gates verified locally: `nox -s lint`, `nox -s format -- --check`, `nox -s typecheck`, `nox -s security_scan`, `nox -s dead_code`, `nox -s unit_tests` (2790/2790 scenarios green, including the new `@tdd_issue_95`/`@tdd_expected_fail` regression scenario), and `nox -s coverage_report` (96.8%, above the 96.5% threshold) all pass. Forgejo CI is currently broken with systemic infra failures (install-step failures across unrelated commits/branches) unrelated to this change — proceeding per maintainer confirmation to disregard CI for this session.
hurui200320 requested changes 2026-08-04 05:03:55 +00:00
Dismissed
hurui200320 left a comment
Member

PR Review: !99 (Ticket #96)

Verdict: Request Changes

The TDD test and @tdd_expected_fail hook are well-implemented and follow the TDD tagging workflow, but the scenario asserts behavior that the current Actor Configuration Standard explicitly says pure-graph edges should not implement. Merging a test that contradicts the authoritative spec creates a spec/code inconsistency on master. This needs to be resolved before the PR can land.

Critical Issues

None

Major Issues

  1. Spec compliance gap: test asserts unsupported pure-graph edge condition
    • File: features/pure_graph_content_not_contains.feature, lines 18–23
    • Problem: The scenario asserts that a pure-graph content_not_contains edge evaluates based on message content. However, docs/index.md §5.4 lists content_not_contains as honored only by "Stream router, conditional nodes, bridge router"pure-graph edges are not included. §5.4.1 further states that for pure-graph edge conditions an unknown type defaults to True (with a warning), and §5.4.2 says a condition declared in a subsystem that does not honor it must follow that subsystem's default behavior. Under the current spec, the existing code in src/cleveractors/langgraph/pure_graph.py is therefore spec-compliant, and the test is asserting behavior the spec explicitly says should not exist.
    • Recommendation: Update docs/index.md §5.4 to add pure-graph edges to the "Honored By" column for content_not_contains (and ensure content_contains already covers them). Do this either in this PR or in the companion bugfix/m1-pure-graph-content-not-contains PR with a clear cross-reference. Until the spec is updated, the TDD test documents behavior that contradicts the source of truth.

Minor Issues

  1. TddExpectedFailPolicy does not enforce required tag-combination rules
    • File: features/tdd_expected_fail.py, lines 40–42 and 75–76
    • Problem: CONTRIBUTING.md §1216–1223 states that the test-environment hooks enforce: (a) any scenario with @tdd_issue_<N> must also have @tdd_issue, and (b) any scenario with @tdd_expected_fail must also have @tdd_issue and at least one @tdd_issue_<N>. The current policy only checks for @tdd_expected_fail and never validates the presence of the companion tags. A malformed tag combination would silently invert failures instead of failing CI with a clear tagging error.
    • Recommendation: Add validation in on_scenario (or a dedicated helper) that raises AssertionError when @tdd_expected_fail is present but @tdd_issue or @tdd_issue_<N> is missing.

Nits

  1. Unused context attribute

    • File: features/steps/pure_graph_content_not_contains_steps.py, line 22
    • context.results = {} is assigned in the background step but never read. It can be removed or replaced with a comment explaining the reset.
  2. Import placement

    • File: features/environment.py, line 12
    • The new from features.tdd_expected_fail import TddExpectedFailPolicy import sits between the module docstring and the sys.setrecursionlimit comment. Moving the comment above the imports (or grouping the import with the other imports) would improve readability.

Summary

This PR correctly implements the TDD issue-capture mechanics: the scenario is tagged @tdd_issue @tdd_issue_95 @tdd_expected_fail, it signals the bug via AssertionError, and the new TddExpectedFailPolicy hook inverts expected failures while guarding against stale tags. The hook logic itself is sound (verified that raising in after_scenario fails the scenario and that context.scenario is available in after_step).

The blocker is specification alignment. Because the current spec says content_not_contains is not honored by pure-graph edges, this regression test captures behavior that the spec currently defines as unsupported. Fix the spec first (or concurrently), then this PR is good to merge. The tag-validation gap in the hook should also be addressed so the new infrastructure fully enforces the TDD rules documented in CONTRIBUTING.md.

(Note: CI failures are acknowledged as broken infrastructure and are excluded from this review per maintainer direction.)

## PR Review: !99 (Ticket #96) ### Verdict: Request Changes The TDD test and `@tdd_expected_fail` hook are well-implemented and follow the TDD tagging workflow, but the scenario asserts behavior that the current Actor Configuration Standard explicitly says pure-graph edges should **not** implement. Merging a test that contradicts the authoritative spec creates a spec/code inconsistency on `master`. This needs to be resolved before the PR can land. ### Critical Issues None ### Major Issues 1. **Spec compliance gap: test asserts unsupported pure-graph edge condition** - **File:** `features/pure_graph_content_not_contains.feature`, lines 18–23 - **Problem:** The scenario asserts that a pure-graph `content_not_contains` edge evaluates based on message content. However, `docs/index.md` §5.4 lists `content_not_contains` as honored only by *"Stream router, conditional nodes, bridge router"* — **pure-graph edges are not included**. §5.4.1 further states that for pure-graph edge conditions an unknown type defaults to `True` (with a warning), and §5.4.2 says a condition declared in a subsystem that does not honor it **must** follow that subsystem's default behavior. Under the current spec, the existing code in `src/cleveractors/langgraph/pure_graph.py` is therefore spec-compliant, and the test is asserting behavior the spec explicitly says should not exist. - **Recommendation:** Update `docs/index.md` §5.4 to add pure-graph edges to the "Honored By" column for `content_not_contains` (and ensure `content_contains` already covers them). Do this either in this PR or in the companion `bugfix/m1-pure-graph-content-not-contains` PR with a clear cross-reference. Until the spec is updated, the TDD test documents behavior that contradicts the source of truth. ### Minor Issues 1. **TddExpectedFailPolicy does not enforce required tag-combination rules** - **File:** `features/tdd_expected_fail.py`, lines 40–42 and 75–76 - **Problem:** `CONTRIBUTING.md` §1216–1223 states that the test-environment hooks enforce: (a) any scenario with `@tdd_issue_<N>` must also have `@tdd_issue`, and (b) any scenario with `@tdd_expected_fail` must also have `@tdd_issue` and at least one `@tdd_issue_<N>`. The current policy only checks for `@tdd_expected_fail` and never validates the presence of the companion tags. A malformed tag combination would silently invert failures instead of failing CI with a clear tagging error. - **Recommendation:** Add validation in `on_scenario` (or a dedicated helper) that raises `AssertionError` when `@tdd_expected_fail` is present but `@tdd_issue` or `@tdd_issue_<N>` is missing. ### Nits 1. **Unused context attribute** - **File:** `features/steps/pure_graph_content_not_contains_steps.py`, line 22 - `context.results = {}` is assigned in the background step but never read. It can be removed or replaced with a comment explaining the reset. 2. **Import placement** - **File:** `features/environment.py`, line 12 - The new `from features.tdd_expected_fail import TddExpectedFailPolicy` import sits between the module docstring and the `sys.setrecursionlimit` comment. Moving the comment above the imports (or grouping the import with the other imports) would improve readability. ### Summary This PR correctly implements the TDD issue-capture mechanics: the scenario is tagged `@tdd_issue @tdd_issue_95 @tdd_expected_fail`, it signals the bug via `AssertionError`, and the new `TddExpectedFailPolicy` hook inverts expected failures while guarding against stale tags. The hook logic itself is sound (verified that raising in `after_scenario` fails the scenario and that `context.scenario` is available in `after_step`). The blocker is specification alignment. Because the current spec says `content_not_contains` is **not** honored by pure-graph edges, this regression test captures behavior that the spec currently defines as unsupported. Fix the spec first (or concurrently), then this PR is good to merge. The tag-validation gap in the hook should also be addressed so the new infrastructure fully enforces the TDD rules documented in `CONTRIBUTING.md`. (Note: CI failures are acknowledged as broken infrastructure and are excluded from this review per maintainer direction.)
CoreRasurae force-pushed tdd/m1-pure-graph-content-not-contains from 7dab619428 to 89fab68bf7 2026-08-04 08:19:01 +00:00 Compare
CoreRasurae force-pushed tdd/m1-pure-graph-content-not-contains from 89fab68bf7 to d219e6943f 2026-08-04 08:51:51 +00:00 Compare
hurui200320 approved these changes 2026-08-04 09:10:20 +00:00
hurui200320 left a comment
Member

PR Review: !99 (Ticket #96)

Verdict: Approve

The TDD-capture PR correctly implements the failing-first regression test for issue #95 and the @tdd_expected_fail inversion hook. The code is well-structured, type-safe, and aligned with the TDD workflow in CONTRIBUTING.md. All previously raised spec-compliance and tag-validation concerns have been addressed. No critical or major code issues remain.

Critical Issues

None

Major Issues

None

Minor Issues

  1. TddExpectedFailPolicy edge cases are not directly exercised
    • File: features/tdd_expected_fail.py
    • Problem: The new hook has several important branches that are not covered by dedicated scenarios: stale-tag detection (scenario passes without any inverted AssertionError), malformed tag-combination validation, and the rule that non-AssertionError failures are never inverted. Only the happy-path inversion is exercised by the new @tdd_issue_95 scenario. If any of these edge-case branches regress, the TDD workflow could silently break.
    • Recommendation: Add a small set of Behave scenarios (or unit-style assertions invoked from a feature) that deliberately trigger each branch and assert the expected outcome. At minimum, verify that a non-AssertionError failure in an @tdd_expected_fail scenario still fails, and that removing @tdd_expected_fail after the bug is fixed causes the stale-tag assertion to fire.

Nits

None

Summary

This PR delivers exactly what issue #96 asks for: a failing-first Behave regression test proving the content_not_contains gap in PureLangGraph._evaluate_edge_condition(), with the correct @tdd_issue @tdd_issue_95 @tdd_expected_fail tagging, and a reusable TddExpectedFailPolicy hook wired into features/environment.py. The step definitions are fully implemented and signal the bug only via AssertionError. The spec alignment work in docs/index.md and the ADR resolve the earlier reviewer concern that the test contradicted the standard.

The only remaining concern is the lack of direct tests for the hook's guard rails. Because those guard rails are what make the TDD workflow trustworthy, they merit a few targeted scenarios. Once those are added (here or as a fast-follow infrastructure test), this is ready to merge.

## PR Review: !99 (Ticket #96) ### Verdict: Approve The TDD-capture PR correctly implements the failing-first regression test for issue #95 and the `@tdd_expected_fail` inversion hook. The code is well-structured, type-safe, and aligned with the TDD workflow in `CONTRIBUTING.md`. All previously raised spec-compliance and tag-validation concerns have been addressed. No critical or major code issues remain. ### Critical Issues None ### Major Issues None ### Minor Issues 1. **TddExpectedFailPolicy edge cases are not directly exercised** - **File:** `features/tdd_expected_fail.py` - **Problem:** The new hook has several important branches that are not covered by dedicated scenarios: stale-tag detection (scenario passes without any inverted `AssertionError`), malformed tag-combination validation, and the rule that non-`AssertionError` failures are never inverted. Only the happy-path inversion is exercised by the new `@tdd_issue_95` scenario. If any of these edge-case branches regress, the TDD workflow could silently break. - **Recommendation:** Add a small set of Behave scenarios (or unit-style assertions invoked from a feature) that deliberately trigger each branch and assert the expected outcome. At minimum, verify that a non-`AssertionError` failure in an `@tdd_expected_fail` scenario still fails, and that removing `@tdd_expected_fail` after the bug is fixed causes the stale-tag assertion to fire. ### Nits None ### Summary This PR delivers exactly what issue #96 asks for: a failing-first Behave regression test proving the `content_not_contains` gap in `PureLangGraph._evaluate_edge_condition()`, with the correct `@tdd_issue @tdd_issue_95 @tdd_expected_fail` tagging, and a reusable `TddExpectedFailPolicy` hook wired into `features/environment.py`. The step definitions are fully implemented and signal the bug only via `AssertionError`. The spec alignment work in `docs/index.md` and the ADR resolve the earlier reviewer concern that the test contradicted the standard. The only remaining concern is the lack of direct tests for the hook's guard rails. Because those guard rails are what make the TDD workflow trustworthy, they merit a few targeted scenarios. Once those are added (here or as a fast-follow infrastructure test), this is ready to merge.
CoreRasurae force-pushed tdd/m1-pure-graph-content-not-contains from d219e6943f to 95f4ee1db3 2026-08-04 09:28:41 +00:00 Compare
CoreRasurae added 1 commit 2026-08-04 09:49:56 +00:00
test(langgraph): capture content_not_contains edge condition regression (#95)
CI / lint (pull_request) Successful in 54s
CI / typecheck (pull_request) Successful in 1m48s
CI / security (pull_request) Successful in 1m20s
CI / quality (pull_request) Successful in 1m17s
CI / build (pull_request) Successful in 1m53s
CI / integration_tests (pull_request) Successful in 4m31s
CI / unit_tests (pull_request) Successful in 5m45s
CI / coverage (pull_request) Successful in 5m3s
CI / status-check (pull_request) Successful in 6s
CI / security (push) Successful in 1m22s
CI / quality (push) Successful in 1m55s
CI / build (push) Successful in 2m9s
CI / benchmark (pull_request) Failing after 21m27s
CI / integration_tests (push) Successful in 4m15s
CI / unit_tests (push) Successful in 5m59s
CI / typecheck (push) Failing after 15m5s
CI / lint (push) Failing after 15m13s
CI / coverage (push) Has been skipped
CI / status-check (push) Failing after 13s
CI / benchmark (push) Successful in 22m57s
46bff0d451
Adds a failing-first Behave regression test proving issue #95:
PureLangGraph._evaluate_edge_condition() has no branch for
content_not_contains (Actor Configuration Standard §5.4), so it falls
into the "unknown condition type" catch-all and always returns True.
When a classifier node has sibling content_contains/content_not_contains
edges (the classic if/else idiom), both fire whenever the configured
text is present, so _get_next_nodes() returns two targets instead of
one.

The scenario is tagged @tdd_issue, @tdd_issue_95, and
@tdd_expected_fail per the project's TDD issue-capture workflow. No
runtime hook previously existed to make @tdd_expected_fail meaningful,
so this commit also adds TddExpectedFailPolicy (features/tdd_expected_fail.py)
and wires it into features/environment.py's after_step/after_scenario
hooks: it flips an @tdd_expected_fail scenario's AssertionError failures
to passing (proving the bug is still present), and fails the scenario
if it ever passes outright without an inversion (catching a stale tag
left after the bug was actually fixed). Non-AssertionError failures are
never inverted.

The actual fix lands separately on bugfix/m1-pure-graph-content-not-contains
per issue #95.

Addresses PR #99 reviews (rui.hu):

- Major: the scenario's premise conflicted with §5.4's Honored-By table,
  which listed content_not_contains as honored only by "Stream router,
  conditional nodes, bridge router" - excluding pure-graph edges. A full
  read of the spec confirmed this was a real, consistent restriction
  (every content_not_contains example was a stream-router split; the
  spec's only graph-edge branching example used two content_contains
  conditions, not a contains/not-contains pair), not an obvious
  omission. Per maintainer decision, the original intent was always
  symmetric with content_contains ("All subsystems"), so this commit
  adds docs/adr/ADR-2034-content-not-contains-all-subsystems.md and
  revises docs/index.md to v1.1.0: §5.4 Honored By extended to "All
  subsystems" for content_not_contains, plus a new §21.1 Revision
  History section.
- Minor: TddExpectedFailPolicy now enforces the CONTRIBUTING.md
  tag-combination rules (@tdd_issue_<N> requires @tdd_issue;
  @tdd_expected_fail requires both @tdd_issue and a @tdd_issue_<N>),
  raising AssertionError on a malformed combination instead of silently
  under-validating the scenario.
- Nits: removed the unused context.results assignment in the
  background step; reordered environment.py so the recursion-limit
  comment/call no longer sits beside the new first-party import as if
  commenting on it.
- Minor: TddExpectedFailPolicy's guard-rail branches - stale-tag
  detection, malformed tag-combination validation, and non-AssertionError
  failures never being inverted - were only exercised indirectly through
  the single happy-path @tdd_issue_95 scenario. Adds
  features/unit_tdd_expected_fail_policy.feature (7 scenarios) and its
  step definitions, which construct behave.model.Scenario/Step objects
  directly and invoke TddExpectedFailPolicy's classmethods against a
  throwaway context, so each guard-rail branch is exercised in isolation
  without depending on a real scenario's outcome (a scenario tagged to
  deliberately go stale or malformed would otherwise fail the real
  suite). Kept in this same commit rather than deferred to a fast-follow
  PR, since none of this has merged to master yet.

All fixes are folded into this same commit rather than split across
PRs, since none of this has merged to master yet - it is one feature
still being iterated on.

Refs: #95, #96
CoreRasurae force-pushed tdd/m1-pure-graph-content-not-contains from 95f4ee1db3 to 46bff0d451 2026-08-04 09:49:56 +00:00 Compare
CoreRasurae merged commit 46bff0d451 into master 2026-08-04 10:07:51 +00:00
CoreRasurae deleted branch tdd/m1-pure-graph-content-not-contains 2026-08-04 10:08:03 +00:00
Sign in to join this conversation.
No Reviewers
No Label
Type
Testing
2 Participants
Notifications
Due Date
No due date set.
Reference: cleveragents/cleveractors-core#99