fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor #1027

2026-03-17T11:32:03Z

CoreRasurae commented

2026-03-17 11:32:03 +00:00

Summary

The plan execute CLI command failed with "Plan is not in an executable state (current: strategize/queued)" after the strategize phase completed successfully. The root cause was that _get_plan_executor() created a second PlanLifecycleService Factory instance with its own in-memory _plans cache. After the executor's run_strategize() advanced the plan to execute/queued (via auto_progress), the CLI handler's separate service instance returned stale strategize/queued state from its cache.

Changes

Bug fix: _get_plan_executor() now accepts an optional lifecycle_service parameter. The plan execute handler passes its own service instance so the CLI handler and PlanExecutor share the same cache, eliminating stale-state reads after phase transitions.
Type safety: The lifecycle_service parameter is typed as PlanLifecycleService | None instead of Any | None, enforcing correct usage at the call site via static analysis (Pyright).
Regression test: Added a BDD scenario (Plan execute shares lifecycle service instance with executor) that asserts _get_plan_executor is called with the same lifecycle service object the CLI handler created, preventing silent regressions.
Documentation: Updated docs/reference/di.md (cache-sharing caveat), docs/reference/plan_cli.md (internal wiring section), and docs/reference/plan_execute.md (CLI executor wiring detail and typed signature).
Changelog: Added entries describing the fix, type safety improvement, and regression test.

Verification

All default nox sessions pass: lint, format, typecheck, unit_tests, docs, build, security_scan, dead_code.

Closes #746
Closes #1026

## Summary The `plan execute` CLI command failed with *"Plan is not in an executable state (current: strategize/queued)"* after the strategize phase completed successfully. The root cause was that `_get_plan_executor()` created a **second** `PlanLifecycleService` Factory instance with its own in-memory `_plans` cache. After the executor's `run_strategize()` advanced the plan to `execute/queued` (via `auto_progress`), the CLI handler's separate service instance returned stale `strategize/queued` state from its cache. ### Changes - **Bug fix:** `_get_plan_executor()` now accepts an optional `lifecycle_service` parameter. The `plan execute` handler passes its own service instance so the CLI handler and `PlanExecutor` share the same cache, eliminating stale-state reads after phase transitions. - **Type safety:** The `lifecycle_service` parameter is typed as `PlanLifecycleService | None` instead of `Any | None`, enforcing correct usage at the call site via static analysis (Pyright). - **Regression test:** Added a BDD scenario (`Plan execute shares lifecycle service instance with executor`) that asserts `_get_plan_executor` is called with the same lifecycle service object the CLI handler created, preventing silent regressions. - **Documentation:** Updated `docs/reference/di.md` (cache-sharing caveat), `docs/reference/plan_cli.md` (internal wiring section), and `docs/reference/plan_execute.md` (CLI executor wiring detail and typed signature). - **Changelog:** Added entries describing the fix, type safety improvement, and regression test. ### Verification All default `nox` sessions pass: `lint`, `format`, `typecheck`, `unit_tests`, `docs`, `build`, `security_scan`, `dead_code`. Closes #746 Closes #1026

CoreRasurae added the

Type

Bug

label 2026-03-17 11:32:09 +00:00

CoreRasurae self-assigned this 2026-03-17 11:32:10 +00:00

CoreRasurae added this to the v3.2.0 milestone 2026-03-17 11:32:10 +00:00

hurui200320 approved these changes 2026-03-17 11:44:48 +00:00

Dismissed

hurui200320 left a comment

PR Review: !1027 (No corresponding ticket — urgent pipeline fix)

Verdict: Approve ✅

The code fix is technically correct, minimal, and well-documented. It properly resolves the stale-cache bug by sharing a single PlanLifecycleService factory instance. No functional, security, or performance concerns. Process and test coverage observations are noted below for follow-up but are non-blocking given the urgency.

Critical Issues

None.

Major Issues

None.

Minor Issues (non-blocking observations)

1. No tests protect the fix (follow-up recommended)

The fix has zero test coverage. If reverted, all existing tests still pass. A follow-up to add a regression test asserting _get_plan_executor(lifecycle_service=service) is recommended.

