fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor #1027
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
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
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "test/e2e-m6-acceptance"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
The
plan executeCLI 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 secondPlanLifecycleServiceFactory instance with its own in-memory_planscache. After the executor'srun_strategize()advanced the plan toexecute/queued(viaauto_progress), the CLI handler's separate service instance returned stalestrategize/queuedstate from its cache.Changes
_get_plan_executor()now accepts an optionallifecycle_serviceparameter. Theplan executehandler passes its own service instance so the CLI handler andPlanExecutorshare the same cache, eliminating stale-state reads after phase transitions.lifecycle_serviceparameter is typed asPlanLifecycleService | Noneinstead ofAny | None, enforcing correct usage at the call site via static analysis (Pyright).Plan execute shares lifecycle service instance with executor) that asserts_get_plan_executoris called with the same lifecycle service object the CLI handler created, preventing silent regressions.docs/reference/di.md(cache-sharing caveat),docs/reference/plan_cli.md(internal wiring section), anddocs/reference/plan_execute.md(CLI executor wiring detail and typed signature).Verification
All default
noxsessions pass:lint,format,typecheck,unit_tests,docs,build,security_scan,dead_code.Closes #746
Closes #1026
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
PlanLifecycleServicefactory 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)
_get_plan_executor(lifecycle_service=service)is recommended.2. Type safety:
Anyinstead of concrete typesrc/cleveragents/cli/commands/plan.py, line 1204 —lifecycle_service: Any | Noneshould ideally bePlanLifecycleService | None. Pre-existing pattern in this function.3. Commit message body describes changes not in the diff
4. Branch name / PR process conventions
test/e2e-m6-acceptancedoesn't followbugfix/convention. PR body is empty. Milestone is v3.2.0 but references issue #746 (v3.5.0).Nits
5. Doc table inconsistency —
plan_cli.mdsummary table still says "Transition to Execute phase" but the detail section was updated.6. Doc snippet missing type annotations —
plan_execute.mdcode example omits annotations present in real code.Summary
The fix correctly shares a single
PlanLifecycleServiceinstance between the CLI handler andPlanExecutor, 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 #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:
Justification for addressing:
The reviewer's observation is correct and is directly supported by two project policies:
The fix introduced a new behavioral contract —
_get_plan_executor()must receive the caller'sPlanLifecycleServiceinstance to avoid stale-cache reads — but no test enforced this contract. If thelifecycle_service=serviceargument were accidentally removed in a future refactoring, all existing tests would still pass, silently reintroducing the bug.What was done:
Plan execute shares lifecycle service instance with executortofeatures/plan_lifecycle_cli_coverage.feature(the existing feature file that already coversplan executeCLI behavior, per CONTRIBUTING.md § BDD Test Organization: "Group new steps with related ones").features/steps/plan_lifecycle_cli_steps.py:step_plan_execute_verify_sharing— runs the CLI handler with_get_plan_executorpatched as a spy, capturing call arguments.step_executor_shares_lifecycle_service— asserts that_get_plan_executorwas called withlifecycle_service=pointing to the same object returned by_get_lifecycle_service().nox -s unit_tests).2. Minor Issue #2 — Type safety:
Anyinstead of concrete typeReviewer statement:
Justification for addressing:
The reviewer's observation is correct and is directly mandated by project policy:
Using
Anyfor a parameter that is always expected to bePlanLifecycleService | Noneeffectively bypasses the static type checker's ability to catch incorrect usage at the call site. The file already usesfrom __future__ import annotations(line 32), so the annotation is a string at runtime — meaning the import can safely go insideTYPE_CHECKINGwith zero runtime cost or circular-import risk.What was done:
PlanLifecycleServiceto the existingif TYPE_CHECKING:block atplan.py:83.lifecycle_service: Any | None = Nonetolifecycle_service: PlanLifecycleService | None = None.docs/reference/plan_execute.mdto reflect the new typed signature.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:
Justification for addressing:
The reviewer's observation is correct. The original commit message body stated:
However, the diff only contained the
lifecycle_servicesharing fix (+10/−3 lines inplan.py, plus docs and changelog). The other three claimed changes are not present in the diff. This is directly against:A misleading commit message makes
git log,git bisect, and code archaeology unreliable.What was done:
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). TheISSUES CLOSED: #746footer was preserved.Items Not Addressed
4. Minor Issue #4 — Branch name / PR process conventions
Reviewer statement:
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.
test/e2e-m6-acceptanceas the head ref). Renaming would require closing and re-creating the PR.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:
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.mddoes 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:
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_executorsignature) was updated as part of Fix #2 to reflect thePlanLifecycleService | Nonetype 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
lintformattypecheckunit_tests(pertinent feature)docsbuildsecurity_scandead_codeFiles Changed
src/cleveragents/cli/commands/plan.pyTYPE_CHECKINGimportfeatures/plan_lifecycle_cli_coverage.featurefeatures/steps/plan_lifecycle_cli_steps.pydocs/reference/plan_execute.mdCHANGELOG.md2098491fb5ff2d824f17New commits pushed, approval review dismissed automatically according to repository settings