perf: route ChatGPT Codex upstream turns over responses_websockets - #1487
perf: route ChatGPT Codex upstream turns over responses_websockets#1487kargnas wants to merge 1 commit into
Conversation
|
github-actions
Bot
added
intake: hygiene-blocked
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds a Codex WebSocket upstream for streamed responses. Eligible requests become WebSocket ChangesCodex WebSocket transport
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant providerFetch
participant codexWsUpstreamFetch
participant CodexWebSocket
participant SSEStream
Client->>providerFetch: send Codex streaming POST
providerFetch->>codexWsUpstreamFetch: route eligible request
codexWsUpstreamFetch->>CodexWebSocket: connect and send response.create
CodexWebSocket-->>codexWsUpstreamFetch: emit response events
codexWsUpstreamFetch->>SSEStream: convert events to SSE chunks
SSEStream-->>Client: return streamed response
CodexWebSocket-->>codexWsUpstreamFetch: reject upgrade or close connection
codexWsUpstreamFetch->>Client: use HTTP SSE fallback or stream error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/server/responses/ws-upstream.ts`:
- Line 49: Update the frame construction in the response handling code so the
request body is spread before the fixed type field, ensuring body.type cannot
override the required "response.create" discriminator.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62e4b534-49fe-49ec-9d15-41ce44708c43
📒 Files selected for processing (2)
src/server/responses/fetch-helpers.tssrc/server/responses/ws-upstream.ts
2fae39e to
b876b16
Compare
kargnas
changed the title
b876b16 to
c6f0c08
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
c6f0c08 to
1ad607a
Compare
|
Local full suite on this branch (rebased on dev @ 3b8f948): 11027 pass / 7 fail / 6 errors, all unrelated to this diff:
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@src/server/responses/fetch-helpers.ts`:
- Around line 135-145: Add a focused regression test near the existing
fetch-helper tests that constructs providerFetch with a stubbed provider fetch,
verifies an eligible string URL routes through codexWsUpstreamFetch, and
verifies a non-eligible request uses the stubbed base fetch. Keep direct adapter
tests unchanged while covering the shared routing behavior introduced in
providerFetch.
In `@src/server/responses/ws-upstream.ts`:
- Line 4: Correct the date in the comment above the SSE queue implementation to
the actual measurement date, or remove the date entirely if it cannot be
verified.
- Around line 22-31: Update shouldUseCodexWsUpstream in
src/server/responses/ws-upstream.ts (lines 22-31) to parse the JSON body and
return true only when the top-level stream property is the boolean true;
preserve false for invalid JSON and nested stream values. Add regressions in
tests/ws-upstream.test.ts (lines 15-26) covering nested stream: true and
whitespace-formatted top-level streaming JSON.
- Around line 122-138: Update the ws.send(frameText) catch block in the upstream
response flow to resolve and return sseFallback(url, init) immediately when
sending fails. Preserve the existing controller error and websocket close
cleanup as needed, but do not construct the successful 200 Response after the
fallback is selected.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 078ecb9c-fd48-4728-b264-a5d90c3eb89a
📒 Files selected for processing (3)
src/server/responses/fetch-helpers.tssrc/server/responses/ws-upstream.tstests/ws-upstream.test.ts
| const base = (provider as OcxProviderConfig & { fetch?: typeof globalThis.fetch }).fetch ?? globalThis.fetch; | ||
| // ChatGPT Codex backend: streaming turns ride the responses_websockets | ||
| // transport (measured ~3s faster TTFT than the SSE POST queue); everything | ||
| // else keeps the provider's HTTP fetch. See ws-upstream.ts for the details. | ||
| const wrapped = (input: Parameters<typeof globalThis.fetch>[0], init?: RequestInit) => { | ||
| if (typeof input === "string" && init && shouldUseCodexWsUpstream(input, init)) { | ||
| return codexWsUpstreamFetch(input, init, base); | ||
| } | ||
| return base(input, init); | ||
| }; | ||
| return wrapped as typeof globalThis.fetch; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a providerFetch routing regression test.
The existing suite calls codexWsUpstreamFetch directly. It does not verify the changed shared routing layer. Add a test that creates providerFetch with a stubbed provider fetch, then verifies that an eligible string URL uses the WebSocket adapter and a non-eligible request uses the stubbed base fetch.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 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 `@src/server/responses/fetch-helpers.ts` around lines 135 - 145, Add a focused
regression test near the existing fetch-helper tests that constructs
providerFetch with a stubbed provider fetch, verifies an eligible string URL
routes through codexWsUpstreamFetch, and verifies a non-eligible request uses
the stubbed base fetch. Keep direct adapter tests unchanged while covering the
shared routing behavior introduced in providerFetch.
Source: Path instructions
| // Upstream WebSocket transport for the ChatGPT Codex backend. | ||
| // | ||
| // Why this exists: the Codex backend serves the responses_websockets path from | ||
| // a measurably faster queue than the plain SSE POST path. Measured 2026-08-12 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future measurement date.
Line 4 states that the measurement occurred on August 12, 2026. That date is after August 11, 2026. Use the actual measurement date or remove the date.
🤖 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 `@src/server/responses/ws-upstream.ts` at line 4, Correct the date in the
comment above the SSE queue implementation to the actual measurement date, or
remove the date entirely if it cannot be verified.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
The WebSocket direction is promising, but this head has several correctness blockers before it is safe to replace the canonical passthrough transport:
-
Parse the root
streamflag instead of substring-matching serialized JSON.body.includes("\"stream\":true")can route a non-streaming request to WS when a nested object containsstream: true, and it depends on one serialization spelling. Parse the top-level object and requirestream === true, with nested/whitespace regressions. -
A synchronous
ws.send()failure must fall back to SSE, not return a synthetic HTTP 200 with an errored body. At that point theresponse.createframe did not leave the client, so SSE fallback is safe and preserves the existing HTTP error/refresh/failover machinery. The current 200 response bypasses those pre-stream handlers. -
Do not turn a mid-stream WebSocket drop into clean EOF. The
closehandler callscontroller.close()when no Responses terminal was observed. DownstreamrelaySseWithFailedTail()only synthesizesresponse.failedwhen its reader throws; on clean EOF it simply closes. So a WS reset afterresponse.createdcan reach the client with noresponse.completed/response.failedterminal at all. Error the stream (or synthesize a failed terminal) when an opened socket closes before a terminal event, and add an integration regression through the passthrough relay rather than asserting that a terminal-less stream is acceptable. -
Do not fabricate
originator: codex_cli_rsfor callers that did not send it. The existing metadata-integrity contract explicitly verifies that pool/forward traffic does not inventoriginatorand only preserves genuine caller metadata. This wrapper silently breaks that invariant after adapter construction and also means this PR is no longer just a transport swap: non-Codex/SDK traffic is represented upstream as Codex CLI traffic specifically to change backend scheduling. Preserve a real incoming originator, or make any synthetic provenance an explicit, separately reviewed policy rather than a transport default.
Please also add routing-level coverage for providerFetch() itself so the shared dispatch boundary is tested, not only codexWsUpstreamFetch() directly.
Current head is also 12 commits behind dev. The GitHub Actions runs on this head are action_required with no jobs, so there is no current cross-platform CI result to validate these transport changes yet. Please rebase and rerun the full maintained matrix after the fixes.
The ChatGPT Codex backend serves the responses_websockets (WS) path from
a measurably faster queue than the plain SSE POST path. Measured
2026-08-12 KST (same account, same payload, strictly sequential):
gpt-5.6-luna TTFT p50 ~1.0s over WS vs ~3.9s over SSE. Codex CLI itself
defaults to WS, so requests through opencodex carried an extra 2-3s of
TTFT that direct Codex CLI usage did not.
Wrap providerFetch() so that streaming POSTs to
chatgpt.com/backend-api/codex/responses dial wss:// instead: the JSON
body goes out as a single response.create frame and returned event
frames are re-encoded as an SSE byte stream, leaving every downstream
consumer (passthrough relay, adapter parsers, usage sniffing) unchanged.
Transport selection parses the body and requires a root-level
stream === true, so nested {"metadata":{"stream":true}} or formatted
JSON cannot misroute. Failure handling: upgrade rejection, a missing
101 within 10s, and a synchronous frame-send failure all fall back to
the existing SSE path (in every case no upstream turn has started, so
the resend cannot double-generate); a socket drop after open but before
a Responses terminal event errors the stream so relaySseWithFailedTail
synthesizes a response.failed terminal instead of a terminal-less clean
EOF. Caller metadata is preserved verbatim — no originator is invented
for callers that did not send one.
After patching, local benchmarks put opencodex within ~0.2s of direct
Codex CLI (Luna TTFT p50 4364ms -> 1424ms; Terra 3322ms -> 1417ms).
Verified tool-call round-trips, 630KB-1.5MB frames, context_length
error relay, and ~600 live requests with no new failure modes.
Known trade-off: Bun's WebSocket does not expose the 101 response
headers, so x-codex-*-reset-at quota hints are not visible on this
path; the periodic quota poller still covers quota tracking.
Regression tests cover root-level stream detection, providerFetch()
routing, frame relay (including WS-only frame dropping), SSE fallback
on upgrade rejection and send failure, mid-stream drop through the
passthrough relay (synthesized failed terminal), header preservation
without originator fabrication, and pre-open abort.
1ad607a to
ab6140a
Compare
|
@Wibias Thanks for the thorough review — all four blockers are addressed in ab6140a (rebased onto dev @ 4fed8d3):
Also added the
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head ab6140a83db061e76b317454a1a43c5e909901bd.
The previous transport blockers are fixed on this head:
- WebSocket selection parses the JSON body and requires root-level
stream === true; - a synchronous
ws.send()failure safely returns to the SSE fallback instead of exposing a synthetic 200/erroring body; - a socket close after open but before a Responses terminal errors the stream, and the relay regression verifies clients receive a synthesized
response.failed+[DONE]rather than clean terminal-less EOF; - the wrapper no longer fabricates
originator: codex_cli_rs; a genuine caller value is preserved verbatim; response.createcannot be overridden by a request-bodytypefield;- the shared
providerFetch()routing boundary now has focused regression coverage.
I did not find a new code-level blocker in the current patch.
The remaining blocker is integration validation. Current dev is 70d2e1758c180188a729dd63812703b76cfeeba6; this head is 11 commits behind with merge base 4fed8d3fe431ad23be83f3aff2af18ef8b8ecd71. Exact-head Cross-platform CI and React Doctor are action_required, and the Cross-platform run contains 0 jobs, so the maintained matrix has not actually executed on this head. This changes the canonical upstream transport, so I do not want to approve it without a current integrated run.
The intervening dev commits do not modify these production transport files, so if the rebase is clean and the resulting exact-head maintained CI is green, I expect this to be ready for approval.
2 participants
Problem
Requests proxied through opencodex to the ChatGPT Codex backend consistently showed 2–3s worse TTFT than the same requests made by Codex CLI directly — even with the same account, same payload, and strictly sequential execution.
Root cause: Codex CLI talks to
chatgpt.com/backend-api/codex/responsesover theresponses_websocketstransport, while opencodex always POSTs SSE. The backend serves the WS path from a measurably faster queue.Measurements (2026-08-12, same account, same payload, sequential, alternating order)
responses_websockets)stream:true)Event timeline shows the gap is upstream scheduling, not transfer: both paths reach
response.createdin ~0.5s, but SSE then waits 2.4–4.0s beforeresponse.output_item.added(WS: 0.8–1.3s).Ruled out: HTTP/2 vs 1.1 (no change),
OpenAI-Beta: responses_websocketsheader on the SSE POST (no change),session_id/prompt_cache_key(no change), warmup effects (no decay over sequential repeats), account differences (A/B with identical account).A second finding: the fast lane keys on WS +
originatortag, not the transport alone — 60KB turns run ~1.5s withoriginator: codex_cli_rsvs ~4.6s without. The patch defaults the header for callers that don't send one (Codex CLI always does).Change
src/server/responses/ws-upstream.ts: for streaming POSTs to the Codex backend, dialwss://with the same headers, send the JSON body as a singleresponse.createframe, and re-encode returned event frames as an SSE byte stream — so the passthrough relay, adapter parsers, and usage sniffing are all unchanged.providerFetch()infetch-helpers.tswraps the provider fetch with this transport swap. Everything that isn't a Codex-backend streaming turn keeps the exact HTTP path.codex.rate_limits,responsesapi.websocket_timing) are dropped so clients see exactly the stream shape they always got.After patching (local proxy vs direct Codex CLI)
Also verified: tool-call round-trips (function_call arguments relay), 630KB / 900KB / 1.26MB / 1.5MB input frames,
context_length_exceededandserver_is_overloadederror relay, and ~600 live requests through the patched proxy with no new failure modes.Known trade-off
Bun's
WebSocketdoes not expose the 101 response headers, sox-codex-*-reset-atquota hints are not visible on this path. The periodic quota poller still covers quota tracking. If there's a preferred way to surface those, happy to adjust.Related: #1217 (stream-stage timing) would make this kind of transport gap visible in the dashboard.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Reliability