Skip to content

Commit 0b837ea

Browse files
committed
fix(write-to-file): address partial filesystem error review
1 parent 75b52e3 commit 0b837ea

4 files changed

Lines changed: 158 additions & 25 deletions

File tree

src/core/task/Task.ts

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1832,18 +1832,36 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
18321832
}
18331833

18341834
/**
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)
1835+
* Finalize a 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+
* The matching partial message may no longer be the final entry if another asynchronous
1841+
* message was inserted between the partial ask and the error handler, so search backward
1842+
* instead of relying on clineMessages.at(-1).
1843+
*/
1844+
async finalizePartialToolAsk(text?: string): Promise<void> {
1845+
const partialToolAsk = this.clineMessages
1846+
.slice()
1847+
.reverse()
1848+
.find(
1849+
(message) =>
1850+
message.partial === true &&
1851+
message.type === "ask" &&
1852+
message.ask === "tool" &&
1853+
(text === undefined || message.text === text),
1854+
)
1855+
1856+
if (!partialToolAsk) {
1857+
return
18461858
}
1859+
1860+
partialToolAsk.partial = false
1861+
await this.saveClineMessages()
1862+
this.updateClineMessage(partialToolAsk).catch((error) => {
1863+
console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error)
1864+
})
18471865
}
18481866

18491867
// Lifecycle

src/core/task/__tests__/Task.spec.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2857,6 +2857,78 @@ describe("Cline", () => {
28572857
saveSpy.mockRestore()
28582858
})
28592859

2860+
it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => {
2861+
const updateSpy = vi
2862+
.spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage")
2863+
.mockResolvedValue(undefined)
2864+
const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true)
2865+
2866+
const task = new Task({
2867+
provider: mockProvider,
2868+
apiConfiguration: mockApiConfig,
2869+
task: "test task",
2870+
startTask: false,
2871+
})
2872+
2873+
const partialToolAsk = {
2874+
ts: Date.now() - 2,
2875+
type: "ask" as const,
2876+
ask: "tool" as const,
2877+
text: "partial tool message",
2878+
partial: true,
2879+
}
2880+
2881+
task.clineMessages.push(partialToolAsk)
2882+
task.clineMessages.push({
2883+
ts: Date.now() - 1,
2884+
type: "say",
2885+
say: "error",
2886+
text: "intervening async message",
2887+
})
2888+
2889+
await task.finalizePartialToolAsk("partial tool message")
2890+
await flushMicrotasks()
2891+
2892+
expect(partialToolAsk.partial).toBe(false)
2893+
expect(saveSpy).toHaveBeenCalled()
2894+
expect(updateSpy).toHaveBeenCalledWith(partialToolAsk)
2895+
2896+
updateSpy.mockRestore()
2897+
saveSpy.mockRestore()
2898+
})
2899+
2900+
it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => {
2901+
const updateSpy = vi
2902+
.spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage")
2903+
.mockResolvedValue(undefined)
2904+
const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true)
2905+
2906+
const task = new Task({
2907+
provider: mockProvider,
2908+
apiConfiguration: mockApiConfig,
2909+
task: "test task",
2910+
startTask: false,
2911+
})
2912+
2913+
task.clineMessages.push({
2914+
ts: Date.now() - 1,
2915+
type: "ask",
2916+
ask: "tool",
2917+
text: "other partial tool message",
2918+
partial: true,
2919+
})
2920+
2921+
await task.finalizePartialToolAsk("target partial tool message")
2922+
await flushMicrotasks()
2923+
2924+
expect(task.clineMessages[0].partial).toBe(true)
2925+
expect(saveSpy).not.toHaveBeenCalled()
2926+
expect(updateSpy).not.toHaveBeenCalled()
2927+
2928+
updateSpy.mockRestore()
2929+
saveSpy.mockRestore()
2930+
})
2931+
28602932
it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => {
28612933
// Pins the .catch arm on the fire-and-forget updateClineMessage call
28622934
// in ask() when a new partial ask arrives while the previous partial

src/core/tools/WriteToFileTool.ts

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,22 +27,26 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
2727
readonly name = "write_to_file" as const
2828

2929
/**
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.
30+
* Tracks filesystem failures from diff-view streaming by task id. Tool instances are
31+
* singletons, so this state must be keyed per task to avoid one task's failing partial
32+
* stream suppressing another task's streaming deltas.
3433
*/
35-
private partialStreamFailed = false
34+
private partialStreamFailuresByTaskId = new Set<string>()
35+
36+
private getPartialStreamFailureKey(task: Task): string {
37+
return `${task.taskId}.${task.instanceId}`
38+
}
3639

3740
override resetPartialState(): void {
3841
super.resetPartialState()
39-
this.partialStreamFailed = false
42+
this.partialStreamFailuresByTaskId.clear()
4043
}
4144

4245
async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
4346
const { pushToolResult, handleError, askApproval } = callbacks
4447
const relPath = params.path
4548
let newContent = params.content
49+
const partialStreamFailureKey = this.getPartialStreamFailureKey(task)
4650

4751
if (!relPath) {
4852
task.consecutiveMistakeCount++
@@ -104,15 +108,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
104108
}
105109

106110
try {
107-
task.consecutiveMistakeCount = 0
108-
109111
// Create parent directories for new files inside the try block so filesystem
110112
// errors (EROFS, EACCES, etc.) route through handleError with proper cleanup
111113
// and consecutive-mistake counting, rather than escaping unhandled.
112114
if (!fileExists) {
113115
await createDirectoriesForFile(absolutePath)
114116
}
115117

118+
task.consecutiveMistakeCount = 0
119+
116120
const provider = task.providerRef.deref()
117121
const state = await provider?.getState()
118122
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
@@ -194,7 +198,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
194198
pushToolResult(message)
195199

196200
await task.diffViewProvider.reset()
197-
this.resetPartialState()
201+
super.resetPartialState()
202+
this.partialStreamFailuresByTaskId.delete(partialStreamFailureKey)
198203

199204
task.processQueuedMessages()
200205

@@ -207,7 +212,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
207212
await task.finalizePartialToolAsk()
208213
await handleError("writing file", error as Error)
209214
await task.diffViewProvider.reset()
210-
this.resetPartialState()
215+
super.resetPartialState()
216+
this.partialStreamFailuresByTaskId.delete(partialStreamFailureKey)
211217
return
212218
}
213219
}
@@ -216,10 +222,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
216222
const relPath: string | undefined = block.params.path
217223
const newContent: string | undefined = block.params.content
218224

