From ccf73cf7920926ae304a3deb9f4482be0972e708 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Wed, 24 Jun 2026 15:55:25 -0700 Subject: [PATCH] Scope fallback to static catalogs --- codex-rs/core/src/session/mod.rs | 15 ++-- codex-rs/core/src/session/model_resolution.rs | 74 ------------------- codex-rs/core/src/session/turn_context.rs | 3 +- .../model-provider/src/amazon_bedrock/mod.rs | 26 ------- codex-rs/models-manager/src/lib.rs | 3 - codex-rs/models-manager/src/manager.rs | 64 +++++++--------- codex-rs/models-manager/src/manager_tests.rs | 49 ++++++------ codex-rs/models-manager/src/resolution.rs | 32 -------- 8 files changed, 59 insertions(+), 207 deletions(-) delete mode 100644 codex-rs/core/src/session/model_resolution.rs delete mode 100644 codex-rs/models-manager/src/resolution.rs diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index f629d3f240..00d2fdca39 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -215,7 +215,6 @@ mod handlers; mod inject; mod input_queue; mod mcp; -mod model_resolution; pub(crate) mod multi_agents; mod review; mod rollout_budget; @@ -568,6 +567,7 @@ impl Codex { ) }; + let config = Arc::new(config); let refresh_strategy = if session_source.is_non_root_agent() { codex_models_manager::manager::RefreshStrategy::Offline } else { @@ -581,13 +581,9 @@ impl Codex { { let _ = models_manager.list_models(refresh_strategy).await; } - let model = model_resolution::resolve_configured_model( - &mut config, - &models_manager, - refresh_strategy, - ) - .await; - let config = Arc::new(config); + let model = models_manager + .get_default_model(&config.model, refresh_strategy) + .await; // Resolve base instructions for the session. Priority order: // 1. config.base_instructions override @@ -1484,9 +1480,8 @@ impl Session { pub(crate) async fn update_settings( &self, - mut updates: SessionSettingsUpdate, + updates: SessionSettingsUpdate, ) -> ConstraintResult<()> { - self.resolve_settings_update_model(&mut updates).await; let notify_config_contributors = !self.services.extensions.config_contributors().is_empty(); let (previous_config, new_config, permission_profile_changed) = { let mut state = self.state.lock().await; diff --git a/codex-rs/core/src/session/model_resolution.rs b/codex-rs/core/src/session/model_resolution.rs deleted file mode 100644 index 3ecbf0ba17..0000000000 --- a/codex-rs/core/src/session/model_resolution.rs +++ /dev/null @@ -1,74 +0,0 @@ -use super::Session; -use super::session::SessionSettingsUpdate; -use crate::config::Config; -use codex_models_manager::UnsupportedModelFallback; -use codex_models_manager::manager::RefreshStrategy; -use codex_models_manager::manager::SharedModelsManager; - -pub(super) async fn resolve_configured_model( - config: &mut Config, - models_manager: &SharedModelsManager, - refresh_strategy: RefreshStrategy, -) -> String { - let resolution = models_manager - .resolve_model(&config.model, refresh_strategy) - .await; - if let Some(fallback) = resolution.fallback.as_ref() { - record_model_fallback(config.model_provider_id.as_str(), fallback); - config.startup_warnings.push(model_fallback_warning( - config.model_provider_id.as_str(), - fallback, - )); - config.model = Some(resolution.model.clone()); - } - resolution.model -} - -impl Session { - pub(super) async fn resolve_settings_update_model(&self, updates: &mut SessionSettingsUpdate) { - let Some(collaboration_mode) = updates.collaboration_mode.clone() else { - return; - }; - let requested_model = Some(collaboration_mode.model().to_string()); - let resolution = self - .services - .models_manager - .resolve_model(&requested_model, RefreshStrategy::OnlineIfUncached) - .await; - let Some(fallback) = resolution.fallback else { - return; - }; - updates.collaboration_mode = Some(collaboration_mode.with_updates( - Some(resolution.model), - /*effort*/ None, - /*developer_instructions*/ None, - )); - let provider_id = { - let state = self.state.lock().await; - state - .session_configuration - .original_config_do_not_use - .model_provider_id - .clone() - }; - record_model_fallback(provider_id.as_str(), &fallback); - } -} - -fn model_fallback_warning(provider_id: &str, fallback: &UnsupportedModelFallback) -> String { - let requested_model = fallback.requested_model.as_str(); - let fallback_model = fallback.fallback_model.as_str(); - format!( - "Model `{requested_model}` is unavailable for provider `{provider_id}`; using \ - `{fallback_model}` instead." - ) -} - -fn record_model_fallback(provider_id: &str, fallback: &UnsupportedModelFallback) { - tracing::warn!( - provider = provider_id, - requested_model = fallback.requested_model.as_str(), - fallback_model = fallback.fallback_model.as_str(), - "requested model is unsupported by provider; using provider fallback" - ); -} diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 6aeaeb0398..da526d4911 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -587,9 +587,8 @@ impl Session { pub(crate) async fn new_turn_with_sub_id( &self, sub_id: String, - mut updates: SessionSettingsUpdate, + updates: SessionSettingsUpdate, ) -> CodexResult> { - self.resolve_settings_update_model(&mut updates).await; let notify_config_contributors = !self.services.extensions.config_contributors().is_empty(); let update_result: CodexResult<_> = { let mut state = self.state.lock().await; diff --git a/codex-rs/model-provider/src/amazon_bedrock/mod.rs b/codex-rs/model-provider/src/amazon_bedrock/mod.rs index 37b57d7aa6..4d0a806662 100644 --- a/codex-rs/model-provider/src/amazon_bedrock/mod.rs +++ b/codex-rs/model-provider/src/amazon_bedrock/mod.rs @@ -305,30 +305,4 @@ mod tests { AMAZON_BEDROCK_GPT_5_4_MODEL_ID ); } - - #[tokio::test] - async fn unsupported_model_falls_back_to_bedrock_gpt_5_4() { - let provider = AmazonBedrockModelProvider::new( - ModelProviderInfo::create_amazon_bedrock_provider(/*aws*/ None), - /*auth_manager*/ None, - ); - let models_manager = - provider.models_manager(PathBuf::new(), /*config_model_catalog*/ None); - - assert_eq!( - models_manager - .resolve_model( - &Some("gpt-5.4-mini".to_string()), - codex_models_manager::manager::RefreshStrategy::Offline, - ) - .await, - codex_models_manager::ModelResolution { - model: AMAZON_BEDROCK_GPT_5_4_MODEL_ID.to_string(), - fallback: Some(codex_models_manager::UnsupportedModelFallback { - requested_model: "gpt-5.4-mini".to_string(), - fallback_model: AMAZON_BEDROCK_GPT_5_4_MODEL_ID.to_string(), - }), - } - ); - } } diff --git a/codex-rs/models-manager/src/lib.rs b/codex-rs/models-manager/src/lib.rs index cc21c78bbc..4a4c590afe 100644 --- a/codex-rs/models-manager/src/lib.rs +++ b/codex-rs/models-manager/src/lib.rs @@ -4,13 +4,10 @@ pub(crate) mod config; pub mod manager; pub mod model_info; pub mod model_presets; -mod resolution; pub mod test_support; pub use codex_protocol::auth::AuthMode; pub use config::ModelsManagerConfig; -pub use resolution::ModelResolution; -pub use resolution::UnsupportedModelFallback; /// Load the bundled model catalog shipped with `codex-models-manager`. pub fn bundled_models_response() diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 0ea93f0154..6ca5bd4393 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -1,5 +1,4 @@ use super::cache::ModelsCacheManager; -use crate::ModelResolution; use crate::collaboration_mode_presets::builtin_collaboration_mode_presets; use crate::config::ModelsManagerConfig; use crate::model_info; @@ -145,8 +144,8 @@ pub trait ModelsManager: fmt::Debug + Send + Sync { // todo(aibrahim): should be visible to core only and sent on session_configured event /// Get the model identifier to use, refreshing according to the specified strategy. /// - /// If `model` is provided, returns it directly. Otherwise selects the default based on - /// auth mode and available models. + /// If `model` is provided, returns it directly unless the implementation applies an explicit + /// provider fallback. Otherwise selects the default based on auth mode and available models. fn get_default_model<'a>( &'a self, model: &'a Option, @@ -167,21 +166,6 @@ pub trait ModelsManager: fmt::Debug + Send + Sync { ) } - /// Resolve a requested model, applying provider-specific fallback policy when configured. - /// - /// Implementations with an open-ended model namespace should preserve explicit model names. - /// Implementations backed by an authoritative catalog may replace unsupported names with a - /// provider-owned fallback. - fn resolve_model<'a>( - &'a self, - model: &'a Option, - refresh_strategy: RefreshStrategy, - ) -> ModelsManagerFuture<'a, ModelResolution> { - Box::pin(async move { - ModelResolution::exact(self.get_default_model(model, refresh_strategy).await) - }) - } - // todo(aibrahim): look if we can tighten it to pub(crate) /// Look up model metadata, applying remote overrides and config adjustments. fn get_model_info<'a>( @@ -466,29 +450,35 @@ impl ModelsManager for StaticModelsManager { builtin_collaboration_mode_presets() } - fn resolve_model<'a>( + fn get_default_model<'a>( &'a self, model: &'a Option, refresh_strategy: RefreshStrategy, - ) -> ModelsManagerFuture<'a, ModelResolution> { - Box::pin(async move { - let Some(requested_model) = model.as_ref() else { - return ModelResolution::exact( - self.get_default_model(model, refresh_strategy).await, - ); - }; - let Some(fallback_model) = self.unsupported_model_fallback.as_ref() else { - return ModelResolution::exact(requested_model.clone()); - }; - if self - .remote_models - .iter() - .any(|candidate| candidate.slug == *requested_model) - { - return ModelResolution::exact(requested_model.clone()); + ) -> ModelsManagerFuture<'a, String> { + Box::pin( + async move { + let Some(requested_model) = model.as_ref() else { + return default_model_from_available(self.list_models(refresh_strategy).await); + }; + let Some(fallback_model) = self.unsupported_model_fallback.as_ref() else { + return requested_model.clone(); + }; + if self + .remote_models + .iter() + .any(|candidate| candidate.slug == *requested_model) + { + requested_model.clone() + } else { + fallback_model.clone() + } } - ModelResolution::fallback(requested_model.clone(), fallback_model.clone()) - }) + .instrument(tracing::info_span!( + "get_default_model", + model.provided = model.is_some(), + refresh_strategy = %refresh_strategy + )), + ) } fn refresh_if_new_etag(&self, _etag: String) -> ModelsManagerFuture<'_, ()> { diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index ab4c59b72b..3915012af8 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -213,66 +213,69 @@ fn static_manager_with_fallback_for_tests( #[tokio::test] async fn static_manager_falls_back_from_unsupported_model_when_configured() { - let supported_model = remote_model("provider.gpt-5.4", "GPT-5.4", 0); let manager = static_manager_with_fallback_for_tests( ModelsResponse { - models: vec![supported_model], + models: vec![remote_model("provider.gpt-5.4", "GPT-5.4", 0)], }, "provider.gpt-5.4", ); assert_eq!( manager - .resolve_model(&Some("gpt-5.4-mini".to_string()), RefreshStrategy::Offline,) + .get_default_model(&Some("gpt-5.4-mini".to_string()), RefreshStrategy::Offline,) .await, - crate::ModelResolution { - model: "provider.gpt-5.4".to_string(), - fallback: Some(crate::UnsupportedModelFallback { - requested_model: "gpt-5.4-mini".to_string(), - fallback_model: "provider.gpt-5.4".to_string(), - }), - } + "provider.gpt-5.4" ); } #[tokio::test] async fn static_manager_preserves_supported_model_with_fallback_configured() { - let supported_model = remote_model("provider.gpt-5.4", "GPT-5.4", 0); let manager = static_manager_with_fallback_for_tests( ModelsResponse { - models: vec![supported_model], + models: vec![remote_model("provider.gpt-5.4", "GPT-5.4", 0)], }, "provider.gpt-5.4", ); assert_eq!( manager - .resolve_model( + .get_default_model( &Some("provider.gpt-5.4".to_string()), RefreshStrategy::Offline, ) .await, - crate::ModelResolution { - model: "provider.gpt-5.4".to_string(), - fallback: None, - } + "provider.gpt-5.4" ); } #[tokio::test] -async fn static_manager_without_fallback_preserves_unknown_model() { +async fn static_manager_with_fallback_preserves_default_selection() { + let manager = static_manager_with_fallback_for_tests( + ModelsResponse { + models: vec![remote_model("provider.gpt-5.4", "GPT-5.4", 0)], + }, + "provider.gpt-5.4", + ); + + assert_eq!( + manager + .get_default_model(&None, RefreshStrategy::Offline) + .await, + "provider.gpt-5.4" + ); +} + +#[tokio::test] +async fn static_manager_without_fallback_preserves_unsupported_model() { let manager = static_manager_for_tests(ModelsResponse { models: vec![remote_model("known-model", "Known", 0)], }); assert_eq!( manager - .resolve_model(&Some("custom-model".to_string()), RefreshStrategy::Offline,) + .get_default_model(&Some("custom-model".to_string()), RefreshStrategy::Offline) .await, - crate::ModelResolution { - model: "custom-model".to_string(), - fallback: None, - } + "custom-model" ); } diff --git a/codex-rs/models-manager/src/resolution.rs b/codex-rs/models-manager/src/resolution.rs deleted file mode 100644 index 512256a06e..0000000000 --- a/codex-rs/models-manager/src/resolution.rs +++ /dev/null @@ -1,32 +0,0 @@ -/// The result of resolving a requested model against a provider's model policy. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct ModelResolution { - pub model: String, - pub fallback: Option, -} - -impl ModelResolution { - pub(crate) fn exact(model: String) -> Self { - Self { - model, - fallback: None, - } - } - - pub(crate) fn fallback(requested_model: String, fallback_model: String) -> Self { - Self { - model: fallback_model.clone(), - fallback: Some(UnsupportedModelFallback { - requested_model, - fallback_model, - }), - } - } -} - -/// Describes a provider-directed fallback from an unsupported requested model. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct UnsupportedModelFallback { - pub requested_model: String, - pub fallback_model: String, -}