Skip to content

Commit 0966556

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

5 files changed

Lines changed: 227 additions & 27 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+
await 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/task/__tests__/Task.throttle.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,12 @@ describe("Task token usage throttling", () => {
6464
let mockProvider: any
6565
let mockApiConfiguration: ProviderSettings
6666
let task: Task
67+
let consoleLogSpy: ReturnType<typeof vi.spyOn>
6768

6869
beforeEach(() => {
6970
// Reset all mocks
7071
vi.clearAllMocks()
72+
consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {})
7173
vi.useFakeTimers()
7274

7375
// Mock provider
@@ -101,6 +103,7 @@ describe("Task token usage throttling", () => {
101103
if (task && !task.abort) {
102104
task.dispose()
103105
}
106+
consoleLogSpy.mockRestore()
104107
})
105108

106109
test("should emit TaskTokenUsageUpdated immediately on first change", async () => {

src/core/tools/WriteToFileTool.ts

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,22 +27,47 @@ 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+
/**
37+
* Tracks partial path stabilization by task id. The tool is a singleton, so using the
38+
* BaseTool singleton path state lets concurrent tasks incorrectly stabilize each other.
39+
*/
40+
private lastSeenPartialPathByTaskId = new Map<string, string | undefined>()
41+
42+
private getPartialStreamFailureKey(task: Task): string {
43+
return `${task.taskId}.${task.instanceId}`
44+
}
45+
46+
private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean {
47+
const key = this.getPartialStreamFailureKey(task)
48+
const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key)
49+
const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath
50+
this.lastSeenPartialPathByTaskId.set(key, partialPath)
51+
return pathHasStabilized && !!partialPath
52+
}
53+
54+
private resetTaskPartialState(task: Task): void {
55+
const key = this.getPartialStreamFailureKey(task)
56+
this.lastSeenPartialPathByTaskId.delete(key)
57+
this.partialStreamFailuresByTaskId.delete(key)
58+
}
3659

3760
override resetPartialState(): void {
3861
super.resetPartialState()
39-
this.partialStreamFailed = false
62+
this.partialStreamFailuresByTaskId.clear()
63+
this.lastSeenPartialPathByTaskId.clear()
4064
}
4165

4266
async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
4367
const { pushToolResult, handleError, askApproval } = callbacks
4468
const relPath = params.path
4569
let newContent = params.content
70+
const partialStreamFailureKey = this.getPartialStreamFailureKey(task)
4671

4772
if (!relPath) {
4873
task.consecutiveMistakeCount++
@@ -104,15 +129,15 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
104129
}
105130

106131
try {
107-
task.consecutiveMistakeCount = 0
108-
109132
// Create parent directories for new files inside the try block so filesystem
110133
// errors (EROFS, EACCES, etc.) route through handleError with proper cleanup
111134
// and consecutive-mistake counting, rather than escaping unhandled.
112135
if (!fileExists) {
113136
await createDirectoriesForFile(absolutePath)
114137
}
115138

139+
task.consecutiveMistakeCount = 0
140+
116141
const provider = task.providerRef.deref()
117142
const state = await provider?.getState()
118143
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
@@ -194,7 +219,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
194219
pushToolResult(message)
195220

196221
await task.diffViewProvider.reset()
197-
this.resetPartialState()
222+
this.resetTaskPartialState(task)
198223

199224
task.processQueuedMessages()
200225

@@ -207,7 +232,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
207232
await task.finalizePartialToolAsk()
208233
await handleError("writing file", error as Error)
209234
await task.diffViewProvider.reset()
210-
this.resetPartialState()
235+
this.resetTaskPartialState(task)
211236
return
212237
}
213238
}
@@ -216,15 +241,17 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
216241
const relPath: string | undefined = block.params.path
217242
const newContent: string | undefined = block.params.content
218243

219-
// A prior streaming delta for this invocation already hit a fatal filesystem error.
244+
const partialStreamFailureKey = this.getPartialStreamFailureKey(task)
245+
246+
// A prior streaming delta for this task already hit a fatal filesystem error.
220247
// Skip further streaming work so we don't create a new partial tool message on every
221248
// subsequent delta. execute() will report the error once when the block completes.
222-
if (this.partialStreamFailed) {
249+
if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) {
223250
return
224251
}
225252

226253
// Wait for path to stabilize before showing UI (prevents truncated paths)
227-
if (!this.hasPathStabilized(relPath) || newContent === undefined) {
254+
if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) {
228255
return
229256
}
230257

@@ -286,9 +313,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
286313
console.error(`Error streaming write_to_file diff view:`, error)
287314
// Mark the stream as failed so later deltas don't re-attempt and spawn a new
288315
// partial tool message each time.
289-
this.partialStreamFailed = true
290-
await task.finalizePartialToolAsk()
291-
await task.diffViewProvider.reset()
316+
this.partialStreamFailuresByTaskId.add(partialStreamFailureKey)
317+
await task.finalizePartialToolAsk(partialMessage)
318+
await task.diffViewProvider.reset().catch((resetError) => {
319+
console.error("Error resetting write_to_file diff view after partial failure:", resetError)
320+
})
292321
}
293322
}
294323
}

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

