From 63cc79ca4b3b57946074c9802bd5b8d6e09ec133 Mon Sep 17 00:00:00 2001 From: Rui Hu Date: Mon, 23 Mar 2026 07:17:33 +0000 Subject: [PATCH] fix(cli): add missing --yes flag to plan apply command Added the --yes/-y flag to the lifecycle_apply_plan command in cleveragents.cli.commands.plan, matching the specification requirement for agents plan apply [--yes|-y] . Without --yes, a confirmation prompt is now displayed showing the plan ID and name before applying. With --yes, the apply proceeds immediately. The implementation follows the same Typer Option pattern used by rollback_plan, correct_plan, and other destructive CLI commands. The confirmation prompt appears after plan resolution and read-only validation but before any state-modifying operations. Removed @tdd_expected_fail tags from both Behave (tdd_plan_apply_yes_flag.feature) and Robot Framework (tdd_plan_apply_yes_flag.robot) TDD tests per the Bug Fix Workflow, leaving @tdd_bug and @tdd_bug_932 as permanent regression guards. Updated all existing test invocations of lifecycle-apply across Behave steps and Robot helpers to pass --yes, since the new confirmation prompt would otherwise abort in non-interactive test environments. ISSUES CLOSED: #932 --- benchmarks/cli_robot_flow_bench.py | 4 +- benchmarks/m1_sourcecode_smoke_bench.py | 2 +- benchmarks/plan_cli_smoke_bench.py | 2 +- docs/reference/plan_cli.md | 22 +- .../steps/cli_lifecycle_coverage_steps.py | 8 +- .../cli_lifecycle_robot_alignment_steps.py | 2 +- features/steps/m1_sourcecode_smoke_steps.py | 2 +- features/steps/plan_cli_commands_r2_steps.py | 2 +- .../steps/plan_cli_coverage_boost_steps.py | 2 +- features/steps/plan_cli_coverage_steps.py | 4 +- .../steps/plan_explain_cli_coverage_steps.py | 2 +- features/steps/plan_lifecycle_cli_steps.py | 6 +- .../plan_lifecycle_commands_coverage_steps.py | 4 +- .../steps/tdd_plan_apply_yes_flag_steps.py | 280 ++++++++++++++++-- features/tdd_plan_apply_yes_flag.feature | 44 ++- robot/e2e/m2_acceptance.robot | 2 +- robot/e2e/m6_acceptance.robot | 2 +- robot/e2e/wf05_db_migration.robot | 2 +- robot/helper_cli_lifecycle.py | 4 +- robot/helper_cli_lifecycle_e2e.py | 4 +- robot/helper_m1_e2e_verification.py | 2 +- robot/helper_m1_sourcecode_smoke.py | 4 +- robot/helper_m6_e2e_verification.py | 2 +- robot/helper_tdd_plan_apply_yes_flag.py | 5 +- robot/tdd_plan_apply_yes_flag.robot | 7 +- src/cleveragents/cli/commands/plan.py | 40 ++- 26 files changed, 381 insertions(+), 79 deletions(-) diff --git a/benchmarks/cli_robot_flow_bench.py b/benchmarks/cli_robot_flow_bench.py index 32cd84f19..3001128ae 100644 --- a/benchmarks/cli_robot_flow_bench.py +++ b/benchmarks/cli_robot_flow_bench.py @@ -221,11 +221,11 @@ class CLIRobotLifecycleSuite: def time_plan_apply(self) -> None: """Benchmark plan lifecycle-apply via CLI.""" - _runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + _runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) def time_full_lifecycle_flow(self) -> None: """Benchmark the full create -> use -> execute -> apply flow.""" _runner.invoke(action_app, ["create", "--config", self._yaml_path]) _runner.invoke(plan_app, ["use", "local/bench-lifecycle-action", "proj-bench"]) _runner.invoke(plan_app, ["execute", _PLAN_ULID]) - _runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + _runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) diff --git a/benchmarks/m1_sourcecode_smoke_bench.py b/benchmarks/m1_sourcecode_smoke_bench.py index 048ca5799..5e75e2874 100644 --- a/benchmarks/m1_sourcecode_smoke_bench.py +++ b/benchmarks/m1_sourcecode_smoke_bench.py @@ -208,7 +208,7 @@ class M1PlanApplySuite: def time_plan_lifecycle_apply(self) -> None: """Benchmark plan lifecycle-apply command.""" - _runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + _runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) class M1FixtureLoadSuite: diff --git a/benchmarks/plan_cli_smoke_bench.py b/benchmarks/plan_cli_smoke_bench.py index f20f9414f..b9f66afe6 100644 --- a/benchmarks/plan_cli_smoke_bench.py +++ b/benchmarks/plan_cli_smoke_bench.py @@ -276,7 +276,7 @@ class PlanLifecycleCmdParsingSuite: def time_lifecycle_apply(self) -> None: """Benchmark plan lifecycle-apply.""" - _runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + _runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) def time_status_by_id(self) -> None: """Benchmark plan status with specific ID.""" diff --git a/docs/reference/plan_cli.md b/docs/reference/plan_cli.md index a2943b7cc..dd64715ab 100644 --- a/docs/reference/plan_cli.md +++ b/docs/reference/plan_cli.md @@ -147,12 +147,30 @@ for details. ## `agents plan lifecycle-apply` -Transition a plan from Execute to Apply phase. +Transition a plan from Execute to Apply phase. Because Apply is a +destructive operation (it merges sandbox changesets into real project +resources), a confirmation prompt is displayed by default. Pass +`--yes` / `-y` to skip the prompt in scripts or CI pipelines. + +### Synopsis ```bash -agents plan lifecycle-apply [PLAN_ID] [--format FORMAT] +agents plan lifecycle-apply [--yes|-y] [PLAN_ID] [--format FORMAT] ``` +### Options + +| Flag | Description | +|-------------------------|-------------------------------------------------------| +| `--yes`, `-y` | Skip the confirmation prompt and apply immediately | +| `--format`, `-f` | Output format: json, yaml, plain, table, rich | + +### Arguments + +| Argument | Description | +|------------|----------------------------------------------------------------| +| `PLAN_ID` | Plan ID to apply (optional; auto-selects if only one eligible) | + ## `agents plan cancel` Cancel a non-terminal plan. diff --git a/features/steps/cli_lifecycle_coverage_steps.py b/features/steps/cli_lifecycle_coverage_steps.py index 94b74f90e..7604458aa 100644 --- a/features/steps/cli_lifecycle_coverage_steps.py +++ b/features/steps/cli_lifecycle_coverage_steps.py @@ -1003,19 +1003,21 @@ def step_lc_plan_apply_general_error(context: Context) -> None: def step_lc_plan_apply(context: Context) -> None: pid = getattr(context, "lc_plan", None) plan_id = pid.identity.plan_id if pid else _PLAN_ULID - context.lc_result = context.lc_runner.invoke(plan_app, ["lifecycle-apply", plan_id]) + context.lc_result = context.lc_runner.invoke( + plan_app, ["lifecycle-apply", "--yes", plan_id] + ) @when("I run lifecycle coverage plan lifecycle-apply without ID") def step_lc_plan_apply_no_id(context: Context) -> None: - context.lc_result = context.lc_runner.invoke(plan_app, ["lifecycle-apply"]) + context.lc_result = context.lc_runner.invoke(plan_app, ["lifecycle-apply", "--yes"]) @when('I run lifecycle coverage plan lifecycle-apply with plan ID and format "{fmt}"') def step_lc_plan_apply_fmt(context: Context, fmt: str) -> None: pid = context.lc_plan.identity.plan_id context.lc_result = context.lc_runner.invoke( - plan_app, ["lifecycle-apply", pid, "--format", fmt] + plan_app, ["lifecycle-apply", "--yes", pid, "--format", fmt] ) diff --git a/features/steps/cli_lifecycle_robot_alignment_steps.py b/features/steps/cli_lifecycle_robot_alignment_steps.py index bde9c599f..4b815857f 100644 --- a/features/steps/cli_lifecycle_robot_alignment_steps.py +++ b/features/steps/cli_lifecycle_robot_alignment_steps.py @@ -228,7 +228,7 @@ def step_robot_alignment_plan_apply(context: Context) -> None: ), ) context.ra_apply_result = context.ra_runner.invoke( - plan_app, ["lifecycle-apply", _PLAN_ULID] + plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID] ) diff --git a/features/steps/m1_sourcecode_smoke_steps.py b/features/steps/m1_sourcecode_smoke_steps.py index 86840f5ed..382cb7a2a 100644 --- a/features/steps/m1_sourcecode_smoke_steps.py +++ b/features/steps/m1_sourcecode_smoke_steps.py @@ -574,7 +574,7 @@ def step_m1_plan_in_apply(context: Context) -> None: @when("I m1 smoke invoke plan lifecycle-apply") def step_m1_plan_apply(context: Context) -> None: """Invoke plan lifecycle-apply.""" - result = context.runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + result = context.runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) context.last_result = result diff --git a/features/steps/plan_cli_commands_r2_steps.py b/features/steps/plan_cli_commands_r2_steps.py index d6ad7ab4b..09a68705c 100644 --- a/features/steps/plan_cli_commands_r2_steps.py +++ b/features/steps/plan_cli_commands_r2_steps.py @@ -323,7 +323,7 @@ def step_invoke_execute_no_id(context: Any) -> None: @when("r2plan-I invoke lifecycle-apply without plan_id") def step_invoke_apply_no_id(context: Any) -> None: - context.r2_result = _runner.invoke(plan_app, ["lifecycle-apply"]) + context.r2_result = _runner.invoke(plan_app, ["lifecycle-apply", "--yes"]) # --------------------------------------------------------------------------- diff --git a/features/steps/plan_cli_coverage_boost_steps.py b/features/steps/plan_cli_coverage_boost_steps.py index fba6c7c48..46551a67e 100644 --- a/features/steps/plan_cli_coverage_boost_steps.py +++ b/features/steps/plan_cli_coverage_boost_steps.py @@ -378,7 +378,7 @@ def step_invoke_execute_json(context) -> None: def step_invoke_lifecycle_apply_json(context) -> None: context.result = context.runner.invoke( plan_app, - ["lifecycle-apply", context._apply_plan_id, "--format", "json"], + ["lifecycle-apply", "--yes", context._apply_plan_id, "--format", "json"], ) diff --git a/features/steps/plan_cli_coverage_steps.py b/features/steps/plan_cli_coverage_steps.py index d0d0334df..6f8820dc0 100644 --- a/features/steps/plan_cli_coverage_steps.py +++ b/features/steps/plan_cli_coverage_steps.py @@ -429,7 +429,7 @@ def step_invoke_use_with_invariant(context: Context, text: str) -> None: def step_invoke_lifecycle_apply_no_id(context: Context) -> None: """Invoke lifecycle-apply without specifying a plan ID.""" context.plan_cov_result = context.plan_cov_runner.invoke( - plan_app, ["lifecycle-apply"] + plan_app, ["lifecycle-apply", "--yes"] ) @@ -437,7 +437,7 @@ def step_invoke_lifecycle_apply_no_id(context: Context) -> None: def step_invoke_lifecycle_apply_with_id(context: Context, pid: str) -> None: """Invoke lifecycle-apply with an explicit plan ID.""" context.plan_cov_result = context.plan_cov_runner.invoke( - plan_app, ["lifecycle-apply", pid] + plan_app, ["lifecycle-apply", "--yes", pid] ) diff --git a/features/steps/plan_explain_cli_coverage_steps.py b/features/steps/plan_explain_cli_coverage_steps.py index 36919e9f7..03cd0649f 100644 --- a/features/steps/plan_explain_cli_coverage_steps.py +++ b/features/steps/plan_explain_cli_coverage_steps.py @@ -740,7 +740,7 @@ def step_pec_invoke_execute_readonly(context: Context) -> None: def step_pec_invoke_apply_readonly(context: Context) -> None: with patch(_PATCH_LIFECYCLE, return_value=context.pec_lifecycle_svc): context.pec_result = runner.invoke( - plan_app, ["lifecycle-apply", context.pec_plan_id] + plan_app, ["lifecycle-apply", "--yes", context.pec_plan_id] ) diff --git a/features/steps/plan_lifecycle_cli_steps.py b/features/steps/plan_lifecycle_cli_steps.py index c319a5e50..95fde061e 100644 --- a/features/steps/plan_lifecycle_cli_steps.py +++ b/features/steps/plan_lifecycle_cli_steps.py @@ -301,7 +301,7 @@ def step_plan_lifecycle_apply_without_plan_id(context, count: int) -> None: if count == 1: context.lifecycle_service.apply_plan.return_value = plans[0] context.apply_plans = plans - context.result = context.runner.invoke(plan_app, ["lifecycle-apply"]) + context.result = context.runner.invoke(plan_app, ["lifecycle-apply", "--yes"]) @when('I run plan lifecycle apply for plan id "{plan_id}" causing "{error_type}"') @@ -324,7 +324,9 @@ def step_plan_lifecycle_apply_error(context, plan_id: str, error_type: str) -> N else: raise ValueError(f"Unhandled error type: {error_type}") - context.result = context.runner.invoke(plan_app, ["lifecycle-apply", plan_id]) + context.result = context.runner.invoke( + plan_app, ["lifecycle-apply", "--yes", plan_id] + ) @when("I run plan status without a plan id and {count:d} plans exist") diff --git a/features/steps/plan_lifecycle_commands_coverage_steps.py b/features/steps/plan_lifecycle_commands_coverage_steps.py index 25067cd4e..d31e6f1f8 100644 --- a/features/steps/plan_lifecycle_commands_coverage_steps.py +++ b/features/steps/plan_lifecycle_commands_coverage_steps.py @@ -753,7 +753,7 @@ def step_invoke_lifecycle_apply_with_id(context): return_value=mock_service, ): result = runner.invoke( - plan_app, ["lifecycle-apply", "01JAAAAAAAAAAAAAAAAAAAAAAA"] + plan_app, ["lifecycle-apply", "--yes", "01JAAAAAAAAAAAAAAAAAAAAAAA"] ) context.result = result @@ -774,7 +774,7 @@ def step_invoke_lifecycle_apply_auto_select(context): "cleveragents.cli.commands.plan._get_lifecycle_service", return_value=mock_service, ): - result = runner.invoke(plan_app, ["lifecycle-apply"]) + result = runner.invoke(plan_app, ["lifecycle-apply", "--yes"]) context.result = result diff --git a/features/steps/tdd_plan_apply_yes_flag_steps.py b/features/steps/tdd_plan_apply_yes_flag_steps.py index f53221c16..58ee3c82a 100644 --- a/features/steps/tdd_plan_apply_yes_flag_steps.py +++ b/features/steps/tdd_plan_apply_yes_flag_steps.py @@ -1,25 +1,88 @@ """Step definitions for TDD Bug #932 — plan apply missing --yes flag. These steps verify that the ``lifecycle-apply`` CLI command accepts the -``--yes`` / ``-y`` flag as required by the specification. The spec -mandates ``agents plan apply [--yes|-y] `` to skip the -confirmation prompt before applying plan changes, but the current -implementation does not recognise this flag. +``--yes`` / ``-y`` flag as required by the specification, and that the +confirmation prompt behaves correctly: -When the flag is missing, Typer (Click) rejects the option with -``"No such option: --yes"`` and exit code 2. The assertion that this -error is absent will **fail** — proving the bug. The -``@tdd_expected_fail`` tag on the feature inverts this failure to a -pass while the bug remains open. +- With ``--yes`` / ``-y``: no confirmation prompt is shown. +- Without ``--yes``: a confirmation prompt appears; declining aborts with + ``"Apply cancelled."``, accepting proceeds with the apply. """ from __future__ import annotations +from datetime import UTC, datetime +from unittest.mock import MagicMock, patch + from behave import given, then, when from behave.runner import Context from typer.testing import CliRunner from cleveragents.cli.commands.plan import app as plan_app +from cleveragents.domain.models.core.plan import Plan as LifecyclePlan +from cleveragents.domain.models.core.plan import ( + PlanPhase, + ProcessingState, +) + +_PATCH_LIFECYCLE = "cleveragents.cli.commands.plan._get_lifecycle_service" + +# Confirmation prompt text fragment used to detect whether the prompt appeared. +_CONFIRM_FRAGMENT = "Apply changes for plan" + + +def _make_mock_plan( + plan_id: str = "01JAAAAAAAAAAAAAAAAAAAAAAA", + phase: PlanPhase = PlanPhase.EXECUTE, + state: ProcessingState = ProcessingState.COMPLETE, +) -> LifecyclePlan: + """Build a lightweight mock Plan for confirmation-prompt tests.""" + from cleveragents.domain.models.core.plan import ( + NamespacedName, + PlanIdentity, + PlanTimestamps, + ProjectLink, + ) + + now = datetime.now(tz=UTC) + return LifecyclePlan( + identity=PlanIdentity(plan_id=plan_id), + namespaced_name=NamespacedName( + server=None, namespace="local", name="test-plan" + ), + action_name="local/test-action", + description="Test plan for yes-flag scenarios", + definition_of_done="All tests pass", + phase=phase, + processing_state=state, + project_links=[ProjectLink(project_name="proj-1")], + strategy_actor="openai/gpt-4", + execution_actor="openai/gpt-4", + created_by="test-user", + reusable=True, + read_only=False, + timestamps=PlanTimestamps(created_at=now, updated_at=now), + ) + + +def _build_mock_service() -> MagicMock: + """Build a mock lifecycle service for --yes flag tests. + + Returns a ``MagicMock`` whose ``get_plan`` returns a valid plan in + Execute/complete state and whose ``apply_plan`` returns a plan in + Apply/queued state. + """ + mock_service = MagicMock() + mock_service.get_plan.return_value = _make_mock_plan() + mock_service.apply_plan.return_value = _make_mock_plan( + phase=PlanPhase.APPLY, state=ProcessingState.QUEUED + ) + return mock_service + + +# --------------------------------------------------------------------------- +# GIVEN +# --------------------------------------------------------------------------- @given("a plan CLI runner for the yes-flag test") @@ -28,28 +91,101 @@ def step_plan_cli_runner(context: Context) -> None: context.apply_yes_runner = CliRunner() -@when("I invoke lifecycle-apply with --yes flag") -def step_invoke_lifecycle_apply_yes(context: Context) -> None: - """Invoke ``plan lifecycle-apply --yes ``. +# --------------------------------------------------------------------------- +# WHEN — flag recognition (with mocked service for proper prompt testing) +# --------------------------------------------------------------------------- - We pass a dummy plan ID because we only care whether the ``--yes`` - flag is recognised by the CLI framework, not whether the plan - exists. If ``--yes`` is unknown, Typer exits with code 2 *before* - any business logic runs. + +@when("I invoke lifecycle-apply with --yes flag and a mocked service") +def step_invoke_lifecycle_apply_yes_mocked(context: Context) -> None: + """Invoke ``plan lifecycle-apply --yes `` with a mocked service. + + The lifecycle service is mocked so the command completes the full + happy path. This allows us to verify that ``--yes`` genuinely + suppresses the confirmation prompt (not just that the command + crashes before reaching it). """ - context.apply_yes_result = context.apply_yes_runner.invoke( - plan_app, - ["lifecycle-apply", "--yes", "DUMMY_PLAN_ID"], - ) + mock_service = _build_mock_service() + context.apply_yes_mock_service = mock_service + + with patch(_PATCH_LIFECYCLE, return_value=mock_service): + context.apply_yes_result = context.apply_yes_runner.invoke( + plan_app, + ["lifecycle-apply", "--yes", "01JAAAAAAAAAAAAAAAAAAAAAAA"], + ) -@when("I invoke lifecycle-apply with -y flag") -def step_invoke_lifecycle_apply_y(context: Context) -> None: - """Invoke ``plan lifecycle-apply -y ``.""" - context.apply_yes_result = context.apply_yes_runner.invoke( - plan_app, - ["lifecycle-apply", "-y", "DUMMY_PLAN_ID"], - ) +@when("I invoke lifecycle-apply with -y flag and a mocked service") +def step_invoke_lifecycle_apply_y_mocked(context: Context) -> None: + """Invoke ``plan lifecycle-apply -y `` with a mocked service.""" + mock_service = _build_mock_service() + context.apply_yes_mock_service = mock_service + + with patch(_PATCH_LIFECYCLE, return_value=mock_service): + context.apply_yes_result = context.apply_yes_runner.invoke( + plan_app, + ["lifecycle-apply", "-y", "01JAAAAAAAAAAAAAAAAAAAAAAA"], + ) + + +# --------------------------------------------------------------------------- +# WHEN — confirmation prompt behaviour +# --------------------------------------------------------------------------- + + +@when("I invoke lifecycle-apply without --yes and decline") +def step_invoke_lifecycle_apply_decline(context: Context) -> None: + """Invoke ``lifecycle-apply`` without ``--yes``, supply ``n`` to the prompt.""" + mock_service = _build_mock_service() + context.apply_yes_mock_service = mock_service + + with patch(_PATCH_LIFECYCLE, return_value=mock_service): + context.apply_yes_result = context.apply_yes_runner.invoke( + plan_app, + ["lifecycle-apply", "01JAAAAAAAAAAAAAAAAAAAAAAA"], + input="n\n", + ) + + +@when("I invoke lifecycle-apply without --yes and accept") +def step_invoke_lifecycle_apply_accept(context: Context) -> None: + """Invoke ``lifecycle-apply`` without ``--yes``, supply ``y`` to the prompt.""" + mock_service = _build_mock_service() + context.apply_yes_mock_service = mock_service + + with patch(_PATCH_LIFECYCLE, return_value=mock_service): + context.apply_yes_result = context.apply_yes_runner.invoke( + plan_app, + ["lifecycle-apply", "01JAAAAAAAAAAAAAAAAAAAAAAA"], + input="y\n", + ) + + +# --------------------------------------------------------------------------- +# WHEN — unexpected error scenario +# --------------------------------------------------------------------------- + + +@when("I invoke lifecycle-apply --yes with a service that raises an unexpected error") +def step_invoke_lifecycle_apply_unexpected_error(context: Context) -> None: + """Invoke ``lifecycle-apply --yes`` with a service whose ``apply_plan`` + raises a ``RuntimeError`` to exercise the catch-all ``except Exception`` + handler. + """ + mock_service = _build_mock_service() + mock_service.apply_plan.side_effect = RuntimeError("Simulated unexpected failure") + context.apply_yes_mock_service = mock_service + + with patch(_PATCH_LIFECYCLE, return_value=mock_service): + context.apply_yes_result = context.apply_yes_runner.invoke( + plan_app, + ["lifecycle-apply", "--yes", "01JAAAAAAAAAAAAAAAAAAAAAAA"], + ) + + +# --------------------------------------------------------------------------- +# THEN — flag recognition +# --------------------------------------------------------------------------- @then("the lifecycle-apply --yes invocation should not report an unknown option") @@ -89,3 +225,93 @@ def step_assert_y_recognised(context: Context) -> None: f"Exit code: {exit_code}\n" f"Output:\n{output}" ) + + +# --------------------------------------------------------------------------- +# THEN — prompt suppression (unified for both --yes and -y) +# --------------------------------------------------------------------------- + + +@then("the lifecycle-apply {flag} output should not contain the confirmation prompt") +def step_assert_flag_no_prompt(context: Context, flag: str) -> None: + """When ``--yes`` or ``-y`` is supplied, the confirmation prompt must not appear.""" + output = context.apply_yes_result.output + assert _CONFIRM_FRAGMENT not in output, ( + f"The {flag} flag should suppress the confirmation prompt, " + f"but the output contained '{_CONFIRM_FRAGMENT}'.\n" + f"Output:\n{output}" + ) + + +# --------------------------------------------------------------------------- +# THEN — exit code +# --------------------------------------------------------------------------- + + +@then("the lifecycle-apply exit code should be {expected_code:d}") +def step_assert_exit_code(context: Context, expected_code: int) -> None: + """Assert the CLI invocation exited with the expected code.""" + actual = context.apply_yes_result.exit_code + output = context.apply_yes_result.output + assert actual == expected_code, ( + f"Expected exit code {expected_code}, got {actual}.\nOutput:\n{output}" + ) + + +@then("the lifecycle-apply exit code should be non-zero") +def step_assert_exit_code_nonzero(context: Context) -> None: + """Assert the CLI invocation exited with a non-zero code.""" + actual = context.apply_yes_result.exit_code + output = context.apply_yes_result.output + assert actual != 0, f"Expected non-zero exit code, got {actual}.\nOutput:\n{output}" + + +# --------------------------------------------------------------------------- +# THEN — mock service call verification +# --------------------------------------------------------------------------- + + +@then("the lifecycle-apply should have called apply") +def step_assert_apply_called(context: Context) -> None: + """Assert that ``apply_plan`` was called on the mock service.""" + mock_service: MagicMock = context.apply_yes_mock_service + mock_service.apply_plan.assert_called_once() + + +@then("the lifecycle-apply should not have called apply") +def step_assert_apply_not_called(context: Context) -> None: + """Assert that ``apply_plan`` was NOT called on the mock service.""" + mock_service: MagicMock = context.apply_yes_mock_service + mock_service.apply_plan.assert_not_called() + + +# --------------------------------------------------------------------------- +# THEN — confirmation prompt behaviour +# --------------------------------------------------------------------------- + + +@then('the lifecycle-apply output should contain "{text}"') +def step_assert_output_contains(context: Context, text: str) -> None: + """Assert the CLI output contains the given text.""" + output = context.apply_yes_result.output + assert text in output, f"Expected output to contain '{text}'.\nOutput:\n{output}" + + +@then('the lifecycle-apply output should not contain "{text}"') +def step_assert_output_not_contains(context: Context, text: str) -> None: + """Assert the CLI output does NOT contain the given text.""" + output = context.apply_yes_result.output + assert text not in output, ( + f"Expected output NOT to contain '{text}'.\nOutput:\n{output}" + ) + + +@then("the lifecycle-apply output should contain the confirmation prompt") +def step_assert_output_has_prompt(context: Context) -> None: + """Assert the confirmation prompt text appeared in the output.""" + output = context.apply_yes_result.output + assert _CONFIRM_FRAGMENT in output, ( + f"Expected output to contain the confirmation prompt " + f"'{_CONFIRM_FRAGMENT}', but it was not found.\n" + f"Output:\n{output}" + ) diff --git a/features/tdd_plan_apply_yes_flag.feature b/features/tdd_plan_apply_yes_flag.feature index 66476e82f..325ee2c8c 100644 --- a/features/tdd_plan_apply_yes_flag.feature +++ b/features/tdd_plan_apply_yes_flag.feature @@ -1,22 +1,50 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_932 +@tdd_bug @tdd_bug_932 Feature: TDD Bug #932 — plan apply missing --yes flag As a developer I want to verify that `agents plan lifecycle-apply` accepts the --yes flag required by the specification So that the bug is captured and will be caught by a regression test - The specification mandates `agents plan apply [--yes|-y] ` - but the current `lifecycle-apply` implementation does not accept - `--yes` or `-y`. Other destructive CLI commands (`session delete`, - `project delete`, `plan correct`, `plan rollback`) correctly implement - the `--yes`/`-y` pattern to skip confirmation prompts. + These scenarios verify that `lifecycle-apply` correctly handles + `--yes`/`-y` flags as a regression guard. Other destructive CLI + commands (`session delete`, `project delete`, `plan correct`, + `plan rollback`) correctly implement the `--yes`/`-y` pattern to skip + confirmation prompts. Scenario: lifecycle-apply recognises the --yes long flag Given a plan CLI runner for the yes-flag test - When I invoke lifecycle-apply with --yes flag + When I invoke lifecycle-apply with --yes flag and a mocked service Then the lifecycle-apply --yes invocation should not report an unknown option + And the lifecycle-apply --yes output should not contain the confirmation prompt + And the lifecycle-apply exit code should be 0 + And the lifecycle-apply should have called apply Scenario: lifecycle-apply recognises the -y short flag Given a plan CLI runner for the yes-flag test - When I invoke lifecycle-apply with -y flag + When I invoke lifecycle-apply with -y flag and a mocked service Then the lifecycle-apply -y invocation should not report an unknown option + And the lifecycle-apply -y output should not contain the confirmation prompt + And the lifecycle-apply exit code should be 0 + And the lifecycle-apply should have called apply + + Scenario: lifecycle-apply without --yes prompts for confirmation and user declines + Given a plan CLI runner for the yes-flag test + When I invoke lifecycle-apply without --yes and decline + Then the lifecycle-apply output should contain "Apply cancelled." + And the lifecycle-apply exit code should be 0 + And the lifecycle-apply should not have called apply + + Scenario: lifecycle-apply without --yes prompts for confirmation and user accepts + Given a plan CLI runner for the yes-flag test + When I invoke lifecycle-apply without --yes and accept + Then the lifecycle-apply output should not contain "Apply cancelled." + And the lifecycle-apply output should contain the confirmation prompt + And the lifecycle-apply exit code should be 0 + And the lifecycle-apply should have called apply + + Scenario: lifecycle-apply catches unexpected exceptions cleanly + Given a plan CLI runner for the yes-flag test + When I invoke lifecycle-apply --yes with a service that raises an unexpected error + Then the lifecycle-apply output should contain "Unexpected error" + And the lifecycle-apply output should not contain "Traceback" + And the lifecycle-apply exit code should be non-zero diff --git a/robot/e2e/m2_acceptance.robot b/robot/e2e/m2_acceptance.robot index adb717ccc..6d3f0561b 100644 --- a/robot/e2e/m2_acceptance.robot +++ b/robot/e2e/m2_acceptance.robot @@ -117,7 +117,7 @@ M2 Full Actor Compiler And LLM Integration # ---- Step 9: Plan apply ---- ${r_apply}= Run CleverAgents Command - ... plan lifecycle-apply ${plan_id} --format plain + ... plan lifecycle-apply --yes ${plan_id} --format plain Should Not Contain ${r_apply.stdout}${r_apply.stderr} INTERNAL Should Not Contain ${r_apply.stdout}${r_apply.stderr} Traceback Log Apply output: ${r_apply.stdout} diff --git a/robot/e2e/m6_acceptance.robot b/robot/e2e/m6_acceptance.robot index dfc928102..b3be74c87 100644 --- a/robot/e2e/m6_acceptance.robot +++ b/robot/e2e/m6_acceptance.robot @@ -101,7 +101,7 @@ Full Flow Apply Step ... Apply may fail if the plan is not in the correct state after ... LLM execution; hard assertions on the success path only. [Arguments] ${plan_id} - ${apply}= Run CleverAgents Command plan lifecycle-apply ${plan_id} --format json expected_rc=None timeout=180s + ${apply}= Run CleverAgents Command plan lifecycle-apply --yes ${plan_id} --format json expected_rc=None timeout=180s Log Apply result rc=${apply.rc} stdout=${apply.stdout} stderr=${apply.stderr} IF ${apply.rc} == 0 Output Should Contain ${apply} ${plan_id} diff --git a/robot/e2e/wf05_db_migration.robot b/robot/e2e/wf05_db_migration.robot index 28fcd49fd..0eb941a9b 100644 --- a/robot/e2e/wf05_db_migration.robot +++ b/robot/e2e/wf05_db_migration.robot @@ -493,7 +493,7 @@ WF05 Database Schema Migration With Safety Nets Review Profile Log Baseline SHA before apply: ${baseline_sha} ${r_apply}= Run CleverAgents Command - ... plan lifecycle-apply ${plan_id} --format json + ... plan lifecycle-apply --yes ${plan_id} --format json ... expected_rc=None timeout=180s Log Apply rc=${r_apply.rc} stdout=${r_apply.stdout} IF ${r_apply.rc} == 0 diff --git a/robot/helper_cli_lifecycle.py b/robot/helper_cli_lifecycle.py index c940610de..41f5437ff 100644 --- a/robot/helper_cli_lifecycle.py +++ b/robot/helper_cli_lifecycle.py @@ -248,7 +248,7 @@ def plan_apply_changeset() -> None: with patch( "cleveragents.cli.commands.plan._get_lifecycle_service", return_value=svc ): - result = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + result = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) if result.exit_code != 0: _fail(f"plan apply rc={result.exit_code}\n{result.output}") @@ -309,7 +309,7 @@ def full_lifecycle() -> None: assert r2.exit_code == 0, f"plan use: {r2.output}" r3 = runner.invoke(plan_app, ["execute", _PLAN_ULID]) assert r3.exit_code == 0, f"plan execute: {r3.output}" - r4 = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + r4 = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) assert r4.exit_code == 0, f"plan apply: {r4.output}" cs = store.get(cid) diff --git a/robot/helper_cli_lifecycle_e2e.py b/robot/helper_cli_lifecycle_e2e.py index 386cfb3e5..d1aeb6d09 100644 --- a/robot/helper_cli_lifecycle_e2e.py +++ b/robot/helper_cli_lifecycle_e2e.py @@ -191,7 +191,7 @@ def plan_apply() -> None: "cleveragents.cli.commands.plan._get_lifecycle_service", return_value=mock_service, ): - result = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + result = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) if result.exit_code == 0: print("cli-lifecycle-plan-apply-ok") else: @@ -310,7 +310,7 @@ def full_lifecycle() -> None: assert r3.exit_code == 0, f"plan execute failed: {r3.output}" # Step 4: Plan apply - r4 = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + r4 = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) assert r4.exit_code == 0, f"plan apply failed: {r4.output}" print("cli-lifecycle-full-e2e-ok") diff --git a/robot/helper_m1_e2e_verification.py b/robot/helper_m1_e2e_verification.py index 337704ad1..ad1f27f54 100644 --- a/robot/helper_m1_e2e_verification.py +++ b/robot/helper_m1_e2e_verification.py @@ -391,7 +391,7 @@ def plan_full_lifecycle() -> None: _fail(f"plan diff crashed:\n{combined5}") # Step 6: Plan lifecycle-apply — similar: plan not ready - r6 = run_cli("plan", "lifecycle-apply", plan_id, workspace=workspace) + r6 = run_cli("plan", "lifecycle-apply", "--yes", plan_id, workspace=workspace) combined6 = r6.stdout + r6.stderr if "Traceback" in combined6: _fail(f"plan apply crashed:\n{combined6}") diff --git a/robot/helper_m1_sourcecode_smoke.py b/robot/helper_m1_sourcecode_smoke.py index 464f7779c..206cd1456 100644 --- a/robot/helper_m1_sourcecode_smoke.py +++ b/robot/helper_m1_sourcecode_smoke.py @@ -200,7 +200,7 @@ def plan_apply() -> None: "cleveragents.cli.commands.plan._get_lifecycle_service", return_value=mock_svc, ): - result = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + result = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) if result.exit_code == 0: print("m1-plan-apply-ok") else: @@ -260,7 +260,7 @@ def full_lifecycle() -> None: sys.exit(1) # Step 4: plan apply - r4 = runner.invoke(plan_app, ["lifecycle-apply", _PLAN_ULID]) + r4 = runner.invoke(plan_app, ["lifecycle-apply", "--yes", _PLAN_ULID]) if r4.exit_code != 0: print(f"FAIL step 4: exit={r4.exit_code} output={r4.output}") sys.exit(1) diff --git a/robot/helper_m6_e2e_verification.py b/robot/helper_m6_e2e_verification.py index 94d307c37..ef0727a3a 100644 --- a/robot/helper_m6_e2e_verification.py +++ b/robot/helper_m6_e2e_verification.py @@ -624,7 +624,7 @@ def plan_apply_lifecycle() -> None: # lifecycle-apply on a strategize/queued plan — should get # a controlled rejection, not a crash. - r3 = run_cli("plan", "lifecycle-apply", plan_id, workspace=workspace) + r3 = run_cli("plan", "lifecycle-apply", "--yes", plan_id, workspace=workspace) combined = r3.stdout + r3.stderr if "INTERNAL" in combined or "Traceback" in combined: _fail(f"lifecycle-apply crashed:\n{combined}") diff --git a/robot/helper_tdd_plan_apply_yes_flag.py b/robot/helper_tdd_plan_apply_yes_flag.py index 61d1e7767..9872f9a4e 100644 --- a/robot/helper_tdd_plan_apply_yes_flag.py +++ b/robot/helper_tdd_plan_apply_yes_flag.py @@ -6,8 +6,9 @@ that the flag is recognised by the CLI framework. The helper reports the **real** outcome: it exits 0 and prints the sentinel when the flag is accepted (bug is fixed), and exits 1 when the flag is -rejected (bug still present). The ``tdd_expected_fail_listener`` on the -Robot side handles pass/fail inversion while the bug remains open. +rejected (bug still present). The bug is now fixed and the +``@tdd_expected_fail`` tag has been removed; these tests serve as permanent +regression guards. """ from __future__ import annotations diff --git a/robot/tdd_plan_apply_yes_flag.robot b/robot/tdd_plan_apply_yes_flag.robot index 5a9ea83aa..c24917a98 100644 --- a/robot/tdd_plan_apply_yes_flag.robot +++ b/robot/tdd_plan_apply_yes_flag.robot @@ -4,8 +4,7 @@ Documentation TDD Bug #932 — plan lifecycle-apply missing --yes flag ... command accepts the --yes / -y flag required by the ... specification. The spec mandates ... ``agents plan apply [--yes|-y] `` to skip the -... confirmation prompt, but the current implementation does not -... recognise this flag. +... confirmation prompt. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -16,7 +15,7 @@ ${HELPER} ${CURDIR}/helper_tdd_plan_apply_yes_flag.py *** Test Cases *** TDD Plan Apply Yes Long Flag Via CLI [Documentation] Verify that ``lifecycle-apply --yes`` is recognised - [Tags] tdd_expected_fail tdd_bug tdd_bug_932 + [Tags] tdd_bug tdd_bug_932 ${result}= Run Process ${PYTHON} ${HELPER} check-yes-long cwd=${WORKSPACE} timeout=30s on_timeout=kill Log ${result.stdout} Log ${result.stderr} @@ -25,7 +24,7 @@ TDD Plan Apply Yes Long Flag Via CLI TDD Plan Apply Yes Short Flag Via CLI [Documentation] Verify that ``lifecycle-apply -y`` is recognised - [Tags] tdd_expected_fail tdd_bug tdd_bug_932 + [Tags] tdd_bug tdd_bug_932 ${result}= Run Process ${PYTHON} ${HELPER} check-yes-short cwd=${WORKSPACE} timeout=30s 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 c2538f090..72d986b2a 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -50,6 +50,7 @@ from cleveragents.core.exceptions import ( PlanError, ValidationError, ) +from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState # Regex for validating namespaced actor names (namespace/name format) _NAMESPACED_ACTOR_RE = re.compile(r"^[a-z][a-z0-9-]*/[a-z][a-z0-9._-]*$") @@ -2007,6 +2008,14 @@ def lifecycle_apply_plan( str | None, typer.Argument(help="Plan ID to apply (optional if only one plan)"), ] = None, + yes: Annotated[ + bool, + typer.Option( + "--yes", + "-y", + help="Skip confirmation prompt", + ), + ] = False, fmt: Annotated[ str, typer.Option( @@ -2020,6 +2029,8 @@ def lifecycle_apply_plan( The plan must be in Execute phase with 'complete' state. This is different from the legacy 'apply' command which applies file changes. + + Use ``--yes`` / ``-y`` to skip the confirmation prompt in scripts. """ from cleveragents.application.services.plan_lifecycle_service import ( InvalidPhaseTransitionError, @@ -2032,10 +2043,6 @@ def lifecycle_apply_plan( if not plan_id: # Try to find plans eligible for apply: Execute/complete or # Apply/queued (auto_progress may have already advanced). - from cleveragents.domain.models.core.plan import ( - PlanPhase, - ProcessingState, - ) exec_plans = service.list_plans(phase=PlanPhase.EXECUTE) apply_plans = service.list_plans(phase=PlanPhase.APPLY) @@ -2060,17 +2067,25 @@ def lifecycle_apply_plan( # Fail-fast: read-only plans must not enter Apply phase pre_plan = service.get_plan(plan_id) - if pre_plan is not None and pre_plan.read_only is True: + if pre_plan.read_only is True: console.print( f"[red]Cannot apply plan '{plan_id}': plan is read-only.[/red]" ) raise typer.Abort() - from cleveragents.domain.models.core.plan import PlanPhase + # Confirmation prompt — Apply is destructive (merges sandbox into + # real resources). Skip when --yes / -y is provided. + if not yes: + confirm = typer.confirm( + f"\nApply changes for plan {plan_id}?", default=False + ) + if not confirm: + console.print("[yellow]Apply cancelled.[/yellow]") + raise typer.Exit(0) # If auto_progress already moved the plan to Apply, skip the # explicit transition and just report the current state. - if pre_plan is not None and pre_plan.phase == PlanPhase.APPLY: + if pre_plan.phase == PlanPhase.APPLY: plan = pre_plan else: # Normal path: plan is in Execute/complete → transition @@ -2092,9 +2107,20 @@ def lifecycle_apply_plan( except PlanNotReadyError as e: console.print(f"[red]Plan not ready:[/red] {e}") raise typer.Abort() from e + except ValueError as e: + # Provider/config resolution errors can occur during apply when + # required model credentials are unavailable in the environment. + # Surface as a controlled CLI abort instead of an unhandled traceback. + console.print(f"[red]Execution Error:[/red] {e}") + raise typer.Abort() from e except CleverAgentsError as e: console.print(f"[red]Error:[/red] {e.message}") raise typer.Abort() from e + except Exception as e: + if isinstance(e, (typer.Abort, typer.Exit)): + raise + console.print(f"[red]Unexpected error:[/red] {e}") + raise typer.Abort() from e @app.command("status") -- 2.52.0