fix: handle malformed native tool calls to prevent hanging #9758
Conversation
When a model sends a malformed tool call (e.g., apply_diff with only
{"path": "file.go"} and missing required parameters), the
NativeToolCallParser.finalizeStreamingToolCall() returns null because
the JSON is incomplete or can't be parsed properly.
Previously, when finalizeStreamingToolCall returned null, the code did
nothing - leaving the partial tool use in assistantMessageContent with
partial: true. This caused the tool to never be executed and no
tool_result was sent back to the API, causing the system to hang.
This fix ensures that when finalizeStreamingToolCall returns null, we
still mark the existing partial tool use as complete (partial: false)
so it gets executed. The tool's validation will catch missing required
parameters and return a proper error message to the model.
Fixes #9542
dosubot
bot
added
size:L
Review completed. All previously flagged issues have been resolved. The latest commit removes the problematic test code that was flagged in previous reviews. No new issues found. Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
hannesrudolph
added
PR - Needs Review
and removed
Issue/PR - Triage
| }) | ||
| }) | ||
|
|
||
| describe("malformed tool calls", () => { |
There was a problem hiding this comment.
These tests seem to pass with the Task.ts changes reverted as well
PR #9542 added a condition that only called presentAssistantMessage() if there was at least one non-tool_use partial block. This broke XML protocol because XML tools are parsed from the text stream (not via native events), and when only a malformed tool_use was the partial block, presentAssistantMessage was never called, causing a hang. This commit restores the original behavior that works for both protocols.
These tests verify that presentAssistantMessage is called at stream end even when only tool_use blocks are partial. The tests explicitly show the difference between the fixed condition and the broken condition that was introduced in PR #9542.
daniel-lxs
force-pushed
the
fix/malformed-native-tool-call-hang
branch
from
643449e to
d942014
Compare
| // For XML protocol: includes both text and tool_use blocks parsed from the text stream | ||
| // For native protocol: tool_use blocks were already presented during streaming via | ||
| // tool_call_partial events, but we still need to present them if they exist (e.g., malformed) | ||
| if (partialBlocks.length > 0) { |
There was a problem hiding this comment.
So this change fixes XML and the other one fixes native tool calls?
src/core/task/__tests__/Task.spec.ts
Outdated
| }) | ||
| }) | ||
|
|
||
| describe("Partial tool_use blocks at stream end", () => { |
There was a problem hiding this comment.
I think this test also passes before and after the Task.ts changes
…_use blocks to prevent hangs
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
src/core/task/__tests__/Task.spec.ts
Outdated
| expect(correctCondition).not.toBe(brokenCondition) | ||
| }) | ||
|
|
||
| it("ACTUAL CODE TEST: Task.ts must NOT filter out tool_use blocks when calling presentAssistantMessage", async () => { |
There was a problem hiding this comment.
The test that reads Task.ts source and checks for a specific condition relies on exact string matching of source formatting, which is brittle. Consider refactoring to test behavior rather than text formatting.
There was a problem hiding this comment.
Yeah I’d probably remove this one
Adds a test that reads the actual Task.ts source code and verifies that the broken condition (filtering out tool_use blocks) is not present, preventing future regressions of #9758
Summary
This PR fixes a regression introduced in PR #9542 that causes malformed tool calls to hang indefinitely for both native and XML protocols.
Root Cause Analysis
The Regression in PR #9542
BEFORE PR #9542 (commit cad6145):
AFTER PR #9542 (commit 0327f12):
PR #9542 added the condition
&& partialBlocks.some((block) => block.type !== "tool_use")thinking it would optimize by not re-presenting native tool blocks that were already presented during streaming.But this breaks XML protocol because:
tool_useblocks from the text stream (they don't come via native events)tool_useis the partial block (no text blocks), the new condition evaluates tofalsepresentAssistantMessageis never calledNative Protocol Issue
For native protocol, PR #9542 also introduced streaming tool calls (
tool_call_partialevents) with three phases:tool_call_start- creates a partial tool usetool_call_delta- accumulates argument chunkstool_call_end- callsfinalizeStreamingToolCall()to complete itWhen a model sends malformed arguments (e.g.,
{"path": "file.go"}instead of the required structure),finalizeStreamingToolCall()returnsnulland the tool was silently ignored, leaving a partial tool use that would hang.The Fix
Remove the restrictive condition: Restore the original behavior that calls
presentAssistantMessagefor ALL partial blocks, including tool_use-only scenariosHandle malformed native tool calls: When
finalizeStreamingToolCall()returnsnull, we now mark the existing partial tool as complete (partial: false) so it proceeds to validation which catches the missing parametersTesting
Important
Fixes hanging issue for malformed tool calls in native and XML protocols by adjusting tool call handling in
Task.ts.Task.ts.presentAssistantMessageis called for all partial blocks, including tool_use-only scenarios.Task.ts, whenfinalizeStreamingToolCall()returnsnull, marks tool as complete (partial: false) to proceed to validation.This description was created by
for 52b890e. You can customize this summary. It will automatically update as commits are pushed.