feat(correction): wire checkpoint rollback into correction service revert flow
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 18s
CI / build (pull_request) Successful in 20s
CI / helm (pull_request) Successful in 22s
CI / typecheck (pull_request) Successful in 1m0s
CI / quality (pull_request) Successful in 3m41s
CI / integration_tests (pull_request) Successful in 3m51s
CI / security (pull_request) Successful in 4m3s
CI / unit_tests (pull_request) Successful in 4m11s
CI / docker (pull_request) Successful in 1m31s
CI / coverage (pull_request) Successful in 11m48s
CI / e2e_tests (pull_request) Successful in 21m45s
CI / status-check (pull_request) Successful in 1s
CI / build (push) Successful in 17s
CI / helm (push) Successful in 21s
CI / quality (push) Successful in 3m48s
CI / integration_tests (push) Successful in 3m58s
CI / typecheck (push) Successful in 3m58s
CI / security (push) Successful in 4m6s
CI / unit_tests (push) Successful in 7m16s
CI / lint (push) Successful in 8m14s
CI / benchmark-regression (push) Has been skipped
CI / docker (push) Successful in 1m36s
CI / e2e_tests (push) Successful in 18m9s
CI / coverage (push) Successful in 12m13s
CI / status-check (push) Successful in 1s
CI / benchmark-publish (push) Successful in 28m11s
CI / benchmark-regression (pull_request) Successful in 54m54s

Implement the full correction-checkpoint rollback pipeline:

- Workspace snapshots: CheckpointService.create_workspace_snapshot()
  creates diff-based checkpoints before decision execution, storing
  only changed file paths in metadata.extra["diff_paths"]

- CorrectionService.revert_decisions(): new high-level entry point
  that creates a correction, computes impact, invokes checkpoint
  rollback, and archives artifacts in a single call

- Physical artifact archival: CheckpointService.archive_artifacts()
  moves files to .cleveragents/archived_artifacts/ instead of just
  flagging metadata. CorrectionService._archive_decision_artifacts()
  delegates to this during revert execution

- Selective rollback: CheckpointService.selective_rollback() wraps
  rollback_to_checkpoint with atomic semantics — captures HEAD before
  rollback and recovers on failure

- Diff-based storage: _compute_diff_snapshot() computes changed paths
  between checkpoints via git diff; snapshots store diff manifest and
  SHA-256 hash in metadata

- CLI: plan rollback now accepts --to-checkpoint <id> in addition to
  the positional checkpoint_id argument; uses selective_rollback for
  atomic execution

