feat(correction): wire checkpoint rollback into correction service revert flow #1199

2026-03-29T08:38:49Z

freemo commented

2026-03-29 08:38:49 +00:00

Summary

Wires checkpoint rollback into the correction service revert flow, enabling automatic workspace snapshots, physical artifact archival, and selective rollback to any checkpoint.

Changes

src/cleveragents/application/services/checkpoint_service.py: Added create_workspace_snapshot(), selective_rollback(), archive_artifacts(), _compute_diff_snapshot() for diff-based storage
src/cleveragents/application/services/correction_service.py: Added revert_decisions() with checkpoint rollback invocation, _archive_decision_artifacts() for physical archival, atomic semantics in execute_revert()
src/cleveragents/application/container.py: Wired checkpoint_service into CorrectionService (also fixes #986)
src/cleveragents/cli/commands/plan.py: Added --to-checkpoint option to rollback; correct uses container-provided service
src/cleveragents/domain/models/core/checkpoint.py: Added pre_decision to checkpoint_type
features/correction_checkpoint_rollback.feature: 12 BDD scenarios
features/tdd_correction_checkpoint_wiring.feature: Removed @tdd_expected_fail (bug #986 fixed)

Bonus Fix

This PR also fixes bug #986 (CorrectionService missing checkpoint_service wiring) by properly wiring checkpoint_service in the DI container and removing the @tdd_expected_fail tag from its TDD test.

Closes #943

## Summary Wires checkpoint rollback into the correction service revert flow, enabling automatic workspace snapshots, physical artifact archival, and selective rollback to any checkpoint. ## Changes - `src/cleveragents/application/services/checkpoint_service.py`: Added `create_workspace_snapshot()`, `selective_rollback()`, `archive_artifacts()`, `_compute_diff_snapshot()` for diff-based storage - `src/cleveragents/application/services/correction_service.py`: Added `revert_decisions()` with checkpoint rollback invocation, `_archive_decision_artifacts()` for physical archival, atomic semantics in `execute_revert()` - `src/cleveragents/application/container.py`: Wired `checkpoint_service` into `CorrectionService` (also fixes #986) - `src/cleveragents/cli/commands/plan.py`: Added `--to-checkpoint` option to `rollback`; `correct` uses container-provided service - `src/cleveragents/domain/models/core/checkpoint.py`: Added `pre_decision` to checkpoint_type - `features/correction_checkpoint_rollback.feature`: 12 BDD scenarios - `features/tdd_correction_checkpoint_wiring.feature`: Removed `@tdd_expected_fail` (bug #986 fixed) ## Bonus Fix This PR also fixes bug #986 (CorrectionService missing checkpoint_service wiring) by properly wiring `checkpoint_service` in the DI container and removing the `@tdd_expected_fail` tag from its TDD test. Closes #943

freemo added this to the v3.5.0 milestone 2026-03-29 08:38:55 +00:00

freemo added the

Type

Feature

label 2026-03-29 08:39:00 +00:00

freemo referenced this pull request

2026-03-29 08:39:14 +00:00

feat(correction): wire checkpoint rollback into correction service revert flow #943

freemo force-pushed feature/correction-checkpoint-rollback from 7467fa304c to 643760004c

2026-03-30 00:10:39 +00:00

Compare

freemo reviewed 2026-03-30 04:20:54 +00:00

freemo left a comment

Review: Looks Good (self-authored — posted as comment)

Clean DI fix wiring checkpoint rollback into the correction service.

Notes

Private method access: self._checkpoint_service._resolve_sandbox_path(plan_id) accesses a private method across service boundaries. Consider making _resolve_sandbox_path() public on CheckpointService.
Good: CLI plan correct now properly resolves CorrectionService from the container — significant DI hygiene improvement.
Good: Proper migration of test assertions from rollback_to_checkpoint to selective_rollback.
Good: Vulture whitelist updated.

## Review: Looks Good (self-authored — posted as comment) Clean DI fix wiring checkpoint rollback into the correction service. ### Notes 1. **Private method access**: `self._checkpoint_service._resolve_sandbox_path(plan_id)` accesses a private method across service boundaries. Consider making `_resolve_sandbox_path()` public on `CheckpointService`. 2. **Good**: CLI `plan correct` now properly resolves `CorrectionService` from the container — significant DI hygiene improvement. 3. **Good**: Proper migration of test assertions from `rollback_to_checkpoint` to `selective_rollback`. 4. **Good**: Vulture whitelist updated.

freemo reviewed 2026-03-30 04:49:49 +00:00

freemo left a comment

Updated Review (Deep Pass): Changes Required

My initial review flagged only the private method access. The deep review reveals more issues.

New Finding: Missing Robot Framework Integration Test

No robot/correction_checkpoint_rollback.robot exists. The PR has BDD tests but no Robot integration test. Per CONTRIBUTING.md §Multi-Level Testing Mandate, integration tests are required.

New Finding: Missing argument validation on new public methods

create_workspace_snapshot(): plan_id, sandbox_ref, decision_id not validated for empty/null
selective_rollback(): plan_id, checkpoint_id not validated
revert_decisions(): plan_id, target_decision_id not validated
Per CONTRIBUTING.md §Argument Validation: "All public methods must validate arguments."

New Finding: Multiple silent exception catches

selective_rollback() line ~407: HEAD capture catches Exception with no logging — sets current_head = None
_compute_diff_snapshot() line ~518: catches Exception, returns empty list silently
_archive_decision_artifacts() line ~804: catches Exception, logs at debug level, returns empty list

Previous finding confirmed: `_checkpoint_service._resolve_sandbox_path()` accesses private method across class boundary.

## Updated Review (Deep Pass): Changes Required My initial review flagged only the private method access. The deep review reveals more issues. ### New Finding: Missing Robot Framework Integration Test No `robot/correction_checkpoint_rollback.robot` exists. The PR has BDD tests but no Robot integration test. Per CONTRIBUTING.md §Multi-Level Testing Mandate, integration tests are required. ### New Finding: Missing argument validation on new public methods - `create_workspace_snapshot()`: `plan_id`, `sandbox_ref`, `decision_id` not validated for empty/null - `selective_rollback()`: `plan_id`, `checkpoint_id` not validated - `revert_decisions()`: `plan_id`, `target_decision_id` not validated Per CONTRIBUTING.md §Argument Validation: "All public methods must validate arguments." ### New Finding: Multiple silent exception catches - `selective_rollback()` line ~407: HEAD capture catches `Exception` with no logging — sets `current_head = None` - `_compute_diff_snapshot()` line ~518: catches `Exception`, returns empty list silently - `_archive_decision_artifacts()` line ~804: catches `Exception`, logs at debug level, returns empty list ### Previous finding confirmed: `_checkpoint_service._resolve_sandbox_path()` accesses private method across class boundary.

freemo requested review from CoreRasurae 2026-03-30 05:14:03 +00:00

freemo requested review from brent.edwards 2026-03-30 05:14:04 +00:00

freemo force-pushed feature/correction-checkpoint-rollback from b0a62ed37c to 38e05ac45a

2026-03-30 21:07:40 +00:00

Compare

freemo scheduled this pull request to auto merge when all checks succeed 2026-03-30 21:08:09 +00:00

freemo merged commit 38e05ac45a into master

2026-03-30 21:29:28 +00:00

freemo deleted branch feature/correction-checkpoint-rollback

2026-03-30 21:29:28 +00:00

freemo referenced this issue from a commit

2026-04-02 16:56:53 +00:00