Conversation
In a chat completion stream only the first delta of a tool call has its id and name. The argument deltas after it carry just "index" (llama.cpp and OpenAI both stream this way). handleToolCallDelta looked tool calls up by id only, so every argument delta became a new function_call item with a random call_id and no name. With streaming on, a Responses API client got one item with the right name and empty arguments, plus one nameless item per argument chunk. Parse "index" and match on it, falling back to the id for streams that do not send it. Argument deltas also report the output_index of their own item now. Assisted-By: Claude Signed-off-by: breken-ai <312387581+breken-ai@users.noreply.github.com>
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/responses/streaming.go" line_range="326-337" />
<code_context>
- break
+ // Find or create the tool call item. Argument deltas after the first
+ // one carry only the index (no ID), so match on the index when present.
+ pos := -1
+ if tc.Index != nil {
+ if p, ok := s.toolCallPos[*tc.Index]; ok {
+ pos = p
+ }
+ } else if tc.ID != "" {
+ for i := range s.toolCalls {
</code_context>
<issue_to_address>
**issue (bug_risk):** When a tool call is first matched by its ID without an index, a later delta that includes an index is not matched by ID because the indexed branch is selected exclusively. That delta creates a duplicate item and registers the index to the duplicate, so subsequent index-only argument chunks are appended to the wrong call.
**Triggers:** When an upstream stream changes from ID-based deltas to indexed deltas, or otherwise sends an indexed delta before the index has been registered for the existing call.
**Suggested fix:** If the index lookup misses, fall back to the ID lookup when `tc.ID` is non-empty, and register the discovered position in `toolCallPos` whenever `tc.Index` is present.
</issue_to_address>If a call was first seen by id without an index, a later delta that adds the index now extends that call instead of starting a duplicate, and the index is remembered for the index-only deltas that follow. Assisted-By: Claude Signed-off-by: breken-ai <312387581+breken-ai@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The test mishandles the [DONE] sentinel, and output-index serialization and offsets remain incorrect.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Fixes streamed tool-call argument assembly in the Responses API by correlating chunks via their stream index.
Changes:
- Adds optional tool-call index metadata.
- Merges indexed argument chunks and updates output positions.
- Adds regression coverage for streamed tool calls.
| File | Description |
|---|---|
pkg/responses/transform.go |
Adds optional tool-call index support. |
pkg/responses/streaming.go |
Correlates and accumulates streamed tool calls. |
pkg/responses/handler_test.go |
Tests chunk merging and output indexes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Nice fix for the chunk merging, but the output_index for tool calls still doesn't account for prior text items in the output list (see streaming.go around line 373 and finalize()), Copilot flagged this and it's unresolved. Please fix or split into a follow-up. Also, model-runner is being deprecated in favor of llmman, please open future PRs there instead. Marking as draft, please mark ready for review once addressed. |
|
I checked llmmanorg/llmman @ 34248d2 for this bug, and llmman doesn't have it. In its The existing test The |
Assisted-By: Claude Signed-off-by: breken-ai <312387581+breken-ai@users.noreply.github.com>
|
Addressed both Copilot findings in 65d5593. The test now skips the terminal |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/responses/streaming.go" line_range="393-400" />
<code_context>
}
}
+// toolCallOutputIndex converts a position in toolCalls to the corresponding
+// position in response.Output. The assistant message, when present, is first.
+func (s *StreamingResponseWriter) toolCallOutputIndex(pos int) int {
+ if s.currentItemID != "" {
+ return pos + 1
+ }
+ return pos
+}
+
+// rememberToolCallIndex records which item a streaming tool call index refers to.
</code_context>
<issue_to_address>
**issue (bug_risk):** When a tool-call delta arrives before any text delta, its `output_index` is emitted as 0 because `currentItemID` is empty. If text arrives later, the message is finalized at output index 0 and the tool call is finalized at output index 1, so the same tool-call item has inconsistent indices across its added, delta, and done events.
**Triggers:** When the upstream stream interleaves tool-call chunks before assistant text chunks.
**Suggested fix:** Track the output-item order independently of whether the text item has been observed, or normalize all event indices after determining the final output ordering.
</issue_to_address>| // toolCallOutputIndex converts a position in toolCalls to the corresponding | ||
| // position in response.Output. The assistant message, when present, is first. | ||
| func (s *StreamingResponseWriter) toolCallOutputIndex(pos int) int { | ||
| if s.currentItemID != "" { | ||
| return pos + 1 | ||
| } | ||
| return pos | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): When a tool-call delta arrives before any text delta, its output_index is emitted as 0 because currentItemID is empty. If text arrives later, the message is finalized at output index 0 and the tool call is finalized at output index 1, so the same tool-call item has inconsistent indices across its added, delta, and done events.
Triggers: When the upstream stream interleaves tool-call chunks before assistant text chunks.
Suggested fix: Track the output-item order independently of whether the text item has been observed, or normalize all event indices after determining the final output ordering.
| func (s *StreamingResponseWriter) toolCallOutputIndex(pos int) int { | ||
| if s.currentItemID != "" { | ||
| return pos + 1 |
|
Nice fix for the ID-matching duplicate bug. However, the output-index bug flagged by the review bots (tool call before text content leads to inconsistent output_index across added/delta/done events) is still unresolved. Please fix that before merge. Also, please open future PRs against https://github.com/llmmanorg/llmman instead. model-runner is being deprecated in favor of it. Converting to draft; please mark ready again once the output-index issue is fixed. |
What
With
stream: true,/v1/responsesbreaks every tool call into severalfunction_callitems: one with the right name and empty arguments, then one nameless item (with a randomcall_id) for each argument chunk.Why
In a chat completion stream only the first delta of a tool call has its
idandname. The deltas after it carry onlyindexand an arguments fragment. llama.cpp does this (tools/server/server-chat.cpponly writesidwhentool_call_delta.idis set, andserver-task.cppclearsid/nameon later argument diffs), and so does OpenAI.StreamingResponseWriter.handleToolCallDeltalooks tool calls up only bytc.ID, andChatToolCallhas noindexfield. An argument delta has an empty id, so it never matches and a new item is created every time.For example, a llama.cpp stream like this:
produces three items on
main:A Responses API client then sees a
get_weathercall with no arguments, plus unnamed calls it cannot dispatch.Fix
Index *int(json:"index,omitempty") toChatToolCall. It is omitted when nil, so request messages do not change.handleToolCallDelta, match on the stream index when it is present. Fall back to the id for streams that do not send an index. Fill in the name if it arrives after the first delta.response.function_call_arguments.deltanow reports theoutput_indexof its own item instead of the last one added.Tests
New
TestHandler_CreateResponse_Streaming_ToolCallArgumentChunksstreams two tool calls in the llama.cpp format (header delta, then index-only argument deltas). It checks the stored response and the delta events.mainit fails with 5function_callitems ({call_a get_weather ""} {call_... "" {"city":} {call_... "" "Paris"}} {call_b get_time ""} {call_... "" {"tz":"CET"}}).{call_a get_weather {"city":"Paris"}}and{call_b get_time {"tz":"CET"}}.Also run:
go test -race ./pkg/responses/...passes with go1.25.8.golangci-lint run ./pkg/responses/...(v2.12.2 from.versions) reports 0 issues.The behavior dates from the first version of the Responses layer (#540).
AI disclosure
This bug was found and the fix and PR text were written with an AI assistant (Claude), working for the breken-ai account. I checked the failure on
mainand the fix with the test above. The commit has anAssisted-By: Claudetrailer and a DCO sign-off.