Lines changed: 78 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
@@ -421,6 +423,25 @@ describe("writeToFileTool", () => {
421423
expect(mockCline.diffViewProvider.open).toHaveBeenCalledWith(testFilePath)
422424
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(testContent, false)
423425
})
426+
it("does not share path stabilization between tasks with the same path", async () => {
427+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
428+
expect(mockCline.ask).not.toHaveBeenCalled()
429+
430+
mockCline.taskId = "task-2"
431+
mockCline.instanceId = "instance-2"
432+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
433+
expect(mockCline.ask).not.toHaveBeenCalled()
434+
435+
mockCline.taskId = "task-1"
436+
mockCline.instanceId = "instance-1"
437+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
438+
expect(mockCline.ask).toHaveBeenCalledTimes(1)
439+
440+
mockCline.taskId = "task-2"
441+
mockCline.instanceId = "instance-2"
442+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
443+
expect(mockCline.ask).toHaveBeenCalledTimes(2)
444+
})
424445
})
425446

426447
describe("user interaction", () => {
@@ -562,6 +583,63 @@ describe("writeToFileTool", () => {
562583
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
563584
})
564585

586+
it("does not reset consecutive mistake count when directory creation fails", async () => {
587+
mockCline.consecutiveMistakeCount = 3
588+
mockedCreateDirectoriesForFile.mockRejectedValue(
589+
Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }),
590+
)
591+
592+
await executeWriteFileTool({}, { fileExists: false })
593+
594+
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
595+
expect(mockCline.consecutiveMistakeCount).toBe(3)
596+
})
597+
598+
it("keeps partial stream failures isolated per task", async () => {
599+
mockCline.diffViewProvider.open.mockRejectedValueOnce(
600+
Object.assign(new Error("EROFS: read-only file system, mkdir '/task-a'"), { code: "EROFS" }),
601+
)
602+
603+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
604+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
605+
expect(mockCline.ask).toHaveBeenCalledTimes(1)
606+
607+
mockCline.taskId = "task-2"
608+
mockCline.instanceId = "instance-2"
609+
mockCline.diffViewProvider.open.mockResolvedValue(undefined)
610+
mockCline.diffViewProvider.update.mockResolvedValue(undefined)
611+
mockCline.diffViewProvider.editType = undefined
612+
613+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
614+
expect(mockCline.ask).toHaveBeenCalledTimes(1)
615+
616+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
617+
618+
expect(mockCline.ask).toHaveBeenCalledTimes(2)
619+
expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(2)
620+
})
621+
622+
it("swallows diff view reset errors during partial failure cleanup", async () => {
623+
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
624+
mockCline.diffViewProvider.open.mockRejectedValue(
625+
Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }),
626+
)
627+
mockCline.diffViewProvider.reset.mockRejectedValue(new Error("reset failed"))
628+
629+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
630+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
631+
632+
expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled()
633+
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
634+
expect(mockHandleError).not.toHaveBeenCalled()
635+
expect(consoleErrorSpy).toHaveBeenCalledWith(
636+
"Error resetting write_to_file diff view after partial failure:",
637+
expect.any(Error),
638+
)
639+
640+
consoleErrorSpy.mockRestore()
641+
})
642+
565643
it.skipIf(process.platform === "win32")(
566644
"EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called",
567645
async () => {

0 commit comments

Comments
 (0)