diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index f42a4ebf03..92bafdc38a 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -21,6 +21,18 @@ interface ApplyPatchParams { patch: string } +/** + * B2: result of a single file operation within a patch. `succeeded` controls + * the whole-patch success state (and therefore the per-patch checkpoint), + * while `wrote` records whether the operation actually wrote a file — a no-op + * update must not produce a change-journal entry for a file that was never + * written. + */ +interface ApplyPatchFileOpResult { + succeeded: boolean + wrote: boolean +} + export class ApplyPatchTool extends BaseTool<"apply_patch"> { readonly name = "apply_patch" as const @@ -104,9 +116,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } // Process each file change. The handlers report whether their file - // operation succeeded, so a rejected approval or a failed local write - // does not get checkpointed as if the patch had succeeded. + // operation succeeded (which controls the whole-patch checkpoint) and + // whether it actually wrote a file (which controls the change journal + // — a no-op update must not be journaled). A rejected approval or a + // failed local write never gets checkpointed as a success. let patchSucceeded = true + const successfulChanges: ApplyPatchFileChange[] = [] for (const change of changes) { const relPath = change.path const absolutePath = path.resolve(task.cwd, relPath) @@ -116,7 +131,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) - return + // B2 partial flush: break, not return - an earlier hunk may have + // already written a file, and those writes must still receive the + // checkpoint, journal entry, and change card. Failing the patch + // also keeps the consecutive-mistake counter from resetting. + patchSucceeded = false + break } // Check if file is write-protected @@ -124,37 +144,84 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (change.type === "add") { // Create new file - patchSucceeded = - (await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)) && - patchSucceeded + const addResult = await this.handleAddFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = addResult.succeeded && patchSucceeded + if (addResult.wrote) { + successfulChanges.push(change) + } } else if (change.type === "delete") { // Delete file - patchSucceeded = - (await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)) && - patchSucceeded + const deleteResult = await this.handleDeleteFile( + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = deleteResult.succeeded && patchSucceeded + if (deleteResult.wrote) { + successfulChanges.push(change) + } } else if (change.type === "update") { - // Update file - patchSucceeded = - (await this.handleUpdateFile( - change, - absolutePath, - relPath, - task, - callbacks, - isWriteProtected, - )) && patchSucceeded + // Update file (a no-op update succeeds without writing) + const updateResult = await this.handleUpdateFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = updateResult.succeeded && patchSucceeded + if (updateResult.wrote) { + successfulChanges.push(change) + } } } - task.consecutiveMistakeCount = 0 - - // B1: one checkpoint for the whole patch (not per file), and only when - // every file operation succeeded. Live setting with default-on - // semantics: skip only when explicitly false. + // Reset the consecutive-mistake counter only after a fully successful + // patch: a failed operation (missing file, rejected move, ...) increments + // the counter, and the count must survive a partially written patch so + // the auto-approval safety net still engages across consecutive failed + // patches. if (patchSucceeded) { + task.consecutiveMistakeCount = 0 + } + + // B1: one checkpoint for the whole patch (not per file). Live + // setting with default-on semantics: skip only when explicitly false. + // B3a partial flush: the checkpoint and journal are also taken when at + // least one file operation wrote, even if a later hunk of the same + // patch failed - the journal then documents exactly the subset that + // was written, and the failed operation was already reported through + // pushToolResult. A fully failed patch (nothing written) leaves no + // checkpoint behind. + if (patchSucceeded || successfulChanges.length > 0) { const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints if (perWriteCheckpoints !== false) { - void checkpointSave(task, false, true).catch(() => {}) + // B2: one journal entry per file that was actually written by + // the patch (the simplest correct design for multi-file patches), + // all referencing the single checkpoint above. A no-op update + // contributes no entry because nothing was written. `movePath`, + // when present, is the file's final location. diffStats is + // omitted: the per-file approval diffs are computed inside the + // handlers and are not retained after the patch completes. + void checkpointSave( + task, + false, + true, + successfulChanges.map((change) => ({ + path: change.movePath ?? change.path, + operation: change.type === "add" ? "create" : change.type, + })), + ).catch(() => {}) } } } catch (error) { @@ -170,7 +237,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file already exists @@ -181,7 +248,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File already exists: ${relPath}. Use Update File instead.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } const newContent = change.newContent || "" @@ -235,7 +303,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Save the changes @@ -253,7 +322,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { pushToolResult(message) await task.diffViewProvider.reset() task.processQueuedMessages() - return true + return { succeeded: true, wrote: true } } private async handleDeleteFile( @@ -262,7 +331,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file exists @@ -273,7 +342,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File not found: ${relPath}. Cannot delete a non-existent file.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -295,7 +365,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!didApprove) { pushToolResult("Delete operation was rejected by the user.") - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Delete the file @@ -305,13 +376,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `Failed to delete file '${relPath}': ${error instanceof Error ? error.message : String(error)}` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } task.didEditFile = true pushToolResult(`Successfully deleted ${relPath}`) task.processQueuedMessages() - return true + return { succeeded: true, wrote: true } } private async handleUpdateFile( @@ -321,9 +393,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks + // A move reports failure when the original file cannot be deleted + // after the copy (both paths would remain on disk). + let moveSucceeded = true + // Check if file exists const fileExists = await fileExistsAtPath(absolutePath) if (!fileExists) { @@ -332,7 +408,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File not found: ${relPath}. Cannot update a non-existent file.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } const originalContent = change.originalContent || "" @@ -347,10 +424,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const diff = formatResponse.createPrettyPatch(relPath, originalContent, newContent) if (!diff) { // A no-op change is not a failure: the patch processed cleanly and - // nothing was written, so the whole-patch success state is kept. + // nothing was written, so the whole-patch success state is kept — + // but `wrote` stays false so the change journal does not document a + // write that never happened. pushToolResult(`No changes needed for '${relPath}'`) await task.diffViewProvider.reset() - return true + return { succeeded: true, wrote: false } } // Check experiment settings @@ -396,7 +475,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Handle file move if specified @@ -409,7 +489,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("rooignore_error", change.movePath) pushToolResult(formatResponse.rooIgnoreError(change.movePath)) await task.diffViewProvider.reset() - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Check if destination path is write-protected @@ -421,7 +502,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Check if destination path is outside workspace @@ -433,7 +515,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return false + // Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical + return { succeeded: false, wrote: false } } // Save new content to the new path @@ -452,11 +535,19 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await fs.writeFile(moveAbsolutePath, newContent, "utf8") } - // Delete the original file + // Delete the original file. A failed deletion leaves both paths on + // disk, so the move must be reported as a failure rather than + // checkpointed and journaled as a completed move. try { await fs.unlink(absolutePath) } catch (error) { + moveSucceeded = false console.error(`Failed to delete original file after move: ${error}`) + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const errorMessage = `Move of '${relPath}' to '${change.movePath}' failed: could not delete the original file.` + await task.say("error", errorMessage) + pushToolResult(formatResponse.toolError(errorMessage)) } await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource) @@ -477,7 +568,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { pushToolResult(message) await task.diffViewProvider.reset() task.processQueuedMessages() - return true + if (!moveSucceeded) { + // The destination file was written on disk before the source + // deletion failed, so the write must still be checkpointed and + // journaled; the move itself is reported as failed. + return { succeeded: false, wrote: true } + } + return { succeeded: true, wrote: true } } override async handlePartial(task: Task, block: ToolUse<"apply_patch">): Promise { diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index 0a3cb5d2e8..87a010e90c 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -466,7 +466,14 @@ export class EditFileTool extends BaseTool<"edit_file"> { pushToolResult(message + replacementInfo) if (perWriteCheckpoints) { - void checkpointSave(task, false, true).catch(() => {}) + // B2: the change-journal entry for this edit is appended inside + // checkpointSave (the hook stays a single call site), keyed by the + // checkpoint commit that call produces. + void checkpointSave(task, false, true, { + path: relPath, + operation: isNewFile ? "create" : "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + }).catch(() => {}) } await task.diffViewProvider.reset() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index cf1500a510..5154af860d 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -13,7 +13,7 @@ import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" -import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" +import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff, type DiffStats } from "../diff/stats" import { checkpointSave } from "../checkpoints" import type { ToolUse } from "../../shared/tools" @@ -110,6 +110,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) + // B2: the approval-diff stats for the write, shared by both the + // approval message and the change-journal entry below. + let approvalDiffStats: DiffStats | null = null + if (isPreventFocusDisruptionEnabled) { task.diffViewProvider.editType = fileExists ? "modify" : "create" if (fileExists) { @@ -123,10 +127,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) + approvalDiffStats = computeDiffStats(unified) const completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -155,10 +160,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) + approvalDiffStats = computeDiffStats(unified) const completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -182,9 +188,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) if (perWriteCheckpoints) { - // Await so the checkpoint (staging + commit) finishes before the next - // queued write starts; otherwise two writes can collapse into one commit. - await checkpointSave(task, false, true).catch(() => {}) + // B2: the change-journal entry for this write is appended inside + // checkpointSave (the hook stays a single call site), keyed by the + // checkpoint commit that call produces. Await so the checkpoint + // (staging + commit) finishes before the next queued write starts; + // otherwise two writes can collapse into one commit. + await checkpointSave(task, false, true, { + path: relPath, + operation: fileExists ? "update" : "create", + diffStats: approvalDiffStats + ? { additions: approvalDiffStats.added, deletions: approvalDiffStats.removed } + : undefined, + }).catch(() => {}) } await task.diffViewProvider.reset() diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index f5f64c939e..f9fc895fdd 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -39,7 +39,10 @@ vi.mock("../../../utils/pathUtils", () => ({ })) vi.mock("../../checkpoints", () => ({ + getCheckpointService: vi.fn(), checkpointSave: vi.fn().mockResolvedValue(undefined), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), })) describe("ApplyPatchTool.execute - delete file success path", () => { @@ -144,6 +147,8 @@ describe("ApplyPatchTool.execute - delete file success path", () => { const mockedCheckpointSave = checkpointSave as MockedFunction it("records one suppressed checkpoint for the whole patch (default-on)", async () => { + mockTask.consecutiveMistakeCount = 1 + await tool.execute({ patch: deletePatch }, mockTask as Task, { askApproval: mockAskApproval, handleError: mockHandleError, @@ -152,7 +157,13 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) expect(mockedCheckpointSave).toHaveBeenCalledOnce() - expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true) + // B2: the delete patch produces one journal write, referencing the + // single checkpoint saved for the whole patch. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, + ]) + // A fully successful delete resets the consecutive-mistake counter. + expect(mockTask.consecutiveMistakeCount).toBe(0) }) it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { @@ -290,6 +301,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => { it("records a checkpoint when the add succeeds", async () => { mockedFileExistsAtPath.mockResolvedValue(false) + mockTask.consecutiveMistakeCount = 1 await tool.execute({ patch: addPatch }, mockTask as Task, { askApproval: mockAskApproval, @@ -299,6 +311,12 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // The journal entry documents exactly the file the add wrote. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/new.ts", operation: "create" }, + ]) + // A fully successful add resets the consecutive-mistake counter. + expect(mockTask.consecutiveMistakeCount).toBe(0) }) it("does not record a checkpoint when the file to update does not exist", async () => { @@ -358,6 +376,66 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockedCheckpointSave).not.toHaveBeenCalled() }) + it("checkpoints the written subset when a later hunk is access-denied", async () => { + // Hunk 1 (src/first.ts) writes; hunk 2 (src/denied.ts) is rejected by + // validateAccess. The access-denied branch must not bypass the partial + // flush: the earlier write still receives the checkpoint/journal/card. + // Hunk 2's context matches the mocked file content so the patch + // passes pre-processing; the denial happens at the per-file access check. + const partialDenyPatch = `*** Begin Patch +*** Add File: src/first.ts ++hello +*** Update File: src/denied.ts +@@ +-original file content ++new content +*** End Patch` + const validateAccess = ( + mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } + ).validateAccess + validateAccess.mockReturnValueOnce(true).mockReturnValueOnce(false) + // The add target does not exist, so hunk 1 writes; fileExistsAtPath + // defaults to true and would otherwise reject the add. + mockedFileExistsAtPath.mockResolvedValueOnce(false) + + await tool.execute({ patch: partialDenyPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/denied.ts") + // Only the first (written) hunk is checkpointed. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, [ + expect.objectContaining({ path: "src/first.ts", operation: "create" }), + ]) + }) + + it("does not record a checkpoint when the first hunk is access-denied", async () => { + // The denial happens before any hunk wrote, so the failed patch must + // leave no checkpoint behind: the partial flush has nothing to flush + // and the failure flag must not open the checkpoint gate. + const validateAccess = ( + mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } + ).validateAccess + validateAccess.mockReturnValueOnce(false) + + const deniedFirstPatch = `*** Begin Patch +*** Add File: src/denied.ts ++hello +*** End Patch` + + await tool.execute({ patch: deniedFirstPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/denied.ts") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + it("does not record a checkpoint when the move destination is write-protected", async () => { // Source path check (execute loop) passes; the move destination fails. const isWriteProtected = ( @@ -396,6 +474,38 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockedCheckpointSave).not.toHaveBeenCalled() }) + it("keeps the mistake count when a patch operation fails", async () => { + // Source path check (execute loop) passes; the move destination fails. + const isWriteProtected = ( + mockTask["rooProtectedController"] as unknown as { + isWriteProtected: MockedFunction<(p: string) => boolean> + } + ).isWriteProtected + isWriteProtected.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The failed operation incremented the counter; the end-of-loop reset + // must only run for a fully successful patch, so the count survives. + expect(mockTask.consecutiveMistakeCount).toBe(1) + }) + + it("clears the mistake count after a fully successful patch", async () => { + mockTask.consecutiveMistakeCount = 2 + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + }) + it("records a checkpoint when the move succeeds", async () => { await tool.execute({ patch: movePatch }, mockTask as Task, { askApproval: mockAskApproval, @@ -423,5 +533,138 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") expect(mockedCheckpointSave).toHaveBeenCalledOnce() }) + + it("records one journal write per file change for a multi-file patch", async () => { + // src/a.ts does not exist (add); src/b.ts does (update). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("a.ts")), + ) + const multiPatch = [ + "*** Begin Patch", + "*** Add File: src/a.ts", + "+alpha", + "*** Update File: src/b.ts", + "@@", + "-original file content", + "+second content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: multiPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/a.ts", operation: "create" }, + { path: "src/b.ts", operation: "update" }, + ]) + }) + + it("journals only the files actually written for a mixed no-op and write patch", async () => { + // src/same.ts exists and the hunk rewrites identical content (a + // no-op update); src/new.ts does not exist (a real write). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("new.ts")), + ) + const mixedPatch = [ + "*** Begin Patch", + "*** Update File: src/same.ts", + "@@", + "-original file content", + "+original file content", + "*** Add File: src/new.ts", + "+fresh content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: mixedPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The no-op update is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith("No changes needed for 'src/same.ts'") + // ...but the journal documents only the file that was actually + // written, even though the whole patch succeeded. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/new.ts", operation: "create" }, + ]) + }) + + it("still checkpoints the successful subset when a later hunk fails", async () => { + // src/first.ts already exists (the add fails); src/second.ts does not + // (the add writes). The whole patch fails, but the written file is + // still documented by the checkpoint and journal. + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(String(filePath).toLowerCase().endsWith("first.ts")), + ) + const partialPatch = [ + "*** Begin Patch", + "*** Add File: src/first.ts", + "+boom", + "*** Add File: src/second.ts", + "+fresh", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: partialPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The failed operation is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File already exists")) + // ...and the successful subset is checkpointed and journaled. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/second.ts", operation: "create" }, + ]) + }) + + it("reports a failed move when the original file cannot be deleted", async () => { + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY: resource busy")) + mockTask.consecutiveMistakeCount = 1 + const movePatch = [ + "*** Begin Patch", + "*** Update File: src/old.ts", + "*** Move to: src/new-location.ts", + "@@", + "-original file content", + "+new content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The copy succeeded but the source still exists, so the move is + // reported as a failed tool error - but the destination write was + // made on disk and must still be covered by the checkpoint/journal. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + expect.objectContaining({ path: "src/new-location.ts", operation: "update" }), + ]) + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + // The failed move incremented the counter, and the failed move + // return must keep the patch from resetting it. + expect(mockTask.consecutiveMistakeCount).toBe(2) + // The error channel carries the exact move-failure message. + expect(mockTask.say).toHaveBeenCalledWith( + "error", + "Move of 'src/old.ts' to 'src/new-location.ts' failed: could not delete the original file.", + ) + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("could not delete the original file"), + ) + }) }) }) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 5b645a0074..35501f021a 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -8,6 +8,7 @@ import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" import { checkpointSave } from "../../checkpoints" +import { computeDiffStats } from "../../diff/stats" import { editFileTool } from "../EditFileTool" vi.mock("fs/promises", () => ({ @@ -57,11 +58,16 @@ vi.mock("../../../utils/path", () => ({ vi.mock("../../diff/stats", () => ({ sanitizeUnifiedDiff: vi.fn((diff) => diff), - computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), + // The real computeDiffStats returns { added, removed } (DiffStats) — + // keep the mock faithful to the production shape. + computeDiffStats: vi.fn(() => ({ added: 1, removed: 1 })), })) vi.mock("../../checkpoints", () => ({ + getCheckpointService: vi.fn(), checkpointSave: vi.fn().mockResolvedValue(undefined), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), })) vi.mock("vscode", () => ({ @@ -788,7 +794,13 @@ describe("editFileTool", () => { expect(mockTask.consecutiveMistakeCount).toBe(0) expect(mockedCheckpointSave).toHaveBeenCalledOnce() - expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true) + // B2: the write info threads the path, operation, and the approval + // diff stats into the checkpoint hook. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + }) }) it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { @@ -815,5 +827,31 @@ describe("editFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) expect(mockedCheckpointSave).not.toHaveBeenCalled() }) + + it("records the checkpoint with a create operation for a new file", async () => { + await executeEditFileTool({ old_string: "", new_string: "New file content" }, { fileExists: false }) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 1, deletions: 1 }, + }) + }) + + it("omits diff stats from the checkpoint write when the diff has no stats", async () => { + // A null approval diff produces no diffStats on the journal write. + vi.mocked(computeDiffStats).mockReturnValueOnce(null) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + }) + }) }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index f9286af36d..1edb9e8ad7 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -9,6 +9,7 @@ import { unescapeHtmlEntities } from "../../../utils/text-normalization" import { everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" import { checkpointSave } from "../../checkpoints" +import { formatResponse } from "../../prompts/responses" import { writeToFileTool } from "../WriteToFileTool" vi.mock("path", async () => { @@ -161,6 +162,7 @@ describe("writeToFileTool", () => { userEdits: null, finalContent: "final content", }), + saveDirectly: vi.fn().mockResolvedValue({ finalContent: "saved" }), scrollToFirstDiff: vi.fn(), updateDiagnosticSettings: vi.fn(), pushToolWriteResult: vi.fn().mockImplementation(async function ( @@ -485,7 +487,17 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}) expect(mockedCheckpointSave).toHaveBeenCalledOnce() - expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true) + // B2: the write info threads the path, operation, and the approval + // diff stats (3 added lines, 0 removed) into the checkpoint hook. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + }) + // The approval message carries the same stats object the journal + // receives - not a coerced boolean or a dropped key. + const approvalMessage = JSON.parse(mockAskApproval.mock.calls[0][1] as string) + expect(approvalMessage.diffStats).toEqual({ added: 3, removed: 0 }) }) it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { @@ -548,5 +560,50 @@ describe("writeToFileTool", () => { expect(settled).toBe(true) expect(processQueuedSpy).toHaveBeenCalledOnce() }) + + it("threads write info with approval diff stats when the prevent-focus-disruption experiment is enabled", async () => { + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeWriteFileTool({}) + + // The experiment branch saves directly (no diff view) and still + // journals the write through the same single checkpoint hook. + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + }) + // The focus-disruption branch threads the stats into the approval + // message as well. + const approvalMessage = JSON.parse(mockAskApproval.mock.calls[0][1] as string) + expect(approvalMessage.diffStats).toEqual({ added: 3, removed: 0 }) + }) + + it("omits diff stats from the checkpoint write when the approval diff is empty", async () => { + // Writing identical content to an existing file produces an empty + // approval diff, so the checkpoint write carries no diffStats. + vi.mocked(formatResponse.createPrettyPatch).mockReturnValueOnce("") + + await executeWriteFileTool({}, { fileExists: true }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "update", + }) + }) }) })