fix(cli): share PlanLifecycleService instance between CLI handler and PlanExecutor
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 16s
CI / lint (pull_request) Successful in 19s
CI / quality (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 43s
CI / security (pull_request) Successful in 50s
CI / unit_tests (pull_request) Successful in 3m11s
CI / integration_tests (pull_request) Successful in 3m37s
CI / e2e_tests (pull_request) Successful in 3m52s
CI / docker (pull_request) Successful in 56s
CI / coverage (pull_request) Successful in 5m57s
CI / build (push) Successful in 14s
CI / lint (push) Successful in 20s
CI / quality (push) Successful in 25s
CI / typecheck (push) Successful in 49s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 52s
CI / unit_tests (push) Successful in 3m18s
CI / integration_tests (push) Successful in 3m31s
CI / docker (push) Successful in 55s
CI / e2e_tests (push) Successful in 4m42s
CI / coverage (push) Successful in 6m0s
CI / benchmark-publish (push) Successful in 20m10s
CI / benchmark-regression (pull_request) Successful in 37m16s
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 16s
CI / lint (pull_request) Successful in 19s
CI / quality (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 43s
CI / security (pull_request) Successful in 50s
CI / unit_tests (pull_request) Successful in 3m11s
CI / integration_tests (pull_request) Successful in 3m37s
CI / e2e_tests (pull_request) Successful in 3m52s
CI / docker (pull_request) Successful in 56s
CI / coverage (pull_request) Successful in 5m57s
CI / build (push) Successful in 14s
CI / lint (push) Successful in 20s
CI / quality (push) Successful in 25s
CI / typecheck (push) Successful in 49s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 52s
CI / unit_tests (push) Successful in 3m18s
CI / integration_tests (push) Successful in 3m31s
CI / docker (push) Successful in 55s
CI / e2e_tests (push) Successful in 4m42s
CI / coverage (push) Successful in 6m0s
CI / benchmark-publish (push) Successful in 20m10s
CI / benchmark-regression (pull_request) Successful in 37m16s
_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
This commit was merged in pull request #1027.
This commit is contained in:
@@ -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."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user