fix(chat-completions): tolerate a missing tool call index when buffering streamed tool calls - #4824
fix(chat-completions): tolerate a missing tool call index when buffering streamed tool calls#4824rioyu123 wants to merge 6 commits into
Conversation
…ing streamed tool calls With buffer_streamed_tool_calls=True, the buffered path keyed tool calls on tool_call_delta.index. The OpenAI SDK's lenient chunk parsing leaves that index as None when an OpenAI-compatible provider omits it, so the replayed ChoiceDeltaToolCall failed pydantic validation and sorting the buffered calls raised TypeError once None and int keys coexisted. The unbuffered path already handled the same stream. Replay indexed calls first and give the index-less call the next free index when building the buffered chunk. Co-Authored-By: Claude Fable 5.1 <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: d2b3f8d5cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fallback_index = ( | ||
| max((call.index for call in ordered_calls if isinstance(call.index, int)), default=-1) | ||
| + 1 |
There was a problem hiding this comment.
When a stream contains a passthrough custom call at index 0 and a function call whose provider omitted index, this calculation also chooses fallback index 0 because it inspects only buffered_calls. buffer_tool_call_stream emits the custom call first, causing handle_stream to add 0 to ignored_tool_call_indexes; the replayed function call is then skipped as though it were the custom call, silently removing a valid function call from the completed response. Include passthrough_tool_call_indexes when selecting the fallback so the generated index cannot collide.
AGENTS.md reference: AGENTS.md:L145-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9930941. The replay fallback now considers both buffered integer indexes and passthrough tool-call indexes, so a missing-index function call receives a collision-free index. I added a regression test with a passthrough custom call at index 0 followed by an unindexed function call.
| tool_call_deltas = [ | ||
| cls._buffered_tool_call_delta(buffered_call) | ||
| for _, buffered_call in sorted(buffered_calls.items()) | ||
| cls._buffered_tool_call_delta(buffered_call, fallback_index=fallback_index) | ||
| for buffered_call in ordered_calls |
There was a problem hiding this comment.
When a provider supplies index on the opening delta but omits it on a later arguments-only delta, accumulation creates separate entries for the numeric index and None. This replay processes them as separate calls, so the None entry lacks call_id and name and _buffered_tool_call_delta raises instead of returning the otherwise complete function call. This is an unreliable-chunk pattern covered by buffer_streamed_tool_calls; when exactly one active call makes the association unambiguous, merge the unindexed continuation into it and reserve rejection for ambiguous multi-call streams.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9930941. An arguments-only unindexed continuation now merges into the existing unindexed slot or the sole active buffered function call. If multiple indexed calls make ownership ambiguous, buffering raises ModelBehaviorError before replay. I added regression tests for both the unambiguous merge and ambiguous rejection paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 993094149b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| occupied_indexes = passthrough_tool_call_indexes | { | ||
| call.index for call in ordered_calls if isinstance(call.index, int) | ||
| } | ||
| fallback_index = max(occupied_indexes, default=-1) + 1 |
There was a problem hiding this comment.
When default non-strict handling receives a custom/passthrough tool-call delta whose provider also omitted index, line 453 stores None in passthrough_tool_call_indexes; if the stream also contains a buffered indexed function call, max(occupied_indexes) then compares None with an integer and raises TypeError, instead of ignoring the unsupported custom call and returning the valid function call. Fresh evidence beyond the earlier indexed-passthrough collision is this opposite missing-index arrangement, where the passthrough index itself is absent; filter the passthrough set to integer indexes before calculating the fallback.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dacd81d. Passthrough identity is now recorded only when the provider supplies an integer index, so None cannot enter numeric fallback allocation or capture a later unindexed function call. I added parameterized regression coverage for a missing-index custom call followed by a function call with index 0 and with index omitted; both cases pass, and the complete streaming test module now has 115 passing tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dacd81d21e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tool_call_index = tool_call_delta.index | ||
| if not isinstance(tool_call_index, int) and not tool_call_delta.id: |
There was a problem hiding this comment.
When an indexed opening delta is followed by an index-less continuation that repeats the same id, this condition is false, so the continuation is stored in the None slot instead of the existing call. A normal arguments continuation that omits name then makes replay raise “without a function name”; if it repeats the name, replay emits a duplicate call. Fresh evidence beyond the earlier arguments-only case is that the repeated ID makes ownership unambiguous, so match it against existing call_id values before allocating a new unindexed slot.
AGENTS.md reference: AGENTS.md:L145-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — addressed in 3a2eef5. Missing-index deltas now resolve a unique repeated function-call ID before allocating the compatibility None slot, so continuations stay attached to the existing indexed call. The same ownership path also rejects duplicate or cross-domain ID ambiguity before mutation. Regression coverage now includes repeated-ID continuations with and without a repeated name, late IDs, passthrough-ID routing, and named continuations into an existing unindexed slot; the complete stream test module passes with 137 tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2eef5c45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tool_call_index = tool_call_delta.index | ||
| if not isinstance(tool_call_index, int): |
There was a problem hiding this comment.
When a function-call opening omits index but a later continuation supplies an integer index and repeats the same id, ID matching is skipped because it only runs for non-integer deltas. The continuation therefore creates a second buffered call instead of completing the None-keyed call; replay then either raises because one half lacks a name or emits duplicate partial calls if the continuation repeats the name. Match the ID before treating a newly supplied index as a distinct call, and re-key the existing unindexed call when the match is unique.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 10195b50. Integer-index deltas now match a unique None-keyed buffered call by ID and re-key that same call before merging. Occupied buffered or passthrough destinations and ambiguous same-ID ownership fail before state mutation.
I added regression coverage for late indexes with and without a repeated name, the provider-supplied replay index, passthrough-index collisions, and mutation-sensitive failure assertions. The focused streaming module passes all 142 tests; the full Windows suite remains at the unchanged 19-failure platform baseline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10195b50c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(tool_call_index, int): | ||
| if None in matching_indexes: |
There was a problem hiding this comment.
When the sole opening delta contains the call ID and function name but omits index, and an arguments continuation later supplies an unused integer index while omitting id, matching_indexes is empty, so this branch leaves the opening under None and creates a second buffered call under the integer index. Replay then raises because the continuation lacks an ID/name instead of returning the complete call. Fresh evidence beyond the prior repeated-ID late-index finding is the repository's established continuation shape in test_stream_response_buffers_tool_call_deltas_when_enabled, where subsequent argument deltas omit id; reconcile the sole None entry when ownership is unambiguous.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 122fda0. A sole ID-less late-index continuation now migrates the unindexed call only when function names are compatible; a different explicit name remains a distinct call, and ambiguous multi-call ownership still fails before mutation. Added success, distinct-name, and atomic ambiguity coverage.
| is_unindexed_passthrough_continuation = ( | ||
| is_unindexed_untyped_continuation |
There was a problem hiding this comment.
When a custom/passthrough opening has an ID but omits index, and a later continuation repeats that ID while supplying an index but omitting type, this predicate cannot recognize the continuation because it requires the new delta to remain unindexed. The new index is also absent from passthrough_tool_call_indexes, so _should_buffer_tool_call_delta classifies the continuation as a function call; replay then raises because that buffered entry has no function name, causing default non-strict handling to fail instead of ignoring the unsupported custom call and preserving any valid function calls. Match known passthrough IDs regardless of whether the continuation introduces an integer index, and promote that index into the passthrough tracking state.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 122fda0. Untyped continuations now route by a known passthrough ID even when they introduce an integer index; a free index is promoted and restored for later deltas, while owner changes and function-index collisions fail before mutation. Added promotion and collision coverage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 122fda0c61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| buffered_name = buffered_calls[None].name | ||
| if not function_name or not buffered_name or function_name == buffered_name: |
There was a problem hiding this comment.
When a completed index-less call is followed by a distinct invocation whose opening supplies a new integer index but delays its ID, this condition treats an identical function name as proof that the delta belongs to the old call. The later ID then overwrites the first call's ID while both argument payloads are concatenated, silently dropping one invocation. Fresh evidence beyond the earlier late-index finding is the added different-name test at lines 1698-1742: changing both calls to invoke the same function turns that supported two-call shape into corruption, so this ambiguous case should fail rather than re-key.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
| is_unindexed_untyped_continuation | ||
| and saw_passthrough_tool_call | ||
| and ( | ||
| not tool_call_delta.id |
There was a problem hiding this comment.
When a nonzero choice contains a tool call, it sets saw_passthrough_tool_call even though the default handler later ignores that choice and processes choice 0. If the sole index-less function call in choice 0 subsequently emits an ID-less, function-less metadata delta such as extra_content or provider_specific_fields, this branch interprets the unrelated nonzero choice as an ownership conflict and raises instead of merging the delta into the sole buffered call. Fresh evidence beyond the earlier unindexed-continuation comments is that the conflicting state originates from a different choice, so track passthrough ownership only for choice 0 here.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
1 participant
Summary
This pull request makes buffered Chat Completions resilient to OpenAI-compatible providers that omit
tool_calls[].indexon function-call deltas.Previously, a recoverable stream could fail validation, split one logical function call into multiple incomplete calls, attach a continuation to the wrong owner, or silently lose a function call when a generated fallback index collided with a passthrough tool call. The fix preserves a usable completed response when ownership is unambiguous and fails early when it is not, preventing silent tool-call corruption.
The buffered path now:
provider_specific_fieldsandextra_contenton late-ID function deltas; andModelBehaviorErrorbefore mutation for ambiguous ownership, owner changes, and buffered/passthrough index collisions.This improves compatibility without broadening the official OpenAI streaming contract. Multiple genuinely distinct function calls whose provider supplies no usable index or unique identity remain unsupported; providers can emit integer indexes, or callers can disable
buffer_streamed_tool_calls.Test plan
tests/models/test_openai_chatcompletions_stream.pywith missing-index, repeated/late ID, name ownership, mixed replay ordering, passthrough promotion, metadata, fallback-collision, and ambiguity coverage: 148 passed.ruff format,ruff check, optional-truthiness validation,mypy src, andpyrightpass.PYTHONUTF8=1, for an effective 8,362 passing cases under the required test environment.Issue number
Closes #4823
Checks
.agents/skills/code-change-verification/scripts/run.sh(makeis unavailable on this Windows host, so I ran the Makefile-equivalent commands directly)/reviewbefore submitting this PR and addressed the findings