-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix: Prevent "Attempting to finalize unknown tool call" warning for GLM models #11111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nstarted tool calls - Add hasStarted check in processFinishReason() to only emit tool_call_end events for tool calls that have been properly started (received a name) - Add diagnostic warning when skipping unstarted tool calls - Add comprehensive tests for the hasStarted behavior with started, unstarted, and mixed scenarios - Fixes issue #11071 where GLM models send tool call IDs without names, causing synchronization issues between rawChunkTracker and streamingToolCalls This prevents the warning "Attempting to finalize unknown tool call" that appears when GLM models send incomplete tool call data (ID without name).
Review complete. The implementation fix is correct and follows the existing pattern in
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
| it("should handle tool call that receives name in a separate chunk", () => { | ||
| // First chunk: ID only | ||
| NativeToolCallParser.processRawChunk({ | ||
| index: 0, | ||
| id: "call_delayed_name", | ||
| }) | ||
|
|
||
| // At this point, tool call is tracked but not started | ||
| let events = NativeToolCallParser.processFinishReason("tool_calls") | ||
| expect(events).toHaveLength(0) | ||
|
|
||
| // Clear state and try again with name | ||
| NativeToolCallParser.clearRawChunkState() | ||
|
|
||
| // Simulate proper sequence with name | ||
| NativeToolCallParser.processRawChunk({ | ||
| index: 0, | ||
| id: "call_delayed_name", | ||
| name: "read_file", | ||
| }) | ||
|
|
||
| events = NativeToolCallParser.processFinishReason("tool_calls") | ||
| expect(events).toHaveLength(1) | ||
| expect(events[0]).toEqual({ | ||
| type: "tool_call_end", | ||
| id: "call_delayed_name", | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't actually test delayed name arrival. The clearRawChunkState() call on line 446 resets all state, so the second phase tests a completely independent scenario rather than a continuation. To properly test delayed name arrival, remove the clear and send two chunks in sequence:
| it("should handle tool call that receives name in a separate chunk", () => { | |
| // First chunk: ID only | |
| NativeToolCallParser.processRawChunk({ | |
| index: 0, | |
| id: "call_delayed_name", | |
| }) | |
| // At this point, tool call is tracked but not started | |
| let events = NativeToolCallParser.processFinishReason("tool_calls") | |
| expect(events).toHaveLength(0) | |
| // Clear state and try again with name | |
| NativeToolCallParser.clearRawChunkState() | |
| // Simulate proper sequence with name | |
| NativeToolCallParser.processRawChunk({ | |
| index: 0, | |
| id: "call_delayed_name", | |
| name: "read_file", | |
| }) | |
| events = NativeToolCallParser.processFinishReason("tool_calls") | |
| expect(events).toHaveLength(1) | |
| expect(events[0]).toEqual({ | |
| type: "tool_call_end", | |
| id: "call_delayed_name", | |
| }) | |
| }) | |
| it("should handle tool call that receives name in a separate chunk", () => { | |
| // First chunk: ID only | |
| NativeToolCallParser.processRawChunk({ | |
| index: 0, | |
| id: "call_delayed_name", | |
| }) | |
| // Second chunk: name arrives for the same index | |
| NativeToolCallParser.processRawChunk({ | |
| index: 0, | |
| name: "read_file", | |
| }) | |
| // Now the tool call should be started and receive end event | |
| const events = NativeToolCallParser.processFinishReason("tool_calls") | |
| expect(events).toHaveLength(1) | |
| expect(events[0]).toEqual({ | |
| type: "tool_call_end", | |
| id: "call_delayed_name", | |
| }) | |
| }) |
Fix it with Roo Code or mention @roomote and request a fix.
This PR addresses issue #11071 by fixing the "Attempting to finalize unknown tool call" warning that appears when using GLM models via LM Studio or OpenAI-compatible endpoints.
Root Cause Analysis
The warning occurred due to a synchronization issue in
NativeToolCallParserbetween two tracking systems:rawChunkTracker- Tracks tool calls during raw chunk processing (keyed by stream index)streamingToolCalls- Tracks tool calls for argument accumulation (keyed by tool call ID)When GLM models send a tool call ID without a name (incomplete data), the tool call gets tracked in
rawChunkTrackerbut never "started" (notool_call_startevent emitted, not added tostreamingToolCalls).The
processFinishReason()method was emittingtool_call_endevents for ALL tracked tool calls, including those that were never started. This causedfinalizeStreamingToolCall()to fail with the warning.Changes Made
Added
hasStartedcheck inprocessFinishReason()tool_call_endevents for tool calls that have been properly started (have a name)finalizeRawChunks()methodComprehensive test coverage
Impact
This fix:
Testing
Added 5 new test cases covering various scenarios. Tests follow the existing pattern in the test suite.
Feedback welcome!
Important
Fixes warning in
NativeToolCallParserby ensuringtool_call_endevents are only emitted for started tool calls, with comprehensive test coverage added.hasStartedcheck inprocessFinishReason()inNativeToolCallParser.tsto emittool_call_endevents only for started tool calls.NativeToolCallParser.spec.tsfor started, unstarted, and mixed tool call scenarios.finish_reasonvalues and delayed name arrival.This description was created by
for b92cd76. You can customize this summary. It will automatically update as commits are pushed.