feat: add OpenAI Responses WebSocket transport for /v1/responses#882
Closed
feat: add OpenAI Responses WebSocket transport for /v1/responses#882
Conversation
Introduce WebSocket-based transport for the Responses API, enabling Codex CLI and other clients to use native WS streaming instead of SSE. - Add custom Node.js server (server.js) with WS upgrade handling that bridges WebSocket frames to upstream SSE and back - Implement WS transport layer: adapter, schema validation, transport selector, and terminal frame finalization - Integrate WS transport into ProxyForwarder with automatic HTTP fallback on handshake/setup failures - Add system setting toggle (enableResponsesWebSocket) with DB migration, validation, settings UI, and i18n (5 languages) - Extend provider test service to support WS transport testing with diagnostic details (handshake time, event count, fallback reason) - Add ResponsesWebSocketTransportSpecialSetting for observability - Update Dockerfile for custom server, ws dependency tracing, and standalone build pipeline - Add comprehensive unit and integration tests
Security: - Add maxPayload (1 MiB) to WebSocket server to prevent DoS - Propagate AbortSignal to upstream WS adapter so client disconnect aborts the upstream connection instead of leaking resources Correctness: - Unify terminal event type sets: add "error" to schema.ts to match server.js, preventing hang on upstream error events - Replace unsafe string replace with TERMINAL_STATE_MAP lookup for deterministic terminal state derivation - Add missing setEnableResponsesWebSocket state refresh after save in settings form - Handle OpenAI protocol errors (previous_response_not_found, websocket_connection_limit_reached) as application-level terminal events without triggering HTTP fallback Hardening: - Add 30s ping/pong keepalive on ingress WS to survive LB idle timeouts - Hoist TextEncoder to module scope to reduce GC pressure - Sanitize websocketUrl before persisting to special settings - Replace hardcoded beta header string with CONTEXT_1M_BETA_HEADER constant reference Tests: - Add tests for response.incomplete and error terminal states - Add tests for disabled toggle and unsupported provider type paths - Add tests for isResponsesWsTerminalEvent and response.cancel frame - Add tests for OpenAI protocol error relay without HTTP fallback
- Fixed 'Type string | number | ((obj: any) => any' is not assignable to type Key | null | undefined' error by converting to string using String() - Fixed formatter function call to convert arrays to strings when value is an array - Fixed unused parameter warning by prefixing with underscore - Fixed unused parameter in virtualized-logs-table.tsx Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> CI Run: https://github.com/ding113/claude-code-hub/actions/runs/22821806312
📝 WalkthroughWalkthrough本次提交在代理层引入“Responses WebSocket”传输支持：新增WS协议适配器、传输决策与终端事件处理、SSE↔WS桥接服务器、数据库 schema 与系统设置开关、前端 i18n/表单与测试覆盖。 Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural enhancement by integrating WebSocket support for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an impressive pull request that adds significant new functionality with WebSocket support for /v1/responses. The implementation is well-structured, introducing a custom Node.js server to handle both HTTP and WebSocket traffic on the same port, which is a clean solution. The new WebSocket adapter, SSE-to-WS bridge, and the fallback mechanism to HTTP are robustly implemented. The extensive test suite added for this new functionality is also commendable.
My review has one suggestion regarding the build script, which is a minor maintainability concern about a regular expression that could be fragile to future Next.js updates. Overall, this is a high-quality contribution.
Note: Security Review did not run due to the size of the PR.
Comment on lines
+22
to
+25
| const match = content.match(/const nextConfig = (.+?)\n\nprocess\.env\.__NEXT_PRIVATE_STANDALONE_CONFIG/s); | ||
| if (!match) { | ||
| throw new Error("[copy-standalone] Failed to extract standalone nextConfig"); | ||
| } |
Contributor
There was a problem hiding this comment.
This regex is quite specific to the current output format of Next.js's standalone server file. It relies on const nextConfig = being followed by exactly two newlines and then process.env.__NEXT_PRIVATE_STANDALONE_CONFIG. This could break in future Next.js versions if they change their build output, causing the build to fail.
To improve maintainability, I'd suggest adding a comment to the code warning about this fragility. A more robust parsing method would be ideal, but might be complex to implement. For now, a warning comment would be very helpful for future updates.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+308
to
+352
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) { | ||
| break; | ||
| } | ||
| if (!value || value.byteLength === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| buffer += decoder.decode(value, { stream: true }); | ||
| if (buffer.length > MAX_SSE_BUFFER_CHARS) { | ||
| throw new Error("Buffered SSE frame exceeded safety limit"); | ||
| } | ||
|
|
||
| const { blocks, rest } = extractSseBlocks(buffer); | ||
| buffer = rest; | ||
|
|
||
| for (const block of blocks) { | ||
| const parsedEvent = parseSseBlock(block); | ||
| const frame = sseEventToWsFrame(parsedEvent); | ||
| if (!frame) { | ||
| continue; | ||
| } | ||
| writeJsonFrame(socket, frame); | ||
| if (RESPONSES_WS_TERMINAL_TYPES.has(frame.type)) { | ||
| terminalSeen = true; | ||
| } | ||
| } | ||
|
|
||
| if (terminalSeen) { | ||
| break; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if (!state.activeAbortController.signal.aborted) { | ||
| writeProtocolError( | ||
| socket, | ||
| "bridge_failed", | ||
| error instanceof Error ? error.message : "Responses WebSocket bridge failed" | ||
| ); | ||
| } | ||
| } finally { | ||
| state.activeTurn = false; | ||
| state.activeAbortController = null; | ||
| } |
There was a problem hiding this comment.
SSE reader resource not cancelled on buffer overflow error.
When buffer.length > MAX_SSE_BUFFER_CHARS (line 319), a throw exits the while loop and the outer catch block at line 341 handles it, but reader.cancel() is never called. The underlying HTTP keep-alive connection to the loopback endpoint stays open, leaking the TCP slot until the server-side closes it.
Suggested change
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) { | |
| break; | |
| } | |
| if (!value || value.byteLength === 0) { | |
| continue; | |
| } | |
| buffer += decoder.decode(value, { stream: true }); | |
| if (buffer.length > MAX_SSE_BUFFER_CHARS) { | |
| throw new Error("Buffered SSE frame exceeded safety limit"); | |
| } | |
| const { blocks, rest } = extractSseBlocks(buffer); | |
| buffer = rest; | |
| for (const block of blocks) { | |
| const parsedEvent = parseSseBlock(block); | |
| const frame = sseEventToWsFrame(parsedEvent); | |
| if (!frame) { | |
| continue; | |
| } | |
| writeJsonFrame(socket, frame); | |
| if (RESPONSES_WS_TERMINAL_TYPES.has(frame.type)) { | |
| terminalSeen = true; | |
| } | |
| } | |
| if (terminalSeen) { | |
| break; | |
| } | |
| } | |
| } catch (error) { | |
| if (!state.activeAbortController.signal.aborted) { | |
| writeProtocolError( | |
| socket, | |
| "bridge_failed", | |
| error instanceof Error ? error.message : "Responses WebSocket bridge failed" | |
| ); | |
| } | |
| } finally { | |
| state.activeTurn = false; | |
| state.activeAbortController = null; | |
| } | |
| const reader = response.body?.getReader(); | |
| if (!reader) { | |
| writeProtocolError(socket, "bridge_stream_missing", "Loopback bridge returned no SSE body"); | |
| return; | |
| } | |
| const decoder = new TextDecoder(); | |
| let buffer = ""; | |
| let terminalSeen = false; | |
| try { | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) { | |
| break; | |
| } | |
| if (!value || value.byteLength === 0) { | |
| continue; | |
| } | |
| buffer += decoder.decode(value, { stream: true }); | |
| if (buffer.length > MAX_SSE_BUFFER_CHARS) { | |
| throw new Error("Buffered SSE frame exceeded safety limit"); | |
| } | |
| const { blocks, rest } = extractSseBlocks(buffer); | |
| buffer = rest; | |
| for (const block of blocks) { | |
| const parsedEvent = parseSseBlock(block); | |
| const frame = sseEventToWsFrame(parsedEvent); | |
| if (!frame) { | |
| continue; | |
| } | |
| writeJsonFrame(socket, frame); | |
| if (RESPONSES_WS_TERMINAL_TYPES.has(frame.type)) { | |
| terminalSeen = true; | |
| } | |
| } | |
| if (terminalSeen) { | |
| break; | |
| } | |
| } | |
| } finally { | |
| reader.cancel().catch(() => {}); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server.js
Line: 308-352
Comment:
SSE reader resource not cancelled on buffer overflow error.
When `buffer.length > MAX_SSE_BUFFER_CHARS` (line 319), a `throw` exits the `while` loop and the outer `catch` block at line 341 handles it, but `reader.cancel()` is never called. The underlying HTTP keep-alive connection to the loopback endpoint stays open, leaking the TCP slot until the server-side closes it.
```suggestion
const reader = response.body?.getReader();
if (!reader) {
writeProtocolError(socket, "bridge_stream_missing", "Loopback bridge returned no SSE body");
return;
}
const decoder = new TextDecoder();
let buffer = "";
let terminalSeen = false;
try {
while (true) {
const { done, value } = await reader.read();
if (done) {
break;
}
if (!value || value.byteLength === 0) {
continue;
}
buffer += decoder.decode(value, { stream: true });
if (buffer.length > MAX_SSE_BUFFER_CHARS) {
throw new Error("Buffered SSE frame exceeded safety limit");
}
const { blocks, rest } = extractSseBlocks(buffer);
buffer = rest;
for (const block of blocks) {
const parsedEvent = parseSseBlock(block);
const frame = sseEventToWsFrame(parsedEvent);
if (!frame) {
continue;
}
writeJsonFrame(socket, frame);
if (RESPONSES_WS_TERMINAL_TYPES.has(frame.type)) {
terminalSeen = true;
}
}
if (terminalSeen) {
break;
}
}
} finally {
reader.cancel().catch(() => {});
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+285
to
+289
| frame: { | ||
| type: "response.create", | ||
| response: params.body as Record<string, unknown> & { model: string }, | ||
| }, | ||
| headers: params.headers, |
There was a problem hiding this comment.
Unsafe cast bypasses model field validation in WebSocket test payload.
params.body is cast directly to Record<string, unknown> & { model: string } without validating that model is actually a non-empty string. If the test body omits or has an invalid model field, the upstream provider will reject the frame with a protocol error surfaced as an opaque WS failure rather than a clear validation error.
The HTTP test path uses parseResponsesWsInitialFrame for validation. Use the same here:
Suggested change
| frame: { | |
| type: "response.create", | |
| response: params.body as Record<string, unknown> & { model: string }, | |
| }, | |
| headers: params.headers, | |
| let frame: ResponsesWsCreateFrame; | |
| try { | |
| frame = parseResponsesWsInitialFrame({ | |
| type: "response.create", | |
| response: params.body, | |
| }); | |
| } catch (err) { | |
| throw new Error(`Invalid test payload for Responses WS: ${err instanceof Error ? err.message : String(err)}`); | |
| } | |
| const response = await sendResponsesWsRequest({ | |
| websocketUrl: decision.websocketUrl, | |
| frame, | |
| headers: params.headers, | |
| isStreaming: true, | |
| handshakeTimeoutMs: params.timeoutMs, | |
| firstEventTimeoutMs: params.timeoutMs, | |
| onOpen: (latencyMs) => { | |
| handshakeLatencyMs = latencyMs; | |
| }, | |
| onEvent: () => { | |
| eventCount += 1; | |
| }, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/provider-testing/test-service.ts
Line: 285-289
Comment:
Unsafe cast bypasses `model` field validation in WebSocket test payload.
`params.body` is cast directly to `Record<string, unknown> & { model: string }` without validating that `model` is actually a non-empty string. If the test body omits or has an invalid `model` field, the upstream provider will reject the frame with a protocol error surfaced as an opaque WS failure rather than a clear validation error.
The HTTP test path uses `parseResponsesWsInitialFrame` for validation. Use the same here:
```suggestion
let frame: ResponsesWsCreateFrame;
try {
frame = parseResponsesWsInitialFrame({
type: "response.create",
response: params.body,
});
} catch (err) {
throw new Error(`Invalid test payload for Responses WS: ${err instanceof Error ? err.message : String(err)}`);
}
const response = await sendResponsesWsRequest({
websocketUrl: decision.websocketUrl,
frame,
headers: params.headers,
isStreaming: true,
handshakeTimeoutMs: params.timeoutMs,
firstEventTimeoutMs: params.timeoutMs,
onOpen: (latencyMs) => {
handshakeLatencyMs = latencyMs;
},
onEvent: () => {
eventCount += 1;
},
});
```
How can I resolve this? If you propose a fix, please make it concise.
Fixed CI formatting failures detected by Biome: - src/components/ui/chart.tsx: indentation adjustments (143 lines changed) - tsconfig.json: compact array/object formatting (22 lines changed) CI Run: https://github.com/ding113/claude-code-hub/actions/runs/22823442958 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+282
to
+283
| const payload = await response.json().catch(async () => ({ body: await response.text() })); | ||
| if (payload && payload.error) { |
There was a problem hiding this comment.
Body already consumed when response.json() fails
response.json() internally calls response.text() which fully consumes the body stream. If JSON parsing fails, the body is now in a "disturbed" state. The catch handler then calls response.text() on an already-consumed body, which will throw TypeError: body used already (Node.js/undici) or return an empty string — meaning the { body: ... } fallback never actually captures any content.
The fix is to read text first and then attempt JSON parsing:
Suggested change
| const payload = await response.json().catch(async () => ({ body: await response.text() })); | |
| if (payload && payload.error) { | |
| const text = await response.text().catch(() => ""); | |
| let payload: unknown; | |
| try { | |
| payload = JSON.parse(text); | |
| } catch { | |
| payload = { body: text }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server.js
Line: 282-283
Comment:
**Body already consumed when `response.json()` fails**
`response.json()` internally calls `response.text()` which fully consumes the body stream. If JSON parsing fails, the body is now in a "disturbed" state. The catch handler then calls `response.text()` on an already-consumed body, which will throw `TypeError: body used already` (Node.js/undici) or return an empty string — meaning the `{ body: ... }` fallback never actually captures any content.
The fix is to read `text` first and then attempt JSON parsing:
```suggestion
const text = await response.text().catch(() => "");
let payload: unknown;
try {
payload = JSON.parse(text);
} catch {
payload = { body: text };
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+337
to
+340
| if (terminalSeen) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Loopback SSE stream not cancelled after terminal event break
When terminalSeen causes the while loop to exit via break, the reader obtained at line 298 is never reader.cancel()'d. The loopback HTTP keep-alive connection remains open and continues to receive/buffer SSE bytes from the Next.js server until the server-side naturally closes the stream (after [DONE]). Under load, this can hold open multiple TCP connections unnecessarily.
The fix is to cancel the reader before breaking:
Suggested change
| if (terminalSeen) { | |
| break; | |
| } | |
| } | |
| if (terminalSeen) { | |
| reader.cancel().catch(() => {}); | |
| break; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server.js
Line: 337-340
Comment:
**Loopback SSE stream not cancelled after terminal event break**
When `terminalSeen` causes the `while` loop to exit via `break`, the `reader` obtained at line 298 is never `reader.cancel()`'d. The loopback HTTP keep-alive connection remains open and continues to receive/buffer SSE bytes from the Next.js server until the server-side naturally closes the stream (after `[DONE]`). Under load, this can hold open multiple TCP connections unnecessarily.
The fix is to cancel the reader before breaking:
```suggestion
if (terminalSeen) {
reader.cancel().catch(() => {});
break;
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+59
to
+62
| function toSseChunk(event: ResponsesWsServerEvent): Uint8Array { | ||
| const payload = JSON.stringify(stripTypeField(event)); | ||
| return sseEncoder.encode(`event: ${event.type}\ndata: ${payload}\n\n`); | ||
| } |
There was a problem hiding this comment.
Unvalidated event.type used directly in SSE output — potential newline injection
event.type is validated by ResponsesWsServerEventSchema as z.string().trim().min(1), but this does not prevent embedded \n or \r\n characters. If an upstream provider sends a crafted type value like "response.completed\ndata: injected", the resulting SSE chunk would contain injected fields that a downstream SSE parser would interpret as real events.
Since the SSE Response is ultimately forwarded to the end client via forwarder.ts, this could confuse the client's SSE parser. Consider stripping or rejecting newlines:
Suggested change
| function toSseChunk(event: ResponsesWsServerEvent): Uint8Array { | |
| const payload = JSON.stringify(stripTypeField(event)); | |
| return sseEncoder.encode(`event: ${event.type}\ndata: ${payload}\n\n`); | |
| } | |
| function toSseChunk(event: ResponsesWsServerEvent): Uint8Array { | |
| const safeType = event.type.replace(/[\r\n]/g, ""); | |
| const payload = JSON.stringify(stripTypeField(event)); | |
| return sseEncoder.encode(`event: ${safeType}\ndata: ${payload}\n\n`); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/responses-ws-adapter.ts
Line: 59-62
Comment:
**Unvalidated `event.type` used directly in SSE output — potential newline injection**
`event.type` is validated by `ResponsesWsServerEventSchema` as `z.string().trim().min(1)`, but this does not prevent embedded `\n` or `\r\n` characters. If an upstream provider sends a crafted `type` value like `"response.completed\ndata: injected"`, the resulting SSE chunk would contain injected fields that a downstream SSE parser would interpret as real events.
Since the SSE `Response` is ultimately forwarded to the end client via `forwarder.ts`, this could confuse the client's SSE parser. Consider stripping or rejecting newlines:
```suggestion
function toSseChunk(event: ResponsesWsServerEvent): Uint8Array {
const safeType = event.type.replace(/[\r\n]/g, "");
const payload = JSON.stringify(stripTypeField(event));
return sseEncoder.encode(`event: ${safeType}\ndata: ${payload}\n\n`);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+163
to
+191
| const socket = new WebSocket(params.websocketUrl, { | ||
| headers: params.headers, | ||
| handshakeTimeout: timeoutProfile.handshakeTimeoutMs, | ||
| }); | ||
|
|
||
| const onAbort = params.abortSignal | ||
| ? () => { | ||
| socket.close(); | ||
| } | ||
| : null; | ||
| if (params.abortSignal && onAbort) { | ||
| if (params.abortSignal.aborted) { | ||
| socket.close(); | ||
| } else { | ||
| params.abortSignal.addEventListener("abort", onAbort, { once: true }); | ||
| } | ||
| } | ||
|
|
||
| handshakeTimeoutId = setTimeout(() => { | ||
| socket.terminate(); | ||
| rejectTransport( | ||
| new ResponsesWsTransportError("Responses WebSocket handshake timed out", { | ||
| fallbackReason: "handshake_timeout", | ||
| allowHttpFallback: true, | ||
| upstreamRequestEstablished: false, | ||
| }) | ||
| ); | ||
| }, timeoutProfile.handshakeTimeoutMs); | ||
|
|
There was a problem hiding this comment.
Dual handshake timeout produces non-deterministic fallbackReason
Both the ws library's built-in handshakeTimeout option (line 165) and the manual setTimeout at line 181 use the same timeoutProfile.handshakeTimeoutMs duration. This creates a race:
- If the
wslibrary fires first, it emits anerrorevent ("Opening handshake has timed out") which reaches theonce('error')handler and setsfallbackReasonto"transport_setup_failed". - If the manual
setTimeoutfires first, it correctly setsfallbackReasonto"handshake_timeout".
Since both timers have identical durations, which one fires first is non-deterministic (depends on the event loop). The intended "handshake_timeout" reason may never be observed in practice.
Consider removing the duplicate handshakeTimeout option from the ws constructor and relying solely on the manual setTimeout — or vice versa — to get a deterministic fallbackReason:
const socket = new WebSocket(params.websocketUrl, {
headers: params.headers,
// Remove handshakeTimeout here; the manual setTimeout below takes sole ownership
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/responses-ws-adapter.ts
Line: 163-191
Comment:
**Dual handshake timeout produces non-deterministic `fallbackReason`**
Both the `ws` library's built-in `handshakeTimeout` option (line 165) and the manual `setTimeout` at line 181 use the same `timeoutProfile.handshakeTimeoutMs` duration. This creates a race:
- If the `ws` library fires first, it emits an `error` event ("Opening handshake has timed out") which reaches the `once('error')` handler and sets `fallbackReason` to `"transport_setup_failed"`.
- If the manual `setTimeout` fires first, it correctly sets `fallbackReason` to `"handshake_timeout"`.
Since both timers have identical durations, which one fires first is non-deterministic (depends on the event loop). The intended `"handshake_timeout"` reason may never be observed in practice.
Consider removing the duplicate `handshakeTimeout` option from the `ws` constructor and relying solely on the manual `setTimeout` — or vice versa — to get a deterministic `fallbackReason`:
```typescript
const socket = new WebSocket(params.websocketUrl, {
headers: params.headers,
// Remove handshakeTimeout here; the manual setTimeout below takes sole ownership
});
```
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| buffer += decoder.decode(value, { stream: true }); | ||
| if (buffer.length > MAX_SSE_BUFFER_CHARS) { |
Contributor
There was a problem hiding this comment.
[Critical] [ERROR-SILENT] SSE reader not canceled on buffer overflow, leaking HTTP connections
Why this is a problem: When the SSE buffer exceeds the 64 KiB safety limit at line 318, the code throws an error. However, the catch block (line 341) and finally block (line 349) do not call reader.cancel() to release the HTTP response body reader. This leaves the loopback HTTP connection open, causing resource leaks that can exhaust system file descriptors under sustained oversized responses.
Suggested fix:
// In the catch block, ensure reader is cleaned up
} catch (error) {
// Cancel the reader if it exists to prevent resource leak
if (reader) {
reader.cancel().catch(() => {});
}
if (!state.activeAbortController.signal.aborted) {
writeProtocolError(
socket,
"bridge_failed",
error instanceof Error ? error.message : "Responses WebSocket bridge failed"
);
}
} finally {
state.activeTurn = false;
state.activeAbortController = null;
}Alternatively, wrap the entire while loop in a try/finally that guarantees reader cleanup:
try {
while (true) {
// ... existing loop code ...
}
} finally {
if (reader) {
reader.cancel().catch(() => {});
}
}
| websocketUrl: decision.websocketUrl, | ||
| frame: { | ||
| type: "response.create", | ||
| response: params.body as Record<string, unknown> & { model: string }, |
Contributor
There was a problem hiding this comment.
[Medium] [TYPE-ANY-USAGE] Unsafe body cast bypasses frame validation
Why this is a problem: The params.body is directly cast to Record<string, unknown> & { model: string } without running through parseResponsesWsInitialFrame(). In contrast, the forwarder.ts path validates the frame with parseResponsesWsInitialFrame() before sending. This inconsistency can produce cryptic transport errors instead of clear validation messages when the body structure is invalid.
Suggested fix:
import { parseResponsesWsInitialFrame } from "@/app/v1/_lib/proxy/responses-ws-schema";
// ... in executeResponsesWsProviderTest ...
const response = await sendResponsesWsRequest({
websocketUrl: decision.websocketUrl,
frame: parseResponsesWsInitialFrame({
type: "response.create",
response: params.body,
}),
headers: params.headers,
isStreaming: true,
handshakeTimeoutMs: params.timeoutMs,
firstEventTimeoutMs: params.timeoutMs,
onOpen: (latencyMs) => {
handshakeLatencyMs = latencyMs;
},
onEvent: () => {
eventCount += 1;
},
});This ensures consistent validation behavior between the proxy path and provider test path.
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds OpenAI Responses WebSocket transport for /v1/responses, implementing a same-port architecture with delayed bridging ingress. The implementation is well-structured with comprehensive test coverage (4 unit tests, 8 integration tests). The feature is gated behind a default-off toggle, ensuring backward compatibility.
PR Size: XL
- Lines changed: 17,031
- Files changed: 70
Recommendation: Consider splitting future PRs of this size into smaller, focused changes for easier review. Suggested splits:
- Core WebSocket transport layer (adapter, schema, transport selection)
- Custom server with WS upgrade handling
- UI/Settings integration and i18n
- Provider testing extensions
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 1 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- [server.js:318] SSE reader not canceled on buffer overflow - When the 64 KiB buffer limit is exceeded, the reader is not cleaned up, causing HTTP connection leaks. See inline comment for fix.
High Priority Issues (Should Fix)
- [test-service.ts:287] Unsafe body cast bypasses frame validation - Inconsistent with the forwarder.ts path which uses parseResponsesWsInitialFrame(). See inline comment for fix.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (maxPayload limit, URL sanitization, AbortSignal propagation)
- Error handling - Issue found (SSE reader cleanup)
- Type safety - Issue found (unsafe cast)
- Documentation accuracy - Clean (comments are accurate, i18n complete)
- Test coverage - Good (unit + integration tests for WS paths)
- Code clarity - Good (well-structured, follows patterns)
Architecture Notes
The delayed-bridging design is sound:
- WS ingress buffers frames until response.create, then runs deferred guards
- Bounded memory via head/tail ring-buffer collector
- Neutral fallback to HTTP without affecting circuit breaker
- Terminal-event settlement for usage/cost extraction
The WebSocket feature is properly gated and safe to merge once the SSE reader leak is fixed.
Automated review by Claude AI
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
tests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.ts (3)
35-48: 类型断言可接受，但建议添加注释说明。
as unknown as Provider是测试中创建部分 mock 对象的常见模式。建议添加简短注释说明这是测试用的最小化 Provider 对象，便于后续维护。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.ts` around lines 35 - 48, The test helper createProvider() uses the double-cast "as unknown as Provider" to coerce a minimal mock into Provider; add a concise inline comment above or next to this return explaining that this is a minimal test-only Provider shape and the double-cast is intentional for brevity (e.g., "Intentional minimal Provider for tests — using unknown->Provider cast to avoid constructing full object"); keep the createProvider name and returned fields unchanged so tests continue to use the same mock.
133-144: 通过类型断言测试私有方法存在脆弱性风险。
vi.spyOn(ProxyForwarder as any, "fetchWithoutAutoDecode")和doForward的访问方式绕过了 TypeScript 类型检查，直接测试内部实现细节。若ProxyForwarder重构这些方法，测试将静默失效。建议：
- 如果这些行为对外部调用者可见，考虑通过公共 API 测试
- 如必须测试内部行为，在测试文件顶部添加注释说明依赖的内部 API
当前实现可接受，但需注意维护成本。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.ts` around lines 133 - 144, The test bypasses TypeScript visibility by spying on internal members (fetchWithoutAutoDecode) and casting ProxyForwarder to access doForward, which makes the test brittle if internals are refactored; either rewrite the test to exercise the same behavior via ProxyForwarder’s public API (calling exported methods that trigger fetchWithoutAutoDecode/doForward) or, if you must assert internal behavior, add a top-of-file comment documenting the dependency on internal API and keep the spy but make it explicit by referencing the symbols fetchWithoutAutoDecode, doForward, and ProxyForwarder so future maintainers know this test relies on those internals.
50-108: Mock session 构造逻辑完整，但存在一定的维护风险。直接使用
Object.create(ProxySession.prototype)+Object.assign创建 mock session 是可行的，但若ProxySession接口变更，此处可能不会及时暴露兼容性问题。可考虑：
- 使用
vi.mocked()或Partial<ProxySession>类型约束- 抽取为可复用的 test factory/fixture
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.ts` around lines 50 - 108, The current createRawPassthroughSession uses Object.create(ProxySession.prototype) + Object.assign which is brittle if the ProxySession shape changes; replace that pattern by building a strongly-typed Partial<ProxySession> object (e.g. const session: Partial<ProxySession> = { ... }) with the same properties and vi.fn stubs, then return session as ProxySession to get compile-time validation, and/or extract createRawPassthroughSession into a reusable test factory/fixture so other tests can reuse the typed factory; keep the same unique symbols (createRawPassthroughSession, getCurrentModel, addSpecialSetting, isHeaderModified, resolveEndpointPolicy) when moving code so tests still reference the same behavior.tests/unit/lib/rate-limit/lease-service.test.ts (1)
130-130: 把系统设置 mock 收敛到一个工厂。
enableResponsesWebSocket只是这次新增的一个字段，但这里已经要在很多个 fixture 里同步同一份默认值。后面再扩展SystemSettings时，很容易漏改某个用例。建议抽一个buildSystemSettingsMock(overrides)，把默认值集中在一处。Also applies to: 208-208, 273-273, 332-332, 402-402, 462-462, 522-522, 583-583, 646-646, 705-705, 766-766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/rate-limit/lease-service.test.ts` at line 130, Several tests duplicate a SystemSettings literal (now including enableResponsesWebSocket) causing brittle fixtures; create a factory function buildSystemSettingsMock(overrides) that returns the full default SystemSettings object (with enableResponsesWebSocket: false and all current defaults) and accepts partial overrides, then replace the repeated inline SystemSettings mocks in lease-service.test.ts (and other tests) with calls to buildSystemSettingsMock({...}) so defaults are centralized and future fields won't require updating every fixture.src/app/v1/_lib/proxy/responses-ws-transport.ts (1)
5-5: 这里改用@/路径别名。当前文件已经在
src/下，继续使用相对导入会和仓库约定不一致。建议修改
-import { sanitizeUrl } from "./errors"; +import { sanitizeUrl } from "@/app/v1/_lib/proxy/errors";As per coding guidelines, "Use path alias
@/to reference files in./src/directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/responses-ws-transport.ts` at line 5, Replace the relative import of sanitizeUrl in responses-ws-transport.ts with the project path-alias form: instead of importing from "./errors" update the import to use the "@/..." alias that points into src (i.e. reference the same module via "@/app/v1/_lib/proxy/errors"); keep the same imported symbol sanitizeUrl and ensure the updated import matches the repo's tsconfig/jsconfig path alias configuration.tests/integration/responses-ws/http-regression-after-ws.test.ts (1)
31-73: 这个回归用例最好先走一轮真实的 WS 生命周期再验 HTTP。当前只验证“挂上 upgrade 支持后 HTTP 路由还在”，还没覆盖“经历过一次 WebSocket 连接/关闭后 HTTP 仍然正常”。如果回归出在连接状态清理或全局单例污染，这个用例会漏掉。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/responses-ws/http-regression-after-ws.test.ts` around lines 31 - 73, 在现有测试（it "keeps the HTTP /v1/responses route alive after WS support"）中，在对 /v1/responses 发起 HTTP POST 之前模拟一次真实的 WS 生命周期：使用同样的 port（通过 getFreePort()）和 spawned child 进程，建立一个 WebSocket 连接到服务器的实时端点（例如 ws://127.0.0.1:${port}/v1/realtime 或项目实际的 WS 路径），等待连接成功后立即关闭（或发送正常的 close/cleanup），确认连接已关闭，然后再执行现有的 fetch POST 对 /v1/responses 并断言 status; 修改点涉及该测试体内的 child 进程 orchestration、在 fetch 之前插入 WS 客户端连接/关闭逻辑以覆盖连接建立与清理场景。tests/integration/responses-ws/billing-observability-parity.test.ts (1)
21-39: 脱敏测试可考虑增强边界场景覆盖。当前测试验证了
reasoning.summary和function_call.arguments的脱敏。建议后续考虑添加以下场景的测试覆盖（可在后续 PR 中完成）：
- 嵌套更深的敏感数据结构
- 空值或 undefined 字段的处理
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/responses-ws/billing-observability-parity.test.ts` around lines 21 - 39, Add additional test cases to the existing "redacts reasoning and tool payloads in trace/log path" suite that exercise deeper and edge-case inputs: call redactResponseBody with (1) nested-sensitive structures (e.g., reasoning.summary containing nested objects/arrays with secret strings and function_call.arguments as a nested JSON object string) and assert JSON.stringify(redacted) does not contain secrets, and (2) inputs with null, undefined, or missing summary/arguments fields to verify they are handled without errors; reuse the same test semantics and assertions around redactResponseBody to ensure these boundaries are covered.tests/integration/responses-ws/custom-server-bootstrap.test.ts (2)
23-23: 为child变量添加类型注解。当前声明缺少类型信息，建议添加显式类型以提高代码可读性和 TypeScript 类型安全性。
建议的修复
- let child = null; + let child: ReturnType<typeof spawn> | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/responses-ws/custom-server-bootstrap.test.ts` at line 23, The variable "child" is declared without a TypeScript type; update its declaration in the test to include an explicit type (e.g., ChildProcess | null or the appropriate test fixture type) so it is not implicitly any—modify the "let child = null;" declaration to "let child: ChildProcess | null = null;" (or another accurate type used in this test) and import any needed types such as ChildProcess from "child_process" if required.
60-68: stderr 处理可能过于严格。当前实现将任何 stderr 输出都视为错误并拒绝 Promise。这可能导致误报，因为服务器可能会在 stderr 上输出警告信息（如 deprecation warnings）而非真正的错误。
建议考虑更宽松的处理策略，例如仅在检测到特定错误模式时才拒绝，或者允许某些警告类型通过。
建议的修复
const handleError = (chunk) => { const text = chunk.toString(); - if (text.trim()) { + // Only treat fatal errors as failures, allow warnings to pass + if (text.includes("Error:") || text.includes("FATAL") || text.includes("Cannot find module")) { clearTimeout(timeout); child.stdout?.off("data", handleReady); child.stderr?.off("data", handleError); reject(new Error(text)); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/responses-ws/custom-server-bootstrap.test.ts` around lines 60 - 68, The current handleError treats any stderr chunk as a fatal error; update handleError (used with child.stderr) to inspect the text and only reject when it matches real error patterns (e.g., /error|exception|failed|unhandled/i or a configurable list), otherwise log or ignore warnings and do not clear the startup timeout or remove the handleReady/handleError listeners for non-fatal stderr; ensure you still reject and perform clearTimeout/offs when a true error pattern is detected so the Promise behavior remains correct.src/app/[locale]/settings/providers/_components/forms/test-result-card.tsx (1)
260-267:websocketFallbackReason值可能需要 i18n 处理。当前实现直接显示
result.websocketFallbackReason的原始值（如proxy_incompatible、disabled、unsupported_provider_type），而其他字段（如transportKind）都使用了 i18n 翻译。为保持用户体验一致性，建议对 fallback reason 值也进行翻译。建议的修复
{result.websocketFallbackReason && ( <div className="flex items-center gap-1"> <span className="text-muted-foreground"> {t("resultCard.labels.websocketFallbackReason")}: </span> - <span>{result.websocketFallbackReason}</span> + <span>{t(`resultCard.fallbackReason.${result.websocketFallbackReason}`)}</span> </div> )}同时需要在对应的 i18n JSON 文件中添加 fallback reason 的翻译键：
{ "resultCard": { "fallbackReason": { "proxy_incompatible": "代理不兼容", "disabled": "已禁用", "unsupported_provider_type": "不支持的供应商类型", "handshake_timeout": "握手超时" } } }Also applies to: 445-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/test-result-card.tsx around lines 260 - 267, The UI is rendering raw result.websocketFallbackReason values instead of translated text; update the rendering in the test-result-card component to pass the fallback reason through the i18n translator (e.g., use t with a key like "resultCard.fallbackReason.{result.websocketFallbackReason}" or a safe mapping function) instead of showing result.websocketFallbackReason directly, and add the corresponding keys and translations (proxy_incompatible, disabled, unsupported_provider_type, handshake_timeout, etc.) to the resultCard.fallbackReason entries in the locale JSON files so all fallback reasons are localized.src/actions/providers.ts (1)
4249-4252: 把websocketFallbackReason收敛成稳定代码，不要暴露自由字符串。如果前端要展示这个原因，
string只会迫使 UI 直接显示后端文案，或者做脆弱的字符串匹配；两种方式都会让 i18n 和兼容性变差。这里更适合返回有限的 reason code（例如handshake_timeout、upgrade_rejected），再由前端映射到多语言文案。As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 4249 - 4252, The websocketFallbackReason field currently typed as string should be changed to a closed set of reason codes (e.g., a string literal union or enum) such as "handshake_timeout" | "upgrade_rejected" | "client_cancelled" | "unsupported_protocol" to avoid exposing freeform backend strings; update the type declaration for websocketFallbackReason in the transport/options interface and all places that set it (search for websocketFallbackReason assignments) to emit one of these codes, and ensure any code paths that previously wrote human-readable messages now map internal error details to one of the stable codes so the frontend can perform i18n mapping.server.js (2)
8-13: 终端事件类型集合需要与 TypeScript 模块保持同步。
RESPONSES_WS_TERMINAL_TYPES在此处定义，同时在responses-ws-schema.ts中也有RESPONSES_WS_TERMINAL_EVENT_TYPES。注释中提到需要保持同步（见responses-ws-schema.ts第 71 行），但目前没有自动化机制保证这一点。建议在代码注释中添加对应的同步说明，或者考虑在测试中验证两边的一致性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 8 - 13, Add a sync note and an automated check: update the comment above RESPONSES_WS_TERMINAL_TYPES to explicitly state it must match RESPONSES_WS_TERMINAL_EVENT_TYPES (the exported constant in responses-ws-schema.ts) and add a unit test that imports both symbols (RESPONSES_WS_TERMINAL_TYPES from server.js and RESPONSES_WS_TERMINAL_EVENT_TYPES from the compiled/JS export of responses-ws-schema.ts) and asserts their sets/arrays are identical; if your test framework requires, convert both to arrays or sets before comparison and fail the test with a clear message when they diverge.
308-340: SSE 流处理循环逻辑正确，但缺少对reader的显式清理。当循环正常结束（
done或terminalSeen）时，reader没有被显式释放。虽然在大多数情况下 GC 会处理，但在某些边缘情况下（如流未完全消费）可能导致资源挂起。建议添加 reader 清理
while (true) { const { done, value } = await reader.read(); if (done) { break; } // ... existing logic ... if (terminalSeen) { break; } } + + reader.releaseLock(); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 308 - 340, Wrap the SSE read loop that uses reader in a try/finally so the reader is explicitly cleaned up when the loop exits; after the existing while loop (or in the finally block) call await reader.cancel() (guarded with if (reader) and try/catch to swallow non-fatal errors) to ensure the stream is closed and resources freed — update the block around the reader usage in server.js (the code using reader, extractSseBlocks, parseSseBlock, sseEventToWsFrame, writeJsonFrame, and RESPONSES_WS_TERMINAL_TYPES) to perform this explicit cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/copy-version-to-standalone.cjs`:
- Around line 16-28: The function extractStandaloneNextConfig currently relies
on a fragile regex that targets Next.js's private symbol
__NEXT_PRIVATE_STANDALONE_CONFIG and calls JSON.parse(match[1]) without error
handling; wrap the JSON.parse in a try-catch inside extractStandaloneNextConfig
to catch syntax errors and rethrow a clearer error that includes the extracted
snippet and a hint about Next.js internal format changes, and consider making
the regex extraction more defensive (e.g., trim the captured group and validate
it looks like JSON before parsing) so failures surface with actionable messages
instead of crashing.
In `@src/app/`[locale]/settings/config/_components/system-settings-form.tsx:
- Around line 393-412: The Switch with id "enable-responses-websocket" lacks an
accessible label; update the JSX around the Switch (the block rendering
enableResponsesWebSocket, t("enableResponsesWebSocket"),
t("enableResponsesWebSocketDesc")) to programmatically associate the visible
text with the control — e.g., add a <label htmlFor="enable-responses-websocket">
(or add aria-labelledby referencing the title/description IDs and
aria-describedby for the description) so the Switch (id
"enable-responses-websocket") is announced properly; keep the existing
checked={enableResponsesWebSocket},
onCheckedChange={setEnableResponsesWebSocket} and disabled={isPending} props
unchanged.
In `@src/app/v1/_lib/proxy/responses-ws-transport.ts`:
- Around line 55-74: The current toResponsesWebSocketUrl implementation uses
pathname.includes("/responses") which incorrectly matches substrings like
"/responses-debug"; change to parse URL.pathname, split by "/", ignore empty
segments, and require the last segment === "responses". Also separate protocol
validation from path validation: in toResponsesWebSocketUrl first
check/normalize supported protocols (http->ws, https->wss, allow ws/wss,
otherwise return null) and then validate the final path segment; apply the same
exact checks in the code that sets websocketFallbackReason so it distinguishes
unsupported_protocol vs unsupported_path (e.g., use symbols to indicate
'unsupported_protocol' when protocol unsupported and 'unsupported_endpoint' or
'unsupported_path' when last path segment !== "responses").
In `@src/components/ui/chart.tsx`:
- Around line 173-181: The current map uses dataKeyStr as the React list key
which can be non-unique; update the key generation inside the map in
src/components/ui/chart.tsx (the .map((item, index) => { ... }) block) to
guarantee uniqueness by combining dataKeyStr with the index or the computed key
variable (e.g., use `${dataKeyStr}-${index}` or `${key}-${index}`), so replace
the existing key={dataKeyStr} with a deterministic unique key that uses index
and/or the computed key/nameKey to avoid duplicate keys.
In `@tests/integration/responses-ws/ingress-delayed-bridging.test.ts`:
- Line 2: The test currently uses getFreePort() to reserve a port then closes
the temp server and calls realServer.listen(port), creating a TOCTOU race;
instead, have the real http.Server call listen(0) directly and then read the
assigned port from server.address(). Specifically, remove the temp
port-reservation flow (getFreePort and its temporary net.createServer usage) and
change the real server listen call to server.listen(0) (where the code
referenced as the real server and the Promise around listen is created), then
after listening resolve the Promise using (server.address() as
AddressInfo).port; also ensure the Promise registers an 'error' handler on the
real server to reject on listen errors to avoid hanging.
In `@tests/integration/responses-ws/outbound-adapter.test.ts`:
- Around line 121-136: The test currently only asserts the flex-tier
firstEventTimeoutMs is greater than 5000ms; update it to assert the resolved
timeout matches the minimum flex floor by checking that
timeoutProfile.firstEventTimeoutMs is at least FLEX_FIRST_EVENT_TIMEOUT_MS (or
equals 60000) to verify the Math.max logic in resolveResponsesWsTimeoutProfile;
reference the resolveResponsesWsTimeoutProfile function and the
FLEX_FIRST_EVENT_TIMEOUT_MS constant when adding the explicit assertion.
In `@tests/integration/responses-ws/session-continuity.test.ts`:
- Around line 240-276: The test creates a WebSocketServer named wss that may not
be closed if the test fails; add centralized cleanup (e.g., an afterEach)
similar to the other describe block to always close/await any created servers.
Concretely, track created servers (or the wss variable) in the surrounding
describe scope and in afterEach call wss.close() (await the close callback or
wrap in a Promise) to ensure the socket is closed and the port freed even when
the test throws; update the session-continuity.test.ts tests that create
WebSocketServer instances to use that shared cleanup.
---
Nitpick comments:
In `@server.js`:
- Around line 8-13: Add a sync note and an automated check: update the comment
above RESPONSES_WS_TERMINAL_TYPES to explicitly state it must match
RESPONSES_WS_TERMINAL_EVENT_TYPES (the exported constant in
responses-ws-schema.ts) and add a unit test that imports both symbols
(RESPONSES_WS_TERMINAL_TYPES from server.js and
RESPONSES_WS_TERMINAL_EVENT_TYPES from the compiled/JS export of
responses-ws-schema.ts) and asserts their sets/arrays are identical; if your
test framework requires, convert both to arrays or sets before comparison and
fail the test with a clear message when they diverge.
- Around line 308-340: Wrap the SSE read loop that uses reader in a try/finally
so the reader is explicitly cleaned up when the loop exits; after the existing
while loop (or in the finally block) call await reader.cancel() (guarded with if
(reader) and try/catch to swallow non-fatal errors) to ensure the stream is
closed and resources freed — update the block around the reader usage in
server.js (the code using reader, extractSseBlocks, parseSseBlock,
sseEventToWsFrame, writeJsonFrame, and RESPONSES_WS_TERMINAL_TYPES) to perform
this explicit cleanup.
In `@src/actions/providers.ts`:
- Around line 4249-4252: The websocketFallbackReason field currently typed as
string should be changed to a closed set of reason codes (e.g., a string literal
union or enum) such as "handshake_timeout" | "upgrade_rejected" |
"client_cancelled" | "unsupported_protocol" to avoid exposing freeform backend
strings; update the type declaration for websocketFallbackReason in the
transport/options interface and all places that set it (search for
websocketFallbackReason assignments) to emit one of these codes, and ensure any
code paths that previously wrote human-readable messages now map internal error
details to one of the stable codes so the frontend can perform i18n mapping.
In `@src/app/`[locale]/settings/providers/_components/forms/test-result-card.tsx:
- Around line 260-267: The UI is rendering raw result.websocketFallbackReason
values instead of translated text; update the rendering in the test-result-card
component to pass the fallback reason through the i18n translator (e.g., use t
with a key like "resultCard.fallbackReason.{result.websocketFallbackReason}" or
a safe mapping function) instead of showing result.websocketFallbackReason
directly, and add the corresponding keys and translations (proxy_incompatible,
disabled, unsupported_provider_type, handshake_timeout, etc.) to the
resultCard.fallbackReason entries in the locale JSON files so all fallback
reasons are localized.
In `@src/app/v1/_lib/proxy/responses-ws-transport.ts`:
- Line 5: Replace the relative import of sanitizeUrl in
responses-ws-transport.ts with the project path-alias form: instead of importing
from "./errors" update the import to use the "@/..." alias that points into src
(i.e. reference the same module via "@/app/v1/_lib/proxy/errors"); keep the same
imported symbol sanitizeUrl and ensure the updated import matches the repo's
tsconfig/jsconfig path alias configuration.
In `@tests/integration/responses-ws/billing-observability-parity.test.ts`:
- Around line 21-39: Add additional test cases to the existing "redacts
reasoning and tool payloads in trace/log path" suite that exercise deeper and
edge-case inputs: call redactResponseBody with (1) nested-sensitive structures
(e.g., reasoning.summary containing nested objects/arrays with secret strings
and function_call.arguments as a nested JSON object string) and assert
JSON.stringify(redacted) does not contain secrets, and (2) inputs with null,
undefined, or missing summary/arguments fields to verify they are handled
without errors; reuse the same test semantics and assertions around
redactResponseBody to ensure these boundaries are covered.
In `@tests/integration/responses-ws/custom-server-bootstrap.test.ts`:
- Line 23: The variable "child" is declared without a TypeScript type; update
its declaration in the test to include an explicit type (e.g., ChildProcess |
null or the appropriate test fixture type) so it is not implicitly any—modify
the "let child = null;" declaration to "let child: ChildProcess | null = null;"
(or another accurate type used in this test) and import any needed types such as
ChildProcess from "child_process" if required.
- Around line 60-68: The current handleError treats any stderr chunk as a fatal
error; update handleError (used with child.stderr) to inspect the text and only
reject when it matches real error patterns (e.g.,
/error|exception|failed|unhandled/i or a configurable list), otherwise log or
ignore warnings and do not clear the startup timeout or remove the
handleReady/handleError listeners for non-fatal stderr; ensure you still reject
and perform clearTimeout/offs when a true error pattern is detected so the
Promise behavior remains correct.
In `@tests/integration/responses-ws/http-regression-after-ws.test.ts`:
- Around line 31-73: 在现有测试（it "keeps the HTTP /v1/responses route alive after WS
support"）中，在对 /v1/responses 发起 HTTP POST 之前模拟一次真实的 WS 生命周期：使用同样的 port（通过
getFreePort()）和 spawned child 进程，建立一个 WebSocket 连接到服务器的实时端点（例如
ws://127.0.0.1:${port}/v1/realtime 或项目实际的 WS 路径），等待连接成功后立即关闭（或发送正常的
close/cleanup），确认连接已关闭，然后再执行现有的 fetch POST 对 /v1/responses 并断言 status;
修改点涉及该测试体内的 child 进程 orchestration、在 fetch 之前插入 WS 客户端连接/关闭逻辑以覆盖连接建立与清理场景。
In `@tests/unit/lib/rate-limit/lease-service.test.ts`:
- Line 130: Several tests duplicate a SystemSettings literal (now including
enableResponsesWebSocket) causing brittle fixtures; create a factory function
buildSystemSettingsMock(overrides) that returns the full default SystemSettings
object (with enableResponsesWebSocket: false and all current defaults) and
accepts partial overrides, then replace the repeated inline SystemSettings mocks
in lease-service.test.ts (and other tests) with calls to
buildSystemSettingsMock({...}) so defaults are centralized and future fields
won't require updating every fixture.
In `@tests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.ts`:
- Around line 35-48: The test helper createProvider() uses the double-cast "as
unknown as Provider" to coerce a minimal mock into Provider; add a concise
inline comment above or next to this return explaining that this is a minimal
test-only Provider shape and the double-cast is intentional for brevity (e.g.,
"Intentional minimal Provider for tests — using unknown->Provider cast to avoid
constructing full object"); keep the createProvider name and returned fields
unchanged so tests continue to use the same mock.
- Around line 133-144: The test bypasses TypeScript visibility by spying on
internal members (fetchWithoutAutoDecode) and casting ProxyForwarder to access
doForward, which makes the test brittle if internals are refactored; either
rewrite the test to exercise the same behavior via ProxyForwarder’s public API
(calling exported methods that trigger fetchWithoutAutoDecode/doForward) or, if
you must assert internal behavior, add a top-of-file comment documenting the
dependency on internal API and keep the spy but make it explicit by referencing
the symbols fetchWithoutAutoDecode, doForward, and ProxyForwarder so future
maintainers know this test relies on those internals.
- Around line 50-108: The current createRawPassthroughSession uses
Object.create(ProxySession.prototype) + Object.assign which is brittle if the
ProxySession shape changes; replace that pattern by building a strongly-typed
Partial<ProxySession> object (e.g. const session: Partial<ProxySession> = { ...
}) with the same properties and vi.fn stubs, then return session as ProxySession
to get compile-time validation, and/or extract createRawPassthroughSession into
a reusable test factory/fixture so other tests can reuse the typed factory; keep
the same unique symbols (createRawPassthroughSession, getCurrentModel,
addSpecialSetting, isHeaderModified, resolveEndpointPolicy) when moving code so
tests still reference the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e9aaab2-eb1a-4508-b387-d66ef2dd6afd
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (69)
Dockerfiledrizzle/0079_chilly_mentallo.sqldrizzle/meta/0079_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/config.jsonmessages/en/settings/providers/form/apiTest.jsonmessages/ja/settings/config.jsonmessages/ja/settings/prices.jsonmessages/ja/settings/providers/form/apiTest.jsonmessages/ru/settings/config.jsonmessages/ru/settings/prices.jsonmessages/ru/settings/providers/form/apiTest.jsonmessages/zh-CN/settings/config.jsonmessages/zh-CN/settings/providers/form/apiTest.jsonmessages/zh-TW/settings/config.jsonmessages/zh-TW/settings/prices.jsonmessages/zh-TW/settings/providers/form/apiTest.jsonnext.config.tspackage.jsonscripts/copy-version-to-standalone.cjsserver.jssrc/actions/providers.tssrc/actions/system-config.tssrc/app/[locale]/dashboard/availability/_components/provider/latency-chart.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/[locale]/settings/providers/_components/forms/test-result-card.tsxsrc/app/v1/_lib/codex/types/response.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/responses-ws-adapter.tssrc/app/v1/_lib/proxy/responses-ws-schema.tssrc/app/v1/_lib/proxy/responses-ws-terminal-finalization.tssrc/app/v1/_lib/proxy/responses-ws-transport.tssrc/app/v1/_lib/proxy/session.tssrc/components/ui/chart.tsxsrc/drizzle/schema.tssrc/lib/config/index.tssrc/lib/config/system-settings-cache.tssrc/lib/provider-testing/test-service.tssrc/lib/provider-testing/types.tssrc/lib/utils/special-settings.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/system-config.tssrc/types/message.tssrc/types/special-settings.tssrc/types/system-config.tstests/integration/billing-model-source.test.tstests/integration/responses-ws/billing-observability-parity.test.tstests/integration/responses-ws/custom-server-bootstrap.test.tstests/integration/responses-ws/http-regression-after-ws.test.tstests/integration/responses-ws/ingress-delayed-bridging.test.tstests/integration/responses-ws/outbound-adapter.test.tstests/integration/responses-ws/provider-testing-backend.test.tstests/integration/responses-ws/session-continuity.test.tstests/unit/actions/system-config-save.test.tstests/unit/lib/config/system-settings-cache.test.tstests/unit/lib/rate-limit/lease-service.test.tstests/unit/lib/timezone/timezone-resolver.test.tstests/unit/provider-test-ws-ui.test.tsxtests/unit/proxy/pricing-no-price.test.tstests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.tstests/unit/proxy/session.test.tstests/unit/repository/system-config-update-missing-columns.test.tstests/unit/responses-ws-frame-schema.test.tstests/unit/responses-ws-terminal-finalization.test.tstests/unit/responses-ws-transport-selector.test.tstests/unit/system-settings-responses-websocket-toggle.test.tstsconfig.json
Comment on lines
+16
to
+28
| function extractStandaloneNextConfig(serverJsPath) { | ||
| if (!fs.existsSync(serverJsPath)) { | ||
| throw new Error(`[copy-standalone] Generated server not found: ${serverJsPath}`); | ||
| } | ||
|
|
||
| const content = fs.readFileSync(serverJsPath, "utf8"); | ||
| const match = content.match(/const nextConfig = (.+?)\n\nprocess\.env\.__NEXT_PRIVATE_STANDALONE_CONFIG/s); | ||
| if (!match) { | ||
| throw new Error("[copy-standalone] Failed to extract standalone nextConfig"); | ||
| } | ||
|
|
||
| return JSON.parse(match[1]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js 15 standalone output format server.js structure
💡 Result:
With output: 'standalone', next build generates a deployable folder at .next/standalone containing:
- a minimal
server.jsentrypoint (use this instead ofnext start) - a trimmed
node_modules/(only traced runtime deps) - your app’s required server build files under
.next/(e.g.,.next/server, manifests, etc.)
Typical shape (approx.):
.next/
standalone/
server.js
package.json
node_modules/ (subset copied by output file tracing)
.next/
server/ (app/server build output + manifests)
...
Important details:
public/and.next/static/are NOT copied into.next/standaloneby default. If you want the standalone server to serve them (instead of a CDN), copy them manually into:.next/standalone/public.next/standalone/.next/static
(Next.js docs even show thecp -r public ... && cp -r .next/static ...pattern.) [1]
- Run it with:
node .next/standalone/server.js[1]- and you can set
PORT/HOSTNAMEvia env vars (documented in the same page). [1]
- The generated
server.jshistorically parsed the port like:
const currentPort = parseInt(process.env.PORT, 10) || 3000(relevant if your platform provides non-numeric ports / named pipes). [2]
Monorepo note (why “structure” sometimes looks nested/weird):
- In Turborepo/monorepos you often must set
outputFileTracingRootto the repo root so Next can trace/copy workspace deps correctly, and you should run the generated.next/standalone/server.js(don’t replace it with a custom server thatrequire('next')). [3]
Sources: Next.js output docs (last updated Feb 27, 2026) [1], Vercel/Next.js discussion about the generated server.js port parsing [2], Vercel/Next.js discussion on standalone + monorepos (outputFileTracingRoot, using generated server) [3].
🏁 Script executed:
# First, let's examine the script file
cat -n scripts/copy-version-to-standalone.cjsRepository: ding113/claude-code-hub
Length of output: 2560
🏁 Script executed:
# Check the package.json for Next.js version
cat -n package.json | grep -A 5 -B 5 '"next"'Repository: ding113/claude-code-hub
Length of output: 392
🏁 Script executed:
# Search for where extractStandaloneNextConfig is used
rg "extractStandaloneNextConfig" --context=5Repository: ding113/claude-code-hub
Length of output: 1740
JSON.parse 缺少错误处理，regex 依赖 Next.js 内部实现。
extractStandaloneNextConfig 通过特定格式的正则表达式提取配置，且依赖于 Next.js 的内部 API __NEXT_PRIVATE_STANDALONE_CONFIG。这个私有 API 在 Next.js 版本更新时可能改变。同时，JSON.parse(match[1]) 缺少错误处理，如果提取的内容非有效 JSON 将导致脚本崩溃。
建议为 JSON.parse 添加 try-catch 捕获，提供更清晰的错误信息：
建议改进
function extractStandaloneNextConfig(serverJsPath) {
if (!fs.existsSync(serverJsPath)) {
throw new Error(`[copy-standalone] Generated server not found: ${serverJsPath}`);
}
const content = fs.readFileSync(serverJsPath, "utf8");
const match = content.match(/const nextConfig = (.+?)\n\nprocess\.env\.__NEXT_PRIVATE_STANDALONE_CONFIG/s);
if (!match) {
throw new Error("[copy-standalone] Failed to extract standalone nextConfig");
}
- return JSON.parse(match[1]);
+ try {
+ return JSON.parse(match[1]);
+ } catch (e) {
+ throw new Error(`[copy-standalone] Failed to parse nextConfig JSON: ${e.message}`);
+ }
}📝 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.
Suggested change
| function extractStandaloneNextConfig(serverJsPath) { | |
| if (!fs.existsSync(serverJsPath)) { | |
| throw new Error(`[copy-standalone] Generated server not found: ${serverJsPath}`); | |
| } | |
| const content = fs.readFileSync(serverJsPath, "utf8"); | |
| const match = content.match(/const nextConfig = (.+?)\n\nprocess\.env\.__NEXT_PRIVATE_STANDALONE_CONFIG/s); | |
| if (!match) { | |
| throw new Error("[copy-standalone] Failed to extract standalone nextConfig"); | |
| } | |
| return JSON.parse(match[1]); | |
| } | |
| function extractStandaloneNextConfig(serverJsPath) { | |
| if (!fs.existsSync(serverJsPath)) { | |
| throw new Error(`[copy-standalone] Generated server not found: ${serverJsPath}`); | |
| } | |
| const content = fs.readFileSync(serverJsPath, "utf8"); | |
| const match = content.match(/const nextConfig = (.+?)\n\nprocess\.env\.__NEXT_PRIVATE_STANDALONE_CONFIG/s); | |
| if (!match) { | |
| throw new Error("[copy-standalone] Failed to extract standalone nextConfig"); | |
| } | |
| try { | |
| return JSON.parse(match[1]); | |
| } catch (e) { | |
| throw new Error(`[copy-standalone] Failed to parse nextConfig JSON: ${e.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/copy-version-to-standalone.cjs` around lines 16 - 28, The function
extractStandaloneNextConfig currently relies on a fragile regex that targets
Next.js's private symbol __NEXT_PRIVATE_STANDALONE_CONFIG and calls
JSON.parse(match[1]) without error handling; wrap the JSON.parse in a try-catch
inside extractStandaloneNextConfig to catch syntax errors and rethrow a clearer
error that includes the extracted snippet and a hint about Next.js internal
format changes, and consider making the regex extraction more defensive (e.g.,
trim the captured group and validate it looks like JSON before parsing) so
failures surface with actionable messages instead of crashing.
Comment on lines
+393
to
+412
| {/* Enable Responses WebSocket */} | ||
| <div className="p-4 rounded-xl bg-white/[0.02] border border-white/5 flex items-center justify-between hover:bg-white/[0.04] transition-colors"> | ||
| <div className="flex items-start gap-3"> | ||
| <div className="w-8 h-8 flex items-center justify-center rounded-lg bg-cyan-500/10 text-cyan-400 shrink-0"> | ||
| <Network className="h-4 w-4" /> | ||
| </div> | ||
| <div> | ||
| <p className="text-sm font-medium text-foreground">{t("enableResponsesWebSocket")}</p> | ||
| <p className="text-xs text-muted-foreground mt-0.5"> | ||
| {t("enableResponsesWebSocketDesc")} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| <Switch | ||
| id="enable-responses-websocket" | ||
| checked={enableResponsesWebSocket} | ||
| onCheckedChange={(checked) => setEnableResponsesWebSocket(checked)} | ||
| disabled={isPending} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
为新开关补上可访问名称。
这里的可见文案没有和 Switch 建立程序化关联，读屏时大概率只会读出“switch”而不知道它控制什么。请给这个开关补 Label htmlFor，或至少加上 aria-labelledby / aria-describedby。这会直接影响辅助技术用户完成配置。
可参考的修复方式
<div className="p-4 rounded-xl bg-white/[0.02] border border-white/5 flex items-center justify-between hover:bg-white/[0.04] transition-colors">
<div className="flex items-start gap-3">
<div className="w-8 h-8 flex items-center justify-center rounded-lg bg-cyan-500/10 text-cyan-400 shrink-0">
<Network className="h-4 w-4" />
</div>
<div>
- <p className="text-sm font-medium text-foreground">{t("enableResponsesWebSocket")}</p>
- <p className="text-xs text-muted-foreground mt-0.5">
+ <Label
+ htmlFor="enable-responses-websocket"
+ className="text-sm font-medium text-foreground"
+ >
+ {t("enableResponsesWebSocket")}
+ </Label>
+ <p
+ id="enable-responses-websocket-description"
+ className="text-xs text-muted-foreground mt-0.5"
+ >
{t("enableResponsesWebSocketDesc")}
</p>
</div>
</div>
<Switch
id="enable-responses-websocket"
+ aria-describedby="enable-responses-websocket-description"
checked={enableResponsesWebSocket}
onCheckedChange={(checked) => setEnableResponsesWebSocket(checked)}
disabled={isPending}
/>
</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/settings/config/_components/system-settings-form.tsx around
lines 393 - 412, The Switch with id "enable-responses-websocket" lacks an
accessible label; update the JSX around the Switch (the block rendering
enableResponsesWebSocket, t("enableResponsesWebSocket"),
t("enableResponsesWebSocketDesc")) to programmatically associate the visible
text with the control — e.g., add a <label htmlFor="enable-responses-websocket">
(or add aria-labelledby referencing the title/description IDs and
aria-describedby for the description) so the Switch (id
"enable-responses-websocket") is announced properly; keep the existing
checked={enableResponsesWebSocket},
onCheckedChange={setEnableResponsesWebSocket} and disabled={isPending} props
unchanged.
Comment on lines
+55
to
+74
| export function toResponsesWebSocketUrl(upstreamUrl: string): string | null { | ||
| try { | ||
| const url = new URL(upstreamUrl); | ||
| if (url.protocol === "http:") { | ||
| url.protocol = "ws:"; | ||
| } else if (url.protocol === "https:") { | ||
| url.protocol = "wss:"; | ||
| } else if (url.protocol !== "ws:" && url.protocol !== "wss:") { | ||
| return null; | ||
| } | ||
|
|
||
| if (!url.pathname.includes("/responses")) { | ||
| return null; | ||
| } | ||
|
|
||
| return url.toString(); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
端点校验过宽，fallback reason 也会被误分。
Line 66 的 includes("/responses") 会把 /v1/responses-debug、/foo/responses-extra 这类路径也当成可用 WS 端点，导致错误地优先走 WebSocket。反过来，Line 133 这里只按 http(s) 前缀推断原因，所以像 wss://host/v1/chat/completions 这样的 URL 会被标成 unsupported_protocol，实际问题是端点不支持。这样 transport 决策和 websocketFallbackReason 观测都会被带偏，建议先解析一次 URL，再分别校验“协议是否受支持”和“最后一个 path segment 是否为 responses”。
Also applies to: 131-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/responses-ws-transport.ts` around lines 55 - 74, The
current toResponsesWebSocketUrl implementation uses
pathname.includes("/responses") which incorrectly matches substrings like
"/responses-debug"; change to parse URL.pathname, split by "/", ignore empty
segments, and require the last segment === "responses". Also separate protocol
validation from path validation: in toResponsesWebSocketUrl first
check/normalize supported protocols (http->ws, https->wss, allow ws/wss,
otherwise return null) and then validate the final path segment; apply the same
exact checks in the code that sets websocketFallbackReason so it distinguishes
unsupported_protocol vs unsupported_path (e.g., use symbols to indicate
'unsupported_protocol' when protocol unsupported and 'unsupported_endpoint' or
'unsupported_path' when last path segment !== "responses").
src/components/ui/chart.tsx
Outdated
Comment on lines
+173
to
+181
| .map((item, index: number) => { | ||
| const dataKeyStr = String(item.dataKey); | ||
| const key = `${nameKey || item.name || dataKeyStr || "value"}`; | ||
| const itemConfig = getPayloadConfigFromPayload(config, item, key); | ||
| const indicatorColor = color || item.payload?.fill || item.color; | ||
|
|
||
| return ( | ||
| <div | ||
| key={item.dataKey} | ||
| key={dataKeyStr} |
There was a problem hiding this comment.
React key 可能不唯一。
使用 dataKeyStr 作为 React 列表的 key 可能导致重复 key 问题，因为多个 payload 项可能具有相同的 dataKey。建议结合 index 确保唯一性。
建议的修复方案
.map((item, index: number) => {
const dataKeyStr = String(item.dataKey);
const key = `${nameKey || item.name || dataKeyStr || "value"}`;
const itemConfig = getPayloadConfigFromPayload(config, item, key);
const indicatorColor = color || item.payload?.fill || item.color;
return (
<div
- key={dataKeyStr}
+ key={`${dataKeyStr}-${index}`}
className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/chart.tsx` around lines 173 - 181, The current map uses
dataKeyStr as the React list key which can be non-unique; update the key
generation inside the map in src/components/ui/chart.tsx (the .map((item, index)
=> { ... }) block) to guarantee uniqueness by combining dataKeyStr with the
index or the computed key variable (e.g., use `${dataKeyStr}-${index}` or
`${key}-${index}`), so replace the existing key={dataKeyStr} with a
deterministic unique key that uses index and/or the computed key/nameKey to
avoid duplicate keys.
| @@ -0,0 +1,255 @@ | |||
| import http from "node:http"; | |||
| import net from "node:net"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'getFreePort|server\.listen\(port, "127\.0\.0\.1", \(error\)' tests/integration/responses-ws/ingress-delayed-bridging.test.tsRepository: ding113/claude-code-hub
Length of output: 435
🌐 Web query:
In Node.js, does http.Server.listen(0, host, callback) allocate an ephemeral port, and are bind failures emitted via the "error" event rather than passed to the callback?
💡 Result:
Yes.
- Ephemeral port allocation: Passing
0as the port (e.g.,httpServer.listen(0, host, cb)) tells Node to let the operating system choose an arbitrary unused port, which you can read back viaserver.address().portafter the server is listening. [1] - Bind/listen failures: The
callbackyou pass tolisten(...)is not an error-first callback; it’s attached to the server’s'listening'event (i.e., it runs only on successful bind). Bind failures likeEADDRINUSE, permission errors, etc., are delivered by emitting the server’s'error'event instead. [1][2]
Sources:
[1] Node.js net.Server.listen([port[, host[, backlog]]][, callback]) docs (port 0, callback is 'listening', errors via 'error', EADDRINUSE example). (nodejs.org)
[2] Node.js http server listen(...) docs (callback added as listener for 'listening'). (nodejs.org)
Citations:
- 1: https://nodejs.org/api/net.html
- 2: https://nodejs.org/download/release/v0.11.7/docs/api/http.html?utm_source=openai
🏁 Script executed:
cat -n tests/integration/responses-ws/ingress-delayed-bridging.test.ts | head -100Repository: ding113/claude-code-hub
Length of output: 3543
🏁 Script executed:
sed -n '68,100p' tests/integration/responses-ws/ingress-delayed-bridging.test.tsRepository: ding113/claude-code-hub
Length of output: 904
应直接在真实 server 上监听 0 端口，而不是先占端口再重开。
当前实现在 getFreePort() 关闭临时服务器后、真实服务器在第 71 行调用 listen(port) 之前存在 TOCTOU 竞争窗口。若该端口被其他进程占用，真实服务器会通过 'error' 事件而非回调参数接收到 EADDRINUSE 错误。由于第 71 行的 Promise 中没有为真实 server 注册错误处理器，绑定失败时 Promise 会无限挂起而非快速失败。直接对真实 http.Server 调用 listen(0)，再从 server.address() 读取系统分配的端口，可消除此竞争窗口。
建议修改
-import net from "node:net";
...
-async function getFreePort() {
- return new Promise((resolve, reject) => {
- const server = net.createServer();
- server.listen(0, "127.0.0.1", () => {
- const address = server.address();
- if (!address || typeof address === "string") {
- reject(new Error("Failed to allocate free port"));
- return;
- }
- const { port } = address;
- server.close((error) => (error ? reject(error) : resolve(port)));
- });
- server.on("error", reject);
- });
-}
...
- const port = await getFreePort();
- await new Promise((resolve, reject) => {
- server.listen(port, "127.0.0.1", (error) => (error ? reject(error) : resolve(undefined)));
- });
+ await new Promise<void>((resolve, reject) => {
+ server.once("error", reject);
+ server.listen(0, "127.0.0.1", () => resolve());
+ });
+ const address = server.address();
+ if (!address || typeof address === "string") {
+ throw new Error("Failed to allocate free port");
+ }
+ const { port } = address;Also applies to: 6-19, 69-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/responses-ws/ingress-delayed-bridging.test.ts` at line 2,
The test currently uses getFreePort() to reserve a port then closes the temp
server and calls realServer.listen(port), creating a TOCTOU race; instead, have
the real http.Server call listen(0) directly and then read the assigned port
from server.address(). Specifically, remove the temp port-reservation flow
(getFreePort and its temporary net.createServer usage) and change the real
server listen call to server.listen(0) (where the code referenced as the real
server and the Promise around listen is created), then after listening resolve
the Promise using (server.address() as AddressInfo).port; also ensure the
Promise registers an 'error' handler on the real server to reject on listen
errors to avoid hanging.
Comment on lines
+121
to
+136
| it("extends idle window for flex service tier", () => { | ||
| const timeoutProfile = resolveResponsesWsTimeoutProfile({ | ||
| frame: { | ||
| type: "response.create", | ||
| response: { | ||
| model: "gpt-5-codex", | ||
| service_tier: "flex", | ||
| }, | ||
| }, | ||
| handshakeTimeoutMs: 1000, | ||
| firstEventTimeoutMs: 5000, | ||
| }); | ||
|
|
||
| expect(timeoutProfile.handshakeTimeoutMs).toBe(1000); | ||
| expect(timeoutProfile.firstEventTimeoutMs).toBeGreaterThan(5000); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 resolveResponsesWsTimeoutProfile 的实现，确认 flex tier 的扩展逻辑
ast-grep --pattern $'function resolveResponsesWsTimeoutProfile($_) {
$$$
}'
# 备选：搜索函数定义
rg -n -A 20 'resolveResponsesWsTimeoutProfile' --type ts -g '!*.test.ts'Repository: ding113/claude-code-hub
Length of output: 3648
🏁 Script executed:
rg -n 'FLEX_FIRST_EVENT_TIMEOUT_MS' --type tsRepository: ding113/claude-code-hub
Length of output: 284
验证扩展超时值的准确性。
flex 服务层的超时扩展采用最小值强制机制（Math.max(requestedFirstEventTimeoutMs, FLEX_FIRST_EVENT_TIMEOUT_MS)，其中 FLEX_FIRST_EVENT_TIMEOUT_MS = 60_000），而非扩展系数。测试断言只验证了超时值大于输入的 5000ms，建议补充对具体的 60_000ms 超时值的验证，以确保 flex 层级的扩展逻辑符合预期。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/responses-ws/outbound-adapter.test.ts` around lines 121 -
136, The test currently only asserts the flex-tier firstEventTimeoutMs is
greater than 5000ms; update it to assert the resolved timeout matches the
minimum flex floor by checking that timeoutProfile.firstEventTimeoutMs is at
least FLEX_FIRST_EVENT_TIMEOUT_MS (or equals 60000) to verify the Math.max logic
in resolveResponsesWsTimeoutProfile; reference the
resolveResponsesWsTimeoutProfile function and the FLEX_FIRST_EVENT_TIMEOUT_MS
constant when adding the explicit assertion.
Comment on lines
+240
to
+276
| it("fails mid-stream disconnect without implicit replay", async () => { | ||
| const port = await getFreePort(); | ||
| const wss = new WebSocketServer({ port, host: "127.0.0.1" }); | ||
| wss.on("connection", (socket) => { | ||
| socket.once("message", () => { | ||
| socket.send( | ||
| JSON.stringify({ | ||
| type: "response.created", | ||
| response: { | ||
| id: "resp_1", | ||
| object: "response", | ||
| created: 1, | ||
| model: "gpt-5-codex", | ||
| status: "in_progress", | ||
| }, | ||
| }) | ||
| ); | ||
| socket.close(); | ||
| }); | ||
| }); | ||
|
|
||
| const response = await sendResponsesWsRequest({ | ||
| websocketUrl: `ws://127.0.0.1:${port}/v1/responses`, | ||
| frame: { | ||
| type: "response.create", | ||
| response: { | ||
| model: "gpt-5-codex", | ||
| }, | ||
| }, | ||
| isStreaming: true, | ||
| handshakeTimeoutMs: 1000, | ||
| firstEventTimeoutMs: 1000, | ||
| }); | ||
|
|
||
| await expect(response.text()).rejects.toThrow("closed before terminal event"); | ||
| await new Promise((resolve) => wss.close(() => resolve(undefined))); | ||
| }); |
There was a problem hiding this comment.
WebSocketServer 资源清理存在风险。
如果测试在执行到第 275 行之前失败（例如断言失败），wss 将不会被关闭，可能导致端口泄漏或影响后续测试。建议参照第二个 describe 块的模式，使用 afterEach 进行统一清理。
建议的修复方案
describe("responses websocket session continuity", () => {
+ let wss: WebSocketServer | null = null;
+
+ afterEach(async () => {
+ if (wss) {
+ await new Promise((resolve) => wss?.close(() => resolve(undefined)));
+ wss = null;
+ }
+ });
+
it("rebinds session from prompt_cache_key terminal event", async () => {
// ... 保持不变
});
it("fails mid-stream disconnect without implicit replay", async () => {
const port = await getFreePort();
- const wss = new WebSocketServer({ port, host: "127.0.0.1" });
+ wss = new WebSocketServer({ port, host: "127.0.0.1" });
wss.on("connection", (socket) => {
// ...
});
// ... 测试逻辑 ...
await expect(response.text()).rejects.toThrow("closed before terminal event");
- await new Promise((resolve) => wss.close(() => resolve(undefined)));
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/responses-ws/session-continuity.test.ts` around lines 240 -
276, The test creates a WebSocketServer named wss that may not be closed if the
test fails; add centralized cleanup (e.g., an afterEach) similar to the other
describe block to always close/await any created servers. Concretely, track
created servers (or the wss variable) in the surrounding describe scope and in
afterEach call wss.close() (await the close callback or wrap in a Promise) to
ensure the socket is closed and the port freed even when the test throws; update
the session-continuity.test.ts tests that create WebSocketServer instances to
use that shared cleanup.
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 `@src/components/ui/chart.tsx`:
- Around line 235-241: The JSX conditional currently uses a truthy check
"item.value &&" which hides valid numeric zero; update the render condition to
explicitly check for null/undefined (use "item.value != null") where the span is
rendered so that 0 is displayed; locate the block that renders the span for
{item.value && ( ... )} in the chart component and replace that condition with
the null/undefined guard.
In `@tsconfig.json`:
- Around line 33-34: The tsconfig includes an invalid duplicated path
".next/dev/dev/types/**/*.ts"; remove that entry from the "include"/paths array
so only the correct Next.js generated type paths remain (e.g. keep
".next/dev/types/**/*.ts" and ".next/types/**/*.ts" where present). Locate the
string ".next/dev/dev/types/**/*.ts" in tsconfig.json and delete that array
element to fix the typo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecb633f8-ed0f-46f2-bf50-6ace01df9871
📒 Files selected for processing (2)
src/components/ui/chart.tsxtsconfig.json
Comment on lines
+235
to
+241
| {item.value && ( | ||
| <span className="text-foreground font-mono font-medium tabular-nums"> | ||
| {typeof item.value === "number" | ||
| ? item.value.toLocaleString() | ||
| : String(item.value)} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
值为 0 时不会渲染。
当前条件 item.value && 使用真值检查，当 item.value 为 0 时条件为假，导致有效的数值 0 不会被显示。应改用 != null 检查以正确处理所有数值。
建议的修复方案
- {item.value && (
+ {item.value != null && (
<span className="text-foreground font-mono font-medium tabular-nums">
{typeof item.value === "number"
? item.value.toLocaleString()
: String(item.value)}
</span>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/chart.tsx` around lines 235 - 241, The JSX conditional
currently uses a truthy check "item.value &&" which hides valid numeric zero;
update the render condition to explicitly check for null/undefined (use
"item.value != null") where the span is rendered so that 0 is displayed; locate
the block that renders the span for {item.value && ( ... )} in the chart
component and replace that condition with the null/undefined guard.
Comment on lines
+33
to
+34
| ".next/dev/types/**/*.ts", | ||
| ".next/dev/dev/types/**/*.ts" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: 检查 .next 目录下的 types 子目录结构
# 查找 .next 目录下所有包含 types 的路径
fd -t d 'types' .next 2>/dev/null || echo ".next directory not found or no types directories exist"
# 列出 .next 目录结构（如果存在）
if [ -d ".next" ]; then
tree -d -L 4 .next 2>/dev/null || find .next -type d -maxdepth 4 2>/dev/null
fiRepository: ding113/claude-code-hub
Length of output: 122
🏁 Script executed:
cat -n tsconfig.json | head -50Repository: ding113/claude-code-hub
Length of output: 1145
🌐 Web query:
Next.js .next/dev/types directory structure generated files
💡 Result:
In recent Next.js versions, route-related TypeScript artifacts are generated into the build output directory (distDir, default .next) when you run next dev, next build, or next typegen. The output is written to:
- Dev (commonly):
.next/dev/types/ - Build/typegen (commonly):
.next/types/(andnext typegenwrites to<distDir>/types) [1]
What you’ll typically find under .next/dev/types/
1) routes.d.ts
A generated declaration file containing the set of valid route strings in your app, used to type href for next/link and navigation APIs (typed routes). Next.js generates this kind of “hidden .d.ts file inside .next” during next dev / next build so the TS compiler can flag invalid links in the editor/CI. [2][3]
2) app/** (mirrors your app/ router tree)
A folder tree that mirrors your app/ directory structure (including route groups, dynamic segments, etc.), containing generated .ts type-check “stubs” for each route segment file (commonly page.ts, layout.ts, and other special files). These generated files are what show up in many build/type errors as paths like .next/types/app/.../page.ts. [4][3]
They exist so Next.js can:
- validate the shape of your exported functions (e.g.,
generateMetadata,generateStaticParams, etc.) against the route they belong to, and - provide globally available helpers like
PageProps<'/some/route'>after type generation. [3]
3) Optional: environment variable typings
If you enable experimental.typedEnv, Next.js generates a .d.ts under .next/types with typed env keys for editor IntelliSense. [2]
Notes
- The exact contents can vary by Next.js version and features (App Router vs Pages Router, typed env, etc.), but it’s all generated output and is safe to delete (it will be regenerated on the next run).
- If you run
tsc --noEmitwithout having generated these files, you may miss route-type checking;next typegenexists specifically to generate them without a full build. [1]
Sources: Next.js CLI (next typegen) output location [1], Next.js TypeScript docs on generated route .d.ts and typed env [2], Next.js page docs noting types are generated by next dev/build/typegen and that helpers become available after generation [3], and examples of generated .next/types/app/... files showing up in type errors [4].
[1] (nextjsjp.org)
[2] (nextjs.org)
[3] (nextjs.org)
[4] (nextjs-forum.com)
Citations:
- 1: https://nextjsjp.org/docs/app/api-reference/cli/next
- 2: https://nextjs.org/docs/app/building-your-application/configuring/typescript
- 3: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 4: https://nextjs-forum.com/post/1145855555731206216?utm_source=openai
第 34 行的路径 .next/dev/dev/types/**/*.ts 包含重复的 dev/dev，这不是有效的 Next.js 路径。
Next.js 的官方文档记录的类型生成路径只有两种：
.next/dev/types/（next dev期间生成）.next/types/（next build或next typegen时生成）
第 34 行的 .next/dev/dev/types/**/*.ts 不在任何官方文档中，应为拼写错误。建议删除这一行。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.json` around lines 33 - 34, The tsconfig includes an invalid
duplicated path ".next/dev/dev/types/**/*.ts"; remove that entry from the
"include"/paths array so only the correct Next.js generated type paths remain
(e.g. keep ".next/dev/types/**/*.ts" and ".next/types/**/*.ts" where present).
Locate the string ".next/dev/dev/types/**/*.ts" in tsconfig.json and delete that
array element to fix the typo.
| const targetOrigin = `http://127.0.0.1:${actualPort}`; | ||
| const wss = createResponsesUpgradeServer({ targetOrigin }); | ||
|
|
||
| server.on("upgrade", (req, socket, head) => { |
Contributor
There was a problem hiding this comment.
[CRITICAL] [SECURITY-VULNERABILITY] WebSocket upgrade allows idle unauthenticated connections (DoS risk)
Why this is a problem: server.on("upgrade") upgrades any request hitting /v1/responses, and handleConnection immediately starts a per-socket setInterval ping loop. There is no first-frame deadline for response.create, and protocol errors keep the socket open. An attacker can open many WebSocket connections and send nothing (or invalid frames) to consume sockets/timers and degrade the Node process.
Suggested fix:
// server.js
const FIRST_FRAME_TIMEOUT_MS = 5_000;
function hasAuthHeaders(headers) {
return (
typeof headers.authorization === "string" ||
typeof headers["x-api-key"] === "string" ||
typeof headers["x-goog-api-key"] === "string"
);
}
server.on("upgrade", (req, socket, head) => {
// ...existing URL parsing...
if (!hasAuthHeaders(req.headers)) {
socket.write("HTTP/1.1 401 Unauthorized\r\nConnection: close\r\n\r\n");
socket.destroy();
return;
}
wss.handleUpgrade(req, socket, head, (ws) => {
wss.emit("connection", ws, req);
});
});
function createResponsesWebSocketConnectionHandler(options) {
return function handleConnection(socket, request) {
let firstFrameTimeoutId = setTimeout(() => {
writeProtocolError(socket, "first_frame_timeout", "Timed out waiting for response.create");
socket.close(1008);
}, FIRST_FRAME_TIMEOUT_MS);
const clearFirstFrameTimeout = () => {
if (firstFrameTimeoutId) {
clearTimeout(firstFrameTimeoutId);
firstFrameTimeoutId = null;
}
};
socket.once("close", clearFirstFrameTimeout);
socket.on("message", async (raw) => {
// ...existing JSON parse...
let createFrame;
try {
createFrame = validateCreateFrame(parsedFrame);
clearFirstFrameTimeout();
} catch (error) {
writeProtocolError(
socket,
"invalid_request",
error instanceof Error ? error.message : "Invalid response.create frame"
);
socket.close(1008);
return;
}
// ...existing forwarding...
});
};
}
| state.activeTurn = false; | ||
| }); | ||
|
|
||
| socket.on("error", () => { |
Contributor
There was a problem hiding this comment.
[HIGH] [ERROR-SILENT] WebSocket error event is swallowed (no logging)
Why this is a problem: The current handler drops the error object and closes the socket:
socket.on("error", () => {
socket.close();
});This makes production debugging of bridge failures much harder (you lose the underlying error cause), and the client gets no actionable signal beyond a disconnect.
Suggested fix:
socket.on("error", (err) => {
console.error("[CCH] WS connection error", err);
if (state.activeAbortController) {
state.activeAbortController.abort();
}
socket.close();
});
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR is XL-sized and introduces a new same-port custom Node server plus WebSocket transport plumbing for /v1/responses. The main blockers are around WebSocket connection hardening: the current upgrade/connection flow can be held open cheaply (idle connections + per-socket ping timer) and some error paths are silent, which makes both DoS resistance and production debugging weaker than necessary.
PR Size: XL
- Lines changed: 16930
- Files changed: 70
- Split suggestions:
-
server.js+ Docker/standalone build changes (Dockerfile,next.config.ts,scripts/copy-version-to-standalone.cjs)
-
- Core proxy WS transport (
src/app/v1/_lib/proxy/responses-ws-*.ts,src/app/v1/_lib/proxy/forwarder.ts)
- Core proxy WS transport (
-
- Provider testing + UI/i18n (
src/lib/provider-testing/*,src/app/[locale]/*,messages/*)
- Provider testing + UI/i18n (
-
- Tests only (
tests/unit/*,tests/integration/responses-ws/*)
- Tests only (
-
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 1 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
server.js:429[CRITICAL] [SECURITY-VULNERABILITY] WebSocket upgrade allows idle unauthenticated connections (DoS risk). (Confidence: 96)
High Priority Issues (Should Fix)
server.js:363[HIGH] [ERROR-SILENT] WebSocketerrorevent is swallowed (no logging). (Confidence: 88)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
Applied the PR size label (size/XL) to PR #882.
Posted 2 inline review comments (diff-only):
server.js:429[CRITICAL] [SECURITY-VULNERABILITY] WS upgrade/connection flow can be held open cheaply (idle connections + per-socket ping timer) → suggested auth gating + first-frame timeout + close-on-protocol-error hardening.server.js:363[HIGH] [ERROR-SILENT] WSerrorevent is swallowed → suggested logging + aborting active turn before closing.
Submitted the required review summary on PR #882 with the issue counts and XL split suggestions.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds OpenAI
/v1/responsesWebSocket transport support, enabling Codex CLI and other clients to use native WebSocket streaming instead of SSE. The implementation follows the existingenableHttp2pattern with a global system toggle (enableResponsesWebSocket).Problem
/v1/responsesendpoint for better real-time interactionSolution
Architecture
server.on("upgrade")for WS -- no second port requiredresponse.create, then runs deferred guards and selects providerresponse.completed/response.failed/response.incomplete/erroreventsSecurity Hardening
maxPayload: 1 MiBto prevent DoS via oversized framesAbortSignalpropagation from client to upstream WS adapter to prevent resource leaks on disconnectwebsocketUrlsanitized before persisting to special settingsChanges
Core Changes
server.js): Node.js server handling both HTTP and/v1/responsesWebSocket upgrade with delayed-bridging ingressresponses-ws-adapter.ts: Outbound Responses WebSocket adapter with configurable timeoutsresponses-ws-schema.ts: Frame schema validationresponses-ws-terminal-finalization.ts: Terminal event handlingresponses-ws-transport.ts: Transport selector with HTTP fallbackforwarder.ts): Request-scoped outbound WS adapter integration with automatic HTTP fallbackSupporting Changes
0079_chilly_mentallo.sqladdsenable_responses_websocketto system configResponsesWebSocketTransportSpecialSettingfor debuggingTests
Breaking Changes
None. This is a fully backwards-compatible feature:
/v1/responsesbehavior unchangedTesting
Automated Tests
tests/unit/responses-ws-*.test.ts(4 files)tests/unit/system-settings-responses-websocket-toggle.test.tstests/unit/provider-test-ws-ui.test.tsxtests/integration/responses-ws/*.test.ts(8 files)Manual Testing Checklist
bun run typecheckpassesbun run lintpassesbun run buildsucceeds/v1/responsesbehavior unchangedChecklist
bun run db:generateDescription enhanced from original version authored by ding113
Greptile Summary
This PR introduces WebSocket transport support for the
/v1/responsesendpoint, enabling Codex CLI clients to use native WS streaming. The implementation follows the existingenableHttp2toggle pattern and is off-by-default, making it fully backwards-compatible. The architecture is well-thought-out: a single-port HTTP+WS server handles upgrade requests, buffers incoming WS frames untilresponse.createarrives, and bridges them to the existing loopback SSE handler viaforwarder.ts— which in turn can dispatch outbound via either WS (newresponses-ws-adapter.ts) or HTTP.Key issues found:
server.jsline 282: The non-SSE loopback response fallback callsresponse.json().catch(() => response.text()), but afterjson()fails (JSON parse error), the response body is already fully consumed —text()in the catch handler will throw or return an empty string. Fix: readtext()first and attemptJSON.parsemanually.server.jsline 337: When a terminal SSE event causes the read loop tobreak, theReadableStreamReaderis nevercancel()'d, leaving the loopback TCP connection open until the server naturally closes it. Fix: callreader.cancel()beforebreak.responses-ws-adapter.tstoSseChunk(line 61):event.typeis used directly in the SSE event line without stripping newline characters. The Zod schema does not reject\r/\nin the type field, so a maliciously-crafted upstream frame could inject extra SSE fields. Fix: sanitizeevent.typebefore embedding.responses-ws-adapter.tsline 163–190: Both thewslibrary's built-inhandshakeTimeoutoption and a manualsetTimeoutare set to the identical duration. Which fires first is non-deterministic, producing either"transport_setup_failed"or"handshake_timeout"as thefallbackReason. The functional HTTP-fallback decision is unaffected, but observability metadata is unreliable. Fix: rely on only one mechanism.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client as Codex Client (WS) participant Server as server.js (WS Ingress) participant Forwarder as forwarder.ts participant Adapter as responses-ws-adapter.ts participant Upstream as Upstream Provider (WS) Client->>Server: WS upgrade /v1/responses Server->>Client: 101 Switching Protocols Client->>Server: {"type":"response.create", "response":{...}} Note over Server: validateCreateFrame() Server->>Forwarder: POST /v1/responses (loopback SSE) Note over Forwarder: selectResponsesWsTransport() alt WS transport enabled & codex provider Forwarder->>Adapter: sendResponsesWsRequest() Adapter->>Upstream: WS connect + response.create frame Upstream-->>Adapter: stream of server events Adapter-->>Forwarder: SSE ReadableStream response else HTTP fallback Forwarder->>Upstream: HTTP POST /v1/responses Upstream-->>Forwarder: SSE response end Forwarder-->>Server: SSE stream (loopback) Note over Server: parseSseBlock() → sseEventToWsFrame() Server-->>Client: WS JSON frames (streaming) Note over Server: terminal event → break loop Server-->>Client: response.completed / response.failedLast reviewed commit: 0259b73