Skip to content

Commit aae67b0

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(assistant-message): route a dropped tool call through the parse-failure teardown
A complete known-tool block whose nativeArgs never parsed was answered with a tool_result inside presentAssistantMessage and dropped before dispatch, so BaseTool.handle() never ran for it. Its parse-failure branch is the one teardown boundary write_to_file has for the per-task state a streaming delta registers outside execute() - the partial tool ask, the abort listener, the streaming failure mark, and any diff preview the delta already opened - so that state outlived the call that failed to parse. Route the dropped call through the same boundary: resolve the tool by name and call onParameterParseFailure() with the ToolCallbacks shape the teardown expects, pointing both report paths at the caller so the dropped call still carries exactly one tool_result. Only write_to_file overrides the hook, so the resolver names it and leaves every other tool on the inherited no-op instead of adding a call that reports nothing. When the teardown surfaced the streaming error a delta captured, that text replaces the generic missing-nativeArgs text in the tool_result, which is what BaseTool does on its own path. The base hook loses its protected modifier so this caller can reach it; its only override already declares it without a visibility modifier.
1 parent a1d9d24 commit aae67b0

3 files changed

Lines changed: 141 additions & 6 deletions

File tree

‎src/core/assistant-message/__tests__/presentAssistantMessage.spec.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { presentAssistantMessage } from "../presentAssistantMessage"
55
import { DispatchState, transitionDispatch } from "../../task/dispatchState"
66
import type { Task } from "../../task/Task"
77
import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
8+
import { writeToFileTool } from "../../tools/WriteToFileTool"
89

