cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(cli): add missing --yes flag to plan apply command #1127

Merged
hurui200320 merged 1 commits from fix/m4-plan-apply-yes-flag into master 2026-03-26 07:50:10 +00:00
Conversation 5 Commits 1 Files Changed 26
+381 -79
hurui200320 commented 2026-03-23 07:17:50 +00:00
Member
Copy Link
Copy Source

Summary

Implements ticket #932 by adding --yes/-y support to plan lifecycle-apply (spec: agents plan apply [--yes|-y] <PLAN_ID>), preserving interactive safety by default while enabling non-interactive CI/script execution.

Closes #932.

What this PR now includes

Core ticket implementation

  • Added --yes/-y to lifecycle_apply_plan in src/cleveragents/cli/commands/plan.py.
  • Added confirmation prompt when --yes is not provided.
  • Kept prompt text spec-aligned: Apply changes for plan <ID>? [y/N]:.
  • Added robust exception handling alignment (ValueError + guarded catch-all) consistent with neighboring lifecycle commands.
  • Moved PlanPhase / ProcessingState imports to module level per CONTRIBUTING.md import rules.

Test/documentation updates (from earlier review cycles)

  • Removed TDD expected-fail inversion tags for the fixed bug tests.
  • Updated Behave/Robot/benchmark call sites that require non-interactive apply flow.
  • Expanded lifecycle-apply CLI reference docs in docs/reference/plan_cli.md.

Cycle 7 fix (this update)

  • Rebased fix/m4-plan-apply-yes-flag onto latest origin/master.
  • Fixed the failing WF05 E2E path by making apply non-interactive:
    • robot/e2e/wf05_db_migration.robot
    • plan lifecycle-apply ${plan_id} --format json
    • → plan lifecycle-apply --yes ${plan_id} --format json
  • This addresses the pipeline failure where Robot hit an interactive prompt and returned rc=1.

Deferred / Known limitations

  • M3 ambiguity remains deferred: ticket acceptance text mentions showing a pre-confirmation summary of pending changes, while the spec example shows only the confirmation prompt before apply. Current implementation matches the spec example.
  • Legacy apply command --yes forwarding behavior remains outside this ticket scope.

Quality gates (latest rerun on rebased branch)

Gate Result
nox -e lint ✅ pass
nox -e typecheck ✅ pass (0 errors)
nox -e unit_tests ✅ pass
nox -e integration_tests ✅ pass
nox -e e2e_tests ✅ pass (42 tests, 0 failed)
nox -e coverage_report ✅ pass (98%, threshold >=97%)

Notes for reviewers

  • Scope of this cycle is intentionally minimal: rebase refresh + explicit --yes in WF05 E2E apply step.
  • No functional expansion beyond preserving the intended non-interactive behavior introduced by ticket #932.
## Summary Implements ticket #932 by adding `--yes/-y` support to `plan lifecycle-apply` (spec: `agents plan apply [--yes|-y] <PLAN_ID>`), preserving interactive safety by default while enabling non-interactive CI/script execution. Closes #932. ## What this PR now includes ### Core ticket implementation - Added `--yes/-y` to `lifecycle_apply_plan` in `src/cleveragents/cli/commands/plan.py`. - Added confirmation prompt when `--yes` is not provided. - Kept prompt text spec-aligned: `Apply changes for plan <ID>? [y/N]:`. - Added robust exception handling alignment (`ValueError` + guarded catch-all) consistent with neighboring lifecycle commands. - Moved `PlanPhase` / `ProcessingState` imports to module level per `CONTRIBUTING.md` import rules. ### Test/documentation updates (from earlier review cycles) - Removed TDD expected-fail inversion tags for the fixed bug tests. - Updated Behave/Robot/benchmark call sites that require non-interactive apply flow. - Expanded `lifecycle-apply` CLI reference docs in `docs/reference/plan_cli.md`. ### Cycle 7 fix (this update) - Rebased `fix/m4-plan-apply-yes-flag` onto latest `origin/master`. - Fixed the failing WF05 E2E path by making apply non-interactive: - `robot/e2e/wf05_db_migration.robot` - `plan lifecycle-apply ${plan_id} --format json` - → `plan lifecycle-apply --yes ${plan_id} --format json` - This addresses the pipeline failure where Robot hit an interactive prompt and returned `rc=1`. ## Deferred / Known limitations - **M3 ambiguity remains deferred:** ticket acceptance text mentions showing a pre-confirmation summary of pending changes, while the spec example shows only the confirmation prompt before apply. Current implementation matches the spec example. - Legacy `apply` command `--yes` forwarding behavior remains outside this ticket scope. ## Quality gates (latest rerun on rebased branch) | Gate | Result | |---|---| | `nox -e lint` | ✅ pass | | `nox -e typecheck` | ✅ pass (0 errors) | | `nox -e unit_tests` | ✅ pass | | `nox -e integration_tests` | ✅ pass | | `nox -e e2e_tests` | ✅ pass (42 tests, 0 failed) | | `nox -e coverage_report` | ✅ pass (98%, threshold >=97%) | ## Notes for reviewers - Scope of this cycle is intentionally minimal: rebase refresh + explicit `--yes` in WF05 E2E apply step. - No functional expansion beyond preserving the intended non-interactive behavior introduced by ticket #932.
hurui200320 added this to the v3.3.0 milestone 2026-03-23 07:17:57 +00:00
hurui200320 added the
Type
Bug
 label 2026-03-23 07:17:58 +00:00
