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/26] 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/26] 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/26] 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/26] fix(write-to-file): capture streaming failure once, report it once --- src/core/tools/WriteToFileTool.ts | 51 +++- .../tools/__tests__/writeToFileTool.spec.ts | 232 +++++++++++++++++- 2 files changed, 267 insertions(+), 16 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c9776553b7..267c728c27 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -353,6 +353,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content + const partialStreamFailureKey = this.getPartialStreamFailureKey(task) + + // A prior streaming delta for this task already hit a fatal filesystem error. + // Skip further streaming work so we don't create a new partial tool message on every + // subsequent delta. execute() will report the error once when the block completes. + if (this.taskPartialStreamState.get(partialStreamFailureKey)?.streamFailed) { + return + } // Get (or create) this task's state; registers the TaskAborted teardown listener // once, so abandoned streams are torn down even if execute() never runs. @@ -385,12 +393,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -406,14 +408,37 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.ask("tool", partialMessage, block.partial).catch(() => {}) if (newContent) { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) - } + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // rethrow: the same filesystem operation is retried in execute() once the + // block completes, and that authoritative non-partial path reports the + // error to the user. Surfacing it here too would show the same error twice. + // Swallowing it here is safe because the agent loop advances naturally when + // the non-partial block arrives (it does not depend on this throw). + console.error(`Error streaming write_to_file diff view:`, error) + // Mark the stream as failed so later deltas don't re-attempt and spawn a new + // partial tool message each time. Retain the original error: if the final + // block later fails to parse, execute() never runs and only + // onParameterParseFailure() can report this failure to the user. + partialStreamState.streamFailed = true + partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) + await this.finalizePartialToolAskAfterFailure(task, partialMessage) + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty). + await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index bec63332a7..f4677277d7 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -97,6 +97,19 @@ describe("writeToFileTool", () => { const testContent = "Line 1\nLine 2\nLine 3" const testContentWithMarkdown = "```javascript\nLine 1\nLine 2\n```" + // The exact payload handlePartial() streams as the partial `tool` ask for the default + // test scenario (new file, readable path, in-workspace, not write-protected). + // finalizePartialToolAsk() no-ops on a text mismatch, so finalize assertions must + // match this exactly: a weaker matcher (e.g. expect.any(String), which a relPath also + // satisfies) would pass a mutant that passes the wrong text and leaves the spinner stuck. + const expectedPartialToolMessage = JSON.stringify({ + tool: "newFileCreated", + path: "test/path.txt", + content: testContent, + isOutsideWorkspace: false, + isProtected: false, + }) + // Mocked functions with correct types const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction @@ -429,6 +442,25 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) }) + it("does not share path stabilization between tasks with the same path", async () => { + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).not.toHaveBeenCalled() + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).not.toHaveBeenCalled() + + mockCline.taskId = "task-1" + mockCline.instanceId = "instance-1" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + }) it("cleans per-task partial state when the task aborts before execute finalization", async () => { let abortCleanup: (() => void) | undefined @@ -465,8 +497,49 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() }) + it("does not issue a partial ask when content is undefined after path stabilization", async () => { + // Delta 1 stabilizes the path. Delta 2 repeats it but carries no content yet: the + // `newContent === undefined` clause must short-circuit the ask even though the path itself has + // stabilized. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ content: undefined }, { isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("does not reopen an already open diff view during streaming", async () => { + // The diff view is already open for this task (isEditing). A stabilized delta must still update + // the streamed content but must not call open() again -- reopening would discard the view's + // current state. + mockCline.diffViewProvider.isEditing = true + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) + }) + it("logs the streaming diff view failure with the write_to_file context", async () => { + // The catch arm logs a context-specific message before swallowing the error (execute() reports + // the authoritative one). The message must keep the write_to_file context so the log is + // actionable. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: permission denied")) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error streaming write_to_file diff view:", + expect.anything(), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) @@ -575,17 +648,170 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("handles partial streaming errors after path stabilizes", async () => { + + + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { + // The same filesystem operation is retried in execute() once the block completes, + // and that authoritative non-partial path reports the error to the user. Surfacing + // it during streaming too would show the same error twice, so handlePartial must NOT + // route streaming errors through handleError. mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) // First call - path not yet stabilized, no error yet await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, error occurs + // Second call with same path - path is now stabilized, error occurs but is swallowed + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial open() fails", async () => { + // Regression test: when diffViewProvider.open() throws during streaming (e.g. EACCES/EROFS + // on a read-only path), the partial tool ask created at the top of handlePartial leaves the + // UI spinner stuck. handlePartial must finalize the partial message and reset the diff view, + // and must NOT surface a duplicate error (execute() reports the authoritative one). + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized await executeWriteFileTool({}, { isPartial: true }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + + // Second call - path stabilized, update() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("does not spawn a new partial tool message on each streaming delta after a failure", async () => { + // Regression test: after diffViewProvider.open() throws and the partial message is + // finalized + diff view reset, the next streaming delta saw a non-partial last message + // and created a brand new "Zoo wants to edit this file" message -- repeating once per + // delta. After the fix, partialStreamFailed short-circuits subsequent deltas so only + // the single initial partial ask is issued. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, ask issued once, open() fails, stream marked failed + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Deltas 3..5 - must be short-circuited, no further asks + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Only the single partial ask from delta 2 should have been issued + expect(mockCline.ask).toHaveBeenCalledTimes(1) + // open() must not be retried after the first failure + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + }) + + + + + + + + + + it("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.update.mockResolvedValue(undefined) + mockCline.diffViewProvider.editType = undefined + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2) + }) + + + + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called + // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate + // in presentAssistantMessage was never reached and the agent loop stalled permanently. + // After the fix the call is removed entirely -- handlePartial never touches the filesystem. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // First call -- path not yet stabilized, returns early + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // Second call -- path stabilized; createDirectoriesForFile must NOT be called from + // handlePartial, so the mock rejection must not trigger and handleError must not be called + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + + + }) }) From e3c10401f760a33cb977cd3cfdc1f1af8de49d3f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:18:25 +0800 Subject: [PATCH 05/26] feat(tools): onParameterParseFailure teardown boundary --- src/core/tools/BaseTool.ts | 39 +++- src/core/tools/WriteToFileTool.ts | 22 ++ .../tools/__tests__/writeToFileTool.spec.ts | 192 ++++++++++++++++++ 3 files changed, 250 insertions(+), 3 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dc16a9d81d 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -155,9 +155,23 @@ export abstract class BaseTool { throw new Error("Tool call is missing native arguments (nativeArgs).") } } catch (error) { - console.error(`Error parsing parameters:`, error) - const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}` - await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + const parseError = error instanceof Error ? error : new Error(String(error)) + console.error(`Error parsing parameters:`, parseError) + // Final args could not be parsed (e.g. the model's tool call was truncated + // mid-JSON by the output token limit), so execute() will never run. If a + // streaming delta already opened a partial "tool" ask (partial: true), + // finalize it here or the webview spinner stays stuck indefinitely. + await task.finalizePartialToolAsk().catch((finalizeError) => { + console.error(`Error finalizing ${this.name} partial tool ask:`, finalizeError) + }) + // execute() never runs on this path, so tools that keep per-task state + // outside execute() (streaming failure marks, abort listeners) get their + // one remaining teardown boundary here. + const reportedStreamingFailure = await this.onParameterParseFailure(task, callbacks, parseError) + if (!reportedStreamingFailure) { + const errorMessage = `Failed to parse ${this.name} parameters: ${parseError.message}` + await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + } // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. // Do NOT call pushToolResult here to avoid duplicate tool_result payloads. return @@ -166,4 +180,23 @@ export abstract class BaseTool { // Execute with typed parameters await this.execute(params, task, callbacks) } + + /** + * Teardown boundary for the native-argument parse-failure path in handle(). + * + * When nativeArgs are missing or malformed, execute() never runs, so per-task + * state a tool registered outside execute() (streaming failure marks, abort + * listeners) is never torn down there. Streaming tools override this to tear + * that state down and, when a streaming delta already failed, to report the + * captured streaming error instead of the generic parse error. + * + * @param task - Task instance + * @param callbacks - Tool execution callbacks + * @param parseError - The native-argument parse error + * @returns true when the override already reported the failure to the user, + * so handle() suppresses the generic parse error + */ + protected async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + return false + } } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 267c728c27..ba25a9f2c0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -174,6 +174,28 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * "writing file" context execute()'s catch uses, and suppress the incidental * parse error. */ + override async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (!state) { + return false + } + this.resetTaskPartialState(task) + // Streaming may have opened the diff view with unapproved partial content. + // execute() never runs on this path, so its error cleanup (revert + reset) + // never fires: restore the document here so a user save cannot persist + // content the write never completed (the same invariant the denial and + // streaming-failure paths maintain). Both helpers no-op when no view is + // open. + await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + if (!state.streamError) { + return false + } + void parseError + await callbacks.handleError("writing file", state.streamError) + return true + } + override resetPartialState(): void { super.resetPartialState() for (const state of this.taskPartialStreamState.values()) { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index f4677277d7..dddd139011 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -541,7 +541,123 @@ describe("writeToFileTool", () => { } }) + it("reports the captured streaming error instead of the parse error when the final block fails to parse, and clears the per-task state", async () => { + // A streaming delta fails with a filesystem error (streamFailed + streamError are + // captured). The final block then arrives without nativeArgs, so execute() never + // runs: the parse-failure teardown boundary must report the original filesystem + // error under the "writing file" context (not the incidental parse error) and tear + // down the per-task state, so the next write_to_file stream in this task is not + // blocked by the stale streamFailed guard and no abort listener leaks. + const fsError = new Error("EACCES: permission denied") + mockCline.diffViewProvider.open.mockRejectedValue(fsError) + // Capture the abort listener of the state created by the first deltas: it is the + // exact reference that the parse-failure teardown must detach. + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError) + expect(mockHandleError).not.toHaveBeenCalledWith( + expect.stringContaining("parsing write_to_file"), + expect.anything(), + ) + // The parse-failure teardown also restores the diff document: the + // streaming failure above already reverted it once (revert + reset), + // and the parse path runs the same cleanup again because execute() + // never runs on this path. + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) + // Per-task state torn down at this boundary: guard cleared, the exact + // registered abort listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + + // The next stream for the same task must issue a partial ask again (the stale + // streamFailed guard is gone). + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("restores the diff document when the final block fails to parse after successful streaming", async () => { + // Streaming opened the diff view with unapproved partial content (open and + // update both succeeded, so no streaming error was captured). The final block + // then arrives without nativeArgs: execute() never runs, so its error cleanup + // never fires. The parse-failure teardown must still restore the document + // (revert before reset) or a user save could persist content the write never + // completed, and must report the generic parse error (no streaming error to + // surface instead). + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + // Delta 1 - stabilize path; delta 2 - streams the partial content into the + // diff view (open + update resolve). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // No streaming error was captured: the generic parse error is reported. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The diff document is restored by the parse path itself (no streaming + // failure happened, so this is the only revert + reset in the test). + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + // Per-task state torn down: guard cleared, exact listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("path stabilization predicate", () => { @@ -755,7 +871,83 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) }) + it("finalizes any open partial tool ask when final args cannot be parsed", async () => { + // Regression test: a write_to_file block whose final args fail to parse (e.g. the + // tool call was truncated mid-JSON by the output token limit) never reaches + // execute(). A streaming delta for that block may already have opened a partial + // `tool` ask (partial: true) -- BaseTool.handle must finalize it, otherwise the + // UI spinner stays stuck even though the parse error bubble was shown. + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, partial ask issued once + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The parse error is still reported, and the open partial ask is finalized first. + // No argument: BaseTool.handle() calls finalizePartialToolAsk() with no text, so a + // mutation passing wrong text would leave findLast() unmatched and the spinner + // stuck. (toHaveBeenCalledWith(undefined) does not match a no-arg call under + // vitest's matcher semantics: [] is not equal to [undefined].) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + }) + + it("continues parse failure cleanup when finalizing the partial ask fails", async () => { + // Pins the .catch arm on task.finalizePartialToolAsk() in BaseTool.handle(): when the + // final args cannot be parsed and finalizing the open partial ask also fails, the + // failure must only be logged so the parse error is still reported to the user. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // Same no-argument contract as the sibling test above: the finalize call in + // BaseTool.handle() carries no text. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + // The parse error is still reported despite the failed finalization. + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + } finally { + consoleErrorSpy.mockRestore() + } + }) From 445c2f5be09661ade42e01fae8ca9e5a61c24afa Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 01:24:08 +0800 Subject: [PATCH 06/26] chore(ci): re-trigger the unit suite after the flaky writeToFileTool partial-path case Same flaky case as the u5 run: the core project passes locally at this head and the case passes in isolation. Re-running to confirm. From 1e0f1c4c5b48a5868e3e47eacd8aec6d38a99b03 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 02:52:47 +0800 Subject: [PATCH 07/26] test(write-to-file): align the partial-path case with handlePartial's no-filesystem contract Same fix as p1066/u5 (6906c026a): platform-unit-test (ubuntu-latest) fails here and passes locally because the failing case is it.skipIf(process.platform === "win32"). The case asserted that the second streaming delta calls createDirectoriesForFile - the exact call this chain removes, because an unguarded mkdir in handlePartial threw EROFS into BaseTool.handle() without setting didRejectTool/didAlreadyUseTool, stalling the agent loop. The chain's own regression test pins the new contract; this older case still pinned the old one, so the two contradicted and only Linux CI saw it. Rewritten as "defers parent directory creation to execute() while streaming": no filesystem work while streaming, directories still created by the authoritative non-partial execute(). Local run: the file passes; the rewritten case also passes with the win32 skip lifted. --- src/core/tools/__tests__/writeToFileTool.spec.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index dddd139011..07556de50a 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -310,14 +310,17 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "defers parent directory creation to execute() while streaming", async () => { - // First call - path not yet stabilized + // Streaming deltas must not touch the filesystem at all. An unguarded + // createDirectoriesForFile here threw EROFS up into BaseTool.handle(), which never + // set didRejectTool/didAlreadyUseTool, so the agent loop stalled permanently. + // The directories are still created - by the authoritative non-partial execute(). + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, ) From b04e2241e937cdb0faebb8f12c048e77311c5092 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 03:34:59 +0800 Subject: [PATCH 08/26] fix(tools): scope the write_to_file stream teardown to one task and finalize the failed retry's ask Two findings on this unit's own state: 1) resetPartialState() is overridden to clear the whole taskPartialStreamState map, and execute() calls it on both the success and the error path. The map is keyed per task precisely so two providers can stream write_to_file through this singleton at once, so task A's execute() was deleting task B's entry while B was still streaming: B loses streamFailed (its next delta re-opens the diff view and spawns a duplicate partial ask - the exact case the stabilization guard prevents) and loses streamError. execute() now calls super.resetPartialState() (the base field is genuinely instance-global) plus resetTaskPartialState(task) for this task only. 2) On the diff-view branch execute() opens its own partial ask before the write. If the write then throws, the catch reported the error and reset without finalizing that ask, leaving the spinner and Save/Reject buttons live for a tool call that had already failed. The catch now finalizes the pending ask first. Tests (writeToFileTool.spec.ts, new per-task stream state isolation block): a second streaming task keeps its streamFailed/streamError across another task's execute(), and a failing save finalizes the ask with the exact partial payload. Both fail on the pre-fix code (2 failed / 38 passed) and pass after (40 passed). Local: eslint clean on both files with --prune-suppressions (no suppression change), package tsc clean. --- src/core/tools/WriteToFileTool.ts | 20 +++++++- .../tools/__tests__/writeToFileTool.spec.ts | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ba25a9f2c0..27944bdfc8 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -208,6 +208,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path let newContent = params.content + // Set when this execute() opens its own partial tool ask (diff-view branch), so + // the catch below can finalize it. Undefined on the saveDirectly branch. + let pendingPartialAsk: string | undefined if (!relPath) { task.consecutiveMistakeCount++ @@ -315,6 +318,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) + pendingPartialAsk = partialMessage await task.ask("tool", partialMessage, true).catch(() => {}) await task.diffViewProvider.open(relPath) } @@ -358,15 +362,27 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + // BaseTool's reset only clears this instance's lastSeenPartialPath; the + // stream state added here is keyed per task. Clearing the whole map from + // one task's execute() would drop another task's streamFailed/streamError + // while it is still streaming, so tear down only this task's entry. + super.resetPartialState() + this.resetTaskPartialState(task) task.processQueuedMessages() return } catch (error) { + // The diff-view branch above may have opened a fresh partial ask for this + // (retried) write. Finalize it before tearing down, or the spinner and + // Save/Reject buttons stay live for a tool call that has already failed. + if (pendingPartialAsk !== undefined) { + await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) + } await handleError("writing file", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + super.resetPartialState() + this.resetTaskPartialState(task) return } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 07556de50a..34b3dfe8a6 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -728,6 +728,52 @@ describe("writeToFileTool", () => { }) }) + describe("per-task stream state isolation", () => { + // A second task streaming through the same singleton while mockCline's execute() + // runs. Structural double, same pattern as the partial-state-cleanup spec. + function buildStreamingTask(taskId: string, instanceId: string) { + return { + taskId, + instanceId, + once: vi.fn(), + off: vi.fn(), + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + } + } + + it("leaves another task's stream state intact when execute() completes", async () => { + const other = buildStreamingTask("task-2", "instance-2") + const otherState = writeToFileTool["getTaskPartialStreamState"](other as never) + otherState.streamFailed = true + otherState.streamError = new Error("other task stream failure") + + await executeWriteFileTool({}) + + // The other task is still streaming: its failure state must survive, or its + // next delta re-opens the diff view and spawns a duplicate partial ask. + const retained = writeToFileTool["taskPartialStreamState"].get("task-2.instance-2") + expect(retained).toBeDefined() + expect(retained?.streamFailed).toBe(true) + expect(retained?.streamError?.message).toBe("other task stream failure") + expect(other.off).not.toHaveBeenCalled() + }) + + it("finalizes the partial ask when the write itself fails", async () => { + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // The diff-view branch opened a partial ask for this write; without the + // finalize the spinner and Save/Reject stay live after the failure. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + }) + }) + describe("user interaction", () => { it("reverts changes when user rejects approval", async () => { mockAskApproval.mockResolvedValue(false) From 0dc92ca73c9c79418df09c7c0bf9fcc362a9583f Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:44:46 +0800 Subject: [PATCH 09/26] fix(tools): do not report a completed teardown when the diff revert fails onParameterParseFailure() tore down the per-task stream state first, then reverted the diff document with the failure logged and swallowed, then reset the diff provider. A failed revert therefore looked like a completed teardown even though the editor can still hold the unapproved partial content - and a user save of that editor lands a write the task never approved, with nothing in the UI saying so. The revert now runs while the recovery state still exists, revertDiffChangesBeforeReset returns whether it succeeded, and a failed revert is surfaced with task.say("error", ...) naming the hazard. The remaining cleanup (reset + per-task teardown) still runs, so the task is never left half-torn-down. Test: the seeded per-task state is observed from inside the failing revertChanges double, so the ordering is pinned (state present during rollback, gone afterwards), and the error say is asserted. Without the fix the test fails (1 failed / 5 passed); with it 6 pass. The streaming-failure cleanup keeps its existing log-only behaviour: that path's error is reported by the authoritative non-partial block in execute(), as its comment states, so adding a second message there would double-report the same failure. Local: writeToFileTool-partial-state-cleanup.spec.ts 6 passed, eslint clean with --prune-suppressions (no suppression change), package tsc clean. --- src/core/tools/WriteToFileTool.ts | 35 +++++++++++++----- ...teToFileTool-partial-state-cleanup.spec.ts | 37 ++++++++++++++++++- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 27944bdfc8..69aed4720c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -144,13 +144,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * streamed content; a user save would then persist a write the task never completed * (denied or failed before approval). Must run BEFORE resetDiffViewAfterWrite(), * since reset() clears the state revertChanges() relies on. No-op when no diff view - * is open. Failures are logged and swallowed so the remaining cleanup (reset, - * per-task state teardown) always continues. + * is open. Failures are logged and reported through the return value: the caller + * must not treat the teardown as complete when this returns false, because the + * document may still hold the unapproved content, but the remaining cleanup (reset, + * per-task state teardown) still runs so the task is not left half-torn-down. */ - private async revertDiffChangesBeforeReset(task: Task): Promise { - await task.diffViewProvider.revertChanges().catch((revertError) => { + private async revertDiffChangesBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.revertChanges() + return true + } catch (revertError) { console.error("Error reverting write_to_file diff view changes:", revertError) - }) + return false + } } private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { @@ -179,15 +185,26 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!state) { return false } - this.resetTaskPartialState(task) // Streaming may have opened the diff view with unapproved partial content. // execute() never runs on this path, so its error cleanup (revert + reset) // never fires: restore the document here so a user save cannot persist // content the write never completed (the same invariant the denial and - // streaming-failure paths maintain). Both helpers no-op when no view is - // open. - await this.revertDiffChangesBeforeReset(task) + // streaming-failure paths maintain). Both helpers no-op when no view is open. + // The revert runs BEFORE the per-task state is torn down: when it fails, the + // document can still hold that unapproved content, and the recovery state has + // to exist while the outcome is decided and reported. + const reverted = await this.revertDiffChangesBeforeReset(task) + this.resetTaskPartialState(task) await this.resetDiffViewAfterWrite(task) + if (!reverted) { + // Do not report a completed teardown: the editor may still show content this + // task never approved, and saving it would land a write the user never + // authorized. The user has to be able to tell that from the UI. + await task.say( + "error", + "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", + ) + } if (!state.streamError) { return false } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 4506742c26..93ba46560c 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -19,6 +19,7 @@ interface CleanupTask { revertChanges: MockedFunction<() => Promise> } finalizePartialToolAsk: MockedFunction<() => Promise> + say: MockedFunction<(...args: unknown[]) => Promise> } function buildTask(taskId: string, instanceId: string): Task { @@ -32,6 +33,7 @@ function buildTask(taskId: string, instanceId: string): Task { revertChanges: vi.fn().mockResolvedValue(undefined), }, finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(undefined), } return task as unknown as Task } @@ -87,8 +89,41 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) }) + it("reverts while the recovery state exists and reports the hazard when the revert fails", async () => { + const task = buildTask("revert-fails-teardown", "inst-5") + const t = task as unknown as CleanupTask + let stateSizeDuringRevert = -1 + t.diffViewProvider.revertChanges = vi.fn(async () => { + // The recovery state must still be present while the rollback runs. + stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size + throw new Error("revert failed") + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // Seed the per-task stream state so this teardown boundary is taken at all. + writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + // Only handleError is reached when no streaming error was recorded; the + // structural double is the existing pattern in this file. + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // A failed rollback is not a completed teardown: the user is told the editor may + // still hold content the task never approved. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(stateSizeDuringRevert).toBe(1) + // The teardown still finished. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + expect(handled).toBe(false) + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { - const task = buildTask("finalize-fails", "inst-5") + const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask t.finalizePartialToolAsk = vi.fn().mockRejectedValue(new Error("finalize failed")) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) From 58c1a2b2062b025c69283522c58dcd08eaef5c98 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:02:41 +0800 Subject: [PATCH 10/26] test(tools): cover rollback failure with a retained stream error, and keep a failing report non-fatal Review follow-up on the open thread: the new teardown test only exercised a failed rollback with no streamError. Two cases added: - rollback fails while a streaming filesystem error is retained: both reports happen - the hazard say and handleError("writing file", streamError) - and the handler returns true. - the hazard report itself rejects: the teardown must not abort, so the streaming error still reaches the user. The second case exposed a real gap: the report was awaited inline, so a rejecting task.say would have thrown out of onParameterParseFailure and swallowed the user's only actionable error. The report now goes through reportRevertFailure(), which logs a failing say and continues - matching finalizePartialToolAskAfterFailure. Verified by stashing the source change: 1 failed / 7 passed without it, 8 passed with it. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 24 +++++++-- ...teToFileTool-partial-state-cleanup.spec.ts | 49 +++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 69aed4720c..c31d33cf53 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -165,6 +165,25 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + /** + * Surface a failed rollback to the user. The hazard has to be visible in the chat, + * not only in the console: the editor may still hold content the task never + * approved and a save of it would land an unauthorized write. A failing say must + * not abort the teardown - a retained streaming error still has to reach the user + * through handleError - so its failure is logged only, matching the other cleanup + * helpers in this file. + */ + private async reportRevertFailure(task: Task): Promise { + await task + .say( + "error", + "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", + ) + .catch((sayError) => { + console.error("Error reporting write_to_file rollback failure:", sayError) + }) + } + /** * Teardown boundary for the handle() parse-failure path, where execute() never * runs and therefore its finally (resetTaskPartialState) never runs either. @@ -200,10 +219,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Do not report a completed teardown: the editor may still show content this // task never approved, and saving it would land a write the user never // authorized. The user has to be able to tell that from the UI. - await task.say( - "error", - "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", - ) + await this.reportRevertFailure(task) } if (!state.streamError) { return false diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 93ba46560c..6718bb7c30 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -122,6 +122,55 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { errorSpy.mockRestore() }) + it("reports the rollback hazard AND the retained streaming error, and returns true", async () => { + const task = buildTask("revert-fails-with-stream-error", "inst-7") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // Both reports happen: the rollback hazard and the error the user can act on. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("still reports the streaming error when the rollback warning itself fails", async () => { + const task = buildTask("rollback-warning-fails", "inst-8") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + t.say = vi.fn().mockRejectedValue(new Error("say failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters[1], + new Error("parameter parse failed"), + ) + + // A failing report must not abort the teardown: the streaming error still lands. + expect(errorSpy).toHaveBeenCalledWith("Error reporting write_to_file rollback failure:", expect.any(Error)) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask From af51675c768e5d220ab59b738db501af6e477463 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:13:44 +0800 Subject: [PATCH 11/26] fix(tools): report the rollback hazard from the failed-partial-stream cleanup too The handlePartial() failure branch called revertDiffChangesBeforeReset() and ignored the result, so a failed restore left unapproved content in the editor with no user-visible signal: the stream error that triggered the cleanup is a different failure and is reported by the authoritative non-partial path in execute(), which is why this branch had stayed log-only. The revert + reset + report sequence is now one helper, cleanupFailedPartialStream(), used by the streaming cleanup; it checks the rollback result and reports the hazard exactly as onParameterParseFailure() does. Test: cleanupFailedPartialStream is driven directly with a rejecting revertChanges double - the hazard say and the provider reset are both asserted. Stashing the source change makes it fail (1 failed / 8 passed); with it 9 pass. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 23 ++++++++++++++++--- ...teToFileTool-partial-state-cleanup.spec.ts | 14 +++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index c31d33cf53..2fd409fe9a 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -184,6 +184,21 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { }) } + /** + * Cleanup for a failed partial stream: restore the diff document, close the view, + * and - when the restore itself failed - tell the user the editor may still hold + * unapproved content, the same way the parse-failure teardown does. Without the + * report, a failed rollback here is invisible: the stream error that triggered the + * cleanup is a different failure and is reported elsewhere. + */ + private async cleanupFailedPartialStream(task: Task): Promise { + const reverted = await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + if (!reverted) { + await this.reportRevertFailure(task) + } + } + /** * Teardown boundary for the handle() parse-failure path, where execute() never * runs and therefore its finally (resetTaskPartialState) never runs either. @@ -506,9 +521,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) await this.finalizePartialToolAskAfterFailure(task, partialMessage) // The write was never approved: restore the document so a user save cannot - // persist the failed streamed content (reset() alone leaves it dirty). - await this.revertDiffChangesBeforeReset(task) - await this.resetDiffViewAfterWrite(task) + // persist the failed streamed content (reset() alone leaves it dirty), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 6718bb7c30..0ff9d72826 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -171,6 +171,20 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { errorSpy.mockRestore() }) + it("surfaces the rollback hazard from the failed-stream cleanup as well", async () => { + const task = buildTask("failed-stream-cleanup", "inst-9") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["cleanupFailedPartialStream"](task) + + // Same contract as the parse-failure teardown: a failed restore is reported. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + errorSpy.mockRestore() + }) + it("logs and continues when finalizing the open partial ask fails", async () => { const task = buildTask("finalize-fails", "inst-6") const t = task as unknown as CleanupTask From 1311c90358bdceb2895edfade6d66bd4f5385560 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:25:38 +0800 Subject: [PATCH 12/26] fix(tools): tear down the per-task stream state when a write is rejected Both rejection exits in execute() returned without resetTaskPartialState(): the saveDirectly branch returned straight away and the diff-view branch returned after revertChanges(). The per-task entry - and with it a streamFailed flag armed by an earlier failed delta - therefore stayed set for the whole task, which suppresses the diff preview of every later write_to_file in that task, and the TaskAborted listener leaked for the task's life. Both exits now call super.resetPartialState() + resetTaskPartialState(task), matching the success and catch paths. Test: seed the executing task's stream state (streamFailed + streamError), reject the approval, and assert the task's entry is gone afterwards. Stashing the source change: 1 failed / 40 passed; with it 41 passed / 5 skipped. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 10 ++++++++++ src/core/tools/__tests__/writeToFileTool.spec.ts | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 2fd409fe9a..aa3d96dd21 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -359,6 +359,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // Rejection is an exit from execute() too. Without the teardown, a + // streamFailed flag armed by an earlier failed delta stays set for the whole + // task, which suppresses the diff preview of every later write_to_file, and + // the TaskAborted listener leaks. + super.resetPartialState() + this.resetTaskPartialState(task) return } @@ -393,6 +399,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!didApprove) { await task.diffViewProvider.revertChanges() + // Same exit contract as the saveDirectly branch above: the per-task stream + // state and its abort listener belong to this execute() call. + super.resetPartialState() + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 34b3dfe8a6..abcf7f67d0 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -784,6 +784,20 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() }) + it("clears this task's stream state when the write is rejected", async () => { + // A failed delta earlier in the same task arms the streamFailed guard. If the + // rejection exit returns without the teardown, every later write_to_file in this + // task loses its diff preview and the TaskAborted listener leaks. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + state.streamError = new Error("stream failure before the rejected write") + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}) + + expect(writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`)).toBeUndefined() + }) + it("reports user edits with diff feedback", async () => { const userEditsValue = "- old line\n+ new line" mockCline.diffViewProvider.saveChanges.mockResolvedValue({ From 5d713e512255b9b910d2329f5f272960ac54333a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:49:41 +0000 Subject: [PATCH 13/26] chore: trigger a fresh review pass at this head From 41ae4568734a8e962c4aa2d9414aa28d8f0b667e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 19:43:33 +0800 Subject: [PATCH 14/26] fix(tools): release the stream state on execute()'s early returns too execute() releases the per-task stream state in its success path, its catch and the approval rejection, but the three early returns (missing path, missing content, rooignore denial) left before any of that. The per-task entry and its TaskAborted listener then survived for the task's lifetime, and a streamFailed flag armed by an earlier failed delta kept suppressing the diff preview of every later write_to_file in that task. Each early return now calls resetTaskPartialState(task) - per-task only, so another task still streaming through this singleton is untouched. Tests: describe('early-return stream state cleanup') with the rooignore-denial case (state map empty, off(TaskAborted, ...) called) and the missing-path case driven through execute() with an empty path. Pin: removing the three releases fails both. Local: core/tools + writeToFileTool.spec + assistant-message + core/task = 1428 passed / 5 skipped; tsc --noEmit 0; eslint 0 err / 0 warn on both files. --- src/core/tools/WriteToFileTool.ts | 15 +++++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 27 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index aa3d96dd21..186d040ecb 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -264,6 +264,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -272,6 +277,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -281,6 +291,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index abcf7f67d0..8eaa9c47cc 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -774,6 +774,33 @@ describe("writeToFileTool", () => { }) }) + describe("early-return stream state cleanup", () => { + it("releases the per-task stream state when a rooignore denial returns early", async () => { + // The denial returns before the try/catch teardown: without the release the abort + // listener stays for the task's lifetime and a retained streamFailed suppresses the + // diff preview of every later write_to_file in this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) + + it("releases the per-task stream state when a missing parameter returns early", async () => { + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await writeToFileTool.execute({ path: "", content: "mock content" }, mockCline, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + }) + describe("user interaction", () => { it("reverts changes when user rejects approval", async () => { mockAskApproval.mockResolvedValue(false) From 7c11c26aeadf1f86a73aaa83c9862f3cb2f2380b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 02:31:48 +0800 Subject: [PATCH 15/26] fix(write-to-file): stop a cancelled partial stream at every provider await in handlePartial() Consistency port of the same fix already carried by #1929 (2f356e6f7) and #1928 (876a93b22). handlePartial() awaited provider.getState(), fileExistsAtPath(), task.ask() and diffViewProvider.open() with no cancellation check. A TaskAborted (or a direct clearTaskState) during any of those awaits runs the teardown that deletes this task's per-task stream state, and the delta already in flight then went on to re-ask, re-open a diff view, or stream a partial delta into a view the teardown had already released - resurrecting state for a task the user cancelled. Added isPartialStreamStillLive() (identity, not presence: a re-created entry for the same key belongs to a new stream) and a check after each of the four awaits. Tests: four cancellation cases in the existing "early-return stream state cleanup" describe, one per await, each asserting the delta stopped before the next side effect (no probe / no ask / no open / no update) and the state stayed released. Negative controls: each guard removed on its own -> exactly 1 failed (four separate runs); restored -> 4 passed. Verification: core/tools 656 passed / 5 skipped, tsc --noEmit 0 errors, eslint --prune-suppressions --max-warnings=0 clean on both touched files with src/eslint-suppressions.json unchanged. --- src/core/tools/WriteToFileTool.ts | 35 ++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 86 +++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 186d040ecb..0267d6e65c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -131,6 +131,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { this.taskPartialStreamState.delete(key) } + /** + * Whether this task's partial stream is still the live one. handlePartial() awaits + * provider state, a filesystem probe, task.ask() and diffViewProvider.open() before it + * touches the diff view; a cancellation during any of those awaits runs the TaskAborted + * teardown (or a direct clearTaskState), which deletes this entry. Continuing would + * re-ask, re-open a diff view, or stream a partial delta into a view the teardown has + * already released for a task the user cancelled. Identity, not presence: a re-created + * entry for the same key belongs to a new stream, and this one must not write into it. + */ + private isPartialStreamStillLive(task: Task, state: TaskPartialStreamState): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) === state + } + private async resetDiffViewAfterWrite(task: Task): Promise { await task.diffViewProvider.reset().catch((resetError) => { console.error("Error resetting write_to_file diff view:", resetError) @@ -484,6 +497,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const provider = task.providerRef.deref() const state = await provider?.getState() + + // Cancelled while provider state was in flight: the teardown already + // released this task's stream state. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -501,6 +521,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { fileExists = task.diffViewProvider.editType === "modify" } else { fileExists = await fileExistsAtPath(absolutePath) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } task.diffViewProvider.editType = fileExists ? "modify" : "create" } @@ -518,12 +541,24 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const partialMessage = JSON.stringify(sharedMessageProps) await task.ask("tool", partialMessage, block.partial).catch(() => {}) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { await task.diffViewProvider.open(relPath!) } + // Cancellation may land while open() is in flight: its abort handler has + // already torn the stream down (and may have reverted or closed this very + // diff view), so streaming the partial content into it now would resurrect a + // view for a task that no longer exists. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + await task.diffViewProvider.update( everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, false, diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 8eaa9c47cc..77a267f57e 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -799,6 +799,92 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + + it("stops before the filesystem probe when the state is released while provider state is in flight", async () => { + // handlePartial() awaits provider.getState() before any side effect. A cancellation + // during that await runs the TaskAborted teardown; the delta already in flight must + // stop there instead of probing, asking and opening a diff view for a dead task. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn(async () => { + writeToFileTool.clearTaskState(mockCline) + return {} + }), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before the partial ask when the state is released during the filesystem probe", async () => { + // Same teardown, one await later. The first delta pins editType, so clear it to + // take the fileExistsAtPath branch again and abort inside it. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.editType = undefined + // mockImplementationOnce: executeWriteFileTool re-arms the default resolved value + // on every call, so a plain mockImplementation would be overwritten. + mockedFileExistsAtPath.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + return false + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before touching the diff view when the stream state is released during an in-flight ask", async () => { + // A cancellation while task.ask() is in flight runs the TaskAborted teardown. The + // delta that was already in flight must not then re-open the diff view for a task + // the user cancelled - that resurrects the state the teardown just released. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.ask.mockImplementation(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before updating the diff view when the task is cancelled while open() is in flight", async () => { + // open() is the first provider await after the partial ask. If TaskAborted lands + // while it is in flight, the teardown has already released this task's stream + // state (and may have reverted or closed this very view), so the delta that is + // already in flight must not stream partial content into a cancelled task's view. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { From 6626914230e6246b532298d587634bd4f9d21ef6 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 03:39:21 +0800 Subject: [PATCH 16/26] fix(write-to-file): let the cancellation teardown own a rejected open()/update() Consistency port of the same fix carried by #1928 (3a0065091), using the isPartialStreamStillLive() helper this unit already has. handlePartial()'s catch treated a rejection from open()/update() as an ordinary filesystem failure. A cancellation that lands while either is in flight runs the TaskAborted teardown first - which releases this task's per-task stream state, reverts or closes this very diff view, and reports the failure itself - and it can also reject the call in flight. The catch then marked the already-released state failed, finalized the partial ask, and ran the failed-stream cleanup a second time: a fresh ask row and a second rollback for a task the user had already cancelled. The catch now checks isPartialStreamStillLive() first, logs the failure, and returns so the teardown owns the outcome. Test: `does not finalize the ask or roll back twice when open() rejects after a cancellation` - open() releases the state and rejects; asserts no update, no finalizePartialToolAsk, no revertChanges, and the state stays released. Negative control: guard removed -> exactly 1 failed; restored -> 1 passed. Verification: core/tools 657 passed / 5 skipped, tsc --noEmit 0 errors, eslint --prune-suppressions --max-warnings=0 clean on both files with src/eslint-suppressions.json unchanged. --- src/core/tools/WriteToFileTool.ts | 12 +++++++++ .../tools/__tests__/writeToFileTool.spec.ts | 26 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0267d6e65c..6ba02de198 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -564,6 +564,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { false, ) } catch (error) { + // A cancellation that lands while open() or update() is in flight runs the + // TaskAborted teardown - which releases this task's stream state, reverts or + // closes this very diff view, and reports the failure itself - and it can also + // reject the call in flight. Marking the already-released state failed, finalizing + // the ask, or running the failed-stream cleanup a second time would resurrect UI + // and roll back twice for a task the user cancelled, so the teardown owns the + // outcome here. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + console.error(`Error streaming write_to_file diff view:`, error) + return + } + // Opening or updating the diff view can throw on filesystem errors // (EACCES/EROFS on read-only paths). Finalize the partial tool message // so the UI spinner doesn't get stuck and reset the diff view. Do NOT diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 77a267f57e..20fa1497d3 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -885,6 +885,32 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + it("does not finalize the ask or roll back twice when open() rejects after a cancellation", async () => { + // A cancellation during open() releases the stream state through the TaskAborted + // teardown (which also reverts or closes this diff view) AND can reject the call in + // flight. The catch used to mark the released state failed, finalize the ask and run + // the failed-stream cleanup again - a second rollback and a fresh ask row for a task + // the user already cancelled. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + throw new Error("EACCES: permission denied, open mock-file") + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // The teardown that already ran owns the outcome: no finalize and no second rollback. + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { From bcd801ea4311839b01f943a9b01473cc97ddedbd Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 13:23:01 +0800 Subject: [PATCH 17/26] fix(write-to-file): release the partial stream state when the preview is suppressed handlePartial() registers this task's partial-stream entry (and its TaskAborted listener) before it checks the prevent-focus-disruption experiment. With the experiment enabled the delta then returns without ever showing a preview, and nothing else releases that entry: the listener stays attached for the rest of the task's life, and a streamFailed mark armed by an earlier delta keeps suppressing this task's later diff previews. Release the bookkeeping on that return. The execute() exits (validation returns, both approval denials, success and the catch) already tear down explicitly on this branch, so this is the only exit left that skips it. Same root cause as the Lifecycle Resource Cleanup row on the sibling units of #1066; the sibling branches carry the same release in their own PRs. --- src/core/tools/WriteToFileTool.ts | 5 ++++ .../tools/__tests__/writeToFileTool.spec.ts | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 6ba02de198..ae1a85e68c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -510,6 +510,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ) if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) return } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 20fa1497d3..2236aca644 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -911,6 +911,30 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + + it("releases the per-task stream state when prevent-focus-disruption skips the partial preview", async () => { + // Delta 1 only pins the path, so the entry is still live afterwards (the stream is in + // flight). Delta 2 reaches the experiment check and returns without ever showing a + // preview: nothing else would release the entry or detach the TaskAborted listener. + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) }) describe("user interaction", () => { From 41ccb425bc5752d66554bbde4aacfa5d2727e0bf Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:08:41 +0800 Subject: [PATCH 18/26] fix(write-to-file): guard every setup step and roll a failed diff-open back Addresses the three #1066/U3 review rows at c1a9f1dcb: - Lifecycle (warning): execute()'s guarded scope now starts before the preflight filesystem work (fileExistsAtPath, createDirectoriesForFile) and ends with an unconditional finally { this.resetTaskPartialState(task) }. A throw from that setup used to escape into BaseTool.handle() with this task's stream entry and TaskAborted listener attached. The same shape was accepted on #1930 (7b783452c). - Persistence Integrity (error): a failed diff-open is now transactional. open() creates the parent directories and an empty placeholder BEFORE it awaits openDiffEditor(), so the catch awaits revertDiffChangesBeforeReset() before resetting: with no active editor the rollback still unlinks the placeholder and removes only the directories this operation created (DiffViewProvider ported verbatim from #1930's 7b783452c / 6cae369d9 - ENOENT tolerated, other failures surfaced, an unapproved dirty buffer discarded rather than saved, a refused discard restoring the pre-stream content and reporting the rollback as failed). - Regression Evidence (warning): focused execute() tests seeding the task's stream state first - missing content, successful completion, a failing write, the preflight directory throw, and the open()-failure rollback - each asserting the entry is removed and the exact TaskAborted listener deregistered. Red first: the preflight-throw test failed before the try/finally move. Negative controls: neutering the finally -> exactly the five exits it owns go red (completion, failure, preflight throw, open-debris, denial); moving the try back below the preflight work -> exactly the preflight test red; removing the revert call in the catch -> exactly the open-debris test red. Verification: writeToFileTool 54/5s, partial-state-cleanup 5, DiffViewProvider 79, Task.spec 172, removeClineFromStack 24 (338 passed); tsc 62 (baseline, none in touched files); eslint 0/0; eslint-suppressions unchanged. --- src/core/tools/WriteToFileTool.ts | 83 ++++--- .../tools/__tests__/writeToFileTool.spec.ts | 105 +++++++++ src/integrations/editor/DiffViewProvider.ts | 131 ++++++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 214 +++++++++++++++++- 4 files changed, 485 insertions(+), 48 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 1adb0312a0..8db69e1912 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -313,46 +313,49 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath) - - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + try { + // The guarded scope starts before the preflight filesystem work: a throw from + // fileExistsAtPath or createDirectoriesForFile must report and tear down like any other + // write failure, not escape into BaseTool.handle() with this task's stream state attached. + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath) + + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + // Create parent directories early for new files to prevent ENOENT errors + // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } - if (newContent.startsWith("```")) { - newContent = newContent.split("\n").slice(1).join("\n") - } + if (newContent.startsWith("```")) { + newContent = newContent.split("\n").slice(1).join("\n") + } - if (newContent.endsWith("```")) { - newContent = newContent.split("\n").slice(0, -1).join("\n") - } + if (newContent.endsWith("```")) { + newContent = newContent.split("\n").slice(0, -1).join("\n") + } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } + if (!task.api.getModel().id.includes("claude")) { + newContent = unescapeHtmlEntities(newContent) + } - const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" + const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath), - content: newContent, - isOutsideWorkspace, - isProtected: isWriteProtected, - } + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath), + content: newContent, + isOutsideWorkspace, + isProtected: isWriteProtected, + } - try { task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -391,7 +394,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // task, which suppresses the diff preview of every later write_to_file, and // the TaskAborted listener leaks. super.resetPartialState() - this.resetTaskPartialState(task) return } @@ -428,7 +430,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Same exit contract as the saveDirectly branch above: the per-task stream // state and its abort listener belong to this execute() call. super.resetPartialState() - this.resetTaskPartialState(task) return } @@ -451,7 +452,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // one task's execute() would drop another task's streamFailed/streamError // while it is still streaming, so tear down only this task's entry. super.resetPartialState() - this.resetTaskPartialState(task) task.processQueuedMessages() @@ -464,10 +464,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) } await handleError("writing file", error as Error) + + // A failed diff-open is not the end of the story: open() creates the parent directories + // and an empty placeholder BEFORE it awaits openDiffEditor(), so a rejection leaves that + // debris behind. Revert first - with no active editor the rollback still unlinks the + // placeholder and removes only the directories this operation created - then reset. + await this.revertDiffChangesBeforeReset(task) await task.diffViewProvider.reset() super.resetPartialState() - this.resetTaskPartialState(task) return + } finally { + // Unconditional: every exit from the guarded scope - success, denial, or a + // throw - releases this task's stream entry and its TaskAborted listener. + this.resetTaskPartialState(task) } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 2236aca644..a1aabb634e 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -935,6 +935,111 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) }) + + it("releases the per-task stream state when content is missing", async () => { + // The missing-content return sits before execute()'s guarded scope, so it needs its own + // release. The state is seeded first so the assertion proves a release happened rather + // than an empty map. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the + // typed params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + expect(pushToolResult).toHaveBeenCalledWith("Missing param error") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // A stream may have opened a diff view for this call; the early return still closes it. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when the write completes", async () => { + // Seed first: without the seed the map is empty either way and the assertion is vacuous. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when the write itself fails", async () => { + // The catch path tears down too: a failed write must not leave the entry (and its + // streamFailed guard) attached to the task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the preflight directory creation throws", async () => { + // The preflight filesystem work sits before execute()'s guarded scope today: when it + // throws, nothing reports the failure and the stream state leaks. It has to be handled + // like any other write failure - reported once, teardown run. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockedCreateDirectoriesForFile.mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied, mkdir '/new-parent'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("rolls the diff-view debris back when open() fails mid-execute", async () => { + // open() creates the parent directories and an empty placeholder before it awaits + // openDiffEditor(). When it rejects, the write failed but the debris did not: the catch + // has to await the rollback (which removes the placeholder and only the directories this + // operation created) before resetting the view, or the next execute() mistakes the + // placeholder for an existing file. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + const revertOrder = mockCline.diffViewProvider.revertChanges.mock.invocationCallOrder[0] + const resetOrder = mockCline.diffViewProvider.reset.mock.invocationCallOrder.at(-1) + expect(resetOrder).toBeGreaterThan(revertOrder) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..ed7f8a3d15 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -21,6 +21,11 @@ import { Task } from "../../core/task/Task" import { DecorationController } from "./DecorationController" +/** Narrow ENOENT test so a rollback can tolerate a file or dir that never landed. */ +function isEnoent(error: unknown): boolean { + return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT" +} + export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes" @@ -513,13 +518,38 @@ export class DiffViewProvider { return JSON.stringify(result) } + /** + * Remove a file this edit created. Tolerates ENOENT: when open() failed before the + * placeholder was written (or the write itself failed) there is nothing to delete, and + * that must not abort the rollback. + */ + private async removeCreatedFile(absolutePath: string): Promise { + try { + await fs.unlink(absolutePath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + + /** Same tolerance for a directory this edit created that never made it to disk. */ + private async removeCreatedDir(dirPath: string): Promise { + try { + await fs.rmdir(dirPath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + async revertChanges(): Promise { - if (!this.relPath || !this.activeDiffEditor) { + if (!this.relPath) { return } const fileExists = this.editType === "modify" - const updatedDocument = this.activeDiffEditor.document const absolutePath = path.resolve(this.cwd, this.relPath) // Stop tracking touches and cancel any pending scroll-to-diff before any @@ -528,21 +558,43 @@ export class DiffViewProvider { this.cancelDeferredScroll() if (!fileExists) { - if (updatedDocument.isDirty) { - await updatedDocument.save() + // open() creates the parent directories and an empty placeholder file BEFORE it + // awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and + // the previous early return here left the placeholder and the new directories on + // disk: the next execute() then saw an empty file and treated the requested new file + // as an existing one, so a denial preserved the debris. The filesystem rollback runs + // either way; only the document work needs an editor. + if (this.activeDiffEditor) { + const updatedDocument = this.activeDiffEditor.document + if (updatedDocument.isDirty) { + // The buffer holds the streamed content of a write that was never approved. Saving + // it here persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. Discard the buffer + // (force-close the tab) instead; the file goes away a moment later anyway. + await this.discardFileTab(absolutePath) + await this.closeAllDiffViews() + } else { + await this.closeAllDiffViews() + // The file was newly created for this edit; close its transiently + // opened tab before deleting it from disk. + await this.closeFileTab(absolutePath) + } } - await this.closeAllDiffViews() - // The file was newly created for this edit; close its transiently - // opened tab before deleting it from disk. - await this.closeFileTab(absolutePath) - await fs.unlink(absolutePath) + await this.removeCreatedFile(absolutePath) // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { - await fs.rmdir(this.createdDirs[i]) + await this.removeCreatedDir(this.createdDirs[i]) } + } else { + // Only reachable after a successful open(), so the editor exists. + const updatedDocument = this.activeDiffEditor?.document + if (!updatedDocument) { + return + } + // Revert document. const edit = new vscode.WorkspaceEdit() @@ -847,6 +899,65 @@ export class DiffViewProvider { // Close the plain (non-diff) editor tab for the target file. Used when the // file was opened transiently for the diff and the user never interacted // with it, so it should not linger after accept/deny. + /** + * Close the tab for this path WITHOUT saving. Used by the new-file rollback, where the + * buffer holds unapproved streamed content that must never reach disk; closeFileTab() + * deliberately skips dirty tabs, so a dirty buffer needs the forced close. + * + * A close that fails or is refused is a rollback failure: the buffer would still hold the + * content the user never approved, one save away from disk. Put the pre-stream content back + * so nothing saveable survives, and propagate - the caller must not keep deleting around a + * buffer it could not discard. + */ + private async discardFileTab(absolutePath: string): Promise { + const tabs = vscode.window.tabGroups.all + .flatMap((group) => group.tabs) + .filter( + (tab) => + tab.input instanceof vscode.TabInputText && + tab.input.uri.scheme === "file" && + arePathsEqual(tab.input.uri.fsPath, absolutePath), + ) + + for (const tab of tabs) { + let closed: boolean + let closeError: Error | undefined + try { + closed = await vscode.window.tabGroups.close(tab, true) + } catch (error) { + closed = false + closeError = error instanceof Error ? error : new Error(String(error)) + } + if (!closed) { + await this.restorePreStreamBuffer(absolutePath) + throw new Error( + `Rollback could not discard the buffer for ${absolutePath}; its unapproved content was restored to the pre-stream state instead.`, + { cause: closeError }, + ) + } + } + } + + /** + * Replace an open buffer's content with what it held before streaming started, so a buffer + * that could not be closed never keeps unapproved content available to save. + */ + private async restorePreStreamBuffer(absolutePath: string): Promise { + const document = vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!document) { + return + } + const edit = new vscode.WorkspaceEdit() + const range = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ) + edit.replace(document.uri, range, this.originalContent ?? "") + await vscode.workspace.applyEdit(edit) + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..89963acfad 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,3 +1,4 @@ +import * as fs from "fs/promises" import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" import * as vscode from "vscode" import * as path from "path" @@ -16,6 +17,10 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + // revertChanges() rolls a new-file edit back by deleting the placeholder and the + // directories the edit created. + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -52,7 +57,7 @@ vi.mock("vscode", () => ({ onDidChangeTextEditorVisibleRanges: vi.fn(() => ({ dispose: vi.fn() })), tabGroups: { all: [], - close: vi.fn(), + close: vi.fn().mockResolvedValue(true), activeTabGroup: { activeTab: undefined }, }, visibleTextEditors: [], @@ -1172,6 +1177,213 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() removes the placeholder and created dirs when open() failed before the editor existed", async () => { + // open() creates the parent dirs and an empty placeholder BEFORE it awaits + // openDiffEditor(). If that await rejects there is no activeDiffEditor, and the + // rollback used to bail out - leaving an empty file that the next execute() mistook + // for an existing file, which a denial then preserved. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() tolerates a placeholder that was never written", async () => { + // The failed open may have died before fs.writeFile ran; ENOENT during the + // rollback is success, not a new failure that would abort the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + // The rollback still attempted the delete - the tolerance is about not + // aborting the rest of the cleanup, not about skipping it. + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + }) + + it("revertChanges() stops before the document work for an existing file when no editor exists", async () => { + // The modify branch used to dereference activeDiffEditor unconditionally. When open() + // never produced an editor there is no document to restore, and the old code threw a + // TypeError out of the denial path instead of finishing the teardown. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "modify", + createdDirs: [], + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("revertChanges() keeps rolling back the remaining directories when one rmdir hits ENOENT", async () => { + // Same tolerance as the placeholder, for the created dirs: a directory that never + // landed must not abort the rest of the rollback and must not surface as a new failure. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(2) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() surfaces a non-ENOENT directory failure instead of swallowing it", async () => { + // The tolerance is scoped to ENOENT: a directory that exists but could not be removed + // is a real rollback failure and must reach the caller rather than be hidden. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") + }) + + it("revertChanges() surfaces a placeholder delete that failed for a non-ENOENT reason", async () => { + // The ENOENT tolerance is scoped: an unlink that fails for another reason means the + // unapproved placeholder is still on disk, and the caller must not be told the + // rollback succeeded. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EACCES: permission denied") + }) + + it("revertChanges() discards the streamed buffer instead of saving unapproved content for a new file", async () => { + // The diff buffer holds the streamed content of a write that was never approved. + // Saving it here persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + expect(editor.document.save).not.toHaveBeenCalled() + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab, true) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() reports a discard the editor refused instead of leaving the buffer open", async () => { + // A forced close can still fail (a vetoing editor). Swallowing that left the unapproved + // streamed content in an open, saveable buffer while the rollback kept deleting the + // file underneath it. The failure must propagate, and the buffer must be put back to + // its pre-stream state so nothing saveable survives. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const streamedBuffer = { + uri: makeUri(mockTargetPath), + getText: vi.fn().mockReturnValue("streamed content"), + isDirty: true, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + } + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + value: [streamedBuffer], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + vi.mocked(vscode.window.tabGroups.close).mockRejectedValueOnce(new Error("close vetoed")) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not discard the buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocuments) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + } + } + + // The buffer went back to its pre-stream state, and the rollback stopped rather than + // deleting the file out from under a buffer it could not discard. + expect(vscode.workspace.applyEdit).toHaveBeenCalled() + // The buffer was put back to its pre-stream state (empty for a new file): the edit that + // was applied replaces the streamed text with originalContent. + const appliedEdit = vi.mocked(vscode.WorkspaceEdit).mock.results[0].value + expect(appliedEdit.replace).toHaveBeenCalledWith( + expect.objectContaining({ fsPath: mockTargetPath }), + expect.anything(), + "", + ) + }) + it("revertChanges() closes the file tab when the file was not open and untouched", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) From 5c0f212192d6a8976b94eb188d62f7d87fd6f10e Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 02:57:14 +0800 Subject: [PATCH 19/26] fix(editor): gate rollback on an in-progress edit and restore buffers before closing CodeRabbit #1931 follow-up. The DiffViewProvider half is ported verbatim from p1066/u5-streaming-failure-capture @ eb245494a so the shared rollback keeps one shape across the stack: - revertChanges() now requires both relPath and isEditing. relPath survives a completed edit, so a later teardown took the new-file branch for the PREVIOUS edit's target and unlinked an existing user file. reset() clears relPath and newContent so no later teardown can reuse a stale target. - 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. - execute()'s cleanup reports the rollback hazard when its own revert failed, the way onParameterParseFailure() and cleanupFailedPartialStream() already do. - Tests: stale-relPath guard, reset() clearing, restore/save ordering before close, refused close, exact TaskAborted deregistration on both early returns, the execute()-cleanup hazard report, and a delta whose stream state is cleared while update() is in flight leaving the teardown in charge. --- src/core/tools/WriteToFileTool.ts | 11 +- .../tools/__tests__/writeToFileTool.spec.ts | 57 +++++++- src/integrations/editor/DiffViewProvider.ts | 41 +++++- .../editor/__tests__/DiffViewProvider.spec.ts | 138 +++++++++++++----- 4 files changed, 199 insertions(+), 48 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 8db69e1912..72239201ef 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -469,9 +469,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // and an empty placeholder BEFORE it awaits openDiffEditor(), so a rejection leaves that // debris behind. Revert first - with no active editor the rollback still unlinks the // placeholder and removes only the directories this operation created - then reset. - await this.revertDiffChangesBeforeReset(task) + // A rollback that did not finish is a second, actionable failure: the placeholder and the + // created directories are still on disk, and the next execute() would treat that debris as + // an existing file. Report it the same way the parse-failure teardown and + // cleanupFailedPartialStream() do - after the reset, so the report is the last thing the + // failed write leaves behind. + const reverted = await this.revertDiffChangesBeforeReset(task) await task.diffViewProvider.reset() super.resetPartialState() + + if (!reverted) { + await this.reportRevertFailure(task) + } return } finally { // Unconditional: every exit from the guarded scope - success, denial, or a diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index a1aabb634e..4d89e0a535 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -661,6 +661,30 @@ describe("writeToFileTool", () => { consoleErrorSpy.mockRestore() } }) + + it("leaves the teardown in charge when the stream state is cleared while update() is in flight", async () => { + // The path is stabilized, so this delta streams. The task is disposed while update() + // awaits: the TaskAborted handler releases this task's state (and reverts or closes the + // diff view itself), and the in-flight update then rejects. Marking the released state + // failed, finalizing the ask, or running the failed-stream cleanup again would resurrect + // UI and roll back twice for a task the user cancelled. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.diffViewProvider.update.mockImplementationOnce(async () => { + writeToFileTool["resetTaskPartialState"](mockCline as never) + throw new Error("update rejected after the task was disposed") + }) + + await executeWriteFileTool({}, { isPartial: true }) + + // The cleanup did not run: no revert, no finalize, and the released state stayed gone. + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockHandleError).not.toHaveBeenCalled() + }) }) describe("path stabilization predicate", () => { @@ -784,7 +808,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) }) it("releases the per-task stream state when a missing parameter returns early", async () => { @@ -933,7 +963,13 @@ describe("writeToFileTool", () => { expect(mockCline.ask).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 content is missing", async () => { @@ -1040,6 +1076,23 @@ describe("writeToFileTool", () => { expect(resetOrder).toBeGreaterThan(revertOrder) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + + it("reports the rollback hazard when execute()'s own cleanup cannot revert the diff", async () => { + // execute() failed after the diff view was opened, and the rollback that follows the + // catch failed too. The debris (placeholder, created directories) is still on disk and the + // editor may still hold unapproved content, so the user has to be told - the same report + // the parse-failure teardown and cleanupFailedPartialStream() make. + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("rollback failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("could not be restored after the failed tool call"), + ) + }) }) describe("user interaction", () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index ed7f8a3d15..72b2345f3f 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,8 @@ export class DiffViewProvider { this.cancelDeferredScroll() await this.closeAllDiffViews() + this.relPath = undefined + this.newContent = 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..eede24ef13 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -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", @@ -1201,6 +1205,7 @@ describe("DiffViewProvider", () => { // 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 +1225,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 +1244,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 +1263,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 +1279,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 +1290,50 @@ 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() 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() 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,47 @@ 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(), - "", - ) }) it("revertChanges() closes the file tab when the file was not open and untouched", async () => { @@ -1390,6 +1444,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 +1466,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 @@ -1467,7 +1525,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), @@ -1682,6 +1740,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 // The user clicked inside the diff pane -- but this is a deny, so it must be ignored. @@ -1873,7 +1933,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 d96c41762a3f0917989a66662200a11a513161e9 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 04:48:28 +0800 Subject: [PATCH 20/26] fix(tools): hand created directories to the diff view and release stream state on any partial failure CodeRabbit #1931 at-head follow-up (change_assessment_commit 5c0f21219). Persistence Integrity (Error) - "Track and pass the directories created during WriteToFileTool.execute() into the active DiffViewProvider transaction, or remove the duplicate preflight directory creation for the diff-view path." Chosen: track and pass, not remove. execute()'s preflight mkdir is the guarded entry point an earlier commit of this PR deliberately moved inside the try scope (a throw there used to escape into BaseTool.handle() with the stream state attached), and the streaming path depends on execute() being the only place that touches the filesystem. Deleting it would trade a rollback gap for that leak again. DiffViewProvider.adoptCreatedDirs() now folds the caller's directories into createdDirs, which is exactly what revertChanges() removes; reset() drops the adopted list so a later transaction cannot rmdir a previous one's directories. Lifecycle Resource Cleanup (Warning) - "Release the current task state when any uncaught partial-stream operation fails ... and remove the state by identity." handlePartial() registers the per-task TaskAborted listener when it creates the stream state, but only the open()/update() block had a catch. Anything else that threw below that point left the entry registered: a stale streamFailed flag then suppressed every later preview for the task and the listener could never fire for a stream that had already ended. The rest of handlePartial now runs inside a guarded scope that releases the entry and rethrows, and the release compares the map value by identity so a late failure from a superseded stream cannot detach a newer stream's listener. Tests: writeToFileTool.spec.ts covers the hand-off, the no-op for an existing file, the release on an uncaught partial failure, and the identity guard; DiffViewProvider.spec.ts covers folding adopted directories into createdDirs and reset() clearing them. Six one-to-one negative controls, one red test each. Local: vitest writeToFileTool + DiffViewProvider 143 passed / 5 skipped; tsc --noEmit 62 (branch baseline). --- src/core/tools/WriteToFileTool.ts | 224 ++++++++++-------- .../tools/__tests__/writeToFileTool.spec.ts | 63 +++++ src/integrations/editor/DiffViewProvider.ts | 16 +- .../editor/__tests__/DiffViewProvider.spec.ts | 39 +++ 4 files changed, 242 insertions(+), 100 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 72239201ef..4a9e965a83 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -120,6 +120,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { this.resetTaskPartialState(task) } + /** + * Release this task's stream entry only while the map still holds THIS object. + * A newer stream for the same task must not be dropped by an older failure's + * cleanup, which would also deregister the newer stream's abort listener. + */ + private releaseTaskPartialStateByIdentity(state: TaskPartialStreamState): void { + if (this.taskPartialStreamState.get(this.getPartialStreamFailureKey(state.task)) === state) { + this.resetTaskPartialState(state.task) + } + } + private resetTaskPartialState(task: Task): void { const key = this.getPartialStreamFailureKey(task) const state = this.taskPartialStreamState.get(key) @@ -330,7 +341,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // 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) + const createdDirs = await createDirectoriesForFile(absolutePath) + // The rollback lives in DiffViewProvider.revertChanges(), which removes only the + // directories recorded there - and its own mkdir returns nothing once these exist. + // Hand this call's directories over, or a failed new-file write leaves them on disk + // and the next execute() treats the leftover placeholder as an existing file. + task.diffViewProvider.adoptCreatedDirs(createdDirs) } if (newContent.startsWith("```")) { @@ -506,120 +522,130 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // once, so abandoned streams are torn down even if execute() never runs. const partialStreamState = this.getTaskPartialStreamState(task) - // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { - return - } - - const provider = task.providerRef.deref() - const state = await provider?.getState() - - // Cancelled while provider state was in flight: the teardown already - // released this task's stream state. - if (!this.isPartialStreamStillLive(task, partialStreamState)) { - return - } - - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) - - 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 - } + // Anything below that throws must not leave this task's stream entry and its + // TaskAborted listener registered: the stream is over, a stale streamFailed flag + // suppresses every later preview for the task, and the listener can never fire + // for a stream that already ended. Release by identity so a newer stream for the + // same task survives, then rethrow - the caller owns reporting. + try { + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { + return + } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + const provider = task.providerRef.deref() + const state = await provider?.getState() - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) + // Cancelled while provider state was in flight: the teardown already + // released this task's stream state. if (!this.isPartialStreamStillLive(task, partialStreamState)) { return } - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } - - 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, - } - - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - if (!this.isPartialStreamStillLive(task, partialStreamState)) { - return - } + 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 + } - if (newContent) { - try { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) - } + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) - // Cancellation may land while open() is in flight: its abort handler has - // already torn the stream down (and may have reverted or closed this very - // diff view), so streaming the partial content into it now would resurrect a - // view for a task that no longer exists. + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) if (!this.isPartialStreamStillLive(task, partialStreamState)) { return } + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) - } catch (error) { - // A cancellation that lands while open() or update() is in flight runs the - // TaskAborted teardown - which releases this task's stream state, reverts or - // closes this very diff view, and reports the failure itself - and it can also - // reject the call in flight. Marking the already-released state failed, finalizing - // the ask, or running the failed-stream cleanup a second time would resurrect UI - // and roll back twice for a task the user cancelled, so the teardown owns the - // outcome here. - if (!this.isPartialStreamStillLive(task, partialStreamState)) { + 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, + } + + const partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + if (newContent) { + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } + + // Cancellation may land while open() is in flight: its abort handler has + // already torn the stream down (and may have reverted or closed this very + // diff view), so streaming the partial content into it now would resurrect a + // view for a task that no longer exists. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // A cancellation that lands while open() or update() is in flight runs the + // TaskAborted teardown - which releases this task's stream state, reverts or + // closes this very diff view, and reports the failure itself - and it can also + // reject the call in flight. Marking the already-released state failed, finalizing + // the ask, or running the failed-stream cleanup a second time would resurrect UI + // and roll back twice for a task the user cancelled, so the teardown owns the + // outcome here. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + console.error(`Error streaming write_to_file diff view:`, error) + return + } + + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // 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) - return + // 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), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) } - - // 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), and - // surface the hazard if that restore itself failed. The stream error is - // reported by the authoritative non-partial path in execute(); the rollback - // hazard is a different failure and nothing else in this path says so. - await this.cleanupFailedPartialStream(task) } + } catch (error) { + this.releaseTaskPartialStateByIdentity(partialStreamState) + throw error } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 4d89e0a535..f5aeb47590 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -168,6 +168,7 @@ describe("writeToFileTool", () => { originalContent: "", open: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), + adoptCreatedDirs: vi.fn(), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ @@ -1389,5 +1390,67 @@ describe("writeToFileTool", () => { + }) + + describe("partial-stream failure teardown and directory ownership", () => { + it("hands the directories it created to the diff view so a rollback removes them", async () => { + // revertChanges() removes only the directories the provider recorded, and its own + // mkdir returns nothing once execute() has made them. Without the hand-off a + // rolled-back new-file write left the parent directories on disk. + const created = ["/new-parent", "/new-parent/nested"] + mockedCreateDirectoriesForFile.mockResolvedValue(created) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.adoptCreatedDirs).toHaveBeenCalledWith(created) + }) + + it("does not hand directories over when the file already exists", async () => { + // An existing file has no directories to roll back; adopting an empty list from + // an unrelated mkdir would still be wrong, but adopting a populated one would + // make a denial rmdir directories the user already had. + await executeWriteFileTool({}, { fileExists: true }) + + expect(mockCline.diffViewProvider.adoptCreatedDirs).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when a partial-stream await throws", async () => { + // An uncaught failure below the state creation used to leave the entry and its + // TaskAborted listener registered: the stale streamFailed flag then suppressed + // every later preview for the task, and the listener could never fire for a + // stream that had already ended. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockRejectedValue(new Error("state read failed")), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + 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(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("keeps a newer stream entry when an older partial failure releases by identity", async () => { + // Identity, not presence: dropping whatever sits under the key would detach the + // live stream's abort listener while cleaning up after a superseded one. + const stale = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + const newer = { ...stale } + mockCline.providerRef.deref.mockImplementation(() => ({ + getState: async () => { + writeToFileTool["taskPartialStreamState"].set("task-1.instance-1", newer) + throw new Error("state read failed") + }, + })) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].get("task-1.instance-1")).toBe(newer) + expect(mockCline.off).not.toHaveBeenCalled() + }) }) }) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 72b2345f3f..ef72cae072 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -38,6 +38,10 @@ export class DiffViewProvider { isEditing = false originalContent: string | undefined private createdDirs: string[] = [] + // Directories a caller (WriteToFileTool.execute) created before this transaction. + // open() folds them into createdDirs so revertChanges() removes them too: without + // them the caller's preflight mkdir left parent directories on disk after a rollback. + private adoptedCreatedDirs: string[] = [] private documentWasOpen = false // Tracks whether the target file's tab was pinned before the diff session. // Closing the tab to open the diff drops VS Code's pin state, so we restore @@ -95,6 +99,15 @@ export class DiffViewProvider { this.taskRef = new WeakRef(task) } + /** + * Record directories a caller created before this diff transaction so the rollback + * removes them too. revertChanges() only deletes what createdDirs holds, and a + * pre-created directory makes the provider's own mkdir return nothing. + */ + adoptCreatedDirs(createdDirs: string[]): void { + this.adoptedCreatedDirs.push(...createdDirs) + } + async open(relPath: string): Promise { this.relPath = relPath const fileExists = this.editType === "modify" @@ -132,7 +145,7 @@ export class DiffViewProvider { // For new files, create any necessary directories and keep track of new // directories to delete if the user denies the operation. - this.createdDirs = await createDirectoriesForFile(absolutePath) + this.createdDirs = [...this.adoptedCreatedDirs, ...(await createDirectoriesForFile(absolutePath))] // Make sure the file exists before we open it. if (!fileExists) { @@ -1253,6 +1266,7 @@ export class DiffViewProvider { this.isEditing = false this.originalContent = undefined this.createdDirs = [] + this.adoptedCreatedDirs = [] this.documentWasOpen = false this.documentWasPinned = false this.activeDiffEditor = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index eede24ef13..c6d591ac5e 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -359,6 +359,35 @@ describe("DiffViewProvider", () => { expect((diffViewProvider as any).documentWasPinned).toBe(true) expect((diffViewProvider as any).documentWasOpen).toBe(true) }) + it("folds directories the caller created into createdDirs so a rollback can remove them", async () => { + // WriteToFileTool.execute() creates the parent directories before the diff view + // exists, so the provider's own mkdir records nothing. Without the adoption + // createdDirs stayed empty and a rolled-back new-file write left them on disk. + const mockEditor = { + document: { + uri: { fsPath: `${mockCwd}/adopt.md`, scheme: "file" }, + getText: vi.fn().mockReturnValue(""), + lineCount: 0, + }, + selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } }, + edit: vi.fn().mockResolvedValue(true), + revealRange: vi.fn(), + } + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockEditor as any) + vi.mocked(vscode.commands.executeCommand).mockResolvedValue(undefined) + vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { + setTimeout(() => callback({ uri: { fsPath: `${mockCwd}/adopt.md`, scheme: "file" } } as any), 0) + return { dispose: vi.fn() } + }) + vi.mocked(vscode.window).visibleTextEditors = [mockEditor as any] + diffViewProvider.adoptCreatedDirs([`${mockCwd}/new-parent`]) + ;(diffViewProvider as any).editType = "create" + + await diffViewProvider.open("adopt.md") + + expect((diffViewProvider as any).createdDirs).toEqual([`${mockCwd}/new-parent`]) + }) + }) describe("scrollToFirstDiff method", () => { @@ -1317,6 +1346,16 @@ describe("DiffViewProvider", () => { expect(diffViewProvider["relPath"]).toBeUndefined() }) + + it("reset() drops adopted directories so a later transaction cannot remove a previous one's", async () => { + // Adoption is per transaction: a stale list would make the next rollback rmdir + // directories belonging to a file the user has since accepted. + diffViewProvider.adoptCreatedDirs([`${mockCwd}/new-parent`]) + + await diffViewProvider.reset() + + expect((diffViewProvider as any).adoptedCreatedDirs).toEqual([]) + }) 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 From 1b09bab32b233940be6e65f6999cc5b213aa3b96 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 06:15:28 +0800 Subject: [PATCH 21/26] test(editor): keep the new diff-view assertions inside the suppression budget The first draft reached for the file's existing "(provider as any)" idiom, which pushed integrations/editor/__tests__/DiffViewProvider.spec.ts over its recorded no-explicit-any count - strict lint compares suppressions in both directions, so an increase is a compile failure even when the total is unchanged. The new test now uses the typed test doubles (makeUri / makeTextDocument / makeTextEditor / makeDisposable), the public editType field, and bracket access for the private directory lists, so the suppression file is untouched. Full "eslint . --ext=ts --max-warnings=0" on the merged tree: exit 0, suppressions numstat empty. vitest writeToFileTool + DiffViewProvider: 143 passed / 5 skipped. tsc --noEmit is 74 on this merge commit (62 before the org-main merge); the 12 new errors are all in files main brought in (webviewMessageHandler.readOriginalContent .spec.ts, ClineProvider.spec.ts, WebviewFocusTracker) and none in the touched files. --- .../editor/__tests__/DiffViewProvider.spec.ts | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index c6d591ac5e..20029db518 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -5,7 +5,7 @@ import * as path from "path" import delay from "delay" import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" -import { makeRange, makeTextDocument, makeTextEditor, makeUri } from "../../../test-utils/vscode" +import { makeDisposable, makeRange, makeTextDocument, makeTextEditor, makeUri } from "../../../test-utils/vscode" // Mock delay vi.mock("delay", () => ({ @@ -363,29 +363,21 @@ describe("DiffViewProvider", () => { // WriteToFileTool.execute() creates the parent directories before the diff view // exists, so the provider's own mkdir records nothing. Without the adoption // createdDirs stayed empty and a rolled-back new-file write left them on disk. - const mockEditor = { - document: { - uri: { fsPath: `${mockCwd}/adopt.md`, scheme: "file" }, - getText: vi.fn().mockReturnValue(""), - lineCount: 0, - }, - selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } }, - edit: vi.fn().mockResolvedValue(true), - revealRange: vi.fn(), - } - vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockEditor as any) + const mockDocument = makeTextDocument({ uri: makeUri(`${mockCwd}/adopt.md`) }) + const mockEditor = makeTextEditor({ document: mockDocument }) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockEditor) vi.mocked(vscode.commands.executeCommand).mockResolvedValue(undefined) vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { - setTimeout(() => callback({ uri: { fsPath: `${mockCwd}/adopt.md`, scheme: "file" } } as any), 0) - return { dispose: vi.fn() } + setTimeout(() => callback(mockDocument), 0) + return makeDisposable() }) - vi.mocked(vscode.window).visibleTextEditors = [mockEditor as any] + vi.mocked(vscode.window).visibleTextEditors = [mockEditor] diffViewProvider.adoptCreatedDirs([`${mockCwd}/new-parent`]) - ;(diffViewProvider as any).editType = "create" + diffViewProvider.editType = "create" await diffViewProvider.open("adopt.md") - expect((diffViewProvider as any).createdDirs).toEqual([`${mockCwd}/new-parent`]) + expect(diffViewProvider["createdDirs"]).toEqual([`${mockCwd}/new-parent`]) }) }) @@ -1354,7 +1346,7 @@ describe("DiffViewProvider", () => { await diffViewProvider.reset() - expect((diffViewProvider as any).adoptedCreatedDirs).toEqual([]) + expect(diffViewProvider["adoptedCreatedDirs"]).toEqual([]) }) 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 From 2a9bfabfb3761c44fabd416178d3149e1ee29251 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 11:18:12 +0800 Subject: [PATCH 22/26] fix(tools): stop a post-commit failure from rolling back a landed write Three CodeRabbit rows at head 1b09bab32, all in the write_to_file teardown: - execute()'s catch called diffViewProvider.reset() directly, so a reset that rejected skipped reportRevertFailure(): the user saw only the write error while the editor still held content nobody approved. It now goes through the same resilient helper the parse-failure and partial-stream teardowns use. - The prevent-focus-disruption rejection exit returned without resetting the provider. That branch never opens a diff view, but the preflight had already adopted this call's created directories and set editType/originalContent, so the next write's open() folded them into its own createdDirs and its rollback rmdir'ed directories belonging to the rejected write. - saveChanges() persists the document and then keeps working (closing the diff views, tab bookkeeping, diagnostics), and trackFileContext() and pushToolWriteResult() run after it. A failure past that point restored the previous content, or unlinked a file the user had just approved being created. saveChanges() now signals its commit point at the document save and the rollback stands down from there; saveDirectly() commits when it returns, because performing the write is what it does. Same shape as the landed rule recorded for #41 Follow-up 11 (6092123982) in #1921 (1b9ae3071): the commit point is where a write becomes irreversible, not where the call finished. Tests: four new cases; each is the only failure when its own guard is removed, and the DiffViewProvider pair pins both directions of the commit signal. --- src/core/tools/WriteToFileTool.ts | 33 +++++++++- .../tools/__tests__/writeToFileTool.spec.ts | 62 +++++++++++++++++++ src/integrations/editor/DiffViewProvider.ts | 7 +++ .../editor/__tests__/DiffViewProvider.spec.ts | 27 ++++++++ 4 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 4a9e965a83..eae1488555 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -324,6 +324,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false + // The point of no return for this write. saveChanges() persists the document and then keeps + // working (closing the diff views, tab bookkeeping, diagnostics), and trackFileContext() and + // pushToolWriteResult() run after it, so a rejection past this point must not roll back a + // write the user already approved: the rollback would restore the previous content, or delete + // a file that was created and saved. Set only once the durable write has landed. + let writeCommitted = false + try { // The guarded scope starts before the preflight filesystem work: a throw from // fileExistsAtPath or createDirectoriesForFile must report and tear down like any other @@ -410,10 +417,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // task, which suppresses the diff preview of every later write_to_file, and // the TaskAborted listener leaks. super.resetPartialState() + // This branch never opened a diff view, but the preflight above already adopted + // this call's created directories and set editType/originalContent on the + // per-task provider. A rejection that leaves them behind hands them to the next + // write's open(), whose rollback would then rmdir directories that belong to + // this rejected write - and fail with ENOTEMPTY once a later file lives in one. + await this.resetDiffViewAfterWrite(task) return } await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // saveDirectly() performs the write itself and reports a failing write, so returning + // means the content is on disk: from here the rollback has to stand down. + writeCommitted = true } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) @@ -449,7 +465,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, () => { + // Signalled by saveChanges() at the document save, not when it returns: the editor + // bookkeeping and diagnostics that follow can still reject, and by then the file + // has already landed. + writeCommitted = true + }) } if (relPath) { @@ -490,8 +511,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // an existing file. Report it the same way the parse-failure teardown and // cleanupFailedPartialStream() do - after the reset, so the report is the last thing the // failed write leaves behind. - const reverted = await this.revertDiffChangesBeforeReset(task) - await task.diffViewProvider.reset() + // After the commit point the write is durable, so restoring the previous content + // (or unlinking a newly created file) would undo an approved write to clean up an + // unrelated failure. Treat the rollback as done rather than performing it. + const reverted = writeCommitted ? true : await this.revertDiffChangesBeforeReset(task) + // The resilient helper, as in the other two teardowns: a reset that rejects must not + // skip the report below, or the user sees only the write error while the editor still + // holds content nobody approved. + await this.resetDiffViewAfterWrite(task) super.resetPartialState() if (!reverted) { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index f5aeb47590..a163e9e937 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -1149,6 +1149,68 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + it("reports the rollback failure even when the diff-view reset rejects", async () => { + // The catch used a bare diffViewProvider.reset(): when that rejected, the rollback report + // after it never ran, so the user saw only the write error while the editor still held + // content nobody approved. The teardown now goes through the same resilient helper the + // parse-failure and partial-stream teardowns already use. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("open failed")) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("could not be restored")) + }) + + it("resets the diff-view provider when the prevent-focus-disruption approval is rejected", async () => { + // This branch never opens a diff view, but the preflight already adopted the created + // directories and set editType/originalContent on the per-task provider. Left behind, the + // next write's open() folds those directories into its own createdDirs and its rollback + // rmdirs directories belonging to this rejected write - ENOTEMPTY once a later file lives + // in one of them. + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + mockCline.diffViewProvider.saveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "final content", + }) + mockAskApproval = vi.fn().mockResolvedValue(false) + + await executeWriteFileTool({}) + + expect(mockCline.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("does not roll back a write that saveChanges already committed", async () => { + // saveChanges() persists the document and then keeps working, and trackFileContext() runs + // after it. A failure past that point used to restore the previous content - or unlink a + // file the user had just approved being created - to clean up an unrelated error. + mockCline.diffViewProvider.saveChanges.mockImplementation( + async (_diagnosticsEnabled: boolean, _writeDelayMs: number, onCommit?: () => void) => { + onCommit?.() + return { newProblemsMessage: "", userEdits: undefined, finalContent: "final content" } + }, + ) + mockCline.fileContextTracker.trackFileContext.mockRejectedValue(new Error("context tracking failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index ef72cae072..37b1a9f4eb 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -344,6 +344,7 @@ export class DiffViewProvider { async saveChanges( diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + onCommit?: () => void, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -361,6 +362,12 @@ export class DiffViewProvider { await updatedDocument.save() } + // The write is irreversible from here: a clean document already matches disk, and a saved + // one has landed. Everything below (closing the diff views, tab bookkeeping, diagnostics) + // can still reject, and a caller that rolls the tool call back after this point would undo + // a write the user approved - so it is told, at the commit point rather than on return. + onCommit?.() + // Stop tracking touches and cancel any pending scroll-to-diff before any // programmatic editor activation below. this.disposeActiveEditorListener() diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 20029db518..8056c6342c 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -970,6 +970,33 @@ describe("DiffViewProvider", () => { expect(mockDelay).toHaveBeenCalledWith(5000) expect(vscode.languages.getDiagnostics).toHaveBeenCalled() }) + + it("signals the commit point once the document write has landed", async () => { + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + const onCommit = vi.fn() + + await diffViewProvider.saveChanges(false, 0, onCommit) + + // The shared document is clean, so nothing is written: the file already matches the + // buffer, which is just as irreversible. The caller stops rolling back here. + expect(onCommit).toHaveBeenCalledTimes(1) + }) + + it("does not signal the commit point when the document save rejects", async () => { + const save = vi.fn().mockRejectedValue(new Error("save failed")) + // Structural double carrying only the members saveChanges() reaches before the commit + // point; the full TextEditor shape needs a live editor host - hence the assertion. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const onCommit = vi.fn() + + await expect(diffViewProvider.saveChanges(false, 0, onCommit)).rejects.toThrow("save failed") + + // A rejected save leaves the file absent or holding the previous content: the caller has + // to keep the rollback available, so signalling here would lose the write's debris. + expect(onCommit).not.toHaveBeenCalled() + }) }) describe("preEditScrollLine capture and restore", () => { From 5ff7bea7300fd5940d367fbdb8d042d72708e8cc Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 10 Oct 2026 11:24:40 +0800 Subject: [PATCH 23/26] style: format the write-path files for the repo-wide prettier check `pnpm format:check` (root `prettier --check .`) became a required step of the compile job on main in 667ff5891, and CI runs it on the merge commit. These four files - all inside this PR's own diff - were already flagged at 1b09bab32, so the gate fails this branch with or without the fixes above. The change is prettier's own output and no file outside the diff is touched. --- ...teToFileTool-partial-state-cleanup.spec.ts | 13 ++++++++---- .../tools/__tests__/writeToFileTool.spec.ts | 20 ++++--------------- src/integrations/editor/DiffViewProvider.ts | 6 +----- .../editor/__tests__/DiffViewProvider.spec.ts | 6 +++--- 4 files changed, 17 insertions(+), 28 deletions(-) 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 0ff9d72826..d009046ba5 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -55,7 +55,10 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { 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 () => { @@ -107,7 +110,9 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { task, // Only handleError is reached when no streaming error was recorded; the // structural double is the existing pattern in this file. - { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters[1], + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters< + (typeof writeToFileTool)["onParameterParseFailure"] + >[1], new Error("parameter parse failed"), ) @@ -135,7 +140,7 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { const handled = await writeToFileTool["onParameterParseFailure"]( task, - { handleError } as unknown as Parameters[1], + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], new Error("parameter parse failed"), ) @@ -160,7 +165,7 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { const handled = await writeToFileTool["onParameterParseFailure"]( task, - { handleError } as unknown as Parameters[1], + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], new Error("parameter parse failed"), ) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index a163e9e937..988591457d 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -168,7 +168,7 @@ describe("writeToFileTool", () => { originalContent: "", open: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), - adoptCreatedDirs: vi.fn(), + adoptCreatedDirs: vi.fn(), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ @@ -1117,7 +1117,9 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}) - expect(writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`)).toBeUndefined() + expect( + writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`), + ).toBeUndefined() }) it("reports user edits with diff feedback", async () => { @@ -1211,8 +1213,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 @@ -1396,12 +1396,6 @@ describe("writeToFileTool", () => { } }) - - - - - - 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" }), @@ -1426,8 +1420,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 @@ -1448,10 +1440,6 @@ describe("writeToFileTool", () => { expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() }) - - - - }) describe("partial-stream failure teardown and directory ownership", () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 37b1a9f4eb..3e131e2f0f 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -612,7 +612,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 @@ -982,10 +981,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 ?? "") await vscode.workspace.applyEdit(edit) } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 8056c6342c..1c4916fc36 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -379,7 +379,6 @@ describe("DiffViewProvider", () => { expect(diffViewProvider["createdDirs"]).toEqual([`${mockCwd}/new-parent`]) }) - }) describe("scrollToFirstDiff method", () => { @@ -1333,7 +1332,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") }) @@ -1365,7 +1366,6 @@ describe("DiffViewProvider", () => { expect(diffViewProvider["relPath"]).toBeUndefined() }) - it("reset() drops adopted directories so a later transaction cannot remove a previous one's", async () => { // Adoption is per transaction: a stale list would make the next rollback rmdir // directories belonging to a file the user has since accepted. From b97f68ea3dc0eb729cf3c4e20bc33b7735b8fedc Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 16:19:42 +0800 Subject: [PATCH 24/26] fix(editor): fail the approved save when the document save reports false saveChanges() awaited the document save and dropped the returned boolean, so a save that reported failure still ran the success bookkeeping and the caller was told the approved content persisted. Capture the awaited result and throw a typed DocumentSaveFailedError before the commit point and before any success bookkeeping, so the caller keeps its rollback and the existing failure handling runs. A true result behaves exactly as before; a save that rejects still propagates as before. This branch gates an on-commit callback after the save rather than a placeholder release, so the ordering promise is derived for this branch: the callback must not run when the save reported false. The statement is inherited from upstream code that predates the chain; this branch is the earliest carrier that is still open, so the shape is fixed here and the later branches port it forward re-derived. The regression doubles resolve an explicit false, so a double resolving undefined cannot pass by accident. Negative controls: reverting to a bare await reddens exactly the two save-false tests (2/88, the same two as the pre-fix production run); moving the throw after the callback reddens exactly the ordering test (1/88). Restored byte for byte, hash verified. See tracking item 6106182216. --- src/integrations/editor/DiffViewProvider.ts | 23 ++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 65 ++++++++++++++++++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 3e131e2f0f..95a038adcc 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -26,6 +26,20 @@ function isEnoent(error: unknown): boolean { return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT" } +/** + * The approved document save reported failure: the content the user accepted never reached disk. + * Typed so the caller's failure handling can tell a failed write from an unrelated teardown error, + * and so a discarded boolean can never let the success bookkeeping run on a write that did not land. + */ +export class DocumentSaveFailedError extends Error { + constructor(absolutePath: string) { + super( + `Failed to save ${absolutePath}: the document save reported failure; the approved content is not on disk.`, + ) + this.name = "DocumentSaveFailedError" + } +} + export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes" @@ -359,7 +373,14 @@ export class DiffViewProvider { const editedContent = updatedDocument.getText() if (updatedDocument.isDirty) { - await updatedDocument.save() + const saved = await updatedDocument.save() + // A save that reports false never landed: the buffer is still dirty and the file holds + // the previous content (or nothing for a new file). Throw before the commit point and + // before any success bookkeeping, so the caller keeps its rollback and the existing + // failure handling runs. A save that rejects still propagates as before. + if (!saved) { + throw new DocumentSaveFailedError(absolutePath) + } } // The write is irreversible from here: a clean document already matches disk, and a saved diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 1c4916fc36..2b759e4869 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,5 +1,10 @@ import * as fs from "fs/promises" -import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" +import { + DocumentSaveFailedError, + DiffViewProvider, + DIFF_VIEW_URI_SCHEME, + DIFF_VIEW_LABEL_CHANGES, +} from "../DiffViewProvider" import * as vscode from "vscode" import * as path from "path" import delay from "delay" @@ -996,6 +1001,64 @@ describe("DiffViewProvider", () => { // to keep the rollback available, so signalling here would lose the write's debris. expect(onCommit).not.toHaveBeenCalled() }) + + it("throws a typed write failure before any success bookkeeping when the save reports false", async () => { + // The double resolves an explicit false, not undefined: a bare await would let a + // double resolving undefined pass by accident, so the reported-failure contract is + // pinned with a real false. + const save = vi.fn().mockResolvedValue(false) + // Structural double carrying only the members saveChanges() reaches at the save; the + // full TextEditor shape needs a live editor host - hence the assertion. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + + await expect(diffViewProvider.saveChanges(false, 0)).rejects.toThrow(DocumentSaveFailedError) + + expect(save).toHaveBeenCalledTimes(1) + // Success bookkeeping must not run: the caller is told the write failed, so the diff + // views stay open and no result is stored for formatFileWriteResponse(). + expect(closeAllDiffViews).not.toHaveBeenCalled() + expect(diffViewProvider.newProblemsMessage).toBeUndefined() + expect(diffViewProvider.userEdits).toBeUndefined() + }) + + it("does not signal the commit point when the save reports false", async () => { + const save = vi.fn().mockResolvedValue(false) + // Structural double, as in the rejected-save test above. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + const onCommit = vi.fn() + + await expect(diffViewProvider.saveChanges(false, 0, onCommit)).rejects.toThrow(DocumentSaveFailedError) + + // Ordering for this branch: the commit point stands in for the placeholder release the + // sibling branches gate on. A callback that ran here would set the caller's + // writeCommitted flag and stand the rollback down on a write that never landed. + expect(onCommit).not.toHaveBeenCalled() + }) + + it("behaves exactly as before when the save reports true", async () => { + const save = vi.fn().mockResolvedValue(true) + // Structural double, as in the rejected-save test above. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + const onCommit = vi.fn() + + const result = await diffViewProvider.saveChanges(false, 0, onCommit) + + expect(save).toHaveBeenCalledTimes(1) + expect(onCommit).toHaveBeenCalledTimes(1) + expect(closeAllDiffViews).toHaveBeenCalledTimes(1) + expect(result.finalContent).toBe("new content") + }) }) describe("preEditScrollLine capture and restore", () => { From 3daf33860bd1e5c8efdd3ee2c616bf74b32b8892 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 18:16:47 +0800 Subject: [PATCH 25/26] fix(editor): fail the rollback when the pre-stream restore edit is not applied restorePreStreamBuffer() awaited vscode.workspace.applyEdit() and dropped the returned boolean, so an edit the workspace reported as not applied was treated as applied: saveBufferClean() then saved a buffer that could still hold the unapproved streamed content, close() could succeed on the resulting clean tab, and the rollback reported success for a restore that never happened. Capture the result the way the close in discardFileTab() already does, treat false as a failed restore, keep a rejection as the cause, and throw before saveBufferClean() runs so no save, close, or delete bookkeeping continues on the assumption that the buffer came back. This is the same defect family as the discarded document-save result fixed in the previous commit, and the shape is taken from it. The regression double resolves an explicit false, so a double resolving undefined cannot pass by accident; a second test pins the rejection as the cause, and a third pins the applied-edit path so the success flow is unchanged. Negative control: reverting the call site to the bare await (the pre-fix production file, blob 95a038adcc7873460236b8304af28b59b5bf4c1e) reddens exactly the two failure tests (2/91, the same two as the pre-fix production run) and leaves the applied-edit guard green. Restored byte for byte, hash verified. The thread's second paragraph asks for the same check on document.save() in saveBufferClean(); its stated consequence, a dirty tab whose close is refused, is already converted into a rollback failure by the close check in discardFileTab(), and the only caller deletes the file a moment later, so that half is dispositioned with evidence in the thread rather than changed here. see review comment 4240879376 --- src/integrations/editor/DiffViewProvider.ts | 24 ++- .../editor/__tests__/DiffViewProvider.spec.ts | 188 ++++++++++++++++++ 2 files changed, 210 insertions(+), 2 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 95a038adcc..149fa3c2df 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -992,7 +992,9 @@ 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. + * never survives a close that the editor vetoes. An edit the workspace did not apply is a + * failed restore and throws, so the rollback cannot report success on a buffer that never + * came back. */ private async restorePreStreamBuffer(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( @@ -1004,7 +1006,25 @@ export class DiffViewProvider { 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) + // applyEdit() resolves false when the edit was not applied: the buffer would still hold the + // unapproved streamed content, and the save that follows would persist exactly what this + // rollback is undoing. Capture the result like the close below does, treat false as a + // failed restore, and throw before saveBufferClean runs so the caller keeps its failure. + // A rejection is kept as the cause. + let applied: boolean + let applyError: Error | undefined + try { + applied = await vscode.workspace.applyEdit(edit) + } catch (error) { + applied = false + applyError = error instanceof Error ? error : new Error(String(error)) + } + if (!applied) { + throw new Error( + `Rollback could not restore the pre-stream buffer for ${absolutePath}; the workspace edit was not applied, so the buffer may still hold the unapproved content.`, + { cause: applyError }, + ) + } } /** diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 2b759e4869..217dfd8aad 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1559,6 +1559,194 @@ describe("DiffViewProvider", () => { } }) + it("revertChanges() fails the rollback when the pre-stream restore edit is not applied", async () => { + // applyEdit() resolves false when the workspace refuses the edit. Dropping that boolean let + // the rollback save and close a buffer that still held the unapproved streamed content, and + // report success for a restore that never happened. + 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") + 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, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + 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: "", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow( + "could not restore the pre-stream buffer", + ) + // The buffer never came back, so nothing may be saved, closed, or deleted on the + // assumption that it did. Counts, not call shapes: save, close, and unlink each have + // exactly one legitimate caller on this path, and all of them sit behind the restore. + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(close).toHaveBeenCalledTimes(0) + expect(fs.unlink).toHaveBeenCalledTimes(0) + } 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() keeps the workspace rejection as the cause of a failed restore", async () => { + // A restore that fails because the workspace rejected the edit keeps its reason: the + // rollback error carries the rejection as its cause, the shape the refused close uses. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const rejection = new Error("edit rejected by the workspace") + 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, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(rejection) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + let restoreFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + restoreFailure = error + } 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) + } + } + + expect(restoreFailure).toBeInstanceOf(Error) + if (restoreFailure instanceof Error) { + expect(restoreFailure.message).toContain("could not restore the pre-stream buffer") + expect(restoreFailure.cause).toBe(rejection) + } + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(close).toHaveBeenCalledTimes(0) + expect(fs.unlink).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() restores, saves and closes when the restore edit is applied", async () => { + // Guard for the fix itself: an applied edit keeps the restore-save-close-delete flow + // exactly as it was; only a refused edit changes behaviour. + 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") + 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, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + 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 expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + } 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) + } + } + + // The restore edit carried the pre-stream content, and the full flow ran exactly once. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("pre-stream content") + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(close).toHaveBeenCalledTimes(1) + expect(fs.unlink).toHaveBeenCalledTimes(1) + }) + 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 23213efef87c2db3b0210e24f8b9eb629e538d6a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sun, 11 Oct 2026 20:56:04 +0800 Subject: [PATCH 26/26] fix(editor): fail the existing-file rollback when the revert edit or save reports false revertChanges() awaited the workspace edit and the document save of its existing-file branch and dropped both returned booleans, so a refused edit or a refused save was reported to the caller as a completed rollback. The parse-failure and partial-stream cleanup paths gate their failure report on revertDiffChangesBeforeReset(), which can only see a throw, so the user was told the teardown was complete while the document could still hold the unapproved content, or the restored original content could still be missing from disk. Validate the edit result before the save runs and the save result before any later bookkeeping, capture each result the way the pre-stream restore and the refused close already do, and keep a rejection as the cause. An applied edit plus a landed save behaves exactly as before; the new-file branch is untouched, its close check is already pinned. This is the second call site of the defect family this branch already fixed twice - the discarded document-save result in saveChanges() and the ignored restore edit in restorePreStreamBuffer() - and the shape is taken from them. The site was registered rather than fixed in the previous round; a registration does not clear a pre-merge row, only code at the head does, so it is fixed here. The regression doubles resolve an explicit false, so a double resolving undefined cannot pass by accident; a test pins each rejection as the cause and a success guard pins the unchanged applied-and-saved flow. The shared editor double now reports a save that landed, because the rollback reads the result. Negative controls: reverting the edit call site to the bare await reddens exactly the two edit tests (2/96), reverting the save call site reddens exactly the two save tests (2/96), and the pre-fix production file reddens exactly the four failure tests (4/96) with the success guard green. Restored byte for byte, hash verified. see tracking item 6095623016 --- src/integrations/editor/DiffViewProvider.ts | 39 +++- .../editor/__tests__/DiffViewProvider.spec.ts | 195 +++++++++++++++++- 2 files changed, 227 insertions(+), 7 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 149fa3c2df..a922b471da 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -652,9 +652,42 @@ export class DiffViewProvider { // Apply the edit and save, since contents shouldn't have changed // this won't show in local history unless of course the user made - // changes and saved during the edit. - await vscode.workspace.applyEdit(edit) - await updatedDocument.save() + // changes and saved during the edit. applyEdit() resolves false when the workspace + // refused the edit and save() resolves false when the editor refused the save: both + // booleans were dropped, so a rollback whose restore never applied or whose save never + // landed was reported to the caller as a completed rollback. Capture each result the + // way the pre-stream restore and the refused close do, keep a rejection as the cause, + // and throw before any later bookkeeping so nothing proceeds on the assumption that + // the original content came back and reached disk. + let applied: boolean + let applyError: Error | undefined + try { + applied = await vscode.workspace.applyEdit(edit) + } catch (error) { + applied = false + applyError = error instanceof Error ? error : new Error(String(error)) + } + if (!applied) { + throw new Error( + `Rollback could not restore the original content for ${absolutePath}; the workspace edit was not applied, so the document may still hold the unapproved content.`, + { cause: applyError }, + ) + } + + let saved: boolean + let saveError: Error | undefined + try { + saved = await updatedDocument.save() + } catch (error) { + saved = false + saveError = error instanceof Error ? error : new Error(String(error)) + } + if (!saved) { + throw new Error( + `Rollback could not save the restored document for ${absolutePath}; the original content may not be on disk.`, + { cause: saveError }, + ) + } await this.closeAllDiffViews() diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 217dfd8aad..f2ebdd5b74 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1226,7 +1226,8 @@ describe("DiffViewProvider", () => { uri: { fsPath: `${mockCwd}/test.txt` }, getText: vi.fn().mockReturnValue("modified"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + // The revert branch reads the save result; report a save that landed. + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, } @@ -1245,12 +1246,15 @@ describe("DiffViewProvider", () => { describe("userTouchedDocument close/keep behavior", () => { const mockTargetPath = `${mockCwd}/mock-target-file.ts` + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. const buildActiveDiffEditor = () => ({ document: { 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 }), }, }) @@ -1790,6 +1794,183 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() fails the existing-file rollback when the revert edit is not applied", async () => { + // applyEdit() resolves false when the workspace refuses the edit. Dropping that boolean let + // the rollback save anyway and report a completed rollback for a restore that never + // happened: the document could still hold the unapproved content, and the caller's + // failure reporting - the whole point of the revertDiffChangesBeforeReset return value - + // was skipped. The edit result is validated before the save runs. + const editor = buildActiveDiffEditor() + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the original content") + + // Counts, not call shapes: the save and both close paths each have exactly one legitimate + // caller on this branch, and all of them sit behind the edit check. + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() keeps the workspace rejection as the cause of a failed existing-file revert", async () => { + // A revert that fails because the workspace rejected the edit keeps its reason: the + // rollback error carries the rejection as its cause, the shape the refused close and the + // pre-stream restore use. + const editor = buildActiveDiffEditor() + const rejection = new Error("edit rejected by the workspace") + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(rejection) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + let revertFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + revertFailure = error + } + + expect(revertFailure).toBeInstanceOf(Error) + if (revertFailure instanceof Error) { + expect(revertFailure.message).toContain("could not restore the original content") + expect(revertFailure.cause).toBe(rejection) + } + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() fails the existing-file rollback when the restored document save reports false", async () => { + // The double resolves an explicit false, not undefined: a bare await would let a double + // resolving undefined pass by accident, so the reported-failure contract is pinned with a + // real false. A refused save means the restored original content never reached disk; the + // rollback must fail before the close and tab bookkeeping assume it did. + const editor = buildActiveDiffEditor() + editor.document.save = vi.fn().mockResolvedValue(false) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored document") + + expect(editor.document.save).toHaveBeenCalledTimes(1) + // Throw before any later bookkeeping: the diff views stay open and the tab decision + // never runs on a document whose restored content may not be on disk. + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() keeps the save rejection as the cause of a failed existing-file revert", async () => { + // Same shape as the edit rejection: a save that rejects is a failed rollback, and the + // rejection is kept as the cause rather than ending up as a reported success. + const editor = buildActiveDiffEditor() + const rejection = new Error("save refused by the editor host") + editor.document.save = vi.fn().mockRejectedValue(rejection) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + let revertFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + revertFailure = error + } + + expect(revertFailure).toBeInstanceOf(Error) + if (revertFailure instanceof Error) { + expect(revertFailure.message).toContain("could not save the restored document") + expect(revertFailure.cause).toBe(rejection) + } + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() reverts, saves and closes when the edit is applied and the save reports true", async () => { + // Guard for the fix itself: an applied edit plus a save that reports true keeps the + // revert-save-close flow exactly as it was; only a refused edit or refused save changes + // behaviour. + const editor = buildActiveDiffEditor() + editor.document.save = vi.fn().mockResolvedValue(true) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + // The revert edit carried the original content back, the save ran once, and the full + // close and tab flow proceeded exactly as before the fix. closeAllDiffViews also runs + // inside the trailing reset(), so the success evidence is "at least once, before the + // tab decision", not an exact count. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("original") + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(closeAllDiffViews.mock.calls.length).toBeGreaterThanOrEqual(1) + expect(closeAllDiffViews.mock.invocationCallOrder[0]).toBeLessThan(closeFileTab.mock.invocationCallOrder[0]) + expect(closeFileTab).toHaveBeenCalledTimes(1) + }) + it("marks userTouchedDocument when the file editor is activated during the diff", async () => { let activeCallback: ((editor: any) => void) | undefined vi.mocked(vscode.window.onDidChangeActiveTextEditor).mockImplementation((cb: any) => { @@ -1832,12 +2013,15 @@ describe("DiffViewProvider", () => { describe("userTouchedDiffEditor keep/close behavior", () => { const mockTargetPath = `${mockCwd}/mock-target-file.ts` + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. const buildActiveDiffEditor = () => ({ document: { 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 }), }, }) @@ -2240,12 +2424,15 @@ describe("DiffViewProvider", () => { describe("auto-close settings decision table", () => { const mockTargetPath = `${mockCwd}/auto-close-test.ts` + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. const buildActiveDiffEditor = () => ({ document: { 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 }), }, })