diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index bc5b8a4426..d691cfc59c 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,47 @@ 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, + rollbackFailure: 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/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts new file mode 100644 index 0000000000..8051ee44c0 --- /dev/null +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-write-to-file-stream-cleanup.spec.ts @@ -0,0 +1,346 @@ +// Regression coverage for the malformed-completion path. +// +// presentAssistantMessage emits its own tool_result and returns for a completed known-tool block +// that has no nativeArgs, so tool.handle() is never reached and BaseTool's parse-failure teardown +// - the one this chain added - never runs. These tests drive the presenter itself, which is how a +// malformed streamed write_to_file actually reaches the teardown. + +import { RooCodeEventName } from "@roo-code/types" +import { afterEach, beforeEach, describe, expect, it, vi, type MockedFunction } from "vitest" + +import { type Task } from "../../task/Task" +import { isValidToolName } from "../../tools/validateToolUse" +import { writeToFileTool } from "../../tools/WriteToFileTool" +import { fileExistsAtPath } from "../../../utils/fs" + +import { presentAssistantMessage } from "../presentAssistantMessage" + +vi.mock("../../task/Task") +vi.mock("../../tools/validateToolUse", () => ({ + validateToolUse: vi.fn(), + isValidToolName: vi.fn().mockReturnValue(true), +})) +// Only the filesystem probe and the readable-path helper have to be observable; every other +// export of those modules stays real, so the streaming path runs against the real code. +vi.mock("../../../utils/fs", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + fileExistsAtPath: vi.fn().mockResolvedValue(false), + createDirectoriesForFile: vi.fn().mockResolvedValue([]), + } +}) +vi.mock("../../../utils/path", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getReadablePath: (_cwd: string, relPath: string) => relPath } +}) +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureToolUsage: vi.fn(), + captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), + }, + }, +})) + +interface PushedToolResult { + type: string + tool_use_id: string + content: string + is_error?: boolean +} + +// Structural double: the presenter and the streaming path only read these members. The double +// assertion in asTask() is the repo's existing pattern for presenter-level tests (see +// writeToFileTool-partial-state-cleanup.spec.ts); Task itself needs a live extension host. +interface PresenterTask { + taskId: string + instanceId: string + cwd: string + abort: boolean + abandoned: boolean + presentAssistantMessageLocked: boolean + presentAssistantMessageHasPendingUpdates: boolean + currentStreamingContentIndex: number + currentStreamingDidCheckpoint: boolean + assistantMessageContent: Array> + userMessageContent: PushedToolResult[] + didCompleteReadingStream: boolean + userMessageContentReady: boolean + didRejectTool: boolean + didAlreadyUseTool: boolean + consecutiveMistakeCount: number + api: { getModel: () => { id: string; info: Record } } + getTaskMode: MockedFunction<() => Promise> + recordToolUsage: MockedFunction<(name: string) => void> + recordToolError: MockedFunction<(name: string, text?: string) => void> + toolRepetitionDetector: { check: MockedFunction<() => { allowExecution: boolean }> } + providerRef: { deref: () => { getState: () => Promise> } } + say: MockedFunction<(type: string, text?: string, images?: unknown) => Promise> + ask: MockedFunction<(type: string, text?: string, partial?: boolean) => Promise<{ response: string }>> + once: MockedFunction<(event: string, listener: () => void) => unknown> + off: MockedFunction<(event: string, listener: () => void) => unknown> + finalizePartialToolAsk: MockedFunction<(text?: string) => Promise> + pushToolResultToUserContent: MockedFunction<(result: PushedToolResult) => boolean> + diffViewProvider: { + editType: "modify" | "create" | undefined + isEditing: boolean + open: MockedFunction<(relPath: string) => Promise> + update: MockedFunction<(content: string, single: boolean) => Promise> + reset: MockedFunction<() => Promise> + revertChanges: MockedFunction<() => Promise> + } +} + +const CALL_ID = "toolu_write_to_file_malformed" +const STREAM_FAILURE = "EACCES: permission denied, open 'src/demo.ts'" + +function buildTask(): PresenterTask { + const diffViewProvider: PresenterTask["diffViewProvider"] = { + editType: undefined, + isEditing: false, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + } + // Mirror the real provider: an open edit is what the rollback below is allowed to revert, + // and both revert and reset end the edit. + diffViewProvider.open.mockImplementation(async () => { + diffViewProvider.isEditing = true + }) + diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewProvider.isEditing = false + }) + diffViewProvider.reset.mockImplementation(async () => { + diffViewProvider.isEditing = false + }) + + const task: PresenterTask = { + taskId: "task-1", + instanceId: "instance-1", + cwd: "/mock/cwd", + abort: false, + abandoned: false, + presentAssistantMessageLocked: false, + presentAssistantMessageHasPendingUpdates: false, + currentStreamingContentIndex: 0, + currentStreamingDidCheckpoint: true, + assistantMessageContent: [], + userMessageContent: [], + didCompleteReadingStream: false, + userMessageContentReady: false, + didRejectTool: false, + didAlreadyUseTool: false, + consecutiveMistakeCount: 0, + api: { getModel: () => ({ id: "test-model", info: {} }) }, + getTaskMode: vi.fn().mockResolvedValue("code"), + recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }) }, + providerRef: { deref: () => ({ getState: async () => ({ experiments: {}, customModes: [] }) }) }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + once: vi.fn(), + off: vi.fn(), + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + pushToolResultToUserContent: vi.fn((result: PushedToolResult) => { + task.userMessageContent.push(result) + return true + }), + diffViewProvider, + } + return task +} + +// The double above is the documented shape of every presenter test in this directory. +const asTask = (task: PresenterTask): Task => task as unknown as Task + +async function present(task: PresenterTask, block: Record): Promise { + task.assistantMessageContent = [block] + task.currentStreamingContentIndex = 0 + await presentAssistantMessage(asTask(task)) +} + +const partialDelta = (content: string): Record => ({ + type: "tool_use", + id: CALL_ID, + name: "write_to_file", + params: { path: "src/demo.ts", content }, + partial: true, +}) + +// The completed block the parser could not finalize: no nativeArgs. +const malformedCompletion = (): Record => ({ + type: "tool_use", + id: CALL_ID, + name: "write_to_file", + params: {}, + partial: false, +}) + +const toolResultsFor = (task: PresenterTask): PushedToolResult[] => + task.userMessageContent.filter((block) => block.type === "tool_result" && block.tool_use_id === CALL_ID) + +const errorSays = (task: PresenterTask): unknown[][] => task.say.mock.calls.filter(([type]) => type === "error") + +// The listener this task registered, read from the registration itself rather than any function. +const registeredAbortListener = (task: PresenterTask): unknown => + task.once.mock.calls.find(([event]) => event === RooCodeEventName.TaskAborted)?.[1] + +// Streams a delta whose diff-view update fails: handlePartial marks the stream failed and keeps +// the error for the authoritative report. The path only counts as stable once it has been seen +// twice, so the first delta only registers the entry and the second reaches the diff view. +async function streamFailedDelta(task: PresenterTask): Promise { + task.diffViewProvider.update.mockRejectedValueOnce(new Error(STREAM_FAILURE)) + await present(task, partialDelta("partial content")) + await present(task, partialDelta("partial content")) +} + +describe("presentAssistantMessage - abandoned write_to_file stream", () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(isValidToolName).mockReturnValue(true) + vi.mocked(fileExistsAtPath).mockReset().mockResolvedValue(false) + vi.spyOn(console, "error").mockImplementation(() => {}) + writeToFileTool.resetPartialState() + }) + + afterEach(() => { + writeToFileTool["taskPartialStreamState"].clear() + vi.restoreAllMocks() + }) + + it("releases the abandoned stream state and reports the captured failure once", async () => { + const task = buildTask() + await streamFailedDelta(task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + const abortListener = registeredAbortListener(task) + expect(abortListener).toBeInstanceOf(Function) + // The stream's own rollback already ran; clear it so the counts below only describe the + // completion path. + task.diffViewProvider.revertChanges.mockClear() + task.diffViewProvider.reset.mockClear() + + await present(task, malformedCompletion()) + + // The leak: the entry and its TaskAborted listener outlived the call. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // Exactly one tool_result for this tool_use_id, carrying the failure the user can act on + // rather than the incidental "missing nativeArgs" text. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].is_error).toBe(true) + expect(results[0].content).toContain(STREAM_FAILURE) + expect(results[0].content).not.toContain("missing nativeArgs") + // Counted by channel rather than toHaveBeenCalledWith: a count of matching says cannot + // auto-pass when the error row is emitted more than once. + const says = errorSays(task) + expect(says).toHaveLength(1) + expect(says[0][1]).toContain("Error writing file:") + // The stream had already reverted, so only the reset is left to do. + expect(task.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(task.diffViewProvider.reset).toHaveBeenCalledTimes(1) + }) + + it("restores the diff document an abandoned stream left open", async () => { + const task = buildTask() + writeToFileTool["getTaskPartialStreamState"](asTask(task)) + task.diffViewProvider.isEditing = true + + await present(task, malformedCompletion()) + + // revertChanges() must run before reset(): reset clears the state the rollback reads, and + // a dirty streamed buffer that survives it is saved by the next open() before approval. + expect(task.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.revertChanges.mock.invocationCallOrder[0]).toBeLessThan( + task.diffViewProvider.reset.mock.invocationCallOrder[0], + ) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // No streaming failure was captured, so the malformed-call report stays the actionable one. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain("missing nativeArgs") + expect(errorSays(task)).toHaveLength(0) + }) + + it("keeps the missing-nativeArgs result for a task that never streamed", async () => { + const task = buildTask() + + await present(task, malformedCompletion()) + + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].is_error).toBe(true) + expect(results[0].content).toContain("missing nativeArgs") + expect(errorSays(task)).toHaveLength(0) + // The cleanup must not create state, touch the diff view, or register a listener for a + // task that never streamed a write. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.once).not.toHaveBeenCalled() + expect(task.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(task.diffViewProvider.reset).not.toHaveBeenCalled() + }) + + it("lets the next write in the same task stream its diff preview again", async () => { + const task = buildTask() + await streamFailedDelta(task) + const opensAfterFailure = task.diffViewProvider.open.mock.calls.length + + await present(task, malformedCompletion()) + + // A retained streamFailed mark makes handlePartial() skip its work for every later delta, + // so this task would never show a diff preview again. + await present(task, partialDelta("next write")) + await present(task, partialDelta("next write")) + + expect(task.diffViewProvider.open.mock.calls.length).toBe(opensAfterFailure + 1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + }) + + it("still emits the required tool_result when the error row cannot be saved", async () => { + const task = buildTask() + await streamFailedDelta(task) + task.say.mockRejectedValue(new Error("chat row unavailable")) + + await expect(present(task, malformedCompletion())).resolves.toBeUndefined() + + // A rejected chat row must not strand the turn: the provider still needs exactly one + // tool_result for this tool_use_id, and the block still has to be marked complete. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain(STREAM_FAILURE) + // The lost chat row must not change which failure is reported: the hook still answered + // that its failure replaces the parse error. + expect(results[0].content).not.toContain("missing nativeArgs") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.userMessageContentReady).toBe(true) + }) + + it("keeps the malformed-call error when the rollback itself failed", async () => { + const task = buildTask() + writeToFileTool["getTaskPartialStreamState"](asTask(task)) + task.diffViewProvider.isEditing = true + task.diffViewProvider.revertChanges.mockRejectedValue(new Error("restore refused")) + + await present(task, malformedCompletion()) + + // A refused rollback is a second, distinct failure: the hook reports it and still returns + // false, so the one tool_result has to carry both it and the malformed-call error - the + // model otherwise never learns the call could not be finalized. + const results = toolResultsFor(task) + expect(results).toHaveLength(1) + expect(results[0].content).toContain("write_to_file rollback failed: restore refused") + expect(results[0].content).toContain("missing nativeArgs") + const says = errorSays(task) + expect(says).toHaveLength(1) + expect(says[0][1]).toContain("Error writing file:") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) +}) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 417dcf7a45..f5a3de9379 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -563,12 +563,58 @@ async function presentAssistantMessageBlock(cline: Task): Promise { // Best-effort only } + // This guard returns before tool.handle(), so handle()'s parse-failure teardown + // never runs on the malformed-completion path: the per-task stream entry and its + // TaskAborted listener would outlive the call, a retained streamFailed mark would + // suppress this task's later diff previews, and a diff document the stream opened + // would keep content the user never approved. Release this task's state before + // emitting the result. The guard owns the single tool_result a native tool call + // must produce, so the tool's handleError folds the failure it reports into that + // one result instead of pushing a second one. + const abandonedStreamFailure: { report?: string } = {} + let reportedStreamFailure = false + if (block.name === "write_to_file") { + try { + reportedStreamFailure = await writeToFileTool.releaseStreamStateOnParseFailure(cline, { + handleError: async (action, error) => { + // Store the report before touching the UI: the failure text is the only + // copy of it, and a chat row that cannot be saved must not cost the model + // the tool_result this guard owes the provider. + abandonedStreamFailure.report = `Error ${action}: ${JSON.stringify(serializeError(error))}` + try { + await cline.say("error", `Error ${action}:\n${error.message}`) + } catch (reportError) { + // A chat row that cannot be saved must not cost the model the tool_result + // this guard owes the provider, and it must not change which failure that + // result reports: letting it escape here would also lose the hook's answer + // on whether the parse error is still worth mentioning. + console.error("Error saving abandoned write_to_file error row:", reportError) + } + }, + }) + } catch (teardownError) { + // The teardown protects this call from a leak; it must never be the reason the + // mandatory tool_result and the block-completion bookkeeping below are skipped. + console.error("Error releasing abandoned write_to_file stream state:", teardownError) + } + } + // The hook returns true when the failure it reported replaces the parse error. When it + // reports anyway and still returns false - a refused rollback with no captured streaming + // error is a second, distinct failure - the model must also learn the call could not be + // finalized, so both facts go into the one tool_result. + let resultContent = errorMessage + if (abandonedStreamFailure.report) { + resultContent = reportedStreamFailure + ? abandonedStreamFailure.report + : `${abandonedStreamFailure.report}\n${errorMessage}` + } + // Push tool_result directly without setting didAlreadyUseTool so streaming can // continue gracefully. cline.pushToolResultToUserContent({ type: "tool_result", tool_use_id: sanitizeToolUseId(toolCallId), - content: formatResponse.toolError(errorMessage), + content: formatResponse.toolError(resultContent), is_error: true, }) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..04e8fde764 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -98,6 +98,31 @@ export abstract class BaseTool { this.lastSeenPartialPath = undefined } + /** + * Teardown boundary for a completed tool call that never reaches execute(): the + * parse-failure catch in handle(), and presentAssistantMessage's missing-nativeArgs + * guard, which emits its own tool_result and returns before handle() is reached. + * No-op for tools without per-task state; the scope is a single task because tool + * instances are singletons shared by concurrent tasks. + * + * Default: there is no per-task streaming state to release, so the generic parse + * error is what the user sees. A tool that keeps per-task stream state may release + * it, restore any diff document a stream opened, and report a more specific failure + * through callbacks.handleError - returning true suppresses the incidental parse + * error so the failure is reported exactly once. Per-task only: a global teardown + * would clobber another task that is still streaming through this singleton. + * + * Public because that caller sits outside handle(). Its guard owns the single + * tool_result a native tool call must produce, so it passes a handleError that folds + * the reported failure into that result instead of pushing a second one. + */ + async releaseStreamStateOnParseFailure( + _task: Task, + _callbacks: Pick, + ): Promise { + return false + } + /** * Main entry point for tool execution. * @@ -157,7 +182,14 @@ export abstract class BaseTool { } catch (error) { console.error(`Error parsing parameters:`, error) const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}` - await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + // execute() never runs on this path, so a tool that keeps per-task streaming + // state must still release THIS task's state and restore any diff document the + // stream opened. If a streaming delta already hit a fatal error, the tool reports + // that (the actionable failure) and the incidental parse error is suppressed. + const reportedStreamFailure = await this.releaseStreamStateOnParseFailure(task, callbacks) + if (!reportedStreamFailure) { + await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + } // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. // Do NOT call pushToolResult here to avoid duplicate tool_result payloads. return diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..5ae08f90a2 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -1,7 +1,7 @@ import path from "path" 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" @@ -22,17 +22,269 @@ 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 + /** Set when the rollback after a streaming failure was itself refused: the dirty + * streamed buffer survives reset(), so the next execute() must fail closed - open() + * would save that unapproved buffer before approval. Reported once by execute() (or by + * onParameterParseFailure() when the final block never parses). */ + rollbackFailure: Error | undefined + /** The task that owns this state; target for abort-listener deregistration. */ + task: Task + /** TaskAborted listener that tears this state down; registered once per task. */ + 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, + rollbackFailure: 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. A revert failure is RETURNED rather than dropped: the caller records it as the + * failure this stream produced, so debris left on disk is reported instead of being + * silently continued past. + */ + private async revertDiffChangesBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.revertChanges() + } catch (revertError) { + console.error("Error reverting write_to_file diff view changes:", revertError) + return revertError instanceof Error ? revertError : new Error(String(revertError)) + } + return undefined + } + + /** + * Whether this task can still act on a partial delta. Mirrors the guard Task uses to bail + * out of its own loops, so a delta never does work for a task that has already stopped. + */ + private isStreamCancelled(task: Task): boolean { + return task.abort === true || task.abandoned === true + } + + private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { + await task.finalizePartialToolAsk(text).catch((finalizeError) => { + console.error("Error finalizing write_to_file partial tool ask:", finalizeError) + }) + } + + /** + * Teardown for the handle() parse-failure path, where execute() never runs and its + * cleanup never runs either. + * + * Releases the per-task stream state: otherwise the abort listener leaks for the task's + * lifetime, and a failed streaming delta leaves the streamFailed guard suppressing the + * diff preview of every later write_to_file in this task. Restores the diff document: + * streaming may have opened it with unapproved partial content, and execute()'s error + * cleanup (revert + reset) never fires here, so a user save could persist it. When a + * streaming delta already hit a fatal filesystem error, THAT is the failure the user can + * act on, so it is reported with the same "writing file" context execute()'s catch uses, + * and true is returned to suppress the incidental parse error - the failure surfaces + * exactly once. + * + * Also reached from presentAssistantMessage's missing-nativeArgs guard: that guard + * emits its own tool_result and returns before handle() runs, so this teardown is + * the only thing that can release the entry a stream left behind. Skip it and the + * per-task entry plus its TaskAborted listener outlive the call, the retained + * streamFailed mark suppresses this task's later diff previews, and a diff document + * the stream opened keeps content the user never approved. That guard passes a + * handleError that feeds the failure into the single tool_result it owns. + */ + override async releaseStreamStateOnParseFailure( + task: Task, + callbacks: Pick, + ): Promise { + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (!state) { + return false + } + + this.resetTaskPartialState(task) + // Only an edit in progress may be reverted. DiffViewProvider keeps relPath after a + // completed write, so reverting against that stale target would roll back - and for a + // new file permanently delete - a file this stream never opened. + let rollbackError: Error | undefined + if (task.diffViewProvider.isEditing) { + rollbackError = await this.revertDiffChangesBeforeReset(task) + } + await this.resetDiffViewAfterWrite(task) + + // A failed rollback is the more actionable failure (debris is still on disk), so when a + // streaming error was also captured it takes the report slot and the streaming error is + // kept behind it as the cause. Without a captured streaming error there was no streaming + // failure to describe: the rollback gets its own message and the parse error stays the + // actionable report. These are two distinct failures, so reporting both does not break the + // single-report rule. + if (rollbackError) { + if (state.streamError) { + await callbacks.handleError( + "writing file", + new Error(`write_to_file rollback failed after a streaming error: ${rollbackError.message}`, { + cause: state.streamError, + }), + ) + return true + } + await callbacks.handleError( + "writing file", + new Error(`write_to_file rollback failed: ${rollbackError.message}`, { cause: rollbackError }), + ) + return false + } + + if (state.streamError) { + await callbacks.handleError("writing file", state.streamError) + return true + } + + return false + } + + 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 let newContent = params.content + // Set when this execute() opens its own partial tool ask (diff-view branch), so + // the catch below can finalize it. Undefined on the saveDirectly branch. + let pendingPartialAsk: string | undefined + + // Fail closed on a refused streaming rollback: the dirty streamed buffer survived + // reset(), and open() saves a dirty existing document BEFORE approval - retrying the + // write here would persist content the user never approved. Report the recorded failure + // once and touch no write path (no open/update/save, no directory creation). The state + // is released here, so the parse-failure path - which only runs when execute() never + // did - can never report the same failure a second time. + const streamState = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (streamState?.rollbackFailure) { + this.resetTaskPartialState(task) + await handleError("writing file", streamState.rollbackFailure) + return + } if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") + // No execute() cleanup on this early return: release THIS task's stream state + // (and only this task's) so the abort listener and the streamFailed guard do not + // outlive the call. + this.resetTaskPartialState(task) pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) await task.diffViewProvider.reset() return @@ -41,6 +293,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (newContent === undefined) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") + // No execute() cleanup on this early return: release THIS task's stream state + // (and only this task's) so the abort listener and the streamFailed guard do not + // outlive the call. + this.resetTaskPartialState(task) pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) await task.diffViewProvider.reset() return @@ -49,53 +305,65 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const accessAllowed = task.rooIgnoreController?.validateAccess(relPath) if (!accessAllowed) { - await task.say("rooignore_error", relPath) + try { + await task.say("rooignore_error", relPath) + } finally { + // The release belongs in a finally: task.say() can reject when the task is cancelled + // or disposed mid-ask, and a rejection that skips it leaks this task's stream state and + // its TaskAborted listener. finally keeps this branch the same shape as the sibling + // units, which run their own cleanup before releasing. + this.resetTaskPartialState(task) + } pushToolResult(formatResponse.rooIgnoreError(relPath)) return } const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath) + try { + // The preflight filesystem work sits inside the guarded scope on purpose: a throw from + // fileExistsAtPath / createDirectoriesForFile must be reported like any other write + // failure, and the per-task stream state must still be released. - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath) - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - if (newContent.startsWith("```")) { - newContent = newContent.split("\n").slice(1).join("\n") - } + // Create parent directories early for new files to prevent ENOENT errors + // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } - if (newContent.endsWith("```")) { - newContent = newContent.split("\n").slice(0, -1).join("\n") - } + if (newContent.startsWith("```")) { + newContent = newContent.split("\n").slice(1).join("\n") + } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } + if (newContent.endsWith("```")) { + newContent = newContent.split("\n").slice(0, -1).join("\n") + } - const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + if (!task.api.getModel().id.includes("claude")) { + newContent = unescapeHtmlEntities(newContent) + } - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath), - content: newContent, - isOutsideWorkspace, - isProtected: isWriteProtected, - } + const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" + const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath), + content: newContent, + isOutsideWorkspace, + isProtected: isWriteProtected, + } - try { task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -136,6 +404,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) + pendingPartialAsk = partialMessage await task.ask("tool", partialMessage, true).catch(() => {}) await task.diffViewProvider.open(relPath) } @@ -178,16 +447,32 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + // BaseTool's reset only clears this instance's lastSeenPartialPath; the + // stream state added here is keyed per task. Clearing the whole map from + // BaseTool's reset only clears this instance's lastSeenPartialState; the per-task + // entry is released by the finally below (clearing the whole map from one task's + // execute() would drop another task's streamFailed/streamError while it is still + // streaming). task.processQueuedMessages() return } catch (error) { + // The diff-view branch above may have opened a fresh partial ask for this + // (retried) write. Finalize it before tearing down, or the spinner and + // Save/Reject buttons stay live for a tool call that has already failed. + if (pendingPartialAsk !== undefined) { + await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) + } await handleError("writing file", error as Error) await task.diffViewProvider.reset() - this.resetPartialState() + super.resetPartialState() return + } finally { + // One teardown covers every exit of the guarded scope: success, both approval denials, + // the catch, and any throw from the preflight filesystem work. Idempotent, so the + // explicit releases on the early returns above stay correct. + this.resetTaskPartialState(task) } } @@ -195,62 +480,165 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content - // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath) || newContent === undefined) { + 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 } - const provider = task.providerRef.deref() - const state = await provider?.getState() - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) - - if (isPreventFocusDisruptionEnabled) { + // A task that was aborted or abandoned can no longer reach execute()'s teardown, so this + // delta must not register per-task state at all: the abort listener would outlive a stream + // that never produces another delta, and the entry's failure mark would suppress the diff + // preview of a later write in a task that already moved on. Same two flags Task itself + // bails on, and the same cancellation-aware shape #1929 established for the streamFailed + // guard: check before acquiring state, then re-check after every await before the next + // observable effect. + if (this.isStreamCancelled(task)) { return } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + // 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) - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { + return } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + // Hoisted above the guarded setup: the diff-view catch below finalizes the same partial + // ask, so the message has to stay in scope once the try block ends. + let partialMessage: string | undefined - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + try { + // Everything from here up to the diff view is setup that can fail before + // execute() ever runs; the catch below owns the teardown for that window. + const provider = task.providerRef.deref() + const state = await provider?.getState() + // First await since the state was registered: a cancellation during getState() would + // otherwise continue into the partial ask below. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, + 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 + } + + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) + + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } + // The filesystem probe is another await boundary, and the ask below is the first thing + // the user can see: a task that stopped must not produce it. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } + + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath!), + content: newContent || "", + isOutsideWorkspace, + isProtected: isWriteProtected, + } + + partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + } catch (error) { + // Unexpected failure in the pre-streaming setup (provider state, the filesystem probe, + // policy checks, message construction): this delta never reaches the diff view or + // execute(), so nothing else releases what the registration acquired. Drop this task's + // entry and its TaskAborted listener, then rethrow - BaseTool.handle() still reports the + // error once, and the diff-view failure path above keeps its own single-report handling. + this.resetTaskPartialState(task) + throw error } - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) + // Last await before the diff view: the ask above can be answered (or the task aborted) + // while it is in flight, and opening a preview for a task that already stopped would + // leave a diff view nobody owns. + if (this.isStreamCancelled(task)) { + this.resetTaskPartialState(task) + return + } if (newContent) { - 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)) + // The ask only exists once the setup above assigned its message; before that there + // is nothing to finalize. + if (partialMessage !== undefined) { + await this.finalizePartialToolAskAfterFailure(task, partialMessage) + } + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty). + const rollbackError = await this.revertDiffChangesBeforeReset(task) + if (rollbackError) { + // The rollback did not finish: the placeholder and any created directories are still + // on disk, and the next execute() would treat that debris as an existing file. A + // logged-only revert failure hides exactly that, so the rollback failure becomes the + // failure this stream reports - the original streaming error stays reachable as the + // cause, and the report still happens exactly once (on the parse-failure path). + const rollbackFailure = new Error( + `write_to_file rollback failed after a streaming error: ${rollbackError.message}`, + { cause: partialStreamState.streamError }, + ) + partialStreamState.streamError = rollbackFailure + // Fail closed for the next execute(): reset() cannot close the dirty diff tab the + // refused restore left behind, so a retry would re-open the view and save the + // unapproved streamed content before approval. Record the failure on this task's + // state; execute() reports it once and touches no write path. + partialStreamState.rollbackFailure = rollbackFailure + } + await this.resetDiffViewAfterWrite(task) + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts new file mode 100644 index 0000000000..c428bbf0ec --- /dev/null +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -0,0 +1,251 @@ +// 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" +import { fileExistsAtPath } from "../../../utils/fs" + +// Only the filesystem probe has to be observable; every other export of utils/fs stays real. +vi.mock("../../../utils/fs", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, fileExistsAtPath: vi.fn().mockResolvedValue(false) } +}) + +// The cleanup primitives only read these members, so a structural double is enough; +// the double assertion is the repo's existing pattern for private-method tests +// (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) + // The registration side of the pairing, asserted BEFORE the cleanup runs: the off() + // assertion below only proves the right listener was deregistered if this one pins which + // listener was registered in the first place. + expect((task as unknown as CleanupTask).once).toHaveBeenCalledWith( + RooCodeEventName.TaskAborted, + state.abortCleanup, + ) + + writeToFileTool.clearTaskState(task) + + 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("returns the revert failure so the caller can report the rollback failure", async () => { + const task = buildTask("revert-fails", "inst-4") + const t = task as unknown as CleanupTask + const revertError = new Error("revert failed") + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(revertError) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(writeToFileTool["revertDiffChangesBeforeReset"](task)).resolves.toBe(revertError) + + 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)) + }) +}) +// handlePartial() acquires per-task state and then crosses several awaits before it touches +// anything the user can see. A task that was aborted or abandoned mid-flight must not leave the +// registration behind and must not produce the partial ask or a diff preview. +describe("WriteToFileTool partial-delta cancellation", () => { + // Structural double: handlePartial only reads these members before the diff view. The double + // assertion is the repo's existing pattern for private-method tests. + beforeEach(() => { + // The probe mock comes from a module-level vi.mock factory, so its call history outlives a + // single test unless it is reset here; the boundary assertions count on it. + vi.mocked(fileExistsAtPath).mockReset().mockResolvedValue(false) + }) + + interface StreamTask { + taskId: string + instanceId: string + abort: boolean + abandoned: boolean + cwd: string + once: MockedFunction<(...args: unknown[]) => unknown> + off: MockedFunction<(...args: unknown[]) => unknown> + ask: MockedFunction<(type: string, text: string, partial?: boolean) => Promise> + providerRef: { deref: () => { getState: () => Promise> } } + diffViewProvider: { + editType: "modify" | "create" | undefined + isEditing: boolean + open: MockedFunction<(relPath: string) => Promise> + update: MockedFunction<(content: string, single: boolean) => Promise> + } + } + + const buildStreamTask = (taskId: string, instanceId: string): StreamTask => ({ + taskId, + instanceId, + abort: false, + abandoned: false, + cwd: "/mock/cwd", + once: vi.fn(), + off: vi.fn(), + ask: vi.fn().mockResolvedValue(undefined), + providerRef: { deref: () => ({ getState: async () => ({ experiments: {} }) }) }, + diffViewProvider: { + editType: "create", + isEditing: true, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + }, + }) + + const partialBlock = { + type: "tool_use", + name: "write_to_file", + partial: true, + params: { path: "src/demo.ts", content: "hello world" }, + } satisfies Record as unknown as Parameters<(typeof writeToFileTool)["handlePartial"]>[1] + + // The path only counts as stable once the same path has been seen twice, so a delta that + // should reach the ask has to start from an entry seeded by an earlier delta. + const seedStablePath = (task: Task) => { + const state = writeToFileTool["getTaskPartialStreamState"](task) + state.lastSeenPartialPath = "src/demo.ts" + return state + } + + it("does not register per-task state for a task that already stopped", async () => { + const task = buildStreamTask("cancel-before-register", "inst-c1") + task.abort = true + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.once).not.toHaveBeenCalled() + expect(task.ask).not.toHaveBeenCalled() + expect(task.diffViewProvider.open).not.toHaveBeenCalled() + }) + + it("releases the state and stops before the next boundary when the task aborts during getState()", async () => { + const task = buildStreamTask("cancel-in-getstate", "inst-c2") + // No editType yet, so the very next step after getState() would be the filesystem probe: + // asserting the probe never ran pins THIS boundary. (Leaving editType set would let the + // suppressed-focus branch release the state and return on its own, which proves nothing + // about the check under test.) + task.diffViewProvider.editType = undefined + seedStablePath(task as unknown as Task) + task.providerRef.deref = () => ({ + getState: async () => { + task.abort = true + return { experiments: {} } + }, + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(vi.mocked(fileExistsAtPath)).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + expect(task.ask).not.toHaveBeenCalled() + }) + + it("releases the state and skips the partial ask when the task is abandoned during the file probe", async () => { + const task = buildStreamTask("cancel-in-probe", "inst-c3") + // No editType yet forces the filesystem probe, the await boundary between the setup and the + // first thing the user sees. + task.diffViewProvider.editType = undefined + seedStablePath(task as unknown as Task) + const probe = vi.mocked(fileExistsAtPath) + probe.mockImplementation(async () => { + task.abandoned = true + return false + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(probe).toHaveBeenCalledTimes(1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(task.ask).not.toHaveBeenCalled() + }) + + it("does not open the diff view when the task aborts while the partial ask is in flight", async () => { + const task = buildStreamTask("cancel-in-ask", "inst-c4") + seedStablePath(task as unknown as Task) + task.ask = vi.fn().mockImplementation(async () => { + task.abort = true + return undefined + }) + + await writeToFileTool.handlePartial(task as unknown as Task, partialBlock) + + expect(task.ask).toHaveBeenCalledTimes(1) + expect(task.diffViewProvider.open).not.toHaveBeenCalled() + expect(task.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..af7b2c5071 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" @@ -96,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 @@ -118,6 +132,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 +145,8 @@ describe("writeToFileTool", () => { return content }) + mockCline.taskId = "task-1" + mockCline.instanceId = "instance-1" mockCline.cwd = "/" mockCline.consecutiveMistakeCount = 0 mockCline.didEditFile = false @@ -186,8 +205,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) @@ -287,14 +310,17 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "defers parent directory creation to execute() while streaming", async () => { - // First call - path not yet stabilized + // Streaming deltas must not touch the filesystem at all. An unguarded + // createDirectoriesForFile here threw EROFS up into BaseTool.handle(), which never + // set didRejectTool/didAlreadyUseTool, so the agent loop stalled permanently. + // The directories are still created - by the authoritative non-partial execute(). + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, ) @@ -419,6 +445,576 @@ 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 + 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() + }) + + 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() + } + }) + + it("releases the per-task stream state when provider state rejects during a partial delta", async () => { + // handlePartial() registers the entry and the TaskAborted listener, then awaits + // provider.getState(). A rejection there never reaches the diff view or execute(), so + // nothing else released what the registration acquired. The error still has to surface, + // so the boundary rethrows and BaseTool.handle() reports it once. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockRejectedValue(new Error("provider state unavailable")), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + expect(mockHandleError).toHaveBeenCalledWith( + "handling partial write_to_file", + expect.objectContaining({ message: "provider state unavailable" }), + ) + }) + }) + + describe("path stabilization predicate", () => { + // 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, + rollbackFailure: 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("parse-failure reporting and early-return cleanup", () => { + it("reports the captured streaming failure once instead of the incidental parse error", async () => { + // A streaming delta hit a fatal filesystem error and the finalized block then + // arrives without nativeArgs: execute() never runs, so its authoritative retry of + // the same filesystem operation never happens either. The captured error is the one + // the user can act on, and it must surface exactly once. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + // The stream opened a diff view: that is what makes the rollback meaningful. + mockCline.diffViewProvider.isEditing = true + state.streamFailed = true + const streamFailure = new Error("EACCES: stream open failed") + state.streamError = streamFailure + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", streamFailure) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The stream may have left a diff view open with content that was never approved. + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + it("keeps the generic parse error when no per-task stream state exists", async () => { + // BaseTool.handle() delegates every missing-nativeArgs error to + // releaseStreamStateOnParseFailure() before reporting the parse error. With no prior + // partial delta there is no state to release: the hook must return false so the + // ordinary parse error still reaches the user - suppressing it here would silence + // every malformed write_to_file call in a task that never streamed. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + // No nativeArgs: this drives BaseTool's parse-failure path. + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Counted by context rather than toHaveBeenCalledWith: a count of matching calls + // cannot auto-pass when handleError fires more than once. + const parseCalls = mockHandleError.mock.calls.filter( + ([context]) => context === "parsing write_to_file args", + ) + const writeCalls = mockHandleError.mock.calls.filter(([context]) => context === "writing file") + expect(parseCalls).toHaveLength(1) + expect(writeCalls).toHaveLength(0) + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(parseCalls[0][1]).toBeInstanceOf(Error) + // The no-state branch returns before touching the diff view or registering anything. + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when execute() returns early on a denied path", async () => { + // The rooignore branch returns before execute()'s success/catch cleanup; without + // this the abort listener and the streamFailed guard outlive the call and suppress + // the diff preview of every later write_to_file in this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the rooignore ask itself rejects", async () => { + // task.say() can reject when the task is cancelled or disposed mid-ask. The release sits + // in a finally, so the stream state and its TaskAborted listener still go away even though + // the ask threw; without it the streamFailed guard suppresses every later diff preview in + // this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockCline.say = vi.fn().mockRejectedValue(new Error("task cancelled during the rooignore ask")) + + await expect(executeWriteFileTool({}, { accessAllowed: false })).rejects.toThrow( + "task cancelled during the rooignore ask", + ) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // The ask threw, so the denial result was never pushed: the release cannot depend on it. + expect(mockPushToolResult).not.toHaveBeenCalled() + }) + + it("does not revert the diff view when the parse-failure teardown has no edit in progress", async () => { + // One unstabilized delta registers this task's stream state without ever opening a diff + // view; the finalized block then arrives without nativeArgs. DiffViewProvider keeps the + // PREVIOUS edit's relPath, so reverting here would roll back - and for a new file delete - + // a file this write never opened. The teardown still resets the view and releases state. + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.isEditing = false + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + // No nativeArgs: this drives BaseTool's parse-failure path. + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when the path is missing", async () => { + // The missing-path return sits before execute()'s guarded scope, so it needs its own + // release. The state is seeded first so the assertion proves a release happened rather + // than an empty map. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { content: testContent }, + nativeArgs: { path: "", content: testContent }, + // An empty path is what the streaming parser produces for a not-yet-complete + // argument object; the typed params cannot express it - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when content is missing", async () => { + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the + // typed params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + }) + describe("per-task stream state isolation", () => { + // A second task streaming through the same singleton while mockCline runs. + // Structural double, same pattern as the partial-state-cleanup spec. + function buildStreamingTask(taskId: string, instanceId: string) { + return { + taskId, + instanceId, + once: vi.fn(), + off: vi.fn(), + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + } + } + + it("leaves another task's stream state intact when execute() completes", async () => { + const other = buildStreamingTask("task-2", "instance-2") + const otherState = writeToFileTool["getTaskPartialStreamState"](other as never) + otherState.streamFailed = true + otherState.streamError = new Error("other task stream failure") + + await executeWriteFileTool({}) + + // The other task is still streaming: its failure state must survive, or its + // next delta re-opens the diff view and spawns a duplicate partial ask. + const retained = writeToFileTool["taskPartialStreamState"].get("task-2.instance-2") + expect(retained).toBeDefined() + expect(retained?.streamFailed).toBe(true) + expect(retained?.streamError?.message).toBe("other task stream failure") + expect(other.off).not.toHaveBeenCalled() + }) + + it("finalizes the partial ask when the write itself fails", async () => { + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + + // The diff-view branch opened a partial ask for this write; without the + // finalize the spinner and Save/Reject stay live after the failure. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + }) + + it("releases this task's stream state when the completed block fails to parse", async () => { + // A streaming delta had failed, so the guard is set; the final block then + // arrives without nativeArgs, so execute() never runs. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + const other = buildStreamingTask("task-2", "instance-2") + writeToFileTool["getTaskPartialStreamState"](other as never).streamFailed = true + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // Otherwise the retained streamFailed suppresses the diff preview of every + // later write_to_file in this task. + expect(writeToFileTool["taskPartialStreamState"].has(`${mockCline.taskId}.${mockCline.instanceId}`)).toBe( + false, + ) + // ...and the cleanup must stay scoped: the other task is still streaming. + expect(writeToFileTool["taskPartialStreamState"].get("task-2.instance-2")?.streamFailed).toBe(true) + }) + it("releases this task's stream state when prevent-focus-disruption skips the partial preview", async () => { + // handlePartial() registers the per-task entry (and its TaskAborted listener) before it + // checks the experiment. With the experiment on, the delta returns without ever opening a + // preview and never reaches execute()'s teardown, so the entry and the listener would stay + // attached for the rest of the task's life - and a streamFailed mark armed by an earlier + // delta would keep suppressing this task's later diff previews. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + const delta = (content: string) => + writeToFileTool.handle( + mockCline, + { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content }, + nativeArgs: { path: testFilePath, content }, + partial: true, + } as ToolUse<"write_to_file">, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }, + ) + + // The first delta only pins the path; the second is the one that reaches the check. + await delta("Line 1") + await delta("Line 1\nLine 2") + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the user rejects the diff-view approval", async () => { + // The diff-view denial returns from inside execute()'s try. Without a release on that + // path the entry and its TaskAborted listener survive a rejected write, and a + // streamFailed mark armed by an earlier delta keeps suppressing this task's later + // diff previews. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockAskApproval.mockResolvedValueOnce(false) + + await executeWriteFileTool({}) + + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the prevent-focus-disruption approval is rejected", async () => { + // Same leak on the experiment branch: the denial returns without a teardown, so the + // listener stays attached for the rest of the task's life. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockAskApproval.mockResolvedValueOnce(false) + + await executeWriteFileTool({}) + + expect(mockAskApproval).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched + // off() argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the preflight directory creation throws", async () => { + // The preflight filesystem work sits before execute()'s guarded scope today: when it + // throws, nothing reports the failure and the stream state leaks. It has to be handled + // like any other write failure - reported once, teardown run. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockedCreateDirectoriesForFile.mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied, mkdir '/new-parent'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) }) describe("user interaction", () => { @@ -460,16 +1056,366 @@ 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(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("keeps a failed rollback as the stream failure instead of dropping it", async () => { + // Streaming failed (open() rejected) and the rollback that follows failed too: the + // placeholder and any created directories are still on disk. Logging the revert failure + // and dropping it leaves the next execute() to treat that debris as an existing file, so + // the rollback failure has to become the failure this stream reports. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue( + Object.assign(new Error("EACCES: rollback failed"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches open(). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + expect(state?.streamFailed).toBe(true) + expect(state?.streamError?.message).toContain("rollback failed") + // The original streaming error must stay reachable behind the reported one. + expect((state?.streamError?.cause as Error | undefined)?.message).toBe( + "EACCES: permission denied, open '/ro/test.py'", + ) + }) + + it("fails closed without retrying any write path when the streaming rollback was refused", async () => { + // update() fails mid-stream and the rollback that follows is refused: the dirty + // streamed buffer survives reset(), which cannot close a dirty diff tab. The next + // execute() would call open() again, and open() saves a dirty existing document + // BEFORE approval - persisting content the user never approved. execute() must fail + // closed on the recorded rollback failure instead of retrying. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue( + Object.assign(new Error("EACCES: rollback failed"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches update() and hits the refused rollback. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const recorded = state?.rollbackFailure + expect(recorded).toBeInstanceOf(Error) + expect(recorded?.message).toContain("rollback failed") + // The original streaming error stays reachable behind the reported failure. + expect((recorded?.cause as Error | undefined)?.message).toBe( + "EROFS: read-only file system, write '/ro/test.py'", + ) + + // The valid final block arrives: execute() must not touch any write path. + await executeWriteFileTool({}) + + // Only the delta's own open()/update() ran; execute() never re-opened the diff view, + // so the dirty buffer could not be saved and no file write or directory creation + // happened - and no approval was even requested. + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockAskApproval).not.toHaveBeenCalled() + expect(mockPushToolResult).not.toHaveBeenCalled() + + // The recorded failure is the single report (counted, not toHaveBeenCalledWith), + // reported with its identity, and the per-task state is released. + const writeCalls = mockHandleError.mock.calls.filter(([context]) => context === "writing file") + expect(writeCalls).toHaveLength(1) + expect(writeCalls[0][1]).toBe(recorded) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports only the execute() failure when the write is retried after a failed stream", async () => { + // Combined path: a streaming delta failed (the failure is captured, not reported), then + // the completed block arrives with valid nativeArgs, so execute() runs and retries the + // same operation. Its failure is the single failure the user hears about - the captured + // streaming error must not also surface, or the same write reports twice. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + // First delta pins the path, second reaches open() and captures the failure. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + expect(state?.streamFailed).toBe(true) + expect(state?.streamError?.message).toBe("EACCES: permission denied, open '/ro/test.py'") + + // The completed block: open() works now, the write itself fails. + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("EROFS: read-only file system, write")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith( + "writing file", + expect.objectContaining({ message: "EROFS: read-only file system, write" }), + ) + expect(mockHandleError).not.toHaveBeenCalledWith( + "writing file", + expect.objectContaining({ message: "EACCES: permission denied, open '/ro/test.py'" }), + ) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports the rollback failure once when the parse-failure cleanup cannot revert the diff", async () => { + // A streaming delta captured a fatal error; the finalized block then fails to parse, so + // the parse-failure cleanup runs - and its revertChanges() fails too. The rollback + // failure is the actionable one: it must be reported exactly once, with the captured + // streaming error kept as its cause, and the incidental parse error must stay silent. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: stream open failed")) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const captured = state?.streamError + expect(captured?.message).toBe("EACCES: stream open failed") + + // The stream had a diff view open - that is the state a rollback is for. + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("EACCES: rollback failed")) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + const [context, reported] = mockHandleError.mock.calls[0] + expect(context).toBe("writing file") + expect(reported.message).toContain("rollback failed") + expect(reported.cause).toBe(captured) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports the captured streaming error once when two real deltas fail and the rollback succeeds", async () => { + // The capture-to-report path end to end: two real partial deltas with open() rejecting, + // then the completed block without nativeArgs so execute() never runs and + // onParameterParseFailure() is the only reporter. The rollback succeeds, so the captured + // streaming error - not the incidental parse error - must reach the user exactly once. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: stream open failed")) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + const state = writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`) + const captured = state?.streamError + expect(captured?.message).toBe("EACCES: stream open failed") + + // The stream left a diff view open; revertChanges() keeps its resolved default, so the + // rollback succeeds and nothing re-stamps the captured error. + mockCline.diffViewProvider.isEditing = true + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + // Identity, not just text: the report must be the original streaming error object. + expect(mockHandleError.mock.calls[0][0]).toBe("writing file") + expect(mockHandleError.mock.calls[0][1]).toBe(captured) + expect(mockHandleError).not.toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports a rollback failure under its own message and keeps the parse error when no stream failed", async () => { + // The diff view opened without any streaming failure; the completed block then fails to + // parse and the rollback itself fails. Reporting "after a streaming error" would describe + // a failure that never happened and would swallow the malformed tool call, so the rollback + // reports under its own message and the parse error is still delivered. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("EACCES: rollback failed")) + + const block = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + } as ToolUse<"write_to_file"> + await writeToFileTool.handle(mockCline, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledTimes(2) + const [context, reported] = mockHandleError.mock.calls[0] + expect(context).toBe("writing file") + expect(reported.message).toBe("write_to_file rollback failed: EACCES: rollback failed") + expect(reported.message).not.toContain("streaming error") + expect(reported.cause).toBeInstanceOf(Error) + // The parse error is the actionable report for the model: it must still be delivered. + expect(mockHandleError.mock.calls[1][0]).toBe("parsing write_to_file args") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + 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 }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + + // 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() }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 60a92eebaa..17d411660a 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, @@ -639,6 +640,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) { diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index e4b15aa27e..5335a304eb 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1166,7 +1166,7 @@ }, "integrations/editor/__tests__/DiffViewProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 310 + "count": 309 } }, "integrations/editor/__tests__/EditorUtils.spec.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..5fe2d9ef1c 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,43 @@ 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) { + // Both guards are required. relPath survives a completed edit, so without isEditing a later + // teardown would take the new-file branch for the PREVIOUS edit's target and unlink an + // existing user file - permanently. isEditing alone is not enough: open() sets it before the + // first await, and that is exactly the state a failed open() leaves behind, where the + // placeholder and the created directories still have to be rolled back. + if (!this.relPath || !this.isEditing) { return } 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,36 +563,56 @@ 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 { - // Revert document. - const edit = new vscode.WorkspaceEdit() - - const fullRange = new vscode.Range( - updatedDocument.positionAt(0), - updatedDocument.positionAt(updatedDocument.getText().length), - ) - - edit.replace(updatedDocument.uri, fullRange, this.stripAllBOMs(this.originalContent ?? "")) + // Only reachable after a successful open(), so the editor exists. + const updatedDocument = this.activeDiffEditor?.document + if (!updatedDocument) { + return + } - // Apply the edit and save, since contents shouldn't have changed - // this won't show in local history unless of course the user made + // Revert the document through the same contract the new-file rollback uses, so a + // refused restore cannot be followed by a save here either. The document is passed + // explicitly: this branch reached it through activeDiffEditor, and resolving it again + // by path could silently pick a different buffer - or none, which would skip the + // restore entirely. + // + // Applying and saving the restore does not show in local history unless the user made // changes and saved during the edit. - await vscode.workspace.applyEdit(edit) - await updatedDocument.save() + await this.restorePreStreamBuffer( + absolutePath, + updatedDocument, + this.stripAllBOMs(this.originalContent ?? ""), + ) + await this.saveBufferClean(absolutePath, updatedDocument) await this.closeAllDiffViews() @@ -847,6 +902,121 @@ 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), + ) + + // Restore and save once, before the loop and independent of it. The document can be open + // only through the diff editor - the user may have closed the plain text tab, leaving + // `tabs` empty - and in that shape the loop never ran, so the streamed buffer stayed + // dirty: closeAllDiffViews() skips dirty tabs and removeCreatedFile() then unlinks the + // file under an unsaved buffer that a later save recreates with the content this rollback + // was meant to discard. Hoisting also stops a multi-tab path from restoring and saving the + // same document once per tab. + await this.restorePreStreamBuffer(absolutePath) + await this.saveBufferClean(absolutePath) + + for (const tab of tabs) { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a + // dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. The buffer was restored and saved clean above, so close() never sees + // a dirty tab and nothing unapproved reaches disk. + let closed: boolean + let closeError: Error | undefined + try { + closed = await vscode.window.tabGroups.close(tab) + } catch (error) { + closed = false + closeError = error instanceof Error ? error : new Error(String(error)) + } + if (!closed) { + throw new Error( + `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 }, + ) + } + } + } + + /** + * Replace an open buffer's content with what it held before streaming started. Runs before + * the rollback closes the tab, so an unapproved buffer is never handed to close() dirty and + * never survives a close that the editor vetoes. + * + * A refused WorkspaceEdit is a failed restore, not a no-op: the buffer still holds content the + * user never approved. Throwing is this method's own contract rather than a caller policy - + * the save that follows cannot run, so nothing saveable survives, and the caller reports the + * rollback as failed instead of telling the user the write was rolled back with unapproved + * content still one save from disk. Returning quietly would be worse than returning false: + * it would swallow the failure. + */ + private async restorePreStreamBuffer( + absolutePath: string, + document?: vscode.TextDocument, + content?: string, + ): Promise { + const target = + document ?? + vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!target) { + return + } + const edit = new vscode.WorkspaceEdit() + const range = new vscode.Range(target.positionAt(0), target.positionAt(target.getText().length)) + edit.replace(target.uri, range, content ?? this.originalContent ?? "") + const restored = await vscode.workspace.applyEdit(edit) + if (!restored) { + throw new Error( + `Rollback could not restore the streamed buffer for ${absolutePath}; the editor refused the restore, so the unapproved content is still in the buffer and unsaved.`, + ) + } + } + + /** + * Save a restored buffer so it is clean when the rollback closes it. close() has no + * force-discard parameter, so a dirty tab would prompt or be refused; saving the restored + * content is what makes the close unconditional. For a new file the placeholder is unlinked + * a moment later, so the saved content is transient by design. + * + * Only ever reached after a successful restore: a refused restore throws above, because a + * buffer whose restore failed still holds the unapproved content this rollback discards and + * must never be saved. + */ + private async saveBufferClean(absolutePath: string, document?: vscode.TextDocument): Promise { + const target = + document ?? + vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!target?.isDirty) { + return + } + const saved = await target.save() + if (!saved) { + throw new Error( + `Rollback could not save the restored buffer for ${absolutePath}; the editor did not persist the restored content, so the buffer is still dirty and one save away from holding what this rollback undid.`, + ) + } + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) @@ -1109,6 +1279,7 @@ export class DiffViewProvider { this.cancelDeferredScroll() await this.closeAllDiffViews() + this.relPath = undefined this.editType = undefined this.isEditing = false this.originalContent = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..2825eb7317 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 @@ -36,7 +41,7 @@ vi.mock("vscode", () => ({ onDidOpenTextDocument: vi.fn(() => ({ dispose: vi.fn() })), openTextDocument: vi.fn().mockResolvedValue({ isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }), textDocuments: [], fs: { @@ -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: [], @@ -169,7 +174,7 @@ describe("DiffViewProvider", () => { diffViewProvider = new DiffViewProvider(mockCwd, mockTask) // Mock the necessary properties and methods - ;(diffViewProvider as any).relPath = "test.txt" + diffViewProvider["relPath"] = "test.txt" ;(diffViewProvider as any).activeDiffEditor = { document: { uri: { fsPath: `${mockCwd}/test.txt` }, @@ -870,7 +875,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("new content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } ;(diffViewProvider as any).preDiagnostics = [] @@ -1015,7 +1020,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } @@ -1040,7 +1045,7 @@ describe("DiffViewProvider", () => { document: { getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), }, } @@ -1060,7 +1065,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: `${mockCwd}/race.ts`, scheme: "file" }, getText: vi.fn().mockReturnValue("a\nCHANGED\nc\nd\n"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), lineCount: 5, lineAt: vi.fn().mockReturnValue({ text: "" }), }, @@ -1099,6 +1104,9 @@ describe("DiffViewProvider", () => { vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockSavedEditor as any) ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + // Self-contained: revertChanges() only acts while an edit is in progress. + diffViewProvider["relPath"] = "test.txt" + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = true ;(diffViewProvider as any).preEditScrollLine = 15 ;(diffViewProvider as any).editType = "modify" @@ -1108,7 +1116,7 @@ describe("DiffViewProvider", () => { uri: { fsPath: `${mockCwd}/test.txt` }, getText: vi.fn().mockReturnValue("modified"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, } @@ -1129,10 +1137,10 @@ describe("DiffViewProvider", () => { const buildActiveDiffEditor = () => ({ document: { - uri: { fsPath: mockTargetPath }, + uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, }) @@ -1172,12 +1180,633 @@ 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, { + isEditing: true, + 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() leaves an earlier edit's file alone when no edit is in progress", async () => { + // reset() used to leave relPath behind while clearing editType. A later stream that never + // opened a diff view then reached this method with a stale relPath and no editType, took + // the new-file branch, and unlinked the PREVIOUS edit's target - an existing user file, + // permanently. Nothing may be deleted unless an edit is actually in progress. + Object.assign(diffViewProvider, { + isEditing: false, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: undefined, + createdDirs: [`${mockCwd}/new-parent`], + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("reset() clears relPath so a later teardown cannot reuse a stale target", async () => { + Object.assign(diffViewProvider, { isEditing: true, relPath: "mock-target-file.ts" }) + + await diffViewProvider.reset() + + expect(diffViewProvider["relPath"]).toBeUndefined() + }) + + it("revertChanges() tolerates a placeholder that was never written", async () => { + // The failed open may have died before fs.writeFile ran; ENOENT during the + // rollback is success, not a new failure that would abort the cleanup. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + 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, { + isEditing: true, + 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, { + isEditing: true, + 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, { + isEditing: true, + 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, { + isEditing: true, + 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() restores the streamed buffer before closing it for a new file", async () => { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: closing + // a dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. The buffer must be restored to its pre-stream content and saved clean + // BEFORE the close, so close() never sees a dirty tab. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const teardown = diffViewProvider as unknown as { + restorePreStreamBuffer: (absolutePath: string) => Promise + saveBufferClean: (absolutePath: string) => Promise + } + let restoreArgs: unknown[] = [] + let saveArgs: unknown[] = [] + let restoreOrder = 0 + let saveOrder = 0 + const restore = vi.spyOn(teardown, "restorePreStreamBuffer").mockResolvedValue(undefined) + const saveClean = vi.spyOn(teardown, "saveBufferClean").mockResolvedValue(undefined) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + 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, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await diffViewProvider.revertChanges() + // mockRestore() clears the recorded history, so the evidence is captured here. + restoreArgs = restore.mock.calls.map((args) => args[0]) + saveArgs = saveClean.mock.calls.map((args) => args[0]) + restoreOrder = restore.mock.invocationCallOrder[0] + saveOrder = saveClean.mock.invocationCallOrder[0] + } finally { + // Do not leak the fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + restore.mockRestore() + saveClean.mockRestore() + } + + expect(restoreArgs).toEqual([mockTargetPath]) + expect(saveArgs).toEqual([mockTargetPath]) + expect(saveOrder).toBeGreaterThan(restoreOrder) + const closeOrder = vi.mocked(vscode.window.tabGroups.close).mock.invocationCallOrder.at(-1) + expect(closeOrder).toBeGreaterThan(saveOrder) + // No force flag: the tab is clean by the time it is closed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() restores and saves the buffer once even when no plain text tab is open", async () => { + // The document can be open only through the diff editor: the user closed the plain text + // tab, so tabGroups.all yields no TabInputText for the path. Restoring inside the tab loop + // meant nothing ran, closeAllDiffViews() then skipped the dirty diff tab, and the unlink + // left a dirty diff tab holding unapproved content that a later save would recreate. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const teardown = diffViewProvider as unknown as { + restorePreStreamBuffer: (absolutePath: string) => Promise + saveBufferClean: (absolutePath: string) => Promise + } + const restore = vi.spyOn(teardown, "restorePreStreamBuffer").mockResolvedValue(undefined) + const saveClean = vi.spyOn(teardown, "saveBufferClean").mockResolvedValue(undefined) + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + // No plain TabInputText tab for this path - only the diff editor holds the document. + get: () => [{ tabs: [] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + let restoreArgs: unknown[] = [] + let saveArgs: unknown[] = [] + let restoreOrder = 0 + let saveOrder = 0 + try { + await diffViewProvider.revertChanges() + // mockRestore() clears the recorded history, so the evidence is captured here. + restoreArgs = restore.mock.calls.map((args) => args[0]) + saveArgs = saveClean.mock.calls.map((args) => args[0]) + restoreOrder = restore.mock.invocationCallOrder[0] + saveOrder = saveClean.mock.invocationCallOrder[0] + } finally { + // Do not leak the fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + restore.mockRestore() + saveClean.mockRestore() + } + + // Exactly once each, restore before save, even with an empty tab list. + expect(restoreArgs).toEqual([mockTargetPath]) + expect(saveArgs).toEqual([mockTargetPath]) + expect(saveOrder).toBeGreaterThan(restoreOrder) + // The rollback still completed: the file it created was unlinked. + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() restores and saves an existing file through the real rollback path", async () => { + // The existing-file branch used to build its own WorkspaceEdit, discard applyEdit()'s boolean, + // and save unconditionally, so a refused restore wrote the unapproved streamed content to an + // existing file. It now runs through the same contract as the new-file branch. textDocuments is + // deliberately left empty: the branch reached this document through activeDiffEditor, and if the + // restore resolved it by path again it would find nothing and skip the restore in silence. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await diffViewProvider.revertChanges() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + expect(vscode.workspace.applyEdit).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace).toHaveBeenCalledWith( + editor.document.uri, + expect.anything(), + "pre-stream content", + ) + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("revertChanges() does not save an existing file when the editor refuses the restore", async () => { + // The user must not be told the write was rolled back while the existing file still holds + // content they never approved: the refusal has to reach the reporting layer, and the save + // that would persist it must not run. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the streamed buffer") + expect(editor.document.save).not.toHaveBeenCalled() + expect(diffViewProvider["keepOrCloseEditedFile"]).not.toHaveBeenCalled() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + }) + + it("revertChanges() reports a save the editor did not perform for an existing file", async () => { + // A restore that applied but did not persist leaves a dirty buffer one save away from disk. + // save() resolves false in that case, and treating it as success would report a rollback that + // never completed. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + editor.document.save = vi.fn().mockResolvedValue(false) + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.workspace, "textDocuments", { get: () => [], configurable: true }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "modify", + createdDirs: [], + originalContent: "pre-stream content", + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored buffer") + expect(diffViewProvider["keepOrCloseEditedFile"]).not.toHaveBeenCalled() + } finally { + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + }) + it("revertChanges() reports a close the editor refused instead of deleting underneath it", async () => { + // Even a restored, saved buffer can fail to close (a vetoing editor). Swallowing that let + // the rollback delete the file underneath an open tab. The failure must propagate, and + // the file must survive: unlinking after a refused close leaves an open tab on a ghost. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: vi.fn().mockResolvedValue(false), + configurable: true, + }) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not close the restored buffer") + // This test owns only the refusal handling; the restore-before-close ordering is + // asserted by the test above. What matters here is that the close was attempted + // without a force flag and the file survived a close the editor vetoed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).not.toHaveBeenCalled() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + }) + it("revertChanges() reports a close that rejected instead of deleting underneath it", async () => { + // tabGroups.close() can reject, not only resolve false (the editor throws on veto). The rejection + // used to be logged and swallowed, so the rollback continued, unlinked the placeholder, and + // removed the directories it had created underneath a tab that was still open. The rejection + // must surface as the rollback failure with the original error kept as its cause, and the + // filesystem cleanup must not run: after a refused discard the caller owns the next step + // (fail closed), so this rollback must not delete around it. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const closeError = new Error("Tab close rejected by the editor") + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: vi.fn().mockRejectedValue(closeError), + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + originalContent: "", + }) + + try { + // The failure is reported, not swallowed: revertChanges() rejects with the rollback + // error, and the original rejection stays reachable as its cause. + const failure = await diffViewProvider.revertChanges().then( + () => undefined, + (error: unknown) => (error instanceof Error ? error : new Error(String(error))), + ) + expect(failure).toBeInstanceOf(Error) + expect(failure?.message).toContain("could not close the restored buffer") + expect(failure?.cause).toBe(closeError) + // Cleanup ownership is retained: the close was attempted without a force flag, and + // neither the placeholder unlink nor the created-directory rmdir ran underneath the + // still-open tab. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + }) + it("revertChanges() restores and saves the buffer through the real rollback path", async () => { + // The two tests above spy restorePreStreamBuffer() and saveBufferClean(), so the restoration + // itself never runs there and only proves ordering. This one leaves both real: the buffer must + // be put back through a WorkspaceEdit and saved clean, which is what stops close() from ever + // seeing a dirty tab holding content the user never approved. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + // The real restore and save look the document up here; without this the methods would find + // nothing and pass without touching anything. + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the fixtures: later tests read the module-level tabGroups.all and textDocuments. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + // The real restore replaced the whole buffer with the pre-stream content. + expect(vscode.workspace.applyEdit).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace).toHaveBeenCalledWith( + editor.document.uri, + expect.anything(), + "pre-stream content", + ) + // The real save ran on the restored buffer, and only then was the tab closed. + expect(editor.document.save).toHaveBeenCalledTimes(1) + const saveOrder = editor.document.save.mock.invocationCallOrder[0] + const closeOrder = vi.mocked(vscode.window.tabGroups.close).mock.invocationCallOrder.at(-1) + expect(closeOrder).toBeGreaterThan(saveOrder) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() neither saves nor deletes when the editor refuses the restore", async () => { + // A refused WorkspaceEdit leaves the unapproved streamed content in the buffer. Saving it + // would persist exactly what this rollback exists to undo, and deleting the placeholder + // underneath would report a rollback that never finished. The refusal has to reach the + // caller that reports failures, which is what the user gets instead of a false success. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.getText = vi.fn().mockReturnValue("streamed content") + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the streamed buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + + // Nothing saveable was left behind, and nothing was closed or deleted on top of the refusal. + expect(editor.document.save).not.toHaveBeenCalled() + expect(vscode.window.tabGroups.close).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + describe("userTouchedDiffEditor keep/close behavior", () => { + const mockTargetPath = `${mockCwd}/mock-target-file.ts` + + const buildActiveDiffEditor = () => ({ + document: { + uri: { fsPath: mockTargetPath, scheme: "file" }, + getText: vi.fn().mockReturnValue("content"), + isDirty: false, + save: vi.fn().mockResolvedValue(true), + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + }, + }) + 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) ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = false ;(diffViewProvider as any).preEditScrollLine = undefined @@ -1198,6 +1827,8 @@ describe("DiffViewProvider", () => { ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = true ;(diffViewProvider as any).preEditScrollLine = undefined @@ -1248,20 +1879,6 @@ describe("DiffViewProvider", () => { expect((diffViewProvider as any).userTouchedDocument).toBe(true) }) - }) - - describe("userTouchedDiffEditor keep/close behavior", () => { - const mockTargetPath = `${mockCwd}/mock-target-file.ts` - - const buildActiveDiffEditor = () => ({ - document: { - uri: { fsPath: mockTargetPath }, - getText: vi.fn().mockReturnValue("content"), - isDirty: false, - save: vi.fn().mockResolvedValue(undefined), - positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), - }, - }) it("saveChanges() keeps the file open when the user clicked inside the diff editor", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) @@ -1470,6 +2087,8 @@ describe("DiffViewProvider", () => { ;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined) ;(diffViewProvider as any).closeFileTab = closeFileTab ;(diffViewProvider as any).relPath = "mock-target-file.ts" + // The guard in revertChanges() requires an edit in progress; this fixture sets fields directly. + diffViewProvider["isEditing"] = true ;(diffViewProvider as any).documentWasOpen = false ;(diffViewProvider as any).userTouchedDocument = false // The user clicked inside the diff pane -- but this is a deny, so it must be ignored. @@ -1661,10 +2280,10 @@ describe("DiffViewProvider", () => { const buildActiveDiffEditor = () => ({ document: { - uri: { fsPath: mockTargetPath }, + uri: { fsPath: mockTargetPath, scheme: "file" }, getText: vi.fn().mockReturnValue("content"), isDirty: false, - save: vi.fn().mockResolvedValue(undefined), + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), }, })