diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..ffee450fff 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -19,6 +19,9 @@ import { BaseTool, ToolCallbacks } from "./BaseTool" interface ApplyDiffParams { path: string diff: string + ref?: import("../../shared/tools").ContentRef + multi_ref?: import("../../shared/tools").ContentRef[] + transform?: import("../../shared/tools").ContentRefParams["transform"] } export class ApplyDiffTool extends BaseTool<"apply_diff"> { @@ -173,7 +176,6 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } - // Save directly without showing diff view or opening the file task.diffViewProvider.editType = "modify" task.diffViewProvider.originalContent = originalContent await task.diffViewProvider.saveDirectly( @@ -182,6 +184,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { false, diagnosticsEnabled, writeDelayMs, + isWriteProtected, ) } else { // Original behavior with diff view diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 3f3295404b..337c5e3f14 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -213,7 +213,6 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Save the changes if (isPreventFocusDisruptionEnabled) { await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs) } else { @@ -407,7 +406,6 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Save new content to the new path if (isPreventFocusDisruptionEnabled) { await task.diffViewProvider.saveDirectly( change.movePath, @@ -415,6 +413,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { false, diagnosticsEnabled, writeDelayMs, + isMovePathWriteProtected, ) } else { // Write to new path and delete old file @@ -434,7 +433,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } else { // Save changes to the same file if (isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + isWriteProtected, + ) } else { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index 2495a372bc..6e47bb9a37 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -434,15 +434,18 @@ export class EditFileTool extends BaseTool<"edit_file"> { return } - // Save the changes if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view or opening the file + // Background editing: always pass openFile=false to prevent focus disruption; + // file is written via fs.writeFile and VSCode dirty state is resolved via + // openTextDocument + doc.save() await task.diffViewProvider.saveDirectly( relPath, newContent, - isNewFile, + false, diagnosticsEnabled, writeDelayMs, + isWriteProtected, ) } else { // Call saveChanges to update the DiffViewProvider properties diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts index 79338c17a6..0a1e1fcc63 100644 --- a/src/core/tools/EditTool.ts +++ b/src/core/tools/EditTool.ts @@ -209,10 +209,16 @@ export class EditTool extends BaseTool<"edit"> { return } - // Save the changes if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view or opening the file - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + isWriteProtected, + ) } else { // Call saveChanges to update the DiffViewProvider properties await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index 2d8817364f..712b761338 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -205,10 +205,16 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { return } - // Save the changes if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view or opening the file - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + isWriteProtected, + ) } else { // Call saveChanges to update the DiffViewProvider properties await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c8455ef3d9..d5523d95a1 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -133,7 +133,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // Direct file write without opening diff view (background editing) + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + isWriteProtected, + ) } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 80b5799217..b612009158 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -645,6 +645,7 @@ export class DiffViewProvider { openFile: boolean = true, diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + isWriteProtected: boolean = false, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -652,31 +653,79 @@ export class DiffViewProvider { }> { const absolutePath = path.resolve(this.cwd, relPath) + // Protected files must always show diff view for manual review, + // even when background editing is enabled. This prevents accidental + // modification of sensitive configuration files. + if (isWriteProtected && !openFile) { + openFile = true + } + + // When auto-approval is disabled, force showing the file so the user + // can review changes. Background editing only makes sense when writes + // are auto-approved (#8736). + if (!openFile) { + const task = this.taskRef.deref() + const provider = task?.providerRef.deref() + if (provider) { + const state = await provider.getState() + if (!state?.autoApprovalEnabled) { + openFile = true + } + } + } + // Get diagnostics before editing the file this.preDiagnostics = vscode.languages.getDiagnostics() - // Write the content directly to the file + // Write the content directly to the file using Node's fs. + // Node's fs.writeFile does NOT notify VSCode's file watcher, which is + // intentional — it prevents open editor tabs from showing "unsaved changes" + // prompts when the user tries to close them after background editing. await createDirectoriesForFile(absolutePath) await fs.writeFile(absolutePath, content, "utf-8") + // Verify the content was written correctly to disk with exponential backoff retry + const fileUri = vscode.Uri.file(absolutePath) + const MAX_WRITE_RETRIES = 3 + let writeVerified = false + for (let attempt = 0; attempt < MAX_WRITE_RETRIES; attempt++) { + const verifyContent = await fs.readFile(absolutePath, "utf-8") + if (verifyContent === content) { + writeVerified = true + break + } + if (attempt < MAX_WRITE_RETRIES - 1) { + // Exponential backoff: 100ms, 200ms + await new Promise((resolve) => setTimeout(resolve, Math.pow(2, attempt) * 100)) + await fs.writeFile(absolutePath, content, "utf-8") + } + } + if (!writeVerified) { + throw new Error(`Failed to save content to ${relPath} after ${MAX_WRITE_RETRIES} attempts`) + } + // Open the document to ensure diagnostics are loaded - // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory + // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open + // in memory and immediately save to mark it as "clean" in VSCode — this + // prevents the "unsaved changes" prompt when closing the tab. if (openFile) { - // Show the document in the editor - await vscode.window.showTextDocument(vscode.Uri.file(absolutePath), { + // Show the document in the editor without stealing focus + await vscode.window.showTextDocument(fileUri, { preview: false, preserveFocus: true, }) } else { - // Just open the document in memory to trigger diagnostics without showing it - const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(absolutePath)) + // Open the document in memory to trigger diagnostics without showing it + const doc = await vscode.workspace.openTextDocument(fileUri) - // Save the document to ensure VSCode recognizes it as saved and triggers diagnostics + // Save the document to ensure VSCode recognizes it as saved and + // triggers diagnostics. Without this, VSCode would show "unsaved + // changes" when the user tries to close the file. if (doc.isDirty) { await doc.save() } - // Force a small delay to ensure diagnostics are triggered + // Small delay to allow diagnostics to be triggered await new Promise((resolve) => setTimeout(resolve, 100)) } @@ -712,15 +761,35 @@ export class DiffViewProvider { newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" } + // Read back the final content to detect any user modifications + // that may have occurred via external editors or file watchers + let detectedUserEdits: string | undefined + try { + const finalDoc = await vscode.workspace.openTextDocument(vscode.Uri.file(absolutePath)) + const finalDocContent = finalDoc.getText() + const normalizedExpected = content.replace(/\r\n|\n/g, "\n") + const normalizedActual = finalDocContent.replace(/\r\n|\n/g, "\n") + + if (normalizedActual !== normalizedExpected) { + detectedUserEdits = formatResponse.createPrettyPatch( + relPath.toPosix(), + normalizedExpected, + normalizedActual, + ) + } + } catch { + // If we can't read back the document, proceed without user edit detection + } + // Store the results for formatFileWriteResponse this.newProblemsMessage = newProblemsMessage - this.userEdits = undefined + this.userEdits = detectedUserEdits this.relPath = relPath this.newContent = content return { newProblemsMessage, - userEdits: undefined, + userEdits: detectedUserEdits, finalContent: content, } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index e99f7bf9c8..2f235746c9 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -8,10 +8,14 @@ vi.mock("delay", () => ({ default: vi.fn().mockResolvedValue(undefined), })) -// Mock fs/promises +// Mock fs/promises — readFile returns what writeFile last wrote +let mockFileContent = "file content" vi.mock("fs/promises", () => ({ - readFile: vi.fn().mockResolvedValue("file content"), - writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockImplementation(() => Promise.resolve(mockFileContent)), + writeFile: vi.fn().mockImplementation((_path: string, content: string) => { + mockFileContent = content + return Promise.resolve(undefined) + }), })) // Mock utils @@ -33,10 +37,13 @@ vi.mock("vscode", () => ({ openTextDocument: vi.fn().mockResolvedValue({ isDirty: false, save: vi.fn().mockResolvedValue(undefined), + getText: vi.fn().mockReturnValue(""), }), textDocuments: [], fs: { stat: vi.fn(), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue(Buffer.from("")), }, }, window: { @@ -122,6 +129,7 @@ describe("DiffViewProvider", () => { getState: vi.fn().mockResolvedValue({ includeDiagnosticMessages: true, maxDiagnosticMessages: 50, + autoApprovalEnabled: true, }), }), }, @@ -358,10 +366,38 @@ describe("DiffViewProvider", () => { }) describe("saveDirectly method", () => { - beforeEach(() => { - // Mock vscode functions + beforeEach(async () => { vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any) vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + const fs = await import("fs/promises") + vi.mocked(fs.readFile).mockResolvedValue("new content" as any) + vi.mocked(fs.writeFile).mockResolvedValue(undefined) + vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue({ + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + getText: vi.fn().mockReturnValue("new content"), + uri: { fsPath: `${mockCwd}/test.ts` }, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + } as any) + }) + + it("should use fs.writeFile instead of vscode.workspace.fs.writeFile", async () => { + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) + + // Verify file was written via fs.writeFile, not vscode.workspace.fs.writeFile + const fs = await import("fs/promises") + expect(fs.writeFile).toHaveBeenCalledWith( + expect.stringContaining(`${mockCwd}/test.ts`), + "new content", + "utf-8", + ) + + // Verify vscode.workspace.fs.writeFile was NOT called + expect(vscode.workspace.fs.writeFile).not.toHaveBeenCalled() + + // Verify result + expect(result.newProblemsMessage).toBe("") + expect(result.finalContent).toBe("new content") }) it("should write content directly to file without opening diff view", async () => { @@ -370,9 +406,13 @@ describe("DiffViewProvider", () => { const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) - // Verify file was written + // Verify file was written via fs.writeFile const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalledWith( + expect.stringContaining(`${mockCwd}/test.ts`), + "new content", + "utf-8", + ) // Verify file was opened without focus expect(vscode.window.showTextDocument).toHaveBeenCalledWith( @@ -390,14 +430,18 @@ describe("DiffViewProvider", () => { expect(result.finalContent).toBe("new content") }) - it("should not open file when openWithoutFocus is false", async () => { + it("should not open file when openFile is false", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) - // Verify file was written + // Verify file was written via fs.writeFile const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalledWith( + expect.stringContaining(`${mockCwd}/test.ts`), + "new content", + "utf-8", + ) - // Verify file was NOT opened + // Verify showTextDocument was NOT called (background mode) expect(vscode.window.showTextDocument).not.toHaveBeenCalled() }) @@ -408,9 +452,9 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 1000) - // Verify file was written + // Verify file was written via fs.writeFile const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalled() // Verify delay was NOT called expect(mockDelay).not.toHaveBeenCalled() @@ -433,10 +477,75 @@ describe("DiffViewProvider", () => { // Verify internal state was updated expect((diffViewProvider as any).newProblemsMessage).toBe("") - expect((diffViewProvider as any).userEdits).toBeUndefined() expect((diffViewProvider as any).relPath).toBe("test.ts") expect((diffViewProvider as any).newContent).toBe("new content") }) + + it("should verify content matches after write and retry if mismatch", async () => { + // First readFile returns wrong content (simulating write failure) + // Second readFile (after retry) returns correct content + let readCount = 0 + const fs = await import("fs/promises") + vi.mocked(fs.readFile).mockImplementation(() => { + readCount++ + if (readCount === 1) { + return Promise.resolve("wrong content" as any) + } + return Promise.resolve("new content" as any) + }) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + // Verify write was called twice (initial + retry) + expect(fs.writeFile).toHaveBeenCalledTimes(2) + + // Verify readFile was called twice (initial verification + retry verification) + expect(fs.readFile).toHaveBeenCalledTimes(2) + + // Verify result still succeeds + expect(result.finalContent).toBe("new content") + }) + + it("should throw error if content verification fails after retry", async () => { + // readFile always returns wrong content + const fs = await import("fs/promises") + vi.mocked(fs.readFile).mockResolvedValue("corrupted" as any) + + await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0)).rejects.toThrow( + "Failed to save content to test.ts after 3 attempts", + ) + }) + + it("should skip user edit detection when openTextDocument fails", async () => { + // openTextDocument throws for user edit detection + vi.mocked(vscode.workspace.openTextDocument).mockRejectedValue(new Error("file not found")) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + // Verify no error is thrown and userEdits is undefined + expect(result.userEdits).toBeUndefined() + expect(result.finalContent).toBe("new content") + }) + + it("should detect user edits in background mode", async () => { + // Simulate user edits: final document content differs from what we wrote + vi.mocked(vscode.workspace.openTextDocument).mockResolvedValue({ + isDirty: false, + getText: vi.fn().mockReturnValue("user modified content"), + uri: { fsPath: `${mockCwd}/test.ts` }, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + } as any) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", false, false, 0) + + // Verify userEdits was detected and returned + expect(result.userEdits).toBeDefined() + expect(typeof result.userEdits).toBe("string") + expect(result.userEdits).toContain("user modified content") + + // Verify internal state was updated + expect((diffViewProvider as any).userEdits).toBeDefined() + }) }) describe("saveChanges method with diagnostic settings", () => { diff --git a/src/package.json b/src/package.json index 8411d9bf98..3379a961be 100644 --- a/src/package.json +++ b/src/package.json @@ -3,7 +3,7 @@ "displayName": "%extension.displayName%", "description": "%extension.description%", "publisher": "ZooCodeOrganization", - "version": "3.56.0", + "version": "3.56.9", "icon": "assets/icons/icon.png", "galleryBanner": { "color": "#617A91",