-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: enhance checkpoint frequency - add checkpoints before file edits #6803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Save checkpoints AFTER file edits instead of before - Add checkpoint before processing new user prompts - Fixes regression where checkpoints were only saved before file edits Fixes #6800
…uency - Save checkpoints BEFORE file editing operations (write_to_file, apply_diff, insert_content, search_and_replace) - Maintain existing checkpoints AFTER file edits - Update checkpointSaveAndMark function to handle before/after timing - Add comprehensive tests for new checkpoint behavior This addresses user feedback requesting checkpoints before edits to allow reverting to the state just before making changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and found it doubles the checkpoint frequency. The math checks out.
| */ | ||
| async function checkpointSaveAndMark(task: Task) { | ||
| if (task.currentStreamingDidCheckpoint) { | ||
| async function checkpointSaveAndMark(task: Task, timing?: "before" | "after") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice implementation of the dual checkpoint strategy! The optional timing parameter is a clean way to distinguish between "before" and "after" checkpoints while maintaining backward compatibility.
One suggestion for future improvement: Consider adding a brief comment here explaining the checkpoint strategy (before for recovery, after for progression) to help future maintainers understand the design decision.
| // Note: The apply_diff test is omitted here because it requires complex mocking | ||
| // of the experiment flags and tool selection logic. The implementation is tested | ||
| // through the other file editing tools (write_to_file, insert_content, search_and_replace) | ||
| // which all follow the same pattern of saving checkpoints after file edits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent test coverage! The tests thoroughly verify both before and after checkpoints, including error handling scenarios.
Minor note: I see the comment about omitting the apply_diff test due to complexity. Would it be worth adding a TODO comment here for potentially adding this test in the future when the experiment flag handling is simplified?
| // This allows users to revert to the state right before a new interaction | ||
| if (this.enableCheckpoints && userContent.length > 0) { | ||
| try { | ||
| await this.checkpointSave(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition of checkpoint before processing new user prompts. This provides users with a recovery point if a new interaction turns out to be problematic.
Future consideration: Making checkpoint timing configurable could be beneficial for users who prioritize performance over granular recovery options.
|
This is already the case on main, closing |
This PR enhances the checkpoint system by adding checkpoints BEFORE file editing operations, in addition to the existing checkpoints AFTER edits.
Changes Made
1. Added Checkpoints Before File Edits
write_to_file,apply_diff,insert_content, andsearch_and_replaceoperations2. Enhanced
checkpointSaveAndMarkFunctiontimingparameter to distinguish between "before" and "after" checkpointscurrentStreamingDidCheckpointstatecurrentStreamingDidCheckpointto true3. Comprehensive Test Coverage
Benefits
This enhancement addresses user feedback from issue #6800 by providing:
Related Issues
Testing
cc @hujianxin @mrubens
Important
Enhances checkpoint system by adding pre-edit checkpoints and updating
checkpointSaveAndMarkfunction, with comprehensive test coverage.write_to_file,apply_diff,insert_content, andsearch_and_replaceoperations inpresentAssistantMessage()inpresentAssistantMessage.ts.checkpointSaveAndMark()to include atimingparameter to distinguish "before" and "after" checkpoints.currentStreamingDidCheckpointstate.currentStreamingDidCheckpointto true.checkpoint-timing.spec.ts.recursivelyMakeClineRequests()inTask.ts.This description was created by
for 9789925. You can customize this summary. It will automatically update as commits are pushed.