hurui200320 added a new dependency 2026-03-23 07:21:05 +00:00
#932 fix(cli): add missing --yes flag to plan apply command
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from e5faeff6ed to e144fcc019 2026-03-23 13:15:47 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from e144fcc019 to 28acb4eaaf 2026-03-23 13:48:48 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 28acb4eaaf to 3d7bf9c0f6 2026-03-24 05:40:48 +00:00 Compare
CoreRasurae requested review from CoreRasurae 2026-03-24 10:43:56 +00:00
CoreRasurae reviewed 2026-03-24 11:11:29 +00:00
CoreRasurae left a comment
Member
Copy Link
Copy Source

Code Review Report -- PR #1127 (Issue #932)

Branch: fix/m4-plan-apply-yes-flag
Scope: All code changes in the branch plus close connections to surrounding code
Review cycles: 4 global passes across all categories (bugs, test coverage, test flaws, performance, security, spec compliance, code quality)

Overall Assessment

The implementation correctly adds the --yes/-y flag to lifecycle-apply, follows the established Typer Option pattern from sibling destructive commands, properly removes the @tdd_expected_fail tag, and comprehensively updates all existing test call-sites. The new Behave scenarios cover flag recognition, prompt suppression, decline behavior, and accept behavior with mock service verification. The benchmarks and Robot tests are correctly updated.

That said, 4 review cycles surfaced the following findings, organized by severity.

MEDIUM Severity

M1. Bug/Inconsistency -- typer.Abort() on user decline produces exit code 1

File: src/cleveragents/cli/commands/plan.py:2085

When the user declines the confirmation prompt, the code raises typer.Abort(), which produces exit code 1 and appends a spurious "Aborted." line to the output. A user declining a destructive action is not an error condition -- it is a normal, expected interaction.

Compare with sibling commands in the same file:

Command On decline Exit code
lifecycle-apply (this PR) typer.Abort() 1
rollback typer.Abort() 1
correct typer.Exit(0) 0
Legacy apply typer.Exit(0) 0

correct and the legacy apply use typer.Exit(0) -- the more appropriate pattern for a user-initiated cancellation. Using Abort() means: (a) the output contains both "Apply cancelled." and "Aborted." (redundant/confusing), (b) exit code 1 can break scripts that check $? to detect real errors, and (c) inconsistency with correct and legacy apply in the same file.

Recommendation: Change line 2085 from raise typer.Abort() to raise typer.Exit(0).

M2. Test Coverage Gap -- Missing exit code assertion on decline scenario

File: features/tdd_plan_apply_yes_flag.feature:28-32

The "user declines" scenario does not assert the exit code:

Scenario: lifecycle-apply without --yes prompts for confirmation and user declines
    ...
    Then the lifecycle-apply output should contain "Apply cancelled."
    And the lifecycle-apply should not have called apply
    # No exit code assertion

All three other scenarios in the same feature file assert the lifecycle-apply exit code should be 0. This asymmetry means the decline path's exit code (currently 1 from typer.Abort()) is untested. If M1 is fixed to use typer.Exit(0), a test should verify exit code 0 on decline.

Recommendation: Add And the lifecycle-apply exit code should be 0 (or the appropriate expected code) to the decline scenario.

M3. Spec Compliance -- Acceptance criterion "summary of pending changes" not implemented

File: src/cleveragents/cli/commands/plan.py:2082

Issue #932's acceptance criteria states:

"Without --yes, a confirmation prompt is displayed showing the plan ID and a summary of pending changes before proceeding"

The current implementation shows only:

Apply changes for plan 01JAAAAAAAAAAAAAAAAAAAAAAA? [y/N]:

No summary of pending changes (files affected, insertions/deletions, affected projects) is displayed before the user confirms. The acceptance criterion checkbox [x] is checked in the issue body, but the "summary of pending changes" part is not satisfied.

Note: The specification's own example output (docs/specification.md line 13234) shows the summary after confirmation, not before, so the spec and the issue criteria are inconsistent with each other. The implementation matches the spec example but not the issue acceptance criteria.

