feat(responses): opt-in upstream Responses WebSocket transport (#2816, carry of #2817) - #3216
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughAdds an optional ChangesProvider WebSocket Transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR adds an opt-in WebSocket transport for eligible HTTPS Responses providers while preserving existing HTTP behavior by default. It is mergeable with owner awareness of a bounded concurrent-update race that can briefly restore an older transport setting during overlapping provider changes. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant providerFetch
participant shouldUseCodexWsUpstream
participant codexWsUpstreamFetch
participant OpenAICompatibleProvider
participant SSEPipeline
providerFetch->>shouldUseCodexWsUpstream: Check provider flag, URL, method, body, and stream
shouldUseCodexWsUpstream->>providerFetch: Select WebSocket transport for eligible HTTPS /responses URL
providerFetch->>codexWsUpstreamFetch: Send streaming Responses request
codexWsUpstreamFetch->>OpenAICompatibleProvider: Dial provider-specific wss URL
OpenAICompatibleProvider-->>codexWsUpstreamFetch: Return Responses WebSocket frames
codexWsUpstreamFetch->>SSEPipeline: Normalize terminal events and re-encode frames as SSE
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation satisfies issue Full details: Out of Scope Changes checkExplanation The changes remain within the linked issue scope. Updates to Full details: Docstring CoverageExplanation Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 7 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
![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: bc884eaa40
ℹ️ 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".
| return false; | ||
| } | ||
| return parsed.protocol === "https:" | ||
| && parsed.pathname.endsWith("/responses"); |
There was a problem hiding this comment.
When an opted-in provider uses a valid responsesPath such as /v1/responses/, createResponsesPassthroughAdapter preserves that path, but this predicate returns false because the pathname does not end exactly in /responses. Such providers silently remain on HTTP/SSE even though the existing Responses URL handling explicitly accepts a trailing slash; normalize trailing slashes or accept /responses/ here.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 72 / 80이 PR은 #2816을 닫는 메인테이너 캐리입니다. 원본은 @gulup(jiangpeng)의 #2817이고, 현재 기본값은 꺼짐입니다. 플래그가 없거나 false면 예전과 같고, 켜도 표면은 현재 검증은 로컬에서 ws-upstream / management-provider-validation 등 169 pass를 주장하고, 이 리뷰 시점 CI는 대부분 pass이며 macos·test 2/4만 아직 pending입니다. PR 본문이 말한 macOS
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
dev, authorship preserved), resolving Feature request: opt-in upstream Responses WebSocket transport for OpenAI-compatible providers #2816: an opt-in provider flagupstreamWebsocketthat lets an HTTPS/responsesupstream be dialed over WebSocket (wss) using the same relay Codex already uses for the canonical ChatGPT backend.https:URLs whose path ends in/responsesare eligible;http://, chat completions, images, and search stay on HTTP SSE. Any pre-open failure (constructor, send, upgrade timeout, oversized frame, 401/403/426/429/5xx close) falls back to the existing SSE path. Official WSresponse.doneis mapped to SSEresponse.completed/failed/incomplete, with unknown or queued status failing closed.GET/POST/PATCH /api/providersexpose and persist the boolean, POST overwrite preserves an omitted value; docs on the provider pages. No dashboard toggle (dropped by the author on review).devand conflicted with themodelDisplayNames/retainModelsPOST carry-over block; resolved here keeping both omit-preserves. The macOS red on the PR head was the knownserver-authwebsocket-passthrough timing case, with all Linux shards green.src/router.ts,src/server/lifecycle.ts,src/server/responses/core.ts, orsrc/server/index.ts; no newstartServerawaits; no request-body or token logging.Closes #2816
Verification
bun x tsc --noEmitclean;bun run privacy:scanpassed.bun test tests/ws-upstream.test.ts tests/management-provider-validation.test.ts tests/core-lab-boundary.test.ts tests/cli-capabilities.test.ts-> 169 pass / 0 fail (opt-in matrix: flag off, HTTPS/responses, HTTP rejected, non-Responses,response.donecompleted/failed/unknown, WSS dial URL, POST/PATCH/overwrite preserve).Checklist
Summary by CodeRabbit
upstreamWebsocketprovider setting for Responses requests.