From 14584bbc32135cf9aa4a047b44d2d5946ae6cdd6 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 20 Oct 2026 13:16:51 -0400 Subject: [PATCH] Reject bare provider names and unknown unpinned selectors Fabro's shared resolver passed any unknown bare selector through verbatim to the highest-priority ready provider. A provider name in the model field, such as `gemini`, is not a catalog model, so it took that path and reached Anthropic with wire model `gemini`. The same path turned any typo into a request on whichever provider happened to win priority. An unqualified unknown selector is now an error. Passthrough of an uncatalogued model id still works, but only with a provider pin, which is also how the operator docs already described it. A bare provider name gets a dedicated error that names the provider and says to pin it and omit the model to use its default. The completions endpoint returns that error as a 400 before any provider call; the models lookup returns it as a 404. Session creation keeps its own provider-name message and no longer passes unknown unpinned names through either. The OpenAPI description of `CreateCompletionRequest.model` no longer advertises a provider id as a selector, and the TypeScript client is regenerated from it. Closes #950 by rejecting the input rather than resolving it: a provider name in the model position is ambiguous with model aliases, and no comparable library resolves it to a provider default. Co-Authored-By: Claude Fable 5.1 --- docs/public/api-reference/fabro-api.yaml | 12 ++-- docs/public/core-concepts/models.mdx | 2 +- .../fabro-server/src/server/handler/models.rs | 3 +- .../src/server/handler/sessions.rs | 11 ++- .../fabro-server/tests/it/api/sessions.rs | 11 ++- .../fabro-server/tests/it/scenario/dry_run.rs | 42 +++++++++++ lib/components/fabro-llm/src/lib.rs | 7 +- lib/components/fabro-llm/src/selection.rs | 72 +++++++++++++++---- .../src/models/create-completion-request.ts | 4 +- 9 files changed, 128 insertions(+), 36 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index e5319e4f4..13f3bbc0c 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9012,12 +9012,16 @@ components: model: type: string description: >- - Model selector: `provider/model`, a model id or alias, or a - provider id. The server picks a ready-provider default when - omitted. + Model selector: `provider/model`, or a model id or alias. A bare + provider name is rejected; pin it with `provider` instead. When + omitted, the server picks the pinned provider's default model, or + a ready-provider default when `provider` is also omitted. provider: type: string - description: Optional provider pin for a bare model selector. + description: >- + Optional provider pin. Scopes a bare model selector to that + provider, which also lets an uncatalogued model id reach a + provider that allows passthrough. system: type: string description: System prompt (convenience; prepended as a system message). diff --git a/docs/public/core-concepts/models.mdx b/docs/public/core-concepts/models.mdx index 5ab6914f4..149294276 100644 --- a/docs/public/core-concepts/models.mdx +++ b/docs/public/core-concepts/models.mdx @@ -130,7 +130,7 @@ capabilities = { text = true, tools = true } `api_model` is the model name sent to that provider's API. It defaults to the exact model id, so omit it when the two strings match. Fabro does not infer vendor prefixes or rewrite the value. -A `provider/model` selector such as `openai/gpt-5.6-sol` pins the provider and names the model by id, alias, or wire id. A bare selector with no provider pin picks the highest-priority ready offering; a separate `provider = "openrouter"` pin selects the OpenRouter offering. Providers with `allow_passthrough = true` also accept `provider/model` selectors for models the catalog does not list. +A `provider/model` selector such as `openai/gpt-5.6-sol` pins the provider and names the model by id, alias, or wire id. A bare selector with no provider pin picks the highest-priority ready offering; a separate `provider = "openrouter"` pin selects the OpenRouter offering. Providers with `allow_passthrough = true` also accept `provider/model` selectors for models the catalog does not list. A bare selector the catalog does not know is an error rather than a passthrough, and a bare provider name such as `gemini` is an error too: pin the provider and omit the model to use its default. Model roles are separate: the provider's `default_model` controls normal model selection for workflow execution, while `small_default = true` on a model row marks the provider's small utility model for metadata tasks such as generated run titles. If a provider has no small default, Fabro falls back to that provider's default model. diff --git a/lib/apps/fabro-server/src/server/handler/models.rs b/lib/apps/fabro-server/src/server/handler/models.rs index 02c5f92b8..53070acfc 100644 --- a/lib/apps/fabro-server/src/server/handler/models.rs +++ b/lib/apps/fabro-server/src/server/handler/models.rs @@ -299,7 +299,8 @@ fn model_selection_response(error: &ModelSelectionError) -> Response { match error { ModelSelectionError::UnknownProvider { .. } | ModelSelectionError::UnknownSelector { .. } - | ModelSelectionError::UnknownSelectorOnProvider { .. } => { + | ModelSelectionError::UnknownSelectorOnProvider { .. } + | ModelSelectionError::ProviderUsedAsModel { .. } => { ApiError::not_found(error.to_string()).into_response() } ModelSelectionError::ProviderUnavailable { .. } diff --git a/lib/apps/fabro-server/src/server/handler/sessions.rs b/lib/apps/fabro-server/src/server/handler/sessions.rs index b8da8740b..5d6d79db3 100644 --- a/lib/apps/fabro-server/src/server/handler/sessions.rs +++ b/lib/apps/fabro-server/src/server/handler/sessions.rs @@ -1523,10 +1523,8 @@ enabled = true .unwrap(), (openrouter, "future-model".to_string()) ); - assert_eq!( - canonical_session_model(&catalog, &both, Some("future-model"), None).unwrap(), - (openai, "future-model".to_string()) - ); + // Without a pin there is no provider to pass through on. + assert!(canonical_session_model(&catalog, &both, Some("future-model"), None).is_err()); } /// A colon in a model ID does not make it provider-qualified. Ollama @@ -1548,9 +1546,8 @@ enabled = true .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()) + assert!( + canonical_session_model(&catalog, &both, Some("future-model:latest"), None).is_err() ); } diff --git a/lib/apps/fabro-server/tests/it/api/sessions.rs b/lib/apps/fabro-server/tests/it/api/sessions.rs index 0605d8989..3c5dd0333 100644 --- a/lib/apps/fabro-server/tests/it/api/sessions.rs +++ b/lib/apps/fabro-server/tests/it/api/sessions.rs @@ -222,11 +222,10 @@ async fn unknown_session_models_preserve_passthrough_on_the_selected_provider() let app = fabro_server::test_support::build_test_router(test_app_state()); let (run_id, _run_id_workspace) = create_run(&app).await; - for model in ["not-a-real-model", "openai/not-a-real-model"] { - let created = create_session_with_model(&app, &run_id, "Ask Fabro", model).await; - assert_eq!(created["model"], "not-a-real-model"); - assert_eq!(created["provider"], "openai"); - } + let created = + create_session_with_model(&app, &run_id, "Ask Fabro", "openai/not-a-real-model").await; + assert_eq!(created["model"], "not-a-real-model"); + assert_eq!(created["provider"], "openai"); } #[tokio::test] @@ -234,7 +233,7 @@ async fn invalid_session_model_refs_are_rejected_at_creation() { let app = fabro_server::test_support::build_test_router(test_app_state()); let (run_id, _run_id_workspace) = create_run(&app).await; - for model in ["openai", "openai/", "anthropic/gpt-5.4"] { + for model in ["openai", "openai/", "anthropic/gpt-5.4", "not-a-real-model"] { let response = create_session_response( &app, &run_id, diff --git a/lib/apps/fabro-server/tests/it/scenario/dry_run.rs b/lib/apps/fabro-server/tests/it/scenario/dry_run.rs index 9ec6aedec..e7b3312c2 100644 --- a/lib/apps/fabro-server/tests/it/scenario/dry_run.rs +++ b/lib/apps/fabro-server/tests/it/scenario/dry_run.rs @@ -167,6 +167,48 @@ async fn completion_non_streaming_returns_valid_json() { assert!(body["usage"]["output"].is_number()); } +/// A provider name in the model field, or a selector the catalog does not +/// know, is refused before any provider is called. Neither may pass through +/// to whichever ready provider wins priority. +#[tokio::test] +async fn completion_with_a_bare_provider_or_unknown_selector_is_rejected_before_any_call() { + let mock_server = MockServer::start_async().await; + let anthropic = mock_server + .mock_async(|when, then| { + when.method("POST").path("/v1/messages"); + then.status(500); + }) + .await; + let app = test_app_with_mock_anthropic(&format!("{}/v1", mock_server.url(""))); + + for (model, expected_detail) in [ + ("gemini", "set provider = \"gemini\""), + ("anthropic", "set provider = \"anthropic\""), + ( + "not-a-real-model", + "unknown model selector 'not-a-real-model'", + ), + ] { + let response = app + .clone() + .oneshot(completion_request_with_model(false, model)) + .await + .unwrap(); + let body = response_json( + response, + StatusCode::BAD_REQUEST, + &format!("POST /api/v1/completions with model {model}"), + ) + .await; + let detail = body["errors"][0]["detail"].as_str().unwrap_or_default(); + assert!( + detail.contains(expected_detail), + "model {model}: unexpected detail {detail:?}" + ); + } + assert_eq!(anthropic.calls_async().await, 0); +} + #[tokio::test] async fn completion_streaming_returns_sse() { let mock_server = MockServer::start_async().await; diff --git a/lib/components/fabro-llm/src/lib.rs b/lib/components/fabro-llm/src/lib.rs index 7d6d6c0fd..d1bbebb55 100644 --- a/lib/components/fabro-llm/src/lib.rs +++ b/lib/components/fabro-llm/src/lib.rs @@ -5,9 +5,10 @@ //! //! - building the catalog from the lithos built-ins and the operator `[llm]` //! overlay, and reading the agent harness a model expects ([`catalog`]); -//! - Fabro's passthrough policy for selections made before a request exists -//! ([`selection`]); at request time the lithos resolver enforces `enabled` -//! and `stands_in_for` itself; +//! - Fabro's selection policy for choices made before a request exists +//! ([`selection`]), including when an uncatalogued model may pass through; at +//! request time the lithos resolver enforces `enabled` and `stands_in_for` +//! itself; //! - constructing a client from a Fabro credential store ([`client`]); //! - model and provider probes ([`probe`]), and the API views of the catalog //! ([`api`]); diff --git a/lib/components/fabro-llm/src/selection.rs b/lib/components/fabro-llm/src/selection.rs index 63ab7aee4..548980388 100644 --- a/lib/components/fabro-llm/src/selection.rs +++ b/lib/components/fabro-llm/src/selection.rs @@ -9,7 +9,10 @@ //! - `provider/model` pins the provider, as the lithos resolver reads it. //! - An unknown selector pinned to a provider passes through verbatim on that //! provider. -//! - An unqualified unknown selector passes through on the default provider. +//! - An unqualified unknown selector is an error. Passthrough needs a provider +//! pin, so a typo never lands on whichever provider happens to win priority. +//! - A bare provider name is an error that points at the provider pin; it is +//! never a model name. //! - No selector picks the default offering (of the pinned provider, when one //! is given). //! @@ -73,6 +76,14 @@ pub enum ModelSelectionError { selector: String, provider: ProviderId, }, + #[error( + "'{selector}' names the provider '{provider}', not a model; set provider = \"{provider}\" \ + and omit the model to use its default" + )] + ProviderUsedAsModel { + selector: String, + provider: ProviderId, + }, #[error( "model selector '{selector}' is known but has no offering on an eligible provider; available providers: {providers:?}" )] @@ -155,6 +166,14 @@ pub fn select<'a>( } let matches = catalog.offerings_matching(selector); if matches.is_empty() { + // A provider name in the model position is the one unknown selector + // with an obvious intent, so say what the caller meant to write. + if let Some(provider) = catalog.enabled_provider(selector) { + return Err(ModelSelectionError::ProviderUsedAsModel { + selector: selector.to_string(), + provider: provider.id().clone(), + }); + } return Err(ModelSelectionError::UnknownSelector { selector: selector.to_string(), }); @@ -230,13 +249,6 @@ pub fn resolve_selection( model: selector, }) } - Err(ModelSelectionError::UnknownSelector { .. }) => { - let default = select_default(catalog, eligible)?; - Ok(SelectedModel { - provider: default.provider.id().clone(), - model: selector.to_string(), - }) - } Err(error) => Err(error), } } @@ -296,17 +308,53 @@ mod tests { } #[test] - fn unknown_selector_passes_through_on_the_default_provider() { + fn unknown_unqualified_selector_is_an_error_not_a_passthrough() { let catalog = test_catalog(); - let selected = resolve_selection( + let error = resolve_selection( &catalog, Some("totally-new-model"), None, &eligible(&["openai", "anthropic"]), ) + .unwrap_err(); + assert_eq!(error, ModelSelectionError::UnknownSelector { + selector: "totally-new-model".to_string(), + }); + } + + #[test] + fn bare_provider_name_is_rejected_with_a_pin_hint() { + let catalog = test_catalog(); + for (selector, provider) in [ + ("openai", builtin::openai()), + ("anthropic", builtin::anthropic()), + ] { + let error = resolve_selection( + &catalog, + Some(selector), + None, + &eligible(&["openai", "anthropic"]), + ) + .unwrap_err(); + assert_eq!(error, ModelSelectionError::ProviderUsedAsModel { + selector: selector.to_string(), + provider: provider.clone(), + }); + assert!( + error + .to_string() + .contains(&format!("provider = \"{provider}\"")) + ); + } + // The pin alone still means that provider's default. + let selected = resolve_selection( + &catalog, + None, + Some(&builtin::openai()), + &eligible(&["openai"]), + ) .unwrap(); - assert_eq!(selected.provider, builtin::anthropic()); - assert_eq!(selected.model, "totally-new-model"); + assert_eq!(selected.provider, builtin::openai()); } #[test] diff --git a/lib/packages/fabro-api-client/src/models/create-completion-request.ts b/lib/packages/fabro-api-client/src/models/create-completion-request.ts index c6f60ca40..e9646276c 100644 --- a/lib/packages/fabro-api-client/src/models/create-completion-request.ts +++ b/lib/packages/fabro-api-client/src/models/create-completion-request.ts @@ -41,11 +41,11 @@ export interface CreateCompletionRequest { */ 'messages': Array; /** - * Model selector: `provider/model`, a model id or alias, or a provider id. The server picks a ready-provider default when omitted. + * Model selector: `provider/model`, or a model id or alias. A bare provider name is rejected; pin it with `provider` instead. When omitted, the server picks the pinned provider\'s default model, or a ready-provider default when `provider` is also omitted. */ 'model'?: string; /** - * Optional provider pin for a bare model selector. + * Optional provider pin. Scopes a bare model selector to that provider, which also lets an uncatalogued model id reach a provider that allows passthrough. */ 'provider'?: string; /**