From 458f7046a52232f5c01dd8bc65dfbce8d5e79fa8 Mon Sep 17 00:00:00 2001 From: andrewgu-oai Date: Tue, 29 Sep 2026 02:15:00 +0000 Subject: [PATCH] Treat explicit provider model catalogs as authoritative (#49135) ## Why Providers with `model_catalog_url` could advertise bundled models absent from their catalog or reuse stale models after a failed refresh. Model metadata could also match by prefix or namespaced suffix instead of the provider's exact model ID. ## What changed - Suppress bundled model fallback and merging for explicit provider catalogs, including when discovery is disabled. - Require unique, non-empty catalog slugs and match metadata by exact model ID. - Serialize explicit catalog refreshes and invalidate in-memory and persisted catalog entries after fetch failures, preventing stale cache reuse. - Report a clear configuration error when startup has no available model, while allowing an explicitly configured `model`. ## Testing Add regression coverage for exact model matching, hidden catalog entries, cache invalidation across restart and recovery, invalid slugs, and empty-catalog startup with and without an explicit model. Update app-server model-list expectations and verify the TUI configuration error. GitOrigin-RevId: ac323a94178e3c48df8d4a159a86fdbac20b03a2 --- .../app-server/tests/suite/v2/model_list.rs | 10 +- codex-rs/core/src/session/mod.rs | 5 + codex-rs/core/tests/suite/remote_models.rs | 46 ++++++ .../model-provider/src/models_endpoint.rs | 56 ++++++- codex-rs/model-provider/src/provider.rs | 96 ++++++------ .../src/api_key_discovery_tests.rs | 7 +- codex-rs/models-manager/src/cache.rs | 3 + codex-rs/models-manager/src/manager.rs | 138 ++++++++++++++++-- codex-rs/models-manager/src/manager_tests.rs | 124 +++++++++++++--- codex-rs/tui/src/app_server_session.rs | 21 ++- 10 files changed, 409 insertions(+), 97 deletions(-) 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 5d26c68e78..d21bebfc37 100644 --- a/codex-rs/app-server/tests/suite/v2/model_list.rs +++ b/codex-rs/app-server/tests/suite/v2/model_list.rs @@ -131,7 +131,15 @@ requires_openai_auth = true }) .await?; let enabled = user_enablement.unwrap_or(app_enablement) && catalog_opt_in; - let expected = if enabled { &remote } else { &bundled }; + let expected: &[ModelPreset] = if enabled { + &remote + } else if catalog_opt_in { + // The rollout gate suppresses discovery, but an explicit catalog still + // prevents bundled models from being advertised for this provider. + &[] + } else { + &bundled + }; assert_eq!( response, ModelListResponse { diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 902d18377c..e08a0fa5a8 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -659,6 +659,11 @@ impl Session { config.http_client_factory(), ) .await; + if model.trim().is_empty() { + return Err(CodexErr::InvalidRequest( + "No models are available. Set `model` explicitly or check your model catalog configuration.".to_string(), + )); + } let trusted_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source) && !matches!(conversation_history, InitialHistory::Resumed(_)); if config diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index dad188d70c..17ef6b3297 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -946,6 +946,7 @@ async fn remote_models_apply_legacy_instructions(auth: CodexAuth) -> Result<()> cwd, config, thread_manager, + home: _home, .. } = builder.build(&server).await?; @@ -1484,6 +1485,51 @@ fn test_remote_model_with_policy( } } +#[test_case(None; "without explicit model")] +#[test_case(Some("gateway-conversation"); "with explicit model")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn empty_model_catalog_requires_explicit_model(configured_model: Option<&str>) -> Result<()> { + skip_if_no_network!(Ok(())); + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/codex/models")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(ModelsResponse::default())) + .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 model = configured_model.map(str::to_string); + let result = test_codex() + .with_auth(CodexAuth::from_api_key("gateway-api-key")) + .with_config(move |config| { + config.model = model; + config.model_provider.model_catalog_url = Some(catalog_url.into()); + config + .features + .enable(Feature::ApiKeyModelDiscovery) + .expect("enable API-key discovery"); + }) + .build_with_auto_env(&server) + .await; + if let Some(model) = configured_model { + let test = result?; + test.submit_turn("hello").await?; + assert_eq!(response.single_request().body_json()["model"], model); + } else { + let error = result.err().expect("startup requires a model"); + assert_eq!( + error.to_string(), + "No models are available. Set `model` explicitly or check your model catalog configuration." + ); + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn model_catalog_url_supplies_conversation_model_and_instructions() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/model-provider/src/models_endpoint.rs b/codex-rs/model-provider/src/models_endpoint.rs index 0fc8a0c8c3..6986451a8c 100644 --- a/codex-rs/model-provider/src/models_endpoint.rs +++ b/codex-rs/model-provider/src/models_endpoint.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::fmt; use std::future::Future; use std::pin::Pin; @@ -181,6 +182,17 @@ impl OpenAiModelsEndpoint { }) .await .map_err(|_| CodexErr::RequestTimeout)??; + if self.provider_info.model_catalog_url.is_some() { + let mut slugs = HashSet::new(); + if models + .iter() + .any(|model| model.slug.trim().is_empty() || !slugs.insert(&model.slug)) + { + return Err(CodexErr::InvalidRequest( + "model catalog must contain unique, non-empty model slugs".to_string(), + )); + } + } Ok(ModelsEndpointResponse { models, etag, @@ -397,6 +409,7 @@ mod tests { use codex_models_manager::manager::ModelsManager; use codex_models_manager::manager::OpenAiModelsManager; use codex_models_manager::manager::RefreshStrategy; + use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::auth::AuthMode; use codex_protocol::config_types::ModelProviderAuthInfo; use codex_protocol::error::CodexErrorDetails; @@ -701,7 +714,7 @@ mod tests { .unwrap(); let model = codex_protocol::openai_models::ModelInfo { used_fallback_model_metadata: false, - ..codex_models_manager::model_info::model_info_from_slug("command-auth-model") + ..model_info_from_slug("command-auth-model") }; Mock::given(method("GET")) .and(path("/models")) @@ -784,8 +797,7 @@ mod tests { 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"); + let mut model = model_info_from_slug("provider-model"); model.visibility = ModelVisibility::List; model.supported_in_api = true; model.used_fallback_model_metadata = false; @@ -834,7 +846,8 @@ mod tests { home.path().to_path_buf(), endpoint, Some(auth_manager.clone()), - ); + ) + .with_provider_catalog(); // 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), @@ -853,6 +866,41 @@ mod tests { } } + #[tokio::test] + async fn explicit_catalog_requires_unique_nonempty_slugs() { + for slugs in [vec![" \t"], vec!["private-model", "private-model"]] { + let server = MockServer::start().await; + let catalog = ModelsResponse { + models: slugs.into_iter().map(model_info_from_slug).collect(), + }; + Mock::given(method("GET")) + .and(path("/codex/models")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(catalog)) + .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("ambiguous model identity must be rejected"); + assert_eq!( + error.to_string(), + "model catalog must contain unique, non-empty model slugs" + ); + } + } + #[tokio::test] async fn explicit_catalog_rejects_oversized_response() { let server = MockServer::start().await; diff --git a/codex-rs/model-provider/src/provider.rs b/codex-rs/model-provider/src/provider.rs index fa30479f7f..b783184cd1 100644 --- a/codex-rs/model-provider/src/provider.rs +++ b/codex-rs/model-provider/src/provider.rs @@ -399,6 +399,12 @@ struct ConfiguredModelProvider { gateway_auth_manager: Option, String>>, } +enum ModelsCacheConfig { + Disk { codex_home: PathBuf }, + Disabled, + Custom(Arc), +} + impl ConfiguredModelProvider { fn new( info: ModelProviderInfo, @@ -411,6 +417,40 @@ impl ConfiguredModelProvider { gateway_auth_manager, } } + + fn create_models_manager( + &self, + config_model_catalog: Option, + cache: ModelsCacheConfig, + ) -> SharedModelsManager { + if let Some(model_catalog) = config_model_catalog { + return Arc::new(StaticModelsManager::new( + self.auth_manager.clone(), + model_catalog, + )); + } + let endpoint = Arc::new(OpenAiModelsEndpoint::new( + self.info.clone(), + self.auth_manager.clone(), + self.gateway_auth_manager.clone(), + )); + let auth_manager = self.auth_manager.clone(); + let manager = match cache { + ModelsCacheConfig::Disk { codex_home } => { + OpenAiModelsManager::new(codex_home, endpoint, auth_manager) + } + ModelsCacheConfig::Disabled => { + OpenAiModelsManager::new_without_cache(endpoint, auth_manager) + } + ModelsCacheConfig::Custom(cache) => { + OpenAiModelsManager::new_with_cache(cache, endpoint, auth_manager) + } + }; + match &self.info.model_catalog_url { + Some(_) => Arc::new(manager.with_provider_catalog()), + None => Arc::new(manager), + } + } } impl ModelProvider for ConfiguredModelProvider { @@ -559,47 +599,14 @@ impl ModelProvider for ConfiguredModelProvider { codex_home: PathBuf, config_model_catalog: Option, ) -> SharedModelsManager { - match config_model_catalog { - Some(model_catalog) => Arc::new(StaticModelsManager::new( - self.auth_manager.clone(), - model_catalog, - )), - None => { - let endpoint = Arc::new(OpenAiModelsEndpoint::new( - self.info.clone(), - self.auth_manager.clone(), - self.gateway_auth_manager.clone(), - )); - Arc::new(OpenAiModelsManager::new( - codex_home, - endpoint, - self.auth_manager.clone(), - )) - } - } + self.create_models_manager(config_model_catalog, ModelsCacheConfig::Disk { codex_home }) } fn models_manager_without_cache( &self, config_model_catalog: Option, ) -> SharedModelsManager { - match config_model_catalog { - Some(model_catalog) => Arc::new(StaticModelsManager::new( - self.auth_manager.clone(), - model_catalog, - )), - None => { - let endpoint = Arc::new(OpenAiModelsEndpoint::new( - self.info.clone(), - self.auth_manager.clone(), - self.gateway_auth_manager.clone(), - )); - Arc::new(OpenAiModelsManager::new_without_cache( - endpoint, - self.auth_manager.clone(), - )) - } - } + self.create_models_manager(config_model_catalog, ModelsCacheConfig::Disabled) } fn models_manager_with_cache( @@ -607,24 +614,7 @@ impl ModelProvider for ConfiguredModelProvider { config_model_catalog: Option, cache: Arc, ) -> SharedModelsManager { - match config_model_catalog { - Some(model_catalog) => Arc::new(StaticModelsManager::new( - self.auth_manager.clone(), - model_catalog, - )), - None => { - let endpoint = Arc::new(OpenAiModelsEndpoint::new( - self.info.clone(), - self.auth_manager.clone(), - self.gateway_auth_manager.clone(), - )); - Arc::new(OpenAiModelsManager::new_with_cache( - cache, - endpoint, - self.auth_manager.clone(), - )) - } - } + self.create_models_manager(config_model_catalog, ModelsCacheConfig::Custom(cache)) } } diff --git a/codex-rs/models-manager/src/api_key_discovery_tests.rs b/codex-rs/models-manager/src/api_key_discovery_tests.rs index 6cda6088f4..e818a0c6ef 100644 --- a/codex-rs/models-manager/src/api_key_discovery_tests.rs +++ b/codex-rs/models-manager/src/api_key_discovery_tests.rs @@ -12,11 +12,8 @@ async fn api_key_discovery_disabled_preserves_command_auth_discovery_and_merging )]; let endpoint = Arc::new(TestModelsEndpoint { has_command_auth: true, - uses_codex_backend: false, - responses: Mutex::new(vec![models.clone()].into()), - etag: None, - fetch_count: AtomicUsize::new(0), - observed_proxy_policy: Mutex::new(None), + responses: Mutex::new(vec![Ok(models.clone())].into()), + ..TestModelsEndpoint::default() }); let manager = OpenAiModelsManager::new_without_cache( endpoint.clone(), diff --git a/codex-rs/models-manager/src/cache.rs b/codex-rs/models-manager/src/cache.rs index 14d08977f4..7a0890760a 100644 --- a/codex-rs/models-manager/src/cache.rs +++ b/codex-rs/models-manager/src/cache.rs @@ -64,6 +64,9 @@ pub type ModelsCacheFuture<'a, T> = Pin + Send + 'a>> #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ModelsCacheEntry { /// Time when the catalog was fetched or last revalidated with the models endpoint. + /// + /// `DateTime::::MIN_UTC` marks an invalidated catalog, both in memory and in storage. + /// The manager rejects it even if the backend uses native TTL. pub fetched_at: DateTime, /// Endpoint ETag used to revalidate the catalog without downloading an unchanged payload. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 7b4d613be4..af734443c0 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -4,6 +4,7 @@ use crate::cache::ModelsCacheEntry; use crate::collaboration_mode_presets::builtin_collaboration_mode_presets; use crate::config::ModelsManagerConfig; use crate::model_info; +use chrono::DateTime; use chrono::Utc; use codex_http_client::HttpClientFactory; use codex_login::AuthManager; @@ -22,6 +23,8 @@ use std::sync::Arc; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use std::time::Duration; +use tokio::sync::Mutex; +use tokio::sync::MutexGuard; use tokio::sync::RwLock; use tokio::sync::TryLockError; use tracing::Instrument as _; @@ -145,7 +148,7 @@ pub trait ModelsManager: fmt::Debug + Send + Sync { ) -> ModelsManagerFuture<'_, ModelsResponse>; /// Best-effort refresh when the in-memory catalog belongs to different credentials. - /// Static catalogs need no refresh. Failures leave the existing cache/default fallback. + /// Static catalogs need no refresh. Failures follow the catalog's fallback policy. fn refresh_after_auth_change( &self, _http_client_factory: HttpClientFactory, @@ -255,12 +258,37 @@ pub type ModelsManagerFuture<'a, T> = Pin + Send + 'a /// Shared model manager handle used across runtime services. pub type SharedModelsManager = Arc; +/// Only explicit catalogs serialize refresh and suppress bundled fallback. Auth and discovery +/// settings are still evaluated when models are requested. +#[derive(Debug)] +enum CatalogSource { + Default, + ExplicitProvider(Mutex<()>), +} + +impl CatalogSource { + fn fallback_models(&self) -> Option> { + match self { + Self::Default => Some(load_remote_models_from_file().unwrap_or_default()), + Self::ExplicitProvider(_) => None, + } + } + + async fn refresh_lock(&self) -> Option> { + match self { + Self::Default => None, + Self::ExplicitProvider(lock) => Some(lock.lock().await), + } + } +} + /// OpenAI-compatible model manager backed by bundled models, cache, and `/models`. #[derive(Debug)] pub struct OpenAiModelsManager { remote_models: RwLock, cache: Option>, endpoint_client: SharedModelsEndpointClient, + catalog_source: CatalogSource, api_key_model_discovery_enabled: AtomicBool, auth_manager: Option>, } @@ -315,21 +343,29 @@ impl OpenAiModelsManager { endpoint_client: Arc, auth_manager: Option>, ) -> Self { - let remote_models = load_remote_models_from_file().unwrap_or_default(); Self { remote_models: RwLock::new(ModelsCacheEntry { fetched_at: Utc::now(), etag: None, client_version: Some(crate::client_version_to_whole()), identity: endpoint_client.identity(), - models: remote_models, + models: load_remote_models_from_file().unwrap_or_default(), }), cache, api_key_model_discovery_enabled: AtomicBool::new(false), endpoint_client, + catalog_source: CatalogSource::Default, auth_manager, } } + + /// Configure a newly constructed manager to use only the provider's explicit catalog. + /// Clear the bundled seed before sharing it; the existing auth identity must be retained. + pub fn with_provider_catalog(mut self) -> Self { + self.catalog_source = CatalogSource::ExplicitProvider(Mutex::new(())); + self.remote_models.get_mut().models.clear(); + self + } } impl StaticModelsManager { @@ -396,7 +432,7 @@ impl ModelsManager for OpenAiModelsManager { if entry.identity.is_some() && entry.identity == self.endpoint_client.identity() { entry.models.clone() } else { - load_remote_models_from_file().unwrap_or_default() + self.catalog_source.fallback_models().unwrap_or_default() } }) } @@ -407,7 +443,7 @@ impl ModelsManager for OpenAiModelsManager { if entry.identity.is_some() && entry.identity == self.endpoint_client.identity() { entry.models.clone() } else { - load_remote_models_from_file().unwrap_or_default() + self.catalog_source.fallback_models().unwrap_or_default() }, ) } @@ -416,6 +452,27 @@ impl ModelsManager for OpenAiModelsManager { self.auth_manager.as_deref() } + #[tracing::instrument(skip_all, fields(model))] + fn get_model_info<'a>( + &'a self, + model: &'a str, + config: &'a ModelsManagerConfig, + ) -> ModelsManagerFuture<'a, ModelInfo> { + Box::pin(async move { + let models = self.get_remote_models().await; + match &self.catalog_source { + CatalogSource::Default => { + construct_model_info_from_candidates(model, &models, config) + } + CatalogSource::ExplicitProvider(_) => construct_model_info( + model, + models.into_iter().find(|candidate| candidate.slug == model), + config, + ), + } + }) + } + fn list_collaboration_modes(&self) -> Vec { builtin_collaboration_mode_presets() } @@ -451,6 +508,7 @@ impl OpenAiModelsManager { } async fn refresh_if_new_etag(&self, etag: String, http_client_factory: HttpClientFactory) { + let refresh_guard = self.catalog_source.refresh_lock().await; let (identity, current_etag) = { let entry = self.remote_models.read().await; (entry.identity.clone(), entry.etag.clone()) @@ -468,6 +526,7 @@ impl OpenAiModelsManager { } return; } + drop(refresh_guard); if let Err(err) = self .refresh_available_models(RefreshStrategy::Online, &http_client_factory) .await @@ -482,6 +541,7 @@ impl OpenAiModelsManager { refresh_strategy: RefreshStrategy, http_client_factory: &HttpClientFactory, ) -> CoreResult<()> { + let _refresh_guard = self.catalog_source.refresh_lock().await; // 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. @@ -521,14 +581,54 @@ impl OpenAiModelsManager { http_client_factory: &HttpClientFactory, ) -> CoreResult<()> { let client_version = crate::client_version_to_whole(); + let request_identity = self.endpoint_client.identity(); let ModelsEndpointResponse { models, etag, identity, - } = self + } = match self .endpoint_client .list_models(&client_version, http_client_factory.clone()) - .await?; + .await + { + Ok(response) => response, + Err(error) => { + if matches!(&self.catalog_source, CatalogSource::ExplicitProvider(_)) { + { + let mut current = self.remote_models.write().await; + if request_identity != self.endpoint_client.identity() { + return Err(error); + } + // Keep this failure in memory even if persistent invalidation fails. + current.fetched_at = DateTime::::MIN_UTC; + current.models.clear(); + current.etag = None; + current.identity.clone_from(&request_identity); + } + if let Some(identity) = request_identity + && let Some(cache) = self.cache.as_ref() + { + let invalidation = async { + if let Some(mut entry) = cache.load(&client_version).await? + && entry.identity.as_deref() == Some(&identity) + && entry.client_version.as_deref() == Some(&client_version) + { + // Preserve backend lookup keys, but force a cache miss after restart. + entry.fetched_at = DateTime::::MIN_UTC; + entry.etag = None; + entry.models.clear(); + cache.store(&entry).await?; + } + Ok::<_, crate::cache::ModelsCacheError>(()) + }; + if let Err(err) = invalidation.await { + error!("failed to invalidate models cache: {err}"); + } + } + } + return Err(error); + } + }; if Some(&identity) != self.endpoint_client.identity().as_ref() { return Ok(()); } @@ -574,7 +674,6 @@ impl OpenAiModelsManager { if entry.identity != self.endpoint_client.identity() { return false; } - // Visible ChatGPT and OpenAI API-key catalogs are authoritative. let remote_only = entry .models .iter() @@ -585,8 +684,7 @@ impl OpenAiModelsManager { .auth_mode() .is_some_and(AuthMode::has_chatgpt_account) })); - if !remote_only { - let mut models = load_remote_models_from_file().unwrap_or_default(); + if !remote_only && let Some(mut models) = self.catalog_source.fallback_models() { for model in entry.models { if let Some(index) = models .iter() @@ -615,6 +713,15 @@ impl OpenAiModelsManager { let Some(identity) = self.endpoint_client.identity() else { return false; }; + { + let current = self.remote_models.read().await; + // A failed fetch must not be replaced by stale cache data even if invalidation failed. + if current.fetched_at == DateTime::::MIN_UTC + && current.identity.as_ref() == Some(&identity) + { + return false; + } + } let cache_entry = match cache.load(&client_version).await { Ok(Some(cache_entry)) => cache_entry, Ok(None) => { @@ -626,6 +733,9 @@ impl OpenAiModelsManager { return false; } }; + if cache_entry.fetched_at == DateTime::::MIN_UTC { + return false; + } if cache_entry.client_version.as_deref() != Some(client_version.as_str()) { info!( expected_version = client_version, @@ -788,6 +898,14 @@ pub(crate) fn construct_model_info_from_candidates( // retry for namespaced slugs like `custom/gpt-5.3-codex`. let remote = find_model_by_longest_prefix(model, candidates) .or_else(|| find_model_by_namespaced_suffix(model, candidates)); + construct_model_info(model, remote, config) +} + +fn construct_model_info( + model: &str, + remote: Option, + config: &ModelsManagerConfig, +) -> ModelInfo { let model_info = if let Some(remote) = remote { ModelInfo { slug: model.to_string(), diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index 4ef297142a..04336f5665 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -16,6 +16,7 @@ use codex_login::ExternalAuth; use codex_login::ExternalAuthRefreshContext; use codex_login::TokenData; use codex_protocol::auth::AuthMode; +use codex_protocol::error::CodexErr; use codex_protocol::openai_models::ModelAccessPrograms; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::turn_input::CyberAccessProgram; @@ -92,17 +93,17 @@ fn assert_models_contain(actual: &[ModelInfo], expected: &[ModelInfo]) { } } -#[derive(Debug)] +#[derive(Debug, Default)] struct TestModelsEndpoint { has_command_auth: bool, uses_codex_backend: bool, - responses: Mutex>>, + responses: Mutex>>>, etag: Option, fetch_count: AtomicUsize, observed_proxy_policy: Mutex>, } -#[derive(Debug)] +#[derive(Debug, Default)] struct TestModelsCache { entry: Mutex>, load_error: bool, @@ -162,6 +163,7 @@ impl ModelsCache for TestModelsCache { if self.store_error { return Err(ModelsCacheError::new("test store failure")); } + *self.entry.lock().unwrap() = Some(entry.clone()); self.stored_entries .lock() .expect("stored entries lock should not be poisoned") @@ -196,23 +198,24 @@ impl ModelsCache for TestModelsCache { impl TestModelsEndpoint { fn new(responses: Vec>) -> Arc { Arc::new(Self { - has_command_auth: false, uses_codex_backend: true, - responses: Mutex::new(responses.into()), - etag: None, - fetch_count: AtomicUsize::new(0), - observed_proxy_policy: Mutex::new(None), + responses: Mutex::new(responses.into_iter().map(Ok).collect()), + ..Self::default() }) } fn without_refresh(responses: Vec>) -> Arc { Arc::new(Self { - has_command_auth: false, - uses_codex_backend: false, + responses: Mutex::new(responses.into_iter().map(Ok).collect()), + ..Self::default() + }) + } + + fn with_command_auth(responses: Vec>>) -> Arc { + Arc::new(Self { + has_command_auth: true, responses: Mutex::new(responses.into()), - etag: None, - fetch_count: AtomicUsize::new(0), - observed_proxy_policy: Mutex::new(None), + ..Self::default() }) } @@ -234,7 +237,7 @@ impl TestModelsEndpoint { .lock() .expect("responses lock should not be poisoned") .pop_front() - .unwrap_or_default(); + .unwrap_or_else(|| Ok(Vec::new()))?; Ok(ModelsEndpointResponse { models, etag: self.etag.clone(), @@ -1051,11 +1054,8 @@ async fn refresh_available_models_keeps_merging_for_custom_api_auth() { let codex_home = tempdir().expect("temp dir"); let endpoint = Arc::new(TestModelsEndpoint { has_command_auth: true, - uses_codex_backend: false, - responses: Mutex::new(vec![remote_models.clone()].into()), - etag: None, - fetch_count: AtomicUsize::new(0), - observed_proxy_policy: Mutex::new(None), + responses: Mutex::new(vec![Ok(remote_models.clone())].into()), + ..TestModelsEndpoint::default() }); let manager = openai_manager_for_tests_with_auth( codex_home.path().to_path_buf(), @@ -1079,6 +1079,86 @@ async fn refresh_available_models_keeps_merging_for_custom_api_auth() { assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); } +#[tokio::test] +async fn authoritative_catalog_preserves_hidden_models_and_matches_exact_ids() { + let model = + remote_model_with_visibility("gpt-6-sol", "Provider Model", /*priority*/ 0, "hide"); + let endpoint = TestModelsEndpoint::with_command_auth(vec![Ok(vec![model.clone()])]); + let manager = OpenAiModelsManager::new_without_cache(endpoint, /*auth_manager*/ None) + .with_provider_catalog(); + assert_eq!( + manager + .raw_model_catalog(RefreshStrategy::Online, DEFAULT_HTTP_CLIENT_FACTORY) + .await + .models, + vec![model] + ); + for unknown in ["gpt-6-sol-other", "namespace/gpt-6-sol"] { + assert_eq!( + manager + .get_model_info(unknown, &ModelsManagerConfig::default()) + .await, + model_info::model_info_from_slug(unknown) + ); + } +} + +#[tokio::test] +async fn authoritative_catalog_failure_invalidates_cache_until_refresh_succeeds() { + // Like a native TTL backend, this cache may return an invalidated stored entry. + let cache = Arc::new(TestModelsCache::default()); + let models = vec![remote_model( + "provider-model", + "Provider Model", + /*priority*/ 0, + )]; + let endpoint = TestModelsEndpoint::with_command_auth(vec![ + Ok(models.clone()), + Err(CodexErr::RequestTimeout), + Ok(models.clone()), + ]); + let manager = OpenAiModelsManager::new_with_cache( + cache.clone(), + endpoint.clone(), + /*auth_manager*/ None, + ) + .with_provider_catalog(); + assert_eq!( + manager + .raw_model_catalog(RefreshStrategy::Online, DEFAULT_HTTP_CLIENT_FACTORY) + .await + .models, + models + ); + for strategy in [RefreshStrategy::Online, RefreshStrategy::Offline] { + assert_eq!( + manager + .raw_model_catalog(strategy, DEFAULT_HTTP_CLIENT_FACTORY) + .await, + ModelsResponse::default() + ); + } + let restarted = + OpenAiModelsManager::new_with_cache(cache, endpoint, /*auth_manager*/ None) + .with_provider_catalog(); + assert_eq!( + restarted + .raw_model_catalog(RefreshStrategy::Offline, DEFAULT_HTTP_CLIENT_FACTORY) + .await, + ModelsResponse::default() + ); + assert_eq!( + restarted + .raw_model_catalog( + RefreshStrategy::OnlineIfUncached, + DEFAULT_HTTP_CLIENT_FACTORY, + ) + .await + .models, + models + ); +} + #[tokio::test] async fn refresh_available_models_uses_cache_when_fresh() { let remote_models = vec![remote_model("cached", "Cached", /*priority*/ 5)]; @@ -1129,12 +1209,10 @@ async fn online_refresh_updates_access_programs_with_unchanged_etag() { }; let responses = vec![vec![granted_model.clone()], vec![revoked_model.clone()]]; let endpoint = Arc::new(TestModelsEndpoint { - has_command_auth: false, uses_codex_backend: true, - responses: Mutex::new(responses.into()), + responses: Mutex::new(responses.into_iter().map(Ok).collect()), etag: Some("stable-catalog-etag".to_string()), - fetch_count: AtomicUsize::new(0), - observed_proxy_policy: Mutex::new(None), + ..TestModelsEndpoint::default() }); let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint.clone()); diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 01b3d347c7..286acd3af6 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -649,7 +649,7 @@ impl AppServerSession { .map(|model| model.model.clone()) }) .or_else(|| available_models.first().map(|model| model.model.clone())) - .wrap_err("model/list returned no models for TUI bootstrap")?; + .wrap_err("No models are available. Set `model` explicitly or check your model catalog configuration.")?; self.default_model = Some(default_model.clone()); self.available_models = available_models.clone(); @@ -2580,6 +2580,25 @@ mod tests { Ok(()) } + #[tokio::test] + async fn bootstrap_empty_catalog_reports_configuration_error() -> Result<()> { + let codex_home = tempfile::tempdir()?; + let mut config = build_config(&codex_home).await; + config.model = None; + config.model_catalog = Some(codex_protocol::openai_models::ModelsResponse::default()); + let mut app_server = crate::start_embedded_app_server_for_picker(&config).await?; + + let error = app_server + .bootstrap(&config) + .await + .err() + .expect("bootstrap requires a model"); + insta::assert_snapshot!(error.to_string(), @"No models are available. Set `model` explicitly or check your model catalog configuration."); + + app_server.shutdown().await?; + Ok(()) + } + fn rate_limit_snapshot(limit_id: &str) -> RateLimitSnapshot { RateLimitSnapshot { limit_id: Some(limit_id.to_string()),