feat(tools): onParameterParseFailure teardown boundary (split 4/6 of #1066)

src/core/tools/BaseTool.ts

@@ -155,9 +155,23 @@ export abstract class BaseTool<TName extends ToolName> {
 				throw new Error("Tool call is missing native arguments (nativeArgs).")
 			}
 		} catch (error) {
-			console.error(`Error parsing parameters:`, error)
-			const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}`
-			await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage))
+			const parseError = error instanceof Error ? error : new Error(String(error))
+			console.error(`Error parsing parameters:`, parseError)
+			// Final args could not be parsed (e.g. the model's tool call was truncated
+			// mid-JSON by the output token limit), so execute() will never run. If a
+			// streaming delta already opened a partial "tool" ask (partial: true),
+			// finalize it here or the webview spinner stays stuck indefinitely.
+			await task.finalizePartialToolAsk().catch((finalizeError) => {
+				console.error(`Error finalizing ${this.name} partial tool ask:`, finalizeError)
+			})
+			// execute() never runs on this path, so tools that keep per-task state
+			// outside execute() (streaming failure marks, abort listeners) get their
+			// one remaining teardown boundary here.
+			const reportedStreamingFailure = await this.onParameterParseFailure(task, callbacks, parseError)
+			if (!reportedStreamingFailure) {
+				const errorMessage = `Failed to parse ${this.name} parameters: ${parseError.message}`
+				await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage))
+			}
 			// Note: handleError already emits a tool_result via formatResponse.toolError in the caller.
 			// Do NOT call pushToolResult here to avoid duplicate tool_result payloads.
 			return
@@ -166,4 +180,23 @@ export abstract class BaseTool<TName extends ToolName> {
 		// Execute with typed parameters
 		await this.execute(params, task, callbacks)
 	}
+
+	/**
+	 * Teardown boundary for the native-argument parse-failure path in handle().
+	 *
+	 * When nativeArgs are missing or malformed, execute() never runs, so per-task
+	 * state a tool registered outside execute() (streaming failure marks, abort
+	 * listeners) is never torn down there. Streaming tools override this to tear
+	 * that state down and, when a streaming delta already failed, to report the
+	 * captured streaming error instead of the generic parse error.
+	 *
+	 * @param task - Task instance
+	 * @param callbacks - Tool execution callbacks
+	 * @param parseError - The native-argument parse error
+	 * @returns true when the override already reported the failure to the user,
+	 *   so handle() suppresses the generic parse error
+	 */
+	protected async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise<boolean> {
+		return false
+	}
 }

src/core/tools/WriteToFileTool.ts

@@ -174,6 +174,28 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
 	 * "writing file" context execute()'s catch uses, and suppress the incidental
 	 * parse error.
 	 */
+	override async onParameterParseFailure(task: Task, callbacks: ToolCallbacks, parseError: Error): Promise<boolean> {
+		const state = this.taskPartialStreamState.get(this.getPartialStreamFailureKey(task))
+		if (!state) {
+			return false
+		}
+		this.resetTaskPartialState(task)
+		// Streaming may have opened the diff view with unapproved partial content.
+		// execute() never runs on this path, so its error cleanup (revert + reset)
+		// never fires: restore the document here so a user save cannot persist
+		// content the write never completed (the same invariant the denial and
+		// streaming-failure paths maintain). Both helpers no-op when no view is
+		// open.
+		await this.revertDiffChangesBeforeReset(task)
+		await this.resetDiffViewAfterWrite(task)
+		if (!state.streamError) {
+			return false
+		}
+		void parseError
+		await callbacks.handleError("writing file", state.streamError)
+		return true
+	}
+
 	override resetPartialState(): void {
 		super.resetPartialState()
 		for (const state of this.taskPartialStreamState.values()) {

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

@@ -541,7 +541,123 @@ describe("writeToFileTool", () => {
 			}
 		})
 
+		it("reports the captured streaming error instead of the parse error when the final block fails to parse, and clears the per-task state", async () => {
+			// A streaming delta fails with a filesystem error (streamFailed + streamError are
+			// captured). The final block then arrives without nativeArgs, so execute() never
+			// runs: the parse-failure teardown boundary must report the original filesystem
+			// error under the "writing file" context (not the incidental parse error) and tear
+			// down the per-task state, so the next write_to_file stream in this task is not
+			// blocked by the stale streamFailed guard and no abort listener leaks.
+			const fsError = new Error("EACCES: permission denied")
+			mockCline.diffViewProvider.open.mockRejectedValue(fsError)
+			// Capture the abort listener of the state created by the first deltas: it is the
+			// exact reference that the parse-failure teardown must detach.
+			let abortListener: (() => void) | undefined
+			mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => {
+				if (event === RooCodeEventName.TaskAborted && abortListener === undefined) {
+					abortListener = listener
+				}
+				return mockCline
+			})
+			const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
+			try {
+				await executeWriteFileTool({}, { isPartial: true })
+				await executeWriteFileTool({}, { isPartial: true })
 
+				const toolUse: ToolUse = {
+					type: "tool_use",
+					name: "write_to_file",
+					params: { path: testFilePath, content: testContent },
+					nativeArgs: undefined,
+					partial: false,
+				}
+				await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, {
+					askApproval: mockAskApproval,
+					handleError: mockHandleError,
+					pushToolResult: vi.fn(),
+				})
+
+				expect(mockHandleError).toHaveBeenCalledTimes(1)
+				expect(mockHandleError).toHaveBeenCalledWith("writing file", fsError)
+				expect(mockHandleError).not.toHaveBeenCalledWith(
+					expect.stringContaining("parsing write_to_file"),
+					expect.anything(),
+				)
+				// The parse-failure teardown also restores the diff document: the
+				// streaming failure above already reverted it once (revert + reset),
+				// and the parse path runs the same cleanup again because execute()
+				// never runs on this path.
+				expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(2)
+				expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(2)
+				// Per-task state torn down at this boundary: guard cleared, the exact
+				// registered abort listener detached.
+				expect(writeToFileTool["taskPartialStreamState"].size).toBe(0)
+				expect(abortListener).toBeTypeOf("function")
+				expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener)
+
+				// The next stream for the same task must issue a partial ask again (the stale
+				// streamFailed guard is gone).
+				mockCline.diffViewProvider.open.mockResolvedValue(undefined)
+				await executeWriteFileTool({}, { isPartial: true })
+				await executeWriteFileTool({}, { isPartial: true })
+				expect(mockCline.ask).toHaveBeenCalledTimes(2)
+			} finally {
+				consoleErrorSpy.mockRestore()
+			}
+		})
+
+		it("restores the diff document when the final block fails to parse after successful streaming", async () => {
+			// Streaming opened the diff view with unapproved partial content (open and
+			// update both succeeded, so no streaming error was captured). The final block
+			// then arrives without nativeArgs: execute() never runs, so its error cleanup
+			// never fires. The parse-failure teardown must still restore the document
+			// (revert before reset) or a user save could persist content the write never
+			// completed, and must report the generic parse error (no streaming error to
+			// surface instead).
+			let abortListener: (() => void) | undefined
+			mockCline.once.mockImplementation((event: RooCodeEventName, listener: () => void) => {
+				if (event === RooCodeEventName.TaskAborted && abortListener === undefined) {
+					abortListener = listener
+				}
+				return mockCline
+			})
+			const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
+			try {
+				// Delta 1 - stabilize path; delta 2 - streams the partial content into the
+				// diff view (open + update resolve).
+				await executeWriteFileTool({}, { isPartial: true })
+				await executeWriteFileTool({}, { isPartial: true })
+				expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1)
+				expect(mockCline.diffViewProvider.update).toHaveBeenCalledTimes(1)
+
+				const toolUse: ToolUse = {
+					type: "tool_use",
+					name: "write_to_file",
+					params: { path: testFilePath, content: testContent },
+					nativeArgs: undefined,
+					partial: false,
+				}
+				await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, {
+					askApproval: mockAskApproval,
+					handleError: mockHandleError,
+					pushToolResult: vi.fn(),
+				})
+
+				// No streaming error was captured: the generic parse error is reported.
+				expect(mockHandleError).toHaveBeenCalledTimes(1)
+				expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error))
+				// The diff document is restored by the parse path itself (no streaming
+				// failure happened, so this is the only revert + reset in the test).
+				expect(mockCline.diffViewProvider.revertChanges).toHaveBeenCalledTimes(1)
+				expect(mockCline.diffViewProvider.reset).toHaveBeenCalledTimes(1)
+				// Per-task state torn down: guard cleared, exact listener detached.
+				expect(writeToFileTool["taskPartialStreamState"].size).toBe(0)
+				expect(abortListener).toBeTypeOf("function")
+				expect(mockCline.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortListener)
+			} finally {
+				consoleErrorSpy.mockRestore()
+			}
+		})
 	})
 
 	describe("path stabilization predicate", () => {
@@ -755,7 +871,83 @@ describe("writeToFileTool", () => {
 			expect(mockCline.diffViewProvider.open).toHaveBeenCalledTimes(1)
 		})
 
+		it("finalizes any open partial tool ask when final args cannot be parsed", async () => {
+			// Regression test: a write_to_file block whose final args fail to parse (e.g. the
+			// tool call was truncated mid-JSON by the output token limit) never reaches
+			// execute(). A streaming delta for that block may already have opened a partial
+			// `tool` ask (partial: true) -- BaseTool.handle must finalize it, otherwise the
+			// UI spinner stays stuck even though the parse error bubble was shown.
+			// Delta 1 - stabilize path (no ask yet)
+			await executeWriteFileTool({}, { fileExists: false, isPartial: true })
+			// Delta 2 - path stabilized, partial ask issued once
+			await executeWriteFileTool({}, { fileExists: false, isPartial: true })
+			expect(mockCline.ask).toHaveBeenCalledTimes(1)
+			expect(mockCline.finalizePartialToolAsk).not.toHaveBeenCalled()
 
+			// Final block arrives but its native args cannot be parsed, so execute() is skipped.
+			const toolUse: ToolUse = {
+				type: "tool_use",
+				name: "write_to_file",
+				params: {
+					path: testFilePath,
+					content: testContent,
+				},
+				partial: false,
+			}
+			await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, {
+				askApproval: mockAskApproval,
+				handleError: mockHandleError,
+				pushToolResult: mockPushToolResult,
+			})
+
+			// The parse error is still reported, and the open partial ask is finalized first.
+			// No argument: BaseTool.handle() calls finalizePartialToolAsk() with no text, so a
+			// mutation passing wrong text would leave findLast() unmatched and the spinner
+			// stuck. (toHaveBeenCalledWith(undefined) does not match a no-arg call under
+			// vitest's matcher semantics: [] is not equal to [undefined].)
+			expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1)
+			expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith()
+			expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error))
+		})
+
+		it("continues parse failure cleanup when finalizing the partial ask fails", async () => {
+			// Pins the .catch arm on task.finalizePartialToolAsk() in BaseTool.handle(): when the
+			// final args cannot be parsed and finalizing the open partial ask also fails, the
+			// failure must only be logged so the parse error is still reported to the user.
+			const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
+			try {
+				mockCline.finalizePartialToolAsk.mockRejectedValue(new Error("finalize failed"))
+
+				// Final block arrives but its native args cannot be parsed, so execute() is skipped.
+				const toolUse: ToolUse = {
+					type: "tool_use",
+					name: "write_to_file",
+					params: {
+						path: testFilePath,
+						content: testContent,
+					},
+					partial: false,
+				}
+				await writeToFileTool.handle(mockCline, toolUse as ToolUse<"write_to_file">, {
+					askApproval: mockAskApproval,
+					handleError: mockHandleError,
+					pushToolResult: vi.fn(),
+				})
+
+				// Same no-argument contract as the sibling test above: the finalize call in
+				// BaseTool.handle() carries no text.
+				expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledTimes(1)
+				expect(mockCline.finalizePartialToolAsk).toHaveBeenCalledWith()
+				expect(consoleErrorSpy).toHaveBeenCalledWith(
+					"Error finalizing write_to_file partial tool ask:",
+					expect.any(Error),
+				)
+				// The parse error is still reported despite the failed finalization.
+				expect(mockHandleError).toHaveBeenCalledWith("parsing write_to_file args", expect.any(Error))
+			} finally {
+				consoleErrorSpy.mockRestore()
+			}
+		})