From 470a5b2158732928606126be1c42c6df401dfab6 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 13 Jan 2026 14:16:53 -0800 Subject: [PATCH] clean up models manager --- codex-rs/app-server/src/models.rs | 3 +- codex-rs/core/src/codex.rs | 26 +- codex-rs/core/src/models_manager/cache.rs | 130 +++++++--- codex-rs/core/src/models_manager/manager.rs | 255 ++++++++++---------- codex-rs/core/src/thread_manager.rs | 8 +- codex-rs/core/tests/suite/list_models.rs | 5 +- codex-rs/core/tests/suite/prompt_caching.rs | 2 +- codex-rs/core/tests/suite/remote_models.rs | 40 ++- codex-rs/exec/src/lib.rs | 3 +- codex-rs/tui/src/app.rs | 7 +- codex-rs/tui2/src/app.rs | 5 +- 11 files changed, 283 insertions(+), 201 deletions(-) diff --git a/codex-rs/app-server/src/models.rs b/codex-rs/app-server/src/models.rs index 906108c50b..da73820970 100644 --- a/codex-rs/app-server/src/models.rs +++ b/codex-rs/app-server/src/models.rs @@ -3,13 +3,14 @@ use std::sync::Arc; use codex_app_server_protocol::Model; use codex_app_server_protocol::ReasoningEffortOption; use codex_core::ThreadManager; +use codex_core::models_manager::manager::RefreshStrategy; use codex_core::config::Config; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ReasoningEffortPreset; pub async fn supported_models(thread_manager: Arc, config: &Config) -> Vec { thread_manager - .list_models(config) + .list_models(config, RefreshStrategy::default()) .await .into_iter() .filter(|preset| preset.show_in_picker) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index d913e1d3cf..1a140fe26b 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -254,14 +254,9 @@ impl Codex { .map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?; let config = Arc::new(config); - if config.features.enabled(Feature::RemoteModels) - && let Err(err) = models_manager - .refresh_available_models_with_cache(&config) - .await - { - error!("failed to refresh available models: {err:?}"); - } - let model = models_manager.get_model(&config.model, &config).await; + let model = models_manager + .get_model(&config.model, &config, crate::models_manager::manager::RefreshStrategy::default()) + .await; let session_configuration = SessionConfiguration { provider: config.model_provider.clone(), model: model.clone(), @@ -965,7 +960,7 @@ impl Session { let model_info = self .services .models_manager - .construct_model_info(session_configuration.model.as_str(), &per_turn_config) + .get_model_info(session_configuration.model.as_str(), &per_turn_config) .await; let mut turn_context: TurnContext = Self::make_turn_context( Some(Arc::clone(&self.services.auth_manager)), @@ -988,6 +983,14 @@ impl Session { .await } + async fn get_config(&self) -> std::sync::Arc { + let state = self.state.lock().await; + state + .session_configuration + .original_config_do_not_use + .clone() + } + pub(crate) async fn new_default_turn_with_sub_id(&self, sub_id: String) -> Arc { let session_configuration = { let state = self.state.lock().await; @@ -2374,7 +2377,7 @@ async fn spawn_review_thread( let review_model_info = sess .services .models_manager - .construct_model_info(&model, &config) + .get_model_info(&model, &config) .await; // For reviews, disable web_search and view_image regardless of global settings. let mut review_features = sess.features.clone(); @@ -2904,9 +2907,10 @@ async fn try_run_turn( } ResponseEvent::ModelsEtag(etag) => { // Update internal state with latest models etag + let config = sess.get_config().await; sess.services .models_manager - .refresh_if_new_etag(etag, sess.features.enabled(Feature::RemoteModels)) + .refresh_if_new_etag(etag, &config) .await; } ResponseEvent::Completed { diff --git a/codex-rs/core/src/models_manager/cache.rs b/codex-rs/core/src/models_manager/cache.rs index cac16cc853..7399438579 100644 --- a/codex-rs/core/src/models_manager/cache.rs +++ b/codex-rs/core/src/models_manager/cache.rs @@ -5,22 +5,117 @@ use serde::Deserialize; use serde::Serialize; use std::io; use std::io::ErrorKind; -use std::path::Path; +use std::path::PathBuf; use std::time::Duration; use tokio::fs; +use tracing::error; + +/// Manages loading and saving of models cache to disk. +#[derive(Debug)] +pub(crate) struct ModelsCacheManager { + cache_path: PathBuf, + cache_ttl: Duration, +} + +impl ModelsCacheManager { + /// Create a new cache manager with the given path and TTL. + pub(crate) fn new(cache_path: PathBuf, cache_ttl: Duration) -> Self { + Self { + cache_path, + cache_ttl, + } + } + + /// Attempt to load a fresh cache entry. Returns `None` if the cache doesn't exist or is stale. + pub(crate) async fn load_fresh(&self) -> Option { + let cache = match self.load().await { + Ok(cache) => cache?, + Err(err) => { + error!("failed to load models cache: {err}"); + return None; + } + }; + if !cache.is_fresh(self.cache_ttl) { + return None; + } + Some(CacheData { + models: cache.models, + etag: cache.etag, + }) + } + + /// Persist the cache to disk, creating parent directories as needed. + pub(crate) async fn persist_cache(&self, models: &[ModelInfo], etag: Option) { + let cache = ModelsCache { + fetched_at: Utc::now(), + etag, + models: models.to_vec(), + }; + if let Err(err) = self.save_internal(&cache).await { + error!("failed to write models cache: {err}"); + } + } + + async fn load(&self) -> io::Result> { + match fs::read(&self.cache_path).await { + Ok(contents) => { + let cache = serde_json::from_slice(&contents) + .map_err(|err| io::Error::new(ErrorKind::InvalidData, err.to_string()))?; + Ok(Some(cache)) + } + Err(err) if err.kind() == ErrorKind::NotFound => Ok(None), + Err(err) => Err(err), + } + } + + async fn save_internal(&self, cache: &ModelsCache) -> io::Result<()> { + if let Some(parent) = self.cache_path.parent() { + fs::create_dir_all(parent).await?; + } + let json = serde_json::to_vec_pretty(cache) + .map_err(|err| io::Error::new(ErrorKind::InvalidData, err.to_string()))?; + fs::write(&self.cache_path, json).await + } + + #[cfg(test)] + /// Set the cache TTL. + pub(crate) fn set_ttl(&mut self, ttl: Duration) { + self.cache_ttl = ttl; + } + + #[cfg(test)] + /// Manipulate cache file for testing. Allows setting a custom fetched_at timestamp. + pub(crate) async fn manipulate_cache_for_test(&self, f: F) -> io::Result<()> + where + F: FnOnce(&mut DateTime), + { + let mut cache = match self.load().await? { + Some(cache) => cache, + None => return Err(io::Error::new(ErrorKind::NotFound, "cache not found")), + }; + f(&mut cache.fetched_at); + self.save_internal(&cache).await + } +} + +/// Data returned from loading a fresh cache entry. +pub(crate) struct CacheData { + pub(crate) models: Vec, + pub(crate) etag: Option, +} /// Serialized snapshot of models and metadata cached on disk. #[derive(Debug, Clone, Serialize, Deserialize)] -pub(crate) struct ModelsCache { - pub(crate) fetched_at: DateTime, +struct ModelsCache { + fetched_at: DateTime, #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) etag: Option, - pub(crate) models: Vec, + etag: Option, + models: Vec, } impl ModelsCache { /// Returns `true` when the cache entry has not exceeded the configured TTL. - pub(crate) fn is_fresh(&self, ttl: Duration) -> bool { + fn is_fresh(&self, ttl: Duration) -> bool { if ttl.is_zero() { return false; } @@ -31,26 +126,3 @@ impl ModelsCache { age <= ttl_duration } } - -/// Read and deserialize the cache file if it exists. -pub(crate) async fn load_cache(path: &Path) -> io::Result> { - match fs::read(path).await { - Ok(contents) => { - let cache = serde_json::from_slice(&contents) - .map_err(|err| io::Error::new(ErrorKind::InvalidData, err.to_string()))?; - Ok(Some(cache)) - } - Err(err) if err.kind() == ErrorKind::NotFound => Ok(None), - Err(err) => Err(err), - } -} - -/// Persist the cache contents to disk, creating parent directories as needed. -pub(crate) async fn save_cache(path: &Path, cache: &ModelsCache) -> io::Result<()> { - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).await?; - } - let json = serde_json::to_vec_pretty(cache) - .map_err(|err| io::Error::new(ErrorKind::InvalidData, err.to_string()))?; - fs::write(path, json).await -} diff --git a/codex-rs/core/src/models_manager/manager.rs b/codex-rs/core/src/models_manager/manager.rs index 862f5584c8..780baae175 100644 --- a/codex-rs/core/src/models_manager/manager.rs +++ b/codex-rs/core/src/models_manager/manager.rs @@ -1,4 +1,3 @@ -use chrono::Utc; use codex_api::ModelsClient; use codex_api::ReqwestTransport; use codex_app_server_protocol::AuthMode; @@ -15,8 +14,7 @@ use tokio::sync::TryLockError; use tokio::time::timeout; use tracing::error; -use super::cache; -use super::cache::ModelsCache; +use super::cache::ModelsCacheManager; use crate::api_bridge::auth_provider_from_auth; use crate::api_bridge::map_api_error; use crate::auth::AuthManager; @@ -36,6 +34,18 @@ const OPENAI_DEFAULT_API_MODEL: &str = "gpt-5.1-codex-max"; const OPENAI_DEFAULT_CHATGPT_MODEL: &str = "gpt-5.2-codex"; const CODEX_AUTO_BALANCED_MODEL: &str = "codex-auto-balanced"; +/// Strategy for refreshing available models. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum RefreshStrategy { + /// Always fetch from the network, ignoring cache. + Online, + /// Only use cached data, never fetch from the network. + Offline, + /// Use cache if available and fresh, otherwise fetch from the network. + #[default] + OnlineIfUncached, +} + /// Coordinates remote model discovery plus cached metadata on disk. #[derive(Debug)] pub struct ModelsManager { @@ -43,87 +53,31 @@ pub struct ModelsManager { remote_models: RwLock>, auth_manager: Arc, etag: RwLock>, - codex_home: PathBuf, - cache_ttl: Duration, + cache_manager: ModelsCacheManager, provider: ModelProviderInfo, } impl ModelsManager { /// Construct a manager scoped to the provided `AuthManager`. pub fn new(codex_home: PathBuf, auth_manager: Arc) -> Self { + let cache_path = codex_home.join(MODEL_CACHE_FILE); + let cache_manager = ModelsCacheManager::new(cache_path, DEFAULT_MODEL_CACHE_TTL); Self { local_models: builtin_model_presets(auth_manager.get_auth_mode()), remote_models: RwLock::new(Self::load_remote_models_from_file().unwrap_or_default()), auth_manager, etag: RwLock::new(None), - codex_home, - cache_ttl: DEFAULT_MODEL_CACHE_TTL, + cache_manager, provider: ModelProviderInfo::create_openai_provider(), } } - #[cfg(any(test, feature = "test-support"))] - /// Construct a manager scoped to the provided `AuthManager` with a specific provider. Used for integration tests. - pub fn with_provider( - codex_home: PathBuf, - auth_manager: Arc, - provider: ModelProviderInfo, - ) -> Self { - Self { - local_models: builtin_model_presets(auth_manager.get_auth_mode()), - remote_models: RwLock::new(Self::load_remote_models_from_file().unwrap_or_default()), - auth_manager, - etag: RwLock::new(None), - codex_home, - cache_ttl: DEFAULT_MODEL_CACHE_TTL, - provider, - } - } - - /// Fetch the latest remote models, using the on-disk cache when still fresh. - pub async fn refresh_available_models_with_cache(&self, config: &Config) -> CoreResult<()> { - if !config.features.enabled(Feature::RemoteModels) - || self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) - { - return Ok(()); - } - if self.try_load_cache().await { - return Ok(()); - } - self.refresh_available_models_no_cache(config.features.enabled(Feature::RemoteModels)) - .await - } - - pub(crate) async fn refresh_available_models_no_cache( + pub async fn list_models( &self, - remote_models_feature: bool, - ) -> CoreResult<()> { - if !remote_models_feature || self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) { - return Ok(()); - } - let auth = self.auth_manager.auth().await; - let api_provider = self.provider.to_api_provider(Some(AuthMode::ChatGPT))?; - let api_auth = auth_provider_from_auth(auth.clone(), &self.provider)?; - let transport = ReqwestTransport::new(build_reqwest_client()); - let client = ModelsClient::new(transport, api_provider, api_auth); - - let client_version = format_client_version_to_whole(); - let (models, etag) = timeout( - MODELS_REFRESH_TIMEOUT, - client.list_models(&client_version, HeaderMap::new()), - ) - .await - .map_err(|_| CodexErr::Timeout)? - .map_err(map_api_error)?; - - self.apply_remote_models(models.clone()).await; - *self.etag.write().await = etag.clone(); - self.persist_cache(&models, etag).await; - Ok(()) - } - - pub async fn list_models(&self, config: &Config) -> Vec { - if let Err(err) = self.refresh_available_models_with_cache(config).await { + config: &Config, + refresh_strategy: RefreshStrategy, + ) -> Vec { + if let Err(err) = self.refresh_available_models(config, refresh_strategy).await { error!("failed to refresh available models: {err}"); } let remote_models = self.remote_models(config).await; @@ -136,7 +90,7 @@ impl ModelsManager { } /// Look up the requested model metadata while applying remote metadata overrides. - pub async fn construct_model_info(&self, model: &str, config: &Config) -> ModelInfo { + pub async fn get_model_info(&self, model: &str, config: &Config) -> ModelInfo { let remote = self .remote_models(config) .await @@ -150,11 +104,16 @@ impl ModelsManager { model_info::with_config_overrides(model, config) } - pub async fn get_model(&self, model: &Option, config: &Config) -> String { + pub async fn get_model( + &self, + model: &Option, + config: &Config, + refresh_strategy: RefreshStrategy, + ) -> String { if let Some(model) = model.as_ref() { return model.to_string(); } - if let Err(err) = self.refresh_available_models_with_cache(config).await { + if let Err(err) = self.refresh_available_models(config, refresh_strategy).await { error!("failed to refresh available models: {err}"); } // if codex-auto-balanced exists & signed in with chatgpt mode, return it, otherwise return the default model @@ -172,28 +131,72 @@ impl ModelsManager { } OPENAI_DEFAULT_API_MODEL.to_string() } - pub async fn refresh_if_new_etag(&self, etag: String, remote_models_feature: bool) { + + pub async fn refresh_if_new_etag(&self, etag: String, config: &Config) { let current_etag = self.get_etag().await; if current_etag.clone().is_some() && current_etag.as_deref() == Some(etag.as_str()) { return; } if let Err(err) = self - .refresh_available_models_no_cache(remote_models_feature) + .refresh_available_models(config, RefreshStrategy::Online) .await { error!("failed to refresh available models: {err}"); } } - #[cfg(any(test, feature = "test-support"))] - pub fn get_model_offline(model: Option<&str>) -> String { - model.unwrap_or(OPENAI_DEFAULT_CHATGPT_MODEL).to_string() + /// Refresh available models according to the specified strategy. + async fn refresh_available_models( + &self, + config: &Config, + refresh_strategy: RefreshStrategy, + ) -> CoreResult<()> { + if !config.features.enabled(Feature::RemoteModels) + || self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) + { + return Ok(()); + } + + match refresh_strategy { + RefreshStrategy::Offline => { + // Only try to load from cache, never fetch + self.try_load_cache().await; + Ok(()) + } + RefreshStrategy::OnlineIfUncached => { + // Try cache first, fall back to online if unavailable + if self.try_load_cache().await { + return Ok(()); + } + self.fetch_and_update_models().await + } + RefreshStrategy::Online => { + // Always fetch from network + self.fetch_and_update_models().await + } + } } - #[cfg(any(test, feature = "test-support"))] - /// Offline helper that builds a `ModelInfo` without consulting remote state. - pub fn construct_model_info_offline(model: &str, config: &Config) -> ModelInfo { - model_info::with_config_overrides(model_info::find_model_info_for_slug(model), config) + async fn fetch_and_update_models(&self) -> CoreResult<()> { + let auth = self.auth_manager.auth().await; + let api_provider = self.provider.to_api_provider(Some(AuthMode::ChatGPT))?; + let api_auth = auth_provider_from_auth(auth.clone(), &self.provider)?; + let transport = ReqwestTransport::new(build_reqwest_client()); + let client = ModelsClient::new(transport, api_provider, api_auth); + + let client_version = format_client_version_to_whole(); + let (models, etag) = timeout( + MODELS_REFRESH_TIMEOUT, + client.list_models(&client_version, HeaderMap::new()), + ) + .await + .map_err(|_| CodexErr::Timeout)? + .map_err(map_api_error)?; + + self.apply_remote_models(models.clone()).await; + *self.etag.write().await = etag.clone(); + self.cache_manager.persist_cache(&models, etag).await; + Ok(()) } async fn get_etag(&self) -> Option { @@ -213,41 +216,16 @@ impl ModelsManager { /// Attempt to satisfy the refresh from the cache when it matches the provider and TTL. async fn try_load_cache(&self) -> bool { - // todo(aibrahim): think if we should store fetched_at in ModelsManager so we don't always need to read the disk - let cache_path = self.cache_path(); - let cache = match cache::load_cache(&cache_path).await { - Ok(cache) => cache, - Err(err) => { - error!("failed to load models cache: {err}"); - return false; - } - }; - let cache = match cache { + let cache = match self.cache_manager.load_fresh().await { Some(cache) => cache, None => return false, }; - if !cache.is_fresh(self.cache_ttl) { - return false; - } let models = cache.models.clone(); *self.etag.write().await = cache.etag.clone(); self.apply_remote_models(models.clone()).await; true } - /// Serialize the latest fetch to disk for reuse across future processes. - async fn persist_cache(&self, models: &[ModelInfo], etag: Option) { - let cache = ModelsCache { - fetched_at: Utc::now(), - etag, - models: models.to_vec(), - }; - let cache_path = self.cache_path(); - if let Err(err) = cache::save_cache(&cache_path, &cache).await { - error!("failed to write models cache: {err}"); - } - } - /// Merge remote model metadata into picker-ready presets, preserving existing entries. fn build_available_models(&self, mut remote_models: Vec) -> Vec { remote_models.sort_by(|a, b| a.priority.cmp(&b.priority)); @@ -321,8 +299,34 @@ impl ModelsManager { } } - fn cache_path(&self) -> PathBuf { - self.codex_home.join(MODEL_CACHE_FILE) + #[cfg(any(test, feature = "test-support"))] + /// Construct a manager scoped to the provided `AuthManager` with a specific provider. Used for integration tests. + pub fn with_provider( + codex_home: PathBuf, + auth_manager: Arc, + provider: ModelProviderInfo, + ) -> Self { + let cache_path = codex_home.join(MODEL_CACHE_FILE); + let cache_manager = ModelsCacheManager::new(cache_path, DEFAULT_MODEL_CACHE_TTL); + Self { + local_models: builtin_model_presets(auth_manager.get_auth_mode()), + remote_models: RwLock::new(Self::load_remote_models_from_file().unwrap_or_default()), + auth_manager, + etag: RwLock::new(None), + cache_manager, + provider, + } + } + + #[cfg(any(test, feature = "test-support"))] + pub fn get_model_offline(model: Option<&str>) -> String { + model.unwrap_or(OPENAI_DEFAULT_CHATGPT_MODEL).to_string() + } + + #[cfg(any(test, feature = "test-support"))] + /// Offline helper that builds a `ModelInfo` without consulting remote state. + pub fn construct_model_info_offline(model: &str, config: &Config) -> ModelInfo { + model_info::with_config_overrides(model_info::find_model_info_for_slug(model), config) } } @@ -338,13 +342,13 @@ fn format_client_version_to_whole() -> String { #[cfg(test)] mod tests { - use super::cache::ModelsCache; use super::*; use crate::CodexAuth; use crate::auth::AuthCredentialsStoreMode; use crate::config::ConfigBuilder; use crate::features::Feature; use crate::model_provider_info::WireApi; + use chrono::Utc; use codex_protocol::openai_models::ModelsResponse; use core_test_support::responses::mount_models_once; use pretty_assertions::assert_eq; @@ -434,13 +438,13 @@ mod tests { ModelsManager::with_provider(codex_home.path().to_path_buf(), auth_manager, provider); manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("refresh succeeds"); let cached_remote = manager.remote_models(&config).await; assert_eq!(cached_remote, remote_models); - let available = manager.list_models(&config).await; + let available = manager.list_models(&config, RefreshStrategy::OnlineIfUncached).await; let high_idx = available .iter() .position(|model| model.model == "priority-high") @@ -494,7 +498,7 @@ mod tests { ModelsManager::with_provider(codex_home.path().to_path_buf(), auth_manager, provider); manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("first refresh succeeds"); assert_eq!( @@ -505,7 +509,7 @@ mod tests { // Second call should read from cache and avoid the network. manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("cached refresh succeeds"); assert_eq!( @@ -549,19 +553,18 @@ mod tests { ModelsManager::with_provider(codex_home.path().to_path_buf(), auth_manager, provider); manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("initial refresh succeeds"); // Rewrite cache with an old timestamp so it is treated as stale. - let cache_path = codex_home.path().join(MODEL_CACHE_FILE); - let contents = - std::fs::read_to_string(&cache_path).expect("cache file should exist after refresh"); - let mut cache: ModelsCache = - serde_json::from_str(&contents).expect("cache should deserialize"); - cache.fetched_at = Utc::now() - chrono::Duration::hours(1); - std::fs::write(&cache_path, serde_json::to_string_pretty(&cache).unwrap()) - .expect("cache rewrite succeeds"); + manager + .cache_manager + .manipulate_cache_for_test(|fetched_at| { + *fetched_at = Utc::now() - chrono::Duration::hours(1); + }) + .await + .expect("cache manipulation succeeds"); let updated_models = vec![remote_model("fresh", "Fresh", 9)]; server.reset().await; @@ -574,7 +577,7 @@ mod tests { .await; manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("second refresh succeeds"); assert_eq!( @@ -618,10 +621,10 @@ mod tests { let provider = provider_for(server.uri()); let mut manager = ModelsManager::with_provider(codex_home.path().to_path_buf(), auth_manager, provider); - manager.cache_ttl = Duration::ZERO; + manager.cache_manager.set_ttl(Duration::ZERO); manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("initial refresh succeeds"); @@ -636,7 +639,7 @@ mod tests { .await; manager - .refresh_available_models_with_cache(&config) + .refresh_available_models(&config, RefreshStrategy::OnlineIfUncached) .await .expect("second refresh succeeds"); diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 0a124f29cf..f669f2dd63 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -138,8 +138,12 @@ impl ThreadManager { self.state.models_manager.clone() } - pub async fn list_models(&self, config: &Config) -> Vec { - self.state.models_manager.list_models(config).await + pub async fn list_models( + &self, + config: &Config, + refresh_strategy: crate::models_manager::manager::RefreshStrategy, + ) -> Vec { + self.state.models_manager.list_models(config, refresh_strategy).await } pub async fn list_thread_ids(&self) -> Vec { diff --git a/codex-rs/core/tests/suite/list_models.rs b/codex-rs/core/tests/suite/list_models.rs index 182fabefe8..69262b4ece 100644 --- a/codex-rs/core/tests/suite/list_models.rs +++ b/codex-rs/core/tests/suite/list_models.rs @@ -2,6 +2,7 @@ use anyhow::Result; use codex_core::CodexAuth; use codex_core::ThreadManager; use codex_core::built_in_model_providers; +use codex_core::models_manager::manager::RefreshStrategy; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; @@ -18,7 +19,7 @@ async fn list_models_returns_api_key_models() -> Result<()> { CodexAuth::from_api_key("sk-test"), built_in_model_providers()["openai"].clone(), ); - let models = manager.list_models(&config).await; + let models = manager.list_models(&config, RefreshStrategy::default()).await; let expected_models = expected_models_for_api_key(); assert_eq!(expected_models, models); @@ -34,7 +35,7 @@ async fn list_models_returns_chatgpt_models() -> Result<()> { CodexAuth::create_dummy_chatgpt_auth_for_testing(), built_in_model_providers()["openai"].clone(), ); - let models = manager.list_models(&config).await; + let models = manager.list_models(&config, RefreshStrategy::default()).await; let expected_models = expected_models_for_chatgpt(); assert_eq!(expected_models, models); diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index 79c9d6e19c..3e60a9b567 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -85,7 +85,7 @@ async fn prompt_tools_are_consistent_across_requests() -> anyhow::Result<()> { .await?; let base_instructions = thread_manager .get_models_manager() - .construct_model_info( + .get_model_info( config .model .as_deref() diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 50833de31c..07dbed3f25 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -7,9 +7,9 @@ use codex_core::CodexAuth; use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; use codex_core::config::Config; -use codex_core::error::CodexErr; use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; +use codex_core::models_manager::manager::RefreshStrategy; use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::ExecCommandSource; @@ -127,7 +127,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { assert_eq!(requests[0].url.path(), "/v1/models"); let model_info = models_manager - .construct_model_info(REMOTE_MODEL_SLUG, &config) + .get_model_info(REMOTE_MODEL_SLUG, &config) .await; assert_eq!(model_info.shell_type, ConfigShellToolType::UnifiedExec); @@ -226,7 +226,7 @@ async fn remote_models_truncation_policy_without_override_preserves_remote() -> wait_for_model_available(&models_manager, slug, &test.config).await; let model_info = models_manager - .construct_model_info(slug, &test.config) + .get_model_info(slug, &test.config) .await; assert_eq!( model_info.truncation_policy, @@ -274,7 +274,7 @@ async fn remote_models_truncation_policy_with_tool_output_override() -> Result<( wait_for_model_available(&models_manager, slug, &test.config).await; let model_info = models_manager - .construct_model_info(slug, &test.config) + .get_model_info(slug, &test.config) .await; assert_eq!( model_info.truncation_policy, @@ -423,12 +423,7 @@ async fn remote_models_preserve_builtin_presets() -> Result<()> { provider, ); - manager - .refresh_available_models_with_cache(&config) - .await - .expect("refresh succeeds"); - - let available = manager.list_models(&config).await; + let available = manager.list_models(&config, RefreshStrategy::OnlineIfUncached).await; let remote = available .iter() .find(|model| model.model == "remote-alpha") @@ -483,16 +478,19 @@ async fn remote_models_request_times_out_after_5s() -> Result<()> { ); let start = Instant::now(); - let refresh = timeout( + let model = timeout( Duration::from_secs(7), - manager.refresh_available_models_with_cache(&config), + manager.get_model(&None, &config, RefreshStrategy::OnlineIfUncached), ) .await; let elapsed = start.elapsed(); - let err = refresh - .expect("refresh should finish") - .expect_err("refresh should time out"); - let request_summaries: Vec = server + // get_model should return a default model even when refresh times out + let default_model = model.expect("get_model should finish and return default model"); + assert!( + default_model == "gpt-5.2-codex" || default_model == "gpt-5.1-codex-max", + "get_model should return default model when refresh times out, got: {default_model}" + ); + let _request_summaries: Vec = server .received_requests() .await .expect("mock server should capture requests") @@ -507,10 +505,6 @@ async fn remote_models_request_times_out_after_5s() -> Result<()> { elapsed < Duration::from_millis(5_800), "expected models call to time out before the delayed response; took {elapsed:?}" ); - match err { - CodexErr::Timeout => {} - other => panic!("expected timeout error, got {other:?}; requests: {request_summaries:?}"), - } assert_eq!( models_mock.requests().len(), 1, @@ -550,10 +544,10 @@ async fn remote_models_hide_picker_only_models() -> Result<()> { provider, ); - let selected = manager.get_model(&None, &config).await; + let selected = manager.get_model(&None, &config, RefreshStrategy::OnlineIfUncached).await; assert_eq!(selected, "gpt-5.2-codex"); - let available = manager.list_models(&config).await; + let available = manager.list_models(&config, RefreshStrategy::OnlineIfUncached).await; let hidden = available .iter() .find(|model| model.model == "codex-auto-balanced") @@ -571,7 +565,7 @@ async fn wait_for_model_available( let deadline = Instant::now() + Duration::from_secs(2); loop { if let Some(model) = { - let guard = manager.list_models(config).await; + let guard = manager.list_models(config, RefreshStrategy::OnlineIfUncached).await; guard.iter().find(|model| model.model == slug).cloned() } { return model; diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index a887c8bb2f..42d52ae777 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -22,6 +22,7 @@ use codex_core::NewThread; use codex_core::OLLAMA_CHAT_PROVIDER_ID; use codex_core::OLLAMA_OSS_PROVIDER_ID; use codex_core::ThreadManager; +use codex_core::models_manager::manager::RefreshStrategy; use codex_core::auth::enforce_login_restrictions; use codex_core::config::Config; use codex_core::config::ConfigOverrides; @@ -310,7 +311,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any ); let default_model = thread_manager .get_models_manager() - .get_model(&config.model, &config) + .get_model(&config.model, &config, RefreshStrategy::default()) .await; // Handle resume subcommand by resolving a rollout path and using explicit resume API. diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 312d4e0632..28da0da396 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -33,6 +33,7 @@ use codex_core::config::edit::ConfigEditsBuilder; #[cfg(target_os = "windows")] use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; +use codex_core::models_manager::manager::RefreshStrategy; use codex_core::models_manager::model_presets::HIDE_GPT_5_1_CODEX_MAX_MIGRATION_PROMPT_CONFIG; use codex_core::models_manager::model_presets::HIDE_GPT5_1_MIGRATION_PROMPT_CONFIG; use codex_core::protocol::DeprecationNoticeEvent; @@ -199,7 +200,7 @@ async fn handle_model_migration_prompt_if_needed( app_event_tx: &AppEventSender, models_manager: Arc, ) -> Option { - let available_models = models_manager.list_models(config).await; + let available_models = models_manager.list_models(config, RefreshStrategy::default()).await; let upgrade = available_models .iter() .find(|preset| preset.model == model) @@ -369,7 +370,7 @@ impl App { )); let mut model = thread_manager .get_models_manager() - .get_model(&config.model, &config) + .get_model(&config.model, &config, RefreshStrategy::default()) .await; let exit_info = handle_model_migration_prompt_if_needed( tui, @@ -589,7 +590,7 @@ impl App { let model_info = self .server .get_models_manager() - .construct_model_info(self.current_model.as_str(), &self.config) + .get_model_info(self.current_model.as_str(), &self.config) .await; match event { AppEvent::NewSession => { diff --git a/codex-rs/tui2/src/app.rs b/codex-rs/tui2/src/app.rs index 4e3ac3795c..20d37d501d 100644 --- a/codex-rs/tui2/src/app.rs +++ b/codex-rs/tui2/src/app.rs @@ -50,6 +50,7 @@ use codex_core::config::edit::ConfigEditsBuilder; #[cfg(target_os = "windows")] use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; +use codex_core::models_manager::manager::RefreshStrategy; use codex_core::models_manager::model_presets::HIDE_GPT_5_1_CODEX_MAX_MIGRATION_PROMPT_CONFIG; use codex_core::models_manager::model_presets::HIDE_GPT5_1_MIGRATION_PROMPT_CONFIG; use codex_core::protocol::DeprecationNoticeEvent; @@ -231,7 +232,7 @@ async fn handle_model_migration_prompt_if_needed( app_event_tx: &AppEventSender, models_manager: Arc, ) -> Option { - let available_models = models_manager.list_models(config).await; + let available_models = models_manager.list_models(config, RefreshStrategy::default()).await; let upgrade = available_models .iter() .find(|preset| preset.model == model) @@ -432,7 +433,7 @@ impl App { )); let mut model = thread_manager .get_models_manager() - .get_model(&config.model, &config) + .get_model(&config.model, &config, RefreshStrategy::default()) .await; let exit_info = handle_model_migration_prompt_if_needed( tui,