-
Notifications
You must be signed in to change notification settings - Fork 244
Gate Responses WebSocket transport on provider capability #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -344,7 +344,9 @@ impl ModelClient { | |
| } | ||
|
|
||
| fn active_ws_version_for_prompt(&self, prompt: &Prompt) -> Option<ResponsesWebsocketVersion> { | ||
| if self.websockets_disabled.load(Ordering::Relaxed) { | ||
| if self.websockets_disabled.load(Ordering::Relaxed) | ||
| || !self.provider.supports_websockets | ||
| { | ||
| return None; | ||
| } | ||
|
|
||
|
|
@@ -395,6 +397,22 @@ impl ModelClient { | |
| )) | ||
| } | ||
|
|
||
| /// Use the session-correlation header names emitted by codex-rs. | ||
| /// | ||
| /// The older fork used underscore-style `conversation_id`, `session_id`, | ||
| /// and `thread_id` headers. OpenAI-compatible gateways may route those | ||
| /// legacy names differently from codex-rs's hyphenated headers. | ||
| fn apply_responses_session_headers( | ||
| &self, | ||
| req_builder: reqwest::RequestBuilder, | ||
| session_id: &str, | ||
| ) -> reqwest::RequestBuilder { | ||
| req_builder | ||
| .header("x-client-request-id", session_id) | ||
| .header("session-id", session_id) | ||
| .header("thread-id", session_id) | ||
|
Comment on lines
+411
to
+413
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No request-level test verifies this header migration across HTTP streaming, WebSocket handshakes, or compaction. Add integration assertions that Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| fn current_reasoning_param( | ||
| &self, | ||
| family: &ModelFamily, | ||
|
|
@@ -658,6 +676,12 @@ impl ModelClient { | |
| } | ||
| } | ||
| WireApi::ResponsesWebsocket => { | ||
| if !self.provider.supports_websockets { | ||
| warn!( | ||
| "responses_websocket transport is disabled for this provider; using responses HTTP stream" | ||
| ); | ||
| return self.stream_responses(prompt, log_tag).await; | ||
|
Comment on lines
+679
to
+683
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Existing custom providers with Useful? React with 👍 / 👎. |
||
| } | ||
| if self.websockets_disabled.load(Ordering::Relaxed) { | ||
| warn!( | ||
| "responses_websocket transport disabled for this session; using responses HTTP stream" | ||
|
|
@@ -946,10 +970,7 @@ impl ModelClient { | |
| if let Some(state) = turn_state.get() { | ||
| req_builder = req_builder.header(X_CODEX_TURN_STATE_HEADER, state); | ||
| } | ||
| req_builder = req_builder | ||
| .header("conversation_id", session_id_str.clone()) | ||
| .header("session_id", session_id_str.clone()) | ||
| .header("thread_id", session_id_str.clone()); | ||
| req_builder = self.apply_responses_session_headers(req_builder, &session_id_str); | ||
| if let Ok(window_id) = HeaderValue::from_str(&self.current_window_id(session_id)) { | ||
| req_builder = req_builder.header(X_CODEX_WINDOW_ID_HEADER, window_id); | ||
| } | ||
|
|
@@ -1450,11 +1471,8 @@ impl ModelClient { | |
| req_builder = req_builder.header(X_CODEX_TURN_STATE_HEADER, state); | ||
| } | ||
|
|
||
| req_builder = req_builder | ||
| // Send `conversation_id`/`session_id` so the server can hit the prompt-cache. | ||
| .header("conversation_id", session_id_str.clone()) | ||
| .header("session_id", session_id_str.clone()) | ||
| .header("thread_id", session_id_str.clone()) | ||
| req_builder = self | ||
| .apply_responses_session_headers(req_builder, &session_id_str) | ||
| .header(reqwest::header::ACCEPT, "text/event-stream") | ||
| .json(&payload_json); | ||
| if let Ok(window_id) = HeaderValue::from_str(&self.current_window_id(session_id)) { | ||
|
|
@@ -2108,10 +2126,7 @@ impl ModelClient { | |
| request = request.header(X_CODEX_WINDOW_ID_HEADER, window_id); | ||
| } | ||
|
|
||
| request = request | ||
| .header("conversation_id", session_id_str.clone()) | ||
| .header("session_id", session_id_str.clone()) | ||
| .header("thread_id", session_id_str.clone()); | ||
| request = self.apply_responses_session_headers(request, &session_id_str); | ||
|
|
||
| if let Some(auth) = auth.as_ref() | ||
| && auth.mode.is_chatgpt() | ||
|
|
@@ -3296,6 +3311,7 @@ mod tests { | |
| stream_max_retries: None, | ||
| stream_idle_timeout_ms: None, | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| } | ||
|
|
@@ -3420,6 +3436,7 @@ mod tests { | |
| stream_max_retries: None, | ||
| stream_idle_timeout_ms: None, | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3470,6 +3487,7 @@ mod tests { | |
| stream_max_retries: None, | ||
| stream_idle_timeout_ms: None, | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3522,6 +3540,7 @@ mod tests { | |
| stream_max_retries: None, | ||
| stream_idle_timeout_ms: None, | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3564,6 +3583,7 @@ mod tests { | |
| stream_max_retries: None, | ||
| stream_idle_timeout_ms: None, | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3714,6 +3734,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3803,6 +3824,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3851,6 +3873,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -3905,6 +3928,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4002,6 +4026,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4111,6 +4136,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4363,6 +4389,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4435,6 +4462,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4471,6 +4499,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4504,6 +4533,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
@@ -4537,6 +4567,7 @@ mod tests { | |
| stream_max_retries: Some(0), | ||
| stream_idle_timeout_ms: Some(1000), | ||
| websocket_connect_timeout_ms: None, | ||
| supports_websockets: false, | ||
| requires_openai_auth: false, | ||
| openrouter: None, | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new tests only verify provider deserialization and defaults; none exercises
ModelClient::stream. Add request-level integration cases proving that WebSocket-preferringWireApi::Responsesand explicitWireApi::ResponsesWebsocketproviders go directly to HTTP when unsupported, whilesupports_websockets = truestill selects WebSocket. Without this, the central dispatch behavior—and the compatibility regression for existing explicit WebSocket configurations—can go unnoticed.AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.