diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index bc5b8a4426..b42a549fef 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -5,6 +5,7 @@ import { ClineProvider } from "../core/webview/ClineProvider" import { TaskRegistry } from "../core/task/TaskRegistry" import { PendingActionSettlementError, type Task } from "../core/task/Task" import { makeProviderStub } from "./helpers/provider-stub" +import { writeToFileTool } from "../core/tools/WriteToFileTool" type MockTask = Pick & Partial> & { @@ -208,6 +209,46 @@ describe("ClineProvider failed history restoration cleanup", () => { expect(task.dispose).toHaveBeenCalledOnce() }) + it("releases the tool's per-task state before a directly disposed task loses its listeners", async () => { + const key = "failed-history-task.inst-1" + const order: string[] = [] + const task = { + taskId: "failed-history-task", + instanceId: "inst-1", + emit: vi.fn(), + once: vi.fn(), + off: vi.fn(), + dispose: vi.fn().mockImplementation(() => { + // Dispose removes every listener, so this is the last moment the abort + // cleanup could still have run; the entry must already be gone here. + order.push(writeToFileTool["taskPartialStreamState"].has(key) ? "state-retained" : "state-cleared") + order.push("dispose") + return Promise.resolve() + }), + } as unknown as Task + // Fixture: the state entry the tool would have created during a partial stream. + writeToFileTool["taskPartialStreamState"].set(key, { + lastSeenPartialPath: undefined, + streamFailed: false, + streamError: undefined, + task, + abortCleanup: () => {}, + }) + const taskEventListeners = new Map([[task, [vi.fn()]]]) + const taskRegistry = new TaskRegistry() + taskRegistry.push(task) + const provider = { taskRegistry, taskEventListeners, log: vi.fn() } as unknown as ClineProvider + + await privateClineProvider.cleanupFailedHistoryTask.call( + provider, + task, + new PendingActionSettlementError("settlement failed"), + ) + + expect(order).toEqual(["state-cleared", "dispose"]) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("keeps the task active after an unrelated history resume failure", async () => { const cleanupListener = vi.fn() const task = { diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-missing-native-args.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-missing-native-args.spec.ts new file mode 100644 index 0000000000..9efd86bfac --- /dev/null +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-missing-native-args.spec.ts @@ -0,0 +1,171 @@ +// npx vitest src/core/assistant-message/__tests__/presentAssistantMessage-missing-native-args.spec.ts + +import { describe, it, expect, beforeEach, vi } from "vitest" +import { presentAssistantMessage } from "../presentAssistantMessage" +import { isValidToolName, validateToolUse } from "../../tools/validateToolUse" + +const mockTeardown = vi.hoisted(() => vi.fn()) +const mockWriteHandle = vi.hoisted(() => vi.fn()) + +vi.mock("../../task/Task") +vi.mock("../../tools/validateToolUse", () => ({ + validateToolUse: vi.fn(), + isValidToolName: vi.fn(() => true), +})) +vi.mock("../../tools/WriteToFileTool", () => ({ + writeToFileTool: { handle: mockWriteHandle, teardownAbandonedStream: mockTeardown }, +})) +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureToolUsage: vi.fn(), + captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), + }, + }, +})) + +type ToolResultBlock = { type: string; tool_use_id: string; content: string; is_error: boolean } + +describe("presentAssistantMessage - finalized block without nativeArgs", () => { + // The presenter reads only a subset of Task. A Record keeps the double honest without + // an `any`; the single cast at the call site is the documented boundary. + let mockTask: Record + let userMessageContent: ToolResultBlock[] + + beforeEach(() => { + mockTeardown.mockReset() + mockWriteHandle.mockReset() + userMessageContent = [] + mockTask = { + taskId: "test-task-id", + instanceId: "test-instance", + abort: false, + presentAssistantMessageLocked: false, + presentAssistantMessageHasPendingUpdates: false, + currentStreamingContentIndex: 0, + assistantMessageContent: [ + { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + // Streaming JSON never parsed: Task completes the block with no nativeArgs. + id: "toolu_1", + nativeArgs: undefined, + }, + ], + userMessageContent, + didCompleteReadingStream: false, + didRejectTool: false, + didAlreadyUseTool: false, + consecutiveMistakeCount: 0, + consecutiveMistakeLimit: 3, + apiConfiguration: { apiProvider: "test-provider" }, + clineMessages: [], + getTaskMode: vi.fn().mockResolvedValue("code"), + api: { getModel: () => ({ id: "test-model", info: {} }) }, + recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }) }, + providerRef: { + deref: () => ({ getState: vi.fn().mockResolvedValue({ mode: "code", customModes: [] }) }), + }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + pushToolResultToUserContent: vi.fn((toolResult: ToolResultBlock) => { + userMessageContent.push(toolResult) + return true + }), + } + }) + + it("tears the write_to_file stream state down instead of dispatching the tool", async () => { + await presentAssistantMessage(mockTask as unknown as Parameters[0]) + + // The malformed call must not be executed, and exactly one error tool_result is + // emitted for the provider. + expect(isValidToolName).toHaveBeenCalled() + expect(mockWriteHandle).not.toHaveBeenCalled() + expect(mockTask.pushToolResultToUserContent).toHaveBeenCalledTimes(1) + expect(userMessageContent).toEqual([ + expect.objectContaining({ + type: "tool_result", + tool_use_id: "toolu_1", + is_error: true, + content: expect.stringContaining("missing nativeArgs"), + }), + ]) + // The guard bypasses handle(), so the per-task stream state has to be released here: + // otherwise the entry, its TaskAborted listener and any streamed diff view leak into + // the next API request of the same task. + expect(mockTeardown).toHaveBeenCalledTimes(1) + expect(mockTeardown).toHaveBeenCalledWith(mockTask) + }) + + it("tears the stream state down when tool validation rejects the call", async () => { + // A mode file restriction (or any validateToolUse failure) lands here. The block + // streamed partial deltas - handlePartial ran - and this exit bypasses handle(), so + // without the teardown the per-task stream state, its TaskAborted listener and any + // streamed diff view leak into the next API request. + mockTask.assistantMessageContent = [ + { + type: "tool_use", + name: "write_to_file", + params: { path: "restricted.ts", content: "partial model output" }, + partial: false, + id: "toolu_2", + nativeArgs: { path: "restricted.ts", content: "partial model output" }, + }, + ] + vi.mocked(validateToolUse).mockImplementationOnce(() => { + throw new Error("write_to_file is not allowed to write restricted.ts in this mode") + }) + + await presentAssistantMessage(mockTask as unknown as Parameters[0]) + + expect(mockWriteHandle).not.toHaveBeenCalled() + expect(userMessageContent).toEqual([ + expect.objectContaining({ + type: "tool_result", + tool_use_id: "toolu_2", + is_error: true, + content: expect.stringContaining("restricted.ts"), + }), + ]) + expect(mockTeardown).toHaveBeenCalledTimes(1) + expect(mockTeardown).toHaveBeenCalledWith(mockTask) + }) + + it("tears the stream state down when the tool repetition limit stops the call", async () => { + mockTask.assistantMessageContent = [ + { + type: "tool_use", + name: "write_to_file", + params: { path: "same.ts", content: "same content" }, + partial: false, + id: "toolu_3", + nativeArgs: { path: "same.ts", content: "same content" }, + }, + ] + vi.mocked(mockTask.toolRepetitionDetector as unknown as { check: unknown }).check = vi.fn().mockReturnValue({ + allowExecution: false, + askUser: { messageKey: "tool_repetition", messageDetail: "write_to_file" }, + }) + + await presentAssistantMessage(mockTask as unknown as Parameters[0]) + + expect(mockWriteHandle).not.toHaveBeenCalled() + // The repetition exit reports through the local pushToolResult, which wraps the + // message in the tool-error envelope rather than the provider is_error flag. + expect(userMessageContent).toEqual([ + expect.objectContaining({ + type: "tool_result", + tool_use_id: "toolu_3", + content: expect.stringContaining("repetition limit reached"), + }), + ]) + expect(mockTeardown).toHaveBeenCalledTimes(1) + expect(mockTeardown).toHaveBeenCalledWith(mockTask) + }) +}) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 417dcf7a45..ee9bb58d69 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -572,6 +572,14 @@ async function presentAssistantMessageBlock(cline: Task): Promise { is_error: true, }) + // A write_to_file call that streamed partial deltas leaves per-task stream state + // (and possibly an open diff view) behind, and this guard bypasses handle(), so + // the teardown that execute()/onParameterParseFailure would have run never does. + // Release it here instead of leaking it into the next API request. + if (block.name === "write_to_file") { + await writeToFileTool.teardownAbandonedStream(cline) + } + break } } @@ -779,6 +787,14 @@ async function presentAssistantMessageBlock(cline: Task): Promise { error.message, ) + // Same leak as the missing-nativeArgs guard: the block streamed partial + // deltas (handlePartial ran), this exit bypasses handle()/execute(), so the + // per-task stream state and any open diff view must be released here. A + // mode file restriction blocking the target path lands exactly here. + if (block.name === "write_to_file") { + await writeToFileTool.teardownAbandonedStream(cline) + } + break } @@ -846,6 +862,12 @@ async function presentAssistantMessageBlock(cline: Task): Promise { `Tool call repetition limit reached for ${block.name}. Please try a different approach.`, ), ) + + // The repetition exit also bypasses handle(): release the stream state and + // diff view the partial deltas created, or they leak into the next request. + if (block.name === "write_to_file") { + await writeToFileTool.teardownAbandonedStream(cline) + } break } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 5ef3e54b56..fc317b3d26 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -110,6 +110,7 @@ import { buildNativeToolsArrayWithRestrictions } from "./build-tools" // core modules import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" +import { writeToFileTool } from "../tools/WriteToFileTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" import { RooProtectedController } from "../protect/RooProtectedController" @@ -379,6 +380,25 @@ export class Task extends EventEmitter implements TaskLike { * @private */ private taskApiConfigReady: Promise + /** + * Whether taskApiConfigReady has settled. The dispose-time metadata retry needs it only while + * _taskApiConfigName is still undefined: persistTaskMetadata() awaits the promise in that case, + * and teardown must not block on it. A legacy history item legitimately has no stored api + * config while its initialization completed, and that task still needs the retry; once the + * name IS known, persistTaskMetadata() skips the await, so an unsettled promise must not + * suppress the repair either. + */ + private taskApiConfigReadySettled = false + + /** + * Set when the derived metadata / task-history stage of a save failed while the message + * write itself succeeded. Drives one awaited retry in disposeOnce() so a task being torn + * down does not leave its history entry stale; cleared on the next successful metadata + * stage. + * + * @private + */ + private pendingTaskMetadataRepair: boolean = false providerRef: WeakRef private readonly globalStoragePath: string @@ -738,12 +758,16 @@ export class Task extends EventEmitter implements TaskLike { this._taskApiConfigName = handoffExecutionContext.apiConfigName this.taskModeReady = Promise.resolve() this.taskApiConfigReady = Promise.resolve() + this.taskApiConfigReadySettled = true TelemetryService.instance.captureTaskCreated(this.taskId) } else if (historyItem) { this._taskMode = historyItem.mode || defaultModeSlug this._taskApiConfigName = historyItem.apiConfigName this.taskModeReady = Promise.resolve() this.taskApiConfigReady = Promise.resolve() + // historyItem.apiConfigName is undefined for tasks saved before the api config was + // recorded. That is a completed initialization, not a pending one. + this.taskApiConfigReadySettled = true TelemetryService.instance.captureTaskRestarted(this.taskId) } else { // For new tasks, don't set the mode/apiConfigName yet - wait for async initialization. @@ -751,6 +775,16 @@ export class Task extends EventEmitter implements TaskLike { this._taskApiConfigName = undefined this.taskModeReady = this.initializeTaskMode(provider) this.taskApiConfigReady = this.initializeTaskApiConfigName(provider) + // Observe settlement without replacing the promise: a rejection must still reach + // every other awaiter of taskApiConfigReady. + void this.taskApiConfigReady.then( + () => { + this.taskApiConfigReadySettled = true + }, + () => { + this.taskApiConfigReadySettled = true + }, + ) TelemetryService.instance.captureTaskCreated(this.taskId) } @@ -1722,11 +1756,13 @@ export class Task extends EventEmitter implements TaskLike { /** * Persist the message array, then refresh the derived metadata / task-history entries. * - * The returned boolean reflects the message write only: `saveTaskMessages` failure - * leaves the on-disk record stale, so callers gating UI updates on durable state must - * skip them. Metadata / task-history stage failures are logged and swallowed — the - * message array is already persisted, and the next save recomputes and re-emits the - * metadata. + * The two stages report separately on purpose. The returned boolean reflects the MESSAGE + * WRITE: `saveTaskMessages` failure leaves the on-disk record stale, so callers gating + * UI updates on durable state must skip them. A metadata / task-history failure does NOT + * turn that result false — the message array really is persisted, and reporting it as + * failed would make callers skip a webview update that is safe. It is not silently + * dropped either: `persistTaskMetadata()` records the failure and `disposeOnce()` retries + * it once, awaited, before the task stops. * * `merge` (default `true`) is passed through to `saveTaskMessages`: the in-memory * snapshot is merged with the on-disk record. `overwriteClineMessages` passes `false` @@ -1745,6 +1781,20 @@ export class Task extends EventEmitter implements TaskLike { return false } + await this.persistTaskMetadata() + + return true + } + + /** + * Recompute the derived task metadata and write the task-history entry for messages that + * are already on disk. Returns whether THIS stage succeeded — deliberately separate from + * `saveClineMessages()` so a metadata failure is never reported as a failed message write. + * + * A failure is recorded in `pendingTaskMetadataRepair` so `disposeOnce()` gets one awaited + * retry before shutdown; any later `saveClineMessages()` also recomputes it from scratch. + */ + private async persistTaskMetadata(): Promise { try { if (this._taskApiConfigName === undefined) { await this.taskApiConfigReady @@ -1771,16 +1821,27 @@ export class Task extends EventEmitter implements TaskLike { this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage) const provider = this.providerRef.deref() - const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status - await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) + // A released provider makes the two calls below optional-chain no-ops, so without this + // guard the stage reports success while persisting nothing and clears the repair flag - + // the history entry then stays behind the messages that ARE on disk and nothing retries + // it. Treat it as the failed stage it is and leave the flag set for disposeOnce(). + if (!provider) { + this.pendingTaskMetadataRepair = true + console.error("Failed to save task metadata: the task has no provider to persist task history.") + return false + } + const existingStatus = provider.taskHistoryStore.get(this.taskId)?.status + await provider.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) + this.pendingTaskMetadataRepair = false + return true } catch (error) { - // The message array was persisted above; a metadata or task-history failure must - // not mask that write (see the method docs). The next saveClineMessages() call - // recomputes and re-emits the metadata update. + // The message array is already durable; a metadata or task-history failure must not + // mask that write (see saveClineMessages). Record it so the retry in disposeOnce() + // runs even if no further save happens before the task is torn down. + this.pendingTaskMetadataRepair = true console.error("Failed to save task metadata:", error) + return false } - - return true } private findMessageByTimestamp(ts: number): ClineMessage | undefined { @@ -3478,6 +3539,12 @@ export class Task extends EventEmitter implements TaskLike { // Remove all event listeners to prevent memory leaks. try { + // The per-task write_to_file stream state lives in a tool singleton and is + // normally released by the TaskAborted listener registered with it. removeAllListeners + // above drops that listener, so a task disposed directly - without an abort - would + // leave the singleton holding this task, its provider, and a stream that can never + // advance again. Release it here, where every disposal path passes. + writeToFileTool.clearTaskState(this) this.removeAllListeners() } catch (error) { console.error("Error removing event listeners:", error) @@ -3520,16 +3587,53 @@ export class Task extends EventEmitter implements TaskLike { try { // If we're not streaming then `abortStream` won't be called. if (this.isStreaming && this.diffViewProvider.isEditing) { - this.diffReversionPromise = this.diffViewProvider.revertChanges().catch(console.error) + // A cancelled stream was never approved. For a create, revertChanges() saves the dirty + // buffer - the partial content the model streamed - before deleting the file, so a + // failed delete leaves unapproved bytes on disk; the discard path empties the buffer + // first, so the only bytes that can reach the placeholder are none. A modify still + // reverts, which restores the content already on disk. + const releaseUnapprovedEdit = + this.diffViewProvider.editType === "modify" + ? this.diffViewProvider.revertChanges() + : this.diffViewProvider.discardUnapprovedStream() + this.diffReversionPromise = releaseUnapprovedEdit.catch(console.error) } } catch (error) { console.error("Error reverting diff changes:", error) } + // A save whose metadata / task-history stage failed leaves the history entry + // behind the messages that are already on disk. Give that stage one awaited chance to + // catch up before the task stops serving. Deliberately last: everything synchronous above + // has to happen before the first yield, because abortTaskOnce() awaits diffReversionPromise + // right after void dispose(), an abandoned stream can reach its finally while this await is + // in flight and leave the revert decision below false, and direct dispose() callers rely on + // the abort flag being set synchronously. Skipped only when the retry could block: while + // taskApiConfigReady has not settled AND _taskApiConfigName is still undefined, + // persistTaskMetadata() awaits that promise and teardown must not wait on it. Once the name + // is known the await is skipped inside persistTaskMetadata(), so the repair is safe to run + // and skipping it would leave the history entry stale for a task that could repair it. + if ( + this.pendingTaskMetadataRepair && + (this.taskApiConfigReadySettled || this._taskApiConfigName !== undefined) + ) { + await this.persistTaskMetadata() + } + await pendingCleanup await this.diffReversionPromise } + /** + * The diff teardown this disposal started, for a continuation that observes its own + * stream released mid-await. Single-owner by construction: the in-flight + * handlePartial() waits for the discard Task.dispose() already started instead of + * running an independent one over the same buffer. + */ + public async waitForDiffReversion(): Promise { + await this.diffReversionPromise + } + // Subtasks // Spawn / Wait / Complete diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 472218fce5..98b5a2ea46 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -1,3 +1,4 @@ +import { writeToFileTool } from "../../tools/WriteToFileTool" import path from "node:path" import { type ProviderSettings, RooCodeEventName } from "@roo-code/types" @@ -196,8 +197,11 @@ describe("Task dispose method", () => { resolveCleanup = resolve }), ) + // A modify: disposal reverts it. Disposal discards a create instead (see the + // create/modify pair in Task.spec.ts), and these tests are about the reversion promise. task.isStreaming = true task.diffViewProvider.isEditing = true + task.diffViewProvider.editType = "modify" const revertChangesSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue( new Promise((resolve) => { resolveReversion = resolve @@ -231,8 +235,11 @@ describe("Task dispose method", () => { const reversion = new Promise((resolve) => { resolveReversion = resolve }) + // A modify: disposal reverts it. Disposal discards a create instead (see the + // create/modify pair in Task.spec.ts), and these tests are about the reversion promise. task.isStreaming = true task.diffViewProvider.isEditing = true + task.diffViewProvider.editType = "modify" const revertChangesSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue(reversion) const disposal = task.dispose() @@ -253,8 +260,11 @@ describe("Task dispose method", () => { test("should log rejected diff reversion and continue final abort persistence", async () => { const reversionError = new Error("reversion failed") const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // A modify: disposal reverts it. Disposal discards a create instead (see the + // create/modify pair in Task.spec.ts), and these tests are about the reversion promise. task.isStreaming = true task.diffViewProvider.isEditing = true + task.diffViewProvider.editType = "modify" vi.spyOn(task.diffViewProvider, "revertChanges").mockRejectedValue(reversionError) const saveMessages = vi.fn().mockResolvedValue(true) Object.defineProperty(task, "saveClineMessages", { value: saveMessages }) @@ -463,4 +473,32 @@ describe("Task.run() idempotency", () => { await t.dispose() startTaskSpy.mockRestore() }) + + test("dispose() releases the write_to_file stream state this task registered", async () => { + // The per-task stream state lives in a tool singleton and is normally released by the + // TaskAborted listener registered with it. dispose() removes every listener, so a task + // disposed directly - the path ClineProvider.cleanupFailedHistoryTask() takes - would + // leave the singleton holding this task, its provider, and a stream that can never + // advance again. + // Spied before construction: a constructor that fails can start the disposal itself, + // and that path has to release the state too. The release is captured in a local + // rather than read back off the spy, because mockRestore() clears the spy's call + // history and the assertion below runs after it. + const released: unknown[] = [] + const clearTaskState = vi.spyOn(writeToFileTool, "clearTaskState").mockImplementation((disposed) => { + released.push(disposed) + }) + const t = new Task({ + provider: mockProvider as unknown as ClineProvider, + apiConfiguration: mockApiConfiguration, + task: "hello", + startTask: false, + }) + try { + await t.dispose() + } finally { + clearTaskState.mockRestore() + } + expect(released).toEqual([t]) + }) }) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d6be42f0bd..aac20f825e 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6669,6 +6669,284 @@ describe("Cline", () => { } }) + it("retries the failed metadata / task-history stage once, awaited, during dispose", async () => { + // A metadata or task-history failure is reported separately from the message write, + // but it must not simply be dropped: the failure is recorded and dispose() awaits + // one retry before the task stops serving, so a task torn down after a partially + // successful save does not leave its history entry behind the messages that are + // already on disk. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const historySpy = vi + .spyOn(mockProvider, "updateTaskHistory") + .mockRejectedValueOnce(new Error("history stage unavailable")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // The message write succeeded, so the save still reports success even though the + // metadata stage failed - and the failure is logged, not swallowed. + await expect(getTaskTestAccess(task).saveClineMessages()).resolves.toBe(true) + expect(historySpy).toHaveBeenCalledTimes(1) + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to save task metadata:", expect.any(Error)) + + await task.dispose() + + // dispose() awaited the retry: by the time it resolves the metadata stage has run + // again with the recomputed history item for this task. + expect(historySpy).toHaveBeenCalledTimes(2) + // The history record is keyed by `id` (the task id), not a taskId field. + expect(historySpy.mock.calls[1][0]).toEqual(expect.objectContaining({ id: task.taskId })) + + historySpy.mockRestore() + }) + + it("still retries for a task whose stored api config is legitimately absent", async () => { + // Tasks saved before the api config was recorded have apiConfigName undefined. Their + // initialization has completed, so the dispose-time repair must still run: gating it on + // the value rather than on the settled promise drops the retry for every such task. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const historySpy = vi + .spyOn(mockProvider, "updateTaskHistory") + .mockRejectedValueOnce(new Error("history stage unavailable")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Bracket access for the private fields: wait for the async api-config + // initialization to settle, then drop the name - the shape of a task loaded from a + // history entry saved before the api config was recorded. + await task["taskApiConfigReady"] + task["_taskApiConfigName"] = undefined + expect(task["taskApiConfigReadySettled"]).toBe(true) + + await expect(getTaskTestAccess(task).saveClineMessages()).resolves.toBe(true) + expect(historySpy).toHaveBeenCalledTimes(1) + + await task.dispose() + + expect(historySpy).toHaveBeenCalledTimes(2) + historySpy.mockRestore() + }) + + it("treats a released provider as a failed metadata stage instead of a silent success", async () => { + // providerRef.deref() returns undefined once the provider is gone. Without the guard the + // two history calls optional-chain to nothing, persistTaskMetadata() returns true and + // CLEARS pendingTaskMetadataRepair: the history entry then stays behind the messages that + // are already on disk and disposeOnce() has nothing left to retry on. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const historySpy = vi.spyOn(mockProvider, "updateTaskHistory").mockResolvedValue([]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + // Bracket access for the private field: the flag must start clear so the assertion below + // proves the failed stage SET it again rather than merely leaving it alone. + await task["taskApiConfigReady"] + task["pendingTaskMetadataRepair"] = false + Object.defineProperty(task, "providerRef", { + value: { deref: vi.fn().mockReturnValue(undefined) }, + configurable: true, + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + const persisted = await task["persistTaskMetadata"]() + + expect(persisted).toBe(false) + expect(historySpy).not.toHaveBeenCalled() + expect(task["pendingTaskMetadataRepair"]).toBe(true) + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("no provider to persist task history")) + errorSpy.mockRestore() + historySpy.mockRestore() + }) + + it("finishes the synchronous teardown before the dispose-time metadata retry", async () => { + // The retry has to sit after disposeOnce()'s synchronous section, not in front of it: + // abortTaskOnce() awaits diffReversionPromise right after void dispose(), an abandoned + // stream can reach its finally while the retry is in flight and leave the revert decision + // below false, and direct dispose() callers rely on the abort flag being set + // synchronously. A task-history write that never settles turns any of those three + // regressions into an observable hang. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + let releaseHistory: (() => void) | undefined + const historyGate = new Promise((resolve) => { + releaseHistory = resolve + }) + const historySpy = vi + .spyOn(mockProvider, "updateTaskHistory") + .mockImplementation(() => historyGate.then(() => [])) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + // Let the async api-config initialization settle first: the retry is deliberately + // skipped while it is pending, which would hide the ordering under test. + await task["taskApiConfigReady"] + task["pendingTaskMetadataRepair"] = true + task.isStreaming = true + task.diffViewProvider.isEditing = true + // A modify: disposal reverts it. The edit type decides which release path disposal takes + // (see the create/modify disposal pair below), and this test is about the ordering. + task.diffViewProvider.editType = "modify" + const revertSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockResolvedValue(undefined) + + const disposing = task.dispose() + // No await before this one: dispose() sets the abort flag synchronously, so a retry moved + // in front of the teardown would park dispose() on the gated task-history write first and + // this would read false. + expect(task.abort).toBe(true) + + await new Promise((resolve) => setImmediate(resolve)) + + // The teardown completed without waiting for the task-history write. + expect(task.abort).toBe(true) + expect(revertSpy).toHaveBeenCalledTimes(1) + // Bracket access for the private field: abortTaskOnce() awaits this promise right after + // void dispose(), so it has to exist once the teardown has run. + expect(task["diffReversionPromise"]).toBeDefined() + // And the retry has actually started: historySpy is gated, so without this assertion the + // test cannot tell "the teardown ran first" from "the retry never ran at all". + expect(historySpy).toHaveBeenCalledTimes(1) + + releaseHistory?.() + await disposing + + expect(historySpy).toHaveBeenCalledTimes(1) + historySpy.mockRestore() + revertSpy.mockRestore() + }) + + it("discards a cancelled create preview instead of saving it through revertChanges", async () => { + // revertChanges() for a create SAVES the dirty buffer - the partial content the model + // streamed and nobody approved - before deleting the file, so a failed delete leaves + // unapproved bytes on disk. A cancelled stream never reached approval, so disposal has to + // take the discard path, which empties the buffer first. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await task["taskApiConfigReady"] + task.isStreaming = true + task.diffViewProvider.isEditing = true + task.diffViewProvider.editType = "create" + const revertSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockResolvedValue(undefined) + const discardSpy = vi.spyOn(task.diffViewProvider, "discardUnapprovedStream").mockResolvedValue(undefined) + + await task.dispose() + + expect(discardSpy).toHaveBeenCalledTimes(1) + expect(revertSpy).not.toHaveBeenCalled() + revertSpy.mockRestore() + discardSpy.mockRestore() + }) + + it("still reverts a cancelled modify preview, whose restore is the content already on disk", async () => { + // The pair of the test above: a modify has prior content, and disposal keeps reverting it + // so the editor shows what was on disk before the stream started. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await task["taskApiConfigReady"] + task.isStreaming = true + task.diffViewProvider.isEditing = true + task.diffViewProvider.editType = "modify" + const revertSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockResolvedValue(undefined) + const discardSpy = vi.spyOn(task.diffViewProvider, "discardUnapprovedStream").mockResolvedValue(undefined) + + await task.dispose() + + expect(revertSpy).toHaveBeenCalledTimes(1) + expect(discardSpy).not.toHaveBeenCalled() + revertSpy.mockRestore() + discardSpy.mockRestore() + }) + + it("does not block teardown when a pending metadata repair has an unsettled api-config init", async () => { + // persistTaskMetadata() awaits taskApiConfigReady. For a task whose async api-config + // initialization never settles, an unconditional dispose-time retry would hang the + // teardown, so the retry is skipped until that promise settles. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Bracket access for the private fields: this is the pathological construction where + // the async api-config initialization never resolves, which no public API can produce. + task["_taskApiConfigName"] = undefined + task["taskApiConfigReady"] = new Promise(() => {}) + task["pendingTaskMetadataRepair"] = true + + const outcome = await Promise.race([ + task.dispose().then(() => "disposed"), + new Promise((resolve) => setTimeout(() => resolve("timed out"), 2000)), + ]) + + expect(outcome).toBe("disposed") + }) + + it("runs the dispose-time metadata retry when the api-config name is already known even if init never settles", async () => { + // persistTaskMetadata() only awaits taskApiConfigReady while _taskApiConfigName is still + // undefined. Once the name is known (a handoff, a resumed history item, or an explicit + // setTaskApiConfigName), the retry cannot block teardown - so skipping it because the + // promise has not settled leaves the history entry stale for a task that was perfectly + // able to repair it. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const historySpy = vi.spyOn(mockProvider, "updateTaskHistory").mockResolvedValue([]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Bracket access for the private fields: the pathological combination - a name that is + // already resolved while the initialization promise never settles - is not reachable + // through the public API. + task["_taskApiConfigName"] = "my-profile" + task["taskApiConfigReady"] = new Promise(() => {}) + task["taskApiConfigReadySettled"] = false + task["pendingTaskMetadataRepair"] = true + + const outcome = await Promise.race([ + task.dispose().then(() => "disposed"), + new Promise((resolve) => setTimeout(() => resolve("timed out"), 2000)), + ]) + + expect(outcome).toBe("disposed") + expect(historySpy).toHaveBeenCalledTimes(1) + historySpy.mockRestore() + }) + it("finalizePartialToolAsk skips the webview update when the message write itself fails", async () => { // Complements the later-stage-failure test above by failing the first save // stage: with the real task directory removed, safeWriteJson's fs.access diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dc16a9d81d 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -155,9 +155,23 @@ export abstract class BaseTool { throw new Error("Tool call is missing native arguments (nativeArgs).") } } catch (error) { - console.error(`Error parsing parameters:`, error) - const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}` - await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + const parseError = error instanceof Error ? error : new Error(String(error)) + console.error(`Error parsing parameters:`, parseError) + // Final args could not be parsed (e.g. the model's tool call was truncated + // mid-JSON by the output token limit), so execute() will never run. If a + // streaming delta already opened a partial "tool" ask (partial: true), + // finalize it here or the webview spinner stays stuck indefinitely. + await task.finalizePartialToolAsk().catch((finalizeError) => { + console.error(`Error finalizing ${this.name} partial tool ask:`, finalizeError) + }) + // execute() never runs on this path, so tools that keep per-task state + // outside execute() (streaming failure marks, abort listeners) get their + // one remaining teardown boundary here. + const reportedStreamingFailure = await this.onParameterParseFailure(task, callbacks, parseError) + if (!reportedStreamingFailure) { + const errorMessage = `Failed to parse ${this.name} parameters: ${parseError.message}` + await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage)) + } // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. // Do NOT call pushToolResult here to avoid duplicate tool_result payloads. return @@ -166,4 +180,23 @@ export abstract class BaseTool { // Execute with typed parameters await this.execute(params, task, callbacks) } + + /** + * Teardown boundary for the native-argument parse-failure path in handle(). + * + * When nativeArgs are missing or malformed, execute() never runs, so per-task + * state a tool registered outside execute() (streaming failure marks, abort + * listeners) is never torn down there. Streaming tools override this to tear + * that state down and, when a streaming delta already failed, to report the + * captured streaming error instead of the generic parse error. + * + * @param task - Task instance + * @param callbacks - Tool execution callbacks + * @param parseError - The native-argument parse error + * @returns true when the override already reported the failure to the user, + * so handle() suppresses the generic parse error + */ + protected async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + return false + } } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..37b60baa24 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,9 +22,321 @@ interface WriteToFileParams { content: string } +/** + * Per-task partial-streaming state tracked by WriteToFileTool. + */ +interface TaskPartialStreamState { + /** Last path seen during streaming; undefined until the first delta. */ + lastSeenPartialPath: string | undefined + /** True once a streaming delta hit a fatal filesystem error. */ + streamFailed: boolean + /** The original filesystem error of the failed streaming delta, reported once + * by onParameterParseFailure() when the final block fails to parse (so + * execute() never runs and would never report it). */ + streamError: Error | undefined + /** The task that owns this state; target for abort-listener deregistration. */ + task: Task + /** TaskAborted listener that tears this state down; registered once per task. */ + abortCleanup: () => void +} + export class WriteToFileTool extends BaseTool<"write_to_file"> { readonly name = "write_to_file" as const + /** + * Per-task partial-streaming state, keyed by task id (taskId + instanceId). + * + * All per-task fields live in one object per task so that resetTaskPartialState() / + * resetPartialState() cannot clear a subset of them and leak the rest (abort + * listener, failure mark, path-stabilization entry) for an abandoned stream. + * + * This deliberately diverges from the sibling streaming tools (ApplyDiffTool, + * EditFileTool, SearchReplaceTool, EditTool), which rely on BaseTool's singleton + * lastSeenPartialPath / resetPartialState and keep no failure state. The divergence is + * intentional, for two reasons: + * + * 1. Only this tool's handlePartial performs failure-prone streaming work + * (diffViewProvider.open/update, which can throw EACCES/EROFS); the siblings only + * send a task.ask preview. Without per-task failure tracking, every later delta for + * a failed path would re-attempt the failing operation and re-spawn a partial tool + * message. + * + * 2. The tool instance is a module-level singleton shared by every task, including + * tasks from different ClineProvider instances (e.g. sidebar and tab-panel + * providers, which activate independently). A single provider streams at most one + * task at a time — TaskScheduler gates task.run() at maxConcurrency=1 and + * delegation disposes the parent before the child starts — so per-task keying is + * reachable specifically across providers, where two providers can stream + * write_to_file concurrently through this same singleton. + * + * Lifting this per-task keying into BaseTool for all streaming tools is a follow-up + * (separate PR); it is deliberately not done here. + */ + private taskPartialStreamState = new Map() + + private getPartialStreamFailureKey(task: Task): string { + return `${task.taskId}.${task.instanceId}` + } + + /** + * Get this task's partial stream state, creating it on first use and registering the + * TaskAborted teardown listener exactly once per task. + */ + private getTaskPartialStreamState(task: Task): TaskPartialStreamState { + const key = this.getPartialStreamFailureKey(task) + const existing = this.taskPartialStreamState.get(key) + if (existing) { + return existing + } + + const state: TaskPartialStreamState = { + lastSeenPartialPath: undefined, + streamFailed: false, + streamError: undefined, + task, + abortCleanup: () => this.resetTaskPartialState(task), + } + this.taskPartialStreamState.set(key, state) + task.once(RooCodeEventName.TaskAborted, state.abortCleanup) + return state + } + + private hasPathStabilizedForTask(state: TaskPartialStreamState, partialPath: string | undefined): boolean { + // Stryker disable next-line ConditionalExpression: the `!== undefined` clause is redundant: when + // lastSeenPartialPath is undefined, the second clause only matches an undefined partialPath, which + // the `!!partialPath` in the return value rejects either way -- no test can distinguish the two. + const pathHasStabilized = state.lastSeenPartialPath !== undefined && state.lastSeenPartialPath === partialPath + state.lastSeenPartialPath = partialPath + return pathHasStabilized && !!partialPath + } + + /** + * Clear a task's partial-stream state from a disposal path that does not abort first. + * Task.dispose() removes every listener, so a task disposed directly (for example + * ClineProvider.cleanupFailedHistoryTask()) never fires the TaskAborted cleanup and + * this singleton would keep the disposed task and its diff-view provider. + */ + public clearTaskState(task: Task): void { + this.resetTaskPartialState(task) + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + const state = this.taskPartialStreamState.get(key) + if (!state) { + return + } + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + this.taskPartialStreamState.delete(key) + } + + /** + * Whether this task's partial stream is still the live one. handlePartial() awaits + * provider state, a filesystem probe and task.ask() before it touches the diff view; a + * cancellation during any of those awaits runs the TaskAborted teardown (or a direct + * clearTaskState), which deletes this entry. Continuing would re-open a diff view and + * re-ask for a task the user already cancelled, resurrecting the state the teardown + * released. Identity, not presence: a re-created entry for the same key belongs to a new + * stream, and this one must not write into it. + */ + private isPartialStreamStillLive(task: Task, state: TaskPartialStreamState): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) === state + } + + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + + /** + * 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 the rollback relies on. No-op when no diff view is + * open. Failures are logged and reported through the return value: the caller must + * not treat the teardown as complete when this returns false, because the document + * may still hold the unapproved content, but the remaining cleanup (reset, per-task + * state teardown) still runs so the task is not left half-torn-down. + */ + private async revertDiffChangesBeforeReset(task: Task): Promise { + // Every caller of this restores content the user never approved, so neither edit + // type may go through revertChanges(): it SAVES. Its create branch saves the dirty + // buffer - unapproved partial model output - before deleting the file, so a failed + // delete leaves that content on disk; its modify branch restores the original + // content and saves it, which for a .rooignore-denied path is a write the policy + // forbids and for any abandoned stream a write the user never approved. + return this.releaseAbandonedDiffView(task) + } + + 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) + }) + } + + /** + * Surface a failed rollback to the user. The hazard has to be visible in the chat, + * not only in the console: the editor may still hold content the task never + * approved and a save of it would land an unauthorized write. A failing say must + * not abort the teardown - a retained streaming error still has to reach the user + * through handleError - so its failure is logged only, matching the other cleanup + * helpers in this file. + */ + private async reportRevertFailure(task: Task): Promise { + await task + .say( + "error", + "write_to_file: the diff editor could not be restored after the failed tool call, so it may still show unapproved content. Do not save that editor.", + ) + .catch((sayError) => { + console.error("Error reporting write_to_file rollback failure:", sayError) + }) + } + + /** + * Cleanup for a failed partial stream: restore the diff document, close the view, + * and - when the restore itself failed - tell the user the editor may still hold + * unapproved content, the same way the parse-failure teardown does. Without the + * report, a failed rollback here is invisible: the stream error that triggered the + * cleanup is a different failure and is reported elsewhere. + */ + private async cleanupFailedPartialStream(task: Task): Promise { + const reverted = await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + if (!reverted) { + await this.reportRevertFailure(task) + } + } + + /** + * Teardown boundary for the handle() parse-failure path, where execute() never + * runs and therefore its finally (resetTaskPartialState) never runs either. + * + * Tears down the per-task stream state: otherwise the abort listener leaks for + * the task's lifetime, and when a streaming delta had failed, the streamFailed + * guard would suppress the diff preview of every later write_to_file in this + * task. Restores the diff document: streaming may have opened it with + * unapproved partial content, and execute()'s error cleanup (revert + reset) + * never fires on this path, so a user save could persist the content without + * the teardown here. When a streaming delta already hit a fatal filesystem + * error, that error is what the user can act on, so report it with the same + * "writing file" context execute()'s catch uses, and suppress the incidental + * parse error. + */ + override async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise { + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (!state) { + return false + } + // Streaming may have opened the diff view with unapproved partial content. + // execute() never runs on this path, so its error cleanup (revert + reset) + // never fires: restore the document here so a user save cannot persist + // content the write never completed (the same invariant the denial and + // streaming-failure paths maintain). Both helpers no-op when no view is open. + // The revert runs BEFORE the per-task state is torn down: when it fails, the + // document can still hold that unapproved content, and the recovery state has + // to exist while the outcome is decided and reported. + const reverted = await this.revertDiffChangesBeforeReset(task) + this.resetTaskPartialState(task) + await this.resetDiffViewAfterWrite(task) + if (!reverted) { + // Do not report a completed teardown: the editor may still show content this + // task never approved, and saving it would land a write the user never + // authorized. The user has to be able to tell that from the UI. + await this.reportRevertFailure(task) + } + if (!state.streamError) { + return false + } + void parseError + await callbacks.handleError("writing file", state.streamError) + return true + } + + /** + * Release the diff view of an abandoned, failed, or denied stream without writing to + * the target file. Neither edit type may go through revertChanges(), because that + * method saves: its create branch persists the dirty buffer (unapproved partial model + * output) before deleting the file, and its modify branch writes the restored original + * content back to a file the user never approved - a write the rooignore policy + * forbids outright for a denied path. discardUnapprovedStream() restores the buffer in + * memory only and removes the artifacts this edit created. + */ + private async releaseAbandonedDiffView(task: Task): Promise { + try { + await task.diffViewProvider.discardUnapprovedStream() + return true + } catch (releaseError) { + console.error("Error releasing the abandoned write_to_file diff view:", releaseError) + return false + } + } + + /** + * Own the diff view that open() publishes AFTER cancellation already released this + * stream. The TaskAborted teardown only knows about the view that existed when it ran; + * a create that completes afterwards leaves its placeholder and the directories open() + * made on disk with no owner, and a modify leaves a dirty preview - nothing else in the + * stream path will touch them, because execute() never runs for a cancelled stream. + * + * Idempotent by construction: the discard is a no-op once reset() has cleared the + * provider (no relPath / activeDiffEditor), so a second settle of the same open() - or + * a later delta that also observes the release - cannot roll back twice or report the + * hazard twice. + */ + private async discardDiffViewOpenedAfterRelease(task: Task): Promise { + if (!task.diffViewProvider.isEditing) { + return + } + const released = await this.releaseAbandonedDiffView(task) + await this.resetDiffViewAfterWrite(task) + if (!released) { + // The buffer could not be restored, so the editor may still show content this task + // never approved; the cancellation itself reports its own outcome, not this hazard. + await this.reportRevertFailure(task) + } + } + + /** + * Teardown for the presenter's missing-nativeArgs guard. A finalized write_to_file + * block whose streamed JSON never parsed never reaches handle(): the presenter emits + * the tool_result and breaks, so neither execute()'s finally nor + * onParameterParseFailure() runs. Without this the per-task stream state (map entry, + * TaskAborted listener, and a diff view that streaming may have opened with unapproved + * partial content) outlives the call and leaks into the next API request: a stale path + * makes the next write's first delta look stabilized, and a retained streamFailed flag + * suppresses that write's preview. + * + * Silent for the malformed call itself: the guard has already pushed the tool_result + * the provider waits for, so reporting the call again would double-report it. A FAILED + * ROLLBACK is the exception - the editor may still hold content this task never + * approved, and that hazard has to be visible in the chat exactly as the parse-failure + * and failed-stream teardowns report it. + */ + async teardownAbandonedStream(task: Task): Promise { + if (!this.taskPartialStreamState.has(this.getPartialStreamFailureKey(task))) { + return + } + + const released = await this.releaseAbandonedDiffView(task) + this.resetTaskPartialState(task) + await this.resetDiffViewAfterWrite(task) + if (!released) { + await this.reportRevertFailure(task) + } + } + + 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 @@ -34,7 +346,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) - await task.diffViewProvider.reset() + // handlePartial() has no missing-parameter guard, so streaming deltas for a + // stabilized path may already have created a partial `tool` ask (partial: true) + // before execute() saw the malformed payload. Finalize it so the UI spinner + // does not stay stuck, mirroring the rooignore and execute-error cleanups. + await this.finalizePartialToolAskAfterFailure(task) + await this.cleanupFailedPartialStream(task) + this.resetTaskPartialState(task) return } @@ -42,7 +360,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) - await task.diffViewProvider.reset() + // Same partial-ask cleanup as the missing-`path` branch above: a partial `tool` + // ask created during streaming would otherwise stay open (partial: true). + await this.finalizePartialToolAskAfterFailure(task) + await this.cleanupFailedPartialStream(task) + this.resetTaskPartialState(task) return } @@ -51,6 +373,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + // handlePartial() has no rooignore guard, so streaming deltas for this denied + // path may already have created a partial `tool` ask (partial: true) and opened + // the diff view before execute() reached the access check. Denying here without + // cleanup would leave the UI spinner stuck (partial: true), the diff view open + // with the denied content still dirty in the editor, and this task's per-task + // stream state leaked. Perform the same cleanup the try/finally path does + // before returning. + await this.finalizePartialToolAskAfterFailure(task) + // The write was denied before approval: restore the document so a user save + // cannot persist the streamed content. + await this.cleanupFailedPartialStream(task) + this.resetTaskPartialState(task) return } @@ -66,12 +400,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - if (newContent.startsWith("```")) { newContent = newContent.split("\n").slice(1).join("\n") } @@ -95,7 +423,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } + // Tracks whether the user approved the write, so the error path only reverts the + // diff document when the content was never approved (an approved edit is kept in + // the editor so the user can save it manually after a late failure). + let writeApproved = false + try { + // Create parent directories for new files inside the try block so filesystem + // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup + // and consecutive-mistake counting, rather than escaping unhandled. + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } + task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -129,9 +469,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // The prevent-focus branch set editType/originalContent on the provider + // before asking. Clear them on denial (no diff document was opened in + // this branch, so reset() is sufficient; the non-prevent-focus denial + // branch resets through revertChanges()), so a later write re-checks + // the file system instead of reusing the stale editType. + await this.resetDiffViewAfterWrite(task) return } + writeApproved = true + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) } else { if (!task.diffViewProvider.isEditing) { @@ -164,6 +512,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } @@ -177,17 +527,41 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) - await task.diffViewProvider.reset() - this.resetPartialState() + await this.resetDiffViewAfterWrite(task) task.processQueuedMessages() return } catch (error) { - await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() + // Finalize any open partial tool message so the UI spinner doesn't get stuck. + // The partial ask fired during streaming (handlePartial) or early in execute sets + // partial: true on the webview message; without this, the spinner persists even + // after the error bubble appears. + await this.finalizePartialToolAskAfterFailure(task) + // The diff cleanup runs in a finally around handleError: the production + // handleError awaits Task.say(), which rejects when the task is aborted, and a + // rejected handleError must not skip restoring the unapproved streamed content. + try { + await handleError("writing file", error as Error) + } finally { + // Before approval the diff document holds unapproved streamed content: + // restore it so a user save cannot persist it. After approval the content + // is the user's accepted edit -- keep it in the editor (dirty) so they can + // save it manually. + let reverted = true + if (!writeApproved) { + reverted = await this.revertDiffChangesBeforeReset(task) + } + await this.resetDiffViewAfterWrite(task) + if (!reverted) { + // The restore failed, so the editor still holds content this task never approved. + // The error the user sees on this path is the write failure, not this one. + await this.reportRevertFailure(task) + } + } return + } finally { + this.resetTaskPartialState(task) } } @@ -195,62 +569,168 @@ 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, - ) + // 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 (isPreventFocusDisruptionEnabled) { + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { return } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + // Hoisted out of the guarded setup: the diff-view catch further down finalizes the + // same partial ask, so the message has to stay in scope once the try block ends. + let partialMessage: string | undefined - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + try { + // Everything from here 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() - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + // Cancelled while provider state was in flight: the teardown already + // released this task's stream state. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) + return + } - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, - } + // 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) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) - if (newContent) { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath!), + content: newContent || "", + isOutsideWorkspace, + isProtected: isWriteProtected, } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) + partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + } catch (error) { + // Unexpected failure in the pre-streaming setup (provider state, the filesystem probe, the + // partial ask): this delta never reaches the diff view or execute(), so nothing else + // releases what the registration above acquired. Drop this task's entry and its + // TaskAborted listener, then rethrow - BaseTool.handle() still reports the error once. + this.resetTaskPartialState(task) + throw error + } + + if (newContent) { + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } + + // Cancellation may land while open() is in flight: its abort handler has + // already torn the stream down (and may have reverted or closed this very + // diff view), so streaming the partial content into it now would resurrect a + // view for a task that no longer exists. The view open() just published is now + // this method's responsibility - the teardown ran before it existed. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + await this.discardDiffViewOpenedAfterRelease(task) + return + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + + // Cancellation can land while update() is in flight. The view it streamed into + // already existed when the TaskAborted teardown ran, so that teardown owns it: + // starting a second discard here would roll the same buffer back twice and report + // the hazard twice. Wait for the task's own reversion instead, then drop the + // provider references this released stream still points at. Without the re-check + // the continuation returns as if it still owned a live stream. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + await task.waitForDiffReversion() + await this.resetDiffViewAfterWrite(task) + return + } + } catch (error) { + // A cancellation that lands while open() or update() is in flight runs the + // TaskAborted teardown - which releases this task's stream state, reverts or + // closes this very diff view, and reports the failure itself - and it can also + // reject the call in flight. Marking the already-released state failed, finalizing + // the ask, or running the failed-stream cleanup a second time would resurrect UI + // and roll back twice for a task the user cancelled, so the teardown owns the + // outcome here. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + console.error(`Error streaming write_to_file diff view:`, error) + // The teardown ran while this call was in flight, so it could not have released the + // view open() had already published (or left half-published). Discard it here. + await this.discardDiffViewOpenedAfterRelease(task) + return + } + + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // rethrow: the same filesystem operation is retried in execute() once the + // block completes, and that authoritative non-partial path reports the + // error to the user. Surfacing it here too would show the same error twice. + // Swallowing it here is safe because the agent loop advances naturally when + // the non-partial block arrives (it does not depend on this throw). + console.error(`Error streaming write_to_file diff view:`, error) + // 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), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts new file mode 100644 index 0000000000..14edf46c6a --- /dev/null +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -0,0 +1,255 @@ +// npx vitest run core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts + +import { RooCodeEventName } from "@roo-code/types" +import { vi, type MockedFunction } from "vitest" + +import { type Task } from "../../task/Task" +import { writeToFileTool } from "../WriteToFileTool" + +// The cleanup primitives only read these members, so a structural double is enough; +// the double assertion is the repo's existing pattern for private-method tests +// (see src/__tests__/removeClineFromStack-delegation.spec.ts). +interface CleanupTask { + taskId: string + instanceId: string + once: MockedFunction<(...args: unknown[]) => unknown> + off: MockedFunction<(...args: unknown[]) => unknown> + diffViewProvider: { + reset: MockedFunction<() => Promise> + revertChanges: MockedFunction<() => Promise> + discardUnapprovedStream: MockedFunction<() => Promise> + editType?: string + } + finalizePartialToolAsk: MockedFunction<() => Promise> + say: MockedFunction<(...args: unknown[]) => 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), + discardUnapprovedStream: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(undefined), + } + return task as unknown as Task +} + +// Private members are reached by bracket notation (AGENTS.md: no `as any`). +const stateFor = (task: Task) => writeToFileTool["taskPartialStreamState"].get(`${task.taskId}.${task.instanceId}`) + +describe("WriteToFileTool per-task partial-state cleanup", () => { + afterEach(() => { + writeToFileTool["taskPartialStreamState"].clear() + vi.restoreAllMocks() + }) + + it("releases the task state and deregisters the abort listener", async () => { + const task = buildTask("cleanup-task", "inst-1") + const state = writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).toHaveBeenCalledWith( + RooCodeEventName.TaskAborted, + state.abortCleanup, + ) + }) + + it("is a no-op for a task that never streamed", async () => { + const task = buildTask("never-streamed", "inst-2") + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).not.toHaveBeenCalled() + }) + + it("logs and continues when resetting the diff view fails", async () => { + const task = buildTask("reset-fails", "inst-3") + const t = task as unknown as CleanupTask + t.diffViewProvider.reset = vi.fn().mockRejectedValue(new Error("reset failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["resetDiffViewAfterWrite"](task) + + expect(errorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + }) + + it("logs and continues when reverting the diff document fails", async () => { + const task = buildTask("revert-fails", "inst-4") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["revertDiffChangesBeforeReset"](task) + + expect(errorSpy).toHaveBeenCalledWith( + "Error releasing the abandoned write_to_file diff view:", + expect.any(Error), + ) + }) + + it("reverts while the recovery state exists and reports the hazard when the revert fails", async () => { + const task = buildTask("revert-fails-teardown", "inst-6") + const t = task as unknown as CleanupTask + let stateSizeDuringRevert = -1 + t.diffViewProvider.discardUnapprovedStream = vi.fn(async () => { + // The recovery state must still be present while the rollback runs. + stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size + throw new Error("revert failed") + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // Seed the per-task stream state so this teardown boundary is taken at all. + writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + // Only handleError is reached when no streaming error was recorded; the + // structural double is the existing pattern in this file. + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters< + (typeof writeToFileTool)["onParameterParseFailure"] + >[1], + new Error("parameter parse failed"), + ) + + // A failed rollback is not a completed teardown: the user is told the editor may + // still hold content the task never approved. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(stateSizeDuringRevert).toBe(1) + // The teardown still finished. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + expect(handled).toBe(false) + errorSpy.mockRestore() + }) + + it("reports the rollback hazard AND the retained streaming error, and returns true", async () => { + const task = buildTask("revert-fails-with-stream-error", "inst-7") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + // Both reports happen: the rollback hazard and the error the user can act on. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("still reports the streaming error when the rollback warning itself fails", async () => { + const task = buildTask("rollback-warning-fails", "inst-8") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + t.say = vi.fn().mockRejectedValue(new Error("say failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + // A failing report must not abort the teardown: the streaming error still lands. + expect(errorSpy).toHaveBeenCalledWith("Error reporting write_to_file rollback failure:", expect.any(Error)) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("surfaces the rollback hazard from the failed-stream cleanup as well", async () => { + const task = buildTask("failed-stream-cleanup", "inst-9") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["cleanupFailedPartialStream"](task) + + // Same contract as the parse-failure teardown: a failed restore is reported. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + errorSpy.mockRestore() + }) + + it("stays silent when the failed-stream cleanup restores the buffer", async () => { + // The hazard report is conditional on the rollback failing. A cleanup that always + // reported "could not be restored" would pass the test above and still spam a false + // warning on every failed stream, so the success path needs its own negative case. + const task = buildTask("failed-stream-cleanup-ok", "inst-12") + const t = task as unknown as CleanupTask + + await writeToFileTool["cleanupFailedPartialStream"](task) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(t.say).not.toHaveBeenCalled() + }) + + it("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)) + }) + + it("routes a new-file rollback through the discard path, never through revertChanges", async () => { + // Pre-approval cleanup restores content the user never approved. revertChanges()'s + // new-file branch SAVES that buffer before deleting the file, so a failed delete + // leaves unapproved model output on disk (for a .rooignore-denied path that is a + // write the policy forbids). The discard path never persists it. + const task = buildTask("new-file-rollback", "inst-10") + const t = task as unknown as CleanupTask + t.diffViewProvider.editType = "create" + + const rolledBack = await writeToFileTool["revertDiffChangesBeforeReset"](task) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(rolledBack).toBe(true) + }) + + it("routes a modify rollback through the discard path as well, because revertChanges() saves", async () => { + // A modify edit does have prior content on disk, but revertChanges() restores it by + // WRITING: applyEdit plus document.save(). Every caller of this helper restores content + // the user never approved, and for a .rooignore-denied path that write is one the policy + // forbids outright - so both edit types take the in-memory discard. + const task = buildTask("modify-rollback", "inst-11") + const t = task as unknown as CleanupTask + t.diffViewProvider.editType = "modify" + + const rolledBack = await writeToFileTool["revertDiffChangesBeforeReset"](task) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(rolledBack).toBe(true) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..e7a88a9db5 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 @@ -151,6 +170,8 @@ describe("writeToFileTool", () => { update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + discardUnapprovedStream: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", userEdits: null, @@ -186,8 +207,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) @@ -224,9 +249,12 @@ describe("writeToFileTool", () => { ...params, }, nativeArgs: { - path: (params.path ?? testFilePath) as any, - content: (params.content ?? testContent) as any, - }, + // The missing-parameter tests inject `undefined` where + // NativeToolArgs["write_to_file"] declares `string`, so one assertion through + // unknown is what models the malformed payload - no `any` needed. + path: Object.prototype.hasOwnProperty.call(params, "path") ? params.path : testFilePath, + content: Object.prototype.hasOwnProperty.call(params, "content") ? params.content : testContent, + } as unknown as ToolUse<"write_to_file">["nativeArgs"], partial: isPartial, } @@ -250,6 +278,430 @@ describe("writeToFileTool", () => { expect(mockCline.rooIgnoreController.validateAccess).toHaveBeenCalledWith(testFilePath) expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath) }) + + it("finalizes the partial ask and clears per-task state when rooignore denies access", async () => { + // handlePartial() has no rooignore guard, so streaming deltas for a denied path + // still create a partial `tool` ask (partial: true) and open the diff view before + // execute() reaches the access check. The denial must clean up all of that: + // finalize the partial ask (spinner does not stick), revert the diff document so a + // user save cannot persist the denied content, reset the diff view (reset failures + // swallowed), and clear the per-task stream state (abort listener + entries). + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + // Record the relative order of discardUnapprovedStream() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + throw new Error("reset failed") + }) + + // Stream two deltas so the path stabilizes: handlePartial registers the abort + // cleanup and opens the partial ask + diff view for the (soon denied) path. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(abortCleanup).toBeTypeOf("function") + + // The completed block now reaches the access check, which denies the path. + await executeWriteFileTool({}, { fileExists: false, accessAllowed: false }) + + expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) + // The denial finalizes without a text match: any open partial tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The denied write's streamed content must be reverted from the diff document + // BEFORE reset() clears the state discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + }) + + describe("missing-parameter early-return cleanup", () => { + // handlePartial() has no missing-parameter guard: two partial streaming calls + // stabilize the path and open the partial `tool` ask + diff view. This establishes + // the "partial ask is open" precondition for the missing-parameter branches below. + async function streamPartialAsk() { + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + } + + it("finalizes the partial ask when content is missing after partial streaming", async () => { + // Streaming deltas create a partial `tool` ask (partial: true), then the completed + // payload is missing `content`. The missing-parameter branch must finalize the ask + // (the spinner must not stick) and still perform the same diff-view revert / reset + // and per-task-state cleanup as the other early-return paths. + // Record the relative order of discardUnapprovedStream() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. The + // revert mock awaits a deferred so the test proves the branch AWAITs + // discardUnapprovedStream() before reset(): with the revert still pending, reset() must + // not have run yet. + const diffViewCallOrder: string[] = [] + let resolveRevert: () => void = () => {} + const revertDeferred = new Promise((resolve) => { + resolveRevert = resolve + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + await revertDeferred + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + await streamPartialAsk() + + // The missing-parameter branch awaits discardUnapprovedStream() before reset(): with the + // deferred revert still pending, reset() must not have run yet. + const executePromise = executeWriteFileTool({ content: undefined }, { fileExists: false }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + + resolveRevert() + await executePromise + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + // The missing-parameter path finalizes without a text match: any open partial ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The streamed content of the failed write must be reverted from the diff + // document before reset() clears the state discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + // The per-task stream state must be gone, not just the diff view: the entry and its + // TaskAborted listener would otherwise live for the rest of the task. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + }) + + it("finalizes the partial ask when path is missing after partial streaming", async () => { + // Same scenario with the `path` field missing: the missing-`path` branch must run + // the identical partial-ask + diff-view + per-task-state cleanup. As in the + // content-missing test above, the revert mock awaits a deferred so the test + // proves the branch AWAITs discardUnapprovedStream() before reset(): with the revert + // still pending, reset() must not have run yet. + const diffViewCallOrder: string[] = [] + let resolveRevert: () => void = () => {} + const revertDeferred = new Promise((resolve) => { + resolveRevert = resolve + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + await revertDeferred + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + await streamPartialAsk() + + // The missing-parameter branch awaits discardUnapprovedStream() before reset(): with the + // deferred revert still pending, reset() must not have run yet. + const executePromise = executeWriteFileTool({ path: undefined }, { fileExists: false }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + + resolveRevert() + await executePromise + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "path") + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The diff document must be reverted before reset() clears the state + // discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + // The per-task stream state must be gone, not just the diff view: the entry and its + // TaskAborted listener would otherwise live for the rest of the task. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + }) + }) + + describe("abandoned-stream teardown (presenter missing-nativeArgs guard)", () => { + it("releases the per-task stream state when the finalized block never reaches handle()", async () => { + // Streaming JSON that never parses: Task marks the block complete with nativeArgs + // undefined, presentAssistantMessage pushes the error tool_result and breaks, so + // neither execute()'s finally nor onParameterParseFailure() runs. The presenter + // routes the abandoned block here instead; without it the entry, the TaskAborted + // listener and the streamed diff view survive into the next API request. + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream = vi.fn(async () => { + diffViewCallOrder.push("discard") + }) + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Two deltas stabilize the path: the entry, the listener, the partial ask and the + // diff view all exist before the block is finalized. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(abortCleanup).toBeTypeOf("function") + + await writeToFileTool.teardownAbandonedStream(mockCline) + + // The streamed content is reverted before the view is reset, and the state is gone. + // A new-file stream was never approved: the buffer must be discarded rather than + // reverted, because revertChanges() would SAVE the partial content before deleting + // the file. The view is still reset and the state released. + expect(diffViewCallOrder).toEqual(["discard", "reset"]) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + // The malformed call itself is not re-reported (the guard already emitted the + // tool_result), and a successful rollback has no hazard to surface. + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockCline.sayAndCreateMissingParamError).not.toHaveBeenCalled() + expect(mockCline.say).not.toHaveBeenCalledWith("error", expect.stringContaining("could not be restored")) + }) + + it("reports the rollback hazard when the streamed content cannot be reverted", async () => { + // Same contract as cleanupFailedPartialStream: the state is still released, but the + // user has to see that the editor may still hold unapproved content. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.discardUnapprovedStream = vi.fn(async () => { + throw new Error("discard failed") + }) + + await writeToFileTool.teardownAbandonedStream(mockCline) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("could not be restored")) + }) + + it("discards a modify teardown too, because revertChanges() would save it", async () => { + // revertChanges() restores the original content and SAVES it. For a stream the user + // never approved that is a write they never asked for - and for a rooignore-denied + // path a write the policy forbids - so both edit types take the in-memory discard. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + mockCline.diffViewProvider.editType = "modify" + mockCline.diffViewProvider.discardUnapprovedStream = vi.fn().mockResolvedValue(undefined) + mockCline.diffViewProvider.revertChanges.mockClear() + + await writeToFileTool.teardownAbandonedStream(mockCline) + + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before touching the diff view when the stream state is released during an in-flight await", async () => { + // A cancellation while task.ask() is in flight runs the TaskAborted teardown. The + // delta that was already in flight must not then re-open the diff view for a task + // the user cancelled - that resurrects the state the teardown just released. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.ask.mockImplementation(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before updating the diff view when the task is cancelled while open() is in flight", async () => { + // open() is the first provider await after the ask. If TaskAborted lands while + // it is in flight, the teardown has already released this task's stream state + // (and may have reverted or closed this very view), so the in-flight delta must + // not stream partial content into a view for a task the user cancelled. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("waits for the task's own discard instead of starting a second one when the abort lands during update()", async () => { + // The view update() streams into already existed when the TaskAborted teardown ran, so + // that teardown owns it. Without a re-check after the await the continuation returns as + // if it still owned a live stream; with an independent discard it would roll the same + // buffer back twice and report the hazard twice. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.diffViewProvider.reset.mockClear() + mockCline.waitForDiffReversion = vi.fn().mockResolvedValue(undefined) + // The cancellation lands inside update(), the last provider await of the delta. + mockCline.diffViewProvider.update.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + // Single owner: the continuation waits for the discard the disposal already started. + expect(mockCline.waitForDiffReversion).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + // It still drops the provider references the released stream points at. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("discards the diff view that open() publishes after the stream state was released", async () => { + // The TaskAborted teardown can only release the view that existed when it ran. When open() + // completes AFTER the release it publishes a view nobody owns any more: for a create that + // is the placeholder plus the directories open() wrote to disk, and execute() never runs + // for a cancelled stream, so nothing else would ever remove them. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.diffViewProvider.reset.mockClear() + // open() is only called when no view is open yet, so the flag starts clear and open() + // itself publishes the view - the real ordering, where the cancellation lands inside the + // window open() is in flight. + mockCline.diffViewProvider.isEditing = false + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + mockCline.diffViewProvider.isEditing = true + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // The delta that lost the race owns the view open() just published, and releasing it must + // never go through revertChanges(): that saves. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("does not roll back twice when a released stream is observed a second time", async () => { + // Idempotency: the discard is a no-op once reset() has cleared the provider, so a second + // observation of the same release cannot roll back again or report a hazard that never + // happened. reset() is what clears isEditing, which is the guard's only input. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + mockCline.say.mockClear() + mockCline.diffViewProvider.isEditing = false + mockCline.diffViewProvider.reset.mockImplementation(async () => { + mockCline.diffViewProvider.isEditing = false + }) + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + mockCline.diffViewProvider.isEditing = true + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // A later settle observes the same release with nothing left to release. + await writeToFileTool["discardDiffViewOpenedAfterRelease"](mockCline) + + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.say).not.toHaveBeenCalled() + }) + it("does not finalize the ask or roll back twice when open() rejects after a cancellation", async () => { + // A cancellation during open() releases the stream state through the TaskAborted + // teardown (which also reverts or closes this diff view) AND rejects the call in + // flight. The catch used to mark the released state failed, finalize the ask and run + // the failed-stream cleanup again - a second rollback and a fresh ask row for a task + // the user already cancelled. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.ask.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + throw new Error("EACCES: permission denied, open mock-file") + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // Only the partial ask this delta issued: no finalize on top of it. Asserted on the + // finalize mock itself - finalizePartialToolAskAfterFailure() calls finalizePartialToolAsk, + // not ask(), so the ask count alone cannot see a regression there. + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + // The teardown that already ran owns the rollback: no second one from the catch. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("does nothing when the task has no stream state to release", async () => { + await writeToFileTool.teardownAbandonedStream(mockCline) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(mockCline.off).not.toHaveBeenCalled() + }) }) describe("file existence detection", () => { @@ -287,15 +739,16 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "does not create directories in handlePartial -- only execute() creates them", async () => { - // First call - path not yet stabilized + // First call - path not yet stabilized, early return await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Second call with same path - path stabilized, handlePartial runs but + // must NOT call createDirectoriesForFile (directory creation belongs in execute) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() }, ) @@ -392,6 +845,25 @@ describe("writeToFileTool", () => { // Should process normally without issues expect(mockCline.consecutiveMistakeCount).toBe(0) }) + + it("does not report a successful write as failed when final diff reset rejects", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockCline.didEditFile).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("partial block handling", () => { @@ -419,6 +891,313 @@ 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("reports the captured streaming error instead of the parse error when the final block fails to parse, and clears the per-task state", async () => { + // A streaming delta fails with a filesystem error (streamFailed + streamError are + // captured). The final block then arrives without nativeArgs, so execute() never + // runs: the parse-failure teardown boundary must report the original filesystem + // error under the "writing file" context (not the incidental parse error) and tear + // down the per-task state, so the next write_to_file stream in this task is not + // blocked by the stale streamFailed guard and no abort listener leaks. + const fsError = new Error("EACCES: permission denied") + mockCline.diffViewProvider.open.mockRejectedValue(fsError) + // Capture the abort listener of the state created by the first deltas: it is the + // exact reference that the parse-failure teardown must detach. + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError) + expect(mockHandleError).not.toHaveBeenCalledWith( + expect.stringContaining("parsing write_to_file"), + expect.anything(), + ) + // The parse-failure teardown also restores the diff document: the + // streaming failure above already reverted it once (revert + reset), + // and the parse path runs the same cleanup again because execute() + // never runs on this path. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) + // Per-task state torn down at this boundary: guard cleared, the exact + // registered abort listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + + // The next stream for the same task must issue a partial ask again (the stale + // streamFailed guard is gone). + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("restores the diff document when the final block fails to parse after successful streaming", async () => { + // Streaming opened the diff view with unapproved partial content (open and + // update both succeeded, so no streaming error was captured). The final block + // then arrives without nativeArgs: execute() never runs, so its error cleanup + // never fires. The parse-failure teardown must still restore the document + // (revert before reset) or a user save could persist content the write never + // completed, and must report the generic parse error (no streaming error to + // surface instead). + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + // Delta 1 - stabilize path; delta 2 - streams the partial content into the + // diff view (open + update resolve). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // No streaming error was captured: the generic parse error is reported. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The diff document is restored by the parse path itself (no streaming + // failure happened, so this is the only revert + reset in the test). + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + // Per-task state torn down: guard cleared, exact listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + } 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 releases what the registration acquired: the entry and its listener would + // survive for the task's lifetime. The error still has to surface (BaseTool reports it). + 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, + task: mockCline, + abortCleanup: () => {}, + } + } + + it("reports a first delta as not stabilized and records the seen path", () => { + const state = makeState(undefined) + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "a.txt")).toBe(false) + expect(state.lastSeenPartialPath).toBe("a.txt") + }) + + it("reports a repeated path as stabilized", () => { + const state = makeState("a.txt") + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "a.txt")).toBe(true) + }) + + it("reports a changed path as not stabilized", () => { + const state = makeState("a.txt") + + expect(writeToFileTool["hasPathStabilizedForTask"](state, "b.txt")).toBe(false) + expect(state.lastSeenPartialPath).toBe("b.txt") + }) + }) + + describe("resetPartialState", () => { + it("resets the base partial path and detaches every task's abort listener", async () => { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + + // Seed one per-task state with an abort listener attached. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(abortCleanup).toBeTypeOf("function") + + // The base-class singleton field is reset by super.resetPartialState(). + writeToFileTool["lastSeenPartialPath"] = "stale-path" + writeToFileTool.resetPartialState() + + expect(writeToFileTool["lastSeenPartialPath"]).toBeUndefined() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + + // The per-task map was cleared too: a fresh delta sequence starts un-stabilized, so no + // second partial ask is issued. + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + }) }) describe("user interaction", () => { @@ -460,16 +1239,677 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("handles partial streaming errors after path stabilizes", async () => { + it("uses safe reset and clears partial state when path is missing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ path: "" }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + expect(mockPushToolResult).toHaveBeenCalledWith("Missing param error") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("uses safe reset and clears partial state when content is missing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ content: undefined }) + + expect(mockCline.recordToolError).toHaveBeenCalledWith("write_to_file") + expect(mockPushToolResult).toHaveBeenCalledWith("Missing param error") + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + 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).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + 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 discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.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 discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + + // Second call - path stabilized, update() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state discardUnapprovedStream() 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("finalizes any open partial tool ask when final args cannot be parsed", async () => { + // Regression test: a write_to_file block whose final args fail to parse (e.g. the + // tool call was truncated mid-JSON by the output token limit) never reaches + // execute(). A streaming delta for that block may already have opened a partial + // `tool` ask (partial: true) -- BaseTool.handle must finalize it, otherwise the + // UI spinner stays stuck even though the parse error bubble was shown. + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, partial ask issued once + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The parse error is still reported, and the open partial ask is finalized first. + // No argument: BaseTool.handle() calls finalizePartialToolAsk() with no text, so a + // mutation passing wrong text would leave findLast() unmatched and the spinner + // stuck. (toHaveBeenCalledWith(undefined) does not match a no-arg call under + // vitest's matcher semantics: [] is not equal to [undefined].) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + }) + + it("continues parse failure cleanup when finalizing the partial ask fails", async () => { + // Pins the .catch arm on task.finalizePartialToolAsk() in BaseTool.handle(): when the + // final args cannot be parsed and finalizing the open partial ask also fails, the + // failure must only be logged so the parse error is still reported to the user. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // Same no-argument contract as the sibling test above: the finalize call in + // BaseTool.handle() carries no text. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + // The parse error is still reported despite the failed finalization. + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("reports a filesystem error only once across the streaming and execute phases", async () => { + // Regression test for the double-error UX defect: a single write_to_file call to a + // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") + // and once in execute() ("writing file"). handlePartial now swallows its error so only + // the authoritative execute() error is surfaced. + const erofs = () => + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }) + mockCline.diffViewProvider.open.mockRejectedValue(erofs()) + mockedCreateDirectoriesForFile.mockRejectedValue(erofs()) + + // Streaming phase: stabilize path then fail (swallowed, no handleError) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Final phase: execute() reports the single authoritative error + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + }) + + it("does not reset consecutive mistake count when directory creation fails", async () => { + mockCline.consecutiveMistakeCount = 3 + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.consecutiveMistakeCount).toBe(3) + }) + + it("reverts the diff document when the write fails before approval", async () => { + // Regression test for the dirty-diff leak: streaming already opened the diff view + // with unapproved content, and the write then failed before the user could approve + // it. reset() alone left the diff document dirty with the streamed content -- a + // user save in the editor would persist a write the task never completed. The + // error path must revert the document (like the approval-denied path does) before + // resetting the provider state. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Stream two deltas so the diff view is open with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + }) + + it("continues cleanup when reverting the diff document fails before approval", async () => { + // Pins the .catch arm on discardUnapprovedStream() in revertDiffChangesBeforeReset(): a failed + // revert (e.g. the diff view was already closed) must only be logged so the + // remaining cleanup (diff view reset + per-task state teardown) always completes. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.discardUnapprovedStream.mockRejectedValue(new Error("revert failed")) + + // Stream two deltas so the diff view opens with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval and the revert fails too. + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error releasing the abandoned write_to_file diff view:", + expect.any(Error), + ) + // The diff view is still reset and the per-task stream state still torn down. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The reference that was registered must be the one removed. + expect(abortCleanup).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps approved diff content in the editor when saving fails after approval", async () => { + // The reverse of the previous test: once the user approved the write, the diff + // content is their accepted edit. A late failure (e.g. saveChanges rejecting) + // must NOT revert it -- the document stays dirty so the user can save it manually. + mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + // With fileExists false the rollback route is discardUnapprovedStream(), so asserting + // only revertChanges here would pass even if the writeApproved guard were dropped and + // the approved edit discarded. Assert the route that can actually fire. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("continues execute error cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + // The execute error path finalizes without a text match: any open partial + // tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + 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("swallows diff view reset errors during partial failure cleanup", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("continues partial failure cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // 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() + }) + + it("EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", async () => { + // Regression test: before the fix, createDirectoriesForFile in execute() sat outside + // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. + // After the fix the call is inside the try block, so filesystem errors are caught and + // routed through handleError with proper diffViewProvider.reset() cleanup. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The tool must not have proceeded to open or save + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message on error so the UI spinner does not get stuck", async () => { + // Regression test: when a filesystem error is thrown in execute() the webview + // message created during handlePartial (or the early ask in execute) is stuck in + // partial: true state, showing an indefinite spinner alongside the error bubble. + // The catch block must call finalizePartialToolAsk() to close the spinner without + // blocking for user input. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + // handleError must still be called + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + + // finalizePartialToolAsk must have been called (no text: the execute error + // path closes whichever partial tool ask is open) to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The write was never approved, so the diff document is reverted before reset + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + }) + + it("runs diff cleanup when handleError rejects", async () => { + // The production handleError awaits Task.say(), which rejects when the task is + // aborted. A rejected handleError must not skip the diff cleanup: the unapproved + // streamed content has to be reverted and the diff view reset, or a user save + // could persist the failed write. The handleError rejection itself propagates + // (it is not swallowed by the cleanup). As in the missing-parameter tests + // above, the revert mock awaits a deferred so the test proves the cleanup + // AWAITs discardUnapprovedStream() before reset(): with the revert still pending, + // reset() must not have run yet. + mockHandleError.mockRejectedValue(new Error("handleError rejected (aborted task)")) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + const diffViewCallOrder: string[] = [] + let resolveRevert: () => void = () => {} + const revertDeferred = new Promise((resolve) => { + resolveRevert = resolve + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + await revertDeferred + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + const executePromise = executeWriteFileTool({}, { fileExists: false }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + // handleError was attempted with the write context and the cleanup has reached + // the deferred revert... + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + // ...and while the revert is still pending, reset() must not have run yet. + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + + resolveRevert() + await expect(executePromise).rejects.toThrow("handleError rejected (aborted task)") + + // The unapproved content is reverted before the diff view reset. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + }) + + describe("prevent focus disruption experiment", () => { + /** + * Enable the PREVENT_FOCUS_DISRUPTION experiment for the current task: the experiment + * branches in execute()/handlePartial() read it from the provider state they fetch. + */ + function enablePreventFocusDisruption(): void { + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + } + + it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + expect(toolResult).toBe("Tool result message") + }) + + it("keeps approved diff content when saveDirectly fails after approval", async () => { + // The experiment branch stamps writeApproved before saveDirectly, so a late failure + // must NOT revert the document (the user approved the edit and can save it + // manually) but must still finalize the partial ask and reset the diff view. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + enablePreventFocusDisruption() + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + // Same reason as the saveChanges case: with fileExists false the rollback route is + // discardUnapprovedStream(), so this is the assertion that can actually fail. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("skips streaming diff view work when the experiment is enabled", async () => { + // With the experiment enabled the tool preview is embedded in the complete message + // built in execute(), so handlePartial must not open or update the diff view while + // streaming. + enablePreventFocusDisruption() + + // Delta 1 - stabilize path; delta 2 - path stabilized but the experiment short-circuits + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when prevent-focus-disruption skips the partial preview", async () => { + // Delta 1 only pins the path, so the entry is still live afterwards (the stream is in + // flight). Delta 2 reaches the experiment check and returns without ever showing a + // preview: nothing else would release the entry or detach the TaskAborted listener. + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + }) + + it("clears the provider state when prevent-focus approval is denied", async () => { + // The prevent-focus branch stamps editType/originalContent on the provider before + // asking. On denial nothing was approved and no diff document was opened, so the + // provider state must be cleared (reset) to make a later write re-check the file + // system instead of reusing the stale editType. The non-prevent-focus denial branch + // resets through revertChanges(); this branch must not call it (no document to + // revert). + enablePreventFocusDisruption() + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockAskApproval).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 07772816b4..70f2aa6503 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, @@ -638,6 +639,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 908159f7ab..17b78bd08c 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1011,7 +1011,7 @@ }, "core/tools/__tests__/writeToFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 5 + "count": 3 } }, "core/tools/helpers/toolResultFormatting.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..06c94f47cd 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -33,6 +33,14 @@ export class DiffViewProvider { isEditing = false originalContent: string | undefined private createdDirs: string[] = [] + /** + * Absolute path of the empty placeholder open() created for a new-file edit, or undefined + * when no placeholder is outstanding. reset() does NOT clear relPath, and saveDirectly() + * sets relPath for a file that was written with approval, so relPath alone cannot tell a + * caller that a file on disk belongs to the abandoned edit. Only this field justifies an + * unlink. + */ + private placeholderPath: string | undefined private documentWasOpen = false // Tracks whether the target file's tab was pinned before the diff session. // Closing the tab to open the diff drops VS Code's pin state, so we restore @@ -132,6 +140,10 @@ export class DiffViewProvider { // Make sure the file exists before we open it. if (!fileExists) { await fs.writeFile(absolutePath, "") + // From here until the placeholder is removed, THIS edit owns that path. Set after + // the write succeeds, so a failed write does not claim ownership of a file we did + // not create. + this.placeholderPath = absolutePath } // If the file was already open, close it (must happen after showing the @@ -336,6 +348,9 @@ export class DiffViewProvider { } const absolutePath = path.resolve(this.cwd, this.relPath) + // The write below is the approved one: whatever placeholder open() created at this path + // becomes real content, so it is no longer an artifact this edit may remove. + this.placeholderPath = undefined const updatedDocument = this.activeDiffEditor.document const editedContent = updatedDocument.getText() @@ -513,6 +528,144 @@ export class DiffViewProvider { return JSON.stringify(result) } + /** + * Release a diff view whose content was never approved: an abandoned partial stream, + * a failed stream, or a write the user was never asked to approve (rooignore denial, + * validation failure). Deliberately NOT the same as revertChanges(), because + * revertChanges() SAVES and neither abandoned case may write to disk: + * + * - create: its new-file branch saves a dirty buffer as-is before deleting the file, + * which writes partial model output the task never approved - and leaves it on disk + * if the delete then fails. + * - modify: its branch restores the original content and saves it. That is a write to + * a file the user never approved, and for a rooignore-denial path a write the + * policy forbids outright. + * + * Here the target file is never written. A create buffer is emptied FIRST, so the + * only bytes that can ever reach the placeholder are none and the tab is clean enough + * to close without a prompt; a modify buffer is restored to the content already on + * disk in memory only, so the file itself is untouched and the tab (now showing the + * original content, still marked dirty) is left for the user rather than force-closed + * over any edits they may have typed into the preview. The placeholder plus the + * directories this edit created are removed either way. + */ + async discardUnapprovedStream(): Promise { + if (!this.relPath) { + return + } + + const absolutePath = path.resolve(this.cwd, this.relPath) + // Snapshot the directories this edit created BEFORE the editor work below, and + // clear the field so nothing else can act on them twice. If an await below + // rejects, the caller runs reset(), which drops relPath/createdDirs - this is + // then the only chance to remove what the abandoned edit left on disk. + const createdDirs = this.createdDirs + this.createdDirs = [] + // Same reason for the placeholder: an await below may reject and the caller then runs + // reset(), which drops this field too. + const placeholderPath = this.placeholderPath + this.placeholderPath = undefined + + let editorFailure: unknown + // open() creates the directories and the empty placeholder BEFORE it assigns + // activeDiffEditor (openDiffEditor() can reject on its 10s timeout or a failed + // vscode.diff call), so an abandoned create can leave artifacts on disk with no + // editor at all. Only the buffer and tab work needs the editor; the artifact + // cleanup below runs either way. + if (this.activeDiffEditor) { + const document = this.activeDiffEditor.document + try { + this.disposeActiveEditorListener() + this.cancelDeferredScroll() + await this.closeAllDiffViews() + + if (document.isDirty) { + // Restore the buffer to what this edit started from - the empty placeholder for a + // create, the content already on disk for a modify. A failed applyEdit leaves the + // unapproved streamed content in the buffer, and saving then would persist exactly + // what this method exists to discard, so the save is conditional and the failure is + // surfaced to the caller as a rollback hazard. + const edit = new vscode.WorkspaceEdit() + const fullRange = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ) + const restoredContent = + this.editType === "modify" ? this.stripAllBOMs(this.originalContent ?? "") : "" + edit.replace(document.uri, fullRange, restoredContent) + const applied = await vscode.workspace.applyEdit(edit) + if (!applied) { + editorFailure = new Error( + `Could not restore the diff editor buffer for ${this.relPath}; it may still hold unapproved content.`, + ) + } else if (this.editType !== "modify") { + // Only the emptied placeholder is ever saved: this edit created that file, and + // closing a dirty tab would prompt. A modify is never saved here - a modify's + // restore is an in-memory revert, and saving it would write to a file the user + // never approved (see the method comment). + await document.save() + } + } + + await this.closeFileTab(absolutePath) + } catch (error) { + // Do NOT stop here: the placeholder and the created directories still have + // to go. The original failure is re-thrown once the artifacts are dealt with, + // so the caller still reports the rollback hazard instead of a silent success. + editorFailure = error + } + } + + let cleanupFailure: unknown + try { + // Only a placeholder THIS edit created may be removed. relPath survives reset(), and + // saveDirectly() sets it for a file that was written with approval, so unlinking + // absolutePath unconditionally can delete content the user approved. + if (placeholderPath) { + await fs.unlink(placeholderPath).catch((error: unknown) => { + // open() creates the placeholder; if it is already gone there is nothing + // left to remove and the discard did its job. + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + }) + } + + // Remove only the directories this edit created, in reverse order. + for (let i = createdDirs.length - 1; i >= 0; i--) { + await fs.rmdir(createdDirs[i]).catch((error: unknown) => { + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + }) + } + } catch (error) { + cleanupFailure = error + console.error("Error removing abandoned write_to_file artifacts:", error) + } + + // The stream session is over whichever way this returns. Task.dispose() runs no reset() + // after the discard, so leaving isEditing true with the editor still referenced would + // keep a disposed task pointing at a live diff view - and the idempotency guard in + // WriteToFileTool.discardDiffViewOpenedAfterRelease() reads isEditing to decide whether + // a late continuation still has work to do. reset() itself is not called here: it closes + // the diff views again, and the callers that do run it still do. + this.isEditing = false + this.editType = undefined + this.activeDiffEditor = undefined + this.originalContent = undefined + this.streamedLines = [] + this.userTouchedDocument = false + this.userTouchedDiffEditor = false + + if (editorFailure) { + throw editorFailure instanceof Error ? editorFailure : new Error(String(editorFailure)) + } + if (cleanupFailure) { + throw cleanupFailure instanceof Error ? cleanupFailure : new Error(String(cleanupFailure)) + } + } + async revertChanges(): Promise { if (!this.relPath || !this.activeDiffEditor) { return @@ -537,6 +690,7 @@ export class DiffViewProvider { // opened tab before deleting it from disk. await this.closeFileTab(absolutePath) await fs.unlink(absolutePath) + this.placeholderPath = undefined // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { @@ -1113,6 +1267,7 @@ export class DiffViewProvider { this.isEditing = false this.originalContent = undefined this.createdDirs = [] + this.placeholderPath = undefined this.documentWasOpen = false this.documentWasPinned = false this.activeDiffEditor = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..845c9aa846 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,3 +1,4 @@ +import * as fs from "fs/promises" import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" import * as vscode from "vscode" import * as path from "path" @@ -15,6 +16,10 @@ vi.mock("delay", () => ({ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), + // The abandoned-stream discard removes the placeholder file and the directories it + // created, so the discard path needs these two. + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), })) @@ -1852,5 +1857,410 @@ describe("DiffViewProvider", () => { expect(closeFileTab).not.toHaveBeenCalled() expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + // An abandoned partial stream was never approved. revertChanges() saves a dirty + // new-file buffer before deleting the file, which would put partial model output on + // disk (and leave it there if the delete fails), so the discard path blanks the + // buffer first and only then saves the empty placeholder before removing it. + describe("discardUnapprovedStream method", () => { + const makeAbandonedDocument = (callOrder: string[], isDirty = true) => ({ + isDirty, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: { fsPath: mockTargetPath, path: mockTargetPath }, + save: vi.fn(async () => { + callOrder.push("save") + }), + }) + + const openAbandonedView = (document: unknown, createdDirs: string[], callOrder: string[]) => + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs, + // open() records the placeholder it wrote; the discard may only delete what it owns. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + closeAllDiffViews: vi.fn(async () => { + callOrder.push("closeDiffViews") + }), + closeFileTab: vi.fn(async () => { + callOrder.push("closeFileTab") + }), + }) + + it("leaves no session behind for a task that was disposed while the discard ran", async () => { + // Task.dispose() starts this discard and runs no reset() afterwards, so the discard + // itself has to end the session. isEditing and the editor reference are what a late + // handlePartial() continuation reads to decide whether it still owns a view, and a + // disposed task must not keep either - nor the streamed lines it just discarded. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + diffViewProvider.isEditing = true + diffViewProvider["streamedLines"] = ["partial model output"] + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.discardUnapprovedStream() + + expect(diffViewProvider.isEditing).toBe(false) + expect(diffViewProvider.editType).toBeUndefined() + expect(diffViewProvider["activeDiffEditor"]).toBeUndefined() + expect(diffViewProvider["streamedLines"]).toEqual([]) + }) + + it("never persists the unapproved buffer: blanks it with an empty replacement, then deletes the placeholder", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["closeDiffViews", "applyEdit", "save", "closeFileTab"]) + // The only content the discard may write is an empty buffer, and it must be + // written to THIS document before the save that would otherwise persist the + // partial model output. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + // The replacement covers the whole buffer: the range is built from + // positionAt(0) to positionAt(getText().length), so nothing can survive it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(replacedText).toBe("") + expect(mockWorkspaceEdit.delete).not.toHaveBeenCalled() + // The placeholder for THIS relPath is what gets removed. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + }) + + it("restores a modify buffer in memory and never saves the target file", async () => { + // revertChanges() restores a modify by applyEdit + document.save(). For a stream the + // user never approved that save is a write they never asked for, and for a + // .rooignore-denied path a write the policy forbids outright, so the discard restores + // the buffer and leaves the file on disk untouched. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + Object.assign(diffViewProvider, { + editType: "modify", + originalContent: "original content", + // A modify owns no placeholder: only open() of a create writes one. + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["closeDiffViews", "applyEdit", "closeFileTab"]) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + expect(replacedText).toBe("original content") + // The replacement covers the whole buffer, so no streamed content survives it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(document.save).not.toHaveBeenCalled() + // Nothing is unlinked for a modify: saveChanges() may have written this very file with + // the user's approval, and this edit created no directories. + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("does not save and reports the hazard when the buffer restore fails to apply", async () => { + // applyEdit resolving false leaves the unapproved partial content in the buffer. Saving + // then would persist exactly what this method exists to discard, so the save is skipped + // and the caller is told the rollback did not happen. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + /could not restore the diff editor buffer/i, + ) + + expect(document.save).not.toHaveBeenCalled() + // The artifacts still have to go: the caller runs reset() next, which drops + // placeholderPath/createdDirs and leaves nothing else able to remove them. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + }) + + it("removes the directories this edit created in reverse order, after the placeholder unlink", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`, `${mockCwd}/mock-dir/nested`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledTimes(createdDirs.length) + // Reverse order, so a parent is never removed before the child inside it, and + // only the directories this edit created - never a pre-existing one. + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => c[0])).toEqual([...createdDirs].reverse()) + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + for (const order of vi.mocked(fs.rmdir).mock.invocationCallOrder) { + expect(order).toBeGreaterThan(unlinkOrder) + } + }) + + it("does not save when the abandoned buffer is already clean", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder, false) + openAbandonedView(document, [], callOrder) + + await diffViewProvider.discardUnapprovedStream() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(document.save).not.toHaveBeenCalled() + expect(mockWorkspaceEdit.replace).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledTimes(1) + }) + + it("still removes the placeholder and the created directories when the editor work rejects, and reports the failure", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) + + // The caller (releaseAbandonedDiffView) treats a throw as "rollback hazard", so + // the failure must still surface - but only after the artifacts are gone, because + // the caller then runs reset(), which drops relPath/createdDirs and leaves + // nothing else able to remove them. + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") + + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + }) + + it("removes the placeholder and created directories even when open() failed before activeDiffEditor was assigned", async () => { + // open() creates the directories (line 130) and the empty placeholder (line 134) + // BEFORE openDiffEditor() assigns activeDiffEditor (line 173). If that call rejects + // - the 10s timeout or a failed vscode.diff - an abandoned create therefore has + // artifacts on disk and no editor, and the early return must not skip the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/mock-dir`], + // open() had already written the placeholder before openDiffEditor() rejected, + // so this edit owns the path. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + // Nothing editor-side ran, and nothing threw: the artifacts are the whole job here. + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + // Bracket access for the private field: the snapshot must be consumed, so a second + // discard cannot try to remove the same directories again. + expect(diffViewProvider["createdDirs"]).toEqual([]) + }) + + it("logs and rejects when the placeholder unlink fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const permissionError = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + vi.mocked(fs.unlink).mockRejectedValueOnce(permissionError) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + "EPERM: operation not permitted", + ) + + expect(errorSpy).toHaveBeenCalledWith( + "Error removing abandoned write_to_file artifacts:", + permissionError, + ) + }) + + it("logs and rejects when removing a created directory fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + const notEmpty = Object.assign(new Error("ENOTEMPTY: directory not empty"), { code: "ENOTEMPTY" }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(notEmpty) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + "ENOTEMPTY: directory not empty", + ) + + expect(errorSpy).toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", notEmpty) + }) + + it("tolerates an already-deleted placeholder and directories (ENOENT) without throwing", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + // Two created directories: the tolerance has to hold for every one of them, so the + // rejection is keyed on the path rather than on which call comes first. + const dirA = `${mockCwd}/mock-dir-a` + const dirB = `${mockCwd}/nested/mock-dir-b` + openAbandonedView(document, [dirA, dirB], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const enoent = Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + // A single unlink call in this flow, so the placeholder stays Once-based. + vi.mocked(fs.unlink).mockRejectedValueOnce(enoent) + vi.mocked(fs.rmdir).mockImplementation(async (dir) => { + if (dir === dirA || dir === dirB) { + throw enoent + } + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + // Both directories were attempted; the assertion is on the paths, not the order. + expect(fs.rmdir).toHaveBeenCalledWith(dirA) + expect(fs.rmdir).toHaveBeenCalledWith(dirB) + expect(errorSpy).not.toHaveBeenCalledWith( + "Error removing abandoned write_to_file artifacts:", + expect.anything(), + ) + + // beforeEach only clears call data, not implementations, so put the default back. + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + }) + + it("reports the editor failure rather than the cleanup failure when both happen", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + vi.spyOn(console, "error").mockImplementation(() => {}) + + // The caller reports a rollback hazard from the thrown error, so the ORIGINAL failure + // is what must surface; the cleanup failure is still logged for the operator. + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") + }) + + it("leaves a file this edit never created a placeholder for on disk", async () => { + // reset() does not clear relPath, and saveDirectly() sets it for an approved write. + // Without ownership tracking the discard would unlink that approved file: an approved + // write_to_file to A, then a new write_to_file whose block exits through a rollback + // path before open() ever ran, still sees relPath === A. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + Object.assign(diffViewProvider, { + relPath: "previously-approved-file.ts", + activeDiffEditor: { document }, + editType: undefined, + createdDirs: [], + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // The buffer is still blanked and the tab closed: the unapproved content is gone + // without touching a file the user approved. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0][2]).toBe("") + }) + + it("removes the placeholder that a new-file open() wrote, driven through the public methods", async () => { + // The ownership lifecycle lives at the public-method layer, so exercise it there: + // open() writes the empty placeholder and records it, and the discard of an abandoned + // create removes exactly that path. + const relPath = "owned-placeholder.ts" + const fsPath = `${mockCwd}/${relPath}` + const mockEditor = { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue(""), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + lineCount: 0, + }, + selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } }, + edit: vi.fn().mockResolvedValue(true), + revealRange: vi.fn(), + } + // Structural double for the mocked editor: the mock only implements the members + // open() touches, so it is routed through unknown rather than any. + const editor = mockEditor as unknown as vscode.TextEditor + vi.mocked(vscode.window).visibleTextEditors = [editor] + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(editor) + vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { + setTimeout(() => callback({ uri: { fsPath, scheme: "file" } } as vscode.TextDocument), 0) + return { dispose: vi.fn() } + }) + vi.mocked(vscode.window.onDidChangeVisibleTextEditors).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.window.onDidChangeTextEditorVisibleRanges).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + diffViewProvider.editType = "create" + + await diffViewProvider.open(relPath) + + // open() created the placeholder, and this edit now owns it. + expect(fs.writeFile).toHaveBeenCalledWith(fsPath, "") + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(fsPath) + }) + + it("does not remove the file once saveChanges() has approved the content", async () => { + // saveChanges() turns the placeholder into approved content and drops this edit's + // claim on it, so a later abandoned cleanup must leave the approved file on disk. + const relPath = "approved-by-save.ts" + const fsPath = `${mockCwd}/${relPath}` + Object.assign(diffViewProvider, { + relPath, + newContent: "approved content", + editType: "create", + createdDirs: [], + placeholderPath: fsPath, + preDiagnostics: [], + activeDiffEditor: { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue("approved content"), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + }, + }, + closeAllDiffViews: vi.fn().mockResolvedValue(undefined), + closeFileTab: vi.fn().mockResolvedValue(undefined), + }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(undefined as never) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.saveChanges(false, 0) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("does nothing when no abandoned view is open", async () => { + Object.assign(diffViewProvider, { relPath: undefined, activeDiffEditor: undefined }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) }) })