-
Notifications
You must be signed in to change notification settings - Fork 220
fix(write-to-file): address partial filesystem error review #1066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0575a35
75b52e3
0966556
16c4d48
be0e154
224690b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2857,6 +2857,78 @@ describe("Cline", () => { | |
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { | ||
| const updateSpy = vi | ||
| .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") | ||
| .mockResolvedValue(undefined) | ||
| const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) | ||
|
|
||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const partialToolAsk = { | ||
| ts: Date.now() - 2, | ||
| type: "ask" as const, | ||
| ask: "tool" as const, | ||
| text: "partial tool message", | ||
| partial: true, | ||
| } | ||
|
|
||
| task.clineMessages.push(partialToolAsk) | ||
| task.clineMessages.push({ | ||
| ts: Date.now() - 1, | ||
| type: "say", | ||
| say: "error", | ||
| text: "intervening async message", | ||
| }) | ||
|
|
||
| await task.finalizePartialToolAsk("partial tool message") | ||
| await flushMicrotasks() | ||
|
|
||
| expect(partialToolAsk.partial).toBe(false) | ||
| expect(saveSpy).toHaveBeenCalled() | ||
| expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Vitest stores spy arguments as live references. Since let snap: Record<string, unknown> | undefined
updateSpy.mockImplementation(async (m) => { snap = { ...m } })
// ... then:
expect(snap?.partial).toBe(false) |
||
|
|
||
| updateSpy.mockRestore() | ||
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => { | ||
| const updateSpy = vi | ||
| .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") | ||
| .mockResolvedValue(undefined) | ||
| const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) | ||
|
|
||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| task.clineMessages.push({ | ||
| ts: Date.now() - 1, | ||
| type: "ask", | ||
| ask: "tool", | ||
| text: "other partial tool message", | ||
| partial: true, | ||
| }) | ||
|
|
||
| await task.finalizePartialToolAsk("target partial tool message") | ||
| await flushMicrotasks() | ||
|
|
||
| expect(task.clineMessages[0].partial).toBe(true) | ||
| expect(saveSpy).not.toHaveBeenCalled() | ||
| expect(updateSpy).not.toHaveBeenCalled() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both tests here pass a text string. The |
||
|
|
||
| updateSpy.mockRestore() | ||
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { | ||
| // Pins the .catch arm on the fire-and-forget updateClineMessage call | ||
| // in ask() when a new partial ask arrives while the previous partial | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import path from "path" | |
| import delay from "delay" | ||
| import fs from "fs/promises" | ||
|
|
||
| import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" | ||
| import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS, RooCodeEventName } from "@roo-code/types" | ||
|
|
||
| import { Task } from "../task/Task" | ||
| import { formatResponse } from "../prompts/responses" | ||
|
|
@@ -26,10 +26,87 @@ interface WriteToFileParams { | |
| export class WriteToFileTool extends BaseTool<"write_to_file"> { | ||
| readonly name = "write_to_file" as const | ||
|
|
||
| /** | ||
| * Tracks filesystem failures from diff-view streaming by task id. Tool instances are | ||
| * singletons, so this state must be keyed per task to avoid one task's failing partial | ||
| * stream suppressing another task's streaming deltas. | ||
| */ | ||
| private partialStreamFailuresByTaskId = new Set<string>() | ||
|
|
||
| /** | ||
| * Tracks partial path stabilization by task id. The tool is a singleton, so using the | ||
| * BaseTool singleton path state lets concurrent tasks incorrectly stabilize each other. | ||
| */ | ||
| private lastSeenPartialPathByTaskId = new Map<string, string | undefined>() | ||
|
|
||
| /** | ||
| * Tracks abort cleanup listeners for per-task partial state so normal execute() | ||
| * finalization can unregister them and abandoned streams are torn down on abort. | ||
| */ | ||
| private partialStateAbortCleanupByTaskId = new Map<string, { task: Task; cleanup: () => void }>() | ||
|
|
||
| private getPartialStreamFailureKey(task: Task): string { | ||
| return `${task.taskId}.${task.instanceId}` | ||
| } | ||
|
|
||
| private registerTaskPartialStateCleanup(task: Task): void { | ||
| const key = this.getPartialStreamFailureKey(task) | ||
| if (this.partialStateAbortCleanupByTaskId.has(key)) { | ||
| return | ||
| } | ||
|
|
||
| const cleanup = () => this.resetTaskPartialState(task) | ||
| this.partialStateAbortCleanupByTaskId.set(key, { task, cleanup }) | ||
| task.once(RooCodeEventName.TaskAborted, cleanup) | ||
| } | ||
|
|
||
| private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean { | ||
| this.registerTaskPartialStateCleanup(task) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling |
||
| const key = this.getPartialStreamFailureKey(task) | ||
| const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key) | ||
| const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath | ||
| this.lastSeenPartialPathByTaskId.set(key, partialPath) | ||
| return pathHasStabilized && !!partialPath | ||
| } | ||
|
|
||
| private resetTaskPartialState(task: Task): void { | ||
| const key = this.getPartialStreamFailureKey(task) | ||
| const abortCleanup = this.partialStateAbortCleanupByTaskId.get(key) | ||
| if (abortCleanup) { | ||
| task.off(RooCodeEventName.TaskAborted, abortCleanup.cleanup) | ||
| this.partialStateAbortCleanupByTaskId.delete(key) | ||
| } | ||
| this.lastSeenPartialPathByTaskId.delete(key) | ||
| this.partialStreamFailuresByTaskId.delete(key) | ||
| } | ||
|
|
||
| private async resetDiffViewAfterWrite(task: Task): Promise<void> { | ||
| await task.diffViewProvider.reset().catch((resetError) => { | ||
| console.error("Error resetting write_to_file diff view:", resetError) | ||
| }) | ||
| } | ||
|
|
||
| private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise<void> { | ||
| await task.finalizePartialToolAsk(text).catch((finalizeError) => { | ||
| console.error("Error finalizing write_to_file partial tool ask:", finalizeError) | ||
| }) | ||
| } | ||
|
|
||
| override resetPartialState(): void { | ||
| super.resetPartialState() | ||
| for (const { task, cleanup } of this.partialStateAbortCleanupByTaskId.values()) { | ||
| task.off(RooCodeEventName.TaskAborted, cleanup) | ||
| } | ||
| this.partialStreamFailuresByTaskId.clear() | ||
| this.lastSeenPartialPathByTaskId.clear() | ||
| this.partialStateAbortCleanupByTaskId.clear() | ||
| } | ||
|
|
||
| async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> { | ||
| const { pushToolResult, handleError, askApproval } = callbacks | ||
| const relPath = params.path | ||
| let newContent = params.content | ||
| const partialStreamFailureKey = this.getPartialStreamFailureKey(task) | ||
|
|
||
| if (!relPath) { | ||
| task.consecutiveMistakeCount++ | ||
|
|
@@ -67,12 +144,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") | ||
| } | ||
|
|
@@ -97,6 +168,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { | |
| } | ||
|
|
||
| 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() | ||
|
|
@@ -179,26 +257,40 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { | |
|
|
||
| pushToolResult(message) | ||
|
|
||
| await task.diffViewProvider.reset() | ||
| this.resetPartialState() | ||
| await this.resetDiffViewAfterWrite(task) | ||
|
|
||
| task.processQueuedMessages() | ||
|
|
||
| return | ||
| } catch (error) { | ||
| // Finalize any open partial tool message so the UI spinner doesn't get stuck. | ||
| // The partial ask fired during streaming (handlePartial) or early in execute sets | ||
| // partial: true on the webview message; without this, the spinner persists even | ||
| // after the error bubble appears. | ||
| await this.finalizePartialToolAskAfterFailure(task) | ||
| await handleError("writing file", error as Error) | ||
| await task.diffViewProvider.reset() | ||
| this.resetPartialState() | ||
| await this.resetDiffViewAfterWrite(task) | ||
| return | ||
| } finally { | ||
| this.resetTaskPartialState(task) | ||
| } | ||
| } | ||
|
|
||
| override async handlePartial(task: Task, block: ToolUse<"write_to_file">): Promise<void> { | ||
| const relPath: string | undefined = block.params.path | ||
| const newContent: string | undefined = block.params.content | ||
|
|
||
| const partialStreamFailureKey = this.getPartialStreamFailureKey(task) | ||
|
|
||
| // A prior streaming delta for this task already hit a fatal filesystem error. | ||
| // Skip further streaming work so we don't create a new partial tool message on every | ||
| // subsequent delta. execute() will report the error once when the block completes. | ||
| if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) { | ||
| return | ||
| } | ||
|
|
||
| // Wait for path to stabilize before showing UI (prevents truncated paths) | ||
| if (!this.hasPathStabilized(relPath) || newContent === undefined) { | ||
| if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) { | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -224,12 +316,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { | |
| task.diffViewProvider.editType = fileExists ? "modify" : "create" | ||
| } | ||
|
|
||
| // Create parent directories early for new files to prevent ENOENT errors | ||
| // in subsequent operations (e.g., diffViewProvider.open) | ||
| if (!fileExists) { | ||
| await createDirectoriesForFile(absolutePath) | ||
| } | ||
|
|
||
| const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false | ||
| const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) | ||
|
|
||
|
|
@@ -245,14 +331,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { | |
| await task.ask("tool", partialMessage, block.partial).catch(() => {}) | ||
|
|
||
| if (newContent) { | ||
| if (!task.diffViewProvider.isEditing) { | ||
| await task.diffViewProvider.open(relPath!) | ||
| } | ||
| try { | ||
| if (!task.diffViewProvider.isEditing) { | ||
| await task.diffViewProvider.open(relPath!) | ||
| } | ||
|
|
||
| await task.diffViewProvider.update( | ||
| everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, | ||
| false, | ||
| ) | ||
| await task.diffViewProvider.update( | ||
| everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent, | ||
| false, | ||
| ) | ||
| } catch (error) { | ||
| // Opening or updating the diff view can throw on filesystem errors | ||
| // (EACCES/EROFS on read-only paths). Finalize the partial tool message | ||
| // so the UI spinner doesn't get stuck and reset the diff view. Do NOT | ||
| // rethrow: the same filesystem operation is retried in execute() once the | ||
| // block completes, and that authoritative non-partial path reports the | ||
| // error to the user. Surfacing it here too would show the same error twice. | ||
| // Swallowing it here is safe because the agent loop advances naturally when | ||
| // the non-partial block arrives (it does not depend on this throw). | ||
| console.error(`Error streaming write_to_file diff view:`, error) | ||
| // Mark the stream as failed so later deltas don't re-attempt and spawn a new | ||
| // partial tool message each time. | ||
| this.partialStreamFailuresByTaskId.add(partialStreamFailureKey) | ||
| await this.finalizePartialToolAskAfterFailure(task, partialMessage) | ||
| await this.resetDiffViewAfterWrite(task) | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findLastis already exported from../../shared/array(same modulefindLastIndexis imported from on line 68). Would that be cleaner here?(Also add
findLastto the import on line 68.)