From 2fa500ae469eb61549cc852383dc0049f3fbca0c Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 03:40:53 +0800 Subject: [PATCH 1/4] fix(write-to-file): close the parse-failure and post-commit teardown gaps This is the U3 unit content re-based onto main's tip 73931fe8be (sha from ls-remote of upstream refs/heads/main at the moment of re-basing, not a rev-parse of a local ref), not merged into it. The parent is main, so this branch is an ancestor of main and the PR diff is the unit's own work: 10 paths, no merge commit, no 105-file refresh riding along. Why re-base rather than merge. The unit's nine spec hunks land inside src/integrations/editor/ __tests__/DiffViewProvider.spec.ts, which main rewrote onto its typed test harness (main's copy: 1780 lines, 71 titles, 0 'as any', 56 makeTextEditor calls; the branch's copy: 2670 lines, 99 titles, 293 'as any', 3 makeTextEditor calls). Merging forced the two harnesses to coexist; re-basing keeps main's helper form and ports the unit's assertions onto it, which is what main's rewrite asks for. What the port changed, so the diff is not a mystery: - The shared double builder gained a construction-time dirty flag: buildActiveDiffEditor(docOverrides) passes docOverrides.isDirty into makeTextDocument. The five tests that needed a dirty document now build one with buildActiveDiffEditor({ isDirty: true }) instead of writing editor.document.isDirty = true after the fact. That is main's own idiom for a dirty document (its spec passes { isDirty: true } into makeTextDocument, and its harness has no setter because the type's isDirty is readonly), and it is equivalent here: the double is not handed to the code under test until after the fixture lines, so no read of isDirty can observe the difference between constructed-dirty and made-dirty. - 29 'as any' casts on the branch's copy became bracket writes or typed doubles; the file now has 0. - The 5 remaining type errors from the merge attempt (TS2540 on the readonly isDirty) are gone. Verification on this head, all with the pinned pnpm 10.8.1 and hooks running: - eslint --max-warnings=0 --no-warn-ignored over all 10 paths: exit 0 each, with src/eslint-suppressions restored to main's blob and no --prune-suppressions, so the claim 'byte-identical to main's suppressions' is a diffed fact and not an inference. - pnpm --dir src exec tsc --noEmit: exit 0, 0 errors. - DiffViewProvider.spec.ts 99 passed (99) - all 99 branch titles survive, 0 lost; writeToFileTool.spec.ts 64 passed | 5 skipped (69); writeToFileTool-partial-state-cleanup.spec.ts 9 passed (9); removeClineFromStack-delegation.spec.ts 24 passed (24); ClineProvider.spec.ts 339 passed (339). Branch intent, unchanged by the port - read from the unit's own commit subjects: fail the approved save when document.save reports false (b97f68ea3), fail the rollback when the pre-stream restore edit is not applied (3daf33860), fail the existing-file rollback when the revert edit or save reports false (23213efef), stop a post-commit failure from rolling back a landed write (2a9bfabfb), plus the parse-failure and post-commit teardown gaps (5adf7ec98). --- .../removeClineFromStack-delegation.spec.ts | 41 + .../presentAssistantMessage.ts | 21 +- src/core/tools/BaseTool.ts | 43 +- src/core/tools/WriteToFileTool.ts | 602 ++++++++-- ...teToFileTool-partial-state-cleanup.spec.ts | 200 ++++ .../tools/__tests__/writeToFileTool.spec.ts | 1064 ++++++++++++++++- src/core/webview/ClineProvider.ts | 6 + src/integrations/editor/DiffViewProvider.ts | 308 ++++- .../editor/__tests__/DiffViewProvider.spec.ts | 857 ++++++++++++- src/utils/fs.ts | 16 +- 10 files changed, 3024 insertions(+), 134 deletions(-) create mode 100644 src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts diff --git a/src/__tests__/removeClineFromStack-delegation.spec.ts b/src/__tests__/removeClineFromStack-delegation.spec.ts index 0db228662e..ddbb655b16 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 PrivateClineProviderMethods = { removeClineFromStack: (this: ClineProvider) => ReturnType @@ -202,6 +203,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/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 73d48b2e7c..9d9867a382 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -577,15 +577,34 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request // Best-effort only } + // A streaming delta may have captured a fatal filesystem error for this task. + // handle() never runs on this path, and clearTaskState() (below) releases the + // state holding it, so the model must learn about it from this single result. + const retainedStreamError = + block.name === "write_to_file" ? writeToFileTool.getRetainedStreamError(cline) : undefined + const reportedMessage = retainedStreamError + ? `${retainedStreamError.message}\n${errorMessage}` + : errorMessage + // Push tool_result directly without setting didAlreadyUseTool so streaming can // continue gracefully. cline.pushToolResultToUserContent({ type: "tool_result", tool_use_id: sanitizeToolUseId(toolCallId), - content: formatResponse.toolError(errorMessage), + content: formatResponse.toolError(reportedMessage), is_error: true, }) + // handle() is never reached on this path, so the parse-failure teardown it + // performs must run here instead: a streaming delta may already have opened a + // partial write_to_file ask and left per-task failure state behind. + if (block.name === "write_to_file") { + writeToFileTool.clearTaskState(cline) + await cline.finalizePartialToolAsk().catch((finalizeError) => { + console.error("Error finalizing write_to_file partial ask:", finalizeError) + }) + } + break } } diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 1e73a8285a..42a5858f99 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,27 @@ 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..fb2e74692f 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,18 +22,285 @@ 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. + */ + /** + * The streaming failure retained for this task, if any. presentAssistantMessage's + * missing-nativeArgs guard runs before handle(), so it must surface this error itself + * before clearTaskState() releases the state that holds it. + */ + public getRetainedStreamError(task: Task): Error | undefined { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task))?.streamError + } + + public clearTaskState(task: Task): void { + this.resetTaskPartialState(task) + } + + /** + * Release this task's stream entry only while the map still holds THIS object. + * A newer stream for the same task must not be dropped by an older failure's + * cleanup, which would also deregister the newer stream's abort listener. + */ + private releaseTaskPartialStateByIdentity(state: TaskPartialStreamState): void { + if (this.taskPartialStreamState.get(this.getPartialStreamFailureKey(state.task)) === state) { + this.resetTaskPartialState(state.task) + } + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + const state = this.taskPartialStreamState.get(key) + 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, task.ask() and diffViewProvider.open() before it + * touches the diff view; a cancellation during any of those awaits runs the TaskAborted + * teardown (or a direct clearTaskState), which deletes this entry. Continuing would + * re-ask, re-open a diff view, or stream a partial delta into a view the teardown has + * already released for a task the user cancelled. Identity, not presence: a re-created + * entry for the same key belongs to a new stream, and this one must not write into it. + */ + private isPartialStreamStillLive(task: Task, state: TaskPartialStreamState): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) === state + } + + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + + /** + * 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 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 { + try { + await task.diffViewProvider.revertChanges() + return true + } catch (revertError) { + console.error("Error reverting write_to_file diff view changes:", revertError) + return false + } + } + + private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise { + 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 + } + + override resetPartialState(): void { + super.resetPartialState() + for (const state of this.taskPartialStreamState.values()) { + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + } + this.taskPartialStreamState.clear() + } + async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise { const { pushToolResult, handleError, askApproval } = callbacks const relPath = params.path let newContent = params.content + // Set when this execute() opens its own partial tool ask (diff-view branch), so + // the catch below can finalize it. Undefined on the saveDirectly branch. + let pendingPartialAsk: string | undefined if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -42,6 +309,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) await task.diffViewProvider.reset() return } @@ -51,51 +323,71 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + + // Returning here skips the try/catch teardown below: release THIS task's stream + // state (and only this task's) so the abort listener and any streamFailed guard do + // not outlive the call. + this.resetTaskPartialState(task) return } const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath) + // The point of no return for this write. saveChanges() persists the document and then keeps + // working (closing the diff views, tab bookkeeping, diagnostics), and trackFileContext() and + // pushToolWriteResult() run after it, so a rejection past this point must not roll back a + // write the user already approved: the rollback would restore the previous content, or delete + // a file that was created and saved. Set only once the durable write has landed. + let writeCommitted = false - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + try { + // The guarded scope starts before the preflight filesystem work: a throw from + // fileExistsAtPath or createDirectoriesForFile must report and tear down like any other + // write failure, not escape into BaseTool.handle() with this task's stream state attached. + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath) + + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" + } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + // Create parent directories early for new files to prevent ENOENT errors + // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) + if (!fileExists) { + const createdDirs = await createDirectoriesForFile(absolutePath) + // The rollback lives in DiffViewProvider.revertChanges(), which removes only the + // directories recorded there - and its own mkdir returns nothing once these exist. + // Hand this call's directories over, or a failed new-file write leaves them on disk + // and the next execute() treats the leftover placeholder as an existing file. + task.diffViewProvider.adoptCreatedDirs(createdDirs) + } - if (newContent.startsWith("```")) { - newContent = newContent.split("\n").slice(1).join("\n") - } + if (newContent.startsWith("```")) { + newContent = newContent.split("\n").slice(1).join("\n") + } - if (newContent.endsWith("```")) { - newContent = newContent.split("\n").slice(0, -1).join("\n") - } + if (newContent.endsWith("```")) { + newContent = newContent.split("\n").slice(0, -1).join("\n") + } - if (!task.api.getModel().id.includes("claude")) { - newContent = unescapeHtmlEntities(newContent) - } + if (!task.api.getModel().id.includes("claude")) { + newContent = unescapeHtmlEntities(newContent) + } - const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" - const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) + const fullPath = relPath ? path.resolve(task.cwd, relPath) : "" + const isOutsideWorkspace = isPathOutsideWorkspace(fullPath) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath), - content: newContent, - isOutsideWorkspace, - isProtected: isWriteProtected, - } + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath), + content: newContent, + isOutsideWorkspace, + isProtected: isWriteProtected, + } - try { task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -129,13 +421,28 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // Rejection is an exit from execute() too. Without the teardown, a + // streamFailed flag armed by an earlier failed delta stays set for the whole + // task, which suppresses the diff preview of every later write_to_file, and + // the TaskAborted listener leaks. + super.resetPartialState() + // This branch never opened a diff view, but the preflight above already adopted + // this call's created directories and set editType/originalContent on the + // per-task provider. A rejection that leaves them behind hands them to the next + // write's open(), whose rollback would then rmdir directories that belong to + // this rejected write - and fail with ENOTEMPTY once a later file lives in one. + await this.resetDiffViewAfterWrite(task) return } await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // saveDirectly() performs the write itself and reports a failing write, so returning + // means the content is on disk: from here the rollback has to stand down. + writeCommitted = true } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) + pendingPartialAsk = partialMessage await task.ask("tool", partialMessage, true).catch(() => {}) await task.diffViewProvider.open(relPath) } @@ -161,10 +468,18 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!didApprove) { await task.diffViewProvider.revertChanges() + // Same exit contract as the saveDirectly branch above: the per-task stream + // state and its abort listener belong to this execute() call. + super.resetPartialState() return } - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, () => { + // Signalled by saveChanges() at the document save, not when it returns: the editor + // bookkeeping and diagnostics that follow can still reject, and by then the file + // has already landed. + writeCommitted = true + }) } if (relPath) { @@ -178,16 +493,60 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) await task.diffViewProvider.reset() - this.resetPartialState() + // BaseTool's reset only clears this instance's lastSeenPartialPath; the + // stream state added here is keyed per task. Clearing the whole map from + // one task's execute() would drop another task's streamFailed/streamError + // while it is still streaming, so tear down only this task's entry. + super.resetPartialState() task.processQueuedMessages() return } catch (error) { - await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() + // The diff-view branch above may have opened a fresh partial ask for this + // (retried) write. Finalize it before tearing down, or the spinner and + // Save/Reject buttons stay live for a tool call that has already failed. + if (pendingPartialAsk !== undefined) { + await this.finalizePartialToolAskAfterFailure(task, pendingPartialAsk) + } + // handleError() reports through say(), which throws once the task is aborted: the + // rollback below must still run, or unapproved content stays in the diff view. Keep + // the report and the rollback in one try/finally so neither can skip the other. + try { + await handleError("writing file", error as Error) + } finally { + // A failed diff-open is not the end of the story: open() creates the parent directories + // and an empty placeholder BEFORE it awaits openDiffEditor(), so a rejection leaves that + // debris behind. Revert first - with no active editor the rollback still unlinks the + // placeholder and removes only the directories this operation created - then reset. + // A rollback that did not finish is a second, actionable failure: the placeholder and the + // created directories are still on disk, and the next execute() would treat that debris as + // an existing file. Report it the same way the parse-failure teardown and + // cleanupFailedPartialStream() do - after the reset, so the report is the last thing the + // failed write leaves behind. + // After the commit point the write is durable, so restoring the previous content + // (or unlinking a newly created file) would undo an approved write to clean up an + // unrelated failure. Treat the rollback as done rather than performing it. + const reverted = writeCommitted ? true : await this.revertDiffChangesBeforeReset(task) + // The resilient helper, as in the other two teardowns: a reset that rejects must not + // skip the report below, or the user sees only the write error while the editor still + // holds content nobody approved. + await this.resetDiffViewAfterWrite(task) + super.resetPartialState() + + if (!reverted) { + // The abort flag makes say() throw on this path too: falling out of the finally + // must not mask the original write failure or skip the reset that already ran. + await this.reportRevertFailure(task).catch((reportError: unknown) => { + console.error("Error reporting the write_to_file rollback failure:", reportError) + }) + } + } return + } finally { + // Unconditional: every exit from the guarded scope - success, denial, or a + // throw - releases this task's stream entry and its TaskAborted listener. + this.resetTaskPartialState(task) } } @@ -195,62 +554,143 @@ 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) { - return - } + // Anything below that throws must not leave this task's stream entry and its + // TaskAborted listener registered: the stream is over, a stale streamFailed flag + // suppresses every later preview for the task, and the listener can never fire + // for a stream that already ended. Release by identity so a newer stream for the + // same task survives, then rethrow - the caller owns reporting. + try { + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { + return + } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + const provider = task.providerRef.deref() + const state = await provider?.getState() - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + // Cancelled while provider state was in flight: the teardown already + // released this task's stream state. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) + + if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) + return + } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, - } + 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, - ) + const partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + if (newContent) { + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } + + // Cancellation may land while open() is in flight: its abort handler has + // already torn the stream down (and may have reverted or closed this very + // diff view), so streaming the partial content into it now would resurrect a + // view for a task that no longer exists. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // A cancellation that lands while open() or update() is in flight runs the + // TaskAborted teardown - which releases this task's stream state, reverts or + // closes this very diff view, and reports the failure itself - and it can also + // reject the call in flight. Marking the already-released state failed, finalizing + // the ask, or running the failed-stream cleanup a second time would resurrect UI + // and roll back twice for a task the user cancelled, so the teardown owns the + // outcome here. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + console.error(`Error streaming write_to_file diff view:`, error) + return + } + + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // rethrow: the same filesystem operation is retried in execute() once the + // block completes, and that authoritative non-partial path reports the + // error to the user. Surfacing it here too would show the same error twice. + // Swallowing it here is safe because the agent loop advances naturally when + // the non-partial block arrives (it does not depend on this throw). + console.error(`Error streaming write_to_file diff view:`, error) + // Mark the stream as failed so later deltas don't re-attempt and spawn a new + // partial tool message each time. Retain the original error: if the final + // block later fails to parse, execute() never runs and only + // onParameterParseFailure() can report this failure to the user. + partialStreamState.streamFailed = true + partialStreamState.streamError = error instanceof Error ? error : new Error(String(error)) + await this.finalizePartialToolAskAfterFailure(task, partialMessage) + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) + } + } + } catch (error) { + this.releaseTaskPartialStateByIdentity(partialStreamState) + throw error } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts new file mode 100644 index 0000000000..2a34031740 --- /dev/null +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -0,0 +1,200 @@ +// npx vitest run core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts + +import { RooCodeEventName } from "@roo-code/types" +import { vi, type MockedFunction } from "vitest" + +import { type Task } from "../../task/Task" +import { writeToFileTool } from "../WriteToFileTool" + +// The cleanup primitives only read these members, so a structural double is enough; +// the double assertion is the repo's existing pattern for private-method tests +// (see src/__tests__/removeClineFromStack-delegation.spec.ts). +interface CleanupTask { + taskId: string + instanceId: string + once: MockedFunction<(...args: unknown[]) => unknown> + off: MockedFunction<(...args: unknown[]) => unknown> + diffViewProvider: { + reset: MockedFunction<() => Promise> + revertChanges: MockedFunction<() => Promise> + } + finalizePartialToolAsk: MockedFunction<() => Promise> + 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), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + say: vi.fn().mockResolvedValue(undefined), + } + return task as unknown as Task +} + +describe("WriteToFileTool per-task partial-state cleanup", () => { + afterEach(() => { + writeToFileTool["taskPartialStreamState"].clear() + vi.restoreAllMocks() + }) + + it("releases the task state and deregisters the abort listener", async () => { + const task = buildTask("cleanup-task", "inst-1") + const state = writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).toHaveBeenCalledWith( + RooCodeEventName.TaskAborted, + state.abortCleanup, + ) + }) + + it("is a no-op for a task that never streamed", async () => { + const task = buildTask("never-streamed", "inst-2") + + writeToFileTool.clearTaskState(task) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect((task as unknown as CleanupTask).off).not.toHaveBeenCalled() + }) + + it("logs and continues when resetting the diff view fails", async () => { + const task = buildTask("reset-fails", "inst-3") + const t = task as unknown as CleanupTask + t.diffViewProvider.reset = vi.fn().mockRejectedValue(new Error("reset failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["resetDiffViewAfterWrite"](task) + + expect(errorSpy).toHaveBeenCalledWith("Error resetting write_to_file diff view:", expect.any(Error)) + }) + + it("logs and continues when reverting the diff document fails", async () => { + const task = buildTask("revert-fails", "inst-4") + const t = task as unknown as CleanupTask + t.diffViewProvider.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["revertDiffChangesBeforeReset"](task) + + expect(errorSpy).toHaveBeenCalledWith("Error reverting write_to_file diff view changes:", expect.any(Error)) + }) + + it("reverts while the recovery state exists and reports the hazard when the revert fails", async () => { + const task = buildTask("revert-fails-teardown", "inst-5") + const t = task as unknown as CleanupTask + let stateSizeDuringRevert = -1 + t.diffViewProvider.revertChanges = vi.fn(async () => { + // The recovery state must still be present while the rollback runs. + stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size + throw new Error("revert failed") + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // Seed the per-task stream state so this teardown boundary is taken at all. + writeToFileTool["getTaskPartialStreamState"](task) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + // Only handleError is reached when no streaming error was recorded; the + // structural double is the existing pattern in this file. + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters< + (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.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(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.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + t.say = vi.fn().mockRejectedValue(new Error("say failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(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.revertChanges = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["cleanupFailedPartialStream"](task) + + // Same contract as the parse-failure teardown: a failed restore is reported. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + errorSpy.mockRestore() + }) + + it("logs and continues when finalizing the open partial ask fails", async () => { + const task = buildTask("finalize-fails", "inst-6") + const t = task as unknown as CleanupTask + 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)) + }) +}) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 7cb9d01585..c2a698dc6c 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 @@ -149,6 +168,7 @@ describe("writeToFileTool", () => { originalContent: "", open: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), + adoptCreatedDirs: vi.fn(), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ @@ -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) @@ -288,14 +312,17 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "defers parent directory creation to execute() while streaming", async () => { - // First call - path not yet stabilized + // Streaming deltas must not touch the filesystem at all. An unguarded + // createDirectoriesForFile here threw EROFS up into BaseTool.handle(), which never + // set didRejectTool/didAlreadyUseTool, so the agent loop stalled permanently. + // The directories are still created - by the authoritative non-partial execute(). + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized - await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false }) expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) }, ) @@ -420,6 +447,671 @@ 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, + toolCallId: "test-tool-call-id", + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError) + expect(mockHandleError).not.toHaveBeenCalledWith( + expect.stringContaining("parsing write_to_file"), + expect.anything(), + ) + // The parse-failure teardown also restores the diff document: the + // streaming failure above already reverted it once (revert + reset), + // and the parse path runs the same cleanup again because execute() + // never runs on this path. + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) + // Per-task state torn down at this boundary: guard cleared, the exact + // registered abort listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + + // The next stream for the same task must issue a partial ask again (the stale + // streamFailed guard is gone). + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("restores the diff document when the final block fails to parse after successful streaming", async () => { + // Streaming opened the diff view with unapproved partial content (open and + // update both succeeded, so no streaming error was captured). The final block + // then arrives without nativeArgs: execute() never runs, so its error cleanup + // never fires. The parse-failure teardown must still restore the document + // (revert before reset) or a user save could persist content the write never + // completed, and must report the generic parse error (no streaming error to + // surface instead). + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + // Delta 1 - stabilize path; delta 2 - streams the partial content into the + // diff view (open + update resolve). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + toolCallId: "test-tool-call-id", + handleError: mockHandleError, + pushToolResult: vi.fn(), + }) + + // No streaming error was captured: the generic parse error is reported. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The diff document is restored by the parse path itself (no streaming + // failure happened, so this is the only revert + reset in the test). + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + // Per-task state torn down: guard cleared, exact listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("leaves the teardown in charge when the stream state is cleared while update() is in flight", async () => { + // The path is stabilized, so this delta streams. The task is disposed while update() + // awaits: the TaskAborted handler releases this task's state (and reverts or closes the + // diff view itself), and the in-flight update then rejects. Marking the released state + // failed, finalizing the ask, or running the failed-stream cleanup again would resurrect + // UI and roll back twice for a task the user cancelled. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.diffViewProvider.update.mockImplementationOnce(async () => { + writeToFileTool["resetTaskPartialState"](mockCline as never) + throw new Error("update rejected after the task was disposed") + }) + + await executeWriteFileTool({}, { isPartial: true }) + + // The cleanup did not run: no revert, no finalize, and the released state stayed gone. + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockHandleError).not.toHaveBeenCalled() + }) + }) + + describe("path stabilization predicate", () => { + // 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("per-task stream state isolation", () => { + // A second task streaming through the same singleton while mockCline's execute() + // runs. Structural double, same pattern as the partial-state-cleanup spec. + function buildStreamingTask(taskId: string, instanceId: string) { + return { + taskId, + instanceId, + once: vi.fn(), + off: vi.fn(), + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + }, + finalizePartialToolAsk: vi.fn().mockResolvedValue(undefined), + } + } + + it("leaves another task's stream state intact when execute() completes", async () => { + const other = buildStreamingTask("task-2", "instance-2") + const otherState = writeToFileTool["getTaskPartialStreamState"](other as never) + otherState.streamFailed = true + otherState.streamError = new Error("other task stream failure") + + await executeWriteFileTool({}) + + // The other task is still streaming: its failure state must survive, or its + // next delta re-opens the diff view and spawns a duplicate partial ask. + const retained = writeToFileTool["taskPartialStreamState"].get("task-2.instance-2") + expect(retained).toBeDefined() + expect(retained?.streamFailed).toBe(true) + expect(retained?.streamError?.message).toBe("other task stream failure") + expect(other.off).not.toHaveBeenCalled() + }) + + it("finalizes the partial ask when the write itself fails", async () => { + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // The diff-view branch opened a partial ask for this write; without the + // finalize the spinner and Save/Reject stay live after the failure. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + }) + }) + + describe("early-return stream state cleanup", () => { + it("releases the per-task stream state when a rooignore denial returns early", async () => { + // The denial returns before the try/catch teardown: without the release the abort + // listener stays for the task's lifetime and a retained streamFailed suppresses the + // diff preview of every later write_to_file in this task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched off() + // argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when a missing parameter returns early", async () => { + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + + await writeToFileTool.execute({ path: "", content: "mock content" }, mockCline, { + askApproval: mockAskApproval, + toolCallId: "test-tool-call-id", + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("stops before the filesystem probe when the state is released while provider state is in flight", async () => { + // handlePartial() awaits provider.getState() before any side effect. A cancellation + // during that await runs the TaskAborted teardown; the delta already in flight must + // stop there instead of probing, asking and opening a diff view for a dead task. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn(async () => { + writeToFileTool.clearTaskState(mockCline) + return {} + }), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before the partial ask when the state is released during the filesystem probe", async () => { + // Same teardown, one await later. The first delta pins editType, so clear it to + // take the fileExistsAtPath branch again and abort inside it. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.editType = undefined + // mockImplementationOnce: executeWriteFileTool re-arms the default resolved value + // on every call, so a plain mockImplementation would be overwritten. + mockedFileExistsAtPath.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + return false + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before touching the diff view when the stream state is released during an in-flight ask", async () => { + // A cancellation while task.ask() is in flight runs the TaskAborted teardown. The + // delta that was already in flight must not then re-open the diff view for a task + // the user cancelled - that resurrects the state the teardown just released. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.ask.mockImplementation(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before updating the diff view when the task is cancelled while open() is in flight", async () => { + // open() is the first provider await after the partial ask. If TaskAborted lands + // while it is in flight, the teardown has already released this task's stream + // state (and may have reverted or closed this very view), so the delta that is + // already in flight must not stream partial content into a cancelled task's view. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + it("does not finalize the ask or roll back twice when open() rejects after a cancellation", async () => { + // A cancellation during open() releases the stream state through the TaskAborted + // teardown (which also reverts or closes this diff view) AND can reject the call in + // flight. The catch used to mark the released state failed, finalize the ask and run + // the failed-stream cleanup again - a second rollback and a fresh ask row for a task + // the user already cancelled. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.revertChanges.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + throw new Error("EACCES: permission denied, open mock-file") + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // The teardown that already ran owns the outcome: no finalize and no second rollback. + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when prevent-focus-disruption skips the partial preview", async () => { + // Delta 1 only pins the path, so the entry is still live afterwards (the stream is in + // flight). Delta 2 reaches the experiment check and returns without ever showing a + // preview: nothing else would release the entry or detach the TaskAborted listener. + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The exact listener this task registered, not just any function: a mismatched off() + // argument would leave the real listener attached. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when content is missing", async () => { + // The missing-content return sits before execute()'s guarded scope, so it needs its own + // release. The state is seeded first so the assertion proves a release happened rather + // than an empty map. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the + // typed params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + toolCallId: "test-tool-call-id", + handleError: mockHandleError, + pushToolResult, + }) + + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith("write_to_file", "content") + expect(pushToolResult).toHaveBeenCalledWith("Missing param error") + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // A stream may have opened a diff view for this call; the early return still closes it. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("releases the per-task stream state when the write completes", async () => { + // Seed first: without the seed the map is empty either way and the assertion is vacuous. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("still rolls back and resets when the error report throws because the task is aborted", async () => { + // say() throws once the task is aborted, and handleError reports through it. The + // rollback and reset still have to run, or the diff view keeps unapproved content. + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + mockHandleError.mockRejectedValueOnce(new Error("aborted")) + + await expect(executeWriteFileTool({})).rejects.toThrow("aborted") + + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when the write itself fails", async () => { + // The catch path tears down too: a failed write must not leave the entry (and its + // streamFailed guard) attached to the task. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when the preflight directory creation throws", async () => { + // The preflight filesystem work sits before execute()'s guarded scope today: when it + // throws, nothing reports the failure and the stream state leaks. It has to be handled + // like any other write failure - reported once, teardown run. + writeToFileTool["getTaskPartialStreamState"](mockCline as never).streamFailed = true + mockedCreateDirectoriesForFile.mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied, mkdir '/new-parent'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("rolls the diff-view debris back when open() fails mid-execute", async () => { + // open() creates the parent directories and an empty placeholder before it awaits + // openDiffEditor(). When it rejects, the write failed but the debris did not: the catch + // has to await the rollback (which removes the placeholder and only the directories this + // operation created) before resetting the view, or the next execute() mistakes the + // placeholder for an existing file. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalled() + const revertOrder = mockCline.diffViewProvider.revertChanges.mock.invocationCallOrder[0] + const resetOrder = mockCline.diffViewProvider.reset.mock.invocationCallOrder.at(-1) + expect(resetOrder).toBeGreaterThan(revertOrder) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports the rollback hazard when execute()'s own cleanup cannot revert the diff", async () => { + // execute() failed after the diff view was opened, and the rollback that follows the + // catch failed too. The debris (placeholder, created directories) is still on disk and the + // editor may still hold unapproved content, so the user has to be told - the same report + // the parse-failure teardown and cleanupFailedPartialStream() make. + mockCline.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("rollback failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("could not be restored after the failed tool call"), + ) + }) }) describe("user interaction", () => { @@ -432,6 +1124,22 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() }) + it("clears this task's stream state when the write is rejected", async () => { + // A failed delta earlier in the same task arms the streamFailed guard. If the + // rejection exit returns without the teardown, every later write_to_file in this + // task loses its diff preview and the TaskAborted listener leaks. + const state = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + state.streamFailed = true + state.streamError = new Error("stream failure before the rejected write") + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}) + + expect( + writeToFileTool["taskPartialStreamState"].get(`${mockCline.taskId}.${mockCline.instanceId}`), + ).toBeUndefined() + }) + it("reports user edits with diff feedback", async () => { const userEditsValue = "- old line\n+ new line" mockCline.diffViewProvider.saveChanges.mockResolvedValue({ @@ -461,16 +1169,358 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("handles partial streaming errors after path stabilizes", async () => { + it("reports the rollback failure even when the diff-view reset rejects", async () => { + // The catch used a bare diffViewProvider.reset(): when that rejected, the rollback report + // after it never ran, so the user saw only the write error while the editor still held + // content nobody approved. The teardown now goes through the same resilient helper the + // parse-failure and partial-stream teardowns already use. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("open failed")) + mockCline.diffViewProvider.revertChanges.mockRejectedValue(new Error("revert failed")) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("could not be restored")) + }) + + it("resets the diff-view provider when the prevent-focus-disruption approval is rejected", async () => { + // This branch never opens a diff view, but the preflight already adopted the created + // directories and set editType/originalContent on the per-task provider. Left behind, the + // next write's open() folds those directories into its own createdDirs and its rollback + // rmdirs directories belonging to this rejected write - ENOTEMPTY once a later file lives + // in one of them. + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + mockCline.diffViewProvider.saveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "final content", + }) + mockAskApproval = vi.fn().mockResolvedValue(false) + + await executeWriteFileTool({}) + + expect(mockCline.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("does not roll back a write that saveChanges already committed", async () => { + // saveChanges() persists the document and then keeps working, and trackFileContext() runs + // after it. A failure past that point used to restore the previous content - or unlink a + // file the user had just approved being created - to clean up an unrelated error. + mockCline.diffViewProvider.saveChanges.mockImplementation( + async (_diagnosticsEnabled: boolean, _writeDelayMs: number, onCommit?: () => void) => { + onCommit?.() + return { newProblemsMessage: "", userEdits: undefined, finalContent: "final content" } + }, + ) + mockCline.fileContextTracker.trackFileContext.mockRejectedValue(new Error("context tracking failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { + // The same filesystem operation is retried in execute() once the block completes, + // and that authoritative non-partial path reports the error to the user. Surfacing + // it during streaming too would show the same error twice, so handlePartial must NOT + // route streaming errors through handleError. mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) // First call - path not yet stabilized, no error yet await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, error occurs + // Second call with same path - path is now stabilized, error occurs but is swallowed + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial open() fails", async () => { + // Regression test: when diffViewProvider.open() throws during streaming (e.g. EACCES/EROFS + // on a read-only path), the partial tool ask created at the top of handlePartial leaves the + // UI spinner stuck. handlePartial must finalize the partial message and reset the diff view, + // and must NOT surface a duplicate error (execute() reports the authoritative one). + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + // Record the relative order of revertChanges() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.revertChanges.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state revertChanges() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("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 }) - expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) + + // 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, + toolCallId: "test-tool-call-id", + 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, + toolCallId: "test-tool-call-id", + 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("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.update.mockResolvedValue(undefined) + mockCline.diffViewProvider.editType = undefined + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2) + }) + + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called + // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate + // in presentAssistantMessage was never reached and the agent loop stalled permanently. + // After the fix the call is removed entirely -- handlePartial never touches the filesystem. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // First call -- path not yet stabilized, returns early + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // Second call -- path stabilized; createDirectoriesForFile must NOT be called from + // handlePartial, so the mock rejection must not trigger and handleError must not be called + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + }) + + describe("partial-stream failure teardown and directory ownership", () => { + it("hands the directories it created to the diff view so a rollback removes them", async () => { + // revertChanges() removes only the directories the provider recorded, and its own + // mkdir returns nothing once execute() has made them. Without the hand-off a + // rolled-back new-file write left the parent directories on disk. + const created = ["/new-parent", "/new-parent/nested"] + mockedCreateDirectoriesForFile.mockResolvedValue(created) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.adoptCreatedDirs).toHaveBeenCalledWith(created) + }) + + it("does not hand directories over when the file already exists", async () => { + // An existing file has no directories to roll back; adopting an empty list from + // an unrelated mkdir would still be wrong, but adopting a populated one would + // make a denial rmdir directories the user already had. + await executeWriteFileTool({}, { fileExists: true }) + + expect(mockCline.diffViewProvider.adoptCreatedDirs).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when a partial-stream await throws", async () => { + // An uncaught failure below the state creation used to leave the entry and its + // TaskAborted listener registered: the stale streamFailed flag then suppressed + // every later preview for the task, and the listener could never fire for a + // stream that had already ended. + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockRejectedValue(new Error("state read failed")), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("keeps a newer stream entry when an older partial failure releases by identity", async () => { + // Identity, not presence: dropping whatever sits under the key would detach the + // live stream's abort listener while cleaning up after a superseded one. + const stale = writeToFileTool["getTaskPartialStreamState"](mockCline as never) + const newer = { ...stale } + mockCline.providerRef.deref.mockImplementation(() => ({ + getState: async () => { + writeToFileTool["taskPartialStreamState"].set("task-1.instance-1", newer) + throw new Error("state read failed") + }, + })) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].get("task-1.instance-1")).toBe(newer) + expect(mockCline.off).not.toHaveBeenCalled() }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6cec3bdbe7..aa4cab38ed 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -77,6 +77,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, @@ -728,6 +729,11 @@ export class ClineProvider removeTaskEventListeners(this.taskEventListeners, 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/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 3c1d33ae28..1d31eaf561 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -21,6 +21,25 @@ import { Task } from "../../core/task/Task" import { DecorationController } from "./DecorationController" +/** Narrow ENOENT test so a rollback can tolerate a file or dir that never landed. */ +function isEnoent(error: unknown): boolean { + return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT" +} + +/** + * The approved document save reported failure: the content the user accepted never reached disk. + * Typed so the caller's failure handling can tell a failed write from an unrelated teardown error, + * and so a discarded boolean can never let the success bookkeeping run on a write that did not land. + */ +export class DocumentSaveFailedError extends Error { + constructor(absolutePath: string) { + super( + `Failed to save ${absolutePath}: the document save reported failure; the approved content is not on disk.`, + ) + this.name = "DocumentSaveFailedError" + } +} + export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes" @@ -31,8 +50,16 @@ export class DiffViewProvider { userEdits?: string editType?: "create" | "modify" isEditing = false + // Set once saveChanges() has passed its commit point. revertChanges() must then stand + // down: the write is durable, and an abort-driven teardown must not restore the old + // content over it (or unlink a newly created file). + private writeCommitted = false originalContent: string | undefined private createdDirs: string[] = [] + // Directories a caller (WriteToFileTool.execute) created before this transaction. + // open() folds them into createdDirs so revertChanges() removes them too: without + // them the caller's preflight mkdir left parent directories on disk after a rollback. + private adoptedCreatedDirs: string[] = [] private documentWasOpen = false // Tracks whether the target file's tab was pinned before the diff session. // Closing the tab to open the diff drops VS Code's pin state, so we restore @@ -90,11 +117,21 @@ export class DiffViewProvider { this.taskRef = new WeakRef(task) } + /** + * Record directories a caller created before this diff transaction so the rollback + * removes them too. revertChanges() only deletes what createdDirs holds, and a + * pre-created directory makes the provider's own mkdir return nothing. + */ + adoptCreatedDirs(createdDirs: string[]): void { + this.adoptedCreatedDirs.push(...createdDirs) + } + async open(relPath: string): Promise { this.relPath = relPath const fileExists = this.editType === "modify" const absolutePath = path.resolve(this.cwd, relPath) this.isEditing = true + this.writeCommitted = false // Capture the current scroll position before we close the tab so we can // restore it after saving/reverting. @@ -127,7 +164,7 @@ export class DiffViewProvider { // For new files, create any necessary directories and keep track of new // directories to delete if the user denies the operation. - this.createdDirs = await createDirectoriesForFile(absolutePath) + this.createdDirs = [...this.adoptedCreatedDirs, ...(await createDirectoriesForFile(absolutePath))] // Make sure the file exists before we open it. if (!fileExists) { @@ -143,12 +180,19 @@ export class DiffViewProvider { const tabs = vscode.window.tabGroups.all .map((tg) => tg.tabs) .flat() - .filter( - (tab) => - tab.input instanceof vscode.TabInputText && - tab.input.uri.scheme === "file" && - arePathsEqual(tab.input.uri.fsPath, absolutePath), - ) + .filter((tab) => { + // A new-file diff session's tab is a TabInputTextDiff, not a plain TabInputText: + // matching only the latter left the dirty diff document open when the user had no + // plain tab for the file, one save away from persisting the unapproved content. + const input = tab.input + if (input instanceof vscode.TabInputText) { + return input.uri.scheme === "file" && arePathsEqual(input.uri.fsPath, absolutePath) + } + if (input instanceof vscode.TabInputTextDiff) { + return input.modified.scheme === "file" && arePathsEqual(input.modified.fsPath, absolutePath) + } + return false + }) for (const tab of tabs) { // Remember the pin state so we can restore it after the diff closes; @@ -163,7 +207,12 @@ export class DiffViewProvider { console.error(`Failed to close tab ${tab.label}`, err) } } - this.documentWasOpen = true + // Only a plain text tab means the file was open for the user: a TabInputTextDiff + // match is the previous diff session's own tab, and treating it as "was open" + // would make keepOrCloseEditedFile() open (and leave) a plain tab afterward. + if (tab.input instanceof vscode.TabInputText) { + this.documentWasOpen = true + } } // Snapshot unrelated preview tabs so we can restore them if opening the @@ -326,6 +375,7 @@ export class DiffViewProvider { async saveChanges( diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + onCommit?: () => void, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -340,9 +390,23 @@ export class DiffViewProvider { const editedContent = updatedDocument.getText() if (updatedDocument.isDirty) { - await updatedDocument.save() + const saved = await updatedDocument.save() + // A save that reports false never landed: the buffer is still dirty and the file holds + // the previous content (or nothing for a new file). Throw before the commit point and + // before any success bookkeeping, so the caller keeps its rollback and the existing + // failure handling runs. A save that rejects still propagates as before. + if (!saved) { + throw new DocumentSaveFailedError(absolutePath) + } } + // The write is irreversible from here: a clean document already matches disk, and a saved + // one has landed. Everything below (closing the diff views, tab bookkeeping, diagnostics) + // can still reject, and a caller that rolls the tool call back after this point would undo + // a write the user approved - so it is told, at the commit point rather than on return. + onCommit?.() + this.writeCommitted = true + // Stop tracking touches and cancel any pending scroll-to-diff before any // programmatic editor activation below. this.disposeActiveEditorListener() @@ -513,13 +577,43 @@ export class DiffViewProvider { return JSON.stringify(result) } + /** + * Remove a file this edit created. Tolerates ENOENT: when open() failed before the + * placeholder was written (or the write itself failed) there is nothing to delete, and + * that must not abort the rollback. + */ + private async removeCreatedFile(absolutePath: string): Promise { + try { + await fs.unlink(absolutePath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + + /** Same tolerance for a directory this edit created that never made it to disk. */ + private async removeCreatedDir(dirPath: string): Promise { + try { + await fs.rmdir(dirPath) + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error + } + } + } + async revertChanges(): Promise { - if (!this.relPath || !this.activeDiffEditor) { + // Both guards are required. relPath survives a completed edit, so without isEditing a later + // teardown would take the new-file branch for the PREVIOUS edit's target and unlink an + // existing user file - permanently. isEditing alone is not enough: open() sets it before the + // first await, and that is exactly the state a failed open() leaves behind, where the + // placeholder and the created directories still have to be rolled back. + if (!this.relPath || !this.isEditing || this.writeCommitted) { return } const fileExists = this.editType === "modify" - const updatedDocument = this.activeDiffEditor.document const absolutePath = path.resolve(this.cwd, this.relPath) // Stop tracking touches and cancel any pending scroll-to-diff before any @@ -528,21 +622,56 @@ export class DiffViewProvider { this.cancelDeferredScroll() if (!fileExists) { - if (updatedDocument.isDirty) { - await updatedDocument.save() + // open() creates the parent directories and an empty placeholder file BEFORE it + // awaits openDiffEditor(). If that await rejects there is no activeDiffEditor, and + // the previous early return here left the placeholder and the new directories on + // disk: the next execute() then saw an empty file and treated the requested new file + // as an existing one, so a denial preserved the debris. The filesystem rollback runs + // either way; only the document work needs an editor. + if (this.activeDiffEditor) { + const updatedDocument = this.activeDiffEditor.document + if (updatedDocument.isDirty) { + // The buffer holds the streamed content of a write that was never approved. Saving + // it here persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. Discard the buffer + // (force-close the tab) instead; the file goes away a moment later anyway. + await this.discardFileTab(absolutePath) + await this.closeAllDiffViews() + } else { + await this.closeAllDiffViews() + // The file was newly created for this edit; close its transiently + // opened tab before deleting it from disk. + await this.closeFileTab(absolutePath) + } } - await this.closeAllDiffViews() - // The file was newly created for this edit; close its transiently - // opened tab before deleting it from disk. - await this.closeFileTab(absolutePath) - await fs.unlink(absolutePath) + if (this.activeDiffEditor) { + await this.removeCreatedFile(absolutePath) + } else { + // open() failed before the diff editor existed, so the file is still the empty + // placeholder this edit created. Another process may have written the path in the + // meantime; an in-place write keeps the inode, so check the content instead and + // leave a non-empty file alone. + const stillEmptyPlaceholder = await fs + .stat(absolutePath) + .then((stats) => stats.size === 0) + .catch(() => false) + if (stillEmptyPlaceholder) { + await this.removeCreatedFile(absolutePath) + } + } // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { - await fs.rmdir(this.createdDirs[i]) + await this.removeCreatedDir(this.createdDirs[i]) } } else { + // Only reachable after a successful open(), so the editor exists. + const updatedDocument = this.activeDiffEditor?.document + if (!updatedDocument) { + return + } + // Revert document. const edit = new vscode.WorkspaceEdit() @@ -555,9 +684,42 @@ export class DiffViewProvider { // Apply the edit and save, since contents shouldn't have changed // this won't show in local history unless of course the user made - // changes and saved during the edit. - await vscode.workspace.applyEdit(edit) - await updatedDocument.save() + // changes and saved during the edit. applyEdit() resolves false when the workspace + // refused the edit and save() resolves false when the editor refused the save: both + // booleans were dropped, so a rollback whose restore never applied or whose save never + // landed was reported to the caller as a completed rollback. Capture each result the + // way the pre-stream restore and the refused close do, keep a rejection as the cause, + // and throw before any later bookkeeping so nothing proceeds on the assumption that + // the original content came back and reached disk. + let applied: boolean + let applyError: Error | undefined + try { + applied = await vscode.workspace.applyEdit(edit) + } catch (error) { + applied = false + applyError = error instanceof Error ? error : new Error(String(error)) + } + if (!applied) { + throw new Error( + `Rollback could not restore the original content for ${absolutePath}; the workspace edit was not applied, so the document may still hold the unapproved content.`, + { cause: applyError }, + ) + } + + let saved: boolean + let saveError: Error | undefined + try { + saved = await updatedDocument.save() + } catch (error) { + saved = false + saveError = error instanceof Error ? error : new Error(String(error)) + } + if (!saved) { + throw new Error( + `Rollback could not save the restored document for ${absolutePath}; the original content may not be on disk.`, + { cause: saveError }, + ) + } await this.closeAllDiffViews() @@ -847,6 +1009,104 @@ export class DiffViewProvider { // Close the plain (non-diff) editor tab for the target file. Used when the // file was opened transiently for the diff and the user never interacted // with it, so it should not linger after accept/deny. + /** + * Close the tab for this path WITHOUT saving. Used by the new-file rollback, where the + * buffer holds unapproved streamed content that must never reach disk; closeFileTab() + * deliberately skips dirty tabs, so a dirty buffer needs the forced close. + * + * A close that fails or is refused is a rollback failure: the buffer would still hold the + * content the user never approved, one save away from disk. Put the pre-stream content back + * so nothing saveable survives, and propagate - the caller must not keep deleting around a + * buffer it could not discard. + */ + private async discardFileTab(absolutePath: string): Promise { + const tabs = vscode.window.tabGroups.all + .flatMap((group) => group.tabs) + .filter( + (tab) => + tab.input instanceof vscode.TabInputText && + tab.input.uri.scheme === "file" && + arePathsEqual(tab.input.uri.fsPath, absolutePath), + ) + + for (const tab of tabs) { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a + // dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. Restore the buffer to its pre-stream content and save it clean first, + // so close() never sees a dirty tab and nothing unapproved reaches disk. + await this.restorePreStreamBuffer(absolutePath) + await this.saveBufferClean(absolutePath) + + let closed: boolean + let closeError: Error | undefined + try { + closed = await vscode.window.tabGroups.close(tab) + } catch (error) { + closed = false + closeError = error instanceof Error ? error : new Error(String(error)) + } + if (!closed) { + throw new Error( + `Rollback could not close the restored buffer for ${absolutePath}; its content was put back to the pre-stream state but the tab is still open.`, + { cause: closeError }, + ) + } + } + } + + /** + * Replace an open buffer's content with what it held before streaming started. Runs before + * the rollback closes the tab, so an unapproved buffer is never handed to close() dirty and + * never survives a close that the editor vetoes. An edit the workspace did not apply is a + * failed restore and throws, so the rollback cannot report success on a buffer that never + * came back. + */ + private async restorePreStreamBuffer(absolutePath: string): Promise { + const document = vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (!document) { + return + } + const edit = new vscode.WorkspaceEdit() + const range = new vscode.Range(document.positionAt(0), document.positionAt(document.getText().length)) + edit.replace(document.uri, range, this.originalContent ?? "") + // applyEdit() resolves false when the edit was not applied: the buffer would still hold the + // unapproved streamed content, and the save that follows would persist exactly what this + // rollback is undoing. Capture the result like the close below does, treat false as a + // failed restore, and throw before saveBufferClean runs so the caller keeps its failure. + // A rejection is kept as the cause. + let applied: boolean + let applyError: Error | undefined + try { + applied = await vscode.workspace.applyEdit(edit) + } catch (error) { + applied = false + applyError = error instanceof Error ? error : new Error(String(error)) + } + if (!applied) { + throw new Error( + `Rollback could not restore the pre-stream buffer for ${absolutePath}; the workspace edit was not applied, so the buffer may still hold the unapproved content.`, + { cause: applyError }, + ) + } + } + + /** + * Save a restored buffer so it is clean when the rollback closes it. close() has no + * force-discard parameter, so a dirty tab would prompt or be refused; saving the restored + * content is what makes the close unconditional. For a new file the placeholder is unlinked + * a moment later, so the saved content is transient by design. + */ + private async saveBufferClean(absolutePath: string): Promise { + const document = vscode.workspace.textDocuments.find( + (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), + ) + if (document?.isDirty) { + await document.save() + } + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) @@ -1110,10 +1370,14 @@ export class DiffViewProvider { this.cancelDeferredScroll() await this.closeAllDiffViews() + this.relPath = undefined + this.newContent = undefined this.editType = undefined this.isEditing = false + this.writeCommitted = false this.originalContent = undefined this.createdDirs = [] + this.adoptedCreatedDirs = [] this.documentWasOpen = false this.documentWasPinned = false this.activeDiffEditor = undefined diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index e94a66e1db..c505532ade 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,12 +1,24 @@ -import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" -import { DecorationController } from "../DecorationController" -import type { Task } from "../../../core/task/Task" -import type { Mock } from "vitest" +import * as fs from "fs/promises" +import { + DocumentSaveFailedError, + DiffViewProvider, + DIFF_VIEW_URI_SCHEME, + DIFF_VIEW_LABEL_CHANGES, +} from "../DiffViewProvider" import * as vscode from "vscode" import delay from "delay" import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" - -import { makePosition, makeRange, makeTextDocument, makeTextEditor, makeUri } from "../../../test-utils/vscode" +import { + makeDisposable, + makePosition, + makeRange, + makeTextDocument, + makeTextEditor, + makeUri, +} from "../../../test-utils/vscode" +import { DecorationController } from "../DecorationController" +import type { Task } from "../../../core/task/Task" +import type { Mock } from "vitest" // Mock delay vi.mock("delay", () => ({ @@ -18,6 +30,12 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + // revertChanges() rolls a new-file edit back by deleting the placeholder and the + // directories the edit created. stat lets the editorless rollback check the placeholder + // is still empty before unlinking it (default: an empty placeholder). + stat: vi.fn().mockResolvedValue({ size: 0 }), + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -54,7 +72,7 @@ vi.mock("vscode", () => ({ onDidChangeTextEditorVisibleRanges: vi.fn(() => ({ dispose: vi.fn() })), tabGroups: { all: [], - close: vi.fn(), + close: vi.fn().mockResolvedValue(true), activeTabGroup: { activeTab: undefined }, }, visibleTextEditors: [], @@ -375,6 +393,26 @@ describe("DiffViewProvider", () => { expect(diffViewProvider["documentWasPinned"]).toBe(true) expect(diffViewProvider["documentWasOpen"]).toBe(true) }) + it("folds directories the caller created into createdDirs so a rollback can remove them", async () => { + // WriteToFileTool.execute() creates the parent directories before the diff view + // exists, so the provider's own mkdir records nothing. Without the adoption + // createdDirs stayed empty and a rolled-back new-file write left them on disk. + const mockDocument = makeTextDocument({ uri: makeUri(`${mockCwd}/adopt.md`) }) + const mockEditor = makeTextEditor({ document: mockDocument }) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockEditor) + vi.mocked(vscode.commands.executeCommand).mockResolvedValue(undefined) + vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { + setTimeout(() => callback(mockDocument), 0) + return makeDisposable() + }) + vi.mocked(vscode.window).visibleTextEditors = [mockEditor] + diffViewProvider.adoptCreatedDirs([`${mockCwd}/new-parent`]) + diffViewProvider.editType = "create" + + await diffViewProvider.open("adopt.md") + + expect(diffViewProvider["createdDirs"]).toEqual([`${mockCwd}/new-parent`]) + }) }) describe("scrollToFirstDiff method", () => { @@ -930,6 +968,96 @@ describe("DiffViewProvider", () => { expect(mockDelay).toHaveBeenCalledWith(5000) expect(vscode.languages.getDiagnostics).toHaveBeenCalled() }) + + it("signals the commit point once the document write has landed", async () => { + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + const onCommit = vi.fn() + + await diffViewProvider.saveChanges(false, 0, onCommit) + + // The shared document is clean, so nothing is written: the file already matches the + // buffer, which is just as irreversible. The caller stops rolling back here. + expect(onCommit).toHaveBeenCalledTimes(1) + // The stand-down flag revertChanges() relies on must be set by saveChanges(), not by + // the caller: remove this assignment in the provider and an abort-driven teardown + // would roll back an approved write. + expect((diffViewProvider as unknown as { writeCommitted: boolean })["writeCommitted"]).toBe(true) + }) + + it("does not signal the commit point when the document save rejects", async () => { + const save = vi.fn().mockRejectedValue(new Error("save failed")) + // Structural double carrying only the members saveChanges() reaches before the commit + // point; the full TextEditor shape needs a live editor host - hence the assertion. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const onCommit = vi.fn() + + await expect(diffViewProvider.saveChanges(false, 0, onCommit)).rejects.toThrow("save failed") + + // A rejected save leaves the file absent or holding the previous content: the caller has + // to keep the rollback available, so signalling here would lose the write's debris. + expect(onCommit).not.toHaveBeenCalled() + expect((diffViewProvider as unknown as { writeCommitted: boolean })["writeCommitted"]).toBe(false) + }) + + it("throws a typed write failure before any success bookkeeping when the save reports false", async () => { + // The double resolves an explicit false, not undefined: a bare await would let a + // double resolving undefined pass by accident, so the reported-failure contract is + // pinned with a real false. + const save = vi.fn().mockResolvedValue(false) + // Structural double carrying only the members saveChanges() reaches at the save; the + // full TextEditor shape needs a live editor host - hence the assertion. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + + await expect(diffViewProvider.saveChanges(false, 0)).rejects.toThrow(DocumentSaveFailedError) + + expect(save).toHaveBeenCalledTimes(1) + // Success bookkeeping must not run: the caller is told the write failed, so the diff + // views stay open and no result is stored for formatFileWriteResponse(). + expect(closeAllDiffViews).not.toHaveBeenCalled() + expect(diffViewProvider.newProblemsMessage).toBeUndefined() + expect(diffViewProvider.userEdits).toBeUndefined() + }) + + it("does not signal the commit point when the save reports false", async () => { + const save = vi.fn().mockResolvedValue(false) + // Structural double, as in the rejected-save test above. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + const onCommit = vi.fn() + + await expect(diffViewProvider.saveChanges(false, 0, onCommit)).rejects.toThrow(DocumentSaveFailedError) + + // Ordering for this branch: the commit point stands in for the placeholder release the + // sibling branches gate on. A callback that ran here would set the caller's + // writeCommitted flag and stand the rollback down on a write that never landed. + expect(onCommit).not.toHaveBeenCalled() + }) + + it("behaves exactly as before when the save reports true", async () => { + const save = vi.fn().mockResolvedValue(true) + // Structural double, as in the rejected-save test above. + diffViewProvider["activeDiffEditor"] = { + document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, + } as unknown as vscode.TextEditor + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + const onCommit = vi.fn() + + const result = await diffViewProvider.saveChanges(false, 0, onCommit) + + expect(save).toHaveBeenCalledTimes(1) + expect(onCommit).toHaveBeenCalledTimes(1) + expect(closeAllDiffViews).toHaveBeenCalledTimes(1) + expect(result.finalContent).toBe("new content") + }) }) describe("preEditScrollLine capture and restore", () => { @@ -1060,14 +1188,20 @@ describe("DiffViewProvider", () => { vi.mocked(vscode.window.showTextDocument).mockResolvedValue(mockSavedEditor) diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + // Self-contained: revertChanges() only acts while an edit is in progress. + diffViewProvider["relPath"] = "test.txt" + diffViewProvider["isEditing"] = true diffViewProvider["documentWasOpen"] = true diffViewProvider["preEditScrollLine"] = 15 - diffViewProvider.editType = "modify" - diffViewProvider.originalContent = "original" + diffViewProvider["editType"] = "modify" + diffViewProvider["originalContent"] = "original" diffViewProvider["activeDiffEditor"] = makeTextEditor({ document: makeTextDocument({ uri: makeUri(`${mockCwd}/test.txt`), getText: vi.fn().mockReturnValue("modified"), + isDirty: false, + // The revert branch reads the save result; report a save that landed. + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue(makePosition()), }), }) @@ -1086,11 +1220,16 @@ describe("DiffViewProvider", () => { describe("userTouchedDocument close/keep behavior", () => { const mockTargetPath = `${mockCwd}/mock-target-file.ts` - const buildActiveDiffEditor = () => + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. + const buildActiveDiffEditor = (docOverrides: { isDirty?: boolean } = {}) => makeTextEditor({ document: makeTextDocument({ uri: makeUri(mockTargetPath), getText: vi.fn().mockReturnValue("content"), + isDirty: docOverrides.isDirty ?? false, + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue(makePosition()), }), }) @@ -1130,17 +1269,510 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() removes the placeholder and created dirs when open() failed before the editor existed", async () => { + // open() creates the parent dirs and an empty placeholder BEFORE it awaits + // openDiffEditor(). If that await rejects there is no activeDiffEditor, and the + // rollback used to bail out - leaving an empty file that the next execute() mistook + // for an existing file, which a denial then preserved. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() keeps a file another process filled before the editorless rollback", async () => { + // open() failed before the diff editor existed, but another process wrote the path + // after the empty placeholder landed. An inode check cannot catch an in-place write, + // so the rollback inspects the size and must leave non-empty content alone. + const createdDirs = [`${mockCwd}/new-parent`] + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.stat).mockResolvedValueOnce({ size: 12 } as Awaited>) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenCalledWith(createdDirs[0]) + }) + + it("revertChanges() tolerates a placeholder that was never written", async () => { + // The failed open may have died before fs.writeFile ran; ENOENT during the + // rollback is success, not a new failure that would abort the cleanup. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + // The rollback still attempted the delete - the tolerance is about not + // aborting the rest of the cleanup, not about skipping it. + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + }) + + it("revertChanges() stands down after saveChanges() recorded the commit", async () => { + // The commit point is recorded on the provider, not only in the tool's local flag: + // an abort-driven teardown that reaches revertChanges() after the diagnostics delay + // must not restore the old content over the approved write or unlink a new file. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + writeCommitted: true, + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + }) + + it("revertChanges() stops before the document work for an existing file when no editor exists", async () => { + // The modify branch used to dereference activeDiffEditor unconditionally. When open() + // never produced an editor there is no document to restore, and the old code threw a + // TypeError out of the denial path instead of finishing the teardown. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "modify", + createdDirs: [], + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("revertChanges() keeps rolling back the remaining directories when one rmdir hits ENOENT", async () => { + // Same tolerance as the placeholder, for the created dirs: a directory that never + // landed must not abort the rest of the rollback and must not surface as a new failure. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(2) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() surfaces a non-ENOENT directory failure instead of swallowing it", async () => { + // The tolerance is scoped to ENOENT: a directory that exists but could not be removed + // is a real rollback failure and must reach the caller rather than be hidden. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") + }) + + it("revertChanges() surfaces a placeholder delete that failed for a non-ENOENT reason", async () => { + // The ENOENT tolerance is scoped: an unlink that fails for another reason means the + // unapproved placeholder is still on disk, and the caller must not be told the + // rollback succeeded. + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EACCES: permission denied") + }) + + it("revertChanges() leaves an earlier edit's file alone when no edit is in progress", async () => { + // reset() used to leave relPath behind while clearing editType. A later stream that never + // opened a diff view then reached this method with a stale relPath and no editType, took + // the new-file branch, and unlinked the PREVIOUS edit's target - an existing user file, + // permanently. Nothing may be deleted unless an edit is actually in progress. + Object.assign(diffViewProvider, { + isEditing: false, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: undefined, + createdDirs: [`${mockCwd}/new-parent`], + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("reset() clears relPath so a later teardown cannot reuse a stale target", async () => { + Object.assign(diffViewProvider, { isEditing: true, relPath: "mock-target-file.ts" }) + + await diffViewProvider.reset() + + expect(diffViewProvider["relPath"]).toBeUndefined() + }) + + it("reset() drops adopted directories so a later transaction cannot remove a previous one's", async () => { + // Adoption is per transaction: a stale list would make the next rollback rmdir + // directories belonging to a file the user has since accepted. + diffViewProvider.adoptCreatedDirs([`${mockCwd}/new-parent`]) + + await diffViewProvider.reset() + + expect(diffViewProvider["adoptedCreatedDirs"]).toEqual([]) + }) + it("revertChanges() restores the streamed buffer before closing it for a new file", async () => { + // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: closing + // a dirty tab prompts or is refused, which is how unapproved streamed content survived a + // "forced" close. The buffer must be restored to its pre-stream content and saved clean + // BEFORE the close, so close() never sees a dirty tab. + const editor = buildActiveDiffEditor({ isDirty: true }) + const teardown = diffViewProvider as unknown as { + restorePreStreamBuffer: (absolutePath: string) => Promise + saveBufferClean: (absolutePath: string) => Promise + } + let restoreArgs: unknown[] = [] + let saveArgs: unknown[] = [] + let restoreOrder = 0 + let saveOrder = 0 + const restore = vi.spyOn(teardown, "restorePreStreamBuffer").mockResolvedValue(undefined) + const saveClean = vi.spyOn(teardown, "saveBufferClean").mockResolvedValue(undefined) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await diffViewProvider.revertChanges() + // mockRestore() clears the recorded history, so the evidence is captured here. + restoreArgs = restore.mock.calls.map((args) => args[0]) + saveArgs = saveClean.mock.calls.map((args) => args[0]) + restoreOrder = restore.mock.invocationCallOrder[0] + saveOrder = saveClean.mock.invocationCallOrder[0] + } finally { + // Do not leak the fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + restore.mockRestore() + saveClean.mockRestore() + } + + expect(restoreArgs).toEqual([mockTargetPath]) + expect(saveArgs).toEqual([mockTargetPath]) + expect(saveOrder).toBeGreaterThan(restoreOrder) + const closeOrder = vi.mocked(vscode.window.tabGroups.close).mock.invocationCallOrder.at(-1) + expect(closeOrder).toBeGreaterThan(saveOrder) + // No force flag: the tab is clean by the time it is closed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() reports a close the editor refused instead of deleting underneath it", async () => { + // Even a restored, saved buffer can fail to close (a vetoing editor). Swallowing that let + // the rollback delete the file underneath an open tab. The failure must propagate, and + // the file must survive: unlinking after a refused close leaves an open tab on a ghost. + const editor = buildActiveDiffEditor({ isDirty: true }) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: vi.fn().mockResolvedValue(false), + configurable: true, + }) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not close the restored buffer") + // This test owns only the refusal handling; the restore-before-close ordering is + // asserted by the test above. What matters here is that the close was attempted + // without a force flag and the file survived a close the editor vetoed. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).not.toHaveBeenCalled() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + }) + + it("revertChanges() fails the rollback when the pre-stream restore edit is not applied", async () => { + // applyEdit() resolves false when the workspace refuses the edit. Dropping that boolean let + // the rollback save and close a buffer that still held the unapproved streamed content, and + // report success for a restore that never happened. + const editor = buildActiveDiffEditor({ isDirty: true }) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow( + "could not restore the pre-stream buffer", + ) + // The buffer never came back, so nothing may be saved, closed, or deleted on the + // assumption that it did. Counts, not call shapes: save, close, and unlink each have + // exactly one legitimate caller on this path, and all of them sit behind the restore. + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(close).toHaveBeenCalledTimes(0) + expect(fs.unlink).toHaveBeenCalledTimes(0) + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + }) + + it("revertChanges() keeps the workspace rejection as the cause of a failed restore", async () => { + // A restore that fails because the workspace rejected the edit keeps its reason: the + // rollback error carries the rejection as its cause, the shape the refused close uses. + const editor = buildActiveDiffEditor({ isDirty: true }) + const rejection = new Error("edit rejected by the workspace") + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(rejection) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + + let restoreFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + restoreFailure = error + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + + expect(restoreFailure).toBeInstanceOf(Error) + if (restoreFailure instanceof Error) { + expect(restoreFailure.message).toContain("could not restore the pre-stream buffer") + expect(restoreFailure.cause).toBe(rejection) + } + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(close).toHaveBeenCalledTimes(0) + expect(fs.unlink).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() restores, saves and closes when the restore edit is applied", async () => { + // Guard for the fix itself: an applied edit keeps the restore-save-close-delete flow + // exactly as it was; only a refused edit changes behaviour. + const editor = buildActiveDiffEditor({ isDirty: true }) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + + // The restore edit carried the pre-stream content, and the full flow ran exactly once. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("pre-stream content") + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(close).toHaveBeenCalledTimes(1) + expect(fs.unlink).toHaveBeenCalledTimes(1) + }) + it("revertChanges() closes the file tab when the file was not open and untouched", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) diffViewProvider["closeFileTab"] = closeFileTab diffViewProvider["relPath"] = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true diffViewProvider["documentWasOpen"] = false diffViewProvider["userTouchedDocument"] = false diffViewProvider["preEditScrollLine"] = undefined - diffViewProvider.editType = "modify" - diffViewProvider.originalContent = "original" + diffViewProvider["editType"] = "modify" + diffViewProvider["originalContent"] = "original" diffViewProvider["activeDiffEditor"] = buildActiveDiffEditor() await diffViewProvider.revertChanges() @@ -1152,15 +1784,17 @@ describe("DiffViewProvider", () => { it("revertChanges() keeps the file open when the user touched it", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) - vi.mocked(vscode.window.showTextDocument).mockResolvedValue(makeTextEditor()) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(makeTextEditor({ revealRange: vi.fn() })) diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) diffViewProvider["closeFileTab"] = closeFileTab diffViewProvider["relPath"] = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true diffViewProvider["documentWasOpen"] = false diffViewProvider["userTouchedDocument"] = true diffViewProvider["preEditScrollLine"] = undefined - diffViewProvider.editType = "modify" - diffViewProvider.originalContent = "original" + diffViewProvider["editType"] = "modify" + diffViewProvider["originalContent"] = "original" diffViewProvider["activeDiffEditor"] = buildActiveDiffEditor() await diffViewProvider.revertChanges() @@ -1169,6 +1803,183 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() fails the existing-file rollback when the revert edit is not applied", async () => { + // applyEdit() resolves false when the workspace refuses the edit. Dropping that boolean let + // the rollback save anyway and report a completed rollback for a restore that never + // happened: the document could still hold the unapproved content, and the caller's + // failure reporting - the whole point of the revertDiffChangesBeforeReset return value - + // was skipped. The edit result is validated before the save runs. + const editor = buildActiveDiffEditor() + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not restore the original content") + + // Counts, not call shapes: the save and both close paths each have exactly one legitimate + // caller on this branch, and all of them sit behind the edit check. + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() keeps the workspace rejection as the cause of a failed existing-file revert", async () => { + // A revert that fails because the workspace rejected the edit keeps its reason: the + // rollback error carries the rejection as its cause, the shape the refused close and the + // pre-stream restore use. + const editor = buildActiveDiffEditor() + const rejection = new Error("edit rejected by the workspace") + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(rejection) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + let revertFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + revertFailure = error + } + + expect(revertFailure).toBeInstanceOf(Error) + if (revertFailure instanceof Error) { + expect(revertFailure.message).toContain("could not restore the original content") + expect(revertFailure.cause).toBe(rejection) + } + expect(editor.document.save).toHaveBeenCalledTimes(0) + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() fails the existing-file rollback when the restored document save reports false", async () => { + // The double resolves an explicit false, not undefined: a bare await would let a double + // resolving undefined pass by accident, so the reported-failure contract is pinned with a + // real false. A refused save means the restored original content never reached disk; the + // rollback must fail before the close and tab bookkeeping assume it did. + const editor = buildActiveDiffEditor() + editor.document.save = vi.fn().mockResolvedValue(false) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored document") + + expect(editor.document.save).toHaveBeenCalledTimes(1) + // Throw before any later bookkeeping: the diff views stay open and the tab decision + // never runs on a document whose restored content may not be on disk. + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() keeps the save rejection as the cause of a failed existing-file revert", async () => { + // Same shape as the edit rejection: a save that rejects is a failed rollback, and the + // rejection is kept as the cause rather than ending up as a reported success. + const editor = buildActiveDiffEditor() + const rejection = new Error("save refused by the editor host") + editor.document.save = vi.fn().mockRejectedValue(rejection) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + let revertFailure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error) { + revertFailure = error + } + + expect(revertFailure).toBeInstanceOf(Error) + if (revertFailure instanceof Error) { + expect(revertFailure.message).toContain("could not save the restored document") + expect(revertFailure.cause).toBe(rejection) + } + expect(closeAllDiffViews).toHaveBeenCalledTimes(0) + expect(closeFileTab).toHaveBeenCalledTimes(0) + }) + + it("revertChanges() reverts, saves and closes when the edit is applied and the save reports true", async () => { + // Guard for the fix itself: an applied edit plus a save that reports true keeps the + // revert-save-close flow exactly as it was; only a refused edit or refused save changes + // behaviour. + const editor = buildActiveDiffEditor() + editor.document.save = vi.fn().mockResolvedValue(true) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + const closeFileTab = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeAllDiffViews"] = closeAllDiffViews + diffViewProvider["closeFileTab"] = closeFileTab + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + editType: "modify", + originalContent: "original", + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + activeDiffEditor: editor, + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + // The revert edit carried the original content back, the save ran once, and the full + // close and tab flow proceeded exactly as before the fix. closeAllDiffViews also runs + // inside the trailing reset(), so the success evidence is "at least once, before the + // tab decision", not an exact count. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("original") + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(closeAllDiffViews.mock.calls.length).toBeGreaterThanOrEqual(1) + expect(closeAllDiffViews.mock.invocationCallOrder[0]).toBeLessThan(closeFileTab.mock.invocationCallOrder[0]) + expect(closeFileTab).toHaveBeenCalledTimes(1) + }) + it("marks userTouchedDocument when the file editor is activated during the diff", async () => { let activeCallback: ((editor: vscode.TextEditor) => void) | undefined vi.mocked(vscode.window.onDidChangeActiveTextEditor).mockImplementation((cb) => { @@ -1212,11 +2023,16 @@ describe("DiffViewProvider", () => { describe("userTouchedDiffEditor keep/close behavior", () => { const mockTargetPath = `${mockCwd}/mock-target-file.ts` - const buildActiveDiffEditor = () => + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. + const buildActiveDiffEditor = (docOverrides: { isDirty?: boolean } = {}) => makeTextEditor({ document: makeTextDocument({ uri: makeUri(mockTargetPath), getText: vi.fn().mockReturnValue("content"), + isDirty: docOverrides.isDirty ?? false, + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue(makePosition()), }), }) @@ -1409,6 +2225,8 @@ describe("DiffViewProvider", () => { diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) diffViewProvider["closeFileTab"] = closeFileTab diffViewProvider["relPath"] = "mock-target-file.ts" + // revertChanges() only acts while an edit is in progress. + diffViewProvider["isEditing"] = true diffViewProvider["documentWasOpen"] = false diffViewProvider["userTouchedDocument"] = false // The user clicked inside the diff pane -- but this is a deny, so it must be ignored. @@ -1594,11 +2412,16 @@ describe("DiffViewProvider", () => { describe("auto-close settings decision table", () => { const mockTargetPath = `${mockCwd}/auto-close-test.ts` - const buildActiveDiffEditor = () => + // Both the approved save (saveChanges) and the existing-file rollback (revertChanges) read + // the save result, so the shared double reports a save that landed; an undefined- + // resolving double would read as a refused save. + const buildActiveDiffEditor = (docOverrides: { isDirty?: boolean } = {}) => makeTextEditor({ document: makeTextDocument({ uri: makeUri(mockTargetPath), getText: vi.fn().mockReturnValue("content"), + isDirty: docOverrides.isDirty ?? false, + save: vi.fn().mockResolvedValue(true), positionAt: vi.fn().mockReturnValue(makePosition()), }), }) diff --git a/src/utils/fs.ts b/src/utils/fs.ts index 9f7af84e4a..a832dd75de 100644 --- a/src/utils/fs.ts +++ b/src/utils/fs.ts @@ -23,9 +23,19 @@ export async function createDirectoriesForFile(filePath: string): Promise= 0; i--) { - await fs.mkdir(dirsToCreate[i]) - newDirectories.push(dirsToCreate[i]) + try { + for (let i = dirsToCreate.length - 1; i >= 0; i--) { + await fs.mkdir(dirsToCreate[i]) + newDirectories.push(dirsToCreate[i]) + } + } catch (error) { + // A later mkdir failure must not leave the directories this call already created + // behind: the caller never receives them, so it cannot clean them up. Remove them + // deepest-first, best effort, and surface the original failure. + for (const directory of [...newDirectories].reverse()) { + await fs.rmdir(directory).catch(() => {}) + } + throw error } return newDirectories From 7699e362cb6b040c9472434d2651e349af19e6ae Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 04:32:52 +0800 Subject: [PATCH 2/4] fix(tools): close a dirty diff tab before unlinking, and stop saveChanges reading fields a reset clears This answers the live review rows on head 2fa500ae46 (main 73931fe8be is its parent; this commit adds no merge, so no new main was merged here). discardFileTab() matched only TabInputText tabs. A rollback of a new file whose plain tab was already closed therefore unlinked the file under a still open, still dirty TabInputTextDiff: TabInputTextDiff is not a TabInputText, and closeAllDiffViews() skips dirty tabs, so nothing in the rollback could reach it. Measured against this head before the change: fs.unlink called once with the target, vscode.window.tabGroups.close called zero times. The filter now also matches a composite diff tab whose modified uri is the target, so the buffer is restored and saved and the tab closed before the unlink, and a refused close keeps the file instead of deleting under the tab. saveChanges() captured editedContent up front but then read this.newContent, this.relPath and this.preDiagnostics after awaiting provider state twice, the tab closes and the diagnostics window. reset() lands inside those awaits from Task.ts (it resets the provider at the top of every new API attempt) and from this tool own teardown paths. Interleaving reset() into the state await threw Cannot read properties of undefined (reading the includes method) with document.save() already called once and the commit point already passed, so a write the user approved was reported as a failure; getNewDiagnostics then compared against the empty list reset() installs, which makes every current problem look new. Those three fields are now captured with editedContent. Same interleaving after the change: still lands mid-save, still clears the fields, and the call returns the saved content instead of throwing. Test-only rows answered in the same commit: the success path now observes writeCommitted inside closeAllDiffViews and asserts onCommit precedes it; the two real restores assert the target uri and the positionAt offsets that make the range full-buffer (content alone passes on a misdirected or partial restore, and the Range double cannot express the extent, so the assertion goes through the offsets the impl actually hands it); the editorless rollback gains the occupied-directory case (retained non-empty file inside the created dir, rmdir rejecting ENOTEMPTY, which removeCreatedDir tolerates only for ENOENT) and the stat-cannot-answer case (skip the unlink, still clean the dirs this edit created). clearTaskState() is now identity-checked like the teardown release beside it, so the guarantee does not rest on instanceId being unique per Task object. The tab double gained modified.scheme because the implementation guard reads modified.scheme; the double was the only thing that did not carry it. Gates run on this tree before commit: vitest over DiffViewProvider.spec, writeToFileTool.spec and writeToFileTool-partial-state-cleanup.spec - 176 passed, 5 skipped (181); tsc --noEmit exit 0; and the package lint script (eslint . --ext=ts --max-warnings=0 --cache ...) exit 0 with the eslint cache deleted first, because per-file eslint over an explicit file list is a quick check and is not the gate. Co-Authored-Changed-By: bseries-merge (review-row answers, controls and measurements for PR #1931). --- src/core/tools/WriteToFileTool.ts | 11 ++ src/integrations/editor/DiffViewProvider.ts | 44 ++++- .../editor/__tests__/DiffViewProvider.spec.ts | 173 +++++++++++++++++- 3 files changed, 216 insertions(+), 12 deletions(-) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index fb2e74692f..42907b70d1 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -126,6 +126,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } public clearTaskState(task: Task): void { + // Identity-checked, like the teardown release above: the entry is dropped only when the map + // still holds THIS task's state object. The key is taskId + instanceId and instanceId is + // minted by crypto.randomUUID() in the Task constructor, so a second instance of the same + // history item does not inherit it - but this caller disposes the task it is clearing for, + // and a clear that matched only the key could still drop an entry a live stream had just + // re-created under the same taskId. Checking identity keeps that guarantee independent of + // how instanceId is minted. + const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) + if (state && state.task !== task) { + return + } this.resetTaskPartialState(task) } diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 1d31eaf561..dcd54506a2 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -388,6 +388,20 @@ export class DiffViewProvider { const absolutePath = path.resolve(this.cwd, this.relPath) const updatedDocument = this.activeDiffEditor.document const editedContent = updatedDocument.getText() + // Capture the fields the continuation below reads, as editedContent is already captured. saveChanges + // awaits provider state twice, the tab closes, and the diagnostics window before it + // normalizes EOLs and builds the user-edits patch, and reset() can land inside any of + // those awaits: Task.ts resets the provider at the top of every new API attempt, and + // this tool resets it on its own teardown paths. Reading this.newContent or this.relPath + // after those awaits threw a TypeError for a write that had already saved and had + // already passed the commit point, and getNewDiagnostics compared against the empty + // list reset() installs, so every current problem looked new. Measured: interleaving + // reset() into the provider-state await throws + // "Cannot read properties of undefined (reading 'includes')" at the EOL line with + // document.save() already called once. + const newContentForEol = this.newContent + const relPathForPatch = this.relPath + const preDiagnosticsForCompare = this.preDiagnostics if (updatedDocument.isDirty) { const saved = await updatedDocument.save() @@ -471,7 +485,7 @@ export class DiffViewProvider { const maxDiagnosticMessages = state?.maxDiagnosticMessages ?? 50 const newProblems = await diagnosticsToProblemsString( - getNewDiagnostics(this.preDiagnostics, postDiagnostics), + getNewDiagnostics(preDiagnosticsForCompare, postDiagnostics), [ vscode.DiagnosticSeverity.Error, // only including errors since warnings can be distracting (if user wants to fix warnings they can use the @problems mention) ], @@ -486,18 +500,18 @@ export class DiffViewProvider { // If the edited content has different EOL characters, we don't want to // show a diff with all the EOL differences. - const newContentEOL = this.newContent.includes("\r\n") ? "\r\n" : "\n" + const newContentEOL = newContentForEol.includes("\r\n") ? "\r\n" : "\n" // Normalize EOL characters without trimming content const normalizedEditedContent = editedContent.replace(/\r\n|\n/g, newContentEOL) // Just in case the new content has a mix of varying EOL characters. - const normalizedNewContent = this.newContent.replace(/\r\n|\n/g, newContentEOL) + const normalizedNewContent = newContentForEol.replace(/\r\n|\n/g, newContentEOL) if (normalizedEditedContent !== normalizedNewContent) { // User made changes before approving edit. const userEdits = formatResponse.createPrettyPatch( - this.relPath.toPosix(), + relPathForPatch!.toPosix(), normalizedNewContent, normalizedEditedContent, ) @@ -1022,12 +1036,22 @@ export class DiffViewProvider { private async discardFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) - .filter( - (tab) => - tab.input instanceof vscode.TabInputText && - tab.input.uri.scheme === "file" && - arePathsEqual(tab.input.uri.fsPath, absolutePath), - ) + .filter((tab) => { + const input = tab.input + if (input instanceof vscode.TabInputText) { + return input.uri.scheme === "file" && arePathsEqual(input.uri.fsPath, absolutePath) + } + // A composite diff tab for the same file counts too. The plain tab can be closed while + // the diff tab survives - the user closes it, or it was never opened separately - + // and TabInputTextDiff is not a TabInputText, so a text-only filter left the tab + // holding the unapproved content open while the rollback unlinked the file under it. + // closeAllDiffViews() will not take it either: it skips dirty tabs. This is the only + // place that can restore and close it before the unlink. + if (input instanceof vscode.TabInputTextDiff) { + return input.modified.scheme === "file" && arePathsEqual(input.modified.fsPath, absolutePath) + } + return false + }) for (const tab of tabs) { // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index c505532ade..17e21c4dcd 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -169,7 +169,7 @@ describe("DiffViewProvider", () => { const makeDiffTabInput = (originalScheme: string, modifiedFsPath: string): vscode.TabInputTextDiff => Object.assign(new vscode.TabInputTextDiff(makeUri(modifiedFsPath), makeUri(modifiedFsPath)), { original: { scheme: originalScheme }, - modified: { fsPath: modifiedFsPath }, + modified: makeUri(modifiedFsPath), }) const makeTab = (input: vscode.Tab["input"], label: string, overrides: Partial = {}): vscode.Tab => ({ @@ -1047,7 +1047,10 @@ describe("DiffViewProvider", () => { diffViewProvider["activeDiffEditor"] = { document: { getText: vi.fn().mockReturnValue("new content"), isDirty: true, save }, } as unknown as vscode.TextEditor - const closeAllDiffViews = vi.fn().mockResolvedValue(undefined) + let commitSeenByCleanup: boolean | undefined + const closeAllDiffViews = vi.fn().mockImplementation(async () => { + commitSeenByCleanup = diffViewProvider["writeCommitted"] + }) diffViewProvider["closeAllDiffViews"] = closeAllDiffViews const onCommit = vi.fn() @@ -1056,6 +1059,11 @@ describe("DiffViewProvider", () => { expect(save).toHaveBeenCalledTimes(1) expect(onCommit).toHaveBeenCalledTimes(1) expect(closeAllDiffViews).toHaveBeenCalledTimes(1) + // Ordering, not just presence: a regression that set the flag and fired the callback after + // cleanup would still satisfy the three counts above and would leave an approved write + // eligible for rollback in the window between the save and the tab close. + expect(commitSeenByCleanup).toBe(true) + expect(onCommit.mock.invocationCallOrder[0]).toBeLessThan(closeAllDiffViews.mock.invocationCallOrder[0]) expect(result.finalContent).toBe("new content") }) }) @@ -1755,11 +1763,168 @@ describe("DiffViewProvider", () => { // The restore edit carried the pre-stream content, and the full flow ran exactly once. expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("pre-stream content") + // The replacement text alone does not say where the text went: a restore aimed at another + // document, or at only part of this one, passes on content alone. The uri is the target + // the rollback located through textDocuments, and the extent is whatever positionAt + // answered with 0 and the full text length - the two offsets the impl hands to Range - + // so both halves of the claim are asserted through members the doubles actually carry. + expect((mockWorkspaceEdit.replace.mock.calls[0]?.[0] as { fsPath?: string }).fsPath).toBe(mockTargetPath) + expect(editor.document.positionAt).toHaveBeenCalledWith(0) + expect(editor.document.positionAt).toHaveBeenLastCalledWith(editor.document.getText().length) expect(editor.document.save).toHaveBeenCalledTimes(1) expect(close).toHaveBeenCalledTimes(1) expect(fs.unlink).toHaveBeenCalledTimes(1) }) + it("revertChanges() leaves a retained file and surfaces the occupied directory it cannot remove", async () => { + // The geometry the rollback is actually in: a non-empty file inside a directory this edit + // created. The retained file must survive, and the directory cannot go - a real rmdir on + // a non-empty directory fails with ENOTEMPTY, which removeCreatedDir tolerates only for + // ENOENT. Asserting the rejection is what stops the rollback from reporting a completed + // cleanup while a directory the user can still see is left behind. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "nested/mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.stat).mockResolvedValueOnce({ size: 12 } as Awaited>) + vi.mocked(fs.rmdir).mockRejectedValueOnce( + Object.assign(new Error("ENOTEMPTY: directory not empty, rmdir"), { code: "ENOTEMPTY" }), + ) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("ENOTEMPTY") + + expect(fs.unlink).not.toHaveBeenCalledWith(`${mockCwd}/nested/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(createdDirs[1]) + }) + + it("revertChanges() skips the unlink when stat cannot answer about the placeholder", async () => { + // stat is the only evidence the editorless rollback has that the path is still the empty + // placeholder this edit created. A stat that cannot answer is read as do-not-delete, and + // the directory cleanup this edit owns still runs. + const createdDirs = [`${mockCwd}/new-parent`] + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.stat).mockRejectedValueOnce( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(fs.stat).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.unlink).not.toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenCalledWith(createdDirs[0]) + }) + + it("revertChanges() restores and closes a dirty diff tab before unlinking the created file", async () => { + // The shape the text-only filter used to miss: the plain tab is gone and only the + // composite diff tab is left, holding the unapproved streamed content. TabInputTextDiff + // is not a TabInputText, and closeAllDiffViews() skips dirty tabs, so nothing used to + // close it and the unlink ran underneath it. + const editor = buildActiveDiffEditor({ isDirty: true }) + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const closeSpy = vi.spyOn(vscode.window.tabGroups, "close").mockResolvedValue(true) + const diffTab = { + input: makeDiffTabInput(DIFF_VIEW_URI_SCHEME, mockTargetPath), + isDirty: true, + label: "mock-target-file.ts (Changes)", + } + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [diffTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + writeCommitted: false, + }) + try { + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + expect(vscode.workspace.applyEdit).toHaveBeenCalledTimes(1) + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(closeSpy).toHaveBeenCalledTimes(1) + expect(fs.unlink).toHaveBeenCalledTimes(1) + expect(closeSpy.mock.invocationCallOrder[0]).toBeLessThan( + (fs.unlink as unknown as Mock).mock.invocationCallOrder[0], + ) + }) + + it("revertChanges() keeps the created file when the dirty diff tab refuses to close", async () => { + // Fail closed: a refused close means unapproved content is still on screen, so the + // unlink must not run. Deleting the file under a tab the user has not dismissed loses + // the content this rollback exists to preserve. + const editor = buildActiveDiffEditor({ isDirty: true }) + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + vi.spyOn(vscode.window.tabGroups, "close").mockResolvedValue(false) + const diffTab = { + input: makeDiffTabInput(DIFF_VIEW_URI_SCHEME, mockTargetPath), + isDirty: true, + label: "mock-target-file.ts (Changes)", + } + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [diffTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + writeCommitted: false, + }) + let failure: unknown + try { + await diffViewProvider.revertChanges() + } catch (error: unknown) { + failure = error + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + } + expect(failure).toBeInstanceOf(Error) + expect((failure as Error).message).toContain("could not close the restored buffer") + expect(fs.unlink).not.toHaveBeenCalledWith(mockTargetPath) + }) + it("revertChanges() closes the file tab when the file was not open and untouched", async () => { const closeFileTab = vi.fn().mockResolvedValue(undefined) vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) @@ -1974,6 +2139,10 @@ describe("DiffViewProvider", () => { // tab decision", not an exact count. expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) expect(mockWorkspaceEdit.replace.mock.calls[0]?.[2]).toBe("original") + // Same reasoning as the pre-stream restore above: content is not evidence about the target. + expect((mockWorkspaceEdit.replace.mock.calls[0]?.[0] as { fsPath?: string }).fsPath).toBe(mockTargetPath) + expect(editor.document.positionAt).toHaveBeenCalledWith(0) + expect(editor.document.positionAt).toHaveBeenLastCalledWith(editor.document.getText().length) expect(editor.document.save).toHaveBeenCalledTimes(1) expect(closeAllDiffViews.mock.calls.length).toBeGreaterThanOrEqual(1) expect(closeAllDiffViews.mock.invocationCallOrder[0]).toBeLessThan(closeFileTab.mock.invocationCallOrder[0]) From e8940caf777a1c6adb1e7136b6191b03c8d01f85 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 05:32:49 +0800 Subject: [PATCH 3/4] fix(editor): report a restored-buffer save that did not land before closing its tab Two review rows from the scheduled scan on 7699e362cb, both on code that commit introduced, landed together because the second is test-only. The ordering row (DiffViewProvider.ts:1051, Minor, Quick win) is real and is now fixed. discardFileTab ran restore -> save -> close and dropped the save result. tabGroups.close() takes preserveFocus, not a force-discard flag, so a tab that is still dirty goes to the editor, which prompts or refuses; and with the boolean dropped the rollback either reported nothing at all (a close that went through despite the dirty buffer) or reported a close failure that named the close rather than the save that never landed. saveBufferClean now reads the result and reports it, which is also how the existing-file rollback already treats the same boolean, so the close is attempted only after a save that landed and the created file is never unlinked underneath a tab whose buffer never got its pre-stream content back on disk. A save that rejects still propagates as before. The case that pins it is new and was measured against the unfixed tree first: "revertChanges() reports a restored-buffer save that did not land instead of closing the dirty tab" failed on 7699e362cb with `promise resolved "undefined" instead of rejecting` (1 failed | 103 passed) - the rollback reporting success on a buffer whose save reported false - and passes after the change (104 passed). The assertions that name the ordering are that close was never called and fs.unlink was never called, so the failure is reported where it happened and nothing after a successful save runs. The fixture row (DiffViewProvider.spec.ts:1788, Minor, Low value) was right about my own case. Its comment claimed a non-empty file inside a directory the edit created while createdDirs was new-parent and new-parent/nested and relPath was nested/mock-target-file.ts, which resolves outside both, so the case asserted an occupied-directory rollback without containing the occupancy it described. The retained file now lives inside the created tree, the parent-directory relation the comment claims is asserted rather than narrated, and the case states what the fixture cannot show: fs.rmdir is scripted to reject once, so it rejects whatever the contents are and does not derive the failure from occupancy - a stateful filesystem double is the fuller version, and it is deliberately not taken under this round budget so the sentence, not a silent upgrade, is what the reviewer sees. The case still fails and passes exactly as it did before the geometry fix (green both ways): the change is what the fixture claims, not what it asserts about the implementation. The third row on this head (DiffViewProvider.ts:404, Minor, Heavy lift) is answered in the thread and not coded. It is accurate about the boundary of the capture fix: three fields became local, the writes are still instance state, so a save awaiting diagnostics can write its results over a later save fields and that one reports the wrong patch or diagnostics under its own path. The honest remedy is a return-value refactor - saveChanges returns its result and every caller carries it - which touches all callers and the tool own commit-point bookkeeping: a unit-scale change, not this round. Gates on this tree: the new case red before and green after as quoted; widened unit scope (core/assistant-message, core/tools, core/task, integrations/editor) 97 files, 2009 passed / 5 skipped; tsc --noEmit exit 0; the package lint script exit 0 with the eslint cache deleted first. The full src suite runs 93 failed / 12667 passed, and the failing files are the same five as at the parent head 7699e362cb run (93 failed / 12666 passed, the single extra pass being this case): dist_assets, ShadowCheckpointService, code-index parser, MarketplaceManager, rules - this worktree has no src/dist, so those are environment gaps and not fallout here. No changeset, no CHANGELOG entry, no lint suppression added or increased. Co-Authored-Changed-By: bseries-merge (review-row answers, controls and measurements for PR #1931). --- src/integrations/editor/DiffViewProvider.ts | 19 ++++- .../editor/__tests__/DiffViewProvider.spec.ts | 85 ++++++++++++++++++- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index dcd54506a2..8cc408e3fe 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -1057,7 +1057,10 @@ export class DiffViewProvider { // tabGroups.close()'s second argument is preserveFocus, not a force-discard flag: a // dirty tab prompts or is refused, which is how unapproved streamed content survived a // "forced" close. Restore the buffer to its pre-stream content and save it clean first, - // so close() never sees a dirty tab and nothing unapproved reaches disk. + // so close() never sees a dirty tab and nothing unapproved reaches disk. saveBufferClean + // reports a save that did not land, so a false there stops this loop before the close is + // attempted: the tab stays open on the restored buffer instead of being closed on a dirty + // one, and the caller keeps its failure instead of receiving a misattributed close error. await this.restorePreStreamBuffer(absolutePath) await this.saveBufferClean(absolutePath) @@ -1121,13 +1124,25 @@ export class DiffViewProvider { * force-discard parameter, so a dirty tab would prompt or be refused; saving the restored * content is what makes the close unconditional. For a new file the placeholder is unlinked * a moment later, so the saved content is transient by design. + * + * The save result is read here, before the caller attempts the close, exactly as the + * existing-file rollback reads it. A save that reports false never landed: the buffer is still + * dirty, so the close would hand content the user never approved to the editor and the failure + * the rollback reported would name the close rather than the save that failed. Reporting it + * here leaves the tab open on the restored-but-unsaved buffer and stops the rollback before it + * unlinks the file underneath that tab. A save that rejects still propagates as before. */ private async saveBufferClean(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( (document) => document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, absolutePath), ) if (document?.isDirty) { - await document.save() + const saved = await document.save() + if (!saved) { + throw new Error( + `Rollback could not save the restored buffer for ${absolutePath}: the document save reported failure, so the buffer is still dirty and the tab is left open rather than closed on content the user never approved.`, + ) + } } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 17e21c4dcd..6025369654 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1776,16 +1776,91 @@ describe("DiffViewProvider", () => { expect(fs.unlink).toHaveBeenCalledTimes(1) }) + it("revertChanges() reports a restored-buffer save that did not land instead of closing the dirty tab", async () => { + // Ordering, not severity: close() has no force-discard parameter, so a tab that is still + // dirty goes to the editor, which prompts or refuses. The teardown used to run the save, + // drop its boolean, attempt the close anyway, and then either report nothing (a close that + // went through despite the dirty buffer) or report a close failure for what was a failed + // save. The order is now save result first, report on false, close only after a save that + // landed - so the tab is left open on restored-but-unsaved content and the created file is + // never unlinked underneath it. Red on the previous shape: the close ran and no failure was + // reported at all. + const editor = buildActiveDiffEditor({ isDirty: true }) + editor.document.save = vi.fn().mockResolvedValue(false) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + const originalDocs = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + const originalClose = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "close") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + get: () => [editor.document], + configurable: true, + }) + const close = vi.fn().mockResolvedValue(true) + Object.defineProperty(vscode.window.tabGroups, "close", { + value: close, + configurable: true, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + isEditing: true, + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "pre-stream content", + }) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocs) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocs) + } + if (originalClose) { + Object.defineProperty(vscode.window.tabGroups, "close", originalClose) + } + } + + // The restore landed first, so the failure being reported is the save and not the edit; the + // save was asked once; and nothing that follows a successful save ran. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(editor.document.save).toHaveBeenCalledTimes(1) + expect(close).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + }) + it("revertChanges() leaves a retained file and surfaces the occupied directory it cannot remove", async () => { // The geometry the rollback is actually in: a non-empty file inside a directory this edit // created. The retained file must survive, and the directory cannot go - a real rmdir on // a non-empty directory fails with ENOTEMPTY, which removeCreatedDir tolerates only for // ENOENT. Asserting the rejection is what stops the rollback from reporting a completed // cleanup while a directory the user can still see is left behind. - const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + // + // Honest about what this fixture can and cannot show: the rmdir rejection below is scripted + // once, so it rejects whatever the contents are and does not derive the failure from + // occupancy. The geometry - the retained file living inside the directory the rollback is + // removing - is therefore the only part of the case that carries the occupancy claim, and + // it is asserted below rather than left to the comment. A stateful filesystem double that + // rejects rmdir only for a non-empty directory is the fuller version of this test. + const occupiedDir = `${mockCwd}/new-parent/nested` + const retainedFile = `${occupiedDir}/mock-target-file.ts` + const createdDirs = [`${mockCwd}/new-parent`, occupiedDir] Object.assign(diffViewProvider, { isEditing: true, - relPath: "nested/mock-target-file.ts", + relPath: "new-parent/nested/mock-target-file.ts", activeDiffEditor: undefined, editType: "create", createdDirs, @@ -1797,9 +1872,13 @@ describe("DiffViewProvider", () => { await expect(diffViewProvider.revertChanges()).rejects.toThrow("ENOTEMPTY") - expect(fs.unlink).not.toHaveBeenCalledWith(`${mockCwd}/nested/mock-target-file.ts`) + expect(fs.unlink).not.toHaveBeenCalledWith(retainedFile) expect(fs.rmdir).toHaveBeenCalledTimes(1) expect(fs.rmdir).toHaveBeenCalledWith(createdDirs[1]) + // The occupancy the case claims: the directory rmdir was asked to remove is the directory + // the retained file is in. Without this the two fixture halves could name unrelated paths + // and the case would still pass on the scripted rejection. + expect(createdDirs.at(-1)).toBe(retainedFile.slice(0, retainedFile.lastIndexOf("/"))) }) it("revertChanges() skips the unlink when stat cannot answer about the placeholder", async () => { From 91413d8915ad9eef3184e1988e0efeafb188c780 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 8 Nov 2026 08:59:20 +0800 Subject: [PATCH 4/4] fix(editor): scope the rollback save failure to the attempt that hit it The rollback's save-failure message promised that the buffer is still dirty and that the tab is left open. Those describe the state after the error, not what this attempt did: a later retry can close that tab, and the buffer's later state is not this rollback's to promise. The message now says where this attempt stopped - before the close was tried, rather than closing on content the user never approved - and the doc comment for saveBufferClean says the same, including that a later attempt may still close the tab. The case for this path already asserted that the close and the unlink never ran; it now pins the wording too, so a message that claims lasting state again fails. Measured with the pinned Node 22.23.4: against the committed source the new assertions fail ("expected 'Rollback could not save the restored ...' to contain 'this attempt stopped before closing t...'"); with this change integrations/editor is 2 files / 112 tests green under vitest.unit.config.ts. The src package's own command (vitest run, same runtime) reports 12708 passed with 52 failures, all in the known set - 40 __tests__/dist_assets.spec.ts, 5 services/rules, 1 ShadowCheckpointService, 4 MarketplaceManager, 2 code-index parser - so 45 known-local plus 7 proven-environment and no head-only failure. eslint --max-warnings=0 with the cache deleted after the last prettier write, prettier --check and tsc --noEmit are clean, src/eslint-suppressions.json stays at blob 42be14463c29 (equal to main tip), and packages/config-eslint is identical to 73931fe8be25. Review rows: the row on the rollback message is answered by this change. The row asking that each save keep its path, patch content and diagnostics in save-local state is answered without a code change - the capture at DiffViewProvider.ts:402-404 already holds all three for the continuation, nothing after the first await in saveChanges reads this.newContent, this.relPath or this.preDiagnostics, and the reporting side reads the captured values at :488, :503, :509 and :514. Both rows sit in code this PR introduces: upstream 5a7f3f81b touched 3 lines of this file and 643 removed lines of its spec, and main has neither saveBufferClean nor the capture block, so there was nothing to drop in favour of an upstream equivalent. --- src/integrations/editor/DiffViewProvider.ts | 8 +++++--- .../editor/__tests__/DiffViewProvider.spec.ts | 10 +++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 8cc408e3fe..e2ee190cf8 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -1129,8 +1129,10 @@ export class DiffViewProvider { * existing-file rollback reads it. A save that reports false never landed: the buffer is still * dirty, so the close would hand content the user never approved to the editor and the failure * the rollback reported would name the close rather than the save that failed. Reporting it - * here leaves the tab open on the restored-but-unsaved buffer and stops the rollback before it - * unlinks the file underneath that tab. A save that rejects still propagates as before. + * here stops this attempt before the close is tried: the tab is still open on the + * restored-but-unsaved buffer when the error is raised, the rollback does not unlink the file + * underneath it, and a later attempt may still close that tab. A save that rejects still + * propagates as before. */ private async saveBufferClean(absolutePath: string): Promise { const document = vscode.workspace.textDocuments.find( @@ -1140,7 +1142,7 @@ export class DiffViewProvider { const saved = await document.save() if (!saved) { throw new Error( - `Rollback could not save the restored buffer for ${absolutePath}: the document save reported failure, so the buffer is still dirty and the tab is left open rather than closed on content the user never approved.`, + `Rollback could not save the restored buffer for ${absolutePath}: the document save reported failure, so this attempt stopped before closing the tab, rather than closing on content the user never approved.`, ) } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 6025369654..4dc3cd590f 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1821,7 +1821,15 @@ describe("DiffViewProvider", () => { }) try { - await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not save the restored buffer") + const failure = await diffViewProvider.revertChanges().then( + () => undefined, + (error: unknown): Error => (error instanceof Error ? error : new Error(String(error))), + ) + // Only this attempt is described: the message says where the rollback stopped and makes + // no promise about the buffer or the tab afterwards, because a later retry may close it. + expect(failure?.message).toContain("could not save the restored buffer") + expect(failure?.message).toContain("this attempt stopped before closing the tab") + expect(failure?.message).not.toMatch(/still dirty|left open/) } finally { if (originalTabs) { Object.defineProperty(vscode.window.tabGroups, "all", originalTabs)