fix(streaming): merge duplicate-index entries in first tool_call chunk - #3633
fix(streaming): merge duplicate-index entries in first tool_call chunk#3633Xsidz wants to merge 1 commit into
Conversation
Fixes openai#3201: when the first streamed chunk contained multiple tool_call delta entries sharing the same index (e.g. two partial updates for index 0), accumulate_delta stored the raw list on the `key not in acc` fast-path, bypassing the index-based merge logic. Later deltas for the same index then only merged into the first physical entry, stranding the second and producing truncated/invalid arguments JSON. Fix: seed indexed-dict lists with [] on first sight and fall through to the existing merge path. Also guard the primitive-extend branch with `acc_value and ...` so the empty seed list doesn't short-circuit there.
![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: ba80359cc0
ℹ️ 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 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 address that feedback".
| # for lists of non-dictionary items we'll only ever get new entries | ||
| # in the array, existing entries will never be changed | ||
| if all(isinstance(x, (str, int, float)) for x in acc_value): | ||
| if acc_value and all(isinstance(x, (str, int, float)) for x in acc_value): |
There was a problem hiding this comment.
When an accumulated list is empty and the next delta contains primitive entries (for example, accumulate_delta({"items": []}, {"items": ["value"]})), this new guard skips the primitive-list branch and the loop below raises TypeError because the string is not a dictionary. Empty primitive lists previously extended correctly, and the function explicitly supports non-dictionary lists; distinguish the seeded indexed-dict case using delta_value rather than treating every empty accumulator as an indexed-object list.
Useful? React with 👍 / 👎.
| if is_list(delta_value) and delta_value and is_dict(delta_value[0]) and "index" in delta_value[0]: | ||
| acc[key] = [] |
There was a problem hiding this comment.
When the stream's initial SSE chunk itself contains multiple tool_calls entries with the same index, this branch is never reached: _accumulate_chunk() returns _convert_initial_chunk_into_snapshot() while the snapshot is None, and that conversion copies choice.delta.to_dict() directly. The duplicate entries therefore remain split, so later deltas merge only into the first physical entry and the final arguments can still be truncated or invalid; the same indexed-list normalization needs to be applied during initial snapshot conversion.
Useful? React with 👍 / 👎.
| if is_list(delta_value) and delta_value and is_dict(delta_value[0]) and "index" in delta_value[0]: | ||
| acc[key] = [] |
There was a problem hiding this comment.
None accumulator path
In the normal role-first stream, the parsed message snapshot has tool_calls=None, and _accumulate_chunk() serializes that snapshot with model_dump() whose exclude_unset default is false. Consequently tool_calls is already present in acc, so the first later tool-call chunk takes the acc_value is None branch and stores the duplicate-index list raw; this new key not in acc normalization never runs for the target scenario, leaving the reported truncated-arguments bug unfixed. Apply the indexed-list normalization when replacing a None accumulator as well.
Useful? React with 👍 / 👎.
Summary
Fixes #3201.
accumulate_deltainsrc/openai/lib/streaming/_deltas.pyhad a fast-path for the first-time-seen case:When a first streaming chunk contained two
tool_callsentries with the sameindex: 0(the API sends partial updates for the same item in one chunk), this stored the raw list directly — bypassing the index-based merge logic used for all subsequent chunks. Later chunks forindex: 0then merged into physical position 0, ignoring the second entry, and the arguments JSON ended up truncated/invalid.Fix: detect indexed-dict lists on first sight, seed
acc[key] = [], and fall through to the existing merge path. Also guard the primitive-listextendbranch withacc_value and ...so the empty seed list doesn't incorrectly short-circuit intoextend.Test plan
tool_calls[index:0]entries merges correctly; subsequent chunk appends to the same entry.tests/test_streaming.py+tests/lib/: 276 passed, 0 failures.