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/24] 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/24] 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/24] 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/24] 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 950bf6aa308884423b041b17a645e98d1bcef1cb Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 01:23:53 +0800 Subject: [PATCH 05/24] chore(ci): re-trigger the unit suite after a flaky writeToFileTool partial-path case The core project passes locally at this head (174 files, 3274 tests) and the case passes in isolation and with the whole core/tools directory. The ubuntu run reported 0 calls to createDirectoriesForFile on the stabilized-path assertion, which does not reproduce; re-running to confirm. From 6906c026a51c3cff8c80f201c12cf6d1d0d422da Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 02:52:12 +0800 Subject: [PATCH 06/24] test(write-to-file): align the partial-path case with handlePartial's no-filesystem contract platform-unit-test (ubuntu-latest) fails on this branch while it passes locally, because the failing case is it.skipIf(process.platform === "win32"): Windows CI and every local run skip it. The case predates this unit. It asserted that the second streaming delta calls createDirectoriesForFile, which is exactly the call this unit removes: an unguarded mkdir in handlePartial threw EROFS up into BaseTool.handle(), which never set didRejectTool/didAlreadyUseTool, so presentAssistantMessage's advancement gate was never reached and the agent loop stalled. The unit's own regression test ("EROFS in handlePartial does not stall agent loop") pins the new contract; this older case still asserted the old one, so the two contradicted and only Linux CI noticed. Rewritten as "defers parent directory creation to execute() while streaming": no filesystem work during streaming, and the directories are still created by the authoritative non-partial execute(). Same intent, new contract. Local run: 34 passed / 5 skipped in the file; the rewritten case also passes when the win32 skip is lifted temporarily, so the flow is verified on this machine too. --- 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 f4677277d7..48a348634a 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 e0b0e0e63dc5ccb1dd14d3e3c5cd582691e4511c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 03:38:03 +0800 Subject: [PATCH 07/24] fix(tools): release the failed task's stream state without clobbering other tasks BaseTool.handle()'s parameter-parse branch reported the error and returned without any teardown, so a task whose streaming delta had failed kept streamFailed in this singleton: every later write_to_file in that task then skipped the diff preview. execute() never runs on that path, so nothing else released it. Adds a protected BaseTool.clearTaskStreamState(task) hook (no-op by default) called from that catch, and WriteToFileTool overrides it with resetTaskPartialState(task). The hook is per-task on purpose: these tool instances are singletons shared by concurrent tasks, and the existing global resetPartialState() clears the whole taskPartialStreamState map. The same cross-task hazard applies inside execute(), which called that map-wide reset on both its success and error paths: task A's write was deleting task B's streamFailed/streamError while B was still streaming (duplicate partial ask, lost error). execute() now calls super.resetPartialState() for the genuinely instance-global base field plus resetTaskPartialState(task). The error path also finalizes the partial ask that the diff-view branch opened, so a failed write no longer leaves the spinner and Save/Reject live. Tests (writeToFileTool.spec.ts, per-task stream state isolation): parse-failure teardown releases this task and keeps the other task's entry; another task's streamFailed/streamError survive execute(); a failing save finalizes the ask with the exact partial payload. All three fail on the pre-fix code (3 failed / 34 passed) and pass after (37 passed). Local: eslint clean on all three files with --prune-suppressions (no suppression change), package tsc clean. --- src/core/tools/BaseTool.ts | 13 ++++ src/core/tools/WriteToFileTool.ts | 24 +++++- .../tools/__tests__/writeToFileTool.spec.ts | 74 +++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..d2e611d94a 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -98,6 +98,13 @@ export abstract class BaseTool { this.lastSeenPartialPath = undefined } + /** + * Release the state this tool holds for one task on paths that never reach + * execute(). No-op for tools without per-task state; the scope is a single task + * because tool instances are singletons shared by concurrent tasks. + */ + protected clearTaskStreamState(_task: Task): void {} + /** * Main entry point for tool execution. * @@ -158,6 +165,12 @@ export abstract class BaseTool { 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)) + // execute() never runs on this path, so a tool that keeps per-task streaming + // state must still release THIS task's state; a completed block that failed + // finalization would otherwise leave a failure flag suppressing later previews + // for the same task. Per-task only: a global teardown would clobber another + // task that is still streaming through this singleton. + this.clearTaskStreamState(task) // 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 diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 267c728c27..7424690f9f 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -174,6 +174,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * "writing file" context execute()'s catch uses, and suppress the incidental * parse error. */ + protected override clearTaskStreamState(task: Task): void { + this.resetTaskPartialState(task) + } + override resetPartialState(): void { super.resetPartialState() for (const state of this.taskPartialStreamState.values()) { @@ -186,6 +190,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++ @@ -293,6 +300,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) } @@ -336,15 +344,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 48a348634a..e18567d196 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -612,6 +612,80 @@ describe("writeToFileTool", () => { }) }) + describe("per-task stream state isolation", () => { + // A second task streaming through the same singleton while mockCline 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) + }) + + it("releases this task's stream state when the completed block fails to parse", async () => { + // A streaming delta had failed, so the guard is set; the final block then + // arrives without nativeArgs, so execute() never runs. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + const other = buildStreamingTask("task-2", "instance-2") + writeToFileTool["getTaskPartialStreamState"](other as never).streamFailed = true + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // Otherwise the retained streamFailed suppresses the diff preview of every + // later write_to_file in this task. + expect(writeToFileTool["taskPartialStreamState"].has(`${mockCline.taskId}.${mockCline.instanceId}`)).toBe(false) + // ...and the cleanup must stay scoped: the other task is still streaming. + expect(writeToFileTool["taskPartialStreamState"].get("task-2.instance-2")?.streamFailed).toBe(true) + }) + }) + describe("user interaction", () => { it("reverts changes when user rejects approval", async () => { mockAskApproval.mockResolvedValue(false) From 77526645737972f85df176358a01122362011bef Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 19:24:10 +0800 Subject: [PATCH 08/24] fix(diff-view): roll back the created file and dirs when open() fails early DiffViewProvider.open() creates the parent directories and writes an empty placeholder BEFORE it awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and revertChanges() bailed out on `!this.activeDiffEditor` - leaving 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 instead of removing it. The streaming-failure cleanup in handlePartial() calls exactly this rollback, so the debris was reachable from the new path in this unit. The filesystem rollback now runs regardless of the editor: only the document work (save the dirty buffer, close the diff views and the tab) needs one. removeCreatedFile/removeCreatedDir tolerate ENOENT, since the failed open may never have written the placeholder - that must not abort the rest of the cleanup. Tests: 'revertChanges() removes the placeholder and created dirs when open() failed before the editor existed' (unlink for the relPath, rmdir in reverse order) and 'revertChanges() tolerates a placeholder that was never written' (ENOENT rejection does not propagate, delete still attempted). Pin: restoring the old `!this.activeDiffEditor` early return fails both. Local: integrations/editor + writeToFileTool.spec + core/task + assistant-message = 786 passed (the single remaining failure, saveChanges default delay, reproduces without these changes); tsc 0; eslint 0 err / 0 warn on both files. --- src/integrations/editor/DiffViewProvider.ts | 67 ++++++++++++++++--- .../editor/__tests__/DiffViewProvider.spec.ts | 43 ++++++++++++ 2 files changed, 100 insertions(+), 10 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..112a9ba67b 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,38 @@ 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) { + await updatedDocument.save() + } + + 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() diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index aee88f4061..5949aa1905 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" @@ -15,6 +16,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 @@ -1165,6 +1170,44 @@ 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() 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) From 753c22f4fc439166d37c804e2297bfc0b4dcc6c8 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 19:31:00 +0800 Subject: [PATCH 09/24] fix(tools): report the captured stream failure on the parse-failure path The unit captured streamError for exactly one reason - when the finalized block never reaches execute(), the authoritative filesystem retry in execute() never happens, and the captured error is the only failure the user can act on. Nothing read it: the hook released the state but BaseTool still reported the incidental parse error, so the capture was dead and the real failure was hidden behind a parse message. clearTaskStreamState becomes releaseStreamStateOnParseFailure(task, callbacks): it releases THIS task's state, restores the diff document the stream opened (revert before reset, as the other teardowns do), and when a streamError was captured reports it with the same 'writing file' context execute()'s catch uses, returning true so BaseTool suppresses the generic parse error. The failure therefore surfaces exactly once. With no captured error the generic parse error is still reported, and the default hook for other tools keeps returning false. execute()'s early returns (missing path, missing content, rooignore denial) returned before the success/catch cleanup, leaving the per-task state and its TaskAborted listener behind for the task's lifetime - and a retained streamFailed suppresses the diff preview of every later write_to_file in that task. They now release only this task's state. Tests: 'reports the captured streaming failure once instead of the incidental parse error' (handleError called once with the captured error, not with the parse error, state released, revertChanges + reset ran) and 'releases the per-task stream state when execute() returns early on a denied path'. Pins: returning false instead of reporting fails the first; removing the release in the rooignore branch fails the second. Local: core/tools + writeToFileTool.spec + assistant-message + core/task = 1420 passed / 5 skipped; tsc 0; eslint 0 err / 0 warn on all three files. --- src/core/tools/BaseTool.ts | 29 ++++++++--- src/core/tools/WriteToFileTool.ts | 51 ++++++++++++++----- .../tools/__tests__/writeToFileTool.spec.ts | 42 +++++++++++++++ 3 files changed, 102 insertions(+), 20 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index d2e611d94a..56d8c33b59 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -103,7 +103,21 @@ export abstract class BaseTool { * execute(). No-op for tools without per-task state; the scope is a single task * because tool instances are singletons shared by concurrent tasks. */ - protected clearTaskStreamState(_task: Task): void {} + /** + * Teardown boundary for the handle() parse-failure path, where execute() never + * runs. Default: there is no per-task streaming state to release, so the generic + * parse error is what the user sees. A tool that keeps per-task stream state may + * release it, restore any diff document a stream opened, and report a more specific + * failure - returning true suppresses the incidental parse error so the failure is + * reported exactly once. Per-task only: a global teardown would clobber another task + * that is still streaming through this singleton. + */ + protected async releaseStreamStateOnParseFailure( + _task: Task, + _callbacks: ToolCallbacks, + ): Promise { + return false + } /** * Main entry point for tool execution. @@ -164,13 +178,14 @@ export abstract class BaseTool { } 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)) // execute() never runs on this path, so a tool that keeps per-task streaming - // state must still release THIS task's state; a completed block that failed - // finalization would otherwise leave a failure flag suppressing later previews - // for the same task. Per-task only: a global teardown would clobber another - // task that is still streaming through this singleton. - this.clearTaskStreamState(task) + // state must still release THIS task's state and restore any diff document the + // stream opened. If a streaming delta already hit a fatal error, the tool reports + // that (the actionable failure) and the incidental parse error is suppressed. + const reportedStreamFailure = await this.releaseStreamStateOnParseFailure(task, callbacks) + if (!reportedStreamFailure) { + 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 diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 7424690f9f..b657bc82e9 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -160,22 +160,38 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } /** - * Teardown boundary for the handle() parse-failure path, where execute() never - * runs and therefore its finally (resetTaskPartialState) never runs either. + * Teardown for the handle() parse-failure path, where execute() never runs and its + * cleanup 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. + * Releases the per-task stream state: otherwise the abort listener leaks for the task's + * lifetime, and a failed streaming delta leaves the streamFailed guard suppressing 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 here, so a user save could persist it. When a + * streaming delta already hit a fatal filesystem error, THAT is the failure the user can + * act on, so it is reported with the same "writing file" context execute()'s catch uses, + * and true is returned to suppress the incidental parse error - the failure surfaces + * exactly once. */ - protected override clearTaskStreamState(task: Task): void { + protected override async releaseStreamStateOnParseFailure( + task: Task, + callbacks: ToolCallbacks, + ): Promise { + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (!state) { + return false + } + this.resetTaskPartialState(task) + await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + + if (!state.streamError) { + return false + } + + await callbacks.handleError("writing file", state.streamError) + return true } override resetPartialState(): void { @@ -197,6 +213,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") + // No execute() cleanup on this early return: release THIS task's stream state + // (and only this task's) so the abort listener and the streamFailed guard do not + // outlive the call. + this.resetTaskPartialState(task) pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) await task.diffViewProvider.reset() return @@ -205,6 +225,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (newContent === undefined) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") + // No execute() cleanup on this early return: release THIS task's stream state + // (and only this task's) so the abort listener and the streamFailed guard do not + // outlive the call. + this.resetTaskPartialState(task) pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) await task.diffViewProvider.reset() return @@ -214,6 +238,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) + this.resetTaskPartialState(task) pushToolResult(formatResponse.rooIgnoreError(relPath)) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index e18567d196..e63799bde3 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -610,6 +610,48 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(1) }) + it("reports the captured streaming failure once instead of the incidental parse error", async () => { + // A streaming delta hit a fatal filesystem error and the finalized block then + // arrives without nativeArgs: execute() never runs, so its authoritative retry of + // the same filesystem operation never happens either. The captured error is the one + // the user can act on, and it must surface exactly once. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + const streamFailure = new Error("EACCES: stream open failed") + state.streamError = streamFailure + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", streamFailure) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The stream may have left a diff view open with content that was never approved. + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when execute() returns early on a denied path", async () => { + // The rooignore branch returns before execute()'s success/catch cleanup; without + // this the abort listener and the streamFailed guard outlive the call and suppress + // 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)) + }) }) describe("per-task stream state isolation", () => { From 875110d16668470dc240b9a385e4966ba85a41d0 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 19:41:07 +0800 Subject: [PATCH 10/24] test(tools): move the cleanup tests under a describe that matches them The parse-failure reporting test and the denied-path cleanup test landed inside describe("resetPartialState") even though neither calls resetPartialState(); they exercise the handle() parse-failure teardown and execute()'s early-return release. They now live in describe("parse-failure reporting and early-return cleanup") as a sibling, leaving only the resetPartialState-specific test in the original block. Local: writeToFileTool.spec 44 tests (39 passed / 5 skipped); core/tools + assistant-message + core/task = 1420 passed / 5 skipped; eslint 0 err / 0 warn. --- src/core/tools/__tests__/writeToFileTool.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index e63799bde3..dd58deb026 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -610,6 +610,10 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(1) }) + + }) + + describe("parse-failure reporting and early-return cleanup", () => { it("reports the captured streaming failure once instead of the incidental parse error", async () => { // A streaming delta hit a fatal filesystem error and the finalized block then // arrives without nativeArgs: execute() never runs, so its authoritative retry of @@ -640,7 +644,6 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("releases the per-task stream state when execute() returns early on a denied path", async () => { // The rooignore branch returns before execute()'s success/catch cleanup; without // this the abort listener and the streamFailed guard outlive the call and suppress @@ -653,7 +656,6 @@ describe("writeToFileTool", () => { expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) }) }) - describe("per-task stream state isolation", () => { // A second task streaming through the same singleton while mockCline runs. // Structural double, same pattern as the partial-state-cleanup spec. @@ -694,6 +696,7 @@ describe("writeToFileTool", () => { 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) From bc9a1562bf5e8d3980a267a98cf849d3101497cf Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 13:35:06 +0800 Subject: [PATCH 11/24] 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 already tear down through the finally block 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. Regression evidence for the revertChanges rollback this unit changed, at the owning layer: - an existing-file revert with no activeDiffEditor stops at the guard instead of dereferencing it (without the guard the call rejects with a TypeError reading 'document'), - a created directory that never landed (rmdir ENOENT) does not abort the remaining rollback, - a non-ENOENT rmdir failure still reaches the caller instead of being swallowed. Negative controls: removing the PFD release turns exactly the new stream-state test red; removing the existing-file guard turns exactly the revert test red; dropping the dir ENOENT tolerance turns exactly the rollback test red; swallowing every dir error turns exactly the non-ENOENT test red. All restored green afterwards. --- src/core/tools/WriteToFileTool.ts | 5 ++ .../tools/__tests__/writeToFileTool.spec.ts | 40 +++++++++++++++ .../editor/__tests__/DiffViewProvider.spec.ts | 51 +++++++++++++++++++ 3 files changed, 96 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index b657bc82e9..34840bca96 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -424,6 +424,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 dd58deb026..e28ee4c858 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -729,6 +729,46 @@ describe("writeToFileTool", () => { // ...and the cleanup must stay scoped: the other task is still streaming. expect(writeToFileTool["taskPartialStreamState"].get("task-2.instance-2")?.streamFailed).toBe(true) }) + it("releases this task's stream state when prevent-focus-disruption skips the partial preview", async () => { + // handlePartial() registers the per-task entry (and its TaskAborted listener) before it + // checks the experiment. With the experiment on, the delta returns without ever opening a + // preview and never reaches execute()'s teardown, so the entry and the listener would stay + // attached for the rest of the task's life - and a streamFailed mark armed by an earlier + // delta would keep suppressing this task's later diff previews. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + const delta = (content: string) => + writeToFileTool.handle( + mockCline, + { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content }, + nativeArgs: { path: testFilePath, content }, + partial: true, + } as ToolUse<"write_to_file">, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }, + ) + + // The first delta only pins the path; the second is the one that reaches the check. + await delta("Line 1") + await delta("Line 1\nLine 2") + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) + }) describe("user interaction", () => { diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 5949aa1905..5c2da6ca2a 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1208,6 +1208,57 @@ describe("DiffViewProvider", () => { 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() 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) From 7b783452ca7b9667898423d1def04940533e6277 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:46:12 +0800 Subject: [PATCH 12/24] fix(write-to-file): report rollback failure, discard unapproved buffer, guard every execute exit Addresses the three #1066/U5 review rows: - Persistence Integrity (error): revertChanges() no longer saves the dirty diff buffer for a new-file rollback - the buffer holds content the user never approved, and saving it persisted exactly what the rollback is undoing before the placeholder was deleted. It force-closes the tab instead. A failing revertChanges() is also no longer swallowed: revertDiffChangesBeforeReset() returns the error and the failure paths record/report it (original streaming error kept as cause). - Lifecycle (warning): execute()'s guarded scope now starts before the preflight filesystem work (fileExistsAtPath / createDirectoriesForFile) and ends in a finally that releases this task's per-task stream state, so both approval denials, the catch, and a preflight throw all tear down. - Regression Evidence (warning): added the new-file rollback test with a non-ENOENT unlink failure asserting revertChanges() rejects, plus the discard-instead-of-save test and three execute()-exit state tests. Red first, then green; negative controls: removing the finally -> exactly the 3 exit tests red; swallowing the revert error -> exactly the rollback test red; saving instead of discarding -> exactly the discard test red; deregistering a mismatched listener -> the 6 listener tests red. Also pins the CR threads: the abort listener registration is asserted before clearTaskState, and off() is asserted against the exact listener reference captured from once() instead of any function. Verification: writeToFileTool 44/5s, partial-state-cleanup 5, DiffViewProvider 78, Task.spec 172; tsc 62 (baseline, none in touched files); eslint 0/0; eslint-suppressions unchanged. --- src/core/tools/WriteToFileTool.ts | 132 +++++++++++------- ...teToFileTool-partial-state-cleanup.spec.ts | 4 + .../tools/__tests__/writeToFileTool.spec.ts | 103 +++++++++++++- src/integrations/editor/DiffViewProvider.ts | 41 +++++- .../editor/__tests__/DiffViewProvider.spec.ts | 55 ++++++++ 5 files changed, 279 insertions(+), 56 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 8284689db2..4fe02ae0ca 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -143,13 +143,18 @@ 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. A revert failure is RETURNED rather than dropped: the caller records it as the + * failure this stream produced, so debris left on disk is reported instead of being + * silently continued past. */ - private async revertDiffChangesBeforeReset(task: Task): Promise { - await task.diffViewProvider.revertChanges().catch((revertError) => { + private async revertDiffChangesBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.revertChanges() + } catch (revertError) { console.error("Error reverting write_to_file diff view changes:", revertError) - }) + return revertError instanceof Error ? revertError : new Error(String(revertError)) + } + return undefined } private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { @@ -182,15 +187,27 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } this.resetTaskPartialState(task) - await this.revertDiffChangesBeforeReset(task) + const rollbackError = await this.revertDiffChangesBeforeReset(task) await this.resetDiffViewAfterWrite(task) - if (!state.streamError) { - return false + // A failed rollback is the more actionable failure (debris is still on disk), so it takes + // the report slot when present; the streaming error is kept behind it as the cause. + if (rollbackError) { + await callbacks.handleError( + "writing file", + new Error(`write_to_file rollback failed after a streaming error: ${rollbackError.message}`, { + cause: state.streamError ?? rollbackError, + }), + ) + return true } - await callbacks.handleError("writing file", state.streamError) - return true + if (state.streamError) { + await callbacks.handleError("writing file", state.streamError) + return true + } + + return false } override resetPartialState(): void { @@ -244,46 +261,50 @@ 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) + try { + // The preflight filesystem work sits inside the guarded scope on purpose: a throw from + // fileExistsAtPath / createDirectoriesForFile must be reported like any other write + // failure, and the per-task stream state must still be released. - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath) - // 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 (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - if (newContent.startsWith("```")) { - newContent = newContent.split("\n").slice(1).join("\n") - } + // 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.endsWith("```")) { - newContent = newContent.split("\n").slice(0, -1).join("\n") - } + if (newContent.startsWith("```")) { + newContent = newContent.split("\n").slice(1).join("\n") + } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } + if (newContent.endsWith("```")) { + newContent = newContent.split("\n").slice(0, -1).join("\n") + } - const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + if (!task.api.getModel().id.includes("claude")) { + newContent = unescapeHtmlEntities(newContent) + } - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath), - content: newContent, - isOutsideWorkspace, - isProtected: isWriteProtected, - } + 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, + } - try { task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -369,10 +390,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.diffViewProvider.reset() // 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) + // BaseTool's reset only clears this instance's lastSeenPartialState; the per-task + // entry is released by the finally below (clearing the whole map from one task's + // execute() would drop another task's streamFailed/streamError while it is still + // streaming). task.processQueuedMessages() @@ -387,8 +408,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await handleError("writing file", error as Error) await task.diffViewProvider.reset() super.resetPartialState() - this.resetTaskPartialState(task) return + } finally { + // One teardown covers every exit of the guarded scope: success, both approval denials, + // the catch, and any throw from the preflight filesystem work. Idempotent, so the + // explicit releases on the early returns above stay correct. + this.resetTaskPartialState(task) } } @@ -484,7 +509,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { 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) + const rollbackError = await this.revertDiffChangesBeforeReset(task) + if (rollbackError) { + // The rollback did not finish: the placeholder and any created directories are still + // on disk, and the next execute() would treat that debris as an existing file. A + // logged-only revert failure hides exactly that, so the rollback failure becomes the + // failure this stream reports - the original streaming error stays reachable as the + // cause, and the report still happens exactly once (on the parse-failure path). + partialStreamState.streamError = new Error( + `write_to_file rollback failed after a streaming error: ${rollbackError.message}`, + { cause: partialStreamState.streamError }, + ) + } await this.resetDiffViewAfterWrite(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 4506742c26..5094c2dc81 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -49,6 +49,10 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { const task = buildTask("cleanup-task", "inst-1") const state = writeToFileTool["getTaskPartialStreamState"](task) expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + // The registration side of the pairing, asserted BEFORE the cleanup runs: the off() + // assertion below only proves the right listener was deregistered if this one pins which + // listener was registered in the first place. + expect((task as unknown as CleanupTask).once).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, state.abortCleanup) writeToFileTool.clearTaskState(task) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index e28ee4c858..a17ebec8db 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -653,7 +653,13 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { accessAllowed: false }) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) - expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) }) }) describe("per-task stream state isolation", () => { @@ -766,7 +772,75 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) - expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + 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 user rejects the diff-view approval", async () => { + // The diff-view denial returns from inside execute()'s try. Without a release on that + // path the entry and its TaskAborted listener survive a rejected write, and a + // streamFailed mark armed by an earlier delta keeps suppressing this task's later + // diff previews. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockAskApproval.mockResolvedValueOnce(false) + + await executeWriteFileTool({}) + + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + 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 prevent-focus-disruption approval is rejected", async () => { + // Same leak on the experiment branch: the denial returns without a teardown, so the + // listener stays attached for the rest of the task's life. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockAskApproval.mockResolvedValueOnce(false) + + await executeWriteFileTool({}) + + expect(mockAskApproval).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + 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) }) }) @@ -862,6 +936,31 @@ describe("writeToFileTool", () => { expect(mockHandleError).not.toHaveBeenCalled() }) + it("keeps a failed rollback as the stream failure instead of dropping it", async () => { + // Streaming failed (open() rejected) and the rollback that follows failed too: the + // placeholder and any created directories are still on disk. Logging the revert failure + // and dropping it leaves the next execute() to treat that debris as an existing file, so + // the rollback failure has to become the failure this stream reports. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue( + Object.assign(new Error("EACCES: rollback failed"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches open(). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + expect(state?.streamFailed).toBe(true) + expect(state?.streamError?.message).toContain("rollback failed") + // The original streaming error must stay reachable behind the reported one. + expect((state?.streamError?.cause as Error | undefined)?.message).toBe( + "EACCES: permission denied, open '/ro/test.py'", + ) + }) + 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( diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 112a9ba67b..7c4122f4d2 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -567,13 +567,18 @@ export class DiffViewProvider { if (this.activeDiffEditor) { const updatedDocument = this.activeDiffEditor.document if (updatedDocument.isDirty) { - await updatedDocument.save() + // 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 this.removeCreatedFile(absolutePath) @@ -894,6 +899,30 @@ 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. + */ + 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) { + try { + await vscode.window.tabGroups.close(tab, true) + } catch (err) { + console.error(`Failed to discard file tab ${tab.label}`, err) + } + } + } + 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 6575032014..fef8583da7 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1266,6 +1266,61 @@ describe("DiffViewProvider", () => { 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() 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) From 6cae369d9f7f050aa5af5a30152c50475b958e10 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:21:56 +0800 Subject: [PATCH 13/24] fix(diff-view): treat a refused discard as a rollback failure Addresses the remaining #1066/U5 review rows: - Persistence Integrity (error): discardFileTab() no longer swallows a failed or refused tabGroups.close(). Before, a buffer holding content the user never approved stayed open and saveable while the rollback deleted the file underneath it. It now restores the buffer to its pre-stream content (restorePreStreamBuffer) and propagates the failure, so revertChanges() stops and the caller records the rollback as failed instead of continuing. - Regression Evidence (warning): added the combined-path test - a partial block whose diff-view open() fails (failure captured, not reported), then the completed block with valid nativeArgs whose execute() retry fails - asserting handleError runs exactly once with the execute() failure and never with the captured streaming error. - Inline thread (WriteToFileTool.ts:190-210): added the handle() test for parse-failure cleanup with a captured streamError, a rejected revertChanges() and a final block without nativeArgs; asserts one handleError call with "writing file", the rollback-failure message and the captured error as cause, and that the parse error is not reported. These two tests also cover the mutation-diff NoCoverage advisories at WriteToFileTool.ts:198-202. Red first: the discard-failure test failed with 'promise resolved "undefined" instead of rejecting' before the fix. Negative controls: swallowing the close failure again -> exactly that test red; removing the pre-stream restore -> exactly that test red; making execute() report the captured streaming error -> exactly the combined-path test red; dropping the revert error again -> exactly the two rollback tests red. Verification: writeToFileTool 46/5s, partial-state-cleanup 5, DiffViewProvider 79, Task.spec 172, presentAssistantMessage 20, removeClineFromStack 24 (346 passed total); tsc 62 (baseline, none in touched files); eslint 0/0; eslint-suppressions unchanged. --- .../tools/__tests__/writeToFileTool.spec.ts | 70 +++++++++++++++++++ src/integrations/editor/DiffViewProvider.ts | 41 ++++++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 65 ++++++++++++++++- 3 files changed, 172 insertions(+), 4 deletions(-) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index a17ebec8db..aea85fe09e 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -961,6 +961,76 @@ describe("writeToFileTool", () => { ) }) + it("reports only the execute() failure when the write is retried after a failed stream", async () => { + // Combined path: a streaming delta failed (the failure is captured, not reported), then + // the completed block arrives with valid nativeArgs, so execute() runs and retries the + // same operation. Its failure is the single failure the user hears about - the captured + // streaming error must not also surface, or the same write reports twice. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches open() and captures the failure. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + expect(state?.streamFailed).toBe(true) + expect(state?.streamError?.message).toBe("EACCES: permission denied, open '/ro/test.py'") + + // The completed block: open() works now, the write itself fails. + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("EROFS: read-only file system, write")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith( + "writing file", + expect.objectContaining({ message: "EROFS: read-only file system, write" }), + ) + expect(mockHandleError).not.toHaveBeenCalledWith( + "writing file", + expect.objectContaining({ message: "EACCES: permission denied, open '/ro/test.py'" }), + ) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports the rollback failure once when the parse-failure cleanup cannot revert the diff", async () => { + // A streaming delta captured a fatal error; the finalized block then fails to parse, so + // the parse-failure cleanup runs - and its revertChanges() fails too. The rollback + // failure is the actionable one: it must be reported exactly once, with the captured + // streaming error kept as its cause, and the incidental parse error must stay silent. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: stream open failed")) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const captured = state?.streamError + expect(captured?.message).toBe("EACCES: stream open failed") + + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("EACCES: rollback failed")) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + const [context, reported] = mockHandleError.mock.calls[0] + expect(context).toBe("writing file") + expect(reported.message).toContain("rollback failed") + expect(reported.cause).toBe(captured) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + 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( diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 7c4122f4d2..ed7f8a3d15 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -903,6 +903,11 @@ export class DiffViewProvider { * 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 @@ -915,14 +920,44 @@ export class DiffViewProvider { ) for (const tab of tabs) { + let closed: boolean + let closeError: Error | undefined try { - await vscode.window.tabGroups.close(tab, true) - } catch (err) { - console.error(`Failed to discard file tab ${tab.label}`, err) + 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 fef8583da7..89963acfad 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -57,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: [], @@ -1321,6 +1321,69 @@ describe("DiffViewProvider", () => { 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) From eb245494a2eb19174f818ac90bcc7f209b6c66ea Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 02:14:43 +0800 Subject: [PATCH 14/24] fix(editor): gate rollback on an in-progress edit and restore buffers before closing CodeRabbit #1930 follow-up: - DiffViewProvider.revertChanges() now requires both relPath and isEditing. relPath survives a completed edit, so a later teardown used the PREVIOUS edit's target and unlinked an existing user file. reset() clears relPath so no later teardown can reuse a stale target. - WriteToFileTool's parse-failure teardown reverts the diff view only while an edit is in progress, for the same reason. - discardFileTab() no longer passes true to tabGroups.close() as a force-discard flag (the parameter is preserveFocus). It restores the buffer to its pre-stream content and saves it clean first, so close() never sees a dirty tab, and a close the editor refuses now fails the rollback instead of deleting the file underneath an open tab. - handlePartial() wraps the provider-state lookup, preflight, policy checks and message construction in a cleanup boundary that releases this task's stream state on any unexpected exit, then rethrows so BaseTool reports once. - Tests: stale-relPath guard, reset() clearing, restore/save ordering before close, refused close, handlePartial boundary release, missing path and missing content releases with exact TaskAborted deregistration, and the revert helper's returned error identity. --- src/core/tools/WriteToFileTool.ts | 99 +++++++---- ...teToFileTool-partial-state-cleanup.spec.ts | 7 +- .../tools/__tests__/writeToFileTool.spec.ts | 116 ++++++++++++ src/integrations/editor/DiffViewProvider.ts | 40 ++++- .../editor/__tests__/DiffViewProvider.spec.ts | 165 ++++++++++++------ 5 files changed, 330 insertions(+), 97 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 4fe02ae0ca..b947af7dff 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -187,7 +187,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } this.resetTaskPartialState(task) - const rollbackError = await this.revertDiffChangesBeforeReset(task) + // Only an edit in progress may be reverted. DiffViewProvider keeps relPath after a + // completed write, so reverting against that stale target would roll back - and for a + // new file permanently delete - a file this stream never opened. + let rollbackError: Error | undefined + if (task.diffViewProvider.isEditing) { + rollbackError = await this.revertDiffChangesBeforeReset(task) + } await this.resetDiffViewAfterWrite(task) // A failed rollback is the more actionable failure (debris is still on disk), so it takes @@ -439,47 +445,64 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - const provider = task.providerRef.deref() - const state = await provider?.getState() - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) - - 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 - } + // Hoisted above the guarded setup: the diff-view catch below finalizes the same partial + // ask, so the message has to stay in scope once the try block ends. + let partialMessage: string | undefined - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + try { + // Everything from here up to the diff view is setup that can fail before + // execute() ever runs; the catch below owns the teardown for that window. + const provider = task.providerRef.deref() + const state = await provider?.getState() + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + 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 + } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, - } + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - const partialMessage = JSON.stringify(sharedMessageProps) + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath!), + content: newContent || "", + isOutsideWorkspace, + isProtected: isWriteProtected, + } + + partialMessage = JSON.stringify(sharedMessageProps) await task.ask("tool", partialMessage, block.partial).catch(() => {}) + } catch (error) { + // Unexpected failure in the pre-streaming setup (provider state, the filesystem probe, + // policy checks, message construction): this delta never reaches the diff view or + // execute(), so nothing else releases what the registration acquired. Drop this task's + // entry and its TaskAborted listener, then rethrow - BaseTool.handle() still reports the + // error once, and the diff-view failure path above keeps its own single-report handling. + this.resetTaskPartialState(task) + throw error + } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { @@ -506,7 +529,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // 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 ask only exists once the setup above assigned its message; before that there + // is nothing to finalize. + if (partialMessage !== undefined) { + 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). const rollbackError = await this.revertDiffChangesBeforeReset(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 5094c2dc81..bb0efaa360 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -80,13 +80,14 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { expect(errorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) }) - it("logs and continues when reverting the diff document fails", async () => { + it("returns the revert failure so the caller can report the rollback failure", 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 revertError = new Error("revert failed") + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(revertError) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - await writeToFileTool["revertDiffChangesBeforeReset"](task) + await expect(writeToFileTool["revertDiffChangesBeforeReset"](task)).resolves.toBe(revertError) expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index aea85fe09e..8c9a0b0849 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -545,6 +545,32 @@ describe("writeToFileTool", () => { }) + + it("releases the per-task stream state when provider state rejects during a partial delta", async () => { + // handlePartial() registers the entry and the TaskAborted listener, then awaits + // provider.getState(). A rejection there never reaches the diff view or execute(), so + // nothing else released what the registration acquired. The error still has to surface, + // so the boundary rethrows and BaseTool.handle() reports it once. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockRejectedValue(new Error("provider state unavailable")), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + 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(mockHandleError).toHaveBeenCalledWith( + "handling partial write_to_file", + expect.objectContaining({ message: "provider state unavailable" }), + ) + }) }) describe("path stabilization predicate", () => { @@ -620,6 +646,8 @@ describe("writeToFileTool", () => { // the same filesystem operation never happens either. The captured error is the one // the user can act on, and it must surface exactly once. const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + // The stream opened a diff view: that is what makes the rollback meaningful. + mockCline.diffViewProvider.isEditing = true state.streamFailed = true const streamFailure = new Error("EACCES: stream open failed") state.streamError = streamFailure @@ -661,6 +689,92 @@ describe("writeToFileTool", () => { expect(abortListener).toBeInstanceOf(Function) expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) }) + + it("does not revert the diff view when the parse-failure teardown has no edit in progress", async () => { + // One unstabilized delta registers this task's stream state without ever opening a diff + // view; the finalized block then arrives without nativeArgs. DiffViewProvider keeps the + // PREVIOUS edit's relPath, so reverting here would roll back - and for a new file delete - + // a file this write never opened. The teardown still resets the view and releases state. + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.isEditing = false + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + // No nativeArgs: this drives BaseTool's parse-failure path. + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when the path is missing", async () => { + // The missing-path 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: { content: testContent }, + nativeArgs: { path: "", content: testContent }, + // An empty path is what the streaming parser produces for a not-yet-complete + // argument object; the typed params cannot express it - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + 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 content is missing", async () => { + 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"> + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + 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) + }) }) describe("per-task stream state isolation", () => { // A second task streaming through the same singleton while mockCline runs. @@ -1008,6 +1122,8 @@ describe("writeToFileTool", () => { const captured = state?.streamError expect(captured?.message).toBe("EACCES: stream open failed") + // The stream had a diff view open - that is the state a rollback is for. + mockCline.diffViewProvider.isEditing = true mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("EACCES: rollback failed")) const block = { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index ed7f8a3d15..8788173c15 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -545,7 +545,12 @@ export class DiffViewProvider { } async revertChanges(): Promise { - if (!this.relPath) { + // Both guards are required. relPath survives a completed edit, so without isEditing a later + // teardown would take the new-file branch for the PREVIOUS edit's target and unlink an + // existing user file - permanently. isEditing alone is not enough: open() sets it before the + // first await, and that is exactly the state a failed open() leaves behind, where the + // placeholder and the created directories still have to be rolled back. + if (!this.relPath || !this.isEditing) { return } @@ -920,18 +925,24 @@ export class DiffViewProvider { ) for (const tab of tabs) { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a + // dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. Restore the buffer to its pre-stream content and save it clean first, + // so close() never sees a dirty tab and nothing unapproved reaches disk. + await this.restorePreStreamBuffer(absolutePath) + await this.saveBufferClean(absolutePath) + let closed: boolean let closeError: Error | undefined try { - closed = await vscode.window.tabGroups.close(tab, true) + closed = await vscode.window.tabGroups.close(tab) } 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.`, + `Rollback could not close the restored buffer for ${absolutePath}; its content was put back to the pre-stream state but the tab is still open.`, { cause: closeError }, ) } @@ -939,8 +950,9 @@ export class DiffViewProvider { } /** - * 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. + * Replace an open buffer's content with what it held before streaming started. Runs before + * the rollback closes the tab, so an unapproved buffer is never handed to close() dirty and + * never survives a close that the editor vetoes. */ private async restorePreStreamBuffer(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( @@ -958,6 +970,21 @@ export class DiffViewProvider { await vscode.workspace.applyEdit(edit) } + /** + * Save a restored buffer so it is clean when the rollback closes it. close() has no + * force-discard parameter, so a dirty tab would prompt or be refused; saving the restored + * content is what makes the close unconditional. For a new file the placeholder is unlinked + * a moment later, so the saved content is transient by design. + */ + private async saveBufferClean(absolutePath: string): Promise { + const document = vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (document?.isDirty) { + await document.save() + } + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) @@ -1220,6 +1247,7 @@ export class DiffViewProvider { this.cancelDeferredScroll() await this.closeAllDiffViews() + this.relPath = undefined this.editType = undefined this.isEditing = false this.originalContent = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 89963acfad..6d72a61045 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -174,7 +174,7 @@ describe("DiffViewProvider", () => { diffViewProvider = new DiffViewProvider(mockCwd, mockTask) // Mock the necessary properties and methods - ;(diffViewProvider as any).relPath = "test.txt" + diffViewProvider["relPath"] = "test.txt" ;(diffViewProvider as any).activeDiffEditor = { document: { uri: { fsPath: `${mockCwd}/test.txt` }, @@ -1104,6 +1104,9 @@ describe("DiffViewProvider", () => { vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockSavedEditor as any) ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + // Self-contained: revertChanges() only acts while an edit is in progress. + diffViewProvider["relPath"] = "test.txt" + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = true ;(diffViewProvider as any).preEditScrollLine = 15 ;(diffViewProvider as any).editType = "modify" @@ -1134,7 +1137,7 @@ describe("DiffViewProvider", () => { const buildActiveDiffEditor = () => ({ document: { - uri: { fsPath: mockTargetPath }, + uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, save: vi.fn().mockResolvedValue(undefined), @@ -1184,6 +1187,7 @@ describe("DiffViewProvider", () => { // for an existing file, which a denial then preserved. const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] Object.assign(diffViewProvider, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "create", @@ -1197,10 +1201,38 @@ describe("DiffViewProvider", () => { expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) }) + it("revertChanges() leaves an earlier edit's file alone when no edit is in progress", async () => { + // reset() used to leave relPath behind while clearing editType. A later stream that never + // opened a diff view then reached this method with a stale relPath and no editType, took + // the new-file branch, and unlinked the PREVIOUS edit's target - an existing user file, + // permanently. Nothing may be deleted unless an edit is actually in progress. + Object.assign(diffViewProvider, { + isEditing: false, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: undefined, + createdDirs: [`${mockCwd}/new-parent`], + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("reset() clears relPath so a later teardown cannot reuse a stale target", async () => { + Object.assign(diffViewProvider, { isEditing: true, relPath: "mock-target-file.ts" }) + + await diffViewProvider.reset() + + expect(diffViewProvider["relPath"]).toBeUndefined() + }) + 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, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "create", @@ -1220,6 +1252,7 @@ describe("DiffViewProvider", () => { // 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, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "modify", @@ -1238,6 +1271,7 @@ describe("DiffViewProvider", () => { // 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, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "create", @@ -1256,6 +1290,7 @@ describe("DiffViewProvider", () => { // 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, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "create", @@ -1271,6 +1306,7 @@ describe("DiffViewProvider", () => { // unapproved placeholder is still on disk, and the caller must not be told the // rollback succeeded. Object.assign(diffViewProvider, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: undefined, editType: "create", @@ -1281,12 +1317,23 @@ describe("DiffViewProvider", () => { 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. + it("revertChanges() restores the streamed buffer before closing it for a new file", async () => { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: closing + // a dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. The buffer must be restored to its pre-stream content and saved clean + // BEFORE the close, so close() never sees a dirty tab. const editor = buildActiveDiffEditor() editor.document.isDirty = true + const teardown = diffViewProvider as unknown as { + restorePreStreamBuffer: (absolutePath: string) => Promise + saveBufferClean: (absolutePath: string) => Promise + } + let restoreArgs: unknown[] = [] + let saveArgs: unknown[] = [] + let restoreOrder = 0 + let saveOrder = 0 + const restore = vi.spyOn(teardown, "restorePreStreamBuffer").mockResolvedValue(undefined) + const saveClean = vi.spyOn(teardown, "saveBufferClean").mockResolvedValue(undefined) const dirtyTab = { input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { uri: makeUri(mockTargetPath), @@ -1300,6 +1347,7 @@ describe("DiffViewProvider", () => { configurable: true, }) Object.assign(diffViewProvider, { + isEditing: true, relPath: "mock-target-file.ts", activeDiffEditor: editor, editType: "create", @@ -1309,31 +1357,36 @@ describe("DiffViewProvider", () => { try { await diffViewProvider.revertChanges() + // mockRestore() clears the recorded history, so the evidence is captured here. + restoreArgs = restore.mock.calls.map((args) => args[0]) + saveArgs = saveClean.mock.calls.map((args) => args[0]) + restoreOrder = restore.mock.invocationCallOrder[0] + saveOrder = saveClean.mock.invocationCallOrder[0] } finally { - // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + // Do not leak the fixture: later tests read the module-level tabGroups.all. if (originalTabs) { Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) } + restore.mockRestore() + saveClean.mockRestore() } - expect(editor.document.save).not.toHaveBeenCalled() - expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab, true) + expect(restoreArgs).toEqual([mockTargetPath]) + expect(saveArgs).toEqual([mockTargetPath]) + expect(saveOrder).toBeGreaterThan(restoreOrder) + const closeOrder = vi.mocked(vscode.window.tabGroups.close).mock.invocationCallOrder.at(-1) + expect(closeOrder).toBeGreaterThan(saveOrder) + // No force flag: the tab is clean by the time it is closed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) 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. + it("revertChanges() reports a close the editor refused instead of deleting underneath it", async () => { + // Even a restored, saved buffer can fail to close (a vetoing editor). Swallowing that let + // the rollback delete the file underneath an open tab. The failure must propagate, and + // the file must survive: unlinking after a refused close leaves an open tab on a ghost. 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), @@ -1342,46 +1395,62 @@ describe("DiffViewProvider", () => { label: "mock-target-file.ts", } const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") - const originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") Object.defineProperty(vscode.window.tabGroups, "all", { get: () => [{ tabs: [dirtyTab] }], configurable: true, }) Object.defineProperty(vscode.workspace, "textDocuments", { - value: [streamedBuffer], + get: () => [editor.document], + configurable: true, + }) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: vi.fn().mockResolvedValue(false), configurable: true, }) Object.assign(diffViewProvider, { + isEditing: true, 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") + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not close the restored buffer") + // This test owns only the refusal handling; the restore-before-close ordering is + // asserted by the test above. What matters here is that the close was attempted + // without a force flag and the file survived a close the editor vetoed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).not.toHaveBeenCalled() } finally { if (originalTabs) { Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) } - if (originalDocuments) { - Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) } } + }) - // 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(), - "", - ) + }) + + describe("userTouchedDiffEditor keep/close behavior", () => { + const mockTargetPath = `${mockCwd}/mock-target-file.ts` + + const buildActiveDiffEditor = () => ({ + document: { + uri: { fsPath: mockTargetPath, scheme: "file" }, + getText: vi.fn().mockReturnValue("content"), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + }, }) it("revertChanges() closes the file tab when the file was not open and untouched", async () => { @@ -1390,6 +1459,8 @@ describe("DiffViewProvider", () => { ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = false ;(diffViewProvider as any).preEditScrollLine = undefined @@ -1410,6 +1481,8 @@ describe("DiffViewProvider", () => { ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = true ;(diffViewProvider as any).preEditScrollLine = undefined @@ -1460,20 +1533,6 @@ describe("DiffViewProvider", () => { expect((diffViewProvider as any).userTouchedDocument).toBe(true) }) - }) - - describe("userTouchedDiffEditor keep/close behavior", () => { - const mockTargetPath = `${mockCwd}/mock-target-file.ts` - - const buildActiveDiffEditor = () => ({ - document: { - uri: { fsPath: mockTargetPath }, - getText: vi.fn().mockReturnValue("content"), - isDirty: false, - save: vi.fn().mockResolvedValue(undefined), - positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), - }, - }) it("saveChanges() keeps the file open when the user clicked inside the diff editor", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) @@ -1682,6 +1741,8 @@ describe("DiffViewProvider", () => { ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // The guard in revertChanges() requires an edit in progress; this fixture sets fields directly. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = false // The user clicked inside the diff pane -- but this is a deny, so it must be ignored. @@ -1873,7 +1934,7 @@ describe("DiffViewProvider", () => { const buildActiveDiffEditor = () => ({ document: { - uri: { fsPath: mockTargetPath }, + uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, save: vi.fn().mockResolvedValue(undefined), From 215e32d3efaaff8c64b801b8ab76a342599ea8a1 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 02:34:54 +0800 Subject: [PATCH 15/24] chore(lint): prune the stale no-explicit-any suppression in DiffViewProvider.spec The rollback tests replaced as-any fixture casts with bracket notation, so the suppressed occurrences in that file dropped from 310 to 309. The strict lint run (CI's "compile" job) compares suppression counts in both directions and exits 2 with "There are suppressions left that do not occur anymore" when a count is left too high. Regenerated with eslint --prune-suppressions; this is the only line that changes. --- src/eslint-suppressions.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 908159f7ab..b95b8c67a9 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1166,7 +1166,7 @@ }, "integrations/editor/__tests__/DiffViewProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 310 + "count": 309 } }, "integrations/editor/__tests__/EditorUtils.spec.ts": { From e1c304f1f0abfa8f17bc6ffd3689a66915a81261 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 09:28:10 +0800 Subject: [PATCH 16/24] fix(tools): report the real failure and never leak the stream state Four review findings on this unit, all in the streaming write_to_file path. 1. Regression Evidence (warning, test only): the capture-to-report path had no end-to-end coverage. Added a test that drives two real partial deltas with diffViewProvider.open() rejecting, then the completed block without nativeArgs so execute() never runs and onParameterParseFailure() is the only reporter, with the rollback succeeding. It asserts the report is the original streaming error object (identity, not just text) and that it is reported once. 2. Lifecycle Resource Cleanup (warning): the rooignore denial branch released the per-task stream state AFTER awaiting task.say(), so a task.say() that rejects - cancellation or disposal mid-ask - skipped the release and leaked the entry plus its TaskAborted listener, which then suppressed every later diff preview in that task. The release now runs in a finally. try/finally rather than releasing before the ask, so the branch keeps the same shape as the sibling units that run their own cleanup before releasing. 3. Review thread (Functional Correctness): a failed rollback reported "rollback failed after a streaming error" even when no streaming error was ever captured, describing a failure that never happened and swallowing the malformed tool call. With a captured stream error the wording and the cause are unchanged; without one the rollback reports under its own message and the parse error is still delivered, because those are two distinct failures. 4. Review thread (Data Integrity): DiffViewProvider.discardFileTab() restored and saved the buffer inside the tab loop, so when the user had closed the plain text tab and only the diff editor held the document, nothing ran: closeAllDiffViews() skips dirty tabs, removeCreatedFile() unlinked the file, and a dirty diff tab survived holding unapproved content that a later save would recreate. The restore and save now run once before the loop, which also stops a multi-tab path from repeating them. Negative controls, each reverted byte-for-byte: - dropping the streamError capture turns the new capture-to-report test red (alongside three existing tests that also cover that capture); - reverting the rollback wording branch turns exactly test 3's test red; - reverting the finally turns exactly test 2's test red; - moving the restore/save back inside the loop turns exactly test 4's test red. Verification: core/tools 658 passed, core/webview 606 passed, activate 79 passed, vitest.misc.config.ts 1845 passed / 13 skipped (with @roo-code/types resolved to this worktree's packages/types/src), full eslint . --ext=ts --max-warnings=0 exit 0, tsc --noEmit 0 errors under the local tsconfig paths override, eslint suppressions unchanged (prune: 0 semantic diffs). --- src/core/tools/WriteToFileTool.ts | 34 +++++-- .../tools/__tests__/writeToFileTool.spec.ts | 90 +++++++++++++++++++ src/integrations/editor/DiffViewProvider.ts | 17 ++-- .../editor/__tests__/DiffViewProvider.spec.ts | 56 ++++++++++++ 4 files changed, 184 insertions(+), 13 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index b947af7dff..47e5e36826 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -196,16 +196,27 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } await this.resetDiffViewAfterWrite(task) - // A failed rollback is the more actionable failure (debris is still on disk), so it takes - // the report slot when present; the streaming error is kept behind it as the cause. + // A failed rollback is the more actionable failure (debris is still on disk), so when a + // streaming error was also captured it takes the report slot and the streaming error is + // kept behind it as the cause. Without a captured streaming error there was no streaming + // failure to describe: the rollback gets its own message and the parse error stays the + // actionable report. These are two distinct failures, so reporting both does not break the + // single-report rule. if (rollbackError) { + if (state.streamError) { + await callbacks.handleError( + "writing file", + new Error(`write_to_file rollback failed after a streaming error: ${rollbackError.message}`, { + cause: state.streamError, + }), + ) + return true + } await callbacks.handleError( "writing file", - new Error(`write_to_file rollback failed after a streaming error: ${rollbackError.message}`, { - cause: state.streamError ?? rollbackError, - }), + new Error(`write_to_file rollback failed: ${rollbackError.message}`, { cause: rollbackError }), ) - return true + return false } if (state.streamError) { @@ -259,8 +270,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) if (!accessAllowed) { - await task.say("rooignore_error", relPath) - this.resetTaskPartialState(task) + try { + await task.say("rooignore_error", relPath) + } finally { + // The release belongs in a finally: task.say() can reject when the task is cancelled + // or disposed mid-ask, and a rejection that skips it leaks this task's stream state and + // its TaskAborted listener. finally keeps this branch the same shape as the sibling + // units, which run their own cleanup before releasing. + this.resetTaskPartialState(task) + } pushToolResult(formatResponse.rooIgnoreError(relPath)) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 8c9a0b0849..915e497d00 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -690,6 +690,28 @@ describe("writeToFileTool", () => { expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) }) + it("releases the per-task stream state when the rooignore ask itself rejects", async () => { + // task.say() can reject when the task is cancelled or disposed mid-ask. The release sits + // in a finally, so the stream state and its TaskAborted listener still go away even though + // the ask threw; without it the streamFailed guard suppresses every later diff preview in + // this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockCline.say = vi.fn().mockRejectedValue(new Error("task cancelled during the rooignore ask")) + + await expect(executeWriteFileTool({}, { accessAllowed: false })).rejects.toThrow( + "task cancelled during the rooignore ask", + ) + + 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) + // The ask threw, so the denial result was never pushed: the release cannot depend on it. + expect(mockPushToolResult).not.toHaveBeenCalled() + }) + it("does not revert the diff view when the parse-failure teardown has no edit in progress", async () => { // One unstabilized delta registers this task's stream state without ever opening a diff // view; the finalized block then arrives without nativeArgs. DiffViewProvider keeps the @@ -1147,6 +1169,74 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + it("reports the captured streaming error once when two real deltas fail and the rollback succeeds", async () => { + // The capture-to-report path end to end: two real partial deltas with open() rejecting, + // then the completed block without nativeArgs so execute() never runs and + // onParameterParseFailure() is the only reporter. The rollback succeeds, so the captured + // streaming error - not the incidental parse error - must reach the user exactly once. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: stream open failed")) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const captured = state?.streamError + expect(captured?.message).toBe("EACCES: stream open failed") + + // The stream left a diff view open; revertChanges() keeps its resolved default, so the + // rollback succeeds and nothing re-stamps the captured error. + mockCline.diffViewProvider.isEditing = true + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + // Identity, not just text: the report must be the original streaming error object. + expect(mockHandleError.mock.calls[0][0]).toBe("writing file") + expect(mockHandleError.mock.calls[0][1]).toBe(captured) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports a rollback failure under its own message and keeps the parse error when no stream failed", async () => { + // The diff view opened without any streaming failure; the completed block then fails to + // parse and the rollback itself fails. Reporting "after a streaming error" would describe + // a failure that never happened and would swallow the malformed tool call, so the rollback + // reports under its own message and the parse error is still delivered. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("EACCES: rollback failed")) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(2) + const [context, reported] = mockHandleError.mock.calls[0] + expect(context).toBe("writing file") + expect(reported.message).toBe("write_to_file rollback failed: EACCES: rollback failed") + expect(reported.message).not.toContain("streaming error") + expect(reported.cause).toBeInstanceOf(Error) + // The parse error is the actionable report for the model: it must still be delivered. + expect(mockHandleError.mock.calls[1][0]).toBe("parsing write_to_file args") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + 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( diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 8788173c15..6567e9fa6c 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -924,14 +924,21 @@ export class DiffViewProvider { arePathsEqual(tab.input.uri.fsPath, absolutePath), ) + // Restore and save once, before the loop and independent of it. The document can be open + // only through the diff editor - the user may have closed the plain text tab, leaving + // `tabs` empty - and in that shape the loop never ran, so the streamed buffer stayed + // dirty: closeAllDiffViews() skips dirty tabs and removeCreatedFile() then unlinks the + // file under an unsaved buffer that a later save recreates with the content this rollback + // was meant to discard. Hoisting also stops a multi-tab path from restoring and saving the + // same document once per tab. + await this.restorePreStreamBuffer(absolutePath) + await this.saveBufferClean(absolutePath) + for (const tab of tabs) { // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a // dirty tab prompts or is refused, which is how unapproved streamed content survived a - // "forced" close. Restore the buffer to its pre-stream content and save it clean first, - // so close() never sees a dirty tab and nothing unapproved reaches disk. - await this.restorePreStreamBuffer(absolutePath) - await this.saveBufferClean(absolutePath) - + // "forced" close. The buffer was restored and saved clean above, so close() never sees + // a dirty tab and nothing unapproved reaches disk. let closed: boolean let closeError: Error | undefined try { diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 6d72a61045..92098e7c80 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1381,6 +1381,62 @@ describe("DiffViewProvider", () => { expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) }) + it("revertChanges() restores and saves the buffer once even when no plain text tab is open", async () => { + // The document can be open only through the diff editor: the user closed the plain text + // tab, so tabGroups.all yields no TabInputText for the path. Restoring inside the tab loop + // meant nothing ran, closeAllDiffViews() then skipped the dirty diff tab, and the unlink + // left a dirty diff tab holding unapproved content that a later save would recreate. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const teardown = diffViewProvider as unknown as { + restorePreStreamBuffer: (absolutePath: string) => Promise + saveBufferClean: (absolutePath: string) => Promise + } + const restore = vi.spyOn(teardown, "restorePreStreamBuffer").mockResolvedValue(undefined) + const saveClean = vi.spyOn(teardown, "saveBufferClean").mockResolvedValue(undefined) + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + // No plain TabInputText tab for this path - only the diff editor holds the document. + get: () => [{ tabs: [] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + let restoreArgs: unknown[] = [] + let saveArgs: unknown[] = [] + let restoreOrder = 0 + let saveOrder = 0 + try { + await diffViewProvider.revertChanges() + // mockRestore() clears the recorded history, so the evidence is captured here. + restoreArgs = restore.mock.calls.map((args) => args[0]) + saveArgs = saveClean.mock.calls.map((args) => args[0]) + restoreOrder = restore.mock.invocationCallOrder[0] + saveOrder = saveClean.mock.invocationCallOrder[0] + } finally { + // Do not leak the fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + restore.mockRestore() + saveClean.mockRestore() + } + + // Exactly once each, restore before save, even with an empty tab list. + expect(restoreArgs).toEqual([mockTargetPath]) + expect(saveArgs).toEqual([mockTargetPath]) + expect(saveOrder).toBeGreaterThan(restoreOrder) + // The rollback still completed: the file it created was unlinked. + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + it("revertChanges() reports a close the editor refused instead of deleting underneath it", async () => { // Even a restored, saved buffer can fail to close (a vetoing editor). Swallowing that let // the rollback delete the file underneath an open tab. The failure must propagate, and From 53ed5b3534142b0eacfcbb1a18ac5a0f4e19ae98 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 12:36:51 +0800 Subject: [PATCH 17/24] fix(editor): treat a refused buffer restore as a rollback failure Clears three pre-merge rows on head e1c304f1f (Persistence Integrity error, Regression Evidence and Lifecycle Resource Cleanup warnings). Persistence Integrity. restorePreStreamBuffer() ignored the WorkspaceEdit result and saveBufferClean() saved anyway, so a refused restore persisted exactly the unapproved streamed content this rollback exists to discard. The contract now lives in the callee, not in each caller: a refused restore throws, so the save that follows it cannot run, and the failure reaches revertChanges() -> WriteToFileTool.revertDiffChangesBeforeReset(), which already returns it as rollbackError for the caller to report. Returning quietly would have been worse than returning false - it would have swallowed the failure. discardFileTab() is byte-for-byte unchanged: #1932 depends on its current shape (restore and save hoisted above the tab loop), and "a failed restore leaves nothing to save" is the callee's own contract, not a policy every caller has to remember. Ownership and the semantic baseline are recorded separately on #41 (6093818950): this unit owns the primitive, #1928's discardUnapprovedStream() is the semantic baseline, port direction #1930 -> #1916 / #1921 / #1929 / #1932. Lifecycle Resource Cleanup. handlePartial() registered per-task stream state without looking at the task, so a delta that arrived after an abort or an abandonment left the entry and its TaskAborted listener behind and could still produce a partial ask or a diff preview nobody owns. It now checks task.abort || task.abandoned before acquiring state and re-checks after each await boundary - provider state, the filesystem probe, the partial ask - before the next observable effect, releasing the entry on every early exit. Same flags Task itself bails on, same cancellation-aware shape #1929 established for the streamFailed guard. Regression Evidence. The new-file rollback tests spied restorePreStreamBuffer() and saveBufferClean(), so the restoration itself never executed. Two tests now run the real implementations against a dirty document in vscode.workspace.textDocuments: one asserts the buffer is put back through a WorkspaceEdit and saved clean before the close, the other that a refused restore neither saves nor deletes and surfaces as a failure rather than a successful rollback. The spied tests stay for the ordering property they actually cover. Negative controls, each reverted byte-for-byte (Buffer snapshot + sha256, all restored true): dropping the applyEdit check turns exactly the refused-restore test red; dropping each of the four cancellation checks turns exactly its own test red. One mutant initially survived because the suppressed-focus branch released the state and returned on its own - the test was rewritten to assert the next boundary (the filesystem probe must not run) and then failed 1:1. Verification: core/tools 662 passed, integrations/editor 92 passed, core/task 795 passed, ClineProvider unaffected; eslint . --ext=ts --max-warnings=0 exit 0; tsc --noEmit 0 errors under the local tsconfig paths override (it caught a real defect first: the second template literal was being read as ErrorOptions); eslint suppressions unchanged (prune: 0 semantic diffs). Formatting re-verified against the current refs/pull/1930/merge (adfba9631): all four touched files produce the same prettier offender set as HEAD, so no new formatting drift. --- src/core/tools/WriteToFileTool.ts | 39 +++++ ...teToFileTool-partial-state-cleanup.spec.ts | 140 ++++++++++++++++++ src/integrations/editor/DiffViewProvider.ts | 18 ++- .../editor/__tests__/DiffViewProvider.spec.ts | 115 ++++++++++++++ 4 files changed, 311 insertions(+), 1 deletion(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 47e5e36826..4692ff0766 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -157,6 +157,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return undefined } + /** + * Whether this task can still act on a partial delta. Mirrors the guard Task uses to bail + * out of its own loops, so a delta never does work for a task that has already stopped. + */ + private isStreamCancelled(task: Task): boolean { + return task.abort === true || task.abandoned === true + } + 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) @@ -454,6 +462,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + // A task that was aborted or abandoned can no longer reach execute()'s teardown, so this + // delta must not register per-task state at all: the abort listener would outlive a stream + // that never produces another delta, and the entry's failure mark would suppress the diff + // preview of a later write in a task that already moved on. Same two flags Task itself + // bails on, and the same cancellation-aware shape #1929 established for the streamFailed + // guard: check before acquiring state, then re-check after every await before the next + // observable effect. + if (this.isStreamCancelled(task)) { + 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. const partialStreamState = this.getTaskPartialStreamState(task) @@ -472,6 +491,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // execute() ever runs; the catch below owns the teardown for that window. const provider = task.providerRef.deref() const state = await provider?.getState() + // First await since the state was registered: a cancellation during getState() would + // otherwise continue into the partial ask below. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -496,6 +521,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { fileExists = await fileExistsAtPath(absolutePath) task.diffViewProvider.editType = fileExists ? "modify" : "create" } + // The filesystem probe is another await boundary, and the ask below is the first thing + // the user can see: a task that stopped must not produce it. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -521,6 +552,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { throw error } + // Last await before the diff view: the ask above can be answered (or the task aborted) + // while it is in flight, and opening a preview for a task that already stopped would + // leave a diff view nobody owns. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { 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 bb0efaa360..9baa20fc9f 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -5,6 +5,13 @@ import { vi, type MockedFunction } from "vitest" import { type Task } from "../../task/Task" import { writeToFileTool } from "../WriteToFileTool" +import { fileExistsAtPath } from "../../../utils/fs" + +// Only the filesystem probe has to be observable; every other export of utils/fs stays real. +vi.mock("../../../utils/fs", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, fileExistsAtPath: vi.fn().mockResolvedValue(false) } +}) // 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 @@ -103,3 +110,136 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { expect(errorSpy).toHaveBeenCalledWith("Error finalizing write_to_file partial tool ask:", expect.any(Error)) }) }) +// handlePartial() acquires per-task state and then crosses several awaits before it touches +// anything the user can see. A task that was aborted or abandoned mid-flight must not leave the +// registration behind and must not produce the partial ask or a diff preview. +describe("WriteToFileTool partial-delta cancellation", () => { + // Structural double: handlePartial only reads these members before the diff view. The double + // assertion is the repo's existing pattern for private-method tests. + beforeEach(() => { + // The probe mock comes from a module-level vi.mock factory, so its call history outlives a + // single test unless it is reset here; the boundary assertions count on it. + vi.mocked(fileExistsAtPath).mockReset().mockResolvedValue(false) + }) + + interface StreamTask { + taskId: string + instanceId: string + abort: boolean + abandoned: boolean + cwd: string + once: MockedFunction<(...args: unknown[]) => unknown> + off: MockedFunction<(...args: unknown[]) => unknown> + ask: MockedFunction<(type: string, text: string, partial?: boolean) => Promise> + providerRef: { deref: () => { getState: () => Promise> } } + diffViewProvider: { + editType: "modify" | "create" | undefined + isEditing: boolean + open: MockedFunction<(relPath: string) => Promise> + update: MockedFunction<(content: string, single: boolean) => Promise> + } + } + + const buildStreamTask = (taskId: string, instanceId: string): StreamTask => ({ + taskId, + instanceId, + abort: false, + abandoned: false, + cwd: "/mock/cwd", + once: vi.fn(), + off: vi.fn(), + ask: vi.fn().mockResolvedValue(undefined), + providerRef: { deref: () => ({ getState: async () => ({ experiments: {} }) }) }, + diffViewProvider: { + editType: "create", + isEditing: true, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + }, + }) + + const partialBlock = { + type: "tool_use", + name: "write_to_file", + partial: true, + params: { path: "src/demo.ts", content: "hello world" }, + } satisfies Record as unknown as Parameters<(typeof writeToFileTool)["handlePartial"]>[1] + + // The path only counts as stable once the same path has been seen twice, so a delta that + // should reach the ask has to start from an entry seeded by an earlier delta. + const seedStablePath = (task: Task) => { + const state = writeToFileTool["getTaskPartialStreamState"](task) + state.lastSeenPartialPath = "src/demo.ts" + return state + } + + it("does not register per-task state for a task that already stopped", async () => { + const task = buildStreamTask("cancel-before-register", "inst-c1") + task.abort = true + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.once).not.toHaveBeenCalled() + expect(task.ask).not.toHaveBeenCalled() + expect(task.diffViewProvider.open).not.toHaveBeenCalled() + }) + + it("releases the state and stops before the next boundary when the task aborts during getState()", async () => { + const task = buildStreamTask("cancel-in-getstate", "inst-c2") + // No editType yet, so the very next step after getState() would be the filesystem probe: + // asserting the probe never ran pins THIS boundary. (Leaving editType set would let the + // suppressed-focus branch release the state and return on its own, which proves nothing + // about the check under test.) + task.diffViewProvider.editType = undefined + seedStablePath(task as unknown as Task) + task.providerRef.deref = () => ({ + getState: async () => { + task.abort = true + return { experiments: {} } + }, + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(vi.mocked(fileExistsAtPath)).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + expect(task.ask).not.toHaveBeenCalled() + }) + + it("releases the state and skips the partial ask when the task is abandoned during the file probe", async () => { + const task = buildStreamTask("cancel-in-probe", "inst-c3") + // No editType yet forces the filesystem probe, the await boundary between the setup and the + // first thing the user sees. + task.diffViewProvider.editType = undefined + seedStablePath(task as unknown as Task) + const probe = vi.mocked(fileExistsAtPath) + probe.mockImplementation(async () => { + task.abandoned = true + return false + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(probe).toHaveBeenCalledTimes(1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.ask).not.toHaveBeenCalled() + }) + + it("does not open the diff view when the task aborts while the partial ask is in flight", async () => { + const task = buildStreamTask("cancel-in-ask", "inst-c4") + seedStablePath(task as unknown as Task) + task.ask = vi.fn().mockImplementation(async () => { + task.abort = true + return undefined + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(task.ask).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.open).not.toHaveBeenCalled() + expect(task.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) +}) \ No newline at end of file diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 6567e9fa6c..9b2aff7d91 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -960,6 +960,13 @@ export class DiffViewProvider { * Replace an open buffer's content with what it held before streaming started. Runs before * the rollback closes the tab, so an unapproved buffer is never handed to close() dirty and * never survives a close that the editor vetoes. + * + * A refused WorkspaceEdit is a failed restore, not a no-op: the buffer still holds content the + * user never approved. Throwing is this method's own contract rather than a caller policy - + * the save that follows cannot run, so nothing saveable survives, and the caller reports the + * rollback as failed instead of telling the user the write was rolled back with unapproved + * content still one save from disk. Returning quietly would be worse than returning false: + * it would swallow the failure. */ private async restorePreStreamBuffer(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( @@ -974,7 +981,12 @@ export class DiffViewProvider { document.positionAt(document.getText().length), ) edit.replace(document.uri, range, this.originalContent ?? "") - await vscode.workspace.applyEdit(edit) + const restored = await vscode.workspace.applyEdit(edit) + if (!restored) { + throw new Error( + `Rollback could not restore the streamed buffer for ${absolutePath}; the editor refused the restore, so the unapproved content is still in the buffer and unsaved.`, + ) + } } /** @@ -982,6 +994,10 @@ export class DiffViewProvider { * force-discard parameter, so a dirty tab would prompt or be refused; saving the restored * content is what makes the close unconditional. For a new file the placeholder is unlinked * a moment later, so the saved content is transient by design. + * + * Only ever reached after a successful restore: a refused restore throws above, because a + * buffer whose restore failed still holds the unapproved content this rollback discards and + * must never be saved. */ private async saveBufferClean(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 92098e7c80..cfe04808cd 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1493,6 +1493,121 @@ describe("DiffViewProvider", () => { } } }) + it("revertChanges() restores and saves the buffer through the real rollback path", async () => { + // The two tests above spy restorePreStreamBuffer() and saveBufferClean(), so the restoration + // itself never runs there and only proves ordering. This one leaves both real: the buffer must + // be put back through a WorkspaceEdit and saved clean, which is what stops close() from ever + // seeing a dirty tab holding content the user never approved. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + 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 originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + // The real restore and save look the document up here; without this the methods would find + // nothing and pass without touching anything. + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the fixtures: later tests read the module-level tabGroups.all and textDocuments. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + // The real restore replaced the whole buffer with the pre-stream content. + expect(vscode.workspace.applyEdit).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace).toHaveBeenCalledWith( + editor.document.uri, + expect.anything(), + "pre-stream content", + ) + // The real save ran on the restored buffer, and only then was the tab closed. + expect(editor.document.save).toHaveBeenCalledTimes(1) + const saveOrder = editor.document.save.mock.invocationCallOrder[0] + const closeOrder = vi.mocked(vscode.window.tabGroups.close).mock.invocationCallOrder.at(-1) + expect(closeOrder).toBeGreaterThan(saveOrder) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() neither saves nor deletes when the editor refuses the restore", async () => { + // A refused WorkspaceEdit leaves the unapproved streamed content in the buffer. Saving it + // would persist exactly what this rollback exists to undo, and deleting the placeholder + // underneath would report a rollback that never finished. The refusal has to reach the + // caller that reports failures, which is what the user gets instead of a false success. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + 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 originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the streamed buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + // Nothing saveable was left behind, and nothing was closed or deleted on top of the refusal. + expect(editor.document.save).not.toHaveBeenCalled() + expect(vscode.window.tabGroups.close).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + }) }) From e41ebe0aab77ab70aa2b1308a32ff913d574face Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 13:00:21 +0800 Subject: [PATCH 18/24] style: apply prettier to the files this PR touches The compile job's "Check formatting" step (pnpm format:check -> prettier --check .) is red on 53ed5b353. It is a formatting gate, not a test or type failure: Lint and Check types were skipped. The step only became reachable because the PR merge ref now carries a main that declares the format:check script and a root .prettierignore; neither exists on this branch's own base, which is why the same files were never checked before. Attribution, so the nature of this commit is on the record: the debt is not from the last round. An LF worktree at 53ed5b353 and at its parent e1c304f1f produce an identical prettier offender list, including all six files the job names, so the previous commit added zero offenders. This unit simply carried files that the gate had never run against. Only files inside this PR's diff are formatted - six, and the six are exactly the offenders the merge ref reports; nothing outside the diff was touched. Formatting is whitespace only: the discardFileTab() region is byte-for-byte identical, its statement sequence and the two hoisted calls above the tab loop are unchanged, which is what #1932 depends on. Verified after merging org main 09e7326cf: prettier --check . reports 0 offenders; core/tools 669, integrations/editor 92 and core/task 829 tests pass; eslint . --ext=ts --max-warnings=0 exits 0 with no suppression count increase; tsc --noEmit reports 0 errors. --- src/core/tools/BaseTool.ts | 5 +--- src/core/tools/WriteToFileTool.ts | 10 +++---- ...teToFileTool-partial-state-cleanup.spec.ts | 12 ++++++--- .../tools/__tests__/writeToFileTool.spec.ts | 26 +++---------------- src/integrations/editor/DiffViewProvider.ts | 6 +---- .../editor/__tests__/DiffViewProvider.spec.ts | 5 ++-- 6 files changed, 21 insertions(+), 43 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 56d8c33b59..1fddd606fb 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -112,10 +112,7 @@ export abstract class BaseTool { * reported exactly once. Per-task only: a global teardown would clobber another task * that is still streaming through this singleton. */ - protected async releaseStreamStateOnParseFailure( - _task: Task, - _callbacks: ToolCallbacks, - ): Promise { + protected async releaseStreamStateOnParseFailure(_task: Task, _callbacks: ToolCallbacks): Promise { return false } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 4692ff0766..38495ee5b0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -185,10 +185,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * and true is returned to suppress the incidental parse error - the failure surfaces * exactly once. */ - protected override async releaseStreamStateOnParseFailure( - task: Task, - callbacks: ToolCallbacks, - ): Promise { + protected override async releaseStreamStateOnParseFailure(task: Task, callbacks: ToolCallbacks): Promise { const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) if (!state) { return false @@ -488,7 +485,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { try { // Everything from here up to the diff view is setup that can fail before - // execute() ever runs; the catch below owns the teardown for that window. + // execute() ever runs; the catch below owns the teardown for that window. const provider = task.providerRef.deref() const state = await provider?.getState() // First await since the state was registered: a cancellation during getState() would @@ -540,8 +537,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) - + await task.ask("tool", partialMessage, block.partial).catch(() => {}) } catch (error) { // Unexpected failure in the pre-streaming setup (provider state, the filesystem probe, // policy checks, message construction): this delta never reaches the diff view or 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 9baa20fc9f..c428bbf0ec 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -59,12 +59,18 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { // The registration side of the pairing, asserted BEFORE the cleanup runs: the off() // assertion below only proves the right listener was deregistered if this one pins which // listener was registered in the first place. - expect((task as unknown as CleanupTask).once).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, state.abortCleanup) + expect((task as unknown as CleanupTask).once).toHaveBeenCalledWith( + RooCodeEventName.TaskAborted, + state.abortCleanup, + ) writeToFileTool.clearTaskState(task) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) - expect((task as unknown as CleanupTask).off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, state.abortCleanup) + expect((task as unknown as CleanupTask).off).toHaveBeenCalledWith( + RooCodeEventName.TaskAborted, + state.abortCleanup, + ) }) it("is a no-op for a task that never streamed", async () => { @@ -242,4 +248,4 @@ describe("WriteToFileTool partial-delta cancellation", () => { expect(task.diffViewProvider.update).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) -}) \ No newline at end of file +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 915e497d00..cbeeb25856 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -544,8 +544,6 @@ describe("writeToFileTool", () => { } }) - - it("releases the per-task stream state when provider state rejects during a partial delta", async () => { // handlePartial() registers the entry and the TaskAborted listener, then awaits // provider.getState(). A rejection there never reaches the diff view or execute(), so @@ -636,7 +634,6 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { isPartial: true }) expect(mockCline.ask).toHaveBeenCalledTimes(1) }) - }) describe("parse-failure reporting and early-return cleanup", () => { @@ -705,7 +702,7 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) const abortListener = mockCline.once.mock.calls.find( ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, - )?.[1] + )?.[1] expect(abortListener).toBeInstanceOf(Function) expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) // The ask threw, so the denial result was never pushed: the release cannot depend on it. @@ -867,7 +864,9 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) // Otherwise the retained streamFailed suppresses the diff preview of every // later write_to_file in this task. - expect(writeToFileTool["taskPartialStreamState"].has(`${mockCline.taskId}.${mockCline.instanceId}`)).toBe(false) + expect(writeToFileTool["taskPartialStreamState"].has(`${mockCline.taskId}.${mockCline.instanceId}`)).toBe( + false, + ) // ...and the cleanup must stay scoped: the other task is still streaming. expect(writeToFileTool["taskPartialStreamState"].get("task-2.instance-2")?.streamFailed).toBe(true) }) @@ -978,7 +977,6 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) - }) describe("user interaction", () => { @@ -1020,8 +1018,6 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - - 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 @@ -1292,14 +1288,6 @@ describe("writeToFileTool", () => { 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" }), @@ -1324,8 +1312,6 @@ describe("writeToFileTool", () => { 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 @@ -1346,9 +1332,5 @@ describe("writeToFileTool", () => { expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() }) - - - - }) }) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 9b2aff7d91..6177a1a490 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -592,7 +592,6 @@ export class DiffViewProvider { for (let i = this.createdDirs.length - 1; i >= 0; i--) { await this.removeCreatedDir(this.createdDirs[i]) } - } else { // Only reachable after a successful open(), so the editor exists. const updatedDocument = this.activeDiffEditor?.document @@ -976,10 +975,7 @@ export class DiffViewProvider { return } const edit = new vscode.WorkspaceEdit() - const range = new vscode.Range( - document.positionAt(0), - document.positionAt(document.getText().length), - ) + const range = new vscode.Range(document.positionAt(0), document.positionAt(document.getText().length)) edit.replace(document.uri, range, this.originalContent ?? "") const restored = await vscode.workspace.applyEdit(edit) if (!restored) { diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index cfe04808cd..d67ab6f727 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1312,7 +1312,9 @@ describe("DiffViewProvider", () => { editType: "create", createdDirs: [], }) - vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) await expect(diffViewProvider.revertChanges()).rejects.toThrow("EACCES: permission denied") }) @@ -1608,7 +1610,6 @@ describe("DiffViewProvider", () => { expect(vscode.window.tabGroups.close).not.toHaveBeenCalled() expect(fs.unlink).not.toHaveBeenCalled() }) - }) describe("userTouchedDiffEditor keep/close behavior", () => { From 9c5476505ef2b60e7397ba663a8d5f12112fbd55 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 15:25:39 +0800 Subject: [PATCH 19/24] fix(editor): fail closed when a rollback restore or save is refused The pre-merge table still reported the Persistence Integrity error at the previous head. The last round fixed one call site - the new-file rollback - but the defect class has a second call site: the existing-file branch of revertChanges() built its own WorkspaceEdit, discarded the boolean result of workspace.applyEdit(), and saved unconditionally, so a refused restore wrote unapproved streamed content into an existing file. Fixing one call site is not fixing the defect class. The branch now runs through the same callee-owned contract as the new-file rollback instead of adding a third inline copy of the check: restorePreStreamBuffer() throws when the editor refuses the restore, so the save that follows cannot run, and saveBufferClean() now treats a save that did not happen as a failure rather than a success. Both methods accept the document explicitly, because this branch reached it through activeDiffEditor; resolving it again by path could pick a different buffer or none, and none would skip the restore in silence. One test leaves the path lookup empty on purpose so that assumption is pinned rather than assumed. Same-class call sites in the same file that this change deliberately does not sweep, registered here rather than changed: the save in showEditedFileWithoutDisruptingFocus, the three applyEdit calls in saveChanges and its append path, the save in saveChanges, and the save in keepOrCloseEditedFile. None is on the rollback path this unit changed; each needs its own row or issue. Nine test fixtures mocked TextDocument.save() as resolving undefined; the API resolves a boolean, so they now resolve true - the new failure check would otherwise be reading a fixture inaccuracy. Verified: integrations/editor 95 passed (three new tests: real restore-and-save for an existing file, no save on a refused restore, failure on a save that did not happen), core/tools 669, core/task 829; eslint . --ext=ts --max-warnings=0 exit 0 with no suppression count increase; tsc --noEmit 0 errors; prettier --check . reports 0 offenders in a worktree checked out with core.autocrlf=false. Negative controls, each restored byte-for-byte: dropping the refused-restore check turns exactly the two refused-restore tests red (the contract now guards both branches); dropping the failed-save check turns exactly its own test red; resolving the existing-file document by path again turns three tests red, which is the evidence that passing the document explicitly is load-bearing. --- src/integrations/editor/DiffViewProvider.ts | 67 ++++++---- .../editor/__tests__/DiffViewProvider.spec.ts | 122 ++++++++++++++++-- 2 files changed, 153 insertions(+), 36 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 6177a1a490..5fe2d9ef1c 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -599,21 +599,20 @@ export class DiffViewProvider { return } - // Revert document. - const edit = new vscode.WorkspaceEdit() - - const fullRange = new vscode.Range( - updatedDocument.positionAt(0), - updatedDocument.positionAt(updatedDocument.getText().length), - ) - - edit.replace(updatedDocument.uri, fullRange, this.stripAllBOMs(this.originalContent ?? "")) - - // Apply the edit and save, since contents shouldn't have changed - // this won't show in local history unless of course the user made + // Revert the document through the same contract the new-file rollback uses, so a + // refused restore cannot be followed by a save here either. The document is passed + // explicitly: this branch reached it through activeDiffEditor, and resolving it again + // by path could silently pick a different buffer - or none, which would skip the + // restore entirely. + // + // Applying and saving the restore does not show in local history unless the user made // changes and saved during the edit. - await vscode.workspace.applyEdit(edit) - await updatedDocument.save() + await this.restorePreStreamBuffer( + absolutePath, + updatedDocument, + this.stripAllBOMs(this.originalContent ?? ""), + ) + await this.saveBufferClean(absolutePath, updatedDocument) await this.closeAllDiffViews() @@ -967,16 +966,22 @@ export class DiffViewProvider { * content still one save from disk. Returning quietly would be worse than returning false: * it would swallow the failure. */ - private async restorePreStreamBuffer(absolutePath: string): Promise { - const document = vscode.workspace.textDocuments.find( - (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), - ) - if (!document) { + private async restorePreStreamBuffer( + absolutePath: string, + document?: vscode.TextDocument, + content?: string, + ): Promise { + const target = + document ?? + vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!target) { 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 ?? "") + const range = new vscode.Range(target.positionAt(0), target.positionAt(target.getText().length)) + edit.replace(target.uri, range, content ?? this.originalContent ?? "") const restored = await vscode.workspace.applyEdit(edit) if (!restored) { throw new Error( @@ -995,12 +1000,20 @@ export class DiffViewProvider { * buffer whose restore failed still holds the unapproved content this rollback discards and * must never be saved. */ - private async saveBufferClean(absolutePath: string): Promise { - const document = vscode.workspace.textDocuments.find( - (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), - ) - if (document?.isDirty) { - await document.save() + private async saveBufferClean(absolutePath: string, document?: vscode.TextDocument): Promise { + const target = + document ?? + vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!target?.isDirty) { + return + } + const saved = await target.save() + if (!saved) { + throw new Error( + `Rollback could not save the restored buffer for ${absolutePath}; the editor did not persist the restored content, so the buffer is still dirty and one save away from holding what this rollback undid.`, + ) } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index d67ab6f727..304db019c3 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -41,7 +41,7 @@ vi.mock("vscode", () => ({ onDidOpenTextDocument: vi.fn(() => ({ dispose: vi.fn() })), openTextDocument: vi.fn().mockResolvedValue({ isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }), textDocuments: [], fs: { @@ -875,7 +875,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("new content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } ;(diffViewProvider as any).preDiagnostics = [] @@ -1020,7 +1020,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } @@ -1045,7 +1045,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } @@ -1065,7 +1065,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: `${mockCwd}/race.ts`, scheme: "file" }, getText: vi.fn().mockReturnValue("a\nCHANGED\nc\nd\n"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), lineCount: 5, lineAt: vi.fn().mockReturnValue({ text: "" }), }, @@ -1116,7 +1116,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: `${mockCwd}/test.txt` }, getText: vi.fn().mockReturnValue("modified"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, } @@ -1140,7 +1140,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, }) @@ -1439,6 +1439,110 @@ describe("DiffViewProvider", () => { expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) }) + it("revertChanges() restores and saves an existing file through the real rollback path", async () => { + // The existing-file branch used to build its own WorkspaceEdit, discard applyEdit()'s boolean, + // and save unconditionally, so a refused restore wrote the unapproved streamed content to an + // existing file. It now runs through the same contract as the new-file branch. textDocuments is + // deliberately left empty: the branch reached this document through activeDiffEditor, and if the + // restore resolved it by path again it would find nothing and skip the restore in silence. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await diffViewProvider.revertChanges() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + expect(vscode.workspace.applyEdit).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace).toHaveBeenCalledWith( + editor.document.uri, + expect.anything(), + "pre-stream content", + ) + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("revertChanges() does not save an existing file when the editor refuses the restore", async () => { + // The user must not be told the write was rolled back while the existing file still holds + // content they never approved: the refusal has to reach the reporting layer, and the save + // that would persist it must not run. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the streamed buffer") + expect(editor.document.save).not.toHaveBeenCalled() + expect(diffViewProvider["keepOrCloseEditedFile"]).not.toHaveBeenCalled() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + }) + + it("revertChanges() reports a save the editor did not perform for an existing file", async () => { + // A restore that applied but did not persist leaves a dirty buffer one save away from disk. + // save() resolves false in that case, and treating it as success would report a rollback that + // never completed. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + editor.document.save = vi.fn().mockResolvedValue(false) + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored buffer") + expect(diffViewProvider["keepOrCloseEditedFile"]).not.toHaveBeenCalled() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + }) it("revertChanges() reports a close the editor refused instead of deleting underneath it", async () => { // Even a restored, saved buffer can fail to close (a vetoing editor). Swallowing that let // the rollback delete the file underneath an open tab. The failure must propagate, and @@ -1620,7 +1724,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, }) @@ -2109,7 +2213,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, }) From 2f15ff7ab1990153d23a8f4effaaa5b14f54455a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 05:47:08 +0800 Subject: [PATCH 20/24] fix(write-to-file): fail closed after refused streaming rollback; cover parse-failure no-state branch Clears the two actionable rows of the CodeRabbit pre-merge table at head 9c5476505ef2b60e7397ba663a8d5f12112fbd55. Persistence Integrity (error): handlePartial() recorded a refused rollback only as the stream error and always continued to resetDiffViewAfterWrite(), so the next execute() re-opened the diff view and could save the dirty, never-approved streamed buffer before approval (reset() cannot close a dirty diff tab). The refused rollback is now recorded as an unrecoverable per-task failure (rollbackFailure). execute() checks it before any write path - no open/update/save, no directory creation - reports the recorded failure exactly once, and releases the per-task state. The parse-failure path keeps reporting the same failure once when the final block never parses, because execute() released the state on its way out. Regression Evidence (warning): the parse-failure hook's no-state branch had no focused test - every parse-path test seeded a stream-state entry first. Added a handle() test that submits a completed block without nativeArgs before any partial delta and counts handleError calls by context: exactly one "parsing write_to_file args", no "writing file", an empty per-task state map, and the diff view untouched. Negative controls: dropping the record line or neutering the execute() guard reddens only the new fail-closed test; flipping the no-state branch to return true reddens only the new parse-error test and stays green against the previous spec, proving that branch was uncovered. The new fail-closed test is red against the pre-fix head. Test doubles for the widened TaskPartialStreamState are completed in this commit. --- .../removeClineFromStack-delegation.spec.ts | 1 + src/core/tools/WriteToFileTool.ts | 27 +++++- .../tools/__tests__/writeToFileTool.spec.ts | 85 +++++++++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index b42a549fef..d691cfc59c 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -231,6 +231,7 @@ describe("ClineProvider failed history restoration cleanup", () => { lastSeenPartialPath: undefined, streamFailed: false, streamError: undefined, + rollbackFailure: undefined, task, abortCleanup: () => {}, }) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 38495ee5b0..ba43cc8e05 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -34,6 +34,11 @@ interface TaskPartialStreamState { * by onParameterParseFailure() when the final block fails to parse (so * execute() never runs and would never report it). */ streamError: Error | undefined + /** Set when the rollback after a streaming failure was itself refused: the dirty + * streamed buffer survives reset(), so the next execute() must fail closed - open() + * would save that unapproved buffer before approval. Reported once by execute() (or by + * onParameterParseFailure() when the final block never parses). */ + rollbackFailure: 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. */ @@ -93,6 +98,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { lastSeenPartialPath: undefined, streamFailed: false, streamError: undefined, + rollbackFailure: undefined, task, abortCleanup: () => this.resetTaskPartialState(task), } @@ -248,6 +254,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // the catch below can finalize it. Undefined on the saveDirectly branch. let pendingPartialAsk: string | undefined + // Fail closed on a refused streaming rollback: the dirty streamed buffer survived + // reset(), and open() saves a dirty existing document BEFORE approval - retrying the + // write here would persist content the user never approved. Report the recorded failure + // once and touch no write path (no open/update/save, no directory creation). The state + // is released here, so the parse-failure path - which only runs when execute() never + // did - can never report the same failure a second time. + const streamState = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (streamState?.rollbackFailure) { + this.resetTaskPartialState(task) + await handleError("writing file", streamState.rollbackFailure) + return + } + if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") @@ -596,10 +615,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // logged-only revert failure hides exactly that, so the rollback failure becomes the // failure this stream reports - the original streaming error stays reachable as the // cause, and the report still happens exactly once (on the parse-failure path). - partialStreamState.streamError = new Error( + const rollbackFailure = new Error( `write_to_file rollback failed after a streaming error: ${rollbackError.message}`, { cause: partialStreamState.streamError }, ) + partialStreamState.streamError = rollbackFailure + // Fail closed for the next execute(): reset() cannot close the dirty diff tab the + // refused restore left behind, so a retry would re-open the view and save the + // unapproved streamed content before approval. Record the failure on this task's + // state; execute() reports it once and touches no write path. + partialStreamState.rollbackFailure = rollbackFailure } await this.resetDiffViewAfterWrite(task) } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index cbeeb25856..af7b2c5071 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -580,6 +580,7 @@ describe("writeToFileTool", () => { lastSeenPartialPath, streamFailed: false, streamError: undefined, + rollbackFailure: undefined, task: mockCline, abortCleanup: () => {}, } @@ -669,6 +670,43 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + it("keeps the generic parse error when no per-task stream state exists", async () => { + // BaseTool.handle() delegates every missing-nativeArgs error to + // releaseStreamStateOnParseFailure() before reporting the parse error. With no prior + // partial delta there is no state to release: the hook must return false so the + // ordinary parse error still reaches the user - suppressing it here would silence + // every malformed write_to_file call in a task that never streamed. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + // No nativeArgs: this drives BaseTool's parse-failure path. + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Counted by context rather than toHaveBeenCalledWith: a count of matching calls + // cannot auto-pass when handleError fires more than once. + const parseCalls = mockHandleError.mock.calls.filter( + ([context]) => context === "parsing write_to_file args", + ) + const writeCalls = mockHandleError.mock.calls.filter(([context]) => context === "writing file") + expect(parseCalls).toHaveLength(1) + expect(writeCalls).toHaveLength(0) + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(parseCalls[0][1]).toBeInstanceOf(Error) + // The no-state branch returns before touching the diff view or registering anything. + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("releases the per-task stream state when execute() returns early on a denied path", async () => { // The rooignore branch returns before execute()'s success/catch cleanup; without // this the abort listener and the streamFailed guard outlive the call and suppress @@ -1093,6 +1131,53 @@ describe("writeToFileTool", () => { ) }) + it("fails closed without retrying any write path when the streaming rollback was refused", async () => { + // update() fails mid-stream and the rollback that follows is refused: the dirty + // streamed buffer survives reset(), which cannot close a dirty diff tab. The next + // execute() would call open() again, and open() saves a dirty existing document + // BEFORE approval - persisting content the user never approved. execute() must fail + // closed on the recorded rollback failure instead of retrying. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue( + Object.assign(new Error("EACCES: rollback failed"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches update() and hits the refused rollback. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const recorded = state?.rollbackFailure + expect(recorded).toBeInstanceOf(Error) + expect(recorded?.message).toContain("rollback failed") + // The original streaming error stays reachable behind the reported failure. + expect((recorded?.cause as Error | undefined)?.message).toBe( + "EROFS: read-only file system, write '/ro/test.py'", + ) + + // The valid final block arrives: execute() must not touch any write path. + await executeWriteFileTool({}) + + // Only the delta's own open()/update() ran; execute() never re-opened the diff view, + // so the dirty buffer could not be saved and no file write or directory creation + // happened - and no approval was even requested. + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockAskApproval).not.toHaveBeenCalled() + expect(mockPushToolResult).not.toHaveBeenCalled() + + // The recorded failure is the single report (counted, not toHaveBeenCalledWith), + // reported with its identity, and the per-task state is released. + const writeCalls = mockHandleError.mock.calls.filter(([context]) => context === "writing file") + expect(writeCalls).toHaveLength(1) + expect(writeCalls[0][1]).toBe(recorded) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("reports only the execute() failure when the write is retried after a failed stream", async () => { // Combined path: a streaming delta failed (the failure is captured, not reported), then // the completed block arrives with valid nativeArgs, so execute() runs and retries the From 605756a7ca8ab758e7af3b8f9f607419a21b3382 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 12:25:47 +0800 Subject: [PATCH 21/24] fix(write-to-file): release the abandoned stream from the malformed-completion caller The parse-failure teardown this unit added is reached only from BaseTool.handle(), but the production malformed-completion path never gets there: presentAssistantMessage emits its own tool_result and returns for a completed known-tool block that has no nativeArgs, so tool.handle() - and with it releaseStreamStateOnParseFailure() - never runs. The per-task stream entry and its TaskAborted listener were retained for the life of the task, a retained streamFailed mark suppressed the diff preview of every later write_to_file in that task, and a diff document the stream opened kept content the user never approved. Release this task's state from that guard, before it emits its single tool_result. The guard owns the one tool_result a native tool call must produce, so the tool is handed a reporter that folds the captured streaming failure into that result instead of pushing a second one: the user still gets the actionable "Error writing file" row, the model still gets exactly one tool_result for the tool_use_id, and the missing-nativeArgs text stays the report when nothing was captured. BaseTool's hook becomes public and names only the handleError member the teardown actually uses, so both entries share one teardown and one report shape. The duplicated JSDoc block above it is folded into the surviving comment. Regression coverage drives presentAssistantMessage itself rather than the handler: a failed streaming delta followed by the malformed completion asserts the entry is gone, the exact registered abort listener is deregistered, the diff document is restored before reset, exactly one tool_result is emitted, and the next write in the same task streams its preview again. Negative controls: dropping the cleanup call reddens the three cleanup tests; dropping the report fold, or the user-visible error row, reddens exactly one; the same spec against the pre-fix presenter fails three of four. --- ...ssage-write-to-file-stream-cleanup.spec.ts | 304 ++++++++++++++++++ .../presentAssistantMessage.ts | 23 +- src/core/tools/BaseTool.ts | 33 +- src/core/tools/WriteToFileTool.ts | 13 +- 4 files changed, 358 insertions(+), 15 deletions(-) create mode 100644 src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts new file mode 100644 index 0000000000..d1b2563874 --- /dev/null +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts @@ -0,0 +1,304 @@ +// Regression coverage for the malformed-completion path. +// +// presentAssistantMessage emits its own tool_result and returns for a completed known-tool block +// that has no nativeArgs, so tool.handle() is never reached and BaseTool's parse-failure teardown +// - the one this chain added - never runs. These tests drive the presenter itself, which is how a +// malformed streamed write_to_file actually reaches the teardown. + +import { RooCodeEventName } from "@roo-code/types" +import { afterEach, beforeEach, describe, expect, it, vi, type MockedFunction } from "vitest" + +import { type Task } from "../../task/Task" +import { isValidToolName } from "../../tools/validateToolUse" +import { writeToFileTool } from "../../tools/WriteToFileTool" +import { fileExistsAtPath } from "../../../utils/fs" + +import { presentAssistantMessage } from "../presentAssistantMessage" + +vi.mock("../../task/Task") +vi.mock("../../tools/validateToolUse", () => ({ + validateToolUse: vi.fn(), + isValidToolName: vi.fn().mockReturnValue(true), +})) +// Only the filesystem probe and the readable-path helper have to be observable; every other +// export of those modules stays real, so the streaming path runs against the real code. +vi.mock("../../../utils/fs", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + fileExistsAtPath: vi.fn().mockResolvedValue(false), + createDirectoriesForFile: vi.fn().mockResolvedValue([]), + } +}) +vi.mock("../../../utils/path", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getReadablePath: (_cwd: string, relPath: string) => relPath } +}) +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureToolUsage: vi.fn(), + captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), + }, + }, +})) + +interface PushedToolResult { + type: string + tool_use_id: string + content: string + is_error?: boolean +} + +// Structural double: the presenter and the streaming path only read these members. The double +// assertion in asTask() is the repo's existing pattern for presenter-level tests (see +// writeToFileTool-partial-state-cleanup.spec.ts); Task itself needs a live extension host. +interface PresenterTask { + taskId: string + instanceId: string + cwd: string + abort: boolean + abandoned: boolean + presentAssistantMessageLocked: boolean + presentAssistantMessageHasPendingUpdates: boolean + currentStreamingContentIndex: number + currentStreamingDidCheckpoint: boolean + assistantMessageContent: Array> + userMessageContent: PushedToolResult[] + didCompleteReadingStream: boolean + didRejectTool: boolean + didAlreadyUseTool: boolean + consecutiveMistakeCount: number + api: { getModel: () => { id: string; info: Record } } + getTaskMode: MockedFunction<() => Promise> + recordToolUsage: MockedFunction<(name: string) => void> + recordToolError: MockedFunction<(name: string, text?: string) => void> + toolRepetitionDetector: { check: MockedFunction<() => { allowExecution: boolean }> } + providerRef: { deref: () => { getState: () => Promise> } } + say: MockedFunction<(type: string, text?: string, images?: unknown) => Promise> + ask: MockedFunction<(type: string, text?: string, partial?: boolean) => Promise<{ response: string }>> + once: MockedFunction<(event: string, listener: () => void) => unknown> + off: MockedFunction<(event: string, listener: () => void) => unknown> + finalizePartialToolAsk: MockedFunction<(text?: string) => Promise> + pushToolResultToUserContent: MockedFunction<(result: PushedToolResult) => boolean> + diffViewProvider: { + editType: "modify" | "create" | undefined + isEditing: boolean + open: MockedFunction<(relPath: string) => Promise> + update: MockedFunction<(content: string, single: boolean) => Promise> + reset: MockedFunction<() => Promise> + revertChanges: MockedFunction<() => Promise> + } +} + +const CALL_ID = "toolu_write_to_file_malformed" +const STREAM_FAILURE = "EACCES: permission denied, open 'src/demo.ts'" + +function buildTask(): PresenterTask { + const diffViewProvider: PresenterTask["diffViewProvider"] = { + editType: undefined, + isEditing: false, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + } + // Mirror the real provider: an open edit is what the rollback below is allowed to revert, + // and both revert and reset end the edit. + diffViewProvider.open.mockImplementation(async () => { + diffViewProvider.isEditing = true + }) + diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewProvider.isEditing = false + }) + diffViewProvider.reset.mockImplementation(async () => { + diffViewProvider.isEditing = false + }) + + const task: PresenterTask = { + taskId: "task-1", + instanceId: "instance-1", + cwd: "/mock/cwd", + abort: false, + abandoned: false, + presentAssistantMessageLocked: false, + presentAssistantMessageHasPendingUpdates: false, + currentStreamingContentIndex: 0, + currentStreamingDidCheckpoint: true, + assistantMessageContent: [], + userMessageContent: [], + didCompleteReadingStream: false, + didRejectTool: false, + didAlreadyUseTool: false, + consecutiveMistakeCount: 0, + api: { getModel: () => ({ id: "test-model", info: {} }) }, + getTaskMode: vi.fn().mockResolvedValue("code"), + recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }) }, + providerRef: { deref: () => ({ getState: async () => ({ experiments: {}, customModes: [] }) }) }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + once: vi.fn(), + off: vi.fn(), + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + pushToolResultToUserContent: vi.fn((result: PushedToolResult) => { + task.userMessageContent.push(result) + return true + }), + diffViewProvider, + } + return task +} + +// The double above is the documented shape of every presenter test in this directory. +const asTask = (task: PresenterTask): Task => task as unknown as Task + +async function present(task: PresenterTask, block: Record): Promise { + task.assistantMessageContent = [block] + task.currentStreamingContentIndex = 0 + await presentAssistantMessage(asTask(task)) +} + +const partialDelta = (content: string): Record => ({ + type: "tool_use", + id: CALL_ID, + name: "write_to_file", + params: { path: "src/demo.ts", content }, + partial: true, +}) + +// The completed block the parser could not finalize: no nativeArgs. +const malformedCompletion = (): Record => ({ + type: "tool_use", + id: CALL_ID, + name: "write_to_file", + params: {}, + partial: false, +}) + +const toolResultsFor = (task: PresenterTask): PushedToolResult[] => + task.userMessageContent.filter((block) => block.type === "tool_result" && block.tool_use_id === CALL_ID) + +const errorSays = (task: PresenterTask): unknown[][] => task.say.mock.calls.filter(([type]) => type === "error") + +// The listener this task registered, read from the registration itself rather than any function. +const registeredAbortListener = (task: PresenterTask): unknown => + task.once.mock.calls.find(([event]) => event === RooCodeEventName.TaskAborted)?.[1] + +// Streams a delta whose diff-view update fails: handlePartial marks the stream failed and keeps +// the error for the authoritative report. The path only counts as stable once it has been seen +// twice, so the first delta only registers the entry and the second reaches the diff view. +async function streamFailedDelta(task: PresenterTask): Promise { + task.diffViewProvider.update.mockRejectedValueOnce(new Error(STREAM_FAILURE)) + await present(task, partialDelta("partial content")) + await present(task, partialDelta("partial content")) +} + +describe("presentAssistantMessage - abandoned write_to_file stream", () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(isValidToolName).mockReturnValue(true) + vi.mocked(fileExistsAtPath).mockReset().mockResolvedValue(false) + vi.spyOn(console, "error").mockImplementation(() => {}) + writeToFileTool.resetPartialState() + }) + + afterEach(() => { + writeToFileTool["taskPartialStreamState"].clear() + vi.restoreAllMocks() + }) + + it("releases the abandoned stream state and reports the captured failure once", async () => { + const task = buildTask() + await streamFailedDelta(task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + const abortListener = registeredAbortListener(task) + expect(abortListener).toBeInstanceOf(Function) + // The stream's own rollback already ran; clear it so the counts below only describe the + // completion path. + task.diffViewProvider.revertChanges.mockClear() + task.diffViewProvider.reset.mockClear() + + await present(task, malformedCompletion()) + + // The leak: the entry and its TaskAborted listener outlived the call. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // Exactly one tool_result for this tool_use_id, carrying the failure the user can act on + // rather than the incidental "missing nativeArgs" text. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].is_error).toBe(true) + expect(results[0].content).toContain(STREAM_FAILURE) + expect(results[0].content).not.toContain("missing nativeArgs") + // Counted by channel rather than toHaveBeenCalledWith: a count of matching says cannot + // auto-pass when the error row is emitted more than once. + const says = errorSays(task) + expect(says).toHaveLength(1) + expect(says[0][1]).toContain("Error writing file:") + // The stream had already reverted, so only the reset is left to do. + expect(task.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(task.diffViewProvider.reset).toHaveBeenCalledTimes(1) + }) + + it("restores the diff document an abandoned stream left open", async () => { + const task = buildTask() + writeToFileTool["getTaskPartialStreamState"](asTask(task)) + task.diffViewProvider.isEditing = true + + await present(task, malformedCompletion()) + + // revertChanges() must run before reset(): reset clears the state the rollback reads, and + // a dirty streamed buffer that survives it is saved by the next open() before approval. + expect(task.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.revertChanges.mock.invocationCallOrder[0]).toBeLessThan( + task.diffViewProvider.reset.mock.invocationCallOrder[0], + ) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // No streaming failure was captured, so the malformed-call report stays the actionable one. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain("missing nativeArgs") + expect(errorSays(task)).toHaveLength(0) + }) + + it("keeps the missing-nativeArgs result for a task that never streamed", async () => { + const task = buildTask() + + await present(task, malformedCompletion()) + + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].is_error).toBe(true) + expect(results[0].content).toContain("missing nativeArgs") + expect(errorSays(task)).toHaveLength(0) + // The cleanup must not create state, touch the diff view, or register a listener for a + // task that never streamed a write. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.once).not.toHaveBeenCalled() + expect(task.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(task.diffViewProvider.reset).not.toHaveBeenCalled() + }) + + it("lets the next write in the same task stream its diff preview again", async () => { + const task = buildTask() + await streamFailedDelta(task) + const opensAfterFailure = task.diffViewProvider.open.mock.calls.length + + await present(task, malformedCompletion()) + + // A retained streamFailed mark makes handlePartial() skip its work for every later delta, + // so this task would never show a diff preview again. + await present(task, partialDelta("next write")) + await present(task, partialDelta("next write")) + + expect(task.diffViewProvider.open.mock.calls.length).toBe(opensAfterFailure + 1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + }) +}) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 417dcf7a45..22c359c544 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -563,12 +563,33 @@ async function presentAssistantMessageBlock(cline: Task): Promise { // Best-effort only } + // This guard returns before tool.handle(), so handle()'s parse-failure teardown + // never runs on the malformed-completion path: the per-task stream entry and its + // TaskAborted listener would outlive the call, a retained streamFailed mark would + // suppress this task's later diff previews, and a diff document the stream opened + // would keep content the user never approved. Release this task's state before + // emitting the result. The guard owns the single tool_result a native tool call + // must produce, so the tool's handleError folds the failure it reports into that + // one result instead of pushing a second one. + const abandonedStreamFailure: { report?: string } = {} + if (block.name === "write_to_file") { + await writeToFileTool.releaseStreamStateOnParseFailure(cline, { + handleError: async (action, error) => { + await cline.say( + "error", + `Error ${action}:\n${error.message ?? JSON.stringify(serializeError(error), null, 2)}`, + ) + abandonedStreamFailure.report = `Error ${action}: ${JSON.stringify(serializeError(error))}` + }, + }) + } + // Push tool_result directly without setting didAlreadyUseTool so streaming can // continue gracefully. cline.pushToolResultToUserContent({ type: "tool_result", tool_use_id: sanitizeToolUseId(toolCallId), - content: formatResponse.toolError(errorMessage), + content: formatResponse.toolError(abandonedStreamFailure.report ?? errorMessage), is_error: true, }) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 1fddd606fb..04e8fde764 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -99,20 +99,27 @@ export abstract class BaseTool { } /** - * Release the state this tool holds for one task on paths that never reach - * execute(). No-op for tools without per-task state; the scope is a single task - * because tool instances are singletons shared by concurrent tasks. - */ - /** - * Teardown boundary for the handle() parse-failure path, where execute() never - * runs. Default: there is no per-task streaming state to release, so the generic - * parse error is what the user sees. A tool that keeps per-task stream state may - * release it, restore any diff document a stream opened, and report a more specific - * failure - returning true suppresses the incidental parse error so the failure is - * reported exactly once. Per-task only: a global teardown would clobber another task - * that is still streaming through this singleton. + * Teardown boundary for a completed tool call that never reaches execute(): the + * parse-failure catch in handle(), and presentAssistantMessage's missing-nativeArgs + * guard, which emits its own tool_result and returns before handle() is reached. + * No-op for tools without per-task state; the scope is a single task because tool + * instances are singletons shared by concurrent tasks. + * + * Default: there is no per-task streaming state to release, so the generic parse + * error is what the user sees. A tool that keeps per-task stream state may release + * it, restore any diff document a stream opened, and report a more specific failure + * through callbacks.handleError - returning true suppresses the incidental parse + * error so the failure is reported exactly once. Per-task only: a global teardown + * would clobber another task that is still streaming through this singleton. + * + * Public because that caller sits outside handle(). Its guard owns the single + * tool_result a native tool call must produce, so it passes a handleError that folds + * the reported failure into that result instead of pushing a second one. */ - protected async releaseStreamStateOnParseFailure(_task: Task, _callbacks: ToolCallbacks): Promise { + async releaseStreamStateOnParseFailure( + _task: Task, + _callbacks: Pick, + ): Promise { return false } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ba43cc8e05..5ae08f90a2 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -190,8 +190,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * act on, so it is reported with the same "writing file" context execute()'s catch uses, * and true is returned to suppress the incidental parse error - the failure surfaces * exactly once. + * + * Also reached from presentAssistantMessage's missing-nativeArgs guard: that guard + * emits its own tool_result and returns before handle() runs, so this teardown is + * the only thing that can release the entry a stream left behind. Skip it and the + * per-task entry plus its TaskAborted listener outlive the call, the retained + * streamFailed mark suppresses this task's later diff previews, and a diff document + * the stream opened keeps content the user never approved. That guard passes a + * handleError that feeds the failure into the single tool_result it owns. */ - protected override async releaseStreamStateOnParseFailure(task: Task, callbacks: ToolCallbacks): Promise { + override async releaseStreamStateOnParseFailure( + task: Task, + callbacks: Pick, + ): Promise { const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) if (!state) { return false From 9bdc349bd04a9901d8e9c2392c7716f9cd9d92b8 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 14:39:28 +0800 Subject: [PATCH 22/24] fix(write-to-file): keep the malformed-call report reachable when the teardown fails Two findings on the caller-side release, both about the one tool_result this guard owes the provider. A rejected Task.say() escaped the reporter, so the guard exited before storing the failure text and before emitting the tool_result: the provider was left with a tool_use that has no matching result and the block-completion bookkeeping never ran. Store the report before attempting the chat row, and treat a teardown that throws as a logged leak rather than a reason to skip the mandatory result. The hook also reports - and still returns false - when a refused rollback found no captured streaming error standing in for the parse error. That is a second, distinct failure, so the single tool_result now carries both it and the missing-nativeArgs error; using the rollback report alone told the model the call was malformed and then stayed silent about it. Dropping the nullish fallback in the error row removes a branch no caller can reach - the hook only ever reports Error values - which is also the surviving mutant the mutation-diff advisory named. Coverage: a rejected chat row still yields exactly one tool_result carrying the streaming failure and marks the block complete; a refused rollback yields one tool_result naming both failures. Negative controls: rethrowing from the teardown reddens exactly the first, dropping the second failure from the result reddens exactly the second. --- ...ssage-write-to-file-stream-cleanup.spec.ts | 39 +++++++++++++++++++ .../presentAssistantMessage.ts | 37 +++++++++++++----- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts index d1b2563874..4123f77857 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts @@ -70,6 +70,7 @@ interface PresenterTask { assistantMessageContent: Array> userMessageContent: PushedToolResult[] didCompleteReadingStream: boolean + userMessageContentReady: boolean didRejectTool: boolean didAlreadyUseTool: boolean consecutiveMistakeCount: number @@ -132,6 +133,7 @@ function buildTask(): PresenterTask { assistantMessageContent: [], userMessageContent: [], didCompleteReadingStream: false, + userMessageContentReady: false, didRejectTool: false, didAlreadyUseTool: false, consecutiveMistakeCount: 0, @@ -301,4 +303,41 @@ describe("presentAssistantMessage - abandoned write_to_file stream", () => { expect(task.diffViewProvider.open.mock.calls.length).toBe(opensAfterFailure + 1) expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) }) + + it("still emits the required tool_result when the error row cannot be saved", async () => { + const task = buildTask() + await streamFailedDelta(task) + task.say.mockRejectedValue(new Error("chat row unavailable")) + + await expect(present(task, malformedCompletion())).resolves.toBeUndefined() + + // A rejected chat row must not strand the turn: the provider still needs exactly one + // tool_result for this tool_use_id, and the block still has to be marked complete. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain(STREAM_FAILURE) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.userMessageContentReady).toBe(true) + }) + + it("keeps the malformed-call error when the rollback itself failed", async () => { + const task = buildTask() + writeToFileTool["getTaskPartialStreamState"](asTask(task)) + task.diffViewProvider.isEditing = true + task.diffViewProvider.revertChanges.mockRejectedValue(new Error("restore refused")) + + await present(task, malformedCompletion()) + + // A refused rollback is a second, distinct failure: the hook reports it and still returns + // false, so the one tool_result has to carry both it and the malformed-call error - the + // model otherwise never learns the call could not be finalized. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain("write_to_file rollback failed: restore refused") + expect(results[0].content).toContain("missing nativeArgs") + const says = errorSays(task) + expect(says).toHaveLength(1) + expect(says[0][1]).toContain("Error writing file:") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 22c359c544..6909b8e730 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -572,16 +572,33 @@ async function presentAssistantMessageBlock(cline: Task): Promise { // must produce, so the tool's handleError folds the failure it reports into that // one result instead of pushing a second one. const abandonedStreamFailure: { report?: string } = {} + let reportedStreamFailure = false if (block.name === "write_to_file") { - await writeToFileTool.releaseStreamStateOnParseFailure(cline, { - handleError: async (action, error) => { - await cline.say( - "error", - `Error ${action}:\n${error.message ?? JSON.stringify(serializeError(error), null, 2)}`, - ) - abandonedStreamFailure.report = `Error ${action}: ${JSON.stringify(serializeError(error))}` - }, - }) + try { + reportedStreamFailure = await writeToFileTool.releaseStreamStateOnParseFailure(cline, { + handleError: async (action, error) => { + // Store the report before touching the UI: the failure text is the only + // copy of it, and a chat row that cannot be saved must not cost the model + // the tool_result this guard owes the provider. + abandonedStreamFailure.report = `Error ${action}: ${JSON.stringify(serializeError(error))}` + await cline.say("error", `Error ${action}:\n${error.message}`) + }, + }) + } catch (teardownError) { + // The teardown protects this call from a leak; it must never be the reason the + // mandatory tool_result and the block-completion bookkeeping below are skipped. + console.error("Error releasing abandoned write_to_file stream state:", teardownError) + } + } + // The hook returns true when the failure it reported replaces the parse error. When it + // reports anyway and still returns false - a refused rollback with no captured streaming + // error is a second, distinct failure - the model must also learn the call could not be + // finalized, so both facts go into the one tool_result. + let resultContent = errorMessage + if (abandonedStreamFailure.report) { + resultContent = reportedStreamFailure + ? abandonedStreamFailure.report + : `${abandonedStreamFailure.report}\n${errorMessage}` } // Push tool_result directly without setting didAlreadyUseTool so streaming can @@ -589,7 +606,7 @@ async function presentAssistantMessageBlock(cline: Task): Promise { cline.pushToolResultToUserContent({ type: "tool_result", tool_use_id: sanitizeToolUseId(toolCallId), - content: formatResponse.toolError(abandonedStreamFailure.report ?? errorMessage), + content: formatResponse.toolError(resultContent), is_error: true, }) From 3e8c7b683577b41b2e56ea06ea0c2b015d82244c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 15:15:01 +0800 Subject: [PATCH 23/24] fix(write-to-file): keep a lost chat row from changing the reported failure The guard's catch kept the mandatory tool_result, but it also swallowed the rejection before the release call could answer whether its failure replaces the parse error, so a task whose chat row could not be saved reported the captured streaming failure together with the incidental missing-nativeArgs text. Catch the row inside the reporter instead: the hook's answer survives, the tool_result names only the failure the user can act on, and the outer catch still covers the rest of the teardown. The rejected-row test now asserts the result does not name the malformed call. Negative control: rethrowing from the inner catch reddens exactly that test, one of six. --- ...sistantMessage-write-to-file-stream-cleanup.spec.ts | 3 +++ src/core/assistant-message/presentAssistantMessage.ts | 10 +++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts index 4123f77857..8051ee44c0 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts @@ -316,6 +316,9 @@ describe("presentAssistantMessage - abandoned write_to_file stream", () => { const results = toolResultsFor(task) expect(results).toHaveLength(1) expect(results[0].content).toContain(STREAM_FAILURE) + // The lost chat row must not change which failure is reported: the hook still answered + // that its failure replaces the parse error. + expect(results[0].content).not.toContain("missing nativeArgs") expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(task.userMessageContentReady).toBe(true) }) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 6909b8e730..f5a3de9379 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -581,7 +581,15 @@ async function presentAssistantMessageBlock(cline: Task): Promise { // copy of it, and a chat row that cannot be saved must not cost the model // the tool_result this guard owes the provider. abandonedStreamFailure.report = `Error ${action}: ${JSON.stringify(serializeError(error))}` - await cline.say("error", `Error ${action}:\n${error.message}`) + try { + await cline.say("error", `Error ${action}:\n${error.message}`) + } catch (reportError) { + // A chat row that cannot be saved must not cost the model the tool_result + // this guard owes the provider, and it must not change which failure that + // result reports: letting it escape here would also lose the hook's answer + // on whether the parse error is still worth mentioning. + console.error("Error saving abandoned write_to_file error row:", reportError) + } }, }) } catch (teardownError) { From f2fc125e309b7c590dd7129bc31ce71dc0bcf15f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 17:58:31 +0800 Subject: [PATCH 24/24] test(diff-view): cover a rejected tab close in the new-file rollback The branch that converts a rejected tabGroups.close() into a rollback failure had no focused test: the spec covered only a close that resolves false. Add the rejected-close test at the same layer, pinning that revertChanges() rejects with the rollback error, keeps the original rejection as its cause, and leaves the placeholder unlink and the created-directory rmdir unrun - after a refused discard the caller owns the next step, so the rollback must not delete underneath the still-open tab. Negative control: treating the rejection as a closed tab (closed = true in the catch) reddens exactly this test and nothing else; against the pre-fix production shape that logged and swallowed the rejection, the test fails with revertChanges() resolving and the delete running underneath the open tab. Addresses the Regression Evidence row of the pre-merge checklist. --- .../editor/__tests__/DiffViewProvider.spec.ts | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 304db019c3..2825eb7317 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1599,6 +1599,76 @@ describe("DiffViewProvider", () => { } } }) + it("revertChanges() reports a close that rejected instead of deleting underneath it", async () => { + // tabGroups.close() can reject, not only resolve false (the editor throws on veto). The rejection + // used to be logged and swallowed, so the rollback continued, unlinked the placeholder, and + // removed the directories it had created underneath a tab that was still open. The rejection + // must surface as the rollback failure with the original error kept as its cause, and the + // filesystem cleanup must not run: after a refused discard the caller owns the next step + // (fail closed), so this rollback must not delete around it. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const closeError = new Error("Tab close rejected by the editor") + 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 originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: vi.fn().mockRejectedValue(closeError), + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + originalContent: "", + }) + + try { + // The failure is reported, not swallowed: revertChanges() rejects with the rollback + // error, and the original rejection stays reachable as its cause. + const failure = await diffViewProvider.revertChanges().then( + () => undefined, + (error: unknown) => (error instanceof Error ? error : new Error(String(error))), + ) + expect(failure).toBeInstanceOf(Error) + expect(failure?.message).toContain("could not close the restored buffer") + expect(failure?.cause).toBe(closeError) + // Cleanup ownership is retained: the close was attempted without a force flag, and + // neither the placeholder unlink nor the created-directory rmdir ran underneath the + // still-open tab. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + }) it("revertChanges() restores and saves the buffer through the real rollback path", async () => { // The two tests above spy restorePreStreamBuffer() and saveBufferClean(), so the restoration // itself never runs there and only proves ordering. This one leaves both real: the buffer must