fix(checkpoints): remove files added after restore target - #64477
fix(checkpoints): remove files added after restore target#64477visualfox-ch wants to merge 1 commit into
Conversation
Compare the pre-rollback snapshot to the selected checkpoint and delete only additions captured by Hermes, while preserving ignored and excluded files. Assisted-by: Hermes:gpt-5.6-sol
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fix for checkpoint restoration removing files added after the restore target. Clean (95 additions, 0 deletions). No security concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the full-directory restore gap; current main still uses git checkout <commit> -- . in tools/checkpoint_manager.py:826-830, so post-checkpoint additions are indeed left behind.
Problems
- The new reconciliation runs after the existing pre-rollback
_take()attools/checkpoint_manager.py:820._take()can crossmax_snapshots, then_prune()rewrites history and runsgit gc --prune=now(tools/checkpoint_manager.py:1053-1118). If the requested target is the oldest retained checkpoint, it can become unreachable before the new diff/checkout code uses it. The added tests do not exercise that retention boundary.
Suggested changes
- Resolve and pin the target tree before
_take(), use the temporary ref for restore, and delete it infinally. - Add small-retention regression coverage for full and file-scoped restores. The timeline-linked #64966 independently covers this target-pinning case.
Automated hermes-sweeper review.
| if ok_current and current_commit and current_commit != commit_hash: | ||
| ok_added, added_out, _ = _run_git( | ||
| [ | ||
| "diff", "--name-only", "--diff-filter=A", "-z", |
There was a problem hiding this comment.
This diff runs after the existing pre-rollback _take() call. _take() can exceed max_snapshots, after which _prune() rewrites the ref and runs gc --prune=now; an oldest retained commit_hash can be unreachable here. Resolve and pin the target tree before _take(), then diff/restore through that temporary ref and delete it in finally.
Summary
git cleanTest plan
python -m pytest tests/tools/test_checkpoint_manager.py tests/test_batch_runner_checkpoint.py tests/integration/test_checkpoint_resumption.py -q -o 'addopts='ruff check tools/checkpoint_manager.py tests/tools/test_checkpoint_manager.pySafety
The restore path never recursively cleans the repository. It only removes paths recorded as additions between the target checkpoint and Hermes' pre-rollback snapshot.