Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: force preview for protected files regardless of background editing #6723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 122 additions & 0 deletions src/core/tools/__tests__/writeToFileTool.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ vi.mock("../../ignore/RooIgnoreController", () => ({
},
}))

vi.mock("../../protect/RooProtectedController", () => ({
RooProtectedController: class {
isWriteProtected() {
return false
}
},
}))

describe("writeToFileTool", () => {
// Test data
const testFilePath = "test/file.txt"
Expand Down Expand Up @@ -143,6 +151,9 @@ describe("writeToFileTool", () => {
mockCline.rooIgnoreController = {
validateAccess: vi.fn().mockReturnValue(true),
}
mockCline.rooProtectedController = {
isWriteProtected: vi.fn().mockReturnValue(false),
}
mockCline.diffViewProvider = {
editType: undefined,
isEditing: false,
Expand Down Expand Up @@ -416,4 +427,115 @@ describe("writeToFileTool", () => {
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
})
})

Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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.

describe("protected files with background editing", () => {
beforeEach(() => {
// Enable background editing experiment
mockCline.providerRef.deref().getState.mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: {
preventFocusDisruption: true,
},
})
})

it("shows diff view for protected files even when background editing is enabled", async () => {
// Mark file as protected
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(true)

await executeWriteFileTool({}, { fileExists: true })

// Should show diff view despite background editing being enabled
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, true)
expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled()
// Should NOT use saveDirectly
expect(mockCline.diffViewProvider.saveDirectly).toBeUndefined()
})

it("uses direct save for non-protected files when background editing is enabled", async () => {
// File is not protected
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(false)

// Add saveDirectly mock
mockCline.diffViewProvider.saveDirectly = vi.fn().mockResolvedValue(undefined)

await executeWriteFileTool({}, { fileExists: false })

// Should NOT show diff view
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled()
// Should use saveDirectly instead
expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith(
testFilePath,
testContent,
false,
true,
1000,
)
})

it("passes isProtected flag to askApproval for protected files", async () => {
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(true)

await executeWriteFileTool({})

// Check that askApproval was called with isWriteProtected = true
expect(mockAskApproval).toHaveBeenCalledWith(
"tool",
expect.any(String),
undefined,
true, // isWriteProtected
)
})

it("handles protected files correctly in partial blocks", async () => {
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(true)

// When background editing is enabled, partial blocks don't call ask
await executeWriteFileTool({}, { isPartial: true })

// With background editing enabled, partial blocks skip the UI update
expect(mockCline.ask).not.toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()

// Now test with background editing disabled
mockCline.providerRef.deref().getState.mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
experiments: {
preventFocusDisruption: false,
},
})

await executeWriteFileTool({}, { isPartial: true })

// With background editing disabled, partial blocks should call ask
expect(mockCline.ask).toHaveBeenCalled()
expect(mockCline.diffViewProvider.open).toHaveBeenCalled()
})

it("respects protection status for .vscode files", async () => {
const vscodePath = ".vscode/settings.json"
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(true)

await executeWriteFileTool({ path: vscodePath })

// Should show diff view for protected .vscode files
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(vscodePath)
expect(mockCline.diffViewProvider.update).toHaveBeenCalled()
})