2. Type safety: Any instead of concrete type

src/cleveragents/cli/commands/plan.py, line 1204 — lifecycle_service: Any | None should ideally be PlanLifecycleService | None. Pre-existing pattern in this function.

3. Commit message body describes changes not in the diff

The commit body claims 4 changes but the diff only contains the lifecycle service sharing fix. Should be corrected for git history clarity.

4. Branch name / PR process conventions

Branch test/e2e-m6-acceptance doesn't follow bugfix/ convention. PR body is empty. Milestone is v3.2.0 but references issue #746 (v3.5.0).

Nits

5. Doc table inconsistency — plan_cli.md summary table still says "Transition to Execute phase" but the detail section was updated.

6. Doc snippet missing type annotations — plan_execute.md code example omits annotations present in real code.

Summary

The fix correctly shares a single PlanLifecycleService instance between the CLI handler and PlanExecutor, eliminating the stale in-memory cache that caused "Plan is not in an executable state" errors. The change is surgical (+10/−3 lines of code), well-documented (CHANGELOG + 3 reference docs), and introduces no risks. Approved for merge as an urgent fix. Process and test gaps should be addressed in follow-up work.

## PR Review: !1027 (No corresponding ticket — urgent pipeline fix) ### Verdict: Approve ✅ The code fix is technically correct, minimal, and well-documented. It properly resolves the stale-cache bug by sharing a single `PlanLifecycleService` factory instance. No functional, security, or performance concerns. Process and test coverage observations are noted below for follow-up but are **non-blocking** given the urgency. --- ### Critical Issues None. ### Major Issues None. ### Minor Issues (non-blocking observations) **1. No tests protect the fix (follow-up recommended)** - The fix has zero test coverage. If reverted, all existing tests still pass. A follow-up to add a regression test asserting `_get_plan_executor(lifecycle_service=service)` is recommended. **2. Type safety: `Any` instead of concrete type** - `src/cleveragents/cli/commands/plan.py`, line 1204 — `lifecycle_service: Any | None` should ideally be `PlanLifecycleService | None`. Pre-existing pattern in this function. **3. Commit message body describes changes not in the diff** - The commit body claims 4 changes but the diff only contains the lifecycle service sharing fix. Should be corrected for git history clarity. **4. Branch name / PR process conventions** - Branch `test/e2e-m6-acceptance` doesn't follow `bugfix/` convention. PR body is empty. Milestone is v3.2.0 but references issue #746 (v3.5.0). ### Nits **5. Doc table inconsistency** — `plan_cli.md` summary table still says "Transition to Execute phase" but the detail section was updated. **6. Doc snippet missing type annotations** — `plan_execute.md` code example omits annotations present in real code. ### Summary The fix correctly shares a single `PlanLifecycleService` instance between the CLI handler and `PlanExecutor`, eliminating the stale in-memory cache that caused "Plan is not in an executable state" errors. The change is surgical (+10/−3 lines of code), well-documented (CHANGELOG + 3 reference docs), and introduces no risks. Approved for merge as an urgent fix. Process and test gaps should be addressed in follow-up work.

CoreRasurae commented

2026-03-17 12:14:43 +00:00

PR #1027 Review Response Report

Reviewer: @hurui200320 (Rui Hu)
Review verdict: Approved ✅
Review date: 2026-03-17T11:44:48Z

Items Addressed

1. Minor Issue #1 — No tests protect the fix

Reviewer statement:

The fix has zero test coverage. If reverted, all existing tests still pass. A follow-up to add a regression test asserting _get_plan_executor(lifecycle_service=service) is recommended.

Justification for addressing:
The reviewer's observation is correct and is directly supported by two project policies:

CONTRIBUTING.md § Commit Completeness: "If your change introduces or modifies behavior, the tests covering that behavior belong in the same commit."
CONTRIBUTING.md § Testing Philosophy: "Every coding task must include or update tests at multiple levels."

