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

src/__tests__/removeClineFromStack-delegation.spec.ts

@@ -5,6 +5,7 @@ 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 MockTask = Pick<Task, "taskId" | "instanceId"> &
 	Partial<Pick<Task, "parentTaskId" | "abort" | "abandoned">> & {
@@ -208,6 +209,46 @@ 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[] = []
+		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)
+	})
+
 	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,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
+	}
 }