From ff2d824f179f4c99f402b3cd8925fd69f7141148 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Tue, 17 Mar 2026 00:34:43 +0000 Subject: [PATCH] fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor _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, causing spurious "Plan is not in an executable state" errors. Fix: _get_plan_executor() now accepts an optional lifecycle_service parameter; the plan execute handler passes its own service instance so both share the same cache. Also addressed review feedback: - Improved type safety: lifecycle_service parameter typed as PlanLifecycleService | None instead of Any | None. - Added BDD regression test verifying the lifecycle service is shared between the CLI handler and the executor. - Updated reference documentation to reflect the type annotation change. ISSUES CLOSED: #1026 --- CHANGELOG.md | 18 +++++++ docs/reference/di.md | 28 ++++++++++ docs/reference/plan_cli.md | 18 ++++++- docs/reference/plan_execute.md | 45 ++++++++++++++++ features/plan_lifecycle_cli_coverage.feature | 4 ++ features/steps/plan_lifecycle_cli_steps.py | 55 ++++++++++++++++++++ src/cleveragents/cli/commands/plan.py | 16 ++++-- 7 files changed, 180 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4de7af46f..38453ca23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,24 @@ ## Unreleased +- Fixed `plan execute` CLI failing with "Plan is not in an executable state + (current: strategize/queued)" after strategize completed successfully. + Root cause: `_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. Fix: `_get_plan_executor()` now accepts an optional + `lifecycle_service` parameter; the `plan execute` handler passes its own + service instance so both share the same cache. + (`src/cleveragents/cli/commands/plan.py`) +- Improved type safety: `_get_plan_executor()` parameter + `lifecycle_service` now typed as `PlanLifecycleService | None` instead + of `Any | None`. + (`src/cleveragents/cli/commands/plan.py`) +- Added BDD regression test verifying that `plan execute` CLI handler + passes its lifecycle service instance to `_get_plan_executor()`, + preventing stale-cache regressions. + (`features/plan_lifecycle_cli_coverage.feature`) - Added four CLI-based integration test cases to M5 E2E verification suite for v3.4.0 milestone acceptance criteria validation. Tests exercise `project create`, `resource add git-checkout`, `project link-resource`, and diff --git a/docs/reference/di.md b/docs/reference/di.md index e53bbdba0..52bfc5fdd 100644 --- a/docs/reference/di.md +++ b/docs/reference/di.md @@ -80,6 +80,34 @@ records decisions during phase transitions: If no `DecisionService` is provided (e.g. in legacy tests), decision recording is silently skipped. +### Cache-Sharing Caveat + +Because `plan_lifecycle_service` is a **Factory**, each +`container.plan_lifecycle_service()` call returns a **new instance** +with its own in-memory `_plans` cache. Code that creates a +`PlanLifecycleService` and separately constructs a `PlanExecutor` +**must** pass the same service instance to both; otherwise the +executor's phase mutations (e.g. `complete_strategize` → +`auto_progress` → `execute/queued`) will not be visible to the +caller's service instance, producing stale state reads. + +```python +# Correct: shared instance +service = container.plan_lifecycle_service() +executor = PlanExecutor(lifecycle_service=service, ...) + +# Wrong: two independent instances with separate caches +service = container.plan_lifecycle_service() # Instance A +executor = PlanExecutor( + lifecycle_service=container.plan_lifecycle_service(), # Instance B + ... +) +``` + +The CLI helper `_get_plan_executor(lifecycle_service=...)` accepts an +optional pre-existing service for exactly this purpose. See +[CLI Executor Wiring](plan_execute.md#cli-executor-wiring-_get_plan_executor). + ## Usage Example ```python diff --git a/docs/reference/plan_cli.md b/docs/reference/plan_cli.md index 328e93030..a2943b7cc 100644 --- a/docs/reference/plan_cli.md +++ b/docs/reference/plan_cli.md @@ -123,12 +123,28 @@ when present on the plan. ## `agents plan execute` -Transition a plan from Strategize to Execute phase. +Run the current plan phase synchronously. Detects the plan's current +phase and processes it inline: + +- **Strategize/queued** — runs the strategize phase to completion, then + auto-progresses to Execute if the automation profile permits. +- **Strategize/complete** — transitions to Execute and runs it. +- **Execute/queued** — runs the execute phase to completion. + +When no plan ID is given, auto-selects the single eligible plan. ```bash agents plan execute [PLAN_ID] [--format FORMAT] ``` +### Internal Wiring + +The CLI handler shares a single `PlanLifecycleService` instance between +the command logic and the `PlanExecutor` to avoid stale in-memory cache +reads after phase transitions. See +[CLI Executor Wiring](plan_execute.md#cli-executor-wiring-_get_plan_executor) +for details. + ## `agents plan lifecycle-apply` Transition a plan from Execute to Apply phase. diff --git a/docs/reference/plan_execute.md b/docs/reference/plan_execute.md index 4e7c7df9d..4dc809f92 100644 --- a/docs/reference/plan_execute.md +++ b/docs/reference/plan_execute.md @@ -125,6 +125,51 @@ assert executor.changeset_store is not None result = executor.run_execute(plan_id) ``` +## CLI Executor Wiring (`_get_plan_executor`) + +The `plan execute` CLI command constructs a `PlanExecutor` via the +internal `_get_plan_executor()` helper in +`cleveragents.cli.commands.plan`. This helper resolves the +`ProviderRegistry` from the DI container and builds +`LLMStrategizeActor` / `LLMExecuteActor` for real LLM calls. + +```python +def _get_plan_executor(lifecycle_service: PlanLifecycleService | None = None): + """Build a PlanExecutor wired with real LLM actors. + + Args: + lifecycle_service: Optional pre-existing PlanLifecycleService. + When provided the executor shares the same service (and its + in-memory plan cache) as the caller. + """ +``` + +### Shared Lifecycle Service Instance + +Because `PlanLifecycleService` is registered as a **Factory** provider +in the DI container, each `container.plan_lifecycle_service()` call +returns a **new instance** with its own in-memory `_plans` cache. The +`plan execute` handler creates one service for CLI-level state reads +and passes it to `_get_plan_executor()` so the executor shares the +same instance: + +```python +# In the plan execute CLI handler: +service = _get_lifecycle_service() +executor = _get_plan_executor(lifecycle_service=service) # shared instance +``` + +This is critical because `executor.run_strategize()` mutates plan +state through the lifecycle service (e.g. advancing from +`strategize/complete` to `execute/queued` via `auto_progress`). If +the executor used a separate service instance, the CLI handler's +subsequent `service.get_plan()` call would return stale cached state, +causing spurious "not in an executable state" errors. + +> **Note:** When `lifecycle_service` is omitted (e.g. from test code +> or standalone scripts), `_get_plan_executor` falls back to creating +> a fresh instance from the container. + ### Properties | Property | Type | Description | diff --git a/features/plan_lifecycle_cli_coverage.feature b/features/plan_lifecycle_cli_coverage.feature index 854f81b83..d258f1309 100644 --- a/features/plan_lifecycle_cli_coverage.feature +++ b/features/plan_lifecycle_cli_coverage.feature @@ -158,3 +158,7 @@ Feature: Plan lifecycle CLI coverage When I run plan cancel for plan id "01ARZ3NDEKTSV4RRFFQ69G5FB5" causing "general error" Then the plan lifecycle command should abort And the plan lifecycle output should contain "Error" + + Scenario: Plan execute shares lifecycle service instance with executor + When I run plan execute verifying lifecycle service sharing + Then the plan executor should receive the same lifecycle service instance diff --git a/features/steps/plan_lifecycle_cli_steps.py b/features/steps/plan_lifecycle_cli_steps.py index 68afa9162..47faba3a5 100644 --- a/features/steps/plan_lifecycle_cli_steps.py +++ b/features/steps/plan_lifecycle_cli_steps.py @@ -512,3 +512,58 @@ def step_lifecycle_apply_runs_single_plan(context) -> None: assert len(context.apply_plans) == 1 plan_id = context.apply_plans[0].identity.plan_id context.lifecycle_service.apply_plan.assert_called_once_with(plan_id) + + +# ------------------------------------------------------------------ +# Regression: lifecycle-service sharing between CLI handler & executor +# ------------------------------------------------------------------ + + +@when("I run plan execute verifying lifecycle service sharing") +def step_plan_execute_verify_sharing(context) -> None: + """Run ``plan execute`` and capture the ``_get_plan_executor`` call. + + After Background has already mocked ``_get_lifecycle_service`` to + return ``context.lifecycle_service``, we patch ``_get_plan_executor`` + so we can later assert it was called with the **same** service + instance. + """ + plan = _make_plan( + plan_id=_ULIDS[0], + name="local/shared-svc-plan", + phase=PlanPhase.STRATEGIZE, + processing_state=ProcessingState.COMPLETE, + ) + context.lifecycle_service.list_plans.return_value = [plan] + context.lifecycle_service.get_plan.return_value = plan + context.lifecycle_service.execute_plan.return_value = plan + + mock_executor = MagicMock() + executor_patcher = patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=mock_executor, + ) + mock_get_executor = executor_patcher.start() + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(executor_patcher.stop) + context._mock_get_plan_executor = mock_get_executor + + context.result = context.runner.invoke(plan_app, ["execute"]) + + +@then("the plan executor should receive the same lifecycle service instance") +def step_executor_shares_lifecycle_service(context) -> None: + """Assert ``_get_plan_executor`` was called with the shared service.""" + mock_fn = context._mock_get_plan_executor + mock_fn.assert_called_once() + call_kwargs = mock_fn.call_args.kwargs + assert "lifecycle_service" in call_kwargs, ( + "_get_plan_executor was not called with a lifecycle_service kwarg; " + f"got kwargs: {call_kwargs}" + ) + assert call_kwargs["lifecycle_service"] is context.lifecycle_service, ( + "Expected _get_plan_executor to receive the same lifecycle service " + "instance that _get_lifecycle_service() returned, but it received " + "a different object." + ) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 4ee099cb4..45404b69e 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -81,6 +81,9 @@ _LEGACY_DEPRECATION_MSG = ( ) if TYPE_CHECKING: + from cleveragents.application.services.plan_lifecycle_service import ( + PlanLifecycleService, + ) from cleveragents.domain.models.core import Change, Plan, Project from cleveragents.domain.models.core.decision import Decision @@ -1201,13 +1204,19 @@ def _get_lifecycle_service(): return container.plan_lifecycle_service() -def _get_plan_executor() -> Any: +def _get_plan_executor(lifecycle_service: PlanLifecycleService | None = None) -> Any: """Build a ``PlanExecutor`` wired with real LLM actors. Resolves the ``ProviderRegistry`` and ``PlanLifecycleService`` from the DI container and constructs ``LLMStrategizeActor`` / ``LLMExecuteActor`` so that ``plan execute`` invocations drive real LLM calls instead of the local-only stub actors. + + Args: + lifecycle_service: Optional pre-existing ``PlanLifecycleService`` + instance. When provided the executor shares the same service + (and its in-memory cache) as the caller, avoiding stale-read + bugs caused by the DI ``Factory`` creating independent instances. """ from cleveragents.application.container import get_container from cleveragents.application.services.llm_actors import ( @@ -1218,7 +1227,8 @@ def _get_plan_executor() -> Any: container = get_container() registry = container.provider_registry() - lifecycle_service = _get_lifecycle_service() + if lifecycle_service is None: + lifecycle_service = _get_lifecycle_service() strategize_actor = LLMStrategizeActor( provider_registry=registry, @@ -1683,7 +1693,7 @@ def execute_plan( ) service = _get_lifecycle_service() - executor = _get_plan_executor() + executor = _get_plan_executor(lifecycle_service=service) if not plan_id: # Auto-discover: look for plans in strategize or execute