From 35e1807f95a86c5fe5bd4f500ff80a344bc56cbc Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Wed, 18 Mar 2026 19:39:11 +0000 Subject: [PATCH 1/2] 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}") From 60af2cae0c34556f8ac1912d58d14d256dd40b64 Mon Sep 17 00:00:00 2001 From: Brent Edwards Date: Thu, 19 Mar 2026 22:10:23 +0000 Subject: [PATCH 2/2] fix(cli): make plan explain accept plan_id for plan-level explanation (#1057) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes `plan explain` to accept both decision IDs and plan IDs as the positional argument, matching the M3 acceptance test usage pattern `plan explain `. ### Problem `explain_decision_cmd` only accepted a decision ULID. When the M3 acceptance test passed a plan ID, `svc.get_decision(plan_id)` returned `None`, causing "Decision not found" error with exit code 1. ### Fix 1. Renamed parameter `decision_id` → `identifier` 2. Tries `svc.get_decision(identifier)` first (backward compat) 3. Falls back to `svc.list_decisions(identifier)` treating it as a plan_id, explaining the root decision 4. Clear error if neither resolves ### Quality Gates | Session | Result | |---------|--------| | `nox -s lint` | PASS | | `nox -s typecheck` | PASS (0 errors) | | `nox -s unit_tests` (explain features) | 46/46 PASS | Closes #968 Reviewed-on: https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1057 Co-authored-by: Brent Edwards Co-committed-by: Brent Edwards --- .../steps/plan_explain_cli_coverage_steps.py | 1 + features/tdd_plan_explain_plan_id.feature | 2 +- robot/tdd_plan_explain_plan_id.robot | 4 +-- src/cleveragents/cli/commands/plan.py | 26 +++++++++++++++---- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/features/steps/plan_explain_cli_coverage_steps.py b/features/steps/plan_explain_cli_coverage_steps.py index 558ea6a0f..36919e9f7 100644 --- a/features/steps/plan_explain_cli_coverage_steps.py +++ b/features/steps/plan_explain_cli_coverage_steps.py @@ -159,6 +159,7 @@ def step_pec_mock_decision_none(context: Context) -> None: context.pec_decision_id = str(ULID()) svc = MagicMock() svc.get_decision.return_value = None + svc.list_decisions.return_value = [] context.pec_container = _mock_container_with_decision_svc(svc) diff --git a/features/tdd_plan_explain_plan_id.feature b/features/tdd_plan_explain_plan_id.feature index 77bb8c4f3..c2dd8579b 100644 --- a/features/tdd_plan_explain_plan_id.feature +++ b/features/tdd_plan_explain_plan_id.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_968 @mock_only +@tdd_bug @tdd_bug_968 @mock_only Feature: TDD Bug #968 — plan explain expects decision_id but test passes plan_id As a developer I want to verify that `plan explain ` succeeds when given a plan ID diff --git a/robot/tdd_plan_explain_plan_id.robot b/robot/tdd_plan_explain_plan_id.robot index ad3e4116c..8f3ca514e 100644 --- a/robot/tdd_plan_explain_plan_id.robot +++ b/robot/tdd_plan_explain_plan_id.robot @@ -17,7 +17,7 @@ TDD Plan Explain Succeeds With Plan ID [Documentation] Verify that ``plan explain `` exits with rc=0 ... when given a plan ID that has associated decisions. ... Bug #968: the command currently exits with rc=1. - [Tags] tdd_expected_fail tdd_bug tdd_bug_968 + [Tags] tdd_bug tdd_bug_968 ${result}= Run Process ${PYTHON} ${HELPER} explain-with-plan-id cwd=${WORKSPACE} timeout=60s on_timeout=kill Log ${result.stdout} Log ${result.stderr} @@ -28,7 +28,7 @@ TDD Plan Explain With Plan ID Shows Root Question [Documentation] Verify that ``plan explain `` output contains ... the root decision question when given a plan ID. ... Bug #968: the command fails before rendering any output. - [Tags] tdd_expected_fail tdd_bug tdd_bug_968 + [Tags] tdd_bug tdd_bug_968 ${result}= Run Process ${PYTHON} ${HELPER} explain-plan-id-shows-question cwd=${WORKSPACE} timeout=60s on_timeout=kill Log ${result.stdout} Log ${result.stderr} diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index c3c137b50..64b669a3d 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -3132,9 +3132,9 @@ def _build_explain_dict( @app.command("explain") def explain_decision_cmd( - decision_id: Annotated[ + identifier: Annotated[ str, - typer.Argument(help="Decision ULID to explain"), + typer.Argument(help="Decision or Plan ULID to explain"), ], fmt: Annotated[ str, @@ -3149,14 +3149,30 @@ def explain_decision_cmd( typer.Option("--show-reasoning", help="Include rationale and actor reasoning"), ] = False, ) -> None: - """Explain a single decision in the plan decision tree.""" + """Explain a single decision or the root decision of a plan.""" from cleveragents.application.container import get_container + from cleveragents.application.services.decision_service import ( + DecisionNotFoundError, + ) container = get_container() svc = container.decision_service() - decision = svc.get_decision(decision_id) + + # First, try treating the identifier as a decision_id (backward compat). + decision = None + with suppress(DecisionNotFoundError): + decision = svc.get_decision(identifier) + + # If not found as a decision, try as a plan_id. if decision is None: - console.print(f"[red]Error:[/red] Decision '{decision_id}' not found.") + decisions = svc.list_decisions(identifier) + if decisions: + decision = decisions[0] + + if decision is None: + console.print( + f"[red]Error:[/red] '{identifier}' not found as a decision or plan." + ) raise typer.Exit(1) data = _build_explain_dict(