From d52c1614f6e4780ce94b54c0357278b842cdebc1 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 12 Nov 2026 09:45:00 +0800 Subject: [PATCH 1/3] fix(editor): release abandoned diff streams and owned placeholders (split 5/6 of #1066, unit U6a) Diff-view half of #1932 replayed onto main tip 53cb5b5e7c80: discardUnapprovedStream and discardOwnedPlaceholder cover streams that were abandoned, changed out from under a retained session, or left by an unconfirmed save; isEditingDifferentPath guards a second path while a session is retained, and placeholderPath stays owned until the rollback removes it. The tool-side half moves to its own unit; plan and measured widths in https://github.com/Zoo-Code-Org/Zoo-Code/issues/1937#issuecomment-6811832404. The spec keeps its dirty-flag assignments legal after upstream rewrote the doubles with the typed test-utils helpers: one locally widened ActiveDiffEditor type on the shared builder instead of eight assertions, because vscode types TextDocument.isDirty as read-only. Review pass on the CodeRabbit change-request filed at this head: a refused buffer restore now stops the rollback outright the same way an unconfirmed save of the restored buffer does, so the tabs stay open, nothing is unlinked and the placeholder keeps its owner -- the spec for that path now requires that no unlink happens instead of asserting one. The commit-point test seeds placeholderPath so its release assertion can fail, and isEditingDifferentPath / editingRelPath are pinned directly (retained path accepted, another path rejected, no normalisation) because every other call site stands in for that predicate with a double. Threads: https://github.com/Zoo-Code-Org/Zoo-Code/pull/1932#discussion_r4545425302 and https://github.com/Zoo-Code-Org/Zoo-Code/pull/2434#discussion_r4545466602. Second review round on the shipped head: a rejected buffer restore or save now holds the artifact sweep as firmly as a refused one does, so the placeholder and the created directories stay owned and on disk and the directory snapshot goes back to the session - which is what tells the caller-side guard to keep the session for a retry instead of resetting it away. Only a confirmed buffer outcome lets the sweep run, so a tab-close failure on an already-restored buffer still removes the artifacts. The contract test that pinned the old behaviour (sweep anyway when applyEdit rejects) is replaced by tests for both rejected awaits, and the owned-placeholder release test now asserts the released state through the public ownsPlaceholder getter. Threads: https://github.com/Zoo-Code-Org/Zoo-Code/pull/1932#discussion_r4545933671 and https://github.com/Zoo-Code-Org/Zoo-Code/pull/1932#discussion_r4545933661. Third review round, filed against the previous head on the sibling pull (#2434 discussion_r4546267986, Minor): a sweep that stops partway - the placeholder unlink is its first step - left the created directories on disk but untracked, because this unit clears the field up front and only ever handed the snapshot back on the unconfirmed-buffer path. The retained session then forgot them and a retry could only ever remove the file. The catch now hands the snapshot back on that path too; the helpers tolerate whatever a partial sweep already removed, so the next discard finishes the sweep in reverse order. Where only a directory removal failed, ownership is already released and the caller-side reset clears the field again, so that path is unchanged. A regression makes the unlink fail with EPERM once, keeps the session, and asserts that the retry removes both directories in reverse order and releases the placeholder. --- src/integrations/editor/DiffViewProvider.ts | 556 ++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 1370 ++++++++++++++++- 2 files changed, 1867 insertions(+), 59 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 3c1d33ae28..20458837c3 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -21,6 +21,11 @@ import { Task } from "../../core/task/Task" import { DecorationController } from "./DecorationController" +/** Narrow ENOENT test so a rollback can tolerate a file or dir that never landed. */ +function isEnoent(error: unknown): boolean { + return typeof error === "object" && error !== null && (error as { code?: unknown }).code === "ENOENT" +} + export const DIFF_VIEW_URI_SCHEME = "cline-diff" export const DIFF_VIEW_LABEL_CHANGES = "Original ↔ Zoo's Changes" @@ -33,6 +38,14 @@ export class DiffViewProvider { isEditing = false originalContent: string | undefined private createdDirs: string[] = [] + /** + * Absolute path of the empty placeholder open() created for a new-file edit, or undefined + * when no placeholder is outstanding. reset() does NOT clear relPath, and saveDirectly() + * sets relPath for a file that was written with approval, so relPath alone cannot tell a + * caller that a file on disk belongs to the abandoned edit. Only this field justifies an + * unlink. + */ + private placeholderPath: string | undefined private documentWasOpen = false // Tracks whether the target file's tab was pinned before the diff session. // Closing the tab to open the diff drops VS Code's pin state, so we restore @@ -132,6 +145,10 @@ export class DiffViewProvider { // Make sure the file exists before we open it. if (!fileExists) { await fs.writeFile(absolutePath, "") + // From here until the placeholder is removed, THIS edit owns that path. Set after + // the write succeeds, so a failed write does not claim ownership of a file we did + // not create. + this.placeholderPath = absolutePath } // If the file was already open, close it (must happen after showing the @@ -326,6 +343,7 @@ export class DiffViewProvider { async saveChanges( diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + onCommit?: () => void, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -339,10 +357,27 @@ export class DiffViewProvider { const updatedDocument = this.activeDiffEditor.document const editedContent = updatedDocument.getText() - if (updatedDocument.isDirty) { - await updatedDocument.save() + if (updatedDocument.isDirty && !(await updatedDocument.save())) { + // save() resolving false (without throwing) means the write did not persist. Stop + // BEFORE the commit point: onCommit has not run and placeholderPath stays owned, so + // the caller still holds an uncommitted cleanup state and the teardown removes what + // open() created. Reporting this write as durable would strand an unpersisted file + // whose ownership had already been released. + throw new Error(`Saving ${absolutePath} was not confirmed; the write was not committed.`) } + // The write is irreversible from here: a clean document already matches disk, and a saved + // one has landed. Everything below (closing the diff views, tab bookkeeping, diagnostics) + // can still reject, and a caller that rolls the tool call back after this point would undo + // a write the user approved - so it is told, at the commit point rather than on return. + onCommit?.() + + // Ownership is released only once the approved write has landed. A save that rejects leaves + // the placeholder as open() created it, and the caller's teardown still has to remove it: + // clearing it here would tell discardUnapprovedStream() that nothing this edit created is + // left to clean, and an empty or half-written new file would survive the failed write. + this.placeholderPath = undefined + // Stop tracking touches and cancel any pending scroll-to-diff before any // programmatic editor activation below. this.disposeActiveEditorListener() @@ -513,14 +548,260 @@ 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 + } + } + } + + /** + * Whether this edit still owns a placeholder file that cleanup must remove. + * True from the moment open() creates the file until a rollback or commit removes + * it; a failed removal leaves it true so the caller can keep the session (and the + * retry path) instead of resetting the ownership away. + */ + get ownsPlaceholder(): boolean { + return this.placeholderPath !== undefined + } + + /** + * Whether an active edit session belongs to a different path than the one a + * call is about to write. A session is retained when its rollback could not + * complete (a refused save keeps \`isEditing\` and the dirty buffer so the + * user's content survives); a later write for ANOTHER path must not reuse + * that session: it would update and save the old document while requesting + * approval for the new path. + */ + isEditingDifferentPath(relPath: string): boolean { + if (!this.isEditing || this.relPath === undefined) { + return false + } + return this.relPath !== relPath + } + + /** + * The path of the retained edit session, when one is active. Used to tell the + * caller which write must be resolved before another path can be written. + */ + get editingRelPath(): string | undefined { + return this.isEditing ? this.relPath : undefined + } + + /** + * Release the placeholder this edit owns when the approved write never committed: + * saveChanges() threw before its commit point, so the file on disk is still the empty + * placeholder open() created. Leaving it behind unowned would make a later attempt + * treat it as the pre-existing target and strand an empty file no rollback recognizes + * anymore. The dirty editor (it holds the approved content for a manual save or retry) + * and the created directories are deliberately untouched: that save needs both the + * buffer and its parent directories. + */ + async discardOwnedPlaceholder(): Promise { + const placeholderPath = this.placeholderPath + if (!placeholderPath) { + return + } + await this.removeCreatedFile(placeholderPath) + this.placeholderPath = undefined + } + + /** + * Release a diff view whose content was never approved: an abandoned partial stream, + * a failed stream, or a write the user was never asked to approve (rooignore denial, + * validation failure). Deliberately NOT the same as revertChanges(), because + * revertChanges() SAVES and neither abandoned case may write to disk: + * + * - create: its new-file branch saves a dirty buffer as-is before deleting the file, + * which writes partial model output the task never approved - and leaves it on disk + * if the delete then fails. + * - modify: its branch restores the original content and saves it. That is a write to + * a file the user never approved, and for a rooignore-denial path a write the + * policy forbids outright. + * + * Here the target file is never written. A create buffer is emptied FIRST, so the + * only bytes that can ever reach the placeholder are none and the tab is clean enough + * to close without a prompt; a modify buffer is restored to the content already on + * disk in memory only, so the file itself is untouched and the tab (now showing the + * original content, still marked dirty) is left for the user rather than force-closed + * over any edits they may have typed into the preview. The placeholder plus the directories this edit + * created are removed once the buffer outcome is confirmed - a restore applyEdit accepted and, for a + * create, a save it confirmed. While that outcome is unconfirmed the artifacts stay owned and on disk: + * deleting them under a buffer that still holds unapproved content leaves that buffer one save away + * from recreating a file nobody approved. + */ + async discardUnapprovedStream(): Promise { + if (!this.relPath) { + return + } + + const absolutePath = path.resolve(this.cwd, this.relPath) + // Snapshot the directories this edit created BEFORE the editor work below, and clear the field so + // nothing else can act on them twice. The sweep below runs when the buffer outcome is confirmed; + // when it is not, the catch hands this snapshot back, so the retained session's next cleanup still + // knows what this edit put on disk. + const createdDirs = this.createdDirs + this.createdDirs = [] + // Same snapshot for the placeholder, but the ownership stays set until the unlink + // actually succeeds: a failed removal (EPERM) must leave the file owned so the + // caller can keep the session and a retry path instead of stranding it unowned. + const placeholderPath = this.placeholderPath + + let editorFailure: unknown + + // open() creates the directories and the empty placeholder BEFORE it assigns + // activeDiffEditor (openDiffEditor() can reject on its 10s timeout or a failed + // vscode.diff call), so an abandoned create can leave artifacts on disk with no + // editor at all. Only the buffer and tab work needs the editor; the artifact + // cleanup below runs either way. + if (this.activeDiffEditor) { + const document = this.activeDiffEditor.document + // Held until the buffer outcome below confirms itself. A restore applyEdit declined, a save that + // did not confirm, or a rejection anywhere before that confirmation leaves unapproved content in + // a buffer whose tab is still open, and unlinking the file underneath it would let a later save + // of that buffer recreate the file with content nobody ever approved. + let bufferUnconfirmed = true + try { + this.disposeActiveEditorListener() + this.cancelDeferredScroll() + + if (document.isDirty) { + // Restore the buffer to what this edit started from - the empty placeholder for a create, + // the content already on disk for a modify. A refused applyEdit leaves the unapproved streamed + // content in the buffer, and saving then would persist exactly what this method exists to + // discard, so the save is conditional and a refused restore stops the rollback outright below. + const edit = new vscode.WorkspaceEdit() + const fullRange = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ) + const restoredContent = + this.editType === "modify" ? this.stripAllBOMs(this.originalContent ?? "") : "" + edit.replace(document.uri, fullRange, restoredContent) + const applied = await vscode.workspace.applyEdit(edit) + if (!applied) { + // A refused restore leaves the unapproved content in the buffer: closing the tabs and sweeping + // the artifacts underneath it would strand that content with nothing left that can recover it, + // so this stops the rollback outright, exactly as an unconfirmed save of the restored buffer + // does below. The catch restores the createdDirs snapshot and the placeholder stays owned, + // does below. The catch restores the createdDirs snapshot and the placeholder stays owned, + // which is what tells the caller to skip reset() and keep the session for a retry. + throw new Error( + `Could not restore the diff editor buffer for ${this.relPath}; it may still hold unapproved content.`, + ) + } else if (this.editType !== "modify") { + // Only the emptied placeholder is ever saved: this edit created that file, and + // closing a dirty tab would prompt. A modify is never saved here - a modify's + // restore is an in-memory revert, and saving it would write to a file the user + // never approved (see the method comment). A false save leaves the buffer dirty + // over the placeholder: closeAllDiffViews() would skip the dirty tab and the + // unlink would delete the file underneath it, leaving the buffer one save away + // from recreating it - the same hazard revertChanges() refuses, so the rollback + // stops here with the placeholder still owned and the session still retryable. + if (!(await document.save())) { + throw new Error( + `Rollback could not save the restored buffer for ${this.relPath}; its clean state is unconfirmed, so the cleanup stopped before closing tabs or deleting the placeholder.`, + ) + } + } + } + // Close the diff tabs only now: a vscode.diff tab is dirty while its modified side + // holds the streamed content, and closeAllDiffViews() deliberately skips dirty tabs to + // avoid a save prompt. Closing before the restore above left that tab open over a file + // this method is about to unlink. + // Confirmed: the buffer holds nothing this edit streamed - restored, and for a create saved, or + // already clean. A rejection from the closes below can leave a tab open over a clean buffer, a + // lesser hazard than an artifact nothing can remove once the caller resets the session, so + // those rejections still fall through to the sweep. + bufferUnconfirmed = false + await this.closeAllDiffViews() + await this.closeFileTab(absolutePath) + } catch (error) { + // An unconfirmed buffer outcome keeps the placeholder and the created directories exactly as + // they are - owned, on disk - because the retained session is what a retry works from. + if (bufferUnconfirmed) { + // The rollback stopped before the artifact sweep: hand the directory snapshot back with + // the still-owned placeholder, so the next cleanup of this session knows what this edit + // created. The retained ownership is what tells the caller to skip reset(). + this.createdDirs = createdDirs + throw error + } + // Only the tab closes failed, on a buffer whose outcome is confirmed: the placeholder and the + // created directories still have to go, because the caller resets the session afterwards and + // nothing else can remove them. The original failure is re-thrown once they are dealt with. + editorFailure = error + } + } + + let cleanupFailure: unknown + try { + // Only a placeholder THIS edit created may be removed. relPath survives reset(), and + // saveDirectly() sets it for a file that was written with approval, so unlinking + // absolutePath unconditionally can delete content the user approved. + if (placeholderPath) { + // The helpers carry the ENOENT tolerance: open() creates the placeholder, + // and if it is already gone there is nothing left to remove. + await this.removeCreatedFile(placeholderPath) + this.placeholderPath = undefined + } + + // Remove only the directories this edit created, in reverse order. + for (let i = createdDirs.length - 1; i >= 0; i--) { + await this.removeCreatedDir(createdDirs[i]) + } + } catch (error) { + // A sweep that stopped partway leaves whatever it did not reach on disk and untracked, + // because this field was cleared up front. Hand the snapshot back: when the unlink is the + // step that failed the placeholder is still owned, the session is retained, and the next + // discard finishes the sweep in reverse order - the helpers tolerate whatever a partial sweep + // already removed. When only a directory removal failed, ownership is already released and + // the caller-side reset clears this field again, so nothing changes on that path. + this.createdDirs = createdDirs + cleanupFailure = error + console.error("Error removing abandoned write_to_file artifacts:", error) + } + + if (editorFailure) { + throw editorFailure instanceof Error ? editorFailure : new Error(String(editorFailure)) + } + if (cleanupFailure) { + throw cleanupFailure instanceof Error ? cleanupFailure : new Error(String(cleanupFailure)) + } + } + async revertChanges(): Promise { - if (!this.relPath || !this.activeDiffEditor) { + if (!this.relPath) { return } const fileExists = this.editType === "modify" - const updatedDocument = this.activeDiffEditor.document const absolutePath = path.resolve(this.cwd, this.relPath) + // Only the placeholder THIS edit owns may be deleted. A stale session (a prior reset() + // whose close failed) can carry relPath onto the next call, and the path derived from + // it may name a file this edit never created - e.g. the previously approved write. + // Ownership is released only once the unlink succeeds: a rollback that fails partway + // (refused restore, EPERM unlink) must leave the placeholder OWNED so the caller can + // skip reset() and keep a retry path, instead of stranding the empty file unowned. + // A double unlink from a later discard is safe - removeCreatedFile tolerates ENOENT. // Stop tracking touches and cancel any pending scroll-to-diff before any // programmatic editor activation below. @@ -528,52 +809,101 @@ 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 diff tab's modified side holds the streamed content, and the tab is dirty + // while it does: closeAllDiffViews() skips dirty tabs (closing one would prompt), + // so the rollback must put the pre-stream content back and confirm a clean save + // BEFORE any tab or file goes. Unlinking the placeholder underneath an open dirty + // diff tab left the unapproved content one save away from recreating the deleted + // file. A refused restore or an unconfirmed save stops the rollback here - before + // the close and the delete - and propagates as the rollback hazard it is. + const edit = new vscode.WorkspaceEdit() + const fullRange = new vscode.Range( + updatedDocument.positionAt(0), + updatedDocument.positionAt(updatedDocument.getText().length), + ) + edit.replace(updatedDocument.uri, fullRange, this.originalContent ?? "") + if (!(await vscode.workspace.applyEdit(edit))) { + throw new Error( + `Rollback could not restore the buffer for ${absolutePath}; it may still hold unapproved content, so the rollback stopped before closing tabs or deleting the file.`, + ) + } + if (!(await updatedDocument.save())) { + throw new Error( + `Rollback could not save the restored buffer for ${absolutePath}; its clean state is unconfirmed, so the rollback stopped before closing tabs or deleting the file.`, + ) + } + // A file-scheme buffer for the same path, if one is open, holds the same + // unapproved content: discard it with the same restore-save-close discipline. + 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.placeholderPath) { + await this.removeCreatedFile(this.placeholderPath) + // The file is gone: release ownership before the directory loop, so a later + // failure there cannot strand an ownership that no longer has a file. + this.placeholderPath = undefined + } // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { - await fs.rmdir(this.createdDirs[i]) + await this.removeCreatedDir(this.createdDirs[i]) } } else { - // Revert document. - const edit = new vscode.WorkspaceEdit() + // The document revert needs the editor. A failed open() (openDiffEditor rejects + // after open() already set relPath/isEditing) leaves none: skip only the document + // work then, and let execution reach the shared teardown below - returning early + // would leave relPath, editType, isEditing and the preview-tab snapshot behind, + // the exact stale-session state this method's rollback guarantees rule out. + const updatedDocument = this.activeDiffEditor?.document + if (updatedDocument) { + // Revert document. + const edit = new vscode.WorkspaceEdit() - const fullRange = new vscode.Range( - updatedDocument.positionAt(0), - updatedDocument.positionAt(updatedDocument.getText().length), - ) + const fullRange = new vscode.Range( + updatedDocument.positionAt(0), + updatedDocument.positionAt(updatedDocument.getText().length), + ) - edit.replace(updatedDocument.uri, fullRange, this.stripAllBOMs(this.originalContent ?? "")) + edit.replace(updatedDocument.uri, fullRange, this.stripAllBOMs(this.originalContent ?? "")) - // 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() + // 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() - await this.closeAllDiffViews() + await this.closeAllDiffViews() - // Read auto-close preferences from state; fall back to defaults that - // preserve the existing behavior when unset. - const revertTask = this.taskRef.deref() - const revertState = await revertTask?.providerRef.deref()?.getState() - - await this.keepOrCloseEditedFile( - absolutePath, - false, - revertState?.autoCloseZooOpenedFiles ?? DEFAULT_AUTO_CLOSE_ZOO_OPENED_FILES, - revertState?.autoCloseZooOpenedFilesAfterUserEdited ?? - DEFAULT_AUTO_CLOSE_ZOO_OPENED_FILES_AFTER_USER_EDITED, - revertState?.autoCloseZooOpenedNewFiles ?? DEFAULT_AUTO_CLOSE_ZOO_OPENED_NEW_FILES, - ) + // Read auto-close preferences from state; fall back to defaults that + // preserve the existing behavior when unset. + const revertTask = this.taskRef.deref() + const revertState = await revertTask?.providerRef.deref()?.getState() + + await this.keepOrCloseEditedFile( + absolutePath, + false, + revertState?.autoCloseZooOpenedFiles ?? DEFAULT_AUTO_CLOSE_ZOO_OPENED_FILES, + revertState?.autoCloseZooOpenedFilesAfterUserEdited ?? + DEFAULT_AUTO_CLOSE_ZOO_OPENED_FILES_AFTER_USER_EDITED, + revertState?.autoCloseZooOpenedNewFiles ?? DEFAULT_AUTO_CLOSE_ZOO_OPENED_NEW_FILES, + ) + } } // Restore any preview tabs the diff evicted, reconstructing the user's @@ -847,6 +1177,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. A refused + // restore or an unconfirmed clean save stops the rollback HERE: closing anyway would + // hand the user a save-confirmation prompt over a buffer still holding unapproved + // content (or an unconfirmed state), and answering it yes writes exactly what this + // rollback exists to discard. + if (!(await this.restorePreStreamBuffer(absolutePath))) { + throw new Error( + `Rollback could not restore the buffer for ${absolutePath}; it may still hold unapproved content, so the tab was left open rather than closed.`, + ) + } + if (!(await this.saveBufferClean(absolutePath))) { + throw new Error( + `Rollback could not save the restored buffer for ${absolutePath}; its clean state is unconfirmed, so the tab was left open rather than closed.`, + ) + } + + 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, so a buffer + * that could not be closed never keeps unapproved content available to save. + * + * Returns false when VS Code refuses the edit: the buffer still holds the streamed content, + * and discardFileTab() stops the rollback rather than closing or saving over it. Returns + * true when no open buffer needed restoring or the restore applied. + */ + 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 true + } + 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 ?? "") + return vscode.workspace.applyEdit(edit) + } + + /** + * Persist a restored buffer so the tab is clean before it is closed: close() refuses or prompts + * on a dirty tab, and a refused close is how unapproved content outlived a "forced" rollback. + * Only reached after discardFileTab()'s restore applied, so what lands here is the pre-stream + * content, never the streamed partial. + * + * Returns false when document.save() does not confirm the save: the buffer may still be dirty, + * so closing would prompt over an unconfirmed state and discardFileTab() stops the rollback + * instead. Returns true when the save completed or there was nothing to save. + */ + 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) { + return true + } + return document.save() + } + private async closeFileTab(absolutePath: string): Promise { const tabs = vscode.window.tabGroups.all .flatMap((group) => group.tabs) @@ -1109,25 +1537,39 @@ export class DiffViewProvider { this.disposeActiveEditorListener() this.cancelDeferredScroll() - await this.closeAllDiffViews() - this.editType = undefined - this.isEditing = false - this.originalContent = undefined - this.createdDirs = [] - this.documentWasOpen = false - this.documentWasPinned = false - this.activeDiffEditor = undefined - this.fadedOverlayController = undefined - this.activeLineController = undefined - this.streamedLines = [] - this.preDiagnostics = [] - this.preEditScrollLine = undefined - this.diffScrollLine = undefined - this.targetFileScrollLine = undefined - this.lastScrolledSource = undefined - this.userTouchedDocument = false - this.userTouchedDiffEditor = false - this.snapshotPreviewTabs = [] + try { + await this.closeAllDiffViews() + } finally { + // Clear the session even when the close rejected: a stale relPath/editType/isEditing + // surviving a failed reset is what turned one bad tab-close into loss of approved + // content - the next execution skips open(), reuses the stale create session, and a + // later denial's rollback deletes the file the previous write approved. The close + // failure still propagates to the caller. + this.editType = undefined + this.isEditing = false + // A session's path must not outlive the session: revertChanges() reads relPath to decide + // what to roll back, and a stale relPath with editType cleared points the next rollback at + // whatever file this provider last touched - a file the current edit never created. + this.relPath = undefined + this.newContent = undefined + this.originalContent = undefined + this.createdDirs = [] + this.placeholderPath = undefined + this.documentWasOpen = false + this.documentWasPinned = false + this.activeDiffEditor = undefined + this.fadedOverlayController = undefined + this.activeLineController = undefined + this.streamedLines = [] + this.preDiagnostics = [] + this.preEditScrollLine = undefined + this.diffScrollLine = undefined + this.targetFileScrollLine = undefined + this.lastScrolledSource = undefined + this.userTouchedDocument = false + this.userTouchedDiffEditor = false + this.snapshotPreviewTabs = [] + } } /** diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index e94a66e1db..7670813d4d 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1,3 +1,4 @@ +import * as fs from "fs/promises" import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider" import { DecorationController } from "../DecorationController" import type { Task } from "../../../core/task/Task" @@ -18,6 +19,10 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + // revertChanges() rolls a new-file edit back by deleting the placeholder and the + // directories the edit created. + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -54,7 +59,7 @@ vi.mock("vscode", () => ({ onDidChangeTextEditorVisibleRanges: vi.fn(() => ({ dispose: vi.fn() })), tabGroups: { all: [], - close: vi.fn(), + close: vi.fn().mockResolvedValue(true), activeTabGroup: { activeTab: undefined }, }, visibleTextEditors: [], @@ -1086,7 +1091,16 @@ describe("DiffViewProvider", () => { describe("userTouchedDocument close/keep behavior", () => { const mockTargetPath = `${mockCwd}/mock-target-file.ts` - const buildActiveDiffEditor = () => + // vscode types `TextDocument.isDirty` as read-only, but the doubles in this block are plain + // objects that a test has to dirty on purpose before `saveChanges()` runs. Widening the + // document view of this one builder keeps every `editor.document.isDirty = ...` below a + // plain assignment: the mutable structural type is what `makeTextDocument` actually hands + // back at runtime, so no assertion and no `any` is needed. + type ActiveDiffEditor = Omit & { + document: Omit & { isDirty: boolean } + } + + const buildActiveDiffEditor = (): ActiveDiffEditor => makeTextEditor({ document: makeTextDocument({ uri: makeUri(mockTargetPath), @@ -1130,6 +1144,731 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) + it("revertChanges() removes the placeholder and created dirs when open() failed before the editor existed", async () => { + // open() creates the parent dirs and an empty placeholder BEFORE it awaits + // openDiffEditor(). If that await rejects there is no activeDiffEditor, and the + // rollback used to bail out - leaving an empty file that the next execute() mistook + // for an existing file, which a denial then preserved. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + // open() claimed the placeholder before its editor open rejected. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + }) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).toHaveBeenCalledWith(`${mockCwd}/mock-target-file.ts`) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() tolerates a placeholder that was never written", async () => { + // The failed open may have died before fs.writeFile ran; ENOENT during the + // rollback is success, not a new failure that would abort the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + // Ownership was claimed (open()'s write succeeded); the file vanished externally + // before the rollback ran. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + }) + 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("open() does not claim placeholder ownership when the placeholder write fails", async () => { + // The failed open may have died inside fs.writeFile(); placeholderPath is set only + // after the write succeeds, so a failed write must not mark a file this edit never + // created as owned, and the rollback must still remove the directories it did + // create without unlinking the unowned path. + const { createDirectoriesForFile } = await import("../../../utils/fs") + vi.mocked(createDirectoriesForFile).mockResolvedValueOnce([`${mockCwd}/new-parent`]) + vi.mocked(fs.writeFile).mockRejectedValueOnce(new Error("EACCES: permission denied")) + + await expect(diffViewProvider.open("mock-target-file.ts")).rejects.toThrow("EACCES: permission denied") + + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + expect(diffViewProvider["createdDirs"]).toEqual([`${mockCwd}/new-parent`]) + + await diffViewProvider.revertChanges() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/new-parent`) + }) + + it("discardOwnedPlaceholder() unlinks the owned placeholder and releases ownership", async () => { + // The approved-but-uncommitted state: the file on disk is still the empty + // placeholder this edit created, so removing it is safe and releasing ownership + // after the removal keeps reset() from stranding it unowned. + diffViewProvider["placeholderPath"] = mockTargetPath + + await diffViewProvider.discardOwnedPlaceholder() + + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + // The public getter, not only the field it reads: nothing else in this spec asserts the released + // state, so a getter that always reported true would pass the suite. + expect(diffViewProvider.ownsPlaceholder).toBe(false) + }) + + it("discardOwnedPlaceholder() keeps ownership when the removal fails", async () => { + // An EPERM unlink must not release ownership: the placeholder is still on disk, + // and the caller needs the intact state to retry or to report the hazard. + diffViewProvider["placeholderPath"] = mockTargetPath + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }), + ) + + await expect(diffViewProvider.discardOwnedPlaceholder()).rejects.toThrow("EPERM: operation not permitted") + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + }) + + it("discardOwnedPlaceholder() is a no-op without ownership and tolerates a vanished file", async () => { + // An edit that never created a file must not unlink anything, and a placeholder + // that vanished externally is still a released ownership, not a new failure. + await diffViewProvider.discardOwnedPlaceholder() + expect(fs.unlink).not.toHaveBeenCalled() + + diffViewProvider["placeholderPath"] = mockTargetPath + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.discardOwnedPlaceholder()).resolves.toBeUndefined() + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + }) + + it("revertChanges() stops before the document work for an existing file when no editor exists", async () => { + // The modify branch used to dereference activeDiffEditor unconditionally. When open() + // never produced an editor there is no document to restore, and the old code threw a + // TypeError out of the denial path instead of finishing the teardown. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "modify", + createdDirs: [], + }) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // Only the document work is skipped: the shared teardown still runs, so no + // stale session survives the rollback. + expect(diffViewProvider["relPath"]).toBeUndefined() + expect(diffViewProvider["isEditing"]).toBe(false) + expect(diffViewProvider["editType"]).toBeUndefined() + }) + + it("revertChanges() keeps rolling back the remaining directories when one rmdir hits ENOENT", async () => { + // Same tolerance as the placeholder, for the created dirs: a directory that never + // landed must not abort the rest of the rollback and must not surface as a new failure. + const createdDirs = [`${mockCwd}/new-parent`, `${mockCwd}/new-parent/nested`] + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs, + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(diffViewProvider.revertChanges()).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(2) + expect(fs.rmdir).toHaveBeenNthCalledWith(1, createdDirs[1]) + expect(fs.rmdir).toHaveBeenNthCalledWith(2, createdDirs[0]) + }) + + it("revertChanges() surfaces a non-ENOENT directory failure instead of swallowing it", async () => { + // The tolerance is scoped to ENOENT: a directory that exists but could not be removed + // is a real rollback failure and must reach the caller rather than be hidden. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") + }) + + it("revertChanges() keeps placeholder ownership when the unlink fails", async () => { + // A rollback that cannot remove the placeholder must leave it OWNED: the caller + // skips reset() on that signal and keeps a retry path, instead of the empty file + // being stranded unowned for the next attempt to mistake for an existing file. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }), + ) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EPERM: operation not permitted") + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + expect(diffViewProvider.ownsPlaceholder).toBe(true) + }) + + it("revertChanges() releases placeholder ownership once the unlink succeeded even if a directory removal fails", async () => { + // Ownership tracks the file, not the whole rollback: after the unlink lands, the + // ownership is released before the directory loop, so a later EBUSY cannot strand + // an ownership whose file no longer exists. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/new-parent`], + placeholderPath: mockTargetPath, + }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EBUSY") + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + }) + + it("discardUnapprovedStream() keeps placeholder ownership when the unlink fails", async () => { + // Same discipline as revertChanges(): the abandoned-stream cleanup runs even with + // no editor, and a failed unlink must leave the placeholder owned so the caller + // can skip reset() and keep a retry path instead of stranding it unowned. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }), + ) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("EPERM: operation not permitted") + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + }) + + it("revertChanges() after reset() does not unlink a file this edit never created", async () => { + // reset() used to leave relPath behind with editType cleared, which is exactly the state + // that sends the NEXT rollback down the new-file branch: fs.unlink on whatever file this + // provider last touched. A truncated tool call or an EACCES in a later write then deleted + // a file the user had already saved. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + + await diffViewProvider.reset() + await diffViewProvider.revertChanges() + + expect(diffViewProvider["relPath"]).toBeUndefined() + expect(fs.unlink).not.toHaveBeenCalled() + }) + it("revertChanges() after a FAILED reset does not delete the previously approved file", async () => { + // An approved new-file write landed and reset() then rejected on the tab close. + // Without the finally, relPath/editType/isEditing and the placeholder ownership + // survived the failure: the next execution skipped open(), reused the stale create + // session, and a later denial's rollback unlinked the file the user had approved. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + isEditing: true, + createdDirs: [], + placeholderPath: mockTargetPath, + }) + diffViewProvider["closeAllDiffViews"] = vi.fn().mockRejectedValue(new Error("close failed")) + + await expect(diffViewProvider.reset()).rejects.toThrow("close failed") + + // The failed reset still released the session and the placeholder ownership. + expect(diffViewProvider["relPath"]).toBeUndefined() + expect(diffViewProvider["editType"]).toBeUndefined() + expect(diffViewProvider["isEditing"]).toBe(false) + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + + // The next edit's denial rolls back owning no placeholder: the previously approved + // file must survive even though a relPath is set again. The tab close succeeds this + // time - only the rollback's delete authority is under test. + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + Object.assign(diffViewProvider, { + relPath: "follow-up-denied.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + }) + await diffViewProvider.revertChanges() + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("saveChanges() signals its commit point at the document save, not on return", async () => { + // Everything after the save - closing the diff views, tab bookkeeping, diagnostics - can + // still reject. A caller that rolls the tool call back on that rejection would undo a + // write that already landed, so it has to be told at the save rather than when the + // method returns. + const order: string[] = [] + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn(async () => { + order.push("save") + return true + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + placeholderPath: mockTargetPath, + closeFileTab: vi.fn().mockResolvedValue(undefined), + restorePreviewTabs: vi.fn().mockResolvedValue(undefined), + }) + diffViewProvider["closeAllDiffViews"] = vi.fn(async () => { + order.push("closeDiffViews") + }) + + await diffViewProvider.saveChanges(false, 0, () => { + order.push("commit") + }) + + expect(order).toEqual(["save", "commit", "closeDiffViews"]) + // Ownership follows the same line: the placeholder open() created is only released once + // the approved content is on disk. + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + }) + + it("saveChanges() withholds the commit point and the placeholder when the save rejects", async () => { + // A rejected save means nothing landed. Signalling the commit point anyway would tell + // the caller the rollback has to stand down, and clearing placeholderPath would tell + // discardUnapprovedStream() that nothing this edit created is left to remove - the empty + // or half-written new file would survive the failed write. + const order: string[] = [] + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn(async () => { + order.push("save") + throw new Error("save rejected") + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + placeholderPath: mockTargetPath, + closeFileTab: vi.fn().mockResolvedValue(undefined), + }) + + await expect(diffViewProvider.saveChanges(false, 0, () => order.push("commit"))).rejects.toThrow( + "save rejected", + ) + + expect(order).toEqual(["save"]) + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + }) + + it("saveChanges() does not commit when the save resolves false", async () => { + // save() can resolve false without throwing: the write did not persist. The commit + // point was never reached, so onCommit must not run and the placeholder must stay + // owned - reporting the write as durable would strand an unpersisted file whose + // cleanup ownership had already been released. + const order: string[] = [] + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn(async () => { + order.push("save") + return false + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + placeholderPath: mockTargetPath, + }) + + await expect(diffViewProvider.saveChanges(false, 0, () => order.push("commit"))).rejects.toThrow( + "was not confirmed; the write was not committed", + ) + + expect(order).toEqual(["save"]) + expect(diffViewProvider["placeholderPath"]).toBe(mockTargetPath) + }) + + it("saveChanges() commits an already-clean document and releases placeholder ownership", async () => { + // A clean document already matches disk and never saves, but it crosses the same + // commit point: the callback must run and the placeholder ownership must be + // released even though no save happened. + const editor = buildActiveDiffEditor() + editor.document.isDirty = false + const onCommit = vi.fn() + diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined) + diffViewProvider["closeFileTab"] = vi.fn().mockResolvedValue(undefined) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + newContent: "content", + activeDiffEditor: editor, + placeholderPath: mockTargetPath, + documentWasOpen: false, + userTouchedDocument: false, + preEditScrollLine: undefined, + editType: "modify", + originalContent: "original", + }) + + await diffViewProvider.saveChanges(false, 0, onCommit) + + expect(editor.document.save).not.toHaveBeenCalled() + expect(onCommit).toHaveBeenCalledTimes(1) + expect(diffViewProvider["placeholderPath"]).toBeUndefined() + }) + + it("revertChanges() surfaces a placeholder delete that failed for a non-ENOENT reason", async () => { + // The ENOENT tolerance is scoped: an unlink that fails for another reason means the + // unapproved placeholder is still on disk, and the caller must not be told the + // rollback succeeded. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }), + ) + + await expect(diffViewProvider.revertChanges()).rejects.toThrow("EACCES: permission denied") + }) + + it("revertChanges() restores the streamed buffer to its pre-stream content before closing and unlinking a new file", async () => { + // The diff buffer holds the streamed content of a write that was never approved. + // Saving it as-is persisted exactly what this rollback is undoing - local history, file + // watchers, and, if the delete below fails, the content itself. The rollback restores the + // pre-stream content first and only then saves, so the only content ever written is the + // empty pre-stream buffer. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + editor.document.save = vi.fn().mockResolvedValue(true) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const dirtyTab = { + input: Object.assign(new vscode.TabInputText(makeUri(mockTargetPath)), { + uri: makeUri(mockTargetPath), + }), + isDirty: true, + label: "mock-target-file.ts", + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + placeholderPath: mockTargetPath, + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + expect(editor.document.save).toHaveBeenCalledTimes(1) + const [, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedText).toBe("") + // No force argument: tabGroups.close()'s second parameter is preserveFocus, not a + // discard flag, so passing true never discarded anything - it only suppressed the + // prompt while the dirty tab was silently refused. + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() cleans and closes the dirty vscode.diff tab before unlinking the new file", async () => { + // The denial-path hazard named on the pre-merge check: openDiffEditor() opens a + // vscode.diff tab whose input is a TabInputTextDiff, closeAllDiffViews() skips that tab + // while its modified side is dirty, and unlinking the placeholder underneath the open + // dirty tab left unapproved content one save away from recreating the deleted file. + // The provider's own tab helpers are deliberately NOT stubbed: the tab cleanup order + // through the real helpers is what is under test. + const callOrder: string[] = [] + const document = { + isDirty: true, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: makeUri(mockTargetPath), + save: vi.fn(async () => { + callOrder.push("save") + document.isDirty = false + return true + }), + } + const diffTab = { + label: `mock-target-file.ts ${DIFF_VIEW_LABEL_CHANGES}`, + input: Object.assign(new vscode.TabInputTextDiff(makeUri("diff-original"), makeUri(mockTargetPath)), { + original: { scheme: DIFF_VIEW_URI_SCHEME, fsPath: "mock-target-file.ts" }, + modified: { scheme: "file", fsPath: mockTargetPath }, + }), + // The tab is dirty exactly while the modified-side buffer is: the rollback has to + // make the buffer clean for the tab to become closeable at all. + get isDirty() { + return document.isDirty + }, + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [diffTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs: [], + originalContent: "", + placeholderPath: mockTargetPath, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + vi.mocked(vscode.window.tabGroups.close).mockImplementation(async () => { + callOrder.push("closeTab") + return true + }) + vi.mocked(fs.unlink).mockImplementation(async () => { + callOrder.push("unlink") + return undefined + }) + + try { + await diffViewProvider.revertChanges() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + // The buffer is put back to its pre-stream (empty) content and saved clean before + // anything closes, and the tab is gone before the file is deleted: no save can + // recreate the abandoned file from a buffer the user never approved. + expect(callOrder.slice(0, 2)).toEqual(["applyEdit", "save"]) + const [, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedText).toBe("") + expect(callOrder.indexOf("closeTab")).toBeGreaterThan(-1) + expect(callOrder.indexOf("closeTab")).toBeLessThan(callOrder.indexOf("unlink")) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("revertChanges() reports a discard the editor refused instead of leaving the buffer open", async () => { + // A forced close can still fail (a vetoing editor). Swallowing that left the unapproved + // streamed content in an open, saveable buffer while the rollback kept deleting the + // file underneath it. The failure must propagate, and the buffer must be put back to + // its pre-stream state so nothing saveable survives. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + // The diff-side restore/save confirms; the file-scheme buffer below is the one + // whose close gets vetoed. + editor.document.save = vi.fn().mockResolvedValue(true) + const streamedBuffer = { + uri: makeUri(mockTargetPath), + getText: vi.fn().mockReturnValue("streamed content"), + isDirty: true, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + save: vi.fn().mockResolvedValue(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 originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + value: [streamedBuffer], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + vi.mocked(vscode.window.tabGroups.close).mockRejectedValueOnce(new Error("close vetoed")) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow("could not close the restored buffer") + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocuments) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + } + } + + // The buffer went back to its pre-stream state and was saved clean BEFORE the close, + // so the close never saw a dirty tab; the rollback then stopped rather than deleting the + // file out from under a buffer it could not discard. + expect(vscode.workspace.applyEdit).toHaveBeenCalled() + expect(streamedBuffer.save).toHaveBeenCalledTimes(1) + expect(vscode.window.tabGroups.close).toHaveBeenCalledWith(dirtyTab) + // The buffer was put back to its pre-stream state (empty for a new file): the edit that + // was applied replaces the streamed text with originalContent. + const appliedEdit = vi.mocked(vscode.WorkspaceEdit).mock.results[0].value + expect(appliedEdit.replace).toHaveBeenCalledWith( + expect.objectContaining({ fsPath: mockTargetPath }), + expect.anything(), + "", + ) + }) + + it("revertChanges() stops the rollback without closing when the buffer restore is refused", async () => { + // applyEdit resolving false leaves the streamed content in the buffer. Saving it would + // persist exactly what this rollback exists to discard, and closing the dirty tab would + // trade the rollback report for a save-confirmation prompt over unapproved content, so + // the rollback stops and reports instead. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + const streamedBuffer = { + uri: makeUri(mockTargetPath), + getText: vi.fn().mockReturnValue("streamed content"), + isDirty: true, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + save: vi.fn().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") + const originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + value: [streamedBuffer], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValueOnce(false) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow(/could not restore the buffer/i) + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocuments) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + } + } + + // The restore was attempted and refused; the save that would have written the unapproved + // streamed content to disk never ran, and the dirty tab was NOT closed - closing it + // would trade a safe rollback report for a save-confirmation prompt over unapproved + // content. The rollback stops here and reports. + expect(vscode.workspace.applyEdit).toHaveBeenCalled() + expect(streamedBuffer.save).not.toHaveBeenCalled() + expect(vscode.window.tabGroups.close).not.toHaveBeenCalled() + }) + + it("revertChanges() stops the rollback without closing when the clean save is not confirmed", async () => { + // document.save() resolving false leaves the buffer's clean state unconfirmed. Closing + // then would prompt the user over a possibly-dirty tab, and answering yes can write + // content the rollback never confirmed clean, so the close is skipped and the failure + // is reported. + const editor = buildActiveDiffEditor() + editor.document.isDirty = true + // The diff-side restore/save confirms; the file-scheme buffer below is the one whose + // save stays unconfirmed. + editor.document.save = vi.fn().mockResolvedValue(true) + const streamedBuffer = { + uri: makeUri(mockTargetPath), + getText: vi.fn().mockReturnValue("streamed content"), + isDirty: true, + positionAt: vi.fn().mockReturnValue({ line: 0, character: 0 }), + 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 originalDocuments = Object.getOwnPropertyDescriptor(vscode.workspace, "textDocuments") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [dirtyTab] }], + configurable: true, + }) + Object.defineProperty(vscode.workspace, "textDocuments", { + value: [streamedBuffer], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: editor, + editType: "create", + createdDirs: [], + originalContent: "", + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + try { + await expect(diffViewProvider.revertChanges()).rejects.toThrow(/could not save the restored buffer/i) + } finally { + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + if (originalDocuments) { + Object.defineProperty(vscode.workspace, "textDocuments", originalDocuments) + } + } + + // The restore applied and the save was attempted, but the unconfirmed save stops the + // rollback before the close. + expect(streamedBuffer.save).toHaveBeenCalledTimes(1) + expect(vscode.window.tabGroups.close).not.toHaveBeenCalled() + }) + 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) @@ -1776,4 +2515,631 @@ describe("DiffViewProvider", () => { expect(vscode.window.showTextDocument).toHaveBeenCalled() }) }) + describe("discardUnapprovedStream method", () => { + const mockTargetPath = `${mockCwd}/mock-target-file.ts` + + const makeAbandonedDocument = (callOrder: string[], isDirty = true) => ({ + isDirty, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: { fsPath: mockTargetPath, path: mockTargetPath }, + save: vi.fn(async () => { + callOrder.push("save") + // A confirmed save: the discard treats an unconfirmed one as a rollback failure. + return true + }), + }) + + const openAbandonedView = (document: unknown, createdDirs: string[], callOrder: string[]) => + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs, + // open() records the placeholder it wrote; the discard may only delete what it owns. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + closeAllDiffViews: vi.fn(async () => { + callOrder.push("closeDiffViews") + }), + closeFileTab: vi.fn(async () => { + callOrder.push("closeFileTab") + }), + }) + + it("never persists the unapproved buffer: blanks it with an empty replacement, then deletes the placeholder", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["applyEdit", "save", "closeDiffViews", "closeFileTab"]) + // The only content the discard may write is an empty buffer, and it must be + // written to THIS document before the save that would otherwise persist the + // partial model output. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + // The replacement covers the whole buffer: the range is built from + // positionAt(0) to positionAt(getText().length), so nothing can survive it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(replacedText).toBe("") + expect(mockWorkspaceEdit.delete).not.toHaveBeenCalled() + // The placeholder for THIS relPath is what gets removed. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + }) + + it("restores a modify buffer in memory and never saves the target file", async () => { + // revertChanges() restores a modify by applyEdit + document.save(). For a stream the + // user never approved that save is a write they never asked for, and for a + // .rooignore-denied path a write the policy forbids outright, so the discard restores + // the buffer and leaves the file on disk untouched. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + Object.assign(diffViewProvider, { + editType: "modify", + originalContent: "original content", + // A modify owns no placeholder: only open() of a create writes one. + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(callOrder).toEqual(["applyEdit", "closeDiffViews", "closeFileTab"]) + const [replacedUri, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedUri).toEqual(document.uri) + expect(replacedText).toBe("original content") + // The replacement covers the whole buffer, so no streamed content survives it. + expect(vi.mocked(vscode.Range)).toHaveBeenCalledWith( + { line: 0, character: 0 }, + { line: 0, character: "partial model output".length }, + ) + expect(document.save).not.toHaveBeenCalled() + // Nothing is unlinked for a modify: saveChanges() may have written this very file with + // the user's approval, and this edit created no directories. + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("stops the cleanup and keeps the placeholder when the buffer restore is refused", async () => { + // applyEdit resolving false leaves the unapproved partial content in the buffer. Saving then + // would persist exactly what this method exists to discard, and closing the tabs or unlinking + // the placeholder underneath that buffer would leave content the user never approved with + // nothing left that can still recover it - so the rollback stops before the close and the + // delete, the same way an unconfirmed save of the restored buffer does. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/new-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(false) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + /could not restore the diff editor buffer/i, + ) + + expect(document.save).not.toHaveBeenCalled() + // Nothing after the refused restore ran: no close and no unlink. + expect(callOrder).toEqual([]) + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // Ownership and the directory snapshot survive the refused restore. The caller skips reset() + // while the placeholder is still owned, so the retry keeps a session and a discard that still + // knows what it may delete. + expect(diffViewProvider["placeholderPath"] as string | undefined).toBe(mockTargetPath) + expect(diffViewProvider["createdDirs"]).toEqual([`${mockCwd}/new-dir`]) + }) + + it("stops the cleanup and keeps placeholder ownership when the save is not confirmed", async () => { + // document.save() resolving false leaves the emptied buffer dirty over the + // placeholder: closeAllDiffViews() would skip the dirty tab and the unlink would + // delete the file underneath it, leaving the buffer one save away from recreating + // the file from a denied write. The discard stops before the close and the delete, + // keeping ownership so the caller skips reset() and the session stays retryable. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + document.save = vi.fn(async () => { + callOrder.push("save") + return false + }) + const closeAllDiffViews = vi.fn(async () => { + callOrder.push("closeDiffViews") + }) + const closeFileTab = vi.fn(async () => { + callOrder.push("closeFileTab") + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs: [`${mockCwd}/new-dir`], + placeholderPath: `${mockCwd}/mock-target-file.ts`, + closeAllDiffViews, + closeFileTab, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow( + /could not save the restored buffer/i, + ) + + // The restore was attempted and the save refused; nothing closes and nothing deletes. + expect(callOrder).toEqual(["applyEdit", "save"]) + expect(closeAllDiffViews).not.toHaveBeenCalled() + expect(closeFileTab).not.toHaveBeenCalled() + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // Ownership is preserved: the placeholder is still on disk and still ours. + expect(diffViewProvider["placeholderPath"] as string | undefined).toBe(`${mockCwd}/mock-target-file.ts`) + }) + + it("removes the directories this edit created in reverse order, after the placeholder unlink", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`, `${mockCwd}/mock-dir/nested`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledTimes(createdDirs.length) + // Reverse order, so a parent is never removed before the child inside it, and + // only the directories this edit created - never a pre-existing one. + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => c[0])).toEqual([...createdDirs].reverse()) + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + for (const order of vi.mocked(fs.rmdir).mock.invocationCallOrder) { + expect(order).toBeGreaterThan(unlinkOrder) + } + }) + + it("does not save when the abandoned buffer is already clean", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder, false) + openAbandonedView(document, [], callOrder) + + await diffViewProvider.discardUnapprovedStream() + + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + expect(document.save).not.toHaveBeenCalled() + expect(mockWorkspaceEdit.replace).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledTimes(1) + }) + + it("keeps the placeholder and the directories when the buffer restore rejects", async () => { + // A rejected applyEdit leaves the restore unconfirmed, exactly like one that resolves false: the + // buffer still holds the unapproved content and its tab is still open, so unlinking the placeholder + // underneath it would leave that buffer one save away from recreating the file with content nobody + // approved. The artifacts stay owned and the directory snapshot goes back to the session, which is + // what tells the caller to skip reset() and keep the retry path open. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockRejectedValue(new Error("applyEdit rejected")) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("applyEdit rejected") + + expect(callOrder).toEqual([]) + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + expect(diffViewProvider["placeholderPath"] as string | undefined).toBe(mockTargetPath) + expect(diffViewProvider["createdDirs"]).toEqual(createdDirs) + }) + + it("keeps the placeholder and the directories when the restored buffer save rejects", async () => { + // A rejected save is as unconfirmed as one resolving false: the buffer is still dirty over the + // placeholder this edit created, so the sweep must not delete the file underneath it. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const createdDirs = [`${mockCwd}/mock-dir`] + openAbandonedView(document, createdDirs, callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + document.save.mockRejectedValueOnce(new Error("save rejected")) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("save rejected") + + // The mock rejects without running its recording body, so the call is counted, not ordered. + expect(document.save).toHaveBeenCalledTimes(1) + expect(callOrder).toEqual([]) + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + expect(diffViewProvider["placeholderPath"] as string | undefined).toBe(mockTargetPath) + expect(diffViewProvider["createdDirs"]).toEqual(createdDirs) + }) + + it("sweeps the artifacts when only the tab closes reject on a confirmed buffer", async () => { + // The half of the contract the two tests above must not swallow: once the buffer outcome is + // confirmed, a close failure still has to leave the artifacts gone, because the caller resets the + // session after a throw and nothing else can remove them. The failure still surfaces. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + closeAllDiffViews: vi.fn().mockRejectedValue(new Error("close rejected")), + }) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("close rejected") + + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + expect(diffViewProvider["placeholderPath"] as string | undefined).toBeUndefined() + }) + + it("removes the placeholder and created directories even when open() failed before activeDiffEditor was assigned", async () => { + // open() creates the directories (line 130) and the empty placeholder (line 134) + // BEFORE openDiffEditor() assigns activeDiffEditor (line 173). If that call rejects + // - the 10s timeout or a failed vscode.diff - an abandoned create therefore has + // artifacts on disk and no editor, and the early return must not skip the cleanup. + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: undefined, + editType: "create", + createdDirs: [`${mockCwd}/mock-dir`], + // open() had already written the placeholder before openDiffEditor() rejected, + // so this edit owns the path. + placeholderPath: `${mockCwd}/mock-target-file.ts`, + }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("mock-target-file.ts")) + expect(fs.rmdir).toHaveBeenCalledWith(`${mockCwd}/mock-dir`) + // Nothing editor-side ran, and nothing threw: the artifacts are the whole job here. + expect(vscode.workspace.applyEdit).not.toHaveBeenCalled() + // Bracket access for the private field: the snapshot must be consumed, so a second + // discard cannot try to remove the same directories again. + expect(diffViewProvider["createdDirs"]).toEqual([]) + }) + + it("logs and rejects when the placeholder unlink fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const permissionError = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + vi.mocked(fs.unlink).mockRejectedValueOnce(permissionError) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("EPERM: operation not permitted") + + expect(errorSpy).toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", permissionError) + }) + + it("hands the created directories back when the unlink fails, so the retained session can finish the sweep", async () => { + // The unlink is the first step of the sweep, so an EPERM there leaves the placeholder owned - + // which is what keeps the session - and the directories this edit created still on disk. + // If the snapshot is not handed back the retained session forgets them and the retry can + // only remove the file. Two directories, so the retry has to show the reverse order again. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + const dirA = `${mockCwd}/mock-dir-a` + const dirB = `${mockCwd}/nested/mock-dir-b` + openAbandonedView(document, [dirA, dirB], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + vi.mocked(fs.unlink).mockRejectedValueOnce( + Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }), + ) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("EPERM: operation not permitted") + + // Owned, on disk and still tracked: that trio is what makes a retry able to finish the job. + expect(diffViewProvider["placeholderPath"] as string | undefined).toBe(mockTargetPath) + expect(diffViewProvider["createdDirs"]).toEqual([dirA, dirB]) + expect(fs.rmdir).not.toHaveBeenCalled() + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledTimes(2) + expect(diffViewProvider["placeholderPath"] as string | undefined).toBeUndefined() + expect(diffViewProvider["createdDirs"]).toEqual([]) + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => c[0])).toEqual([dirB, dirA]) + expect(errorSpy).toHaveBeenCalledTimes(1) + errorSpy.mockRestore() + }) + + it("logs and rejects when removing a created directory fails for a reason other than ENOENT", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + const notEmpty = Object.assign(new Error("ENOTEMPTY: directory not empty"), { code: "ENOTEMPTY" }) + vi.mocked(fs.rmdir).mockRejectedValueOnce(notEmpty) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("ENOTEMPTY: directory not empty") + + expect(errorSpy).toHaveBeenCalledWith("Error removing abandoned write_to_file artifacts:", notEmpty) + }) + + it("tolerates an already-deleted placeholder and directories (ENOENT) without throwing", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + // Two created directories: the tolerance has to hold for every one of them, so the + // rejection is keyed on the path rather than on which call comes first. + const dirA = `${mockCwd}/mock-dir-a` + const dirB = `${mockCwd}/nested/mock-dir-b` + openAbandonedView(document, [dirA, dirB], callOrder) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + const enoent = Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + // A single unlink call in this flow, so the placeholder stays Once-based. + vi.mocked(fs.unlink).mockRejectedValueOnce(enoent) + vi.mocked(fs.rmdir).mockImplementation(async (dir) => { + if (dir === dirA || dir === dirB) { + throw enoent + } + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + // Both directories were attempted; the assertion is on the paths, not the order. + expect(fs.rmdir).toHaveBeenCalledWith(dirA) + expect(fs.rmdir).toHaveBeenCalledWith(dirB) + expect(errorSpy).not.toHaveBeenCalledWith( + "Error removing abandoned write_to_file artifacts:", + expect.anything(), + ) + + // beforeEach only clears call data, not implementations, so put the default back. + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + }) + + it("reports the editor failure rather than the cleanup failure when both happen", async () => { + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + openAbandonedView(document, [`${mockCwd}/mock-dir`], callOrder) + // The buffer outcome has to be confirmed for the sweep to run at all, which is what makes both + // failures possible in the same call: applyEdit accepted the restore, a tab close then rejects and + // the placeholder unlink that follows it rejects too. + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + Object.assign(diffViewProvider, { + closeAllDiffViews: vi.fn().mockRejectedValue(new Error("close rejected")), + }) + vi.mocked(fs.unlink).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // The caller reports a rollback hazard from the thrown error, so the ORIGINAL failure is what must + // surface; the cleanup failure is still logged for the operator. + await expect(diffViewProvider.discardUnapprovedStream()).rejects.toThrow("close rejected") + expect(errorSpy).toHaveBeenCalledWith( + "Error removing abandoned write_to_file artifacts:", + expect.objectContaining({ code: "EPERM" }), + ) + }) + + it("leaves a file this edit never created a placeholder for on disk", async () => { + // reset() does not clear relPath, and saveDirectly() sets it for an approved write. + // Without ownership tracking the discard would unlink that approved file: an approved + // write_to_file to A, then a new write_to_file whose block exits through a rollback + // path before open() ever ran, still sees relPath === A. + const callOrder: string[] = [] + const document = makeAbandonedDocument(callOrder) + Object.assign(diffViewProvider, { + relPath: "previously-approved-file.ts", + activeDiffEditor: { document }, + editType: undefined, + createdDirs: [], + placeholderPath: undefined, + }) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await expect(diffViewProvider.discardUnapprovedStream()).resolves.toBeUndefined() + + expect(fs.unlink).not.toHaveBeenCalled() + expect(fs.rmdir).not.toHaveBeenCalled() + // The buffer is still blanked and the tab closed: the unapproved content is gone + // without touching a file the user approved. + expect(mockWorkspaceEdit.replace).toHaveBeenCalledTimes(1) + expect(mockWorkspaceEdit.replace.mock.calls[0][2]).toBe("") + }) + + it("removes the placeholder that a new-file open() wrote, driven through the public methods", async () => { + // The ownership lifecycle lives at the public-method layer, so exercise it there: + // open() writes the empty placeholder and records it, and the discard of an abandoned + // create removes exactly that path. + const relPath = "owned-placeholder.ts" + const fsPath = `${mockCwd}/${relPath}` + const mockEditor = { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue(""), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + lineCount: 0, + }, + selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } }, + edit: vi.fn().mockResolvedValue(true), + revealRange: vi.fn(), + } + // Structural double for the mocked editor: the mock only implements the members + // open() touches, so it is routed through unknown rather than any. + const editor = mockEditor as unknown as vscode.TextEditor + vi.mocked(vscode.window).visibleTextEditors = [editor] + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(editor) + vi.mocked(vscode.workspace.onDidOpenTextDocument).mockImplementation((callback) => { + setTimeout(() => callback({ uri: { fsPath, scheme: "file" } } as vscode.TextDocument), 0) + return { dispose: vi.fn() } + }) + vi.mocked(vscode.window.onDidChangeVisibleTextEditors).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.window.onDidChangeTextEditorVisibleRanges).mockReturnValue({ dispose: vi.fn() }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + diffViewProvider.editType = "create" + + await diffViewProvider.open(relPath) + + // open() created the placeholder, and this edit now owns it. + expect(fs.writeFile).toHaveBeenCalledWith(fsPath, "") + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).toHaveBeenCalledWith(fsPath) + }) + + it("does not remove the file once saveChanges() has approved the content", async () => { + // saveChanges() turns the placeholder into approved content and drops this edit's + // claim on it, so a later abandoned cleanup must leave the approved file on disk. + const relPath = "approved-by-save.ts" + const fsPath = `${mockCwd}/${relPath}` + Object.assign(diffViewProvider, { + relPath, + newContent: "approved content", + editType: "create", + createdDirs: [], + placeholderPath: fsPath, + preDiagnostics: [], + activeDiffEditor: { + document: { + uri: { fsPath, scheme: "file" }, + getText: vi.fn().mockReturnValue("approved content"), + isDirty: false, + save: vi.fn().mockResolvedValue(undefined), + }, + }, + closeAllDiffViews: vi.fn().mockResolvedValue(undefined), + closeFileTab: vi.fn().mockResolvedValue(undefined), + }) + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + vi.mocked(vscode.window.showTextDocument).mockResolvedValue(undefined as never) + vi.mocked(vscode.workspace.applyEdit).mockResolvedValue(true) + + await diffViewProvider.saveChanges(false, 0) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("closes the dirty vscode.diff tab that only TabInputTextDiff matches, before the unlink", async () => { + // The hazard named on #1932: openDiffEditor() opens a vscode.diff tab whose input is a + // TabInputTextDiff, so a rollback helper that looks for TabInputText tabs finds nothing, + // closeAllDiffViews() then skips the tab while it is dirty, and the streamed content + // survives in a buffer one save away from recreating a file the user never approved. + // The provider's own tab helpers are deliberately NOT stubbed here: the tab-input matching + // is what is under test. + const callOrder: string[] = [] + const document = { + isDirty: true, + getText: () => "partial model output", + positionAt: (offset: number) => ({ line: 0, character: offset }), + uri: makeUri(mockTargetPath), + save: vi.fn(async () => { + callOrder.push("save") + document.isDirty = false + return true + }), + } + const diffTab = { + label: `mock-target-file.ts ${DIFF_VIEW_LABEL_CHANGES}`, + input: Object.assign(new vscode.TabInputTextDiff(makeUri("diff-original"), makeUri(mockTargetPath)), { + original: { scheme: DIFF_VIEW_URI_SCHEME, fsPath: "mock-target-file.ts" }, + modified: { scheme: "file", fsPath: mockTargetPath }, + }), + // The tab is dirty exactly while the modified-side buffer is: the discard has to make + // the buffer clean for the tab to become closeable at all. + get isDirty() { + return document.isDirty + }, + } + const originalTabs = Object.getOwnPropertyDescriptor(vscode.window.tabGroups, "all") + Object.defineProperty(vscode.window.tabGroups, "all", { + get: () => [{ tabs: [diffTab] }], + configurable: true, + }) + Object.assign(diffViewProvider, { + relPath: "mock-target-file.ts", + activeDiffEditor: { document }, + editType: "create", + createdDirs: [], + placeholderPath: mockTargetPath, + }) + vi.mocked(vscode.workspace.applyEdit).mockImplementation(async () => { + callOrder.push("applyEdit") + return true + }) + vi.mocked(vscode.window.tabGroups.close).mockImplementation(async () => { + callOrder.push("closeTab") + return true + }) + vi.mocked(fs.unlink).mockImplementation(async () => { + callOrder.push("unlink") + return undefined + }) + + try { + await diffViewProvider.discardUnapprovedStream() + } finally { + // Do not leak the tab fixture: later tests read the module-level tabGroups.all. + if (originalTabs) { + Object.defineProperty(vscode.window.tabGroups, "all", originalTabs) + } + } + + // The buffer is emptied before anything is closed, and the only content the discard + // writes is an empty buffer: a later save has nothing left to persist. + expect(callOrder.slice(0, 2)).toEqual(["applyEdit", "save"]) + const [, , replacedText] = mockWorkspaceEdit.replace.mock.calls[0] + expect(replacedText).toBe("") + // The tab is gone before the file is, so no save can recreate the abandoned file from a + // buffer the user never approved. + expect(callOrder.indexOf("closeTab")).toBeGreaterThan(-1) + expect(callOrder.indexOf("closeTab")).toBeLessThan(callOrder.indexOf("unlink")) + expect(fs.unlink).toHaveBeenCalledWith(mockTargetPath) + }) + + it("does nothing when no abandoned view is open", async () => { + Object.assign(diffViewProvider, { relPath: undefined, activeDiffEditor: undefined }) + + await diffViewProvider.discardUnapprovedStream() + + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + describe("isEditingDifferentPath and editingRelPath", () => { + // The production predicate the streaming guard and the different-path rejection both call. + // Everywhere else a test double stands in for it, so only these assertions can fail when the + // predicate itself regresses - for example to always reporting "not a different path". + it("reports no different path while no edit session is active", () => { + Object.assign(diffViewProvider, { isEditing: false, relPath: "leftover/path.ts" }) + + expect(diffViewProvider.isEditingDifferentPath("any/path.ts")).toBe(false) + expect(diffViewProvider.editingRelPath).toBeUndefined() + }) + + it("accepts the retained path and rejects another one", () => { + Object.assign(diffViewProvider, { isEditing: true, relPath: "retained/path.ts" }) + + expect(diffViewProvider.isEditingDifferentPath("retained/path.ts")).toBe(false) + expect(diffViewProvider.isEditingDifferentPath("other/path.ts")).toBe(true) + expect(diffViewProvider.editingRelPath).toBe("retained/path.ts") + }) + + it("compares the paths exactly as the caller spelled them", () => { + // Deliberately no normalisation: the predicate is a plain inequality, so a caller that hands + // over a differently spelled form of the retained path is treated as a second write. Pinning + // it here is what stops a later normalisation from silently widening the guard with nobody + // deciding that. + Object.assign(diffViewProvider, { isEditing: true, relPath: "dir/path.ts" }) + + expect(diffViewProvider.isEditingDifferentPath("./dir/path.ts")).toBe(true) + expect(diffViewProvider.isEditingDifferentPath("dir/path.ts")).toBe(false) + }) + }) }) From 725b8a3eb42fe3bd8c71e23b56a4ac7729ce5450 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 12 Nov 2026 09:48:29 +0800 Subject: [PATCH 2/3] fix(write-to-file): tear down per-task stream state on parse failure and dispose (split 5/6 of #1066, unit U6b) Tool-side half of #1932, stacked on unit U6a (3c5036a39e8a): the onParameterParseFailure teardown seam on BaseTool and the rewired native-args path, the per-task partial stream state and its cleanup primitives on WriteToFileTool released on every execute() exit, and the dispose-time clearTaskState handoff in ClineProvider. BaseTool.ts and ClineProvider.ts arrive here rather than on this branch's cumulative diff because the split takes them out of #1932; plan and measured widths in https://github.com/Zoo-Code-Org/Zoo-Code/issues/1937#issuecomment-6811832404. writeToFileTool.spec.ts drops two no-explicit-any suppressions (5 -> 3) rather than renewing them. Review round (change-request answers): WriteToFileTool now carries one ownsPreview flag in the per-task stream state, set once the delta commits to the diff view and read at the parse-failure hook, at both missing-parameter returns and at the pre-streaming catch. A stream whose path never stabilized, or that was found to belong to another path's retained session, no longer tears down a diff session it never opened; the stream that did reach the view still owes it a rollback before its state is released. saveDirectly now takes the commit signal its own filesystem write carries and releases the placeholder at the point saveChanges() releases it, so the direct-write rollback stands down at the write instead of when the call returns. That provider hunk rides in this commit because the signal can only be fired inside saveDirectly. The tool specs pin both sides of the ownership rule: a never-stabilized stream leaves a retained session alone, a stream that reached the view still gets its preview discarded before the view is reset, and the commit-point test now has the placeholder it asserts on. --- .../removeClineFromStack-delegation.spec.ts | 51 + src/core/tools/BaseTool.ts | 43 +- src/core/tools/WriteToFileTool.ts | 704 ++++++- ...teToFileTool-partial-state-cleanup.spec.ts | 303 +++ .../tools/__tests__/writeToFileTool.spec.ts | 1646 ++++++++++++++++- src/core/webview/ClineProvider.ts | 6 + src/eslint-suppressions.json | 2 +- src/integrations/editor/DiffViewProvider.ts | 14 + 8 files changed, 2697 insertions(+), 72 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..e032f74dc4 100644 --- a/src/__tests__/removeClineFromStack-delegation.spec.ts +++ b/src/__tests__/removeClineFromStack-delegation.spec.ts @@ -1,10 +1,12 @@ // npx vitest run __tests__/removeClineFromStack-delegation.spec.ts import { describe, it, expect, vi } from "vitest" +import { RooCodeEventName } from "@roo-code/types" 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 +204,55 @@ 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[] = [] + // The listener identity matters: the release must detach THIS function, so the + // fixture binds it to a variable instead of an anonymous inline no-op. + const abortCleanup = vi.fn() + 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, + differentPathStream: false, + // The entry stands for a stream whose path never stabilized, so it never owned the view. + ownsPreview: 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) + // Dropping the map entry is not enough: the registered TaskAborted listener must + // be detached with the same function reference it was registered with. + expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + }) + it("keeps the task active after an unrelated history resume failure", async () => { const cleanupListener = vi.fn() const task = { diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 1e73a8285a..827011fba0 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..e429d1f35f 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -1,7 +1,7 @@ import path from "path" import fs from "fs/promises" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS, RooCodeEventName } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -22,9 +22,341 @@ 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 this stream was found to be editing a different path than the retained session. */ + differentPathStream: boolean + /** True once this stream has reached the diff view (open/update below), i.e. once it owns + * the preview and artifacts a teardown here would restore. */ + ownsPreview: boolean + /** 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}` + } + + /** + * Whether this task's stream is the one that reached the diff view. Only that stream may tear the + * view down: a stream whose path never stabilized, or that was found to belong to a different + * path than the retained session, never opened or updated the view, so discarding or resetting + * there would throw away a session it does not own - the same hazard the different-path + * rejection in execute() guards, one level earlier. + */ + private streamOwnsPreview(task: Task): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task))?.ownsPreview === true + } + + /** + * 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, + differentPathStream: false, + ownsPreview: false, + 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 { + const pathHasStabilized = state.lastSeenPartialPath !== undefined && state.lastSeenPartialPath === partialPath + state.lastSeenPartialPath = partialPath + return pathHasStabilized && !!partialPath + } + + /** + * Clear a task's partial-stream state from a disposal path that does not abort first. + * Task.dispose() removes every listener, so a task disposed directly (for example + * ClineProvider.cleanupFailedHistoryTask()) never fires the TaskAborted cleanup and + * this singleton would keep the disposed task and its diff-view provider. + */ + public clearTaskState(task: Task): void { + this.resetTaskPartialState(task) + } + + private resetTaskPartialState(task: Task): void { + const key = this.getPartialStreamFailureKey(task) + const state = this.taskPartialStreamState.get(key) + if (!state) { + return + } + state.task.off(RooCodeEventName.TaskAborted, state.abortCleanup) + this.taskPartialStreamState.delete(key) + } + + /** + * Whether this task's partial stream is still the live one. handlePartial() awaits provider + * state, a filesystem probe, task.ask() and the diff view's open() before it streams content; + * a cancellation during any of those awaits runs the TaskAborted teardown (or a direct + * clearTaskState), which deletes this entry. Continuing would re-ask and re-open a diff view + * for a task the user already cancelled, resurrecting the state the teardown released. + * Identity, not presence: an entry re-created under the same key belongs to a new stream, + * and this delta must not write into it. + */ + private isPartialStreamStillLive(task: Task, state: TaskPartialStreamState): boolean { + return this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task)) === state + } + + private async resetDiffViewAfterWrite(task: Task): Promise { + await task.diffViewProvider.reset().catch((resetError) => { + console.error("Error resetting write_to_file diff view:", resetError) + }) + } + + /** + * Release the diff view of a write that was never approved, then let the caller reset it. + * + * reset() clears the provider's state but leaves the diff document dirty with the + * streamed content; a user save would then persist a write the task never completed + * (denied or failed before approval). Must run BEFORE resetDiffViewAfterWrite(), + * since reset() clears the state the discard relies on. No-op when no diff view + * is open. Failures are logged and reported through the return value: the caller + * must not treat the teardown as complete when this returns false, because the + * document may still hold the unapproved content, but the remaining cleanup (reset, + * per-task state teardown) still runs so the task is not left half-torn-down. + * + * This is deliberately not revertChanges(): that method SAVES. For a new file it writes + * the streamed partial output into the placeholder before deleting it, and for an + * existing file it writes the restored original back to a file this stream never + * changed. discardUnapprovedStream() empties or restores the buffer in memory and + * closes the tab only once it is clean, so nothing the user did not approve reaches + * disk and the modified side of the vscode.diff tab - a TabInputTextDiff, which + * discardFileTab() cannot see - is what actually gets discarded. + */ + private async discardUnapprovedStreamBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.discardUnapprovedStream() + return true + } catch (discardError) { + console.error("Error discarding the unapproved write_to_file diff view:", discardError) + return false + } + } + + /** + * Cleanup for an approved write whose saveChanges() failed before its commit point: + * approval is not persistence - nothing landed, so the placeholder open() created is + * still the empty file on disk and still owned. Remove it before reset() forgets the + * ownership, or the empty file is stranded unowned: a later attempt would treat it as + * the pre-existing target and a later denial's rollback would no longer delete it. + * The dirty editor keeps the approved content for a manual save or retry. A removal + * that fails (EPERM, EACCES) must not be swallowed: ownership is preserved (the + * provider keeps placeholderPath when the unlink rejects) and the caller must skip + * reset() - clearing the session would strand the placeholder unowned with no retry + * path - and report the hazard instead. Returns whether the placeholder was removed. + */ + private async discardOwnedPlaceholderBeforeReset(task: Task): Promise { + try { + await task.diffViewProvider.discardOwnedPlaceholder() + return true + } catch (placeholderError) { + console.error("Error removing the uncommitted write_to_file placeholder:", placeholderError) + return false + } + } + + /** + * Surface a placeholder this edit still owns but could not remove. Like the rollback + * report, the hazard has to be visible in the chat: the empty file is still on disk, + * still owned, and the diff session was left open for a retry instead of being reset. + * A failing say must not abort the teardown, so its failure is logged only. + */ + private async reportPlaceholderStranded(task: Task): Promise { + await task + .say( + "error", + "write_to_file: the placeholder created for this new file could not be removed after the failed write, so this edit still owns it and the diff session was left open for a retry instead of being reset.", + ) + .catch((sayError) => { + console.error("Error reporting write_to_file stranded placeholder failure:", sayError) + }) + } + + 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.discardUnapprovedStreamBeforeReset(task) + // A failed rollback that still owns the placeholder must keep the session: + // reset() would forget that ownership and strand the empty file unowned, so it + // is skipped and the hazard reported instead - the same discipline execute() + // uses on its error path. + const strandedPlaceholder = !reverted && task.diffViewProvider.ownsPlaceholder + if (!strandedPlaceholder) { + await this.resetDiffViewAfterWrite(task) + } + if (!reverted) { + await this.reportRevertFailure(task) + } + if (strandedPlaceholder) { + await this.reportPlaceholderStranded(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 + } + if (state.differentPathStream || !state.ownsPreview) { + // This stream never owned the diff session: either it carries another path's retained, + // approved edit, or its own path never stabilized enough to reach the view. Discarding or + // resetting the view here would throw that session away, so only release this stream's + // own bookkeeping and report any streaming failure. + this.resetTaskPartialState(task) + if (!state.streamError) { + return false + } + void parseError + await callbacks.handleError("writing file", state.streamError) + return true + } + // 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.discardUnapprovedStreamBeforeReset(task) + this.resetTaskPartialState(task) + // The same ownership guard as execute(): a rollback that could not remove the + // placeholder leaves it owned, and reset() would strand it unowned. + const strandedPlaceholder = !reverted && task.diffViewProvider.ownsPlaceholder + if (!strandedPlaceholder) { + 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 (strandedPlaceholder) { + await this.reportPlaceholderStranded(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 @@ -34,7 +366,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) - await task.diffViewProvider.reset() + // Returning here skips the cleanup below: release THIS task's stream state (and only this + // task's) so the abort listener and any streamFailed guard do not outlive the call, and + // tear down the diff view an earlier streaming delta may have opened - otherwise its + // unapproved content sits in the editor until the next write. Only the stream that reached + // the view may do that teardown: a never-stabilized path never touched it, and a retained + // session may belong to another path's approved edit. + const ownsPreview = this.streamOwnsPreview(task) + this.resetTaskPartialState(task) + if (ownsPreview) { + await this.cleanupFailedPartialStream(task) + } return } @@ -42,7 +384,41 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) - await task.diffViewProvider.reset() + + // Returning here skips the cleanup below: release THIS task's stream state (and only this + // task's) so the abort listener and any streamFailed guard do not outlive the call. The + // teardown belongs to the stream that reached the diff view: a call whose path never + // stabilized never touched the view, and discarding or resetting then would throw away a + // retained session that belongs to another path. + const ownsPreview = this.streamOwnsPreview(task) + this.resetTaskPartialState(task) + if (ownsPreview) { + await this.cleanupFailedPartialStream(task) + } + return + } + + // A retained session belongs to ITS path: when a rollback could not complete, + // the provider keeps the dirty buffer and relPath so the user's content survives + // a retry. Writing a DIFFERENT path through that session would update and save + // the old document while asking approval for the new one, so the write is + // rejected before any cleanup path can reset the retained session. + if (task.diffViewProvider.isEditingDifferentPath(relPath)) { + task.consecutiveMistakeCount++ + task.recordToolError("write_to_file") + const retainedPath = task.diffViewProvider.editingRelPath ?? "another file" + // The retained session is only released by a retry of ITS path (which reuses the + // session and reaches reset()) - saving or closing the editor does not clear + // isEditing/relPath in this process, so that remedy would loop. + const retainedErrorMessage = + `Cannot write '${relPath}': an unfinished write to '${retainedPath}' must be resolved first. ` + + `Retry the write to '${retainedPath}' to reuse and finish that session, then retry this write.` + await task.say("error", retainedErrorMessage) + pushToolResult(formatResponse.toolError(retainedErrorMessage)) + // This return skips the try/finally below, so the per-task stream state a delta may + // have created for the rejected path must be released here: otherwise its TaskAborted + // listener leaks and a streamFailed flag would suppress every later write_to_file delta. + this.resetTaskPartialState(task) return } @@ -51,11 +427,26 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) + + // Returning here skips the cleanup below: release THIS task's stream state (and + // only this task's) so the abort listener and any streamFailed guard do not outlive + // the call. The denial can arrive after a streaming delta opened the denied path and + // streamed unapproved content into it, so the diff view is torn down here too - the + // write never happens, and a later save of that editor would land it anyway. + this.resetTaskPartialState(task) + await this.cleanupFailedPartialStream(task) return } const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false + // The point of no return for this write. saveChanges() persists the document and then keeps + // working (closing the diff views, tab bookkeeping, diagnostics), and trackFileContext() and + // pushToolWriteResult() run after it, so a rejection past this point must not roll back a + // write the user already approved: the rollback would restore the previous content, or delete + // a file that was created and saved. Set only once the durable write has landed. + let writeCommitted = false + let fileExists: boolean const absolutePath = path.resolve(task.cwd, relPath) @@ -66,12 +457,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { task.diffViewProvider.editType = fileExists ? "modify" : "create" } - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open, fs.readFile) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } - if (newContent.startsWith("```")) { newContent = newContent.split("\n").slice(1).join("\n") } @@ -95,7 +480,19 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } + // Tracks whether the user approved the write, so the error path only reverts the + // diff document when the content was never approved (an approved edit is kept in + // the editor so the user can save it manually after a late failure). + let writeApproved = false + try { + // Create parent directories for new files inside the try block so filesystem + // errors (EROFS, EACCES, etc.) route through handleError with proper cleanup + // and consecutive-mistake counting, rather than escaping unhandled. + if (!fileExists) { + await createDirectoriesForFile(absolutePath) + } + task.consecutiveMistakeCount = 0 const provider = task.providerRef.deref() @@ -129,10 +526,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { + // The prevent-focus branch set editType/originalContent on the provider + // before asking. Clear them on denial (no diff document was opened in + // this branch, so reset() is sufficient; the non-prevent-focus denial + // branch resets through revertChanges()), so a later write re-checks + // the file system instead of reusing the stale editType. + await this.resetDiffViewAfterWrite(task) return } - - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + writeApproved = true + + // saveDirectly() performs the write itself, so it is the only place that can say when the + // content landed. The flag is set there, at the filesystem write, and not when the call + // returns: the document open, the forced save and the diagnostics that follow can still + // reject, and by then the content the user approved is already on disk, which is where the + // rollback below has to stand down from. + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + () => { + writeCommitted = true + }, + ) } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) @@ -164,7 +582,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + writeApproved = true + + 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) { @@ -177,17 +602,61 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) - await task.diffViewProvider.reset() - this.resetPartialState() + await this.resetDiffViewAfterWrite(task) task.processQueuedMessages() return } catch (error) { - await handleError("writing file", error as Error) - await task.diffViewProvider.reset() - this.resetPartialState() + // Finalize any open partial tool message so the UI spinner doesn't get stuck. + // The partial ask fired during streaming (handlePartial) or early in execute sets + // partial: true on the webview message; without this, the spinner persists even + // after the error bubble appears. + await this.finalizePartialToolAskAfterFailure(task) + // The diff cleanup runs in a finally around handleError: the production + // handleError awaits Task.say(), which rejects when the task is aborted, and a + // rejected handleError must not skip restoring the unapproved streamed content. + try { + await handleError("writing file", error as Error) + } finally { + // Before approval the diff document holds unapproved streamed content: + // restore it so a user save cannot persist it. After approval the content + // is the user's accepted edit -- keep it in the editor (dirty) so they can + // save it manually. + let reverted = true + let strandedPlaceholder = false + // Two independent stand-downs: the user approved this content, or the write is already + // durable. Either one makes a rollback undo something the user wanted rather than + // cleaning up this call's debris. + if (!writeApproved && !writeCommitted) { + reverted = await this.discardUnapprovedStreamBeforeReset(task) + // A denial whose rollback could not remove the placeholder (refused restore, + // EPERM unlink) leaves it owned: reset() would forget that ownership and + // strand the empty file unowned, so it is skipped and the hazard reported - + // the same discipline as the pre-commit save-failure path below. + strandedPlaceholder = !reverted && task.diffViewProvider.ownsPlaceholder + } else if (writeApproved && !writeCommitted) { + // saveChanges() failed before its commit point: the write never persisted, + // so the owned empty placeholder must not be stranded unowned by reset(). + strandedPlaceholder = !(await this.discardOwnedPlaceholderBeforeReset(task)) + } + if (!strandedPlaceholder) { + await this.resetDiffViewAfterWrite(task) + } + if (!reverted) { + // The restore failed, so the editor still holds content this task never approved. + // The error the user sees on this path is the write failure, not this one. + await this.reportRevertFailure(task) + } + if (strandedPlaceholder) { + // The placeholder is still on disk and still owned; reset() was skipped so + // the session keeps that ownership for a retry. Make the hazard visible. + await this.reportPlaceholderStranded(task) + } + } return + } finally { + this.resetTaskPartialState(task) } } @@ -195,62 +664,181 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const relPath: string | undefined = block.params.path const newContent: string | undefined = block.params.content - // Wait for path to stabilize before showing UI (prevents truncated paths) - if (!this.hasPathStabilized(relPath) || newContent === undefined) { + const partialStreamFailureKey = this.getPartialStreamFailureKey(task) + + // A prior streaming delta for this task already hit a fatal filesystem error. + // Skip further streaming work so we don't create a new partial tool message on every + // subsequent delta. execute() will report the error once when the block completes. + if (this.taskPartialStreamState.get(partialStreamFailureKey)?.streamFailed) { return } - const provider = task.providerRef.deref() - const state = await provider?.getState() - const isPreventFocusDisruptionEnabled = experiments.isEnabled( - state?.experiments ?? {}, - EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, - ) + // Get (or create) this task's state; registers the TaskAborted teardown listener + // once, so abandoned streams are torn down even if execute() never runs. + const partialStreamState = this.getTaskPartialStreamState(task) - if (isPreventFocusDisruptionEnabled) { + // Wait for path to stabilize before showing UI (prevents truncated paths) + if (!this.hasPathStabilizedForTask(partialStreamState, relPath) || newContent === undefined) { return } - // relPath is guaranteed non-null after hasPathStabilized - let fileExists: boolean - const absolutePath = path.resolve(task.cwd, relPath!) + // Everything below runs after getTaskPartialStreamState() registered this task's entry and its + // TaskAborted listener, so it belongs inside a boundary that releases them. The awaits here look + // like reads and are not: provider.getState() is an await into the extension host and the + // filesystem probe can reject. If either does, this delta reaches neither the streaming block + // nor execute()'s teardown, and BaseTool.handle() only reports the error - which would leave + // the singleton holding a task that nothing else ever releases. - if (task.diffViewProvider.editType !== undefined) { - fileExists = task.diffViewProvider.editType === "modify" - } else { - fileExists = await fileExistsAtPath(absolutePath) - task.diffViewProvider.editType = fileExists ? "modify" : "create" - } + // Declared outside the try: the streaming catch below reads it, and a try block's + // own scope is not visible to code that runs after the try. + let partialMessage: string | undefined - // Create parent directories early for new files to prevent ENOENT errors - // in subsequent operations (e.g., diffViewProvider.open) - if (!fileExists) { - await createDirectoriesForFile(absolutePath) - } + try { + const provider = task.providerRef.deref() + const state = await provider?.getState() + // Cancelled while provider state was in flight: the teardown already released this + // task's stream state, so this delta must not go on to ask or open a diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } - const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false - const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + const isPreventFocusDisruptionEnabled = experiments.isEnabled( + state?.experiments ?? {}, + EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, + ) - const sharedMessageProps: ClineSayTool = { - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(task.cwd, relPath!), - content: newContent || "", - isOutsideWorkspace, - isProtected: isWriteProtected, - } + if (isPreventFocusDisruptionEnabled) { + // The preview is suppressed for this stream: release the entry registered above so the + // abort listener and any failure mark do not outlive a delta that never shows a diff + // view and never reaches execute()'s teardown. + super.resetPartialState() + this.resetTaskPartialState(task) + return + } - const partialMessage = JSON.stringify(sharedMessageProps) - await task.ask("tool", partialMessage, block.partial).catch(() => {}) + // relPath is guaranteed non-null after hasPathStabilized + let fileExists: boolean + const absolutePath = path.resolve(task.cwd, relPath!) - if (newContent) { - if (!task.diffViewProvider.isEditing) { - await task.diffViewProvider.open(relPath!) + if (task.diffViewProvider.editType !== undefined) { + fileExists = task.diffViewProvider.editType === "modify" + } else { + fileExists = await fileExistsAtPath(absolutePath) + task.diffViewProvider.editType = fileExists ? "modify" : "create" } - await task.diffViewProvider.update( - everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, - false, - ) + // Cancelled during the filesystem probe: stop before the partial ask and the diff view. + // (The probe above is the only await in this branch.) + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) + + const sharedMessageProps: ClineSayTool = { + tool: fileExists ? "editedExistingFile" : "newFileCreated", + path: getReadablePath(task.cwd, relPath!), + content: newContent || "", + isOutsideWorkspace, + isProtected: isWriteProtected, + } + + // A retained session belongs to its own path: streaming a different path's deltas + // must not open a partial ask (the final rejection in execute() does not finalize it, + // leaving a spinner) and must not touch the diff view. Remember the stream so a + // later parse failure does not discard the retained, approved session. + if (task.diffViewProvider.isEditingDifferentPath(relPath!)) { + partialStreamState.differentPathStream = true + return + } + + partialMessage = JSON.stringify(sharedMessageProps) + await task.ask("tool", partialMessage, block.partial).catch(() => {}) + + // Cancelled while the partial ask was in flight: the teardown released this task's + // state, so this delta must not touch the diff view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + } catch (error) { + // Unexpected failure in the pre-streaming window: nothing was streamed and nothing was shown, + // so this is not the retained-streaming-failure case the inner catch below keeps state for. + // Release this delta's entry and its listener before the error reaches BaseTool.handle(), + // then rethrow so the delta is still reported. Identity-guarded: if a cancellation already + // released the entry and a newer stream re-created it under the same key, that entry stays. + // An earlier delta of THIS stream may already have filled the view with unapproved content, + // in which case the discard has to run before the entry goes: afterwards nothing else in this + // path can restore that preview. A stream that never reached the view leaves any session + // (its own or another path's) exactly as it found it. + if (this.isPartialStreamStillLive(task, partialStreamState)) { + if (partialStreamState.ownsPreview) { + await this.cleanupFailedPartialStream(task) + } + this.resetTaskPartialState(task) + } + throw error + } + + if (newContent) { + // From here this delta is committed to the diff view: the open() and update() below are what + // put unapproved partial content into it, so this stream - not a later call - owns whatever the + // view and the created artifacts hold. Every teardown site below gates on that ownership. + partialStreamState.ownsPreview = true + try { + if (!task.diffViewProvider.isEditing) { + await task.diffViewProvider.open(relPath!) + } + + // Cancelled while open() was in flight: do not stream partial content into a + // cancelled task's view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + await task.diffViewProvider.update( + everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, + false, + ) + } catch (error) { + // The task may have been aborted while open() or update() was in flight. The teardown + // already released this entry and the disposal path already owns the diff cleanup, so + // marking, finalizing and restoring here would be a second cleanup of the same stream: + // a second partial-tool row and a second restore racing the first. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + // Opening or updating the diff view can throw on filesystem errors + // (EACCES/EROFS on read-only paths). Finalize the partial tool message + // so the UI spinner doesn't get stuck and reset the diff view. Do NOT + // 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) + + // Finalizing is an await: a cancellation during it has already run the teardown and the + // disposal cleanup, so the restore below must not run a second time over the same view. + if (!this.isPartialStreamStillLive(task, partialStreamState)) { + return + } + + // The write was never approved: restore the document so a user save cannot + // persist the failed streamed content (reset() alone leaves it dirty), and + // surface the hazard if that restore itself failed. The stream error is + // reported by the authoritative non-partial path in execute(); the rollback + // hazard is a different failure and nothing else in this path says so. + await this.cleanupFailedPartialStream(task) + } } } } diff --git a/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts new file mode 100644 index 0000000000..6fd8ba202f --- /dev/null +++ b/src/core/tools/__tests__/writeToFileTool-partial-state-cleanup.spec.ts @@ -0,0 +1,303 @@ +// 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> + discardUnapprovedStream: 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), + discardUnapprovedStream: 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 discarding the unapproved diff view fails", async () => { + const task = buildTask("revert-fails", "inst-4") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["discardUnapprovedStreamBeforeReset"](task) + + expect(errorSpy).toHaveBeenCalledWith( + "Error discarding the unapproved write_to_file diff view:", + expect.any(Error), + ) + }) + + it("discards while the recovery state exists and reports the hazard when the discard fails", async () => { + const task = buildTask("revert-fails-teardown", "inst-6") + const t = task as unknown as CleanupTask + let stateSizeDuringRevert = -1 + t.diffViewProvider.discardUnapprovedStream = vi.fn(async () => { + // The recovery state must still be present while the rollback runs. + stateSizeDuringRevert = writeToFileTool["taskPartialStreamState"].size + throw new Error("revert failed") + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // Seed the per-task stream state so this teardown boundary is taken at all, and mark it as a + // stream that reached the diff view: that is the preview the hook below owes a rollback, and + // ownsPreview is the flag it checks before touching the view. + writeToFileTool["getTaskPartialStreamState"](task).ownsPreview = true + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + // Only handleError is reached when no streaming error was recorded; the + // structural double is the existing pattern in this file. + { handleError: vi.fn().mockResolvedValue(undefined) } as unknown as Parameters< + (typeof writeToFileTool)["onParameterParseFailure"] + >[1], + new Error("parameter parse failed"), + ) + + // A failed rollback is not a completed teardown: the user is told the editor may + // still hold content the task never approved. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(stateSizeDuringRevert).toBe(1) + // The teardown still finished. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + expect(handled).toBe(false) + errorSpy.mockRestore() + }) + + it("reports the rollback hazard AND the retained streaming error, and returns true", async () => { + const task = buildTask("revert-fails-with-stream-error", "inst-7") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + // The scenario stands for a delta that had already streamed into the diff view - that is + // the preview this teardown is on the hook to roll back - and ownsPreview is the flag the + // parse-failure hook checks before it touches the view. + state.ownsPreview = true + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + // Both reports happen: the rollback hazard and the error the user can act on. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("still reports the streaming error when the rollback warning itself fails", async () => { + const task = buildTask("rollback-warning-fails", "inst-8") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + t.say = vi.fn().mockRejectedValue(new Error("say failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const state = writeToFileTool["getTaskPartialStreamState"](task) + // The scenario stands for a delta that had already streamed into the diff view - that is + // the preview this teardown is on the hook to roll back - and ownsPreview is the flag the + // parse-failure hook checks before it touches the view. + state.ownsPreview = true + const streamError = new Error("filesystem failure while streaming") + state.streamFailed = true + state.streamError = streamError + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + // A failing report must not abort the teardown: the streaming error still lands. + expect(errorSpy).toHaveBeenCalledWith("Error reporting write_to_file rollback failure:", expect.any(Error)) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(handled).toBe(true) + errorSpy.mockRestore() + }) + + it("surfaces the rollback hazard from the failed-stream cleanup as well", async () => { + const task = buildTask("failed-stream-cleanup", "inst-9") + const t = task as unknown as CleanupTask + t.diffViewProvider.discardUnapprovedStream = vi.fn().mockRejectedValue(new Error("revert failed")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await writeToFileTool["cleanupFailedPartialStream"](task) + + // Same contract as the parse-failure teardown: a failed restore is reported. + expect(t.say).toHaveBeenCalledWith("error", expect.stringContaining("unapproved")) + expect(t.diffViewProvider.reset).toHaveBeenCalled() + errorSpy.mockRestore() + }) + + it("stays silent when the failed-stream cleanup restored the editor", async () => { + // Every other case here makes the discard reject, so a cleanup that warned after EVERY + // restore still passed. "The editor may hold content you never approved" is only true when + // the restore failed; saying it regardless trains the user to ignore the warning. + const task = buildTask("failed-stream-clean", "inst-10") + const t = task as unknown as CleanupTask + + await writeToFileTool["cleanupFailedPartialStream"](task) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(t.say).not.toHaveBeenCalled() + }) + + it("stays silent when the parse-failure teardown restored the editor", async () => { + // The same contract on the other teardown boundary: a completed restore is not a hazard, + // so nothing is reported and the incidental parse error is left to the caller. + const task = buildTask("parse-failure-clean", "inst-11") + const t = task as unknown as CleanupTask + // The scenario owns the preview the teardown below is responsible for, and ownsPreview is + // the flag the hook checks before it touches the view. + writeToFileTool["getTaskPartialStreamState"](task).ownsPreview = true + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + expect(t.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(t.say).not.toHaveBeenCalled() + expect(handleError).not.toHaveBeenCalled() + expect(handled).toBe(false) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases only its own bookkeeping when a parse failure follows a stream that never reached the view", async () => { + // The ownership case at this boundary: a stream whose path never stabilized still has + // state, and its final block can still fail to parse. The provider may be holding a + // session from an earlier call - together with the placeholder ownership that goes with + // it - and this stream never opened or updated that view, so the hook has to release the + // bookkeeping and leave the provider state alone: discarding or resetting here would throw + // away a session this stream does not own. + const task = buildTask("parse-failure-never-owned", "inst-12") + const t = task as unknown as CleanupTask + writeToFileTool["getTaskPartialStreamState"](task) + const handleError = vi.fn().mockResolvedValue(undefined) + + const handled = await writeToFileTool["onParameterParseFailure"]( + task, + { handleError } as unknown as Parameters<(typeof writeToFileTool)["onParameterParseFailure"]>[1], + new Error("parameter parse failed"), + ) + + expect(handled).toBe(false) + expect(t.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(t.diffViewProvider.reset).not.toHaveBeenCalled() + expect(t.say).not.toHaveBeenCalled() + expect(handleError).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("reports a retained streaming error without touching a view this stream never owned", async () => { + // Same ownership answer, with a filesystem failure on the books: the error is still + // the user's to act on, so it is reported with execute()'s "writing file" context - + // reporting it never required touching the other session's view. + const task = buildTask("never-owned-stream-error", "inst-13") + const t = task as unknown as CleanupTask + 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"), + ) + + expect(handled).toBe(true) + expect(handleError).toHaveBeenCalledWith("writing file", streamError) + expect(t.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(t.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("logs and continues when finalizing the open partial ask fails", async () => { + const task = buildTask("finalize-fails", "inst-5") + const t = task as unknown as CleanupTask + 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..25d6e407bc 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 @@ -147,10 +166,18 @@ describe("writeToFileTool", () => { editType: undefined, isEditing: false, originalContent: "", + // A retained-session contract used by execute(): the double reports no + // retained session and mirrors the provider's path comparison. + isEditingDifferentPath: vi.fn((_relPath: string) => false), + editingRelPath: undefined as string | undefined, + ownsPlaceholder: false, open: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue(undefined), reset: vi.fn().mockResolvedValue(undefined), revertChanges: vi.fn().mockResolvedValue(undefined), + discardUnapprovedStream: vi.fn().mockResolvedValue(undefined), + discardOwnedPlaceholder: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), saveChanges: vi.fn().mockResolvedValue({ newProblemsMessage: "", userEdits: null, @@ -186,8 +213,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) @@ -223,10 +254,13 @@ describe("writeToFileTool", () => { content: testContent, ...params, }, + // The harness builds deliberately malformed payloads (a missing or + // undefined path/content), which the typed args cannot express. One assertion + // through unknown keeps that case testable without introducing `any`. nativeArgs: { - path: (params.path ?? testFilePath) as any, - content: (params.content ?? testContent) as any, - }, + path: params.path ?? testFilePath, + content: params.content ?? testContent, + } as unknown as ToolUse<"write_to_file">["nativeArgs"], partial: isPartial, } @@ -288,18 +322,70 @@ describe("writeToFileTool", () => { ) it.skipIf(process.platform === "win32")( - "creates parent directories when path has stabilized (partial)", + "does not create directories in handlePartial -- only execute() creates them", async () => { - // First call - path not yet stabilized + // First call - path not yet stabilized, early return await executeWriteFileTool({}, { fileExists: false, isPartial: true }) expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized + // Second call with same path - path stabilized, handlePartial runs but + // must NOT call createDirectoriesForFile (directory creation belongs in execute) await executeWriteFileTool({}, { fileExists: false, isPartial: true }) - expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() }, ) + it("does not stream a different path's delta into a retained session", async () => { + // The retained session keeps isEditing true, so a different path's delta would + // skip open() and write its partial content into the old document. The streaming + // guard must leave the preview untouched and let execute() report the rejection. + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.isEditingDifferentPath = vi.fn(() => true) + + await executeWriteFileTool({ path: "other.txt" }, { fileExists: false, isPartial: true }) + await executeWriteFileTool({ path: "other.txt" }, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + // The partial ask is opened before the guard used to run; the different-path + // stream must not leave a spinner behind in the first place. + expect(mockCline.ask).not.toHaveBeenCalledWith("tool", expect.anything(), true) + }) + + it("preserves the retained session when a different path's stream fails to parse", async () => { + // The retained session belongs to another, approved edit: a parse failure on a + // different path's stream must not discard or reset it. + mockCline.diffViewProvider.isEditing = true + mockCline.diffViewProvider.isEditingDifferentPath = vi.fn(() => true) + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + + await executeWriteFileTool({ path: "other.txt" }, { fileExists: false, isPartial: true }) + await executeWriteFileTool({ path: "other.txt" }, { fileExists: false, isPartial: true }) + + // Final block arrives without native arguments, so execute() is skipped and the + // parse-failure teardown runs. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: "other.txt", content: "streamed content" }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The retained session's own delta is untouched. + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + it("does not create directories when file exists", async () => { await executeWriteFileTool({}, { fileExists: true }) @@ -393,6 +479,25 @@ describe("writeToFileTool", () => { // Should process normally without issues expect(mockCline.consecutiveMistakeCount).toBe(0) }) + + it("does not report a successful write as failed when final diff reset rejects", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith("Tool result message") + expect(mockCline.didEditFile).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) }) describe("partial block handling", () => { @@ -420,6 +525,689 @@ 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("stops before the filesystem probe when the state is released while provider state is in flight", async () => { + // handlePartial() awaits provider.getState() before any side effect. A cancellation + // during that await runs the TaskAborted teardown; the delta already in flight must + // stop there instead of probing, asking and opening a diff view for a dead task. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + // Clear the cached editType so the filesystem probe branch is the one that would run: + // the check after provider.getState() is the only thing that can keep it from running. + mockCline.diffViewProvider.editType = undefined + mockedFileExistsAtPath.mockClear() + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn(async () => { + writeToFileTool.clearTaskState(mockCline) + return {} + }), + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // The probe is the first side effect after getState(): the check above is what keeps it + // from running for a cancelled task. + expect(mockedFileExistsAtPath).not.toHaveBeenCalled() + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before the partial ask when the state is released during the filesystem probe", async () => { + // Same teardown, one await later. The first delta pins editType, so clear it to take + // the fileExistsAtPath branch again and abort inside it. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.editType = undefined + // mockImplementationOnce: executeWriteFileTool re-arms the default resolved value on + // every call, so a plain mockImplementation would be overwritten. + mockedFileExistsAtPath.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + return false + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before touching the diff view when the stream state is released during an in-flight ask", async () => { + // A cancellation while task.ask() is in flight runs the TaskAborted teardown. The delta + // that was already in flight must not then re-open the diff view for a task the user + // cancelled - that resurrects the state the teardown just released. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.ask.mockImplementation(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("stops before updating the diff view when the task is cancelled while open() is in flight", async () => { + // open() is the first provider await after the partial ask. If TaskAborted lands while + // it is in flight, the teardown has already released this task's stream state (and may + // have reverted or closed this very view), so the delta that is already in flight must + // not stream partial content into a cancelled task's view. + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.update.mockClear() + mockCline.diffViewProvider.open.mockImplementationOnce(async () => { + writeToFileTool.clearTaskState(mockCline) + }) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("releases the per-task stream state when provider state rejects during a partial delta", async () => { + // provider.getState() looks like a read and is an await into the extension host. It runs + // after this task's entry and its TaskAborted listener were registered, and a rejection + // here reaches neither the streaming block nor execute()'s teardown - BaseTool.handle() + // only reports the error. Without the outer boundary the singleton keeps the task, its + // diff-view provider and a listener that nothing else removes. + const stateFailure = Object.assign(new Error("EPERM: could not read the extension state"), { + code: "EPERM", + }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + mockCline.providerRef.deref.mockReturnValue({ getState: vi.fn().mockRejectedValue(stateFailure) }) + // The second delta already streamed and opened the view; only the failing delta matters here. + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.finalizePartialToolAsk.mockClear() + mockHandleError.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // Releasing the state must not swallow the failure: BaseTool.handle() still reports it. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", stateFailure) + // An earlier delta of THIS stream had already opened the view with unapproved content, so + // releasing the entry has to roll that preview back first: nothing else on this path + // reaches it once the state is gone. No new open happens for the failing delta. + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when the filesystem probe rejects during a partial delta", async () => { + // The probe looks like a read and is an await into the filesystem, registered + // after this task's entry and its TaskAborted listener. A rejection here reaches + // neither the streaming block nor execute()'s teardown, so the outer boundary must + // release them and rethrow - the singleton must not keep the task. + const probeFailure = Object.assign(new Error("EPERM: could not probe the target path"), { + code: "EPERM", + }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + // The earlier deltas set editType from the probe; clear it so the failing delta + // actually reaches fileExistsAtPath() again. + mockCline.diffViewProvider.editType = undefined + mockedFileExistsAtPath.mockRejectedValueOnce(probeFailure) + mockCline.diffViewProvider.open.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockHandleError.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + // Releasing the state must not swallow the failure: BaseTool.handle() still reports it. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", probeFailure) + // Same shape as the state-rejection case above: the earlier delta of this stream opened the + // view, so the release owes it a rollback before the entry goes. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + }) + + it("does not run a second cleanup when the task aborts while open() is in flight and open() then rejects", async () => { + // openDiffEditor() has command-failure and timeout rejection paths, so a cancellation can + // land while open() is pending and open() can then reject. The TaskAborted teardown already + // released this entry and task disposal already owns the diff cleanup, so the streaming + // catch must stop instead of finalizing the ask and reverting a second time over the same + // view - which would also write the failure mark into an entry that is no longer live. + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(abortCleanup).toBeInstanceOf(Function) + mockCline.diffViewProvider.open.mockImplementation(async () => { + abortCleanup?.() + throw new Error("command 'vscode.diff' rejected while the task was cancelled") + }) + mockCline.finalizePartialToolAsk.mockClear() + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + mockHandleError.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The cancellation is not a streaming failure: nothing is retained for the next stream, + // and the delta does not report an error the user did not cause. + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("stops before the diff restore when the task aborts while the failed ask is being finalized", async () => { + // The cleanup has two awaits. A cancellation that lands in the gap between them has + // already run the teardown, and task disposal owns the diff cleanup from that moment, so + // restoring and resetting here would race the disposal over the same view. + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(abortCleanup).toBeInstanceOf(Function) + mockCline.diffViewProvider.open.mockRejectedValue(new Error("command 'vscode.diff' rejected")) + mockCline.finalizePartialToolAsk.mockImplementation(async () => { + abortCleanup?.() + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // The first cleanup step ran; the second must not. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + }) + + it("does not treat a changed path between deltas as stabilized", async () => { + // Delta 1 streams "alpha.txt"; delta 2 streams "beta.txt" for the same task. The path changed + // between deltas, so it must not count as stabilized and no partial `tool` ask may be issued for + // the still-changing second path. + await executeWriteFileTool({ path: "alpha.txt" }, { isPartial: true }) + await executeWriteFileTool({ path: "beta.txt" }, { isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + }) + + it("does not issue a partial ask when content is undefined after path stabilization", async () => { + // Delta 1 stabilizes the path. Delta 2 repeats it but carries no content yet: the + // `newContent === undefined` clause must short-circuit the ask even though the path itself has + // stabilized. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({ content: undefined }, { isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("does not reopen an already open diff view during streaming", async () => { + // The diff view is already open for this task (isEditing). A stabilized delta must still update + // the streamed content but must not call open() again -- reopening would discard the view's + // current state. + mockCline.diffViewProvider.isEditing = true + + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false) + }) + + it("logs the streaming diff view failure with the write_to_file context", async () => { + // The catch arm logs a context-specific message before swallowing the error (execute() reports + // the authoritative one). The message must keep the write_to_file context so the log is + // actionable. + mockCline.diffViewProvider.open.mockRejectedValue(new Error("EACCES: permission denied")) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error streaming write_to_file diff view:", + expect.anything(), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("reports the captured streaming error instead of the parse error when the final block fails to parse, and clears the per-task state", async () => { + // A streaming delta fails with a filesystem error (streamFailed + streamError are + // captured). The final block then arrives without nativeArgs, so execute() never + // runs: the parse-failure teardown boundary must report the original filesystem + // error under the "writing file" context (not the incidental parse error) and tear + // down the per-task state, so the next write_to_file stream in this task is not + // blocked by the stale streamFailed guard and no abort listener leaks. + const fsError = new Error("EACCES: permission denied") + mockCline.diffViewProvider.open.mockRejectedValue(fsError) + // Capture the abort listener of the state created by the first deltas: it is the + // exact reference that the parse-failure teardown must detach. + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", + }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError) + expect(mockHandleError).not.toHaveBeenCalledWith( + expect.stringContaining("parsing write_to_file"), + expect.anything(), + ) + // The parse-failure teardown also restores the diff document: the + // streaming failure above already reverted it once (revert + reset), + // and the parse path runs the same cleanup again because execute() + // never runs on this path. + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2) + // Per-task state torn down at this boundary: guard cleared, the exact + // registered abort listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + + // The next stream for the same task must issue a partial ask again (the stale + // streamFailed guard is gone). + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(2) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("restores the diff document when the final block fails to parse after successful streaming", async () => { + // Streaming opened the diff view with unapproved partial content (open and + // update both succeeded, so no streaming error was captured). The final block + // then arrives without nativeArgs: execute() never runs, so its error cleanup + // never fires. The parse-failure teardown must still restore the document + // (revert before reset) or a user save could persist content the write never + // completed, and must report the generic parse error (no streaming error to + // surface instead). + let abortListener: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted && abortListener === undefined) { + abortListener = listener + } + return mockCline + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + // Delta 1 - stabilize path; delta 2 - streams the partial content into the + // diff view (open + update resolve). + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1) + + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath, content: testContent }, + nativeArgs: undefined, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", + }) + + // No streaming error was captured: the generic parse error is reported. + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + // The diff document is restored by the parse path itself (no streaming + // failure happened, so this is the only revert + reset in the test). + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + // Per-task state torn down: guard cleared, exact listener detached. + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(abortListener).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + } finally { + consoleErrorSpy.mockRestore() + } + }) + }) + + 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, + differentPathStream: false, + // A hand-built state never reached the diff view; the flag is what the cleanup sites read. + ownsPreview: 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("early-exit stream state cleanup", () => { + it("releases the per-task stream state when a rooignore denial returns early", async () => { + // A partial delta creates the per-task state and registers the abort listener; the + // denial then returns before the cleanup, which used to leave both behind for the + // task's lifetime (a retained streamFailed also suppresses later diff previews). + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // The teardown has to detach the function this tool registered; expect.any(Function) + // also passes when a different callback is removed and the real one keeps firing. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("releases the per-task stream state when a missing parameter returns early", async () => { + await executeWriteFileTool({}, { isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await writeToFileTool.execute({ path: "", content: "mock content" }, mockCline, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // This call's stream only ever reached the missing-parameter return, so it never opened the + // diff view: the early return must leave the provider alone, because reset() there + // clears relPath/isEditing - which is what a session retained by an earlier call needs + // to survive - and forgets whose placeholder the empty file is. The per-task bookkeeping + // is released above regardless. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when content is missing", async () => { + // The missing-content return sits before execute()'s guarded scope, so it needs its own + // release. The state is seeded first so the assertion proves a release happened rather + // than an empty map. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the + // typed params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult, + toolCallId: "test-tool-call-id", + }) + + 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) + // This call's stream only ever reached the missing-parameter return, so it never opened the + // diff view: the early return must leave the provider alone, because reset() there + // clears relPath/isEditing - which is what a session retained by an earlier call needs to + // survive - and forgets whose placeholder the empty file is. The per-task bookkeeping is + // released above regardless. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + }) + + it("discards the streamed preview when a rooignore denial returns early", async () => { + // The denial can arrive after a streaming delta opened the denied path and streamed + // content into it. This branch reported the denial and released the stream state, but + // never touched the diff view: the unapproved content sat in a saveable editor, and the + // provider kept this call's editType/relPath for whatever write came next. + await executeWriteFileTool({}, { isPartial: true }) + + await executeWriteFileTool({}, { accessAllowed: false }) + + expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], + ) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + }) + + it("leaves a retained session untouched when a parameter is missing and this stream never reached the view", async () => { + // The state entry is seeded by hand because the case the ownership guard exists for is a + // provider session that an EARLIER call left retained (a rollback that could not complete + // keeps relPath/isEditing and the placeholder ownership) while this call's own stream + // never opened the view. Discarding or resetting that provider state here would throw the + // retained session away, so the early return must not touch it. + writeToFileTool["getTaskPartialStreamState"](mockCline as never) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the typed + // params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + }) + + it("still discards the preview this stream opened when a parameter is missing", async () => { + // The counterpart of the case above: two deltas of one path do reach the diff view, so the + // missing-content return owes that unapproved preview a rollback before it releases the + // state - the ownership guard must not skip a teardown this stream actually earned. + await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + mockCline.diffViewProvider.discardUnapprovedStream.mockClear() + mockCline.diffViewProvider.reset.mockClear() + + const toolUse = { + type: "tool_use", + name: "write_to_file", + params: { path: testFilePath }, + nativeArgs: { path: testFilePath, content: undefined }, + // The fixture's point is a nativeArgs object whose content never arrived, which the typed + // params cannot express - hence the double assertion. + partial: false, + } as unknown as ToolUse<"write_to_file"> + const pushToolResult = vi.fn() + await writeToFileTool.handle(mockCline, toolUse, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], + ) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) describe("user interaction", () => { @@ -461,16 +1249,854 @@ describe("writeToFileTool", () => { expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() }) - it("handles partial streaming errors after path stabilizes", async () => { + it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => { + // The same filesystem operation is retried in execute() once the block completes, + // and that authoritative non-partial path reports the error to the user. Surfacing + // it during streaming too would show the same error twice, so handlePartial must NOT + // route streaming errors through handleError. mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed")) // First call - path not yet stabilized, no error yet await executeWriteFileTool({}, { isPartial: true }) expect(mockHandleError).not.toHaveBeenCalled() - // Second call with same path - path is now stabilized, error occurs + // Second call with same path - path is now stabilized, error occurs but is swallowed + await executeWriteFileTool({}, { isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial open() fails", async () => { + // Regression test: when diffViewProvider.open() throws during streaming (e.g. EACCES/EROFS + // on a read-only path), the partial tool ask created at the top of handlePartial leaves the + // UI spinner stuck. handlePartial must finalize the partial message and reset the diff view, + // and must NOT surface a duplicate error (execute() reports the authoritative one). + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }), + ) + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized + await executeWriteFileTool({}, { isPartial: true }) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Second call - path stabilized, open() rejects + await executeWriteFileTool({}, { isPartial: true }) + + // Exact streamed payload: finalizePartialToolAsk() no-ops on a text mismatch, so + // a wrong argument (e.g. relPath) would leave the spinner stuck. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + // The failed write's streamed content must be reverted before reset() clears the + // state discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => { + // Same regression as above but for the streaming update() call failing after open() succeeds. + mockCline.diffViewProvider.update.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }), + ) + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // First call - path not yet stabilized await executeWriteFileTool({}, { isPartial: true }) - 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 discardUnapprovedStream() relies on. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("does not spawn a new partial tool message on each streaming delta after a failure", async () => { + // Regression test: after diffViewProvider.open() throws and the partial message is + // finalized + diff view reset, the next streaming delta saw a non-partial last message + // and created a brand new "Zoo wants to edit this file" message -- repeating once per + // delta. After the fix, partialStreamFailed short-circuits subsequent deltas so only + // the single initial partial ask is issued. + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, ask issued once, open() fails, stream marked failed + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Deltas 3..5 - must be short-circuited, no further asks + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Only the single partial ask from delta 2 should have been issued + expect(mockCline.ask).toHaveBeenCalledTimes(1) + // open() must not be retried after the first failure + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1) + }) + + it("finalizes any open partial tool ask when final args cannot be parsed", async () => { + // Regression test: a write_to_file block whose final args fail to parse (e.g. the + // tool call was truncated mid-JSON by the output token limit) never reaches + // execute(). A streaming delta for that block may already have opened a partial + // `tool` ask (partial: true) -- BaseTool.handle must finalize it, otherwise the + // UI spinner stays stuck even though the parse error bubble was shown. + // Delta 1 - stabilize path (no ask yet) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // Delta 2 - path stabilized, partial ask issued once + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled() + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The parse error is still reported, and the open partial ask is finalized first. + // No argument: BaseTool.handle() calls finalizePartialToolAsk() with no text, so a + // mutation passing wrong text would leave findLast() unmatched and the spinner + // stuck. (toHaveBeenCalledWith(undefined) does not match a no-arg call under + // vitest's matcher semantics: [] is not equal to [undefined].) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + }) + + it("continues parse failure cleanup when finalizing the partial ask fails", async () => { + // Pins the .catch arm on task.finalizePartialToolAsk() in BaseTool.handle(): when the + // final args cannot be parsed and finalizing the open partial ask also fails, the + // failure must only be logged so the parse error is still reported to the user. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + // Final block arrives but its native args cannot be parsed, so execute() is skipped. + const toolUse: ToolUse = { + type: "tool_use", + name: "write_to_file", + params: { + path: testFilePath, + content: testContent, + }, + partial: false, + } + await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: vi.fn(), + toolCallId: "test-tool-call-id", + }) + + // Same no-argument contract as the sibling test above: the finalize call in + // BaseTool.handle() carries no text. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + // The parse error is still reported despite the failed finalization. + expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error)) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("reports a filesystem error only once across the streaming and execute phases", async () => { + // Regression test for the double-error UX defect: a single write_to_file call to a + // read-only path failed twice -- once in handlePartial ("handling partial write_to_file") + // and once in execute() ("writing file"). handlePartial now swallows its error so only + // the authoritative execute() error is surfaced. + const erofs = () => + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }) + mockCline.diffViewProvider.open.mockRejectedValue(erofs()) + mockedCreateDirectoriesForFile.mockRejectedValue(erofs()) + + // Streaming phase: stabilize path then fail (swallowed, no handleError) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + // Final phase: execute() reports the single authoritative error + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + }) + + it("does not reset consecutive mistake count when directory creation fails", async () => { + mockCline.consecutiveMistakeCount = 3 + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.consecutiveMistakeCount).toBe(3) + }) + + it("reverts the diff document when the write fails before approval", async () => { + // Regression test for the dirty-diff leak: streaming already opened the diff view + // with unapproved content, and the write then failed before the user could approve + // it. reset() alone left the diff document dirty with the streamed content -- a + // user save in the editor would persist a write the task never completed. The + // error path must revert the document (like the approval-denied path does) before + // resetting the provider state. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + // Record the relative order of discardUnapprovedStream() and reset() (vitest mocks expose + // no invocationCallOrder). + const diffViewCallOrder: string[] = [] + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + // Stream two deltas so the diff view is open with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + }) + + it("continues cleanup when reverting the diff document fails before approval", async () => { + // Pins the .catch arm on discardUnapprovedStream() in discardUnapprovedStreamBeforeReset(): a failed + // revert (e.g. the diff view was already closed) must only be logged so the + // remaining cleanup (diff view reset + per-task state teardown) always completes. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + let abortCleanup: (() => void) | undefined + mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => { + if (event === RooCodeEventName.TaskAborted) { + abortCleanup = listener + } + return mockCline + }) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.diffViewProvider.discardUnapprovedStream.mockRejectedValue(new Error("revert failed")) + + // Stream two deltas so the diff view opens with the unapproved content... + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + // ...then the completed block fails before approval and the revert fails too. + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error discarding the unapproved write_to_file diff view:", + expect.any(Error), + ) + // The diff view is still reset and the per-task stream state still torn down. + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The reference that was registered must be the one removed. + expect(abortCleanup).toBeTypeOf("function") + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps approved diff content in the editor when saving fails after approval", async () => { + // The reverse of the previous test: once the user approved the write, the diff + // content is their accepted edit. A late failure (e.g. saveChanges rejecting) + // must NOT revert it -- the document stays dirty so the user can save it manually. + mockCline.diffViewProvider.saveChanges.mockRejectedValueOnce(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.saveChanges).toHaveBeenCalled() + // The error path restores through discardUnapprovedStream(), not revertChanges(), so the + // discard is the call that would have thrown the approved edit away. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("does not roll back a write whose commit point has already passed", async () => { + // saveChanges() signals the commit point at the document save and then keeps working + // (closing the diff views, tab bookkeeping, diagnostics). A rejection in that tail is a + // failure AFTER a durable write: rolling back here would restore the previous content or + // delete a file the user had just approved being created. + let signalledAtTheSave = false + mockCline.diffViewProvider.saveChanges.mockImplementationOnce(async (...args: unknown[]) => { + const onCommit = args[2] as (() => void) | undefined + signalledAtTheSave = typeof onCommit === "function" + onCommit?.() + throw new Error("closeAllDiffViews rejected") + }) + + await executeWriteFileTool({}, { fileExists: false }) + + // The stand-down only works if execute() actually handed saveChanges() the signal. + expect(signalledAtTheSave).toBe(true) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("removes the owned placeholder when saveChanges fails before the commit point", async () => { + // save() resolving false makes saveChanges() throw before onCommit: approval is not + // persistence. The tool must not report success, and the empty placeholder open() + // created must be removed while it is still owned - reset() would otherwise strand + // it unowned for a later attempt to mistake for the pre-existing target. + const order: string[] = [] + mockCline.diffViewProvider.saveChanges.mockImplementationOnce(async () => { + throw new Error("Saving /test/path.txt was not confirmed; the write was not committed.") + }) + mockCline.diffViewProvider.discardOwnedPlaceholder = vi.fn(async () => { + order.push("discardPlaceholder") + }) + mockCline.diffViewProvider.reset = vi.fn(async () => { + order.push("reset") + }) + + await executeWriteFileTool({}, { fileExists: false }) + + // No success: the failure reaches handleError, not a write result. + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled() + // The approved content stays in the editor (no discard of the buffer), but the + // owned placeholder is removed BEFORE reset() clears the ownership. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.discardOwnedPlaceholder).toHaveBeenCalledTimes(1) + expect(order).toEqual(["discardPlaceholder", "reset"]) + }) + + it("reports the stranded placeholder and skips the reset when its removal fails", async () => { + // Pre-commit save failure followed by an EPERM placeholder removal: the hazard + // must reach the user, and reset() must not run - it would clear the ownership + // and strand the empty placeholder unowned with no retry path. + mockCline.diffViewProvider.saveChanges.mockImplementationOnce(async () => { + throw new Error("Saving /test/path.txt was not confirmed; the write was not committed.") + }) + mockCline.diffViewProvider.discardOwnedPlaceholder = vi + .fn() + .mockRejectedValue(Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" })) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled() + // The hazard is user-visible, not only a console line... + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("placeholder")) + // ...and ownership survives: reset() did not run to forget it. + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error removing the uncommitted write_to_file placeholder:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps the placeholder owned and skips the reset when a denied new-file rollback cannot remove it", async () => { + // A denial rolls the new file back; if the rollback cannot remove the placeholder + // (refused restore, EPERM unlink) the provider still owns it. reset() would forget + // that ownership and strand the empty file unowned, so it must be skipped and the + // hazard reported - the same discipline as the pre-commit save-failure path. + mockAskApproval.mockResolvedValue(false) + mockCline.diffViewProvider.revertChanges = vi + .fn() + .mockRejectedValue(Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" })) + mockCline.diffViewProvider.discardUnapprovedStream = vi + .fn() + .mockRejectedValue(Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" })) + // The rollback left the placeholder on disk: the provider still owns it. + mockCline.diffViewProvider.ownsPlaceholder = true + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // Ownership survives: reset() did not run to forget it... + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + // ...and the hazard is user-visible, not only a console line. + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("placeholder")) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps the session unreset when the denied rollback cannot confirm the placeholder save", async () => { + // A refused document.save() (resolves false) is a rollback failure, not a success: + // both rollback helpers stop with the placeholder still owned, so reset() must be + // skipped - it would forget the ownership and leave the dirty buffer one save away + // from recreating the denied file - and the hazard must be reported. + mockAskApproval.mockResolvedValue(false) + mockCline.diffViewProvider.revertChanges = vi + .fn() + .mockRejectedValue( + new Error( + "Rollback could not save the restored buffer for test/path.txt; its clean state is unconfirmed.", + ), + ) + mockCline.diffViewProvider.discardUnapprovedStream = vi + .fn() + .mockRejectedValue( + new Error( + "Rollback could not save the restored buffer for test/path.txt; its clean state is unconfirmed, so the cleanup stopped before closing tabs or deleting the placeholder.", + ), + ) + // The refused save left the placeholder on disk: the provider still owns it. + mockCline.diffViewProvider.ownsPlaceholder = true + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + // Ownership survives: reset() did not run to forget it... + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + // ...the rollback hazard and the stranded placeholder are both user-visible: + // assert each report separately, so dropping one cannot hide behind the other. + expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("placeholder")) + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("could not be restored after the failed tool call"), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("rejects a write for a different path while a session is retained", async () => { + // A refused rollback keeps isEditing + the dirty buffer for a retry of ITS + // path. Writing a different path through that session would update and save + // the old document while asking approval for the new one, so the call is + // rejected before any cleanup which could reset the retained session. + mockCline.diffViewProvider.isEditingDifferentPath = vi.fn( + (_relPath: string) => _relPath !== "test/path.txt", + ) + mockCline.diffViewProvider.editingRelPath = "test/path.txt" + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await executeWriteFileTool({ path: "other/path.txt" }, { fileExists: false }) + + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("unfinished write to 'test/path.txt' must be resolved first"), + ) + // The remedy must name the only path that actually releases the session. + expect(mockCline.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("Retry the write to 'test/path.txt'"), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("still completes the teardown when the stranded-placeholder report itself rejects", async () => { + // The hazard report must not abort the cleanup: a failing say is logged and the + // error path completes - the session stays unreset so the placeholder ownership + // survives for a retry. + mockCline.diffViewProvider.saveChanges.mockImplementationOnce(async () => { + throw new Error("Saving /test/path.txt was not confirmed; the write was not committed.") + }) + mockCline.diffViewProvider.discardOwnedPlaceholder = vi + .fn() + .mockRejectedValue(Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" })) + mockCline.say = vi.fn().mockRejectedValue(new Error("webview is gone")) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error reporting write_to_file stranded placeholder failure:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("continues execute error cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + // The execute error path finalizes without a text match: any open partial + // tool ask is closed. + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps partial stream failures isolated per task", async () => { + mockCline.diffViewProvider.open.mockRejectedValueOnce( + Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + mockCline.taskId = "task-2" + mockCline.instanceId = "instance-2" + mockCline.diffViewProvider.open.mockResolvedValue(undefined) + mockCline.diffViewProvider.update.mockResolvedValue(undefined) + mockCline.diffViewProvider.editType = undefined + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockCline.ask).toHaveBeenCalledTimes(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).toHaveBeenCalledTimes(2) + expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2) + }) + + it("swallows diff view reset errors during partial failure cleanup", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error resetting write_to_file diff view:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("continues partial failure cleanup when finalizing partial ask fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + mockCline.diffViewProvider.open.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(expectedPartialToolMessage) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called", async () => { + // Regression test: before the fix, createDirectoriesForFile was called in handlePartial + // with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called + // handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate + // in presentAssistantMessage was never reached and the agent loop stalled permanently. + // After the fix the call is removed entirely -- handlePartial never touches the filesystem. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + // First call -- path not yet stabilized, returns early + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockHandleError).not.toHaveBeenCalled() + + // Second call -- path stabilized; createDirectoriesForFile must NOT be called from + // handlePartial, so the mock rejection must not trigger and handleError must not be called + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("EROFS in execute() routes through handleError with cleanup rather than escaping unhandled", async () => { + // Regression test: before the fix, createDirectoriesForFile in execute() sat outside + // the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely. + // After the fix the call is inside the try block, so filesystem errors are caught and + // routed through handleError with proper diffViewProvider.reset() cleanup. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + // The tool must not have proceeded to open or save + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + + it("finalizes partial tool message on error so the UI spinner does not get stuck", async () => { + // Regression test: when a filesystem error is thrown in execute() the webview + // message created during handlePartial (or the early ask in execute) is stuck in + // partial: true state, showing an indefinite spinner alongside the error bubble. + // The catch block must call finalizePartialToolAsk() to close the spinner without + // blocking for user input. + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + + await executeWriteFileTool({}, { fileExists: false }) + + // handleError must still be called + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + + // finalizePartialToolAsk must have been called (no text: the execute error + // path closes whichever partial tool ask is open) to dismiss the spinner + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + // The write was never approved, so the diff document is reverted before reset + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + }) + + it("runs diff cleanup when handleError rejects", async () => { + // The production handleError awaits Task.say(), which rejects when the task is + // aborted. A rejected handleError must not skip the diff cleanup: the unapproved + // streamed content has to be reverted and the diff view reset, or a user save + // could persist the failed write. The handleError rejection itself propagates + // (it is not swallowed by the cleanup). As in the missing-parameter tests + // above, the revert mock awaits a deferred so the test proves the cleanup + // AWAITs discardUnapprovedStream() before reset(): with the revert still pending, + // reset() must not have run yet. + mockHandleError.mockRejectedValue(new Error("handleError rejected (aborted task)")) + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + const diffViewCallOrder: string[] = [] + let resolveRevert: () => void = () => {} + const revertDeferred = new Promise((resolve) => { + resolveRevert = resolve + }) + mockCline.diffViewProvider.discardUnapprovedStream.mockImplementation(async () => { + diffViewCallOrder.push("revert") + await revertDeferred + }) + mockCline.diffViewProvider.reset.mockImplementation(async () => { + diffViewCallOrder.push("reset") + }) + + const executePromise = executeWriteFileTool({}, { fileExists: false }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + // handleError was attempted with the write context and the cleanup has reached + // the deferred revert... + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + // ...and while the revert is still pending, reset() must not have run yet. + expect(mockCline.diffViewProvider.reset).not.toHaveBeenCalled() + + resolveRevert() + await expect(executePromise).rejects.toThrow("handleError rejected (aborted task)") + + // The unapproved content is reverted before the diff view reset. + expect(diffViewCallOrder).toEqual(["revert", "reset"]) + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + }) + }) + + describe("prevent focus disruption experiment", () => { + /** + * Enable the PREVENT_FOCUS_DISRUPTION experiment for the current task: the experiment + * branches in execute()/handlePartial() read it from the provider state they fetch. + */ + function enablePreventFocusDisruption(): void { + mockCline.providerRef = { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }), + } + } + + it("saves through saveDirectly without diff editor interaction when the experiment is enabled", async () => { + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + // The commit signal: saveDirectly fires it at the filesystem write, which is the only + // place that can say the content landed. + expect.any(Function), + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(true) + expect(toolResult).toBe("Tool result message") + }) + + it("keeps approved diff content when saveDirectly fails after approval", async () => { + // The experiment branch stamps writeApproved before saveDirectly, so a late failure + // must NOT revert the document (the user approved the edit and can save it + // manually) but must still finalize the partial ask and reset the diff view. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + enablePreventFocusDisruption() + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(new Error("save failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith(undefined) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalled() + // discardUnapprovedStream() is what the error path actually calls; asserting only on + // revertChanges() could never fail. + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("does not roll back a write that saveDirectly already landed", async () => { + // The prevent-focus-disruption branch never opens a diff view: saveDirectly() performs + // the write itself, so returning means the content is on disk. A failure in what follows + // (tracking the file, building the result) must not run the rollback over a landed write. + enablePreventFocusDisruption() + mockCline.fileContextTracker.trackFileContext.mockRejectedValueOnce(new Error("tracker down")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledTimes(1) + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.discardUnapprovedStream).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("skips streaming diff view work when the experiment is enabled", async () => { + // With the experiment enabled the tool preview is embedded in the complete message + // built in execute(), so handlePartial must not open or update the diff view while + // streaming. + enablePreventFocusDisruption() + + // Delta 1 - stabilize path; delta 2 - path stabilized but the experiment short-circuits + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled() + }) + + it("releases the per-task stream state when prevent-focus-disruption skips the partial preview", async () => { + // Delta 1 only pins the path, so the entry is still live afterwards (the stream is in + // flight). Delta 2 reaches the experiment check and returns without ever showing a + // preview: nothing else would release the entry or detach the TaskAborted listener. + enablePreventFocusDisruption() + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + expect(writeToFileTool["taskPartialStreamState"].size).toBe(1) + + await executeWriteFileTool({}, { fileExists: false, isPartial: true }) + + expect(mockCline.ask).not.toHaveBeenCalled() + expect(writeToFileTool["taskPartialStreamState"].size).toBe(0) + // Delta 1 registered the listener; delta 2 must detach THAT function, not merely + // some function with the right name. + const abortListener = mockCline.once.mock.calls.find( + ([event]: unknown[]) => event === RooCodeEventName.TaskAborted, + )?.[1] + expect(abortListener).toBeInstanceOf(Function) + expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener) + }) + + it("clears the provider state when prevent-focus approval is denied", async () => { + // The prevent-focus branch stamps editType/originalContent on the provider before + // asking. On denial nothing was approved and no diff document was opened, so the + // provider state must be cleared (reset) to make a later write re-check the file + // system instead of reusing the stale editType. The non-prevent-focus denial branch + // resets through revertChanges(); this branch must not call it (no document to + // revert). + enablePreventFocusDisruption() + mockAskApproval.mockResolvedValue(false) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockAskApproval).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1) + expect(mockCline.diffViewProvider.revertChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) }) }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 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/eslint-suppressions.json b/src/eslint-suppressions.json index ae3d504003..1f419bc2b2 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -896,7 +896,7 @@ }, "core/tools/__tests__/writeToFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 5 + "count": 3 } }, "core/tools/helpers/toolResultFormatting.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 20458837c3..e5c41ea53f 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -1579,6 +1579,8 @@ export class DiffViewProvider { * @param relPath - Relative path to the file * @param content - Content to write to the file * @param openFile - Whether to show the file in editor (false = open in memory only for diagnostics) + * @param onCommit - Called at the filesystem write below, the point after which a rollback + * would delete content that already landed * @returns Result of the save operation including any new problems detected */ async saveDirectly( @@ -1587,6 +1589,7 @@ export class DiffViewProvider { openFile: boolean = true, diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + onCommit?: () => void, ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -1600,6 +1603,17 @@ export class DiffViewProvider { // Write the content directly to the file await createDirectoriesForFile(absolutePath) await fs.writeFile(absolutePath, content, "utf-8") + // The write is the commit point, the same line saveChanges() signals its commit and releases + // the placeholder at: the document open, the forced save and the diagnostics below can all + // still reject, and a caller that rolled the call back after this point would delete content + // that already landed. + onCommit?.() + // Ownership is released at the same point saveChanges() releases it, and only for the + // placeholder that names this very file: a session retained on another path keeps its own + // placeholder owned, because this write never created it. + if (this.placeholderPath === absolutePath) { + this.placeholderPath = undefined + } // Open the document to ensure diagnostics are loaded // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory From a1d9d242d74bc8ff8479f81fa21840c140be8ff6 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 12 Nov 2026 18:48:40 +0800 Subject: [PATCH 3/3] test(write-to-file): stream the preview before asserting the denial cleanup One review row on #2434 read against head 725b8a3eb42fe3bd8c71e23b56a4ac7729ce5450: - writeToFileTool.spec.ts:1133 (Trivial, Maintainability & Code Quality): hasPathStabilized ForTask() records the path on the first delta and only stabilises on the second matching one, so the rooignore-denial test was checking discardUnapprovedStream and reset against a diff view that was never opened. Send a second matching delta first, assert open() and update() ran, and pin the streamed preview before the denial cleanup in call order. Gates run in this worktree against this commit: - pnpm --dir src exec vitest run core/tools/__tests__/writeToFileTool.spec.ts -> 1 test file passed, 79 tests passed, 5 skipped. - pnpm --dir src exec eslint --max-warnings=0 core/tools/__tests__/writeToFileTool.spec.ts -> clean. - pnpm --dir src exec prettier --write/--check on the same file -> clean. - src/eslint-suppressions.json untouched. Not run here: knip, compile, e2e-mock, platform-unit-test matrices, mutation-diff, codecov. No mutant was executed against this suite locally by this lane. --- src/core/tools/__tests__/writeToFileTool.spec.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 25d6e407bc..5de5fb57ff 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -1130,12 +1130,23 @@ describe("writeToFileTool", () => { // content into it. This branch reported the denial and released the stream state, but // never touched the diff view: the unapproved content sat in a saveable editor, and the // provider kept this call's editType/relPath for whatever write came next. + // The first delta only records the path in hasPathStabilizedForTask(); it takes a second matching + // delta before open()/update() run. Without a streamed preview this test would check cleanup calls + // against a diff view that was never opened, so stream it first and pin its order before the denial. await executeWriteFileTool({}, { isPartial: true }) + await executeWriteFileTool({}, { isPartial: true }) + + expect(mockCline.diffViewProvider.open).toHaveBeenCalled() + expect(mockCline.diffViewProvider.update).toHaveBeenCalled() await executeWriteFileTool({}, { accessAllowed: false }) expect(mockCline.say).toHaveBeenCalledWith("rooignore_error", testFilePath) expect(mockCline.diffViewProvider.discardUnapprovedStream).toHaveBeenCalledTimes(1) + // The streamed preview above is what the denial cleanup has to run after. + expect(mockCline.diffViewProvider.open.mock.invocationCallOrder[0]).toBeLessThan( + mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0], + ) expect(mockCline.diffViewProvider.discardUnapprovedStream.mock.invocationCallOrder[0]).toBeLessThan( mockCline.diffViewProvider.reset.mock.invocationCallOrder[0], )