fix: stream responses transform through chat completions#154
Conversation
📝 WalkthroughWalkthroughThe OpenAI bridge now routes SSE-accepting ChangesResponses SSE Streaming Conversion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/openai-bridge-upstream-responses.test.mjs (1)
91-170: 💤 Low valueOptional: Add a hang-guard for the /responses-probe regression test.
The upstream
/v1/responseshandler at lines 95-99 intentionally never responds. If a future change causes the bridge to start probing/responsesagain, the bridge request will hang indefinitely instead of producing a clear assertion failure, sincerequestTexthas no timeout. Consider adding a short upstream timeout (e.g.,res.socket?.setTimeout) or a wall-clock guard around the request to fail fast with a useful message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/openai-bridge-upstream-responses.test.mjs` around lines 91 - 170, The /v1/responses probe in the test intentionally never responds, which can cause the test to hang indefinitely; add a short hang-guard to fail fast — either set a socket timeout on the incoming upstream request inside the upstream handler for '/v1/responses' (e.g., use req.socket.setTimeout(...) and in its callback end the response with a 504 or similar) or wrap the requestText call in the test with a wall-clock timeout promise that rejects after a few seconds and fails the test; update the test named "openai-bridge streams chat/completions directly when Responses client requests SSE" referencing the upstream handler and requestText so the test will abort quickly if /v1/responses is ever probed again.cli/openai-bridge.js (2)
910-925: 💤 Low valueDetach the
closelistener after upstream finishes.
res.once('close', abortUpstream)is registered for the lifetime of the response. After successful completion the listener still fires oncecloseis emitted;upstreamReq.destroy()on an already-finished request is a safe no-op, so this is not a bug — but thestateandupstreamReqclosures stay reachable from theresevent listener until the socket is GC'd. Removing the listener (and pairing it withres.off('close', abortUpstream)oncefinishresolves) keeps the lifetime tighter and avoids a small retention window when many concurrent streams are in flight.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/openai-bridge.js` around lines 910 - 925, The close listener attached with res.once('close', abortUpstream) should be removed when the upstream work finishes to avoid retaining the finish/upstreamReq/settled closures; update code so that finish (or the promise resolution path) calls res.off('close', abortUpstream) (guarding that typeof res.off === 'function') after it marks settled and resolves, and also ensure any other early-return paths call the same cleanup so abortUpstream is detached whenever the upstream completes.
788-794: ⚖️ Poor tradeoffStreaming tool-call argument deltas are not emitted.
writeChatCompletionChunkAsResponsesSseaccumulatesdelta.tool_callsintostate.toolCallswithout emitting SSE events during the stream. Tool calls are only announced infinishChatStreamResponsesSseon completion withresponse.output_item.addedimmediately followed byresponse.output_item.done. This differs from text content handling, which emitsresponse.output_text.deltaevents during streaming.Clients relying on
response.function_call_arguments.deltato render progressive tool-call arguments or begin execution before all arguments arrive will instead receive complete tool calls all-at-once. While this defers alignment with OpenAI Responses API streaming events, it is reasonable to defer given effort vs. immediate impact.Also applies to: 818-841
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/openai-bridge.js` around lines 788 - 794, The function writeChatCompletionChunkAsResponsesSse currently accumulates streaming tool-call fragments into state.toolCalls via appendChatStreamToolCall but never emits progressive SSE events; change it so that whenever appendChatStreamToolCall appends or updates a tool call fragment inside writeChatCompletionChunkAsResponsesSse you also emit a SSE event named response.function_call_arguments.delta (matching the text delta flow) carrying the incremental arguments/fragment and identifying which toolCall (e.g., by index or id) is being updated; ensure the same logic is added to the analogous block around lines 818-841 and keep finishChatStreamResponsesSse behavior (response.output_item.added + response.output_item.done) for completion only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/openai-bridge.js`:
- Around line 910-925: The close listener attached with res.once('close',
abortUpstream) should be removed when the upstream work finishes to avoid
retaining the finish/upstreamReq/settled closures; update code so that finish
(or the promise resolution path) calls res.off('close', abortUpstream) (guarding
that typeof res.off === 'function') after it marks settled and resolves, and
also ensure any other early-return paths call the same cleanup so abortUpstream
is detached whenever the upstream completes.
- Around line 788-794: The function writeChatCompletionChunkAsResponsesSse
currently accumulates streaming tool-call fragments into state.toolCalls via
appendChatStreamToolCall but never emits progressive SSE events; change it so
that whenever appendChatStreamToolCall appends or updates a tool call fragment
inside writeChatCompletionChunkAsResponsesSse you also emit a SSE event named
response.function_call_arguments.delta (matching the text delta flow) carrying
the incremental arguments/fragment and identifying which toolCall (e.g., by
index or id) is being updated; ensure the same logic is added to the analogous
block around lines 818-841 and keep finishChatStreamResponsesSse behavior
(response.output_item.added + response.output_item.done) for completion only.
In `@tests/unit/openai-bridge-upstream-responses.test.mjs`:
- Around line 91-170: The /v1/responses probe in the test intentionally never
responds, which can cause the test to hang indefinitely; add a short hang-guard
to fail fast — either set a socket timeout on the incoming upstream request
inside the upstream handler for '/v1/responses' (e.g., use
req.socket.setTimeout(...) and in its callback end the response with a 504 or
similar) or wrap the requestText call in the test with a wall-clock timeout
promise that rejects after a few seconds and fails the test; update the test
named "openai-bridge streams chat/completions directly when Responses client
requests SSE" referencing the upstream handler and requestText so the test will
abort quickly if /v1/responses is ever probed again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c94139fb-5d8f-473b-9152-71cbf59399d1
📒 Files selected for processing (2)
cli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjs
📜 Review details
🔇 Additional comments (1)
cli/openai-bridge.js (1)
858-877: ⚡ Quick winMove error details into response object per OpenAI Responses API shape — but verify official spec first.
The current
failChatStreamResponsesSseplaces error at the top level:{ type, response: {...}, error: string }. However, this conflicts with other error handlers in the same file (lines ~909, ~943) which also use top-levelerror. Before restructuring, verify against the official OpenAI Responses API schema whether error should live insideresponseas a nested object. Official documentation is incomplete; confirm with OpenAI's TypeScript SDK types or API reference whether consumers expectresponse.error.messageor top-level error, and whether all error handlers should be consistent.
2 participants
Summary
/v1/responsesbridge requests directly through upstream/chat/completionsstreaming instead of blocking on upstream/responses[DONE]/responsesprobe and for premature upstream stream terminationValidation
NODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules node --test tests/unit/openai-bridge-upstream-responses.test.mjs tests/unit/builtin-proxy-responses-shim.test.mjsNODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run lintNODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run test:unit— 527 tests passedNODE_PATH=/home/moltbot/clawd-wechat/codexmate/node_modules npm run test:e2egit diff --checkNote: this worktree reuses the existing dependency install from
/home/moltbot/clawd-wechat/codexmate/node_modulesviaNODE_PATHbecause the clean worktree does not have its ownnode_modulesdirectory.Summary by CodeRabbit