Skip to content

Conversation

@roomote
Copy link
Collaborator

@roomote roomote commented Jun 18, 2025

Summary

This PR fixes issue #4827 by modifying the checkpoint creation timing to occur before file changes are made instead of after. This allows users to revert to the state before changes were applied, which is the expected and more useful behavior.

Changes Made

Core Fix

  • Modified all file editing tools to create checkpoints BEFORE applying changes:
    • (both batch and single file cases)
    • (legacy implementation)

Implementation Details

  • Checkpoints are now created after user approval but before file modifications
  • For batch operations in apply_diff, a single checkpoint is created before processing all approved files
  • Added fallback checkpoint creation in for edge cases
  • All checkpoint creation is conditional on cline.enableCheckpoints being true

Testing

  • ✅ All existing checkpoint tests continue to pass (34/34 tests passing)
  • ✅ No breaking changes to existing functionality
  • ✅ Linting and type checking pass

Impact

  • Before: Checkpoints captured the state AFTER changes, making them less useful for reverting
  • After: Checkpoints capture the state BEFORE changes, allowing proper reversion to pre-change state
  • Users can now effectively use checkpoints to undo file modifications

Fixes #4827


Important

Checkpoints are now created before file modifications in various tools, allowing users to revert to the pre-change state, with fallback mechanisms for edge cases.

  • Behavior:
    • Checkpoints are now created before file modifications in writeToFileTool, applyDiffTool, insertContentTool, and searchAndReplaceTool.
    • For batch operations in multiApplyDiffTool, a single checkpoint is created before processing all approved files.
    • Added fallback checkpoint creation in presentAssistantMessage.ts for edge cases.
  • Implementation:
    • Checkpoints are created after user approval but before file modifications.
    • All checkpoint creation is conditional on cline.enableCheckpoints being true.
  • Testing:
    • All existing checkpoint tests continue to pass (34/34 tests passing).

This description was created by Ellipsis for c7a1651. You can customize this summary. It will automatically update as commits are pushed.

- Modified all file editing tools (write_to_file, apply_diff, insert_content, search_and_replace) to create checkpoints BEFORE applying changes instead of after
- This allows users to revert to the state before changes were made, which is the expected behavior
- Updated both legacy and multi-file apply_diff implementations
- Added fallback checkpoint creation in presentAssistantMessage for edge cases
- All existing checkpoint tests continue to pass
@roomote roomote requested review from cte, jr and mrubens as code owners June 18, 2025 15:29
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jun 18, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 18, 2025
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cte does this PR look right to you?

@cte
Copy link
Collaborator

cte commented Jun 18, 2025

@roomote I would expect this PR to also remove the existing logic that saves checkpoints after changes have been made. Can you find where we're doing that and see if we need to make any changes there? Also, I think the pre-existing logic only needs to call saveCheckpoint in a single place, whereas this PR seems to call it in a bunch of places; is it possible to hook in at a single place (prior to changes being made)?

Addresses @cte's feedback in PR #4840 comment:
- Remove existing logic that saves checkpoints AFTER changes in presentAssistantMessage.ts
- Consolidate checkpoint creation to askApproval function for file-editing tools
- Remove individual checkpoint calls from writeToFileTool, applyDiffTool, insertContentTool, and searchAndReplaceTool
- Handle special case for multiApplyDiffTool batch operations
- Maintains fix for #4827: checkpoints are created BEFORE file modifications

This reduces code duplication and centralizes checkpoint logic in a single place.
@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Jun 18, 2025

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

@cte
Copy link
Collaborator

cte commented Jun 18, 2025

@roomote Please remove the unnecessary comments in this PR.

Addresses comment by @cte in PR #4840 to remove redundant inline comments
that reference the issue number, as this information is already captured
in the commit history and PR description.
@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Jun 18, 2025

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 18, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 18, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented Jun 19, 2025

A few suggestions:

  1. Consolidate checkpoint logic in multiApplyDiffTool - The checkpoint creation is duplicated in multiple places (lines 306, 350, 379). Since we now have centralized checkpoint creation in askApproval, consider refactoring multiApplyDiffTool to use the standard approval flow.

  2. Optimize isFileEditingTool check - This check runs for all tools, not just file-editing ones. Minor inefficiency but worth addressing.

  3. Extract file-editing tools list - The array ["write_to_file", "apply_diff", "insert_content", "search_and_replace"] could be a reusable constant.

Also something to keep in mind, if creating more checkpoints is making the file edit tools take longer we might want to update the timeout on the e2e tests.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Jun 19, 2025
@KJ7LNW
Copy link
Contributor

KJ7LNW commented Jun 30, 2025

+1

please make sure the user interface shows the rollback icon in the correct location.

it would be nice if you show a dotted horizontal-rule as the "line in the sand" for where it gets reverted for extra clarity because exactly where it reverts based on the icon is somewhat hard to figure out

only show the horizontal rule if the mouse hovers

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to Triage in Roo Code Roadmap Jul 7, 2025
@hannesrudolph hannesrudolph added Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. and removed PR - Changes Requested labels Jul 7, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 7, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Jul 7, 2025
@cte cte deleted the fix-4827 branch July 31, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Checkpoints should be created before changes, not after

7 participants