Skip to content

Commit 27465f8

Browse files
fix(write-to-file): tear down per-task stream state on parse failure and dispose (split 5/6 of #1066, unit U6b)
Tool-side half of #1932, stacked on unit U6a (3c5036a): the onParameterParseFailure teardown seam on BaseTool and the rewired native-args path, the per-task partial stream state and its cleanup primitives on WriteToFileTool released on every execute() exit, and the dispose-time clearTaskState handoff in ClineProvider. BaseTool.ts and ClineProvider.ts arrive here rather than on this branch's cumulative diff because the split takes them out of #1932; plan and measured widths in #1937 (comment). writeToFileTool.spec.ts drops two no-explicit-any suppressions (5 -> 3) rather than renewing them.
1 parent 3c5036a commit 27465f8

7 files changed

Lines changed: 2496 additions & 70 deletions

‎src/__tests__/removeClineFromStack-delegation.spec.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
// npx vitest run __tests__/removeClineFromStack-delegation.spec.ts
22

33
import { describe, it, expect, vi } from "vitest"
4+
import { RooCodeEventName } from "@roo-code/types"
45
import { ClineProvider } from "../core/webview/ClineProvider"
56
import { TaskRegistry } from "../core/task/TaskRegistry"
67
import { PendingActionSettlementError, type Task } from "../core/task/Task"
78
import { makeProviderStub } from "./helpers/provider-stub"
9+
import { writeToFileTool } from "../core/tools/WriteToFileTool"
810

911
type PrivateClineProviderMethods = {
1012
removeClineFromStack: (this: ClineProvider) => ReturnType<ClineProvider["removeClineFromStack"]>
@@ -202,6 +204,53 @@ describe("ClineProvider failed history restoration cleanup", () => {
202204
expect(task.dispose).toHaveBeenCalledOnce()
203205
})
204206

207+
it("releases the tool's per-task state before a directly disposed task loses its listeners", async () => {
208+
const key = "failed-history-task.inst-1"
209+
const order: string[] = []
210+
// The listener identity matters: the release must detach THIS function, so the
211+
// fixture binds it to a variable instead of an anonymous inline no-op.
212+
const abortCleanup = vi.fn()
213+
const task = {
214+
taskId: "failed-history-task",
215+
instanceId: "inst-1",
216+
emit: vi.fn(),
217+
once: vi.fn(),
218+
off: vi.fn(),
219+
dispose: vi.fn().mockImplementation(() => {
220+
// Dispose removes every listener, so this is the last moment the abort
221+
// cleanup could still have run; the entry must already be gone here.
222+
order.push(writeToFileTool["taskPartialStreamState"].has(key) ? "state-retained" : "state-cleared")
223+
order.push("dispose")
224+
return Promise.resolve()
225+
}),
226+
} as unknown as Task
227+
// Fixture: the state entry the tool would have created during a partial stream.
228+
writeToFileTool["taskPartialStreamState"].set(key, {
229+
lastSeenPartialPath: undefined,
230+
streamFailed: false,
231+
differentPathStream: false,
232+
streamError: undefined,
233+
task,
234+
abortCleanup,
235+
})
236+
const taskEventListeners = new Map([[task, [vi.fn()]]])
237+
const taskRegistry = new TaskRegistry()
238+
taskRegistry.push(task)
239+
const provider = { taskRegistry, taskEventListeners, log: vi.fn() } as unknown as ClineProvider
240+
241+
await privateClineProvider.cleanupFailedHistoryTask.call(
242+
provider,
243+
task,
244+
new PendingActionSettlementError("settlement failed"),
245+
)
246+
247+
expect(order).toEqual(["state-cleared", "dispose"])
248+
expect(writeToFileTool["taskPartialStreamState"].size).toBe(0)
249+
// Dropping the map entry is not enough: the registered TaskAborted listener must
250+
// be detached with the same function reference it was registered with.
251+
expect(task.off).toHaveBeenCalledWith(RooCodeEventName.TaskAborted, abortCleanup)
252+
})
253+
205254
it("keeps the task active after an unrelated history resume failure", async () => {
206255
const cleanupListener = vi.fn()
207256
const task = {

‎src/core/tools/BaseTool.ts‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,23 @@ export abstract class BaseTool<TName extends ToolName> {
155155
throw new Error("Tool call is missing native arguments (nativeArgs).")
156156
}
157157
} catch (error) {
158-
console.error(`Error parsing parameters:`, error)
159-
const errorMessage = `Failed to parse ${this.name} parameters: ${error instanceof Error ? error.message : String(error)}`
160-
await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage))
158+
const parseError = error instanceof Error ? error : new Error(String(error))
159+
console.error(`Error parsing parameters:`, parseError)
160+
// Final args could not be parsed (e.g. the model's tool call was truncated
161+
// mid-JSON by the output token limit), so execute() will never run. If a
162+
// streaming delta already opened a partial "tool" ask (partial: true),
163+
// finalize it here or the webview spinner stays stuck indefinitely.
164+
await task.finalizePartialToolAsk().catch((finalizeError) => {
165+
console.error(`Error finalizing ${this.name} partial tool ask:`, finalizeError)
166+
})
167+
// execute() never runs on this path, so tools that keep per-task state
168+
// outside execute() (streaming failure marks, abort listeners) get their
169+
// one remaining teardown boundary here.
170+
const reportedStreamingFailure = await this.onParameterParseFailure(task, callbacks, parseError)
171+
if (!reportedStreamingFailure) {
172+
const errorMessage = `Failed to parse ${this.name} parameters: ${parseError.message}`
173+
await callbacks.handleError(`parsing ${this.name} args`, new Error(errorMessage))
174+
}
161175
// Note: handleError already emits a tool_result via formatResponse.toolError in the caller.
162176
// Do NOT call pushToolResult here to avoid duplicate tool_result payloads.
163177
return
@@ -166,4 +180,27 @@ export abstract class BaseTool<TName extends ToolName> {
166180
// Execute with typed parameters
167181
await this.execute(params, task, callbacks)
168182
}
183+
184+
/**
185+
* Teardown boundary for the native-argument parse-failure path in handle().
186+
*
187+
* When nativeArgs are missing or malformed, execute() never runs, so per-task
188+
* state a tool registered outside execute() (streaming failure marks, abort
189+
* listeners) is never torn down there. Streaming tools override this to tear
190+
* that state down and, when a streaming delta already failed, to report the
191+
* captured streaming error instead of the generic parse error.
192+
*
193+
* @param _task - Task instance
194+
* @param _callbacks - Tool execution callbacks
195+
* @param _parseError - The native-argument parse error
196+
* @returns true when the override already reported the failure to the user,
197+
* so handle() suppresses the generic parse error
198+
*/
199+
protected async onParameterParseFailure(
200+
_task: Task,
201+
_callbacks: ToolCallbacks,
202+
_parseError: Error,
203+
): Promise<boolean> {
204+
return false
205+
}
169206
}

0 commit comments

Comments
 (0)