feat(openai): add stream-to-nonstream chat handling - #4447
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for converting a non-stream client request into an upstream streaming request, tracking that via a new RelayInfo flag, accumulating streamed Responses events into a single final chat-completion payload, and exposing a channel-level toggle to enable this behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Relay as Relay (gin)
participant Upstream as Upstream (Responses stream)
participant Acc as Accumulator
participant Handler as Stream→NonStream Handler
Client->>Relay: non-stream chat request
Relay->>Relay: detect non_stream_upstream_stream condition\nforce request.Stream=true, set UpstreamStreamForNonStream
Relay->>Upstream: proxied streaming request
Upstream-->>Relay: SSE-like data: events (responses stream)
Relay->>Acc: feed each event into responsesStreamAccumulator
Acc-->>Acc: merge content, reasoning, tool-call deltas, usage
Relay->>Handler: on upstream EOF, call Stream→NonStream handler with accumulated items
Handler->>Relay: build final chat.completion response + usage
Relay->>Client: return non-stream chat-completion payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 199-239: The fallback usage estimator misses contributions from
full tool-call names and the initial full arguments payload; update the
"response.output_item.added"/"response.output_item.done" branch so that when you
set toolCallNameByID[callID] and toolCallArgsByID[callID] (i.e. when
streamResp.Item.Name and streamResp.Item.Arguments are non-empty) you also
append those strings into usageText (same buffer used in the
"response.function_call_arguments.delta" case), ensuring tool-call names and
full argument payloads are counted when the Responses stream lacks a usage
block; keep using the existing identifiers (usageText, toolCallNameByID,
toolCallArgsByID, toolCallCanonicalIDByItemID, toolCallIndexByID,
streamResp.Item) to locate and modify the code.
In `@relay/channel/openai/relay-openai.go`:
- Around line 327-330: The fallback branch that adds synthetic tool-call tokens
increments usage.CompletionTokens by toolCount * 7 but doesn't update
usage.TotalTokens, so total is underreported; after calling
service.ResponseText2Usage(...) and adding toolCount * 7 to
usage.CompletionTokens in the code path guarded by containStreamUsage, recompute
usage.TotalTokens (e.g., set it to usage.PromptTokens + usage.CompletionTokens
or otherwise recalc from its constituent fields) so TotalTokens reflects the
synthetic tool-call tokens; adjust the block around containStreamUsage, usage,
ResponseText2Usage, CompletionTokens and toolCount accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6545440-671e-4009-ab74-69901893f0c0
📒 Files selected for processing (8)
dto/channel_settings.gorelay/channel/openai/adaptor.gorelay/channel/openai/chat_via_responses.gorelay/channel/openai/relay-openai.gorelay/chat_completions_via_responses.gorelay/common/relay_info.gorelay/compatible_handler.goweb/src/components/table/channels/modals/EditChannelModal.jsx
| if info.UpstreamStreamForNonStream { | ||
| usage, newApiErr := openaichannel.OaiResponsesToChatStreamToNonStreamHandler(c, info, httpResp) | ||
| if newApiErr != nil { | ||
| service.ResetStatusCode(newApiErr, statusCodeMappingStr) | ||
| return nil, newApiErr | ||
| } | ||
| return usage, nil |
There was a problem hiding this comment.
Only use the SSE aggregation path when the upstream actually streams.
UpstreamStreamForNonStream means we asked for a stream, not that the provider honored it. If the upstream still returns buffered JSON, this branch will feed a non-SSE body into OaiResponsesToChatStreamToNonStreamHandler instead of falling back to OaiResponsesToChatHandler.
Suggested fix
- if info.UpstreamStreamForNonStream {
+ if info.UpstreamStreamForNonStream && info.IsStream {
usage, newApiErr := openaichannel.OaiResponsesToChatStreamToNonStreamHandler(c, info, httpResp)
if newApiErr != nil {
service.ResetStatusCode(newApiErr, statusCodeMappingStr)
return nil, newApiErr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if info.UpstreamStreamForNonStream { | |
| usage, newApiErr := openaichannel.OaiResponsesToChatStreamToNonStreamHandler(c, info, httpResp) | |
| if newApiErr != nil { | |
| service.ResetStatusCode(newApiErr, statusCodeMappingStr) | |
| return nil, newApiErr | |
| } | |
| return usage, nil | |
| if info.UpstreamStreamForNonStream && info.IsStream { | |
| usage, newApiErr := openaichannel.OaiResponsesToChatStreamToNonStreamHandler(c, info, httpResp) | |
| if newApiErr != nil { | |
| service.ResetStatusCode(newApiErr, statusCodeMappingStr) | |
| return nil, newApiErr | |
| } | |
| return usage, nil |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
relay/channel/openai/relay-openai.go (2)
385-393: Redundant first marshal.The
responseBodyproduced at line 385 is immediately discarded —applyUsagePostProcessingmutatessimpleResponse.Usage, and the result is re-marshalled at line 390. The first marshal can be dropped (the error check it provides is also re-performed by the second one).♻️ Proposed simplification
- responseBody, err := common.Marshal(simpleResponse) - if err != nil { - return nil, types.NewOpenAIError(err, types.ErrorCodeJsonMarshalFailed, http.StatusInternalServerError) - } applyUsagePostProcessing(info, &simpleResponse.Usage, common.StringToByteSlice(lastStreamData)) - responseBody, err = common.Marshal(simpleResponse) + responseBody, err := common.Marshal(simpleResponse) if err != nil { return nil, types.NewOpenAIError(err, types.ErrorCodeJsonMarshalFailed, http.StatusInternalServerError) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/relay-openai.go` around lines 385 - 393, Remove the redundant initial marshal of simpleResponse: drop the first call to common.Marshal and its error check (the responseBody variable and the first error branch), then call applyUsagePostProcessing(info, &simpleResponse.Usage, common.StringToByteSlice(lastStreamData)) and perform a single marshal afterwards using common.Marshal(simpleResponse); this keeps only the final serialization and its error handling for responseBody and avoids unnecessary work.
375-383: AlignCreatedfield assignment withchat_via_responses.gofor consistency.The struct field
OpenAITextResponse.Createdis typed asany. Inchat_via_responses.go(line 338), this field is assigned directly ascreatedAt(int64) without anany()wrapper. Here at lines 375-383, the same field receivesany(createdAt). Use the direct assignment to match the other construction site.♻️ Proposed change
- createdAny := any(createdAt) simpleResponse := dto.OpenAITextResponse{ Id: responseId, Model: model, Object: object, - Created: createdAny, + Created: createdAt, Choices: responseChoices, Usage: *usage, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/relay-openai.go` around lines 375 - 383, The Created field on dto.OpenAITextResponse is being wrapped with any(createdAt) here; make it consistent with the other usage by assigning the int64 createdAt directly (remove the createdAny := any(createdAt) helper and set Created: createdAt) when constructing OpenAITextResponse to match the construction in chat_via_responses (use the existing createdAt variable and the OpenAITextResponse.Created field).relay/channel/openai/chat_via_responses.go (2)
267-274: Subtle control flow in the error case — confirm intent.
breakat line 271 exits the innerswitch, not the outerfor. Because the assignment at line 274 is unreachable after the switch case ends (Go does not fall through), the upstream-providedOpenAIErrorcorrectly takes precedence over the generic error. The outerforis then terminated via theif streamErr != nil { break }at line 278. Behavior is correct, but the use of a barebreaknext to a fallthrough-style assignment is easy to misread; a small comment, or restructuring asif ... { streamErr = ...; } else { streamErr = ... }, would prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/chat_via_responses.go` around lines 267 - 274, The switch case handling "response.error"/"response.failed" in chat_via_responses.go uses a bare break inside the inner switch which is easy to misread; update the logic in the block that inspects streamResp.Response so it explicitly assigns streamErr in an if/else style (e.g. if oaiErr := streamResp.Response.GetOpenAIError(); oaiErr != nil && oaiErr.Type != "" { streamErr = types.WithOpenAIError(*oaiErr, http.StatusInternalServerError) } else { streamErr = types.NewOpenAIError(fmt.Errorf("responses stream error: %s", streamResp.Type), types.ErrorCodeBadResponse, http.StatusInternalServerError) }) or add a clarifying comment next to the bare break so the intent is obvious; adjust within the same function that references streamResp, streamErr, GetOpenAIError, types.WithOpenAIError and types.NewOpenAIError.
94-368: Significant duplication withOaiResponsesToChatStreamHandler.The new handler reproduces almost all of the upstream-event parsing logic (
response.created,response.output_text.delta,response.output_item.*,response.function_call_arguments.delta,response.completed,response.error|failed, the tool-call ID/name/args bookkeeping, and the usage merge) that already lives inOaiResponsesToChatStreamHandler(lines 370-827). Future spec changes (new event types, new usage fields, edge-case fixes like the previously-flagged tool-call name/argument token accounting) will now have to be applied in two places, which is easy to miss.Consider extracting the shared state machine (e.g. an internal
responsesStreamAccumulatorwithApply(ev *dto.ResponsesStreamResponse)andResult()returning text/reasoning/usage/tool-calls), and let both the streaming and stream-to-non-stream handlers compose it. Not a blocker for this PR, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/chat_via_responses.go` around lines 94 - 368, The handler OaiResponsesToChatStreamToNonStreamHandler duplicates the event-parsing/state logic from OaiResponsesToChatStreamHandler; extract that shared logic into a new responsesStreamAccumulator type with methods Apply(ev *dto.ResponsesStreamResponse) error and Result() (messageText string, reasoningText string, usage *dto.Usage, toolCalls []dto.ToolCallResponse, err *types.NewAPIError); move the local state (outputText, reasoningText, usage, usageText, sawToolCall, toolCallIndexByID, toolCallNameByID, toolCallArgsByID, toolCallCanonicalIDByItemID) and mergeUsage into the accumulator, have both OaiResponsesToChatStreamToNonStreamHandler and OaiResponsesToChatStreamHandler create an accumulator, call Apply for each parsed streamResp, and use Result() to build the final chatResp/responseBody; ensure Apply handles the same event types (response.created, response.output_text.delta, response.output_item.*, response.function_call_arguments.delta, response.completed, response.error/failed) and preserves existing semantics (including service.ResponseText2Usage fallback).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 290-292: The fallback that replaces usage when usage.TotalTokens
== 0 loses any tool-call token adjustments; modify the block that calls
service.ResponseText2Usage(c, usageText.String(), info.UpstreamModelName,
info.GetEstimatePromptTokens()) so that after computing usage you add the same
tool-call adjustment used elsewhere: increment usage.CompletionTokens by
toolCount * 7 (using the same toolCount tracked in tool-call branches) and then
set usage.TotalTokens = usage.PromptTokens + usage.CompletionTokens; ensure you
reference mergeUsage, ResponseText2Usage, info.GetEstimatePromptTokens(), and
usage.TotalTokens to find the right spot and maintain parity with the
OaiStreamToNonStreamHandler adjustment logic.
In `@relay/channel/openai/relay-openai.go`:
- Line 389: applyUsagePostProcessing is being fed lastStreamData (a single SSE
chunk) which doesn't match the provider JSON shapes expected by
extractCachedTokensFromBody / extractMoonshotCachedTokensFromBody /
extractLlamaCachedTokensFromBody, so cached-token extraction silently no-ops;
fix by passing the full marshalled response body (responseBody) into
applyUsagePostProcessing where the handler has the accumulated response, or for
Moonshot/llama paths change the streaming loop to accumulate and call the
respective extractMoonshotCachedTokensFromBody /
extractLlamaCachedTokensFromBody per-chunk (or aggregate chunks) so the parsers
receive complete provider-shaped JSON instead of lastStreamData.
- Around line 334-362: The loop assumes contiguous zero-based indices by
iterating 0..len(map)-1 for choices and toolCallsByChoice, which silently drops
sparse or non-zero-based keys (streamChoice.Index, deltaToolCall.Index); change
both loops to iterate the actual map keys (for idx := range choices and for tIdx
:= range choiceToolCalls), collect the integer keys, sort them if deterministic
order is required, then process entries using those keys (updating
choice.Message, FinishReason, tool calls, and appending to responseChoices) so
sparse indices are preserved.
---
Nitpick comments:
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 267-274: The switch case handling
"response.error"/"response.failed" in chat_via_responses.go uses a bare break
inside the inner switch which is easy to misread; update the logic in the block
that inspects streamResp.Response so it explicitly assigns streamErr in an
if/else style (e.g. if oaiErr := streamResp.Response.GetOpenAIError(); oaiErr !=
nil && oaiErr.Type != "" { streamErr = types.WithOpenAIError(*oaiErr,
http.StatusInternalServerError) } else { streamErr =
types.NewOpenAIError(fmt.Errorf("responses stream error: %s", streamResp.Type),
types.ErrorCodeBadResponse, http.StatusInternalServerError) }) or add a
clarifying comment next to the bare break so the intent is obvious; adjust
within the same function that references streamResp, streamErr, GetOpenAIError,
types.WithOpenAIError and types.NewOpenAIError.
- Around line 94-368: The handler OaiResponsesToChatStreamToNonStreamHandler
duplicates the event-parsing/state logic from OaiResponsesToChatStreamHandler;
extract that shared logic into a new responsesStreamAccumulator type with
methods Apply(ev *dto.ResponsesStreamResponse) error and Result() (messageText
string, reasoningText string, usage *dto.Usage, toolCalls
[]dto.ToolCallResponse, err *types.NewAPIError); move the local state
(outputText, reasoningText, usage, usageText, sawToolCall, toolCallIndexByID,
toolCallNameByID, toolCallArgsByID, toolCallCanonicalIDByItemID) and mergeUsage
into the accumulator, have both OaiResponsesToChatStreamToNonStreamHandler and
OaiResponsesToChatStreamHandler create an accumulator, call Apply for each
parsed streamResp, and use Result() to build the final chatResp/responseBody;
ensure Apply handles the same event types (response.created,
response.output_text.delta, response.output_item.*,
response.function_call_arguments.delta, response.completed,
response.error/failed) and preserves existing semantics (including
service.ResponseText2Usage fallback).
In `@relay/channel/openai/relay-openai.go`:
- Around line 385-393: Remove the redundant initial marshal of simpleResponse:
drop the first call to common.Marshal and its error check (the responseBody
variable and the first error branch), then call applyUsagePostProcessing(info,
&simpleResponse.Usage, common.StringToByteSlice(lastStreamData)) and perform a
single marshal afterwards using common.Marshal(simpleResponse); this keeps only
the final serialization and its error handling for responseBody and avoids
unnecessary work.
- Around line 375-383: The Created field on dto.OpenAITextResponse is being
wrapped with any(createdAt) here; make it consistent with the other usage by
assigning the int64 createdAt directly (remove the createdAny := any(createdAt)
helper and set Created: createdAt) when constructing OpenAITextResponse to match
the construction in chat_via_responses (use the existing createdAt variable and
the OpenAITextResponse.Created field).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6dd8824-1dd7-44e4-a9c6-d3694dd0a5a6
📒 Files selected for processing (2)
relay/channel/openai/chat_via_responses.gorelay/channel/openai/relay-openai.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/openai/relay-openai.go`:
- Around line 376-385: The code currently fabricates a fake assistant choice
when responseChoices is empty by appending a dto.OpenAITextResponseChoice with
constant.FinishReasonStop; instead, stop creating a fabricated response and
return/propagate an error to surface the malformed or invalid upstream stream.
Locate the branch that checks len(responseChoices) == 0 (the code that appends
dto.OpenAITextResponseChoice) and replace the append with returning an error
describing "no choices parsed from upstream" (or propagate an existing parse
error) so callers can handle the failure rather than receiving a synthetic
assistant message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dac1b2d-2e26-4031-a7b5-e7dca4135dc3
📒 Files selected for processing (2)
relay/channel/openai/chat_via_responses.gorelay/channel/openai/relay-openai.go
✅ Files skipped from review due to trivial changes (1)
- relay/channel/openai/chat_via_responses.go
# Conflicts: # relay/channel/openai/chat_via_responses.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 415-451: The code fabricates a synthetic assistant choice even
when the accumulator (acc) has no assistant text, no reasoning, and no tool
calls; change the logic in the finalization path (after calling acc.Result()) to
detect when messageText == "" && reasoning == "" && len(toolCalls) == 0 and
return an error (or nil response with a non-success error) instead of
constructing msg/Choices/OpenAITextResponse; also apply the same guard to the
streaming path where synthetic start/stop chunks are emitted (the blocks around
the streaming finalizers referenced in the review, e.g., the regions analogous
to the code at acc.Result() and the streaming emit ranges mentioned) so empty
terminal streams are rejected consistently.
- Around line 117-120: Create a single helper function (e.g.,
finishReasonFromResponse or mapResponseToFinishReason) that takes a Response (or
its Status and IncompleteDetails) and returns the canonical finish_reason
strings ("stop", "tool_calls", "length", "content_filter", etc.) by mapping
Response.Status and IncompleteDetails to the correct reason; replace the current
ad-hoc logic that only checks for tool calls in the converter and streaming
paths and call this helper from
service/openaicompat.ResponsesResponseToChatCompletionsResponse and the three
spots in relay/channel/openai/chat_via_responses.go where finish_reason is
currently derived (the stream-to-non-stream handler, the streaming mid-response
branch, and the streaming final-chunk branch) so all four locations reuse the
same mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37130cb5-0a3a-45ef-ae21-3edf9c6a3115
📒 Files selected for processing (3)
relay/channel/openai/chat_via_responses.gorelay/channel/openai/relay-openai.goweb/classic/src/components/table/channels/modals/EditChannelModal.jsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
relay/channel/openai/chat_via_responses.go (1)
443-446:⚠️ Potential issue | 🟠 MajorMap
finish_reasonfrom Responses terminal status, not only tool-call presence.
finish_reasonis still hard-coded to"stop"/"tool_calls"here. That can misreport truncated/filtered/incomplete upstream completions as normal stops. Please reuse a canonical status→finish_reason mapping in these finalization points.Also applies to: 805-808, 845-848
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openai/chat_via_responses.go` around lines 443 - 446, The finishReason should be derived from the Responses terminal status instead of hardcoding based solely on toolCalls: replace the current finishReason assignment (the variable finishReason and the toolCalls check) with a call to the shared canonical mapping function (e.g., mapTerminalStatusToFinishReason or the existing equivalent used elsewhere) that takes the response/terminal status and returns the correct finish_reason, and only fallback to "tool_calls" when that mapping indicates tool usage or if no status is available; apply the same change to the other finalization sites that currently mirror this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@relay/channel/openai/chat_via_responses.go`:
- Around line 443-446: The finishReason should be derived from the Responses
terminal status instead of hardcoding based solely on toolCalls: replace the
current finishReason assignment (the variable finishReason and the toolCalls
check) with a call to the shared canonical mapping function (e.g.,
mapTerminalStatusToFinishReason or the existing equivalent used elsewhere) that
takes the response/terminal status and returns the correct finish_reason, and
only fallback to "tool_calls" when that mapping indicates tool usage or if no
status is available; apply the same change to the other finalization sites that
currently mirror this logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3123d05a-c0ca-4bb0-af12-1174d1b4b0c2
📒 Files selected for processing (2)
relay/channel/openai/chat_via_responses.gorelay/channel/openai/chat_via_responses_test.go
51fdfc5 to
2b6f1df
Compare
Important
📝 变更描述 / Description
(简述:做了什么?为什么这样改能生效?请基于你对代码逻辑的理解来写,避免粘贴未经整理的内容)
加入了非流式 OpenAI Chat Completions 请求到流式 Chat Completions / Responses 请求的转换,以及请求体转换后对响应体自动转换的功能。涉及 Responses 上游时,依赖于模型设置内的 Chat Completions -> Responses 转换才能工作。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix,我已提交或关联对应 Issue,且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告,以证明变更生效)
Summary by CodeRabbit
New Features
Bug Fixes
Tests