fix(cli): Container.resolve() does not exist — plan tree/explain/correct crash with AttributeError #647

Closed
opened 2026-03-09 09:25:57 +00:00 by hurui200320 · 12 comments
Member

Metadata

  • Commit Message: fix(cli): replace non-existent Container.resolve() with named provider calls
  • Branch: bugfix/m3-container-resolve

Parent

Epic: #401 (E2E Integration Testing — bug discovered during #494 M3 acceptance gate re-verification)

Background and Context

During re-verification of the M3 acceptance gate (#494) against the current master, it was discovered that three CLI commands (plan tree, plan explain, plan correct) crash immediately when invoked against a real DI container. The root cause is that all three call container.resolve(DecisionService), but the Container class (extending dependency_injector.containers.DeclarativeContainer) has no resolve() method.

The plan correct call site was introduced by commit 61fc70d8 (PR #639, fixing #606). The plan tree and plan explain call sites use the same non-existent pattern and were likely introduced in the same wave of changes.

The existing M3 integration tests (robot/m3_e2e_verification.robot / helper_m3_e2e_verification.py) do not catch this bug because they mock get_container() with MagicMock, which auto-creates any attribute without raising errors.

Current Behavior

Running any of the three affected commands crashes immediately:

$ .nox/integration_tests-3-13/bin/agents plan tree any-plan-id --format json
Wrapping unexpected exception: AttributeError: 'DynamicContainer' object has no attribute 'resolve'
Error [500] INTERNAL: An unexpected error occurred

The same crash occurs for:

  • agents plan explain <decision_id>
  • agents plan correct <decision_id> --mode revert --guidance "..." --dry-run
  • agents plan correct <decision_id> --mode revert --guidance "..." --yes

Full traceback for plan tree:

File "src/cleveragents/cli/commands/plan.py", line 2972, in tree_decisions_cmd
    svc: DecisionService = container.resolve(_DS)
                           ^^^^^^^^^^^^^^^^^
AttributeError: 'DynamicContainer' object has no attribute 'resolve'

Expected Behavior

The commands should resolve DecisionService from the container using the named provider (container.decision_service()) and execute normally, as all other commands in plan.py do (e.g., tell_command at line 179-180 uses container.plan_service()).

Affected Code

All three call sites are in src/cleveragents/cli/commands/plan.py:

  • Line 2417 (correct_decision_cmd): decision_svc: DecisionService = container.resolve(_DS)
  • Line 2834 (explain_decision_cmd): svc: DecisionService = container.resolve(_DS)
  • Line 2972 (tree_decisions_cmd): svc: DecisionService = container.resolve(_DS)

Reproduction

.nox/integration_tests-3-13/bin/agents plan tree any-plan-id --format json

Or via Python:

from typer.testing import CliRunner
from cleveragents.cli.commands.plan import app

runner = CliRunner()
result = runner.invoke(app, ["tree", "any-plan-id", "--format", "json"])
# result.exception is AttributeError: 'DynamicContainer' object has no attribute 'resolve'

Acceptance Criteria

  • container.resolve(_DS) is replaced with container.decision_service() at all three call sites in plan.py
  • agents plan tree, agents plan explain, and agents plan correct no longer crash with AttributeError
  • Integration test mocks in helper_m3_e2e_verification.py are updated to match the corrected call pattern (.decision_service.return_value instead of .resolve.return_value)

Subtasks

  • Replace container.resolve(_DS) with container.decision_service() at plan.py lines 2417, 2834, 2972
  • Update mock setup in robot/helper_m3_e2e_verification.py to use container.decision_service.return_value instead of container.resolve.return_value
  • Update any BDD step definitions that mock container.resolve for these commands
  • Tests (Behave): Verify existing scenarios still pass
  • Tests (Robot): Verify M3 E2E suite still 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(cli): replace non-existent Container.resolve() with named provider calls` - **Branch**: `bugfix/m3-container-resolve` ## Parent Epic: #401 (E2E Integration Testing — bug discovered during #494 M3 acceptance gate re-verification) ## Background and Context During re-verification of the M3 acceptance gate (#494) against the current master, it was discovered that three CLI commands (`plan tree`, `plan explain`, `plan correct`) crash immediately when invoked against a real DI container. The root cause is that all three call `container.resolve(DecisionService)`, but the `Container` class (extending `dependency_injector.containers.DeclarativeContainer`) has no `resolve()` method. The `plan correct` call site was introduced by commit `61fc70d8` (PR #639, fixing #606). The `plan tree` and `plan explain` call sites use the same non-existent pattern and were likely introduced in the same wave of changes. The existing M3 integration tests (`robot/m3_e2e_verification.robot` / `helper_m3_e2e_verification.py`) do not catch this bug because they mock `get_container()` with `MagicMock`, which auto-creates any attribute without raising errors. ## Current Behavior Running any of the three affected commands crashes immediately: ``` $ .nox/integration_tests-3-13/bin/agents plan tree any-plan-id --format json Wrapping unexpected exception: AttributeError: 'DynamicContainer' object has no attribute 'resolve' Error [500] INTERNAL: An unexpected error occurred ``` The same crash occurs for: - `agents plan explain <decision_id>` - `agents plan correct <decision_id> --mode revert --guidance "..." --dry-run` - `agents plan correct <decision_id> --mode revert --guidance "..." --yes` Full traceback for `plan tree`: ``` File "src/cleveragents/cli/commands/plan.py", line 2972, in tree_decisions_cmd svc: DecisionService = container.resolve(_DS) ^^^^^^^^^^^^^^^^^ AttributeError: 'DynamicContainer' object has no attribute 'resolve' ``` ## Expected Behavior The commands should resolve `DecisionService` from the container using the named provider (`container.decision_service()`) and execute normally, as all other commands in `plan.py` do (e.g., `tell_command` at line 179-180 uses `container.plan_service()`). ## Affected Code All three call sites are in `src/cleveragents/cli/commands/plan.py`: - Line 2417 (`correct_decision_cmd`): `decision_svc: DecisionService = container.resolve(_DS)` - Line 2834 (`explain_decision_cmd`): `svc: DecisionService = container.resolve(_DS)` - Line 2972 (`tree_decisions_cmd`): `svc: DecisionService = container.resolve(_DS)` ## Reproduction ```bash .nox/integration_tests-3-13/bin/agents plan tree any-plan-id --format json ``` Or via Python: ```python from typer.testing import CliRunner from cleveragents.cli.commands.plan import app runner = CliRunner() result = runner.invoke(app, ["tree", "any-plan-id", "--format", "json"]) # result.exception is AttributeError: 'DynamicContainer' object has no attribute 'resolve' ``` ## Acceptance Criteria - `container.resolve(_DS)` is replaced with `container.decision_service()` at all three call sites in `plan.py` - `agents plan tree`, `agents plan explain`, and `agents plan correct` no longer crash with `AttributeError` - Integration test mocks in `helper_m3_e2e_verification.py` are updated to match the corrected call pattern (`.decision_service.return_value` instead of `.resolve.return_value`) ## Subtasks - [x] Replace `container.resolve(_DS)` with `container.decision_service()` at `plan.py` lines 2417, 2834, 2972 - [x] Update mock setup in `robot/helper_m3_e2e_verification.py` to use `container.decision_service.return_value` instead of `container.resolve.return_value` - [x] Update any BDD step definitions that mock `container.resolve` for these commands - [x] Tests (Behave): Verify existing scenarios still pass - [x] Tests (Robot): Verify M3 E2E suite still passes - [x] Verify coverage >=97% via `nox -s coverage_report` - [x] 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 this to the v3.2.0 milestone 2026-03-09 09:27:21 +00:00
Owner

PM Status — Day 29

TDD counterpart created: #648 — assigned to @brent.edwards, milestone v3.2.0, Priority/Critical, Points/2.

Per CONTRIBUTING.md TDD Bug Fix Workflow, this bug cannot be fixed until the TDD failing tests in #648 are merged first. Added MoSCoW/Must Have and Points/2 labels. Assigned to @hurui200320 (Rui) since he reported the bug and it's a straightforward 3-line fix.

Priority: This is a blocking M3 acceptance gate regression — plan tree, plan explain, and plan correct are all broken on current master. Rui, please coordinate with Brent on #648 so the TDD tests land quickly, then submit your fix PR on branch bugfix/m3-container-resolve.

## PM Status — Day 29 **TDD counterpart created**: #648 — assigned to @brent.edwards, milestone v3.2.0, Priority/Critical, Points/2. Per CONTRIBUTING.md TDD Bug Fix Workflow, this bug cannot be fixed until the TDD failing tests in #648 are merged first. Added MoSCoW/Must Have and Points/2 labels. Assigned to @hurui200320 (Rui) since he reported the bug and it's a straightforward 3-line fix. **Priority**: This is a blocking M3 acceptance gate regression — `plan tree`, `plan explain`, and `plan correct` are all broken on current master. Rui, please coordinate with Brent on #648 so the TDD tests land quickly, then submit your fix PR on branch `bugfix/m3-container-resolve`.
Owner

PM Note — Dependency Added

This bug fix is blocked by TDD issue #648 per the mandatory TDD workflow in CONTRIBUTING.md. The failing test scaffolding in #648 must be merged before the fix can proceed.

Dependencies:

  • Blocked by: #648 (TDD failing tests)
  • Blocked by: #627 (Behave @tdd_expected_fail tag infrastructure)
  • Blocked by: #628 (Robot tdd_expected_fail tag infrastructure)
**PM Note — Dependency Added** This bug fix is **blocked by** TDD issue #648 per the mandatory TDD workflow in CONTRIBUTING.md. The failing test scaffolding in #648 must be merged before the fix can proceed. **Dependencies:** - Blocked by: #648 (TDD failing tests) - Blocked by: #627 (Behave `@tdd_expected_fail` tag infrastructure) - Blocked by: #628 (Robot `tdd_expected_fail` tag infrastructure)
Owner

PM Note (Day 29) — Assignment

Changes:

Rationale: Critical M3 bug (Container.resolve crash, 2 SP). Rui has capacity and this is the highest-priority item — bugs always take precedence over feature work per CONTRIBUTING.md. The corresponding TDD test is #648 (assigned to @aditya).

@hurui200320 — This is a critical bug fix. Please prioritize this over #628 and #550. The Container.resolve method crashes under specific conditions. Investigate, write the fix, and coordinate with @aditya who is writing the TDD test (#648).

**PM Note (Day 29) — Assignment** **Changes:** - **Assignee**: (unassigned) → @hurui200320 **Rationale:** Critical M3 bug (Container.resolve crash, 2 SP). Rui has capacity and this is the highest-priority item — bugs always take precedence over feature work per CONTRIBUTING.md. The corresponding TDD test is #648 (assigned to @aditya). @hurui200320 — This is a critical bug fix. Please prioritize this over #628 and #550. The Container.resolve method crashes under specific conditions. Investigate, write the fix, and coordinate with @aditya who is writing the TDD test (#648).
Owner

PM Status — Day 31 (2026-03-11)

Critical path item. This bug blocks M3 (v3.2.0) closure.

Dependency chain:

  • TDD counterpart: #648 (assigned @aditya)
  • TDD PR: #670 — the only clean PR in the repo (no merge conflicts)
  • PR #670 has REQUEST_CHANGES from 2 reviewers with 17 findings total
  • Bug fix cannot begin until PR #670 merges

Action items:

  1. @aditya — please address the 17 review findings on PR #670 as top priority. This is the single biggest blocker for M3 closure.
  2. @hurui200320 — once PR #670 merges, please begin the bug fix on branch bugfix/m3-container-resolve-crash per CONTRIBUTING.md naming convention.

Note: This issue should reference TDD counterpart #648 in the body for traceability.

## PM Status — Day 31 (2026-03-11) **Critical path item.** This bug blocks M3 (v3.2.0) closure. **Dependency chain:** - TDD counterpart: #648 (assigned @aditya) - TDD PR: #670 — the **only clean PR** in the repo (no merge conflicts) - PR #670 has `REQUEST_CHANGES` from 2 reviewers with 17 findings total - Bug fix cannot begin until PR #670 merges **Action items:** 1. @aditya — please address the 17 review findings on PR #670 as top priority. This is the single biggest blocker for M3 closure. 2. @hurui200320 — once PR #670 merges, please begin the bug fix on branch `bugfix/m3-container-resolve-crash` per CONTRIBUTING.md naming convention. **Note:** This issue should reference TDD counterpart #648 in the body for traceability.
Owner

PM Status — Day 32: TDD Workflow Tracking

TDD Pipeline Status:

Stage Item Status
1. TDD Issue #648 Open, assigned to @aditya
2. TDD PR #670 Open, BLOCKED — 2 unaddressed blocking findings from @hamza.khyari
3. TDD merge Waiting on PR #670
4. Bug fix PR Cannot start until TDD tests are merged

Blocker Detail: PR #670 has had 3 review rounds. @hurui200320 and @CoreRasurae's earlier findings were all addressed (both reviews are stale). However, @hamza.khyari's latest review (#2157) on the current commit has 2 blocking findings:

  • M-1: @tdd_expected_fail can mask unrelated failures (any crash is silently inverted, not just the expected AttributeError)
  • M-2: _cleanup() does not reset Settings._instance singleton (known leak pattern fixed in other TDD helpers)

Action needed: @aditya — Please address M-1 and M-2 from @hamza.khyari's review on PR #670, then request re-review. Once merged, the bug fix for this issue can proceed.

Assignee note: Bug fix is assigned to @hurui200320. The fix is straightforward (replace container.resolve(_DS) with container.decision_service() at 3 call sites in plan.py). PR #723 already fixes the parallel session DI bugs (#554/#570/#680) and is approved — merging that first is recommended to reduce conflicts.

### PM Status — Day 32: TDD Workflow Tracking **TDD Pipeline Status:** | Stage | Item | Status | |-------|------|--------| | 1. TDD Issue | #648 | Open, assigned to @aditya | | 2. TDD PR | #670 | Open, **BLOCKED** — 2 unaddressed blocking findings from @hamza.khyari | | 3. TDD merge | — | Waiting on PR #670 | | 4. Bug fix PR | — | Cannot start until TDD tests are merged | **Blocker Detail:** PR #670 has had 3 review rounds. @hurui200320 and @CoreRasurae's earlier findings were all addressed (both reviews are stale). However, @hamza.khyari's latest review (#2157) on the current commit has 2 blocking findings: - **M-1**: `@tdd_expected_fail` can mask unrelated failures (any crash is silently inverted, not just the expected AttributeError) - **M-2**: `_cleanup()` does not reset `Settings._instance` singleton (known leak pattern fixed in other TDD helpers) **Action needed:** @aditya — Please address M-1 and M-2 from @hamza.khyari's review on PR #670, then request re-review. Once merged, the bug fix for this issue can proceed. **Assignee note:** Bug fix is assigned to @hurui200320. The fix is straightforward (replace `container.resolve(_DS)` with `container.decision_service()` at 3 call sites in `plan.py`). PR #723 already fixes the parallel session DI bugs (#554/#570/#680) and is approved — merging that first is recommended to reduce conflicts.
Owner

PM Status — Day 36: Critical Path Escalation

TDD PR #670 is STILL BLOCKED — 5 days since last status update (Day 31).

Blocker: @hamza.khyari's review on PR #670 has 2 blocking findings (M-1 and M-2) that @aditya has not addressed.

TDD Pipeline Status:

Stage Item Status Days Stuck
1. TDD Issue #648 Open
2. TDD PR #670 BLOCKED — 2 unaddressed review findings 5 days
3. TDD merge Waiting on PR #670
4. Bug fix PR Cannot start

Action required — CRITICAL:

@aditya — PR #670 has been stuck for 5 days. The two findings from @hamza.khyari are:

  • M-1: @tdd_expected_fail can mask unrelated failures (any crash is silently inverted)
  • M-2: _cleanup() does not reset Settings._instance singleton

These are specific, actionable findings. Please address them today and request re-review. This is the single biggest blocker for M3 closure.

Impact: Bug #647 (Container.resolve crash) blocks M3 (v3.2.0) which is 18 days past target. Every day of delay on PR #670 is a day added to M3's overdue status.

@hurui200320 — Once PR #670 merges, begin the bug fix immediately on branch bugfix/m3-container-resolve-crash.


PM status comment — Day 36

## PM Status — Day 36: Critical Path Escalation **TDD PR #670 is STILL BLOCKED** — 5 days since last status update (Day 31). **Blocker**: @hamza.khyari's review on PR #670 has 2 blocking findings (M-1 and M-2) that @aditya has not addressed. **TDD Pipeline Status:** | Stage | Item | Status | Days Stuck | |-------|------|--------|------------| | 1. TDD Issue | #648 | Open | — | | 2. TDD PR | #670 | **BLOCKED** — 2 unaddressed review findings | **5 days** | | 3. TDD merge | — | Waiting on PR #670 | — | | 4. Bug fix PR | — | Cannot start | — | **Action required — CRITICAL:** @aditya — PR #670 has been stuck for 5 days. The two findings from @hamza.khyari are: - **M-1**: `@tdd_expected_fail` can mask unrelated failures (any crash is silently inverted) - **M-2**: `_cleanup()` does not reset `Settings._instance` singleton These are specific, actionable findings. Please address them **today** and request re-review. This is the single biggest blocker for M3 closure. **Impact**: Bug #647 (Container.resolve crash) blocks M3 (v3.2.0) which is **18 days past target**. Every day of delay on PR #670 is a day added to M3's overdue status. @hurui200320 — Once PR #670 merges, begin the bug fix immediately on branch `bugfix/m3-container-resolve-crash`. --- *PM status comment — Day 36*
Owner

Day 36 — Dependency link verification and action items

The Forgejo REST API for creating issue dependencies has a confirmed bug in version 14.0.2 (gitea-1.22.0) — the POST /issues/{index}/dependencies endpoint always resolves dependency_id to 0, causing "repository does not exist" errors regardless of the payload format. This affects all programmatic dependency creation.

Manual action required by a maintainer via the Forgejo web UI:

The following dependency links need to be created (bug depends on TDD = TDD must complete first):

Bug Issue Depends On (TDD Issue) Status
#967#977 plan execute phase processing NEEDS LINK
#968#978 plan explain plan_id handling NEEDS LINK
#969#979 plan correct plan_id handling NEEDS LINK
#980#981 skill add regression persistence NEEDS LINK

To create each link: Open the bug issue in the web UI → scroll to the Dependencies section → click "Add Dependency" → enter the TDD issue number → select "is blocked by".

Additionally, verify these existing links are correct:

Bug Issue Depends On (TDD Issue) Status
#647#648 Container.resolve crash Verify exists
#932#950 plan apply --yes flag Verify exists
#822 → TDD PR #929 checkpoint rollback Verify TDD issue link
#821#840 ACMS tier logic Verify exists
#823#838 subplan spawn #838 CLOSED — fix can start
#797#841 actor list DB update Verify exists
#783#842 init --yes input Verify exists
**Day 36 — Dependency link verification and action items** The Forgejo REST API for creating issue dependencies has a confirmed bug in version 14.0.2 (gitea-1.22.0) — the `POST /issues/{index}/dependencies` endpoint always resolves `dependency_id` to 0, causing "repository does not exist" errors regardless of the payload format. This affects all programmatic dependency creation. **Manual action required by a maintainer via the Forgejo web UI:** The following dependency links need to be created (bug depends on TDD = TDD must complete first): | Bug Issue | Depends On (TDD Issue) | Status | |-----------|----------------------|--------| | #967 → #977 | plan execute phase processing | **NEEDS LINK** | | #968 → #978 | plan explain plan_id handling | **NEEDS LINK** | | #969 → #979 | plan correct plan_id handling | **NEEDS LINK** | | #980 → #981 | skill add regression persistence | **NEEDS LINK** | To create each link: Open the **bug issue** in the web UI → scroll to the Dependencies section → click "Add Dependency" → enter the TDD issue number → select "is blocked by". Additionally, verify these existing links are correct: | Bug Issue | Depends On (TDD Issue) | Status | |-----------|----------------------|--------| | #647 → #648 | Container.resolve crash | Verify exists | | #932 → #950 | plan apply --yes flag | Verify exists | | #822 → TDD PR #929 | checkpoint rollback | Verify TDD issue link | | #821 → #840 | ACMS tier logic | Verify exists | | #823 → #838 | subplan spawn | #838 CLOSED — fix can start | | #797 → #841 | actor list DB update | Verify exists | | #783 → #842 | init --yes input | Verify exists |
Owner

PM Status — Day 37

TDD PR #670 still not merged — 6 days stuck. This is the single oldest unresolved blocker in the project.

Update since Day 36:

  • @aditya responded to PR #670 on Day 36 after 5-day stall (per Day 36 schedule adherence notes)
  • PR #670 status is unclear — need to verify whether M-1 and M-2 from @hamza.khyari's review were actually addressed
  • If fixes were pushed, @hamza.khyari needs to re-review and convert to APPROVED

Additional context (Day 37):

  • The bug count has surged from 12 to 26 bugs project-wide. This bug (#647) is one of the oldest and most critical.
  • If PR #670 cannot be merged by Day 38 EOD, I recommend reassigning the TDD work. The current delay is unacceptable for a Priority/Critical M3 bug that has been open since Day 29 (8+ days).

@aditya — Confirm: Did you push fixes for M-1 and M-2 on PR #670? If yes, request re-review from @hamza.khyari.
@hamza.khyari — If Aditya has pushed fixes, please fast-track re-review of PR #670.

Who Action Deadline
@aditya Confirm M-1/M-2 fixes pushed Day 37 EOD
@hamza.khyari Re-review PR #670 if fixes pushed Day 38 EOD
PM Reassign if not resolved Day 38 EOD

PM status — Day 37

## PM Status — Day 37 **TDD PR #670 still not merged — 6 days stuck.** This is the single oldest unresolved blocker in the project. **Update since Day 36:** - @aditya responded to PR #670 on Day 36 after 5-day stall (per Day 36 schedule adherence notes) - PR #670 status is unclear — need to verify whether M-1 and M-2 from @hamza.khyari's review were actually addressed - If fixes were pushed, @hamza.khyari needs to re-review and convert to APPROVED **Additional context (Day 37):** - The bug count has surged from 12 to 26 bugs project-wide. This bug (#647) is one of the oldest and most critical. - **If PR #670 cannot be merged by Day 38 EOD**, I recommend reassigning the TDD work. The current delay is unacceptable for a Priority/Critical M3 bug that has been open since Day 29 (8+ days). @aditya — Confirm: Did you push fixes for M-1 and M-2 on PR #670? If yes, request re-review from @hamza.khyari. @hamza.khyari — If Aditya has pushed fixes, please fast-track re-review of PR #670. | Who | Action | Deadline | |-----|--------|----------| | @aditya | Confirm M-1/M-2 fixes pushed | Day 37 EOD | | @hamza.khyari | Re-review PR #670 if fixes pushed | Day 38 EOD | | PM | Reassign if not resolved | Day 38 EOD | --- *PM status — Day 37*
Author
Member

Implementation Notes

Discovery

Upon investigation, the three container.resolve(_DS) call sites in plan.py have already been fixed in master — they were replaced with container.decision_service() at some point after the issue was filed. The calls now use the correct DI container named provider pattern consistent with the rest of the codebase (e.g., container.plan_service()).

Remaining Fixes

Two Robot Framework test helpers still used the old container.resolve.return_value mock pattern, which would cause those integration tests to silently pass (since MagicMock auto-creates any attribute), but not correctly test the production code path:

  1. robot/helper_m3_decision_validation_smoke.py (line 283): Changed mock_container.resolve.return_value = mock_decision_svcmock_container.decision_service.return_value = mock_decision_svc
  2. robot/helper_m4_correction_subplan_smoke.py (line 123): Changed container.resolve.return_value = mock_decision_svccontainer.decision_service.return_value = mock_decision_svc

Note: robot/helper_m4_e2e_cli.py and robot/helper_m4_e2e_cli_errors.py already use the correct pattern (mock_container.decision_service.return_value) and just have comments mentioning the old approach — no changes needed.

BDD Feature Regression Guard

The @tdd_bug and @tdd_bug_647 tags on features/container_resolve_crash.feature were removed since the bug is now fixed and the scenarios serve as permanent regression guards. The step definitions in features/steps/container_resolve_crash_steps.py use a real DI container (not MagicMock), which is the key design choice — this is what catches the bug that MagicMock-based tests miss.

Quality Gate Results

  • lint: All checks passed
  • typecheck: 0 errors, 0 warnings
  • unit_tests: 471 features, 12421 scenarios, 47538 steps — all passed
  • integration_tests: 1727 tests, 1727 passed, 0 failed
  • coverage_report: 98% overall (≥97% threshold met)
  • e2e_tests: 34/37 passed — 3 failures are due to Anthropic API credit exhaustion (BadRequestError: Your credit balance is too low), not code issues
## Implementation Notes ### Discovery Upon investigation, the three `container.resolve(_DS)` call sites in `plan.py` have **already been fixed** in master — they were replaced with `container.decision_service()` at some point after the issue was filed. The calls now use the correct DI container named provider pattern consistent with the rest of the codebase (e.g., `container.plan_service()`). ### Remaining Fixes Two Robot Framework test helpers still used the old `container.resolve.return_value` mock pattern, which would cause those integration tests to silently pass (since MagicMock auto-creates any attribute), but not correctly test the production code path: 1. **`robot/helper_m3_decision_validation_smoke.py`** (line 283): Changed `mock_container.resolve.return_value = mock_decision_svc` → `mock_container.decision_service.return_value = mock_decision_svc` 2. **`robot/helper_m4_correction_subplan_smoke.py`** (line 123): Changed `container.resolve.return_value = mock_decision_svc` → `container.decision_service.return_value = mock_decision_svc` Note: `robot/helper_m4_e2e_cli.py` and `robot/helper_m4_e2e_cli_errors.py` already use the correct pattern (`mock_container.decision_service.return_value`) and just have comments mentioning the old approach — no changes needed. ### BDD Feature Regression Guard The `@tdd_bug` and `@tdd_bug_647` tags on `features/container_resolve_crash.feature` were removed since the bug is now fixed and the scenarios serve as permanent regression guards. The step definitions in `features/steps/container_resolve_crash_steps.py` use a **real DI container** (not MagicMock), which is the key design choice — this is what catches the bug that MagicMock-based tests miss. ### Quality Gate Results - **lint**: ✅ All checks passed - **typecheck**: ✅ 0 errors, 0 warnings - **unit_tests**: ✅ 471 features, 12421 scenarios, 47538 steps — all passed - **integration_tests**: ✅ 1727 tests, 1727 passed, 0 failed - **coverage_report**: ✅ 98% overall (≥97% threshold met) - **e2e_tests**: 34/37 passed — 3 failures are due to Anthropic API credit exhaustion (`BadRequestError: Your credit balance is too low`), not code issues
Author
Member

Self-QA Implementation Notes (Cycles 1–2)

Cycle 1

Review findings: 1 Critical, 1 Major, 2 Minor, 1 Nit

  • [Critical] @tdd_bug @tdd_bug_647 permanent tags were incorrectly removed from features/container_resolve_crash.feature. Per CONTRIBUTING.md § "TDD Bug Test Tags", these are permanent and must never be removed.
  • [Major] Missing CHANGELOG.md entry — required by PR process.
  • [Minor] Robot mock containers in robot/helper_m3_decision_validation_smoke.py and robot/helper_m4_correction_subplan_smoke.py used bare MagicMock() without spec=Container, allowing silent attribute auto-creation — the exact class of bug that caused #647.
  • [Minor] Branch not rebased onto latest master.
  • [Nit] Feature file used tentative language ("appears fixed" instead of "is fixed").

Fixes applied:

  1. Restored @tdd_bug @tdd_bug_647 tags on line 1 of features/container_resolve_crash.feature.
  2. Added CHANGELOG entry under ## Unreleased describing the fix from the user's perspective with (#647) reference.
  3. Added spec=Container to MagicMock() calls in both Robot helpers, with corresponding from cleveragents.application.container import Container imports.
  4. Rebased branch onto current origin/master (af5e331b).
  5. Replaced tentative language with definitive "Bug #647 is fixed; these scenarios serve as permanent regression guards."
  6. Corrected commit message body to accurately describe tag changes (only @tdd_expected_fail removed; permanent tags retained).

All quality gates passed: lint , typecheck , unit_tests (12421 scenarios) , integration_tests (1727 tests) , e2e_tests (41 tests) , coverage 98% .

Cycle 2

Review findings: 0 Critical, 0 Major, 2 Minor (non-blocking, pre-existing), 2 Nits (non-blocking)

  • All 5 Cycle 1 issues confirmed resolved.
  • Non-blocking minor: Robot helper mock containers don't explicitly configure plan_lifecycle_service() and event_bus() providers (pre-existing pattern).
  • Non-blocking minor: Robot test assertions only check exit code + sentinel string, no output content validation (pre-existing pattern; BDD tests provide content-level validation).
  • Non-blocking nit: PR description mentions @tdd_expected_fail removal but the tag was already absent on master.
  • Non-blocking nit: .robot file documentation still says "appears fixed" while .feature file was updated.

Verdict: Approved — no blocking issues remain.

Remaining Issues

None blocking. The 2 minor and 2 nit items from Cycle 2 are all pre-existing patterns or documentation-level concerns that do not affect correctness or completeness of this PR.

## Self-QA Implementation Notes (Cycles 1–2) ### Cycle 1 **Review findings:** 1 Critical, 1 Major, 2 Minor, 1 Nit - **[Critical]** `@tdd_bug @tdd_bug_647` permanent tags were incorrectly removed from `features/container_resolve_crash.feature`. Per CONTRIBUTING.md § "TDD Bug Test Tags", these are permanent and must never be removed. - **[Major]** Missing CHANGELOG.md entry — required by PR process. - **[Minor]** Robot mock containers in `robot/helper_m3_decision_validation_smoke.py` and `robot/helper_m4_correction_subplan_smoke.py` used bare `MagicMock()` without `spec=Container`, allowing silent attribute auto-creation — the exact class of bug that caused #647. - **[Minor]** Branch not rebased onto latest master. - **[Nit]** Feature file used tentative language ("appears fixed" instead of "is fixed"). **Fixes applied:** 1. Restored `@tdd_bug @tdd_bug_647` tags on line 1 of `features/container_resolve_crash.feature`. 2. Added CHANGELOG entry under `## Unreleased` describing the fix from the user's perspective with `(#647)` reference. 3. Added `spec=Container` to `MagicMock()` calls in both Robot helpers, with corresponding `from cleveragents.application.container import Container` imports. 4. Rebased branch onto current `origin/master` (`af5e331b`). 5. Replaced tentative language with definitive "Bug #647 is fixed; these scenarios serve as permanent regression guards." 6. Corrected commit message body to accurately describe tag changes (only `@tdd_expected_fail` removed; permanent tags retained). All quality gates passed: lint ✅, typecheck ✅, unit_tests (12421 scenarios) ✅, integration_tests (1727 tests) ✅, e2e_tests (41 tests) ✅, coverage 98% ✅. ### Cycle 2 **Review findings:** 0 Critical, 0 Major, 2 Minor (non-blocking, pre-existing), 2 Nits (non-blocking) - All 5 Cycle 1 issues confirmed resolved. - Non-blocking minor: Robot helper mock containers don't explicitly configure `plan_lifecycle_service()` and `event_bus()` providers (pre-existing pattern). - Non-blocking minor: Robot test assertions only check exit code + sentinel string, no output content validation (pre-existing pattern; BDD tests provide content-level validation). - Non-blocking nit: PR description mentions `@tdd_expected_fail` removal but the tag was already absent on master. - Non-blocking nit: `.robot` file documentation still says "appears fixed" while `.feature` file was updated. **Verdict: ✅ Approved** — no blocking issues remain. ### Remaining Issues None blocking. The 2 minor and 2 nit items from Cycle 2 are all pre-existing patterns or documentation-level concerns that do not affect correctness or completeness of this PR.
Author
Member

Self-QA Implementation Notes (Cycles 1–1)

Cycle 1

Review findings: Verdict Approve with 0C/0M/2m/1n.

  • [minor][Test quality] robot/helper_m3_decision_validation_smoke.py (~312–316): assertions are mostly exit-code/sentinel and do not strongly verify output contract or provider interaction.
  • [minor][Test coverage quality] robot/helper_m4_correction_subplan_smoke.py (~162–166, 209–213, 250–254): correction smoke checks are largely exit-code/sentinel and may miss behavioral regressions.
  • [nit][Mock hardening] robot/helper_m3_decision_validation_smoke.py and robot/helper_m4_correction_subplan_smoke.py: spec_set=Container would be stricter than spec=Container.

Fixes applied: None in this cycle. Since the verdict is Approve, no additional fix iteration was required.

Remaining Issues

  • Non-blocking quality improvements remain:
    • Add stronger structured-output and DI interaction assertions to M3 smoke tests.
    • Add payload/interaction assertions for M4 revert/append/dry-run smoke paths.
    • Optionally switch MagicMock(spec=Container) to MagicMock(spec_set=Container) for stricter mock safety.
  • These are deferred as follow-up refinements because they are not approval blockers for this PR.
## Self-QA Implementation Notes (Cycles 1–1) ### Cycle 1 **Review findings:** Verdict **Approve** with **0C/0M/2m/1n**. - **[minor][Test quality]** `robot/helper_m3_decision_validation_smoke.py` (~312–316): assertions are mostly exit-code/sentinel and do not strongly verify output contract or provider interaction. - **[minor][Test coverage quality]** `robot/helper_m4_correction_subplan_smoke.py` (~162–166, 209–213, 250–254): correction smoke checks are largely exit-code/sentinel and may miss behavioral regressions. - **[nit][Mock hardening]** `robot/helper_m3_decision_validation_smoke.py` and `robot/helper_m4_correction_subplan_smoke.py`: `spec_set=Container` would be stricter than `spec=Container`. **Fixes applied:** None in this cycle. Since the verdict is **Approve**, no additional fix iteration was required. ### Remaining Issues - Non-blocking quality improvements remain: - Add stronger structured-output and DI interaction assertions to M3 smoke tests. - Add payload/interaction assertions for M4 revert/append/dry-run smoke paths. - Optionally switch `MagicMock(spec=Container)` to `MagicMock(spec_set=Container)` for stricter mock safety. - These are deferred as follow-up refinements because they are not approval blockers for this PR.
Author
Member

Self-QA Implementation Notes (Cycle 1)

Cycle 1 — Review

Verdict: APPROVE (0C/1M/0m/2n)

Review findings:

  • Major (1): PR has merge conflicts with master (mergeable: false). The branch is based on af5e331 but master has advanced to d196e90. Likely a CHANGELOG.md conflict from other recently-merged PRs. This is a mechanical rebase issue and does not affect code correctness.
  • Nit (1): spec=Container could be strengthened to spec_set=Container for stricter mock safety in robot/helper_m3_decision_validation_smoke.py and robot/helper_m4_correction_subplan_smoke.py.
  • Nit (2): Feature file could note that the production fix was in a prior commit, but the commit message body already explains this clearly.

Fixes applied: None needed — verdict was APPROVE. The merge conflict requires a rebase before merging.

Assessment Summary

Aspect Status
Spec compliance All acceptance criteria from #647 satisfied
Correctness Mock patterns correctly mirror production code
Test coverage 98% (≥97% threshold)
Test quality spec=Container prevents silent mock mismatches
Code quality Clean diff, proper Conventional Changelog format
CI pipeline Run #7656 passed all gates
Self-QA (prior) Two self-QA cycles performed before this review

Remaining Issues

  • Rebase needed: Branch must be rebased onto current master to resolve merge conflicts (likely CHANGELOG.md only) before merge.
  • Optional: Consider spec_set=Container instead of spec=Container for stricter mock fidelity (nit, not blocking).
## Self-QA Implementation Notes (Cycle 1) ### Cycle 1 — Review **Verdict:** ✅ APPROVE (0C/1M/0m/2n) **Review findings:** - **Major (1):** PR has merge conflicts with master (`mergeable: false`). The branch is based on `af5e331` but master has advanced to `d196e90`. Likely a CHANGELOG.md conflict from other recently-merged PRs. This is a mechanical rebase issue and does not affect code correctness. - **Nit (1):** `spec=Container` could be strengthened to `spec_set=Container` for stricter mock safety in `robot/helper_m3_decision_validation_smoke.py` and `robot/helper_m4_correction_subplan_smoke.py`. - **Nit (2):** Feature file could note that the production fix was in a prior commit, but the commit message body already explains this clearly. **Fixes applied:** None needed — verdict was APPROVE. The merge conflict requires a rebase before merging. ### Assessment Summary | Aspect | Status | |--------|--------| | Spec compliance | ✅ All acceptance criteria from #647 satisfied | | Correctness | ✅ Mock patterns correctly mirror production code | | Test coverage | ✅ 98% (≥97% threshold) | | Test quality | ✅ `spec=Container` prevents silent mock mismatches | | Code quality | ✅ Clean diff, proper Conventional Changelog format | | CI pipeline | ✅ Run #7656 passed all gates | | Self-QA (prior) | ✅ Two self-QA cycles performed before this review | ### Remaining Issues - **Rebase needed:** Branch must be rebased onto current master to resolve merge conflicts (likely CHANGELOG.md only) before merge. - **Optional:** Consider `spec_set=Container` instead of `spec=Container` for stricter mock fidelity (nit, not blocking).
hurui200320 2026-03-30 05:45:10 +00:00
Sign in to join this conversation.
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/cleveragents-core#647
No description provided.