219-
// A prior streaming delta for this invocation already hit a fatal filesystem error.
225+
const partialStreamFailureKey = this.getPartialStreamFailureKey(task)
226+
227+
// A prior streaming delta for this task already hit a fatal filesystem error.
220228
// Skip further streaming work so we don't create a new partial tool message on every
221229
// subsequent delta. execute() will report the error once when the block completes.
222-
if (this.partialStreamFailed) {
230+
if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) {
223231
return
224232
}
225233

@@ -286,8 +294,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
286294
console.error(`Error streaming write_to_file diff view:`, error)
287295
// Mark the stream as failed so later deltas don't re-attempt and spawn a new
288296
// partial tool message each time.
289-
this.partialStreamFailed = true
290-
await task.finalizePartialToolAsk()
297+
this.partialStreamFailuresByTaskId.add(partialStreamFailureKey)
298+
await task.finalizePartialToolAsk(partialMessage)
291299
await task.diffViewProvider.reset()
292300
}
293301
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ describe("writeToFileTool", () => {
128128
return content
129129
})
130130

131+
mockCline.taskId = "task-1"
132+
mockCline.instanceId = "instance-1"
131133
mockCline.cwd = "/"
132134
mockCline.consecutiveMistakeCount = 0
133135
mockCline.didEditFile = false
@@ -562,6 +564,39 @@ describe("writeToFileTool", () => {
562564
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
563565
})
564566

567+
it("does not reset consecutive mistake count when directory creation fails", async () => {
568+
mockCline.consecutiveMistakeCount = 3
569+
mockedCreateDirectoriesForFile.mockRejectedValue(
570+
Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }),
571+
)
572+
573+
await executeWriteFileTool({}, { fileExists: false })
574+
575+
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
576+
expect(mockCline.consecutiveMistakeCount).toBe(3)
577+
})
578+
579+
it("keeps partial stream failures isolated per task", async () => {
580+
mockCline.diffViewProvider.open.mockRejectedValueOnce(
581+
Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }),
582+
)
583+
584+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
585+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
586+
expect(mockCline.ask).toHaveBeenCalledTimes(1)
587+
588+
mockCline.taskId = "task-2"
589+
mockCline.instanceId = "instance-2"
590+
mockCline.diffViewProvider.open.mockResolvedValue(undefined)
591+
mockCline.diffViewProvider.update.mockResolvedValue(undefined)
592+
mockCline.diffViewProvider.editType = undefined
593+
594+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
595+
596+
expect(mockCline.ask).toHaveBeenCalledTimes(2)
597+
expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2)
598+
})
599+
565600
it.skipIf(process.platform === "win32")(
566601
"EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called",
567602
async () => {

0 commit comments

Comments
 (0)