fix(write-to-file): address partial filesystem error review - #1066
fix(write-to-file): address partial filesystem error review#1066easonLiangWorldedtech wants to merge 6 commits into
Conversation
…oo-Code-Org#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
…_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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPartial tool cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WriteToFileTool
participant DiffViewProvider
participant Task
participant ErrorHandler
WriteToFileTool->>DiffViewProvider: Stream diff open/update
DiffViewProvider-->>WriteToFileTool: Filesystem failure
WriteToFileTool->>Task: finalizePartialToolAsk(text?)
WriteToFileTool->>DiffViewProvider: Reset diff view
WriteToFileTool->>WriteToFileTool: Record task failure and skip later deltas
WriteToFileTool->>ErrorHandler: Handle execute-phase error once
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 276-300: Update the catch block in handlePartial so the
task.diffViewProvider.reset() cleanup is wrapped in a nested try/catch. Swallow
or log any reset failure while preserving the existing
partialStreamFailuresByTaskId marking and finalizePartialToolAsk cleanup,
ensuring no exception escapes handlePartial.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 468c8910-8760-4b70-9c62-a3381b90840b
📒 Files selected for processing (4)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/tools/WriteToFileTool.tssrc/core/tools/__tests__/writeToFileTool.spec.ts
0b837ea to
0966556
Compare
|
Updated the branch with commit Summary of fixes:
Validation run locally:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/tools/WriteToFileTool.ts (1)
221-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuarantee task-state cleanup when diff reset fails.
diffViewProvider.reset()can reject. On the success path that enters the outer catch and falsely reports a completed write as failed; on either path it preventsresetTaskPartialState(task), leaving failure/path entries behind. Suppress/log reset failures and move task-state cleanup into afinally; also clear it before the approval-declined returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/WriteToFileTool.ts` around lines 221 - 235, The write handling flow around diffViewProvider.reset and resetTaskPartialState must always clean task state even when diff reset fails. Suppress or log reset errors, move resetTaskPartialState(task) into a finally block, and ensure it runs before approval-declined returns while preserving successful writes and existing error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 221-235: The write handling flow around diffViewProvider.reset and
resetTaskPartialState must always clean task state even when diff reset fails.
Suppress or log reset errors, move resetTaskPartialState(task) into a finally
block, and ensure it runs before approval-declined returns while preserving
successful writes and existing error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3ccf322-b3be-4471-9c3d-8c7d16c65256
📒 Files selected for processing (5)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/Task.throttle.test.tssrc/core/tools/WriteToFileTool.tssrc/core/tools/__tests__/writeToFileTool.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/tools/tests/writeToFileTool.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/tools/WriteToFileTool.ts (1)
239-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant cleanup: the inner
finallyalready runs before thecatchbody.
resetTaskPartialState(task)executes here on every path, including the throwing one, so the second call in the catch'sfinally(Line 252) is a no-op repeat. A single outertry { ... } catch { ... } finally { this.resetTaskPartialState(task) }expresses the same guarantee with one less nesting level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/WriteToFileTool.ts` around lines 239 - 241, Remove the redundant inner finally cleanup around the WriteToFileTool operation and restructure the surrounding try/catch so a single outer finally calls resetTaskPartialState(task). Preserve the existing catch behavior while ensuring resetTaskPartialState executes exactly once on every path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 247-253: Guard both Task.finalizePartialToolAsk calls in
src/core/tools/WriteToFileTool.ts at lines 247-253 and 335-336 with catch
handlers that log failures without rethrowing. Ensure the surrounding cleanup
continues to handle the original write error, reset the diff view via
resetDiffViewAfterWrite, and preserve handlePartial’s no-rethrow contract; apply
the same protection to the overload receiving partialMessage.
---
Nitpick comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 239-241: Remove the redundant inner finally cleanup around the
WriteToFileTool operation and restructure the surrounding try/catch so a single
outer finally calls resetTaskPartialState(task). Preserve the existing catch
behavior while ensuring resetTaskPartialState executes exactly once on every
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9ba64ca-936d-4cda-a1bd-549c328f5067
📒 Files selected for processing (2)
src/core/tools/WriteToFileTool.tssrc/core/tools/__tests__/writeToFileTool.spec.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/tools/WriteToFileTool.ts (1)
29-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClean up per-task
write_to_filepartial state on task abort.
handlePartial()can populatepartialStreamFailuresByTaskIdand update path stabilization state beforeexecute()completes. A cancelled task aborts before its finalize path, so the task-keyed entries can remain on the singleton tool and grow over a session. Add teardown for these task keys, for example fromTask.dispose()/abort hooks or a matching abort handler, so abandonedwrite_to_filestreams do not leak state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/WriteToFileTool.ts` around lines 29 - 58, Add abort/disposal cleanup for the per-task state maintained by WriteToFileTool, invoking resetTaskPartialState(task) when a task is cancelled before execute() finalization. Ensure both partialStreamFailuresByTaskId and lastSeenPartialPathByTaskId entries are removed for abandoned streams, while preserving normal completion behavior.
🧹 Nitpick comments (1)
src/core/tools/__tests__/writeToFileTool.spec.ts (1)
613-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
console.errorspy safely against assertion failures.
consoleErrorSpy.mockRestore()is only reached if every precedingexpect(...)passes. If any assertion throws first, the spy leaks into later tests, silently swallowingconsole.erroroutput and potentially masking unrelated failures for the rest of the run.♻️ Suggested fix
it("continues execute error cleanup when finalizing partial ask fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - mockedCreateDirectoriesForFile.mockRejectedValue( - Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), - ) - mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) - - await executeWriteFileTool({}, { fileExists: false }) - - expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() - expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) - expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() - expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error finalizing write_to_file partial tool ask:", - expect.any(Error), - ) - - consoleErrorSpy.mockRestore() + try { + mockedCreateDirectoriesForFile.mockRejectedValue( + Object.assign(new Error("EACCES: permission denied, mkdir '/ro'"), { code: "EACCES" }), + ) + mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed")) + + await executeWriteFileTool({}, { fileExists: false }) + + expect(mockCline.finalizePartialToolAsk).toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Error finalizing write_to_file partial tool ask:", + expect.any(Error), + ) + } finally { + consoleErrorSpy.mockRestore() + } })Alternatively, add a global
afterEach(() => vi.restoreAllMocks())if one doesn't already exist.Also applies to: 675-694
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/writeToFileTool.spec.ts` around lines 613 - 631, Ensure the console.error spy in the “continues execute error cleanup when finalizing partial ask fails” test is restored even when an assertion fails by using guaranteed cleanup such as a try/finally block. Apply the same safe restoration to the related test around the second referenced section, or use an existing suite-wide afterEach cleanup if appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 29-58: Add abort/disposal cleanup for the per-task state
maintained by WriteToFileTool, invoking resetTaskPartialState(task) when a task
is cancelled before execute() finalization. Ensure both
partialStreamFailuresByTaskId and lastSeenPartialPathByTaskId entries are
removed for abandoned streams, while preserving normal completion behavior.
---
Nitpick comments:
In `@src/core/tools/__tests__/writeToFileTool.spec.ts`:
- Around line 613-631: Ensure the console.error spy in the “continues execute
error cleanup when finalizing partial ask fails” test is restored even when an
assertion fails by using guaranteed cleanup such as a try/finally block. Apply
the same safe restoration to the related test around the second referenced
section, or use an existing suite-wide afterEach cleanup if appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9a6f719-f4f2-455a-bc40-296111f9c54e
📒 Files selected for processing (2)
src/core/tools/WriteToFileTool.tssrc/core/tools/__tests__/writeToFileTool.spec.ts
github-actions
Bot
added
the
awaiting-review
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/tools/WriteToFileTool.ts (1)
111-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing-param early returns bypass the new per-task cleanup and error-safe reset.
These two guard clauses return before the
tryblock, so they skip both:
- the new
resetTaskPartialState(task)cleanup that thefinallyblock otherwise always performs, leaving stalelastSeenPartialPathByTaskId/partialStreamFailuresByTaskIdentries and a retained abort listener (andTaskreference) for this task until it eventually aborts or a globalresetPartialState()runs; and- the new
resetDiffViewAfterWritewrapper, calling the rawtask.diffViewProvider.reset()instead — reintroducing the unguarded-reset risk fixed elsewhere in this PR.If a prior partial delta already registered abort cleanup / seeded path-stabilization state for this task, a subsequent malformed block (missing
path/content) leaves that state stale for the next write_to_file call in the same task.🛠️ Proposed fix
if (!relPath) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "path")) - await task.diffViewProvider.reset() + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return } if (newContent === undefined) { task.consecutiveMistakeCount++ task.recordToolError("write_to_file") pushToolResult(await task.sayAndCreateMissingParamError("write_to_file", "content")) - await task.diffViewProvider.reset() + await this.resetDiffViewAfterWrite(task) + this.resetTaskPartialState(task) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/WriteToFileTool.ts` around lines 111 - 125, Update the missing-parameter guards in the write_to_file flow to perform the same per-task cleanup as the try/finally path by invoking resetTaskPartialState(task), and replace direct task.diffViewProvider.reset() calls with the resetDiffViewAfterWrite wrapper. Preserve the existing error recording, missing-parameter result, and early-return behavior for both relPath and newContent validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/tools/WriteToFileTool.ts`:
- Around line 111-125: Update the missing-parameter guards in the write_to_file
flow to perform the same per-task cleanup as the try/finally path by invoking
resetTaskPartialState(task), and replace direct task.diffViewProvider.reset()
calls with the resetDiffViewAfterWrite wrapper. Preserve the existing error
recording, missing-parameter result, and early-return behavior for both relPath
and newContent validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57700e7d-e297-4257-a748-c7d81ed9ecde
📒 Files selected for processing (2)
src/core/tools/WriteToFileTool.tssrc/core/tools/__tests__/writeToFileTool.spec.ts
3 participants
Summary
This PR addresses all review comments from PR #727 regarding the
write_to_filefilesystem error handling fix.Closes: #703 #727
Changes
1. Task.ts —
finalizePartialToolAsk()improvementsat(-1)to find the last message, now searches for any partial tool ask matching the expected pattern (type === "ask", ask === "tool", partial === true). This prevents issues where async gaps betweentask.ask("tool", ...)and the catch block could insert new messages.partial=falsethrough the proper persistence path (not just webview update), ensuring state survives reload/resume — addressing CodeRabbit's actionable comment.updateClineMessagecall in try/catch so if it fails, we don't interrupt the error flow.2. WriteToFileTool.ts — Review fixes
consecutiveMistakeCount = 0to aftercreateDirectoriesForFilesucceeds, preventing permanent read-only paths from zeroing the runaway-loop guard on every EROFS attempt.partialStreamFailedfrom a singleton instance flag to a per-taskId map (Map<string, boolean>), preventing cross-task interference when multiple sessions run concurrently.finalizePartialToolAsk()call in the catch block to use the improved search logic that finds the correct partial ask by type and text pattern.3. writeToFileTool.spec.ts — New regression tests
4. Task.spec.ts — Thin layer test for
finalizePartialToolAsk()Task.finalizePartialToolAsk()correctly finds and finalizes partial tool asks even when they're not the last message in the array.updateClineMessageis called.Review Comments Addressed
finalizePartialToolAskupdateClineMessagepartialStreamFailedhas cross-task riskat(-1)could find wrong message in async gappartial=falsemutationTask.finalizePartialToolAsk()Testing
Related
Summary by CodeRabbit
Summary by CodeRabbit