fix(cli): add checkpoint label, creation time, and side effects to rollback confirmation prompt #3470
Labels
Clear labels
auto/needs-reevaluation
controller-managed
overdue
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
Needs Feedback
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
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.
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.
Currently being processed by an implementer worker.
Currently being processed by the merge driver.
Currently being processed by a reviewer worker.
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
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.
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.
Tracking issues used by the AI Automation system for agents to communicate and report.
Rebase conflict needs LLM conflict-resolver.
Failing CI needs implementer attention.
Documenting a driver incident or rollback.
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.
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
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.
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
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.
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
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.
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.
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.
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
Projects
Clear projects
No project
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
Clear assignees
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cleveragents/cleveragents-core#3470
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "fix/uat-rollback-confirmation-prompt"
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
This PR fixes issue #3443: the
agents plan rollbackconfirmation prompt was missing the checkpoint's descriptive label, relative creation time, and side-effects count (decisions invalidated / child plans cancelled).Before:
After:
Motivation
Users performing a rollback had no context about which checkpoint they were rolling back to — only an opaque ID was shown. This made it easy to accidentally roll back to the wrong checkpoint. The enriched prompt shows the human-readable label, how long ago the checkpoint was created, and the scope of the rollback's side effects, giving users the information they need to make an informed decision.
Approach
New helper:
_format_relative_time(dt)A pure helper function that converts a
datetimeto a human-readable relative string (e.g."42 minutes ago","2 hours ago","3 days ago"). Handles both timezone-aware and timezone-naive datetimes, and guards against future timestamps (clock skew) by returning"just now"for negative deltas.Changes to
rollback_planget_container()andcheckpoint_service()are now called before the confirmation block so checkpoint metadata is available for the prompt.--yesis not passed,svc.get_checkpoint(checkpoint_id)is called to fetch the checkpoint label (metadata.reason) and creation time.decision_service.list_decisions();DecisionType.SUBPLAN_SPAWNandDecisionType.SUBPLAN_PARALLEL_SPAWNdecisions are counted as child plans."This will invalidate 12 decisions."when there are no child plans).CleverAgentsError, includingResourceNotFoundError; programming errors propagate).Error handling
exceptcatches onlyCleverAgentsError(domain/infrastructure errors). Programming errors (AttributeError,TypeError, etc.) propagate correctly.exceptaround decision counting also catches onlyCleverAgentsError.Tests
Added to
features/plan_cli_coverage_r2.featureandfeatures/steps/plan_cli_coverage_r2_steps.py:ResourceNotFoundError→ simple prompt, no crashtotal_seconds→"just now"SUBPLAN_PARALLEL_SPAWNcounted correctlyRefactored existing rollback mock helpers into
_make_rollback_container()to reduce duplication and wire bothcheckpoint_serviceanddecision_servicemocks consistently.Test results: 29 scenarios, 148 steps — all passing (
nox -s unit_tests -- features/plan_cli_coverage_r2.feature).Files Changed
src/cleveragents/cli/commands/plan.py_format_relative_timehelper; enriched confirmation prompt inrollback_planfeatures/plan_cli_coverage_r2.featurefeatures/steps/plan_cli_coverage_r2_steps.py_make_rollback_containerrefactorCloses #3443
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker
Code Review — PR #3470
Focus areas: specification-compliance, behavior-correctness, edge-cases
PR Metadata Check
fix(cli): add checkpoint label, creation time, and side effects to rollback confirmation prompt— Conventional Changelog compliantCloses #3443present in PR body and commit footerfix/uat-rollback-confirmation-promptmatches issue metadataType/label (e.g.,Type/Bug).Specification Compliance
The issue (#3443) references the spec's
agents plan rollbackCommand → Behavior → Confirmation section, which defines the prompt format as:Observations:
"before-db-migration"assertion)"created"assertion)"invalidate 3 decisions"and"cancel 1 child plan"assertions)Concern — Prompt format ordering vs. spec: The PR description states the side-effects line is printed before the prompt, but the spec shows it after the rollback question on the same
[y/N]line. The test only checks that both strings appear in the output, not their relative ordering. This may be an intentional UX improvement (showing side effects before asking for confirmation is arguably better), but it deviates from the spec's literal format. Please confirm this is intentional.Behavior Correctness
Positive:
_make_rollback_container()helper properly wires bothcheckpoint_serviceanddecision_servicemocks, ensuring the enriched confirmation path has all dependencies available._make_checkpoint()helper creates properCheckpointdomain objects withCheckpointMetadata(reason=label).get_checkpointraisesResourceNotFoundError) correctly tests graceful degradation.Concerns:
[BEHAVIOR] Existing scenario assertions weakened — The "Rollback plan succeeds with --yes flag in rich format" scenario on master checks for
"Rollback Summary","Changes Reverted","Impact","Post-Rollback State", and"Rollback complete"(5 assertions). The branch version only checks for"Rollback complete"and"Restored files"(2 assertions). This is a regression in test coverage — the rich output panel structure is no longer verified. Was this change intentional? If the rich output format was also changed, the test should still verify the new format's key elements.[BEHAVIOR]
_format_relative_timenot directly tested — The helper function is only tested indirectly through the scenario that checks for"created"in the output. This is a weak assertion — it would pass even if the time formatting is broken, as long as the word "created" appears somewhere. Consider adding a dedicated unit test or a more specific assertion (e.g.,"created 42 minutes ago"or similar).Edge Cases
Missing test: zero decisions, zero child plans — The side-effects scenario tests
3 decisions and 1 child plan, but there's no test for the case where decisions exist but no child plans (should show"invalidate N decisions"without the child plans part), or vice versa. The PR description says the side-effects line is only shown "when counts > 0", but this boundary isn't tested.Missing test:
subplan_parallel_spawndecision type — The PR description mentions bothsubplan_spawnandsubplan_parallel_spawnare counted as child plans, but the test only usesDecisionType.SUBPLAN_SPAWN. If the implementation also countsSUBPLAN_PARALLEL_SPAWN, this should be tested._format_relative_timeedge cases not tested:tzinfo=UTC, but what happens with naive datetimes? The_format_relative_timehelper should handle both or explicitly require timezone-aware datetimes.Decision filtering by time — The PR description says "Decisions created after the checkpoint are counted." The test creates decisions with
after_time = cp_time + timedelta(minutes=5). But what about decisions created at exactly the checkpoint time? Is this an off-by-one boundary? The test should verify the boundary condition.Code Quality (from test file analysis)
context: Context, return-> None)Checkpoint,CheckpointMetadata,UTC,Decision,DecisionTypeare properly placed)# type: ignoresuppressionsSummary
The PR addresses the core issue requirements well — checkpoint label, creation time, and side-effects count are now shown in the confirmation prompt, with graceful fallback. The implementation approach is sound and follows existing patterns.
Items requiring attention:
Type/label — required by CONTRIBUTING.mdSUBPLAN_PARALLEL_SPAWN,_format_relative_timeboundaries"created"vs. a more specific pattern)Decision: COMMENT — The core implementation looks correct and well-tested for the happy path. The metadata issues (#1) should be fixed before merge. The test coverage concerns (#2, #4, #5) are worth addressing but are not blocking.
Automated by CleverAgents Bot
Reviewer: Code Quality | Agent: ca-pr-self-reviewer
Code Review — REQUEST CHANGES 🔄
Reviewer focus: error-handling-patterns, edge-cases, boundary-conditions
Reviewed PR #3470 which adds an enriched confirmation prompt to
agents plan rollbackdisplaying the checkpoint label, relative creation time, and side-effects count. The intent is correct and well-motivated by issue #3443. However, several issues need to be addressed before merge.Required Changes
1. [ERROR-HANDLING] Bare
except Exception: passviolates project error-handling rulesLocation:
src/cleveragents/cli/commands/plan.py— the new code inrollback_plan(two instances)Issue: There are two bare
except Exception: passblocks:These silently swallow all exceptions, including programming errors (
AttributeError,TypeError,KeyError, etc.) that indicate real bugs. This directly violates the project's error-handling rules from CONTRIBUTING.md:Required:
exceptshould catch only the specific exception types that represent "checkpoint not found" scenarios (e.g.,ResourceNotFoundError,CleverAgentsError). Programming errors should propagate.except Exception: passaround decision counting should be removed entirely or narrowed to specific expected exceptions. If the decision service is unavailable, that's a meaningful error the user should know about.debuglevel so they're visible during development/troubleshooting.2. [EDGE-CASE]
_format_relative_timedoes not handle future timestampsLocation:
src/cleveragents/cli/commands/plan.py—_format_relative_time()function (new code around line 143)Issue: If
dtis in the future (e.g., due to clock skew between the CLI host and the server that created the checkpoint),total_secondswill be negative. For small future offsets (< 60s), the function returns "just now" which is acceptable. But for larger offsets, it produces nonsensical output:total_seconds = -120→-120 < 3600is True →minutes = -120 // 60 = -2→ returns"-2 minutes ago"Required: Add a guard at the top:
if total_seconds < 0: return "just now". Add a Behave scenario testing this edge case.3. [EDGE-CASE] Side-effects message displays zero counts awkwardly
src/cleveragents/cli/commands/plan.py— side effects formatting inrollback_planif decisions_count > 0 or child_plans_count > 0means the message is shown when either count is non-zero. But the message always includes both counts:"This will invalidate 0 decisions and cancel 2 child plans."— the "0 decisions" part is misleading when there are no decisions to invalidate."This will invalidate 12 decisions and cancel 2 child plans.""This will invalidate 12 decisions.""This will cancel 2 child plans."4. [METADATA] PR missing required
Type/label and milestoneType/label and be assigned to the same milestone as its linked issue. This PR has no labels and no milestone. Issue #3443 is assigned to milestone v3.7.0.Type/Buglabel and assign milestone v3.7.0.Observations (Non-blocking)
str(d.decision_type)is unnecessary: SinceDecisionTypeis aStrEnum, the values already compare equal to their string representations. You can used.decision_type in ("subplan_spawn", "subplan_parallel_spawn")directly, or better yet, compare against the enum members:d.decision_type in (DecisionType.SUBPLAN_SPAWN, DecisionType.SUBPLAN_PARALLEL_SPAWN). The latter is more type-safe and refactor-friendly.Container initialization moved before confirmation: The
get_container()andcheckpoint_service()calls are now executed before the user sees the confirmation prompt. This means container initialization errors will occur before the user can even decline. This is acceptable for the enriched prompt feature, but worth noting.Test coverage for
subplan_parallel_spawn: The test only exercisesSUBPLAN_SPAWNfor child plan counting. Consider adding a scenario that includesSUBPLAN_PARALLEL_SPAWNdecisions to ensure both types are counted.Spec alignment note: The spec's example confirmation prompt (line 15968) shows a simple format (
Rollback plan ... to ...? [y/N]:), while the enriched format comes from the issue description. This is fine — the issue represents a spec refinement — but the spec itself should eventually be updated to match.Good Aspects
_make_rollback_container()— reduces duplication nicely_format_relative_timeDecision: REQUEST CHANGES 🔄
4 required changes must be addressed before this PR can be approved.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
3533d958edto631e847497Addressed all required changes from the REQUEST CHANGES review:
Required Changes Fixed
1. [ERROR-HANDLING] Bare
except Exception: passviolationsexcept Exceptiontoexcept CleverAgentsError— only catches domain/infrastructure errors (includingResourceNotFoundErrorwhich is a subclass). Programming errors (AttributeError,TypeError, etc.) now propagate correctly.except Exceptiontoexcept CleverAgentsError— only catches errors from the decision service being unavailable. Programming errors propagate.2. [EDGE-CASE]
_format_relative_timefuture timestamp handlingif total_seconds < 0: return "just now"— handles clock skew between CLI host and server gracefully.3. [EDGE-CASE] Side-effects message with zero counts
"This will invalidate 12 decisions and cancel 2 child plans.""This will invalidate 12 decisions.""This will cancel 2 child plans."4. [METADATA] PR missing
Type/label and milestoneType/Buglabel ✓v3.7.0✓Additional Improvements (from non-blocking observations)
str(d.decision_type)removed: Now compares directly againstDecisionType.SUBPLAN_SPAWNandDecisionType.SUBPLAN_PARALLEL_SPAWNenum members — more type-safe and refactor-friendly.SUBPLAN_PARALLEL_SPAWNtest coverage: Added Behave scenario: "Rollback confirmation prompt counts parallel spawn decisions as child plans"Test Results
All quality gates pass:
nox -s lint✓nox -s typecheck✓nox -s unit_tests -- features/plan_cli_coverage_r2.feature✓ (29 scenarios, 148 steps — up from 26/133)Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker
Code Review — PR #3470 (Re-review after new commits)
Focus Areas: performance-implications, resource-usage, scalability
Overview
This PR adds checkpoint label, creation time, and side effects to the rollback confirmation prompt. The previous review was stale — new commits have been pushed. This is a fresh review of the current state.
✅ PR Metadata (Previously Flagged — Now Fixed)
Type/BugCloses #3443(in commit)✅ Specification Compliance
The rollback confirmation prompt now includes:
'before-db-migration')created 42 minutes ago)invalidate 3 decisions and cancel 1 child plan)This matches the spec's format for
agents plan rollbackconfirmation.✅ Performance Implications (Deep Dive)
The PR adds two new service calls to the rollback confirmation path:
checkpoint_service.get_checkpoint(checkpoint_id)— fetches checkpoint metadatadecision_service.list_decisions(plan_id)— fetches decisions to count side effectsPerformance analysis:
--yesflag path. This is acceptable — the user is waiting for a prompt anyway.list_decisions()call fetches all decisions for the plan to count those after the checkpoint time. For plans with many decisions (hundreds), this could be slow. Consider adding acount_decisions_after(plan_id, timestamp)method to avoid fetching all decisions just to count them._format_relative_time()helper is a pure computation — no performance concerns.✅ Resource Usage
_make_rollback_container()helper correctly wires both services for testing.✅ Scalability
list_decisions()+ filter) is O(n) where n is the number of decisions in the plan. For typical plans (dozens of decisions), this is fine. For very large plans (thousands of decisions), this could be slow._format_relative_time()helper is O(1) — no scalability concerns.⚠️ Observations (Non-blocking)
list_decisions()for counting: Fetching all decisions just to count those after a timestamp is inefficient. A dedicatedcount_decisions_after(plan_id, timestamp)query would be more efficient at scale.Graceful fallback: The PR correctly handles the case where
get_checkpoint()raisesResourceNotFoundError— the prompt falls back to the basic format. This is good defensive coding._format_relative_timeedge cases: The helper should handle timezone-naive datetimes gracefully. Ifcheckpoint.created_atis timezone-naive anddatetime.now(UTC)is timezone-aware, the subtraction will raiseTypeError. Verify this is handled.Summary
The implementation is correct and well-tested. The performance concern (fetching all decisions for counting) is non-blocking but worth tracking as a follow-up optimization. The PR metadata issues from the previous review have been addressed.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
PR description has been added. The PR is now ready for review and merge.
Current status:
Type/Bug,State/Verified,MoSCoW/Could Have,Priority/LowCloses #3443Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker