Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an inbound WebSocket transport for the Codex Responses endpoint, adding WebSocket upgrade handling, control framing, and bounded SSE reassembly. The review feedback highlights several issues: WebSocket handshake headers should be stripped before forwarding upstream to prevent protocol upgrade conflicts; errors from framer.finish() must be handled rather than ignored; send failures during warmup frames should terminate the outer session loop; test environment variables should be managed via a drop guard to avoid flakiness; and concat! for multi-line string literals in tests should be replaced with standard Rust string continuation escapes.
| let mut turn_headers = handshake_headers.clone(); | ||
| turn_headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); | ||
| turn_headers.remove(header::CONTENT_LENGTH); | ||
| turn_headers.remove(header::CONTENT_ENCODING); |
There was a problem hiding this comment.
[HIGH] Forwarding WebSocket handshake headers upstream
Problem: The WebSocket handshake headers (such as Upgrade, Connection, and Sec-WebSocket-*) are cloned from handshake_headers and forwarded upstream to the HTTP/SSE endpoint. This can cause the upstream server (or intermediate proxies/CDNs like Cloudflare) to reject the request or attempt an unexpected protocol upgrade.
Rationale: WebSocket-specific headers should be stripped before forwarding the request as a standard HTTP/SSE turn.
Suggestion: Remove the WebSocket handshake headers from turn_headers before dispatching.
| let mut turn_headers = handshake_headers.clone(); | |
| turn_headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); | |
| turn_headers.remove(header::CONTENT_LENGTH); | |
| turn_headers.remove(header::CONTENT_ENCODING); | |
| let mut turn_headers = handshake_headers.clone(); | |
| turn_headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); | |
| turn_headers.remove(header::CONTENT_LENGTH); | |
| turn_headers.remove(header::CONTENT_ENCODING); | |
| turn_headers.remove(header::UPGRADE); | |
| turn_headers.remove(header::CONNECTION); | |
| turn_headers.remove("sec-websocket-key"); | |
| turn_headers.remove("sec-websocket-version"); | |
| turn_headers.remove("sec-websocket-extensions"); | |
| turn_headers.remove("sec-websocket-protocol"); |
| if !terminal_seen && is_current() { | ||
| if let Ok(Some(tail)) = framer.finish() { | ||
| let s = match std::str::from_utf8(&tail) { | ||
| Ok(s) => s, | ||
| Err(_) => { | ||
| let err_frame = build_ws_error_frame( | ||
| 502, | ||
| "protocol_error", | ||
| "websocket_protocol_error", | ||
| "Invalid UTF-8 in upstream SSE frame", | ||
| None, | ||
| ); | ||
| let _ = out_tx | ||
| .send((turn_gen, Message::Text(err_frame.into()))) | ||
| .await; | ||
| return; | ||
| } | ||
| }; | ||
| if let Some(payload) = parse_sse_block(s) { | ||
| if payload != "[DONE]" { | ||
| if let Some(p_type) = parse_payload_type(&payload) { | ||
| let _ = out_tx.send((turn_gen, Message::Text(payload.into()))).await; | ||
| if terminal_status_from_type(&p_type).is_some() { | ||
| terminal_seen = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Unhandled error from framer.finish()
Problem: If framer.finish() returns an error (e.g., due to size limit or count limit violations on the trailing bytes), the error is silently ignored via if let Ok(Some(tail)) = framer.finish(). This can leave the client hanging or unaware of a protocol violation.
Rationale: Errors on the cleanup path should be handled and reported to the client consistently with the main loop.
Suggestion: Match on the result of framer.finish() and send a protocol error frame if it fails.
if !terminal_seen && is_current() {
match framer.finish() {
Ok(Some(tail)) => {
let s = match std::str::from_utf8(&tail) {
Ok(s) => s,
Err(_) => {
let err_frame = build_ws_error_frame(
502,
"protocol_error",
"websocket_protocol_error",
"Invalid UTF-8 in upstream SSE frame",
None,
);
let _ = out_tx
.send((turn_gen, Message::Text(err_frame.into())))
.await;
return;
}
};
if let Some(payload) = parse_sse_block(s) {
if payload != "[DONE]" {
if let Some(p_type) = parse_payload_type(&payload) {
let _ = out_tx.send((turn_gen, Message::Text(payload.into()))).await;
if terminal_status_from_type(&p_type).is_some() {
terminal_seen = true;
}
} else {
let err_frame = build_ws_error_frame(
502,
"protocol_error",
"websocket_protocol_error",
"Invalid JSON payload in upstream SSE frame",
None,
);
let _ = out_tx
.send((turn_gen, Message::Text(err_frame.into())))
.await;
return;
}
}
}
}
Ok(None) => {}
Err(err) => {
let err_frame = build_ws_error_frame(
502,
"protocol_error",
"websocket_protocol_error",
&err.to_string(),
None,
);
let _ = out_tx
.send((turn_gen, Message::Text(err_frame.into())))
.await;
return;
}
}
}| if let Err(err) = ws_tx.send(Message::Text(f.into())).await { | ||
| tracing::debug!(error = %err, "failed to send warmup frame"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Incomplete loop break on send failure
Problem: If sending a warmup frame fails, break only exits the inner for f in frames loop, but the outer loop continues to run. This can cause unnecessary iterations or state checks when the connection is already closed.
Rationale: A send failure on a closed connection should immediately terminate the socket session loop.
Suggestion: Return from handle_socket or break the outer loop on send failure.
| if let Err(err) = ws_tx.send(Message::Text(f.into())).await { | |
| tracing::debug!(error = %err, "failed to send warmup frame"); | |
| break; | |
| } | |
| if let Err(err) = ws_tx.send(Message::Text(f.into())).await { | |
| tracing::debug!(error = %err, "failed to send warmup frame"); | |
| return; | |
| } |
| fn cleanup(account_env: &str, client_env: &str) { | ||
| std::env::remove_var(account_env); | ||
| std::env::remove_var(client_env); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Use drop guard for environment variables in tests
Problem: The tests rely on process-global environment variables and clean them up manually at the end of each test using cleanup. If a test panics, the cleanup is bypassed, leading to test flakiness and cross-test interference.
Rationale: According to the general rules, process-global environment variables in tests should be cleaned up using a drop guard at the end of the test instead of resetting them on entry or manually cleaning them up.
Suggestion: Implement an EnvGuard drop guard and use it to automatically clean up environment variables.
struct EnvGuard {
vars: Vec<String>,
}
impl Drop for EnvGuard {
fn drop(&mut self) {
for var in &self.vars {
std::env::remove_var(var);
}
}
}References
- When testing code that relies on process-global environment variables, prevent test flakiness and cross-test interference by using unique suffixes (such as std::process::id()) for environment variable names, and clean them up using a drop guard at the end of the test instead of resetting them on entry.
| let body = concat!( | ||
| "data: {\"type\":\"response.created\",\"response\":{\"id\":\"r1\"}}\n\n", | ||
| "data: {\"type\":\"response.output_text.delta\",\"delta\":\"hi\"}\n\n", | ||
| "data: {\"type\":\"response.completed\",\"response\":{\"id\":\"r1\"}}\n\n", | ||
| "data: {\"type\":\"response.output_text.delta\",\"delta\":\"stale\"}\n\n" | ||
| ); |
There was a problem hiding this comment.
[MEDIUM] Avoid concat! for multi-line string literals
Problem: The test uses concat! to join multi-line string literals.
Rationale: According to the general rules, a backslash \ at the end of a line in a string literal acts as a string continuation escape, which discards the newline and all leading whitespace on the following line. Multi-line string literals formatted this way do not contain unintended leading spaces, and using concat! is not necessary.
Suggestion: Rewrite the multi-line string literal using backslash escapes.
let body = "data: {\"type\":\"response.created\",\"response\":{\"id\":\"r1\"}}\n\n\
data: {\"type\":\"response.output_text.delta\",\"delta\":\"hi\"}\n\n\
data: {\"type\":\"response.completed\",\"response\":{\"id\":\"r1\"}}\n\n\
data: {\"type\":\"response.output_text.delta\",\"delta\":\"stale\"}\n\n";References
- In Rust, a backslash \ at the end of a line in a string literal acts as a string continuation escape, which discards the newline and all leading whitespace on the following line. Multi-line string literals formatted this way do not contain unintended leading spaces, and using concat! is not necessary to avoid whitespace.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7bea579a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut turn_headers = handshake_headers.clone(); | ||
| turn_headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); | ||
| turn_headers.remove(header::CONTENT_LENGTH); | ||
| turn_headers.remove(header::CONTENT_ENCODING); |
There was a problem hiding this comment.
When a real Codex client supplies OpenAI-Beta: responses_websockets=2026-02-06 during the upgrade, cloning the handshake headers here passes that WebSocket-only selector—and the Sec-WebSocket-* handshake fields—into the upstream HTTP POST because the passthrough strip list does not remove them. The repository's HTTP request paths instead send OpenAI-Beta: responses=experimental; rewrite that selector and strip WebSocket handshake metadata before forwarding each turn so the ChatGPT backend receives a valid HTTP Responses request.
Useful? React with 👍 / 👎.
| match type_str { | ||
| "response.completed" => Some(TerminalStatus::Completed), | ||
| "response.failed" => Some(TerminalStatus::Failed), | ||
| "response.incomplete" => Some(TerminalStatus::Incomplete), | ||
| _ => None, |
There was a problem hiding this comment.
On a 200 OK SSE stream ending with data: {"type":"error",...}, this match returns None, even though the existing Responses translator and outbound WebSocket transport both treat error as terminal. run_turn consequently forwards the real error frame, reaches EOF with terminal_seen == false, and sends a second, misleading 502 websocket_protocol_error; include error in terminal classification so backend rate-limit or policy errors end the turn after the original frame.
Useful? React with 👍 / 👎.
| if index + 1 >= length { | ||
| return None; | ||
| } | ||
| if byte_at(index + 1) != b'\n' { | ||
| return Some(0); |
There was a problem hiding this comment.
For a standards-compliant upstream that terminates SSE lines with bare carriage returns, the valid blank-line delimiter \r\r reaches this branch and is treated as ordinary content because only \r\n combinations are recognized. Multiple events are then merged into one block and fail JSON parsing, producing a spurious 502; recognize \r\r alongside the existing LF and CRLF delimiter variants.
Useful? React with 👍 / 👎.
| a bare base produces `/responses`. Registering all three lets an operator use either CLI setup | ||
| style (§ "Codex CLI setup" below) without shunt needing to know which one a given client chose. | ||
|
|
||
| ### WebSocket transport |
There was a problem hiding this comment.
This new WebSocket section conflicts with the unchanged “Out of scope / follow-up” entry later in the same document, which still says the endpoint is HTTP/SSE-only and that WebSocket support is future work. Remove or update that stale entry so operators are not given mutually exclusive support information.
AGENTS.md reference: AGENTS.md:L47-L53
Useful? React with 👍 / 👎.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| let body_bytes = Bytes::from(serde_json::to_vec(&raw_json).unwrap_or_default()); | ||
|
|
||
| let mut turn_headers = handshake_headers.clone(); |
There was a problem hiding this comment.
Authentication runs only during the initial WebSocket upgrade. Later response.create turns use refreshed runtime state without revalidating the caller, so a socket authenticated with a token that is subsequently rotated or removed can continue submitting provider-funded turns. If a reload changes the configured authentication header name, each turn also retains the original upgrade headers while the refreshed sanitizer removes only the new header name, allowing the old custom credential header to reach the default Codex upstream. Reauthenticate each turn, or preserve and use the original authentication snapshot when sanitizing the stored headers.
How this was verified: The upgrade is the only authentication call, while later turns clone its headers and dispatch using refreshed state whose sanitizer recognizes only the current configured credential header.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/codex_endpoint/websocket.rs
Line: 164
Comment:
**Stale Socket Authentication**
Authentication runs only during the initial WebSocket upgrade. Later `response.create` turns use refreshed runtime state without revalidating the caller, so a socket authenticated with a token that is subsequently rotated or removed can continue submitting provider-funded turns. If a reload changes the configured authentication header name, each turn also retains the original upgrade headers while the refreshed sanitizer removes only the new header name, allowing the old custom credential header to reach the default Codex upstream. Reauthenticate each turn, or preserve and use the original authentication snapshot when sanitizing the stored headers.
**How this was verified:** The upgrade is the only authentication call, while later turns clone its headers and dispatch using refreshed state whose sanitizer recognizes only the current configured credential header.
**Knowledge Base Used:**
- [Inbound authentication and caller access](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/shunt/-/docs/inbound-access-control.md)
- [Restore shared credential-slot stripping](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/shunt/-/reverts/rollback_391-20260818-credential-slot-forwarding-9fe69f8.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| // one immutable runtime snapshot while later turns observe newer config. | ||
| let state = state.refreshed(); | ||
| let started_at = Instant::now(); | ||
| let dispatch_res = forward_turn(state, model, pool_key, headers, body, started_at).await; |
There was a problem hiding this comment.
WebSocket turns bypass server.limits.max_request_bytes. The upgrade accepts messages up to the hard-coded 4 MiB limit and calls forward_turn directly, while the HTTP path applies the configured size checks first. When an operator configures a limit below 4 MiB, an oversized response.create is therefore serialized and forwarded upstream instead of being rejected. Apply the configured limit before dispatching each turn.
Knowledge Base Used: Request processing gateway
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/codex_endpoint/websocket.rs
Line: 215
Comment:
**Request Limit Bypassed**
WebSocket turns bypass `server.limits.max_request_bytes`. The upgrade accepts messages up to the hard-coded 4 MiB limit and calls `forward_turn` directly, while the HTTP path applies the configured size checks first. When an operator configures a limit below 4 MiB, an oversized `response.create` is therefore serialized and forwarded upstream instead of being rejected. Apply the configured limit before dispatching each turn.
**Knowledge Base Used:** [Request processing gateway](https://app.greptile.com/passionfactory/-/custom-context/knowledge-base/pleaseai/shunt/-/docs/request-processing.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/codex_endpoint/websocket.rs">
<violation number="1" location="src/codex_endpoint/websocket.rs:370">
P1: When the upstream emits a `type: "error"` event, this condition does not stop the relay, so later events can be sent after a terminal backend error. Include `error` in the terminal classification and stop processing it.</violation>
</file>
<file name="src/server.rs">
<violation number="1" location="src/server.rs:265">
P2: When `server.max_concurrent_requests` is configured, this WebSocket route releases its permit immediately after the `101` handshake. Active sockets can therefore bypass the documented cap and create more live turn tasks than the configured limit; hold a WebSocket-specific permit until socket closure.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CLI as Codex CLI
participant WS as WebSocket Upgrade Handler
participant Auth as Inbound Auth
participant Router as Shared Route Dispatcher
participant Pool as Account Pool
participant SSE as SSE Reassembler
participant Upstream as Upstream Provider
Note over CLI,Upstream: Inbound Responses WebSocket Transport Flow
CLI->>WS: GET /responses (WebSocket upgrade)
WS->>Auth: authenticate_inbound (token check)
alt Valid token
Auth-->>WS: Client identity
WS-->>CLI: 101 Switching Protocols
else Missing/invalid token
Auth-->>WS: 401 Unauthorized
WS-->>CLI: HTTP 401 (no upgrade)
end
CLI->>WS: response.create frame (generate: true)
WS->>WS: Parse frame, strip control flags, force stream: true
alt generate: false warmup
WS->>CLI: Local response.created + response.completed frames
else generate: true
WS->>Router: forward_turn (same as HTTP POST)
Router->>Pool: Select account (sticky by session + client)
Pool-->>Router: Account credentials
Router->>Upstream: HTTP POST (streaming SSE, swapped auth headers)
Upstream-->>SSE: SSE events (bounded 4 MiB frames)
SSE-->>WS: Stream events (backpressure, bounded queue)
WS-->>CLI: Forward frames until terminal event
end
alt Socket disconnect or replacement turn
WS->>WS: Cancel active upstream body
else Binary frame
WS-->>CLI: Error frame, close socket
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| let _ = out_tx.send((turn_gen, Message::Text(payload.into()))).await; | ||
|
|
||
| if terminal_status_from_type(&p_type).is_some() { |
There was a problem hiding this comment.
P1: When the upstream emits a type: "error" event, this condition does not stop the relay, so later events can be sent after a terminal backend error. Include error in the terminal classification and stop processing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/codex_endpoint/websocket.rs, line 370:
<comment>When the upstream emits a `type: "error"` event, this condition does not stop the relay, so later events can be sent after a terminal backend error. Include `error` in the terminal classification and stop processing it.</comment>
<file context>
@@ -0,0 +1,424 @@
+
+ let _ = out_tx.send((turn_gen, Message::Text(payload.into()))).await;
+
+ if terminal_status_from_type(&p_type).is_some() {
+ terminal_seen = true;
+ break;
</file context>
| router = router.route(path, post(codex_endpoint::post)); | ||
| router = router.route( | ||
| path, | ||
| get(codex_endpoint::websocket::get).post(codex_endpoint::post), |
There was a problem hiding this comment.
P2: When server.max_concurrent_requests is configured, this WebSocket route releases its permit immediately after the 101 handshake. Active sockets can therefore bypass the documented cap and create more live turn tasks than the configured limit; hold a WebSocket-specific permit until socket closure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/server.rs, line 265:
<comment>When `server.max_concurrent_requests` is configured, this WebSocket route releases its permit immediately after the `101` handshake. Active sockets can therefore bypass the documented cap and create more live turn tasks than the configured limit; hold a WebSocket-specific permit until socket closure.</comment>
<file context>
@@ -260,7 +260,10 @@ pub fn build_router(config: Config) -> Result<(Router, SharedState, AppState), C
- router = router.route(path, post(codex_endpoint::post));
+ router = router.route(
+ path,
+ get(codex_endpoint::websocket::get).post(codex_endpoint::post),
+ );
}
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb03a0eae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .as_ref() | ||
| .map(|auth| auth.header().clone()); | ||
|
|
||
| ws.on_upgrade(move |socket| handle_socket(socket, state, pool_key, headers, auth_header)) |
There was a problem hiding this comment.
When [server.auth] is absent—the documented loopback/personal-use mode—this accepts a WebSocket upgrade from any browser Origin. A malicious page can therefore connect to ws://127.0.0.1:3001/v1/responses, submit response.create frames, read model output, and consume the operator's upstream account without encountering the CORS preflight that protects JSON HTTP requests; validate Origin/Host for ungated upgrades or require authentication for this transport.
Useful? React with 👍 / 👎.
| let Some(codex_endpoint) = state.config.server.codex_endpoint.as_ref() else { | ||
| return; |
There was a problem hiding this comment.
When an existing socket survives a config reload that removes [server.codex_endpoint], every subsequent live response.create reaches this branch and silently returns without sending a terminal error or closing the connection. The client consequently waits indefinitely for a response; enqueue a Responses-shaped error frame (and optionally close the socket) instead of abandoning the turn.
AGENTS.md reference: AGENTS.md:L41-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8f6fa8297
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let auth_header = state | ||
| .inbound_auth | ||
| .as_ref() | ||
| .map(|auth| auth.header().clone()); |
There was a problem hiding this comment.
When a socket handshake includes a credential in a custom [server.admin].header and that header name changes during hot reload, later turns clone the original handshake headers but sanitize them using only the refreshed admin header; this snapshot preserves only the old [server.auth] header. The former admin header can therefore be forwarded to the upstream despite the guarantee documented in site/src/content/docs/guides/inbound-codex-endpoint.md:83; snapshot and remove the handshake-time admin header as well.
AGENTS.md reference: AGENTS.md:L47-L49
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/codex_endpoint/websocket.rs">
<violation number="1" location="src/codex_endpoint/websocket.rs:583">
P2: When an unauthenticated browser connects through an IPv6 literal without an explicit port, `same_origin_or_non_browser` compares `::1` with `[::1]` and rejects the legitimate same-origin upgrade. Parse bracketed `Host` authorities with an IPv6-aware parser before comparing host and port.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return false; | ||
| }; | ||
| let origin_port = origin.port_or_known_default(); | ||
| let host_without_port = host.rsplit_once(':').map_or(host, |(host, port)| { |
There was a problem hiding this comment.
P2: When an unauthenticated browser connects through an IPv6 literal without an explicit port, same_origin_or_non_browser compares ::1 with [::1] and rejects the legitimate same-origin upgrade. Parse bracketed Host authorities with an IPv6-aware parser before comparing host and port.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/codex_endpoint/websocket.rs, line 583:
<comment>When an unauthenticated browser connects through an IPv6 literal without an explicit port, `same_origin_or_non_browser` compares `::1` with `[::1]` and rejects the legitimate same-origin upgrade. Parse bracketed `Host` authorities with an IPv6-aware parser before comparing host and port.</comment>
<file context>
@@ -525,3 +559,42 @@ async fn run_turn(context: TurnContext) {
+ return false;
+ };
+ let origin_port = origin.port_or_known_default();
+ let host_without_port = host.rsplit_once(':').map_or(host, |(host, port)| {
+ if port.parse::<u16>().is_ok() {
+ host.trim_matches(['[', ']'])
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8877e563
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## WebSocket transport | ||
|
|
||
| The three Responses paths accept both HTTP `POST` and authenticated WebSocket `GET` upgrades. Authentication completes before `101 Switching Protocols`. On a socket, `generate: false` warmups complete locally; live `response.create` frames reuse the HTTP account pool, force upstream streaming, and forward each SSE `data:` payload as a WebSocket text frame through the first terminal event. Replacing a turn or closing the socket cancels the active upstream body. Client frames and SSE events are limited to 4 MiB, sends apply bounded backpressure, and protocol/upstream failures use standalone `type: "error"` frames with only safe response metadata. |
There was a problem hiding this comment.
The new WebSocket section conflicts with site/src/content/docs/guides/connect-codex-cli.mdx:90-111,284-289, which still says shunt has no WebSocket route, instructs users not to enable supports_websockets, and describes the endpoint as HTTP/SSE-only; the ja/ko/zh-cn copies repeat those instructions, and docs/codex-configuration.md:831 retains the same claim. Update these affected guides and translations so users are not directed away from the transport added here.
AGENTS.md reference: AGENTS.md:L47-L53
Useful? React with 👍 / 👎.
| let Some(codex_endpoint) = &state.config.server.codex_endpoint else { | ||
| return ShuntError::bad_gateway("codex endpoint is not configured".to_string()) | ||
| .into_response(); |
There was a problem hiding this comment.
When the endpoint was enabled at boot but a reload removes [server.codex_endpoint], its routes remain registered, so a new WebSocket upgrade reaches this branch and receives the raw Anthropic-shaped ShuntError. This bypasses into_openai_error_shape, unlike the other pre-upgrade failures, and prevents Responses clients from parsing this gateway-owned 502 through their expected error path.
AGENTS.md reference: AGENTS.md:L41-L42
Useful? React with 👍 / 👎.
| if index + 2 >= length { | ||
| return None; | ||
| } | ||
| return if byte_at(index + 2) == b'\n' { | ||
| Some(3) | ||
| } else { | ||
| Some(0) |
There was a problem hiding this comment.
When an upstream terminates the data line with LF and the following blank line with bare CR (...\n\r), this branch accepts the delimiter only if the CR is followed by LF. The fresh \r\r handling therefore still misses this standards-valid mixed ending (and similarly \r\n\r), causing adjacent events to be merged and rejected as invalid JSON with a spurious 502; recognize bare CR as the second line ending without requiring a following LF.
Useful? React with 👍 / 👎.
eb8877e to
3440be0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3440be027d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| origin_host.eq_ignore_ascii_case(host_without_port) | ||
| && origin_port | ||
| == host_port.or_else(|| match origin.scheme() { | ||
| "http" => Some(80), | ||
| "https" => Some(443), | ||
| _ => None, | ||
| }) |
There was a problem hiding this comment.
When an ungated endpoint is exposed as wss://example.com with Host: example.com, a page from http://example.com passes this check: origin_port is 80, and the missing Host port is incorrectly defaulted from the Origin scheme to the same value, even though the socket's default port is 443. This fresh evidence shows the origin-validation fix still permits a cross-origin browser socket in that deployment; compare the Origin against the effective request scheme and port rather than using the Origin to supply the target's missing port.
Useful? React with 👍 / 👎.
| ## WebSocket transport | ||
|
|
||
| The three Responses paths accept both HTTP `POST` and authenticated WebSocket `GET` upgrades. Authentication completes before `101 Switching Protocols`. On a socket, `generate: false` warmups complete locally; live `response.create` frames reuse the HTTP account pool, force upstream streaming, and forward each SSE `data:` payload as a WebSocket text frame through the first terminal event. Replacing a turn or closing the socket cancels the active upstream body. Client frames and SSE events are limited to 4 MiB, sends apply bounded backpressure, and protocol/upstream failures use standalone `type: "error"` frames with only safe response metadata. |
There was a problem hiding this comment.
This new WebSocket behavior is absent from the user-facing reference surfaces: site/src/content/docs/reference/endpoints.md:42-44 still lists these paths as POST-only, while site/src/content/docs/reference/configuration.md:291 says the table registers only POST routes; the ja/ko/zh-cn copies retain the same descriptions. Update those reference pages and translations so users consulting the endpoint or configuration reference can discover and correctly understand the newly registered GET upgrades.
AGENTS.md reference: AGENTS.md:L47-L53
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c789293d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "response.completed" => Some(TerminalStatus::Completed), | ||
| "response.failed" => Some(TerminalStatus::Failed), | ||
| "response.incomplete" => Some(TerminalStatus::Incomplete), | ||
| "error" => Some(TerminalStatus::Failed), | ||
| _ => None, |
There was a problem hiding this comment.
response.done as terminal
When a Responses-compatible upstream ends its SSE stream with {"type":"response.done"}, this match returns None, although the existing Responses translator and stream observer both classify response.done as a normal terminal event. run_turn therefore forwards the valid terminal frame and then emits a misleading 502 websocket_protocol_error at EOF; include response.done in this terminal set and cover it with a WebSocket regression test.
AGENTS.md reference: src/AGENTS.md:L29-L30
Useful? React with 👍 / 👎.
8c78929 to
576dd99
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 576dd99e96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let mut ws = match ws { | ||
| Ok(ws) => ws, | ||
| Err(rejection) => return rejection.into_response(), |
There was a problem hiding this comment.
When an authenticated client sends a malformed upgrade—for example, an unsupported Sec-WebSocket-Version or missing Connection: upgrade—this returns Axum's raw WebSocketUpgradeRejection response rather than the OpenAI Responses error envelope used by every other gateway-owned failure on these paths. Responses clients therefore cannot parse or surface the handshake error through their normal error path; reshape this rejection before returning it.
AGENTS.md reference: AGENTS.md:L42-L42
Useful? React with 👍 / 👎.
1 participant
Summary
Adds authenticated WebSocket transport support for the inbound OpenAI Responses (Codex) endpoint (
/backend-api/codex/responses,/responses,/v1/responses).Problem
The inbound Codex endpoint previously accepted only HTTP
POSTrequests. Clients using WebSocket connections (such as Codex CLI / desktop when configured for WebSocket or usingwire_api = "responses") could not connect or stream turns over an established socket.Solution
GETWebSocket upgrade handler on all registered Responses paths (/backend-api/codex/responses,/responses,/v1/responses).101 Switching Protocols, rejecting unauthenticated connections before upgrade.response.create,response.processedack, close, etc.) with support for localgenerate: falsewarmups.forward_turndispatch machinery: WebSocket turns use the exact same account-pool selection, model routing, and headers allowlist as HTTP POST turns.tests/inbound_codex_websocket.rsand unit tests insrc/codex_endpoint/frame.rs.docs/m11-inbound-codex-endpoint.mdand localized site reference (en,ja,ko,zh-cn).Verification
cargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --test inbound_codex_websocketcargo test --lib codex_endpoint::Summary by cubic
Adds an authenticated WebSocket transport for the inbound Codex Responses endpoint so socket-based clients (e.g. Codex CLI with
wire_api = "responses") can stream turns, where previously only HTTPPOSTwas accepted./backend-api/codex/responses,/responses,/v1/responses) now also acceptGETWebSocket upgrades.101 Switching Protocols; unauthenticated connections and upgrades from disallowed origins are rejected before the handshake.forward_turnpath, so account-pool selection, model routing, and the headers allowlist match HTTPPOST, including stripping reloaded admin headers.tokio-tungstenite0.29 alongside the existing 0.28 dependency and enables theaxumwsfeature.docs/and the localized site reference.Written for commit 576dd99. Summary will update on new commits.