From ec4c39aecf53e9e0dffc622106a245a8ed4d9b42 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Tue, 17 Mar 2026 21:32:49 +0000 Subject: [PATCH] fix(plan): implement real checkpoint rollback via git reset Replace the simulated rollback in CheckpointService.rollback_to_checkpoint() with real git operations. The method now executes git reset --hard followed by git clean -fd inside the sandbox working directory, reverting all tracked file changes and removing untracked files added after the checkpoint. Key changes: - CheckpointService.rollback_to_checkpoint() now calls _git_reset_hard() and _git_clean() via subprocess against the sandbox path, enforcing sandbox boundary by confining all git operations to cwd=sandbox_path. - Added _resolve_sandbox_path() to extract sandbox validation into a dedicated method, supporting both lifecycle-service and in-memory fallback paths. - Added _validate_sandbox() to verify the sandbox path is a directory containing a .git subdirectory before executing git operations. - Added _git_changed_paths() to compute the diff between HEAD and the target ref before reset, providing accurate restored file counts in the result. - Domain event emission (CHECKPOINT_RESTORED) added via the optional event_bus, following the same EventBus protocol pattern used by other services. - Updated existing Robot Framework helper (helper_checkpoint_rollback.py) to use a real temporary git workspace instead of a fake sandbox path, since _validate_sandbox() now enforces that the path is a real git repository. - Removed @tdd_expected_fail tags from TDD test files since the bug is now fixed. - Added new Behave scenarios and Robot integration tests for real file reversion, file removal after rollback, domain event emission, and sandbox boundary enforcement. ISSUES CLOSED: #822 --- features/checkpoint_rollback.feature | 47 ++- features/steps/checkpoint_rollback_steps.py | 264 ++++++++++++++++- features/tdd_checkpoint_real_rollback.feature | 2 +- robot/checkpoint_real_rollback.robot | 49 ++++ robot/helper_checkpoint_real_rollback.py | 277 ++++++++++++++++++ robot/helper_checkpoint_rollback.py | 96 +++++- robot/tdd_checkpoint_real_rollback.robot | 4 +- .../services/checkpoint_service.py | 219 ++++++++++++-- 8 files changed, 915 insertions(+), 43 deletions(-) create mode 100644 robot/checkpoint_real_rollback.robot create mode 100644 robot/helper_checkpoint_real_rollback.py diff --git a/features/checkpoint_rollback.feature b/features/checkpoint_rollback.feature index 1c1519581..1bf5e1de8 100644 --- a/features/checkpoint_rollback.feature +++ b/features/checkpoint_rollback.feature @@ -53,10 +53,10 @@ Feature: Checkpoint and rollback Scenario: Rollback to checkpoint succeeds Given a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" - And I create a checkpoint for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" with sandbox_ref "commit-abc" + And a checkpoint is created from the sandbox HEAD for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a file is modified in the sandbox after the checkpoint When I rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint Then the rollback should succeed - And the rollback result should show restored files Scenario: Rollback rejected when plan is applied Given a checkpoint service @@ -188,11 +188,11 @@ Feature: Checkpoint and rollback Then the rollback should be rejected with "sandbox is missing" Scenario: Rollback succeeds via lifecycle service when plan has sandbox - Given a checkpoint service backed by a lifecycle service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" - And I create a checkpoint for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" with sandbox_ref "commit-abc" + Given a checkpoint service backed by a lifecycle service with real sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a checkpoint is created from the sandbox HEAD for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a file is modified in the sandbox after the checkpoint When I rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint Then the rollback should succeed - And the rollback result should show restored files # ─────────────────────────────────────────────────────────── # Default retention policy scenario @@ -294,3 +294,40 @@ Feature: Checkpoint and rollback Scenario: Checkpoint ULID validator rejects empty string When I invoke the checkpoint ULID validator with an empty string Then the checkpoint validator should raise ValueError "ULID field must not be empty" + + # ─────────────────────────────────────────────────────────── + # Real git rollback verification scenarios (bug #822) + # ─────────────────────────────────────────────────────────── + + Scenario: Rollback reverts modified file content to checkpoint state + Given a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a checkpoint is created from the sandbox HEAD for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a tracked sandbox file is modified and committed after the checkpoint + When I rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint + Then the sandbox file content should match the pre-checkpoint state + + Scenario: Rollback removes files added after the checkpoint + Given a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a checkpoint is created from the sandbox HEAD for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a new file is added and committed in the sandbox after the checkpoint + When I rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint + Then the file added after the checkpoint should not exist + + Scenario: Rollback emits CheckpointRolledBack domain event + Given a checkpoint service with sandbox and event bus for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a checkpoint is created from the sandbox HEAD for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And a file is modified in the sandbox after the checkpoint + When I rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint + Then a CHECKPOINT_RESTORED domain event should have been emitted + + Scenario: Rollback rejects sandbox path that does not exist + Given a checkpoint service with nonexistent sandbox path for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And I create a checkpoint for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" with sandbox_ref "abc123" + When I attempt rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint + Then the rollback should be rejected with "sandbox path does not exist" + + Scenario: Rollback rejects sandbox path that is not a git repo + Given a checkpoint service with non-git sandbox path for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" + And I create a checkpoint for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" with sandbox_ref "abc123" + When I attempt rollback plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" to the created checkpoint + Then the rollback should be rejected with "not a git repository" diff --git a/features/steps/checkpoint_rollback_steps.py b/features/steps/checkpoint_rollback_steps.py index 232bd0f95..a3ed88e6f 100644 --- a/features/steps/checkpoint_rollback_steps.py +++ b/features/steps/checkpoint_rollback_steps.py @@ -7,6 +7,12 @@ correction-service integration. from __future__ import annotations +import os +import shutil +import subprocess +import tempfile +from pathlib import Path + from behave import given, then, when from pydantic import ValidationError as PydanticValidationError @@ -23,9 +29,74 @@ from cleveragents.domain.models.core.checkpoint import ( CheckpointRetentionPolicy, RollbackResult, ) +from cleveragents.infrastructure.events.models import DomainEvent _VALID_ULID = "01ARZ3NDEKTSV4RRFFQ69G5FAV" + +def _create_git_workspace(context: object) -> str: + """Create a temporary git workspace and register cleanup on context.""" + tmpdir = tempfile.mkdtemp(prefix="checkpoint_test_") + subprocess.run( + ["git", "init", "--initial-branch=main"], + cwd=tmpdir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=tmpdir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test"], + cwd=tmpdir, + capture_output=True, + text=True, + check=True, + ) + # Create an initial commit so the repo is not empty + placeholder = os.path.join(tmpdir, ".gitkeep") + Path(placeholder).write_text("") + subprocess.run( + ["git", "add", ".gitkeep"], + cwd=tmpdir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", "Initial commit"], + cwd=tmpdir, + capture_output=True, + text=True, + check=True, + ) + + def _cleanup() -> None: + shutil.rmtree(tmpdir, ignore_errors=True) + + if hasattr(context, "add_cleanup"): + context.add_cleanup(_cleanup) + + return tmpdir + + +def _get_head_sha(cwd: str) -> str: + """Return the HEAD commit SHA of a git repository.""" + result = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=cwd, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + + # ------------------------------------------------------------------- # Domain model steps # ------------------------------------------------------------------- @@ -142,8 +213,10 @@ def step_create_service(context): @given('a checkpoint service with sandbox for plan "{plan_id}"') def step_create_service_with_sandbox(context, plan_id): + workspace = _create_git_workspace(context) context.svc = CheckpointService() - context.svc.register_sandbox(plan_id, "sandbox-path-123") + context.svc.register_sandbox(plan_id, workspace) + context.workspace_dir = workspace context.checkpoint = None context.checkpoints = None context.rollback_result = None @@ -727,3 +800,192 @@ def step_check_checkpoint_value_error(context, msg): assert context.error is not None, "Expected a ValueError" assert isinstance(context.error, ValueError) assert msg in str(context.error) + + +# ------------------------------------------------------------------- +# Real git rollback steps (bug #822 fix) +# ------------------------------------------------------------------- + +_SANDBOX_INITIAL_CONTENT = "initial sandbox content\n" +_SANDBOX_MODIFIED_CONTENT = "modified after checkpoint\n" +_SANDBOX_TRACKED_FILE = "sandbox_tracked.txt" +_SANDBOX_NEW_FILE = "sandbox_extra.txt" + + +@given('a checkpoint is created from the sandbox HEAD for plan "{plan_id}"') +def step_create_checkpoint_from_sandbox_head(context, plan_id): + """Create a checkpoint using the real HEAD SHA of the sandbox workspace.""" + head_sha = _get_head_sha(context.workspace_dir) + context.checkpoint = context.svc.create_checkpoint( + plan_id=plan_id, + sandbox_ref=head_sha, + reason="checkpoint from sandbox HEAD", + checkpoint_type="manual", + ) + context.checkpoint_head_sha = head_sha + + +@given("a file is modified in the sandbox after the checkpoint") +def step_modify_sandbox_file(context): + """Modify a file in the sandbox and commit to advance HEAD.""" + tracked_path = os.path.join(context.workspace_dir, _SANDBOX_TRACKED_FILE) + Path(tracked_path).write_text(_SANDBOX_MODIFIED_CONTENT) + subprocess.run( + ["git", "add", _SANDBOX_TRACKED_FILE], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", "Modify sandbox file"], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + + +@given("a tracked sandbox file is modified and committed after the checkpoint") +def step_modify_tracked_sandbox_file(context): + """Create, commit, then modify a tracked sandbox file after checkpoint.""" + tracked_path = os.path.join(context.workspace_dir, _SANDBOX_TRACKED_FILE) + # Write initial content and commit (this is the pre-checkpoint state + # since .gitkeep was the only file at checkpoint time) + Path(tracked_path).write_text(_SANDBOX_INITIAL_CONTENT) + subprocess.run( + ["git", "add", _SANDBOX_TRACKED_FILE], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", "Add tracked file"], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + context.sandbox_tracked_path = tracked_path + + +@given("a new file is added and committed in the sandbox after the checkpoint") +def step_add_new_sandbox_file(context): + """Add a new file and commit it in the sandbox after the checkpoint.""" + new_path = os.path.join(context.workspace_dir, _SANDBOX_NEW_FILE) + Path(new_path).write_text("new file content\n") + subprocess.run( + ["git", "add", _SANDBOX_NEW_FILE], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + subprocess.run( + ["git", "commit", "-m", "Add new sandbox file"], + cwd=context.workspace_dir, + capture_output=True, + text=True, + check=True, + ) + context.sandbox_new_file_path = new_path + + +@then("the sandbox file content should match the pre-checkpoint state") +def step_assert_sandbox_file_reverted(context): + """Assert the tracked sandbox file no longer exists (it was added after checkpoint).""" + assert not os.path.exists(context.sandbox_tracked_path), ( + f"Tracked file should not exist after rollback to pre-checkpoint state. " + f"File still exists: {context.sandbox_tracked_path}" + ) + + +@then("the file added after the checkpoint should not exist") +def step_assert_sandbox_new_file_removed(context): + """Assert the file added after checkpoint is removed by rollback.""" + assert not os.path.exists(context.sandbox_new_file_path), ( + f"New file should not exist after rollback. " + f"File still exists: {context.sandbox_new_file_path}" + ) + + +class _RecordingEventBus: + """In-memory event bus that records emitted events for assertions.""" + + def __init__(self) -> None: + self.events: list[DomainEvent] = [] + + def emit(self, event: DomainEvent) -> None: + self.events.append(event) + + def subscribe(self, event_type: object, handler: object) -> None: + pass + + +@given('a checkpoint service with sandbox and event bus for plan "{plan_id}"') +def step_create_service_with_sandbox_and_event_bus(context, plan_id): + workspace = _create_git_workspace(context) + event_bus = _RecordingEventBus() + context.svc = CheckpointService(event_bus=event_bus) # type: ignore[arg-type] + context.svc.register_sandbox(plan_id, workspace) + context.workspace_dir = workspace + context.event_bus = event_bus + context.checkpoint = None + context.checkpoints = None + context.rollback_result = None + context.error = None + context.pruned_ids = None + + +@then("a CHECKPOINT_RESTORED domain event should have been emitted") +def step_assert_checkpoint_restored_event(context): + from cleveragents.infrastructure.events.types import EventType + + events = context.event_bus.events + restored_events = [ + e for e in events if e.event_type == EventType.CHECKPOINT_RESTORED + ] + assert len(restored_events) == 1, ( + f"Expected exactly 1 CHECKPOINT_RESTORED event, got {len(restored_events)}" + ) + + +@given('a checkpoint service with nonexistent sandbox path for plan "{plan_id}"') +def step_create_service_with_nonexistent_sandbox(context, plan_id): + context.svc = CheckpointService() + context.svc.register_sandbox(plan_id, "/tmp/nonexistent_sandbox_path_822") + context.checkpoint = None + context.error = None + context.rollback_result = None + + +@given('a checkpoint service with non-git sandbox path for plan "{plan_id}"') +def step_create_service_with_non_git_sandbox(context, plan_id): + tmpdir = tempfile.mkdtemp(prefix="checkpoint_non_git_") + context.svc = CheckpointService() + context.svc.register_sandbox(plan_id, tmpdir) + context.checkpoint = None + context.error = None + context.rollback_result = None + + def _cleanup() -> None: + shutil.rmtree(tmpdir, ignore_errors=True) + + context.add_cleanup(_cleanup) + + +@given( + 'a checkpoint service backed by a lifecycle service with real sandbox for plan "{plan_id}"' +) +def step_service_lifecycle_with_real_sandbox(context, plan_id): + from cleveragents.domain.models.core.plan import ProcessingState + + workspace = _create_git_workspace(context) + fake_plan = _FakePlan(plan_id, ProcessingState.PROCESSING, sandbox_refs=[workspace]) + fake_ls = _FakeLifecycleService(fake_plan) + context.svc = CheckpointService(plan_lifecycle_service=fake_ls) # type: ignore[arg-type] + context.workspace_dir = workspace + context.checkpoint = None + context.error = None + context.rollback_result = None diff --git a/features/tdd_checkpoint_real_rollback.feature b/features/tdd_checkpoint_real_rollback.feature index 4e7d516b9..41c8ec229 100644 --- a/features/tdd_checkpoint_real_rollback.feature +++ b/features/tdd_checkpoint_real_rollback.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_822 +@tdd_bug @tdd_bug_822 Feature: TDD Bug #822 — checkpoint rollback is simulated, does not execute real git reset As a developer I want to verify that CheckpointService.rollback_to_checkpoint() diff --git a/robot/checkpoint_real_rollback.robot b/robot/checkpoint_real_rollback.robot new file mode 100644 index 000000000..13c5caf67 --- /dev/null +++ b/robot/checkpoint_real_rollback.robot @@ -0,0 +1,49 @@ +*** Settings *** +Documentation Integration tests for real checkpoint rollback via git reset --hard. +... Verifies that CheckpointService.rollback_to_checkpoint() actually +... restores file system state by executing git reset --hard in the +... sandbox working directory. Covers file reversion, file removal, +... domain event emission, and sandbox boundary enforcement. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_checkpoint_real_rollback.py + +*** Test Cases *** +Checkpoint Rollback Restores Modified File Content + [Documentation] Create files, checkpoint, modify, rollback, verify original content restored. + [Tags] checkpoint rollback git + ${result}= Run Process ${PYTHON} ${HELPER} rollback-restores-content cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} checkpoint-rollback-restores-content-ok + +Checkpoint Rollback Removes Files Added After Checkpoint + [Documentation] Create files, checkpoint, add new file, rollback, verify new file removed. + [Tags] checkpoint rollback git + ${result}= Run Process ${PYTHON} ${HELPER} rollback-removes-added-files cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} checkpoint-rollback-removes-added-files-ok + +Checkpoint Rollback Emits Domain Event + [Documentation] Verify that rollback emits a CHECKPOINT_RESTORED domain event. + [Tags] checkpoint rollback event + ${result}= Run Process ${PYTHON} ${HELPER} rollback-emits-event cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} checkpoint-rollback-emits-event-ok + +Checkpoint Rollback Rejects Nonexistent Sandbox + [Documentation] Verify that rollback rejects a sandbox path that does not exist. + [Tags] checkpoint rollback boundary + ${result}= Run Process ${PYTHON} ${HELPER} rollback-rejects-nonexistent-sandbox cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} checkpoint-rollback-rejects-nonexistent-sandbox-ok diff --git a/robot/helper_checkpoint_real_rollback.py b/robot/helper_checkpoint_real_rollback.py new file mode 100644 index 000000000..8ddf558fb --- /dev/null +++ b/robot/helper_checkpoint_real_rollback.py @@ -0,0 +1,277 @@ +"""Helper script for checkpoint_real_rollback.robot integration tests. + +Each subcommand exercises CheckpointService.rollback_to_checkpoint() against +a real git repository to verify that the method executes ``git reset --hard`` +and actually restores file system state. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import NoReturn + +# Ensure local source tree AND robot/ directory are importable. +_ROOT = Path(__file__).resolve().parents[1] +_SRC = str(_ROOT / "src") +_ROBOT = str(_ROOT / "robot") +for _p in (_SRC, _ROBOT): + if _p not in sys.path: + sys.path.insert(0, _p) + +from cleveragents.application.services.checkpoint_service import ( # noqa: E402 + CheckpointService, +) +from cleveragents.infrastructure.events.models import DomainEvent # noqa: E402 +from cleveragents.infrastructure.events.types import EventType # noqa: E402 + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +_INITIAL_CONTENT = "initial content\n" +_MODIFIED_CONTENT = "modified after checkpoint\n" +_TRACKED_FILENAME = "tracked.txt" +_NEW_FILENAME = "extra.txt" +_PLAN_ID = "01JBG822CHKPT000PXAN000000" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _fail(msg: str) -> NoReturn: + """Print failure message to stderr and exit with code 1.""" + print(msg, file=sys.stderr) + sys.exit(1) + + +def _run_git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + """Run a git command and return the completed process.""" + return subprocess.run( + ["git", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + timeout=30, + ) + + +def _get_head_sha(cwd: str) -> str: + """Return the HEAD commit SHA of a git repository.""" + result = _run_git(["rev-parse", "HEAD"], cwd=cwd) + return result.stdout.strip() + + +def _create_workspace() -> str: + """Create a temporary git workspace with an initial committed file. + + Returns the path to the temporary directory. + """ + tmpdir = tempfile.mkdtemp(prefix="checkpoint_822_robot_") + try: + _run_git(["init", "--initial-branch=main"], cwd=tmpdir) + _run_git(["config", "user.email", "test@example.com"], cwd=tmpdir) + _run_git(["config", "user.name", "Test"], cwd=tmpdir) + + tracked_path = os.path.join(tmpdir, _TRACKED_FILENAME) + Path(tracked_path).write_text(_INITIAL_CONTENT, encoding="utf-8") + _run_git(["add", _TRACKED_FILENAME], cwd=tmpdir) + _run_git(["commit", "-m", "Initial commit"], cwd=tmpdir) + except Exception: + shutil.rmtree(tmpdir, ignore_errors=True) + raise + return tmpdir + + +class _RecordingEventBus: + """In-memory event bus that records emitted events.""" + + def __init__(self) -> None: + self.events: list[DomainEvent] = [] + + def emit(self, event: DomainEvent) -> None: + self.events.append(event) + + def subscribe(self, event_type: object, handler: object) -> None: + pass + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def rollback_restores_content() -> None: + """Verify that rollback reverts a modified tracked file.""" + tmpdir = _create_workspace() + try: + tracked_path = os.path.join(tmpdir, _TRACKED_FILENAME) + initial_sha = _get_head_sha(tmpdir) + + service = CheckpointService() + service.register_sandbox(_PLAN_ID, tmpdir) + checkpoint = service.create_checkpoint( + plan_id=_PLAN_ID, + sandbox_ref=initial_sha, + reason="Integration test checkpoint", + checkpoint_type="manual", + ) + + # Modify the tracked file and commit + Path(tracked_path).write_text(_MODIFIED_CONTENT) + _run_git(["add", _TRACKED_FILENAME], cwd=tmpdir) + _run_git(["commit", "-m", "Modify tracked file"], cwd=tmpdir) + + # Invoke rollback + service.rollback_to_checkpoint( + plan_id=_PLAN_ID, + checkpoint_id=checkpoint.checkpoint_id, + ) + + # Assert file content matches checkpoint state + actual = Path(tracked_path).read_text() + if actual != _INITIAL_CONTENT: + _fail( + f"Tracked file not reverted after rollback. " + f"Expected: {_INITIAL_CONTENT!r}, Got: {actual!r}." + ) + + print("checkpoint-rollback-restores-content-ok") + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +def rollback_removes_added_files() -> None: + """Verify that rollback removes files added after the checkpoint.""" + tmpdir = _create_workspace() + try: + initial_sha = _get_head_sha(tmpdir) + + service = CheckpointService() + service.register_sandbox(_PLAN_ID, tmpdir) + checkpoint = service.create_checkpoint( + plan_id=_PLAN_ID, + sandbox_ref=initial_sha, + reason="Integration test checkpoint", + checkpoint_type="manual", + ) + + # Add a new file and commit + new_path = os.path.join(tmpdir, _NEW_FILENAME) + Path(new_path).write_text("new file content\n") + _run_git(["add", _NEW_FILENAME], cwd=tmpdir) + _run_git(["commit", "-m", "Add new file"], cwd=tmpdir) + + # Invoke rollback + service.rollback_to_checkpoint( + plan_id=_PLAN_ID, + checkpoint_id=checkpoint.checkpoint_id, + ) + + # Assert the new file no longer exists + if os.path.exists(new_path): + _fail(f"New file still exists after rollback: {new_path}.") + + print("checkpoint-rollback-removes-added-files-ok") + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +def rollback_emits_event() -> None: + """Verify that rollback emits a CHECKPOINT_RESTORED domain event.""" + tmpdir = _create_workspace() + try: + initial_sha = _get_head_sha(tmpdir) + event_bus = _RecordingEventBus() + + service = CheckpointService(event_bus=event_bus) # type: ignore[arg-type] + service.register_sandbox(_PLAN_ID, tmpdir) + checkpoint = service.create_checkpoint( + plan_id=_PLAN_ID, + sandbox_ref=initial_sha, + reason="Integration test checkpoint", + checkpoint_type="manual", + ) + + # Modify and commit so rollback has something to do + tracked_path = os.path.join(tmpdir, _TRACKED_FILENAME) + Path(tracked_path).write_text(_MODIFIED_CONTENT) + _run_git(["add", _TRACKED_FILENAME], cwd=tmpdir) + _run_git(["commit", "-m", "Modify tracked file"], cwd=tmpdir) + + service.rollback_to_checkpoint( + plan_id=_PLAN_ID, + checkpoint_id=checkpoint.checkpoint_id, + ) + + restored_events = [ + e for e in event_bus.events if e.event_type == EventType.CHECKPOINT_RESTORED + ] + if len(restored_events) != 1: + _fail(f"Expected 1 CHECKPOINT_RESTORED event, got {len(restored_events)}.") + + print("checkpoint-rollback-emits-event-ok") + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +def rollback_rejects_nonexistent_sandbox() -> None: + """Verify that rollback rejects a nonexistent sandbox path.""" + from cleveragents.core.exceptions import BusinessRuleViolation + + tmpdir = _create_workspace() + try: + initial_sha = _get_head_sha(tmpdir) + + service = CheckpointService() + service.register_sandbox(_PLAN_ID, "/tmp/nonexistent_sandbox_path_822") + checkpoint = service.create_checkpoint( + plan_id=_PLAN_ID, + sandbox_ref=initial_sha, + reason="Integration test checkpoint", + checkpoint_type="manual", + ) + + try: + service.rollback_to_checkpoint( + plan_id=_PLAN_ID, + checkpoint_id=checkpoint.checkpoint_id, + ) + _fail("Expected BusinessRuleViolation for nonexistent sandbox path") + except BusinessRuleViolation as exc: + if "sandbox path does not exist" not in str(exc): + _fail(f"Unexpected error message: {exc}") + + print("checkpoint-rollback-rejects-nonexistent-sandbox-ok") + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "rollback-restores-content": rollback_restores_content, + "rollback-removes-added-files": rollback_removes_added_files, + "rollback-emits-event": rollback_emits_event, + "rollback-rejects-nonexistent-sandbox": rollback_rejects_nonexistent_sandbox, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + cmd = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/helper_checkpoint_rollback.py b/robot/helper_checkpoint_rollback.py index 8da546a43..d3493e0b6 100644 --- a/robot/helper_checkpoint_rollback.py +++ b/robot/helper_checkpoint_rollback.py @@ -6,13 +6,68 @@ prune, delete, guards, metadata, and correction-service integration. from __future__ import annotations +import os +import shutil +import subprocess import sys +import tempfile +from pathlib import Path from cleveragents.application.services.checkpoint_service import CheckpointService from cleveragents.application.services.correction_service import CorrectionService from cleveragents.core.exceptions import BusinessRuleViolation, ResourceNotFoundError from cleveragents.domain.models.core.checkpoint import CheckpointRetentionPolicy + +def _create_temp_git_workspace() -> str: + """Create a temporary git workspace with an initial commit.""" + tmpdir = tempfile.mkdtemp(prefix="checkpoint_rollback_test_") + subprocess.run( + ["git", "init", "--initial-branch=main", tmpdir], + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + tracked = os.path.join(tmpdir, "tracked.txt") + Path(tracked).write_text("initial content\n") + subprocess.run( + ["git", "add", "tracked.txt"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "commit", "-m", "Initial commit"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + return tmpdir + + +def _get_head_sha(cwd: str) -> str: + """Return HEAD commit SHA.""" + result = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=cwd, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + + _PLAN_ID = "01ARZ3NDEKTSV4RRFFQ69G5FAV" @@ -34,13 +89,40 @@ def _list_checkpoints() -> None: def _rollback_checkpoint() -> None: - svc = CheckpointService() - svc.register_sandbox(_PLAN_ID, "sandbox-123") - cp = svc.create_checkpoint(_PLAN_ID, "commit-abc") - result = svc.rollback_to_checkpoint(_PLAN_ID, cp.checkpoint_id) - assert result.restored_files_count > 0 - assert result.from_checkpoint_id == cp.checkpoint_id - print("rollback-checkpoint-ok") + tmpdir = _create_temp_git_workspace() + try: + initial_sha = _get_head_sha(tmpdir) + svc = CheckpointService() + svc.register_sandbox(_PLAN_ID, tmpdir) + cp = svc.create_checkpoint(_PLAN_ID, initial_sha) + + # Modify the tracked file and commit + Path(os.path.join(tmpdir, "tracked.txt")).write_text("modified\n") + subprocess.run( + ["git", "add", "tracked.txt"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "commit", "-m", "Modify tracked file"], + cwd=tmpdir, + check=True, + capture_output=True, + ) + + result = svc.rollback_to_checkpoint(_PLAN_ID, cp.checkpoint_id) + assert result.restored_files_count > 0 + assert result.from_checkpoint_id == cp.checkpoint_id + + # Verify actual file reversion + content = Path(os.path.join(tmpdir, "tracked.txt")).read_text() + assert content == "initial content\n", ( + f"Expected file reverted, got: {content!r}" + ) + print("rollback-checkpoint-ok") + finally: + shutil.rmtree(tmpdir, ignore_errors=True) def _rollback_applied_guard() -> None: diff --git a/robot/tdd_checkpoint_real_rollback.robot b/robot/tdd_checkpoint_real_rollback.robot index 400f36de2..7b5131eef 100644 --- a/robot/tdd_checkpoint_real_rollback.robot +++ b/robot/tdd_checkpoint_real_rollback.robot @@ -17,7 +17,7 @@ ${HELPER} ${CURDIR}/helper_tdd_checkpoint_real_rollback.py *** Test Cases *** TDD Checkpoint Rollback Restores File Content [Documentation] Verify that rollback reverts a modified file to its checkpoint state - [Tags] tdd_expected_fail tdd_bug tdd_bug_822 + [Tags] tdd_bug tdd_bug_822 ${result}= Run Process ${PYTHON} ${HELPER} rollback-restores-content cwd=${WORKSPACE} timeout=30s on_timeout=kill Log ${result.stdout} Log ${result.stderr} @@ -26,7 +26,7 @@ TDD Checkpoint Rollback Restores File Content TDD Checkpoint Rollback Removes Added Files [Documentation] Verify that rollback removes files added after the checkpoint - [Tags] tdd_expected_fail tdd_bug tdd_bug_822 + [Tags] tdd_bug tdd_bug_822 ${result}= Run Process ${PYTHON} ${HELPER} rollback-removes-added-files cwd=${WORKSPACE} timeout=30s on_timeout=kill Log ${result.stdout} Log ${result.stderr} diff --git a/src/cleveragents/application/services/checkpoint_service.py b/src/cleveragents/application/services/checkpoint_service.py index 64fad5293..6f3d178d1 100644 --- a/src/cleveragents/application/services/checkpoint_service.py +++ b/src/cleveragents/application/services/checkpoint_service.py @@ -20,7 +20,9 @@ tests that do not require the lifecycle service. from __future__ import annotations import logging +import subprocess from datetime import UTC, datetime +from pathlib import Path from typing import TYPE_CHECKING from ulid import ULID @@ -37,6 +39,8 @@ from cleveragents.domain.models.core.checkpoint import ( CheckpointRetentionPolicy, RollbackResult, ) +from cleveragents.infrastructure.events.models import DomainEvent +from cleveragents.infrastructure.events.types import EventType if TYPE_CHECKING: from cleveragents.application.services.plan_lifecycle_service import ( @@ -45,6 +49,7 @@ if TYPE_CHECKING: from cleveragents.infrastructure.database.repositories import ( CheckpointRepository, ) + from cleveragents.infrastructure.events.protocol import EventBus logger = logging.getLogger(__name__) @@ -63,15 +68,20 @@ class CheckpointService: lifecycle service is *not* available (e.g. in unit tests), the service falls back to the in-memory ``_plan_applied`` / ``_plan_sandbox_refs`` helpers. + + When *event_bus* is supplied, domain events are emitted on rollback + operations (``CHECKPOINT_RESTORED``). """ def __init__( self, repository: CheckpointRepository | None = None, plan_lifecycle_service: PlanLifecycleService | None = None, + event_bus: EventBus | None = None, ) -> None: self._repository = repository self._plan_lifecycle_service = plan_lifecycle_service + self._event_bus = event_bus # In-memory fallback stores (used only when repository is None) self._checkpoints: dict[str, Checkpoint] = {} self._plan_index: dict[str, list[str]] = {} @@ -195,7 +205,12 @@ class CheckpointService: plan_id: str, checkpoint_id: str, ) -> RollbackResult: - """Restore sandbox state to a named checkpoint. + """Restore sandbox state to a named checkpoint via ``git reset --hard``. + + Executes a real ``git reset --hard `` followed by + ``git clean -fd`` inside the sandbox working directory to revert + all tracked file changes and remove untracked files added after + the checkpoint. Args: plan_id: Plan owning the checkpoint. @@ -205,33 +220,16 @@ class CheckpointService: A ``RollbackResult`` describing restored files. Raises: - BusinessRuleViolation: If plan is applied or sandbox missing. + BusinessRuleViolation: If plan is applied, sandbox missing, + sandbox path is not a git repository, or the git reset + operation fails. ResourceNotFoundError: If checkpoint does not exist. ValidationError: If checkpoint does not belong to the plan. """ # Guard: plan must not be applied and sandbox must exist. # Prefer querying persistent plan state via the lifecycle service; # fall back to in-memory flags when the service is not wired. - if self._plan_lifecycle_service is not None: - plan = self._plan_lifecycle_service.get_plan(plan_id) - if plan is None: # pragma: no cover - defensive - raise ResourceNotFoundError(resource_type="plan", resource_id=plan_id) - from cleveragents.domain.models.core.plan import ProcessingState - - if plan.processing_state == ProcessingState.APPLIED: - raise BusinessRuleViolation("Cannot rollback: plan is already applied") - if not plan.sandbox_refs: - raise BusinessRuleViolation( - "Cannot rollback: sandbox is missing for this plan" - ) - else: - # In-memory fallback (used in unit tests without lifecycle service) - if plan_id in self._plan_applied: - raise BusinessRuleViolation("Cannot rollback: plan is already applied") - if plan_id not in self._plan_sandbox_refs: - raise BusinessRuleViolation( - "Cannot rollback: sandbox is missing for this plan" - ) + sandbox_path = self._resolve_sandbox_path(plan_id) # Look up checkpoint checkpoint = self._get_checkpoint(checkpoint_id) @@ -242,12 +240,21 @@ class CheckpointService: f"Checkpoint {checkpoint_id} does not belong to plan {plan_id}" ) - # Simulate rollback: in a real implementation this would call - # git reset --hard in the worktree. Here we - # produce a result that describes the restore. sandbox_ref = checkpoint.sandbox_ref - changed_paths = [f"restored:{sandbox_ref}"] - restored_count = 1 + + # Validate sandbox boundary: path must exist and be a git repo + self._validate_sandbox(sandbox_path) + + # Compute the list of changed paths before reset for reporting + changed_paths = self._git_changed_paths(sandbox_path, sandbox_ref) + + # Execute real git reset --hard to restore sandbox to checkpoint + self._git_reset_hard(sandbox_path, sandbox_ref) + + # Remove untracked files that were added after the checkpoint + self._git_clean(sandbox_path) + + restored_count = len(changed_paths) logger.info( "checkpoint.rollback", @@ -255,9 +262,26 @@ class CheckpointService: "checkpoint_id": checkpoint_id, "plan_id": plan_id, "sandbox_ref": sandbox_ref, + "restored_files_count": restored_count, + "changed_paths": changed_paths, }, ) + # Emit domain event for rollback + if self._event_bus is not None: + self._event_bus.emit( + DomainEvent( + event_type=EventType.CHECKPOINT_RESTORED, + plan_id=plan_id, + details={ + "checkpoint_id": checkpoint_id, + "sandbox_ref": sandbox_ref, + "restored_files_count": restored_count, + "changed_paths": changed_paths, + }, + ) + ) + return RollbackResult( restored_files_count=restored_count, changed_paths=changed_paths, @@ -371,6 +395,147 @@ class CheckpointService: # Private helpers # ------------------------------------------------------------------ + def _resolve_sandbox_path(self, plan_id: str) -> str: + """Resolve and validate the sandbox working directory for a plan. + + Applies guard checks (plan must not be applied, sandbox must + exist) and returns the filesystem path of the sandbox. + + Args: + plan_id: The plan whose sandbox to resolve. + + Returns: + The sandbox filesystem path. + + Raises: + BusinessRuleViolation: If plan is applied or sandbox missing. + ResourceNotFoundError: If plan not found (lifecycle path). + """ + if self._plan_lifecycle_service is not None: + plan = self._plan_lifecycle_service.get_plan(plan_id) + if plan is None: # pragma: no cover - defensive + raise ResourceNotFoundError(resource_type="plan", resource_id=plan_id) + from cleveragents.domain.models.core.plan import ProcessingState + + if plan.processing_state == ProcessingState.APPLIED: + raise BusinessRuleViolation("Cannot rollback: plan is already applied") + if not plan.sandbox_refs: + raise BusinessRuleViolation( + "Cannot rollback: sandbox is missing for this plan" + ) + return str(plan.sandbox_refs[0]) + + # In-memory fallback (used in unit tests without lifecycle service) + if plan_id in self._plan_applied: + raise BusinessRuleViolation("Cannot rollback: plan is already applied") + if plan_id not in self._plan_sandbox_refs: + raise BusinessRuleViolation( + "Cannot rollback: sandbox is missing for this plan" + ) + return self._plan_sandbox_refs[plan_id] + + def _validate_sandbox(self, sandbox_path: str) -> None: + """Validate that the sandbox path exists and is a git repository. + + Args: + sandbox_path: Filesystem path to the sandbox. + + Raises: + BusinessRuleViolation: If the path does not exist or is not + a git repository. + """ + path = Path(sandbox_path) + if not path.is_dir(): + raise BusinessRuleViolation( + f"Cannot rollback: sandbox path does not exist: {sandbox_path}" + ) + git_dir = path / ".git" + if not git_dir.exists(): + raise BusinessRuleViolation( + f"Cannot rollback: sandbox is not a git repository: {sandbox_path}" + ) + + def _run_git( + self, + args: list[str], + cwd: str, + ) -> subprocess.CompletedProcess[str]: + """Run a git command inside the sandbox directory. + + Args: + args: Git subcommand and arguments. + cwd: Working directory for the git process. + + Returns: + The completed process result. + + Raises: + BusinessRuleViolation: If the git command fails. + """ + try: + return subprocess.run( + ["git", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + timeout=60, + ) + except subprocess.CalledProcessError as exc: + raise BusinessRuleViolation( + f"Git operation failed: git {' '.join(args)}: {exc.stderr.strip()}" + ) from exc + except subprocess.TimeoutExpired as exc: + raise BusinessRuleViolation( + f"Git operation timed out: git {' '.join(args)}" + ) from exc + + def _git_changed_paths(self, sandbox_path: str, target_ref: str) -> list[str]: + """Compute the list of files that differ between HEAD and target ref. + + Args: + sandbox_path: Filesystem path to the sandbox. + target_ref: Git commit reference to compare against. + + Returns: + List of relative file paths that changed. + """ + result = self._run_git( + ["diff", "--name-only", target_ref, "HEAD"], + cwd=sandbox_path, + ) + paths = [line for line in result.stdout.strip().splitlines() if line] + # Also include untracked files that would be removed by git clean + untracked_result = self._run_git( + ["ls-files", "--others", "--exclude-standard"], + cwd=sandbox_path, + ) + untracked = [ + line for line in untracked_result.stdout.strip().splitlines() if line + ] + all_paths = list(dict.fromkeys(paths + untracked)) + return all_paths + + def _git_reset_hard(self, sandbox_path: str, target_ref: str) -> None: + """Execute ``git reset --hard `` in the sandbox. + + Enforces sandbox boundary by running the command only within the + sandbox directory. + + Args: + sandbox_path: Filesystem path to the sandbox. + target_ref: Git commit reference to reset to. + """ + self._run_git(["reset", "--hard", target_ref], cwd=sandbox_path) + + def _git_clean(self, sandbox_path: str) -> None: + """Execute ``git clean -fd`` to remove untracked files and dirs. + + Args: + sandbox_path: Filesystem path to the sandbox. + """ + self._run_git(["clean", "-fd"], cwd=sandbox_path) + def _get_checkpoint(self, checkpoint_id: str) -> Checkpoint: """Retrieve a checkpoint via repo or in-memory store.""" if self._repository is not None: -- 2.52.0