From 35e1807f95a86c5fe5bd4f500ff80a344bc56cbc Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Wed, 18 Mar 2026 19:39:11 +0000 Subject: [PATCH] fix(cli): restore phase-aware execution in plan execute command Consolidate the execute_plan CLI handler to eliminate a redundant service.get_plan() call (the separate read-only pre-check now reuses the same current_plan reference used for phase detection), update the command-table description from the stale 'Transition to Execute phase' to 'Run phase-aware plan execution', and replace the static post-execution hint with a state-aware message that distinguishes execute/complete (ready for apply) from other states (continue executing). Update corresponding Behave test mocks to match the reduced get_plan call sequence and the updated output panel title. ISSUES CLOSED: #967 --- features/plan_lifecycle_cli_coverage.feature | 2 +- .../plan_lifecycle_commands_coverage_steps.py | 24 +++++------- ...tdd_plan_execute_phase_processing_steps.py | 20 ++++------ src/cleveragents/cli/commands/plan.py | 37 ++++++++++++------- 4 files changed, 42 insertions(+), 41 deletions(-) diff --git a/features/plan_lifecycle_cli_coverage.feature b/features/plan_lifecycle_cli_coverage.feature index d258f1309..e9e47c29b 100644 --- a/features/plan_lifecycle_cli_coverage.feature +++ b/features/plan_lifecycle_cli_coverage.feature @@ -47,7 +47,7 @@ Feature: Plan lifecycle CLI coverage When I run plan execute without a plan id with 1 complete plans Then the plan lifecycle command should succeed And the execute command should run the single ready plan - And the plan lifecycle output should contain "Execute phase" + And the plan lifecycle output should contain "Plan Executed" Scenario: Plan execute handles invalid phase transition When I run plan execute for plan id "01ARZ3NDEKTSV4RRFFQ69G5FAV" causing "invalid transition" diff --git a/features/steps/plan_lifecycle_commands_coverage_steps.py b/features/steps/plan_lifecycle_commands_coverage_steps.py index 34920e59e..25067cd4e 100644 --- a/features/steps/plan_lifecycle_commands_coverage_steps.py +++ b/features/steps/plan_lifecycle_commands_coverage_steps.py @@ -650,14 +650,12 @@ def step_invoke_execute_plan_queued(context): executed_plan = _make_mock_lifecycle_plan(phase="execute", state="queued") completed_plan = _make_mock_lifecycle_plan(phase="execute", state="complete") - # get_plan call sequence: - # 1. read-only check (pre_plan) - # 2. strategize state inspection (current_plan) - # 3. re-fetch after inline strategize - # 4. inline execute state inspection - # 5. re-fetch after inline execute + # get_plan call sequence (consolidated read-only + phase check): + # 1. current_plan (null + read-only + strategize state) + # 2. re-fetch after inline strategize + # 3. inline execute state inspection + # 4. re-fetch after inline execute mock_service.get_plan.side_effect = [ - queued_plan, queued_plan, executed_plan, executed_plan, @@ -702,14 +700,12 @@ def step_invoke_execute_plan_auto_progressed(context): executed_plan = _make_mock_lifecycle_plan(phase="execute", state="queued") completed_plan = _make_mock_lifecycle_plan(phase="execute", state="complete") - # get_plan call sequence: - # 1. read-only check (pre_plan) - # 2. strategize state inspection (current_plan) - # 3. re-fetch after inline strategize → execute/queued - # 4. inline execute state inspection → execute/queued (triggers run_execute) - # 5. re-fetch after inline execute → execute/complete + # get_plan call sequence (consolidated read-only + phase check): + # 1. current_plan (null + read-only + strategize state) + # 2. re-fetch after inline strategize → execute/queued + # 3. inline execute state inspection → execute/queued (triggers run_execute) + # 4. re-fetch after inline execute → execute/complete mock_service.get_plan.side_effect = [ - queued_plan, queued_plan, executed_plan, executed_plan, diff --git a/features/steps/tdd_plan_execute_phase_processing_steps.py b/features/steps/tdd_plan_execute_phase_processing_steps.py index 59effb32e..250de6c32 100644 --- a/features/steps/tdd_plan_execute_phase_processing_steps.py +++ b/features/steps/tdd_plan_execute_phase_processing_steps.py @@ -141,13 +141,11 @@ def step_plan_in_strategize_queued(context: Context) -> None: ) # get_plan call sequence in the CLI execute_plan handler: - # 1. pre_plan (read-only check) - # 2. current_plan (phase detection) - # 3. re-fetch after run_strategize (auto_progress moved to Execute) - # 4. re-fetch for inline execute check - # 5. re-fetch after run_execute + # 1. current_plan (phase detection + read-only check — merged) + # 2. re-fetch after run_strategize (auto_progress moved to Execute) + # 3. re-fetch for inline execute check + # 4. re-fetch after run_execute context.mock_service_967.get_plan.side_effect = [ - queued_plan, queued_plan, execute_queued_plan, execute_queued_plan, @@ -340,13 +338,11 @@ def step_single_queued_plan_auto_discovery(context: Context) -> None: ] # get_plan call sequence after auto-discovery selects the plan: - # 1. pre_plan (read-only check) - # 2. current_plan (phase detection) - # 3. re-fetch after run_strategize - # 4. re-fetch for inline execute check - # 5. re-fetch after run_execute + # 1. current_plan (phase detection + read-only check — merged) + # 2. re-fetch after run_strategize + # 3. re-fetch for inline execute check + # 4. re-fetch after run_execute context.mock_service_967.get_plan.side_effect = [ - queued_plan, queued_plan, execute_queued_plan, execute_queued_plan, diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 9f16ff103..c3c137b50 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -10,7 +10,7 @@ plan lifecycle. | ``agents plan use`` | Create plan from action + project(s) | | ``agents plan lifecycle-list``| List plans with optional filters | | ``agents plan status`` | Show plan status / details | -| ``agents plan execute`` | Transition to Execute phase | +| ``agents plan execute`` | Run phase-aware plan execution | | ``agents plan lifecycle-apply``| Transition to Apply phase | | ``agents plan cancel`` | Cancel a non-terminal plan | | ``agents plan diff`` | Show ChangeSet as unified diff | @@ -1824,20 +1824,19 @@ def execute_plan( pre.execution_environment = execution_environment.lower() service._commit_plan(pre) - # Fail-fast: read-only plans must not enter Execute phase - pre_plan = service.get_plan(plan_id) - if pre_plan is not None and pre_plan.read_only is True: - console.print( - f"[red]Cannot execute plan '{plan_id}': plan is read-only.[/red]" - ) - raise typer.Abort() - # Determine current phase and run the appropriate processing current_plan = service.get_plan(plan_id) if current_plan is None: console.print(f"[red]Plan '{plan_id}' not found.[/red]") raise typer.Abort() + # Fail-fast: read-only plans must not enter Execute phase + if current_plan.read_only is True: + console.print( + f"[red]Cannot execute plan '{plan_id}': plan is read-only.[/red]" + ) + raise typer.Abort() + # If the plan is still in Strategize/queued, run the strategize # phase inline before transitioning to Execute. This implements # the auto_strategize behaviour described in the specification: @@ -1888,11 +1887,21 @@ def execute_plan( data = _plan_spec_dict(plan) console.print(format_output(data, fmt)) else: - _print_lifecycle_plan(plan, title="Plan Executing") - console.print( - "\n[dim]Plan is now in Execute phase. " - "Run 'agents plan apply ' when ready.[/dim]" - ) + _print_lifecycle_plan(plan, title="Plan Executed") + phase_label = f"{plan.phase.value}/{plan.state.value}" + if plan.phase == PlanPhase.EXECUTE and plan.state in ( + ProcessingState.COMPLETE, + ProcessingState.APPLIED, + ): + console.print( + f"\n[dim]Plan execution completed ({phase_label}). " + "Run 'agents plan lifecycle-apply ' when ready.[/dim]" + ) + else: + console.print( + f"\n[dim]Plan is now in {phase_label} state. " + "Run 'agents plan execute ' to continue.[/dim]" + ) except PreflightRejection as e: console.print(f"[red]Pre-flight check failed:[/red] {e}")