[core] Support interleaved response items#30876
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c61a3738
ℹ️ 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".
| let completed_item_id = item.id().map(str::to_owned); | ||
| let previously_streamed_item = if let Some(item_id) = completed_item_id.as_deref() { | ||
| streamed_items.remove(item_id) | ||
| } else { | ||
| None | ||
| active_item | ||
| .as_ref() | ||
| .and_then(|active| streamed_items.remove(&active.id())) | ||
| }; | ||
| let completes_active_item = match (&completed_item_id, active_item.as_ref()) { | ||
| (Some(completed_item_id), Some(active)) => completed_item_id == &active.id(), | ||
| (None, Some(_)) => true, | ||
| _ => false, | ||
| }; |
There was a problem hiding this comment.
Preserve streaming tool diffs across unrelated item completions
When a custom tool call is streaming input deltas and another response item completes before that tool call's own output_item.done, this new ID-based path can process the unrelated completion while keeping the active item alive, but the handler has already take()n and finish()ed active_tool_argument_diff_consumer at the top of the arm. In interleaved streams such as custom_tool_call.added → patch deltas → reasoning.done → more patch deltas, the later deltas are ignored, so live apply-patch/tool-diff updates disappear even though the tool call is still in progress; the consumer should only be finished when the completed item is the active tool call/call_id.
Useful? React with 👍 / 👎.
| if let (Some(item_id), Some(delta), Some(summary_index)) = | ||
| (event.item_id, event.delta, event.summary_index) |
There was a problem hiding this comment.
Keep fallback for summary deltas without item IDs
For streams that emit response.reasoning_summary_text.delta (and the matching part-added event) while a reasoning item is active but do not include item_id, this new parser branch now returns Ok(None) and silently drops the live reasoning update. The previous code and existing test helper shape supported those deltas by associating them with the active reasoning item, so older/legacy response streams lose all streaming reasoning summaries even though the final reasoning item can still arrive; preserve a fallback path instead of requiring item_id unconditionally.
Useful? React with 👍 / 👎.
| active_item | ||
| .as_ref() | ||
| .and_then(|active| streamed_items.remove(&active.id())) |
There was a problem hiding this comment.
Don't let id-less tool completions close active items
When an interleaved output_item.done is a tool/function call without an id (the helpers and Responses payloads commonly carry only call_id), this fallback removes whichever message/reasoning item is currently active from streamed_items. A stream like reasoning.added → function_call.done → more reasoning_summary_text.delta will therefore report the reasoning item as unknown (or later emit a duplicate start on reasoning completion), even though the tool completion was unrelated to that active item. The fallback should not consume the active streamed item for id-less tool completions.
Useful? React with 👍 / 👎.
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
1 final question here
| let completed_item_id = item.id().map(str::to_owned); | ||
| let completes_active_item = match (&completed_item_id, active_item.as_ref()) { | ||
| (Some(completed_item_id), Some(active)) => completed_item_id == &active.id(), | ||
| (None, Some(active)) => { |
There was a problem hiding this comment.
aligned - will update
d0da607 to
4d84452
Compare
Summary
Stack
Validation