The fix introduced a new behavioral contract — _get_plan_executor() must receive the caller's PlanLifecycleService instance to avoid stale-cache reads — but no test enforced this contract. If the lifecycle_service=service argument were accidentally removed in a future refactoring, all existing tests would still pass, silently reintroducing the bug.

What was done:

Added a new BDD scenario Plan execute shares lifecycle service instance with executor to features/plan_lifecycle_cli_coverage.feature (the existing feature file that already covers plan execute CLI behavior, per CONTRIBUTING.md § BDD Test Organization: "Group new steps with related ones").
Added two step definitions in features/steps/plan_lifecycle_cli_steps.py:
- step_plan_execute_verify_sharing — runs the CLI handler with _get_plan_executor patched as a spy, capturing call arguments.
- step_executor_shares_lifecycle_service — asserts that _get_plan_executor was called with lifecycle_service= pointing to the same object returned by _get_lifecycle_service().
The test was verified passing (30/30 scenarios in the feature file via nox -s unit_tests).

2. Minor Issue #2 — Type safety: `Any` instead of concrete type

Reviewer statement:

src/cleveragents/cli/commands/plan.py, line 1204 — lifecycle_service: Any | None should ideally be PlanLifecycleService | None. Pre-existing pattern in this function.

Justification for addressing:
The reviewer's observation is correct and is directly mandated by project policy:

CONTRIBUTING.md § Type Safety: "Full Annotations: In languages with type annotation support, every function signature, variable declaration, and return type should be annotated with explicit types."
CONTRIBUTING.md § Type Safety: "No Suppression: never disable [the type checker] via configuration files and never use inline comments or annotations to suppress individual type checking errors."

Using Any for a parameter that is always expected to be PlanLifecycleService | None effectively bypasses the static type checker's ability to catch incorrect usage at the call site. The file already uses from __future__ import annotations (line 32), so the annotation is a string at runtime — meaning the import can safely go inside TYPE_CHECKING with zero runtime cost or circular-import risk.

What was done:

Added PlanLifecycleService to the existing if TYPE_CHECKING: block at plan.py:83.
Changed the function signature from lifecycle_service: Any | None = None to lifecycle_service: PlanLifecycleService | None = None.
Updated the code snippet in docs/reference/plan_execute.md to reflect the new typed signature.
Verified via nox -s typecheck (Pyright): 0 errors, 1 pre-existing warning (unrelated).

3. Minor Issue #3 — Commit message body describes changes not in the diff

Reviewer statement:

The commit body claims 4 changes but the diff only contains the lifecycle service sharing fix. Should be corrected for git history clarity.

Justification for addressing:
The reviewer's observation is correct. The original commit message body stated:

Changes:
- start_strategize() loads the plan's action from the persistence layer
- plan execute CLI catches PreflightRejection for user-friendly errors.
- plan execute CLI runs the execute phase inline via PlanExecutor
- lifecycle-apply CLI handles plans already auto-progressed to apply/queued

However, the diff only contained the lifecycle_service sharing fix (+10/−3 lines in plan.py, plus docs and changelog). The other three claimed changes are not present in the diff. This is directly against:

CONTRIBUTING.md § Commit Message Format: "the body should be appropriate in length for the scope of the change — detailed enough to explain what was done and why"
CONTRIBUTING.md § Commit Hygiene: "Self-review the diff. Before committing, review the staged diff to confirm only the intended changes are included."

A misleading commit message makes git log, git bisect, and code archaeology unreliable.

What was done:

Amended the commit with a new message whose first line matches the PR title (fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor), and whose body accurately describes only the changes present in the diff: the stale-cache root cause, the fix, and the review-feedback improvements (type safety, regression test, doc update). The ISSUES CLOSED: #746 footer was preserved.

Items Not Addressed

4. Minor Issue #4 — Branch name / PR process conventions

Reviewer statement:

Branch test/e2e-m6-acceptance doesn't follow bugfix/ convention. PR body is empty. Milestone is v3.2.0 but references issue #746 (v3.5.0).

Justification for not addressing:
This comment concerns PR metadata and branch-naming conventions — process and administrative items that are outside the scope of code fixes.