it("respects protection status for .roo files", async () => {
const rooPath = ".roo/config.json"
mockCline.rooProtectedController.isWriteProtected.mockReturnValue(true)

await executeWriteFileTool({ path: rooPath })

// Should show diff view for protected .roo files
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(rooPath)
expect(mockCline.diffViewProvider.update).toHaveBeenCalled()
})
})
})
5 changes: 3 additions & 2 deletions src/core/tools/applyDiffTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ export async function applyDiffToolLegacy(
// Check if file is write-protected
const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false

if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view
// For protected files, always show diff view regardless of background editing setting
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {
// Direct file write without diff view (only for non-protected files)
const completeMessage = JSON.stringify({
...sharedMessageProps,
diff: diffContent,
Expand Down
10 changes: 5 additions & 5 deletions src/core/tools/insertContentTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ export async function insertContentTool(
isProtected: isWriteProtected,
} satisfies ClineSayTool)

// Show diff view if focus disruption prevention is disabled
if (!isPreventFocusDisruptionEnabled) {
// Show diff view if focus disruption prevention is disabled OR if file is protected
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.open(relPath)
await cline.diffViewProvider.update(updatedContent, true)
cline.diffViewProvider.scrollToFirstDiff()
Expand All @@ -159,7 +159,7 @@ export async function insertContentTool(

if (!didApprove) {
// Revert changes if diff view was shown
if (!isPreventFocusDisruptionEnabled) {
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.revertChanges()
}
pushToolResult("Changes were rejected by the user.")
Expand All @@ -168,8 +168,8 @@ export async function insertContentTool(
}

// Save the changes
if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view or opening the file
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {
// Direct file write without diff view or opening the file (only for non-protected files)
await cline.diffViewProvider.saveDirectly(relPath, updatedContent, false, diagnosticsEnabled, writeDelayMs)
} else {
// Call saveChanges to update the DiffViewProvider properties
Expand Down
23 changes: 14 additions & 9 deletions src/core/tools/multiApplyDiffTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,8 +517,10 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION,
)

// For batch operations, we've already gotten approval
// Check if file is write-protected
const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false

// For batch operations, we've already gotten approval
const sharedMessageProps: ClineSayTool = {
tool: "appliedDiff",
path: getReadablePath(cline.cwd, relPath),
Expand Down Expand Up @@ -549,8 +551,8 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
// Set up diff view
cline.diffViewProvider.editType = "modify"

// Show diff view if focus disruption prevention is disabled
if (!isPreventFocusDisruptionEnabled) {
// Show diff view if focus disruption prevention is disabled OR if file is protected
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.open(relPath)
await cline.diffViewProvider.update(originalContent!, true)
cline.diffViewProvider.scrollToFirstDiff()
Expand All @@ -560,21 +562,20 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
}

// Ask for approval (same for both flows)
const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false
didApprove = await askApproval("tool", operationMessage, toolProgressStatus, isWriteProtected)

if (!didApprove) {
// Revert changes if diff view was shown
if (!isPreventFocusDisruptionEnabled) {
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.revertChanges()
}
results.push(`Changes to ${relPath} were not approved by user`)
continue
}

// Save the changes
if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view or opening the file
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {
// Direct file write without diff view or opening the file (only for non-protected files)
await cline.diffViewProvider.saveDirectly(
relPath,
originalContent!,
Expand All @@ -588,8 +589,12 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
}
} else {
// Batch operations - already approved above
if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view or opening the file
// Check if any files in the batch are protected
const hasProtectedFiles = operationsToApprove.some(
(op) => cline.rooProtectedController?.isWriteProtected(op.path) || false,
)
if (isPreventFocusDisruptionEnabled && !hasProtectedFiles) {
Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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:

Suggested change
if (isPreventFocusDisruptionEnabled && !hasProtectedFiles) {
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {

// Direct file write without diff view or opening the file (only if no protected files)
cline.diffViewProvider.editType = "modify"
cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8")
await cline.diffViewProvider.saveDirectly(
Expand Down
10 changes: 5 additions & 5 deletions src/core/tools/searchAndReplaceTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ export async function searchAndReplaceTool(
isProtected: isWriteProtected,
} satisfies ClineSayTool)

// Show diff view if focus disruption prevention is disabled
if (!isPreventFocusDisruptionEnabled) {
// Show diff view if focus disruption prevention is disabled OR if file is protected
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.open(validRelPath)
await cline.diffViewProvider.update(newContent, true)
cline.diffViewProvider.scrollToFirstDiff()
Expand All @@ -229,7 +229,7 @@ export async function searchAndReplaceTool(

if (!didApprove) {
// Revert changes if diff view was shown
if (!isPreventFocusDisruptionEnabled) {
if (!isPreventFocusDisruptionEnabled || isWriteProtected) {
await cline.diffViewProvider.revertChanges()
}
pushToolResult("Changes were rejected by the user.")
Expand All @@ -238,8 +238,8 @@ export async function searchAndReplaceTool(
}

// Save the changes
if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view or opening the file
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {
// Direct file write without diff view or opening the file (only for non-protected files)
await cline.diffViewProvider.saveDirectly(validRelPath, newContent, false, diagnosticsEnabled, writeDelayMs)
} else {
// Call saveChanges to update the DiffViewProvider properties
Expand Down
5 changes: 3 additions & 2 deletions src/core/tools/writeToFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,9 @@ export async function writeToFileTool(
EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION,
)

if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view
// For protected files, always show diff view regardless of background editing setting
if (isPreventFocusDisruptionEnabled && !isWriteProtected) {
Copy link
Contributor Author

@roomote roomote bot Aug 5, 2025

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.

// Direct file write without diff view (only for non-protected files)
// Check for code omissions before proceeding
if (detectCodeOmission(cline.diffViewProvider.originalContent || "", newContent, predictedLineCount)) {
if (cline.diffStrategy) {
Expand Down
Loading