fix(chat-completions): emit tool call arguments once#363
Merged
lidge-jun merged 1 commit intoJul 24, 2026
Merged
Conversation
Buffer Responses function-call argument events and emit one complete Chat Completions tool-call delta per index. Preserve pending calls across valid terminal frames, use finalized snapshots defensively, and ignore duplicate done events.
📝 WalkthroughWalkthroughThe SSE converter now buffers streamed tool-call identity and arguments by stable index, emits each complete tool call once, preserves non-empty buffered arguments, and flushes pending calls before terminal frames. Tests cover parallel calls and multiple incomplete or duplicate event sequences. ChangesTool-call streaming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesEvents
participant responsesSseToChatCompletionsSse
participant ChatCompletionsClient
ResponsesEvents->>responsesSseToChatCompletionsSse: send output-item and argument events
responsesSseToChatCompletionsSse->>responsesSseToChatCompletionsSse: buffer identity and argument text
responsesSseToChatCompletionsSse->>ChatCompletionsClient: emit one complete tool-call chunk
responsesSseToChatCompletionsSse->>ChatCompletionsClient: flush pending calls before finish
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@src/chat/outbound.ts`:
- Around line 277-285: Extract a shared resolveToolIndex helper for the
response.output_item.added and response.output_item.done handlers in
outbound.ts. Centralize the lookup order of callId, itemId, and new index
allocation, and ensure the helper keeps toolCallIdByIndex, toolIndexByItemId,
and related index maps synchronized; replace both duplicated resolution
implementations with this helper while preserving each handler’s existing
argument and name updates.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0077a661-bfa4-40a1-ac4a-432a3d815fe2
📒 Files selected for processing (2)
src/chat/outbound.tstests/chat-completions-endpoint.test.ts
Comment on lines
+277
to
+285
| toolCallIdByIndex.set(toolIndex, callId); | ||
| if (typeof item.id === "string") toolIndexByItemId.set(item.id, toolIndex); | ||
| if (name) toolNameByIndex.set(toolIndex, name); | ||
| const frame = chunkBase(id, model, created); | ||
| frame.choices = [{ | ||
| index: 0, | ||
| delta: { | ||
| tool_calls: [{ | ||
| index: toolIndex, | ||
| id: callId, | ||
| type: "function", | ||
| // Always include name so clients that replace (not merge) tool_calls | ||
| // deltas never end up with function.name-less history. | ||
| function: { name, arguments: "" }, | ||
| }], | ||
| }, | ||
| finish_reason: null, | ||
| }]; | ||
| emit(frame); | ||
| if (!toolArgumentsByIndex.has(toolIndex)) { | ||
| toolArgumentsByIndex.set( | ||
| toolIndex, | ||
| typeof item.arguments === "string" ? item.arguments : "", | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a shared resolveToolIndex helper to dedupe index resolution.
response.output_item.added (Lines 272-279) and response.output_item.done (Lines 322-328) both re-implement "look up toolIndex by callId, fall back to itemId, else allocate new, then keep all three maps in sync." The two implementations are subtly different (added only checks the callId map before allocating; done checks both) which makes them easy to accidentally diverge the next time either handler is touched — exactly the kind of drift this PR's fix is designed to prevent.
♻️ Proposed refactor
+ const resolveToolIndex = (callId: string, itemId: string | undefined): number => {
+ const existing = toolIndexByCallId.get(callId)
+ ?? (itemId ? toolIndexByItemId.get(itemId) : undefined);
+ const toolIndex = existing ?? nextToolIndex++;
+ toolIndexByCallId.set(callId, toolIndex);
+ toolCallIdByIndex.set(toolIndex, callId);
+ if (itemId) toolIndexByItemId.set(itemId, toolIndex);
+ return toolIndex;
+ };In response.output_item.added:
- let toolIndex = toolIndexByCallId.get(callId);
- if (toolIndex === undefined) {
- toolIndex = nextToolIndex++;
- toolIndexByCallId.set(callId, toolIndex);
- }
- toolCallIdByIndex.set(toolIndex, callId);
- if (typeof item.id === "string") toolIndexByItemId.set(item.id, toolIndex);
+ const toolIndex = resolveToolIndex(callId, typeof item.id === "string" ? item.id : undefined);In response.output_item.done:
- const itemId = typeof item.id === "string" ? item.id : undefined;
- const existingIndex = toolIndexByCallId.get(callId)
- ?? (itemId ? toolIndexByItemId.get(itemId) : undefined);
- const toolIndex = existingIndex ?? nextToolIndex++;
- toolIndexByCallId.set(callId, toolIndex);
- toolCallIdByIndex.set(toolIndex, callId);
- if (itemId) toolIndexByItemId.set(itemId, toolIndex);
+ const toolIndex = resolveToolIndex(callId, typeof item.id === "string" ? item.id : undefined);Also applies to: 321-339
🤖 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 `@src/chat/outbound.ts` around lines 277 - 285, Extract a shared
resolveToolIndex helper for the response.output_item.added and
response.output_item.done handlers in outbound.ts. Centralize the lookup order
of callId, itemId, and new index allocation, and ensure the helper keeps
toolCallIdByIndex, toolIndexByItemId, and related index maps synchronized;
replace both duplicated resolution implementations with this helper while
preserving each handler’s existing argument and name updates.
lidge-jun
added a commit
that referenced
this pull request
Jul 24, 2026
beab5b3); goalplan roadmap lock
lidge-jun
added a commit
that referenced
this pull request
Jul 24, 2026
…:348, collectChatCompletion :479)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
outbound bridge
output_item.done, with a terminal flush when a validcompleted/incomplete response omits the item-done event
missing or unexpectedly empty
Fixes #361.
Why
The internal Responses stream can contain incremental
response.function_call_arguments.deltaevents, a finalizedresponse.function_call_arguments.doneevent, and a completeresponse.output_item.donesnapshot. The converter forwarded incremental arguments and the item snapshot as Chat Completions
deltas. Clients that correctly append
function.argumentstherefore received duplicated JSON suchas:
Buffering only the tool-call arguments and emitting one complete tool-call delta keeps ordinary
text/reasoning streaming, preserves the final snapshot, and leaves clients that replace the
function object with a complete
id/name/argumentstuple. An emitted-index guard preventsduplicate item-done events from recreating the corruption, while a terminal flush preserves known
tool calls on valid incomplete responses.
This intentionally delays tool-argument visibility until the call or response is finalized;
ordinary text and reasoning remain incremental.
Verification
bun install --frozen-lockfilebun run test tests/chat-completions-endpoint.test.ts— 25 passed, 0 failedbun run typecheckbun run privacy:scangit diff --check upstream/dev...HEADbun run test— changed suite passed; the full Windows run reached 3,878 passed and 4 skipped,with 2 unrelated
Cursor native exec bridgefailures because this environment lacksprintfbun run test tests/cursor-native-exec.test.tswith a temporary compatibleprintfonPATH— 16 passed, confirming the two full-suite failures were environment-dependent
The focused tests cover the issue #361 duplicate sequence, authoritative done-frame reconciliation,
interleaved parallel calls with complete stable identities, missing/empty final arguments,
duplicate done events, terminal flushing, and finalized arguments without an item-done event.
Checklist
Summary by CodeRabbit