diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index c0e8a6cd1a..7eb40d9585 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -293,12 +293,14 @@ export function parseVitestTestFiles(report, runRoot) { } export function preferDirectTestFiles(testFiles, sourceFiles) { + // Spec files follow the lowerCamel source-name convention (e.g. + // writeToFileTool.spec.ts for WriteToFileTool.ts), so match case-insensitively. const sourceNames = sourceFiles.map((sourceFile) => path.posix.basename(sourceFile, path.posix.extname(sourceFile))) const direct = testFiles.filter((testFile) => { - const testName = path.posix.basename(testFile) + const testName = path.posix.basename(testFile).toLowerCase() return sourceNames.some( (sourceName) => - testName.startsWith(`${sourceName}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), + testName.startsWith(`${sourceName.toLowerCase()}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), ) }) return direct.length > 0 ? direct : testFiles diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index 0f39dc507f..0792ec5a1c 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -208,6 +208,20 @@ describe("preferDirectTestFiles", () => { ]) assert.deepEqual(preferDirectTestFiles(related, ["webview-ui/src/utils/unmatched.ts"]), related) }) + + it("matches lowerCamel spec names against PascalCase sources case-insensitively", () => { + const related = [ + "src/core/tools/__tests__/writeToFileTool.spec.ts", + "src/core/task/__tests__/Task.spec.ts", + "src/core/tools/__tests__/presentAssistantMessage-custom-tool.spec.ts", + ] + assert.deepEqual( + preferDirectTestFiles(related, ["src/core/tools/WriteToFileTool.ts", "src/core/task/Task.ts"]), + ["src/core/tools/__tests__/writeToFileTool.spec.ts", "src/core/task/__tests__/Task.spec.ts"], + ) + // No source with a matching spec name: fall back to all related tests. + assert.deepEqual(preferDirectTestFiles(related, ["src/core/tools/ReadFileTool.ts"]), related) + }) }) describe("related-test discovery", () => { diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts index 1ef25e852b..3e2685bf33 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -79,6 +79,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }, say: vi.fn().mockResolvedValue(undefined), ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked" }), + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), } // Add pushToolResultToUserContent method after mockTask is created so it can reference mockTask diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..27c8742f4a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -66,7 +66,7 @@ import { ApiStream, GroundingSource } from "../../api/transform/stream" import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" // shared -import { findLastIndex } from "../../shared/array" +import { findLast, findLastIndex } from "../../shared/array" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" import { t } from "../../i18n" @@ -1212,6 +1212,15 @@ 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. + */ private async saveClineMessages(): Promise { try { await saveTaskMessages({ @@ -1219,7 +1228,12 @@ export class Task extends EventEmitter implements TaskLike { taskId: this.taskId, globalStoragePath: this.globalStoragePath, }) + } catch (error) { + console.error("Failed to save Roo messages:", error) + return false + } + try { if (this._taskApiConfigName === undefined) { await this.taskApiConfigReady } @@ -1247,11 +1261,14 @@ export class Task extends EventEmitter implements TaskLike { const provider = this.providerRef.deref() const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) - return true } catch (error) { - console.error("Failed to save Roo messages:", error) - return false + // The message array was persisted above; a metadata or task-history failure must + // not mask that write (see the method docs). The next saveClineMessages() call + // recomputes and re-emits the metadata update. + console.error("Failed to save task metadata:", error) } + + return true } private findMessageByTimestamp(ts: number): ClineMessage | undefined { @@ -1955,6 +1972,56 @@ export class Task extends EventEmitter implements TaskLike { return formatResponse.toolError(formatResponse.missingToolParameterError(paramName)) } + /** + * Finalize a partial "tool" ask message without blocking for user input. + * Call this in error paths where a partial tool message was opened during streaming + * but execution failed before the normal approval flow could close it, so the webview + * spinner does not get stuck in a loading state. + * + * The matching partial message may no longer be the final entry if another asynchronous + * message was inserted between the partial ask and the error handler, so search backward + * instead of relying on clineMessages.at(-1). + * + * Any in-progress `progressStatus` on the message is cleared as well: the ask is being + * finalized because it will NOT complete, so a stale "in progress" indicator would be + * misleading (the normal completion path overwrites it with the final status instead). + * + * `isAnswered` is stamped true because the ask is resolved by the system rather than + * by the user: ChatView only shows ask buttons for unanswered messages, so leaving it + * unset would keep Save/Reject armed for a write that already failed. + */ + async finalizePartialToolAsk(text?: string): Promise { + const partialToolAsk = findLast( + this.clineMessages, + (message) => + message.partial === true && + message.type === "ask" && + message.ask === "tool" && + (text === undefined || message.text === text), + ) + + if (!partialToolAsk) { + return + } + + partialToolAsk.partial = false + partialToolAsk.progressStatus = undefined + partialToolAsk.isAnswered = true + const saved = await this.saveClineMessages() + if (!saved) { + // The persistence write failed: the on-disk record still carries `partial: true` + // while the in-memory message is finalized. Skip the webview-only update so the + // two views do not diverge (a later state resync or restart reload would flip the + // spinner back on from the stale disk record). The next saveClineMessages() call + // re-persists the full message array and repairs the disk record. + console.error("[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update") + return + } + await this.updateClineMessage(partialToolAsk).catch((error) => { + console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error) + }) + } + // Lifecycle // Start / Resume / Abort / Dispose diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..89b410b487 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1,5 +1,6 @@ // npx vitest core/task/__tests__/Task.spec.ts +import * as fsReal from "fs" import * as os from "os" import * as path from "path" @@ -10,6 +11,7 @@ import type { Mock } from "vitest" import { providerIdentifiers, RooCodeEventName, + type ClineMessage, type GlobalState, type ProviderSettings, type ModelInfo, @@ -3640,6 +3642,402 @@ describe("Cline", () => { saveSpy.mockRestore() }) + it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { + let updateSnapshot: Record | undefined + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockImplementation(async (message) => { + updateSnapshot = { ...message } + }) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + progressStatus: { text: "Generating…", icon: "sync" }, + } + + task.clineMessages.push(partialToolAsk) + task.clineMessages.push({ + ts: Date.now() - 1, + type: "say", + say: "error", + text: "intervening async message", + }) + + await task.finalizePartialToolAsk("partial tool message") + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(partialToolAsk.progressStatus).toBeUndefined() + // The ask is resolved by the system, not the user: stamp isAnswered so ChatView + // does not keep Save/Reject armed for a write that already failed. + expect(task.clineMessages[0].isAnswered).toBe(true) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + expect(updateSnapshot?.partial).toBe(false) + expect(updateSnapshot?.progressStatus).toBeUndefined() + expect(updateSnapshot?.isAnswered).toBe(true) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => { + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + task.clineMessages.push({ + ts: Date.now() - 1, + type: "ask", + ask: "tool", + text: "other partial tool message", + partial: true, + }) + + await task.finalizePartialToolAsk("target partial tool message") + await flushMicrotasks() + + expect(task.clineMessages[0].partial).toBe(true) + expect(task.clineMessages[0].isAnswered).toBeUndefined() + expect(saveSpy).not.toHaveBeenCalled() + expect(updateSpy).not.toHaveBeenCalled() + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk updates the latest partial tool ask when no text is provided", async () => { + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const olderPartialToolAsk = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "tool" as const, + text: "older partial tool message", + partial: true, + } + const latestPartialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "latest partial tool message", + partial: true, + } + + task.clineMessages.push(olderPartialToolAsk) + task.clineMessages.push(latestPartialToolAsk) + + await task.finalizePartialToolAsk() + await flushMicrotasks() + + expect(olderPartialToolAsk.partial).toBe(true) + expect(task.clineMessages[0].isAnswered).toBeUndefined() + expect(latestPartialToolAsk.partial).toBe(false) + // Only the finalized ask is stamped answered; the untouched one is not. + expect(task.clineMessages[1].isAnswered).toBe(true) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(latestPartialToolAsk) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk logs (instead of rejecting) when updateClineMessage rejects", async () => { + // Pins the .catch arm on updateClineMessage in finalizePartialToolAsk: the + // partial flag must already be persisted (saveClineMessages ran first), the + // failure must only be logged, and finalize must still resolve so callers' + // error-path cleanup (diff-view reset, resetTaskPartialState) always completes. + const boom = new Error("updateClineMessage boom") + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockImplementation(async () => { + throw boom + }) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + expect(saveSpy).toHaveBeenCalled() + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] updateClineMessage failed:", + boom, + ) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk logs and skips the webview update when persistence fails", async () => { + // Pins the saveClineMessages-failure guard in finalizePartialToolAsk: while the + // on-disk record still carries partial: true, a webview-only update would diverge + // the two views (a later state resync or restart reload would flip the spinner + // back on). finalize must log, skip updateClineMessage, and still resolve so + // callers' error-path cleanup completes. + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(false) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + // The in-memory message is still finalized so the ask is resolved by the system... + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // ...and the persistence failure is observed instead of silently swallowed. + expect(saveSpy).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update", + ) + // ...but the webview update is skipped so disk (still partial: true) and + // webview do not diverge until the next save repairs the record. + expect(updateSpy).not.toHaveBeenCalled() + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + + it("finalizePartialToolAsk still updates the webview when a later save stage fails", async () => { + // saveClineMessages() reports the message write separately from the metadata / + // task-history stages: the message array persisted while a later stage failed + // must still count as a successful save, so the finalized ask reaches the + // webview. A stale metadata entry is recomputed and re-emitted by the next + // saveClineMessages() call. + // saveTaskMessages() persists through safeWriteJson, which only mocks the + // fs/promises write helpers: its real fs.access gate and lockfile need the + // task directory to exist (uuid v7 is mocked to the fixed id below). + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.mkdirSync(taskDir, { recursive: true }) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const metadataFailure = new Error("task history stage failed") + const historySpy = vi.spyOn(mockProvider, "updateTaskHistory").mockRejectedValueOnce(metadataFailure) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // The message array persisted, so the webview update must run even though a + // later save stage failed... + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + // ...and the later-stage failure is observed instead of silently swallowed. + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to save task metadata:", expect.any(Error)) + + updateSpy.mockRestore() + historySpy.mockRestore() + }) + + it("finalizePartialToolAsk skips the webview update when the message write itself fails", async () => { + // Complements the later-stage-failure test above by failing the first save + // stage: with the real task directory removed, safeWriteJson's fs.access + // throws before anything is persisted, saveClineMessages() reports the + // failed message write, and the skip guard keeps the webview update off. + const taskDir = path.join(os.tmpdir(), "test-storage", "tasks", "00000000-0000-7000-8000-000000000000") + fsReal.rmSync(taskDir, { recursive: true, force: true }) + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const partialToolAsk = { + ts: Date.now() - 1, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + + task.clineMessages.push(partialToolAsk) + + await expect(task.finalizePartialToolAsk("partial tool message")).resolves.toBeUndefined() + await flushMicrotasks() + + // The in-memory ask is still finalized... (the flags are set before saving) + expect(partialToolAsk.partial).toBe(false) + expect(task.clineMessages[0].isAnswered).toBe(true) + // ...but the failed message write skips the webview update and surfaces + // both failure logs instead of updating on an unpersisted save. + expect(updateSpy).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to save Roo messages:", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[Task#finalizePartialToolAsk] saveClineMessages failed; skipping webview update", + ) + + // Restore the directory for sibling tests that persist through the real fs. + fsReal.mkdirSync(taskDir, { recursive: true }) + + updateSpy.mockRestore() + }) + + it("finalizePartialToolAsk ignores partial asks that match only some predicate clauses", async () => { + // Each distractor below satisfies a strict subset of the findLast predicate + // clauses, so no single clause (or a wrong combination of clauses) may select + // it: partial, type, ask kind, and text must all hold together. + const updateSpy = vi + .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") + .mockResolvedValue(undefined) + const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const partialToolAsk: ClineMessage = { + ts: Date.now() - 4, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: true, + } + // Completed (non-partial) tool ask with the same text. + const completedToolAsk: ClineMessage = { + ts: Date.now() - 3, + type: "ask" as const, + ask: "tool" as const, + text: "partial tool message", + partial: false, + } + // Partial ask of a different kind. + const nonToolAsk: ClineMessage = { + ts: Date.now() - 2, + type: "ask" as const, + ask: "completion_result" as const, + text: "partial tool message", + partial: true, + } + // Deliberately malformed: a "say" message that still carries the tool ask + // fields. The ClineMessage schema allows both fields, and the predicate under + // test reads message.ask on any message, so this is exactly the distractor + // the type clause exists to filter out. + const sayWithToolAsk: ClineMessage = { + ts: Date.now() - 1, + type: "say" as const, + say: "error" as const, + text: "partial tool message", + partial: true, + ask: "tool" as const, + } + + task.clineMessages.push(partialToolAsk) + task.clineMessages.push(completedToolAsk) + task.clineMessages.push(nonToolAsk) + task.clineMessages.push(sayWithToolAsk) + + await task.finalizePartialToolAsk("partial tool message") + await flushMicrotasks() + + expect(partialToolAsk.partial).toBe(false) + expect(partialToolAsk.isAnswered).toBe(true) + // Every distractor stays untouched: only the genuine partial tool ask is + // finalized. + expect(completedToolAsk.partial).toBe(false) + expect(completedToolAsk.isAnswered).toBeUndefined() + expect(nonToolAsk.partial).toBe(true) + expect(nonToolAsk.isAnswered).toBeUndefined() + expect(sayWithToolAsk.partial).toBe(true) + expect(saveSpy).toHaveBeenCalledTimes(1) + expect(updateSpy).toHaveBeenCalledTimes(1) + expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) + + updateSpy.mockRestore() + saveSpy.mockRestore() + }) + it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { // Pins the .catch arm on the fire-and-forget updateClineMessage call // in ask() when a new partial ask arrives while the previous partial diff --git a/src/core/task/__tests__/Task.throttle.test.ts b/src/core/task/__tests__/Task.throttle.test.ts index eaacb32faf..989af6d2f1 100644 --- a/src/core/task/__tests__/Task.throttle.test.ts +++ b/src/core/task/__tests__/Task.throttle.test.ts @@ -69,6 +69,8 @@ describe("Task token usage throttling", () => { beforeEach(() => { // Reset all mocks vi.clearAllMocks() + // console.log is intentionally not spied: the previous spy masked a Vitest + // worker-teardown race under --coverage, not a real task rejection. vi.useFakeTimers() // Mock provider diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dd15059662 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -156,6 +156,13 @@ export abstract class BaseTool { } } catch (error) { console.error(`Error parsing parameters:`, error) + // 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) + }) 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)) // Note: handleError already emits a tool_result via formatResponse.toolError in the caller. diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ae026b4b86..de860183e0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -2,7 +2,7 @@ import path from "path" import delay from "delay" import fs from "fs/promises" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS, RooCodeEventName } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -23,9 +23,135 @@ 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 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, + 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 + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + const state = this.taskPartialStreamState.get(key) + if (!state) { + return + } + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + this.taskPartialStreamState.delete(key) + } + + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + + /** + * Restore the diff editor document to its pre-streaming state and close the view. + * + * reset() clears the provider's state but leaves the diff document dirty with the + * streamed content; a user save would then persist a write the task never completed + * (denied or failed before approval). Must run BEFORE resetDiffViewAfterWrite(), + * since reset() clears the state revertChanges() relies on. No-op when no diff view + * is open. Failures are logged and swallowed so the remaining cleanup (reset, + * per-task state teardown) always continues. + */ + private async revertDiffChangesBeforeReset(task: Task): Promise { + await task.diffViewProvider.revertChanges().catch((revertError) => { + console.error("Error reverting write_to_file diff view changes:", revertError) + }) + } + + private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { + await task.finalizePartialToolAsk(text).catch((finalizeError) => { + console.error("Error finalizing write_to_file partial tool ask:", finalizeError) + }) + } + + 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 @@ -35,7 +161,14 @@ 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.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } @@ -43,7 +176,12 @@ 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.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } @@ -52,6 +190,19 @@ 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.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } @@ -67,12 +218,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") } @@ -96,7 +241,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() @@ -133,6 +290,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) } else { if (!task.diffViewProvider.isEditing) { @@ -166,6 +325,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } + writeApproved = true + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } @@ -179,17 +340,28 @@ 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) { + // 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) await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() + // Before approval the diff document holds unapproved streamed content: restore it + // so a user save cannot persist it. After approval the content is the user's + // accepted edit -- keep it in the editor (dirty) so they can save it manually. + if (!writeApproved) { + await this.revertDiffChangesBeforeReset(task) + } + await this.resetDiffViewAfterWrite(task) return + } finally { + this.resetTaskPartialState(task) } } @@ -197,8 +369,21 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content + const partialStreamFailureKey = this.getPartialStreamFailureKey(task) + + // A prior streaming delta for this task already hit a fatal filesystem error. + // Skip further streaming work so we don't create a new partial tool message on every + // subsequent delta. execute() will report the error once when the block completes. + if (this.taskPartialStreamState.get(partialStreamFailureKey)?.streamFailed) { + return + } + + // Get (or create) this task's state; registers the TaskAborted teardown listener + // once, so abandoned streams are torn down even if execute() never runs. + const partialStreamState = this.getTaskPartialStreamState(task) + // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath) || newContent === undefined) { + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { return } @@ -224,12 +409,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -245,14 +424,34 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.ask("tool", partialMessage, block.partial).catch(() => {}) if (newContent) { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) - } + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // rethrow: the same filesystem operation is retried in execute() once the + // block completes, and that authoritative non-partial path reports the + // error to the user. Surfacing it here too would show the same error twice. + // Swallowing it here is safe because the agent loop advances naturally when + // the non-partial block arrives (it does not depend on this throw). + console.error(`Error streaming write_to_file diff view:`, error) + // Mark the stream as failed so later deltas don't re-attempt and spawn a new + // partial tool message each time. + partialStreamState.streamFailed = true + await this.finalizePartialToolAskAfterFailure(task, partialMessage) + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty). + await this.revertDiffChangesBeforeReset(task) + await this.resetDiffViewAfterWrite(task) + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..b1e40797cb 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,7 @@ describe("writeToFileTool", () => { update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", userEdits: null, @@ -186,8 +206,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,8 +248,13 @@ 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 the casts are required to + // model a malformed payload. + path: (Object.prototype.hasOwnProperty.call(params, "path") ? params.path : testFilePath) as any, + content: (Object.prototype.hasOwnProperty.call(params, "content") + ? params.content + : testContent) as any, }, partial: isPartial, } @@ -250,6 +279,123 @@ 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 revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.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 revertChanges() 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 revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + await streamPartialAsk() + + await executeWriteFileTool({ content: undefined }, { fileExists: false }) + + 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 revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + 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. + // Record the relative order of revertChanges() and reset(): vitest mocks expose + // no invocationCallOrder, so the ordering assertion uses this sequence. + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + await streamPartialAsk() + + await executeWriteFileTool({ path: undefined }, { fileExists: false }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "path") + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The diff document must be reverted before reset() clears the state + // revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) }) describe("file existence detection", () => { @@ -287,15 +433,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 +539,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 +585,168 @@ 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() + } + }) + }) + + 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, + 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 +788,573 @@ 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 revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + + // Second call - path stabilized, update() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("does not spawn a new partial tool message on each streaming delta after a failure", async () => { + // Regression test: after diffViewProvider.open() throws and the partial message is + // finalized + diff view reset, the next streaming delta saw a non-partial last message + // and created a brand new "Zoo wants to edit this file" message -- repeating once per + // delta. After the fix, partialStreamFailed short-circuits subsequent deltas so only + // the single initial partial ask is issued. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, ask issued once, open() fails, stream marked failed + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Deltas 3..5 - must be short-circuited, no further asks + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Only the single partial ask from delta 2 should have been issued + expect(mockCline.ask).toHaveBeenCalledTimes(1) + // open() must not be retried after the first failure + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + }) + + it("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. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + 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(), + }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + 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 revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Stream two deltas so the diff view is open with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + }) + + it("continues cleanup when reverting the diff document fails before approval", async () => { + // Pins the .catch arm on revertChanges() in revertDiffChangesBeforeReset(): a failed + // revert (e.g. the diff view was already closed) must only be logged so the + // remaining cleanup (diff view reset + per-task state teardown) always completes. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + + // Stream two deltas so the diff view opens with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval and the revert fails too. + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error reverting write_to_file diff view changes:", + expect.any(Error), + ) + // The diff view is still reset and the per-task stream state still torn down. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, expect.any(Function)) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps approved diff content in the editor when saving fails after approval", async () => { + // The reverse of the previous test: once the user approved the write, the diff + // content is their accepted edit. A late failure (e.g. saveChanges rejecting) + // must NOT revert it -- the document stays dirty so the user can save it manually. + mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("continues execute error cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + // The execute error path finalizes without a text match: any open partial + // tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + 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.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("continues partial failure cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // 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.revertChanges).toHaveBeenCalledTimes(1) + }) + }) + + describe("prevent focus disruption experiment", () => { + /** + * Enable the PREVENT_FOCUS_DISRUPTION experiment for the current task: the experiment + * branches in execute()/handlePartial() read it from the provider state they fetch. + */ + function enablePreventFocusDisruption(): void { + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + } + + it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + expect(toolResult).toBe("Tool result message") + }) + + it("keeps approved diff content when saveDirectly fails after approval", async () => { + // The experiment branch stamps writeApproved before saveDirectly, so a late failure + // must NOT revert the document (the user approved the edit and can save it + // manually) but must still finalize the partial ask and reset the diff view. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + enablePreventFocusDisruption() + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("skips streaming diff view work when the experiment is enabled", async () => { + // With the experiment enabled the tool preview is embedded in the complete message + // built in execute(), so handlePartial must not open or update the diff view while + // streaming. + enablePreventFocusDisruption() + + // Delta 1 - stabilize path; delta 2 - path stabilized but the experiment short-circuits + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() }) }) })