fix(openai): synthesize terminal chunk for complete streams - #2294
fix(openai): synthesize terminal chunk for complete streams#2294Moyucharm wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe OpenAI transformer now wraps inbound response streams. The wrapper tracks output and usage, synthesizes missing finish chunks after eligible clean EOF, preserves existing completion markers, and emits a terminal done response. ChangesOpenAI inbound stream completion
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR adds terminal chunks for otherwise complete streaming responses while preserving existing completion and error behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ResponseStream as llm.Response stream
participant InboundStream as newOpenAIInboundStream
participant TransformStream
participant StreamEvent as httpclient.StreamEvent
ResponseStream-->>InboundStream: clean EOF
InboundStream->>InboundStream: synthesize missing finish chunks
InboundStream->>InboundStream: append llm.DoneResponse
InboundStream->>TransformStream: finalized responses
TransformStream->>StreamEvent: convert response chunks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Hi, thanks for working on synthesizing terminal chunks for incomplete streams. This directly relates to a problem I'm hitting on the OpenAI Responses + pass-through + streaming path: With Would this PR's synthesize-terminal-chunk idea also cover the Responses protocol + pass-through streaming path (not just Chat Completions)? If not, is there a preferred way to handle the terminal event in the raw pass-through stream (e.g. skip the |
|
Thanks for the detailed report — this PR would not cover that path. The change here is scoped to the OpenAI Chat Completions inbound transformer. It synthesizes a missing finish_reason and [DONE] after a transformed Chat Completions stream reaches a clean EOF with positive completion evidence. With passThroughBody enabled, applyPassThroughStream replaces the transformed stream with the raw provider stream, so the synthesized chunks from this PR are intentionally bypassed. Your case also sounds different semantically: the upstream already emitted a valid response.completed, so nothing should be synthesized. The raw fan-out should preserve that event, and the SSE writer should recognize it through IsTerminalStreamEvent, either from the SSE event: response.completed field or from type: "response.completed" in the JSON data. I would avoid skipping the terminalSeen check for all pass-through streams, since that would also make genuinely truncated Responses streams appear successful and would weaken the incomplete-stream protection added by #2185. The preferred fix seems to be on the raw pass-through terminal-observation path: verify that captureRawProviderStream delivers the response.completed event to the raw channel; verify that the event retains its Type and Data when it reaches the writer; ensure IsTerminalStreamEvent recognizes the provider’s exact wire shape; add an end-to-end regression test for Responses + passThroughBody where response.completed is forwarded without an injected STREAM_ERROR. Recent unstable already recognizes response.completed both from StreamEvent.Type and JSON data.type via #2068. If you are already on a version containing that change, could you share the exact final SSE frame emitted by the provider and the AxonHub version/commit? That would help determine whether this is an unrecognized event shape or an event-delivery issue in the fan-out path. So I think this should be handled as a separate pass-through regression rather than extending this PR’s Chat Completions synthesis logic. |
2 participants
Summary
Some OpenAI-compatible upstreams emit the complete response content and a
usage chunk, then close the SSE stream without sending
finish_reasonor[DONE]. Strict OpenAI-compatible clients interpret this as an incompletestream.
This affects coding-agent clients such as pi when the upstream response is
otherwise complete.
Changes
transformer.
a final chunk with an empty
deltaand:finish_reason: "stop"for text/reasoning outputfinish_reason: "tool_calls"for tool-call output[DONE]after a synthesized terminal chunk.finish_reasonand[DONE]behavior.meaningful output was produced, so genuinely truncated streams remain
distinguishable.
Reproduction
The affected upstream shape is:
choices: []usage chunk.finish_reasonor[DONE].Testing
Added coverage for:
finish_reasonfinish_reason[DONE]Validation:
cd llm && go test ./transformer/openai/...cd llm && go vet ./transformer/openai/...go test ./internal/server/api/... ./internal/server/orchestrator/...go vet ./internal/server/api/... ./internal/server/orchestrator/...Follow-up to #1924; this complements #2185 by handling complete weak-provider
streams rather than merely reporting incomplete streams.
Summary by CodeRabbit