diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 9ded5ee70..bc0fd7f3a 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -7943,7 +7943,9 @@ components: description: | Catalog model ID or alias, optionally qualified as `provider:selector`. A provider-qualified selector may be a - canonical model ID, alias, or provider API ID. Legacy + canonical model ID, alias, or provider API ID. A value counts as + qualified only when the text before the first `:` names a known + provider, so model IDs containing a colon stay whole. Legacy `provider/model` references remain accepted. The server stores the canonical model ID. provider: @@ -14126,7 +14128,10 @@ components: A fallback model reference. Bare values name a provider, canonical model ID, or alias. Provider-qualified values use `provider:selector`; the selector may be a canonical model ID, alias, - or provider API ID and may contain `/` or additional colons. Legacy + or provider API ID and may contain `/` or additional colons. A value + is treated as qualified only when the text before the first `:` names + a known provider, so model IDs that contain a colon — ollama + `name:tag` values, Bedrock inference-profile IDs — stay whole. Legacy `provider/model` references remain accepted. example: openrouter:moonshotai/kimi-k3 diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 1f1a2dd8e..665efeadc 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -144,7 +144,7 @@ name = "claude-sonnet-4-5" Provider values are catalog provider ID strings. Built-in IDs like `anthropic` and `openai` work, and settings-defined IDs like `proxy` work after they are added under `[llm.providers.]`. -For a qualified fallback, the selector may be that provider's canonical model ID, alias, or API ID. Fabro splits on the first `:`, so provider API IDs may contain `/` or additional colons: +For a qualified fallback, the selector may be that provider's canonical model ID, alias, or API ID. Fabro splits on the first `:` when the part before it names a known provider, so provider API IDs may contain `/` or additional colons: ```toml title="run.toml" [run.model] @@ -156,6 +156,8 @@ fallbacks = [ The first entry could equivalently be written as `"openrouter:moonshotai/kimi-k3"` using OpenRouter's API ID; both forms resolve to its canonical `kimi-k3` offering. The unqualified `gpt-terra` alias uses normal ready-provider priority selection. Legacy `provider/model` fallback references remain accepted but are normalized to `provider:model`. +A colon alone does not make a reference qualified. Many model IDs contain one — ollama `name:tag` values, Bedrock inference-profile IDs and ARNs — so Fabro treats the reference as qualified only when the text before the first `:` names a known provider. `"llama3:8b"` stays a single model ID, while `"ollama:llama3:8b"` pins the `ollama` provider and passes `llama3:8b` as the selector. + At run creation, Fabro resolves the primary selector and every node selector against the ready-provider snapshot. It persists the selected canonical model slug and provider, so resuming the run does not choose a different provider just because credentials or priorities changed. The configured fallback chain remains available for failures that occur while the materialized run is executing. Historical built-in provider API IDs are accepted for compatibility and normalize before this selection. For example, `name = "openai/gpt-5.6-sol"` is treated as the canonical `gpt-5.6-sol` selector; omit `provider` to use readiness and priority, or set `provider` separately to pin an offering. diff --git a/lib/apps/fabro-server/src/server/handler/sessions.rs b/lib/apps/fabro-server/src/server/handler/sessions.rs index 2cc9c8168..dce2e6891 100644 --- a/lib/apps/fabro-server/src/server/handler/sessions.rs +++ b/lib/apps/fabro-server/src/server/handler/sessions.rs @@ -857,7 +857,8 @@ fn canonical_session_model( } let model_ref = requested .parse::() - .map_err(|err| ApiError::bad_request(err.to_string()))?; + .map_err(|err| ApiError::bad_request(err.to_string()))? + .qualify(catalog); let (qualified_provider, selector) = match model_ref { SettingsModelRef::Qualified { provider, selector } => { let requested_provider = ProviderId::new(provider); @@ -1629,6 +1630,31 @@ reasoning = false ); } + /// A colon in a model ID does not make it provider-qualified. Ollama + /// `name:tag` values and Bedrock ARNs must still reach the provider. + #[test] + fn canonical_session_model_passes_through_colon_bearing_model_ids() { + let catalog = portable_session_catalog(); + let openai = ProviderId::openai(); + let openrouter = ProviderId::new("openrouter"); + let both = std::collections::HashSet::from([openai.clone(), openrouter.clone()]); + + assert_eq!( + canonical_session_model( + &catalog, + &both, + Some("future-model:latest"), + Some(&openrouter), + ) + .unwrap(), + (openrouter, "future-model:latest".to_string()) + ); + assert_eq!( + canonical_session_model(&catalog, &both, Some("future-model:latest"), None).unwrap(), + (openai, "future-model:latest".to_string()) + ); + } + #[test] fn canonical_session_model_rejects_an_unavailable_explicit_provider() { let catalog = portable_session_catalog(); diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 8bafc82c1..220b1cadd 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -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::{ModelRegistry, ResolvedModelRef}; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; use fabro_vault::Vault; use tokio::runtime::Handle; @@ -645,12 +645,11 @@ fn resolve_fallback_chain( if settings.fallbacks.is_empty() { return Ok(Vec::new()); } - let registry = CatalogModelRegistry { catalog }; let primary = catalog.get_on_provider(provider, model); let mut chain = Vec::new(); for model_ref in &settings.fallbacks { - match model_ref.resolve(®istry)? { + match model_ref.resolve(catalog)? { ResolvedModelRef::Provider(provider_name) => { let provider_id = canonical_provider_id(catalog, &provider_name); if !eligible.contains(&provider_id) { @@ -718,20 +717,6 @@ fn canonical_provider_id(catalog: &Catalog, provider_name: &str) -> ProviderId { .map_or(provider_id, |provider| provider.id.clone()) } -struct CatalogModelRegistry<'a> { - catalog: &'a Catalog, -} - -impl ModelRegistry for CatalogModelRegistry<'_> { - fn is_provider(&self, token: &str) -> bool { - self.catalog.provider(&ProviderId::from(token)).is_some() - } - - fn is_model(&self, token: &str) -> bool { - self.catalog.is_model_selector(token) - } -} - /// Build the launch-time MCP config from resolved settings, resolving any /// `{{ env.* }}` and `{{ secrets.* }}` tokens in the transport /// (`command`/`url`/`env`/`headers`) against the worker process environment and @@ -1455,6 +1440,32 @@ enabled = true } } + /// A colon in a model ID does not make it provider-qualified, so an + /// unknown colon-bearing selector still passes through to a provider + /// instead of failing the run with an unknown-provider error. + #[test] + fn resolve_fallback_chain_passes_through_colon_bearing_model_ids() { + let catalog = portable_model_catalog(); + let settings = ResolvedRunModelSettings { + fallbacks: vec!["future-model:latest".parse::().unwrap()], + ..ResolvedRunModelSettings::default() + }; + + let chain = resolve_fallback_chain( + &catalog, + &ProviderId::openai(), + "gpt-5.6-sol", + &settings, + &HashSet::from([ProviderId::openai(), ProviderId::new("openrouter")]), + ) + .unwrap(); + + assert_eq!(chain, vec![FallbackTarget { + provider: ProviderId::openai().to_string(), + model: "future-model:latest".to_string(), + }]); + } + #[test] fn resolve_fallback_chain_keeps_qualified_legacy_references_as_provider_pins() { let catalog = test_catalog(); diff --git a/lib/foundation/fabro-types/src/settings/model_ref.rs b/lib/foundation/fabro-types/src/settings/model_ref.rs index 7de845a7d..1529a5888 100644 --- a/lib/foundation/fabro-types/src/settings/model_ref.rs +++ b/lib/foundation/fabro-types/src/settings/model_ref.rs @@ -13,6 +13,12 @@ //! The parser produces [`ModelRef`]; ambiguity resolution against a known //! registry of providers and models happens at consumption time via //! [`ModelRef::resolve`]. +//! +//! That split matters for the `:` form. Model IDs legitimately contain colons — +//! ollama `name:tag` values, Bedrock inference-profile ARNs — so no separator +//! is safe to split on by shape alone. Parsing leaves colon-bearing tokens bare +//! and [`ModelRef::qualify`] promotes only the ones whose prefix names a +//! provider. use std::fmt; use std::str::FromStr; @@ -72,22 +78,25 @@ impl FromStr for ModelRef { return Err(ParseModelRefError::Empty); } - // 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())), - }, - }; + // A `:` may separate provider from selector, but model IDs legitimately + // contain colons — ollama `name:tag` values, Bedrock ARNs. Only a + // registry can tell the two apart, so colon-bearing tokens stay bare + // here and [`ModelRef::qualify`] promotes the ones that name a + // provider. + if trimmed.contains(':') { + return Ok(Self::Bare(trimmed.to_owned())); + } + // Legacy `provider/model`. A selector with a further `/` needs the + // `provider:selector` form. + let Some((provider, selector)) = trimmed.split_once('/') else { + return Ok(Self::Bare(trimmed.to_owned())); + }; + if selector.contains('/') { + return Err(ParseModelRefError::TooManySlashes { + input: input.to_owned(), + }); + } if provider.is_empty() || selector.is_empty() { return Err(ParseModelRefError::EmptySide { input: input.to_owned(), @@ -153,9 +162,37 @@ pub trait ModelRegistry { } impl ModelRef { + /// Promote a bare `provider:selector` token to [`ModelRef::Qualified`] when + /// the prefix names a known provider. + /// + /// Parsing alone cannot do this. A model ID may itself contain a colon — + /// ollama `name:tag` values, Bedrock inference-profile ARNs — and those + /// must stay whole. Anything else is returned unchanged. + #[must_use] + pub fn qualify(self, registry: &dyn ModelRegistry) -> Self { + let Self::Bare(token) = self else { + return self; + }; + match token.split_once(':') { + Some((provider, selector)) + if !provider.is_empty() + && !selector.is_empty() + && registry.is_provider(provider) => + { + Self::Qualified { + provider: provider.to_owned(), + selector: selector.to_owned(), + } + } + _ => Self::Bare(token), + } + } + /// Resolve this reference against a registry. /// /// - [`ModelRef::Qualified`] always resolves to a model. + /// - A bare `provider:selector` token is qualified first — see + /// [`ModelRef::qualify`]. /// - [`ModelRef::Bare`] resolves to a provider if the token is only a /// provider, to a model if the token is only a model, and returns /// [`AmbiguousModelRef`] if the token matches both a provider and a model @@ -164,25 +201,25 @@ impl ModelRef { &self, registry: &dyn ModelRegistry, ) -> Result { - match self { + match self.clone().qualify(registry) { Self::Qualified { provider, selector } => Ok(ResolvedModelRef::Model { - provider: Some(provider.clone()), - selector: selector.clone(), + provider: Some(provider), + selector, }), Self::Bare(token) => { - let is_provider = registry.is_provider(token); - let is_model = registry.is_model(token); + let is_provider = registry.is_provider(&token); + let is_model = registry.is_model(&token); match (is_provider, is_model) { - (true, false) => Ok(ResolvedModelRef::Provider(token.clone())), + (true, false) => Ok(ResolvedModelRef::Provider(token)), (true, true) => Err(AmbiguousModelRef { input: token.clone(), providers: vec![token.clone()], - models: vec![token.clone()], + models: vec![token], }), // Known and unknown bare models leave provider selection to the runtime. (false, _) => Ok(ResolvedModelRef::Model { provider: None, - selector: token.clone(), + selector: token, }), } } @@ -190,6 +227,17 @@ impl ModelRef { } } +impl ModelRegistry for fabro_model::Catalog { + fn is_provider(&self, token: &str) -> bool { + self.provider(&fabro_model::ProviderId::from(token)) + .is_some() + } + + fn is_model(&self, token: &str) -> bool { + self.is_model_selector(token) + } +} + impl Serialize for ModelRef { fn serialize(&self, serializer: S) -> Result { serializer.serialize_str(&self.to_string()) @@ -248,15 +296,21 @@ mod tests { ); } + /// Parsing cannot tell a provider prefix from a model ID that contains a + /// colon, so it defers to [`ModelRef::qualify`]. #[test] - fn parses_colon_qualified() { - assert_eq!( - "gemini:gemini-flash".parse::().unwrap(), - ModelRef::Qualified { - provider: "gemini".into(), - selector: "gemini-flash".into(), - } - ); + fn parses_colon_tokens_as_bare() { + for input in [ + "gemini:gemini-flash", + "openrouter:moonshotai/kimi-k3", + "bedrock:us.anthropic.claude-haiku-4-5-20251001-v1:0", + ] { + assert_eq!( + input.parse::().unwrap(), + ModelRef::Bare(input.into()), + "{input}" + ); + } } #[test] @@ -271,23 +325,55 @@ mod tests { } #[test] - fn colon_qualified_selector_may_contain_slashes_and_colons() { - assert_eq!( - "openrouter:moonshotai/kimi-k3".parse::().unwrap(), - ModelRef::Qualified { - provider: "openrouter".into(), - selector: "moonshotai/kimi-k3".into(), - } - ); - assert_eq!( - "bedrock:us.anthropic.claude-haiku-4-5-20251001-v1:0" - .parse::() - .unwrap(), - ModelRef::Qualified { - provider: "bedrock".into(), - selector: "us.anthropic.claude-haiku-4-5-20251001-v1:0".into(), - } - ); + fn qualify_promotes_a_known_provider_prefix() { + let reg = TestRegistry { + providers: &["openrouter", "bedrock"], + models: &[], + }; + for (input, selector) in [ + ("openrouter:kimi-k3", "kimi-k3"), + ("openrouter:moonshotai/kimi-k3", "moonshotai/kimi-k3"), + ( + "bedrock:us.anthropic.claude-haiku-4-5-20251001-v1:0", + "us.anthropic.claude-haiku-4-5-20251001-v1:0", + ), + ] { + let qualified = input.parse::().unwrap().qualify(®); + let provider = input.split_once(':').unwrap().0; + assert_eq!( + qualified, + ModelRef::Qualified { + provider: provider.into(), + selector: selector.into(), + }, + "{input}" + ); + } + } + + /// The regression this guards: a model ID that merely contains a colon — + /// an ollama `name:tag`, a Bedrock ARN — must not be read as qualified. + #[test] + fn qualify_leaves_colon_bearing_model_ids_bare() { + let reg = TestRegistry { + providers: &["ollama", "bedrock"], + models: &[], + }; + for input in [ + "llama3:8b", + "qwen3.5:latest", + "us.anthropic.claude-haiku-4-5-20251001-v1:0", + "arn:aws:bedrock:us-east-1:1234:inference-profile/us.anthropic.claude-fable-5", + ":foo", + "foo:", + ] { + let parsed = input.parse::().unwrap(); + assert_eq!( + parsed.clone().qualify(®), + parsed, + "{input} should stay bare" + ); + } } #[test] @@ -310,14 +396,6 @@ mod tests { "foo/".parse::().unwrap_err(), ParseModelRefError::EmptySide { .. } )); - assert!(matches!( - ":foo".parse::().unwrap_err(), - ParseModelRefError::EmptySide { .. } - )); - assert!(matches!( - "foo:".parse::().unwrap_err(), - ParseModelRefError::EmptySide { .. } - )); } #[test] @@ -407,14 +485,21 @@ mod tests { m: ModelRef, } + // Colon tokens stay bare until a registry qualifies them, and survive + // the round trip verbatim either way. let input = r#"{"m":"openrouter:moonshotai/kimi-k3"}"#; let parsed: Wrap = serde_json::from_str(input).unwrap(); - assert!(matches!( + assert_eq!( parsed.m, - ModelRef::Qualified { ref provider, ref selector } - if provider == "openrouter" && selector == "moonshotai/kimi-k3" - )); + ModelRef::Bare("openrouter:moonshotai/kimi-k3".into()) + ); let rendered = serde_json::to_string(&parsed).unwrap(); assert_eq!(rendered, input); + + let legacy: Wrap = serde_json::from_str(r#"{"m":"gemini/gemini-flash"}"#).unwrap(); + assert_eq!( + serde_json::to_string(&legacy).unwrap(), + r#"{"m":"gemini:gemini-flash"}"# + ); } } diff --git a/lib/packages/fabro-api-client/src/models/create-run-session-request.ts b/lib/packages/fabro-api-client/src/models/create-run-session-request.ts index 20f9c44b2..f079d1b01 100644 --- a/lib/packages/fabro-api-client/src/models/create-run-session-request.ts +++ b/lib/packages/fabro-api-client/src/models/create-run-session-request.ts @@ -17,7 +17,7 @@ export interface CreateRunSessionRequest { 'title'?: string; /** - * Catalog model ID or alias, optionally qualified as `provider:selector`. A provider-qualified selector may be a canonical model ID, alias, or provider API ID. Legacy `provider/model` references remain accepted. The server stores the canonical model ID. + * Catalog model ID or alias, optionally qualified as `provider:selector`. A provider-qualified selector may be a canonical model ID, alias, or provider API ID. A value counts as qualified only when the text before the first `:` names a known provider, so model IDs containing a colon stay whole. Legacy `provider/model` references remain accepted. The server stores the canonical model ID. */ 'model'?: string; /**