Repository navigation
Conversation
When assistant text streamed before a tool call, the message output item opened at outputItemIndex without advancing it, so the following tool call reused the same output_index. The message's done events also read the mutated counter, drifting from its added index. Reserve a dedicated index for the message and order the final output array by output_index.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change gives streamed reasoning, assistant messages, and function calls distinct output indices. Completion events reuse the assistant message index, and ChangesStreaming Output Index Tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Converter as processStreamChunk
participant State as StreamingState
participant Completion as createCompletionEvents
Client->>Converter: reasoning or content delta
Converter->>State: assign output index
Converter-->>Client: streamed output events
Client->>Converter: tool_calls delta
Converter-->>Client: function_call output event
Client->>Completion: create completion events
Completion->>State: read stored indices
Completion->>Completion: sort output items by index
Completion-->>Client: response.completed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gateway/src/responses/responses.spec.ts (1)
636-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for reasoning followed by streamed tool-call chunks.
This test covers message → tool. Since this PR also changes
reasoningOutputIndex, add a case with reasoning, multiple tool-call argument chunks, and a later second tool call to assert emitted indices stay stable and align with final output order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/responses/responses.spec.ts` around lines 636 - 696, Extend the existing streaming response test around processStreamChunk/createCompletionEvents to cover reasoning followed by multiple tool-call chunks and a later second tool call. Add assertions that reasoningOutputIndex stays stable across added/done events, that each streamed function_call keeps a consistent output_index as arguments arrive in chunks, and that the final response.completed output order matches the assigned output_index sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/gateway/src/responses/tools/convert-streaming-to-responses.ts`:
- Around line 263-267: The reasoning output slot is only being recorded in
convert-streaming-to-responses’s reasoning-start path, but state.outputItemIndex
is not advanced there, so later tool-call chunks can reuse a stale output_index.
Update the reasoning-start handling in convert-streaming-to-responses so that it
claims the current slot immediately by advancing state.outputItemIndex when
state.reasoningOutputIndex is set, and remove the deferred increments in the
later close-reasoning branch so the output_item.added / response.output
positions stay aligned.
---
Nitpick comments:
In `@apps/gateway/src/responses/responses.spec.ts`:
- Around line 636-696: Extend the existing streaming response test around
processStreamChunk/createCompletionEvents to cover reasoning followed by
multiple tool-call chunks and a later second tool call. Add assertions that
reasoningOutputIndex stays stable across added/done events, that each streamed
function_call keeps a consistent output_index as arguments arrive in chunks, and
that the final response.completed output order matches the assigned output_index
sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6a0e03e1-ea75-43fd-9cd9-17568e667291
📒 Files selected for processing (2)
apps/gateway/src/responses/responses.spec.tsapps/gateway/src/responses/tools/convert-streaming-to-responses.ts
|
thanks @serhiizghama! wondering if the PR coderabbit feedback is relevant before merging this? a specific proof of concept or payload to test would be also helpful, although not necessary |
…nses-message-tool-output-index # Conflicts: # apps/gateway/src/responses/tools/convert-streaming-to-responses.ts
|
Rebased on main — the conflict was just the message output item picking up the new |
|
@serhiizghama please read my actual comment |
The close-reasoning branch ran on every tool-call chunk while no message had started, so reasoning followed by multi-chunk tool calls inflated the shared index and a later tool call got an output_index past its final response.output position. Claim the reasoning slot once when reasoning starts and drop the deferred increments.
…nses-message-tool-output-index
|
Sorry, you're right — I answered the wrong thing last time and skipped both of your actual questions. On the coderabbit feedback: it was a real gap, and it was in my own fix. When reasoning is streamed and then tool calls arrive with no assistant message, the "close reasoning" branch ran on every tool-call chunk, because For the proof of concept, here's the exact chunk sequence — reasoning, then a two-chunk processStreamChunk({ choices: [{ delta: { reasoning: "thinking" } }] }, state);
processStreamChunk({ choices: [{ delta: { tool_calls: [{ index: 0, id: "call_a", function: { name: "get_weather", arguments: "" } }] } }] }, state);
processStreamChunk({ choices: [{ delta: { tool_calls: [{ index: 0, function: { arguments: '{"city":"NYC"}' } }] } }] }, state);
processStreamChunk({ choices: [{ delta: { tool_calls: [{ index: 1, id: "call_b", function: { name: "get_time", arguments: "{}" } }] } }] }, state);
createCompletionEvents(state);Before the fix, the streamed events tell the client After the fix they line up — To be clear about how I verified this: I ran that sequence through the streaming-conversion functions directly ( |
|
Thanks for the thorough follow-up — verified your PoC and the fix, everything checks out. One thing before merge: |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed the annotation fix directly (a74a3e4): |
|
Nice catch on the annotations event, thanks for fixing it directly — pulled it in, 42/42 still green on my end. |
2 participants
When the chat-completions stream emits assistant text before a tool call — common when a model says something like "let me check" and then calls a function — the streamed Responses events gave the message and the function_call the same
output_index.The message output item opens at
state.outputItemIndexbut never advances it (unlike reasoning and tool items, which claim-and-increment), so the next tool call reuses index 0. On top of that the message'soutput_text.done/content_part.done/output_item.doneevents read the counter after the tool call bumped it, so the message's own added and done events disagreed on the index. And the finaloutputarray was always built as [reasoning, tools, message], which contradicts the streamed order once text comes first.Reserve a dedicated index for the message when it opens (and record the reasoning index too), use it consistently across its added/delta/done events, and sort the final
outputarray byoutput_indexso it matches the stream. Content-only and tool-only streams are unaffected — their indices are unchanged.Added a unit test for the text-then-tool_call case asserting the two items get distinct indices, the message keeps one index across added/done, and the completed output is ordered message-before-tool.
Summary by CodeRabbit
output_indexvalues, keeping reasoning, assistant messages, and function/tool calls in the correct final sequence.output_indexthroughout the stream.output_indexstability and final output ordering.