910
vi.mock("../../task/Task")
1011
vi.mock("@roo-code/telemetry", () => ({
@@ -106,3 +107,76 @@ describe("transitionDispatch", () => {
106107
expect(() => transitionDispatch(from, to)).toThrow("invalid transition")
107108
})
108109
})
110+
describe("a dropped tool call still runs the parse-failure teardown", () => {
111+
// The dropped call is answered before the tool is dispatched, so this is the only
112+
// point where the per-task state a streaming delta registered outside execute()
113+
// can still be torn down.
114+
let task: Task
115+
let pushToolResultToUserContent: Mock
116+
117+
beforeEach(() => {
118+
pushToolResultToUserContent = vi.fn().mockReturnValue(true)
119+
task = {
120+
taskId: "task-id",
121+
instanceId: "instance-id",
122+
abort: false,
123+
didRejectTool: false,
124+
didAlreadyUseTool: false,
125+
consecutiveMistakeCount: 0,
126+
consecutiveMistakeLimit: 3,
127+
dispatchState: DispatchState.IDLE,
128+
pendingWork: null,
129+
currentStreamingContentIndex: 0,
130+
assistantMessageContent: [
131+
{ type: "tool_use", id: "call-1", name: "write_to_file", params: {}, partial: false },
132+
] as unknown as Task["assistantMessageContent"],
133+
userMessageContent: [],
134+
didCompleteReadingStream: false,
135+
activeToolContexts: new Map(),
136+
say: vi.fn().mockResolvedValue(undefined),
137+
recordToolError: vi.fn(),
138+
pushToolResultToUserContent,
139+
getTaskMode: vi.fn().mockResolvedValue("code"),
140+
} as unknown as Task
141+
})
142+
143+
afterEach(() => {
144+
vi.restoreAllMocks()
145+
})
146+
147+
it("runs the teardown for the dropped call and keeps the generic error when it reports nothing", async () => {
148+
const teardownSpy = vi.spyOn(writeToFileTool, "onParameterParseFailure").mockResolvedValue(false)
149+
150+
await presentAssistantMessage(task, snapshot)
151+
152+
expect(teardownSpy).toHaveBeenCalledTimes(1)
153+
expect(teardownSpy).toHaveBeenCalledWith(
154+
task,
155+
expect.objectContaining({ toolCallId: "call-1" }),
156+
expect.objectContaining({ message: expect.stringContaining("missing nativeArgs") }),
157+
)
158+
expect(pushToolResultToUserContent).toHaveBeenCalledTimes(1)
159+
expect(pushToolResultToUserContent.mock.calls[0][0]).toEqual(
160+
expect.objectContaining({ type: "tool_result", tool_use_id: "call-1", is_error: true }),
161+
)
162+
expect(String(pushToolResultToUserContent.mock.calls[0][0].content)).toContain("missing nativeArgs")
163+
})
164+
165+
it("puts the reported streaming error in the one tool_result instead of the generic text", async () => {
166+
const teardownSpy = vi
167+
.spyOn(writeToFileTool, "onParameterParseFailure")
168+
.mockImplementationOnce(async (_task, callbacks, _parseError) => {
169+
await callbacks.handleError("writing file", new Error("ENOSPC: no space left on device, write"))
170+
return true
171+
})
172+
173+
await presentAssistantMessage(task, snapshot)
174+
175+
expect(teardownSpy).toHaveBeenCalledTimes(1)
176+
expect(pushToolResultToUserContent).toHaveBeenCalledTimes(1)
177+
const payload = pushToolResultToUserContent.mock.calls[0][0]
178+
expect(String(payload.content)).toContain("ENOSPC: no space left on device, write")
179+
expect(String(payload.content)).not.toContain("missing nativeArgs")
180+
expect(payload.tool_use_id).toBe("call-1")
181+
})
182+
})

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { t } from "../../i18n"
1212

1313
import { defaultModeSlug, getModeBySlug } from "../../shared/modes"
1414
import type { AutoApprovalContext, ToolResponse, ToolUse, McpToolUse } from "../../shared/tools"
15+
import type { ToolCallbacks } from "../tools/BaseTool"
1516

1617
import { buildAutoDenyReason } from "../auto-approval"
1718

@@ -71,6 +72,51 @@ export function toTelemetryToolName(
7172
return "invalid_tool_call"
7273
}
7374

75+
/**
76+
* The one tool instance the parse-failure teardown can act on for a dropped call.
77+
*
78+
* write_to_file is the only tool that overrides onParameterParseFailure(); every other tool
79+
* inherits the base no-op that reports nothing and tears nothing down, so resolving those
80+
* names here would only add a call with no effect. Unknown, MCP, and custom-tool names
81+
* resolve to no tool.
82+
*/
83+
function parseFailureTeardownTool(toolName: string) {
84+
return toolName === "write_to_file" ? writeToFileTool : undefined
85+
}
86+
87+
/**
88+
* Run the parse-failure teardown for a known tool whose call never reached handle().
89+
*
90+
* The dispatch-scope callbacks do not exist at the point where the call is dropped, so this
91+
* builds the ToolCallbacks shape the teardown expects and points both report paths at the
92+
* caller instead of pushing a second payload: the caller keeps owning the single tool_result
93+
* the dropped call must carry, and the returned text tells it which error to put in it. The
94+
* write_to_file override reports through handleError; pushToolResult records a string result
95+
* the same way so a later override cannot have its report dropped on the floor.
96+
*
97+
* @returns the error text the teardown reported, or undefined when it reported nothing
98+
*/
99+
async function reportedErrorFromParseFailureTeardown(
100+
cline: Task,
101+
toolCallId: string,
102+
toolName: string,
103+
errorMessage: string,
104+
): Promise<string | undefined> {
105+
const tool = parseFailureTeardownTool(toolName)
106+
if (!tool) {
107+
return undefined
108+
}
109+
let reported: string | undefined
110+
const callbacks: ToolCallbacks = {
111+
askApproval: async () => false,
112+
handleError: async (_action: string, error: Error) => void (reported = error.message),
113+
pushToolResult: (content: ToolResponse) => (reported ??= typeof content === "string" ? content : undefined),
114+
toolCallId,
115+
}
116+
await tool.onParameterParseFailure(cline, callbacks, new Error(errorMessage))
117+
return reported
118+
}
119+
74120
/**
75121
* Processes and presents assistant message content to the user interface.
76122
*
@@ -577,12 +623,26 @@ async function presentAssistantMessageBlock(cline: Task, policySnapshot: Request
577623
// Best-effort only
578624
}
579625

626+
// The call is dropped before dispatch, so the tool never sees it: the parse-failure
627+
// boundary in BaseTool - write_to_file's only teardown for the partial ask, the abort
628+
// listener, the streaming failure mark, and any diff preview a delta already opened -
629+
// would not run either. Route the failure through that same boundary rather than
630+
// writing a parallel cleanup here. When the teardown already surfaced the streaming
631+
// error a delta captured, that is the error the user can act on, so it replaces the
632+
// generic missing-nativeArgs text in the one tool_result below instead of being pushed
633+
// as a second error for the same tool_use_id.
634+
const teardownReport = await reportedErrorFromParseFailureTeardown(
635+
cline,
636+
toolCallId,
637+
block.name,
638+
errorMessage,
639+
)
580640
// Push tool_result directly without setting didAlreadyUseTool so streaming can
581641
// continue gracefully.
582642
cline.pushToolResultToUserContent({
583643
type: "tool_result",
584644
tool_use_id: sanitizeToolUseId(toolCallId),
585-
content: formatResponse.toolError(errorMessage),
645+
content: formatResponse.toolError(teardownReport ?? errorMessage),
586646
is_error: true,
587647
})
588648

‎src/core/tools/BaseTool.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -190,17 +190,18 @@ export abstract class BaseTool<TName extends ToolName> {
190190
* that state down and, when a streaming delta already failed, to report the
191191
* captured streaming error instead of the generic parse error.
192192
*
193+
* The method is public on purpose: presentAssistantMessage() drops a complete known-tool
194+
* block whose nativeArgs never parsed before the call reaches handle(), and it routes the
195+
* failure through this same boundary so the teardown still runs for that call. The only
196+
* override already declares the method without a visibility modifier.
197+
*
193198
* @param _task - Task instance
194199
* @param _callbacks - Tool execution callbacks
195200
* @param _parseError - The native-argument parse error
196201
* @returns true when the override already reported the failure to the user,
197202
* so handle() suppresses the generic parse error
198203
*/
199-
protected async onParameterParseFailure(
200-
_task: Task,
201-
_callbacks: ToolCallbacks,
202-
_parseError: Error,
203-
): Promise<boolean> {
204+
async onParameterParseFailure(_task: Task, _callbacks: ToolCallbacks, _parseError: Error): Promise<boolean> {
204205
return false
205206
}
206207
}

0 commit comments

Comments
 (0)