From e2a03d9492573a733f5fe65ef70ad3edb6ad704a Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 00:38:29 +0800 Subject: [PATCH] feat(tools): onParameterParseFailure teardown boundary --- src/core/tools/BaseTool.ts | 39 +++- src/core/tools/WriteToFileTool.ts | 22 ++ .../tools/__tests__/writeToFileTool.spec.ts | 192 ++++++++++++++++++ 3 files changed, 250 insertions(+), 3 deletions(-) diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 83a733c7b0..dc16a9d81d 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -155,9 +155,23 @@ export abstract class BaseTool { 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 { // 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 { + return false + } } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 267c728c27..ba25a9f2c0 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/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 { + 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()) { diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index f4677277d7..dddd139011 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/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() + } + })