-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: force preview for protected files regardless of background editing #6723
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
- Modified writeToFileTool to always show diff view for protected files - Modified applyDiffTool to always show diff view for protected files - Modified insertContentTool to always show diff view for protected files - Modified searchAndReplaceTool to always show diff view for protected files - Modified multiApplyDiffTool to handle protected files correctly - Added comprehensive tests for the new behavior Fixes #6722
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.
Reviewing my own code is like debugging in a mirror - everything looks backward but the bugs are still mine.
| const hasProtectedFiles = operationsToApprove.some( | ||
| (op) => cline.rooProtectedController?.isWriteProtected(op.path) || false, | ||
| ) | ||
| if (isPreventFocusDisruptionEnabled && !hasProtectedFiles) { |
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.
Is this intentional? The condition for checking protected files in batch operations uses a different pattern than the other tools. Consider refactoring to match the pattern used elsewhere for consistency:
| if (isPreventFocusDisruptionEnabled && !hasProtectedFiles) { | |
| if (isPreventFocusDisruptionEnabled && !isWriteProtected) { |
| if (isPreventFocusDisruptionEnabled) { | ||
| // Direct file write without diff view | ||
| // For protected files, always show diff view regardless of background editing setting | ||
| if (isPreventFocusDisruptionEnabled && !isWriteProtected) { |
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.
The comment 'only for non-protected files' appears multiple times across the codebase. Could we improve readability by extracting this logic into a well-named variable like shouldUseDirectSave? This would make the intent clearer and reduce repetition.
| expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
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.
This test suite is quite comprehensive! However, it might benefit from being split into smaller, more focused describe blocks. For example, you could have separate blocks for 'direct save behavior', 'diff view behavior', and 'protection flag handling'. Also, consider adding a test case for when rooProtectedController is undefined/null to ensure the fallback to false works correctly.
|
I feel like this defeats the purpose of background edits, background edits is an experimental feature that depends on another pending feature to be complete. I'll close this for now but feel free to continue the discussion. |
Summary
This PR fixes an issue where protected files (like
.vscode/,.roo/, etc.) could be edited without preview when the "Background editing" experimental feature is enabled. This was a security concern as these configuration files should always require explicit approval before modification.Problem
When the
PREVENT_FOCUS_DISRUPTIONexperiment (Background editing) is enabled, the file editing tools would bypass the diff view and directly save changes to files. This behavior was applied to all files, including protected configuration files, which should always require preview and explicit approval.Solution
Modified all file editing tools to check if a file is write-protected and force the diff view regardless of the background editing setting:
writeToFileTool: Added condition to check!isWriteProtectedbefore using direct saveapplyDiffTool: Same fix appliedinsertContentTool: Modified to show diff view when file is protectedsearchAndReplaceTool: Modified to show diff view when file is protectedmultiApplyDiffTool: Added protection checks for both single and batch operationsTesting
Added comprehensive test coverage in
writeToFileTool.spec.tsto verify:isProtectedflag is correctly passed to approval dialogs.vscode/,.roo/, etc.)All existing tests pass, confirming no regression in functionality.
Related Issue
Fixes #6722
Important
This PR ensures protected files always require a diff view for approval, even with background editing enabled, by modifying several file editing tools.
writeToFileTool,applyDiffTool,insertContentTool,searchAndReplaceTool, andmultiApplyDiffToolregardless of background editing setting.writeToFileTool.spec.tsto ensure protected files always show diff view..vscode/and.roo/files.isProtectedflag is passed to approval dialogs.Background editingis enabled #6722.This description was created by
for fb30e4e. You can customize this summary. It will automatically update as commits are pushed.