Recommendation: Either implement a pre-confirmation summary (e.g., plan phase, project names, artifact count) or update the issue acceptance criteria to clarify that the summary appears post-confirmation. Consider this deferrable to a follow-up if agreed.

LOW Severity

L1. Robustness -- Missing except Exception catch-all handler

File: src/cleveragents/cli/commands/plan.py:2107-2115

The lifecycle_apply_plan command's exception handlers end at except CleverAgentsError. The neighboring lifecycle_execute_plan command (line 1999) includes a catch-all except Exception as e: that produces a clean "[red]Unexpected error:[/red]" message. Without this, an unexpected exception (e.g., TypeError from a service bug) would produce a raw traceback.

Note: If a catch-all is added, it must re-raise typer.Abort and typer.Exit to avoid intercepting the decline path and other early-abort paths. The pattern used elsewhere in the codebase is:

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

L2. Documentation -- Description text not updated

File: docs/reference/plan_cli.md:148-153

The synopsis line was updated to include [--yes|-y], but the description still only says "Transition a plan from Execute to Apply phase." There is no mention of the confirmation prompt, what --yes does, or that Apply is destructive. Other commands in the same file have richer descriptions.

L3. Code Quality -- Dead code is not None guards

File: src/cleveragents/cli/commands/plan.py:2073, 2091

service.get_plan(plan_id) at line 2072 never returns None -- it raises NotFoundError (a CleverAgentsError subclass) if the plan does not exist. Therefore the if pre_plan is not None checks at lines 2073 and 2091 are unreachable dead code. They are defensive but misleading, suggesting None is a possible return value when the API contract guarantees it is not.

L4. Test Coverage Gap -- No test for --yes after positional argument

All test files

Every test invocation uses ["lifecycle-apply", "--yes", plan_id] (flag before argument). No test exercises ["lifecycle-apply", plan_id, "--yes"]. While Typer/Click supports both orderings, this alternate ordering is untested.

L5. Test Coverage Gap -- No test for auto-select + interactive prompt

File: features/tdd_plan_apply_yes_flag.feature

All TDD scenarios pass an explicit plan ID. No test anywhere exercises the combined path: no plan_id given -> auto-select resolves one plan -> confirmation prompt appears -> user accepts/declines. All existing auto-select tests in other files pass --yes, skipping the prompt entirely.

INFORMATIONAL

I1. Code Quality -- Duplicate PlanPhase import

PlanPhase is imported at line 2045 (inside the if not plan_id: block) and again at line 2087 (unconditionally). The second import is necessary (for when plan_id is provided directly), but the visual redundancy could be eliminated by hoisting the import to the top of the try block.

I2. Code Quality -- typer.confirm without explicit default=False

Line 2082 uses typer.confirm(...) without default=False. The implicit default is False (same behavior), but session delete and skill remove specify it explicitly for readability. Minor inconsistency.

I3. Test Design -- Robot helper only tests flag recognition

robot/helper_tdd_plan_apply_yes_flag.py uses DUMMY_PLAN_ID (not a valid ULID), so the command fails after flag parsing. This is intentional (the helper only tests that --yes/-y are recognized), but it provides no assurance about the correctness of the --yes code path beyond argument parsing.

Summary

Severity Count Categories
Medium 3 Bug/Inconsistency (M1), Test Coverage (M2), Spec Compliance (M3)
Low 5 Robustness (L1), Documentation (L2), Code Quality (L3), Test Coverage (L4, L5)
Informational 3 Code Quality (I1, I2), Test Design (I3)

The core implementation is sound and the fix achieves its primary goal. M1 (Abort vs Exit on decline) is the most actionable finding as it affects script compatibility and output cleanliness. M2 follows naturally from M1. M3 may be deferrable depending on team agreement on the acceptance criteria interpretation.

