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 11 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 👍 / 👎.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Once an unindexed passthrough call has been seen, any later unindexed delta with no type, no function, and no id is classified as a passthrough continuation. But buffered_calls[None] can also exist then for an unindexed function call, so that delta is ambiguous and may belong to the buffered function. Could this raise instead of routing to passthrough, with a mixed unindexed passthrough+function regression?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9628029f0c
ℹ️ 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".
| is_unindexed_untyped_continuation = ( | ||
| not isinstance(tool_call_delta.index, int) and is_untyped_continuation | ||
| ) |
There was a problem hiding this comment.
When default non-strict buffering receives an index-less custom call followed by an untyped continuation that omits the repeated ID but introduces an integer index, this predicate is false solely because the continuation is now indexed. Since no passthrough index has been recorded, the continuation falls through to _should_buffer_tool_call_delta(), creates an empty function call, and makes replay raise “without a tool call id,” also preventing any valid buffered function calls from being emitted. Fresh evidence beyond the existing repeated-ID continuation comment is that an ID-less continuation with a sole unindexed passthrough owner is still misclassified; reset and narrow the ownership resolver rather than adding another special-case branch.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 14dd22b. An ID-less indexed continuation now promotes a sole unindexed passthrough owner only when a passthrough-specific payload makes the association unambiguous. The promoted index and ID mapping stay aligned for later chunks, while buffered-function index collisions fail before mutation. Added regression coverage for the promotion, subsequent ID-based continuation, and a valid buffered function; the full stream-handler module now passes 152 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14dd22b64a
ℹ️ 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".
| elif len(buffered_calls) == 1: | ||
| sole_index = next(iter(buffered_calls)) | ||
| sole_name = buffered_calls[sole_index].name | ||
| if not sole_name or sole_name == function_name: | ||
| tool_call_index = sole_index |
There was a problem hiding this comment.
When an indexed foo call is already buffered and a second foo invocation starts without index or id before receiving its ID later, this branch assigns the second opening to the first call solely because their names match. Its arguments are consequently concatenated onto the first invocation, while the late ID either creates an incomplete None entry or produces two partial calls, causing replay to fail or corrupt the calls instead of rejecting the ambiguity. Fresh evidence beyond the previously reported inverse late-index case is this indexed-first/unindexed-second ordering, which remained distinct before this patch; reset and narrow the ownership resolver rather than adding another name-based special case.
AGENTS.md reference: AGENTS.md:L97-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9a2eadd. A repeated function name is no longer treated as ownership evidence for an unindexed, ID-less delta when an indexed call is already buffered. Arguments-only continuations and same-ID continuations remain supported; the ambiguous repeated-name case now raises ModelBehaviorError before accumulation. Added regression coverage, and the full stream-handler module passes 152 tests.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a2eadddb8
ℹ️ 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".
| passthrough_tool_call_indexes_by_id: dict[str, int | None] = {} | ||
| unindexed_passthrough_tool_call_count = 0 | ||
| saw_passthrough_tool_call = False |
There was a problem hiding this comment.
These new ID/count states support a large resolver that infers, reindexes, and even rewrites custom/passthrough continuations, although the required change is only to buffer a function call whose provider omitted index; passthrough calls were already forwarded, and ambiguous provider streams already have the supported alternative of disabling buffer_streamed_tool_calls. After the repeated ownership edge cases documented in this review, retaining this parallel state machine makes unrelated custom-call behavior part of the patch and violates the repository's mandatory complexity-reset checkpoint; reset the implementation to the narrow missing-function-index compatibility slot, forward passthrough calls unchanged, and reject ambiguous ownership rather than adding more resolver branches.
AGENTS.md reference: AGENTS.md:L97-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 0046b18. I reset the implementation to the narrow missing-function-index contract: passthrough deltas are forwarded unchanged, only their existing integer indexes are retained to avoid replay collisions, and ambiguous or contradictory ownership fails before mutation. The passthrough ID/count, promotion, reindexing, and type-rewrite machinery has been removed, and the combinatorial suite was replaced with focused contract and unsupported-case coverage. The complete stream-handler module now passes 117 tests; the net PR diff is 114 runtime lines and 284 test lines.
|
@sylvesterkaczmarek Good catch — confirmed. An anonymous unindexed delta cannot be attributed safely once both an unindexed passthrough call and an unindexed function call are active. I changed that path to raise |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2 participants
Summary
This pull request makes buffered Chat Completions resilient when an OpenAI-compatible provider omits
tool_calls[].indexfrom function-call deltas.The buffered path now:
ModelBehaviorErrorbefore mutation when ownership is ambiguous or conflicting; andThe change intentionally does not infer, promote, reindex, or rewrite passthrough ownership. Multiple distinct function calls without enough index or ID information remain unsupported; affected callers can disable
buffer_streamed_tool_calls.Test plan
tests/models/test_openai_chatcompletions_stream.py: 123 passed.PYTHONUTF8=1.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