From 2d859496e2e8675c5685d8d4c5d2b2cf85556d5b Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 23 Oct 2026 14:38:46 -0400 Subject: [PATCH] Serialize generated provider overlays with the TOML library --- lib/apps/fabro-server/src/install.rs | 23 +- lib/apps/fabro-server/src/server/tests.rs | 78 +++++++ lib/apps/fabro-server/src/test_support.rs | 10 +- lib/components/fabro-llm/src/catalog.rs | 210 +++++++++++++----- lib/components/fabro-llm/src/test_support.rs | 9 +- lib/foundation/fabro-config/src/layers/llm.rs | 35 ++- 6 files changed, 275 insertions(+), 90 deletions(-) diff --git a/lib/apps/fabro-server/src/install.rs b/lib/apps/fabro-server/src/install.rs index 49a3416a6..e3effb263 100644 --- a/lib/apps/fabro-server/src/install.rs +++ b/lib/apps/fabro-server/src/install.rs @@ -2155,29 +2155,14 @@ fn install_catalog_with_base_url( let Some(base_url) = state.upstreams.provider_base_urls.get(provider.id()) else { return Ok(Catalog::clone(&INSTALL_CATALOG)); }; - let overlay = fabro_config::LlmLayer( - toml::from_str(&format!( - "[providers.{}]\nbase_url = {}\n", - toml_key(provider.id().as_str()), - toml::Value::String(base_url.clone()) - )) - .context("install provider base URL overlay should parse")?, - ); + let overlay = fabro_config::LlmLayer::from_providers([( + provider.id().to_string(), + toml::Table::from_iter([("base_url".into(), toml::Value::String(base_url.clone()))]), + )]); fabro_llm::build_catalog(&overlay, &|_| None) .context("install catalog with provider base URL override should build") } -fn toml_key(key: &str) -> String { - if key - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') - { - key.to_string() - } else { - format!("{key:?}") - } -} - #[cfg(test)] fn provider_base_url_override(state: &InstallAppState, provider: &CatalogProvider) -> String { state diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index a1f978590..a97b79af9 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -1729,6 +1729,84 @@ async fn resolve_llm_client_reads_openai_token_from_vault() { assert!(llm_result.auth_issues.is_empty()); } +#[tokio::test] +async fn dotted_provider_starts_and_uses_its_vault_key_with_inferred_or_explicit_profile() { + for explicit_profile in [false, true] { + let upstream = MockServer::start_async().await; + let completion = upstream + .mock_async(|when, then| { + when.method(POST) + .path("/v1/chat/completions") + .header("authorization", "Bearer dotted-provider-test-key") + .json_body_includes(r#"{"model":"wire-model"}"#); + then.status(200) + .header("content-type", "application/json") + .json_body(json!({ + "id": "chatcmpl-dotted", + "object": "chat.completion", + "created": 1_700_000_000, + "model": "wire-model", + "choices": [{ + "index": 0, + "message": {"role": "assistant", "content": "OK"}, + "finish_reason": "stop" + }], + "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2} + })); + }) + .await; + let mut overlay = r#" +[providers."my.gw"] +display_name = "Gateway" +base_url = "http://unused.invalid" +auth = { type = "bearer" } +[providers."my.gw".models."model.v1"] +display_name = "Model" +api_model = "wire-model" +"# + .to_string(); + if explicit_profile { + overlay.push_str( + r#" +[providers."my.gw".metadata.agent] +profile = "openai" +"#, + ); + } + let state = TestAppStateBuilder::new() + .runtime_settings(default_test_server_settings(), RunLayer::default()) + .env_lookup(|_| None) + .llm_overlay_toml(&overlay) + .provider_base_url("my.gw", upstream.base_url()) + .build(); + state + .stores + .vault + .set( + "MY_GW_API_KEY", + "dotted-provider-test-key", + SecretType::Token, + None, + ) + .await + .unwrap(); + let llm = state.resolve_llm_client().await.unwrap(); + let response = llm + .client + .complete( + LlmRequest::builder() + .model("my.gw/model.v1") + .user("Hello") + .build() + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.text(), "OK"); + completion.assert_async().await; + } +} + #[tokio::test] async fn resolve_llm_client_ignores_env_lookup_provider_tokens() { let state = test_app_state_with_env_lookup( diff --git a/lib/apps/fabro-server/src/test_support.rs b/lib/apps/fabro-server/src/test_support.rs index 6374fba43..bd61d3009 100644 --- a/lib/apps/fabro-server/src/test_support.rs +++ b/lib/apps/fabro-server/src/test_support.rs @@ -364,12 +364,10 @@ pub fn llm_overlay_with_provider_base_url( provider: impl Into, base_url: impl Into, ) -> LlmLayer { - let provider = provider.into(); - llm_overlay_from_toml(&format!( - "[providers.{}]\nbase_url = {}\n", - toml::Value::String(provider), - toml::Value::String(base_url.into()) - )) + LlmLayer::from_providers([( + provider.into(), + toml::Table::from_iter([("base_url".into(), toml::Value::String(base_url.into()))]), + )]) } /// The catalog a test app state builds from `overlay`. diff --git a/lib/components/fabro-llm/src/catalog.rs b/lib/components/fabro-llm/src/catalog.rs index 731f8fe44..f223bed38 100644 --- a/lib/components/fabro-llm/src/catalog.rs +++ b/lib/components/fabro-llm/src/catalog.rs @@ -17,6 +17,7 @@ use lithos_llm::catalog::{ adapter_ids, codec_ids, }; use serde::Deserialize; +use toml::{Table, Value}; /// The metadata namespace agent harnesses read. const AGENT_METADATA_NAMESPACE: &str = "agent"; @@ -30,75 +31,59 @@ pub fn build_catalog( overlay: &LlmLayer, env_lookup: &dyn Fn(&str) -> Option, ) -> Result { - let mut builder = Catalog::builder().with_builtin(); - if !overlay.is_empty() { - let mut document = overlay.to_overlay_toml(); - document.insert_str(0, "schema_version = 1\n"); - builder = builder.toml_layer("settings [llm]", &document)?; - } - if let Some(base_url) = env_lookup(EnvVars::OPENAI_BASE_URL) { - let document = format!( - "schema_version = 1\n[providers.openai]\nbase_url = {}\n", - toml::Value::String(base_url.trim_end_matches("/v1").to_string()) - ); - builder = builder.toml_layer("OPENAI_BASE_URL", &document)?; - } - let catalog = builder.build()?; - // The coding agent reads a provider's harness profile from - // `metadata.agent.profile` and refuses a provider without one. The lithos - // built-ins all declare theirs; an operator-defined provider that does - // not gets the profile its wire protocol implies, layered on last. + let base_url_overlay = env_lookup(EnvVars::OPENAI_BASE_URL).map(|base_url| { + LlmLayer::from_providers([( + "openai".into(), + Table::from_iter([( + "base_url".into(), + Value::String(base_url.trim_end_matches("/v1").to_string()), + )]), + )]) + }); + let build_base = || { + let mut builder = Catalog::builder().with_builtin(); + if !overlay.is_empty() { + builder = add_catalog_layer(builder, "settings [llm]", overlay)?; + } + if let Some(overlay) = &base_url_overlay { + builder = add_catalog_layer(builder, "OPENAI_BASE_URL", overlay)?; + } + Ok::<_, CatalogError>(builder) + }; + let catalog = build_base()?.build()?; + // Pebble needs provider-level profile metadata. Fill only missing profiles + // after the operator overlay has selected the provider's wire protocol. let implied = implied_agent_profiles(&catalog); if implied.is_empty() { return Ok(catalog); } - let mut builder = Catalog::builder().with_builtin(); - if !overlay.is_empty() { - let mut document = overlay.to_overlay_toml(); - document.insert_str(0, "schema_version = 1\n"); - builder = builder.toml_layer("settings [llm]", &document)?; - } - if let Some(base_url) = env_lookup(EnvVars::OPENAI_BASE_URL) { - let document = format!( - "schema_version = 1\n[providers.openai]\nbase_url = {}\n", - toml::Value::String(base_url.trim_end_matches("/v1").to_string()) - ); - builder = builder.toml_layer("OPENAI_BASE_URL", &document)?; - } - builder - .toml_layer("implied agent profiles", &implied) - .and_then(CatalogBuilder::build) + add_catalog_layer(build_base()?, "implied agent profiles", &implied)?.build() } -/// A TOML layer declaring `metadata.agent.profile` for every provider that -/// has none, as the profile implied by the provider's adapter. Empty when -/// every provider already says. -fn implied_agent_profiles(catalog: &Catalog) -> String { - use std::fmt::Write as _; +fn add_catalog_layer( + builder: CatalogBuilder, + name: &str, + overlay: &LlmLayer, +) -> Result { + // The fixed schema declaration is outside the settings overlay. All + // dynamic keys and values are serialized together by the TOML library. + let document = format!("schema_version = 1\n{}", overlay.to_overlay_toml()); + builder.toml_layer(name, &document) +} - let mut document = String::new(); - for provider in catalog.providers() { +/// Metadata for providers without an explicit harness profile. +fn implied_agent_profiles(catalog: &Catalog) -> LlmLayer { + LlmLayer::from_providers(catalog.providers().filter_map(|provider| { if agent_metadata(provider.metadata()).profile.is_some() { - continue; + return None; } - let profile = adapter_agent_profile(provider); - let _ = writeln!( - document, - "[providers.{}.metadata.agent]\nprofile = {}\n", - provider.id(), - toml::Value::String( - serde_json::to_value(profile) - .ok() - .and_then(|value| value.as_str().map(str::to_owned)) - .unwrap_or_else(|| "openai".to_string()) - ), - ); - } - if document.is_empty() { - return document; - } - document.insert_str(0, "schema_version = 1\n"); - document + let profile = Value::try_from(adapter_agent_profile(provider)) + .expect("agent profiles serialize as strings"); + let agent = Table::from_iter([("profile".into(), profile)]); + let metadata = Table::from_iter([("agent".into(), Value::Table(agent))]); + let fields = Table::from_iter([("metadata".into(), Value::Table(metadata))]); + Some((provider.id().to_string(), fields)) + })) } /// The profile a provider's wire protocol implies, for a provider whose @@ -313,6 +298,111 @@ capabilities = { text = true, tools = true } ); } + #[test] + fn dotted_provider_profiles_preserve_identity_metadata_and_model_overrides() { + let source = r#" +[providers."my.gw"] +display_name = "Gateway" +base_url = "http://127.0.0.1:1234/v1" +auth = { type = "bearer" } +[providers."my.gw".metadata.agent] +reasoning_by_default = false +[providers."my.gw".models."model.v1"] +display_name = "Model" +api_model = "wire-model" +[providers."my.gw".models."model.v1".metadata.agent] +profile = "anthropic" +[providers.my] +display_name = "Sibling" +base_url = "http://127.0.0.1:5678" +auth = { type = "none" } +[providers.my.metadata.agent] +profile = "gemini" +"#; + for enabled in [true, false] { + let mut overlay = LlmLayer(toml::from_str(source).unwrap()); + overlay.0["providers"]["my.gw"] + .as_table_mut() + .unwrap() + .insert("enabled".into(), Value::Boolean(enabled)); + let catalog = build_catalog(&overlay, &|name| { + (name == EnvVars::OPENAI_BASE_URL).then(|| "http://127.0.0.1:4321/v1".into()) + }) + .unwrap(); + let gateway = catalog.provider("my.gw").unwrap(); + assert_eq!(gateway.id().as_str(), "my.gw"); + assert_eq!(gateway.base_url(), "http://127.0.0.1:1234/v1"); + assert_eq!( + agent_metadata(gateway.metadata()).profile, + Some(AgentProfileKind::OpenAi) + ); + assert_eq!( + agent_metadata(gateway.metadata()).reasoning_by_default, + Some(false) + ); + assert_eq!(gateway.model("model.v1").unwrap().api_model(), "wire-model"); + assert_eq!( + model_agent_profile(gateway, gateway.model("model.v1").unwrap()), + AgentProfileKind::Anthropic + ); + assert_eq!( + fabro_auth::expected_secret_name(gateway).as_deref(), + Some("MY_GW_API_KEY") + ); + assert_eq!( + agent_profile(&catalog, "my", None), + Some(AgentProfileKind::Gemini) + ); + assert_eq!(catalog.enabled_provider("my.gw").is_some(), enabled); + assert_eq!( + catalog.provider("openai").unwrap().base_url(), + "http://127.0.0.1:4321" + ); + } + } + + #[test] + fn provider_names_remain_literal_with_inferred_and_explicit_profiles() { + for id in [ + "my.gw", "my.gw.v2", "my-gw", "my_gw", ".gateway", "gateway.", + ] { + for explicit in [None, Some("openai"), Some("gemini")] { + let mut fields: Table = toml::from_str( + r#" +display_name = "Gateway" +base_url = "http://127.0.0.1:1234" +auth = { type = "none" } +"#, + ) + .unwrap(); + if let Some(profile) = explicit { + fields.insert( + "metadata".into(), + Value::Table(Table::from_iter([( + "agent".into(), + Value::Table(Table::from_iter([( + "profile".into(), + Value::String(profile.into()), + )])), + )])), + ); + } + let overlay = LlmLayer::from_providers([(id.into(), fields)]); + let catalog = build_catalog(&overlay, &|_| None).unwrap(); + assert_eq!(catalog.provider(id).unwrap().id().as_str(), id); + assert_eq!( + agent_profile(&catalog, id, None), + Some(if explicit == Some("gemini") { + AgentProfileKind::Gemini + } else { + AgentProfileKind::OpenAi + }) + ); + assert!(catalog.provider("my").is_err()); + } + } + } + /// The implied profile follows the provider's first codec, so a host that /// speaks Anthropic Messages gets the Anthropic harness. #[test] diff --git a/lib/components/fabro-llm/src/test_support.rs b/lib/components/fabro-llm/src/test_support.rs index 61acf8c9b..e34ab4d35 100644 --- a/lib/components/fabro-llm/src/test_support.rs +++ b/lib/components/fabro-llm/src/test_support.rs @@ -35,10 +35,11 @@ pub fn test_catalog_with_overlay(overlay: &str) -> Catalog { /// The test catalog with one provider's base URL pointed elsewhere. #[must_use] pub fn test_catalog_with_provider_base_url(provider: &str, base_url: &str) -> Catalog { - test_catalog_with_overlay(&format!( - "[providers.{provider}]\nbase_url = {}\n", - toml::Value::String(base_url.to_string()) - )) + let overlay = LlmLayer::from_providers([( + provider.to_string(), + toml::Table::from_iter([("base_url".into(), toml::Value::String(base_url.to_string()))]), + )]); + crate::build_catalog(&overlay, &|_| None).expect("test catalog should build") } /// Builds a text response attributed to `provider/model`. diff --git a/lib/foundation/fabro-config/src/layers/llm.rs b/lib/foundation/fabro-config/src/layers/llm.rs index 8daa204c8..28134dd22 100644 --- a/lib/foundation/fabro-config/src/layers/llm.rs +++ b/lib/foundation/fabro-config/src/layers/llm.rs @@ -18,7 +18,7 @@ //! validates it when the catalog is built. use serde::{Deserialize, Serialize}; -use toml::Table; +use toml::{Table, Value}; use super::combine::Combine; @@ -28,6 +28,23 @@ use super::combine::Combine; pub struct LlmLayer(pub Table); impl LlmLayer { + /// Build an overlay from provider tables, treating every provider name as + /// one key. TOML quoting and escaping are handled by `to_overlay_toml`. + #[must_use] + pub fn from_providers(providers: impl IntoIterator) -> Self { + let providers: Table = providers + .into_iter() + .map(|(id, fields)| (id, Value::Table(fields))) + .collect(); + if providers.is_empty() { + return Self::default(); + } + Self(Table::from_iter([( + "providers".into(), + Value::Table(providers), + )])) + } + #[must_use] pub fn is_empty(&self) -> bool { self.0.is_empty() @@ -74,6 +91,22 @@ mod tests { LlmLayer(toml::from_str(source).unwrap()) } + #[test] + fn generated_provider_tables_round_trip_literal_keys_and_values() { + // The layer is raw TOML; catalog validation separately restricts IDs. + for name in ["my.gw", "my.gw.v2", "a b", "a\"b", "a\\b", "a\nb", "日本語"] { + let value = "https://example.test/\"quoted\"\\path\nnext\tline"; + let fields = Table::from_iter([("base_url".into(), Value::String(value.into()))]); + let source = LlmLayer::from_providers([(name.into(), fields.clone())]); + let parsed = layer(&source.to_overlay_toml()); + assert_eq!(parsed, source); + let providers = parsed.0["providers"].as_table().unwrap(); + assert_eq!(providers.len(), 1); + assert_eq!(providers[name].as_table().unwrap(), &fields); + } + assert!(LlmLayer::from_providers([]).is_empty()); + } + #[test] fn higher_layer_wins_scalars_and_merges_tables() { let higher = layer(