Skip to content

Commit 0575a35

Browse files
awschmedereasonliang28
authored andcommitted
fix: prevent agent loop stall from WriteToFileTool filesystem errors (#703)
- Remove unguarded createDirectoriesForFile call from handlePartial; the call was a redundant optimization (execute() already creates dirs before open()) and its unguarded throw caused the partial-block advancement gate in presentAssistantMessage to be skipped, permanently stalling the agent loop - Move createDirectoriesForFile in execute() inside the try block so EROFS/ EACCES errors route through handleError with diffViewProvider.reset() cleanup and consecutive-mistake counting, rather than escaping unhandled - Add regression tests covering both failure paths
1 parent 569b43d commit 0575a35

2 files changed

Lines changed: 57 additions & 16 deletions

File tree

src/core/tools/WriteToFileTool.ts

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
6767
task.diffViewProvider.editType = fileExists ? "modify" : "create"
6868
}
6969

70-
// Create parent directories early for new files to prevent ENOENT errors
71-
// in subsequent operations (e.g., diffViewProvider.open, fs.readFile)
72-
if (!fileExists) {
73-
await createDirectoriesForFile(absolutePath)
74-
}
75-
7670
if (newContent.startsWith("```")) {
7771
newContent = newContent.split("\n").slice(1).join("\n")
7872
}
@@ -99,6 +93,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
9993
try {
10094
task.consecutiveMistakeCount = 0
10195

96+
// Create parent directories for new files inside the try block so filesystem
97+
// errors (EROFS, EACCES, etc.) route through handleError with proper cleanup
98+
// and consecutive-mistake counting, rather than escaping unhandled.
99+
if (!fileExists) {
100+
await createDirectoriesForFile(absolutePath)
101+
}
102+
102103
const provider = task.providerRef.deref()
103104
const state = await provider?.getState()
104105
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
@@ -224,12 +225,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
224225
task.diffViewProvider.editType = fileExists ? "modify" : "create"
225226
}
226227

227-
// Create parent directories early for new files to prevent ENOENT errors
228-
// in subsequent operations (e.g., diffViewProvider.open)
229-
if (!fileExists) {
230-
await createDirectoriesForFile(absolutePath)
231-
}
232-
233228
const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false
234229
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
235230

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

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,16 @@ describe("writeToFileTool", () => {
287287
)
288288

289289
it.skipIf(process.platform === "win32")(
290-
"creates parent directories when path has stabilized (partial)",
290+
"does not create directories in handlePartial -- only execute() creates them",
291291
async () => {
292-
// First call - path not yet stabilized
292+
// First call - path not yet stabilized, early return
293293
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
294294
expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled()
295295

296-
// Second call with same path - path is now stabilized
296+
// Second call with same path - path stabilized, handlePartial runs but
297+
// must NOT call createDirectoriesForFile (directory creation belongs in execute)
297298
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
298-
expect(mockedCreateDirectoriesForFile).toHaveBeenCalledWith(absoluteFilePath)
299+
expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled()
299300
},
300301
)
301302

@@ -471,5 +472,50 @@ describe("writeToFileTool", () => {
471472
await executeWriteFileTool({}, { isPartial: true })
472473
expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error))
473474
})
475+
476+
it.skipIf(process.platform === "win32")(
477+
"EROFS in handlePartial does not stall agent loop -- createDirectoriesForFile is not called",
478+
async () => {
479+
// Regression test: before the fix, createDirectoriesForFile was called in handlePartial
480+
// with no .catch() guard. An EROFS throw escaped to BaseTool.handle(), which called
481+
// handleError but did not set didRejectTool/didAlreadyUseTool, so the advancement gate
482+
// in presentAssistantMessage was never reached and the agent loop stalled permanently.
483+
// After the fix the call is removed entirely -- handlePartial never touches the filesystem.
484+
mockedCreateDirectoriesForFile.mockRejectedValue(
485+
Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }),
486+
)
487+
488+
// First call -- path not yet stabilized, returns early
489+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
490+
expect(mockHandleError).not.toHaveBeenCalled()
491+
492+
// Second call -- path stabilized; createDirectoriesForFile must NOT be called from
493+
// handlePartial, so the mock rejection must not trigger and handleError must not be called
494+
await executeWriteFileTool({}, { fileExists: false, isPartial: true })
495+
expect(mockedCreateDirectoriesForFile).not.toHaveBeenCalled()
496+
expect(mockHandleError).not.toHaveBeenCalled()
497+
},
498+
)
499+
500+
it.skipIf(process.platform === "win32")(
501+
"EROFS in execute() routes through handleError with cleanup rather than escaping unhandled",
502+
async () => {
503+
// Regression test: before the fix, createDirectoriesForFile in execute() sat outside
504+
// the try block (lines 70-74), so an EROFS error escaped the catch at line 188 entirely.
505+
// After the fix the call is inside the try block, so filesystem errors are caught and
506+
// routed through handleError with proper diffViewProvider.reset() cleanup.
507+
mockedCreateDirectoriesForFile.mockRejectedValue(
508+
Object.assign(new Error("EROFS: read-only file system, mkdir '/scratch'"), { code: "EROFS" }),
509+
)
510+
511+
await executeWriteFileTool({}, { fileExists: false })
512+
513+
expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error))
514+
expect(mockCline.diffViewProvider.reset).toHaveBeenCalled()
515+
// The tool must not have proceeded to open or save
516+
expect(mockCline.diffViewProvider.open).not.toHaveBeenCalled()
517+
expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled()
518+
},
519+
)
474520
})
475521
})

0 commit comments

Comments
 (0)