From cf5abe64d6e8eebde4181d49a3afde4705441b5c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:24 +0800 Subject: [PATCH 01/18] fix(task): stage-independent saveClineMessages + finalize open partial tool ask --- ...resentAssistantMessage-custom-tool.spec.ts | 1 + src/core/task/Task.ts | 80 +++- src/core/task/__tests__/Task.spec.ts | 400 ++++++++++++++++++ 3 files changed, 476 insertions(+), 5 deletions(-) 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 aa278e077d..3bdc4ede7b 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -89,6 +89,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 7f92f3647f..dd07714a3a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -68,7 +68,7 @@ import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" import { OutputTokenLimitError } from "../../api/providers/utils/output-token-limit-error" // 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" @@ -1678,7 +1678,19 @@ export class Task extends EventEmitter implements TaskLike { } } - /** Persists Cline messages and updates task metadata in the history store. Returns false on failure. */ + /** + * 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. + * + * `merge` (default `true`) is passed through to `saveTaskMessages`: the in-memory + * snapshot is merged with the on-disk record. `overwriteClineMessages` passes `false` + * to replace the stored messages outright. + */ private async saveClineMessages(merge = true): Promise { try { await saveTaskMessages({ @@ -1687,7 +1699,12 @@ export class Task extends EventEmitter implements TaskLike { globalStoragePath: this.globalStoragePath, merge, }) + } catch (error) { + console.error("Failed to save Roo messages:", error) + return false + } + try { if (this._taskApiConfigName === undefined) { await this.taskApiConfigReady } @@ -1715,11 +1732,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 { @@ -2712,6 +2732,56 @@ export class Task extends EventEmitter implements TaskLike { return formatResponse.toolError(formatResponse.missingToolParameterError(paramName)) } + /** + * 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). + * + * 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( + this.clineMessages, + (message) => + message.partial === true && + message.type === "ask" && + message.ask === "tool" && + (text === undefined || message.text === text), + ) + + if (!partialToolAsk) { + return + } + + partialToolAsk.partial = false + partialToolAsk.progressStatus = undefined + partialToolAsk.isAnswered = true + 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) + }) + } + // Lifecycle // Start / Resume / Abort / Dispose diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 88db6f2f2a..4f8b198c5f 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" @@ -10,6 +11,7 @@ import type { Mock } from "vitest" import { providerIdentifiers, RooCodeEventName, + type ClineMessage, type GlobalState, type HistoryItem, type ProviderSettings, @@ -5922,6 +5924,404 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { + let updateSnapshot: Record | undefined + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockImplementation(async (message) => { + updateSnapshot = { ...message } + }) + 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, + progressStatus: { text: "Generating…", icon: "sync" }, + } + + 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(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() + }) + + 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(task.clineMessages[0].isAnswered).toBeUndefined() + expect(saveSpy).not.toHaveBeenCalled() + expect(updateSpy).not.toHaveBeenCalled() + + updateSpy.mockRestore() + 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(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) + + updateSpy.mockRestore() + 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(task.clineMessages[0].isAnswered).toBe(true) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] updateClineMessage failed:", + boom, + ) + + updateSpy.mockRestore() + 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 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") + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + try { + fsReal.rmSync(taskDir, { recursive: true, force: 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() + + // 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", + ) + } finally { + // Restore the shared directory for sibling tests that persist through the + // real fs, even when the operation rejects or an assertion fails. + 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 + // 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 d172c95ae3e451b47c90f2ae829f7674e8483e4c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:24 +0800 Subject: [PATCH 02/18] feat(write-to-file): per-task partial stream state + cleanup primitives --- .../removeClineFromStack-delegation.spec.ts | 41 +++++ src/core/tools/WriteToFileTool.ts | 165 +++++++++++++++++- .../tools/__tests__/writeToFileTool.spec.ts | 116 ++++++++++++ src/core/webview/ClineProvider.ts | 6 + 4 files changed, 326 insertions(+), 2 deletions(-) diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index bc5b8a4426..b42a549fef 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -5,6 +5,7 @@ import { ClineProvider } from "../core/webview/ClineProvider" import { TaskRegistry } from "../core/task/TaskRegistry" import { PendingActionSettlementError, type Task } from "../core/task/Task" import { makeProviderStub } from "./helpers/provider-stub" +import { writeToFileTool } from "../core/tools/WriteToFileTool" type MockTask = Pick & Partial> & { @@ -208,6 +209,46 @@ describe("ClineProvider failed history restoration cleanup", () => { expect(task.dispose).toHaveBeenCalledOnce() }) + it("releases the tool's per-task state before a directly disposed task loses its listeners", async () => { + const key = "failed-history-task.inst-1" + const order: string[] = [] + const task = { + taskId: "failed-history-task", + instanceId: "inst-1", + emit: vi.fn(), + once: vi.fn(), + off: vi.fn(), + dispose: vi.fn().mockImplementation(() => { + // Dispose removes every listener, so this is the last moment the abort + // cleanup could still have run; the entry must already be gone here. + order.push(writeToFileTool["taskPartialStreamState"].has(key) ? "state-retained" : "state-cleared") + order.push("dispose") + return Promise.resolve() + }), + } as unknown as Task + // Fixture: the state entry the tool would have created during a partial stream. + writeToFileTool["taskPartialStreamState"].set(key, { + lastSeenPartialPath: undefined, + streamFailed: false, + streamError: undefined, + task, + abortCleanup: () => {}, + }) + const taskEventListeners = new Map([[task, [vi.fn()]]]) + const taskRegistry = new TaskRegistry() + taskRegistry.push(task) + const provider = { taskRegistry, taskEventListeners, log: vi.fn() } as unknown as ClineProvider + + await privateClineProvider.cleanupFailedHistoryTask.call( + provider, + task, + new PendingActionSettlementError("settlement failed"), + ) + + expect(order).toEqual(["state-cleared", "dispose"]) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("keeps the task active after an unrelated history resume failure", async () => { const cleanupListener = vi.fn() const task = { diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ae026b4b86..c9776553b7 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" @@ -23,9 +23,165 @@ 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 original filesystem error of the failed streaming delta, reported once + * by onParameterParseFailure() when the final block fails to parse (so + * execute() never runs and would never report it). */ + streamError: Error | undefined + /** 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 + /** + * 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 + * 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 taskPartialStreamState = new Map() + + private getPartialStreamFailureKey(task: Task): string { + return `${task.taskId}.${task.instanceId}` + } + + /** + * 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) + const existing = this.taskPartialStreamState.get(key) + if (existing) { + return existing + } + + const state: TaskPartialStreamState = { + lastSeenPartialPath: undefined, + streamFailed: false, + streamError: undefined, + task, + abortCleanup: () => this.resetTaskPartialState(task), + } + this.taskPartialStreamState.set(key, state) + task.once(RooCodeEventName.TaskAborted, state.abortCleanup) + return state + } + + 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 + } + + /** + * Clear a task's partial-stream state from a disposal path that does not abort first. + * Task.dispose() removes every listener, so a task disposed directly (for example + * ClineProvider.cleanupFailedHistoryTask()) never fires the TaskAborted cleanup and + * this singleton would keep the disposed task and its diff-view provider. + */ + public clearTaskState(task: Task): void { + this.resetTaskPartialState(task) + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + const state = this.taskPartialStreamState.get(key) + if (!state) { + return + } + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + this.taskPartialStreamState.delete(key) + } + + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + + /** + * 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) + }) + } + + /** + * Teardown boundary for the handle() parse-failure path, where execute() never + * runs and therefore its finally (resetTaskPartialState) never runs either. + * + * Tears down the per-task stream state: otherwise the abort listener leaks for + * the task's lifetime, and when a streaming delta had failed, the streamFailed + * guard would suppress the diff preview of every later write_to_file in this + * task. Restores the diff document: streaming may have opened it with + * unapproved partial content, and execute()'s error cleanup (revert + reset) + * never fires on this path, so a user save could persist the content without + * the teardown here. When a streaming delta already hit a fatal filesystem + * error, that error is what the user can act on, so report it with the same + * "writing file" context execute()'s catch uses, and suppress the incidental + * parse error. + */ + override resetPartialState(): void { + super.resetPartialState() + for (const state of this.taskPartialStreamState.values()) { + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + } + this.taskPartialStreamState.clear() + } + async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path @@ -197,8 +353,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content + + // 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.hasPathStabilized(relPath) || newContent === undefined) { + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..bec63332a7 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" @@ -118,6 +119,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) => { @@ -128,6 +132,8 @@ describe("writeToFileTool", () => { return content }) + mockCline.taskId = "task-1" + mockCline.instanceId = "instance-1" mockCline.cwd = "/" mockCline.consecutiveMistakeCount = 0 mockCline.didEditFile = false @@ -186,8 +192,12 @@ 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") + mockCline.processQueuedMessages = vi.fn() mockAskApproval = vi.fn().mockResolvedValue(true) mockHandleError = vi.fn().mockResolvedValue(undefined) @@ -419,6 +429,111 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) }) + + 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?.() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + 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() + }) + + + + + + }) + + 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, + streamError: undefined, + 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", () => { @@ -470,6 +585,7 @@ describe("writeToFileTool", () => { // Second call with same path - path is now stabilized, error occurs await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 874e2f8f08..75612323f9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -60,6 +60,7 @@ import { } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { TaskRegistry } from "../task/TaskRegistry" +import { writeToFileTool } from "../tools/WriteToFileTool" import { TaskScheduler } from "../task/TaskScheduler" import { getEffectiveTaskApiConfiguration, @@ -636,6 +637,11 @@ export class ClineProvider this.taskEventListeners.delete(task) } + // Dispose removes all listeners, so the tool's per-task state must be released + // while the task is still here; otherwise the singleton keeps the disposed task + // and its diff-view provider. + writeToFileTool.clearTaskState(task) + try { await task.dispose() } catch (error) { From 52699c6cdc7d90e52d6ad60ad64dc2e3cb9982e2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:24 +0800 Subject: [PATCH 03/18] test(write-to-file): cover partial-state cleanup primitives directly --- ...teToFileTool-partial-state-cleanup.spec.ts | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts new file mode 100644 index 0000000000..4506742c26 --- /dev/null +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -0,0 +1,100 @@ +// npx vitest run core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts + +import { RooCodeEventName } from "@roo-code/types" +import { vi, type MockedFunction } from "vitest" + +import { type Task } from "../../task/Task" +import { writeToFileTool } from "../WriteToFileTool" + +// The cleanup primitives only read these members, so a structural double is enough; +// the double assertion is the repo's existing pattern for private-method tests +// (see src/__tests__/removeClineFromStack-delegation.spec.ts). +interface CleanupTask { + taskId: string + instanceId: string + once: MockedFunction<(...args: unknown[]) => unknown> + off: MockedFunction<(...args: unknown[]) => unknown> + diffViewProvider: { + reset: MockedFunction<() => Promise> + revertChanges: MockedFunction<() => Promise> + } + finalizePartialToolAsk: MockedFunction<() => Promise> +} + +function buildTask(taskId: string, instanceId: string): Task { + const task: CleanupTask = { + taskId, + instanceId, + once: vi.fn(), + off: vi.fn(), + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + } + return task as unknown as Task +} + +// Private members are reached by bracket notation (AGENTS.md: no `as any`). +const stateFor = (task: Task) => writeToFileTool["taskPartialStreamState"].get(`${task.taskId}.${task.instanceId}`) + +describe("WriteToFileTool per-task partial-state cleanup", () => { + afterEach(() => { + writeToFileTool["taskPartialStreamState"].clear() + vi.restoreAllMocks() + }) + + it("releases the task state and deregisters the abort listener", async () => { + const task = buildTask("cleanup-task", "inst-1") + const state = writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, state.abortCleanup) + }) + + it("is a no-op for a task that never streamed", async () => { + const task = buildTask("never-streamed", "inst-2") + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).not.toHaveBeenCalled() + }) + + it("logs and continues when resetting the diff view fails", async () => { + const task = buildTask("reset-fails", "inst-3") + const t = task as unknown as CleanupTask + t.diffViewProvider.reset = vi.fn().mockRejectedValue(new Error("reset failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["resetDiffViewAfterWrite"](task) + + expect(errorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + }) + + it("logs and continues when reverting the diff document fails", async () => { + const task = buildTask("revert-fails", "inst-4") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["revertDiffChangesBeforeReset"](task) + + expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) + }) + + it("logs and continues when finalizing the open partial ask fails", async () => { + const task = buildTask("finalize-fails", "inst-5") + const t = task as unknown as CleanupTask + t.finalizePartialToolAsk = vi.fn().mockRejectedValue(new Error("finalize failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["finalizePartialToolAskAfterFailure"](task, "partial text") + + expect(errorSpy).toHaveBeenCalledWith("Error finalizing write_to_file partial tool ask:", expect.any(Error)) + }) +}) From 4b23b6a270aa34368963613e145bb410c0fcd567 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:24 +0800 Subject: [PATCH 04/18] fix(write-to-file): capture streaming failure once, report it once --- src/core/tools/WriteToFileTool.ts | 51 +++- .../tools/__tests__/writeToFileTool.spec.ts | 232 +++++++++++++++++- 2 files changed, 267 insertions(+), 16 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c9776553b7..267c728c27 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -353,6 +353,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content + 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.taskPartialStreamState.get(partialStreamFailureKey)?.streamFailed) { + return + } // Get (or create) this task's state; registers the TaskAborted teardown listener // once, so abandoned streams are torn down even if execute() never runs. @@ -385,12 +393,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) @@ -406,14 +408,37 @@ 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. Retain the original error: if the final + // block later fails to parse, execute() never runs and only + // onParameterParseFailure() can report this failure to the user. + partialStreamState.streamFailed = true + partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) + 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 bec63332a7..f4677277d7 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 @@ -429,6 +442,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) + }) it("cleans per-task partial state when the task aborts before execute finalization", async () => { let abortCleanup: (() => void) | undefined @@ -465,8 +497,49 @@ describe("writeToFileTool", () => { 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() + } + }) }) @@ -575,17 +648,170 @@ 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).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" }), + ) + // 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 }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // 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() }) + + 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" }), + ) + // 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 }) + + // Second call - path stabilized, update() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // 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() + }) + + 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("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("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() + }) + + + + }) }) From e3c10401f760a33cb977cd3cfdc1f1af8de49d3f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:25 +0800 Subject: [PATCH 05/18] feat(tools): onParameterParseFailure teardown boundary --- src/core/tools/BaseTool.ts | 39 +++- src/core/tools/WriteToFileTool.ts | 22 ++ .../tools/__tests__/writeToFileTool.spec.ts | 192 ++++++++++++++++++ 3 files changed, 250 insertions(+), 3 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dc16a9d81d 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -155,9 +155,23 @@ export abstract class BaseTool { throw new Error("Tool call is missing native arguments (nativeArgs).") } } catch (error) { - console.error(`Error parsing parameters:`, error) - 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)) + const parseError = error instanceof Error ? error : new Error(String(error)) + console.error(`Error parsing parameters:`, parseError) + // 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) + }) + // execute() never runs on this path, so tools that keep per-task state + // outside execute() (streaming failure marks, abort listeners) get their + // one remaining teardown boundary here. + const reportedStreamingFailure = await this.onParameterParseFailure(task, callbacks, parseError) + if (!reportedStreamingFailure) { + const errorMessage = `Failed to parse ${this.name} parameters: ${parseError.message}` + await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + } // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. // Do NOT call pushToolResult here to avoid duplicate tool_result payloads. return @@ -166,4 +180,23 @@ export abstract class BaseTool { // Execute with typed parameters await this.execute(params, task, callbacks) } + + /** + * Teardown boundary for the native-argument parse-failure path in handle(). + * + * When nativeArgs are missing or malformed, execute() never runs, so per-task + * state a tool registered outside execute() (streaming failure marks, abort + * listeners) is never torn down there. Streaming tools override this to tear + * that state down and, when a streaming delta already failed, to report the + * captured streaming error instead of the generic parse error. + * + * @param task - Task instance + * @param callbacks - Tool execution callbacks + * @param parseError - The native-argument parse error + * @returns true when the override already reported the failure to the user, + * so handle() suppresses the generic parse error + */ + protected async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + return false + } } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 267c728c27..ba25a9f2c0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -174,6 +174,28 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * "writing file" context execute()'s catch uses, and suppress the incidental * parse error. */ + override async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (!state) { + return false + } + this.resetTaskPartialState(task) + // Streaming may have opened the diff view with unapproved partial content. + // execute() never runs on this path, so its error cleanup (revert + reset) + // never fires: restore the document here so a user save cannot persist + // content the write never completed (the same invariant the denial and + // streaming-failure paths maintain). Both helpers no-op when no view is + // open. + await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + if (!state.streamError) { + return false + } + void parseError + await callbacks.handleError("writing file", state.streamError) + return true + } + override resetPartialState(): void { super.resetPartialState() for (const state of this.taskPartialStreamState.values()) { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index f4677277d7..dddd139011 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -541,7 +541,123 @@ describe("writeToFileTool", () => { } }) + it("reports the captured streaming error instead of the parse error when the final block fails to parse, and clears the per-task state", async () => { + // A streaming delta fails with a filesystem error (streamFailed + streamError are + // captured). The final block then arrives without nativeArgs, so execute() never + // runs: the parse-failure teardown boundary must report the original filesystem + // error under the "writing file" context (not the incidental parse error) and tear + // down the per-task state, so the next write_to_file stream in this task is not + // blocked by the stale streamFailed guard and no abort listener leaks. + const fsError = new Error("EACCES: permission denied") + mockCline.diffViewProvider.open.mockRejectedValue(fsError) + // Capture the abort listener of the state created by the first deltas: it is the + // exact reference that the parse-failure teardown must detach. + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError) + expect(mockHandleError).not.toHaveBeenCalledWith( + expect.stringContaining("parsing write_to_file"), + expect.anything(), + ) + // The parse-failure teardown also restores the diff document: the + // streaming failure above already reverted it once (revert + reset), + // and the parse path runs the same cleanup again because execute() + // never runs on this path. + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) + // Per-task state torn down at this boundary: guard cleared, the exact + // registered abort listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + + // The next stream for the same task must issue a partial ask again (the stale + // streamFailed guard is gone). + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("restores the diff document when the final block fails to parse after successful streaming", async () => { + // Streaming opened the diff view with unapproved partial content (open and + // update both succeeded, so no streaming error was captured). The final block + // then arrives without nativeArgs: execute() never runs, so its error cleanup + // never fires. The parse-failure teardown must still restore the document + // (revert before reset) or a user save could persist content the write never + // completed, and must report the generic parse error (no streaming error to + // surface instead). + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + // Delta 1 - stabilize path; delta 2 - streams the partial content into the + // diff view (open + update resolve). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // No streaming error was captured: the generic parse error is reported. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The diff document is restored by the parse path itself (no streaming + // failure happened, so this is the only revert + reset in the test). + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + // Per-task state torn down: guard cleared, exact listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("path stabilization predicate", () => { @@ -755,7 +871,83 @@ 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. + // No argument: BaseTool.handle() calls finalizePartialToolAsk() with no text, so a + // mutation passing wrong text would leave findLast() unmatched and the spinner + // stuck. (toHaveBeenCalledWith(undefined) does not match a no-arg call under + // vitest's matcher semantics: [] is not equal to [undefined].) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + 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(), + }) + + // Same no-argument contract as the sibling test above: the finalize call in + // BaseTool.handle() carries no text. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + 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() + } + }) From 445c2f5be09661ade42e01fae8ca9e5a61c24afa Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 01:24:08 +0800 Subject: [PATCH 06/18] chore(ci): re-trigger the unit suite after the flaky writeToFileTool partial-path case Same flaky case as the u5 run: the core project passes locally at this head and the case passes in isolation. Re-running to confirm. From 1e0f1c4c5b48a5868e3e47eacd8aec6d38a99b03 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 02:52:47 +0800 Subject: [PATCH 07/18] test(write-to-file): align the partial-path case with handlePartial's no-filesystem contract Same fix as p1066/u5 (6906c026a): platform-unit-test (ubuntu-latest) fails here and passes locally because the failing case is it.skipIf(process.platform === "win32"). The case asserted that the second streaming delta calls createDirectoriesForFile - the exact call this chain removes, because an unguarded mkdir in handlePartial threw EROFS into BaseTool.handle() without setting didRejectTool/didAlreadyUseTool, stalling the agent loop. The chain's own regression test pins the new contract; this older case still pinned the old one, so the two contradicted and only Linux CI saw it. Rewritten as "defers parent directory creation to execute() while streaming": no filesystem work while streaming, directories still created by the authoritative non-partial execute(). Local run: the file passes; the rewritten case also passes with the win32 skip lifted. --- src/core/tools/__tests__/writeToFileTool.spec.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index dddd139011..07556de50a 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -310,14 +310,17 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "defers parent directory creation to execute() while streaming", async () => { - // First call - path not yet stabilized + // Streaming deltas must not touch the filesystem at all. An unguarded + // createDirectoriesForFile here threw EROFS up into BaseTool.handle(), which never + // set didRejectTool/didAlreadyUseTool, so the agent loop stalled permanently. + // The directories are still created - by the authoritative non-partial execute(). + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, ) From b04e2241e937cdb0faebb8f12c048e77311c5092 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 03:34:59 +0800 Subject: [PATCH 08/18] fix(tools): scope the write_to_file stream teardown to one task and finalize the failed retry's ask Two findings on this unit's own state: 1) resetPartialState() is overridden to clear the whole taskPartialStreamState map, and execute() calls it on both the success and the error path. The map is keyed per task precisely so two providers can stream write_to_file through this singleton at once, so task A's execute() was deleting task B's entry while B was still streaming: B loses streamFailed (its next delta re-opens the diff view and spawns a duplicate partial ask - the exact case the stabilization guard prevents) and loses streamError. execute() now calls super.resetPartialState() (the base field is genuinely instance-global) plus resetTaskPartialState(task) for this task only. 2) On the diff-view branch execute() opens its own partial ask before the write. If the write then throws, the catch reported the error and reset without finalizing that ask, leaving the spinner and Save/Reject buttons live for a tool call that had already failed. The catch now finalizes the pending ask first. Tests (writeToFileTool.spec.ts, new per-task stream state isolation block): a second streaming task keeps its streamFailed/streamError across another task's execute(), and a failing save finalizes the ask with the exact partial payload. Both fail on the pre-fix code (2 failed / 38 passed) and pass after (40 passed). Local: eslint clean on both files with --prune-suppressions (no suppression change), package tsc clean. --- src/core/tools/WriteToFileTool.ts | 20 +++++++- .../tools/__tests__/writeToFileTool.spec.ts | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ba25a9f2c0..27944bdfc8 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -208,6 +208,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path let newContent = params.content + // Set when this execute() opens its own partial tool ask (diff-view branch), so + // the catch below can finalize it. Undefined on the saveDirectly branch. + let pendingPartialAsk: string | undefined if (!relPath) { task.consecutiveMistakeCount++ @@ -315,6 +318,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) + pendingPartialAsk = partialMessage await task.ask("tool", partialMessage, true).catch(() => {}) await task.diffViewProvider.open(relPath) } @@ -358,15 +362,27 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + // BaseTool's reset only clears this instance's lastSeenPartialPath; the + // stream state added here is keyed per task. Clearing the whole map from + // one task's execute() would drop another task's streamFailed/streamError + // while it is still streaming, so tear down only this task's entry. + super.resetPartialState() + this.resetTaskPartialState(task) task.processQueuedMessages() return } catch (error) { + // The diff-view branch above may have opened a fresh partial ask for this + // (retried) write. Finalize it before tearing down, or the spinner and + // Save/Reject buttons stay live for a tool call that has already failed. + if (pendingPartialAsk !== undefined) { + await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) + } await handleError("writing file", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + super.resetPartialState() + this.resetTaskPartialState(task) return } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 07556de50a..34b3dfe8a6 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -728,6 +728,52 @@ describe("writeToFileTool", () => { }) }) + describe("per-task stream state isolation", () => { + // A second task streaming through the same singleton while mockCline's execute() + // runs. Structural double, same pattern as the partial-state-cleanup spec. + function buildStreamingTask(taskId: string, instanceId: string) { + return { + taskId, + instanceId, + once: vi.fn(), + off: vi.fn(), + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + } + } + + it("leaves another task's stream state intact when execute() completes", async () => { + const other = buildStreamingTask("task-2", "instance-2") + const otherState = writeToFileTool["getTaskPartialStreamState"](other as never) + otherState.streamFailed = true + otherState.streamError = new Error("other task stream failure") + + await executeWriteFileTool({}) + + // The other task is still streaming: its failure state must survive, or its + // next delta re-opens the diff view and spawns a duplicate partial ask. + const retained = writeToFileTool["taskPartialStreamState"].get("task-2.instance-2") + expect(retained).toBeDefined() + expect(retained?.streamFailed).toBe(true) + expect(retained?.streamError?.message).toBe("other task stream failure") + expect(other.off).not.toHaveBeenCalled() + }) + + it("finalizes the partial ask when the write itself fails", async () => { + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // The diff-view branch opened a partial ask for this write; without the + // finalize the spinner and Save/Reject stay live after the failure. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + }) + }) + describe("user interaction", () => { it("reverts changes when user rejects approval", async () => { mockAskApproval.mockResolvedValue(false) From 0dc92ca73c9c79418df09c7c0bf9fcc362a9583f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:44:46 +0800 Subject: [PATCH 09/18] fix(tools): do not report a completed teardown when the diff revert fails onParameterParseFailure() tore down the per-task stream state first, then reverted the diff document with the failure logged and swallowed, then reset the diff provider. A failed revert therefore looked like a completed teardown even though the editor can still hold the unapproved partial content - and a user save of that editor lands a write the task never approved, with nothing in the UI saying so. The revert now runs while the recovery state still exists, revertDiffChangesBeforeReset returns whether it succeeded, and a failed revert is surfaced with task.say("error", ...) naming the hazard. The remaining cleanup (reset + per-task teardown) still runs, so the task is never left half-torn-down. Test: the seeded per-task state is observed from inside the failing revertChanges double, so the ordering is pinned (state present during rollback, gone afterwards), and the error say is asserted. Without the fix the test fails (1 failed / 5 passed); with it 6 pass. The streaming-failure cleanup keeps its existing log-only behaviour: that path's error is reported by the authoritative non-partial block in execute(), as its comment states, so adding a second message there would double-report the same failure. Local: writeToFileTool-partial-state-cleanup.spec.ts 6 passed, eslint clean with --prune-suppressions (no suppression change), package tsc clean. --- src/core/tools/WriteToFileTool.ts | 35 +++++++++++++----- ...teToFileTool-partial-state-cleanup.spec.ts | 37 ++++++++++++++++++- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 27944bdfc8..69aed4720c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -144,13 +144,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * 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. + * is open. Failures are logged and reported through the return value: the caller + * must not treat the teardown as complete when this returns false, because the + * document may still hold the unapproved content, but the remaining cleanup (reset, + * per-task state teardown) still runs so the task is not left half-torn-down. */ - private async revertDiffChangesBeforeReset(task: Task): Promise { - await task.diffViewProvider.revertChanges().catch((revertError) => { + private async revertDiffChangesBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.revertChanges() + return true + } catch (revertError) { console.error("Error reverting write_to_file diff view changes:", revertError) - }) + return false + } } private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { @@ -179,15 +185,26 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!state) { return false } - this.resetTaskPartialState(task) // Streaming may have opened the diff view with unapproved partial content. // execute() never runs on this path, so its error cleanup (revert + reset) // never fires: restore the document here so a user save cannot persist // content the write never completed (the same invariant the denial and - // streaming-failure paths maintain). Both helpers no-op when no view is - // open. - await this.revertDiffChangesBeforeReset(task) + // streaming-failure paths maintain). Both helpers no-op when no view is open. + // The revert runs BEFORE the per-task state is torn down: when it fails, the + // document can still hold that unapproved content, and the recovery state has + // to exist while the outcome is decided and reported. + const reverted = await this.revertDiffChangesBeforeReset(task) + this.resetTaskPartialState(task) await this.resetDiffViewAfterWrite(task) + if (!reverted) { + // Do not report a completed teardown: the editor may still show content this + // task never approved, and saving it would land a write the user never + // authorized. The user has to be able to tell that from the UI. + await task.say( + "error", + "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", + ) + } if (!state.streamError) { return false } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 4506742c26..93ba46560c 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -19,6 +19,7 @@ interface CleanupTask { revertChanges: MockedFunction<() => Promise> } finalizePartialToolAsk: MockedFunction<() => Promise> + say: MockedFunction<(...args: unknown[]) => Promise> } function buildTask(taskId: string, instanceId: string): Task { @@ -32,6 +33,7 @@ function buildTask(taskId: string, instanceId: string): Task { revertChanges: vi.fn().mockResolvedValue(undefined), }, finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(undefined), } return task as unknown as Task } @@ -87,8 +89,41 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) }) + it("reverts while the recovery state exists and reports the hazard when the revert fails", async () => { + const task = buildTask("revert-fails-teardown", "inst-5") + const t = task as unknown as CleanupTask + let stateSizeDuringRevert = -1 + t.diffViewProvider.revertChanges = vi.fn(async () => { + // The recovery state must still be present while the rollback runs. + stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size + throw new Error("revert failed") + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // Seed the per-task stream state so this teardown boundary is taken at all. + writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + // Only handleError is reached when no streaming error was recorded; the + // structural double is the existing pattern in this file. + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // A failed rollback is not a completed teardown: the user is told the editor may + // still hold content the task never approved. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(stateSizeDuringRevert).toBe(1) + // The teardown still finished. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + expect(handled).toBe(false) + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { - const task = buildTask("finalize-fails", "inst-5") + const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask t.finalizePartialToolAsk = vi.fn().mockRejectedValue(new Error("finalize failed")) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) From 58c1a2b2062b025c69283522c58dcd08eaef5c98 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:02:41 +0800 Subject: [PATCH 10/18] test(tools): cover rollback failure with a retained stream error, and keep a failing report non-fatal Review follow-up on the open thread: the new teardown test only exercised a failed rollback with no streamError. Two cases added: - rollback fails while a streaming filesystem error is retained: both reports happen - the hazard say and handleError("writing file", streamError) - and the handler returns true. - the hazard report itself rejects: the teardown must not abort, so the streaming error still reaches the user. The second case exposed a real gap: the report was awaited inline, so a rejecting task.say would have thrown out of onParameterParseFailure and swallowed the user's only actionable error. The report now goes through reportRevertFailure(), which logs a failing say and continues - matching finalizePartialToolAskAfterFailure. Verified by stashing the source change: 1 failed / 7 passed without it, 8 passed with it. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 24 +++++++-- ...teToFileTool-partial-state-cleanup.spec.ts | 49 +++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 69aed4720c..c31d33cf53 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -165,6 +165,25 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + /** + * Surface a failed rollback to the user. The hazard has to be visible in the chat, + * not only in the console: the editor may still hold content the task never + * approved and a save of it would land an unauthorized write. A failing say must + * not abort the teardown - a retained streaming error still has to reach the user + * through handleError - so its failure is logged only, matching the other cleanup + * helpers in this file. + */ + private async reportRevertFailure(task: Task): Promise { + await task + .say( + "error", + "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", + ) + .catch((sayError) => { + console.error("Error reporting write_to_file rollback failure:", sayError) + }) + } + /** * Teardown boundary for the handle() parse-failure path, where execute() never * runs and therefore its finally (resetTaskPartialState) never runs either. @@ -200,10 +219,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Do not report a completed teardown: the editor may still show content this // task never approved, and saving it would land a write the user never // authorized. The user has to be able to tell that from the UI. - await task.say( - "error", - "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", - ) + await this.reportRevertFailure(task) } if (!state.streamError) { return false diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 93ba46560c..6718bb7c30 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -122,6 +122,55 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { errorSpy.mockRestore() }) + it("reports the rollback hazard AND the retained streaming error, and returns true", async () => { + const task = buildTask("revert-fails-with-stream-error", "inst-7") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // Both reports happen: the rollback hazard and the error the user can act on. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("still reports the streaming error when the rollback warning itself fails", async () => { + const task = buildTask("rollback-warning-fails", "inst-8") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + t.say = vi.fn().mockRejectedValue(new Error("say failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // A failing report must not abort the teardown: the streaming error still lands. + expect(errorSpy).toHaveBeenCalledWith("Error reporting write_to_file rollback failure:", expect.any(Error)) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask From af51675c768e5d220ab59b738db501af6e477463 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:13:44 +0800 Subject: [PATCH 11/18] fix(tools): report the rollback hazard from the failed-partial-stream cleanup too The handlePartial() failure branch called revertDiffChangesBeforeReset() and ignored the result, so a failed restore left unapproved content in the editor with no user-visible signal: the stream error that triggered the cleanup is a different failure and is reported by the authoritative non-partial path in execute(), which is why this branch had stayed log-only. The revert + reset + report sequence is now one helper, cleanupFailedPartialStream(), used by the streaming cleanup; it checks the rollback result and reports the hazard exactly as onParameterParseFailure() does. Test: cleanupFailedPartialStream is driven directly with a rejecting revertChanges double - the hazard say and the provider reset are both asserted. Stashing the source change makes it fail (1 failed / 8 passed); with it 9 pass. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 23 ++++++++++++++++--- ...teToFileTool-partial-state-cleanup.spec.ts | 14 +++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c31d33cf53..2fd409fe9a 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -184,6 +184,21 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + /** + * Cleanup for a failed partial stream: restore the diff document, close the view, + * and - when the restore itself failed - tell the user the editor may still hold + * unapproved content, the same way the parse-failure teardown does. Without the + * report, a failed rollback here is invisible: the stream error that triggered the + * cleanup is a different failure and is reported elsewhere. + */ + private async cleanupFailedPartialStream(task: Task): Promise { + const reverted = await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + if (!reverted) { + await this.reportRevertFailure(task) + } + } + /** * Teardown boundary for the handle() parse-failure path, where execute() never * runs and therefore its finally (resetTaskPartialState) never runs either. @@ -506,9 +521,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) 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) + // persist the failed streamed content (reset() alone leaves it dirty), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 6718bb7c30..0ff9d72826 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -171,6 +171,20 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { errorSpy.mockRestore() }) + it("surfaces the rollback hazard from the failed-stream cleanup as well", async () => { + const task = buildTask("failed-stream-cleanup", "inst-9") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["cleanupFailedPartialStream"](task) + + // Same contract as the parse-failure teardown: a failed restore is reported. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask From 1311c90358bdceb2895edfade6d66bd4f5385560 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:25:38 +0800 Subject: [PATCH 12/18] fix(tools): tear down the per-task stream state when a write is rejected Both rejection exits in execute() returned without resetTaskPartialState(): the saveDirectly branch returned straight away and the diff-view branch returned after revertChanges(). The per-task entry - and with it a streamFailed flag armed by an earlier failed delta - therefore stayed set for the whole task, which suppresses the diff preview of every later write_to_file in that task, and the TaskAborted listener leaked for the task's life. Both exits now call super.resetPartialState() + resetTaskPartialState(task), matching the success and catch paths. Test: seed the executing task's stream state (streamFailed + streamError), reject the approval, and assert the task's entry is gone afterwards. Stashing the source change: 1 failed / 40 passed; with it 41 passed / 5 skipped. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 10 ++++++++++ src/core/tools/__tests__/writeToFileTool.spec.ts | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 2fd409fe9a..aa3d96dd21 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -359,6 +359,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // Rejection is an exit from execute() too. Without the teardown, a + // streamFailed flag armed by an earlier failed delta stays set for the whole + // task, which suppresses the diff preview of every later write_to_file, and + // the TaskAborted listener leaks. + super.resetPartialState() + this.resetTaskPartialState(task) return } @@ -393,6 +399,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!didApprove) { await task.diffViewProvider.revertChanges() + // Same exit contract as the saveDirectly branch above: the per-task stream + // state and its abort listener belong to this execute() call. + super.resetPartialState() + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 34b3dfe8a6..abcf7f67d0 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -784,6 +784,20 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() }) + it("clears this task's stream state when the write is rejected", async () => { + // A failed delta earlier in the same task arms the streamFailed guard. If the + // rejection exit returns without the teardown, every later write_to_file in this + // task loses its diff preview and the TaskAborted listener leaks. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + state.streamError = new Error("stream failure before the rejected write") + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}) + + expect(writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`)).toBeUndefined() + }) + it("reports user edits with diff feedback", async () => { const userEditsValue = "- old line\n+ new line" mockCline.diffViewProvider.saveChanges.mockResolvedValue({ From 5d713e512255b9b910d2329f5f272960ac54333a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:49:41 +0000 Subject: [PATCH 13/18] chore: trigger a fresh review pass at this head From 41ae4568734a8e962c4aa2d9414aa28d8f0b667e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 19:43:33 +0800 Subject: [PATCH 14/18] fix(tools): release the stream state on execute()'s early returns too execute() releases the per-task stream state in its success path, its catch and the approval rejection, but the three early returns (missing path, missing content, rooignore denial) left before any of that. The per-task entry and its TaskAborted listener then survived for the task's lifetime, and a streamFailed flag armed by an earlier failed delta kept suppressing the diff preview of every later write_to_file in that task. Each early return now calls resetTaskPartialState(task) - per-task only, so another task still streaming through this singleton is untouched. Tests: describe('early-return stream state cleanup') with the rooignore-denial case (state map empty, off(TaskAborted, ...) called) and the missing-path case driven through execute() with an empty path. Pin: removing the three releases fails both. Local: core/tools + writeToFileTool.spec + assistant-message + core/task = 1428 passed / 5 skipped; tsc --noEmit 0; eslint 0 err / 0 warn on both files. --- src/core/tools/WriteToFileTool.ts | 15 +++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 27 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index aa3d96dd21..186d040ecb 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -264,6 +264,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -272,6 +277,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -281,6 +291,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index abcf7f67d0..8eaa9c47cc 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -774,6 +774,33 @@ describe("writeToFileTool", () => { }) }) + describe("early-return stream state cleanup", () => { + it("releases the per-task stream state when a rooignore denial returns early", async () => { + // The denial returns before the try/catch teardown: without the release the abort + // listener stays for the task's lifetime and a retained streamFailed suppresses the + // diff preview of every later write_to_file in this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) + + it("releases the per-task stream state when a missing parameter returns early", async () => { + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await writeToFileTool.execute({ path: "", content: "mock content" }, mockCline, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + }) + describe("user interaction", () => { it("reverts changes when user rejects approval", async () => { mockAskApproval.mockResolvedValue(false) From 7c11c26aeadf1f86a73aaa83c9862f3cb2f2380b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 02:31:48 +0800 Subject: [PATCH 15/18] fix(write-to-file): stop a cancelled partial stream at every provider await in handlePartial() Consistency port of the same fix already carried by #1929 (2f356e6f7) and #1928 (876a93b22). handlePartial() awaited provider.getState(), fileExistsAtPath(), task.ask() and diffViewProvider.open() with no cancellation check. A TaskAborted (or a direct clearTaskState) during any of those awaits runs the teardown that deletes this task's per-task stream state, and the delta already in flight then went on to re-ask, re-open a diff view, or stream a partial delta into a view the teardown had already released - resurrecting state for a task the user cancelled. Added isPartialStreamStillLive() (identity, not presence: a re-created entry for the same key belongs to a new stream) and a check after each of the four awaits. Tests: four cancellation cases in the existing "early-return stream state cleanup" describe, one per await, each asserting the delta stopped before the next side effect (no probe / no ask / no open / no update) and the state stayed released. Negative controls: each guard removed on its own -> exactly 1 failed (four separate runs); restored -> 4 passed. Verification: core/tools 656 passed / 5 skipped, tsc --noEmit 0 errors, eslint --prune-suppressions --max-warnings=0 clean on both touched files with src/eslint-suppressions.json unchanged. --- src/core/tools/WriteToFileTool.ts | 35 ++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 86 +++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 186d040ecb..0267d6e65c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -131,6 +131,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { this.taskPartialStreamState.delete(key) } + /** + * Whether this task's partial stream is still the live one. handlePartial() awaits + * provider state, a filesystem probe, task.ask() and diffViewProvider.open() before it + * touches the diff view; a cancellation during any of those awaits runs the TaskAborted + * teardown (or a direct clearTaskState), which deletes this entry. Continuing would + * re-ask, re-open a diff view, or stream a partial delta into a view the teardown has + * already released for a task the user cancelled. Identity, not presence: a re-created + * entry for the same key belongs to a new stream, and this one must not write into it. + */ + private isPartialStreamStillLive(task: Task, state: TaskPartialStreamState): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) === state + } + private async resetDiffViewAfterWrite(task: Task): Promise { await task.diffViewProvider.reset().catch((resetError) => { console.error("Error resetting write_to_file diff view:", resetError) @@ -484,6 +497,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const provider = task.providerRef.deref() const state = await provider?.getState() + + // Cancelled while provider state was in flight: the teardown already + // released this task's stream state. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -501,6 +521,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { fileExists = task.diffViewProvider.editType === "modify" } else { fileExists = await fileExistsAtPath(absolutePath) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } task.diffViewProvider.editType = fileExists ? "modify" : "create" } @@ -518,12 +541,24 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const partialMessage = JSON.stringify(sharedMessageProps) await task.ask("tool", partialMessage, block.partial).catch(() => {}) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { await task.diffViewProvider.open(relPath!) } + // Cancellation may land while open() is in flight: its abort handler has + // already torn the stream down (and may have reverted or closed this very + // diff view), so streaming the partial content into it now would resurrect a + // view for a task that no longer exists. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + await task.diffViewProvider.update( everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, false, diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 8eaa9c47cc..77a267f57e 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -799,6 +799,92 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + + it("stops before the filesystem probe when the state is released while provider state is in flight", async () => { + // handlePartial() awaits provider.getState() before any side effect. A cancellation + // during that await runs the TaskAborted teardown; the delta already in flight must + // stop there instead of probing, asking and opening a diff view for a dead task. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn(async () => { + writeToFileTool.clearTaskState(mockCline) + return {} + }), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before the partial ask when the state is released during the filesystem probe", async () => { + // Same teardown, one await later. The first delta pins editType, so clear it to + // take the fileExistsAtPath branch again and abort inside it. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.editType = undefined + // mockImplementationOnce: executeWriteFileTool re-arms the default resolved value + // on every call, so a plain mockImplementation would be overwritten. + mockedFileExistsAtPath.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + return false + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before touching the diff view when the stream state is released during an in-flight ask", async () => { + // A cancellation while task.ask() is in flight runs the TaskAborted teardown. The + // delta that was already in flight must not then re-open the diff view for a task + // the user cancelled - that resurrects the state the teardown just released. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.ask.mockImplementation(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before updating the diff view when the task is cancelled while open() is in flight", async () => { + // open() is the first provider await after the partial ask. If TaskAborted lands + // while it is in flight, the teardown has already released this task's stream + // state (and may have reverted or closed this very view), so the delta that is + // already in flight must not stream partial content into a cancelled task's view. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { From 6626914230e6246b532298d587634bd4f9d21ef6 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 03:39:21 +0800 Subject: [PATCH 16/18] fix(write-to-file): let the cancellation teardown own a rejected open()/update() Consistency port of the same fix carried by #1928 (3a0065091), using the isPartialStreamStillLive() helper this unit already has. handlePartial()'s catch treated a rejection from open()/update() as an ordinary filesystem failure. A cancellation that lands while either is in flight runs the TaskAborted teardown first - which releases this task's per-task stream state, reverts or closes this very diff view, and reports the failure itself - and it can also reject the call in flight. The catch then marked the already-released state failed, finalized the partial ask, and ran the failed-stream cleanup a second time: a fresh ask row and a second rollback for a task the user had already cancelled. The catch now checks isPartialStreamStillLive() first, logs the failure, and returns so the teardown owns the outcome. Test: `does not finalize the ask or roll back twice when open() rejects after a cancellation` - open() releases the state and rejects; asserts no update, no finalizePartialToolAsk, no revertChanges, and the state stays released. Negative control: guard removed -> exactly 1 failed; restored -> 1 passed. Verification: core/tools 657 passed / 5 skipped, tsc --noEmit 0 errors, eslint --prune-suppressions --max-warnings=0 clean on both files with src/eslint-suppressions.json unchanged. --- src/core/tools/WriteToFileTool.ts | 12 +++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 26 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0267d6e65c..6ba02de198 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -564,6 +564,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { false, ) } catch (error) { + // A cancellation that lands while open() or update() is in flight runs the + // TaskAborted teardown - which releases this task's stream state, reverts or + // closes this very diff view, and reports the failure itself - and it can also + // reject the call in flight. Marking the already-released state failed, finalizing + // the ask, or running the failed-stream cleanup a second time would resurrect UI + // and roll back twice for a task the user cancelled, so the teardown owns the + // outcome here. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + console.error(`Error streaming write_to_file diff view:`, error) + return + } + // 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 diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 77a267f57e..20fa1497d3 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -885,6 +885,32 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + it("does not finalize the ask or roll back twice when open() rejects after a cancellation", async () => { + // A cancellation during open() releases the stream state through the TaskAborted + // teardown (which also reverts or closes this diff view) AND can reject the call in + // flight. The catch used to mark the released state failed, finalize the ask and run + // the failed-stream cleanup again - a second rollback and a fresh ask row for a task + // the user already cancelled. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + throw new Error("EACCES: permission denied, open mock-file") + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // The teardown that already ran owns the outcome: no finalize and no second rollback. + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { From bcd801ea4311839b01f943a9b01473cc97ddedbd Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 13:23:01 +0800 Subject: [PATCH 17/18] fix(write-to-file): release the partial stream state when the preview is suppressed handlePartial() registers this task's partial-stream entry (and its TaskAborted listener) before it checks the prevent-focus-disruption experiment. With the experiment enabled the delta then returns without ever showing a preview, and nothing else releases that entry: the listener stays attached for the rest of the task's life, and a streamFailed mark armed by an earlier delta keeps suppressing this task's later diff previews. Release the bookkeeping on that return. The execute() exits (validation returns, both approval denials, success and the catch) already tear down explicitly on this branch, so this is the only exit left that skips it. Same root cause as the Lifecycle Resource Cleanup row on the sibling units of #1066; the sibling branches carry the same release in their own PRs. --- src/core/tools/WriteToFileTool.ts | 5 ++++ .../tools/__tests__/writeToFileTool.spec.ts | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 6ba02de198..ae1a85e68c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -510,6 +510,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ) if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 20fa1497d3..2236aca644 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -911,6 +911,30 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + + it("releases the per-task stream state when prevent-focus-disruption skips the partial preview", async () => { + // Delta 1 only pins the path, so the entry is still live afterwards (the stream is in + // flight). Delta 2 reaches the experiment check and returns without ever showing a + // preview: nothing else would release the entry or detach the TaskAborted listener. + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) }) describe("user interaction", () => { From 41ccb425bc5752d66554bbde4aacfa5d2727e0bf Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:08:41 +0800 Subject: [PATCH 18/18] fix(write-to-file): guard every setup step and roll a failed diff-open back Addresses the three #1066/U3 review rows at c1a9f1dcb: - Lifecycle (warning): execute()'s guarded scope now starts before the preflight filesystem work (fileExistsAtPath, createDirectoriesForFile) and ends with an unconditional finally { this.resetTaskPartialState(task) }. A throw from that setup used to escape into BaseTool.handle() with this task's stream entry and TaskAborted listener attached. The same shape was accepted on #1930 (7b783452c). - Persistence Integrity (error): a failed diff-open is now transactional. open() creates the parent directories and an empty placeholder BEFORE it awaits openDiffEditor(), so the catch awaits revertDiffChangesBeforeReset() before resetting: with no active editor the rollback still unlinks the placeholder and removes only the directories this operation created (DiffViewProvider ported verbatim from #1930's 7b783452c / 6cae369d9 - ENOENT tolerated, other failures surfaced, an unapproved dirty buffer discarded rather than saved, a refused discard restoring the pre-stream content and reporting the rollback as failed). - Regression Evidence (warning): focused execute() tests seeding the task's stream state first - missing content, successful completion, a failing write, the preflight directory throw, and the open()-failure rollback - each asserting the entry is removed and the exact TaskAborted listener deregistered. Red first: the preflight-throw test failed before the try/finally move. Negative controls: neutering the finally -> exactly the five exits it owns go red (completion, failure, preflight throw, open-debris, denial); moving the try back below the preflight work -> exactly the preflight test red; removing the revert call in the catch -> exactly the open-debris test red. Verification: writeToFileTool 54/5s, partial-state-cleanup 5, DiffViewProvider 79, Task.spec 172, removeClineFromStack 24 (338 passed); tsc 62 (baseline, none in touched files); eslint 0/0; eslint-suppressions unchanged. --- src/core/tools/WriteToFileTool.ts | 83 ++++--- .../tools/__tests__/writeToFileTool.spec.ts | 105 +++++++++ src/integrations/editor/DiffViewProvider.ts | 131 ++++++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 214 +++++++++++++++++- 4 files changed, 485 insertions(+), 48 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 1adb0312a0..8db69e1912 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -313,46 +313,49 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath) - - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + try { + // The guarded scope starts before the preflight filesystem work: a throw from + // fileExistsAtPath or createDirectoriesForFile must report and tear down like any other + // write failure, not escape into BaseTool.handle() with this task's stream state attached. + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath) + + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + 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) - } + // 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") - } + if (newContent.startsWith("```")) { + newContent = newContent.split("\n").slice(1).join("\n") + } - if (newContent.endsWith("```")) { - newContent = newContent.split("\n").slice(0, -1).join("\n") - } + if (newContent.endsWith("```")) { + newContent = newContent.split("\n").slice(0, -1).join("\n") + } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } + if (!task.api.getModel().id.includes("claude")) { + newContent = unescapeHtmlEntities(newContent) + } - const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" + const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath), - content: newContent, - isOutsideWorkspace, - isProtected: isWriteProtected, - } + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath), + content: newContent, + isOutsideWorkspace, + isProtected: isWriteProtected, + } - try { task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -391,7 +394,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // task, which suppresses the diff preview of every later write_to_file, and // the TaskAborted listener leaks. super.resetPartialState() - this.resetTaskPartialState(task) return } @@ -428,7 +430,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Same exit contract as the saveDirectly branch above: the per-task stream // state and its abort listener belong to this execute() call. super.resetPartialState() - this.resetTaskPartialState(task) return } @@ -451,7 +452,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // one task's execute() would drop another task's streamFailed/streamError // while it is still streaming, so tear down only this task's entry. super.resetPartialState() - this.resetTaskPartialState(task) task.processQueuedMessages() @@ -464,10 +464,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) } await handleError("writing file", error as Error) + + // A failed diff-open is not the end of the story: open() creates the parent directories + // and an empty placeholder BEFORE it awaits openDiffEditor(), so a rejection leaves that + // debris behind. Revert first - with no active editor the rollback still unlinks the + // placeholder and removes only the directories this operation created - then reset. + await this.revertDiffChangesBeforeReset(task) await task.diffViewProvider.reset() super.resetPartialState() - this.resetTaskPartialState(task) return + } finally { + // Unconditional: every exit from the guarded scope - success, denial, or a + // throw - releases this task's stream entry and its TaskAborted listener. + this.resetTaskPartialState(task) } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 2236aca644..a1aabb634e 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -935,6 +935,111 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) }) + + it("releases the per-task stream state when content is missing", async () => { + // The missing-content return sits before execute()'s guarded scope, so it needs its own + // release. The state is seeded first so the assertion proves a release happened rather + // than an empty map. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the + // typed params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + expect(pushToolResult).toHaveBeenCalledWith("Missing param error") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // A stream may have opened a diff view for this call; the early return still closes it. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when the write completes", async () => { + // Seed first: without the seed the map is empty either way and the assertion is vacuous. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when the write itself fails", async () => { + // The catch path tears down too: a failed write must not leave the entry (and its + // streamFailed guard) attached to the task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the preflight directory creation throws", async () => { + // The preflight filesystem work sits before execute()'s guarded scope today: when it + // throws, nothing reports the failure and the stream state leaks. It has to be handled + // like any other write failure - reported once, teardown run. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockedCreateDirectoriesForFile.mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied, mkdir '/new-parent'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("rolls the diff-view debris back when open() fails mid-execute", async () => { + // open() creates the parent directories and an empty placeholder before it awaits + // openDiffEditor(). When it rejects, the write failed but the debris did not: the catch + // has to await the rollback (which removes the placeholder and only the directories this + // operation created) before resetting the view, or the next execute() mistakes the + // placeholder for an existing file. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + const revertOrder = mockCline.diffViewProvider.revertChanges.mock.invocationCallOrder[0] + const resetOrder = mockCline.diffViewProvider.reset.mock.invocationCallOrder.at(-1) + expect(resetOrder).toBeGreaterThan(revertOrder) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..ed7f8a3d15 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -21,6 +21,11 @@ import { Task } from "../../core/task/Task" import { DecorationController } from "./DecorationController" +/** Narrow ENOENT test so a rollback can tolerate a file or dir that never landed. */ +function isEnoent(error: unknown): boolean { + return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT" +} + export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes" @@ -513,13 +518,38 @@ export class DiffViewProvider { return JSON.stringify(result) } + /** + * Remove a file this edit created. Tolerates ENOENT: when open() failed before the + * placeholder was written (or the write itself failed) there is nothing to delete, and + * that must not abort the rollback. + */ + private async removeCreatedFile(absolutePath: string): Promise { + try { + await fs.unlink(absolutePath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + + /** Same tolerance for a directory this edit created that never made it to disk. */ + private async removeCreatedDir(dirPath: string): Promise { + try { + await fs.rmdir(dirPath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + async revertChanges(): Promise { - if (!this.relPath || !this.activeDiffEditor) { + if (!this.relPath) { return } const fileExists = this.editType === "modify" - const updatedDocument = this.activeDiffEditor.document const absolutePath = path.resolve(this.cwd, this.relPath) // Stop tracking touches and cancel any pending scroll-to-diff before any @@ -528,21 +558,43 @@ export class DiffViewProvider { this.cancelDeferredScroll() if (!fileExists) { - if (updatedDocument.isDirty) { - await updatedDocument.save() + // open() creates the parent directories and an empty placeholder file BEFORE it + // awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and + // the previous early return here left the placeholder and the new directories on + // disk: the next execute() then saw an empty file and treated the requested new file + // as an existing one, so a denial preserved the debris. The filesystem rollback runs + // either way; only the document work needs an editor. + if (this.activeDiffEditor) { + const updatedDocument = this.activeDiffEditor.document + if (updatedDocument.isDirty) { + // The buffer holds the streamed content of a write that was never approved. Saving + // it here persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. Discard the buffer + // (force-close the tab) instead; the file goes away a moment later anyway. + await this.discardFileTab(absolutePath) + await this.closeAllDiffViews() + } else { + await this.closeAllDiffViews() + // The file was newly created for this edit; close its transiently + // opened tab before deleting it from disk. + await this.closeFileTab(absolutePath) + } } - await this.closeAllDiffViews() - // The file was newly created for this edit; close its transiently - // opened tab before deleting it from disk. - await this.closeFileTab(absolutePath) - await fs.unlink(absolutePath) + await this.removeCreatedFile(absolutePath) // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { - await fs.rmdir(this.createdDirs[i]) + await this.removeCreatedDir(this.createdDirs[i]) } + } else { + // Only reachable after a successful open(), so the editor exists. + const updatedDocument = this.activeDiffEditor?.document + if (!updatedDocument) { + return + } + // Revert document. const edit = new vscode.WorkspaceEdit() @@ -847,6 +899,65 @@ export class DiffViewProvider { // Close the plain (non-diff) editor tab for the target file. Used when the // file was opened transiently for the diff and the user never interacted // with it, so it should not linger after accept/deny. + /** + * Close the tab for this path WITHOUT saving. Used by the new-file rollback, where the + * buffer holds unapproved streamed content that must never reach disk; closeFileTab() + * deliberately skips dirty tabs, so a dirty buffer needs the forced close. + * + * A close that fails or is refused is a rollback failure: the buffer would still hold the + * content the user never approved, one save away from disk. Put the pre-stream content back + * so nothing saveable survives, and propagate - the caller must not keep deleting around a + * buffer it could not discard. + */ + private async discardFileTab(absolutePath: string): Promise { + const tabs = vscode.window.tabGroups.all + .flatMap((group) => group.tabs) + .filter( + (tab) => + tab.input instanceof vscode.TabInputText && + tab.input.uri.scheme === "file" && + arePathsEqual(tab.input.uri.fsPath, absolutePath), + ) + + for (const tab of tabs) { + let closed: boolean + let closeError: Error | undefined + try { + closed = await vscode.window.tabGroups.close(tab, true) + } catch (error) { + closed = false + closeError = error instanceof Error ? error : new Error(String(error)) + } + if (!closed) { + await this.restorePreStreamBuffer(absolutePath) + throw new Error( + `Rollback could not discard the buffer for ${absolutePath}; its unapproved content was restored to the pre-stream state instead.`, + { cause: closeError }, + ) + } + } + } + + /** + * Replace an open buffer's content with what it held before streaming started, so a buffer + * that could not be closed never keeps unapproved content available to save. + */ + private async restorePreStreamBuffer(absolutePath: string): Promise { + const document = vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!document) { + return + } + const edit = new vscode.WorkspaceEdit() + const range = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ) + edit.replace(document.uri, range, this.originalContent ?? "") + await vscode.workspace.applyEdit(edit) + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..89963acfad 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,3 +1,4 @@ +import * as fs from "fs/promises" import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" import * as vscode from "vscode" import * as path from "path" @@ -16,6 +17,10 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + // revertChanges() rolls a new-file edit back by deleting the placeholder and the + // directories the edit created. + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -52,7 +57,7 @@ vi.mock("vscode", () => ({ onDidChangeTextEditorVisibleRanges: vi.fn(() => ({ dispose: vi.fn() })), tabGroups: { all: [], - close: vi.fn(), + close: vi.fn().mockResolvedValue(true), activeTabGroup: { activeTab: undefined }, }, visibleTextEditors: [], @@ -1172,6 +1177,213 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() removes the placeholder and created dirs when open() failed before the editor existed", async () => { + // open() creates the parent dirs and an empty placeholder BEFORE it awaits + // openDiffEditor(). If that await rejects there is no activeDiffEditor, and the + // rollback used to bail out - leaving an empty file that the next execute() mistook + // for an existing file, which a denial then preserved. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() tolerates a placeholder that was never written", async () => { + // The failed open may have died before fs.writeFile ran; ENOENT during the + // rollback is success, not a new failure that would abort the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + // The rollback still attempted the delete - the tolerance is about not + // aborting the rest of the cleanup, not about skipping it. + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + }) + + it("revertChanges() stops before the document work for an existing file when no editor exists", async () => { + // The modify branch used to dereference activeDiffEditor unconditionally. When open() + // never produced an editor there is no document to restore, and the old code threw a + // TypeError out of the denial path instead of finishing the teardown. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "modify", + createdDirs: [], + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("revertChanges() keeps rolling back the remaining directories when one rmdir hits ENOENT", async () => { + // Same tolerance as the placeholder, for the created dirs: a directory that never + // landed must not abort the rest of the rollback and must not surface as a new failure. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(2) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() surfaces a non-ENOENT directory failure instead of swallowing it", async () => { + // The tolerance is scoped to ENOENT: a directory that exists but could not be removed + // is a real rollback failure and must reach the caller rather than be hidden. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") + }) + + it("revertChanges() surfaces a placeholder delete that failed for a non-ENOENT reason", async () => { + // The ENOENT tolerance is scoped: an unlink that fails for another reason means the + // unapproved placeholder is still on disk, and the caller must not be told the + // rollback succeeded. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EACCES: permission denied") + }) + + it("revertChanges() discards the streamed buffer instead of saving unapproved content for a new file", async () => { + // The diff buffer holds the streamed content of a write that was never approved. + // Saving it here persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + expect(editor.document.save).not.toHaveBeenCalled() + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab, true) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() reports a discard the editor refused instead of leaving the buffer open", async () => { + // A forced close can still fail (a vetoing editor). Swallowing that left the unapproved + // streamed content in an open, saveable buffer while the rollback kept deleting the + // file underneath it. The failure must propagate, and the buffer must be put back to + // its pre-stream state so nothing saveable survives. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const streamedBuffer = { + uri: makeUri(mockTargetPath), + getText: vi.fn().mockReturnValue("streamed content"), + isDirty: true, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + } + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + value: [streamedBuffer], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + vi.mocked(vscode.window.tabGroups.close).mockRejectedValueOnce(new Error("close vetoed")) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not discard the buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocuments) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + } + } + + // The buffer went back to its pre-stream state, and the rollback stopped rather than + // deleting the file out from under a buffer it could not discard. + expect(vscode.workspace.applyEdit).toHaveBeenCalled() + // The buffer was put back to its pre-stream state (empty for a new file): the edit that + // was applied replaces the streamed text with originalContent. + const appliedEdit = vi.mocked(vscode.WorkspaceEdit).mock.results[0].value + expect(appliedEdit.replace).toHaveBeenCalledWith( + expect.objectContaining({ fsPath: mockTargetPath }), + expect.anything(), + "", + ) + }) + it("revertChanges() closes the file tab when the file was not open and untouched", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true)