Repository navigation
fix(responses): join streamed tool call argument chunks - #1067
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>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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>
|
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. |
|
@ericcurtin fixed the output-index issue in 2e6b7d6. Output indices are now assigned when each item is added and stay fixed, so a tool call that comes before the text uses the same
Noted on llmman. I checked earlier that llmman assembles these tool calls correctly (see my comment above), so there's nothing to port. I've marked this ready for review. |
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="394" />
<code_context>
SequenceNumber: s.nextSeq(),
Item: item,
- OutputIndex: len(s.toolCalls) - 1,
+ OutputIndex: s.toolCallOutputIndex(pos),
})
}
</code_context>
<issue_to_address>
**Index zero is omitted**
When the tool call is the first output item and its output index is 0, `sendEvent` marshals this event through `StreamEvent.OutputIndex`, whose `json:"output_index,omitempty"` tag omits zero. When a tool call occupies output index 0, clients receive `response.function_call_arguments.delta` without the required `output_index` and cannot rely on the event’s item position.
Ensure `output_index` is serialized for zero-valued indices, for example by removing `omitempty` from the field’s JSON tag.
</issue_to_address>| SequenceNumber: s.nextSeq(), | ||
| ItemID: item.ID, | ||
| OutputIndex: len(s.toolCalls) - 1, | ||
| OutputIndex: s.toolCallOutputIndex(pos), |
There was a problem hiding this comment.
🟡 Medium · Index zero is omitted
When the tool call is the first output item and its output index is 0, sendEvent marshals this event through StreamEvent.OutputIndex, whose json:"output_index,omitempty" tag omits zero. When a tool call occupies output index 0, clients receive response.function_call_arguments.delta without the required output_index and cannot rely on the event’s item position.
Ensure output_index is serialized for zero-valued indices, for example by removing omitempty from the field’s JSON tag.
Prompt for AI agents
In `pkg/responses/streaming.go` at line 394:
**Index zero is omitted**
When the tool call is the first output item and its output index is 0, `sendEvent` marshals this event through `StreamEvent.OutputIndex`, whose `json:"output_index,omitempty"` tag omits zero. When a tool call occupies output index 0, clients receive `response.function_call_arguments.delta` without the required `output_index` and cannot rely on the event’s item position.
Ensure `output_index` is serialized for zero-valued indices, for example by removing `omitempty` from the field’s JSON tag.
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues were identified; only a minor test-coverage nit remains.
0 open findings
1 resolved since last review
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…dded A tool call that started before any assistant text was added at output index 0, then switched to index 1 once text arrived, and the stored output listed the message first. Assign each item its output index when it is added, use it for all of its events, and place it at that index in the response output. Tool calls that share a stream index but carry different IDs are now kept as separate calls instead of having their arguments joined. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Eric Curtin <eric.curtin@docker.com> Signed-off-by: breken-ai <312387581+breken-ai@users.noreply.github.com>
2e6b7d6 to
709e000
Compare
ericcurtin
left a comment
ericcurtin
left a comment
There was a problem hiding this comment.
Reviewed. Verified build, race tests and lint locally. Pushed a small fix so calls sharing a stream index but with different IDs stay separate.
|
Merged, thanks. I pushed one small fix: tool calls that share a stream index but have different IDs now stay separate (test added). Follow-on: model-runner is being deprecated in favour of https://github.com/llmmanorg/llmman, so please open future PRs there. |
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.