diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d2b812af..595123305 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). message listing available built-in profiles. The resolved profile name is also logged at debug level for observability. +- **CheckpointManager rollback_to always returned False** (#7488): Fixed a data + integrity bug in `CheckpointManager.create_checkpoint()` where `sandbox_path` + was computed from `sandbox.context.sandbox_path` but never stored in the + checkpoint metadata. As a result, `rollback_to()` always found + `checkpoint.metadata.get("sandbox_path")` returning `None` and silently + skipped the rollback, returning `False`. The fix adds `sandbox_path` to the + metadata dict before constructing the `SandboxCheckpoint`, enabling + `rollback_to()` to correctly restore the sandbox filesystem state. + ### Added - **TDD Issue-Capture Test Activation** (#7025): Replaced 234 bare `@skip` tags diff --git a/features/checkpoint_manager_coverage.feature b/features/checkpoint_manager_coverage.feature index d4334cc58..ee185ae37 100644 --- a/features/checkpoint_manager_coverage.feature +++ b/features/checkpoint_manager_coverage.feature @@ -71,6 +71,23 @@ Feature: CheckpointManager full coverage When I invoke create_checkpoint with plan "plan-s1" phase "pre_execute" and no metadata Then the snapshot directory should contain copies of the sample files + Scenario: Creating a checkpoint with a real sandbox path stores sandbox_path in metadata + Given a freshly created CheckpointManager + And a temporary sandbox directory with sample files + And a mock sandbox whose context points to the temporary directory + When I invoke create_checkpoint with plan "plan-s2" phase "pre_execute" and no metadata + Then the returned checkpoint metadata should contain the sandbox path + + Scenario: Rollback succeeds without manually supplying sandbox_path in metadata + Given a freshly created CheckpointManager + And a temporary sandbox directory with sample files + And a mock sandbox whose context points to the temporary directory + And a checkpoint was created for the temporary sandbox without explicit sandbox_path metadata + And the temporary sandbox files are then modified + When I invoke rollback_to on the created checkpoint + Then the rollback result should be true + And the temporary sandbox should contain the original files + # --------------------------------------------------------------------------- # _snapshot_directory – branches # --------------------------------------------------------------------------- diff --git a/features/steps/checkpoint_manager_coverage_steps.py b/features/steps/checkpoint_manager_coverage_steps.py index 038204418..542ae71c6 100644 --- a/features/steps/checkpoint_manager_coverage_steps.py +++ b/features/steps/checkpoint_manager_coverage_steps.py @@ -650,3 +650,43 @@ def step_delete_unknown_cp(context: Context, cp_id: str) -> None: @then("the checkpoint delete result should be false") def step_assert_delete_false(context: Context) -> None: assert context.delete_result is False, "Expected delete to return False" + + +# =========================================================================== +# Bug fix #7488 — sandbox_path auto-stored in metadata +# =========================================================================== + + +@then("the returned checkpoint metadata should contain the sandbox path") +def step_assert_metadata_has_sandbox_path(context: Context) -> None: + """Verify that create_checkpoint() automatically stores sandbox_path in metadata.""" + cp = context.cp_result + _register_cleanup(context, os.path.dirname(cp.snapshot_path)) + assert "sandbox_path" in cp.metadata, ( + "Expected 'sandbox_path' key in checkpoint metadata, but it was not found. " + "Bug #7488: create_checkpoint() must store sandbox_path in metadata." + ) + assert cp.metadata["sandbox_path"] == context.temp_sandbox, ( + f"Expected sandbox_path={context.temp_sandbox!r}, " + f"got {cp.metadata['sandbox_path']!r}" + ) + + +@given( + "a checkpoint was created for the temporary sandbox without explicit sandbox_path metadata" +) +def step_create_cp_for_temp_sandbox_no_explicit_path(context: Context) -> None: + """Create a checkpoint without manually supplying sandbox_path in metadata. + + After the fix for bug #7488, create_checkpoint() should automatically + store sandbox_path in metadata from sandbox.context.sandbox_path. + """ + context.cp_result = context.mgr.create_checkpoint( + sandbox=context.mock_sb, + plan_id="plan-rb-autopath", + phase="pre_execute", + # Intentionally NOT passing sandbox_path in metadata — the fix should + # auto-populate it from sandbox.context.sandbox_path. + ) + _register_cleanup(context, os.path.dirname(context.cp_result.snapshot_path)) + context.crafted_cp = context.cp_result diff --git a/src/cleveragents/infrastructure/sandbox/checkpoint.py b/src/cleveragents/infrastructure/sandbox/checkpoint.py index cd65b346d..65ffc4ef9 100644 --- a/src/cleveragents/infrastructure/sandbox/checkpoint.py +++ b/src/cleveragents/infrastructure/sandbox/checkpoint.py @@ -148,6 +148,9 @@ class CheckpointManager: if sandbox.context is not None: sandbox_path = sandbox.context.sandbox_path + if sandbox_path is not None: + meta["sandbox_path"] = str(sandbox_path) + snapshot_path = self._snapshot_directory(sandbox_path, checkpoint_id) checkpoint = SandboxCheckpoint(