Skip to content

Commit 75b52e3

Browse files
awschmedereasonliang28
authored andcommitted
fix: clear stuck UI spinner and duplicate/repeated errors on write_to_file filesystem failure
When write_to_file hits a filesystem error (EROFS/EACCES) the streaming phase left the "Zoo wants to edit this file" spinner running, surfaced the same error twice (handlePartial + execute), and spawned a new partial tool message on every subsequent streaming delta. - Add Task.finalizePartialToolAsk() to finalize a partial tool ask without blocking on user input, dismissing the spinner. - handlePartial swallows streaming filesystem errors (after finalizing the spinner and resetting the diff view) so only the authoritative execute() error is reported, eliminating the duplicate error bubble. - Track partialStreamFailed so later streaming deltas short-circuit instead of re-attempting and spawning repeated partial tool messages. - Add regression tests for spinner finalization, single-error reporting, and no repeated partial messages.
1 parent 0575a35 commit 75b52e3

3 files changed

Lines changed: 178 additions & 10 deletions

File tree

src/core/task/Task.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1831,6 +1831,21 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
18311831
return formatResponse.toolError(formatResponse.missingToolParameterError(paramName))
18321832
}
18331833

1834+
/**
1835+
* Finalize the last partial "tool" ask message without blocking for user input.
1836+
* Call this in error paths where a partial tool message was opened during streaming
1837+
* but execution failed before the normal approval flow could close it, so the webview
1838+
* spinner does not get stuck in a loading state.
1839+
*/
1840+
async finalizePartialToolAsk(): Promise<void> {
1841+
const lastMessage = this.clineMessages.at(-1)
1842+
1843+
if (lastMessage && lastMessage.partial && lastMessage.type === "ask" && lastMessage.ask === "tool") {
1844+
lastMessage.partial = false
1845+
await this.updateClineMessage(lastMessage)
1846+
}
1847+
}
1848+
18341849
// Lifecycle
18351850
// Start / Resume / Abort / Dispose
18361851

