From 888be42a20c5a727214d898c4e335ac2e27161af Mon Sep 17 00:00:00 2001 From: andrewgu-oai Date: Fri, 18 Sep 2026 22:43:38 +0000 Subject: [PATCH] Support explicit provider model catalog URLs (#46561) ## Why Providers need to serve Codex model metadata independently of their inference endpoint. A custom inference `base_url` alone should not opt API-key sessions into remote catalog discovery. ## What changed - Add `model_catalog_url` to provider configuration and thread-config serialization. Fetch the full catalog URL with provider authentication, headers, query parameters, and `client_version`, while preserving inference routing. - Require an explicit catalog URL for API-key discovery with a custom base URL. Keep default OpenAI discovery on the Codex backend, and apply `api_key_model_discovery` gating to provider API keys and cached catalogs. - Include the catalog URL in cache identity so different catalogs do not share cached metadata. - Limit explicitly configured catalog responses to 1 MiB, reject redirects, and suppress URL and response diagnostics that could expose credentials. ## Testing Add coverage for catalog configuration round-trips, query encoding, authentication and header reuse, cache identity and discovery gating, oversized responses, and redirect rejection without credential forwarding. Add an integration test verifying that catalog model metadata and instructions reach conversation requests. GitOrigin-RevId: 7f6ac5eec257278685e1867a6bcebae6514a365a --- .../thread_processor_tests.rs | 1 + .../app-server/tests/suite/v2/model_list.rs | 39 ++- codex-rs/codex-api/src/endpoint/models.rs | 89 +++++- .../codex-api/tests/models_integration.rs | 12 +- codex-rs/config/src/thread_config.rs | 1 + .../proto/codex.thread_config.v1.proto | 1 + .../proto/codex.thread_config.v1.rs | 2 + codex-rs/config/src/thread_config/remote.rs | 7 + codex-rs/core/config.schema.json | 4 + codex-rs/core/tests/responses_headers.rs | 3 + codex-rs/core/tests/suite/client.rs | 4 + .../core/tests/suite/client_websockets.rs | 1 + .../core/tests/suite/models_cache_auth.rs | 5 + codex-rs/core/tests/suite/remote_models.rs | 66 +++++ .../suite/stream_error_allows_next_turn.rs | 1 + .../core/tests/suite/stream_no_completed.rs | 1 + .../async_scorer/sampler/connection_pool.rs | 2 + codex-rs/http-client/src/client.rs | 6 + codex-rs/http-client/src/transport.rs | 1 + codex-rs/http-client/src/transport_tests.rs | 7 +- codex-rs/login/src/auth/default_client.rs | 3 +- codex-rs/login/src/auth_env_telemetry.rs | 1 + codex-rs/model-provider-info/src/lib.rs | 6 + .../src/model_provider_info_tests.rs | 35 +++ .../model-provider/src/models_endpoint.rs | 267 +++++++++++++++++- .../model-provider/src/models_identity.rs | 18 +- .../src/models_identity_tests.rs | 14 + codex-rs/model-provider/src/provider.rs | 62 ++-- .../model-provider/src/shared_state_tests.rs | 2 + codex-rs/models-manager/src/manager.rs | 23 +- 30 files changed, 615 insertions(+), 69 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs index 8a6922da4e..44c29bda15 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs @@ -594,6 +594,7 @@ mod thread_processor_behavior_tests { let session_provider = ModelProviderInfo { name: "session".to_string(), base_url: Some("http://127.0.0.1:8061/api/codex".to_string()), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/app-server/tests/suite/v2/model_list.rs b/codex-rs/app-server/tests/suite/v2/model_list.rs index 217f0cafa7..5d26c68e78 100644 --- a/codex-rs/app-server/tests/suite/v2/model_list.rs +++ b/codex-rs/app-server/tests/suite/v2/model_list.rs @@ -39,14 +39,16 @@ use wiremock::matchers::path; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); const INVALID_REQUEST_ERROR_CODE: i64 = -32600; -#[test_case(None, false; "default off")] -#[test_case(None, true; "app rollout")] -#[test_case(Some(false), true; "user opt out")] -#[test_case(Some(true), false; "user opt in")] +#[test_case(None, false, true; "default off")] +#[test_case(None, true, true; "app rollout")] +#[test_case(Some(false), true, true; "user opt out")] +#[test_case(Some(true), false, true; "user opt in")] +#[test_case(Some(true), false, false; "base URL without catalog opt in")] #[tokio::test] async fn api_key_model_discovery_startup_enablement_respects_user_config( user_enablement: Option, app_enablement: bool, + catalog_opt_in: bool, ) -> Result<()> { let server = MockServer::start().await; let mut remote_model = codex_models_manager::bundled_models_response()? @@ -69,9 +71,24 @@ async fn api_key_model_discovery_startup_enablement_respects_user_config( let feature_config = user_enablement .map(|enabled| format!("[features]\napi_key_model_discovery = {enabled}\n")) .unwrap_or_default(); + let catalog_config = if catalog_opt_in { + format!("model_catalog_url = \"{server_uri}/v1/models\"\n") + } else { + String::new() + }; std::fs::write( codex_home.path().join("config.toml"), - format!("openai_base_url = \"{server_uri}/v1\"\n{feature_config}"), + format!( + r#" +model_provider = "catalog-test" +{feature_config} +[model_providers.catalog-test] +name = "OpenAI" +base_url = "{server_uri}/v1" +requires_openai_auth = true +{catalog_config} +"# + ), )?; login_with_api_key( codex_home.path(), @@ -113,7 +130,7 @@ async fn api_key_model_discovery_startup_enablement_respects_user_config( }, }) .await?; - let enabled = user_enablement.unwrap_or(app_enablement); + let enabled = user_enablement.unwrap_or(app_enablement) && catalog_opt_in; let expected = if enabled { &remote } else { &bundled }; assert_eq!( response, @@ -135,7 +152,8 @@ async fn api_key_model_discovery_startup_enablement_respects_user_config( .received_requests() .await .expect("request recording is enabled") - .is_empty() + .iter() + .all(|request| request.url.path() != "/v1/models") ); } Ok(()) @@ -344,9 +362,14 @@ async fn list_models_uses_remote_catalog_as_source_of_truth( model = "mock-model" approval_policy = "never" sandbox_mode = "read-only" -openai_base_url = "{server_uri}/v1" +model_provider = "catalog-test" [features] api_key_model_discovery = true +[model_providers.catalog-test] +name = "OpenAI" +base_url = "{server_uri}/v1" +requires_openai_auth = true +model_catalog_url = "{server_uri}/v1/models" "# ), )?; diff --git a/codex-rs/codex-api/src/endpoint/models.rs b/codex-rs/codex-api/src/endpoint/models.rs index ed8909ab1f..c00bc0115e 100644 --- a/codex-rs/codex-api/src/endpoint/models.rs +++ b/codex-rs/codex-api/src/endpoint/models.rs @@ -11,6 +11,7 @@ use http::HeaderMap; use http::Method; use http::header::ETAG; use std::sync::Arc; +use url::Url; pub struct ModelsClient { session: EndpointSession, @@ -44,17 +45,36 @@ impl ModelsClient { request.url } + /// Builds a full catalog URL, preserving provider routing parameters and client version. + pub fn catalog_request_url( + provider: &Provider, + catalog_url: &str, + client_version: &str, + ) -> Result { + let invalid = |message: &str| ApiError::InvalidRequest { + message: message.to_string(), + }; + let mut url = Url::parse(catalog_url) + .map_err(|_| invalid("model_catalog_url must be an absolute URL"))?; + { + let mut query = url.query_pairs_mut(); + if let Some(params) = &provider.query_params { + query.extend_pairs(params); + } + query.append_pair("client_version", client_version); + } + Ok(url.into()) + } + + /// Fetches and decodes a catalog, optionally bounding response bytes before decoding. pub async fn list_models( &self, request_url: String, extra_headers: HeaderMap, + response_body_limit_bytes: Option, ) -> Result<(Vec, Option), ApiError> { let (body, header_etag) = self - .list_models_raw( - request_url, - extra_headers, - /*response_body_limit_bytes*/ None, - ) + .list_models_raw(request_url, extra_headers, response_body_limit_bytes) .await?; let ModelsResponse { models } = serde_json::from_slice::(&body).map_err(|e| { @@ -207,6 +227,39 @@ mod tests { } } + #[test] + fn catalog_request_url_preserves_routing_and_encodes_queries() { + let mut provider = provider("https://gateway.example/v1"); + provider.query_params = Some(std::collections::HashMap::from([( + "api-version".to_string(), + "2026 09".to_string(), + )])); + let url = ModelsClient::::catalog_request_url( + &provider, + "https://catalog.example/codex/models?deployment=one", + "1.2.3", + ) + .unwrap(); + assert_eq!( + url, + "https://catalog.example/codex/models?deployment=one&api-version=2026+09&client_version=1.2.3" + ); + } + + #[test] + fn catalog_request_url_rejects_relative_urls() { + let provider = provider("https://gateway.example/v1"); + for catalog_url in ["/codex/models", "codex/models"] { + let error = ModelsClient::::catalog_request_url( + &provider, + catalog_url, + "1.2.3", + ) + .unwrap_err(); + assert!(matches!(error, ApiError::InvalidRequest { .. })); + } + } + #[tokio::test] async fn response_body_limit_survives_auth_retry_without_limiting_other_models_requests() { let transport = CapturingTransport::default(); @@ -217,7 +270,7 @@ mod tests { let client = ModelsClient::new(transport.clone(), provider, auth.clone()); client - .list_models_raw( + .list_models( request_url.clone(), HeaderMap::new(), /*response_body_limit_bytes*/ Some(64), @@ -233,7 +286,11 @@ mod tests { } let (models, _) = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect("ordinary request on the same client should remain unbounded"); assert!(models.is_empty()); @@ -268,7 +325,11 @@ mod tests { let client = ModelsClient::new(transport.clone(), provider, Arc::new(DummyAuth)); let (models, _) = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect("request should succeed"); @@ -327,7 +388,11 @@ mod tests { let client = ModelsClient::new(transport, provider, Arc::new(DummyAuth)); let (models, _) = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect("request should succeed"); @@ -352,7 +417,11 @@ mod tests { let client = ModelsClient::new(transport, provider, Arc::new(DummyAuth)); let (models, etag) = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect("request should succeed"); diff --git a/codex-rs/codex-api/tests/models_integration.rs b/codex-rs/codex-api/tests/models_integration.rs index 2227437618..ed43d8aaed 100644 --- a/codex-rs/codex-api/tests/models_integration.rs +++ b/codex-rs/codex-api/tests/models_integration.rs @@ -81,7 +81,11 @@ async fn invalid_models_response_reports_bounded_decode_metadata() { let client = ModelsClient::new(transport, provider, Arc::new(DummyAuth)); let error = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect_err("invalid catalog should fail to decode"); @@ -187,7 +191,11 @@ async fn models_client_hits_models_endpoint() { let client = ModelsClient::new(transport, provider, Arc::new(DummyAuth)); let (models, _) = client - .list_models(request_url, HeaderMap::new()) + .list_models( + request_url, + HeaderMap::new(), + /*response_body_limit_bytes*/ None, + ) .await .expect("models request should succeed"); diff --git a/codex-rs/config/src/thread_config.rs b/codex-rs/config/src/thread_config.rs index 654605c85a..61965e3431 100644 --- a/codex-rs/config/src/thread_config.rs +++ b/codex-rs/config/src/thread_config.rs @@ -298,6 +298,7 @@ mod tests { ModelProviderInfo { name: name.to_string(), base_url: Some("http://127.0.0.1:8061/api/codex".to_string()), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.proto b/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.proto index 5bcff76986..b9f89c95c6 100644 --- a/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.proto +++ b/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.proto @@ -49,6 +49,7 @@ message ModelProvider { bool requires_openai_auth = 16; bool supports_websockets = 17; bool supports_standalone_web_search = 18; + optional string model_catalog_url = 19; } message StringMap { diff --git a/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.rs b/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.rs index 05b1cf0eca..6ffc7bbdf6 100644 --- a/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.rs +++ b/codex-rs/config/src/thread_config/proto/codex.thread_config.v1.rs @@ -77,6 +77,8 @@ pub struct ModelProvider { pub supports_websockets: bool, #[prost(bool, tag = "18")] pub supports_standalone_web_search: bool, + #[prost(string, optional, tag = "19")] + pub model_catalog_url: ::core::option::Option<::prost::alloc::string::String>, } #[derive(Clone, PartialEq, ::prost::Message)] pub struct StringMap { diff --git a/codex-rs/config/src/thread_config/remote.rs b/codex-rs/config/src/thread_config/remote.rs index 8acb6e49f9..16a937b414 100644 --- a/codex-rs/config/src/thread_config/remote.rs +++ b/codex-rs/config/src/thread_config/remote.rs @@ -173,6 +173,7 @@ fn model_provider_from_proto( let info = ModelProviderInfo { name: provider.name, base_url: provider.base_url, + model_catalog_url: provider.model_catalog_url.map(Into::into), env_key: provider.env_key, env_key_instructions: provider.env_key_instructions, experimental_bearer_token: provider.experimental_bearer_token.map(Into::into), @@ -205,6 +206,7 @@ fn model_provider_to_proto( let ModelProviderInfo { name, base_url, + model_catalog_url, env_key, env_key_instructions, experimental_bearer_token, @@ -228,6 +230,7 @@ fn model_provider_to_proto( id: id.into(), name, base_url, + model_catalog_url: model_catalog_url.map(RedactedString::into_inner), env_key, env_key_instructions, experimental_bearer_token: experimental_bearer_token.map(RedactedString::into_inner), @@ -473,6 +476,9 @@ mod tests { id: "local".to_string(), name: "Local".to_string(), base_url: Some("http://127.0.0.1:8061/api/codex".to_string()), + model_catalog_url: Some( + "http://127.0.0.1:8061/api/codex/models".to_string(), + ), env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -543,6 +549,7 @@ mod tests { ModelProviderInfo { name: "Local".to_string(), base_url: Some("http://127.0.0.1:8061/api/codex".to_string()), + model_catalog_url: Some("http://127.0.0.1:8061/api/codex/models".into()), env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index df68076a7b..08f1ed5774 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2549,6 +2549,10 @@ "description": "Additional HTTP headers to include in requests to this provider where the (key, value) pairs are the header name and value.", "type": "object" }, + "model_catalog_url": { + "description": "Optional full URL for a Codex-native model catalog. When unset, OpenAI discovery uses the Codex backend unless `base_url` overrides the inference endpoint.", + "type": "string" + }, "name": { "default": "", "description": "Friendly display name.", diff --git a/codex-rs/core/tests/responses_headers.rs b/codex-rs/core/tests/responses_headers.rs index 3ce7110ac9..0b1b91dec3 100644 --- a/codex-rs/core/tests/responses_headers.rs +++ b/codex-rs/core/tests/responses_headers.rs @@ -74,6 +74,7 @@ async fn responses_stream_includes_subagent_header_on_review() { let provider = ModelProviderInfo { name: "mock".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -214,6 +215,7 @@ async fn responses_stream_includes_subagent_header_on_other() { let provider = ModelProviderInfo { name: "mock".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -335,6 +337,7 @@ async fn responses_respects_model_info_overrides_from_config() { let provider = ModelProviderInfo { name: "mock".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 4760c9d638..ce0ce86784 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1604,6 +1604,7 @@ async fn send_provider_auth_request(server: &MockServer, auth: ModelProviderAuth let provider = ModelProviderInfo { name: "corp".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -3105,6 +3106,7 @@ async fn azure_responses_request_does_not_store_and_preserves_prefixed_item_ids( let provider = ModelProviderInfo { name: "azure".into(), base_url: Some(format!("{}/openai", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -3733,6 +3735,7 @@ async fn azure_overrides_assign_properties_used_for_responses_url() { let provider = ModelProviderInfo { name: "custom".to_string(), base_url: Some(format!("{}/openai", server.uri())), + model_catalog_url: None, // Reuse the existing environment variable to avoid using unsafe code env_key: Some(EXISTING_ENV_VAR_WITH_NON_EMPTY_VALUE.to_string()), experimental_bearer_token: None, @@ -3818,6 +3821,7 @@ async fn env_var_overrides_loaded_auth() { let provider = ModelProviderInfo { name: ModelProviderInfo::create_openai_provider(/*base_url*/ None).name, base_url: Some(format!("{}/openai", server.uri())), + model_catalog_url: None, // Reuse the existing environment variable to avoid using unsafe code env_key: Some(EXISTING_ENV_VAR_WITH_NON_EMPTY_VALUE.to_string()), query_params: Some(std::collections::HashMap::from([( diff --git a/codex-rs/core/tests/suite/client_websockets.rs b/codex-rs/core/tests/suite/client_websockets.rs index c6a4daa55c..b9a0492e1a 100755 --- a/codex-rs/core/tests/suite/client_websockets.rs +++ b/codex-rs/core/tests/suite/client_websockets.rs @@ -2721,6 +2721,7 @@ fn websocket_provider_with_connect_timeout( ModelProviderInfo { name: "mock-ws".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/core/tests/suite/models_cache_auth.rs b/codex-rs/core/tests/suite/models_cache_auth.rs index 430236ce8f..4521237045 100644 --- a/codex-rs/core/tests/suite/models_cache_auth.rs +++ b/codex-rs/core/tests/suite/models_cache_auth.rs @@ -310,6 +310,11 @@ async fn auth_and_provider_switches_do_not_reuse_chatgpt_catalog() -> Result<()> .with_home(home.clone()) .with_auth(CodexAuth::from_api_key("api-key")) .with_config(|config| { + config.model_provider.model_catalog_url = config + .model_provider + .base_url + .as_ref() + .map(|base_url| format!("{base_url}/models").into()); config .features .enable(codex_features::Feature::ApiKeyModelDiscovery) diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index b5a652c5a9..dad188d70c 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -933,6 +933,11 @@ async fn remote_models_apply_legacy_instructions(auth: CodexAuth) -> Result<()> .features .enable(Feature::ApiKeyModelDiscovery) .expect("enable API-key model discovery"); + config.model_provider.model_catalog_url = config + .model_provider + .base_url + .as_ref() + .map(|base_url| format!("{base_url}/models").into()); config.update_plan_enabled = true; config.model = Some("gpt-5.2".to_string()); }); @@ -1478,3 +1483,64 @@ fn test_remote_model_with_policy( experimental_supported_tools: Vec::new(), } } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_catalog_url_supplies_conversation_model_and_instructions() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = MockServer::start().await; + let instructions = "Use the gateway's reviewed conversation instructions."; + let mut model = codex_models_manager::model_info::model_info_from_slug("gateway-conversation"); + model.visibility = ModelVisibility::List; + model.supported_in_api = true; + model.support_verbosity = false; + model.used_fallback_model_metadata = false; + model + .model_messages + .as_mut() + .expect("fallback instructions") + .instructions_template = Some(instructions.to_string()); + let catalog = ModelsResponse { + models: vec![model], + }; + Mock::given(method("GET")) + .and(path("/codex/models")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(&catalog)) + .expect(1) + .mount(&server) + .await; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + let catalog_url = format!("{}/codex/models", server.uri()); + let test = test_codex() + .with_auth(CodexAuth::from_api_key("gateway-api-key")) + .with_model("gateway-conversation") + .with_config(move |config| { + config.model_provider.name = "Gateway".to_string(); + config.model_provider.model_catalog_url = Some(catalog_url.into()); + config + .features + .enable(Feature::ApiKeyModelDiscovery) + .expect("enable API-key discovery"); + config.model_verbosity = Some(codex_protocol::config_types::Verbosity::High); + }) + .build_with_auto_env(&server) + .await?; + let received = test + .thread_manager + .get_models_manager() + .raw_model_catalog( + RefreshStrategy::OnlineIfUncached, + codex_core::test_support::default_http_client_factory(), + ) + .await; + assert_eq!(received.models, catalog.models); + test.submit_turn("hello").await?; + let request = response.single_request().body_json(); + assert_eq!(request["model"], "gateway-conversation"); + assert_eq!(request["instructions"], instructions); + assert_eq!(request["text"].get("verbosity"), None); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs b/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs index d7dcf56c0b..e46734522f 100644 --- a/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs +++ b/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs @@ -66,6 +66,7 @@ async fn continue_after_stream_error() { let provider = ModelProviderInfo { name: "mock-openai".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, env_key: Some("PATH".into()), env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/core/tests/suite/stream_no_completed.rs b/codex-rs/core/tests/suite/stream_no_completed.rs index e294c578c8..676b5a9b3b 100644 --- a/codex-rs/core/tests/suite/stream_no_completed.rs +++ b/codex-rs/core/tests/suite/stream_no_completed.rs @@ -48,6 +48,7 @@ async fn retries_on_early_close() { let model_provider = ModelProviderInfo { name: "openai".into(), base_url: Some(format!("{}/v1", server.uri())), + model_catalog_url: None, // Environment variable that should exist in the test environment. // ModelClient will return an error if the environment variable for the // provider is not set. diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/sampler/connection_pool.rs b/codex-rs/ext/guardian-v2/src/async_scorer/sampler/connection_pool.rs index 0358d1ecc2..8fa4227171 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/sampler/connection_pool.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/sampler/connection_pool.rs @@ -22,6 +22,7 @@ use codex_api::TransportError; use codex_api::build_session_headers; use codex_http_client::ClientRouteClass; use codex_login::CodexAuth; +use codex_login::default_client::ClientRedirectPolicy; use codex_login::default_client::add_originator_header; use codex_login::default_client::create_client_for_route_async; use codex_login::default_client::default_headers; @@ -216,6 +217,7 @@ impl ConnectionPool { self.config.http_client_factory.clone(), url, ClientRouteClass::Api, + ClientRedirectPolicy::Default, ) .await .map_err(|error| { diff --git a/codex-rs/http-client/src/client.rs b/codex-rs/http-client/src/client.rs index 9cda749aa2..420d5ad1b7 100644 --- a/codex-rs/http-client/src/client.rs +++ b/codex-rs/http-client/src/client.rs @@ -40,6 +40,12 @@ impl HttpClient { Self::from_parts(inner, RequestLogging::Disabled) } + /// Suppresses URL and response-header diagnostics while preserving this client's routing. + pub fn without_request_logging(mut self) -> Self { + self.request_logging = RequestLogging::Disabled; + self + } + pub(crate) fn from_parts(inner: reqwest::Client, request_logging: RequestLogging) -> Self { Self { inner, diff --git a/codex-rs/http-client/src/transport.rs b/codex-rs/http-client/src/transport.rs index ca351c1f00..13bddd0489 100644 --- a/codex-rs/http-client/src/transport.rs +++ b/codex-rs/http-client/src/transport.rs @@ -85,6 +85,7 @@ impl ReqwestTransport { } fn map_error(err: reqwest::Error) -> TransportError { + let err = err.without_url(); if err.is_connect() { TransportError::Connection(err.without_url()) } else if err.is_timeout() { diff --git a/codex-rs/http-client/src/transport_tests.rs b/codex-rs/http-client/src/transport_tests.rs index a8ab11aba5..07e49f97f8 100644 --- a/codex-rs/http-client/src/transport_tests.rs +++ b/codex-rs/http-client/src/transport_tests.rs @@ -18,10 +18,9 @@ async fn enabled_request_logging_emits_transport_url_and_body() { #[tokio::test] async fn disabled_request_logging_suppresses_transport_url_and_body() { - let logs = capture_transport_logs(HttpClient::new_without_request_logging( - test_reqwest_client(), - )) - .await; + let logs = + capture_transport_logs(HttpClient::new(test_reqwest_client()).without_request_logging()) + .await; assert!(logs.contains("log capture sentinel")); assert!(!logs.contains("url-secret")); diff --git a/codex-rs/login/src/auth/default_client.rs b/codex-rs/login/src/auth/default_client.rs index d26a82fbc7..a13421397f 100644 --- a/codex-rs/login/src/auth/default_client.rs +++ b/codex-rs/login/src/auth/default_client.rs @@ -273,6 +273,7 @@ pub async fn create_client_for_route_async( http_client_factory: HttpClientFactory, request_url: String, route_class: ClientRouteClass, + redirect_policy: ClientRedirectPolicy, ) -> std::io::Result { let permit = ROUTE_AWARE_CLIENT_BUILD_PERMIT .acquire() @@ -284,7 +285,7 @@ pub async fn create_client_for_route_async( &http_client_factory, &request_url, route_class, - ClientRedirectPolicy::Default, + redirect_policy, ) .map_err(std::io::Error::from) }) diff --git a/codex-rs/login/src/auth_env_telemetry.rs b/codex-rs/login/src/auth_env_telemetry.rs index c4dc6f07b8..b2d83bf175 100644 --- a/codex-rs/login/src/auth_env_telemetry.rs +++ b/codex-rs/login/src/auth_env_telemetry.rs @@ -61,6 +61,7 @@ mod tests { let provider = ModelProviderInfo { name: "Custom".to_string(), base_url: None, + model_catalog_url: None, env_key: Some("sk-should-not-leak".to_string()), env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/model-provider-info/src/lib.rs b/codex-rs/model-provider-info/src/lib.rs index 7d9e408983..3be93a6830 100644 --- a/codex-rs/model-provider-info/src/lib.rs +++ b/codex-rs/model-provider-info/src/lib.rs @@ -137,6 +137,9 @@ pub struct ModelProviderInfo { pub name: String, /// Base URL for the provider's OpenAI-compatible API. pub base_url: Option, + /// Optional full URL for a Codex-native model catalog. When unset, OpenAI discovery + /// uses the Codex backend unless `base_url` overrides the inference endpoint. + pub model_catalog_url: Option, /// Environment variable that stores the user's API key for this provider. pub env_key: Option, @@ -510,6 +513,7 @@ other non-default provider fields are not supported" ModelProviderInfo { name: OPENAI_PROVIDER_NAME.into(), base_url, + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -554,6 +558,7 @@ other non-default provider fields are not supported" // this is unset. A configured value is therefore unambiguously an // endpoint override. base_url: None, + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -734,6 +739,7 @@ pub fn create_oss_provider_with_base_url(base_url: &str, wire_api: WireApi) -> M ModelProviderInfo { name: "gpt-oss".into(), base_url: Some(base_url.into()), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, diff --git a/codex-rs/model-provider-info/src/model_provider_info_tests.rs b/codex-rs/model-provider-info/src/model_provider_info_tests.rs index a7bcaa45dc..c0c3971936 100644 --- a/codex-rs/model-provider-info/src/model_provider_info_tests.rs +++ b/codex-rs/model-provider-info/src/model_provider_info_tests.rs @@ -62,6 +62,7 @@ base_url = "http://localhost:11434/v1" let expected_provider = ModelProviderInfo { name: "Ollama".into(), base_url: Some("http://localhost:11434/v1".into()), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -96,6 +97,7 @@ query_params = { api-version = "2025-04-01-preview" } let expected_provider = ModelProviderInfo { name: "Azure".into(), base_url: Some("https://xxxxx.openai.azure.com/openai".into()), + model_catalog_url: None, env_key: Some("AZURE_OPENAI_API_KEY".into()), env_key_instructions: None, experimental_bearer_token: None, @@ -134,6 +136,7 @@ supports_standalone_web_search = true let expected_provider = ModelProviderInfo { name: "Example".into(), base_url: Some("https://example.com".into()), + model_catalog_url: None, env_key: Some("API_KEY".into()), env_key_instructions: None, experimental_bearer_token: None, @@ -317,6 +320,7 @@ fn test_create_amazon_bedrock_provider() { ModelProviderInfo { name: "Amazon Bedrock".to_string(), base_url: None, + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -814,3 +818,34 @@ refresh_interval_ms = 0 assert_eq!(auth.refresh_interval_ms, 0); assert_eq!(auth.refresh_interval(), None); } + +#[test] +fn model_catalog_url_deserializes_without_changing_inference_routing() { + let provider: ModelProviderInfo = toml::from_str( + r#" +name = "Gateway" +base_url = "https://gateway.example/v1" +model_catalog_url = "https://gateway.example/codex/catalog?token=catalog-secret" +"#, + ) + .unwrap(); + assert!(!format!("{provider:?}").contains("catalog-secret")); + assert_eq!( + provider, + ModelProviderInfo { + name: "Gateway".to_string(), + base_url: Some("https://gateway.example/v1".to_string()), + model_catalog_url: Some( + "https://gateway.example/codex/catalog?token=catalog-secret".into() + ), + ..ModelProviderInfo::default() + } + ); + assert_eq!( + provider + .to_api_provider(Some(AuthMode::ApiKey)) + .unwrap() + .base_url, + "https://gateway.example/v1" + ); +} diff --git a/codex-rs/model-provider/src/models_endpoint.rs b/codex-rs/model-provider/src/models_endpoint.rs index a063a90d14..93639407a9 100644 --- a/codex-rs/model-provider/src/models_endpoint.rs +++ b/codex-rs/model-provider/src/models_endpoint.rs @@ -20,6 +20,7 @@ use codex_login::AuthManager; use codex_login::CodexAuth; use codex_login::GatewayAuthManager; use codex_login::collect_auth_env_telemetry; +use codex_login::default_client::ClientRedirectPolicy; use codex_login::default_client::create_client_for_route_async; use codex_model_provider_info::CHATGPT_CODEX_BASE_URL; use codex_model_provider_info::ModelProviderInfo; @@ -41,6 +42,8 @@ use crate::combined_auth::compose_auth; const MODELS_REFRESH_TIMEOUT: Duration = Duration::from_secs(5); const MODELS_ENDPOINT: &str = "/models"; +// Bound downloads from explicitly configured catalogs before decoding or caching them. +const MAX_MODEL_CATALOG_BYTES: usize = 1024 * 1024; /// Provider-owned OpenAI-compatible `/models` endpoint. #[derive(Debug)] @@ -57,11 +60,16 @@ impl OpenAiModelsEndpoint { auth_manager: Option>, gateway_auth_manager: Option, String>>, ) -> Self { + let redirect_policy = if provider_info.model_catalog_url.is_some() { + ClientRedirectPolicy::Reject + } else { + ClientRedirectPolicy::Default + }; Self { provider_info, auth_manager, gateway_auth_manager, - transport_builder: Arc::new(RouteAwareModelsTransportBuilder), + transport_builder: Arc::new(RouteAwareModelsTransportBuilder { redirect_policy }), } } @@ -90,9 +98,10 @@ impl OpenAiModelsEndpoint { let identity = crate::models_identity::identity(&self.provider_info, auth.as_ref())?; let auth_mode = auth.as_ref().map(CodexAuth::auth_mode); let mut api_provider = self.provider_info.to_api_provider(auth_mode)?; - if auth.as_ref().is_some_and(CodexAuth::is_api_key_auth) + if (auth.as_ref().is_some_and(CodexAuth::is_api_key_auth) || self.has_provider_api_key()) && self.supports_api_key_models() && self.provider_info.base_url.is_none() + && self.provider_info.model_catalog_url.is_none() { // Codex metadata is served by the Codex backend, not the public /v1/models API. api_provider.base_url = CHATGPT_CODEX_BASE_URL.to_string(); @@ -104,8 +113,15 @@ impl OpenAiModelsEndpoint { ) .await?; let api_auth = resolved.auth; - let request_url = - ModelsClient::::request_url(&api_provider, client_version); + let request_url = match self.provider_info.model_catalog_url.as_deref() { + Some(catalog_url) => ModelsClient::::catalog_request_url( + &api_provider, + catalog_url, + client_version, + ) + .map_err(map_api_error)?, + None => ModelsClient::::request_url(&api_provider, client_version), + }; let auth_telemetry = auth_header_telemetry(api_auth.as_ref()); let agent_identity_telemetry = if let Some(CodexAuth::AgentIdentity(auth)) = auth.as_ref() { Some(agent_identity_telemetry(auth)) @@ -113,6 +129,7 @@ impl OpenAiModelsEndpoint { None }; let request_telemetry: Arc = Arc::new(ModelsRequestTelemetry { + include_response_debug: self.provider_info.model_catalog_url.is_none(), auth_mode: auth_mode.map(|mode| TelemetryAuthMode::from(mode).to_string()), auth_header_attached: auth_telemetry.attached, auth_header_name: auth_telemetry.name, @@ -126,10 +143,30 @@ impl OpenAiModelsEndpoint { .await?; let client = ModelsClient::new(transport, api_provider, api_auth) .with_telemetry(Some(request_telemetry)); + let response_body_limit_bytes = self + .provider_info + .model_catalog_url + .as_ref() + .map(|_| MAX_MODEL_CATALOG_BYTES); client - .list_models(request_url, HeaderMap::new()) + .list_models(request_url, HeaderMap::new(), response_body_limit_bytes) .await - .map_err(map_api_error) + .map_err(|mut error| { + if self.provider_info.model_catalog_url.is_some() + && let codex_api::ApiError::Transport(TransportError::Http { + url, + headers, + body, + .. + }) = &mut error + { + // Provider diagnostics may echo URL credentials or other secrets. + *url = None; + *headers = None; + *body = None; + } + map_api_error(error) + }) }) .await .map_err(|_| CodexErr::RequestTimeout)??; @@ -151,7 +188,13 @@ impl OpenAiModelsEndpoint { impl ModelsEndpointClient for OpenAiModelsEndpoint { fn supports_api_key_models(&self) -> bool { - self.provider_info.is_openai() + self.provider_info.model_catalog_url.is_some() + || (self.provider_info.is_openai() && self.provider_info.base_url.is_none()) + } + + fn has_provider_api_key(&self) -> bool { + self.provider_info.env_key.is_some() + || self.provider_info.experimental_bearer_token.is_some() } fn identity(&self) -> Option { @@ -198,7 +241,9 @@ trait ModelsTransportBuilder: fmt::Debug + Send + Sync { } #[derive(Debug)] -struct RouteAwareModelsTransportBuilder; +struct RouteAwareModelsTransportBuilder { + redirect_policy: ClientRedirectPolicy, +} impl ModelsTransportBuilder for RouteAwareModelsTransportBuilder { fn build( @@ -206,16 +251,27 @@ impl ModelsTransportBuilder for RouteAwareModelsTransportBuilder { http_client_factory: HttpClientFactory, request_url: String, ) -> ModelsTransportFuture<'_> { + let redirect_policy = self.redirect_policy; Box::pin(async move { - create_client_for_route_async(http_client_factory, request_url, ClientRouteClass::Api) - .await - .map(ReqwestTransport::from_http_client) + let client = create_client_for_route_async( + http_client_factory, + request_url, + ClientRouteClass::Api, + redirect_policy, + ) + .await?; + let client = match redirect_policy { + ClientRedirectPolicy::Default => client, + ClientRedirectPolicy::Reject => client.without_request_logging(), + }; + Ok(ReqwestTransport::from_http_client(client)) }) } } #[derive(Clone)] struct ModelsRequestTelemetry { + include_response_debug: bool, auth_mode: Option, auth_header_attached: bool, auth_header_name: Option<&'static str>, @@ -234,6 +290,7 @@ impl RequestTelemetry for ModelsRequestTelemetry { let success = status.is_some_and(|code| code.is_success()) && error.is_none(); let error_message = error.map(telemetry_transport_error_message); let response_debug = error + .filter(|_| self.include_response_debug) .map(extract_response_debug_context) .unwrap_or_default(); let status = status.map(|status| status.as_u16()); @@ -331,6 +388,8 @@ mod tests { use codex_models_manager::manager::RefreshStrategy; use codex_protocol::auth::AuthMode; use codex_protocol::config_types::ModelProviderAuthInfo; + use codex_protocol::error::CodexErrorDetails; + use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use pretty_assertions::assert_eq; use wiremock::Mock; @@ -390,7 +449,7 @@ mod tests { ( "OpenAI", Some("https://example.com/codex"), - Some("https://example.com/codex/models"), + None, "https://example.com/codex", ), ( @@ -434,6 +493,41 @@ mod tests { } } + #[tokio::test] + async fn provider_api_key_without_endpoint_overrides_uses_codex_backend() { + for auth in [ + None, + Some(CodexAuth::create_dummy_chatgpt_auth_for_testing()), + ] { + let capture = Arc::new(CaptureModelsUrl(Mutex::new(/*t*/ None))); + let auth_manager = auth.map(AuthManager::from_auth_for_testing); + let endpoint = Arc::new(OpenAiModelsEndpoint { + provider_info: ModelProviderInfo { + experimental_bearer_token: Some("provider-key".into()), + ..ModelProviderInfo::create_openai_provider(/*base_url*/ None) + }, + auth_manager: auth_manager.clone(), + gateway_auth_manager: None, + transport_builder: capture.clone(), + }); + let manager = OpenAiModelsManager::new_without_cache(endpoint, auth_manager); + manager.set_api_key_model_discovery_enabled(/*enabled*/ true); + manager + .raw_model_catalog( + RefreshStrategy::Online, + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + ) + .await; + assert_eq!( + *capture.0.lock().unwrap(), + Some(format!( + "{CHATGPT_CODEX_BASE_URL}/models?client_version={}", + codex_models_manager::client_version_to_whole(), + )) + ); + } + } + fn provider_info_with_command_auth() -> ModelProviderInfo { ModelProviderInfo { auth: Some(ModelProviderAuthInfo { @@ -671,4 +765,153 @@ mod tests { vec!["Bearer token-2", "Bearer token-5", "Bearer token-9"] ); } + + #[tokio::test] + async fn explicit_catalog_reuses_auth_headers_and_query_parameters() { + for auth in [ + CodexAuth::create_dummy_chatgpt_auth_for_testing(), + CodexAuth::from_api_key("test-key"), + ] { + let server = MockServer::start().await; + let mut model = + codex_models_manager::model_info::model_info_from_slug("provider-model"); + model.visibility = ModelVisibility::List; + model.supported_in_api = true; + model.used_fallback_model_metadata = false; + let expected = ModelsResponse { + models: vec![model], + }; + Mock::given(method("GET")) + .and(path("/codex/models")) + .and(header( + "authorization", + format!("Bearer {}", auth.get_token().unwrap()), + )) + .and(header("x-provider-header", "preserved")) + .and(query_param("deployment", "one")) + .and(query_param("api-version", "2026-09")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(&expected)) + .expect(1) + .mount(&server) + .await; + let auth_manager = AuthManager::from_auth_for_testing(auth); + // The parallel residency test must not change this catalog's cache identity. + let provider = ModelProviderInfo { + model_catalog_url: Some( + format!("{}/codex/models?deployment=one", server.uri()).into(), + ), + query_params: Some(std::collections::HashMap::from([( + "api-version".to_string(), + "2026-09".into(), + )])), + http_headers: Some(std::collections::HashMap::from([ + ("x-provider-header".to_string(), "preserved".into()), + ( + codex_login::default_client::RESIDENCY_HEADER_NAME.to_string(), + "us".into(), + ), + ])), + ..ModelProviderInfo::create_openai_provider(Some(format!("{}/v1", server.uri()))) + }; + let home = tempfile::tempdir().unwrap(); + let endpoint = Arc::new(OpenAiModelsEndpoint::new( + provider, + Some(auth_manager.clone()), + /*gateway_auth_manager*/ None, + )); + let manager = OpenAiModelsManager::new( + home.path().to_path_buf(), + endpoint, + Some(auth_manager.clone()), + ); + // SIWC does not require the API-key rollout flag. + manager.set_api_key_model_discovery_enabled( + auth_manager.auth_mode() == Some(codex_protocol::auth::AuthMode::ApiKey), + ); + for _ in 0..2 { + assert_eq!( + manager + .raw_model_catalog( + RefreshStrategy::OnlineIfUncached, + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault) + ) + .await, + expected + ); + } + } + } + + #[tokio::test] + async fn explicit_catalog_rejects_oversized_response() { + let server = MockServer::start().await; + let mut body = br#"{"models":[]}"#.to_vec(); + body.resize(MAX_MODEL_CATALOG_BYTES + 1, b' '); + Mock::given(method("GET")) + .and(path("/codex/models")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_bytes(body)) + .expect(1) + .mount(&server) + .await; + let endpoint = OpenAiModelsEndpoint::new( + ModelProviderInfo { + model_catalog_url: Some(format!("{}/codex/models", server.uri()).into()), + ..ModelProviderInfo::default() + }, + /*auth_manager*/ None, + /*gateway_auth_manager*/ None, + ); + let error = endpoint + .list_models( + "1.2.3", + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + ) + .await + .expect_err("oversized catalog should be rejected"); + let CodexErrorDetails::InvalidRequest(message) = error.details() else { + panic!("expected a response-size error, got {error:?}"); + }; + assert_eq!( + message, + &format!("response body exceeds the {MAX_MODEL_CATALOG_BYTES} byte limit") + ); + } + + #[tokio::test] + async fn explicit_catalog_rejects_redirects_without_forwarding_provider_credentials() { + let server = MockServer::start().await; + let destination = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/codex/models")) + .respond_with( + ResponseTemplate::new(/*s*/ 302) + .insert_header("location", format!("{}/codex/models", destination.uri())) + .set_body_string("catalog-secret"), + ) + .expect(1) + .mount(&server) + .await; + let endpoint = OpenAiModelsEndpoint::new( + ModelProviderInfo { + base_url: Some(server.uri()), + model_catalog_url: Some( + format!("{}/codex/models?token=catalog-secret", server.uri()).into(), + ), + experimental_bearer_token: Some("provider-key".into()), + ..ModelProviderInfo::default() + }, + /*auth_manager*/ None, + /*gateway_auth_manager*/ None, + ); + let error = endpoint + .list_models( + "1.2.3", + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + ) + .await + .expect_err("redirect should be rejected"); + assert!(!error.to_string().contains("catalog-secret")); + assert!(!format!("{error:?}").contains("catalog-secret")); + assert_eq!(destination.received_requests().await.unwrap().len(), 0); + } } diff --git a/codex-rs/model-provider/src/models_identity.rs b/codex-rs/model-provider/src/models_identity.rs index 73343d9df8..409ff83a07 100644 --- a/codex-rs/model-provider/src/models_identity.rs +++ b/codex-rs/model-provider/src/models_identity.rs @@ -14,7 +14,7 @@ pub(crate) fn identity( provider_info: &ModelProviderInfo, auth: Option<&CodexAuth>, ) -> CoreResult { - let mut provider = provider_info.to_api_provider(auth.map(CodexAuth::auth_mode))?; + let mut api_provider = provider_info.to_api_provider(auth.map(CodexAuth::auth_mode))?; let mut digest = Sha256::new(); // Length-prefix every field to avoid ambiguity between adjacent values. let mut field = |value: &[u8]| { @@ -22,9 +22,15 @@ pub(crate) fn identity( digest.update(value); }; field(b"models-cache-v1"); - field(provider.name.as_bytes()); - field(provider.base_url.as_bytes()); - let mut query: Vec<_> = provider.query_params.iter().flatten().collect(); + field(api_provider.name.as_bytes()); + field(api_provider.base_url.as_bytes()); + // Preserve existing cache identities when no explicit catalog is configured. + // The API provider resolves base_url defaults; model_catalog_url stays on ModelProviderInfo. + if let Some(catalog_url) = &provider_info.model_catalog_url { + field(b"model_catalog_url"); + field(catalog_url.as_bytes()); + } + let mut query: Vec<_> = api_provider.query_params.iter().flatten().collect(); query.sort(); field(&(query.len() as u64).to_le_bytes()); for (name, value) in query { @@ -63,9 +69,9 @@ pub(crate) fn identity( // different account, so only credentials with stable owner metadata can be reused. if !has_stable_account || explicit_bearer { let headers = resolve_provider_auth(auth, provider_info)?.to_auth_headers(); - provider.headers.extend(headers); + api_provider.headers.extend(headers); } - let mut headers: Vec<_> = provider.headers.iter().collect(); + let mut headers: Vec<_> = api_provider.headers.iter().collect(); headers.sort_by(|(a, av), (b, bv)| { a.as_str() .cmp(b.as_str()) diff --git a/codex-rs/model-provider/src/models_identity_tests.rs b/codex-rs/model-provider/src/models_identity_tests.rs index 4732265de1..16f88b41ae 100644 --- a/codex-rs/model-provider/src/models_identity_tests.rs +++ b/codex-rs/model-provider/src/models_identity_tests.rs @@ -87,3 +87,17 @@ fn cache_identity_distinguishes_auth_requirements_and_unknown_plans() { provider.requires_openai_auth = false; assert_ne!(key, identity(&provider, Some(&first)).unwrap()); } + +#[test] +fn cache_identity_tracks_catalog_url() { + let auth = CodexAuth::from_api_key("test-key"); + let mut provider = + ModelProviderInfo::create_openai_provider(Some("https://gateway.example/v1".into())); + let bundled = identity(&provider, Some(&auth)).unwrap(); + provider.model_catalog_url = Some("https://gateway.example/catalog-one".into()); + let first = identity(&provider, Some(&auth)).unwrap(); + provider.model_catalog_url = Some("https://gateway.example/catalog-two".into()); + let second = identity(&provider, Some(&auth)).unwrap(); + assert_ne!(bundled, first); + assert_ne!(first, second); +} diff --git a/codex-rs/model-provider/src/provider.rs b/codex-rs/model-provider/src/provider.rs index 5f7bd4b035..08afa3e56f 100644 --- a/codex-rs/model-provider/src/provider.rs +++ b/codex-rs/model-provider/src/provider.rs @@ -679,6 +679,7 @@ mod tests { ModelProviderInfo { name: "mock".into(), base_url: Some(base_url), + model_catalog_url: None, env_key: None, env_key_instructions: None, experimental_bearer_token: None, @@ -1380,33 +1381,50 @@ printf '%s\n' '{"AccessKeyId":"exported","SecretAccessKey":"secret"}' models: remote_models.clone(), }), ) - .expect(1) + .expect(2) .mount(&server) .await; let mut provider_info = provider_for(server.uri()); provider_info.experimental_bearer_token = Some("provider-token".into()); - let provider = create_model_provider( - provider_info, - Some(AuthManager::from_auth_for_testing( - CodexAuth::create_dummy_chatgpt_auth_for_testing(), - )), - ); + provider_info.model_catalog_url = Some(format!("{}/models", server.uri()).into()); + provider_info.http_headers = Some(std::collections::HashMap::from([( + codex_login::default_client::RESIDENCY_HEADER_NAME.to_string(), + "us".into(), + )])); + for auth in [ + None, + Some(CodexAuth::create_dummy_chatgpt_auth_for_testing()), + ] { + // Disabled discovery must ignore the catalog cached by the enabled run. + for enabled in [true, false] { + let provider = create_model_provider( + provider_info.clone(), + auth.clone().map(AuthManager::from_auth_for_testing), + ); + let manager = + provider.models_manager(test_codex_home(), /*config_model_catalog*/ None); + manager.set_api_key_model_discovery_enabled(enabled); + let refresh_strategy = if enabled { + RefreshStrategy::Online + } else { + RefreshStrategy::Offline + }; + let catalog = manager + .raw_model_catalog( + refresh_strategy, + HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), + ) + .await; - let manager = - provider.models_manager(test_codex_home(), /*config_model_catalog*/ None); - let catalog = manager - .raw_model_catalog( - RefreshStrategy::Online, - HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault), - ) - .await; - - assert!( - catalog - .models - .iter() - .any(|model| model.slug == "provider-model") - ); + assert_eq!( + catalog + .models + .iter() + .any(|model| model.slug == "provider-model"), + enabled + ); + } + } } } diff --git a/codex-rs/model-provider/src/shared_state_tests.rs b/codex-rs/model-provider/src/shared_state_tests.rs index 2bcb05bde6..09caa4cf87 100644 --- a/codex-rs/model-provider/src/shared_state_tests.rs +++ b/codex-rs/model-provider/src/shared_state_tests.rs @@ -271,6 +271,7 @@ async fn models_cache_is_reused_only_for_matching_gateway_configuration() { home.path().to_path_buf(), ); let mut info = ModelProviderInfo { + model_catalog_url: Some(format!("{}/models", server.uri()).into()), http_headers: Some(std::collections::HashMap::from([( codex_login::default_client::RESIDENCY_HEADER_NAME.to_string(), "us".into(), @@ -354,6 +355,7 @@ async fn models_observe_gateway_rotation(instance: ProviderInstance) { home.path().to_path_buf(), ); let info = ModelProviderInfo { + model_catalog_url: Some(format!("{}/models", server.uri()).into()), gateway_oauth: Some(GatewayOAuthConfig { authorization_url: format!("{}/authorize", server.uri()), token_url: format!("{}/token", server.uri()), diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 88151f7735..7b4d613be4 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -54,6 +54,12 @@ pub trait ModelsEndpointClient: fmt::Debug + Send + Sync { false } + /// Returns whether explicit provider configuration supplies API-key authentication. + /// This takes precedence over any unrelated first-party login used by the picker. + fn has_provider_api_key(&self) -> bool { + false + } + /// Fetches the latest remote model catalog and optional ETag. fn list_models<'a>( &'a self, @@ -476,9 +482,13 @@ impl OpenAiModelsManager { refresh_strategy: RefreshStrategy, http_client_factory: &HttpClientFactory, ) -> CoreResult<()> { - // Gate cache loading as well as requests; disabled sessions use bundled API-key models. - if self.supports_api_key_discovery() - && !self.api_key_model_discovery_enabled.load(Ordering::SeqCst) + // API-key discovery must be enabled and supported before reusing a remote catalog. + // Otherwise even a matching cache from an earlier run would bypass bundled-only behavior. + // Command-auth providers retain their existing discovery behavior. + if self.uses_api_key_auth() + && !self.endpoint_client.has_command_auth() + && (!self.endpoint_client.supports_api_key_models() + || !self.api_key_model_discovery_enabled.load(Ordering::SeqCst)) { return Ok(()); } @@ -541,7 +551,12 @@ impl OpenAiModelsManager { fn supports_api_key_discovery(&self) -> bool { self.endpoint_client.supports_api_key_models() && !self.endpoint_client.has_command_auth() - && self + && self.uses_api_key_auth() + } + + fn uses_api_key_auth(&self) -> bool { + self.endpoint_client.has_provider_api_key() + || self .auth_manager .as_ref() .is_some_and(|auth_manager| auth_manager.auth_mode() == Some(AuthMode::ApiKey))