fix(translator): keep tool arguments when openai-compat upstreams send per-chunk usage - #5430
fix(translator): keep tool arguments when openai-compat upstreams send per-chunk usage#5430ggbdpq wants to merge 2 commits into
Conversation
|
This pull request targeted The base branch has been automatically changed to |
…d per-chunk usage The OpenAI-to-Claude streaming translator finalized the whole stream as soon as a chunk carried usage while SawToolCall was set (shortcut added in 677dbe1). Upstreams that attach usage to every chunk (e.g. vLLM) hit that condition on the very first tool-call chunk, which only carries id and function.name, so the tool_use block was closed before any argument delta arrived and every later function.arguments was dropped: clients saw input: {} and every tool call failed. Decouple "usage seen" from "stream terminal": while finish_reason is still empty, buffer the latest usage values on the conversion params instead of finalizing; finish_reason or the [DONE] marker emits the buffered values. This preserves the router-for-me#5308 behavior of reporting real usage when an upstream omits finish_reason, and restores full tool arguments for per-chunk-usage upstreams. Fixes router-for-me#5419 Generated-by: GLM-5.3-Flash (ZCode)
08b93b6 to
14ee0c8
Compare
![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: 14ee0c84de
ℹ️ 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".
| if usage := root.Get("usage"); usage.Exists() && usage.Type != gjson.Null { | ||
| inputTokens, outputTokens, cachedTokens, cacheWriteTokens := extractOpenAIUsage(usage) | ||
| emitAnthropicMessageDelta(param, &results, inputTokens, outputTokens, cachedTokens, cacheWriteTokens) | ||
| emitMessageStopIfNeeded(param, &results) | ||
| if param.FinishReason != "" { |
There was a problem hiding this comment.
When an upstream sends usage with the tool-argument chunks but puts finish_reason in a separate chunk without usage, this nested check never emits the buffered values even though the stream is now known to be terminal. The translator waits until [DONE], so direct SDK consumers—or compatible upstreams that delay or omit that marker—never receive message_delta/message_stop; the finish-reason path should flush Last*Tokens when LastUsageSeen is set.
Useful? React with 👍 / 👎.
…hunk When an upstream attaches usage to the tool-argument chunks but sends finish_reason on a separate chunk without usage, the buffered values were only flushed at [DONE]. Direct Anthropic SSE consumers rely on message_delta/message_stop to detect the end of the stream, so they waited for a marker compatible upstreams may delay or omit. Close the stream on the finish_reason chunk when its own usage or the buffered values are available; without either, keep waiting for the usage-only chunk that OpenAI's include_usage mode sends afterwards. Fixes the review comment on router-for-me#5430 Generated-by: GLM-5.3-Flash (ZCode)
|
Fixed in 2974d85: the finish_reason chunk now closes the stream immediately, flushing the buffered values when its own usage is absent. One guard from the original suggestion: when neither is available, the translator keeps waiting for the usage-only chunk that OpenAI's include_usage mode sends after finish_reason (locked in by the existing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2974d857fe
ℹ️ 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".
| } else if hasUsage { | ||
| // The stream has not terminated yet. Ending it here would drop | ||
| // tool call arguments that arrive in later chunks, so buffer | ||
| // the latest usage values instead; finish_reason or [DONE] | ||
| // emits them. |
There was a problem hiding this comment.
When an upstream omits finish_reason but sends an include_usage tail chunk with choices: [] and then delays or omits [DONE], this branch buffers that unambiguously terminal chunk instead of emitting message_delta and message_stop. The previous param.SawToolCall path closed such streams immediately; the existing TestStreamingTool_UsageWithoutFinishReasonEmitsMessageDelta now masks the timing regression because runStream always appends [DONE]. Continue buffering usage attached to choice/delta chunks, but finalize when the usage-bearing chunk has no choices.
Useful? React with 👍 / 👎.
|
Heads-up on the failing ❌: this PR intentionally touches |
1 participant
Summary
usage(the|| param.SawToolCallshortcut from 677dbe1, fix(claude): emit message_delta on [DONE] when OpenAI finish_reason is omitted #5308). Upstreams that attach usage to every chunk (self-hosted vLLM etc.) therefore closed thetool_useblock while the arguments accumulator was still empty, and every laterfunction.argumentsdelta was dropped. Clients receivedinput: {}and every tool call failed (report + wire-level analysis in Tool arguments lost when openai-compat upstream sends per-chunk usage #5419).finish_reasonis still empty, the latest usage values are buffered on the conversion params instead of finalizing; they are emitted whenfinish_reasonarrives, or at[DONE]as a fallback.finish_reason, and restores full tool arguments for per-chunk-usage upstreams.Verification
TestStreamingTool_PerChunkUsageKeepsToolArguments(3-chunk vLLM-style sequence from #5419)tool arguments lost: got ""), passes afterTestStreamingTool_PerChunkUsageWithoutFinishReasonUsesBufferedUsageoutput_tokens = 5), passes afterTestStreamingTool_UsageWithoutFinishReasonEmitsMessageDelta,TestStreamingTool_OmittedFinishReasonEmitsMessageDeltaOnDone,TestStreamingText_OmittedFinishReasonEmitsEndTurnOnDone)go test ./internal/translator/openai/claude/go test ./internal/translator/... ./sdk/...go test -p 2 ./...mainon this Windows machine (parallel compile load also crashed the Go compiler 3 times there, incl. once on a cleanmaintree)go build ./cmd/servergofmtNotes on the flake disclaimer:
TestXAIExecutorExecuteImagesUsesImagesEndpointAndPublishesUsage,TestXAIExecutorExecuteVideosCreate,TestXAIWebsocketsExecuteStreamSendsResponseCreateWithPreviousResponseID, andTestAntigravityAuthHasCreditsRequiredHomeBalanceUsesKVeach failed only under full-suite parallel load on this machine and pass individually; a-count=3run of the executor package on unmodifiedmainreproduced failures across a wider set. None of them touch this translator. Upstream CI is the authoritative gate.AI use
Implemented by GLM-5.3-Flash (ZCode) from the root-cause analysis in #5419, reviewed and verified locally by a human.
Checklist