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/17] 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/17] 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/17] 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/17] 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/17] 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 9b93a6f882a4a5690aaffb763b6660eadaa308be 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 06/17] fix(write-to-file): run diff cleanup when handleError rejects --- src/core/tools/WriteToFileTool.ts | 56 ++- .../tools/__tests__/writeToFileTool.spec.ts | 383 +++++++++++++++++- 2 files changed, 424 insertions(+), 15 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ba25a9f2c0..653f3c729b 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -245,12 +245,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - if (newContent.startsWith("```")) { newContent = newContent.split("\n").slice(1).join("\n") } @@ -274,7 +268,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } + // Tracks whether the user approved the write, so the error path only reverts the + // diff document when the content was never approved (an approved edit is kept in + // the editor so the user can save it manually after a late failure). + let writeApproved = false + try { + // Create parent directories for new files inside the try block so filesystem + // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup + // and consecutive-mistake counting, rather than escaping unhandled. + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } + task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -308,9 +314,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // The prevent-focus branch set editType/originalContent on the provider + // before asking. Clear them on denial (no diff document was opened in + // this branch, so reset() is sufficient; the non-prevent-focus denial + // branch resets through revertChanges()), so a later write re-checks + // the file system instead of reusing the stale editType. + await this.resetDiffViewAfterWrite(task) return } + writeApproved = true + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) } else { if (!task.diffViewProvider.isEditing) { @@ -344,6 +358,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } @@ -357,17 +373,35 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) - await task.diffViewProvider.reset() - this.resetPartialState() + await this.resetDiffViewAfterWrite(task) task.processQueuedMessages() return } catch (error) { - await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() + // Finalize any open partial tool message so the UI spinner doesn't get stuck. + // The partial ask fired during streaming (handlePartial) or early in execute sets + // partial: true on the webview message; without this, the spinner persists even + // after the error bubble appears. + await this.finalizePartialToolAskAfterFailure(task) + // The diff cleanup runs in a finally around handleError: the production + // handleError awaits Task.say(), which rejects when the task is aborted, and a + // rejected handleError must not skip restoring the unapproved streamed content. + try { + await handleError("writing file", error as Error) + } finally { + // Before approval the diff document holds unapproved streamed content: + // restore it so a user save cannot persist it. After approval the content + // is the user's accepted edit -- keep it in the editor (dirty) so they can + // save it manually. + if (!writeApproved) { + await this.revertDiffChangesBeforeReset(task) + } + await this.resetDiffViewAfterWrite(task) + } return + } finally { + this.resetTaskPartialState(task) } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index dddd139011..7bdd3cbdb2 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -170,6 +170,7 @@ describe("writeToFileTool", () => { update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", userEdits: null, @@ -310,15 +311,16 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "does not create directories in handlePartial -- only execute() creates them", async () => { - // First call - path not yet stabilized + // First call - path not yet stabilized, early return await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Second call with same path - path stabilized, handlePartial runs but + // must NOT call createDirectoriesForFile (directory creation belongs in execute) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() }, ) @@ -415,6 +417,25 @@ describe("writeToFileTool", () => { // Should process normally without issues expect(mockCline.consecutiveMistakeCount).toBe(0) }) + + it("does not report a successful write as failed when final diff reset rejects", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockCline.didEditFile).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("partial block handling", () => { @@ -949,11 +970,146 @@ describe("writeToFileTool", () => { } }) + it("reports a filesystem error only once across the streaming and execute phases", async () => { + // Regression test for the double-error UX defect: a single write_to_file call to a + // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") + // and once in execute() ("writing file"). handlePartial now swallows its error so only + // the authoritative execute() error is surfaced. + const erofs = () => + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }) + mockCline.diffViewProvider.open.mockRejectedValue(erofs()) + mockedCreateDirectoriesForFile.mockRejectedValue(erofs()) + + // Streaming phase: stabilize path then fail (swallowed, no handleError) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Final phase: execute() reports the single authoritative error + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + }) + + it("does not reset consecutive mistake count when directory creation fails", async () => { + mockCline.consecutiveMistakeCount = 3 + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.consecutiveMistakeCount).toBe(3) + }) + + it("reverts the diff document when the write fails before approval", async () => { + // Regression test for the dirty-diff leak: streaming already opened the diff view + // with unapproved content, and the write then failed before the user could approve + // it. reset() alone left the diff document dirty with the streamed content -- a + // user save in the editor would persist a write the task never completed. The + // error path must revert the document (like the approval-denied path does) before + // resetting the provider state. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Stream two deltas so the diff view is open with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + }) + + it("continues cleanup when reverting the diff document fails before approval", async () => { + // Pins the .catch arm on revertChanges() in revertDiffChangesBeforeReset(): a failed + // revert (e.g. the diff view was already closed) must only be logged so the + // remaining cleanup (diff view reset + per-task state teardown) always completes. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + + // Stream two deltas so the diff view opens with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval and the revert fails too. + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error reverting write_to_file diff view changes:", + expect.any(Error), + ) + // The diff view is still reset and the per-task stream state still torn down. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The reference that was registered must be the one removed. + expect(abortCleanup).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps approved diff content in the editor when saving fails after approval", async () => { + // The reverse of the previous test: once the user approved the write, the diff + // content is their accepted edit. A late failure (e.g. saveChanges rejecting) + // must NOT revert it -- the document stays dirty so the user can save it manually. + mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) + await executeWriteFileTool({}, { fileExists: false }) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + it("continues execute error cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + await executeWriteFileTool({}, { fileExists: false }) + // The execute error path finalizes without a text match: any open partial + // tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) it("keeps partial stream failures isolated per task", async () => { mockCline.diffViewProvider.open.mockRejectedValueOnce( @@ -979,7 +1135,53 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2) }) + it("swallows diff view reset errors during partial failure cleanup", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("continues partial failure cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { // Regression test: before the fix, createDirectoriesForFile was called in handlePartial @@ -1002,8 +1204,181 @@ describe("writeToFileTool", () => { expect(mockHandleError).not.toHaveBeenCalled() }) + it("EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", async () => { + // Regression test: before the fix, createDirectoriesForFile in execute() sat outside + // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. + // After the fix the call is inside the try block, so filesystem errors are caught and + // routed through handleError with proper diffViewProvider.reset() cleanup. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + await executeWriteFileTool({}, { fileExists: false }) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The tool must not have proceeded to open or save + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message on error so the UI spinner does not get stuck", async () => { + // Regression test: when a filesystem error is thrown in execute() the webview + // message created during handlePartial (or the early ask in execute) is stuck in + // partial: true state, showing an indefinite spinner alongside the error bubble. + // The catch block must call finalizePartialToolAsk() to close the spinner without + // blocking for user input. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + // handleError must still be called + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // finalizePartialToolAsk must have been called (no text: the execute error + // path closes whichever partial tool ask is open) to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The write was never approved, so the diff document is reverted before reset + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + }) + + it("runs diff cleanup when handleError rejects", async () => { + // The production handleError awaits Task.say(), which rejects when the task is + // aborted. A rejected handleError must not skip the diff cleanup: the unapproved + // streamed content has to be reverted and the diff view reset, or a user save + // could persist the failed write. The handleError rejection itself propagates + // (it is not swallowed by the cleanup). As in the missing-parameter tests + // above, the revert mock awaits a deferred so the test proves the cleanup + // AWAITs revertChanges() before reset(): with the revert still pending, + // reset() must not have run yet. + mockHandleError.mockRejectedValue(new Error("handleError rejected (aborted task)")) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + const diffViewCallOrder: string[] = [] + let resolveRevert: () => void = () => {} + const revertDeferred = new Promise((resolve) => { + resolveRevert = resolve + }) + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + await revertDeferred + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + const executePromise = executeWriteFileTool({}, { fileExists: false }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + // handleError was attempted with the write context and the cleanup has reached + // the deferred revert... + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + // ...and while the revert is still pending, reset() must not have run yet. + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + + resolveRevert() + await expect(executePromise).rejects.toThrow("handleError rejected (aborted task)") + + // The unapproved content is reverted before the diff view reset. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + }) + + describe("prevent focus disruption experiment", () => { + /** + * Enable the PREVENT_FOCUS_DISRUPTION experiment for the current task: the experiment + * branches in execute()/handlePartial() read it from the provider state they fetch. + */ + function enablePreventFocusDisruption(): void { + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + } + + it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + expect(toolResult).toBe("Tool result message") + }) + + it("keeps approved diff content when saveDirectly fails after approval", async () => { + // The experiment branch stamps writeApproved before saveDirectly, so a late failure + // must NOT revert the document (the user approved the edit and can save it + // manually) but must still finalize the partial ask and reset the diff view. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + enablePreventFocusDisruption() + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("skips streaming diff view work when the experiment is enabled", async () => { + // With the experiment enabled the tool preview is embedded in the complete message + // built in execute(), so handlePartial must not open or update the diff view while + // streaming. + enablePreventFocusDisruption() + + // Delta 1 - stabilize path; delta 2 - path stabilized but the experiment short-circuits + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("clears the provider state when prevent-focus approval is denied", async () => { + // The prevent-focus branch stamps editType/originalContent on the provider before + // asking. On denial nothing was approved and no diff document was opened, so the + // provider state must be cleared (reset) to make a later write re-check the file + // system instead of reusing the stale editType. The non-prevent-focus denial branch + // resets through revertChanges(); this branch must not call it (no document to + // revert). + enablePreventFocusDisruption() + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockAskApproval).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + }) }) }) From 820a765c4242ed033bf8f06853b25d206d413b8a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:50:29 +0800 Subject: [PATCH 07/17] 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, pinning the ordering (state present during rollback, gone afterwards), plus the error say. Without the fix: 1 failed / 5 passed; with it: 6 passed. eslint clean with --prune-suppressions (no suppression change); package tsc reports no errors in the touched files. --- src/core/tools/WriteToFileTool.ts | 35 ++++++++++++++----- ...teToFileTool-partial-state-cleanup.spec.ts | 35 +++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 653f3c729b..6a218c8bdf 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..1154664e48 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,6 +89,39 @@ 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-6") + 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 t = task as unknown as CleanupTask From 5030d11fd1965c30bea1f277a3728487b2a340fe Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:04:04 +0800 Subject: [PATCH 08/17] test(tools): cover rollback failure with a retained stream error, and keep a failing report non-fatal Same follow-up as #1931's open review thread, kept identical across the chain: - 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 report was awaited inline, so a rejecting task.say would have thrown out of onParameterParseFailure and swallowed the user's only actionable error. It 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 6a218c8bdf..7d81e5f8f2 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 1154664e48..b77fe9a089 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-5") const t = task as unknown as CleanupTask From c5eb9ec5731e77c381d0cd150ba6ebbc4d3b20d2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:16:46 +0800 Subject: [PATCH 09/17] fix(tools): report the rollback hazard from every write_to_file cleanup path Same CodeRabbit Persistence Integrity class as #1931: this unit's cleanup paths call revertDiffChangesBeforeReset() and discard the result, so a failed restore left unapproved content in the editor with no user-visible signal - the error the user sees on those paths is the write failure, not the failed rollback. - revert -> reset -> report is now one helper, cleanupFailedPartialStream(), used by the streaming cleanup. - The execute() finally keeps its approved/unapproved split: after approval the accepted edit stays in the editor; before approval a failed restore is now reported. Test: cleanupFailedPartialStream driven directly with a rejecting revertChanges double - hazard say and provider reset both asserted. Stashing the source change: 1 failed / 8 passed; with it 9 passed. eslint clean with --prune-suppressions (no suppression change); package tsc reports nothing in the touched files. --- src/core/tools/WriteToFileTool.ts | 31 ++++++++++++++++--- ...teToFileTool-partial-state-cleanup.spec.ts | 14 +++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 7d81e5f8f2..241b72a654 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. @@ -427,10 +442,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // restore it so a user save cannot persist it. After approval the content // is the user's accepted edit -- keep it in the editor (dirty) so they can // save it manually. + let reverted = true if (!writeApproved) { - await this.revertDiffChangesBeforeReset(task) + reverted = await this.revertDiffChangesBeforeReset(task) } await this.resetDiffViewAfterWrite(task) + if (!reverted) { + // The restore failed, so the editor still holds content this task never approved. + // The error the user sees on this path is the write failure, not this one. + await this.reportRevertFailure(task) + } } return } finally { @@ -524,9 +545,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 b77fe9a089..2cc0d4d63c 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-5") const t = task as unknown as CleanupTask From 8a9a484028aa5eefa3ab9d4674183c9f44daf321 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 07:35:24 +0800 Subject: [PATCH 10/17] test(tools): drop the as-any casts from the write_to_file harness The harness built nativeArgs with two `as any` casts to smuggle deliberately malformed payloads (a missing or undefined path/content) past the typed args. The object is now built as a plain partial record and asserted once through unknown onto ToolUse<"write_to_file">["nativeArgs"], with a comment stating why the assertion is there. Real suppression decrease, staged as required: core/tools/__tests__/writeToFileTool.spec.ts @typescript-eslint/no-explicit-any 5 -> 3 (edited in place so the file's diff stays one line). Spec behaviour unchanged: 54 passed / 5 skipped; eslint clean with --max-warnings=0; package tsc reports nothing in the file. --- src/core/tools/__tests__/writeToFileTool.spec.ts | 9 ++++++--- src/eslint-suppressions.json | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 7bdd3cbdb2..3056445ee6 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -247,10 +247,13 @@ describe("writeToFileTool", () => { content: testContent, ...params, }, + // The harness builds deliberately malformed payloads (a missing or + // undefined path/content), which the typed args cannot express. One assertion + // through unknown keeps that case testable without introducing `any`. nativeArgs: { - path: (params.path ?? testFilePath) as any, - content: (params.content ?? testContent) as any, - }, + path: params.path ?? testFilePath, + content: params.content ?? testContent, + } as unknown as ToolUse<"write_to_file">["nativeArgs"], partial: isPartial, } diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 24db0bf433..07e8b34c10 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1011,7 +1011,7 @@ }, "core/tools/__tests__/writeToFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 5 + "count": 3 } }, "core/tools/helpers/toolResultFormatting.ts": { From 8b11960b6ad319e49a54ad2bfc400554fd3c2e0d Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:49:45 +0000 Subject: [PATCH 11/17] chore: trigger a fresh review pass at this head From 1dfd76f9b707250ca1135f6532f7c9c7aab91f74 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 20:04:08 +0800 Subject: [PATCH 12/17] fix(tools): release the stream state on every execute() exit handlePartial() creates the per-task stream state and registers the TaskAborted listener, and execute() releases it in the success path, the catch and the approval rejection. The three early returns (missing path, missing content, rooignore denial) returned before any of that, so the entry and its listener 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-exit stream state cleanup') - the rooignore-denial case (state map drops from 1 to 0, off(TaskAborted, ...) called) and the missing-path case driven through execute() directly (the harness backfills default params, so it cannot reach that branch). Pin: removing the three releases fails both. Local: core/tools + writeToFileTool.spec + assistant-message + core/task = 1441 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 | 29 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 241b72a654..460b1bac89 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -261,6 +261,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 cleanup 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 } @@ -269,6 +274,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 cleanup 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 } @@ -278,6 +288,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 cleanup 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 3056445ee6..8abfc29c90 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -749,6 +749,35 @@ describe("writeToFileTool", () => { }) }) + describe("early-exit stream state cleanup", () => { + it("releases the per-task stream state when a rooignore denial returns early", async () => { + // A partial delta creates the per-task state and registers the abort listener; the + // denial then returns before the cleanup, which used to leave both behind for the + // task's lifetime (a retained streamFailed also suppresses later diff previews). + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + 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 () => { + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + 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 66670859811d186658196ffce0b85f60acd02564 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Fri, 9 Oct 2026 13:36:46 +0800 Subject: [PATCH 13/17] 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. Every exit of execute() is already covered on this branch by the finally block, so this is the only path that skipped a teardown. 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 +++++ src/core/tools/__tests__/writeToFileTool.spec.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 460b1bac89..76db0079a4 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -504,6 +504,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 8abfc29c90..ff581c1a37 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -1394,6 +1394,22 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() }) + 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. + enablePreventFocusDisruption() + + 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)) + }) + it("clears the provider state when prevent-focus approval is denied", async () => { // The prevent-focus branch stamps editType/originalContent on the provider before // asking. On denial nothing was approved and no diff document was opened, so the From 443993e57d65b75c8b1a3a2121af7e495104d538 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:55:43 +0800 Subject: [PATCH 14/17] fix(write-to-file): re-check the stream state after every await, and roll an open() failure back Ports the two fixes already accepted on #1930 (U5) and #1929 (U4) instead of re-deriving them: - Persistence Integrity: DiffViewProvider.revertChanges() no longer returns early when activeDiffEditor is missing. open() creates the parent directories and an empty placeholder BEFORE it awaits openDiffEditor(); when that await rejects the rollback used to bail out and left the debris on disk, so the next execute() mistook the requested new file for an existing one and a denial preserved it. The filesystem rollback now runs either way, tolerates ENOENT (removeCreatedFile/removeCreatedDir), surfaces any other failure, and discards an unapproved dirty buffer instead of saving it - a refused discard restores the pre-stream content and reports the rollback as failed. Ported verbatim from 6cae369d9 / 7b783452c. - Lifecycle: handlePartial() now re-checks that this task's stream entry is still the live one (isPartialStreamStillLive, identity not presence) after provider.getState(), after the filesystem probe, after the partial ask, and after diffViewProvider.open(). A cancellation during any of those awaits runs the TaskAborted teardown; without the check the delta already in flight re-asked and re-opened a diff view for a task the user had cancelled, resurrecting the state the teardown released. - Regression Evidence: added the missing-content test (seed the task's stream state, execute() with a valid path and content undefined) asserting the missing-param result, the entry removed, the exact TaskAborted listener deregistered and the diff view reset. Red first: the four liveness tests failed before the checks existed. Negative controls, one-to-one: removing the check after getState() -> only the probe test red; after the probe -> only the ask test red; after the ask -> only the diff-view test red; after open() -> only the update test red; removing the release at the missing-content exit -> only that test red. Verification: writeToFileTool 62/5s, partial-state-cleanup 9, DiffViewProvider 79, Task.spec 172, removeClineFromStack 24 (346 passed); tsc 62 (baseline, none in touched files); eslint 0/0; eslint-suppressions unchanged. --- src/core/tools/WriteToFileTool.ts | 37 +++ .../tools/__tests__/writeToFileTool.spec.ts | 128 +++++++++++ src/integrations/editor/DiffViewProvider.ts | 131 ++++++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 214 +++++++++++++++++- 4 files changed, 499 insertions(+), 11 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index b6c4b1093a..3ec12036d0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -130,6 +130,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 the diff view's open() before it streams content; + * a cancellation during any of those awaits runs the TaskAborted teardown (or a direct + * clearTaskState), which deletes this entry. Continuing would re-ask and re-open a diff view + * for a task the user already cancelled, resurrecting the state the teardown released. + * Identity, not presence: an entry re-created under the same key belongs to a new stream, + * and this delta 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) @@ -496,6 +509,12 @@ 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, so this delta must not go on to ask or open a diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -521,6 +540,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } + // Cancelled during the filesystem probe: stop before the partial ask and the diff view. + // (The probe above is the only await in this branch.) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -535,12 +560,24 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const partialMessage = JSON.stringify(sharedMessageProps) await task.ask("tool", partialMessage, block.partial).catch(() => {}) + // Cancelled while the partial ask was in flight: the teardown released this task's + // state, so this delta must not touch the diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { await task.diffViewProvider.open(relPath!) } + // Cancelled while open() was in flight: do not stream partial content into a + // cancelled task's view. + 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 ff581c1a37..0a0f6c35fd 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -510,6 +510,99 @@ describe("writeToFileTool", () => { expect(mockCline.ask).toHaveBeenCalledTimes(2) }) + 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() + // Clear the cached editType so the filesystem probe branch is the one that would run: + // the check after provider.getState() is the only thing that can keep it from running. + mockCline.diffViewProvider.editType = undefined + mockedFileExistsAtPath.mockClear() + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn(async () => { + writeToFileTool.clearTaskState(mockCline) + return {} + }), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // The probe is the first side effect after getState(): the check above is what keeps it + // from running for a cancelled task. + expect(mockedFileExistsAtPath).not.toHaveBeenCalled() + 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) + }) + 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 @@ -776,6 +869,41 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + + 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() + }) }) 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 19e85126d8a54e64e19abb3fcad7cdf7147edfad Mon Sep 17 00:00:00 2001 From: eason liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 11:20:50 +0800 Subject: [PATCH 15/17] fix(write-to-file): cover the pre-streaming window and discard without saving CodeRabbit Lifecycle Resource Cleanup on #1932: "handlePartial() then awaits provider?.getState() at lines 510-511 outside its local cleanup try. If that await, or a later synchronous operation, rejects, BaseTool.handle() only calls callbacks.handleError() ... it does not clear the map entry or remove the listener", and "if the task is aborted while diffViewProvider.open() is pending and open() then rejects, the abort listener has already cleared the state and task disposal can already start diff cleanup, but the catch still unconditionally finalizes the ask and calls revert/reset cleanup. This duplicates cleanup after cancellation." Everything after the entry is registered now runs inside a boundary that releases it: the pre-streaming window is wrapped, its catch releases this delta's entry (identity-guarded, so a newer stream's entry survives) and rethrows; the streaming catch checks liveness before it marks, finalizes, or restores, and again between the two cleanup awaits, so a cancellation that lands in either gap stops instead of running a second cleanup over a view disposal already owns. This is the mirror image of U4/#1929 61dd05a2c: that unit wrapped the setup work and left open()/update() outside, this one wrapped open()/update() and left provider.getState() outside. The rule is recorded in #41 (https://github.com/Zoo-Code-Org/Zoo-Code/issues/41#issuecomment-6093034261): an await that happens after per-task state is registered belongs inside the boundary that releases it, including awaits that look like read-only setup. CodeRabbit Persistence Integrity on #1932: "revertChanges() now avoids saving activeDiffEditor.document and calls discardFileTab() ... That helper only finds TabInputText tabs (lines 912-920), but openDiffEditor() creates a vscode.diff tab with TabInputTextDiff ... A user can later save the still-open dirty modified-side buffer and recreate the unapproved streamed file." The rollback of an unapproved stream now goes through discardUnapprovedStream(), ported from the earliest unit that defines it (U7/#1928 f8f7ce19a), together with the placeholderPath ownership tracking that keeps the discard from deleting a file the user approved, and with the discard's own suite. It empties or restores the buffer in memory, closes the tab only once it is clean, and then removes the placeholder and the directories this edit created; the target file is never written, so nothing the user did not approve reaches disk. One deliberate change to the ported shape: closeAllDiffViews() now runs AFTER the buffer restore instead of before it. A vscode.diff tab is dirty while its modified side holds the streamed content and closeAllDiffViews() deliberately skips dirty tabs, so calling it first left exactly the tab the row names open over a file the discard then unlinked. The same one-line move has to land in #1928 and #1929 so the primitive stays identical across the chain; it is called out here rather than silently diverging. Out of Scope Changes was answered before this commit: the per-file declaration is in #41 (https://github.com/Zoo-Code-Org/Zoo-Code/issues/41#issuecomment-6093028889) and appended to #1937's body. BaseTool.ts belongs to U3/#1931 (e3c10401f) and ClineProvider.ts to U4/#1929 (d172c95ae) and only appear in this PR's file list because the chain is opened against main; DiffViewProvider.ts and its spec are this unit's own, because the cleanup that U6's finally invokes is that code. Negative controls (Buffer snapshots, sha256 cd36a1095acd4c72 / 39863d8ea2df56c8 / 781972c962cf3882 verified after every mutant): outer catch not releasing -> 1 red; no liveness check at the top of the streaming catch -> 1 red; no liveness check between the two cleanup awaits -> 1 red; rollback routed back through revertChanges -> 16 red; closeAllDiffViews moved back before the restore -> 3 red; placeholder ownership guard removed -> 3 red. Local: sweep (core/tools, integrations/editor, core/task, core/webview) 97 files / 1994 passed / 5 skipped; tsc --noEmit 0 with the local @roo-code/types paths override; eslint . --ext=ts --max-warnings=0 exit 0; eslint-suppressions.json untouched. --- src/core/tools/WriteToFileTool.ts | 161 ++++--- ...teToFileTool-partial-state-cleanup.spec.ts | 20 +- .../tools/__tests__/writeToFileTool.spec.ts | 141 +++++- src/integrations/editor/DiffViewProvider.ts | 147 ++++++ .../editor/__tests__/DiffViewProvider.spec.ts | 448 ++++++++++++++++++ 5 files changed, 831 insertions(+), 86 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 3ec12036d0..ec22fd13c4 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -150,23 +150,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } /** - * Restore the diff editor document to its pre-streaming state and close the view. + * Release the diff view of a write that was never approved, then let the caller reset it. * * 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 + * since reset() clears the state the discard relies on. No-op when no diff view * 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. + * + * This is deliberately not revertChanges(): that method SAVES. For a new file it writes + * the streamed partial output into the placeholder before deleting it, and for an + * existing file it writes the restored original back to a file this stream never + * changed. discardUnapprovedStream() empties or restores the buffer in memory and + * closes the tab only once it is clean, so nothing the user did not approve reaches + * disk and the modified side of the vscode.diff tab - a TabInputTextDiff, which + * discardFileTab() cannot see - is what actually gets discarded. */ - private async revertDiffChangesBeforeReset(task: Task): Promise { + private async discardUnapprovedStreamBeforeReset(task: Task): Promise { try { - await task.diffViewProvider.revertChanges() + await task.diffViewProvider.discardUnapprovedStream() return true - } catch (revertError) { - console.error("Error reverting write_to_file diff view changes:", revertError) + } catch (discardError) { + console.error("Error discarding the unapproved write_to_file diff view:", discardError) return false } } @@ -204,7 +212,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { * cleanup is a different failure and is reported elsewhere. */ private async cleanupFailedPartialStream(task: Task): Promise { - const reverted = await this.revertDiffChangesBeforeReset(task) + const reverted = await this.discardUnapprovedStreamBeforeReset(task) await this.resetDiffViewAfterWrite(task) if (!reverted) { await this.reportRevertFailure(task) @@ -239,7 +247,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // 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) + const reverted = await this.discardUnapprovedStreamBeforeReset(task) this.resetTaskPartialState(task) await this.resetDiffViewAfterWrite(task) if (!reverted) { @@ -470,7 +478,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // save it manually. let reverted = true if (!writeApproved) { - reverted = await this.revertDiffChangesBeforeReset(task) + reverted = await this.discardUnapprovedStreamBeforeReset(task) } await this.resetDiffViewAfterWrite(task) if (!reverted) { @@ -507,65 +515,89 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { 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, so this delta must not go on to ask or open a diff view. - if (!this.isPartialStreamStillLive(task, partialStreamState)) { - return - } + // Everything below runs after getTaskPartialStreamState() registered this task's entry and its + // TaskAborted listener, so it belongs inside a boundary that releases them. The awaits here look + // like reads and are not: provider.getState() is an await into the extension host and the + // filesystem probe can reject. If either does, this delta reaches neither the streaming block + // nor execute()'s teardown, and BaseTool.handle() only reports the error - which would leave + // the singleton holding a task that nothing else ever releases. - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) + // Declared outside the try: the streaming catch below reads it, and a try block's + // own scope is not visible to code that runs after the try. + let partialMessage: string | undefined - 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 - } + try { + 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, so this delta must not go on to ask or open a diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) + return + } - // Cancelled during the filesystem probe: stop before the partial ask and the diff view. - // (The probe above is the only await in this branch.) - if (!this.isPartialStreamStillLive(task, partialStreamState)) { - return - } + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, - } + // Cancelled during the filesystem probe: stop before the partial ask and the diff view. + // (The probe above is the only await in this branch.) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) - // Cancelled while the partial ask was in flight: the teardown released this task's - // state, so this delta must not touch the diff view. - if (!this.isPartialStreamStillLive(task, partialStreamState)) { - return + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath!), + content: newContent || "", + isOutsideWorkspace, + isProtected: isWriteProtected, + } + + partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + + // Cancelled while the partial ask was in flight: the teardown released this task's + // state, so this delta must not touch the diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + } catch (error) { + // Unexpected failure in the pre-streaming window: nothing was streamed and nothing was shown, + // so this is not the retained-streaming-failure case the inner catch below keeps state for. + // Release this delta's entry and its listener before the error reaches BaseTool.handle(), + // then rethrow so the delta is still reported. Identity-guarded: if a cancellation already + // released the entry and a newer stream re-created it under the same key, that entry stays. + if (this.isPartialStreamStillLive(task, partialStreamState)) { + this.resetTaskPartialState(task) + } + throw error } + if (newContent) { try { if (!task.diffViewProvider.isEditing) { @@ -583,6 +615,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { false, ) } catch (error) { + // The task may have been aborted while open() or update() was in flight. The teardown + // already released this entry and the disposal path already owns the diff cleanup, so + // marking, finalizing and restoring here would be a second cleanup of the same stream: + // a second partial-tool row and a second restore racing the first. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + 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 @@ -599,6 +639,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { partialStreamState.streamFailed = true partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) await this.finalizePartialToolAskAfterFailure(task, partialMessage) + + // Finalizing is an await: a cancellation during it has already run the teardown and the + // disposal cleanup, so the restore below must not run a second time over the same view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + // 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 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 2cc0d4d63c..06e557a0ad 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -16,6 +16,7 @@ interface CleanupTask { off: MockedFunction<(...args: unknown[]) => unknown> diffViewProvider: { reset: MockedFunction<() => Promise> + discardUnapprovedStream: MockedFunction<() => Promise> revertChanges: MockedFunction<() => Promise> } finalizePartialToolAsk: MockedFunction<() => Promise> @@ -31,6 +32,7 @@ function buildTask(taskId: string, instanceId: string): Task { diffViewProvider: { reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + discardUnapprovedStream: vi.fn().mockResolvedValue(undefined), }, finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), say: vi.fn().mockResolvedValue(undefined), @@ -78,22 +80,22 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { expect(errorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) }) - it("logs and continues when reverting the diff document fails", async () => { + it("logs and continues when discarding the unapproved diff view 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")) + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - await writeToFileTool["revertDiffChangesBeforeReset"](task) + await writeToFileTool["discardUnapprovedStreamBeforeReset"](task) - expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) + expect(errorSpy).toHaveBeenCalledWith("Error discarding the unapproved write_to_file diff view:", expect.any(Error)) }) - it("reverts while the recovery state exists and reports the hazard when the revert fails", async () => { + it("discards while the recovery state exists and reports the hazard when the discard fails", async () => { const task = buildTask("revert-fails-teardown", "inst-6") const t = task as unknown as CleanupTask let stateSizeDuringRevert = -1 - t.diffViewProvider.revertChanges = vi.fn(async () => { + t.diffViewProvider.discardUnapprovedStream = vi.fn(async () => { // The recovery state must still be present while the rollback runs. stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size throw new Error("revert failed") @@ -125,7 +127,7 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { 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")) + t.diffViewProvider.discardUnapprovedStream = 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") @@ -149,7 +151,7 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { 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.diffViewProvider.discardUnapprovedStream = 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) @@ -174,7 +176,7 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { 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")) + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) await writeToFileTool["cleanupFailedPartialStream"](task) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 0a0f6c35fd..e7a8a13458 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -170,6 +170,7 @@ describe("writeToFileTool", () => { update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + discardUnapprovedStream: vi.fn().mockResolvedValue(undefined), saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", @@ -603,6 +604,106 @@ describe("writeToFileTool", () => { expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) }) + it("releases the per-task stream state when provider state rejects during a partial delta", async () => { + // provider.getState() looks like a read and is an await into the extension host. It runs + // after this task's entry and its TaskAborted listener were registered, and a rejection + // here reaches neither the streaming block nor execute()'s teardown - BaseTool.handle() + // only reports the error. Without the outer boundary the singleton keeps the task, its + // diff-view provider and a listener that nothing else removes. + const stateFailure = Object.assign(new Error("EPERM: could not read the extension state"), { + code: "EPERM", + }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + mockCline.providerRef.deref.mockReturnValue({ getState: vi.fn().mockRejectedValue(stateFailure) }) + // The second delta already streamed and opened the view; only the failing delta matters here. + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockHandleError.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // Releasing the state must not swallow the failure: BaseTool.handle() still reports it. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", stateFailure) + // Nothing was streamed and nothing was shown, so there is no preview left to roll back. + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + }) + + it("does not run a second cleanup when the task aborts while open() is in flight and open() then rejects", async () => { + // openDiffEditor() has command-failure and timeout rejection paths, so a cancellation can + // land while open() is pending and open() can then reject. The TaskAborted teardown already + // released this entry and task disposal already owns the diff cleanup, so the streaming + // catch must stop instead of finalizing the ask and reverting a second time over the same + // view - which would also write the failure mark into an entry that is no longer live. + 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 }) + expect(abortCleanup).toBeInstanceOf(Function) + mockCline.diffViewProvider.open.mockImplementation(async () => { + abortCleanup?.() + throw new Error("command 'vscode.diff' rejected while the task was cancelled") + }) + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + mockHandleError.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The cancellation is not a streaming failure: nothing is retained for the next stream, + // and the delta does not report an error the user did not cause. + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("stops before the diff restore when the task aborts while the failed ask is being finalized", async () => { + // The cleanup has two awaits. A cancellation that lands in the gap between them has + // already run the teardown, and task disposal owns the diff cleanup from that moment, so + // restoring and resetting here would race the disposal over the same view. + 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 }) + expect(abortCleanup).toBeInstanceOf(Function) + mockCline.diffViewProvider.open.mockRejectedValue(new Error("command 'vscode.diff' rejected")) + mockCline.finalizePartialToolAsk.mockImplementation(async () => { + abortCleanup?.() + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // The first cleanup step ran; the second must not. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + 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 @@ -704,7 +805,7 @@ describe("writeToFileTool", () => { // 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.discardUnapprovedStream).toHaveBeenCalledTimes(2) expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) // Per-task state torn down at this boundary: guard cleared, the exact // registered abort listener detached. @@ -765,7 +866,7 @@ describe("writeToFileTool", () => { 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.discardUnapprovedStream).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) // Per-task state torn down: guard cleared, exact listener detached. expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) @@ -971,10 +1072,10 @@ describe("writeToFileTool", () => { mockCline.diffViewProvider.open.mockRejectedValue( Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), ) - // Record the relative order of revertChanges() and reset() (vitest mocks expose + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose // no invocationCallOrder). const diffViewCallOrder: string[] = [] - mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { diffViewCallOrder.push("revert") }) mockCline.diffViewProvider.reset.mockImplementation(async () => { @@ -992,7 +1093,7 @@ describe("writeToFileTool", () => { // 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. + // state discardUnapprovedStream() relies on. expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) @@ -1002,10 +1103,10 @@ describe("writeToFileTool", () => { mockCline.diffViewProvider.update.mockRejectedValue( Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), ) - // Record the relative order of revertChanges() and reset() (vitest mocks expose + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose // no invocationCallOrder). const diffViewCallOrder: string[] = [] - mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { diffViewCallOrder.push("revert") }) mockCline.diffViewProvider.reset.mockImplementation(async () => { @@ -1022,7 +1123,7 @@ describe("writeToFileTool", () => { // 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. + // state discardUnapprovedStream() relies on. expect(diffViewCallOrder).toEqual(["revert", "reset"]) expect(mockHandleError).not.toHaveBeenCalled() }) @@ -1173,10 +1274,10 @@ describe("writeToFileTool", () => { mockedCreateDirectoriesForFile.mockRejectedValue( Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), ) - // Record the relative order of revertChanges() and reset() (vitest mocks expose + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose // no invocationCallOrder). const diffViewCallOrder: string[] = [] - mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { diffViewCallOrder.push("revert") }) mockCline.diffViewProvider.reset.mockImplementation(async () => { @@ -1194,7 +1295,7 @@ describe("writeToFileTool", () => { }) it("continues cleanup when reverting the diff document fails before approval", async () => { - // Pins the .catch arm on revertChanges() in revertDiffChangesBeforeReset(): a failed + // Pins the .catch arm on discardUnapprovedStream() in discardUnapprovedStreamBeforeReset(): a failed // revert (e.g. the diff view was already closed) must only be logged so the // remaining cleanup (diff view reset + per-task state teardown) always completes. const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) @@ -1209,7 +1310,7 @@ describe("writeToFileTool", () => { mockedCreateDirectoriesForFile.mockRejectedValue( Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), ) - mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + mockCline.diffViewProvider.discardUnapprovedStream.mockRejectedValue(new Error("revert failed")) // Stream two deltas so the diff view opens with the unapproved content... await executeWriteFileTool({}, { fileExists: false, isPartial: true }) @@ -1219,7 +1320,7 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error reverting write_to_file diff view changes:", + "Error discarding the unapproved write_to_file diff view:", expect.any(Error), ) // The diff view is still reset and the per-task stream state still torn down. @@ -1260,7 +1361,7 @@ describe("writeToFileTool", () => { // tool ask is closed. expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( "Error finalizing write_to_file partial tool ask:", @@ -1307,7 +1408,7 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( @@ -1331,7 +1432,7 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockHandleError).not.toHaveBeenCalled() expect(consoleErrorSpy).toHaveBeenCalledWith( @@ -1401,7 +1502,7 @@ describe("writeToFileTool", () => { // path closes whichever partial tool ask is open) to dismiss the spinner expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) // The write was never approved, so the diff document is reverted before reset - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) }) it("runs diff cleanup when handleError rejects", async () => { @@ -1411,7 +1512,7 @@ describe("writeToFileTool", () => { // could persist the failed write. The handleError rejection itself propagates // (it is not swallowed by the cleanup). As in the missing-parameter tests // above, the revert mock awaits a deferred so the test proves the cleanup - // AWAITs revertChanges() before reset(): with the revert still pending, + // AWAITs discardUnapprovedStream() before reset(): with the revert still pending, // reset() must not have run yet. mockHandleError.mockRejectedValue(new Error("handleError rejected (aborted task)")) mockedCreateDirectoriesForFile.mockRejectedValue( @@ -1422,7 +1523,7 @@ describe("writeToFileTool", () => { const revertDeferred = new Promise((resolve) => { resolveRevert = resolve }) - mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { diffViewCallOrder.push("revert") await revertDeferred }) @@ -1436,7 +1537,7 @@ describe("writeToFileTool", () => { // handleError was attempted with the write context and the cleanup has reached // the deferred revert... expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) // ...and while the revert is still pending, reset() must not have run yet. expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index ed7f8a3d15..9155529015 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -38,6 +38,14 @@ export class DiffViewProvider { isEditing = false originalContent: string | undefined private createdDirs: string[] = [] + /** + * Absolute path of the empty placeholder open() created for a new-file edit, or undefined + * when no placeholder is outstanding. reset() does NOT clear relPath, and saveDirectly() + * sets relPath for a file that was written with approval, so relPath alone cannot tell a + * caller that a file on disk belongs to the abandoned edit. Only this field justifies an + * unlink. + */ + private placeholderPath: string | undefined 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 @@ -137,6 +145,10 @@ export class DiffViewProvider { // Make sure the file exists before we open it. if (!fileExists) { await fs.writeFile(absolutePath, "") + // From here until the placeholder is removed, THIS edit owns that path. Set after + // the write succeeds, so a failed write does not claim ownership of a file we did + // not create. + this.placeholderPath = absolutePath } // If the file was already open, close it (must happen after showing the @@ -341,6 +353,9 @@ export class DiffViewProvider { } const absolutePath = path.resolve(this.cwd, this.relPath) + // The write below is the approved one: whatever placeholder open() created at this path + // becomes real content, so it is no longer an artifact this edit may remove. + this.placeholderPath = undefined const updatedDocument = this.activeDiffEditor.document const editedContent = updatedDocument.getText() @@ -544,6 +559,134 @@ export class DiffViewProvider { } } + /** + * Release a diff view whose content was never approved: an abandoned partial stream, + * a failed stream, or a write the user was never asked to approve (rooignore denial, + * validation failure). Deliberately NOT the same as revertChanges(), because + * revertChanges() SAVES and neither abandoned case may write to disk: + * + * - create: its new-file branch saves a dirty buffer as-is before deleting the file, + * which writes partial model output the task never approved - and leaves it on disk + * if the delete then fails. + * - modify: its branch restores the original content and saves it. That is a write to + * a file the user never approved, and for a rooignore-denial path a write the + * policy forbids outright. + * + * Here the target file is never written. A create buffer is emptied FIRST, so the + * only bytes that can ever reach the placeholder are none and the tab is clean enough + * to close without a prompt; a modify buffer is restored to the content already on + * disk in memory only, so the file itself is untouched and the tab (now showing the + * original content, still marked dirty) is left for the user rather than force-closed + * over any edits they may have typed into the preview. The placeholder plus the + * directories this edit created are removed either way. + */ + async discardUnapprovedStream(): Promise { + if (!this.relPath) { + return + } + + const absolutePath = path.resolve(this.cwd, this.relPath) + // Snapshot the directories this edit created BEFORE the editor work below, and + // clear the field so nothing else can act on them twice. If an await below + // rejects, the caller runs reset(), which drops relPath/createdDirs - this is + // then the only chance to remove what the abandoned edit left on disk. + const createdDirs = this.createdDirs + this.createdDirs = [] + // Same reason for the placeholder: an await below may reject and the caller then runs + // reset(), which drops this field too. + const placeholderPath = this.placeholderPath + this.placeholderPath = undefined + + let editorFailure: unknown + // open() creates the directories and the empty placeholder BEFORE it assigns + // activeDiffEditor (openDiffEditor() can reject on its 10s timeout or a failed + // vscode.diff call), so an abandoned create can leave artifacts on disk with no + // editor at all. Only the buffer and tab work needs the editor; the artifact + // cleanup below runs either way. + if (this.activeDiffEditor) { + const document = this.activeDiffEditor.document + try { + this.disposeActiveEditorListener() + this.cancelDeferredScroll() + + if (document.isDirty) { + // Restore the buffer to what this edit started from - the empty placeholder for a + // create, the content already on disk for a modify. A failed applyEdit leaves the + // unapproved streamed content in the buffer, and saving then would persist exactly + // what this method exists to discard, so the save is conditional and the failure is + // surfaced to the caller as a rollback hazard. + const edit = new vscode.WorkspaceEdit() + const fullRange = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ) + const restoredContent = this.editType === "modify" ? this.stripAllBOMs(this.originalContent ?? "") : "" + edit.replace(document.uri, fullRange, restoredContent) + const applied = await vscode.workspace.applyEdit(edit) + if (!applied) { + editorFailure = new Error( + `Could not restore the diff editor buffer for ${this.relPath}; it may still hold unapproved content.`, + ) + } else if (this.editType !== "modify") { + // Only the emptied placeholder is ever saved: this edit created that file, and + // closing a dirty tab would prompt. A modify is never saved here - a modify's + // restore is an in-memory revert, and saving it would write to a file the user + // never approved (see the method comment). + await document.save() + } + } + + // Close the diff tabs only now: a vscode.diff tab is dirty while its modified side + // holds the streamed content, and closeAllDiffViews() deliberately skips dirty tabs to + // avoid a save prompt. Closing before the restore above left that tab open over a file + // this method is about to unlink. + await this.closeAllDiffViews() + await this.closeFileTab(absolutePath) + } catch (error) { + // Do NOT stop here: the placeholder and the created directories still have + // to go. The original failure is re-thrown once the artifacts are dealt with, + // so the caller still reports the rollback hazard instead of a silent success. + editorFailure = error + } + } + + let cleanupFailure: unknown + try { + // Only a placeholder THIS edit created may be removed. relPath survives reset(), and + // saveDirectly() sets it for a file that was written with approval, so unlinking + // absolutePath unconditionally can delete content the user approved. + if (placeholderPath) { + await fs.unlink(placeholderPath).catch((error: unknown) => { + // open() creates the placeholder; if it is already gone there is nothing + // left to remove and the discard did its job. + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + }) + } + + // Remove only the directories this edit created, in reverse order. + for (let i = createdDirs.length - 1; i >= 0; i--) { + await fs.rmdir(createdDirs[i]).catch((error: unknown) => { + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + }) + } + } catch (error) { + cleanupFailure = error + console.error("Error removing abandoned write_to_file artifacts:", error) + } + + if (editorFailure) { + throw editorFailure instanceof Error ? editorFailure : new Error(String(editorFailure)) + } + if (cleanupFailure) { + throw cleanupFailure instanceof Error ? cleanupFailure : new Error(String(cleanupFailure)) + } + } + + async revertChanges(): Promise { if (!this.relPath) { return @@ -582,6 +725,9 @@ export class DiffViewProvider { } await this.removeCreatedFile(absolutePath) + // The placeholder this edit owned is gone; a later discard must not delete whatever + // occupies the path now. + this.placeholderPath = undefined // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { @@ -1224,6 +1370,7 @@ export class DiffViewProvider { this.isEditing = false this.originalContent = undefined this.createdDirs = [] + this.placeholderPath = undefined 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 89963acfad..1daeeec704 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -2065,4 +2065,452 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) }) + describe("discardUnapprovedStream method", () => { + const mockTargetPath = `${mockCwd}/mock-target-file.ts` + + const makeAbandonedDocument = (callOrder: string[], isDirty = true) => ({ + isDirty, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: { fsPath: mockTargetPath, path: mockTargetPath }, + save: vi.fn(async () => { + callOrder.push("save") + }), + }) + + const openAbandonedView = (document: unknown, createdDirs: string[], callOrder: string[]) => + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs, + // open() records the placeholder it wrote; the discard may only delete what it owns. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + closeAllDiffViews: vi.fn(async () => { + callOrder.push("closeDiffViews") + }), + closeFileTab: vi.fn(async () => { + callOrder.push("closeFileTab") + }), + }) + + it("never persists the unapproved buffer: blanks it with an empty replacement, then deletes the placeholder", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["applyEdit", "save", "closeDiffViews", "closeFileTab"]) + // The only content the discard may write is an empty buffer, and it must be + // written to THIS document before the save that would otherwise persist the + // partial model output. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + // The replacement covers the whole buffer: the range is built from + // positionAt(0) to positionAt(getText().length), so nothing can survive it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(replacedText).toBe("") + expect(mockWorkspaceEdit.delete).not.toHaveBeenCalled() + // The placeholder for THIS relPath is what gets removed. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + }) + + it("restores a modify buffer in memory and never saves the target file", async () => { + // revertChanges() restores a modify by applyEdit + document.save(). For a stream the + // user never approved that save is a write they never asked for, and for a + // .rooignore-denied path a write the policy forbids outright, so the discard restores + // the buffer and leaves the file on disk untouched. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + Object.assign(diffViewProvider, { + editType: "modify", + originalContent: "original content", + // A modify owns no placeholder: only open() of a create writes one. + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["applyEdit", "closeDiffViews", "closeFileTab"]) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + expect(replacedText).toBe("original content") + // The replacement covers the whole buffer, so no streamed content survives it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(document.save).not.toHaveBeenCalled() + // Nothing is unlinked for a modify: saveChanges() may have written this very file with + // the user's approval, and this edit created no directories. + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("does not save and reports the hazard when the buffer restore fails to apply", async () => { + // applyEdit resolving false leaves the unapproved partial content in the buffer. Saving + // then would persist exactly what this method exists to discard, so the save is skipped + // and the caller is told the rollback did not happen. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow(/could not restore the diff editor buffer/i) + + expect(document.save).not.toHaveBeenCalled() + // The artifacts still have to go: the caller runs reset() next, which drops + // placeholderPath/createdDirs and leaves nothing else able to remove them. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + }) + + it("removes the directories this edit created in reverse order, after the placeholder unlink", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`, `${mockCwd}/mock-dir/nested`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledTimes(createdDirs.length) + // Reverse order, so a parent is never removed before the child inside it, and + // only the directories this edit created - never a pre-existing one. + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => c[0])).toEqual([...createdDirs].reverse()) + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + for (const order of vi.mocked(fs.rmdir).mock.invocationCallOrder) { + expect(order).toBeGreaterThan(unlinkOrder) + } + }) + + it("does not save when the abandoned buffer is already clean", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder, false) + openAbandonedView(document, [], callOrder) + + await diffViewProvider.discardUnapprovedStream() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(document.save).not.toHaveBeenCalled() + expect(mockWorkspaceEdit.replace).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledTimes(1) + }) + + it("still removes the placeholder and the created directories when the editor work rejects, and reports the failure", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) + + // The caller (releaseAbandonedDiffView) treats a throw as "rollback hazard", so + // the failure must still surface - but only after the artifacts are gone, because + // the caller then runs reset(), which drops relPath/createdDirs and leaves + // nothing else able to remove them. + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") + + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + }) + + it("removes the placeholder and created directories even when open() failed before activeDiffEditor was assigned", async () => { + // open() creates the directories (line 130) and the empty placeholder (line 134) + // BEFORE openDiffEditor() assigns activeDiffEditor (line 173). If that call rejects + // - the 10s timeout or a failed vscode.diff - an abandoned create therefore has + // artifacts on disk and no editor, and the early return must not skip the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/mock-dir`], + // open() had already written the placeholder before openDiffEditor() rejected, + // so this edit owns the path. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + // Nothing editor-side ran, and nothing threw: the artifacts are the whole job here. + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + // Bracket access for the private field: the snapshot must be consumed, so a second + // discard cannot try to remove the same directories again. + expect(diffViewProvider["createdDirs"]).toEqual([]) + }) + + it("logs and rejects when the placeholder unlink fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const permissionError = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + vi.mocked(fs.unlink).mockRejectedValueOnce(permissionError) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("EPERM: operation not permitted") + + expect(errorSpy).toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", permissionError) + }) + + it("logs and rejects when removing a created directory fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + const notEmpty = Object.assign(new Error("ENOTEMPTY: directory not empty"), { code: "ENOTEMPTY" }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(notEmpty) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("ENOTEMPTY: directory not empty") + + expect(errorSpy).toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", notEmpty) + }) + + it("tolerates an already-deleted placeholder and directories (ENOENT) without throwing", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + // Two created directories: the tolerance has to hold for every one of them, so the + // rejection is keyed on the path rather than on which call comes first. + const dirA = `${mockCwd}/mock-dir-a` + const dirB = `${mockCwd}/nested/mock-dir-b` + openAbandonedView(document, [dirA, dirB], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const enoent = Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + // A single unlink call in this flow, so the placeholder stays Once-based. + vi.mocked(fs.unlink).mockRejectedValueOnce(enoent) + vi.mocked(fs.rmdir).mockImplementation(async (dir) => { + if (dir === dirA || dir === dirB) { + throw enoent + } + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + // Both directories were attempted; the assertion is on the paths, not the order. + expect(fs.rmdir).toHaveBeenCalledWith(dirA) + expect(fs.rmdir).toHaveBeenCalledWith(dirB) + expect(errorSpy).not.toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", expect.anything()) + + // beforeEach only clears call data, not implementations, so put the default back. + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + }) + + it("reports the editor failure rather than the cleanup failure when both happen", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + vi.spyOn(console, "error").mockImplementation(() => {}) + + // The caller reports a rollback hazard from the thrown error, so the ORIGINAL failure + // is what must surface; the cleanup failure is still logged for the operator. + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") + }) + + it("leaves a file this edit never created a placeholder for on disk", async () => { + // reset() does not clear relPath, and saveDirectly() sets it for an approved write. + // Without ownership tracking the discard would unlink that approved file: an approved + // write_to_file to A, then a new write_to_file whose block exits through a rollback + // path before open() ever ran, still sees relPath === A. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + Object.assign(diffViewProvider, { + relPath: "previously-approved-file.ts", + activeDiffEditor: { document }, + editType: undefined, + createdDirs: [], + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // The buffer is still blanked and the tab closed: the unapproved content is gone + // without touching a file the user approved. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0][2]).toBe("") + }) + + it("removes the placeholder that a new-file open() wrote, driven through the public methods", async () => { + // The ownership lifecycle lives at the public-method layer, so exercise it there: + // open() writes the empty placeholder and records it, and the discard of an abandoned + // create removes exactly that path. + const relPath = "owned-placeholder.ts" + const fsPath = `${mockCwd}/${relPath}` + const mockEditor = { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue(""), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + lineCount: 0, + }, + selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } }, + edit: vi.fn().mockResolvedValue(true), + revealRange: vi.fn(), + } + // Structural double for the mocked editor: the mock only implements the members + // open() touches, so it is routed through unknown rather than any. + const editor = mockEditor as unknown as vscode.TextEditor + vi.mocked(vscode.window).visibleTextEditors = [editor] + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(editor) + vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { + setTimeout(() => callback({ uri: { fsPath, scheme: "file" } } as vscode.TextDocument), 0) + return { dispose: vi.fn() } + }) + vi.mocked(vscode.window.onDidChangeVisibleTextEditors).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.window.onDidChangeTextEditorVisibleRanges).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + diffViewProvider.editType = "create" + + await diffViewProvider.open(relPath) + + // open() created the placeholder, and this edit now owns it. + expect(fs.writeFile).toHaveBeenCalledWith(fsPath, "") + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(fsPath) + }) + + it("does not remove the file once saveChanges() has approved the content", async () => { + // saveChanges() turns the placeholder into approved content and drops this edit's + // claim on it, so a later abandoned cleanup must leave the approved file on disk. + const relPath = "approved-by-save.ts" + const fsPath = `${mockCwd}/${relPath}` + Object.assign(diffViewProvider, { + relPath, + newContent: "approved content", + editType: "create", + createdDirs: [], + placeholderPath: fsPath, + preDiagnostics: [], + activeDiffEditor: { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue("approved content"), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + }, + }, + closeAllDiffViews: vi.fn().mockResolvedValue(undefined), + closeFileTab: vi.fn().mockResolvedValue(undefined), + }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(undefined as never) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.saveChanges(false, 0) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("closes the dirty vscode.diff tab that only TabInputTextDiff matches, before the unlink", async () => { + // The hazard named on #1932: openDiffEditor() opens a vscode.diff tab whose input is a + // TabInputTextDiff, so a rollback helper that looks for TabInputText tabs finds nothing, + // closeAllDiffViews() then skips the tab while it is dirty, and the streamed content + // survives in a buffer one save away from recreating a file the user never approved. + // The provider's own tab helpers are deliberately NOT stubbed here: the tab-input matching + // is what is under test. + const callOrder: string[] = [] + const document = { + isDirty: true, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: makeUri(mockTargetPath), + save: vi.fn(async () => { + callOrder.push("save") + document.isDirty = false + }), + } + const diffTab = { + label: `mock-target-file.ts ${DIFF_VIEW_LABEL_CHANGES}`, + input: Object.assign(new vscode.TabInputTextDiff(makeUri("diff-original"), makeUri(mockTargetPath)), { + original: { scheme: DIFF_VIEW_URI_SCHEME, fsPath: "mock-target-file.ts" }, + modified: { scheme: "file", fsPath: mockTargetPath }, + }), + // The tab is dirty exactly while the modified-side buffer is: the discard has to make + // the buffer clean for the tab to become closeable at all. + get isDirty() { + return document.isDirty + }, + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [diffTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + vi.mocked(vscode.window.tabGroups.close).mockImplementation(async () => { + callOrder.push("closeTab") + return true + }) + vi.mocked(fs.unlink).mockImplementation(async () => { + callOrder.push("unlink") + return undefined + }) + + try { + await diffViewProvider.discardUnapprovedStream() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + // The buffer is emptied before anything is closed, and the only content the discard + // writes is an empty buffer: a later save has nothing left to persist. + expect(callOrder.slice(0, 2)).toEqual(["applyEdit", "save"]) + const [, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedText).toBe("") + // The tab is gone before the file is, so no save can recreate the abandoned file from a + // buffer the user never approved. + expect(callOrder.indexOf("closeTab")).toBeGreaterThan(-1) + expect(callOrder.indexOf("closeTab")).toBeLessThan(callOrder.indexOf("unlink")) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("does nothing when no abandoned view is open", async () => { + Object.assign(diffViewProvider, { relPath: undefined, activeDiffEditor: undefined }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + }) From cda6aea1bb7567096dc3de73dc8bda3b8c6dae2f Mon Sep 17 00:00:00 2001 From: eason liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 11:32:41 +0800 Subject: [PATCH 16/17] style: satisfy the repository prettier gate on the files this PR changes The compile job's first step is pnpm format:check (prettier --check .); it failed at 19e85126d naming exactly these files, all of them inside this PR's own diff: src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts src/core/tools/__tests__/writeToFileTool.spec.ts src/core/tools/WriteToFileTool.ts src/integrations/editor/__tests__/DiffViewProvider.spec.ts src/integrations/editor/DiffViewProvider.ts Formatting only: prettier collapsed signatures and object literals that fit the 120-column print width and re-indented the affected test bodies. No behaviour change. Verified after the change: sweep (core/tools, integrations/editor, core/task, core/webview) 97 files / 1994 passed / 5 skipped, eslint . --ext=ts --max-warnings=0 exit 0, prettier --check clean on all five files, eslint-suppressions.json untouched. --- src/core/tools/WriteToFileTool.ts | 3 +- ...teToFileTool-partial-state-cleanup.spec.ts | 18 ++++++-- .../tools/__tests__/writeToFileTool.spec.ts | 2 - src/integrations/editor/DiffViewProvider.ts | 10 ++-- .../editor/__tests__/DiffViewProvider.spec.ts | 46 +++++++++++-------- 5 files changed, 43 insertions(+), 36 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ec22fd13c4..7a0a72270c 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -577,7 +577,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } - partialMessage = JSON.stringify(sharedMessageProps) + partialMessage = JSON.stringify(sharedMessageProps) await task.ask("tool", partialMessage, block.partial).catch(() => {}) // Cancelled while the partial ask was in flight: the teardown released this task's @@ -597,7 +597,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { throw error } - if (newContent) { try { if (!task.diffViewProvider.isEditing) { diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index 06e557a0ad..df5ade2c48 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -57,7 +57,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 () => { @@ -88,7 +91,10 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { await writeToFileTool["discardUnapprovedStreamBeforeReset"](task) - expect(errorSpy).toHaveBeenCalledWith("Error discarding the unapproved write_to_file diff view:", expect.any(Error)) + expect(errorSpy).toHaveBeenCalledWith( + "Error discarding the unapproved write_to_file diff view:", + expect.any(Error), + ) }) it("discards while the recovery state exists and reports the hazard when the discard fails", async () => { @@ -109,7 +115,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"), ) @@ -137,7 +145,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"), ) @@ -162,7 +170,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 e7a8a13458..39ab950c05 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -1046,8 +1046,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 diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 9155529015..b73914dad0 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -620,7 +620,8 @@ export class DiffViewProvider { document.positionAt(0), document.positionAt(document.getText().length), ) - const restoredContent = this.editType === "modify" ? this.stripAllBOMs(this.originalContent ?? "") : "" + const restoredContent = + this.editType === "modify" ? this.stripAllBOMs(this.originalContent ?? "") : "" edit.replace(document.uri, fullRange, restoredContent) const applied = await vscode.workspace.applyEdit(edit) if (!applied) { @@ -686,7 +687,6 @@ export class DiffViewProvider { } } - async revertChanges(): Promise { if (!this.relPath) { return @@ -733,7 +733,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 @@ -1096,10 +1095,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 1daeeec704..6e1c98b9f8 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1276,7 +1276,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") }) @@ -2077,7 +2079,7 @@ describe("DiffViewProvider", () => { callOrder.push("save") }), }) - + const openAbandonedView = (document: unknown, createdDirs: string[], callOrder: string[]) => Object.assign(diffViewProvider, { relPath: "mock-target-file.ts", @@ -2093,7 +2095,7 @@ describe("DiffViewProvider", () => { callOrder.push("closeFileTab") }), }) - + it("never persists the unapproved buffer: blanks it with an empty replacement, then deletes the placeholder", async () => { const callOrder: string[] = [] const document = makeAbandonedDocument(callOrder) @@ -2102,9 +2104,9 @@ describe("DiffViewProvider", () => { callOrder.push("applyEdit") return true }) - + await diffViewProvider.discardUnapprovedStream() - + expect(callOrder).toEqual(["applyEdit", "save", "closeDiffViews", "closeFileTab"]) // The only content the discard may write is an empty buffer, and it must be // written to THIS document before the save that would otherwise persist the @@ -2170,23 +2172,25 @@ describe("DiffViewProvider", () => { openAbandonedView(document, [], callOrder) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) - await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow(/could not restore the diff editor buffer/i) + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + /could not restore the diff editor buffer/i, + ) expect(document.save).not.toHaveBeenCalled() // The artifacts still have to go: the caller runs reset() next, which drops // placeholderPath/createdDirs and leaves nothing else able to remove them. expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) }) - + it("removes the directories this edit created in reverse order, after the placeholder unlink", async () => { const callOrder: string[] = [] const document = makeAbandonedDocument(callOrder) const createdDirs = [`${mockCwd}/mock-dir`, `${mockCwd}/mock-dir/nested`] openAbandonedView(document, createdDirs, callOrder) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) - + await diffViewProvider.discardUnapprovedStream() - + expect(fs.unlink).toHaveBeenCalledTimes(1) expect(fs.rmdir).toHaveBeenCalledTimes(createdDirs.length) // Reverse order, so a parent is never removed before the child inside it, and @@ -2197,37 +2201,37 @@ describe("DiffViewProvider", () => { expect(order).toBeGreaterThan(unlinkOrder) } }) - + it("does not save when the abandoned buffer is already clean", async () => { const callOrder: string[] = [] const document = makeAbandonedDocument(callOrder, false) openAbandonedView(document, [], callOrder) - + await diffViewProvider.discardUnapprovedStream() - + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() expect(document.save).not.toHaveBeenCalled() expect(mockWorkspaceEdit.replace).not.toHaveBeenCalled() expect(fs.unlink).toHaveBeenCalledTimes(1) }) - + it("still removes the placeholder and the created directories when the editor work rejects, and reports the failure", async () => { const callOrder: string[] = [] const document = makeAbandonedDocument(callOrder) const createdDirs = [`${mockCwd}/mock-dir`] openAbandonedView(document, createdDirs, callOrder) vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) - + // The caller (releaseAbandonedDiffView) treats a throw as "rollback hazard", so // the failure must still surface - but only after the artifacts are gone, because // the caller then runs reset(), which drops relPath/createdDirs and leaves // nothing else able to remove them. await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") - + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) }) - + it("removes the placeholder and created directories even when open() failed before activeDiffEditor was assigned", async () => { // open() creates the directories (line 130) and the empty placeholder (line 134) // BEFORE openDiffEditor() assigns activeDiffEditor (line 173). If that call rejects @@ -2307,7 +2311,10 @@ describe("DiffViewProvider", () => { // Both directories were attempted; the assertion is on the paths, not the order. expect(fs.rmdir).toHaveBeenCalledWith(dirA) expect(fs.rmdir).toHaveBeenCalledWith(dirB) - expect(errorSpy).not.toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", expect.anything()) + expect(errorSpy).not.toHaveBeenCalledWith( + "Error removing abandoned write_to_file artifacts:", + expect.anything(), + ) // beforeEach only clears call data, not implementations, so put the default back. vi.mocked(fs.rmdir).mockResolvedValue(undefined) @@ -2506,11 +2513,10 @@ describe("DiffViewProvider", () => { it("does nothing when no abandoned view is open", async () => { Object.assign(diffViewProvider, { relPath: undefined, activeDiffEditor: undefined }) - + await diffViewProvider.discardUnapprovedStream() - + expect(fs.unlink).not.toHaveBeenCalled() }) }) - }) From 694bed436f9a248332f9349dc52487d34481f719 Mon Sep 17 00:00:00 2001 From: eason liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 10 Oct 2026 13:01:02 +0800 Subject: [PATCH 17/17] fix(diff-view): close tabs on a clean buffer and stand the rollback down after the commit point Three CodeRabbit threads on #1932 plus the two review rows they point at. Data Integrity & Integration (Critical, DiffViewProvider.ts:691): "revertChanges() can delete a file that this edit never created ... reset() (Lines 1213-1241) does not clear relPath ... That is exactly the state that takes the new-file branch: fileExists is false, the if (this.activeDiffEditor) block is skipped, and removeCreatedFile(absolutePath) runs fs.unlink." reset() now clears relPath and newContent, so no path survives its session, and the regression test the thread asked for proves revertChanges() after reset() never reaches fs.unlink. Deliberate deviation, recorded because it is a scope call rather than an oversight: the thread's other half - gating the rollback on an active edit session - is NOT taken here. That guard belongs to #1931's shape of revertChanges() (5c0f21219), which lands before this unit in the merge order; carrying a second shape of the same guard in the last unit is how one method ends up with two. It is filed as a follow-up against #1931's next push, and the tests here record the current behaviour rather than the desired one. Functional Correctness (Minor, writeToFileTool.spec.ts:1344): "Both 'keeps approved diff content' tests assert only revertChanges not called, so they always pass ... A regression that discards the user's approved edit would pass both tests." Both now assert on discardUnapprovedStream(), the method the error path actually calls. Maintainability (Minor, writeToFileTool.spec.ts:957): "Two new early-release tests check off with expect.any(Function). They still pass if the tool removes a different function." Both sites capture the registered listener and assert off with that reference. Maintainability (Trivial, writeToFileTool-partial-state-cleanup.spec.ts:196): "Every rollback test here makes discardUnapprovedStream reject. No test proves that a successful discard stays silent." Added for both teardown boundaries; they kill the return-true and if (!reverted) survivors the mutation advisory named. Ported from the units that land before this one, so the chain converges on one shape: - 5c0f21219 (#1931): discardFileTab() restores the pre-stream buffer and saves it clean BEFORE closing, and close() is called without its second argument - that parameter is preserveFocus, not a force-discard flag, so a dirty tab was silently refused while the rollback kept deleting the file underneath it. saveBufferClean() comes with it. - 2a9bfabfb (#1931): saveChanges() takes an onCommit signal raised at the document save, and execute() stands the rollback down once it fires. saveDirectly() commits when it returns, because performing the write is what it does. - 8084eab2e (#1929): saveChanges() releases placeholderPath only after the approved save lands, so a rejected save leaves the discard something to remove. Security & Privacy (Major, WriteToFileTool.ts:315): "If handlePartial() opened the denied path before the final block, the denial branch clears only task stream state. It does not revert the streamed content or reset the diff view." The rooignore denial now runs the same discard-then-reset teardown as the other boundaries. The two missing-parameter returns are corrected in the reply on that thread - they did already reset - but neither discarded what an earlier delta had streamed, so they run the same helper now. Negative controls (Buffer snapshots, sha256 a1fa0dd263514c5c provider / 49b07146e60b1f4a tool, verified after every mutant): forced close restored -> 2 red; no saveBufferClean -> 1 red; reset() keeping relPath -> 1 red; commit point raised on return -> 1 red; placeholder released before the save -> 1 red; rooignore branch not tearing the view down -> 1 red; either missing-parameter branch back to a bare reset() -> 1 red each; commit point never wired -> 1 red; detach passing a different function -> 9 red; cleanup always reporting -> 1 red; parse-failure teardown always reporting -> 1 red. Two mutants are equivalent and carry documented Stryker directives: dropping writeCommitted after saveDirectly, and shortening the catch guard to !writeApproved - on this unit the approval flag already stands the rollback down on every path the commit point can be reached from. Local: sweep (core/tools, integrations/editor, core/task, core/webview) 97 files / 2002 passed / 5 skipped; tsc --noEmit 0 with the local @roo-code/types paths override; eslint . --ext=ts --max-warnings=0 exit 0; prettier --check clean on all five files and on the current refs/pull/1932/merge (9dcf3d8bb, 0 dirty); eslint-suppressions.json untouched; it( 65 -> 68, 95 -> 98, 9 -> 11, no removals. --- src/core/tools/WriteToFileTool.ts | 44 +++++++-- ...teToFileTool-partial-state-cleanup.spec.ts | 35 +++++++ .../tools/__tests__/writeToFileTool.spec.ts | 92 +++++++++++++++++- src/integrations/editor/DiffViewProvider.ts | 47 +++++++-- .../editor/__tests__/DiffViewProvider.spec.ts | 95 ++++++++++++++++++- 5 files changed, 291 insertions(+), 22 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 7a0a72270c..eebe5ce9ba 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -284,9 +284,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Returning here skips the cleanup 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. + // the call, and tear down the diff view an earlier streaming delta may have opened - + // otherwise its unapproved content sits in the editor until the next write. this.resetTaskPartialState(task) - await task.diffViewProvider.reset() + await this.cleanupFailedPartialStream(task) return } @@ -297,9 +298,9 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Returning here skips the cleanup 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. + // the call, and tear down the diff view an earlier streaming delta may have opened. this.resetTaskPartialState(task) - await task.diffViewProvider.reset() + await this.cleanupFailedPartialStream(task) return } @@ -311,13 +312,23 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // Returning here skips the cleanup 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. + // the call. The denial can arrive after a streaming delta opened the denied path and + // streamed unapproved content into it, so the diff view is torn down here too - the + // write never happens, and a later save of that editor would land it anyway. this.resetTaskPartialState(task) + await this.cleanupFailedPartialStream(task) return } 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 + let fileExists: boolean const absolutePath = path.resolve(task.cwd, relPath) @@ -409,6 +420,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { writeApproved = true 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. + // Stryker disable next-line AssignmentExpression: equivalent on this path - the + // approval flag already stands the rollback down, so removing the assignment changes no + // observable behaviour today. It stays because this branch lands the write without a diff + // view, and the commit point is the fact that says so. + writeCommitted = true } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) @@ -442,7 +460,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { writeApproved = true - 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) { @@ -477,7 +500,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // is the user's accepted edit -- keep it in the editor (dirty) so they can // save it manually. let reverted = true - if (!writeApproved) { + // Two independent stand-downs: the user approved this content, or the write is already + // durable. Either one makes a rollback undo something the user wanted rather than + // cleaning up this call's debris. + // Stryker disable next-line LogicalOperator: writeCommitted is the second, independent + // stand-down - every path that sets it has already set writeApproved, so no test can tell && + // from || here. It stays because it names the durable fact the provider signals at the + // save, not the approval fact, and it is the guard that survives if the approval flag moves. + if (!writeApproved && !writeCommitted) { reverted = await this.discardUnapprovedStreamBeforeReset(task) } await this.resetDiffViewAfterWrite(task) diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts index df5ade2c48..57a9828f84 100644 --- a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -195,6 +195,41 @@ describe("WriteToFileTool per-task partial-state cleanup", () => { errorSpy.mockRestore() }) + it("stays silent when the failed-stream cleanup restored the editor", async () => { + // Every other case here makes the discard reject, so a cleanup that warned after EVERY + // restore still passed. "The editor may hold content you never approved" is only true when + // the restore failed; saying it regardless trains the user to ignore the warning. + const task = buildTask("failed-stream-clean", "inst-10") + const t = task as unknown as CleanupTask + + await writeToFileTool["cleanupFailedPartialStream"](task) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(t.say).not.toHaveBeenCalled() + }) + + it("stays silent when the parse-failure teardown restored the editor", async () => { + // The same contract on the other teardown boundary: a completed restore is not a hazard, + // so nothing is reported and the incidental parse error is left to the caller. + const task = buildTask("parse-failure-clean", "inst-11") + const t = task as unknown as CleanupTask + writeToFileTool["getTaskPartialStreamState"](task) + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.say).not.toHaveBeenCalled() + expect(handleError).not.toHaveBeenCalled() + expect(handled).toBe(false) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + 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 diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 39ab950c05..e4ccc186b9 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -954,7 +954,13 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}, { accessAllowed: false }) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) - expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + // The teardown has to detach the function this tool registered; expect.any(Function) + // also passes when a different callback is removed and the real one keeps firing. + 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 () => { @@ -968,6 +974,12 @@ describe("writeToFileTool", () => { }) expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // An earlier delta may have opened the diff view for this call: the early return has + // to discard its unapproved content before reset() drops the state that discard reads. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], + ) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) @@ -1002,9 +1014,32 @@ describe("writeToFileTool", () => { )?.[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. + // A stream may have opened a diff view for this call; the early return discards + // its unapproved content first, then closes it. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], + ) expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) + + it("discards the streamed preview when a rooignore denial returns early", async () => { + // The denial can arrive after a streaming delta opened the denied path and streamed + // content into it. This branch reported the denial and released the stream state, but + // never touched the diff view: the unapproved content sat in a saveable editor, and the + // provider kept this call's editType/relPath for whatever write came next. + await executeWriteFileTool({}, { isPartial: true }) + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], + ) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) }) describe("user interaction", () => { @@ -1341,7 +1376,31 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() - expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + // The error path restores through discardUnapprovedStream(), not revertChanges(), so the + // discard is the call that would have thrown the approved edit away. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("does not roll back a write whose commit point has already passed", async () => { + // saveChanges() signals the commit point at the document save and then keeps working + // (closing the diff views, tab bookkeeping, diagnostics). A rejection in that tail is a + // failure AFTER a durable write: rolling back here would restore the previous content or + // delete a file the user had just approved being created. + let signalledAtTheSave = false + mockCline.diffViewProvider.saveChanges.mockImplementationOnce(async (...args: unknown[]) => { + const onCommit = args[2] as (() => void) | undefined + signalledAtTheSave = typeof onCommit === "function" + onCommit?.() + throw new Error("closeAllDiffViews rejected") + }) + + await executeWriteFileTool({}, { fileExists: false }) + + // The stand-down only works if execute() actually handed saveChanges() the signal. + expect(signalledAtTheSave).toBe(true) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) @@ -1598,7 +1657,9 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() - expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + // discardUnapprovedStream() is what the error path actually calls; asserting only on + // revertChanges() could never fail. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() expect(mockCline.didEditFile).toBe(false) } finally { @@ -1606,6 +1667,21 @@ describe("writeToFileTool", () => { } }) + it("does not roll back a write that saveDirectly already landed", async () => { + // The prevent-focus-disruption branch never opens a diff view: saveDirectly() performs + // the write itself, so returning means the content is on disk. A failure in what follows + // (tracking the file, building the result) must not run the rollback over a landed write. + enablePreventFocusDisruption() + mockCline.fileContextTracker.trackFileContext.mockRejectedValueOnce(new Error("tracker down")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + it("skips streaming diff view work when the experiment is enabled", async () => { // With the experiment enabled the tool preview is embedded in the complete message // built in execute(), so handlePartial must not open or update the diff view while @@ -1634,7 +1710,13 @@ describe("writeToFileTool", () => { expect(mockCline.ask).not.toHaveBeenCalled() expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) - expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + // Delta 1 registered the listener; delta 2 must detach THAT function, not merely + // some function with the right name. + 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("clears the provider state when prevent-focus approval is denied", async () => { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index b73914dad0..7d6cae3a2a 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -343,6 +343,7 @@ export class DiffViewProvider { async saveChanges( diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + onCommit?: () => void, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -353,9 +354,6 @@ export class DiffViewProvider { } const absolutePath = path.resolve(this.cwd, this.relPath) - // The write below is the approved one: whatever placeholder open() created at this path - // becomes real content, so it is no longer an artifact this edit may remove. - this.placeholderPath = undefined const updatedDocument = this.activeDiffEditor.document const editedContent = updatedDocument.getText() @@ -363,6 +361,18 @@ 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?.() + + // Ownership is released only once the approved write has landed. A save that rejects leaves + // the placeholder as open() created it, and the caller's teardown still has to remove it: + // clearing it here would tell discardUnapprovedStream() that nothing this edit created is + // left to clean, and an empty or half-written new file would survive the failed write. + this.placeholderPath = undefined + // Stop tracking touches and cancel any pending scroll-to-diff before any // programmatic editor activation below. this.disposeActiveEditorListener() @@ -1065,18 +1075,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 }, ) } @@ -1100,6 +1116,20 @@ export class DiffViewProvider { await vscode.workspace.applyEdit(edit) } + /** + * Persist a restored buffer so the tab is clean before it is closed: close() refuses or prompts + * on a dirty tab, and a refused close is how unapproved content outlived a "forced" rollback. + * The content being saved here is the pre-stream content, never the streamed partial. + */ + 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) @@ -1364,6 +1394,11 @@ export class DiffViewProvider { await this.closeAllDiffViews() this.editType = undefined this.isEditing = false + // A session's path must not outlive the session: revertChanges() reads relPath to decide + // what to roll back, and a stale relPath with editType cleared points the next rollback at + // whatever file this provider last touched - a file the current edit never created. + this.relPath = undefined + this.newContent = undefined this.originalContent = undefined this.createdDirs = [] this.placeholderPath = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 6e1c98b9f8..253276b53b 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1266,6 +1266,86 @@ describe("DiffViewProvider", () => { await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") }) + it("revertChanges() after reset() does not unlink a file this edit never created", async () => { + // reset() used to leave relPath behind with editType cleared, which is exactly the state + // that sends the NEXT rollback down the new-file branch: fs.unlink on whatever file this + // provider last touched. A truncated tool call or an EACCES in a later write then deleted + // a file the user had already saved. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + + await diffViewProvider.reset() + await diffViewProvider.revertChanges() + + expect(diffViewProvider["relPath"]).toBeUndefined() + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("saveChanges() signals its commit point at the document save, not on return", async () => { + // Everything after the save - closing the diff views, tab bookkeeping, diagnostics - can + // still reject. A caller that rolls the tool call back on that rejection would undo a + // write that already landed, so it has to be told at the save rather than when the + // method returns. + const order: string[] = [] + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn(async () => { + order.push("save") + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + closeFileTab: vi.fn().mockResolvedValue(undefined), + restorePreviewTabs: vi.fn().mockResolvedValue(undefined), + }) + diffViewProvider["closeAllDiffViews"] = vi.fn(async () => { + order.push("closeDiffViews") + }) + + await diffViewProvider.saveChanges(false, 0, () => { + order.push("commit") + }) + + expect(order).toEqual(["save", "commit", "closeDiffViews"]) + // Ownership follows the same line: the placeholder open() created is only released once + // the approved content is on disk. + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + }) + + it("saveChanges() withholds the commit point and the placeholder when the save rejects", async () => { + // A rejected save means nothing landed. Signalling the commit point anyway would tell + // the caller the rollback has to stand down, and clearing placeholderPath would tell + // discardUnapprovedStream() that nothing this edit created is left to remove - the empty + // or half-written new file would survive the failed write. + const order: string[] = [] + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn(async () => { + order.push("save") + throw new Error("save rejected") + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + placeholderPath: mockTargetPath, + closeFileTab: vi.fn().mockResolvedValue(undefined), + }) + + await expect(diffViewProvider.saveChanges(false, 0, () => order.push("commit"))).rejects.toThrow( + "save rejected", + ) + + expect(order).toEqual(["save"]) + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + }) + 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 @@ -1319,7 +1399,10 @@ describe("DiffViewProvider", () => { } expect(editor.document.save).not.toHaveBeenCalled() - expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab, true) + // No force argument: tabGroups.close()'s second parameter is preserveFocus, not a + // discard flag, so passing true never discarded anything - it only suppressed the + // prompt while the dirty tab was silently refused. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) }) @@ -1335,6 +1418,7 @@ describe("DiffViewProvider", () => { getText: vi.fn().mockReturnValue("streamed content"), isDirty: true, positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + save: vi.fn().mockResolvedValue(undefined), } const dirtyTab = { input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { @@ -1363,7 +1447,7 @@ describe("DiffViewProvider", () => { 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") } finally { if (originalTabs) { Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) @@ -1373,9 +1457,12 @@ describe("DiffViewProvider", () => { } } - // 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. + // The buffer went back to its pre-stream state and was saved clean BEFORE the close, + // so the close never saw a dirty tab; the rollback then stopped rather than deleting the + // file out from under a buffer it could not discard. expect(vscode.workspace.applyEdit).toHaveBeenCalled() + expect(streamedBuffer.save).toHaveBeenCalledTimes(1) + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) // 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