fix(models): give late reasoning items their own output slot in Chat Completions streaming - #5042
Rainmemery wants to merge 3 commits into
Conversation
…Completions streaming (openai#5041) The reasoning item is created lazily on the first thinking/reasoning delta, and the creation sites hardcoded output_index=0. When a text or tool-call chunk had already been streamed, two items were announced with index 0, and _reasoning_output_count flipped from 0 to 1 after the layout latched, making assistant_message_output_idx - _reasoning_output_count negative and silently dropping the last function call from the before-message prefix. The final assembly also always prepended the reasoning item, so an item announced at a later index landed at list position 0. - _StreamOutputLayout now allocates the reasoning slot lazily via reasoning_output_index(); function calls and the assistant message keep their original slot model so a fallback call still keeps its position relative to streamed calls - the slot is stored in state.reasoning_content_index_and_output[0] (never consumed before) and every reasoning event (added, summary part/text, reasoning text, done) now reports that slot instead of 0 - response.completed.output is assembled by sorting on the announced indexes, so positions stay consistent with the wire even for a reasoning item created after visible chunks Regression tests cover a reasoning item arriving after a text chunk and after a function-call chunk; both previously collided on index 0.
![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: 82332dfedb
ℹ️ 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".
| ) from exc | ||
|
|
||
| output_index = self._reasoning_output_count(state) | ||
| output_index = self._reasoning_output_count() |
There was a problem hiding this comment.
When an unannounced fallback tool call precedes a later call that starts streaming, and reasoning then arrives after that streamed call, the later call has already reserved index 1 and reasoning takes index 2; during finalization this newly added reasoning count shifts the earlier fallback call from its reserved index 0 to index 1. Both calls consequently emit output_index=1, index 0 is absent, and response.completed.output no longer corresponds to the streamed indexes, so index-based consumers can overwrite or misassociate an item. Derive the fallback call's index from the stable occupied/reserved layout rather than adding the late reasoning count.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
| ) | ||
|
|
||
| outputs.extend(output_layout.function_calls_after_message(state)) | ||
| output_entries.sort(key=lambda entry: entry[0]) |
There was a problem hiding this comment.
When a LiteLLM or AnyLLM reasoning-enabled stream emits visible assistant content before late reasoning_content or signed thinking_blocks, this sort now stores the turn as [message, reasoning]. On the next request, Converter.items_to_messages() treats reasoning as pending data for a following assistant message; when the next user item flushes the already-built assistant message, that trailing pending data is cleared without ever being attached. The follow-up request therefore loses DeepSeek reasoning content or Anthropic signed thinking blocks and can be rejected by the provider, so the replay conversion must associate this newly supported trailing reasoning item with the preceding message from the same turn.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
… call slots Addresses the Codex review findings on the PR: - Converter.items_to_messages now attaches pending reasoning state to the current assistant message when a reasoning item arrives after that message was already built. A streamed turn can now store [message, reasoning], and leaving the state pending dropped DeepSeek reasoning_content and Anthropic signed thinking blocks when the next item flushed the message. - Late reasoning items no longer shift unannounced function calls onto already-announced output indexes: tracked-but-unannounced calls reserve the slots the layout formula assumes they occupy when a neighboring slot is handed out (on streamed call announcement and on message announcement). Adds regression tests for both paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dab6d91402
ℹ️ 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: | ||
| function_calls_before_message = ( | ||
| self.assistant_message_output_idx - self._reasoning_output_count(state) | ||
| self.assistant_message_output_idx - self._reasoning_output_count() |
There was a problem hiding this comment.
When text has already announced the assistant at index 0, a fallback call is tracked, a second call begins streaming at index 2, and late reasoning then takes index 3, this message-present branch never reserves index 1. At finalization, _reasoning_output_count() changes and assigns the fallback call index 2, colliding with the second call and making response.completed.output inconsistent with the streamed indexes. This is fresh evidence beyond the earlier fallback report: the new reservation loops cover only the pre-message path; allocate all tracked outputs from the same stable reserved map, including calls after an announced message.
AGENTS.md reference: AGENTS.md:L115-L116
Useful? React with 👍 / 👎.
|
Addressed both Codex review findings in dab6d91:
Full |
Follow-up to the Codex review: the reservation loops only covered the pre-message path. When the assistant message is announced before any call is tracked, a tracked fallback call and a later streamed call both derived their slots from the live reasoning count at finalization, so late reasoning shifted the fallback call onto the streamed call's announced index. The message-announced branch now reserves the slots of tracked-but-unannounced calls below the announcing call from the same stable formula.
1 participant
Fixes #5041
Summary
ChatCmplStreamHandlercreates the reasoning item lazily, the first time a thinking block /reasoning_content/reasoningdelta arrives, and the three creation sites hardcodedoutput_index=0. When a text or tool-call chunk had already been streamed, that slot was taken, so:output_index == 0;_reasoning_output_countflipped from0to1after_StreamOutputLayouthad latched the assistant message slot, makingassistant_message_output_idx - _reasoning_output_countnegative -function_calls_before_messagethen sliced off the last function call, and the fallback call's slot collided with the reasoning slot;Change
_StreamOutputLayoutnow allocates the reasoning slot lazily viareasoning_output_index():state.reasoning_content_index_and_output[0](that tuple element was never consumed before), and every reasoning event - added, summary part added/text delta, reasoning text delta, summary part done, reasoning text done, item done - reports that slot instead of a hardcoded0;test_fallback_function_call_keeps_index_before_streamed_callandtest_mixed_function_calls_before_text_keep_tracked_orderpass unchanged);response.completed.outputis assembled by sorting on the announced indexes, keeping positions consistent with what consumers saw on the wire even when the reasoning item is created after visible chunks.Reasoning-first streams (the only ordering covered before) produce byte-identical event sequences.
Test plan
pytest tests/models/- 881 passed, 11 skipped, including two new regression tests:output_indexcollided with the message and the completed output mis-ordered; now message=0, reasoning=1, output[message, reasoning];[function_call, reasoning].The remaining full-suite run only fails on
tests/tracing/test_import_side_effects.py::test_core_imports_do_not_require_legacy_httpx, which fails identically on pristinemainin this environment (subprocess environment issue, unrelated to this change).