Skip to content

fix(llm): keep choices of OpenAI streaming chunks that also carry usage - #9394

Open
Ayush Suryawanshi (GloballyOptimized) wants to merge 2 commits into
crewAIInc:mainfrom
GloballyOptimized:fix/openai-stream-usage-choices
Open

Ayush Suryawanshi (GloballyOptimized) wants to merge 2 commits into
crewAIInc:mainfrom
GloballyOptimized:fix/openai-stream-usage-choices

Conversation

@GloballyOptimized

@GloballyOptimized Ayush Suryawanshi (GloballyOptimized) commented Nov 12, 2026 •

Copy link
Copy Markdown

Related issue

Fixes #8775

Summary

The Chat Completions streaming loops continue as soon as a chunk has usage, so anything else in that chunk (the last text delta, the last tool-call arguments, or finish_reason) gets dropped. DeepSeek's final chunk carries both usage and finish_reason, so DeepSeek users always lose the finish reason.

This removes the continue in the three streaming paths (sync, async, async with response_model). OpenAI's separate usage-only chunk (choices=[]) is still skipped by the existing empty-choices check, and usage is still tracked once at the end.

Verification

  • Tests added or updated for the changed behavior
  • Relevant tests and quality checks pass locally

New tests in tests/llms/openai/test_streaming_usage_chunks.py feed synthetic chunks through a stub client (no network). They cover text, tool-call and finish_reason cases for sync and async, plus structured output and controls for the normal OpenAI shape. They fail on main and pass with this change. The existing OpenAI, OpenAI-compatible and Snowflake tests pass, and ruff/mypy are clean.

Additional context

Supersedes #8776, which was closed unmerged.

This PR was prepared with AI assistance (Claude Code); I reviewed and tested it. Per CONTRIBUTING it needs the llm-generated label, which I can't add as an external contributor. Could a maintainer please apply it?

🤖 Generated with Claude Code

The Chat Completions streaming handlers (sync, async, and async
response_model) did `continue` as soon as a chunk had `usage`, so any
`choices` on that same chunk were dropped: the final text delta, the
last tool-call argument fragment, and the finish_reason. OpenAI-compatible
servers such as DeepSeek send exactly such a terminal chunk.

Record usage and then fall through to normal choice handling. Chunks
with `choices=[]` (OpenAI's separate usage trailer) are still skipped by
the existing empty-choices check, and usage is still tracked once.

Fixes crewAIInc#8775

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@strix-security

strix-security Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Strix Security Review

Warning

This pull request has 2 commits after the last Strix review (7fe8b7f). Strix has not reviewed these changes.
Automatic review on push is off for this repository. To review the latest changes, tag @strix-security in a comment, or turn on re-review on push.

✅ No open security findings.

PR summary: Removes three `continue` statements that made the OpenAI (and OpenAI-compatible) streaming handlers skip a chunk's `choices` whenever the same chunk also carried token `usage`, so terminal text deltas, tool-call argument fragments and `finish_reason` are no longer silently dropped. Adds a test module covering the sync, async and async structured-output streaming paths.

Review summary

This PR removes the continue that made the three OpenAI Chat Completions streaming handlers discard a chunk's choices whenever that chunk also carried usage, so terminal deltas, tool-call argument fragments and finish_reason are now processed instead of dropped.

I traced the newly-reachable code after each of the three hunks (sync streaming, async streaming, and async streaming with response_model) to every sink it touches: text accumulation and stream-chunk event emission, tool-call accumulation and the available_functions execution path including the json.loads of assembled arguments, and the pydantic model_validate_json structured-output path. The removed continue was the only branch difference, so content on a usage-bearing chunk now traverses exactly the code any other streamed chunk already traversed — a provider that wanted to deliver text, tool-call fragments or a finish reason could already do so on a chunk without usage. No new sink, trust boundary or attacker position is introduced: tool execution still requires the name to be present in the caller's available_functions map, tool-call indices are dict keys rather than sequence indices, and finish_reason and response_id are only recorded as event metadata with no control flow depending on them. Usage accounting is unchanged in both value and count — usage_data was already assigned before the deleted statement and is still persisted once per call in finalization, with OpenAI's choices=[] usage trailer still skipped by the retained guard, and the counters feed reporting and telemetry only, never a rate, budget or authorization decision.

The added test module is test-only and contains no real credentials. No security issues found in the changed code.

Ruled out (4)
  • Does letting a usage-bearing chunk fall through double-count token usage or let a provider-controlled chunk shape bypass a budget, rate or cost limit?
    The `usage_data` assignment was already above the deleted statement and is persisted once per call in finalization, OpenAI's `choices=[]` trailer is still skipped by the retained guard, and the counters feed only reporting and telemetry, never an enforcement decision.
  • Can the newly-processed terminal chunk cause an arbitrary function to run or arguments to be parsed unsafely?
    Tool execution still requires the accumulated name to be a key in the caller's `available_functions` map before invocation, and a non-object JSON payload is caught and reported as a tool-usage error.
  • Does the change route provider data into event listeners, trace records or terminal output in a way that is new?
    Stream-chunk and call-completed events are emitted on the same paths and with the same volume for every other chunk, adding at most one extra chunk event carrying text the model already produced in the same response.
  • Can the schema properties of a remote tool definition weaken model validation through the async `response_model` path this PR touches?
    The structured-output path validates the streamed JSON against a model built from the caller-supplied `response_model`; remote `inputSchema` handling happens when tools are constructed, before streaming, and is not reached by these hunks.

Review 1 on this PR · 7fe8b7f · requested Nov 12 12:32 UTC when the PR was opened


Reviewed by Strix
Re-run review · Configure security review settings

This branch has not been deployed

No deployments
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.

[BUG] OpenAI streaming skips text, tool deltas and finish_reason on chunks with usage

1 participant