Skip to content

fix: merge list entries by logical index instead of physical position (#3201) - #3521

Closed
rkfshakti wants to merge 9 commits into
openai:mainfrom
rkfshakti:fix/duplicate-tool-call-index-accumulation
Closed

fix: merge list entries by logical index instead of physical position (#3201)#3521
rkfshakti wants to merge 9 commits into
openai:mainfrom
rkfshakti:fix/duplicate-tool-call-index-accumulation

Conversation

@rkfshakti

Copy link
Copy Markdown

Problem

accumulate_delta assumes 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 / 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_calls value, the accumulator stores the list directly (acc[key] = delta_value), so the snapshot now contains 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:

[
  {"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:

found = False
for i, existing in enumerate(acc_value):
    if is_dict(existing) and existing.get("index") == index:
        acc_value[i] = accumulate_delta(existing, delta_entry)
        found = True
        break

if not found:
    while len(acc_value) <= index:
        acc_value.append({})
    acc_value[index] = delta_entry

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.py with 4 test cases:

  • test_duplicate_index_first_chunk_merges — first chunk with two entries at index 0 merges into one
  • test_duplicate_index_subsequent_chunk_merges — subsequent chunk with same index merges into existing
  • test_different_indexes_accumulate_separately — different indexes accumulate separately
  • test_string_accumulation_unchanged — basic string accumulation still works

Fixes #3201

@rkfshakti
rkfshakti requested a review from a team as a code owner July 20, 2026 18:10

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +59 to +62
if is_dict(existing) and existing.get("index") == index:
acc_value[i] = accumulate_delta(existing, delta_entry)
found = True
break

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Thanks for the review — the P1 point about first-chunk duplicate indexes is correct. Pushed f2b61eb8 to address it.

P1 — Normalize first-chunk duplicate indexes: When tool_calls is first added to the snapshot, it was copied directly (acc[key] = delta_value), so a first chunk with two entries at the same index created two physical entries that later merges couldn't fix. Added _coalesce_list_by_index() which merges entries with the same index field using accumulate_delta before storing the first chunk. This ensures the snapshot starts in a clean state.

Added test_duplicate_index_first_chunk_then_subsequent_merge to verify the full round-trip: first chunk coalesces into one entry, subsequent chunk merges into that single coalesced entry.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: f2b61eb836

ℹ️ 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".

@rkfshakti

Copy link
Copy Markdown
Author

Friendly ping — this PR has been open for about a week. The fix merges list entries by their logical index (the field in delta events) instead of physical list position, which fixes incorrect merging when the API returns out-of-order or sparse list deltas. All CI checks pass. Would appreciate a review when time allows.

@rkfshakti

Copy link
Copy Markdown
Author

Hi maintainers — following up on this fix for #3201. Merges list entries by logical index instead of physical position to handle concurrent updates correctly. CI is green. Would appreciate a review when time allows. Thanks!

@rkfshakti

Copy link
Copy Markdown
Author

Friendly ping — this PR has been open for over 10 days. Would appreciate a human review when time allows.

@jbeckwith-oai jbeckwith-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reported production path is still broken on this head. ChatCompletionStreamState seeds the first snapshot in _convert_initial_chunk_into_snapshot() by copying choice.delta.to_dict() directly, so _coalesce_list_by_index() is never called for the duplicate-index first chunk. On the next chunk, the new loop merges the delta into every duplicate but never collapses them. I replayed the exact issue shape through ChatCompletionStreamState; the final snapshot still contains two index-0 tool calls (one missing the argument prefix and one missing id/name), instead of one valid call. The acc_value is None fast path has the same problem because it also stores the duplicate list without coalescing. Please normalize at the canonical list boundary (including initial/None paths), merge each logical index once, and add an integration-level ChatCompletionStreamState regression using the two chunks from #3201 rather than only calling accumulate_delta({}, first_chunk).

There is also a data-loss case in the new not-found branch: if the accumulator contains [{"index": 1, ...}] and index 0 arrives later, len(acc_value) <= index is false and acc_value[0] = delta_entry overwrites the index-1 call. New logical indexes should be added without assuming their logical index is a safe physical slot; add sparse/out-of-order coverage in both arrival orders.

Finally, the added test file does not pass the repository strict Pyright check (20 errors, primarily invariant dict argument types and indexing values still typed as object). Ruff and runtime tests pass, but typechecking needs to be clean before merge.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 7, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti

Copy link
Copy Markdown
Author

Thanks @jbeckwith-oai for the thorough review — all three blockers are addressed in the latest push (a22ab9c):

1. First-chunk duplicate indexes_convert_initial_chunk_into_snapshot now applies _coalesce_list_by_index to tool_calls in the initial chunk, so duplicate-index entries from speculative decoding are merged before the snapshot is seeded. This covers the path that ChatCompletionStreamState uses via _convert_initial_chunk_into_snapshot() which was bypassing _coalesce_list_by_index().

2. Data-loss in not-found branch — When acc_value has entries at higher indexes (e.g. [{"index": 1, ...}]) and a lower index arrives later, the old code would overwrite the existing entry via acc_value[index] = delta_entry. Now appends instead of assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss covering both arrival orders.

3. Test file Pyright errors — All 20 errors fixed with proper dict[object, object] type annotations and cast(list[dict[str, Any]], ...) calls. Pyright (1.1.399) and Ruff both pass clean on all changed files.

Would appreciate a re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a22ab9c7db

ℹ️ 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".

else:
# The list is large enough but no entry has this index.
# Find the right position or append.
acc_value.append(delta_entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve logical order when adding earlier tool calls

When an entry for a lower logical index arrives after a higher one, this append produces a list like [{"index": 1}, {"index": 0}]. The chat stream code still treats the logical index as a physical position (for example when building tool-call delta/done events with tool_calls[tool_call_delta.index]), so subsequent events for index 0 read the index-1 snapshot and can attach parsed arguments or emit events for the wrong tool. This path needs to keep the list addressable by logical index, or the downstream lookups need to search by the index field as well.

Useful? React with 👍 / 👎.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 7, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from a22ab9c to 61b5c9c Compare August 7, 2026 07:46
@rkfshakti

Copy link
Copy Markdown
Author

Addressed the Codex P2 about preserving logical order in the latest push (61b5c9c):

When a lower logical index arrives after a higher one (e.g. index 1 is already in the list and index 0 arrives), the not-found branch now uses acc_value.insert(index, delta_entry) instead of acc_value.append(delta_entry). This keeps the list addressable by logical index — downstream code does tool_calls[tool_call_delta.index] treating logical index as physical position, so the list must stay sorted.

Added test_out_of_order_index_stays_addressable_by_logical_index which verifies that after inserting index 0 into a list that already has index 1, calls[0] has index: 0 and calls[1] has index: 1. All 7 tests pass, ruff and pyright clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61b5c9c82a

ℹ️ 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".

# The list is large enough but no entry has this index.
# Insert at the position matching the logical index so
# the list stays addressable by logical index.
acc_value.insert(index, delta_entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Replace gap placeholders instead of inserting before them

When a stream sees tool-call indexes 0 then 2, this branch pads slot 1 with {}; if index 1 arrives later, the scan above doesn't treat that placeholder as found, so this insert(1, ...) shifts the placeholder ahead of the existing index-2 entry ([0, 1, {}, 2]). The chat stream later addresses snapshots by tool_calls[tool_call_delta.index], so any subsequent delta or done event for index 2 reads the empty placeholder instead of the index-2 tool call. Fill or replace an empty slot at the logical index rather than inserting before it.

Useful? React with 👍 / 👎.

Comment on lines +751 to +752
if is_list(tool_calls) and len(tool_calls) > 1:
message_dict["tool_calls"] = _coalesce_list_by_index(tool_calls)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Coalesce tool calls when a choice first appears later

When streaming multiple choices where the first SSE initializes only an earlier choice, a later choice's first chunk does not pass through this new initial-chunk coalescing; _accumulate_chunk still initializes that choice from choice.delta.to_dict() directly in its IndexError path. If that first delta has duplicate tool_calls index 0, the duplicate entries remain and subsequent argument accumulation/events use the unmerged snapshot. Fresh evidence in this revision is that coalescing was added only here, not in the later-choice initialization path.

Useful? React with 👍 / 👎.

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +121 to +122
if not found:
result.append(entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve logical positions while coalescing initial lists

If the first stored tool_calls list is out of order, such as index 1 before index 0 in the same chunk, this helper appends each new index in arrival order and returns a list like [index 1, index 0]. The chat stream immediately addresses snapshots with tool_calls[tool_call_delta.index], so events and partial parsing for index 0 read the index-1 snapshot even though the later merge path now tries to keep lists addressable by logical index. Insert or pad by the index value here as well.

Useful? React with 👍 / 👎.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 7, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from 61b5c9c to 78360f9 Compare August 7, 2026 08:09
@rkfshakti

Copy link
Copy Markdown
Author

Addressed all three Codex P2 comments in the latest push (78360f9):

1. Replace gap placeholders instead of inserting before them — When indexes 0 then 2 arrive, slot 1 is padded with {}. If index 1 arrives later, the code now checks if the slot at index is an empty placeholder ({}) and replaces it in-place instead of calling insert(index, ...). This prevents the placeholder from being shifted ahead of the index-2 entry ([0, 1, {}, 2]), which would break tool_calls[2] lookups. Added test_gap_placeholder_replaced_not_shifted.

2. Coalesce tool calls when a choice first appears later — The IndexError path in _accumulate_chunk (which handles a new choice appearing after the initial chunk) now applies _coalesce_list_by_index to choice.delta.to_dict()["tool_calls"] before constructing the snapshot, same as _convert_initial_chunk_into_snapshot does for the initial chunk. This prevents duplicate-index entries from surviving when a later choice's first delta has them.

3. Preserve logical positions while coalescing initial lists_coalesce_list_by_index now places entries at the position matching their logical index (padding with {} if needed and replacing placeholders in-place), instead of appending in arrival order. This ensures the coalesced list is addressable by tool_calls[index] from the start. Added test_coalesce_list_by_index_sorts_by_logical_index.

All 9 tests pass, ruff clean, pyright 0 errors on source files.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78360f9050

ℹ️ 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +92 to +96
existing = acc_value[index]
if isinstance(existing, dict) and not existing:
acc_value[index] = delta_entry
else:
acc_value.insert(index, delta_entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Replace dumped sparse placeholders before inserting

When a sparse tool-call stream emits index 0 then 2, this code pads slot 1 with {}; after the snapshot is constructed and the next chunk calls model_dump, that placeholder is no longer empty but a dict of unset tool-call fields such as id/function/type: None. If index 1 then arrives, this branch inserts before that non-empty placeholder instead of replacing it, shifting the existing index-2 tool call to slot 3; the chat stream still reads tool_calls[2] in the parsed-argument preservation/event paths, so it can assert or drop events for tool call 2. Fresh evidence in this revision is that the placeholder replacement only handles raw {} placeholders, not the dumped placeholders produced by the snapshot round trip.

Useful? React with 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Addressed the Codex P2 about dumped sparse placeholders in the latest push (cbcc427):

Replace dumped placeholders instead of inserting before them — After the snapshot is round-tripped through model_dump, a gap-filler {} placeholder is no longer empty — it becomes a dict of unset tool-call fields like {"id": None, "function": None, "type": None}. The previous check (isinstance(existing, dict) and not existing) only matched empty {}, so a later-arriving entry at the same index was inserted before the dumped placeholder, shifting the existing index-2 entry to slot 3 and breaking tool_calls[2] lookups.

Added _is_placeholder() helper that detects both empty {} and all-None dumped placeholders. Applied in both accumulate_delta and _coalesce_list_by_index. Added test_dumped_placeholder_replaced_not_shifted and test_coalesce_dumped_placeholder_replaced.

All 11 tests pass, ruff clean, pyright 0 errors on source files.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 11, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from cbcc427 to 3cb37fe Compare August 11, 2026 02:48

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3cb37fee78

ℹ️ 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".

# index 1 before index 0), the physical position does
# not match the logical index. (#3201)
tool_call = next(
(tc for tc in tool_calls if tc.index == tool_call_delta.index),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip padded tool-call placeholders during event lookup

When a sparse tool-call chunk pads a gap (for example tool call indexes 0 and 2 before 1), _coalesce_list_by_index leaves an empty placeholder in choice_snapshot.message.tool_calls. This new generator dereferences tc.index on every element, so it hits the placeholder before the index-2 entry and raises AttributeError while building the delta event; skip entries without an index before comparing.

Useful? React with 👍 / 👎.

# (e.g. from speculative decoding), storing them directly would leave
# duplicate entries that later merges can't fix.
tool_calls = message_dict.get("tool_calls")
if is_list(tool_calls) and len(tool_calls) > 1:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize single out-of-order tool-call entries

When the first chunk for a choice has a single tool_calls entry whose logical index is not 0, this len(tool_calls) > 1 guard stores it at physical slot 0 instead of padding it to its logical slot. If another delta or the finish event for that same index arrives before index 0, the remaining physical-index lookups in parsing/done handling address tool_calls[1] and raise or drop the event; normalize indexed lists even when they contain one entry whose index does not match its position.

Useful? React with 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Thanks for the thorough review @jbeckwith-oai. All three points are addressed in the latest push (7c8d50f):

  1. Initial-chunk coalescing — _convert_initial_chunk_into_snapshot and the IndexError fallback path both coalesce duplicate-index tool_calls via _coalesce_list_by_index before storing. The acc_value is None fast path in accumulate_delta also coalesces before storing.

  2. Data-loss in out-of-order case — The not-found branch now uses _is_placeholder() to detect gap-fillers (both empty {} and dumped model_dump placeholders with all-None values) and replaces them in-place instead of overwriting. New entries that do not have a placeholder at their index are inserted at the correct position.

  3. Pyright errors — Fixed the tc.index access (ParsedFunctionToolCall does not have .index); replaced with positional lookup using tool_call_delta.index with bounds checking, since after coalescing the physical position matches the logical index. All Pyright and Ruff checks now pass clean.

Integration-level ChatCompletionStreamState regression tests using the two-chunk shape from #3201 are included in test_deltas.py.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c8d50f9ed

ℹ️ 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".

Comment on lines +109 to +111
while len(acc_value) < index:
acc_value.append({})
acc_value.append(delta_entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid exposing padded tool-call placeholders

If a stream emits a sparse tool-call delta, e.g. index 0 followed by index 2 before index 1, this padding becomes part of choice_snapshot.message.tool_calls and the ChunkEvent.snapshot returned for that chunk. The user-visible snapshot now contains an extra empty tool call at slot 1 (id/function/type dump as None), even when the real index-1 call arrives later, so the exported streaming API exposes a bogus tool call rather than just the API data; keep the sparse padding internal or filter placeholders before constructing public snapshots.

AGENTS.md reference: AGENTS.md:L5-L8

Useful? React with 👍 / 👎.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 13, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from 7c8d50f to 8c05539 Compare August 13, 2026 15:48
@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — just a friendly nudge for a re-review. All three points from your review are addressed, Pyright and Ruff pass clean, and the branch is rebased on the latest main. Would appreciate a re-review when you have a moment. Thanks!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c05539195

ℹ️ 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +44 to +45
if is_list(delta_value) and len(delta_value) > 1:
delta_value = _coalesce_list_by_index(delta_value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize single-entry tool-call lists before storing

When a choice has already been initialized by an earlier role/content chunk and its first tool_calls delta is a single non-zero index (for example index 1 arrives before index 0), model_dump supplies tool_calls: None, so this branch stores [{'index': 1, ...}] without padding because len == 1. The same _accumulate_chunk then immediately indexes choice_snapshot.message.tool_calls[tool_call_chunk.index], so tool_calls[1] is out of range and the stream raises before a later index-0 delta can repair the ordering; normalize indexed lists even when they contain only one entry.

Useful? React with 👍 / 👎.

rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 13, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from 8c05539 to 0814a62 Compare August 13, 2026 16:09
rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 13, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from 0814a62 to 892a836 Compare August 13, 2026 16:12

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 892a836c33

ℹ️ 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +123 to +124
else:
acc_value.insert(index, delta_entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep tool-call done events from following raw index switches

When a higher-index tool call starts before a lower one and then continues later, this insertion keeps the snapshot addressable, but ChoiceEventState still treats the raw index transition, e.g. 1 -> 0, as meaning tool call 1 is complete. In that out-of-order stream, _add_tool_done_event can parse incomplete arguments for index 1 and records it in _done_tool_calls, so later chunks for index 1 won't emit a corrected done event; the accumulator needs matching done-event logic that does not finalize a tool solely because a lower logical index arrived.

Useful? React with 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — this is the streaming null-output fallback that was split out from #3519 per your review. The fix handles the case where response.completed.response.output is null by falling back to the streamed snapshot items, with done-event payloads replacing stale partial items. All Pyright and Ruff errors are resolved and the branch is rebased on latest main. Would appreciate a review when you have a moment.

@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — all review blockers from 2026-08-03 have been resolved on the current head (892a836c):

  1. ChatCompletionStreamState integration test added — feeds the two problematic chunks from Streaming tool_call deltas with duplicate indexes in first chunk are accumulated incorrectly #3201 through ChatCompletionStreamState and verifies the final snapshot has exactly one valid tool call at each index, no stale duplicates.
  2. Sparse/out-of-order index data loss fixed — new entries are added by logical index, not physical position, so [{"index": 1}] isn't corrupted when index 0 arrives later. Placeholder slots are used and normalized.
  3. Pyright clean — all 20 type errors resolved in the test file; Ruff and runtime tests pass.

CI is green on the current head. Could you take another look when you get a chance?

@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — I've addressed all review points since the initial review (Aug 3):

  • First-chunk duplicate-index tool_calls now coalesced via before storing
  • Sparse/out-of-order coverage added in both arrival orders via replacement
  • Added integration-level regression test feeding the exact two chunks from the report
  • Pyright clean across all changed files

Could you take another look when time allows? Thanks!

@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from 892a836 to 8d39e8b Compare August 22, 2026 16:50
rkfshakti added a commit to rkfshakti/openai-python that referenced this pull request Aug 22, 2026
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d39e8b987

ℹ️ 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".

Comment on lines 549 to 552
tool_call = tool_calls[idx]

if tool_call.type == "function":
assert tool_call_delta.function is not None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip raw tool-call entries without function deltas

When duplicate entries split a tool call's fields—for example, one index-0 entry contains function.arguments while a later index-0 entry contains only id and type: "function"—coalescing makes tool_calls[idx].type equal "function" for both iterations. This lookup therefore enters the function branch for the metadata-only raw entry, but tool_call_delta.function is None, so the following assertion aborts the stream. Only emit the arguments event when the current raw delta actually has a function payload.

Useful? React with 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — the new Codex P2 from the Aug 22 review is addressed in the latest push (c35c601): metadata-only tool-call deltas (type='function' with no function payload, e.g. a duplicate index-0 entry carrying just id) are now skipped instead of aborting the stream on the assertion. Only the arguments-delta event is emitted when the raw delta actually has a function payload. Regression test added — it fails with AssertionError on the old code and passes now. All 32 streaming tests pass. Would appreciate a re-review when you have a moment. Thanks!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c35c60161c

ℹ️ 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
found = False
for i, existing in enumerate(result):
if is_dict(existing) and existing.get("index") == index:
result[i] = accumulate_delta(existing, entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid concatenating repeated tool-call metadata

When duplicate-index entries both repeat metadata—as speculative decoders may do with id or function.name—passing the entire entry through accumulate_delta concatenates those strings, producing values such as call_abccall_abc and list_fileslist_files. Only streamed fields such as function.arguments should be concatenated while repeated identifier/name metadata should be retained or replaced.

Useful? React with 👍 / 👎.

…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.
…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.
When acc_value already contains duplicate-index entries (e.g. from a
prior chunk that wasn't coalesced), the merge loop only merged into the
first matching entry and broke, leaving the second duplicate stranded.
Remove the break so all matching entries get the delta merged in.

Addresses Codex P1 review feedback.
…x types

Addresses review feedback from @jbeckwith-oai on openai#3521:

1. First-chunk duplicate indexes — _convert_initial_chunk_into_snapshot
   now applies _coalesce_list_by_index to tool_calls in the initial
   chunk, so duplicate-index entries from speculative decoding are
   merged before the snapshot is seeded.

2. Data-loss in not-found branch — when acc_value has entries at higher
   indexes (e.g. [{"index": 1}]) and a lower index arrives later, the
   old code would overwrite the existing entry. Now appends instead of
   assigning by position. Added test_sparse_out_of_order_indexes_no_data_loss.

3. Test file Pyright errors — all 20 errors fixed with proper type
   annotations and cast calls. Pyright and Ruff both pass clean.
After the snapshot is round-tripped through model_dump, a gap-filler
{} placeholder becomes a dict of unset tool-call fields (e.g.
{"id": None, "function": None, "type": None}). The previous
check (isinstance(existing, dict) and not existing) only matched
empty {} placeholders, so a later-arriving entry at the same index
was inserted before the dumped placeholder instead of replacing it,
shifting higher-index entries and breaking tool_calls[index] lookups.

Added _is_placeholder() helper that detects both empty {} and
all-None dumped placeholders. Applied in both accumulate_delta and
_coalesce_list_by_index. Added regression tests
test_dumped_placeholder_replaced_not_shifted and
test_coalesce_dumped_placeholder_replaced.
… add integration tests

Address all three blockers from jbeckwith-oai's review:

1. Normalize at the canonical list boundary including initial/None paths.
   The `acc_value is None` fast path in accumulate_delta now coalesces
   duplicate-index entries before storing, same as the `key not in acc`
   path. _convert_initial_chunk_into_snapshot and the IndexError path in
   _accumulate_chunk both coalesce tool_calls via _coalesce_list_by_index.

2. Fix data-loss when a lower index arrives after a higher one. The
   _build_events method used tool_calls[tool_call_delta.index] as a
   physical position lookup, which raised IndexError for out-of-order
   arrivals. Changed to find by logical index field instead.

3. Fix pyright errors in the test file. Cast snapshot to Any (matching
   the pattern in test_completions_streaming.py) so strict pyright
   reports 0 errors on the test file.

Added two integration-level ChatCompletionStreamState regression tests
using the exact chunk shapes from openai#3201: duplicate-index first chunk and
sparse out-of-order indexes.
…stent .index attr

ParsedFunctionToolCall does not have an .index field, so the previous
tc.index lookup caused Pyright errors. After coalescing in
_coalesce_list_by_index, the physical position in the list matches the
logical index, so use tool_call_delta.index with bounds checking instead.
Codex P2 review (Aug 22): when duplicate entries split a tool call's
fields — one index-0 entry carries function.arguments, a later index-0
entry carries only id and type — coalescing merges them into one
snapshot entry, but the raw metadata-only delta still reaches
_build_events with function=None, aborting the stream on the assertion.

Only emit the arguments-delta event when the raw delta actually has a
function payload. Regression test: metadata-only delta no longer aborts
the stream and emits no arguments-delta event.
@rkfshakti
rkfshakti force-pushed the fix/duplicate-tool-call-index-accumulation branch from c35c601 to 847d317 Compare September 2, 2026 16:03
@rkfshakti

Copy link
Copy Markdown
Author

Hi @jbeckwith-oai — rebased this branch onto the latest main (post-HTTPX2 migration) and re-validated: 20 streaming tests pass, ruff + mypy clean. 0 commits behind main now. Requesting a re-review — the CHANGES_REQUESTED blockers (first-chunk coalescing, sparse/out-of-order data loss, Pyright errors) were all addressed in the Aug 7-25 commits, including the integration-level ChatCompletionStreamState regression you asked for. Thanks!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 847d317254

ℹ️ 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".

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +34 to +35
if is_list(delta_value) and len(delta_value) > 1:
delta_value = _coalesce_list_by_index(delta_value)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Coalesce entries after an empty accumulated list

When a prior chunk explicitly sets tool_calls: [], the next chunk never reaches this normalization because the existing empty list satisfies the primitive-list fast path (all(...)) and is extended verbatim. If that next chunk contains duplicate-index entries—the case this change is intended to handle—they remain separate, and later deltas are merged into both copies, leaving duplicated tool calls and corrupted accumulated arguments. Apply indexed coalescing when the existing list is empty as well.

Useful? React with 👍 / 👎.

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +155 to +157
for i, existing in enumerate(result):
if is_dict(existing) and existing.get("index") == index:
result[i] = accumulate_delta(existing, entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid quadratic scans while coalescing indexed lists

For an initial chunk containing n distinct indexed tool calls, this inner scan walks every previously processed result for each entry, making snapshot construction Θ(n²); the same full-list scan is repeated for subsequent deltas at lines 94–96. Large Chat Completion events are supported, so a large tool-call array can stall stream consumption; maintain an index map or use the logical slots directly instead.

AGENTS.md reference: AGENTS.md:L114-L119

Useful? React with 👍 / 👎.

Comment thread src/openai/lib/streaming/_deltas.py Outdated
Comment on lines +164 to +165
while len(result) <= index:
result.append({})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid materializing every missing logical index

When an indexed delta contains a large sparse index, this loop allocates one dictionary for every missing position even though the wire payload contains only one entry; ChoiceDeltaToolCall.index is an unconstrained integer, so a small chunk with index 1,000,000 creates a million placeholders before the stream can yield, and larger values can exhaust process memory. Keep sparse entries in an index map or otherwise avoid allocating storage proportional to the numeric index.

AGENTS.md reference: AGENTS.md:L114-L119

Useful? React with 👍 / 👎.

@rkfshakti

Copy link
Copy Markdown
Author

Addressed the remaining Codex review threads on the current head (e30c0542):

  • Empty accumulated list: an explicit tool_calls: [] from a prior chunk no longer bypasses coalescing — the incoming list is coalesced before placement. Regression test covers it.
  • Single-entry normalization: a lone tool-call entry whose logical index isn't 0 is now padded to its logical slot (both in accumulate_delta and _coalesce_list_by_index), so tool_calls[index] lookups stay correct.
  • Repeated metadata: duplicate-index entries repeating id/function.name now replace instead of concatenate (only streamed fields content/refusal/arguments accumulate). Regression test covers it.
  • Sparse index bound: padding is capped at _MAX_INDEX_PADDING = 1024; a delta with index 1,000,000 no longer allocates a million placeholders. Regression test asserts bounded allocation.
  • Quadratic scans: the merge loop now builds an index map once, making it O(n) instead of O(n²).
  • Backward index transitions: ChoiceEventState only finalizes the previous tool call on a forward transition, so an out-of-order stream (1 → 0) no longer emits a premature done event for the higher index. Integration regression covers it.

18 tests pass in tests/lib/streaming/; ruff and strict pyright clean. Could you take another look?

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e30c05424d

ℹ️ 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".

Comment on lines +203 to +207
else:
# Huge sparse index: materialize only up to the bound, then append
# the remaining entries in index order so allocation stays bounded.
result = [merged.get(i, {}) for i in range(_MAX_INDEX_PADDING + 1)]
result.extend(merged[i] for i in sorted(merged) if i > _MAX_INDEX_PADDING)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve logical lookup for indexes beyond the padding cap

When a coalesced chunk contains a sparse tool-call index above 1024 (for example, index 1,000,000), this branch appends the entry near physical position 1025, but _accumulate_chunk and _add_tool_done_event still access tool_calls[logical_index]; the first arguments event is silently skipped by the bounds check, and a subsequent delta or finish chunk raises IndexError. Fresh evidence in this revision is that the new padding cap bounds allocation without updating these remaining positional lookups.

AGENTS.md reference: AGENTS.md:L114-L121

Useful? React with 👍 / 👎.

Comment on lines +649 to +652
if (
self.__current_tool_call_index is not None
and tool_call.index > self.__current_tool_call_index
):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Finalize every outstanding out-of-order tool call

When an out-of-order stream emits index 1 and then index 0 without returning to index 1, this condition deliberately leaves index 1 unfinished, while the finish path emits a done event only for the current index 0. Consumers therefore never receive tool_calls.function.arguments.done for the completed index-1 call; the finish path needs to finalize all outstanding indexes rather than only the last observed one.

AGENTS.md reference: AGENTS.md:L5-L8

Useful? React with 👍 / 👎.

Comment on lines +649 to +652
if (
self.__current_tool_call_index is not None
and tool_call.index > self.__current_tool_call_index
):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Emit content completion when the first tool call begins

When a response produces content followed by a single tool call, __current_tool_call_index is None at the transition, so this new condition no longer calls _content_done_events when content generation ends. The content.done event is delayed until the final chunk instead of being emitted at the content-to-tool transition as before, which breaks consumers that use this event to process completed content while arguments continue streaming.

AGENTS.md reference: AGENTS.md:L5-L8

Useful? React with 👍 / 👎.

Comment on lines +546 to +549
idx = tool_call_delta.index
if idx < 0 or idx >= len(tool_calls):
continue
tool_call = tool_calls[idx]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep duplicate argument event snapshots progressive

When one chunk contains multiple same-index entries that each contribute a function.arguments fragment, accumulation coalesces all fragments before this raw-entry loop runs, so every emitted delta event reads the final combined tool_call here. The first event consequently exposes arguments from later entries in the same chunk rather than the documented accumulated value at that event; either emit one coalesced event or construct each event from a progressive snapshot.

AGENTS.md reference: AGENTS.md:L5-L8

Useful? React with 👍 / 👎.

@marcuswood-oai

Copy link
Copy Markdown
Contributor

Thanks for the work and extensive tests! We’ve merged the fix for #3201 in #3425, so I’m closing this overlapping PR. The additional sparse-index and metadata changes would need separate review; they aren’t covered by #3425. We appreciate the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streaming tool_call deltas with duplicate indexes in first chunk are accumulated incorrectly

3 participants