diff --git a/lib/crates/fabro-config/src/layers/llm.rs b/lib/crates/fabro-config/src/layers/llm.rs index 343e65a5e..8f3d13ae9 100644 --- a/lib/crates/fabro-config/src/layers/llm.rs +++ b/lib/crates/fabro-config/src/layers/llm.rs @@ -217,7 +217,7 @@ where /// A typed credential reference. Literal secret strings are rejected at /// deserialization so settings never carry a successful "secret string" /// representation. -#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(into = "String", try_from = "String")] pub enum CredentialRef { /// Structured credential stored in `fabro-vault` keyed by ``. @@ -227,30 +227,21 @@ pub enum CredentialRef { Env(String), } -impl CredentialRef { - /// The string form, e.g. `"credential:openai_codex"` or - /// `"env:KIMI_API_KEY"`. Stable for serialization round-trips. - #[must_use] - pub fn as_serialized(&self) -> String { - match self { - Self::Credential(id) => format!("credential:{id}"), - Self::Env(name) => format!("env:{name}"), - } - } -} - impl std::fmt::Display for CredentialRef { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // Display deliberately writes only the typed reference form, never // any resolved secret value. Env names and credential IDs are not // themselves secret. - f.write_str(&self.as_serialized()) + match self { + Self::Credential(id) => write!(f, "credential:{id}"), + Self::Env(name) => write!(f, "env:{name}"), + } } } impl From for String { fn from(value: CredentialRef) -> Self { - value.as_serialized() + value.to_string() } } @@ -309,14 +300,6 @@ impl TryFrom for CredentialRef { } } -impl<'de> Deserialize<'de> for CredentialRef { - fn deserialize>(deserializer: D) -> Result { - use serde::de::Error; - let s = String::deserialize(deserializer)?; - s.parse().map_err(D::Error::custom) - } -} - // --------------------------------------------------------------------------- // Combine glue for primitives that don't already have it. // --------------------------------------------------------------------------- diff --git a/lib/crates/fabro-dev/tests/it/policy.rs b/lib/crates/fabro-dev/tests/it/policy.rs index 37678b353..86f4f593c 100644 --- a/lib/crates/fabro-dev/tests/it/policy.rs +++ b/lib/crates/fabro-dev/tests/it/policy.rs @@ -4,7 +4,7 @@ //! product-level invariants. They run as part of `cargo nextest` and are //! cheap (text scans only). -use std::path::{Path, PathBuf}; +use walkdir::WalkDir; use crate::workspace_root; @@ -16,6 +16,9 @@ use crate::workspace_root; /// /// The allowed-callers list below is the policy boundary. Adding a new /// caller is intentional and requires updating this list. +/// +/// The walker only descends into `lib/`, so non-`lib/` paths (docs, top-level +/// markdown) are not part of the allowlist. const BOOTSTRAP_CATALOG_ALLOWED_PATH_FRAGMENTS: &[&str] = &[ // The bootstrap module itself. "lib/crates/fabro-model/src/bootstrap_catalog", @@ -30,17 +33,48 @@ const BOOTSTRAP_CATALOG_ALLOWED_PATH_FRAGMENTS: &[&str] = &[ "test_support", "/tests/it/", "/tests/policy.rs", - // Documentation files referencing the policy. - "docs/", - "CLAUDE.md", - "AGENTS.md", ]; #[test] +#[expect( + clippy::disallowed_methods, + reason = "policy test reads source files synchronously with std::fs" +)] fn bootstrap_catalog_references_stay_in_allowlist() { let root = workspace_root(); - let mut violations: Vec<(PathBuf, usize, String)> = Vec::new(); - walk_rust_sources(&root, &mut |path, contents| { + let lib_root = root.join("lib"); + let mut violations: Vec<(String, usize, String)> = Vec::new(); + + let walker = WalkDir::new(&lib_root).into_iter().filter_entry(|entry| { + // Skip generated/output directories at any depth. + let name = entry.file_name().to_string_lossy(); + !matches!( + name.as_ref(), + "target" | ".git" | "node_modules" | "dist" | "build" + ) + }); + + for entry in walker.flatten() { + let path = entry.path(); + if !path.is_file() || path.extension().is_none_or(|ext| ext != "rs") { + continue; + } + let Ok(contents) = std::fs::read_to_string(path) else { + continue; + }; + // Cheap early-out: avoids per-line work for the ~99% of files with no + // reference to the symbol. + if !contents.contains("bootstrap_catalog") { + continue; + } + let rel = path.strip_prefix(&root).unwrap_or(path); + let rel_str = rel.to_string_lossy().replace('\\', "/"); + let path_allowed = BOOTSTRAP_CATALOG_ALLOWED_PATH_FRAGMENTS + .iter() + .any(|frag| rel_str.contains(frag)); + if path_allowed { + continue; + } for (idx, line) in contents.lines().enumerate() { if !line.contains("bootstrap_catalog") { continue; @@ -50,57 +84,17 @@ fn bootstrap_catalog_references_stay_in_allowlist() { if trimmed.starts_with("//") || trimmed.starts_with("/*") || trimmed.starts_with('*') { continue; } - let rel = path.strip_prefix(&root).unwrap_or(path); - let rel_str = rel.to_string_lossy().replace('\\', "/"); - if BOOTSTRAP_CATALOG_ALLOWED_PATH_FRAGMENTS - .iter() - .any(|frag| rel_str.contains(frag)) - { - continue; - } - violations.push((rel.to_path_buf(), idx + 1, line.to_string())); + violations.push((rel_str.clone(), idx + 1, line.to_string())); } - }); + } assert!( violations.is_empty(), "bootstrap_catalog (install-only) referenced from non-allowlisted source files:\n{}\n\nIf this is intentional, add the path fragment to BOOTSTRAP_CATALOG_ALLOWED_PATH_FRAGMENTS in lib/crates/fabro-dev/tests/it/policy.rs.", violations .into_iter() - .map(|(p, l, s)| format!(" {}:{}: {}", p.display(), l, s.trim())) + .map(|(p, l, s)| format!(" {p}:{l}: {}", s.trim())) .collect::>() .join("\n"), ); } - -#[expect( - clippy::disallowed_methods, - reason = "policy test reads source files synchronously with std::fs" -)] -fn walk_rust_sources(root: &Path, on_file: &mut dyn FnMut(&Path, &str)) { - let mut stack: Vec = vec![root.join("lib")]; - while let Some(dir) = stack.pop() { - let Ok(entries) = std::fs::read_dir(&dir) else { - continue; - }; - for entry in entries.flatten() { - let path = entry.path(); - let name = entry.file_name(); - let name_str = name.to_string_lossy(); - // Skip generated/output directories. - if matches!( - name_str.as_ref(), - "target" | ".git" | "node_modules" | "dist" | "build" - ) { - continue; - } - if path.is_dir() { - stack.push(path); - } else if path.extension().is_some_and(|ext| ext == "rs") { - if let Ok(contents) = std::fs::read_to_string(&path) { - on_file(&path, &contents); - } - } - } - } -} diff --git a/lib/crates/fabro-llm/src/adapter_registry.rs b/lib/crates/fabro-llm/src/adapter_registry.rs index 5722e5582..6e99f1d81 100644 --- a/lib/crates/fabro-llm/src/adapter_registry.rs +++ b/lib/crates/fabro-llm/src/adapter_registry.rs @@ -18,6 +18,7 @@ use std::sync::Arc; use fabro_auth::ApiKeyHeader; use fabro_model::adapter::{self as model_adapter, AdapterMetadata}; +use crate::client::auth_value; use crate::provider::ProviderAdapter; use crate::providers; @@ -60,96 +61,95 @@ impl AdapterConfig { } } -fn auth_value(header: &ApiKeyHeader) -> String { - match header { - ApiKeyHeader::Bearer(value) | ApiKeyHeader::Custom { value, .. } => value.clone(), - } -} - /// Factory function signature. Takes a fully-resolved [`AdapterConfig`] and /// returns a registered-ready [`ProviderAdapter`]. /// /// Adapter constructors are infallible today; if a future adapter needs to /// fail at construction time, add a separate fallible factory variant /// rather than re-shaping every existing factory. -pub type AdapterFactory = fn(&AdapterConfig) -> Arc; +pub type AdapterFactory = fn(AdapterConfig) -> Arc; -const KIMI_BASE_URL: &str = "https://api.moonshot.ai/v1"; - -fn build_anthropic(config: &AdapterConfig) -> Arc { +fn build_anthropic(config: AdapterConfig) -> Arc { let mut adapter = providers::AnthropicAdapter::new(auth_value(&config.auth_header)); - if let Some(base_url) = config.base_url.clone() { + if let Some(base_url) = config.base_url { adapter = adapter.with_base_url(base_url); } if !config.extra_headers.is_empty() { - adapter = adapter.with_default_headers(config.extra_headers.clone()); + adapter = adapter.with_default_headers(config.extra_headers); } Arc::new(adapter) } -fn build_openai(config: &AdapterConfig) -> Arc { +fn build_openai(config: AdapterConfig) -> Arc { let mut adapter = providers::OpenAiAdapter::new(auth_value(&config.auth_header)); - if let Some(base_url) = config.base_url.clone() { + if let Some(base_url) = config.base_url { adapter = adapter.with_base_url(base_url); } if !config.extra_headers.is_empty() { - adapter = adapter.with_default_headers(config.extra_headers.clone()); + adapter = adapter.with_default_headers(config.extra_headers); } if config.codex_mode { adapter = adapter.with_codex_mode(); } - if let Some(org_id) = config.org_id.clone() { + if let Some(org_id) = config.org_id { adapter = adapter.with_org_id(org_id); } - if let Some(project_id) = config.project_id.clone() { + if let Some(project_id) = config.project_id { adapter = adapter.with_project_id(project_id); } Arc::new(adapter) } -fn build_gemini(config: &AdapterConfig) -> Arc { +fn build_gemini(config: AdapterConfig) -> Arc { let mut adapter = providers::GeminiAdapter::new(auth_value(&config.auth_header)); - if let Some(base_url) = config.base_url.clone() { + if let Some(base_url) = config.base_url { adapter = adapter.with_base_url(base_url); } if !config.extra_headers.is_empty() { - adapter = adapter.with_default_headers(config.extra_headers.clone()); + adapter = adapter.with_default_headers(config.extra_headers); } Arc::new(adapter) } -fn build_openai_compatible(config: &AdapterConfig) -> Arc { - let base_url = config - .base_url - .clone() - .unwrap_or_else(|| KIMI_BASE_URL.to_string()); +fn build_openai_compatible(config: AdapterConfig) -> Arc { + // `openai_compatible` providers vary widely in base URL; the catalog must + // pre-resolve `[llm.providers.].base_url` before constructing + // `AdapterConfig`. There is no sensible default — silently routing to one + // provider's host would produce wrong-host requests for every other. + let base_url = config.base_url.expect( + "openai_compatible adapter requires a base_url; resolve it from provider settings before \ + building AdapterConfig", + ); let mut adapter = providers::OpenAiCompatibleAdapter::new(auth_value(&config.auth_header), base_url) - .with_name(config.provider_id.clone()); + .with_name(config.provider_id); if !config.extra_headers.is_empty() { - adapter = adapter.with_default_headers(config.extra_headers.clone()); + adapter = adapter.with_default_headers(config.extra_headers); } Arc::new(adapter) } +/// Single source of truth pairing every adapter key with its factory. Both +/// `factory_for` and `registered_keys` derive from this table. +const FACTORIES: &[(&str, AdapterFactory)] = &[ + ("anthropic", build_anthropic), + ("openai", build_openai), + ("gemini", build_gemini), + ("openai_compatible", build_openai_compatible), +]; + /// Look up a factory by adapter key. Returns `None` if the key has no factory /// registered. #[must_use] pub fn factory_for(adapter_key: &str) -> Option { - match adapter_key { - "anthropic" => Some(build_anthropic), - "openai" => Some(build_openai), - "gemini" => Some(build_gemini), - "openai_compatible" => Some(build_openai_compatible), - _ => None, - } + FACTORIES + .iter() + .find_map(|(key, factory)| (*key == adapter_key).then_some(*factory)) } /// Iterate every adapter key with a factory registered. pub fn registered_keys() -> impl Iterator { - ["anthropic", "openai", "gemini", "openai_compatible"] - .iter() - .copied() + FACTORIES.iter().map(|(key, _)| *key) } /// Look up adapter metadata by key, ensuring the metadata + factory pair @@ -201,7 +201,7 @@ mod tests { name: "x-api-key".to_string(), value: "test-key".to_string(), }); - let adapter = factory_for("anthropic").unwrap()(&config); + let adapter = factory_for("anthropic").unwrap()(config); assert_eq!(adapter.name(), "anthropic"); } @@ -216,7 +216,14 @@ mod tests { org_id: None, project_id: None, }; - let adapter = factory_for("openai_compatible").unwrap()(&config); + let adapter = factory_for("openai_compatible").unwrap()(config); assert_eq!(adapter.name(), "kimi"); } + + #[test] + #[should_panic(expected = "openai_compatible adapter requires a base_url")] + fn openai_compatible_factory_panics_without_base_url() { + let config = AdapterConfig::new("kimi", ApiKeyHeader::Bearer("k".to_string())); + let _ = factory_for("openai_compatible").unwrap()(config); + } } diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index 865cff207..18711d952 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -321,7 +321,7 @@ impl Client { } } -fn auth_value(auth_header: &ApiKeyHeader) -> String { +pub(crate) fn auth_value(auth_header: &ApiKeyHeader) -> String { match auth_header { ApiKeyHeader::Bearer(value) | ApiKeyHeader::Custom { value, .. } => value.clone(), } diff --git a/lib/crates/fabro-llm/src/types.rs b/lib/crates/fabro-llm/src/types.rs index 58e785207..80f46cf83 100644 --- a/lib/crates/fabro-llm/src/types.rs +++ b/lib/crates/fabro-llm/src/types.rs @@ -410,29 +410,10 @@ pub struct RateLimitInfo { } // --- 3.8 ReasoningEffort --- - -#[derive( - Debug, - Clone, - Copy, - PartialEq, - Eq, - Hash, - Serialize, - Deserialize, - strum::Display, - strum::EnumString, - strum::IntoStaticStr, -)] -#[serde(rename_all = "lowercase")] -#[strum(serialize_all = "lowercase")] -pub enum ReasoningEffort { - Low, - Medium, - High, - XHigh, - Max, -} +// +// Re-exported from `fabro-model` so catalog data, request validation, OpenAPI +// replacement types, and the LLM client share one enum. +pub use fabro_model::ReasoningEffort; // --- 3.6 Request --- @@ -1277,30 +1258,4 @@ mod tests { fn tool_choice_mode_str_named() { assert_eq!(ToolChoice::named("get_weather").mode_str(), "named"); } - - #[test] - fn reasoning_effort_from_str_round_trip() { - use std::str::FromStr; - assert_eq!(ReasoningEffort::from_str("low"), Ok(ReasoningEffort::Low)); - assert_eq!( - ReasoningEffort::from_str("medium"), - Ok(ReasoningEffort::Medium) - ); - assert_eq!(ReasoningEffort::from_str("high"), Ok(ReasoningEffort::High)); - assert_eq!( - ReasoningEffort::from_str("xhigh"), - Ok(ReasoningEffort::XHigh) - ); - assert_eq!(ReasoningEffort::from_str("max"), Ok(ReasoningEffort::Max)); - assert_eq!(ReasoningEffort::XHigh.to_string(), "xhigh"); - assert_eq!(<&'static str>::from(ReasoningEffort::XHigh), "xhigh"); - assert_eq!(ReasoningEffort::Max.to_string(), "max"); - assert_eq!(<&'static str>::from(ReasoningEffort::Max), "max"); - } - - #[test] - fn reasoning_effort_from_str_rejects_unknown() { - use std::str::FromStr; - assert!(ReasoningEffort::from_str("bogus").is_err()); - } } diff --git a/lib/crates/fabro-model/src/adapter.rs b/lib/crates/fabro-model/src/adapter.rs index f9168b625..68db1f792 100644 --- a/lib/crates/fabro-model/src/adapter.rs +++ b/lib/crates/fabro-model/src/adapter.rs @@ -8,6 +8,8 @@ //! (in `fabro-model`) and the LLM factory registry (in `fabro-llm`) must agree //! on the same set of adapter keys; the parity is enforced by tests. +use strum::VariantArray; + use crate::Speed; use crate::reasoning::ReasoningEffort; @@ -64,13 +66,9 @@ pub struct AdapterMetadata { pub controls: AdapterControlCapabilities, } -const FULL_REASONING_EFFORTS: &[ReasoningEffort] = &[ - ReasoningEffort::Low, - ReasoningEffort::Medium, - ReasoningEffort::High, - ReasoningEffort::XHigh, - ReasoningEffort::Max, -]; +/// Every reasoning-effort variant. Re-exposed as a const slice so static +/// adapter metadata can reference it without re-listing variants. +const FULL_REASONING_EFFORTS: &[ReasoningEffort] = ReasoningEffort::VARIANTS; const FAST_SPEEDS: &[Speed] = &[Speed::Fast];