From 0575a3544d12fb89e68dc9a29811db3dfa633a38 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Thu, 25 Jun 2026 21:58:46 -0700 Subject: [PATCH 01/17] fix: prevent agent loop stall from WriteToFileTool filesystem errors (#703) - Remove unguarded createDirectoriesForFile call from handlePartial; the call was a redundant optimization (execute() already creates dirs before open()) and its unguarded throw caused the partial-block advancement gate in presentAssistantMessage to be skipped, permanently stalling the agent loop - Move createDirectoriesForFile in execute() inside the try block so EROFS/ EACCES errors route through handleError with diffViewProvider.reset() cleanup and consecutive-mistake counting, rather than escaping unhandled - Add regression tests covering both failure paths --- src/core/tools/WriteToFileTool.ts | 19 +++---- .../tools/__tests__/writeToFileTool.spec.ts | 54 +++++++++++++++++-- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ae026b4b86..e3d804025b 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -67,12 +67,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - if (newContent.startsWith("```")) { newContent = newContent.split("\n").slice(1).join("\n") } @@ -99,6 +93,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { try { task.consecutiveMistakeCount = 0 + // Create parent directories for new files inside the try block so filesystem + // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup + // and consecutive-mistake counting, rather than escaping unhandled. + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } + const provider = task.providerRef.deref() const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true @@ -224,12 +225,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..71e7605109 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -287,15 +287,16 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "does not create directories in handlePartial -- only execute() creates them", async () => { - // First call - path not yet stabilized + // First call - path not yet stabilized, early return await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Second call with same path - path stabilized, handlePartial runs but + // must NOT call createDirectoriesForFile (directory creation belongs in execute) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() }, ) @@ -471,5 +472,50 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) }) + + it.skipIf(process.platform === "win32")( + "EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", + async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called + // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate + // in presentAssistantMessage was never reached and the agent loop stalled permanently. + // After the fix the call is removed entirely -- handlePartial never touches the filesystem. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // First call -- path not yet stabilized, returns early + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // Second call -- path stabilized; createDirectoriesForFile must NOT be called from + // handlePartial, so the mock rejection must not trigger and handleError must not be called + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }, + ) + + it.skipIf(process.platform === "win32")( + "EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", + async () => { + // Regression test: before the fix, createDirectoriesForFile in execute() sat outside + // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. + // After the fix the call is inside the try block, so filesystem errors are caught and + // routed through handleError with proper diffViewProvider.reset() cleanup. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The tool must not have proceeded to open or save + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }, + ) }) }) From 75b52e35d4cf01e058bb71fa0957d51b10f53842 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Thu, 25 Jun 2026 22:19:28 -0700 Subject: [PATCH 02/17] fix: clear stuck UI spinner and duplicate/repeated errors on write_to_file filesystem failure When write_to_file hits a filesystem error (EROFS/EACCES) the streaming phase left the "Zoo wants to edit this file" spinner running, surfaced the same error twice (handlePartial + execute), and spawned a new partial tool message on every subsequent streaming delta. - Add Task.finalizePartialToolAsk() to finalize a partial tool ask without blocking on user input, dismissing the spinner. - handlePartial swallows streaming filesystem errors (after finalizing the spinner and resetting the diff view) so only the authoritative execute() error is reported, eliminating the duplicate error bubble. - Track partialStreamFailed so later streaming deltas short-circuit instead of re-attempting and spawning repeated partial tool messages. - Add regression tests for spinner finalization, single-error reporting, and no repeated partial messages. --- src/core/task/Task.ts | 15 +++ src/core/tools/WriteToFileTool.ts | 56 +++++++-- .../tools/__tests__/writeToFileTool.spec.ts | 117 +++++++++++++++++- 3 files changed, 178 insertions(+), 10 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 8f69a3a0d4..3920d21712 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1831,6 +1831,21 @@ export class Task extends EventEmitter implements TaskLike { return formatResponse.toolError(formatResponse.missingToolParameterError(paramName)) } + /** + * Finalize the last partial "tool" ask message without blocking for user input. + * Call this in error paths where a partial tool message was opened during streaming + * but execution failed before the normal approval flow could close it, so the webview + * spinner does not get stuck in a loading state. + */ + async finalizePartialToolAsk(): Promise { + const lastMessage = this.clineMessages.at(-1) + + if (lastMessage && lastMessage.partial && lastMessage.type === "ask" && lastMessage.ask === "tool") { + lastMessage.partial = false + await this.updateClineMessage(lastMessage) + } + } + // Lifecycle // Start / Resume / Abort / Dispose diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index e3d804025b..11d7f1b253 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -26,6 +26,19 @@ interface WriteToFileParams { export class WriteToFileTool extends BaseTool<"write_to_file"> { readonly name = "write_to_file" as const + /** + * Set when a filesystem error aborts diff-view streaming during handlePartial for the + * current tool invocation. Subsequent streaming deltas for the same block then skip the + * doomed open()/update() retry, which would otherwise create a fresh "Zoo wants to edit + * this file" message on every delta. Cleared by resetPartialState() between invocations. + */ + private partialStreamFailed = false + + override resetPartialState(): void { + super.resetPartialState() + this.partialStreamFailed = false + } + async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path @@ -187,6 +200,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } catch (error) { + // Finalize any open partial tool message so the UI spinner doesn't get stuck. + // The partial ask fired during streaming (handlePartial) or early in execute sets + // partial: true on the webview message; without this, the spinner persists even + // after the error bubble appears. + await task.finalizePartialToolAsk() await handleError("writing file", error as Error) await task.diffViewProvider.reset() this.resetPartialState() @@ -198,6 +216,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content + // A prior streaming delta for this invocation already hit a fatal filesystem error. + // Skip further streaming work so we don't create a new partial tool message on every + // subsequent delta. execute() will report the error once when the block completes. + if (this.partialStreamFailed) { + return + } + // Wait for path to stabilize before showing UI (prevents truncated paths) if (!this.hasPathStabilized(relPath) || newContent === undefined) { return @@ -240,14 +265,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.ask("tool", partialMessage, block.partial).catch(() => {}) if (newContent) { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) - } + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // rethrow: the same filesystem operation is retried in execute() once the + // block completes, and that authoritative non-partial path reports the + // error to the user. Surfacing it here too would show the same error twice. + // Swallowing it here is safe because the agent loop advances naturally when + // the non-partial block arrives (it does not depend on this throw). + console.error(`Error streaming write_to_file diff view:`, error) + // Mark the stream as failed so later deltas don't re-attempt and spawn a new + // partial tool message each time. + this.partialStreamFailed = true + await task.finalizePartialToolAsk() + await task.diffViewProvider.reset() + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 71e7605109..17b477859b 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -186,6 +186,7 @@ describe("writeToFileTool", () => { } mockCline.say = vi.fn().mockResolvedValue(undefined) mockCline.ask = vi.fn().mockResolvedValue(undefined) + mockCline.finalizePartialToolAsk = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") @@ -461,16 +462,104 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("handles partial streaming errors after path stabilizes", async () => { + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { + // The same filesystem operation is retried in execute() once the block completes, + // and that authoritative non-partial path reports the error to the user. Surfacing + // it during streaming too would show the same error twice, so handlePartial must NOT + // route streaming errors through handleError. mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) // First call - path not yet stabilized, no error yet await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, error occurs + // Second call with same path - path is now stabilized, error occurs but is swallowed await executeWriteFileTool({}, { isPartial: true }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial open() fails", async () => { + // Regression test: when diffViewProvider.open() throws during streaming (e.g. EACCES/EROFS + // on a read-only path), the partial tool ask created at the top of handlePartial leaves the + // UI spinner stuck. handlePartial must finalize the partial message and reset the diff view, + // and must NOT surface a duplicate error (execute() reports the authoritative one). + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + + // Second call - path stabilized, update() rejects + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("does not spawn a new partial tool message on each streaming delta after a failure", async () => { + // Regression test: after diffViewProvider.open() throws and the partial message is + // finalized + diff view reset, the next streaming delta saw a non-partial last message + // and created a brand new "Zoo wants to edit this file" message -- repeating once per + // delta. After the fix, partialStreamFailed short-circuits subsequent deltas so only + // the single initial partial ask is issued. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, ask issued once, open() fails, stream marked failed + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Deltas 3..5 - must be short-circuited, no further asks + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Only the single partial ask from delta 2 should have been issued + expect(mockCline.ask).toHaveBeenCalledTimes(1) + // open() must not be retried after the first failure + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + }) + + it("reports a filesystem error only once across the streaming and execute phases", async () => { + // Regression test for the double-error UX defect: a single write_to_file call to a + // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") + // and once in execute() ("writing file"). handlePartial now swallows its error so only + // the authoritative execute() error is surfaced. + const erofs = () => + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }) + mockCline.diffViewProvider.open.mockRejectedValue(erofs()) + mockedCreateDirectoriesForFile.mockRejectedValue(erofs()) + + // Streaming phase: stabilize path then fail (swallowed, no handleError) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Final phase: execute() reports the single authoritative error + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) }) it.skipIf(process.platform === "win32")( @@ -517,5 +606,27 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() }, ) + + it.skipIf(process.platform === "win32")( + "finalizes partial tool message on error so the UI spinner does not get stuck", + async () => { + // Regression test: when a filesystem error is thrown in execute() the webview + // message created during handlePartial (or the early ask in execute) is stuck in + // partial: true state, showing an indefinite spinner alongside the error bubble. + // The catch block must call finalizePartialToolAsk() to close the spinner without + // blocking for user input. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + // handleError must still be called + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + + // finalizePartialToolAsk must have been called to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + }, + ) }) }) From 0966556d548c06ba6d8eef75da76d354f0165be4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 31 Jul 2026 04:53:05 +0800 Subject: [PATCH 03/17] fix(write-to-file): address partial filesystem error review --- src/core/task/Task.ts | 40 +++++++--- src/core/task/__tests__/Task.spec.ts | 72 +++++++++++++++++ src/core/task/__tests__/Task.throttle.test.ts | 3 + src/core/tools/WriteToFileTool.ts | 61 +++++++++++---- .../tools/__tests__/writeToFileTool.spec.ts | 78 +++++++++++++++++++ 5 files changed, 227 insertions(+), 27 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 3920d21712..d6bad9a5c1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1832,18 +1832,36 @@ export class Task extends EventEmitter implements TaskLike { } /** - * Finalize the last partial "tool" ask message without blocking for user input. - * Call this in error paths where a partial tool message was opened during streaming - * but execution failed before the normal approval flow could close it, so the webview - * spinner does not get stuck in a loading state. - */ - async finalizePartialToolAsk(): Promise { - const lastMessage = this.clineMessages.at(-1) - - if (lastMessage && lastMessage.partial && lastMessage.type === "ask" && lastMessage.ask === "tool") { - lastMessage.partial = false - await this.updateClineMessage(lastMessage) + * Finalize a partial "tool" ask message without blocking for user input. + * Call this in error paths where a partial tool message was opened during streaming + * but execution failed before the normal approval flow could close it, so the webview + * spinner does not get stuck in a loading state. + * + * The matching partial message may no longer be the final entry if another asynchronous + * message was inserted between the partial ask and the error handler, so search backward + * instead of relying on clineMessages.at(-1). + */ + async finalizePartialToolAsk(text?: string): Promise { + const partialToolAsk = this.clineMessages + .slice() + .reverse() + .find( + (message) => + message.partial === true && + message.type === "ask" && + message.ask === "tool" && + (text === undefined || message.text === text), + ) + + if (!partialToolAsk) { + return } + + partialToolAsk.partial = false + await this.saveClineMessages() + await this.updateClineMessage(partialToolAsk).catch((error) => { + console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error) + }) } // Lifecycle diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d9f7240c5c..72bab4b79f 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2857,6 +2857,78 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + task.clineMessages.push({ + ts: Date.now() - 1, + type: "say", + say: "error", + text: "intervening async message", + }) + + await task.finalizePartialToolAsk("partial tool message") + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => { + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + task.clineMessages.push({ + ts: Date.now() - 1, + type: "ask", + ask: "tool", + text: "other partial tool message", + partial: true, + }) + + await task.finalizePartialToolAsk("target partial tool message") + await flushMicrotasks() + + expect(task.clineMessages[0].partial).toBe(true) + expect(saveSpy).not.toHaveBeenCalled() + expect(updateSpy).not.toHaveBeenCalled() + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { // Pins the .catch arm on the fire-and-forget updateClineMessage call // in ask() when a new partial ask arrives while the previous partial diff --git a/src/core/task/__tests__/Task.throttle.test.ts b/src/core/task/__tests__/Task.throttle.test.ts index 34d78a4ef9..8cdcc81d17 100644 --- a/src/core/task/__tests__/Task.throttle.test.ts +++ b/src/core/task/__tests__/Task.throttle.test.ts @@ -64,10 +64,12 @@ describe("Task token usage throttling", () => { let mockProvider: any let mockApiConfiguration: ProviderSettings let task: Task + let consoleLogSpy: ReturnType beforeEach(() => { // Reset all mocks vi.clearAllMocks() + consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}) vi.useFakeTimers() // Mock provider @@ -101,6 +103,7 @@ describe("Task token usage throttling", () => { if (task && !task.abort) { task.dispose() } + consoleLogSpy.mockRestore() }) test("should emit TaskTokenUsageUpdated immediately on first change", async () => { diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 11d7f1b253..1133e8c66f 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -27,22 +27,47 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { readonly name = "write_to_file" as const /** - * Set when a filesystem error aborts diff-view streaming during handlePartial for the - * current tool invocation. Subsequent streaming deltas for the same block then skip the - * doomed open()/update() retry, which would otherwise create a fresh "Zoo wants to edit - * this file" message on every delta. Cleared by resetPartialState() between invocations. + * Tracks filesystem failures from diff-view streaming by task id. Tool instances are + * singletons, so this state must be keyed per task to avoid one task's failing partial + * stream suppressing another task's streaming deltas. */ - private partialStreamFailed = false + private partialStreamFailuresByTaskId = new Set() + + /** + * Tracks partial path stabilization by task id. The tool is a singleton, so using the + * BaseTool singleton path state lets concurrent tasks incorrectly stabilize each other. + */ + private lastSeenPartialPathByTaskId = new Map() + + private getPartialStreamFailureKey(task: Task): string { + return `${task.taskId}.${task.instanceId}` + } + + private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean { + const key = this.getPartialStreamFailureKey(task) + const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key) + const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath + this.lastSeenPartialPathByTaskId.set(key, partialPath) + return pathHasStabilized && !!partialPath + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + this.lastSeenPartialPathByTaskId.delete(key) + this.partialStreamFailuresByTaskId.delete(key) + } override resetPartialState(): void { super.resetPartialState() - this.partialStreamFailed = false + this.partialStreamFailuresByTaskId.clear() + this.lastSeenPartialPathByTaskId.clear() } async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path let newContent = params.content + const partialStreamFailureKey = this.getPartialStreamFailureKey(task) if (!relPath) { task.consecutiveMistakeCount++ @@ -104,8 +129,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } try { - task.consecutiveMistakeCount = 0 - // Create parent directories for new files inside the try block so filesystem // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup // and consecutive-mistake counting, rather than escaping unhandled. @@ -113,6 +136,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await createDirectoriesForFile(absolutePath) } + task.consecutiveMistakeCount = 0 + const provider = task.providerRef.deref() const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true @@ -194,7 +219,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetTaskPartialState(task) task.processQueuedMessages() @@ -207,7 +232,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.finalizePartialToolAsk() await handleError("writing file", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + this.resetTaskPartialState(task) return } } @@ -216,15 +241,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content - // A prior streaming delta for this invocation already hit a fatal filesystem error. + const partialStreamFailureKey = this.getPartialStreamFailureKey(task) + + // A prior streaming delta for this task already hit a fatal filesystem error. // Skip further streaming work so we don't create a new partial tool message on every // subsequent delta. execute() will report the error once when the block completes. - if (this.partialStreamFailed) { + if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) { return } // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath) || newContent === undefined) { + if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) { return } @@ -286,9 +313,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { console.error(`Error streaming write_to_file diff view:`, error) // Mark the stream as failed so later deltas don't re-attempt and spawn a new // partial tool message each time. - this.partialStreamFailed = true - await task.finalizePartialToolAsk() - await task.diffViewProvider.reset() + this.partialStreamFailuresByTaskId.add(partialStreamFailureKey) + await task.finalizePartialToolAsk(partialMessage) + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view after partial failure:", resetError) + }) } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 17b477859b..c6a10d3ea3 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -128,6 +128,8 @@ describe("writeToFileTool", () => { return content }) + mockCline.taskId = "task-1" + mockCline.instanceId = "instance-1" mockCline.cwd = "/" mockCline.consecutiveMistakeCount = 0 mockCline.didEditFile = false @@ -421,6 +423,25 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) }) + it("does not share path stabilization between tasks with the same path", async () => { + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).not.toHaveBeenCalled() + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).not.toHaveBeenCalled() + + mockCline.taskId = "task-1" + mockCline.instanceId = "instance-1" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + }) }) describe("user interaction", () => { @@ -562,6 +583,63 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) }) + it("does not reset consecutive mistake count when directory creation fails", async () => { + mockCline.consecutiveMistakeCount = 3 + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.consecutiveMistakeCount).toBe(3) + }) + + it("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.update.mockResolvedValue(undefined) + mockCline.diffViewProvider.editType = undefined + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2) + }) + + it("swallows diff view reset errors during partial failure cleanup", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view after partial failure:", + expect.any(Error), + ) + + consoleErrorSpy.mockRestore() + }) + it.skipIf(process.platform === "win32")( "EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { From 16c4d485bef6d1e50f8a6e0c31acda95b5c7b275 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 31 Jul 2026 05:54:31 +0800 Subject: [PATCH 04/17] fix(write-to-file): guard diff reset cleanup --- src/core/tools/WriteToFileTool.ts | 162 ++++++++++-------- .../tools/__tests__/writeToFileTool.spec.ts | 20 ++- 2 files changed, 105 insertions(+), 77 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 1133e8c66f..836a928c45 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -57,6 +57,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { this.partialStreamFailuresByTaskId.delete(key) } + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + override resetPartialState(): void { super.resetPartialState() this.partialStreamFailuresByTaskId.clear() @@ -147,92 +153,104 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) - if (isPreventFocusDisruptionEnabled) { - task.diffViewProvider.editType = fileExists ? "modify" : "create" - if (fileExists) { - const absolutePath = path.resolve(task.cwd, relPath) - task.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") + try { + if (isPreventFocusDisruptionEnabled) { + task.diffViewProvider.editType = fileExists ? "modify" : "create" + if (fileExists) { + const absolutePath = path.resolve(task.cwd, relPath) + task.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") + } else { + task.diffViewProvider.originalContent = "" + } + + let unified = fileExists + ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) + : convertNewFileToUnifiedDiff(newContent, relPath) + unified = sanitizeUnifiedDiff(unified) + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + content: unified, + diffStats: computeDiffStats(unified) || undefined, + } satisfies ClineSayTool) + + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) + + if (!didApprove) { + return + } + + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + ) } else { - task.diffViewProvider.originalContent = "" + if (!task.diffViewProvider.isEditing) { + const partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, true).catch(() => {}) + await task.diffViewProvider.open(relPath) + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + true, + ) + + await delay(300) + task.diffViewProvider.scrollToFirstDiff() + + let unified = fileExists + ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) + : convertNewFileToUnifiedDiff(newContent, relPath) + unified = sanitizeUnifiedDiff(unified) + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + content: unified, + diffStats: computeDiffStats(unified) || undefined, + } satisfies ClineSayTool) + + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) + + if (!didApprove) { + await task.diffViewProvider.revertChanges() + return + } + + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } - let unified = fileExists - ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) - : convertNewFileToUnifiedDiff(newContent, relPath) - unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - content: unified, - diffStats: computeDiffStats(unified) || undefined, - } satisfies ClineSayTool) - - const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) - - if (!didApprove) { - return + if (relPath) { + await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) - } else { - if (!task.diffViewProvider.isEditing) { - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, true).catch(() => {}) - await task.diffViewProvider.open(relPath) - } + task.didEditFile = true - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - true, - ) + const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) - await delay(300) - task.diffViewProvider.scrollToFirstDiff() + pushToolResult(message) - let unified = fileExists - ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) - : convertNewFileToUnifiedDiff(newContent, relPath) - unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - content: unified, - diffStats: computeDiffStats(unified) || undefined, - } satisfies ClineSayTool) + await this.resetDiffViewAfterWrite(task) - const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) + task.processQueuedMessages() - if (!didApprove) { - await task.diffViewProvider.revertChanges() - return - } - - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + return + } finally { + this.resetTaskPartialState(task) } - - if (relPath) { - await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) - } - - task.didEditFile = true - - const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) - - pushToolResult(message) - - await task.diffViewProvider.reset() - this.resetTaskPartialState(task) - - task.processQueuedMessages() - - return } catch (error) { // Finalize any open partial tool message so the UI spinner doesn't get stuck. // The partial ask fired during streaming (handlePartial) or early in execute sets // partial: true on the webview message; without this, the spinner persists even // after the error bubble appears. - await task.finalizePartialToolAsk() - await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetTaskPartialState(task) + try { + await task.finalizePartialToolAsk() + await handleError("writing file", error as Error) + await this.resetDiffViewAfterWrite(task) + } finally { + this.resetTaskPartialState(task) + } return } } @@ -315,9 +333,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // partial tool message each time. this.partialStreamFailuresByTaskId.add(partialStreamFailureKey) await task.finalizePartialToolAsk(partialMessage) - await task.diffViewProvider.reset().catch((resetError) => { - console.error("Error resetting write_to_file diff view after partial failure:", resetError) - }) + await this.resetDiffViewAfterWrite(task) } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index c6a10d3ea3..3f3a21f146 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -191,6 +191,7 @@ describe("writeToFileTool", () => { mockCline.finalizePartialToolAsk = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") + mockCline.processQueuedMessages = vi.fn() mockAskApproval = vi.fn().mockResolvedValue(true) mockHandleError = vi.fn().mockResolvedValue(undefined) @@ -396,6 +397,20 @@ describe("writeToFileTool", () => { // Should process normally without issues expect(mockCline.consecutiveMistakeCount).toBe(0) }) + + it("does not report a successful write as failed when final diff reset rejects", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockCline.didEditFile).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + + consoleErrorSpy.mockRestore() + }) }) describe("partial block handling", () => { @@ -632,10 +647,7 @@ describe("writeToFileTool", () => { expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() - expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error resetting write_to_file diff view after partial failure:", - expect.any(Error), - ) + expect(consoleErrorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) consoleErrorSpy.mockRestore() }) From be0e1548ed48863d6762dfea2034eb7eaa23c49c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 31 Jul 2026 06:33:55 +0800 Subject: [PATCH 05/17] fix(write-to-file): guard partial ask finalization --- src/core/tools/WriteToFileTool.ts | 160 +++++++++--------- .../tools/__tests__/writeToFileTool.spec.ts | 41 +++++ 2 files changed, 118 insertions(+), 83 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 836a928c45..01ee610537 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -63,6 +63,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { + await task.finalizePartialToolAsk(text).catch((finalizeError) => { + console.error("Error finalizing write_to_file partial tool ask:", finalizeError) + }) + } + override resetPartialState(): void { super.resetPartialState() this.partialStreamFailuresByTaskId.clear() @@ -153,105 +159,93 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) - try { - if (isPreventFocusDisruptionEnabled) { - task.diffViewProvider.editType = fileExists ? "modify" : "create" - if (fileExists) { - const absolutePath = path.resolve(task.cwd, relPath) - task.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") - } else { - task.diffViewProvider.originalContent = "" - } - - let unified = fileExists - ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) - : convertNewFileToUnifiedDiff(newContent, relPath) - unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - content: unified, - diffStats: computeDiffStats(unified) || undefined, - } satisfies ClineSayTool) - - const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) - - if (!didApprove) { - return - } - - await task.diffViewProvider.saveDirectly( - relPath, - newContent, - false, - diagnosticsEnabled, - writeDelayMs, - ) + if (isPreventFocusDisruptionEnabled) { + task.diffViewProvider.editType = fileExists ? "modify" : "create" + if (fileExists) { + const absolutePath = path.resolve(task.cwd, relPath) + task.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") } else { - if (!task.diffViewProvider.isEditing) { - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, true).catch(() => {}) - await task.diffViewProvider.open(relPath) - } - - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - true, - ) - - await delay(300) - task.diffViewProvider.scrollToFirstDiff() - - let unified = fileExists - ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) - : convertNewFileToUnifiedDiff(newContent, relPath) - unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - content: unified, - diffStats: computeDiffStats(unified) || undefined, - } satisfies ClineSayTool) - - const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) - - if (!didApprove) { - await task.diffViewProvider.revertChanges() - return - } - - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + task.diffViewProvider.originalContent = "" } - if (relPath) { - await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) + let unified = fileExists + ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) + : convertNewFileToUnifiedDiff(newContent, relPath) + unified = sanitizeUnifiedDiff(unified) + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + content: unified, + diffStats: computeDiffStats(unified) || undefined, + } satisfies ClineSayTool) + + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) + + if (!didApprove) { + return } - task.didEditFile = true + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + } else { + if (!task.diffViewProvider.isEditing) { + const partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, true).catch(() => {}) + await task.diffViewProvider.open(relPath) + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + true, + ) - const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) + await delay(300) + task.diffViewProvider.scrollToFirstDiff() - pushToolResult(message) + let unified = fileExists + ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) + : convertNewFileToUnifiedDiff(newContent, relPath) + unified = sanitizeUnifiedDiff(unified) + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + content: unified, + diffStats: computeDiffStats(unified) || undefined, + } satisfies ClineSayTool) - await this.resetDiffViewAfterWrite(task) + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) - task.processQueuedMessages() + if (!didApprove) { + await task.diffViewProvider.revertChanges() + return + } + + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + } - return - } finally { - this.resetTaskPartialState(task) + if (relPath) { + await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } + + task.didEditFile = true + + const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) + + pushToolResult(message) + + await this.resetDiffViewAfterWrite(task) + + task.processQueuedMessages() + + return } catch (error) { // Finalize any open partial tool message so the UI spinner doesn't get stuck. // The partial ask fired during streaming (handlePartial) or early in execute sets // partial: true on the webview message; without this, the spinner persists even // after the error bubble appears. - try { - await task.finalizePartialToolAsk() - await handleError("writing file", error as Error) - await this.resetDiffViewAfterWrite(task) - } finally { - this.resetTaskPartialState(task) - } + await this.finalizePartialToolAskAfterFailure(task) + await handleError("writing file", error as Error) + await this.resetDiffViewAfterWrite(task) return + } finally { + this.resetTaskPartialState(task) } } @@ -332,7 +326,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Mark the stream as failed so later deltas don't re-attempt and spawn a new // partial tool message each time. this.partialStreamFailuresByTaskId.add(partialStreamFailureKey) - await task.finalizePartialToolAsk(partialMessage) + await this.finalizePartialToolAskAfterFailure(task, partialMessage) await this.resetDiffViewAfterWrite(task) } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 3f3a21f146..862bc6a232 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -610,6 +610,26 @@ describe("writeToFileTool", () => { expect(mockCline.consecutiveMistakeCount).toBe(3) }) + it("continues execute error cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + + consoleErrorSpy.mockRestore() + }) + it("keeps partial stream failures isolated per task", async () => { mockCline.diffViewProvider.open.mockRejectedValueOnce( Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }), @@ -652,6 +672,27 @@ describe("writeToFileTool", () => { consoleErrorSpy.mockRestore() }) + it("continues partial failure cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + + consoleErrorSpy.mockRestore() + }) + it.skipIf(process.platform === "win32")( "EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { From 224690bcf131f9124f0482cf3dd3282c674bcf4c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 31 Jul 2026 06:56:21 +0800 Subject: [PATCH 06/17] fix(write-to-file): clean partial state on task abort --- src/core/tools/WriteToFileTool.ts | 29 +++++- .../tools/__tests__/writeToFileTool.spec.ts | 88 +++++++++++++------ 2 files changed, 87 insertions(+), 30 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 01ee610537..2d442b22e1 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -2,7 +2,7 @@ import path from "path" import delay from "delay" import fs from "fs/promises" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS, RooCodeEventName } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -39,11 +39,29 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { */ private lastSeenPartialPathByTaskId = new Map() + /** + * Tracks abort cleanup listeners for per-task partial state so normal execute() + * finalization can unregister them and abandoned streams are torn down on abort. + */ + private partialStateAbortCleanupByTaskId = new Map void }>() + private getPartialStreamFailureKey(task: Task): string { return `${task.taskId}.${task.instanceId}` } + private registerTaskPartialStateCleanup(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + if (this.partialStateAbortCleanupByTaskId.has(key)) { + return + } + + const cleanup = () => this.resetTaskPartialState(task) + this.partialStateAbortCleanupByTaskId.set(key, { task, cleanup }) + task.once(RooCodeEventName.TaskAborted, cleanup) + } + private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean { + this.registerTaskPartialStateCleanup(task) const key = this.getPartialStreamFailureKey(task) const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key) const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath @@ -53,6 +71,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { private resetTaskPartialState(task: Task): void { const key = this.getPartialStreamFailureKey(task) + const abortCleanup = this.partialStateAbortCleanupByTaskId.get(key) + if (abortCleanup) { + task.off(RooCodeEventName.TaskAborted, abortCleanup.cleanup) + this.partialStateAbortCleanupByTaskId.delete(key) + } this.lastSeenPartialPathByTaskId.delete(key) this.partialStreamFailuresByTaskId.delete(key) } @@ -71,8 +94,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { override resetPartialState(): void { super.resetPartialState() + for (const { task, cleanup } of this.partialStateAbortCleanupByTaskId.values()) { + task.off(RooCodeEventName.TaskAborted, cleanup) + } this.partialStreamFailuresByTaskId.clear() this.lastSeenPartialPathByTaskId.clear() + this.partialStateAbortCleanupByTaskId.clear() } async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 862bc6a232..ea38e2d059 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -1,5 +1,6 @@ import * as path from "path" +import { RooCodeEventName } from "@roo-code/types" import type { MockedFunction } from "vitest" import { fileExistsAtPath, createDirectoriesForFile } from "../../../utils/fs" @@ -188,6 +189,8 @@ describe("writeToFileTool", () => { } mockCline.say = vi.fn().mockResolvedValue(undefined) mockCline.ask = vi.fn().mockResolvedValue(undefined) + mockCline.once = vi.fn() + mockCline.off = vi.fn() mockCline.finalizePartialToolAsk = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") @@ -457,6 +460,29 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(2) }) + + it("cleans per-task partial state when the task aborts before execute finalization", async () => { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.once).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + + abortCleanup?.() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + }) }) describe("user interaction", () => { @@ -612,22 +638,24 @@ describe("writeToFileTool", () => { it("continues execute error cleanup when finalizing partial ask fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - mockedCreateDirectoriesForFile.mockRejectedValue( - Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), - ) - mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) - - await executeWriteFileTool({}, { fileExists: false }) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() - expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() - expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error finalizing write_to_file partial tool ask:", - expect.any(Error), - ) + await executeWriteFileTool({}, { fileExists: false }) - consoleErrorSpy.mockRestore() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } }) it("keeps partial stream failures isolated per task", async () => { @@ -674,23 +702,25 @@ describe("writeToFileTool", () => { it("continues partial failure cleanup when finalizing partial ask fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - mockCline.diffViewProvider.open.mockRejectedValue( - Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), - ) - mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) - - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() - expect(mockHandleError).not.toHaveBeenCalled() - expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error finalizing write_to_file partial tool ask:", - expect.any(Error), - ) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - consoleErrorSpy.mockRestore() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } }) it.skipIf(process.platform === "win32")( From c17b15ba4a2aeae20ece1b8d4c19dc4758b15cfe Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Wed, 12 Aug 2026 20:09:26 +0800 Subject: [PATCH 07/17] fix(write-to-file): address remaining review cleanup --- src/core/task/Task.ts | 20 ++-- src/core/task/__tests__/Task.spec.ts | 49 +++++++++- src/core/tools/WriteToFileTool.ts | 10 +- .../tools/__tests__/writeToFileTool.spec.ts | 96 +++++++++++++++---- 4 files changed, 143 insertions(+), 32 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 34b2c608bf..61f4c6531f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -65,7 +65,7 @@ import { ApiStream, GroundingSource } from "../../api/transform/stream" import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" // shared -import { findLastIndex } from "../../shared/array" +import { findLast, findLastIndex } from "../../shared/array" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" import { t } from "../../i18n" @@ -1840,16 +1840,14 @@ export class Task extends EventEmitter implements TaskLike { * instead of relying on clineMessages.at(-1). */ async finalizePartialToolAsk(text?: string): Promise { - const partialToolAsk = this.clineMessages - .slice() - .reverse() - .find( - (message) => - message.partial === true && - message.type === "ask" && - message.ask === "tool" && - (text === undefined || message.text === text), - ) + const partialToolAsk = findLast( + this.clineMessages, + (message) => + message.partial === true && + message.type === "ask" && + message.ask === "tool" && + (text === undefined || message.text === text), + ) if (!partialToolAsk) { return diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 82aa0e13d4..a886ab211c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3351,9 +3351,12 @@ describe("Cline", () => { }) it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { + let updateSnapshot: Record | undefined const updateSpy = vi .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") - .mockResolvedValue(undefined) + .mockImplementation(async (message) => { + updateSnapshot = { ...message } + }) const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) const task = new Task({ @@ -3385,6 +3388,7 @@ describe("Cline", () => { expect(partialToolAsk.partial).toBe(false) expect(saveSpy).toHaveBeenCalled() expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + expect(updateSnapshot?.partial).toBe(false) updateSpy.mockRestore() saveSpy.mockRestore() @@ -3422,6 +3426,49 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk updates the latest partial tool ask when no text is provided", async () => { + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const olderPartialToolAsk = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "tool" as const, + text: "older partial tool message", + partial: true, + } + const latestPartialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "latest partial tool message", + partial: true, + } + + task.clineMessages.push(olderPartialToolAsk) + task.clineMessages.push(latestPartialToolAsk) + + await task.finalizePartialToolAsk() + await flushMicrotasks() + + expect(olderPartialToolAsk.partial).toBe(true) + expect(latestPartialToolAsk.partial).toBe(false) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(latestPartialToolAsk) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { // Pins the .catch arm on the fire-and-forget updateClineMessage call // in ask() when a new partial ask arrives while the previous partial diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 2d442b22e1..75cd0a1090 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -61,7 +61,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean { - this.registerTaskPartialStateCleanup(task) const key = this.getPartialStreamFailureKey(task) const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key) const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath @@ -106,13 +105,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path let newContent = params.content - const partialStreamFailureKey = this.getPartialStreamFailureKey(task) if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) - await task.diffViewProvider.reset() + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } @@ -120,7 +119,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) - await task.diffViewProvider.reset() + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } @@ -289,6 +289,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + this.registerTaskPartialStateCleanup(task) + // Wait for path to stabilize before showing UI (prevents truncated paths) if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) { return diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index ea38e2d059..ffca317b5f 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -231,8 +231,10 @@ describe("writeToFileTool", () => { ...params, }, nativeArgs: { - path: (params.path ?? testFilePath) as any, - content: (params.content ?? testContent) as any, + path: (Object.prototype.hasOwnProperty.call(params, "path") ? params.path : testFilePath) as any, + content: (Object.prototype.hasOwnProperty.call(params, "content") + ? params.content + : testContent) as any, }, partial: isPartial, } @@ -476,6 +478,7 @@ describe("writeToFileTool", () => { expect(mockCline.once).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) abortCleanup?.() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(1) @@ -524,6 +527,62 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + it("uses safe reset and clears partial state when path is missing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ path: "" }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + expect(mockPushToolResult).toHaveBeenCalledWith("Missing param error") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("uses safe reset and clears partial state when content is missing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ content: undefined }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + expect(mockPushToolResult).toHaveBeenCalledWith("Missing param error") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { // The same filesystem operation is retried in execute() once the block completes, // and that authoritative non-partial path reports the error to the user. Surfacing @@ -556,7 +615,7 @@ describe("writeToFileTool", () => { // Second call - path stabilized, open() rejects await executeWriteFileTool({}, { isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() }) @@ -573,7 +632,7 @@ describe("writeToFileTool", () => { // Second call - path stabilized, update() rejects await executeWriteFileTool({}, { isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() }) @@ -684,20 +743,25 @@ describe("writeToFileTool", () => { it("swallows diff view reset errors during partial failure cleanup", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - mockCline.diffViewProvider.open.mockRejectedValue( - Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), - ) - mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) - - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() - expect(mockHandleError).not.toHaveBeenCalled() - expect(consoleErrorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - consoleErrorSpy.mockRestore() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } }) it("continues partial failure cleanup when finalizing partial ask fails", async () => { From 6f99772f3f9eff746b40396a977e5bdc4bd6ecdb Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 20 Aug 2026 00:29:41 +0800 Subject: [PATCH 08/17] fix(write-to-file): clean up partial state on rooignore denial Address the remaining review comments on PR #1066: - The rooignore early-return in execute() now performs the same cleanup as the try/finally path: finalize the partial tool ask (so the UI spinner does not stick), reset the diff view through the error-safe wrapper, and clear the per-task partial state (abort listener + path-stabilization + failure map). handlePartial() has no rooignore guard, so a denied path can still have streamed a partial ask before the access check runs. - Document that the per-task keying deliberately diverges from the sibling streaming tools: it is reachable across ClineProvider instances (sidebar and tab-panel providers stream independently through the shared tool singleton), while a single provider streams at most one task at a time (TaskScheduler maxConcurrency=1, delegation disposes the parent first). Lifting the keying into BaseTool for all tools is a follow-up PR. Validation: - vitest run core/tools/__tests__/writeToFileTool.spec.ts: 34 passed / 8 skipped - eslint --prune-suppressions --max-warnings=0 on both changed files: clean - pnpm check-types (tsc --noEmit): clean --- src/core/tools/WriteToFileTool.ts | 42 ++++++++++++++++--- .../tools/__tests__/writeToFileTool.spec.ts | 42 +++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 75cd0a1090..44b8fb65fc 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -27,15 +27,38 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { readonly name = "write_to_file" as const /** - * Tracks filesystem failures from diff-view streaming by task id. Tool instances are - * singletons, so this state must be keyed per task to avoid one task's failing partial - * stream suppressing another task's streaming deltas. + * Tracks filesystem failures from diff-view streaming, keyed by task id (taskId + + * instanceId). + * + * This deliberately diverges from the sibling streaming tools (ApplyDiffTool, + * EditFileTool, SearchReplaceTool, EditTool), which rely on BaseTool's singleton + * lastSeenPartialPath / resetPartialState and keep no failure state. The divergence is + * intentional, for two reasons: + * + * 1. Only this tool's handlePartial performs failure-prone streaming work + * (diffViewProvider.open/update, which can throw EACCES/EROFS); the siblings only + * send a task.ask preview. Without per-task failure tracking, every later delta for + * a failed path would re-attempt the failing operation and re-spawn a partial tool + * message. + * + * 2. The tool instance is a module-level singleton shared by every task, including + * tasks from different ClineProvider instances (e.g. sidebar and tab-panel + * providers, which activate independently). A single provider streams at most one + * task at a time — TaskScheduler gates task.run() at maxConcurrency=1 and + * delegation disposes the parent before the child starts — so per-task keying is + * reachable specifically across providers, where two providers can stream + * write_to_file concurrently through this same singleton. + * + * Lifting this per-task keying into BaseTool for all streaming tools is a follow-up + * (separate PR); it is deliberately not done here. */ private partialStreamFailuresByTaskId = new Set() /** - * Tracks partial path stabilization by task id. The tool is a singleton, so using the - * BaseTool singleton path state lets concurrent tasks incorrectly stabilize each other. + * Tracks partial path stabilization, keyed by task id (taskId + instanceId), so one + * task's streaming deltas cannot stabilize another task's path. Keyed per task for the + * same cross-provider reason as partialStreamFailuresByTaskId (see that note; the + * divergence from the sibling tools' BaseTool state is deliberate). */ private lastSeenPartialPathByTaskId = new Map() @@ -129,6 +152,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + // handlePartial() has no rooignore guard, so streaming deltas for this denied + // path may already have created a partial `tool` ask (partial: true) and opened + // the diff view before execute() reached the access check. Denying here without + // cleanup would leave the UI spinner stuck (partial: true), the diff view open, + // and this task's abort listener / path-stabilization / failure-map entries + // leaked. Perform the same cleanup the try/finally path does before returning. + await this.finalizePartialToolAskAfterFailure(task) + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index ffca317b5f..9ed8f92cbf 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -259,6 +259,48 @@ describe("writeToFileTool", () => { expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(testFilePath) expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) }) + + it("finalizes the partial ask and clears per-task state when rooignore denies access", async () => { + // handlePartial() has no rooignore guard, so streaming deltas for a denied path + // still create a partial `tool` ask (partial: true) and open the diff view before + // execute() reaches the access check. The denial must clean up all of that: + // finalize the partial ask (spinner does not stick), reset the diff view (reset + // failures swallowed), and clear the per-task state (abort listener + maps). + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + // Stream two deltas so the path stabilizes: handlePartial registers the abort + // cleanup and opens the partial ask + diff view for the (soon denied) path. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(abortCleanup).toBeTypeOf("function") + + // The completed block now reaches the access check, which denies the path. + await executeWriteFileTool({}, { fileExists: false, accessAllowed: false }) + + expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("file existence detection", () => { From 4189f88fde83637d2f86063966733a2739232a51 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 23 Aug 2026 18:14:27 +0800 Subject: [PATCH 09/17] test(task): cover finalizePartialToolAsk updateClineMessage rejection path The .catch arm in finalizePartialToolAsk was the only executable line added by this PR without test coverage. Add a spec that makes updateClineMessage reject and pins the contract: the partial flag is still persisted (saveClineMessages ran first), the failure is logged instead of propagated, and finalize resolves so error-path cleanup (diff-view reset, per-task state reset) always completes. --- src/core/task/__tests__/Task.spec.ts | 45 ++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index a886ab211c..2b952686b2 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3469,6 +3469,51 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk logs (instead of rejecting) when updateClineMessage rejects", async () => { + // Pins the .catch arm on updateClineMessage in finalizePartialToolAsk: the + // partial flag must already be persisted (saveClineMessages ran first), the + // failure must only be logged, and finalize must still resolve so callers' + // error-path cleanup (diff-view reset, resetTaskPartialState) always completes. + const boom = new Error("updateClineMessage boom") + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockImplementation(async () => { + throw boom + }) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] updateClineMessage failed:", + boom, + ) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { // Pins the .catch arm on the fire-and-forget updateClineMessage call // in ask() when a new partial ask arrives while the previous partial From f3e4d409eb822a908373a5533bd59df86b20e22b Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 29 Aug 2026 14:40:41 +0800 Subject: [PATCH 10/17] fix(tools): finalize open partial tool ask on args parse failure A tool block whose final args fail to parse (e.g. the tool call was truncated mid-JSON by the output token limit) never reaches execute(), so a partial 'tool' ask opened during streaming was left with partial: true and the webview spinner stayed stuck indefinitely. Finalize it in BaseTool.handle's parse-failure path before reporting the error. finalizePartialToolAsk also clears any stale progressStatus on the finalized message, and the affected presentAssistantMessage task mock gains the new Task method. --- ...resentAssistantMessage-custom-tool.spec.ts | 1 + src/core/task/Task.ts | 5 +++ src/core/task/__tests__/Task.spec.ts | 3 ++ src/core/tools/BaseTool.ts | 7 ++++ .../tools/__tests__/writeToFileTool.spec.ts | 34 +++++++++++++++++++ 5 files changed, 50 insertions(+) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts index 1ef25e852b..3e2685bf33 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -79,6 +79,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }, say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), } // Add pushToolResultToUserContent method after mockTask is created so it can reference mockTask diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 61f4c6531f..e1c12dcbc0 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1838,6 +1838,10 @@ export class Task extends EventEmitter implements TaskLike { * The matching partial message may no longer be the final entry if another asynchronous * message was inserted between the partial ask and the error handler, so search backward * instead of relying on clineMessages.at(-1). + * + * Any in-progress `progressStatus` on the message is cleared as well: the ask is being + * finalized because it will NOT complete, so a stale "in progress" indicator would be + * misleading (the normal completion path overwrites it with the final status instead). */ async finalizePartialToolAsk(text?: string): Promise { const partialToolAsk = findLast( @@ -1854,6 +1858,7 @@ export class Task extends EventEmitter implements TaskLike { } partialToolAsk.partial = false + partialToolAsk.progressStatus = undefined await this.saveClineMessages() await this.updateClineMessage(partialToolAsk).catch((error) => { console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 2b952686b2..b13b71074c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3372,6 +3372,7 @@ describe("Cline", () => { ask: "tool" as const, text: "partial tool message", partial: true, + progressStatus: { text: "Generating…", icon: "sync" }, } task.clineMessages.push(partialToolAsk) @@ -3386,9 +3387,11 @@ describe("Cline", () => { await flushMicrotasks() expect(partialToolAsk.partial).toBe(false) + expect(partialToolAsk.progressStatus).toBeUndefined() expect(saveSpy).toHaveBeenCalled() expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) expect(updateSnapshot?.partial).toBe(false) + expect(updateSnapshot?.progressStatus).toBeUndefined() updateSpy.mockRestore() saveSpy.mockRestore() diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dd15059662 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -156,6 +156,13 @@ export abstract class BaseTool { } } catch (error) { console.error(`Error parsing parameters:`, error) + // Final args could not be parsed (e.g. the model's tool call was truncated + // mid-JSON by the output token limit), so execute() will never run. If a + // streaming delta already opened a partial "tool" ask (partial: true), + // finalize it here or the webview spinner stays stuck indefinitely. + await task.finalizePartialToolAsk().catch((finalizeError) => { + console.error(`Error finalizing ${this.name} partial tool ask:`, finalizeError) + }) const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}` await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 9ed8f92cbf..fca6630702 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -704,6 +704,40 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) }) + it("finalizes any open partial tool ask when final args cannot be parsed", async () => { + // Regression test: a write_to_file block whose final args fail to parse (e.g. the + // tool call was truncated mid-JSON by the output token limit) never reaches + // execute(). A streaming delta for that block may already have opened a partial + // `tool` ask (partial: true) -- BaseTool.handle must finalize it, otherwise the + // UI spinner stays stuck even though the parse error bubble was shown. + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, partial ask issued once + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The parse error is still reported, and the open partial ask is finalized first. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + }) + it("reports a filesystem error only once across the streaming and execute phases", async () => { // Regression test for the double-error UX defect: a single write_to_file call to a // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") From c47e3c7f9c780c2652466b5a305b5c329de07b7d Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sat, 29 Aug 2026 15:16:40 +0800 Subject: [PATCH 11/17] fix(write-to-file): address review findings on partial-stream cleanup - Stamp isAnswered when finalizing a partial tool ask so ChatView stops arming Save/Reject for a write that already failed - Revert the diff document before reset() on unapproved failure/denial paths so a user save cannot persist streamed content that was never approved - Consolidate the per-task stream state (failure mark, path stabilization, abort listener) into one keyed object per task so reset cannot clear a subset and leak the rest - Strengthen finalize assertions to the exact streamed payload (expect.any(String) let a relPath mutant through) and pin the no-text finalize args on the error/denial paths - Add regression tests: revert-before-reset ordering on the failure/denial paths, and no-revert after approval when saving fails late --- src/core/task/Task.ts | 5 + src/core/task/__tests__/Task.spec.ts | 9 ++ src/core/tools/WriteToFileTool.ts | 143 ++++++++++++------ .../tools/__tests__/writeToFileTool.spec.ts | 135 +++++++++++++++-- 4 files changed, 232 insertions(+), 60 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index e1c12dcbc0..954db300eb 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1842,6 +1842,10 @@ export class Task extends EventEmitter implements TaskLike { * Any in-progress `progressStatus` on the message is cleared as well: the ask is being * finalized because it will NOT complete, so a stale "in progress" indicator would be * misleading (the normal completion path overwrites it with the final status instead). + * + * `isAnswered` is stamped true because the ask is resolved by the system rather than + * by the user: ChatView only shows ask buttons for unanswered messages, so leaving it + * unset would keep Save/Reject armed for a write that already failed. */ async finalizePartialToolAsk(text?: string): Promise { const partialToolAsk = findLast( @@ -1859,6 +1863,7 @@ export class Task extends EventEmitter implements TaskLike { partialToolAsk.partial = false partialToolAsk.progressStatus = undefined + partialToolAsk.isAnswered = true await this.saveClineMessages() await this.updateClineMessage(partialToolAsk).catch((error) => { console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index b13b71074c..5d414ef363 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3388,10 +3388,14 @@ describe("Cline", () => { expect(partialToolAsk.partial).toBe(false) expect(partialToolAsk.progressStatus).toBeUndefined() + // The ask is resolved by the system, not the user: stamp isAnswered so ChatView + // does not keep Save/Reject armed for a write that already failed. + expect(task.clineMessages[0].isAnswered).toBe(true) expect(saveSpy).toHaveBeenCalled() expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) expect(updateSnapshot?.partial).toBe(false) expect(updateSnapshot?.progressStatus).toBeUndefined() + expect(updateSnapshot?.isAnswered).toBe(true) updateSpy.mockRestore() saveSpy.mockRestore() @@ -3422,6 +3426,7 @@ describe("Cline", () => { await flushMicrotasks() expect(task.clineMessages[0].partial).toBe(true) + expect(task.clineMessages[0].isAnswered).toBeUndefined() expect(saveSpy).not.toHaveBeenCalled() expect(updateSpy).not.toHaveBeenCalled() @@ -3464,7 +3469,10 @@ describe("Cline", () => { await flushMicrotasks() expect(olderPartialToolAsk.partial).toBe(true) + expect(task.clineMessages[0].isAnswered).toBeUndefined() expect(latestPartialToolAsk.partial).toBe(false) + // Only the finalized ask is stamped answered; the untouched one is not. + expect(task.clineMessages[1].isAnswered).toBe(true) expect(saveSpy).toHaveBeenCalled() expect(updateSpy).toHaveBeenCalledWith(latestPartialToolAsk) @@ -3506,6 +3514,7 @@ describe("Cline", () => { await flushMicrotasks() expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) expect(saveSpy).toHaveBeenCalled() expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) expect(consoleErrorSpy).toHaveBeenCalledWith( diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 44b8fb65fc..181c2b0c47 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -23,12 +23,29 @@ interface WriteToFileParams { content: string } +/** + * Per-task partial-streaming state tracked by WriteToFileTool. + */ +interface TaskPartialStreamState { + /** Last path seen during streaming; undefined until the first delta. */ + lastSeenPartialPath: string | undefined + /** True once a streaming delta hit a fatal filesystem error. */ + streamFailed: boolean + /** The task that owns this state; target for abort-listener deregistration. */ + task: Task + /** TaskAborted listener that tears this state down; registered once per task. */ + abortCleanup: () => void +} + export class WriteToFileTool extends BaseTool<"write_to_file"> { readonly name = "write_to_file" as const /** - * Tracks filesystem failures from diff-view streaming, keyed by task id (taskId + - * instanceId). + * Per-task partial-streaming state, keyed by task id (taskId + instanceId). + * + * All per-task fields live in one object per task so that resetTaskPartialState() / + * resetPartialState() cannot clear a subset of them and leak the rest (abort + * listener, failure mark, path-stabilization entry) for an abandoned stream. * * This deliberately diverges from the sibling streaming tools (ApplyDiffTool, * EditFileTool, SearchReplaceTool, EditTool), which rely on BaseTool's singleton @@ -52,54 +69,48 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * Lifting this per-task keying into BaseTool for all streaming tools is a follow-up * (separate PR); it is deliberately not done here. */ - private partialStreamFailuresByTaskId = new Set() - - /** - * Tracks partial path stabilization, keyed by task id (taskId + instanceId), so one - * task's streaming deltas cannot stabilize another task's path. Keyed per task for the - * same cross-provider reason as partialStreamFailuresByTaskId (see that note; the - * divergence from the sibling tools' BaseTool state is deliberate). - */ - private lastSeenPartialPathByTaskId = new Map() - - /** - * Tracks abort cleanup listeners for per-task partial state so normal execute() - * finalization can unregister them and abandoned streams are torn down on abort. - */ - private partialStateAbortCleanupByTaskId = new Map void }>() + private taskPartialStreamState = new Map() private getPartialStreamFailureKey(task: Task): string { return `${task.taskId}.${task.instanceId}` } - private registerTaskPartialStateCleanup(task: Task): void { + /** + * Get this task's partial stream state, creating it on first use and registering the + * TaskAborted teardown listener exactly once per task. + */ + private getTaskPartialStreamState(task: Task): TaskPartialStreamState { const key = this.getPartialStreamFailureKey(task) - if (this.partialStateAbortCleanupByTaskId.has(key)) { - return + const existing = this.taskPartialStreamState.get(key) + if (existing) { + return existing } - const cleanup = () => this.resetTaskPartialState(task) - this.partialStateAbortCleanupByTaskId.set(key, { task, cleanup }) - task.once(RooCodeEventName.TaskAborted, cleanup) + const state: TaskPartialStreamState = { + lastSeenPartialPath: undefined, + streamFailed: false, + task, + abortCleanup: () => this.resetTaskPartialState(task), + } + this.taskPartialStreamState.set(key, state) + task.once(RooCodeEventName.TaskAborted, state.abortCleanup) + return state } - private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean { - const key = this.getPartialStreamFailureKey(task) - const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key) - const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath - this.lastSeenPartialPathByTaskId.set(key, partialPath) + private hasPathStabilizedForTask(state: TaskPartialStreamState, partialPath: string | undefined): boolean { + const pathHasStabilized = state.lastSeenPartialPath !== undefined && state.lastSeenPartialPath === partialPath + state.lastSeenPartialPath = partialPath return pathHasStabilized && !!partialPath } private resetTaskPartialState(task: Task): void { const key = this.getPartialStreamFailureKey(task) - const abortCleanup = this.partialStateAbortCleanupByTaskId.get(key) - if (abortCleanup) { - task.off(RooCodeEventName.TaskAborted, abortCleanup.cleanup) - this.partialStateAbortCleanupByTaskId.delete(key) + const state = this.taskPartialStreamState.get(key) + if (!state) { + return } - this.lastSeenPartialPathByTaskId.delete(key) - this.partialStreamFailuresByTaskId.delete(key) + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + this.taskPartialStreamState.delete(key) } private async resetDiffViewAfterWrite(task: Task): Promise { @@ -108,6 +119,22 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + /** + * Restore the diff editor document to its pre-streaming state and close the view. + * + * reset() clears the provider's state but leaves the diff document dirty with the + * streamed content; a user save would then persist a write the task never completed + * (denied or failed before approval). Must run BEFORE resetDiffViewAfterWrite(), + * since reset() clears the state revertChanges() relies on. No-op when no diff view + * is open. Failures are logged and swallowed so the remaining cleanup (reset, + * per-task state teardown) always continues. + */ + private async revertDiffChangesBeforeReset(task: Task): Promise { + await task.diffViewProvider.revertChanges().catch((revertError) => { + console.error("Error reverting write_to_file diff view changes:", revertError) + }) + } + private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { await task.finalizePartialToolAsk(text).catch((finalizeError) => { console.error("Error finalizing write_to_file partial tool ask:", finalizeError) @@ -116,12 +143,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { override resetPartialState(): void { super.resetPartialState() - for (const { task, cleanup } of this.partialStateAbortCleanupByTaskId.values()) { - task.off(RooCodeEventName.TaskAborted, cleanup) + for (const state of this.taskPartialStreamState.values()) { + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) } - this.partialStreamFailuresByTaskId.clear() - this.lastSeenPartialPathByTaskId.clear() - this.partialStateAbortCleanupByTaskId.clear() + this.taskPartialStreamState.clear() } async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { @@ -133,6 +158,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) + await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) this.resetTaskPartialState(task) return @@ -142,6 +168,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) this.resetTaskPartialState(task) return @@ -155,10 +182,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // handlePartial() has no rooignore guard, so streaming deltas for this denied // path may already have created a partial `tool` ask (partial: true) and opened // the diff view before execute() reached the access check. Denying here without - // cleanup would leave the UI spinner stuck (partial: true), the diff view open, - // and this task's abort listener / path-stabilization / failure-map entries - // leaked. Perform the same cleanup the try/finally path does before returning. + // cleanup would leave the UI spinner stuck (partial: true), the diff view open + // with the denied content still dirty in the editor, and this task's per-task + // stream state leaked. Perform the same cleanup the try/finally path does + // before returning. await this.finalizePartialToolAskAfterFailure(task) + // The write was denied before approval: restore the document so a user save + // cannot persist the streamed content. + await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) this.resetTaskPartialState(task) return @@ -199,6 +230,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } + // Tracks whether the user approved the write, so the error path only reverts the + // diff document when the content was never approved (an approved edit is kept in + // the editor so the user can save it manually after a late failure). + let writeApproved = false + try { // Create parent directories for new files inside the try block so filesystem // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup @@ -243,6 +279,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) } else { if (!task.diffViewProvider.isEditing) { @@ -276,6 +314,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } @@ -301,6 +341,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // after the error bubble appears. await this.finalizePartialToolAskAfterFailure(task) await handleError("writing file", error as Error) + // Before approval the diff document holds unapproved streamed content: restore it + // so a user save cannot persist it. After approval the content is the user's + // accepted edit -- keep it in the editor (dirty) so they can save it manually. + if (!writeApproved) { + await this.revertDiffChangesBeforeReset(task) + } await this.resetDiffViewAfterWrite(task) return } finally { @@ -317,14 +363,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // A prior streaming delta for this task already hit a fatal filesystem error. // Skip further streaming work so we don't create a new partial tool message on every // subsequent delta. execute() will report the error once when the block completes. - if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) { + if (this.taskPartialStreamState.get(partialStreamFailureKey)?.streamFailed) { return } - this.registerTaskPartialStateCleanup(task) + // Get (or create) this task's state; registers the TaskAborted teardown listener + // once, so abandoned streams are torn down even if execute() never runs. + const partialStreamState = this.getTaskPartialStreamState(task) // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) { + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { return } @@ -386,8 +434,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { console.error(`Error streaming write_to_file diff view:`, error) // Mark the stream as failed so later deltas don't re-attempt and spawn a new // partial tool message each time. - this.partialStreamFailuresByTaskId.add(partialStreamFailureKey) + partialStreamState.streamFailed = true await this.finalizePartialToolAskAfterFailure(task, partialMessage) + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty). + await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index fca6630702..e726825009 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -97,6 +97,19 @@ describe("writeToFileTool", () => { const testContent = "Line 1\nLine 2\nLine 3" const testContentWithMarkdown = "```javascript\nLine 1\nLine 2\n```" + // The exact payload handlePartial() streams as the partial `tool` ask for the default + // test scenario (new file, readable path, in-workspace, not write-protected). + // finalizePartialToolAsk() no-ops on a text mismatch, so finalize assertions must + // match this exactly: a weaker matcher (e.g. expect.any(String), which a relPath also + // satisfies) would pass a mutant that passes the wrong text and leaves the spinner stuck. + const expectedPartialToolMessage = JSON.stringify({ + tool: "newFileCreated", + path: "test/path.txt", + content: testContent, + isOutsideWorkspace: false, + isProtected: false, + }) + // Mocked functions with correct types const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction @@ -264,8 +277,9 @@ describe("writeToFileTool", () => { // handlePartial() has no rooignore guard, so streaming deltas for a denied path // still create a partial `tool` ask (partial: true) and open the diff view before // execute() reaches the access check. The denial must clean up all of that: - // finalize the partial ask (spinner does not stick), reset the diff view (reset - // failures swallowed), and clear the per-task state (abort listener + maps). + // finalize the partial ask (spinner does not stick), revert the diff document so a + // user save cannot persist the denied content, reset the diff view (reset failures + // swallowed), and clear the per-task stream state (abort listener + entries). const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) try { let abortCleanup: (() => void) | undefined @@ -275,7 +289,16 @@ describe("writeToFileTool", () => { } return mockCline }) - mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + // Record the relative order of revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + throw new Error("reset failed") + }) // Stream two deltas so the path stabilizes: handlePartial registers the abort // cleanup and opens the partial ask + diff view for the (soon denied) path. @@ -289,8 +312,11 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, accessAllowed: false }) expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The denial finalizes without a text match: any open partial tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The denied write's streamed content must be reverted from the diff document + // BEFORE reset() clears the state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( "Error resetting write_to_file diff view:", @@ -649,6 +675,15 @@ describe("writeToFileTool", () => { mockCline.diffViewProvider.open.mockRejectedValue( Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) // First call - path not yet stabilized await executeWriteFileTool({}, { isPartial: true }) @@ -657,8 +692,12 @@ describe("writeToFileTool", () => { // Second call - path stabilized, open() rejects await executeWriteFileTool({}, { isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) @@ -667,6 +706,15 @@ describe("writeToFileTool", () => { mockCline.diffViewProvider.update.mockRejectedValue( Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) // First call - path not yet stabilized await executeWriteFileTool({}, { isPartial: true }) @@ -674,8 +722,12 @@ describe("writeToFileTool", () => { // Second call - path stabilized, update() rejects await executeWriteFileTool({}, { isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) @@ -771,6 +823,53 @@ describe("writeToFileTool", () => { expect(mockCline.consecutiveMistakeCount).toBe(3) }) + it("reverts the diff document when the write fails before approval", async () => { + // Regression test for the dirty-diff leak: streaming already opened the diff view + // with unapproved content, and the write then failed before the user could approve + // it. reset() alone left the diff document dirty with the streamed content -- a + // user save in the editor would persist a write the task never completed. The + // error path must revert the document (like the approval-denied path does) before + // resetting the provider state. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Stream two deltas so the diff view is open with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + }) + + it("keeps approved diff content in the editor when saving fails after approval", async () => { + // The reverse of the previous test: once the user approved the write, the diff + // content is their accepted edit. A late failure (e.g. saveChanges rejecting) + // must NOT revert it -- the document stays dirty so the user can save it manually. + // Restore the factory default for directory creation: the previous test left the + // mock rejecting, and vi.clearAllMocks() keeps the last implementation. + mockedCreateDirectoriesForFile.mockResolvedValue([]) + mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + it("continues execute error cleanup when finalizing partial ask fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) try { @@ -781,8 +880,11 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + // The execute error path finalizes without a text match: any open partial + // tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( "Error finalizing write_to_file partial tool ask:", @@ -828,7 +930,8 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expect.any(String)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( @@ -851,7 +954,8 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( @@ -925,8 +1029,11 @@ describe("writeToFileTool", () => { // handleError must still be called expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - // finalizePartialToolAsk must have been called to dismiss the spinner - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + // finalizePartialToolAsk must have been called (no text: the execute error + // path closes whichever partial tool ask is open) to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The write was never approved, so the diff document is reverted before reset + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) }, ) }) From d7f8038c8ce0e2162f774aaf90483475db935fa4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 3 Sep 2026 01:40:49 +0800 Subject: [PATCH 12/17] test(write-to-file): cover partial-ask finalize rejection, revert failure, and prevent-focus-disruption branch --- .../tools/__tests__/writeToFileTool.spec.ts | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index e726825009..03addf58e9 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -167,6 +167,7 @@ describe("writeToFileTool", () => { update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", userEdits: null, @@ -790,6 +791,42 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) }) + it("continues parse failure cleanup when finalizing the partial ask fails", async () => { + // Pins the .catch arm on task.finalizePartialToolAsk() in BaseTool.handle(): when the + // final args cannot be parsed and finalizing the open partial ask also fails, the + // failure must only be logged so the parse error is still reported to the user. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + // The parse error is still reported despite the failed finalization. + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + } finally { + consoleErrorSpy.mockRestore() + } + }) + it("reports a filesystem error only once across the streaming and execute phases", async () => { // Regression test for the double-error UX defect: a single write_to_file call to a // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") @@ -853,6 +890,36 @@ describe("writeToFileTool", () => { expect(diffViewCallOrder).toEqual(["revert", "reset"]) }) + it("continues cleanup when reverting the diff document fails before approval", async () => { + // Pins the .catch arm on revertChanges() in revertDiffChangesBeforeReset(): a failed + // revert (e.g. the diff view was already closed) must only be logged so the + // remaining cleanup (diff view reset + per-task state teardown) always completes. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + + // Stream two deltas so the diff view opens with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval and the revert fails too. + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error reverting write_to_file diff view changes:", + expect.any(Error), + ) + // The diff view is still reset and the per-task stream state still torn down. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + } finally { + consoleErrorSpy.mockRestore() + } + }) + it("keeps approved diff content in the editor when saving fails after approval", async () => { // The reverse of the previous test: once the user approved the write, the diff // content is their accepted edit. A late failure (e.g. saveChanges rejecting) @@ -1037,4 +1104,83 @@ describe("writeToFileTool", () => { }, ) }) + + describe("prevent focus disruption experiment", () => { + /** + * Enable the PREVENT_FOCUS_DISRUPTION experiment for the current task: the experiment + * branches in execute()/handlePartial() read it from the provider state they fetch. + */ + function enablePreventFocusDisruption(): void { + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + } + + beforeEach(() => { + // The tests before this describe leave the directory-creation mock rejecting; + // restore the factory default so the experiment branch runs to completion. + mockedCreateDirectoriesForFile.mockResolvedValue([]) + }) + + it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + expect(toolResult).toBe("Tool result message") + }) + + it("keeps approved diff content when saveDirectly fails after approval", async () => { + // The experiment branch stamps writeApproved before saveDirectly, so a late failure + // must NOT revert the document (the user approved the edit and can save it + // manually) but must still finalize the partial ask and reset the diff view. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + enablePreventFocusDisruption() + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("skips streaming diff view work when the experiment is enabled", async () => { + // With the experiment enabled the tool preview is embedded in the complete message + // built in execute(), so handlePartial must not open or update the diff view while + // streaming. + enablePreventFocusDisruption() + + // Delta 1 - stabilize path; delta 2 - path stabilized but the experiment short-circuits + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + }) }) From f8b6b357c8e8fdf63c35f4a58a783f1f21e65f3a Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 4 Sep 2026 10:21:04 +0800 Subject: [PATCH 13/17] fix(mutation): match direct spec files case-insensitively Spec files follow the lowerCamel source-name convention (writeToFileTool.spec.ts for WriteToFileTool.ts), but preferDirectTestFiles compared names case-sensitively. Any PR touching several sources with mixed-case spec names silently collapsed the related-test set to the direct matches, leaving the rest of the touched code as phantom NoCoverage mutants. Match both sides lowercased and cover the convention in the gate's own unit tests. --- scripts/stryker-diff.mjs | 6 ++++-- scripts/stryker-diff.test.mjs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index c0e8a6cd1a..7eb40d9585 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -293,12 +293,14 @@ export function parseVitestTestFiles(report, runRoot) { } export function preferDirectTestFiles(testFiles, sourceFiles) { + // Spec files follow the lowerCamel source-name convention (e.g. + // writeToFileTool.spec.ts for WriteToFileTool.ts), so match case-insensitively. const sourceNames = sourceFiles.map((sourceFile) => path.posix.basename(sourceFile, path.posix.extname(sourceFile))) const direct = testFiles.filter((testFile) => { - const testName = path.posix.basename(testFile) + const testName = path.posix.basename(testFile).toLowerCase() return sourceNames.some( (sourceName) => - testName.startsWith(`${sourceName}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), + testName.startsWith(`${sourceName.toLowerCase()}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), ) }) return direct.length > 0 ? direct : testFiles diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index 0f39dc507f..0792ec5a1c 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -208,6 +208,20 @@ describe("preferDirectTestFiles", () => { ]) assert.deepEqual(preferDirectTestFiles(related, ["webview-ui/src/utils/unmatched.ts"]), related) }) + + it("matches lowerCamel spec names against PascalCase sources case-insensitively", () => { + const related = [ + "src/core/tools/__tests__/writeToFileTool.spec.ts", + "src/core/task/__tests__/Task.spec.ts", + "src/core/tools/__tests__/presentAssistantMessage-custom-tool.spec.ts", + ] + assert.deepEqual( + preferDirectTestFiles(related, ["src/core/tools/WriteToFileTool.ts", "src/core/task/Task.ts"]), + ["src/core/tools/__tests__/writeToFileTool.spec.ts", "src/core/task/__tests__/Task.spec.ts"], + ) + // No source with a matching spec name: fall back to all related tests. + assert.deepEqual(preferDirectTestFiles(related, ["src/core/tools/ReadFileTool.ts"]), related) + }) }) describe("related-test discovery", () => { From 5d320011a0ba05be22ded1ce9fea833189291854 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 4 Sep 2026 10:21:25 +0800 Subject: [PATCH 14/17] test(task): cover finalizePartialToolAsk predicate clause mismatches The findLast predicate's partial, type, ask, and text clauses were only ever exercised by messages that matched every clause, so a single mutated clause (or a wrong combination) survived mutation testing. Seed the message list with distractors that each satisfy only a strict subset of the clauses and assert that only the genuine partial tool ask is finalized. --- src/core/task/__tests__/Task.spec.ts | 78 ++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 2e042de171..d988d90071 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -10,6 +10,7 @@ import type { Mock } from "vitest" import { providerIdentifiers, RooCodeEventName, + type ClineMessage, type GlobalState, type ProviderSettings, type ModelInfo, @@ -3816,6 +3817,83 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk ignores partial asks that match only some predicate clauses", async () => { + // Each distractor below satisfies a strict subset of the findLast predicate + // clauses, so no single clause (or a wrong combination of clauses) may select + // it: partial, type, ask kind, and text must all hold together. + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk: ClineMessage = { + ts: Date.now() - 4, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + // Completed (non-partial) tool ask with the same text. + const completedToolAsk: ClineMessage = { + ts: Date.now() - 3, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: false, + } + // Partial ask of a different kind. + const nonToolAsk: ClineMessage = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "completion_result" as const, + text: "partial tool message", + partial: true, + } + // Deliberately malformed: a "say" message that still carries the tool ask + // fields. The ClineMessage schema allows both fields, and the predicate under + // test reads message.ask on any message, so this is exactly the distractor + // the type clause exists to filter out. + const sayWithToolAsk: ClineMessage = { + ts: Date.now() - 1, + type: "say" as const, + say: "error" as const, + text: "partial tool message", + partial: true, + ask: "tool" as const, + } + + task.clineMessages.push(partialToolAsk) + task.clineMessages.push(completedToolAsk) + task.clineMessages.push(nonToolAsk) + task.clineMessages.push(sayWithToolAsk) + + await task.finalizePartialToolAsk("partial tool message") + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(partialToolAsk.isAnswered).toBe(true) + // Every distractor stays untouched: only the genuine partial tool ask is + // finalized. + expect(completedToolAsk.partial).toBe(false) + expect(completedToolAsk.isAnswered).toBeUndefined() + expect(nonToolAsk.partial).toBe(true) + expect(nonToolAsk.isAnswered).toBeUndefined() + expect(sayWithToolAsk.partial).toBe(true) + expect(saveSpy).toHaveBeenCalledTimes(1) + expect(updateSpy).toHaveBeenCalledTimes(1) + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { // Pins the .catch arm on the fire-and-forget updateClineMessage call // in ask() when a new partial ask arrives while the previous partial From 26dda51af4e97cf9b6518e32d127472c515cc208 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 4 Sep 2026 10:31:56 +0800 Subject: [PATCH 15/17] test(tools): cover write_to_file stabilization and cleanup branches The mutation gate flagged 8 surviving mutants in WriteToFileTool: the hasPathStabilizedForTask predicate clauses were only observable indirectly (an undefined path hits the same early return either way), the changed-path/content-undefined/isEditing-open/short-circuit branches of handlePartial were never asserted, and resetPartialState() had no observable effect in any test. Add predicate-level tests for the clause the return guard makes unobservable (documented with a Stryker disable directive), handlePartial branch tests, a context assertion on the streaming-failure log, and a resetPartialState test that pins the base-class reset, abort-listener detachment, and per-task map clear. The disable directive covers the redundant '!== undefined' clause: when lastSeenPartialPath is undefined the second clause only matches an undefined partialPath, which the '!!partialPath' return guard rejects either way, so no test can distinguish the two. --- src/core/tools/WriteToFileTool.ts | 3 + .../tools/__tests__/writeToFileTool.spec.ts | 119 ++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 181c2b0c47..e213fae125 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -98,6 +98,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } private hasPathStabilizedForTask(state: TaskPartialStreamState, partialPath: string | undefined): boolean { + // Stryker disable next-line ConditionalExpression: the `!== undefined` clause is redundant: when + // lastSeenPartialPath is undefined, the second clause only matches an undefined partialPath, which + // the `!!partialPath` in the return value rejects either way -- no test can distinguish the two. const pathHasStabilized = state.lastSeenPartialPath !== undefined && state.lastSeenPartialPath === partialPath state.lastSeenPartialPath = partialPath return pathHasStabilized && !!partialPath diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 03addf58e9..5e00fcb0ee 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -555,6 +555,125 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(2) }) + + it("does not treat a changed path between deltas as stabilized", async () => { + // Delta 1 streams "alpha.txt"; delta 2 streams "beta.txt" for the same task. The path changed + // between deltas, so it must not count as stabilized and no partial `tool` ask may be issued for + // the still-changing second path. + await executeWriteFileTool({ path: "alpha.txt" }, { isPartial: true }) + await executeWriteFileTool({ path: "beta.txt" }, { isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + }) + + it("does not issue a partial ask when content is undefined after path stabilization", async () => { + // Delta 1 stabilizes the path. Delta 2 repeats it but carries no content yet: the + // `newContent === undefined` clause must short-circuit the ask even though the path itself has + // stabilized. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ content: undefined }, { isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("does not reopen an already open diff view during streaming", async () => { + // The diff view is already open for this task (isEditing). A stabilized delta must still update + // the streamed content but must not call open() again -- reopening would discard the view's + // current state. + mockCline.diffViewProvider.isEditing = true + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) + }) + + it("logs the streaming diff view failure with the write_to_file context", async () => { + // The catch arm logs a context-specific message before swallowing the error (execute() reports + // the authoritative one). The message must keep the write_to_file context so the log is + // actionable. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: permission denied")) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error streaming write_to_file diff view:", + expect.anything(), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + }) + + describe("path stabilization predicate", () => { + // The predicate is exercised directly (it is private) because not all of its branches are + // observable through handlePartial(): an undefined path reaches the same early return either + // way, so the clause-by-clause behavior must be pinned at the predicate level. + function makeState(lastSeenPartialPath: string | undefined) { + return { + lastSeenPartialPath, + streamFailed: false, + task: mockCline, + abortCleanup: () => {}, + } + } + + it("reports a first delta as not stabilized and records the seen path", () => { + const state = makeState(undefined) + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "a.txt")).toBe(false) + expect(state.lastSeenPartialPath).toBe("a.txt") + }) + + it("reports a repeated path as stabilized", () => { + const state = makeState("a.txt") + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "a.txt")).toBe(true) + }) + + it("reports a changed path as not stabilized", () => { + const state = makeState("a.txt") + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "b.txt")).toBe(false) + expect(state.lastSeenPartialPath).toBe("b.txt") + }) + }) + + describe("resetPartialState", () => { + it("resets the base partial path and detaches every task's abort listener", async () => { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + + // Seed one per-task state with an abort listener attached. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(abortCleanup).toBeTypeOf("function") + + // The base-class singleton field is reset by super.resetPartialState(). + writeToFileTool["lastSeenPartialPath"] = "stale-path" + writeToFileTool.resetPartialState() + + expect(writeToFileTool["lastSeenPartialPath"]).toBeUndefined() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + + // The per-task map was cleared too: a fresh delta sequence starts un-stabilized, so no + // second partial ask is issued. + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + }) }) describe("user interaction", () => { From 4fd1a9fd8b72eebae18342636ab024cf1028ad38 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 4 Sep 2026 11:26:40 +0800 Subject: [PATCH 16/17] fix(core): address CodeRabbit review findings on write_to_file and task persistence - WriteToFileTool: finalize the open partial tool ask in both missing-parameter early-return branches so the UI spinner does not stay stuck, mirroring the rooignore and execute-error cleanups; add regression tests covering the partial-ask precondition for both branches. - Task.finalizePartialToolAsk: check the saveClineMessages() result and, on persistence failure, log and skip the webview-only update so the on-disk record (still partial: true) and the webview do not diverge until the next save repairs it; add regression test. - writeToFileTool spec: wrap the console.error spy in try/finally, reset the createDirectoriesForFile factory default in the shared beforeEach (vi.clearAllMocks keeps the last implementation), document the required nativeArgs casts, and un-skip the three win32-gated regression tests, which pass on Windows with the filesystem mocked. - Task.throttle spec: remove the console.log spy, which masked a Vitest worker-teardown race under --coverage rather than a real task rejection (19/19 pass without it, with and without coverage). --- src/core/task/Task.ts | 11 +- src/core/task/__tests__/Task.spec.ts | 47 +++++ src/core/task/__tests__/Task.throttle.test.ts | 5 +- src/core/tools/WriteToFileTool.ts | 8 + .../tools/__tests__/writeToFileTool.spec.ts | 189 +++++++++++------- 5 files changed, 179 insertions(+), 81 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 6e499b7744..b43e2ba6c1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1990,7 +1990,16 @@ export class Task extends EventEmitter implements TaskLike { partialToolAsk.partial = false partialToolAsk.progressStatus = undefined partialToolAsk.isAnswered = true - await this.saveClineMessages() + const saved = await this.saveClineMessages() + if (!saved) { + // The persistence write failed: the on-disk record still carries `partial: true` + // while the in-memory message is finalized. Skip the webview-only update so the + // two views do not diverge (a later state resync or restart reload would flip the + // spinner back on from the stale disk record). The next saveClineMessages() call + // re-persists the full message array and repairs the disk record. + console.error("[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update") + return + } await this.updateClineMessage(partialToolAsk).catch((error) => { console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error) }) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d988d90071..a6c8851315 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3817,6 +3817,53 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk logs and skips the webview update when persistence fails", async () => { + // Pins the saveClineMessages-failure guard in finalizePartialToolAsk: while the + // on-disk record still carries partial: true, a webview-only update would diverge + // the two views (a later state resync or restart reload would flip the spinner + // back on). finalize must log, skip updateClineMessage, and still resolve so + // callers' error-path cleanup completes. + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(false) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + // The in-memory message is still finalized so the ask is resolved by the system... + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // ...and the persistence failure is observed instead of silently swallowed. + expect(saveSpy).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update", + ) + // ...but the webview update is skipped so disk (still partial: true) and + // webview do not diverge until the next save repairs the record. + expect(updateSpy).not.toHaveBeenCalled() + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("finalizePartialToolAsk ignores partial asks that match only some predicate clauses", async () => { // Each distractor below satisfies a strict subset of the findLast predicate // clauses, so no single clause (or a wrong combination of clauses) may select diff --git a/src/core/task/__tests__/Task.throttle.test.ts b/src/core/task/__tests__/Task.throttle.test.ts index a022f15114..989af6d2f1 100644 --- a/src/core/task/__tests__/Task.throttle.test.ts +++ b/src/core/task/__tests__/Task.throttle.test.ts @@ -65,12 +65,12 @@ describe("Task token usage throttling", () => { let mockProvider: any let mockApiConfiguration: ProviderSettings let task: Task - let consoleLogSpy: ReturnType beforeEach(() => { // Reset all mocks vi.clearAllMocks() - consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + // console.log is intentionally not spied: the previous spy masked a Vitest + // worker-teardown race under --coverage, not a real task rejection. vi.useFakeTimers() // Mock provider @@ -106,7 +106,6 @@ describe("Task token usage throttling", () => { if (task && !task.abort) { task.dispose() } - consoleLogSpy.mockRestore() }) test("should emit TaskTokenUsageUpdated immediately on first change", async () => { diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index e213fae125..de860183e0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -161,6 +161,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) + // handlePartial() has no missing-parameter guard, so streaming deltas for a + // stabilized path may already have created a partial `tool` ask (partial: true) + // before execute() saw the malformed payload. Finalize it so the UI spinner + // does not stay stuck, mirroring the rooignore and execute-error cleanups. + await this.finalizePartialToolAskAfterFailure(task) await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) this.resetTaskPartialState(task) @@ -171,6 +176,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + // Same partial-ask cleanup as the missing-`path` branch above: a partial `tool` + // ask created during streaming would otherwise stay open (partial: true). + await this.finalizePartialToolAskAfterFailure(task) await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) this.resetTaskPartialState(task) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 5e00fcb0ee..fb1528994f 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -132,6 +132,9 @@ describe("writeToFileTool", () => { mockedPathResolve.mockReturnValue(absoluteFilePath) mockedFileExistsAtPath.mockResolvedValue(false) + // vi.clearAllMocks() keeps the last mock implementation; reset the factory default here + // so no test depends on declaration order or an earlier test's rejection. + mockedCreateDirectoriesForFile.mockResolvedValue([]) mockedIsPathOutsideWorkspace.mockReturnValue(false) mockedGetReadablePath.mockReturnValue("test/path.txt") mockedUnescapeHtmlEntities.mockImplementation((content) => { @@ -245,6 +248,9 @@ describe("writeToFileTool", () => { ...params, }, nativeArgs: { + // The missing-parameter tests inject `undefined` where + // NativeToolArgs["write_to_file"] declares `string`, so the casts are required to + // model a malformed payload. path: (Object.prototype.hasOwnProperty.call(params, "path") ? params.path : testFilePath) as any, content: (Object.prototype.hasOwnProperty.call(params, "content") ? params.content @@ -330,6 +336,48 @@ describe("writeToFileTool", () => { }) }) + describe("missing-parameter early-return cleanup", () => { + // handlePartial() has no missing-parameter guard: two partial streaming calls + // stabilize the path and open the partial `tool` ask + diff view. This establishes + // the "partial ask is open" precondition for the missing-parameter branches below. + async function streamPartialAsk() { + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + } + + it("finalizes the partial ask when content is missing after partial streaming", async () => { + // Streaming deltas create a partial `tool` ask (partial: true), then the completed + // payload is missing `content`. The missing-parameter branch must finalize the ask + // (the spinner must not stick) and still perform the same diff-view revert / reset + // and per-task-state cleanup as the other early-return paths. + await streamPartialAsk() + + await executeWriteFileTool({ content: undefined }, { fileExists: false }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + // The missing-parameter path finalizes without a text match: any open partial ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes the partial ask when path is missing after partial streaming", async () => { + // Same scenario with the `path` field missing: the missing-`path` branch must run + // the identical partial-ask + diff-view + per-task-state cleanup. + await streamPartialAsk() + + await executeWriteFileTool({ path: undefined }, { fileExists: false }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "path") + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + }) + describe("file existence detection", () => { it.skipIf(process.platform === "win32")("detects existing file and sets editType to modify", async () => { await executeWriteFileTool({}, { fileExists: true }) @@ -474,16 +522,21 @@ describe("writeToFileTool", () => { it("does not report a successful write as failed when final diff reset rejects", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) - - await executeWriteFileTool({}, { fileExists: false }) + try { + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) - expect(mockHandleError).not.toHaveBeenCalled() - expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") - expect(mockCline.didEditFile).toBe(true) - expect(consoleErrorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + await executeWriteFileTool({}, { fileExists: false }) - consoleErrorSpy.mockRestore() + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockCline.didEditFile).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } }) }) @@ -1043,9 +1096,6 @@ describe("writeToFileTool", () => { // The reverse of the previous test: once the user approved the write, the diff // content is their accepted edit. A late failure (e.g. saveChanges rejecting) // must NOT revert it -- the document stays dirty so the user can save it manually. - // Restore the factory default for directory creation: the previous test left the - // mock rejecting, and vi.clearAllMocks() keeps the last implementation. - mockedCreateDirectoriesForFile.mockResolvedValue([]) mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) await executeWriteFileTool({}, { fileExists: false }) @@ -1153,75 +1203,66 @@ describe("writeToFileTool", () => { } }) - it.skipIf(process.platform === "win32")( - "EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", - async () => { - // Regression test: before the fix, createDirectoriesForFile was called in handlePartial - // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called - // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate - // in presentAssistantMessage was never reached and the agent loop stalled permanently. - // After the fix the call is removed entirely -- handlePartial never touches the filesystem. - mockedCreateDirectoriesForFile.mockRejectedValue( - Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), - ) + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called + // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate + // in presentAssistantMessage was never reached and the agent loop stalled permanently. + // After the fix the call is removed entirely -- handlePartial never touches the filesystem. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) - // First call -- path not yet stabilized, returns early - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockHandleError).not.toHaveBeenCalled() + // First call -- path not yet stabilized, returns early + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() - // Second call -- path stabilized; createDirectoriesForFile must NOT be called from - // handlePartial, so the mock rejection must not trigger and handleError must not be called - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - expect(mockHandleError).not.toHaveBeenCalled() - }, - ) + // Second call -- path stabilized; createDirectoriesForFile must NOT be called from + // handlePartial, so the mock rejection must not trigger and handleError must not be called + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) - it.skipIf(process.platform === "win32")( - "EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", - async () => { - // Regression test: before the fix, createDirectoriesForFile in execute() sat outside - // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. - // After the fix the call is inside the try block, so filesystem errors are caught and - // routed through handleError with proper diffViewProvider.reset() cleanup. - mockedCreateDirectoriesForFile.mockRejectedValue( - Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), - ) + it("EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", async () => { + // Regression test: before the fix, createDirectoriesForFile in execute() sat outside + // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. + // After the fix the call is inside the try block, so filesystem errors are caught and + // routed through handleError with proper diffViewProvider.reset() cleanup. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) - await executeWriteFileTool({}, { fileExists: false }) + await executeWriteFileTool({}, { fileExists: false }) - expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() - // The tool must not have proceeded to open or save - expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() - expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() - }, - ) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The tool must not have proceeded to open or save + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) - it.skipIf(process.platform === "win32")( - "finalizes partial tool message on error so the UI spinner does not get stuck", - async () => { - // Regression test: when a filesystem error is thrown in execute() the webview - // message created during handlePartial (or the early ask in execute) is stuck in - // partial: true state, showing an indefinite spinner alongside the error bubble. - // The catch block must call finalizePartialToolAsk() to close the spinner without - // blocking for user input. - mockedCreateDirectoriesForFile.mockRejectedValue( - Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), - ) + it("finalizes partial tool message on error so the UI spinner does not get stuck", async () => { + // Regression test: when a filesystem error is thrown in execute() the webview + // message created during handlePartial (or the early ask in execute) is stuck in + // partial: true state, showing an indefinite spinner alongside the error bubble. + // The catch block must call finalizePartialToolAsk() to close the spinner without + // blocking for user input. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) - await executeWriteFileTool({}, { fileExists: false }) + await executeWriteFileTool({}, { fileExists: false }) - // handleError must still be called - expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // handleError must still be called + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - // finalizePartialToolAsk must have been called (no text: the execute error - // path closes whichever partial tool ask is open) to dismiss the spinner - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) - // The write was never approved, so the diff document is reverted before reset - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) - }, - ) + // finalizePartialToolAsk must have been called (no text: the execute error + // path closes whichever partial tool ask is open) to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The write was never approved, so the diff document is reverted before reset + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + }) }) describe("prevent focus disruption experiment", () => { @@ -1241,12 +1282,6 @@ describe("writeToFileTool", () => { } } - beforeEach(() => { - // The tests before this describe leave the directory-creation mock rejecting; - // restore the factory default so the experiment branch runs to completion. - mockedCreateDirectoriesForFile.mockResolvedValue([]) - }) - it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { enablePreventFocusDisruption() From 32286439531c092d076d77bf0a7c96aff3fd0fcb Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 4 Sep 2026 21:08:34 +0800 Subject: [PATCH 17/17] fix(task): report save stages independently in saveClineMessages Split saveClineMessages so the persisted message write is reported separately from the task-metadata / task-history stages: a failure in a later stage no longer masks a successful message write, so finalizePartialToolAsk still delivers the finalized ask to the webview. Add regression tests for both save-stage failure paths (the real-fs message write and the later metadata stage), and pin the diff-view call order (revert before reset) in the writeToFile missing-parameter tests. Addresses the CodeRabbit review findings on PR 1066. --- src/core/task/Task.ts | 23 ++++- src/core/task/__tests__/Task.spec.ts | 97 +++++++++++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 28 +++++- 3 files changed, 141 insertions(+), 7 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b43e2ba6c1..27c8742f4a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1212,6 +1212,15 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Persist the message array, then refresh the derived metadata / task-history entries. + * + * The returned boolean reflects the message write only: `saveTaskMessages` failure + * leaves the on-disk record stale, so callers gating UI updates on durable state must + * skip them. Metadata / task-history stage failures are logged and swallowed — the + * message array is already persisted, and the next save recomputes and re-emits the + * metadata. + */ private async saveClineMessages(): Promise { try { await saveTaskMessages({ @@ -1219,7 +1228,12 @@ export class Task extends EventEmitter implements TaskLike { taskId: this.taskId, globalStoragePath: this.globalStoragePath, }) + } catch (error) { + console.error("Failed to save Roo messages:", error) + return false + } + try { if (this._taskApiConfigName === undefined) { await this.taskApiConfigReady } @@ -1247,11 +1261,14 @@ export class Task extends EventEmitter implements TaskLike { const provider = this.providerRef.deref() const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) - return true } catch (error) { - console.error("Failed to save Roo messages:", error) - return false + // The message array was persisted above; a metadata or task-history failure must + // not mask that write (see the method docs). The next saveClineMessages() call + // recomputes and re-emits the metadata update. + console.error("Failed to save task metadata:", error) } + + return true } private findMessageByTimestamp(ts: number): ClineMessage | undefined { diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index a6c8851315..89b410b487 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1,5 +1,6 @@ // npx vitest core/task/__tests__/Task.spec.ts +import * as fsReal from "fs" import * as os from "os" import * as path from "path" @@ -3864,6 +3865,102 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk still updates the webview when a later save stage fails", async () => { + // saveClineMessages() reports the message write separately from the metadata / + // task-history stages: the message array persisted while a later stage failed + // must still count as a successful save, so the finalized ask reaches the + // webview. A stale metadata entry is recomputed and re-emitted by the next + // saveClineMessages() call. + // saveTaskMessages() persists through safeWriteJson, which only mocks the + // fs/promises write helpers: its real fs.access gate and lockfile need the + // task directory to exist (uuid v7 is mocked to the fixed id below). + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const metadataFailure = new Error("task history stage failed") + const historySpy = vi.spyOn(mockProvider, "updateTaskHistory").mockRejectedValueOnce(metadataFailure) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // The message array persisted, so the webview update must run even though a + // later save stage failed... + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + // ...and the later-stage failure is observed instead of silently swallowed. + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to save task metadata:", expect.any(Error)) + + updateSpy.mockRestore() + historySpy.mockRestore() + }) + + it("finalizePartialToolAsk skips the webview update when the message write itself fails", async () => { + // Complements the later-stage-failure test above by failing the first save + // stage: with the real task directory removed, safeWriteJson's fs.access + // throws before anything is persisted, saveClineMessages() reports the + // failed message write, and the skip guard keeps the webview update off. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.rmSync(taskDir, { recursive: true, force: true }) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + // The in-memory ask is still finalized... (the flags are set before saving) + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // ...but the failed message write skips the webview update and surfaces + // both failure logs instead of updating on an unpersisted save. + expect(updateSpy).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to save Roo messages:", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update", + ) + + // Restore the directory for sibling tests that persist through the real fs. + fsReal.mkdirSync(taskDir, { recursive: true }) + + updateSpy.mockRestore() + }) + it("finalizePartialToolAsk ignores partial asks that match only some predicate clauses", async () => { // Each distractor below satisfies a strict subset of the findLast predicate // clauses, so no single clause (or a wrong combination of clauses) may select diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index fb1528994f..b1e40797cb 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -351,6 +351,15 @@ describe("writeToFileTool", () => { // payload is missing `content`. The missing-parameter branch must finalize the ask // (the spinner must not stick) and still perform the same diff-view revert / reset // and per-task-state cleanup as the other early-return paths. + // Record the relative order of revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) await streamPartialAsk() await executeWriteFileTool({ content: undefined }, { fileExists: false }) @@ -358,22 +367,33 @@ describe("writeToFileTool", () => { expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") // The missing-parameter path finalizes without a text match: any open partial ask is closed. expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The streamed content of the failed write must be reverted from the diff + // document before reset() clears the state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) it("finalizes the partial ask when path is missing after partial streaming", async () => { // Same scenario with the `path` field missing: the missing-`path` branch must run // the identical partial-ask + diff-view + per-task-state cleanup. + // Record the relative order of revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) await streamPartialAsk() await executeWriteFileTool({ path: undefined }, { fileExists: false }) expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "path") expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The diff document must be reverted before reset() clears the state + // revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) })