fix: merge list entries by logical index instead of physical position (#3201)#3521
fix: merge list entries by logical index instead of physical position (#3201)#3521rkfshakti wants to merge 2 commits into
Conversation
…openai#3201) accumulate_delta assumed that an indexed list entry's 'index' field matches its physical position in the Python list. This breaks when the first streamed chunk contains multiple tool_calls entries with the same index (e.g. from speculative decoding with vLLM). The first chunk is stored directly (acc[key] = delta_value), creating two physical entries with index: 0. Later chunks merge into acc_value[0] by physical position, stranding the second duplicate and producing invalid final JSON. Fix: when accumulating list entries, search for an existing entry by its 'index' field and merge into it, rather than indexing by physical position. New entries are inserted at their logical index.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feeea11a35
ℹ️ 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".
| if is_dict(existing) and existing.get("index") == index: | ||
| acc_value[i] = accumulate_delta(existing, delta_entry) | ||
| found = True | ||
| break |
There was a problem hiding this comment.
Normalize first-chunk duplicate indexes before later merges
When tool_calls is first added to the snapshot it is still copied directly, so this loop can receive an acc_value that already contains two dicts with the same index from the first chunk. In that scenario this code merges the next delta into only the first matching entry and then breaks, leaving the earlier duplicate entry stranded; the example in the commit message still produces duplicated index 0 entries and broken accumulated arguments unless the existing list is coalesced before or during this merge.
Useful? React with 👍 / 👎.
…ctly (openai#3201) Codex P1 review: when tool_calls is first added to the snapshot it is copied directly (acc[key] = delta_value), so a first chunk with two entries at the same index creates two physical entries that later merges can't fix — the merge only hits the first matching entry and breaks, leaving the earlier duplicate stranded. Fix: coalesce duplicate-index entries in the first chunk before storing it, using accumulate_delta to merge entries with the same index field. Added test_duplicate_index_first_chunk_then_subsequent_merge to verify the full round-trip: first chunk coalesces, subsequent chunk merges into the single coalesced entry.
|
Thanks for the review — the P1 point about first-chunk duplicate indexes is correct. Pushed P1 — Normalize first-chunk duplicate indexes: When Added @codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Problem
accumulate_deltaassumes that an indexed list entry's"index"field matches its physical position in the Python list. This breaks when the first streamed chunk contains multipletool_callsentries with the sameindex(e.g. from speculative decoding with vLLM / Kimi K2.6).Example first chunk
{ "delta": { "tool_calls": [ {"index": 0, "id": "call_abc", "function": {"name": "list_files"}, "type": "function"}, {"index": 0, "function": {"arguments": " {\""}} ] } }Because this is the first
tool_callsvalue, the accumulator stores the list directly (acc[key] = delta_value), so the snapshot now contains two physical entries with index: 0. Later chunks merge intoacc_value[0]by physical position, stranding the second duplicate and producing invalid final JSON:[ {"index": 0, "id": "call_abc", "function": {"name": "list_files", "arguments": "path\": \".\"}"}}, {"index": 0, "function": {"arguments": " {\""}} ]Fix
When accumulating list entries, search for an existing entry by its
"index"field and merge into it, rather than indexing by physical position:This ensures entries with the same logical index are merged into one, regardless of their physical position in the list.
Tests
Added
tests/lib/streaming/test_deltas.pywith 4 test cases:test_duplicate_index_first_chunk_merges— first chunk with two entries at index 0 merges into onetest_duplicate_index_subsequent_chunk_merges— subsequent chunk with same index merges into existingtest_different_indexes_accumulate_separately— different indexes accumulate separatelytest_string_accumulation_unchanged— basic string accumulation still worksFixes #3201