## Code Review Report -- PR #1127 (Issue #932) **Branch:** `fix/m4-plan-apply-yes-flag` **Scope:** All code changes in the branch plus close connections to surrounding code **Review cycles:** 4 global passes across all categories (bugs, test coverage, test flaws, performance, security, spec compliance, code quality) --- ### Overall Assessment The implementation correctly adds the `--yes`/`-y` flag to `lifecycle-apply`, follows the established Typer Option pattern from sibling destructive commands, properly removes the `@tdd_expected_fail` tag, and comprehensively updates all existing test call-sites. The new Behave scenarios cover flag recognition, prompt suppression, decline behavior, and accept behavior with mock service verification. The benchmarks and Robot tests are correctly updated. That said, 4 review cycles surfaced the following findings, organized by severity. --- ## MEDIUM Severity ### M1. Bug/Inconsistency -- `typer.Abort()` on user decline produces exit code 1 **File:** `src/cleveragents/cli/commands/plan.py:2085` When the user declines the confirmation prompt, the code raises `typer.Abort()`, which produces **exit code 1** and appends a spurious `"Aborted."` line to the output. A user declining a destructive action is not an error condition -- it is a normal, expected interaction. Compare with sibling commands in the same file: | Command | On decline | Exit code | |---|---|---| | `lifecycle-apply` (this PR) | `typer.Abort()` | **1** | | `rollback` | `typer.Abort()` | **1** | | `correct` | `typer.Exit(0)` | **0** | | Legacy `apply` | `typer.Exit(0)` | **0** | `correct` and the legacy `apply` use `typer.Exit(0)` -- the more appropriate pattern for a user-initiated cancellation. Using `Abort()` means: (a) the output contains both `"Apply cancelled."` and `"Aborted."` (redundant/confusing), (b) exit code 1 can break scripts that check `$?` to detect real errors, and (c) inconsistency with `correct` and legacy `apply` in the same file. **Recommendation:** Change line 2085 from `raise typer.Abort()` to `raise typer.Exit(0)`. --- ### M2. Test Coverage Gap -- Missing exit code assertion on decline scenario **File:** `features/tdd_plan_apply_yes_flag.feature:28-32` The "user declines" scenario does not assert the exit code: ```gherkin Scenario: lifecycle-apply without --yes prompts for confirmation and user declines ... Then the lifecycle-apply output should contain "Apply cancelled." And the lifecycle-apply should not have called apply # No exit code assertion ``` All three other scenarios in the same feature file assert `the lifecycle-apply exit code should be 0`. This asymmetry means the decline path's exit code (currently 1 from `typer.Abort()`) is untested. If M1 is fixed to use `typer.Exit(0)`, a test should verify exit code 0 on decline. **Recommendation:** Add `And the lifecycle-apply exit code should be 0` (or the appropriate expected code) to the decline scenario. --- ### M3. Spec Compliance -- Acceptance criterion "summary of pending changes" not implemented **File:** `src/cleveragents/cli/commands/plan.py:2082` Issue #932's acceptance criteria states: > "Without `--yes`, a confirmation prompt is displayed showing the plan ID **and a summary of pending changes** before proceeding" The current implementation shows only: ``` Apply changes for plan 01JAAAAAAAAAAAAAAAAAAAAAAA? [y/N]: ``` No summary of pending changes (files affected, insertions/deletions, affected projects) is displayed before the user confirms. The acceptance criterion checkbox `[x]` is checked in the issue body, but the "summary of pending changes" part is not satisfied. Note: The specification's own example output (`docs/specification.md` line 13234) shows the summary **after** confirmation, not before, so the spec and the issue criteria are inconsistent with each other. The implementation matches the spec example but not the issue acceptance criteria. **Recommendation:** Either implement a pre-confirmation summary (e.g., plan phase, project names, artifact count) or update the issue acceptance criteria to clarify that the summary appears post-confirmation. Consider this deferrable to a follow-up if agreed. --- ## LOW Severity ### L1. Robustness -- Missing `except Exception` catch-all handler **File:** `src/cleveragents/cli/commands/plan.py:2107-2115` The `lifecycle_apply_plan` command's exception handlers end at `except CleverAgentsError`. The neighboring `lifecycle_execute_plan` command (line 1999) includes a catch-all `except Exception as e:` that produces a clean `"[red]Unexpected error:[/red]"` message. Without this, an unexpected exception (e.g., `TypeError` from a service bug) would produce a raw traceback. **Note:** If a catch-all is added, it **must** re-raise `typer.Abort` and `typer.Exit` to avoid intercepting the decline path and other early-abort paths. The pattern used elsewhere in the codebase is: ```python 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 ``` --- ### L2. Documentation -- Description text not updated **File:** `docs/reference/plan_cli.md:148-153` The synopsis line was updated to include `[--yes|-y]`, but the description still only says *"Transition a plan from Execute to Apply phase."* There is no mention of the confirmation prompt, what `--yes` does, or that Apply is destructive. Other commands in the same file have richer descriptions. --- ### L3. Code Quality -- Dead code `is not None` guards **File:** `src/cleveragents/cli/commands/plan.py:2073, 2091` `service.get_plan(plan_id)` at line 2072 never returns `None` -- it raises `NotFoundError` (a `CleverAgentsError` subclass) if the plan does not exist. Therefore the `if pre_plan is not None` checks at lines 2073 and 2091 are unreachable dead code. They are defensive but misleading, suggesting `None` is a possible return value when the API contract guarantees it is not. --- ### L4. Test Coverage Gap -- No test for `--yes` after positional argument **All test files** Every test invocation uses `["lifecycle-apply", "--yes", plan_id]` (flag before argument). No test exercises `["lifecycle-apply", plan_id, "--yes"]`. While Typer/Click supports both orderings, this alternate ordering is untested. --- ### L5. Test Coverage Gap -- No test for auto-select + interactive prompt **File:** `features/tdd_plan_apply_yes_flag.feature` All TDD scenarios pass an explicit plan ID. No test anywhere exercises the combined path: no `plan_id` given -> auto-select resolves one plan -> confirmation prompt appears -> user accepts/declines. All existing auto-select tests in other files pass `--yes`, skipping the prompt entirely. --- ## INFORMATIONAL ### I1. Code Quality -- Duplicate `PlanPhase` import `PlanPhase` is imported at line 2045 (inside the `if not plan_id:` block) and again at line 2087 (unconditionally). The second import is necessary (for when `plan_id` is provided directly), but the visual redundancy could be eliminated by hoisting the import to the top of the `try` block. ### I2. Code Quality -- `typer.confirm` without explicit `default=False` Line 2082 uses `typer.confirm(...)` without `default=False`. The implicit default is `False` (same behavior), but `session delete` and `skill remove` specify it explicitly for readability. Minor inconsistency. ### I3. Test Design -- Robot helper only tests flag recognition `robot/helper_tdd_plan_apply_yes_flag.py` uses `DUMMY_PLAN_ID` (not a valid ULID), so the command fails after flag parsing. This is intentional (the helper only tests that `--yes`/`-y` are recognized), but it provides no assurance about the correctness of the `--yes` code path beyond argument parsing. --- ## Summary | Severity | Count | Categories | |---|---|---| | Medium | 3 | Bug/Inconsistency (M1), Test Coverage (M2), Spec Compliance (M3) | | Low | 5 | Robustness (L1), Documentation (L2), Code Quality (L3), Test Coverage (L4, L5) | | Informational | 3 | Code Quality (I1, I2), Test Design (I3) | The core implementation is sound and the fix achieves its primary goal. M1 (Abort vs Exit on decline) is the most actionable finding as it affects script compatibility and output cleanliness. M2 follows naturally from M1. M3 may be deferrable depending on team agreement on the acceptance criteria interpretation.
docs/reference/plan_cli.md Outdated
@@ -151,3 +151,3 @@
```bash
agents plan lifecycle-apply [PLAN_ID] [--format FORMAT]
agents plan lifecycle-apply [--yes|-y] [PLAN_ID] [--format FORMAT]
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[L2] The synopsis was updated to include [--yes|-y], but the description text (line 150) still only says "Transition a plan from Execute to Apply phase." Consider expanding to mention the confirmation prompt and the purpose of --yes.

✅ Fixed. Expanded the description to: "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."

**[L2]** The synopsis was updated to include `[--yes|-y]`, but the description text (line 150) still only says "Transition a plan from Execute to Apply phase." Consider expanding to mention the confirmation prompt and the purpose of `--yes`. --- ✅ **Fixed.** Expanded the description to: *"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."*
features/tdd_plan_apply_yes_flag.feature Outdated
@@ -23,0 +29,4 @@
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 should not have called apply
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[M2] This scenario does not assert the exit code. All three other scenarios in this feature assert the lifecycle-apply exit code should be 0. The decline path currently exits with code 1 (from typer.Abort()), but this is untested.

✅ Fixed. Added And the lifecycle-apply exit code should be 0 to the decline scenario. This assertion now passes because M1 was fixed to use typer.Exit(0) instead of typer.Abort().

**[M2]** This scenario does not assert the exit code. All three other scenarios in this feature assert `the lifecycle-apply exit code should be 0`. The decline path currently exits with code 1 (from `typer.Abort()`), but this is untested. --- ✅ **Fixed.** Added `And the lifecycle-apply exit code should be 0` to the decline scenario. This assertion now passes because M1 was fixed to use `typer.Exit(0)` instead of `typer.Abort()`.
src/cleveragents/cli/commands/plan.py Outdated
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[L1] Unlike the neighboring lifecycle_execute_plan (line 1999), this command has no except Exception catch-all. Unexpected exceptions will produce raw tracebacks instead of clean [red]Unexpected error:[/red] messages.

If adding a catch-all, remember to re-raise typer.Abort and typer.Exit:

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

✅ Fixed. Added the catch-all handler using exactly this pattern, matching lifecycle_execute_plan. The isinstance check ensures typer.Abort and typer.Exit (including the new typer.Exit(0) from the decline path) pass through cleanly.

**[L1]** Unlike the neighboring `lifecycle_execute_plan` (line 1999), this command has no `except Exception` catch-all. Unexpected exceptions will produce raw tracebacks instead of clean `[red]Unexpected error:[/red]` messages. If adding a catch-all, remember to re-raise `typer.Abort` and `typer.Exit`: ```python 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 ``` --- ✅ **Fixed.** Added the catch-all handler using exactly this pattern, matching `lifecycle_execute_plan`. The `isinstance` check ensures `typer.Abort` and `typer.Exit` (including the new `typer.Exit(0)` from the decline path) pass through cleanly.
src/cleveragents/cli/commands/plan.py Outdated
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[L3] get_plan() never returns None -- it raises NotFoundError. This is not None guard is dead code (always True). Same applies to line 2091. Defensive but misleading.

✅ Fixed. Removed both is not None guards. Verified that PlanLifecycleService.get_plan() has return type Plan (not Optional[Plan]) and raises NotFoundError if the plan doesn't exist.

**[L3]** `get_plan()` never returns `None` -- it raises `NotFoundError`. This `is not None` guard is dead code (always True). Same applies to line 2091. Defensive but misleading. --- ✅ **Fixed.** Removed both `is not None` guards. Verified that `PlanLifecycleService.get_plan()` has return type `Plan` (not `Optional[Plan]`) and raises `NotFoundError` if the plan doesn't exist.
src/cleveragents/cli/commands/plan.py Outdated
@@ -2069,0 +2079,4 @@
# 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}?")
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[M3] Issue #932 acceptance criteria says the prompt should show "the plan ID and a summary of pending changes." Currently only the plan ID is shown. The spec example shows the summary after confirmation, so there is ambiguity. Consider either adding a pre-confirmation summary or clarifying the acceptance criteria.

⏸️ Deferred. The spec example in docs/specification.md shows "Apply changes for plan <ID>? [y/N]: y" without a pre-confirmation summary — the summary appears after confirmation in the spec output. The implementation matches the spec example. This is a ticket-vs-spec ambiguity that should be resolved with the ticket author. Noted in PR description as a known limitation.

**[M3]** Issue #932 acceptance criteria says the prompt should show "the plan ID and a summary of pending changes." Currently only the plan ID is shown. The spec example shows the summary *after* confirmation, so there is ambiguity. Consider either adding a pre-confirmation summary or clarifying the acceptance criteria. --- ⏸️ **Deferred.** The spec example in `docs/specification.md` shows `"Apply changes for plan <ID>? [y/N]: y"` without a pre-confirmation summary — the summary appears *after* confirmation in the spec output. The implementation matches the spec example. This is a ticket-vs-spec ambiguity that should be resolved with the ticket author. Noted in PR description as a known limitation.
src/cleveragents/cli/commands/plan.py Outdated
@@ -2069,0 +2082,4 @@
confirm = typer.confirm(f"\nApply changes for plan {plan_id}?")
if not confirm:
console.print("[yellow]Apply cancelled.[/yellow]")
raise typer.Abort()
CoreRasurae commented 2026-03-24 11:11:29 +00:00
Member
Copy Link
Copy Source

[M1] typer.Abort() here produces exit code 1 and appends "Aborted." to output. User declining a confirmation is not an error. correct_decision and legacy apply in this same file use typer.Exit(0) for the decline path, which is the more appropriate pattern.

Consider: raise typer.Exit(0)

✅ Fixed. Changed to raise typer.Exit(0). Also added default=False to the typer.confirm() call for consistency with sibling commands (I2). The decline scenario now exits cleanly with code 0 and no spurious "Aborted." message.

**[M1]** `typer.Abort()` here produces exit code 1 and appends `"Aborted."` to output. User declining a confirmation is not an error. `correct_decision` and legacy `apply` in this same file use `typer.Exit(0)` for the decline path, which is the more appropriate pattern. Consider: `raise typer.Exit(0)` --- ✅ **Fixed.** Changed to `raise typer.Exit(0)`. Also added `default=False` to the `typer.confirm()` call for consistency with sibling commands (I2). The decline scenario now exits cleanly with code 0 and no spurious "Aborted." message.
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 3d7bf9c0f6 to 759d4eca7f 2026-03-24 11:46:09 +00:00 Compare
hurui200320 commented 2026-03-24 11:48:01 +00:00
Author
Member
Copy Link
Copy Source

Review Response — Cycle 3

Thanks for the thorough review @CoreRasurae. All findings were verified against the code. Here's the summary:

✅ Addressed (8 items)

ID Fix
M1 typer.Abort() → typer.Exit(0) on user decline — consistent with correct_decision and legacy apply
M2 Added exit code 0 assertion to decline scenario
L1 Added except Exception catch-all matching lifecycle_execute_plan pattern
L2 Expanded plan_cli.md description to mention confirmation prompt and --yes purpose
L3 Removed both dead is not None guards — get_plan() raises NotFoundError, never returns None
I1 Hoisted PlanPhase/ProcessingState import to top of try block, eliminating duplicate
I2 Added default=False to typer.confirm() for consistency

⏸️ Deferred (1 item)

ID Reason
M3 Spec example shows summary after confirmation, not before. Implementation matches spec. Ticket-vs-spec ambiguity — recommend discussing with ticket author.

ℹ️ Acknowledged, no action (3 items)

ID Reason
L4 Typer/Click handles both flag orderings natively; low risk
L5 Auto-select + prompt is a separate concern outside this ticket's scope
I3 Robot helper tests flag recognition by design

Additional

  • Branch rebased onto latest master (a854de7e)
  • All 6 quality gates pass (lint, typecheck, 12,297 unit scenarios, 1,702 integration tests, 37 e2e tests, ≥97% coverage)
## Review Response — Cycle 3 Thanks for the thorough review @CoreRasurae. All findings were verified against the code. Here's the summary: ### ✅ Addressed (8 items) | ID | Fix | |----|-----| | **M1** | `typer.Abort()` → `typer.Exit(0)` on user decline — consistent with `correct_decision` and legacy `apply` | | **M2** | Added exit code 0 assertion to decline scenario | | **L1** | Added `except Exception` catch-all matching `lifecycle_execute_plan` pattern | | **L2** | Expanded `plan_cli.md` description to mention confirmation prompt and `--yes` purpose | | **L3** | Removed both dead `is not None` guards — `get_plan()` raises `NotFoundError`, never returns `None` | | **I1** | Hoisted `PlanPhase`/`ProcessingState` import to top of `try` block, eliminating duplicate | | **I2** | Added `default=False` to `typer.confirm()` for consistency | ### ⏸️ Deferred (1 item) | ID | Reason | |----|--------| | **M3** | Spec example shows summary *after* confirmation, not before. Implementation matches spec. Ticket-vs-spec ambiguity — recommend discussing with ticket author. | ### ℹ️ Acknowledged, no action (3 items) | ID | Reason | |----|--------| | **L4** | Typer/Click handles both flag orderings natively; low risk | | **L5** | Auto-select + prompt is a separate concern outside this ticket's scope | | **I3** | Robot helper tests flag recognition by design | ### Additional - Branch rebased onto latest `master` (`a854de7e`) - All 6 quality gates pass (lint, typecheck, 12,297 unit scenarios, 1,702 integration tests, 37 e2e tests, ≥97% coverage)
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 759d4eca7f to 41a7c02b12 2026-03-24 12:24:21 +00:00 Compare
freemo approved these changes 2026-03-24 15:26:53 +00:00
Dismissed
freemo left a comment
Owner
Copy Link
Copy Source

Review: APPROVED

Excellent fix with thorough test coverage. The core change adds --yes/-y Typer option with proper confirmation prompt and abort flow. The ValueError exception handler for provider/config resolution errors is a good defensive catch, and the catch-all except Exception with explicit re-raise for typer.Abort/typer.Exit prevents traceback leaks. BDD expanded from 2 to 5 scenarios, and all 13 existing test files touching lifecycle-apply properly updated to pass --yes. TDD @tdd_expected_fail tag correctly removed. Well done.

Minor Note

The import of PlanPhase/ProcessingState was moved from inside an if block but still sits inside the function body rather than at module level. Per CONTRIBUTING.md §Import Guidelines: "Ensure all imports are at the top of the Python file." Very minor style inconsistency.

## Review: APPROVED Excellent fix with thorough test coverage. The core change adds `--yes/-y` Typer option with proper confirmation prompt and abort flow. The `ValueError` exception handler for provider/config resolution errors is a good defensive catch, and the catch-all `except Exception` with explicit re-raise for `typer.Abort`/`typer.Exit` prevents traceback leaks. BDD expanded from 2 to 5 scenarios, and all 13 existing test files touching `lifecycle-apply` properly updated to pass `--yes`. TDD `@tdd_expected_fail` tag correctly removed. Well done. ### Minor Note The import of `PlanPhase`/`ProcessingState` was moved from inside an `if` block but still sits inside the function body rather than at module level. Per CONTRIBUTING.md §Import Guidelines: *"Ensure all imports are at the top of the Python file."* Very minor style inconsistency.
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 41a7c02b12 to 70ae83ea59 2026-03-25 05:37:17 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 70ae83ea59 to d82c432029 2026-03-25 11:34:36 +00:00 Compare
hurui200320 dismissed freemo's review 2026-03-25 11:34:36 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-25 11:35:01 +00:00
hurui200320 commented 2026-03-25 11:35:53 +00:00
Author
Member
Copy Link
Copy Source

Review Response — Cycle 5 (Jeff's approval note)

Thanks for the approval and the import note @freemo.

✅ Addressed

ID Fix
Import-1 Moved PlanPhase and ProcessingState from function-local import (inside lifecycle_apply_plan body) to module-level import at top of plan.py, per CONTRIBUTING.md §Import Guidelines: "Ensure all imports are at the top of the Python file."

Additional

  • Branch rebased onto latest master (83c22b83)
  • All 6 quality gates pass (lint, typecheck, 12,424 unit scenarios, 1,727 integration tests, 41 e2e tests, ≥97% coverage)
## Review Response — Cycle 5 (Jeff's approval note) Thanks for the approval and the import note @freemo. ### ✅ Addressed | ID | Fix | |----|-----| | **Import-1** | Moved `PlanPhase` and `ProcessingState` from function-local import (inside `lifecycle_apply_plan` body) to module-level import at top of `plan.py`, per CONTRIBUTING.md §Import Guidelines: *"Ensure all imports are at the top of the Python file."* | ### Additional - Branch rebased onto latest `master` (`83c22b83`) - All 6 quality gates pass (lint, typecheck, 12,424 unit scenarios, 1,727 integration tests, 41 e2e tests, ≥97% coverage)
hurui200320 canceled auto merging this pull request when all checks succeed 2026-03-25 11:36:21 +00:00
hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-25 11:36:34 +00:00
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from d82c432029 to 3464590bdf 2026-03-25 11:56:08 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 3464590bdf to 8db95b3f3f 2026-03-25 12:52:41 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 8db95b3f3f to ee202789bc 2026-03-25 13:08:46 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from ee202789bc to c4cfae2865 2026-03-26 02:26:19 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from c4cfae2865 to 22741ec5fd 2026-03-26 06:00:55 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 22741ec5fd to a1908bb0dc 2026-03-26 06:35:09 +00:00 Compare
hurui200320 commented 2026-03-26 06:36:28 +00:00
Author
Member
Copy Link
Copy Source

Review Response — Cycle 7 (rebase + WF05 e2e fix)

Addressed the latest CI failure after rebasing onto current origin/master.

✅ Addressed

ID Severity Issue Resolution
WF05-E2E-1 Must-fix E2E.Wf05 Db Migration failed because lifecycle-apply prompted interactively in Robot (Apply changes for plan <ID>? [y/N]:) Updated WF05 test apply invocation to non-interactive mode: plan lifecycle-apply --yes ${plan_id} --format json in robot/e2e/wf05_db_migration.robot
Rebase-2 Must-fix Branch needed to stay aligned with latest master before final push Fetched and verified against latest origin/master; branch is rebased/current (0 behind / 1 ahead)

✅ Validation rerun

  • nox -e lint — pass
  • nox -e typecheck — pass
  • nox -e unit_tests — pass
  • nox -e integration_tests — pass
  • nox -e e2e_tests — pass (42 tests)
  • nox -e coverage_report — pass (98%, threshold >=97%)

⏸️ Deferred remains unchanged

  • M3 ticket/spec ambiguity around pre-confirmation “summary of pending changes” remains deferred as previously documented.

PR description has been updated to reflect this cycle.

## Review Response — Cycle 7 (rebase + WF05 e2e fix) Addressed the latest CI failure after rebasing onto current `origin/master`. ### ✅ Addressed | ID | Severity | Issue | Resolution | |---|---|---|---| | WF05-E2E-1 | Must-fix | `E2E.Wf05 Db Migration` failed because `lifecycle-apply` prompted interactively in Robot (`Apply changes for plan <ID>? [y/N]:`) | Updated WF05 test apply invocation to non-interactive mode: `plan lifecycle-apply --yes ${plan_id} --format json` in `robot/e2e/wf05_db_migration.robot` | | Rebase-2 | Must-fix | Branch needed to stay aligned with latest master before final push | Fetched and verified against latest `origin/master`; branch is rebased/current (`0 behind / 1 ahead`) | ### ✅ Validation rerun - `nox -e lint` — pass - `nox -e typecheck` — pass - `nox -e unit_tests` — pass - `nox -e integration_tests` — pass - `nox -e e2e_tests` — pass (42 tests) - `nox -e coverage_report` — pass (98%, threshold >=97%) ### ⏸️ Deferred remains unchanged - M3 ticket/spec ambiguity around pre-confirmation “summary of pending changes” remains deferred as previously documented. PR description has been updated to reflect this cycle.
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from a1908bb0dc to 230cc4f730 2026-03-26 06:42:09 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from 230cc4f730 to f7074a2eca 2026-03-26 07:13:51 +00:00 Compare
hurui200320 force-pushed fix/m4-plan-apply-yes-flag from f7074a2eca to 63cc79ca4b 2026-03-26 07:34:44 +00:00 Compare
hurui200320 merged commit 414abb1396 into master 2026-03-26 07:50:10 +00:00
hurui200320 deleted branch fix/m4-plan-apply-yes-flag 2026-03-26 07:50:10 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
CoreRasurae
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
 controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
 overdue
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
Type
Bug
Milestone
No items
No Milestone v3.3.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Blocks
#932 fix(cli): add missing --yes flag to plan apply command
cleveragents/cleveragents-core
Reference: cleveragents/cleveragents-core#1127
Delete Branch "fix/m4-plan-apply-yes-flag"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?