fix(streaming): handle dupe-index delta entries - #3204
hrolfurinn wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57597c785c
ℹ️ 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".
|
Thanks for taking the time to work on this! We’ve merged the fix for duplicate-index tool-call deltas in #3425, so I’m closing this as a duplicate. We appreciate the contribution. |
|
@marcuswood-oai Glad to see a fix is in. Would have appreciated you landing it via this PR. Regardless, glad it's closed. My inbox has been crowded by AI PR dupes referencing this fix for the past four months. And also with comments on Chris' original Issue he posted when we found the bug. I can't imagine what your inbox must look like. Happy someone's tying up all the loose ends. |
Changes being requested
Handles case where multiple tool call entries in a single streaming delta share the same index. That dupe index shape is permissible according to the spec. Previously could result in initial tool call arg chunk being dropped, e.g. intended arguments
{ "city": "Reykjavik"}become truncatedcity": "Reykjavik" }. Bug was observed against vLLM with spec. decoding enabled.Route the first delta through the index-merge path. Duplicate-index entries now merge instead of becoming separate list elements. The early-return branches (
key not in acc,acc_value is None) copied the delta list as-is, skipping the merge. Same fix in_assistants.py._completions.pynow seeds initial snapshots throughaccumulate_delta.Additional context & links
Fixes #3201