src/core/tools/WriteToFileTool.ts

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,19 @@ interface WriteToFileParams {
2626
export class WriteToFileTool extends BaseTool<"write_to_file"> {
2727
readonly name = "write_to_file" as const
2828

29+
/**
30+
* Set when a filesystem error aborts diff-view streaming during handlePartial for the
31+
* current tool invocation. Subsequent streaming deltas for the same block then skip the
32+
* doomed open()/update() retry, which would otherwise create a fresh "Zoo wants to edit
33+
* this file" message on every delta. Cleared by resetPartialState() between invocations.
34+
*/
35+
private partialStreamFailed = false
36+
37+
override resetPartialState(): void {
38+
super.resetPartialState()
39+
this.partialStreamFailed = false
40+
}
41+
2942
async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
3043
const { pushToolResult, handleError, askApproval } = callbacks
3144
const relPath = params.path
@@ -187,6 +200,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
187200

188201
return
189202
} catch (error) {
203+
// Finalize any open partial tool message so the UI spinner doesn't get stuck.
204+
// The partial ask fired during streaming (handlePartial) or early in execute sets
205+
// partial: true on the webview message; without this, the spinner persists even
206+
// after the error bubble appears.
207+
await task.finalizePartialToolAsk()
190208
await handleError("writing file", error as Error)
191209
await task.diffViewProvider.reset()
192210
this.resetPartialState()
@@ -198,6 +216,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
198216
const relPath: string | undefined = block.params.path
199217
const newContent: string | undefined = block.params.content
200218

219+
// A prior streaming delta for this invocation already hit a fatal filesystem error.
220+
// Skip further streaming work so we don't create a new partial tool message on every
221+
// subsequent delta. execute() will report the error once when the block completes.
222+
if (this.partialStreamFailed) {
223+
return
224+
}
225+
201226
// Wait for path to stabilize before showing UI (prevents truncated paths)
202227
if (!this.hasPathStabilized(relPath) || newContent === undefined) {
203228
return
@@ -240,14 +265,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
240265
await task.ask("tool", partialMessage, block.partial).catch(() => {})
241266

242267
if (newContent) {
243-
if (!task.diffViewProvider.isEditing) {
244-
await task.diffViewProvider.open(relPath!)
245-
}
268+
try {
269+
if (!task.diffViewProvider.isEditing) {
270+
await task.diffViewProvider.open(relPath!)
271+
}
246272

247-
await task.diffViewProvider.update(
248-
everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent,
249-
false,
250-
)
273+
await task.diffViewProvider.update(
274+
everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent,
275+
false,
276+
)
277+
} catch (error) {
278+
// Opening or updating the diff view can throw on filesystem errors
279+
// (EACCES/EROFS on read-only paths). Finalize the partial tool message
280+
// so the UI spinner doesn't get stuck and reset the diff view. Do NOT
281+
// rethrow: the same filesystem operation is retried in execute() once the
282+
// block completes, and that authoritative non-partial path reports the
283+
// error to the user. Surfacing it here too would show the same error twice.
284+
// Swallowing it here is safe because the agent loop advances naturally when
285+
// the non-partial block arrives (it does not depend on this throw).
286+
console.error(`Error streaming write_to_file diff view:`, error)
287+
// Mark the stream as failed so later deltas don't re-attempt and spawn a new
288+
// partial tool message each time.
289+
this.partialStreamFailed = true
290+
await task.finalizePartialToolAsk()
291+
await task.diffViewProvider.reset()
292+
}
251293
}
252294
}
253295
}

src/core/tools/__tests__/writeToFileTool.spec.ts

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,7 @@ describe("writeToFileTool", () => {
186186
}
187187
mockCline.say = vi.fn().mockResolvedValue(undefined)
188188
mockCline.ask = vi.fn().mockResolvedValue(undefined)
189+
mockCline.finalizePartialToolAsk = vi.fn().mockResolvedValue(undefined)
189190
mockCline.recordToolError = vi.fn()
190191
mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error")
191192

@@ -461,16 +462,104 @@ describe("writeToFileTool", () => {
461462
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
462463
})
463464

464-
it("handles partial streaming errors after path stabilizes", async () => {
465+
it("swallows partial streaming errors instead of surfacing a duplicate error bubble", async () => {
466+
// The same filesystem operation is retried in execute() once the block completes,
467+
// and that authoritative non-partial path reports the error to the user. Surfacing
468+
// it during streaming too would show the same error twice, so handlePartial must NOT
469+
// route streaming errors through handleError.
465470
mockCline.diffViewProvider.open.mockRejectedValue(new Error("Open failed"))
466471

467472
// First call - path not yet stabilized, no error yet
468473
await executeWriteFileTool({}, { isPartial: true })
469474
expect(mockHandleError).not.toHaveBeenCalled()
470475

471-
// Second call with same path - path is now stabilized, error occurs
476+
// Second call with same path - path is now stabilized, error occurs but is swallowed
472477
await executeWriteFileTool({}, { isPartial: true })
473-
expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error))
478+
expect(mockHandleError).not.toHaveBeenCalled()
479+
})
480+
481+
it("finalizes partial tool message and resets diff view when handlePartial open() fails", async () => {
482+
// Regression test: when diffViewProvider.open() throws during streaming (e.g. EACCES/EROFS
483+
// on a read-only path), the partial tool ask created at the top of handlePartial leaves the
484+
// UI spinner stuck. handlePartial must finalize the partial message and reset the diff view,
485+
// and must NOT surface a duplicate error (execute() reports the authoritative one).
486+
mockCline.diffViewProvider.open.mockRejectedValue(
487+
Object.assign(new Error("EACCES: permission denied, open '/ro/test.py'"), { code: "EACCES" }),
488+
)
489+
490+
// First call - path not yet stabilized
491+
await executeWriteFileTool({}, { isPartial: true })
492+
expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled()
493+
494+
// Second call - path stabilized, open() rejects
495+
await executeWriteFileTool({}, { isPartial: true })
496+
497+
expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled()
498+
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
499+
expect(mockHandleError).not.toHaveBeenCalled()
500+
})
501+
502+
it("finalizes partial tool message and resets diff view when handlePartial update() fails", async () => {
503+
// Same regression as above but for the streaming update() call failing after open() succeeds.
504+
mockCline.diffViewProvider.update.mockRejectedValue(
505+
Object.assign(new Error("EROFS: read-only file system, write '/ro/test.py'"), { code: "EROFS" }),
506+
)
507+
508+
// First call - path not yet stabilized
509+
await executeWriteFileTool({}, { isPartial: true })
510+
511+
// Second call - path stabilized, update() rejects
512+
await executeWriteFileTool({}, { isPartial: true })
513+
514+
expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled()
515+
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
516+
expect(mockHandleError).not.toHaveBeenCalled()
517+
})
518+
519+
it("does not spawn a new partial tool message on each streaming delta after a failure", async () => {
520+
// Regression test: after diffViewProvider.open() throws and the partial message is
521+
// finalized + diff view reset, the next streaming delta saw a non-partial last message
522+
// and created a brand new "Zoo wants to edit this file" message -- repeating once per
523+
// delta. After the fix, partialStreamFailed short-circuits subsequent deltas so only
524+
// the single initial partial ask is issued.
525+
mockCline.diffViewProvider.open.mockRejectedValue(
526+
Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }),
527+
)
528+
529+
// Delta 1 - stabilize path (no ask yet)
530+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
531+
// Delta 2 - path stabilized, ask issued once, open() fails, stream marked failed
532+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
533+
// Deltas 3..5 - must be short-circuited, no further asks
534+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
535+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
536+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
537+
538+
// Only the single partial ask from delta 2 should have been issued
539+
expect(mockCline.ask).toHaveBeenCalledTimes(1)
540+
// open() must not be retried after the first failure
541+
expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1)
542+
})
543+
544+
it("reports a filesystem error only once across the streaming and execute phases", async () => {
545+
// Regression test for the double-error UX defect: a single write_to_file call to a
546+
// read-only path failed twice -- once in handlePartial ("handling partial write_to_file")
547+
// and once in execute() ("writing file"). handlePartial now swallows its error so only
548+
// the authoritative execute() error is surfaced.
549+
const erofs = () =>
550+
Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" })
551+
mockCline.diffViewProvider.open.mockRejectedValue(erofs())
552+
mockedCreateDirectoriesForFile.mockRejectedValue(erofs())
553+
554+
// Streaming phase: stabilize path then fail (swallowed, no handleError)
555+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
556+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
557+
558+
// Final phase: execute() reports the single authoritative error
559+
await executeWriteFileTool({}, { fileExists: false })
560+
561+
expect(mockHandleError).toHaveBeenCalledTimes(1)
562+
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
474563
})
475564

476565
it.skipIf(process.platform === "win32")(
@@ -517,5 +606,27 @@ describe("writeToFileTool", () => {
517606
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
518607
},
519608
)
609+
610+
it.skipIf(process.platform === "win32")(
611+
"finalizes partial tool message on error so the UI spinner does not get stuck",
612+
async () => {
613+
// Regression test: when a filesystem error is thrown in execute() the webview
614+
// message created during handlePartial (or the early ask in execute) is stuck in
615+
// partial: true state, showing an indefinite spinner alongside the error bubble.
616+
// The catch block must call finalizePartialToolAsk() to close the spinner without
617+
// blocking for user input.
618+
mockedCreateDirectoriesForFile.mockRejectedValue(
619+
Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }),
620+
)
621+
622+
await executeWriteFileTool({}, { fileExists: false })
623+
624+
// handleError must still be called
625+
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
626+
627+
// finalizePartialToolAsk must have been called to dismiss the spinner
628+
expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled()
629+
},
630+
)
520631
})
521632
})

0 commit comments

Comments
 (0)