fix: allow streamed usage on the tool-loop chat passthrough - #930
fix: allow streamed usage on the tool-loop chat passthrough#930seonghobae wants to merge 1 commit into
Conversation
/v1/chat/completions rejected stream=true + stream_options.include_usage=true whenever tools were present, on the premise that this gateway "cannot emit that SSE contract" for tool calls. That premise was wrong for this code path: tools_list routes through the single-agent tool_loop, which already resolves via proxy_completion (stream forced to False upstream) before _chat_response_sse_chunks frames the complete response as SSE -- its usage is already known, not synthesized. Scope the fail-closed guard to the case where it's actually true: the multi-step conduct path (response_format without tools), whose usage would reflect only the final synthesizer step and undercount the real workflow cost. Also remove _chat_response_sse_chunks's text-derived usage_source="estimated" fallback for a provider response with no usage field. A token count guessed from response text is not this gateway's usage to report as fact; when the provider omits it, the usage chunk is now silently omitted from the stream, matching the same provider-reported-only honesty boundary already enforced on the other two streaming usage paths (ModelClient.stream_chat, chat_completion_chunks).
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The failing Generated by Claude Code |
|
Closing as superseded by #925 (merged), which fixed the same root cause — the One real design difference, worth recording rather than silently dropping: this PR removes If there's a case where silent omission is actually the better contract (e.g. a client that treats absence as "not measured" and mislabeling as a live bug class), happy to revisit — but for now Generated by Claude Code Generated by Claude Code |
Summary
/v1/chat/completionsrejectedstream=true+stream_options.include_usage=truewhenevertoolswere present, on the premise (from PR fix: accept Chat Completions streaming usage options #914) that this gateway "cannot emit that SSE contract" for tool calls. That premise is factually wrong for this specific path: atools_listrequest routes through the single-agent tool-loop, which already resolves viaproxy_completion(upstreamstreamis forced toFalse) before_chat_response_sse_chunksframes the complete response as SSE — itsusageis already known when framing starts, not synthesized from partial output.response_format-without-toolsconduct path, whose usage would reflect only the final synthesizer step and undercount the real workflow cost._chat_response_sse_chunks's text-derivedusage_source: "estimated"fallback for a provider response with nousagefield — a token count guessed from response text is not this gateway's usage to report as fact. When the provider omits it, the usage chunk is now silently omitted from the stream, matching the same provider-reported-only honesty boundary already enforced on the other two streaming usage paths (ModelClient.stream_chat's relay-only usage,chat_completion_chunks'cost.measurement_status=="measured"gate).This was root-caused via an 18-agent adversarially-verified investigation (gather → hypothesize → verify → synthesize) that read the actual code paths, the existing regression tests, the git history behind the original guard, and OpenAI's own API docs for
stream_options+tools compatibility.Developer experience
tests/test_chat_tools_passthrough_controls_http_honesty.pyandtests/test_stream_options_null_flags_noop_http_honesty.pysplit their combined tools+response_format 400 assertions into two tests each, one confirming the new 200 success path with a realusage_source: "reported"frame, one confirmingresponse_format-only still fails closed._chat_response_sse_chunks's now-unusedprompt_textparameter and theestimate_tokensimport that only that removed branch used.User experience
stream_options.include_usage=truegets a 200 response with honest usage instead of an unconditional 400, while a client relying on a fabricated text-derived token estimate no longer receives one.Test plan
python3 -m pytest tests/test_chat_tools_passthrough_controls_http_honesty.py tests/test_stream_options_null_flags_noop_http_honesty.py tests/test_streaming.py tests/test_chat_stream_options_http_honesty.py tests/test_completions_stream_options_http_honesty.py tests/test_empty_stream_options_include_noop_http_honesty.py tests/test_responses_stream_options_http_honesty.py tests/test_routing_latency_stream_options_bool_coerce_http_honesty.py tests/test_sdk_null_object_optional_noop_http_honesty.py tests/test_stream_options_false_tool_choice_empty_noop_http_honesty.py tests/test_stream_options_unknown_null_reject_http_honesty.py -q— 78 passedpython3 -m pytest tests -q(full suite) — 2770 passed, 1 skipped, 1 failed (pre-existingfast_mlsirmModuleNotFoundErrorin this sandbox, unrelated and reproducible identically on unmergedmain)Generated by Claude Code