- DI wiring: Container now injects checkpoint_service into
  CorrectionService; CLI correct command uses container-provided
  service instead of ad-hoc instance (fixes bug #986)

- Checkpoint model: pre_decision added to allowed checkpoint_type
  values

- TDD: Removed @tdd_expected_fail from wiring test feature since the
  DI bug is now fixed

ISSUES CLOSED: #943
This commit was merged in pull request #1199.
This commit is contained in:
2026-03-29 08:38:29 +00:00
parent afe6b849fe
commit 38e05ac45a
18 changed files with 953 additions and 79 deletions
@@ -0,0 +1,111 @@
@phase2 @correction @checkpoint @rollback
Feature: Correction checkpoint rollback wiring
As an operator
I want corrections to roll back filesystem state via workspace checkpoints
So that reverted decisions leave no stale artifacts on disk
#
# Workspace snapshot creation
#
Scenario: Workspace snapshot created before decision execution
Given ccr a checkpoint service
When ccr I create a workspace snapshot for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" decision "01DEC1S10N0000000000000000" with sandbox_ref "abc123"
Then ccr the snapshot should be created with type "pre_decision"
And ccr the snapshot should be aligned to decision "01DEC1S10N0000000000000000"
And ccr the snapshot metadata should indicate diff-based storage
Scenario: Workspace snapshot stores diff manifest in metadata
Given ccr a checkpoint service
When ccr I create a workspace snapshot for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV" decision "01DEC1S10N0000000000000000" with sandbox_ref "abc123"
Then ccr the snapshot metadata extra should contain "diff_based" as true
And ccr the snapshot metadata extra should contain a "diff_hash"
# ───────────────────────────────────────────────────────────
# CorrectionService.revert_decisions() wiring
# ───────────────────────────────────────────────────────────
Scenario: revert_decisions invokes checkpoint rollback
Given ccr a correction service with checkpoint support
When ccr I call revert_decisions for plan "01PL4N000000000000000000" targeting decision "01DEC0000000000000000000"
Then ccr the correction result should have status "applied"
And ccr the correction result should have reverted decisions
Scenario: revert_decisions without checkpoint service still succeeds
Given ccr a correction service without checkpoint support
When ccr I call revert_decisions for plan "01PL4N000000000000000000" targeting decision "01DEC0000000000000000000"
Then ccr the correction result should have status "applied"
And ccr the correction result checkpoint_restored should be false
# ───────────────────────────────────────────────────────────
# Physical artifact archival
# ───────────────────────────────────────────────────────────
Scenario: Artifacts physically moved to archive directory
Given ccr a temporary sandbox directory with artifact files
And ccr a checkpoint service
When ccr I archive artifacts from the sandbox
Then ccr the artifacts should exist in the archive directory
And ccr the artifacts should not exist in their original locations
Scenario: Archive tolerates missing artifact files
Given ccr a temporary sandbox directory without artifact files
And ccr a checkpoint service
When ccr I archive artifacts from the sandbox
Then ccr no errors should be raised
And ccr the archived count should be 0
# ───────────────────────────────────────────────────────────
# Selective rollback
# ───────────────────────────────────────────────────────────
Scenario: Selective rollback to a specific checkpoint
Given ccr a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
And ccr checkpoints "cp1" and "cp2" exist for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
When ccr I selectively rollback to checkpoint "cp1"
Then ccr the rollback should succeed
Scenario: Selective rollback is atomic on failure
Given ccr a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
When ccr I attempt selective rollback to a nonexistent checkpoint
Then ccr the sandbox should remain at the original HEAD
# ───────────────────────────────────────────────────────────
# Diff-based storage
# ───────────────────────────────────────────────────────────
Scenario: Diff-based checkpoint stores only changed file paths
Given ccr a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
And ccr an initial checkpoint exists for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
And ccr a file is modified and committed in the sandbox
When ccr I create a workspace snapshot capturing the diff
Then ccr the snapshot diff_paths should include the modified file
# ───────────────────────────────────────────────────────────
# Atomic rollback
# ───────────────────────────────────────────────────────────
Scenario: Atomic rollback fully restores on success
Given ccr a checkpoint service with sandbox for plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
And ccr a checkpoint is created from sandbox HEAD
And ccr a file is modified and committed in the sandbox
When ccr I perform selective rollback to the checkpoint
Then ccr the modified file should be reverted to its original state
# ───────────────────────────────────────────────────────────
# DI container wiring
# ───────────────────────────────────────────────────────────
Scenario: DI container wires checkpoint_service into CorrectionService
Given ccr a fresh DI container
When ccr I resolve the correction_service from the container
Then ccr the correction_service should have a non-None checkpoint_service
# ───────────────────────────────────────────────────────────
# CLI rollback --to-checkpoint
# ───────────────────────────────────────────────────────────
Scenario: plan rollback --to-checkpoint option accepted
Given ccr a CLI runner
When ccr I invoke plan rollback with --to-checkpoint option
Then ccr the command should recognize the --to-checkpoint flag
@@ -95,6 +95,7 @@ def _make_plan(_plan_id: str) -> Plan:
def make_mock_container(
decisions: list[SimpleNamespace],
influence_edges: dict[str, list[str]],
correction_svc: MagicMock | None = None,
) -> MagicMock:
"""Build a mock DI container returning DecisionService and PlanLifecycleService."""
mock_decision_svc = MagicMock()
@@ -110,6 +111,8 @@ def make_mock_container(
mock_container = MagicMock()
mock_container.decision_service.return_value = mock_decision_svc
mock_container.plan_lifecycle_service.return_value = mock_plan_svc
if correction_svc is not None:
mock_container.correction_service.return_value = correction_svc
return mock_container
@@ -211,7 +211,7 @@ def step_build_session_factory_invalid(context: Any) -> None:
# SQLAlchemy lazily connects; the factory itself succeeds, but
# calling it to create a session and executing SQL raises.
context.boost_session_factory = _build_session_factory(
"sqlite:///nonexistent/path/to/db.sqlite"
"sqlite:////proc/nonexistent/impossible/db.sqlite"
)
@@ -0,0 +1,434 @@
"""Step definitions for correction_checkpoint_rollback.feature.
Exercises the wiring of checkpoint rollback into the correction service
revert flow, including workspace snapshots, physical artifact archival,
selective rollback, diff-based storage, and atomic rollback semantics.
"""
from __future__ import annotations
import os
import subprocess
import tempfile
from pathlib import Path
from behave import given, then, when
from cleveragents.application.services.checkpoint_service import CheckpointService
from cleveragents.application.services.correction_service import CorrectionService
from cleveragents.core.exceptions import BusinessRuleViolation, ResourceNotFoundError
# ─── Helpers ───────────────────────────────────────────────────────────
def _init_sandbox(tmpdir: str) -> str:
"""Create a bare git repo sandbox in tmpdir and return its path."""
sandbox = os.path.join(tmpdir, "sandbox")
os.makedirs(sandbox)
subprocess.run(["git", "init"], cwd=sandbox, capture_output=True, check=True)
subprocess.run(
["git", "config", "user.email", "test@test.com"],
cwd=sandbox,
capture_output=True,
check=True,
)
subprocess.run(
["git", "config", "user.name", "Test"],
cwd=sandbox,
capture_output=True,
check=True,
)
# Initial commit
init_file = os.path.join(sandbox, "README.md")
with open(init_file, "w") as f:
f.write("initial\n")
subprocess.run(["git", "add", "."], cwd=sandbox, capture_output=True, check=True)
subprocess.run(
["git", "commit", "-m", "init"],
cwd=sandbox,
capture_output=True,
check=True,
)
return sandbox
def _get_head(sandbox: str) -> str:
"""Get current HEAD commit hash."""
result = subprocess.run(
["git", "rev-parse", "HEAD"],
cwd=sandbox,
capture_output=True,
text=True,
check=True,
)
return result.stdout.strip()
PLAN_ID = "01ARZ3NDEKTSV4RRFFQ69G5FAV"
# ─── Workspace snapshot creation ──────────────────────────────────────
@given("ccr a checkpoint service")
def step_ccr_checkpoint_service(context):
context.ccr_svc = CheckpointService()
@when(
'ccr I create a workspace snapshot for plan "{plan_id}" '
'decision "{decision_id}" with sandbox_ref "{ref}"'
)
def step_ccr_create_workspace_snapshot(context, plan_id, decision_id, ref):
context.ccr_snapshot = context.ccr_svc.create_workspace_snapshot(
plan_id=plan_id,
sandbox_ref=ref,
decision_id=decision_id,
)
@then('ccr the snapshot should be created with type "{cp_type}"')
def step_ccr_snapshot_type(context, cp_type):
assert context.ccr_snapshot.checkpoint_type == cp_type
@then('ccr the snapshot should be aligned to decision "{decision_id}"')
def step_ccr_snapshot_decision(context, decision_id):
assert context.ccr_snapshot.decision_id == decision_id
@then("ccr the snapshot metadata should indicate diff-based storage")
def step_ccr_snapshot_diff_based(context):
assert context.ccr_snapshot.metadata.extra.get("diff_based") is True
@then('ccr the snapshot metadata extra should contain "diff_based" as true')
def step_ccr_snapshot_diff_based_true(context):
assert context.ccr_snapshot.metadata.extra.get("diff_based") is True
@then('ccr the snapshot metadata extra should contain a "diff_hash"')
def step_ccr_snapshot_diff_hash(context):
assert "diff_hash" in context.ccr_snapshot.metadata.extra
assert isinstance(context.ccr_snapshot.metadata.extra["diff_hash"], str)
assert len(context.ccr_snapshot.metadata.extra["diff_hash"]) > 0
# ─── CorrectionService.revert_decisions() wiring ─────────────────────
@given("ccr a correction service with checkpoint support")
def step_ccr_correction_with_checkpoint(context):
context.ccr_cp_svc = CheckpointService()
context.ccr_corr_svc = CorrectionService(
checkpoint_service=context.ccr_cp_svc,
)
@given("ccr a correction service without checkpoint support")
def step_ccr_correction_without_checkpoint(context):
context.ccr_corr_svc = CorrectionService()
@when(
'ccr I call revert_decisions for plan "{plan_id}" '
'targeting decision "{decision_id}"'
)
def step_ccr_revert_decisions(context, plan_id, decision_id):
context.ccr_result = context.ccr_corr_svc.revert_decisions(
plan_id=plan_id,
target_decision_id=decision_id,
)
@then('ccr the correction result should have status "{status}"')
def step_ccr_result_status(context, status):
assert context.ccr_result.status.value == status
@then("ccr the correction result should have reverted decisions")
def step_ccr_result_has_reverted(context):
assert len(context.ccr_result.reverted_decisions) > 0
@then("ccr the correction result checkpoint_restored should be false")
def step_ccr_result_no_checkpoint(context):
assert context.ccr_result.checkpoint_restored is False
# ─── Physical artifact archival ───────────────────────────────────────
@given("ccr a temporary sandbox directory with artifact files")
def step_ccr_sandbox_with_artifacts(context):
context.ccr_tmpdir = tempfile.mkdtemp()
sandbox = os.path.join(context.ccr_tmpdir, "sandbox")
os.makedirs(sandbox)
# Create artifact files
context.ccr_artifact_paths = ["output/result.txt", "output/log.txt"]
for rel in context.ccr_artifact_paths:
full = os.path.join(sandbox, rel)
os.makedirs(os.path.dirname(full), exist_ok=True)
with open(full, "w") as f:
f.write(f"artifact: {rel}\n")
context.ccr_sandbox_path = sandbox
@given("ccr a temporary sandbox directory without artifact files")
def step_ccr_sandbox_without_artifacts(context):
context.ccr_tmpdir = tempfile.mkdtemp()
sandbox = os.path.join(context.ccr_tmpdir, "sandbox")
os.makedirs(sandbox)
context.ccr_artifact_paths = ["nonexistent/file.txt"]
context.ccr_sandbox_path = sandbox
@when("ccr I archive artifacts from the sandbox")
def step_ccr_archive_artifacts(context):
context.ccr_error = None
try:
context.ccr_archived = context.ccr_svc.archive_artifacts(
context.ccr_sandbox_path,
context.ccr_artifact_paths,
)
except Exception as exc:
context.ccr_error = exc
context.ccr_archived = []
@then("ccr the artifacts should exist in the archive directory")
def step_ccr_artifacts_in_archive(context):
archive_root = (
Path(context.ccr_sandbox_path) / ".cleveragents" / "archived_artifacts"
)
for rel in context.ccr_archived:
assert (archive_root / rel).exists(), f"Archived file missing: {rel}"
@then("ccr the artifacts should not exist in their original locations")
def step_ccr_artifacts_not_in_original(context):
for rel in context.ccr_archived:
original = Path(context.ccr_sandbox_path) / rel
assert not original.exists(), f"Original file still exists: {rel}"
@then("ccr no errors should be raised")
def step_ccr_no_errors(context):
assert context.ccr_error is None
@then("ccr the archived count should be {count:d}")
def step_ccr_archived_count(context, count):
assert len(context.ccr_archived) == count
# ─── Selective rollback ───────────────────────────────────────────────
@given('ccr a checkpoint service with sandbox for plan "{plan_id}"')
def step_ccr_svc_with_sandbox(context, plan_id):
context.ccr_tmpdir = tempfile.mkdtemp()
context.ccr_sandbox = _init_sandbox(context.ccr_tmpdir)
context.ccr_svc = CheckpointService()
context.ccr_svc.register_sandbox(plan_id, context.ccr_sandbox)
context.ccr_plan_id = plan_id
@given('ccr checkpoints "cp1" and "cp2" exist for plan "{plan_id}"')
def step_ccr_two_checkpoints(context, plan_id):
head1 = _get_head(context.ccr_sandbox)
context.ccr_cp1 = context.ccr_svc.create_checkpoint(
plan_id=plan_id,
sandbox_ref=head1,
reason="cp1",
)
# Make a change and commit for cp2
fpath = os.path.join(context.ccr_sandbox, "file2.txt")
with open(fpath, "w") as f:
f.write("cp2 content\n")
subprocess.run(
["git", "add", "."], cwd=context.ccr_sandbox, capture_output=True, check=True
)
subprocess.run(
["git", "commit", "-m", "cp2"],
cwd=context.ccr_sandbox,
capture_output=True,
check=True,
)
head2 = _get_head(context.ccr_sandbox)
context.ccr_cp2 = context.ccr_svc.create_checkpoint(
plan_id=plan_id,
sandbox_ref=head2,
reason="cp2",
)
@when('ccr I selectively rollback to checkpoint "cp1"')
def step_ccr_selective_rollback_cp1(context):
context.ccr_rollback_result = context.ccr_svc.selective_rollback(
context.ccr_plan_id,
context.ccr_cp1.checkpoint_id,
)
@then("ccr the rollback should succeed")
def step_ccr_rollback_success(context):
assert context.ccr_rollback_result is not None
assert context.ccr_rollback_result.restored_files_count >= 0
@when("ccr I attempt selective rollback to a nonexistent checkpoint")
def step_ccr_selective_rollback_nonexistent(context):
context.ccr_original_head = _get_head(context.ccr_sandbox)
context.ccr_rollback_error = None
try:
context.ccr_svc.selective_rollback(
context.ccr_plan_id,
"01NONEXISTENT0000000000000",
)
except (ResourceNotFoundError, BusinessRuleViolation) as exc:
context.ccr_rollback_error = exc
@then("ccr the sandbox should remain at the original HEAD")
def step_ccr_sandbox_at_original_head(context):
assert context.ccr_rollback_error is not None
current_head = _get_head(context.ccr_sandbox)
assert current_head == context.ccr_original_head
# ─── Diff-based storage ──────────────────────────────────────────────
@given('ccr an initial checkpoint exists for plan "{plan_id}"')
def step_ccr_initial_checkpoint(context, plan_id):
head = _get_head(context.ccr_sandbox)
context.ccr_initial_cp = context.ccr_svc.create_checkpoint(
plan_id=plan_id,
sandbox_ref=head,
reason="initial",
)
@given("ccr a file is modified and committed in the sandbox")
def step_ccr_modify_file(context):
fpath = os.path.join(context.ccr_sandbox, "modified.txt")
with open(fpath, "w") as f:
f.write("modified content\n")
subprocess.run(
["git", "add", "."], cwd=context.ccr_sandbox, capture_output=True, check=True
)
subprocess.run(
["git", "commit", "-m", "modify"],
cwd=context.ccr_sandbox,
capture_output=True,
check=True,
)
context.ccr_modified_file = "modified.txt"
@when("ccr I create a workspace snapshot capturing the diff")
def step_ccr_workspace_snapshot_diff(context):
head = _get_head(context.ccr_sandbox)
context.ccr_diff_snapshot = context.ccr_svc.create_workspace_snapshot(
plan_id=context.ccr_plan_id,
sandbox_ref=head,
decision_id="01DEC0000000000000000000",
)
@then("ccr the snapshot diff_paths should include the modified file")
def step_ccr_diff_paths_contain_modified(context):
diff_paths = context.ccr_diff_snapshot.metadata.extra.get("diff_paths", [])
assert context.ccr_modified_file in diff_paths, (
f"Expected {context.ccr_modified_file!r} in {diff_paths}"
)
# ─── Atomic rollback ─────────────────────────────────────────────────
@given("ccr a checkpoint is created from sandbox HEAD")
def step_ccr_checkpoint_from_head(context):
head = _get_head(context.ccr_sandbox)
context.ccr_atomic_cp = context.ccr_svc.create_checkpoint(
plan_id=context.ccr_plan_id,
sandbox_ref=head,
reason="atomic-test",
)
# Record original content
readme = os.path.join(context.ccr_sandbox, "README.md")
with open(readme) as f:
context.ccr_original_readme = f.read()
@when("ccr I perform selective rollback to the checkpoint")
def step_ccr_selective_rollback_atomic(context):
context.ccr_rollback_result = context.ccr_svc.selective_rollback(
context.ccr_plan_id,
context.ccr_atomic_cp.checkpoint_id,
)
@then("ccr the modified file should be reverted to its original state")
def step_ccr_file_reverted(context):
readme = os.path.join(context.ccr_sandbox, "README.md")
with open(readme) as f:
content = f.read()
assert content == context.ccr_original_readme
# ─── DI container wiring ─────────────────────────────────────────────
@given("ccr a fresh DI container")
def step_ccr_fresh_container(context):
# Container will be resolved in the when step
pass
@when("ccr I resolve the correction_service from the container")
def step_ccr_resolve_correction_service(context):
from cleveragents.application.container import get_container
container = get_container()
context.ccr_container_correction_svc = container.correction_service()
@then("ccr the correction_service should have a non-None checkpoint_service")
def step_ccr_correction_has_checkpoint(context):
svc = context.ccr_container_correction_svc
assert svc._checkpoint_service is not None, (
"CorrectionService._checkpoint_service should be wired by the DI container"
)
# ─── CLI rollback --to-checkpoint ─────────────────────────────────────
@given("ccr a CLI runner")
def step_ccr_cli_runner(context):
from typer.testing import CliRunner
context.ccr_cli_runner = CliRunner()
@when("ccr I invoke plan rollback with --to-checkpoint option")
def step_ccr_cli_rollback_to_checkpoint(context):
from cleveragents.cli.commands.plan import app as plan_app
result = context.ccr_cli_runner.invoke(
plan_app,
["rollback", "PLAN-DUMMY", "--to-checkpoint", "CP-DUMMY", "--yes"],
)
context.ccr_cli_result = result
@then("ccr the command should recognize the --to-checkpoint flag")
def step_ccr_cli_to_checkpoint_flag(context):
result = context.ccr_cli_result
# Typer/Click returns exit code 2 with "No such option" for unrecognized flags.
# Any other exit code means the flag was accepted by the CLI parser.
assert "No such option" not in result.output, (
f"--to-checkpoint was not recognized: {result.output}"
)
+5 -7
View File
@@ -493,7 +493,7 @@ def _make_rollback_result() -> RollbackResult:
def step_mock_rollback_success(context: Context) -> None:
mock_container = MagicMock()
mock_cp_svc = MagicMock()
mock_cp_svc.rollback_to_checkpoint.return_value = _make_rollback_result()
mock_cp_svc.selective_rollback.return_value = _make_rollback_result()
mock_container.checkpoint_service.return_value = mock_cp_svc
p = patch(_PATCH_CONTAINER, return_value=mock_container)
@@ -507,7 +507,7 @@ def step_mock_rollback_success(context: Context) -> None:
def step_mock_rollback_business_rule(context: Context) -> None:
mock_container = MagicMock()
mock_cp_svc = MagicMock()
mock_cp_svc.rollback_to_checkpoint.side_effect = BusinessRuleViolation(
mock_cp_svc.selective_rollback.side_effect = BusinessRuleViolation(
"Plan is already applied"
)
mock_container.checkpoint_service.return_value = mock_cp_svc
@@ -523,7 +523,7 @@ def step_mock_rollback_business_rule(context: Context) -> None:
def step_mock_rollback_not_found(context: Context) -> None:
mock_container = MagicMock()
mock_cp_svc = MagicMock()
mock_cp_svc.rollback_to_checkpoint.side_effect = ResourceNotFoundError(
mock_cp_svc.selective_rollback.side_effect = ResourceNotFoundError(
"Checkpoint not found"
)
mock_container.checkpoint_service.return_value = mock_cp_svc
@@ -537,9 +537,7 @@ def step_mock_rollback_not_found(context: Context) -> None:
def step_mock_rollback_validation(context: Context) -> None:
mock_container = MagicMock()
mock_cp_svc = MagicMock()
mock_cp_svc.rollback_to_checkpoint.side_effect = ValidationError(
"Invalid checkpoint"
)
mock_cp_svc.selective_rollback.side_effect = ValidationError("Invalid checkpoint")
mock_container.checkpoint_service.return_value = mock_cp_svc
p = patch(_PATCH_CONTAINER, return_value=mock_container)
@@ -551,7 +549,7 @@ def step_mock_rollback_validation(context: Context) -> None:
def step_mock_rollback_clever_error(context: Context) -> None:
mock_container = MagicMock()
mock_cp_svc = MagicMock()
mock_cp_svc.rollback_to_checkpoint.side_effect = CleverAgentsError(
mock_cp_svc.selective_rollback.side_effect = CleverAgentsError(
"unknown rollback failure"
)
mock_container.checkpoint_service.return_value = mock_cp_svc
@@ -48,7 +48,6 @@ _ULID_A = "01ARZ3NDEKTSV4RRFFQ69G5FAA"
_PATCH_GET_LIFECYCLE = "cleveragents.cli.commands.plan._get_lifecycle_service"
_PATCH_GET_APPLY = "cleveragents.cli.commands.plan._get_apply_service"
_PATCH_CORRECTION_SVC = "cleveragents.cli.commands.plan.CorrectionService"
_PATCH_RESOLVE_ACTIVE = "cleveragents.cli.commands.plan._resolve_active_plan_id"
_PATCH_CONTAINER = "cleveragents.application.container.get_container"
@@ -502,15 +501,9 @@ def _invoke_correct(
mock_decision_svc.get_influence_edges.return_value = {}
mock_container = MagicMock()
mock_container.decision_service.return_value = mock_decision_svc
mock_container.correction_service.return_value = mock_correction
# Patch CorrectionService to return our mock when instantiated
with (
patch(
"cleveragents.application.services.correction_service.CorrectionService",
return_value=mock_correction,
),
patch(_PATCH_CONTAINER, return_value=mock_container),
):
with patch(_PATCH_CONTAINER, return_value=mock_container):
context.uncov_result = context.uncov_runner.invoke(plan_app, args)
@@ -27,9 +27,6 @@ from cleveragents.cli.commands.plan import app as plan_app
runner = CliRunner()
_PATCH_CONTAINER = "cleveragents.application.container.get_container"
_PATCH_CORRECTION_SVC = (
"cleveragents.application.services.correction_service.CorrectionService"
)
# Fixed ULIDs for deterministic assertions
_PLAN_ID = str(ULID())
@@ -186,10 +183,11 @@ def _invoke(
else:
args.append("--yes")
with (
patch(_PATCH_CORRECTION_SVC, return_value=context.pctw_correction_svc),
patch(_PATCH_CONTAINER, return_value=context.pctw_mock_container),
):
context.pctw_mock_container.correction_service.return_value = (
context.pctw_correction_svc
)
with patch(_PATCH_CONTAINER, return_value=context.pctw_mock_container):
context.pctw_result = runner.invoke(plan_app, args)
@@ -654,14 +654,9 @@ def _invoke_correct(
mock_decision_svc.get_influence_edges.return_value = {}
mock_container = MagicMock()
mock_container.decision_service.return_value = mock_decision_svc
mock_container.correction_service.return_value = context.pec_correction_svc
with (
patch(
"cleveragents.application.services.correction_service.CorrectionService",
return_value=context.pec_correction_svc,
),
patch(_PATCH_CONTAINER, return_value=mock_container),
):
with patch(_PATCH_CONTAINER, return_value=mock_container):
context.pec_result = runner.invoke(plan_app, args, input=input_text)
@@ -28,7 +28,6 @@ from typer.testing import CliRunner
from cleveragents.cli.commands.plan import app as plan_app
from features.mocks.tdd_plan_correct_plan_id_fixtures import (
PATCH_CONTAINER,
PATCH_CORRECTION_SVC,
PATCH_RESOLVE_PLAN,
PLAN_ID,
ROOT_DECISION_ID,
@@ -96,11 +95,10 @@ def step_tpcpid_invoke_with_plan_id(context: Context) -> None:
"""
args = build_cli_args(context.tpcpid_plan_id, mode="revert")
context.tpcpid_mock_container.correction_service.return_value = (
context.tpcpid_correction_svc
)
with (
patch(
PATCH_CORRECTION_SVC,
return_value=context.tpcpid_correction_svc,
),
patch(
PATCH_CONTAINER,
return_value=context.tpcpid_mock_container,
@@ -126,11 +124,10 @@ def step_tpcpid_invoke_with_plan_id_append(context: Context) -> None:
"""
args = build_cli_args(context.tpcpid_plan_id, mode="append")
context.tpcpid_mock_container.correction_service.return_value = (
context.tpcpid_correction_svc
)
with (
patch(
PATCH_CORRECTION_SVC,
return_value=context.tpcpid_correction_svc,
),
patch(
PATCH_CONTAINER,
return_value=context.tpcpid_mock_container,
@@ -1,23 +1,14 @@
@tdd_expected_fail @tdd_issue @tdd_issue_986 @mock_only
@tdd_issue @tdd_issue_986 @mock_only
Feature: TDD Issue #986 — CorrectionService missing checkpoint_service wiring in DI container
As a developer
I want to verify that CorrectionService receives checkpoint_service
from the DI container
So that revert-mode corrections can perform checkpoint rollback
Bug #986: The DI container registers CorrectionService with only
event_bus but omits checkpoint_service, despite CheckpointService
being registered in the same container. As a result, revert-mode
corrections silently skip checkpoint rollback because the dependency
is never injected.
Additionally, the CLI creates an ad-hoc CorrectionService instance
that bypasses the container entirely, which means even if the
container wiring is fixed, the CLI path remains broken.
These tests assert the expected wiring and will FAIL until the bug
is fixed. The @tdd_expected_fail tag inverts the result so CI
passes.
Bug #986 (fixed in #943): The DI container now registers
CorrectionService with both checkpoint_service and event_bus.
The CLI correct command also uses the container-provided service
instead of creating an ad-hoc instance.
Scenario: DI container wires checkpoint_service into CorrectionService
Given tccw a fresh DI container with an in-memory database
+9 -12
View File
@@ -105,6 +105,8 @@ def _test_dry_run_tree() -> None:
analyze_affected=[_ROOT_ID, _CHILD_A, _CHILD_B],
)
container.correction_service.return_value = correction_svc
args = [
"correct",
_ROOT_ID,
@@ -116,10 +118,7 @@ def _test_dry_run_tree() -> None:
_PLAN_ID,
"--dry-run",
]
with (
patch(_PATCH_CORRECTION_SVC, return_value=correction_svc),
patch(_PATCH_CONTAINER, return_value=container),
):
with patch(_PATCH_CONTAINER, return_value=container):
result = runner.invoke(plan_app, args)
assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}"
@@ -144,6 +143,8 @@ def _test_execute_tree() -> None:
execute_affected=[_ROOT_ID, _CHILD_A, _CHILD_B],
)
container.correction_service.return_value = correction_svc
args = [
"correct",
_ROOT_ID,
@@ -155,10 +156,7 @@ def _test_execute_tree() -> None:
_PLAN_ID,
"--yes",
]
with (
patch(_PATCH_CORRECTION_SVC, return_value=correction_svc),
patch(_PATCH_CONTAINER, return_value=container),
):
with patch(_PATCH_CONTAINER, return_value=container):
result = runner.invoke(plan_app, args)
assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}"
@@ -177,6 +175,8 @@ def _test_leaf_empty() -> None:
analyze_affected=[_LEAF_ID],
)
container.correction_service.return_value = correction_svc
args = [
"correct",
_LEAF_ID,
@@ -188,10 +188,7 @@ def _test_leaf_empty() -> None:
_PLAN_ID,
"--dry-run",
]
with (
patch(_PATCH_CORRECTION_SVC, return_value=correction_svc),
patch(_PATCH_CONTAINER, return_value=container),
):
with patch(_PATCH_CONTAINER, return_value=container):
result = runner.invoke(plan_app, args)
assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}"
+1 -2
View File
@@ -30,7 +30,6 @@ if _FEATURES not in sys.path:
from features.mocks.tdd_plan_correct_plan_id_fixtures import ( # noqa: E402
PATCH_CONTAINER,
PATCH_CORRECTION_SVC,
PATCH_RESOLVE_PLAN,
PLAN_ID,
ROOT_DECISION_ID,
@@ -63,11 +62,11 @@ def _run_plan_correct(mode: str, sentinel: str) -> None:
"""
mock_container = make_default_container()
correction_svc = make_correction_svc(ROOT_DECISION_ID, mode=mode)
mock_container.correction_service.return_value = correction_svc
args = build_cli_args(PLAN_ID, mode=mode)
with (
patch(PATCH_CORRECTION_SVC, return_value=correction_svc),
patch(PATCH_CONTAINER, return_value=mock_container),
patch(PATCH_RESOLVE_PLAN, return_value=PLAN_ID),
):
@@ -802,6 +802,7 @@ class Container(containers.DeclarativeContainer):
# are emitted in production (N9).
correction_service = providers.Singleton(
CorrectionService,
checkpoint_service=checkpoint_service,
event_bus=event_bus,
)
@@ -19,7 +19,9 @@ tests that do not require the lifecycle service.
from __future__ import annotations
import hashlib
import logging
import shutil
import subprocess
from datetime import UTC, datetime
from pathlib import Path
@@ -322,6 +324,247 @@ class CheckpointService:
"""
return self._get_checkpoint(checkpoint_id)
# ------------------------------------------------------------------
# Workspace snapshot (diff-based)
# ------------------------------------------------------------------
def create_workspace_snapshot(
self,
plan_id: str,
sandbox_ref: str,
decision_id: str,
*,
reason: str = "pre-decision snapshot",
phase: str = "execute",
) -> Checkpoint:
"""Create a diff-based workspace snapshot before decision execution.
Captures only files that differ from the previous checkpoint (or
the initial sandbox state when no prior checkpoints exist). The
diff manifest is stored in the checkpoint metadata ``extra`` dict
under the ``"diff_paths"`` key.
Args:
plan_id: Plan owning the workspace.
sandbox_ref: Git commit hash of the current HEAD.
decision_id: Decision about to execute.
reason: Human-readable snapshot reason.
phase: Plan phase (defaults to ``"execute"``).
Returns:
The newly created ``Checkpoint`` with diff metadata.
"""
# Compute diff paths against the previous checkpoint
diff_paths = self._compute_diff_snapshot(plan_id, sandbox_ref)
diff_size = sum(len(p.encode()) for p in diff_paths)
checkpoint = self.create_checkpoint(
plan_id=plan_id,
sandbox_ref=sandbox_ref,
reason=reason,
source_tool="workspace_snapshot",
phase=phase,
decision_id=decision_id,
checkpoint_type="pre_decision",
size_bytes=diff_size or 0,
)
# Store diff manifest in metadata extra
checkpoint.metadata.extra["diff_paths"] = diff_paths
checkpoint.metadata.extra["diff_based"] = True
checkpoint.metadata.extra["diff_hash"] = hashlib.sha256(
"|".join(sorted(diff_paths)).encode()
).hexdigest()
logger.info(
"checkpoint.workspace_snapshot_created",
extra={
"checkpoint_id": checkpoint.checkpoint_id,
"plan_id": plan_id,
"decision_id": decision_id,
"diff_paths_count": len(diff_paths),
},
)
return checkpoint
def selective_rollback(
self,
plan_id: str,
checkpoint_id: str,
) -> RollbackResult:
"""Roll back to a specific checkpoint with atomic semantics.
This method wraps :meth:`rollback_to_checkpoint` with
all-or-nothing transaction semantics. If the rollback fails
partway through, a best-effort recovery attempt restores the
sandbox to its pre-rollback HEAD.
Args:
plan_id: Plan owning the checkpoint.
checkpoint_id: Any checkpoint to roll back to (not just the
most recent).
Returns:
A ``RollbackResult`` on success.
Raises:
BusinessRuleViolation: If any rollback step fails and
recovery also fails.
ResourceNotFoundError: If checkpoint does not exist.
ValidationError: If checkpoint does not belong to the plan.
"""
# Capture current HEAD for recovery
sandbox_path = self._resolve_sandbox_path(plan_id)
self._validate_sandbox(sandbox_path)
try:
current_head = self._run_git(
["rev-parse", "HEAD"], cwd=sandbox_path
).stdout.strip()
except Exception:
current_head = None
try:
result = self.rollback_to_checkpoint(plan_id, checkpoint_id)
logger.info(
"checkpoint.selective_rollback_success",
extra={
"plan_id": plan_id,
"checkpoint_id": checkpoint_id,
"restored_files_count": result.restored_files_count,
},
)
return result
except Exception:
# Atomic rollback: attempt recovery to original HEAD
if current_head is not None:
try:
self._git_reset_hard(sandbox_path, current_head)
self._git_clean(sandbox_path)
logger.warning(
"checkpoint.selective_rollback_recovered",
extra={
"plan_id": plan_id,
"checkpoint_id": checkpoint_id,
"recovered_to": current_head,
},
)
except Exception:
logger.error(
"checkpoint.selective_rollback_recovery_failed",
extra={
"plan_id": plan_id,
"checkpoint_id": checkpoint_id,
},
exc_info=True,
)
raise
def archive_artifacts(
self,
sandbox_path: str,
artifact_paths: list[str],
archive_dir: str | None = None,
) -> list[str]:
"""Physically move artifact files to an archive directory.
Moves each file from ``sandbox_path / artifact`` to the archive
directory, preserving relative paths. Files that do not exist
are silently skipped.
Args:
sandbox_path: Filesystem path to the sandbox root.
artifact_paths: Relative paths of artifacts to archive.
archive_dir: Override archive directory. Defaults to
``sandbox_path / .cleveragents / archived_artifacts``.
Returns:
List of paths that were successfully archived.
"""
sandbox = Path(sandbox_path)
archive_root = (
Path(archive_dir)
if archive_dir
else sandbox / ".cleveragents" / "archived_artifacts"
)
archived: list[str] = []
for rel_path in artifact_paths:
source = sandbox / rel_path
if not source.exists():
continue
dest = archive_root / rel_path
dest.parent.mkdir(parents=True, exist_ok=True)
shutil.move(str(source), str(dest))
archived.append(rel_path)
if archived:
logger.info(
"checkpoint.artifacts_archived",
extra={
"sandbox_path": sandbox_path,
"archived_count": len(archived),
"archived_paths": archived,
},
)
return archived
# ------------------------------------------------------------------
# Diff computation
# ------------------------------------------------------------------
def _compute_diff_snapshot(
self,
plan_id: str,
current_ref: str,
) -> list[str]:
"""Compute changed file paths between the last checkpoint and current ref.
If no previous checkpoint exists, returns an empty list (the
first snapshot is the baseline).
Args:
plan_id: Plan to query checkpoints for.
current_ref: Current git ref (commit hash).
Returns:
List of relative paths that changed since the last checkpoint.
"""
existing = self.list_checkpoints(plan_id)
if not existing:
return []
previous_ref = existing[-1].sandbox_ref
if previous_ref == current_ref:
return []
# Try to resolve sandbox path for git diff
try:
sandbox_path = self._resolve_sandbox_path(plan_id)
except Exception:
# No sandbox available — return empty diff
return []
try:
result = self._run_git(
["diff", "--name-only", previous_ref, current_ref],
cwd=sandbox_path,
)
return [line for line in result.stdout.strip().splitlines() if line]
except Exception:
logger.warning(
"checkpoint.diff_computation_failed",
extra={
"plan_id": plan_id,
"previous_ref": previous_ref,
"current_ref": current_ref,
},
exc_info=True,
)
return []
def delete_checkpoint(self, checkpoint_id: str) -> None:
"""Delete a single checkpoint.
@@ -504,11 +504,17 @@ class CorrectionService:
# Revert always re-enters Strategize from the decision point.
phase_target = "strategize"
# --- Physical artifact archival ---
archived = self._archive_decision_artifacts(
request.plan_id,
impact.artifacts_to_archive,
)
result = CorrectionResult(
correction_id=correction_id,
status=CorrectionStatus.APPLIED,
reverted_decisions=impact.affected_decisions,
archived_artifacts=impact.artifacts_to_archive,
archived_artifacts=archived or impact.artifacts_to_archive,
checkpoint_restored=checkpoint_restored,
actor_state_ref=actor_state_ref,
user_intervention_decision_id=user_intervention_id,
@@ -669,6 +675,57 @@ class CorrectionService:
)
return self.execute_append(correction_id)
# ------------------------------------------------------------------
# Revert decisions (full rollback + artifact archival)
# ------------------------------------------------------------------
def revert_decisions(
self,
plan_id: str,
target_decision_id: str,
decision_tree: dict[str, list[str]] | None = None,
influence_edges: dict[str, list[str]] | None = None,
decisions: dict[str, Decision] | None = None,
guidance: str = "",
) -> CorrectionResult:
"""Revert decisions with checkpoint rollback and physical artifact archival.
This is the high-level entry point that combines:
1. Creating a correction request.
2. Computing impact analysis.
3. Invoking checkpoint rollback via ``CheckpointService``.
4. Physically archiving artifacts from reverted decisions.
5. Returning a complete ``CorrectionResult``.
The method is atomic: if checkpoint rollback fails, no artifacts
are archived and the correction is marked as FAILED.
Args:
plan_id: Plan owning the decision tree.
target_decision_id: Decision to revert from.
decision_tree: Structural tree adjacency list.
influence_edges: Influence DAG adjacency list.
decisions: Mapping of decision_id ``Decision`` objects.
guidance: Human-supplied correction guidance.
Returns:
``CorrectionResult`` with reverted decisions, archived
artifacts, and re-execution metadata.
"""
request = self.request_correction(
plan_id=plan_id,
target_decision_id=target_decision_id,
mode=CorrectionMode.REVERT,
guidance=guidance,
)
return self.execute_revert(
request.correction_id,
decision_tree=decision_tree,
influence_edges=influence_edges,
decisions=decisions,
)
# ------------------------------------------------------------------
# Query helpers
# ------------------------------------------------------------------
@@ -721,6 +778,40 @@ class CorrectionService:
# Internal helpers
# ------------------------------------------------------------------
def _archive_decision_artifacts(
self,
plan_id: str,
artifact_paths: list[str],
) -> list[str]:
"""Physically archive artifacts via the checkpoint service.
Delegates to ``CheckpointService.archive_artifacts`` when a
checkpoint service and sandbox path are available. Falls back
gracefully when the service is absent or the sandbox cannot be
resolved.
Args:
plan_id: Plan owning the artifacts.
artifact_paths: Relative paths to archive.
Returns:
List of successfully archived paths (may be empty).
"""
if self._checkpoint_service is None:
return []
try:
sandbox_path = self._checkpoint_service._resolve_sandbox_path(plan_id)
return self._checkpoint_service.archive_artifacts(
sandbox_path, artifact_paths
)
except Exception as exc:
logger.debug(
"correction.artifact_archival_skipped",
plan_id=plan_id,
reason=str(exc),
)
return []
# ------------------------------------------------------------------
# Revert re-execution helpers
# ------------------------------------------------------------------
+26 -9
View File
@@ -2886,9 +2886,6 @@ def correct_decision(
agents plan correct --mode revert -g "Use FastAPI instead" DEC-001
agents plan correct --mode append -g "Add caching layer" --dry-run DEC-002
"""
from cleveragents.application.services.correction_service import (
CorrectionService,
)
from cleveragents.core.exceptions import ResourceNotFoundError as RNF
from cleveragents.domain.models.core.correction import CorrectionMode
@@ -2954,7 +2951,7 @@ def correct_decision(
# Fetch influence DAG edges
influence_edges = decision_svc.get_influence_edges(resolved_plan_id)
svc = CorrectionService(event_bus=container.event_bus())
svc = container.correction_service()
# Create the correction request
request = svc.request_correction(
@@ -3187,9 +3184,16 @@ def rollback_plan(
typer.Argument(help="Plan ID to rollback"),
],
checkpoint_id: Annotated[
str,
typer.Argument(help="Checkpoint ID to restore"),
],
str | None,
typer.Argument(help="Checkpoint ID to restore (positional)"),
] = None,
to_checkpoint: Annotated[
str | None,
typer.Option(
"--to-checkpoint",
help="Checkpoint ID to restore (named option)",
),
] = None,
yes: Annotated[
bool,
typer.Option(
@@ -3213,8 +3217,12 @@ def rollback_plan(
the specified checkpoint. The plan must not be applied, and the
sandbox must still exist.
The checkpoint can be specified either as a positional argument or
via the ``--to-checkpoint`` named option.
Examples:
agents plan rollback --yes 01ARZ3NDEK... 01BRZ4NFEK...
agents plan rollback --to-checkpoint 01BRZ4NFEK... 01ARZ3NDEK...
"""
import time
@@ -3226,9 +3234,18 @@ def rollback_plan(
ResourceNotFoundError as RNF,
)
# Resolve checkpoint from positional arg or --to-checkpoint option
resolved_checkpoint_id = checkpoint_id or to_checkpoint
if not resolved_checkpoint_id:
console.print(
"[red]Error:[/red] checkpoint ID required. "
"Provide as positional argument or via --to-checkpoint."
)
raise typer.Abort()
if not yes:
confirm = typer.confirm(
f"\nRollback plan {plan_id} to checkpoint {checkpoint_id}?"
f"\nRollback plan {plan_id} to checkpoint {resolved_checkpoint_id}?"
)
if not confirm:
console.print("[yellow]Rollback cancelled.[/yellow]")
@@ -3238,7 +3255,7 @@ def rollback_plan(
container = get_container()
svc = container.checkpoint_service()
t0 = time.monotonic()
result = svc.rollback_to_checkpoint(plan_id, checkpoint_id)
result = svc.selective_rollback(plan_id, resolved_checkpoint_id)
elapsed = time.monotonic() - t0
# Spec-aligned output envelope (lines 15760-15797)
@@ -120,7 +120,7 @@ class Checkpoint(BaseModel):
@classmethod
def validate_checkpoint_type(cls: type[Checkpoint], v: str) -> str:
"""Ensure checkpoint_type is one of the allowed values."""
allowed = {"pre_write", "post_step", "manual"}
allowed = {"pre_write", "post_step", "manual", "pre_decision"}
if v not in allowed:
raise ValueError(
f"checkpoint_type must be one of {sorted(allowed)}, got '{v}'"
+6
View File
@@ -1197,3 +1197,9 @@ _make_runtime_handler # noqa: B018, F821
# LSP config fields — public API (#835)
LspTransport # noqa: B018, F821
# Checkpoint rollback wiring (#943) — public API for correction service
create_workspace_snapshot # noqa: B018, F821
selective_rollback # noqa: B018, F821
archive_artifacts # noqa: B018, F821
revert_decisions # noqa: B018, F821