cleveragents/cleveragents-core

Fork 3

Bug: Redundant scenario.clear_status() call in apply_tdd_inversion creates a fragile _cached_status=untested window #8295

New issue

Open

opened 2026-04-13 08:11:03 +00:00 by hurui200320 · 1 comment

hurui200320 commented

2026-04-13 08:11:03 +00:00

Member

Metadata

Commit Message: fix(testing): remove redundant clear_status() call in apply_tdd_inversion expected-failure path
Branch: fix/tdd-inversion-redundant-clear-status

Background and Context

In apply_tdd_inversion() in features/environment.py, the expected-failure inversion path (when failed=True) ends with:

scenario.clear_status()            # ← line 218: sets _cached_status = Status.untested
scenario.set_status(Status.passed) # ← line 219: sets _cached_status = Status.passed
return False

The clear_status() call on line 218 is unnecessary. set_status(Status.passed) on line 219 unconditionally overwrites _cached_status regardless of its prior value. The clear_status() call serves no purpose here — it only creates a brief window where _cached_status = Status.untested before being immediately overwritten.

This is inconsistent with the unexpected-pass path in the same function (line 239), which correctly calls only scenario.set_status(Status.failed) without a preceding clear_status().

Current Behavior

The clear_status() call sets _cached_status = Status.untested. If anything reads scenario.status between line 218 and line 219 (e.g., a formatter, a hook, or a concurrent access in a future parallel execution model), compute_status() is triggered. In the presence of Status.untested steps (see companion issue #8291), compute_status() returns Status.untested instead of Status.passed, leaving _cached_status in a perpetual recomputation loop.

Proof:

from behave.model import Scenario, Step, Status
from features.environment import apply_tdd_inversion

s = Scenario('<test>', 1, 'Scenario', 'test',
             tags=['tdd_expected_fail', 'tdd_issue', 'tdd_issue_999'])
s.hook_failed = False
s.was_dry_run = False
step = Step('<test>', 1, 'When', 'when', 'failing step')
step.status = Status.failed
step.exception = AssertionError('bug')
s.steps.append(step)
s.set_status(Status.failed)

apply_tdd_inversion(s, True)

# After inversion, _cached_status = Status.passed (correct).
# But if clear_status() is called (as it is on line 218 before set_status):
s.clear_status()
# _cached_status = Status.untested — compute_status() will be triggered on next access.
# With Status.untested steps (issue #8291), compute_status() returns Status.untested.
# This is the fragile window created by the redundant clear_status() call.

The clear_status() call is a Behave idiom used before compute_status() is expected to run (e.g., at the start of Scenario.run() to force recomputation). It is not appropriate here, where we are explicitly setting a final status and do not want recomputation.

Expected Behavior

The scenario.clear_status() call on line 218 should be removed. scenario.set_status(Status.passed) alone is sufficient and correct. This matches the pattern used in the unexpected-pass path (line 239) and eliminates the fragile _cached_status = untested window entirely.

The fix is a one-line deletion.

Acceptance Criteria

The scenario.clear_status() call on line 218 of features/environment.py is removed.
The unexpected-pass path (line 239) is verified to already not have a clear_status() call (it should not — this is confirming consistency).
All existing Behave scenarios in features/tdd_expected_fail_infrastructure.feature and features/testing/tdd_tag_validation.feature continue to pass.
A new Behave scenario is added to features/tdd_expected_fail_infrastructure.feature verifying that after inversion, scenario.status returns Status.passed even if scenario.clear_status() is called externally (i.e., all steps are properly reset so compute_status() returns Status.passed).
Coverage remains ≥ 97%.

Supporting Information

File: features/environment.py, function apply_tdd_inversion, line 218.
Behave internals: TagAndStatusStatement.clear_status() sets _cached_status = Status.untested. TagAndStatusStatement.set_status(value) sets _cached_status = value directly. TagAndStatusStatement.status (property) returns _cached_status if it is in final_status = (Status.passed, Status.failed, Status.skipped), otherwise calls compute_status(). Status.untested is NOT in final_status, so it always triggers recomputation.
Why clear_status() is used elsewhere: In Scenario.run(), clear_status() is called before the after-scenario hook to force compute_status() to recompute from step statuses. That is the correct use. In apply_tdd_inversion, we are setting a definitive final status — clear_status() is wrong here.
Companion issues: #8291 (untested-step reset gap, which this issue amplifies) and #8294 (non-AssertionError guard, the primary flakiness cause).

Subtasks

Remove scenario.clear_status() on line 218 of features/environment.py
Verify the unexpected-pass path (line 239) does not have a clear_status() call
Add a Behave scenario to features/tdd_expected_fail_infrastructure.feature: "apply_tdd_inversion scenario status remains passed after external clear_status call"
Add step definitions for the new scenario in features/steps/tdd_expected_fail_infrastructure_steps.py
Verify nox -e unit_tests passes
Verify coverage ≥ 97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `fix(testing): remove redundant clear_status() call in apply_tdd_inversion expected-failure path` - **Branch**: `fix/tdd-inversion-redundant-clear-status` ## Background and Context In `apply_tdd_inversion()` in `features/environment.py`, the expected-failure inversion path (when `failed=True`) ends with: ```python scenario.clear_status() # ← line 218: sets _cached_status = Status.untested scenario.set_status(Status.passed) # ← line 219: sets _cached_status = Status.passed return False ``` The `clear_status()` call on line 218 is **unnecessary**. `set_status(Status.passed)` on line 219 unconditionally overwrites `_cached_status` regardless of its prior value. The `clear_status()` call serves no purpose here — it only creates a brief window where `_cached_status = Status.untested` before being immediately overwritten. This is inconsistent with the unexpected-pass path in the same function (line 239), which correctly calls only `scenario.set_status(Status.failed)` without a preceding `clear_status()`. ## Current Behavior The `clear_status()` call sets `_cached_status = Status.untested`. If anything reads `scenario.status` between line 218 and line 219 (e.g., a formatter, a hook, or a concurrent access in a future parallel execution model), `compute_status()` is triggered. In the presence of `Status.untested` steps (see companion issue #8291), `compute_status()` returns `Status.untested` instead of `Status.passed`, leaving `_cached_status` in a perpetual recomputation loop. **Proof:** ```python from behave.model import Scenario, Step, Status from features.environment import apply_tdd_inversion s = Scenario('<test>', 1, 'Scenario', 'test', tags=['tdd_expected_fail', 'tdd_issue', 'tdd_issue_999']) s.hook_failed = False s.was_dry_run = False step = Step('<test>', 1, 'When', 'when', 'failing step') step.status = Status.failed step.exception = AssertionError('bug') s.steps.append(step) s.set_status(Status.failed) apply_tdd_inversion(s, True) # After inversion, _cached_status = Status.passed (correct). # But if clear_status() is called (as it is on line 218 before set_status): s.clear_status() # _cached_status = Status.untested — compute_status() will be triggered on next access. # With Status.untested steps (issue #8291), compute_status() returns Status.untested. # This is the fragile window created by the redundant clear_status() call. ``` The `clear_status()` call is a Behave idiom used before `compute_status()` is expected to run (e.g., at the start of `Scenario.run()` to force recomputation). It is **not** appropriate here, where we are explicitly setting a final status and do not want recomputation. ## Expected Behavior The `scenario.clear_status()` call on line 218 should be removed. `scenario.set_status(Status.passed)` alone is sufficient and correct. This matches the pattern used in the unexpected-pass path (line 239) and eliminates the fragile `_cached_status = untested` window entirely. The fix is a one-line deletion. ## Acceptance Criteria - [ ] The `scenario.clear_status()` call on line 218 of `features/environment.py` is removed. - [ ] The unexpected-pass path (line 239) is verified to already not have a `clear_status()` call (it should not — this is confirming consistency). - [ ] All existing Behave scenarios in `features/tdd_expected_fail_infrastructure.feature` and `features/testing/tdd_tag_validation.feature` continue to pass. - [ ] A new Behave scenario is added to `features/tdd_expected_fail_infrastructure.feature` verifying that after inversion, `scenario.status` returns `Status.passed` even if `scenario.clear_status()` is called externally (i.e., all steps are properly reset so `compute_status()` returns `Status.passed`). - [ ] Coverage remains ≥ 97%. ## Supporting Information - **File**: `features/environment.py`, function `apply_tdd_inversion`, line 218. - **Behave internals**: `TagAndStatusStatement.clear_status()` sets `_cached_status = Status.untested`. `TagAndStatusStatement.set_status(value)` sets `_cached_status = value` directly. `TagAndStatusStatement.status` (property) returns `_cached_status` if it is in `final_status = (Status.passed, Status.failed, Status.skipped)`, otherwise calls `compute_status()`. `Status.untested` is NOT in `final_status`, so it always triggers recomputation. - **Why `clear_status()` is used elsewhere**: In `Scenario.run()`, `clear_status()` is called before the after-scenario hook to force `compute_status()` to recompute from step statuses. That is the correct use. In `apply_tdd_inversion`, we are setting a definitive final status — `clear_status()` is wrong here. - **Companion issues**: #8291 (untested-step reset gap, which this issue amplifies) and #8294 (non-AssertionError guard, the primary flakiness cause). ## Subtasks - [ ] Remove `scenario.clear_status()` on line 218 of `features/environment.py` - [ ] Verify the unexpected-pass path (line 239) does not have a `clear_status()` call - [ ] Add a Behave scenario to `features/tdd_expected_fail_infrastructure.feature`: "apply_tdd_inversion scenario status remains passed after external clear_status call" - [ ] Add step definitions for the new scenario in `features/steps/tdd_expected_fail_infrastructure_steps.py` - [ ] Verify `nox -e unit_tests` passes - [ ] Verify coverage ≥ 97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

hurui200320 added the

labels

2026-04-13 08:11:10 +00:00

HAL9000 added

and removed

labels

2026-04-13 08:34:28 +00:00

HAL9000 added this to the v3.2.0 milestone

2026-04-13 08:36:06 +00:00

HAL9000 commented

2026-04-13 08:36:23 +00:00

Owner

✅ Verified — Redundant clear_status() call creates a fragile window where _cached_status=untested, potentially causing intermittent test failures. Should Have fix for v3.2.0. Verified.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

✅ **Verified** — Redundant clear_status() call creates a fragile window where _cached_status=untested, potentially causing intermittent test failures. **Should Have** fix for v3.2.0. Verified. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 referenced this issue

2026-04-13 17:19:30 +00:00

[AUTO-HUMAN] Status: Human Liaison Supervisor (Cycle 1) #8357