From e0f73f963a23e44cc909fbd0b579ea800475d850 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Jul 2026 18:08:26 -0400 Subject: [PATCH 1/3] feat(workflow): make model fallback chains portable --- .../fabro-workflow/src/operations/start.rs | 409 ++++++++++++++++-- .../fabro-types/src/run_event/infra.rs | 2 + 2 files changed, 364 insertions(+), 47 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 729140879..b85d31717 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -32,7 +32,8 @@ use crate::artifact_upload::ArtifactSink; use crate::context::Context; use crate::error::{self, Error}; use crate::event::{ - Emitter, Event, EventBody, RunEventLogger, RunEventSink, RunNoticeLevel, append_event_to_sink, + Emitter, Event, EventBody, RunEventLogger, RunEventSink, RunNoticeCode, RunNoticeLevel, + append_event_to_sink, }; use crate::handler::HandlerRegistry; use crate::outcome::{Outcome, StageOutcome}; @@ -59,6 +60,7 @@ struct RunSession { emitter: Arc, sandbox: SandboxSpec, llm: LlmSpec, + fallback_notices: Vec, interviewer: Arc, steering_hub: Arc, on_node: crate::OnNodeCallback, @@ -87,9 +89,101 @@ struct RunSession { } struct ResolvedStartLlm { - model: String, - provider_id: ProviderId, - fallback_chain: Vec, + model: String, + provider_id: ProviderId, + fallback_chain: Vec, + fallback_notices: Vec, +} + +#[derive(Debug, Default, PartialEq, Eq)] +struct ResolvedFallbackChain { + targets: Vec, + notices: Vec, +} + +#[derive(Debug, PartialEq, Eq)] +enum ModelFallbackNotice { + ProviderUnconfigured { + reference: String, + provider: ProviderId, + }, + NoConfiguredOffering { + reference: String, + }, + NoCompatibleModel { + reference: String, + provider: ProviderId, + }, + MatchesPrimary { + reference: String, + target: FallbackTarget, + }, + Duplicate { + reference: String, + target: FallbackTarget, + }, + ChainEmpty, +} + +impl ModelFallbackNotice { + fn code(&self) -> RunNoticeCode { + match self { + Self::ChainEmpty => RunNoticeCode::ModelFallbackChainEmpty, + _ => RunNoticeCode::ModelFallbackSkipped, + } + } + + fn level(&self) -> RunNoticeLevel { + match self { + Self::MatchesPrimary { .. } | Self::Duplicate { .. } => RunNoticeLevel::Info, + Self::ProviderUnconfigured { .. } + | Self::NoConfiguredOffering { .. } + | Self::NoCompatibleModel { .. } + | Self::ChainEmpty => RunNoticeLevel::Warn, + } + } + + fn message(&self) -> String { + match self { + Self::ProviderUnconfigured { + reference, + provider, + } => { + format!( + "Model fallback `{reference}` was skipped because provider `{provider}` is not configured." + ) + } + Self::NoConfiguredOffering { reference } => { + format!( + "Model fallback `{reference}` was skipped because none of its providers are configured." + ) + } + Self::NoCompatibleModel { + reference, + provider, + } => { + format!( + "Model fallback `{reference}` was skipped because provider `{provider}` has no compatible model." + ) + } + Self::MatchesPrimary { reference, target } => { + format!( + "Model fallback `{reference}` was skipped because it resolves to the primary target `{}:{}`.", + target.provider, target.model + ) + } + Self::Duplicate { reference, target } => { + format!( + "Model fallback `{reference}` was skipped because target `{}:{}` already appears in the fallback chain.", + target.provider, target.model + ) + } + Self::ChainEmpty => { + "No usable model fallbacks remain after filtering the configured fallback candidates." + .to_string() + } + } + } } pub struct StartServices { @@ -482,6 +576,7 @@ impl RunSession { model_controls: resolved.model.controls.clone(), dry_run: resolved.execution.mode == RunMode::DryRun, }, + fallback_notices: llm.fallback_notices, interviewer, steering_hub: services.steering_hub, on_node: services.on_node, @@ -615,96 +710,153 @@ fn resolve_start_llm( settings.model.provider.as_deref(), false, )?; - let fallback_chain = + let fallback_resolution = resolve_fallback_chain(catalog, &provider_id, &model, &settings.model, &eligible)?; Ok(ResolvedStartLlm { model, provider_id, - fallback_chain, + fallback_chain: fallback_resolution.targets, + fallback_notices: fallback_resolution.notices, }) } +/// Resolve fallback candidates against the configured provider snapshot. +/// +/// Known unconfigured providers, the resolved primary target, and duplicate +/// targets are omitted while the order of the remaining candidates is kept. fn resolve_fallback_chain( catalog: &Catalog, provider: &ProviderId, model: &str, settings: &ResolvedRunModelSettings, eligible: &HashSet, -) -> Result, Error> { +) -> Result { if settings.fallbacks.is_empty() { - return Ok(Vec::new()); + return Ok(ResolvedFallbackChain::default()); } + let primary = catalog.get_on_provider(provider, model); - let mut chain = Vec::new(); + let primary_target = FallbackTarget { + provider: provider.to_string(), + model: model.to_string(), + }; + let mut resolution = ResolvedFallbackChain::default(); + let mut seen = HashSet::new(); for model_ref in &settings.fallbacks { - match model_ref.resolve(catalog)? { + let reference = model_ref.to_string(); + let target = match model_ref.resolve(catalog)? { ResolvedModelRef::Provider(provider_name) => { - let provider_id = canonical_provider_id(catalog, &provider_name); + let provider_id = catalog_provider_id(catalog, &provider_name)?; if !eligible.contains(&provider_id) { - return Err(ModelSelectionError::ProviderUnavailable { - provider: provider_id, - } - .into()); + resolution + .notices + .push(ModelFallbackNotice::ProviderUnconfigured { + reference, + provider: provider_id, + }); + continue; } - if let Some(model) = + let Some(model) = primary.and_then(|reference| catalog.closest(&provider_id, reference)) - { - chain.push(FallbackTarget { - provider: provider_id.to_string(), - model: model.id.to_string(), - }); + else { + resolution + .notices + .push(ModelFallbackNotice::NoCompatibleModel { + reference, + provider: provider_id, + }); + continue; + }; + FallbackTarget { + provider: provider_id.to_string(), + model: model.id.to_string(), } } ResolvedModelRef::Model { provider: fallback_provider, selector, } => { - if let Some(provider) = fallback_provider { - let provider = canonical_provider_id(catalog, &provider); + if let Some(provider_name) = fallback_provider { + let provider = catalog_provider_id(catalog, &provider_name)?; if !eligible.contains(&provider) { - return Err(ModelSelectionError::ProviderUnavailable { provider }.into()); + resolution + .notices + .push(ModelFallbackNotice::ProviderUnconfigured { + reference, + provider, + }); + continue; } match catalog.resolve_on_provider(&provider, &selector) { - Ok(info) => chain.push(FallbackTarget { + Ok(info) => FallbackTarget { provider: info.provider.to_string(), model: info.id.to_string(), - }), + }, Err(ModelSelectionError::UnknownSelectorOnProvider { .. }) => { - chain.push(FallbackTarget { + FallbackTarget { provider: provider.to_string(), model: selector, - }); + } } Err(error) => return Err(error.into()), } } else { match catalog.select(&selector, None, eligible) { - Ok(info) => chain.push(FallbackTarget { + Ok(info) => FallbackTarget { provider: info.provider.to_string(), model: info.id.to_string(), - }), - Err(ModelSelectionError::UnknownSelector { .. }) => { - chain.push(FallbackTarget { - provider: provider.to_string(), - model: selector, - }); + }, + Err(ModelSelectionError::NoEligibleOffering { .. }) => { + resolution + .notices + .push(ModelFallbackNotice::NoConfiguredOffering { reference }); + continue; } + Err(ModelSelectionError::UnknownSelector { .. }) => FallbackTarget { + provider: provider.to_string(), + model: selector, + }, Err(error) => return Err(error.into()), } } } + }; + + if target == primary_target { + resolution + .notices + .push(ModelFallbackNotice::MatchesPrimary { reference, target }); + continue; } + + let target_key = (target.provider.clone(), target.model.clone()); + if !seen.insert(target_key) { + resolution + .notices + .push(ModelFallbackNotice::Duplicate { reference, target }); + continue; + } + resolution.targets.push(target); } - Ok(chain) + + if resolution.targets.is_empty() { + resolution.notices.push(ModelFallbackNotice::ChainEmpty); + } + + Ok(resolution) } -fn canonical_provider_id(catalog: &Catalog, provider_name: &str) -> ProviderId { - let provider_id = ProviderId::from(provider_name); +fn catalog_provider_id( + catalog: &Catalog, + provider_name: &str, +) -> Result { + let provider = ProviderId::from(provider_name); catalog - .provider(&provider_id) - .map_or(provider_id, |provider| provider.id.clone()) + .provider(&provider) + .map(|provider| provider.id.clone()) + .ok_or(ModelSelectionError::UnknownProvider { provider }) } /// Build the launch-time MCP config from resolved settings. Secret tokens in @@ -827,6 +979,10 @@ impl RunSession { let store_progress_logger = RunEventLogger::new(self.event_sink.clone()); store_progress_logger.register(self.emitter.as_ref()); + for notice in &self.fallback_notices { + self.emitter + .notice(notice.level(), notice.code(), notice.message()); + } let init_options = InitOptions { run_store: self.run_store.clone(), @@ -1257,6 +1413,19 @@ tools = true vision = false reasoning = false +[providers.openai.models."gpt-5.4-mini"] +display_name = "GPT-5.4 Mini" +family = "gpt-5" +aliases = ["mini"] + +[providers.openai.models."gpt-5.4-mini".limits] +context_window = 1000 + +[providers.openai.models."gpt-5.4-mini".features] +tools = true +vision = false +reasoning = false + [providers.openrouter] display_name = "OpenRouter" adapter = "openai_compatible" @@ -1283,6 +1452,152 @@ reasoning = false Catalog::from_settings(&settings).unwrap() } + #[test] + fn resolve_start_llm_infers_primary_and_filters_global_fallbacks() { + let catalog = portable_model_catalog(); + let mut settings = ResolvedRunSettings::default(); + settings.model.name = Some("gpt-56-sol".to_string()); + settings.model.fallbacks = vec![ + "openai:gpt-56-sol".parse::().unwrap(), + "openrouter:gpt-56-sol".parse::().unwrap(), + "openrouter:openai/gpt-5.6-sol".parse::().unwrap(), + ]; + + let resolved = resolve_start_llm( + &catalog, + &[ProviderId::new("openrouter"), ProviderId::openai()], + &settings, + ) + .unwrap(); + + assert_eq!(resolved.provider_id, ProviderId::openai()); + assert_eq!(resolved.model, "gpt-5.6-sol"); + assert_eq!(resolved.fallback_chain, vec![FallbackTarget { + provider: "openrouter".to_string(), + model: "gpt-5.6-sol".to_string(), + }]); + assert_eq!(resolved.fallback_notices, vec![ + ModelFallbackNotice::MatchesPrimary { + reference: "openai:gpt-56-sol".to_string(), + target: FallbackTarget { + provider: "openai".to_string(), + model: "gpt-5.6-sol".to_string(), + }, + }, + ModelFallbackNotice::Duplicate { + reference: "openrouter:openai/gpt-5.6-sol".to_string(), + target: FallbackTarget { + provider: "openrouter".to_string(), + model: "gpt-5.6-sol".to_string(), + }, + }, + ]); + } + + #[test] + fn resolve_fallback_chain_skips_unconfigured_provider_and_preserves_order() { + let catalog = test_catalog(); + let settings = ResolvedRunModelSettings { + fallbacks: vec![ + "gemini".parse::().unwrap(), + "gemini:unused".parse::().unwrap(), + "openai:gpt-5.4-mini".parse::().unwrap(), + "anthropic:claude-fable-5".parse::().unwrap(), + ], + ..ResolvedRunModelSettings::default() + }; + + let resolution = resolve_fallback_chain( + catalog.as_ref(), + &ProviderId::anthropic(), + "claude-opus-4-6", + &settings, + &HashSet::from([ProviderId::anthropic(), ProviderId::openai()]), + ) + .unwrap(); + + assert_eq!(resolution.targets, vec![ + FallbackTarget { + provider: "openai".to_string(), + model: "gpt-5.4-mini".to_string(), + }, + FallbackTarget { + provider: "anthropic".to_string(), + model: "claude-fable-5".to_string(), + }, + ]); + assert_eq!(resolution.notices, vec![ + ModelFallbackNotice::ProviderUnconfigured { + reference: "gemini".to_string(), + provider: ProviderId::gemini(), + }, + ModelFallbackNotice::ProviderUnconfigured { + reference: "gemini:unused".to_string(), + provider: ProviderId::gemini(), + }, + ]); + assert_eq!(resolution.notices[0].level(), RunNoticeLevel::Warn); + assert_eq!( + resolution.notices[0].code(), + RunNoticeCode::ModelFallbackSkipped + ); + } + + #[test] + fn resolve_fallback_chain_skips_model_without_configured_offering() { + let catalog = portable_model_catalog(); + let settings = ResolvedRunModelSettings { + fallbacks: vec!["mini".parse::().unwrap()], + ..ResolvedRunModelSettings::default() + }; + + let resolution = resolve_fallback_chain( + &catalog, + &ProviderId::new("openrouter"), + "gpt-5.6-sol", + &settings, + &HashSet::from([ProviderId::new("openrouter")]), + ) + .unwrap(); + + assert!(resolution.targets.is_empty()); + assert_eq!(resolution.notices, vec![ + ModelFallbackNotice::NoConfiguredOffering { + reference: "mini".to_string(), + }, + ModelFallbackNotice::ChainEmpty, + ]); + assert_eq!( + resolution.notices[1].code(), + RunNoticeCode::ModelFallbackChainEmpty + ); + assert_eq!(resolution.notices[1].level(), RunNoticeLevel::Warn); + } + + #[test] + fn resolve_fallback_chain_rejects_unknown_qualified_provider() { + let catalog = portable_model_catalog(); + let settings = ResolvedRunModelSettings { + fallbacks: vec!["missing/model".parse::().unwrap()], + ..ResolvedRunModelSettings::default() + }; + + let error = resolve_fallback_chain( + &catalog, + &ProviderId::openai(), + "gpt-5.6-sol", + &settings, + &catalog.all_provider_ids(), + ) + .unwrap_err(); + + assert!(matches!( + error, + Error::ModelSelection(ModelSelectionError::UnknownProvider { provider }) + if provider == ProviderId::new("missing") + )); + } + #[test] fn resolve_fallback_chain_resolves_provider_fallbacks() { let catalog = test_catalog(); @@ -1300,7 +1615,7 @@ reasoning = false ) .unwrap(); - assert_eq!(chain, vec![FallbackTarget { + assert_eq!(chain.targets, vec![FallbackTarget { provider: "openai".to_string(), model: "gpt-5.5".to_string(), }]); @@ -1323,7 +1638,7 @@ reasoning = false ) .unwrap(); - assert_eq!(chain, vec![FallbackTarget { + assert_eq!(chain.targets, vec![FallbackTarget { provider: "openai".to_string(), model: "gpt-5.4-mini".to_string(), }]); @@ -1346,7 +1661,7 @@ reasoning = false ) .unwrap(); - assert_eq!(chain, vec![FallbackTarget { + assert_eq!(chain.targets, vec![FallbackTarget { provider: "openrouter".to_string(), model: "gpt-5.6-sol".to_string(), }]); @@ -1369,7 +1684,7 @@ reasoning = false ) .unwrap(); - assert_eq!(chain, vec![FallbackTarget { + assert_eq!(chain.targets, vec![FallbackTarget { provider: "openrouter".to_string(), model: "gpt-5.6-sol".to_string(), }]); @@ -1412,7 +1727,7 @@ enabled = true .unwrap(); assert_eq!( - chain, + chain.targets, vec![ FallbackTarget { provider: "openrouter".to_string(), @@ -1448,7 +1763,7 @@ enabled = true ) .unwrap(); - assert_eq!(chain, vec![FallbackTarget { + assert_eq!(chain.targets, vec![FallbackTarget { provider: ProviderId::openai().to_string(), model: "future-model:latest".to_string(), }]); @@ -1474,7 +1789,7 @@ enabled = true ) .unwrap(); - assert_eq!(chain, vec![ + assert_eq!(chain.targets, vec![ FallbackTarget { provider: "openai".to_string(), model: "gpt-5.6-sol".to_string(), diff --git a/lib/foundation/fabro-types/src/run_event/infra.rs b/lib/foundation/fabro-types/src/run_event/infra.rs index 5348b4931..2230e950f 100644 --- a/lib/foundation/fabro-types/src/run_event/infra.rs +++ b/lib/foundation/fabro-types/src/run_event/infra.rs @@ -30,6 +30,8 @@ pub enum RunNoticeCode { GitPushFailed, GithubTokenFailed, GithubTokenRefreshLimited, + ModelFallbackChainEmpty, + ModelFallbackSkipped, PullRequestFailed, SandboxCleanupFailed, SandboxGitUnavailable, From 48fd09aaab6302686113a7a8c4af2cca434f9210 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Jul 2026 22:23:25 -0400 Subject: [PATCH 2/3] refactor(workflow): simplify fallback chain resolution Follow-up cleanup on the portable fallback chain work. - Add `Catalog::require_provider` and `Catalog::provider_id`, replacing the `catalog_provider_id` free function in `start.rs` and two copies of the same `provider(..).ok_or_else(UnknownProvider)` block inside the catalog. - Add `FallbackTarget::new` and use it for the six struct literals that each stringified a provider and model by hand. - Extract per-candidate resolution into `resolve_fallback_candidate`, returning a `FallbackCandidate` that is either a target or the skip reason. This flattens `resolve_fallback_chain` from four levels of nesting to one loop and splits the qualified/unqualified model arms into separate match patterns. - Drop the `seen` HashSet and its per-candidate key clones in favor of a `contains` check on the chain being built; fallback chains hold a handful of entries. Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-workflow/src/operations/start.rs | 189 +++++++++--------- lib/foundation/fabro-model/src/catalog.rs | 47 +++-- 2 files changed, 124 insertions(+), 112 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index b85d31717..e21317596 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -7,7 +7,7 @@ use fabro_auth::{CredentialSource, VaultCredentialSource}; use fabro_interview::{AutoApproveInterviewer, Interviewer}; use fabro_llm::client::Client as LlmClient; use fabro_mcp::config::McpServerSettings; -use fabro_model::{Catalog, FallbackTarget, ModelSelectionError, ProviderId}; +use fabro_model::{Catalog, FallbackTarget, Model, ModelSelectionError, ProviderId}; use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::from_environment::{ daytona_config_from_environment, docker_config_from_environment_with_secrets, @@ -15,12 +15,12 @@ use fabro_sandbox::from_environment::{ }; use fabro_sandbox::{DockerSandboxOptions, SandboxSpec}; use fabro_static::EnvVars; -use fabro_types::settings::ResolvedModelRef; use fabro_types::settings::run::{ ApprovalMode, McpServerSettings as ResolvedMcpServerSettings, PullRequestSettings, ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, RunNamespace as ResolvedRunSettings, RunPrepareSettings as ResolvedRunPrepareSettings, }; +use fabro_types::settings::{ModelRef, ResolvedModelRef}; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; use fabro_vault::Vault; use tokio::runtime::Handle; @@ -737,108 +737,31 @@ fn resolve_fallback_chain( } let primary = catalog.get_on_provider(provider, model); - let primary_target = FallbackTarget { - provider: provider.to_string(), - model: model.to_string(), - }; + let primary_target = FallbackTarget::new(provider, model); let mut resolution = ResolvedFallbackChain::default(); - let mut seen = HashSet::new(); for model_ref in &settings.fallbacks { - let reference = model_ref.to_string(); - let target = match model_ref.resolve(catalog)? { - ResolvedModelRef::Provider(provider_name) => { - let provider_id = catalog_provider_id(catalog, &provider_name)?; - if !eligible.contains(&provider_id) { - resolution - .notices - .push(ModelFallbackNotice::ProviderUnconfigured { - reference, - provider: provider_id, - }); + let target = + match resolve_fallback_candidate(catalog, provider, primary, eligible, model_ref)? { + FallbackCandidate::Skipped(notice) => { + resolution.notices.push(notice); continue; } - let Some(model) = - primary.and_then(|reference| catalog.closest(&provider_id, reference)) - else { - resolution - .notices - .push(ModelFallbackNotice::NoCompatibleModel { - reference, - provider: provider_id, - }); - continue; - }; - FallbackTarget { - provider: provider_id.to_string(), - model: model.id.to_string(), - } - } - ResolvedModelRef::Model { - provider: fallback_provider, - selector, - } => { - if let Some(provider_name) = fallback_provider { - let provider = catalog_provider_id(catalog, &provider_name)?; - if !eligible.contains(&provider) { - resolution - .notices - .push(ModelFallbackNotice::ProviderUnconfigured { - reference, - provider, - }); - continue; - } - match catalog.resolve_on_provider(&provider, &selector) { - Ok(info) => FallbackTarget { - provider: info.provider.to_string(), - model: info.id.to_string(), - }, - Err(ModelSelectionError::UnknownSelectorOnProvider { .. }) => { - FallbackTarget { - provider: provider.to_string(), - model: selector, - } - } - Err(error) => return Err(error.into()), - } - } else { - match catalog.select(&selector, None, eligible) { - Ok(info) => FallbackTarget { - provider: info.provider.to_string(), - model: info.id.to_string(), - }, - Err(ModelSelectionError::NoEligibleOffering { .. }) => { - resolution - .notices - .push(ModelFallbackNotice::NoConfiguredOffering { reference }); - continue; - } - Err(ModelSelectionError::UnknownSelector { .. }) => FallbackTarget { - provider: provider.to_string(), - model: selector, - }, - Err(error) => return Err(error.into()), - } - } - } - }; + FallbackCandidate::Target(target) => target, + }; + let reference = model_ref.to_string(); if target == primary_target { resolution .notices .push(ModelFallbackNotice::MatchesPrimary { reference, target }); - continue; - } - - let target_key = (target.provider.clone(), target.model.clone()); - if !seen.insert(target_key) { + } else if resolution.targets.contains(&target) { resolution .notices .push(ModelFallbackNotice::Duplicate { reference, target }); - continue; + } else { + resolution.targets.push(target); } - resolution.targets.push(target); } if resolution.targets.is_empty() { @@ -848,15 +771,85 @@ fn resolve_fallback_chain( Ok(resolution) } -fn catalog_provider_id( +/// The outcome of resolving one fallback candidate: either a dispatchable +/// target or the reason the candidate cannot be used. +enum FallbackCandidate { + Target(FallbackTarget), + Skipped(ModelFallbackNotice), +} + +/// Resolve one fallback reference against the configured provider snapshot. +/// +/// `primary` is the primary offering, used to pick the closest capability match +/// when a candidate names a provider but no model. Unknown selectors pinned to +/// a configured provider pass through verbatim so a model newer than the +/// catalog still dispatches; candidates whose provider is unconfigured are +/// skipped. +fn resolve_fallback_candidate( catalog: &Catalog, - provider_name: &str, -) -> Result { - let provider = ProviderId::from(provider_name); - catalog - .provider(&provider) - .map(|provider| provider.id.clone()) - .ok_or(ModelSelectionError::UnknownProvider { provider }) + primary_provider: &ProviderId, + primary: Option<&Model>, + eligible: &HashSet, + model_ref: &ModelRef, +) -> Result { + let reference = model_ref.to_string(); + + Ok(match model_ref.resolve(catalog)? { + ResolvedModelRef::Provider(provider_name) => { + let provider = catalog.provider_id(&provider_name)?; + if !eligible.contains(&provider) { + return Ok(FallbackCandidate::Skipped( + ModelFallbackNotice::ProviderUnconfigured { + reference, + provider, + }, + )); + } + match primary.and_then(|primary| catalog.closest(&provider, primary)) { + Some(model) => FallbackCandidate::Target(FallbackTarget::new(provider, &model.id)), + None => FallbackCandidate::Skipped(ModelFallbackNotice::NoCompatibleModel { + reference, + provider, + }), + } + } + ResolvedModelRef::Model { + provider: Some(provider_name), + selector, + } => { + let provider = catalog.provider_id(&provider_name)?; + if !eligible.contains(&provider) { + return Ok(FallbackCandidate::Skipped( + ModelFallbackNotice::ProviderUnconfigured { + reference, + provider, + }, + )); + } + match catalog.resolve_on_provider(&provider, &selector) { + Ok(info) => { + FallbackCandidate::Target(FallbackTarget::new(&info.provider, &info.id)) + } + Err(ModelSelectionError::UnknownSelectorOnProvider { .. }) => { + FallbackCandidate::Target(FallbackTarget::new(provider, selector)) + } + Err(error) => return Err(error.into()), + } + } + ResolvedModelRef::Model { + provider: None, + selector, + } => match catalog.select(&selector, None, eligible) { + Ok(info) => FallbackCandidate::Target(FallbackTarget::new(&info.provider, &info.id)), + Err(ModelSelectionError::NoEligibleOffering { .. }) => { + FallbackCandidate::Skipped(ModelFallbackNotice::NoConfiguredOffering { reference }) + } + Err(ModelSelectionError::UnknownSelector { .. }) => { + FallbackCandidate::Target(FallbackTarget::new(primary_provider, selector)) + } + Err(error) => return Err(error.into()), + }, + }) } /// Build the launch-time MCP config from resolved settings. Secret tokens in diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index e9b46fcc4..ba799a361 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -405,6 +405,18 @@ pub struct FallbackTarget { pub model: String, } +impl FallbackTarget { + /// Build a target from anything that renders as a provider name and model + /// ID, so callers holding [`ProviderId`]/[`ModelId`] or bare passthrough + /// selectors all use one constructor. + pub fn new(provider: impl std::fmt::Display, model: impl std::fmt::Display) -> Self { + Self { + provider: provider.to_string(), + model: model.to_string(), + } + } +} + #[derive(Debug, Clone, PartialEq)] pub struct CatalogProvider { pub id: ProviderId, @@ -910,17 +922,30 @@ impl Catalog { .and_then(|idx| self.models.get(*idx)) } + /// Look up a provider by ID or alias, failing when the catalog has no such + /// provider. + pub fn require_provider( + &self, + provider: &ProviderId, + ) -> Result<&CatalogProvider, ModelSelectionError> { + self.provider(provider) + .ok_or_else(|| ModelSelectionError::UnknownProvider { + provider: provider.clone(), + }) + } + + /// Canonicalize a provider name or alias to its catalog ID. + pub fn provider_id(&self, name: &str) -> Result { + Ok(self.require_provider(&ProviderId::from(name))?.id.clone()) + } + /// Resolve a canonical ID, alias, or API ID on exactly one provider. pub fn resolve_on_provider( &self, provider: &ProviderId, selector: &str, ) -> Result<&Model, ModelSelectionError> { - let provider = - self.provider(provider) - .ok_or_else(|| ModelSelectionError::UnknownProvider { - provider: provider.clone(), - })?; + let provider = self.require_provider(provider)?; if let Some(model) = self.get_on_provider(&provider.id, selector) { return Ok(model); } @@ -1056,11 +1081,7 @@ impl Catalog { provider: &ProviderId, eligible_providers: &HashSet, ) -> Result { - let provider = - self.provider(provider) - .ok_or_else(|| ModelSelectionError::UnknownProvider { - provider: provider.clone(), - })?; + let provider = self.require_provider(provider)?; let ready = eligible_providers.iter().any(|eligible| { self.provider(eligible) .is_some_and(|eligible| eligible.id == provider.id) @@ -1482,10 +1503,8 @@ impl Catalog { .iter() .filter_map(|provider_str| { let provider = ProviderId::from(provider_str.clone()); - self.closest(&provider, reference).map(|m| FallbackTarget { - provider: provider_str.clone(), - model: m.id.to_string(), - }) + self.closest(&provider, reference) + .map(|m| FallbackTarget::new(provider_str, &m.id)) }) .collect() } From 8a597ca2640d8c97146158a5d2a7c9ad3f5f03b0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Jul 2026 22:37:51 -0400 Subject: [PATCH 3/3] fix(workflow): report the real cause when a fallback cannot resolve Addresses review feedback on the fallback notice work. A provider-only fallback such as `openrouter` needs the primary model's catalog entry to find the closest capability match. When the primary is itself a passthrough selector there is no entry, so every provider-only candidate was skipped with "provider `X` has no compatible model" even when that provider had plenty. Adds a `PrimaryNotInCatalog` notice that names the missing primary instead of blaming the provider. Also from review: - `code()` was a wildcard fallthrough, which docs/internal/events-strategy.md forbids for new variants. Now exhaustive. - `NoConfiguredOffering` discarded the `providers` list that `NoEligibleOffering` hands it. The notice now names the providers that do offer the model. - `ModelFallbackNotice::reference` was a rendered `String`; it is now the `ModelRef` it came from, which also drops the per-candidate double allocation the previous refactor introduced. - `ResolvedStartLlm` unpacked and repacked `ResolvedFallbackChain` field-for-field; it now holds it directly. - Added `FallbackTarget: Display` as `provider:model`, replacing two hand-written `"{}:{}"` format strings. - `Catalog::select` still inlined the `require_provider` body. - Emission moved to `ModelFallbackNotice::emit_all`, covered by a new test proving notices reach the event stream with the right level, code, and message. Nothing tested that hand-off before. Documented in `resolve_fallback_chain` why an unknown provider stays a hard error while an unconfigured one is skipped, and that an unqualified unknown selector pins to the primary's provider. Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-workflow/src/operations/start.rs | 277 ++++++++++++++---- lib/foundation/fabro-model/src/catalog.rs | 14 +- 2 files changed, 228 insertions(+), 63 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index e21317596..54d3ae572 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -89,10 +89,9 @@ struct RunSession { } struct ResolvedStartLlm { - model: String, - provider_id: ProviderId, - fallback_chain: Vec, - fallback_notices: Vec, + model: String, + provider_id: ProviderId, + fallbacks: ResolvedFallbackChain, } #[derive(Debug, Default, PartialEq, Eq)] @@ -101,25 +100,35 @@ struct ResolvedFallbackChain { notices: Vec, } +/// Why one fallback candidate did not make it into the chain, or that the whole +/// chain came out empty. Resolution happens before the run's event sink is +/// wired up, so these are carried to [`RunSession::run`] and emitted there. #[derive(Debug, PartialEq, Eq)] enum ModelFallbackNotice { ProviderUnconfigured { - reference: String, + reference: ModelRef, provider: ProviderId, }, NoConfiguredOffering { - reference: String, + reference: ModelRef, + providers: Vec, + }, + /// The candidate named a provider but no model, and the primary model is + /// not in the catalog, so there is nothing to match its capabilities to. + PrimaryNotInCatalog { + reference: ModelRef, + primary: FallbackTarget, }, NoCompatibleModel { - reference: String, + reference: ModelRef, provider: ProviderId, }, MatchesPrimary { - reference: String, + reference: ModelRef, target: FallbackTarget, }, Duplicate { - reference: String, + reference: ModelRef, target: FallbackTarget, }, ChainEmpty, @@ -129,7 +138,12 @@ impl ModelFallbackNotice { fn code(&self) -> RunNoticeCode { match self { Self::ChainEmpty => RunNoticeCode::ModelFallbackChainEmpty, - _ => RunNoticeCode::ModelFallbackSkipped, + Self::ProviderUnconfigured { .. } + | Self::NoConfiguredOffering { .. } + | Self::PrimaryNotInCatalog { .. } + | Self::NoCompatibleModel { .. } + | Self::MatchesPrimary { .. } + | Self::Duplicate { .. } => RunNoticeCode::ModelFallbackSkipped, } } @@ -138,6 +152,7 @@ impl ModelFallbackNotice { Self::MatchesPrimary { .. } | Self::Duplicate { .. } => RunNoticeLevel::Info, Self::ProviderUnconfigured { .. } | Self::NoConfiguredOffering { .. } + | Self::PrimaryNotInCatalog { .. } | Self::NoCompatibleModel { .. } | Self::ChainEmpty => RunNoticeLevel::Warn, } @@ -153,9 +168,22 @@ impl ModelFallbackNotice { "Model fallback `{reference}` was skipped because provider `{provider}` is not configured." ) } - Self::NoConfiguredOffering { reference } => { + Self::NoConfiguredOffering { + reference, + providers, + } => { + let providers = providers + .iter() + .map(ProviderId::to_string) + .collect::>() + .join(", "); format!( - "Model fallback `{reference}` was skipped because none of its providers are configured." + "Model fallback `{reference}` was skipped because none of its providers are configured. It is offered by: {providers}." + ) + } + Self::PrimaryNotInCatalog { reference, primary } => { + format!( + "Model fallback `{reference}` was skipped because the primary model `{primary}` is not in the catalog, so there is no capability profile to match against." ) } Self::NoCompatibleModel { @@ -168,14 +196,12 @@ impl ModelFallbackNotice { } Self::MatchesPrimary { reference, target } => { format!( - "Model fallback `{reference}` was skipped because it resolves to the primary target `{}:{}`.", - target.provider, target.model + "Model fallback `{reference}` was skipped because it resolves to the primary target `{target}`." ) } Self::Duplicate { reference, target } => { format!( - "Model fallback `{reference}` was skipped because target `{}:{}` already appears in the fallback chain.", - target.provider, target.model + "Model fallback `{reference}` was skipped because target `{target}` already appears in the fallback chain." ) } Self::ChainEmpty => { @@ -184,6 +210,13 @@ impl ModelFallbackNotice { } } } + + /// Publish every notice on the run's event stream. + fn emit_all(notices: &[Self], emitter: &Emitter) { + for notice in notices { + emitter.notice(notice.level(), notice.code(), notice.message()); + } + } } pub struct StartServices { @@ -571,12 +604,12 @@ impl RunSession { llm: LlmSpec { model: llm.model.clone(), provider_id: llm.provider_id.clone(), - fallback_chain: llm.fallback_chain, + fallback_chain: llm.fallbacks.targets, mcp_servers, model_controls: resolved.model.controls.clone(), dry_run: resolved.execution.mode == RunMode::DryRun, }, - fallback_notices: llm.fallback_notices, + fallback_notices: llm.fallbacks.notices, interviewer, steering_hub: services.steering_hub, on_node: services.on_node, @@ -710,21 +743,27 @@ fn resolve_start_llm( settings.model.provider.as_deref(), false, )?; - let fallback_resolution = + let fallbacks = resolve_fallback_chain(catalog, &provider_id, &model, &settings.model, &eligible)?; Ok(ResolvedStartLlm { model, provider_id, - fallback_chain: fallback_resolution.targets, - fallback_notices: fallback_resolution.notices, + fallbacks, }) } /// Resolve fallback candidates against the configured provider snapshot. /// -/// Known unconfigured providers, the resolved primary target, and duplicate -/// targets are omitted while the order of the remaining candidates is kept. +/// Candidates that cannot be used in this environment — an unconfigured +/// provider, no compatible model, a target equal to the primary, or a duplicate +/// — are dropped, and each drop records a [`ModelFallbackNotice`] that the run +/// emits at startup. Remaining candidates keep their configured order. +/// +/// A provider the catalog has never heard of is a different case: that is a +/// typo rather than an environment difference, so it fails the run instead of +/// being skipped. This is what keeps a chain portable without letting a +/// misspelled provider silently disappear. fn resolve_fallback_chain( catalog: &Catalog, provider: &ProviderId, @@ -736,22 +775,27 @@ fn resolve_fallback_chain( return Ok(ResolvedFallbackChain::default()); } - let primary = catalog.get_on_provider(provider, model); - let primary_target = FallbackTarget::new(provider, model); + let primary_model = catalog.get_on_provider(provider, model); + let primary = FallbackTarget::new(provider, model); let mut resolution = ResolvedFallbackChain::default(); for model_ref in &settings.fallbacks { - let target = - match resolve_fallback_candidate(catalog, provider, primary, eligible, model_ref)? { - FallbackCandidate::Skipped(notice) => { - resolution.notices.push(notice); - continue; - } - FallbackCandidate::Target(target) => target, - }; + let target = match resolve_fallback_candidate( + catalog, + &primary, + primary_model, + eligible, + model_ref, + )? { + FallbackCandidate::Skipped(notice) => { + resolution.notices.push(notice); + continue; + } + FallbackCandidate::Target(target) => target, + }; - let reference = model_ref.to_string(); - if target == primary_target { + let reference = model_ref.clone(); + if target == primary { resolution .notices .push(ModelFallbackNotice::MatchesPrimary { reference, target }); @@ -780,19 +824,23 @@ enum FallbackCandidate { /// Resolve one fallback reference against the configured provider snapshot. /// -/// `primary` is the primary offering, used to pick the closest capability match -/// when a candidate names a provider but no model. Unknown selectors pinned to -/// a configured provider pass through verbatim so a model newer than the -/// catalog still dispatches; candidates whose provider is unconfigured are -/// skipped. +/// `primary_model` is the primary's catalog entry, used to pick the closest +/// capability match when a candidate names a provider but no model. It is +/// `None` when the primary is itself a passthrough selector. +/// +/// A selector the catalog does not know passes through verbatim so a model +/// newer than the catalog still dispatches. When the candidate named a +/// provider, it passes through on that provider; when it did not, it passes +/// through on the primary's provider, which means such a fallback gives no +/// cross-provider failover. fn resolve_fallback_candidate( catalog: &Catalog, - primary_provider: &ProviderId, - primary: Option<&Model>, + primary: &FallbackTarget, + primary_model: Option<&Model>, eligible: &HashSet, model_ref: &ModelRef, ) -> Result { - let reference = model_ref.to_string(); + let reference = model_ref.clone(); Ok(match model_ref.resolve(catalog)? { ResolvedModelRef::Provider(provider_name) => { @@ -805,7 +853,17 @@ fn resolve_fallback_candidate( }, )); } - match primary.and_then(|primary| catalog.closest(&provider, primary)) { + // Without a catalog entry for the primary there is no capability + // profile to match against, which is not the provider's fault. + let Some(primary_model) = primary_model else { + return Ok(FallbackCandidate::Skipped( + ModelFallbackNotice::PrimaryNotInCatalog { + reference, + primary: primary.clone(), + }, + )); + }; + match catalog.closest(&provider, primary_model) { Some(model) => FallbackCandidate::Target(FallbackTarget::new(provider, &model.id)), None => FallbackCandidate::Skipped(ModelFallbackNotice::NoCompatibleModel { reference, @@ -841,11 +899,14 @@ fn resolve_fallback_candidate( selector, } => match catalog.select(&selector, None, eligible) { Ok(info) => FallbackCandidate::Target(FallbackTarget::new(&info.provider, &info.id)), - Err(ModelSelectionError::NoEligibleOffering { .. }) => { - FallbackCandidate::Skipped(ModelFallbackNotice::NoConfiguredOffering { reference }) + Err(ModelSelectionError::NoEligibleOffering { providers, .. }) => { + FallbackCandidate::Skipped(ModelFallbackNotice::NoConfiguredOffering { + reference, + providers, + }) } Err(ModelSelectionError::UnknownSelector { .. }) => { - FallbackCandidate::Target(FallbackTarget::new(primary_provider, selector)) + FallbackCandidate::Target(FallbackTarget::new(&primary.provider, selector)) } Err(error) => return Err(error.into()), }, @@ -972,10 +1033,9 @@ impl RunSession { let store_progress_logger = RunEventLogger::new(self.event_sink.clone()); store_progress_logger.register(self.emitter.as_ref()); - for notice in &self.fallback_notices { - self.emitter - .notice(notice.level(), notice.code(), notice.message()); - } + // Emit after the logger is registered so the notices reach the run + // store, and before `run.started` so they read as launch-time context. + ModelFallbackNotice::emit_all(&self.fallback_notices, self.emitter.as_ref()); let init_options = InitOptions { run_store: self.run_store.clone(), @@ -1465,20 +1525,20 @@ reasoning = false assert_eq!(resolved.provider_id, ProviderId::openai()); assert_eq!(resolved.model, "gpt-5.6-sol"); - assert_eq!(resolved.fallback_chain, vec![FallbackTarget { + assert_eq!(resolved.fallbacks.targets, vec![FallbackTarget { provider: "openrouter".to_string(), model: "gpt-5.6-sol".to_string(), }]); - assert_eq!(resolved.fallback_notices, vec![ + assert_eq!(resolved.fallbacks.notices, vec![ ModelFallbackNotice::MatchesPrimary { - reference: "openai:gpt-56-sol".to_string(), + reference: "openai:gpt-56-sol".parse().unwrap(), target: FallbackTarget { provider: "openai".to_string(), model: "gpt-5.6-sol".to_string(), }, }, ModelFallbackNotice::Duplicate { - reference: "openrouter:openai/gpt-5.6-sol".to_string(), + reference: "openrouter:openai/gpt-5.6-sol".parse().unwrap(), target: FallbackTarget { provider: "openrouter".to_string(), model: "gpt-5.6-sol".to_string(), @@ -1521,11 +1581,11 @@ reasoning = false ]); assert_eq!(resolution.notices, vec![ ModelFallbackNotice::ProviderUnconfigured { - reference: "gemini".to_string(), + reference: "gemini".parse().unwrap(), provider: ProviderId::gemini(), }, ModelFallbackNotice::ProviderUnconfigured { - reference: "gemini:unused".to_string(), + reference: "gemini:unused".parse().unwrap(), provider: ProviderId::gemini(), }, ]); @@ -1536,6 +1596,99 @@ reasoning = false ); } + /// The resolver builds notices before the run's event sink exists, so this + /// covers the hand-off: each notice must reach the event stream as a + /// `run.notice` carrying its own level, code, and rendered message. + #[test] + fn fallback_notices_reach_the_event_stream() { + let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); + let captured = Arc::new(Mutex::new(Vec::new())); + let sink = Arc::clone(&captured); + emitter.on_event(move |event| sink.lock().unwrap().push(event.clone())); + + let notices = vec![ + ModelFallbackNotice::ProviderUnconfigured { + reference: "gemini".parse().unwrap(), + provider: ProviderId::gemini(), + }, + ModelFallbackNotice::MatchesPrimary { + reference: "openai:gpt-5.6-sol".parse().unwrap(), + target: FallbackTarget::new("openai", "gpt-5.6-sol"), + }, + ModelFallbackNotice::ChainEmpty, + ]; + + ModelFallbackNotice::emit_all(¬ices, emitter.as_ref()); + + let events = captured.lock().unwrap(); + let emitted = events + .iter() + .map(|event| match &event.body { + EventBody::RunNotice(props) => { + (props.level, props.code.clone(), props.message.clone()) + } + other => panic!("expected run.notice body, got {other:?}"), + }) + .collect::>(); + + assert_eq!(emitted, vec![ + ( + RunNoticeLevel::Warn, + RunNoticeCode::ModelFallbackSkipped.to_string(), + "Model fallback `gemini` was skipped because provider `gemini` is not configured." + .to_string(), + ), + ( + RunNoticeLevel::Info, + RunNoticeCode::ModelFallbackSkipped.to_string(), + "Model fallback `openai:gpt-5.6-sol` was skipped because it resolves to the primary target `openai:gpt-5.6-sol`." + .to_string(), + ), + ( + RunNoticeLevel::Warn, + RunNoticeCode::ModelFallbackChainEmpty.to_string(), + "No usable model fallbacks remain after filtering the configured fallback candidates." + .to_string(), + ), + ]); + } + + /// A provider-only fallback cannot be matched when the primary model is a + /// passthrough selector, because there is no capability profile to compare + /// against. The notice must name that cause rather than blaming the + /// provider, which may well have compatible models. + #[test] + fn resolve_fallback_chain_blames_missing_primary_not_the_fallback_provider() { + let catalog = portable_model_catalog(); + let settings = ResolvedRunModelSettings { + fallbacks: vec!["openrouter".parse::().unwrap()], + ..ResolvedRunModelSettings::default() + }; + + let resolution = resolve_fallback_chain( + &catalog, + &ProviderId::openai(), + "gpt-5.9-not-in-catalog", + &settings, + &HashSet::from([ProviderId::openai(), ProviderId::new("openrouter")]), + ) + .unwrap(); + + assert!(resolution.targets.is_empty()); + assert_eq!(resolution.notices, vec![ + ModelFallbackNotice::PrimaryNotInCatalog { + reference: "openrouter".parse().unwrap(), + primary: FallbackTarget::new("openai", "gpt-5.9-not-in-catalog"), + }, + ModelFallbackNotice::ChainEmpty, + ]); + let message = resolution.notices[0].message(); + assert!( + message.contains("primary model `openai:gpt-5.9-not-in-catalog` is not in the catalog"), + "notice should name the missing primary: {message}" + ); + } + #[test] fn resolve_fallback_chain_skips_model_without_configured_offering() { let catalog = portable_model_catalog(); @@ -1556,7 +1709,8 @@ reasoning = false assert!(resolution.targets.is_empty()); assert_eq!(resolution.notices, vec![ ModelFallbackNotice::NoConfiguredOffering { - reference: "mini".to_string(), + reference: "mini".parse().unwrap(), + providers: vec![ProviderId::openai()], }, ModelFallbackNotice::ChainEmpty, ]); @@ -1565,6 +1719,13 @@ reasoning = false RunNoticeCode::ModelFallbackChainEmpty ); assert_eq!(resolution.notices[1].level(), RunNoticeLevel::Warn); + assert!( + resolution.notices[0] + .message() + .contains("offered by: openai"), + "notice should name the providers that offer the model: {}", + resolution.notices[0].message() + ); } #[test] diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index ba799a361..2e16f378c 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -417,6 +417,14 @@ impl FallbackTarget { } } +impl std::fmt::Display for FallbackTarget { + /// Renders as `provider:model`, matching the qualified form accepted by + /// model references. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}:{}", self.provider, self.model) + } +} + #[derive(Debug, Clone, PartialEq)] pub struct CatalogProvider { pub id: ProviderId, @@ -976,11 +984,7 @@ impl Catalog { .collect::>(); if let Some(explicit_provider) = explicit_provider { - let provider = self.provider(explicit_provider).ok_or_else(|| { - ModelSelectionError::UnknownProvider { - provider: explicit_provider.clone(), - } - })?; + let provider = self.require_provider(explicit_provider)?; if !eligible.contains(&provider.id) { return Err(ModelSelectionError::ProviderUnavailable { provider: provider.id.clone(),