fix(write-to-file): run diff cleanup when handleError rejects (split 5/6 of #1066)

src/__tests__/removeClineFromStack-delegation.spec.ts

@@ -1,10 +1,12 @@
 // npx vitest run __tests__/removeClineFromStack-delegation.spec.ts
 
 import { describe, it, expect, vi } from "vitest"
+import { RooCodeEventName } from "@roo-code/types"
 import { ClineProvider } from "../core/webview/ClineProvider"
 import { TaskRegistry } from "../core/task/TaskRegistry"
 import { PendingActionSettlementError, type Task } from "../core/task/Task"
 import { makeProviderStub } from "./helpers/provider-stub"
+import { writeToFileTool } from "../core/tools/WriteToFileTool"
 
 type PrivateClineProviderMethods = {
 	removeClineFromStack: (this: ClineProvider) => ReturnType<ClineProvider["removeClineFromStack"]>
@@ -202,6 +204,52 @@ describe("ClineProvider failed history restoration cleanup", () => {
 		expect(task.dispose).toHaveBeenCalledOnce()
 	})
 
+	it("releases the tool's per-task state before a directly disposed task loses its listeners", async () => {
+		const key = "failed-history-task.inst-1"
+		const order: string[] = []
+		// The listener identity matters: the release must detach THIS function, so the
+		// fixture binds it to a variable instead of an anonymous inline no-op.
+		const abortCleanup = vi.fn()
+		const task = {
+			taskId: "failed-history-task",
+			instanceId: "inst-1",
+			emit: vi.fn(),
+			once: vi.fn(),
+			off: vi.fn(),
+			dispose: vi.fn().mockImplementation(() => {
+				// Dispose removes every listener, so this is the last moment the abort
+				// cleanup could still have run; the entry must already be gone here.
+				order.push(writeToFileTool["taskPartialStreamState"].has(key) ? "state-retained" : "state-cleared")
+				order.push("dispose")
+				return Promise.resolve()
+			}),
+		} as unknown as Task
+		// Fixture: the state entry the tool would have created during a partial stream.
+		writeToFileTool["taskPartialStreamState"].set(key, {
+			lastSeenPartialPath: undefined,
+			streamFailed: false,
+			streamError: undefined,
+			task,
+			abortCleanup,
+		})
+		const taskEventListeners = new Map([[task, [vi.fn()]]])
+		const taskRegistry = new TaskRegistry()
+		taskRegistry.push(task)
+		const provider = { taskRegistry, taskEventListeners, log: vi.fn() } as unknown as ClineProvider
+
+		await privateClineProvider.cleanupFailedHistoryTask.call(
+			provider,
+			task,
+			new PendingActionSettlementError("settlement failed"),
+		)
+
+		expect(order).toEqual(["state-cleared", "dispose"])
+		expect(writeToFileTool["taskPartialStreamState"].size).toBe(0)
+		// Dropping the map entry is not enough: the registered TaskAborted listener must
+		// be detached with the same function reference it was registered with.
+		expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup)
+	})
+
 	it("keeps the task active after an unrelated history resume failure", async () => {
 		const cleanupListener = vi.fn()
 		const task = {

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,27 @@ 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
+	}
 }