From 0575a3544d12fb89e68dc9a29811db3dfa633a38 Mon Sep 17 00:00:00 2001 From: awschmeder Date: Thu, 25 Jun 2026 21:58:46 -0700 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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")(