From 8a41d4665f0a409eca6242993450203ef20b397f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 15:42:03 -0400 Subject: [PATCH] Simplify model reference parsing and catalog indexing Follow-up cleanup on the provider-qualified selector work. - build_model_indexes now takes the paired (Model, CatalogModelSettings) slice it is built from, instead of a separate settings map. This drops a per-model map lookup with two cloned key components and removes the expect() panic path for an invariant the caller already guarantees. - get_on_provider expresses the exact-then-legacy lookup as one closure applied twice, rather than a nested then/flatten chain. - ModelRef::from_str selects the separator first and then checks both sides once, so the empty-side check is no longer duplicated across two branches and the slash split no longer allocates a Vec. - Shorten the TooManySlashes message to the action the user should take. - Merge the two near-identical fallback chain tests into one that runs both qualified selector forms through the same assertion. - The fallbacks splice test now asserts through the existing Serialize impl instead of hand-rolling the ModelRefOrSplice rendering. Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-workflow/src/operations/start.rs | 107 +++++++----------- .../fabro-config/src/tests/combine.rs | 18 +-- lib/foundation/fabro-model/src/catalog.rs | 34 ++---- .../fabro-types/src/settings/model_ref.rs | 56 ++++----- 4 files changed, 80 insertions(+), 135 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 576850624..8bafc82c1 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -1402,8 +1402,11 @@ reasoning = false }]); } + /// A qualified fallback resolves to the same offering whether the selector + /// is the canonical model ID or the provider's API ID. The trailing bare + /// alias still goes through ready-provider priority selection. #[test] - fn resolve_fallback_chain_supports_openrouter_kimi_then_portable_terra() { + fn resolve_fallback_chain_resolves_qualified_model_id_and_api_id_alike() { let overrides: fabro_model::catalog::LlmCatalogSettings = toml::from_str( r" [providers.openrouter] @@ -1412,76 +1415,44 @@ enabled = true ) .unwrap(); let catalog = Catalog::from_builtin_with_overrides(&overrides).unwrap(); - let settings = ResolvedRunModelSettings { - fallbacks: vec![ - "openrouter:kimi-k3".parse::().unwrap(), - "gpt-terra".parse::().unwrap(), - ], - ..ResolvedRunModelSettings::default() - }; - let chain = resolve_fallback_chain( - &catalog, - &ProviderId::new("kimi"), - "kimi-k3", - &settings, - &HashSet::from([ - ProviderId::new("kimi"), - ProviderId::new("openrouter"), - ProviderId::openai(), - ]), - ) - .unwrap(); + for selector in ["openrouter:kimi-k3", "openrouter:moonshotai/kimi-k3"] { + let settings = ResolvedRunModelSettings { + fallbacks: vec![ + selector.parse::().unwrap(), + "gpt-terra".parse::().unwrap(), + ], + ..ResolvedRunModelSettings::default() + }; - assert_eq!(chain, vec![ - FallbackTarget { - provider: "openrouter".to_string(), - model: "kimi-k3".to_string(), - }, - FallbackTarget { - provider: "openai".to_string(), - model: "gpt-5.6-terra".to_string(), - }, - ]); - } + let chain = resolve_fallback_chain( + &catalog, + &ProviderId::new("kimi"), + "kimi-k3", + &settings, + &HashSet::from([ + ProviderId::new("kimi"), + ProviderId::new("openrouter"), + ProviderId::openai(), + ]), + ) + .unwrap(); - #[test] - fn resolve_fallback_chain_canonicalizes_provider_api_id() { - let overrides: fabro_model::catalog::LlmCatalogSettings = toml::from_str( - r" -[providers.openrouter] -enabled = true -", - ) - .unwrap(); - let catalog = Catalog::from_builtin_with_overrides(&overrides).unwrap(); - let settings = ResolvedRunModelSettings { - fallbacks: vec![ - "openrouter:moonshotai/kimi-k3".parse::().unwrap(), - "gpt-terra".parse::().unwrap(), - ], - ..ResolvedRunModelSettings::default() - }; - - let chain = resolve_fallback_chain( - &catalog, - &ProviderId::new("kimi"), - "kimi-k3", - &settings, - &HashSet::from([ProviderId::new("kimi"), ProviderId::new("openrouter")]), - ) - .unwrap(); - - assert_eq!(chain, vec![ - FallbackTarget { - provider: "openrouter".to_string(), - model: "kimi-k3".to_string(), - }, - FallbackTarget { - provider: "openrouter".to_string(), - model: "gpt-5.6-terra".to_string(), - }, - ]); + assert_eq!( + chain, + vec![ + FallbackTarget { + provider: "openrouter".to_string(), + model: "kimi-k3".to_string(), + }, + FallbackTarget { + provider: "openai".to_string(), + model: "gpt-5.6-terra".to_string(), + }, + ], + "{selector}" + ); + } } #[test] diff --git a/lib/foundation/fabro-config/src/tests/combine.rs b/lib/foundation/fabro-config/src/tests/combine.rs index a66d2634e..426656596 100644 --- a/lib/foundation/fabro-config/src/tests/combine.rs +++ b/lib/foundation/fabro-config/src/tests/combine.rs @@ -7,7 +7,7 @@ use fabro_types::settings::InterpString; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::server::LogDestination; -use crate::{Combine, ModelRefOrSplice, SettingsLayer, StringOrSplice}; +use crate::{Combine, SettingsLayer, StringOrSplice}; fn parse(input: &str) -> SettingsLayer { input @@ -112,18 +112,10 @@ fallbacks = ["anthropic", "..."] ); let merged = higher.combine(lower); let fallbacks = merged.run.unwrap().model.unwrap().fallbacks; - let rendered = fallbacks - .iter() - .map(|entry| match entry { - ModelRefOrSplice::ModelRef(model_ref) => model_ref.to_string(), - ModelRefOrSplice::Splice => "...".to_string(), - }) - .collect::>(); - assert_eq!(rendered, vec![ - "anthropic", - "openrouter:moonshotai/kimi-k3", - "gpt-terra", - ]); + assert_eq!( + serde_json::to_value(&fallbacks).unwrap(), + serde_json::json!(["anthropic", "openrouter:moonshotai/kimi-k3", "gpt-terra",]) + ); } #[test] diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index c250dd800..2bdab1e6f 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -799,14 +799,14 @@ impl Catalog { .then_with(|| left.id.cmp(&right.id)) }); warn_multiple_probe_models(&models_with_settings); + let (offering_index, provider_selector_index, canonical_candidates, alias_candidates) = + build_model_indexes(&models_with_settings); let mut model_settings_by_offering = HashMap::new(); let mut models = Vec::new(); for (model, settings) in models_with_settings { model_settings_by_offering.insert((model.provider.clone(), model.id.clone()), settings); models.push(model); } - let (offering_index, provider_selector_index, canonical_candidates, alias_candidates) = - build_model_indexes(&models, &model_settings_by_offering); Ok(Self { models, @@ -892,19 +892,13 @@ impl Catalog { #[must_use] pub fn get_on_provider(&self, provider: &ProviderId, selector: &str) -> Option<&Model> { let provider = self.provider(provider)?; - let exact = self - .provider_selector_index - .get(&(provider.id.clone(), selector.to_string())); - let index = exact.or_else(|| { - let normalized = normalize_legacy_builtin_selector(selector); - (normalized.as_ref() != selector) - .then(|| { - self.provider_selector_index - .get(&(provider.id.clone(), normalized.into_owned())) - }) - .flatten() - }); - index.and_then(|idx| self.models.get(*idx)) + let lookup = |selector: &str| { + self.provider_selector_index + .get(&(provider.id.clone(), selector.to_string())) + }; + let index = + lookup(selector).or_else(|| lookup(&normalize_legacy_builtin_selector(selector)))?; + self.models.get(*index) } /// Look up a canonical offering by its composite identity. @@ -1489,15 +1483,12 @@ type ModelIndexes = ( HashMap>, ); -fn build_model_indexes( - models: &[Model], - model_settings: &HashMap<(ProviderId, ModelId), CatalogModelSettings>, -) -> ModelIndexes { +fn build_model_indexes(models: &[(Model, CatalogModelSettings)]) -> ModelIndexes { let mut offering_index = HashMap::new(); let mut provider_selector_index = HashMap::new(); let mut canonical_candidates = HashMap::>::new(); let mut alias_candidates = HashMap::>::new(); - for (idx, model) in models.iter().enumerate() { + for (idx, (model, settings)) in models.iter().enumerate() { offering_index.insert((model.provider.clone(), model.id.clone()), idx); provider_selector_index .insert((model.provider.clone(), model.id.as_str().to_string()), idx); @@ -1509,9 +1500,6 @@ fn build_model_indexes( provider_selector_index.insert((model.provider.clone(), alias.clone()), idx); alias_candidates.entry(alias.clone()).or_default().push(idx); } - let settings = model_settings - .get(&(model.provider.clone(), model.id.clone())) - .expect("every catalog model should have resolved settings"); provider_selector_index.insert((model.provider.clone(), settings.api_id.clone()), idx); } ( diff --git a/lib/foundation/fabro-types/src/settings/model_ref.rs b/lib/foundation/fabro-types/src/settings/model_ref.rs index 6d88565ad..7de845a7d 100644 --- a/lib/foundation/fabro-types/src/settings/model_ref.rs +++ b/lib/foundation/fabro-types/src/settings/model_ref.rs @@ -48,7 +48,7 @@ impl fmt::Display for ParseModelRefError { Self::TooManySlashes { input } => { write!( f, - "model reference {input:?}: legacy provider/model references allow one \"/\"; use \"provider:selector\" when the selector contains \"/\"" + "model reference {input:?}: qualify it as \"provider:selector\" when the selector contains \"/\"" ) } Self::EmptySide { input } => { @@ -72,37 +72,31 @@ impl FromStr for ModelRef { return Err(ParseModelRefError::Empty); } - if let Some((provider, selector)) = trimmed.split_once(':') { - if provider.is_empty() || selector.is_empty() { - return Err(ParseModelRefError::EmptySide { - input: input.to_owned(), - }); - } - return Ok(Self::Qualified { - provider: provider.to_owned(), - selector: selector.to_owned(), + // A `:` splits provider from selector. The selector keeps any further + // `:` or `/`, so provider API IDs survive intact. Without a `:`, a + // single `/` is the legacy qualified form. + let (provider, selector) = match trimmed.split_once(':') { + Some(qualified) => qualified, + None => match trimmed.split_once('/') { + Some((_, selector)) if selector.contains('/') => { + return Err(ParseModelRefError::TooManySlashes { + input: input.to_owned(), + }); + } + Some(legacy) => legacy, + None => return Ok(Self::Bare(trimmed.to_owned())), + }, + }; + + if provider.is_empty() || selector.is_empty() { + return Err(ParseModelRefError::EmptySide { + input: input.to_owned(), }); } - - let parts: Vec<&str> = trimmed.split('/').collect(); - match parts.as_slice() { - [bare] => Ok(Self::Bare((*bare).to_owned())), - [provider, selector] => { - if provider.is_empty() || selector.is_empty() { - Err(ParseModelRefError::EmptySide { - input: input.to_owned(), - }) - } else { - Ok(Self::Qualified { - provider: (*provider).to_owned(), - selector: (*selector).to_owned(), - }) - } - } - _ => Err(ParseModelRefError::TooManySlashes { - input: input.to_owned(), - }), - } + Ok(Self::Qualified { + provider: provider.to_owned(), + selector: selector.to_owned(), + }) } } @@ -302,7 +296,7 @@ mod tests { assert!(matches!(err, ParseModelRefError::TooManySlashes { .. })); assert_eq!( err.to_string(), - r#"model reference "a/b/c": legacy provider/model references allow one "/"; use "provider:selector" when the selector contains "/""# + r#"model reference "a/b/c": qualify it as "provider:selector" when the selector contains "/""# ); }