Repository navigation
fix(openai): keep every streamed tool call when a chunk carries several - #960
alexcastgonz (pcbeingused333) wants to merge 2 commits into
Conversation
The chat completion stream postprocessor read only delta.tool_calls[0] and appended argument fragments to the last call opened. Providers that stream parallel tool calls in one delta (OpenAI-compatible endpoints such as Gemini's, Together, vLLM) lost every call after the first in the logged span, and fragments interleaved across indexes went to the wrong call. Walk every entry of delta.tool_calls and continue the call named by its index, falling back to the last call when a provider omits index. A new id still opens a new call, so providers that reuse index 0 keep working. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b9e5da08b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| else: | ||
| tool_call = tool_calls[-1] if tool_calls else None | ||
| # A new id always opens a new call, even under an index already in use. | ||
| if tool_call is not None and tool_id and tool_call["id"] and tool_call["id"] != tool_id: |
There was a problem hiding this comment.
When a compatible provider first emits an ID-less call and then starts a distinct call with a nonempty ID at the same reused index, this guard stays false because tool_call["id"] is falsy. The following merge path drops the new ID/name/type and concatenates its arguments into the prior call; before this change, any nonempty differing ID opened a new entry, so these streams now produce a silently corrupted span. Compare incoming IDs even when the stored one is None (or explicitly handle late ID hydration) rather than unconditionally merging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch: a call that arrived without an id stored id=None, so the guard never fired and a later call with an id under the same index was merged into it. Fixed in f8972ee: any non-empty id that differs from the stored one now opens a new call, as before this change; id-less continuation fragments still extend the current call (the GLM-4.6 test still passes). Added test_chat_completion_streaming_call_with_id_after_call_without_id_under_reused_index, which fails on the previous commit.
| return openai.types.chat.ChatCompletionChunk.model_validate( | ||
| { |
There was a problem hiding this comment.
The new regression tests only pass handcrafted chunks through the postprocessor, so they never establish that Gemini/Together/vLLM produce this multi-call wire shape or that the full wrap_openai streaming path logs it correctly; a mistaken synthetic payload or parser/version difference can therefore leave all three tests green while the production bug remains. Keep these as supplemental unit cases, but add a VCR-backed test against at least one claimed compatible provider, as required for response-shape-dependent integration behavior.
AGENTS.md reference: AGENTS.md:L184-L186
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There's no cassette because the official OpenAI API never streams several calls in one delta, and recording one needs keys for a provider that does (Gemini's OpenAI-compatible endpoint, Together, vLLM), which aren't available here. The shape was reproduced end to end through wrap_openai against a local OpenAI-compatible SSE endpoint (before: only the first call in the span; after: both). If a maintainer can record a cassette against one of those providers, happy to wire it in; or this can include that end-to-end test with a local fake server instead, if you prefer.
…ess call A call that arrived without an id stored id=None, so a later call with an id under the same (reused) index was merged into it. A non-empty id that differs from the stored one now always opens a new call, as before this branch. Fragments without an id still continue the current call (GLM-4.6 behavior). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
1 participant
What
ChatCompletionWrapper._postprocess_streaming_resultsread onlydelta.tool_calls[0]and appended argument fragments to the last call it had opened. OpenAI itself streams one call per chunk, so this works there. OpenAI-compatible providers that send parallel tool calls in a single delta (Gemini's OpenAI-compatible endpoint, Together, vLLM) lost every call after the first in the logged span. Argument fragments interleaved across two indexes were also appended to the wrong call.The loop now walks every entry of
delta.tool_callsand continues the call named by itsindex. When a provider omitsindex(the GLM-4.6 case covered bytest_chat_completion_streaming_none_arguments), a fragment still continues the last call, and a newidstill opens a new call, so providers that reuse index 0 keep working.Through
wrap_openai, a stream whose first chunk carries two calls, Paris and Rome (reproduced against a local OpenAI-compatible SSE endpoint):tool_calls=get_weather {"city": "Paris"}onlyget_weather {"city": "Paris"},get_weather {"city": "Rome"}, matching what the caller receivedTests
New tests in
TestZAICompatibleOpenAI, next to the existing GLM-4.6 postprocessor test, built fromopenai.types.chat.ChatCompletionChunkobjects:test_chat_completion_streaming_parallel_tool_calls_in_one_chunk: two complete calls in one delta. Fails on main (second call missing).test_chat_completion_streaming_interleaved_tool_call_arguments: fragments for indexes 0 and 1 interleaved across chunks. Fails on main (arguments land on the wrong call).test_chat_completion_streaming_new_tool_call_id_under_reused_index: every call at index 0 with distinct ids. Passes on main and after the fix; it guards the behavior kept on purpose.I didn't add a VCR cassette: the official OpenAI API never streams several calls in one delta, so recording one needs one of those third-party providers. Happy to add one if you'd rather have it.
Run from
py/with thetest_openai(latest)files and env (BRAINTRUST_TEST_PACKAGE_VERSION=latest,-p no:braintrust, openai 3.29.0):test_openai.py,test_oai_attachments.py,test_openai_openrouter_gateway.py→ 84 passed (81 on main plus the 3 new tests). ruff 0.15.21 format/check, codespell 2.2.5 andpylint --errors-onlyare clean on the changed files. The older openai matrix versions (1.92/1.77/1.71) were not run locally.Follow-up
The LiteLLM integration has the same loop (
integrations/litellm/tracing.py~L727,_postprocess_completion_streaming_results). Happy to follow up with the same change there.This change was written with Claude Code; the tests and the reproduction above were run before opening the PR.