fix(sandbox): store sandbox_path in checkpoint metadata to enable rollback
CheckpointManager.create_checkpoint() computed sandbox_path from
sandbox.context.sandbox_path but never stored it in the metadata dict.
This caused rollback_to() to always find metadata.get('sandbox_path')
returning None, silently skip the rollback, and return False.
The fix adds sandbox_path to the metadata dict immediately after it is
resolved from the sandbox context, before the SandboxCheckpoint is
constructed. rollback_to() can now retrieve the path and correctly
restore the sandbox filesystem state.
Added two new BDD scenarios to checkpoint_manager_coverage.feature:
- Verifies sandbox_path is automatically stored in metadata on create
- Verifies rollback succeeds without manually supplying sandbox_path
ISSUES CLOSED: #7488
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user