Branch renaming at this stage would invalidate the existing PR on Forgejo (PR #1027 tracks test/e2e-m6-acceptance as the head ref). Renaming would require closing and re-creating the PR.
PR body and milestone are metadata editable directly on the Forgejo PR form and do not require code changes. These are administrative follow-ups, not code review fixes.
The branch name originates from issue #746's metadata (Branch: test/e2e-m6-acceptance), which was set by a project owner. Changing it unilaterally would contradict the issue specification.

5. Nit #5 — Doc table inconsistency

Reviewer statement:

plan_cli.md summary table still says "Transition to Execute phase" but the detail section was updated.

Justification for not addressing:
The reviewer themselves labelled this as a Nit, indicating it is a non-blocking cosmetic observation. The inconsistency between the summary table header text and the expanded detail section in docs/reference/plan_cli.md does not affect code behavior, type safety, or test correctness. It can be addressed in a future documentation cleanup pass.

6. Nit #6 — Doc snippet missing type annotations

Reviewer statement:

plan_execute.md code example omits annotations present in real code.

Justification for not addressing:
The reviewer themselves labelled this as a Nit. The code snippet in the documentation is a simplified illustration; omitting some annotations is a common documentation practice for readability. Note that the specific snippet the reviewer refers to (the general _get_plan_executor signature) was updated as part of Fix #2 to reflect the PlanLifecycleService | None type change, but the broader annotation gaps in other doc snippets throughout the file were not addressed since they are pre-existing and marked as a nit.

Verification Summary

Nox session	Result
`lint`	Passed
`format`	Passed (1475 files unchanged)
`typecheck`	Passed (0 errors, 1 pre-existing warning)
`unit_tests` (pertinent feature)	Passed (30/30 scenarios)
`docs`	Passed
`build`	Passed
`security_scan`	Passed
`dead_code`	Passed

Files Changed

File	Change
`src/cleveragents/cli/commands/plan.py`	Type annotation fix + `TYPE_CHECKING` import
`features/plan_lifecycle_cli_coverage.feature`	+1 regression scenario
`features/steps/plan_lifecycle_cli_steps.py`	+2 step definitions
`docs/reference/plan_execute.md`	Doc snippet updated for typed signature
`CHANGELOG.md`	+2 entries (type safety, regression test)

## PR #1027 Review Response Report **Reviewer:** @hurui200320 (Rui Hu) **Review verdict:** Approved ✅ **Review date:** 2026-03-17T11:44:48Z --- ### Items Addressed #### 1. Minor Issue #1 — No tests protect the fix **Reviewer statement:** > The fix has zero test coverage. If reverted, all existing tests still pass. A follow-up to add a regression test asserting `_get_plan_executor(lifecycle_service=service)` is recommended. **Justification for addressing:** The reviewer's observation is correct and is directly supported by two project policies: - **CONTRIBUTING.md § Commit Completeness:** *"If your change introduces or modifies behavior, the tests covering that behavior belong in the same commit."* - **CONTRIBUTING.md § Testing Philosophy:** *"Every coding task must include or update tests at multiple levels."* The fix introduced a new behavioral contract — `_get_plan_executor()` must receive the caller's `PlanLifecycleService` instance to avoid stale-cache reads — but no test enforced this contract. If the `lifecycle_service=service` argument were accidentally removed in a future refactoring, all existing tests would still pass, silently reintroducing the bug. **What was done:** - Added a new BDD scenario `Plan execute shares lifecycle service instance with executor` to `features/plan_lifecycle_cli_coverage.feature` (the existing feature file that already covers `plan execute` CLI behavior, per CONTRIBUTING.md § BDD Test Organization: *"Group new steps with related ones"*). - Added two step definitions in `features/steps/plan_lifecycle_cli_steps.py`: - `step_plan_execute_verify_sharing` — runs the CLI handler with `_get_plan_executor` patched as a spy, capturing call arguments. - `step_executor_shares_lifecycle_service` — asserts that `_get_plan_executor` was called with `lifecycle_service=` pointing to the **same object** returned by `_get_lifecycle_service()`. - The test was verified passing (30/30 scenarios in the feature file via `nox -s unit_tests`). --- #### 2. Minor Issue #2 — Type safety: `Any` instead of concrete type **Reviewer statement:** > `src/cleveragents/cli/commands/plan.py`, line 1204 — `lifecycle_service: Any | None` should ideally be `PlanLifecycleService | None`. Pre-existing pattern in this function. **Justification for addressing:** The reviewer's observation is correct and is directly mandated by project policy: - **CONTRIBUTING.md § Type Safety:** *"Full Annotations: In languages with type annotation support, every function signature, variable declaration, and return type should be annotated with explicit types."* - **CONTRIBUTING.md § Type Safety:** *"No Suppression: never disable [the type checker] via configuration files and never use inline comments or annotations to suppress individual type checking errors."* Using `Any` for a parameter that is always expected to be `PlanLifecycleService | None` effectively bypasses the static type checker's ability to catch incorrect usage at the call site. The file already uses `from __future__ import annotations` (line 32), so the annotation is a string at runtime — meaning the import can safely go inside `TYPE_CHECKING` with zero runtime cost or circular-import risk. **What was done:** - Added `PlanLifecycleService` to the existing `if TYPE_CHECKING:` block at `plan.py:83`. - Changed the function signature from `lifecycle_service: Any | None = None` to `lifecycle_service: PlanLifecycleService | None = None`. - Updated the code snippet in `docs/reference/plan_execute.md` to reflect the new typed signature. - Verified via `nox -s typecheck` (Pyright): 0 errors, 1 pre-existing warning (unrelated). --- #### 3. Minor Issue #3 — Commit message body describes changes not in the diff **Reviewer statement:** > The commit body claims 4 changes but the diff only contains the lifecycle service sharing fix. Should be corrected for git history clarity. **Justification for addressing:** The reviewer's observation is correct. The original commit message body stated: > *Changes:* > *- start_strategize() loads the plan's action from the persistence layer* > *- plan execute CLI catches PreflightRejection for user-friendly errors.* > *- plan execute CLI runs the execute phase inline via PlanExecutor* > *- lifecycle-apply CLI handles plans already auto-progressed to apply/queued* However, the diff only contained the `lifecycle_service` sharing fix (+10/−3 lines in `plan.py`, plus docs and changelog). The other three claimed changes are not present in the diff. This is directly against: - **CONTRIBUTING.md § Commit Message Format:** *"the body should be appropriate in length for the scope of the change — detailed enough to explain what was done and why"* - **CONTRIBUTING.md § Commit Hygiene:** *"Self-review the diff. Before committing, review the staged diff to confirm only the intended changes are included."* A misleading commit message makes `git log`, `git bisect`, and code archaeology unreliable. **What was done:** - Amended the commit with a new message whose first line matches the PR title (`fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor`), and whose body accurately describes only the changes present in the diff: the stale-cache root cause, the fix, and the review-feedback improvements (type safety, regression test, doc update). The `ISSUES CLOSED: #746` footer was preserved. --- ### Items Not Addressed #### 4. Minor Issue #4 — Branch name / PR process conventions **Reviewer statement:** > Branch `test/e2e-m6-acceptance` doesn't follow `bugfix/` convention. PR body is empty. Milestone is v3.2.0 but references issue #746 (v3.5.0). **Justification for not addressing:** This comment concerns PR metadata and branch-naming conventions — process and administrative items that are outside the scope of code fixes. - **Branch renaming** at this stage would invalidate the existing PR on Forgejo (PR #1027 tracks `test/e2e-m6-acceptance` as the head ref). Renaming would require closing and re-creating the PR. - **PR body and milestone** are metadata editable directly on the Forgejo PR form and do not require code changes. These are administrative follow-ups, not code review fixes. - The branch name originates from issue #746's metadata (`Branch: test/e2e-m6-acceptance`), which was set by a project owner. Changing it unilaterally would contradict the issue specification. --- #### 5. Nit #5 — Doc table inconsistency **Reviewer statement:** > `plan_cli.md` summary table still says "Transition to Execute phase" but the detail section was updated. **Justification for not addressing:** The reviewer themselves labelled this as a **Nit**, indicating it is a non-blocking cosmetic observation. The inconsistency between the summary table header text and the expanded detail section in `docs/reference/plan_cli.md` does not affect code behavior, type safety, or test correctness. It can be addressed in a future documentation cleanup pass. --- #### 6. Nit #6 — Doc snippet missing type annotations **Reviewer statement:** > `plan_execute.md` code example omits annotations present in real code. **Justification for not addressing:** The reviewer themselves labelled this as a **Nit**. The code snippet in the documentation is a simplified illustration; omitting some annotations is a common documentation practice for readability. Note that the specific snippet the reviewer refers to (the general `_get_plan_executor` signature) **was** updated as part of Fix #2 to reflect the `PlanLifecycleService | None` type change, but the broader annotation gaps in other doc snippets throughout the file were not addressed since they are pre-existing and marked as a nit. --- ### Verification Summary | Nox session | Result | |---|---| | `lint` | Passed | | `format` | Passed (1475 files unchanged) | | `typecheck` | Passed (0 errors, 1 pre-existing warning) | | `unit_tests` (pertinent feature) | Passed (30/30 scenarios) | | `docs` | Passed | | `build` | Passed | | `security_scan` | Passed | | `dead_code` | Passed | ### Files Changed | File | Change | |---|---| | `src/cleveragents/cli/commands/plan.py` | Type annotation fix + `TYPE_CHECKING` import | | `features/plan_lifecycle_cli_coverage.feature` | +1 regression scenario | | `features/steps/plan_lifecycle_cli_steps.py` | +2 step definitions | | `docs/reference/plan_execute.md` | Doc snippet updated for typed signature | | `CHANGELOG.md` | +2 entries (type safety, regression test) |

CoreRasurae added a new dependency 2026-03-17 12:18:31 +00:00

#746 test(e2e): E2E acceptance criteria for M6 (v3.5.0) — autonomy hardening

CoreRasurae force-pushed test/e2e-m6-acceptance from 2098491fb5 to ff2d824f17

2026-03-17 12:22:56 +00:00

Compare

CoreRasurae dismissed hurui200320's review 2026-03-17 12:22:56 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

CoreRasurae added a new dependency 2026-03-17 12:26:04 +00:00

#1026 fix: Failing E2E tests on master after plan execute cache-sharing fix

CoreRasurae removed a dependency 2026-03-17 12:26:12 +00:00

#1026 fix: Failing E2E tests on master after plan execute cache-sharing fix

CoreRasurae added a new dependency 2026-03-17 12:27:03 +00:00

#1026 fix: Failing E2E tests on master after plan execute cache-sharing fix

CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-03-17 12:27:41 +00:00

CoreRasurae merged commit ff2d824f17 into master

2026-03-17 12:29:39 +00:00

CoreRasurae deleted branch test/e2e-m6-acceptance

2026-03-17 12:29:39 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Blocks

#746 test(e2e): E2E acceptance criteria for M6 (v3.5.0) — autonomy hardening

cleveragents/cleveragents-core

#1026 fix: Failing E2E tests on master after plan execute cache-sharing fix

cleveragents/cleveragents-core

Reference: cleveragents/cleveragents-core#1027

fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor #1027

Summary

Changes

Verification

PR Review: !1027 (No corresponding ticket — urgent pipeline fix)

Verdict: Approve ✅

Critical Issues

Major Issues

Minor Issues (non-blocking observations)

Nits

Summary

PR #1027 Review Response Report

Items Addressed

1. Minor Issue #1 — No tests protect the fix

2. Minor Issue #2 — Type safety: Any instead of concrete type

3. Minor Issue #3 — Commit message body describes changes not in the diff

Items Not Addressed

4. Minor Issue #4 — Branch name / PR process conventions

5. Nit #5 — Doc table inconsistency

6. Nit #6 — Doc snippet missing type annotations

Verification Summary

Files Changed

2. Minor Issue #2 — Type safety: `Any` instead of concrete type