fix: improve checkpoint timing logic to address regression #6801
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
dosubot
bot
added
size:XL
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| await checkpointSaveAndMark(cline) | ||
| await writeToFileTool(cline, block, askApproval, handleError, pushToolResult, removeClosingTag) | ||
| // Save checkpoint AFTER file edit | ||
| await checkpointSaveAndMark(cline) |
There was a problem hiding this comment.
Good fix! Moving the checkpoint save to AFTER the file edit operations ensures we capture the successful state rather than the pre-edit state. This directly addresses the user's first concern about reverting to the state immediately after a successful edit.
| // 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.
Smart placement of the checkpoint save here. By saving before processing new user prompts (when userContent.length > 0), users can now roll back invalid interactions. The error handling with try-catch ensures the task continues even if checkpoint saving fails.
|
|
||
| // 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) |
There was a problem hiding this comment.
Is it intentional to skip the apply_diff test? While I understand the complexity with experiment flags, having at least a basic test for apply_diff checkpoint timing would improve coverage. Could we add a simplified test that doesn't require the full experiment setup?
hannesrudolph
added
the
Issue/PR - Triage
|
Closing for now, issue was closed |
This PR fixes the checkpoint logic regression reported in #6800.
Problem
The checkpoint system was only creating checkpoints before file edits, which made it difficult to:
Solution
write_to_file,apply_diff,insert_content,search_and_replace)recursivelyMakeClineRequestsChanges
src/core/assistant-message/presentAssistantMessage.tsto save checkpoints after file editssrc/core/task/Task.tsto save checkpoints before processing new user promptssrc/core/checkpoints/__tests__/checkpoint-timing.spec.tsTesting
Fixes #6800
Important
Fixes checkpoint logic regression by saving checkpoints after file edits and before new prompts, with tests added for verification.
presentAssistantMessage.tsforwrite_to_file,apply_diff,insert_content,search_and_replace.recursivelyMakeClineRequestsinTask.ts.checkpoint-timing.spec.tsto verify checkpoint saving after file edits and before new prompts.This description was created by
for 67e6a22. You can customize this summary. It will automatically update as commits are pushed.