From 631e847497e95ef45767298eb32f5f989a421fd1 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 18:07:56 +0000 Subject: [PATCH] fix(cli): add checkpoint label, creation time, and side effects to rollback confirmation prompt Resolves issue #3443: the `agents plan rollback` confirmation prompt was missing the checkpoint's descriptive label, relative creation time, and side-effects count (decisions invalidated / child plans cancelled). Changes: - Added `_format_relative_time(dt)` helper to produce human-readable relative timestamps (e.g. "42 minutes ago", "2 hours ago"). - In `rollback_plan`, moved `get_container()` / `checkpoint_service()` before the confirmation block so checkpoint metadata is available. - When `--yes` is not passed, `svc.get_checkpoint()` is called to fetch the checkpoint label (`metadata.reason`) and creation time. - Decisions created after the checkpoint are counted via `decision_service.list_decisions()`; `subplan_spawn` / `subplan_parallel_spawn` decisions are counted as child plans. - Side-effects line is printed before the prompt when counts > 0. - Falls back to the original simple prompt if checkpoint metadata cannot be fetched (e.g. checkpoint not found). - Updated existing rollback mock helpers to wire `get_checkpoint` and `decision_service` properly. - Added 3 new Behave scenarios covering label display, side-effects display, and fallback behaviour. Closes #3443 --- features/plan_cli_coverage_r2.feature | 48 ++++ features/steps/plan_cli_coverage_r2_steps.py | 269 +++++++++++++++++-- src/cleveragents/cli/commands/plan.py | 107 +++++++- 3 files changed, 393 insertions(+), 31 deletions(-) diff --git a/features/plan_cli_coverage_r2.feature b/features/plan_cli_coverage_r2.feature index 8b269f7e9..5cbc427bb 100644 --- a/features/plan_cli_coverage_r2.feature +++ b/features/plan_cli_coverage_r2.feature @@ -222,3 +222,51 @@ Feature: Plan CLI coverage round 2 – remaining uncovered lines When I invoke r2cov plan rollback without --yes and decline Then the r2cov command should abort And the r2cov output should contain "Rollback cancelled" + + # =================================================================== + # rollback_plan — enriched confirmation prompt (issue #3443) + # =================================================================== + + Scenario: Rollback confirmation prompt shows checkpoint label and creation time + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback with label "before-db-migration" + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "before-db-migration" + And the r2cov output should contain "created" + + Scenario: Rollback confirmation prompt shows side effects when decisions exist + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback with 3 decisions and 1 child plan + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "invalidate 3 decisions" + And the r2cov output should contain "cancel 1 child plan" + + Scenario: Rollback confirmation prompt falls back when checkpoint not found + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback where get_checkpoint raises not found + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "Rollback" + + Scenario: Rollback confirmation prompt shows only decisions when no child plans + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback with 4 decisions and no child plans + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "invalidate 4 decisions" + + Scenario: Rollback confirmation prompt counts parallel spawn decisions as child plans + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback with 2 parallel spawn child plans + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "cancel 2 child plans" + + Scenario: Rollback confirmation prompt handles future checkpoint timestamp gracefully + Given a r2cov CLI runner + And a mocked checkpoint service for r2cov rollback with a future checkpoint timestamp + When I invoke r2cov plan rollback without --yes and accept + Then the r2cov command should exit normally + And the r2cov output should contain "just now" diff --git a/features/steps/plan_cli_coverage_r2_steps.py b/features/steps/plan_cli_coverage_r2_steps.py index 8f0d03979..88591c1d5 100644 --- a/features/steps/plan_cli_coverage_r2_steps.py +++ b/features/steps/plan_cli_coverage_r2_steps.py @@ -22,7 +22,7 @@ Targets remaining uncovered lines in cleveragents/cli/commands/plan.py: from __future__ import annotations import io -from datetime import datetime +from datetime import UTC, datetime from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock, patch @@ -41,7 +41,11 @@ from cleveragents.core.exceptions import ( ResourceNotFoundError, ValidationError, ) -from cleveragents.domain.models.core.checkpoint import RollbackResult +from cleveragents.domain.models.core.checkpoint import ( + Checkpoint, + CheckpointMetadata, + RollbackResult, +) from cleveragents.domain.models.core.multi_project import ( ChangeSetSummary, MultiProjectMetadata, @@ -489,13 +493,46 @@ def _make_rollback_result() -> RollbackResult: ) -@given("a mocked checkpoint service for r2cov rollback that succeeds") -def step_mock_rollback_success(context: Context) -> None: +def _make_checkpoint( + *, + label: str = "before-auth-refactor", + created_at: datetime | None = None, +) -> Checkpoint: + """Create a Checkpoint domain object for use in rollback test mocks.""" + return Checkpoint( + checkpoint_id=_ULID_CP, + plan_id=_ULID_A, + sandbox_ref="abc123", + created_at=created_at or datetime(2026, 4, 5, 10, 0, 0, tzinfo=UTC), + metadata=CheckpointMetadata(reason=label), + ) + + +def _make_rollback_container( + *, + rollback_result: RollbackResult | None = None, + rollback_side_effect: Exception | None = None, +) -> MagicMock: + """Build a mock container with checkpoint_service and decision_service wired.""" mock_container = MagicMock() mock_cp_svc = MagicMock() - mock_cp_svc.selective_rollback.return_value = _make_rollback_result() + mock_cp_svc.get_checkpoint.return_value = _make_checkpoint() + if rollback_side_effect is not None: + mock_cp_svc.selective_rollback.side_effect = rollback_side_effect + else: + mock_cp_svc.selective_rollback.return_value = ( + rollback_result or _make_rollback_result() + ) mock_container.checkpoint_service.return_value = mock_cp_svc + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_container.decision_service.return_value = mock_decision_svc + return mock_container + +@given("a mocked checkpoint service for r2cov rollback that succeeds") +def step_mock_rollback_success(context: Context) -> None: + mock_container = _make_rollback_container() p = patch(_PATCH_CONTAINER, return_value=mock_container) p.start() context._r2cov_cleanups.append(p.stop) @@ -505,13 +542,9 @@ def step_mock_rollback_success(context: Context) -> None: "a mocked checkpoint service for r2cov rollback that raises BusinessRuleViolation" ) def step_mock_rollback_business_rule(context: Context) -> None: - mock_container = MagicMock() - mock_cp_svc = MagicMock() - mock_cp_svc.selective_rollback.side_effect = BusinessRuleViolation( - "Plan is already applied" + mock_container = _make_rollback_container( + rollback_side_effect=BusinessRuleViolation("Plan is already applied") ) - mock_container.checkpoint_service.return_value = mock_cp_svc - p = patch(_PATCH_CONTAINER, return_value=mock_container) p.start() context._r2cov_cleanups.append(p.stop) @@ -521,13 +554,9 @@ def step_mock_rollback_business_rule(context: Context) -> None: "a mocked checkpoint service for r2cov rollback that raises ResourceNotFoundError" ) def step_mock_rollback_not_found(context: Context) -> None: - mock_container = MagicMock() - mock_cp_svc = MagicMock() - mock_cp_svc.selective_rollback.side_effect = ResourceNotFoundError( - "Checkpoint not found" + mock_container = _make_rollback_container( + rollback_side_effect=ResourceNotFoundError("Checkpoint not found") ) - mock_container.checkpoint_service.return_value = mock_cp_svc - p = patch(_PATCH_CONTAINER, return_value=mock_container) p.start() context._r2cov_cleanups.append(p.stop) @@ -535,11 +564,9 @@ def step_mock_rollback_not_found(context: Context) -> None: @given("a mocked checkpoint service for r2cov rollback that raises ValidationError") def step_mock_rollback_validation(context: Context) -> None: - mock_container = MagicMock() - mock_cp_svc = MagicMock() - mock_cp_svc.selective_rollback.side_effect = ValidationError("Invalid checkpoint") - mock_container.checkpoint_service.return_value = mock_cp_svc - + mock_container = _make_rollback_container( + rollback_side_effect=ValidationError("Invalid checkpoint") + ) p = patch(_PATCH_CONTAINER, return_value=mock_container) p.start() context._r2cov_cleanups.append(p.stop) @@ -547,13 +574,9 @@ def step_mock_rollback_validation(context: Context) -> None: @given("a mocked checkpoint service for r2cov rollback that raises CleverAgentsError") def step_mock_rollback_clever_error(context: Context) -> None: - mock_container = MagicMock() - mock_cp_svc = MagicMock() - mock_cp_svc.selective_rollback.side_effect = CleverAgentsError( - "unknown rollback failure" + mock_container = _make_rollback_container( + rollback_side_effect=CleverAgentsError("unknown rollback failure") ) - mock_container.checkpoint_service.return_value = mock_cp_svc - p = patch(_PATCH_CONTAINER, return_value=mock_container) p.start() context._r2cov_cleanups.append(p.stop) @@ -699,6 +722,196 @@ def step_invoke_rollback_decline(context: Context) -> None: ) +@when("I invoke r2cov plan rollback without --yes and accept") +def step_invoke_rollback_accept(context: Context) -> None: + context.r2cov_result = context.r2cov_runner.invoke( + plan_app, + ["rollback", _ULID_A, _ULID_CP], + input="y\n", + ) + + +# ====================================================================== +# Given — enriched confirmation prompt mocks (issue #3443) +# ====================================================================== + + +@given('a mocked checkpoint service for r2cov rollback with label "{label}"') +def step_mock_rollback_with_label(context: Context, label: str) -> None: + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.return_value = ( + _make_checkpoint(label=label) + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + +@given( + "a mocked checkpoint service for r2cov rollback with {decisions:d} decisions " + "and {child_plans:d} child plan" +) +def step_mock_rollback_with_decisions( + context: Context, decisions: int, child_plans: int +) -> None: + from datetime import timedelta + + from ulid import ULID + + from cleveragents.domain.models.core.decision import Decision, DecisionType + + cp_time = datetime(2026, 4, 5, 10, 0, 0, tzinfo=UTC) + after_time = cp_time + timedelta(minutes=5) + + # Build decision list: (decisions - child_plans) regular + child_plans spawns + decision_list = [] + regular_count = decisions - child_plans + for i in range(regular_count): + decision_list.append( + Decision( + decision_id=str(ULID()), + plan_id=_ULID_A, + sequence_number=i, + decision_type=DecisionType.IMPLEMENTATION_CHOICE, + question="What to do?", + chosen_option="Do it", + created_at=after_time, + ) + ) + for i in range(child_plans): + decision_list.append( + Decision( + decision_id=str(ULID()), + plan_id=_ULID_A, + sequence_number=regular_count + i, + decision_type=DecisionType.SUBPLAN_SPAWN, + question="Spawn child?", + chosen_option="Yes", + created_at=after_time, + ) + ) + + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.return_value = ( + _make_checkpoint(created_at=cp_time) + ) + mock_container.decision_service.return_value.list_decisions.return_value = ( + decision_list + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + +@given( + "a mocked checkpoint service for r2cov rollback where get_checkpoint raises not found" +) +def step_mock_rollback_get_checkpoint_not_found(context: Context) -> None: + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.side_effect = ( + ResourceNotFoundError("Checkpoint not found") + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + +@given( + "a mocked checkpoint service for r2cov rollback with {decisions:d} decisions " + "and no child plans" +) +def step_mock_rollback_with_decisions_no_child_plans( + context: Context, decisions: int +) -> None: + from datetime import timedelta + + from ulid import ULID + + from cleveragents.domain.models.core.decision import Decision, DecisionType + + cp_time = datetime(2026, 4, 5, 10, 0, 0, tzinfo=UTC) + after_time = cp_time + timedelta(minutes=5) + + decision_list = [ + Decision( + decision_id=str(ULID()), + plan_id=_ULID_A, + sequence_number=i, + decision_type=DecisionType.IMPLEMENTATION_CHOICE, + question="What to do?", + chosen_option="Do it", + created_at=after_time, + ) + for i in range(decisions) + ] + + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.return_value = ( + _make_checkpoint(created_at=cp_time) + ) + mock_container.decision_service.return_value.list_decisions.return_value = ( + decision_list + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + +@given( + "a mocked checkpoint service for r2cov rollback with {child_plans:d} " + "parallel spawn child plans" +) +def step_mock_rollback_with_parallel_spawn(context: Context, child_plans: int) -> None: + from datetime import timedelta + + from ulid import ULID + + from cleveragents.domain.models.core.decision import Decision, DecisionType + + cp_time = datetime(2026, 4, 5, 10, 0, 0, tzinfo=UTC) + after_time = cp_time + timedelta(minutes=5) + + decision_list = [ + Decision( + decision_id=str(ULID()), + plan_id=_ULID_A, + sequence_number=i, + decision_type=DecisionType.SUBPLAN_PARALLEL_SPAWN, + question="Spawn parallel child?", + chosen_option="Yes", + created_at=after_time, + ) + for i in range(child_plans) + ] + + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.return_value = ( + _make_checkpoint(created_at=cp_time) + ) + mock_container.decision_service.return_value.list_decisions.return_value = ( + decision_list + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + +@given( + "a mocked checkpoint service for r2cov rollback with a future checkpoint timestamp" +) +def step_mock_rollback_with_future_timestamp(context: Context) -> None: + from datetime import timedelta + + future_time = datetime.now(UTC) + timedelta(hours=1) + mock_container = _make_rollback_container() + mock_container.checkpoint_service.return_value.get_checkpoint.return_value = ( + _make_checkpoint(created_at=future_time) + ) + p = patch(_PATCH_CONTAINER, return_value=mock_container) + p.start() + context._r2cov_cleanups.append(p.stop) + + # ====================================================================== # Then — common assertions # ====================================================================== diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index db8f7d3f8..7467dd4e4 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -122,6 +122,42 @@ def validate_namespaced_actor(value: str, flag_name: str) -> str: return value +def _format_relative_time(dt: datetime) -> str: + """Format a datetime as a human-readable relative time string. + + Returns a string like "42 minutes ago", "2 hours ago", "3 days ago", + or "just now" for very recent timestamps. + + Args: + dt: The datetime to format (timezone-aware or naive UTC). + + Returns: + A human-readable relative time string. + """ + from datetime import UTC + + now = datetime.now(UTC) + # Normalise naive datetimes to UTC + if dt.tzinfo is None: + dt = dt.replace(tzinfo=UTC) + delta = now - dt + total_seconds = int(delta.total_seconds()) + + # Guard against future timestamps (e.g. clock skew between CLI host and server) + if total_seconds < 0: + return "just now" + if total_seconds < 60: + return "just now" + if total_seconds < 3600: + minutes = total_seconds // 60 + return f"{minutes} minute{'s' if minutes != 1 else ''} ago" + if total_seconds < 86400: + hours = total_seconds // 3600 + return f"{hours} hour{'s' if hours != 1 else ''} ago" + days = total_seconds // 86400 + return f"{days} day{'s' if days != 1 else ''} ago" + + def _validate_plan_ulid(plan_id: str) -> str: """Validate that ``plan_id`` is a valid ULID before querying v3 storage. @@ -3383,17 +3419,82 @@ def rollback_plan( ) raise typer.Abort() + container = get_container() + svc = container.checkpoint_service() + if not yes: - confirm = typer.confirm( + # Default prompt (used as fallback if metadata fetch fails) + prompt_text = ( f"\nRollback plan {plan_id} to checkpoint {resolved_checkpoint_id}?" ) + + # Fetch checkpoint metadata to enrich the confirmation prompt + try: + checkpoint = svc.get_checkpoint(resolved_checkpoint_id) + label = checkpoint.metadata.reason + relative_time = _format_relative_time(checkpoint.created_at) + label_display = f"'{label}'" if label else resolved_checkpoint_id + + prompt_text = ( + f"\nRoll back plan {plan_id} to checkpoint " + f"{label_display} (created {relative_time})?" + ) + + # Count decisions and child plans that would be invalidated + decisions_count = 0 + child_plans_count = 0 + try: + from datetime import UTC + + from cleveragents.domain.models.core.decision import DecisionType + + decision_svc = container.decision_service() + all_decisions = decision_svc.list_decisions(plan_id) + cp_time = checkpoint.created_at + if cp_time.tzinfo is None: + cp_time = cp_time.replace(tzinfo=UTC) + for d in all_decisions: + d_time = d.created_at + if d_time.tzinfo is None: + d_time = d_time.replace(tzinfo=UTC) + if d_time > cp_time: + decisions_count += 1 + if d.decision_type in ( + DecisionType.SUBPLAN_SPAWN, + DecisionType.SUBPLAN_PARALLEL_SPAWN, + ): + child_plans_count += 1 + except CleverAgentsError: + # Best-effort: if the decision service is unavailable, skip + # the side-effects line rather than blocking the confirmation. + pass + + # Build side-effects message including only non-zero counts + side_effects_parts: list[str] = [] + if decisions_count > 0: + side_effects_parts.append( + f"invalidate {decisions_count} " + f"decision{'s' if decisions_count != 1 else ''}" + ) + if child_plans_count > 0: + side_effects_parts.append( + f"cancel {child_plans_count} child " + f"plan{'s' if child_plans_count != 1 else ''}" + ) + if side_effects_parts: + console.print("This will " + " and ".join(side_effects_parts) + ".") + + except CleverAgentsError: + # If checkpoint metadata fetch fails (e.g. ResourceNotFoundError), + # fall back to the default prompt. + pass + + confirm = typer.confirm(prompt_text) if not confirm: console.print("[yellow]Rollback cancelled.[/yellow]") raise typer.Abort() try: - container = get_container() - svc = container.checkpoint_service() t0 = time.monotonic() result = svc.selective_rollback(plan_id, resolved_checkpoint_id) elapsed = time.monotonic() - t0 -- 2.52.0