fix(cli): add missing --yes flag to plan apply command #1127
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
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
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
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?
Summary
Implements ticket #932 by adding
--yes/-ysupport toplan 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
--yes/-ytolifecycle_apply_planinsrc/cleveragents/cli/commands/plan.py.--yesis not provided.Apply changes for plan <ID>? [y/N]:.ValueError+ guarded catch-all) consistent with neighboring lifecycle commands.PlanPhase/ProcessingStateimports to module level perCONTRIBUTING.mdimport rules.Test/documentation updates (from earlier review cycles)
lifecycle-applyCLI reference docs indocs/reference/plan_cli.md.Cycle 7 fix (this update)
fix/m4-plan-apply-yes-flagonto latestorigin/master.robot/e2e/wf05_db_migration.robotplan lifecycle-apply ${plan_id} --format jsonplan lifecycle-apply --yes ${plan_id} --format jsonrc=1.Deferred / Known limitations
applycommand--yesforwarding behavior remains outside this ticket scope.Quality gates (latest rerun on rebased branch)
nox -e lintnox -e typechecknox -e unit_testsnox -e integration_testsnox -e e2e_testsnox -e coverage_reportNotes for reviewers
--yesin WF05 E2E apply step.e5faeff6ede144fcc019e144fcc01928acb4eaaf28acb4eaaf3d7bf9c0f6Code Review Report -- PR #1127 (Issue #932)
Branch:
fix/m4-plan-apply-yes-flagScope: 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/-yflag tolifecycle-apply, follows the established Typer Option pattern from sibling destructive commands, properly removes the@tdd_expected_failtag, 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 1File:
src/cleveragents/cli/commands/plan.py:2085When 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:
lifecycle-apply(this PR)typer.Abort()rollbacktyper.Abort()correcttyper.Exit(0)applytyper.Exit(0)correctand the legacyapplyusetyper.Exit(0)-- the more appropriate pattern for a user-initiated cancellation. UsingAbort()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 withcorrectand legacyapplyin the same file.Recommendation: Change line 2085 from
raise typer.Abort()toraise typer.Exit(0).M2. Test Coverage Gap -- Missing exit code assertion on decline scenario
File:
features/tdd_plan_apply_yes_flag.feature:28-32The "user declines" scenario does not assert the exit code:
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 fromtyper.Abort()) is untested. If M1 is fixed to usetyper.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:2082Issue #932's acceptance criteria states:
The current implementation shows only:
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.mdline 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 Exceptioncatch-all handlerFile:
src/cleveragents/cli/commands/plan.py:2107-2115The
lifecycle_apply_plancommand's exception handlers end atexcept CleverAgentsError. The neighboringlifecycle_execute_plancommand (line 1999) includes a catch-allexcept Exception as e:that produces a clean"[red]Unexpected error:[/red]"message. Without this, an unexpected exception (e.g.,TypeErrorfrom a service bug) would produce a raw traceback.Note: If a catch-all is added, it must re-raise
typer.Abortandtyper.Exitto avoid intercepting the decline path and other early-abort paths. The pattern used elsewhere in the codebase is:L2. Documentation -- Description text not updated
File:
docs/reference/plan_cli.md:148-153The 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--yesdoes, or that Apply is destructive. Other commands in the same file have richer descriptions.L3. Code Quality -- Dead code
is not NoneguardsFile:
src/cleveragents/cli/commands/plan.py:2073, 2091service.get_plan(plan_id)at line 2072 never returnsNone-- it raisesNotFoundError(aCleverAgentsErrorsubclass) if the plan does not exist. Therefore theif pre_plan is not Nonechecks at lines 2073 and 2091 are unreachable dead code. They are defensive but misleading, suggestingNoneis a possible return value when the API contract guarantees it is not.L4. Test Coverage Gap -- No test for
--yesafter positional argumentAll 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.featureAll TDD scenarios pass an explicit plan ID. No test anywhere exercises the combined path: no
plan_idgiven -> 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
PlanPhaseimportPlanPhaseis imported at line 2045 (inside theif not plan_id:block) and again at line 2087 (unconditionally). The second import is necessary (for whenplan_idis provided directly), but the visual redundancy could be eliminated by hoisting the import to the top of thetryblock.I2. Code Quality --
typer.confirmwithout explicitdefault=FalseLine 2082 uses
typer.confirm(...)withoutdefault=False. The implicit default isFalse(same behavior), butsession deleteandskill removespecify it explicitly for readability. Minor inconsistency.I3. Test Design -- Robot helper only tests flag recognition
robot/helper_tdd_plan_apply_yes_flag.pyusesDUMMY_PLAN_ID(not a valid ULID), so the command fails after flag parsing. This is intentional (the helper only tests that--yes/-yare recognized), but it provides no assurance about the correctness of the--yescode path beyond argument parsing.Summary
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.
@ -151,3 +151,3 @@```bashagents plan lifecycle-apply [PLAN_ID] [--format FORMAT]agents plan lifecycle-apply [--yes|-y] [PLAN_ID] [--format FORMAT][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/-yto skip the prompt in scripts or CI pipelines."@ -23,0 +29,4 @@Given a plan CLI runner for the yes-flag testWhen I invoke lifecycle-apply without --yes and declineThen the lifecycle-apply output should contain "Apply cancelled."And the lifecycle-apply should not have called apply[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 (fromtyper.Abort()), but this is untested.✅ Fixed. Added
And the lifecycle-apply exit code should be 0to the decline scenario. This assertion now passes because M1 was fixed to usetyper.Exit(0)instead oftyper.Abort().[L1] Unlike the neighboring
lifecycle_execute_plan(line 1999), this command has noexcept Exceptioncatch-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.Abortandtyper.Exit:✅ Fixed. Added the catch-all handler using exactly this pattern, matching
lifecycle_execute_plan. Theisinstancecheck ensurestyper.Abortandtyper.Exit(including the newtyper.Exit(0)from the decline path) pass through cleanly.[L3]
get_plan()never returnsNone-- it raisesNotFoundError. Thisis not Noneguard is dead code (always True). Same applies to line 2091. Defensive but misleading.✅ Fixed. Removed both
is not Noneguards. Verified thatPlanLifecycleService.get_plan()has return typePlan(notOptional[Plan]) and raisesNotFoundErrorif the plan doesn't exist.@ -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}?")[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.mdshows"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.@ -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()[M1]
typer.Abort()here produces exit code 1 and appends"Aborted."to output. User declining a confirmation is not an error.correct_decisionand legacyapplyin this same file usetyper.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 addeddefault=Falseto thetyper.confirm()call for consistency with sibling commands (I2). The decline scenario now exits cleanly with code 0 and no spurious "Aborted." message.3d7bf9c0f6759d4eca7fReview Response — Cycle 3
Thanks for the thorough review @CoreRasurae. All findings were verified against the code. Here's the summary:
✅ Addressed (8 items)
typer.Abort()→typer.Exit(0)on user decline — consistent withcorrect_decisionand legacyapplyexcept Exceptioncatch-all matchinglifecycle_execute_planpatternplan_cli.mddescription to mention confirmation prompt and--yespurposeis not Noneguards —get_plan()raisesNotFoundError, never returnsNonePlanPhase/ProcessingStateimport to top oftryblock, eliminating duplicatedefault=Falsetotyper.confirm()for consistency⏸️ Deferred (1 item)
ℹ️ Acknowledged, no action (3 items)
Additional
master(a854de7e)759d4eca7f41a7c02b12Review: APPROVED
Excellent fix with thorough test coverage. The core change adds
--yes/-yTyper option with proper confirmation prompt and abort flow. TheValueErrorexception handler for provider/config resolution errors is a good defensive catch, and the catch-allexcept Exceptionwith explicit re-raise fortyper.Abort/typer.Exitprevents traceback leaks. BDD expanded from 2 to 5 scenarios, and all 13 existing test files touchinglifecycle-applyproperly updated to pass--yes. TDD@tdd_expected_failtag correctly removed. Well done.Minor Note
The import of
PlanPhase/ProcessingStatewas moved from inside anifblock 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.41a7c02b1270ae83ea5970ae83ea59d82c432029New commits pushed, approval review dismissed automatically according to repository settings
Review Response — Cycle 5 (Jeff's approval note)
Thanks for the approval and the import note @freemo.
✅ Addressed
PlanPhaseandProcessingStatefrom function-local import (insidelifecycle_apply_planbody) to module-level import at top ofplan.py, per CONTRIBUTING.md §Import Guidelines: "Ensure all imports are at the top of the Python file."Additional
master(83c22b83)d82c4320293464590bdf3464590bdf8db95b3f3f8db95b3f3fee202789bcee202789bcc4cfae2865c4cfae286522741ec5fd22741ec5fda1908bb0dcReview Response — Cycle 7 (rebase + WF05 e2e fix)
Addressed the latest CI failure after rebasing onto current
origin/master.✅ Addressed
E2E.Wf05 Db Migrationfailed becauselifecycle-applyprompted interactively in Robot (Apply changes for plan <ID>? [y/N]:)plan lifecycle-apply --yes ${plan_id} --format jsoninrobot/e2e/wf05_db_migration.robotorigin/master; branch is rebased/current (0 behind / 1 ahead)✅ Validation rerun
nox -e lint— passnox -e typecheck— passnox -e unit_tests— passnox -e integration_tests— passnox -e e2e_tests— pass (42 tests)nox -e coverage_report— pass (98%, threshold >=97%)⏸️ Deferred remains unchanged
PR description has been updated to reflect this cycle.
a1908bb0dc230cc4f730230cc4f730f7074a2ecaf7074a2eca63cc79ca4b