From 941c6e83f940de365e188e6ca5d3bdf26f2c4fcf Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 19:20:02 -0400 Subject: [PATCH] route fabro-config parsing through settings fromstr --- lib/crates/fabro-config/src/builders.rs | 28 ++++++++++++------- lib/crates/fabro-config/src/defaults.rs | 5 ++-- .../fabro-config/src/layers/settings.rs | 10 ++++--- lib/crates/fabro-config/src/lib.rs | 1 - lib/crates/fabro-config/src/load.rs | 4 +-- lib/crates/fabro-config/src/parse.rs | 12 ++++---- lib/crates/fabro-config/src/project.rs | 14 +++++----- lib/crates/fabro-config/src/resolve/mod.rs | 9 +++--- lib/crates/fabro-config/src/tests/combine.rs | 2 +- lib/crates/fabro-config/src/tests/defaults.rs | 6 ++-- .../fabro-config/src/tests/resolve_server.rs | 27 +++++++++--------- 11 files changed, 62 insertions(+), 56 deletions(-) diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index b91655dbe..ed714a5aa 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -6,7 +6,6 @@ use fabro_types::{ServerSettings, UserSettings, WorkflowSettings}; use crate::defaults::DEFAULTS_LAYER; use crate::load::load_settings_path; -use crate::parse::parse_settings_layer; use crate::resolve::{ ResolveError, resolve_cli, resolve_features, resolve_project, resolve_run, resolve_server, resolve_workflow, @@ -73,7 +72,8 @@ impl ServerSettingsBuilder { } pub fn from_toml(source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Self::from_layer(&layer) } @@ -115,13 +115,15 @@ impl UserSettingsBuilder { } pub fn from_toml(source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Self::from_layer(&layer) } pub fn from_toml_with_cli_overrides(source: &str, cli: &CliLayer) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Self::from_layer_with_cli_overrides(&layer, cli) } @@ -166,7 +168,8 @@ impl RunSettingsBuilder { } pub fn from_toml(source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Self::from_layer(&layer) } @@ -211,7 +214,8 @@ pub fn server_runtime_settings_from_toml( run_overrides: Option, server_overrides: Option, ) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; resolve_server_runtime_settings(layer, run_overrides, server_overrides) } @@ -261,7 +265,8 @@ impl WorkflowSettingsBuilder { } pub fn from_toml(source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Self::from_layer(&layer) .map_err(|errors| Error::resolve("failed to resolve workflow settings", errors.into())) @@ -288,7 +293,8 @@ impl WorkflowSettingsBuilder { } pub fn workflow_toml(self, source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Ok(self.workflow_layer(layer)) } @@ -304,7 +310,8 @@ impl WorkflowSettingsBuilder { } pub fn project_toml(self, source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Ok(self.project_layer(layer)) } @@ -320,7 +327,8 @@ impl WorkflowSettingsBuilder { } pub fn user_toml(self, source: &str) -> Result { - let layer = parse_settings_layer(source) + let layer = source + .parse::() .map_err(|err| Error::parse("Failed to parse settings file", err))?; Ok(self.user_layer(layer)) } diff --git a/lib/crates/fabro-config/src/defaults.rs b/lib/crates/fabro-config/src/defaults.rs index d4684211a..22358d3c1 100644 --- a/lib/crates/fabro-config/src/defaults.rs +++ b/lib/crates/fabro-config/src/defaults.rs @@ -1,8 +1,9 @@ use std::sync::LazyLock; -use crate::{SettingsLayer, parse_settings_layer}; +use crate::SettingsLayer; pub(crate) static DEFAULTS_LAYER: LazyLock = LazyLock::new(|| { - parse_settings_layer(include_str!("defaults.toml")) + include_str!("defaults.toml") + .parse::() .expect("embedded defaults.toml must parse as a valid SettingsLayer") }); diff --git a/lib/crates/fabro-config/src/layers/settings.rs b/lib/crates/fabro-config/src/layers/settings.rs index e665de65e..bc6204cdd 100644 --- a/lib/crates/fabro-config/src/layers/settings.rs +++ b/lib/crates/fabro-config/src/layers/settings.rs @@ -9,6 +9,8 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; +use crate::parse::{ParseError, parse_settings}; + use super::cli::CliLayer; use super::features::FeaturesLayer; use super::project::ProjectLayer; @@ -36,10 +38,10 @@ pub(crate) struct SettingsLayer { } impl FromStr for SettingsLayer { - type Err = toml::de::Error; + type Err = ParseError; fn from_str(source: &str) -> Result { - toml::from_str(source) + parse_settings(source) } } @@ -103,7 +105,7 @@ impl SettingsLayer { /// with `["dev-token"]`. Use anywhere a test needs a starter /// `SettingsLayer` that the strict resolver will accept. #[must_use] - pub fn test_default() -> Self { + pub(crate) fn test_default() -> Self { let mut layer = Self::default(); layer.ensure_test_auth_methods(); layer @@ -112,7 +114,7 @@ impl SettingsLayer { /// If `server.auth.methods` is unset, populate it with `["dev-token"]`. /// Existing methods (set by a fixture) are preserved. Use to make a /// parsed-from-TOML layer resolve cleanly without overriding test intent. - pub fn ensure_test_auth_methods(&mut self) { + pub(crate) fn ensure_test_auth_methods(&mut self) { use fabro_types::settings::ServerAuthMethod; use super::server::{ServerAuthLayer, ServerLayer as ServerLayerTy}; diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index b1c43e559..c02ffa4f7 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -52,7 +52,6 @@ pub use layers::{ }; pub(crate) use layers::{Combine, SettingsLayer}; pub use parse::ParseError; -pub(crate) use parse::parse_settings_layer; pub use resolve::{ ResolveError, resolve_cli, resolve_features, resolve_project, resolve_run, resolve_server, resolve_workflow, diff --git a/lib/crates/fabro-config/src/load.rs b/lib/crates/fabro-config/src/load.rs index 1e0f10c5a..5a2254603 100644 --- a/lib/crates/fabro-config/src/load.rs +++ b/lib/crates/fabro-config/src/load.rs @@ -7,12 +7,12 @@ use std::path::{Path, PathBuf}; use fabro_types::settings::InterpString; -use crate::parse::parse_settings_layer; use crate::{Error, Result, RunGoalLayer, SettingsLayer}; pub(crate) fn load_settings_path(path: &Path) -> Result { let content = std::fs::read_to_string(path).map_err(|source| Error::read_file(path, source))?; - let mut layer = parse_settings_layer(&content) + let mut layer = content + .parse::() .map_err(|err| Error::parse_file("Failed to parse settings file", path, err))?; let base_dir = path.parent().unwrap_or_else(|| Path::new(".")); resolve_goal_file_paths(&mut layer, base_dir); diff --git a/lib/crates/fabro-config/src/parse.rs b/lib/crates/fabro-config/src/parse.rs index f3fb77232..50d842b31 100644 --- a/lib/crates/fabro-config/src/parse.rs +++ b/lib/crates/fabro-config/src/parse.rs @@ -58,7 +58,7 @@ impl fmt::Display for VersionError { impl std::error::Error for VersionError {} -pub(crate) fn parse_settings_layer(input: &str) -> Result { +pub(crate) fn parse_settings(input: &str) -> Result { let raw: toml::Value = toml::from_str(input).map_err(|e| ParseError::Toml(e.to_string()))?; validate_version(&raw).map_err(ParseError::Version)?; @@ -135,19 +135,19 @@ mod tests { #[test] fn parses_empty_file() { - let file = parse_settings_layer("").unwrap(); + let file = "".parse::().unwrap(); assert_eq!(file, SettingsLayer::default()); } #[test] fn parses_minimal_valid_file() { - let file = parse_settings_layer("_version = 1\n").unwrap(); + let file = "_version = 1\n".parse::().unwrap(); assert_eq!(file.version, Some(1)); } #[test] fn rejects_legacy_version_key_with_rename_hint() { - let err = parse_settings_layer("version = 1").unwrap_err(); + let err = "version = 1".parse::().unwrap_err(); assert!(matches!( err, ParseError::Version(VersionError::LegacyVersionKey) @@ -157,13 +157,13 @@ mod tests { #[test] fn rejects_unknown_top_level_key() { - let err = parse_settings_layer("unknown_key = 1").unwrap_err(); + let err = "unknown_key = 1".parse::().unwrap_err(); assert!(matches!(err, ParseError::UnknownTopLevelKey { .. })); } #[test] fn higher_version_rejected_with_upgrade_hint() { - let err = parse_settings_layer("_version = 99").unwrap_err(); + let err = "_version = 99".parse::().unwrap_err(); assert!(err.to_string().contains("Upgrade")); } } diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index 72c96f653..bf8cfca31 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -404,7 +404,7 @@ mod tests { #[test] fn parse_minimal_config() { - let config = crate::parse_settings_layer("_version = 1\n").unwrap(); + let config = "_version = 1\n".parse::().unwrap(); assert_eq!(config.version, Some(1)); assert!(config.project.is_none()); } @@ -429,14 +429,13 @@ directory = "custom/" #[test] fn parse_with_run_execution_retros() { - let config = crate::parse_settings_layer( - " + let config = " _version = 1 [run.execution] retros = true -", - ) +" + .parse::() .unwrap(); assert_eq!( config @@ -450,7 +449,8 @@ retros = true #[test] fn parse_rejects_legacy_llm_section() { - let err = crate::parse_settings_layer("_version = 1\n[llm]\nprovider = \"openai\"\n") + let err = "_version = 1\n[llm]\nprovider = \"openai\"\n" + .parse::() .unwrap_err(); let text = format!("{err:#}"); assert!( @@ -461,7 +461,7 @@ retros = true #[test] fn parse_higher_version_errors() { - let err = crate::parse_settings_layer("_version = 2\n").unwrap_err(); + let err = "_version = 2\n".parse::().unwrap_err(); let chain = format!("{err:#}"); assert!( chain.contains("Upgrade") || chain.to_lowercase().contains("version"), diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index d6f69a1f0..88edbd070 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -58,12 +58,11 @@ mod tests { use fabro_types::settings::run::{HookType, McpTransport, TlsMode}; - use crate::{WorkflowSettingsBuilder, parse_settings_layer}; + use crate::{SettingsLayer, WorkflowSettingsBuilder}; #[test] fn resolve_preserves_source_templates_for_mcp_and_hook_strings() { - let settings = parse_settings_layer( - r#" + let settings = r#" _version = 1 [server.auth] @@ -98,8 +97,8 @@ url = "https://hooks.example.com" [run.hooks.headers] Authorization = "Bearer {{ env.HOOK_TOKEN }}" -"#, - ) +"# + .parse::() .expect("settings fixture should parse"); let resolved = WorkflowSettingsBuilder::from_layer(&settings) diff --git a/lib/crates/fabro-config/src/tests/combine.rs b/lib/crates/fabro-config/src/tests/combine.rs index 30196e172..16470d06c 100644 --- a/lib/crates/fabro-config/src/tests/combine.rs +++ b/lib/crates/fabro-config/src/tests/combine.rs @@ -4,7 +4,7 @@ use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use crate::{Combine, SettingsLayer, StringOrSplice}; fn parse(input: &str) -> SettingsLayer { - crate::parse_settings_layer(input).expect("fixture should parse") + input.parse::().expect("fixture should parse") } #[test] diff --git a/lib/crates/fabro-config/src/tests/defaults.rs b/lib/crates/fabro-config/src/tests/defaults.rs index 774452ae7..51044b4fa 100644 --- a/lib/crates/fabro-config/src/tests/defaults.rs +++ b/lib/crates/fabro-config/src/tests/defaults.rs @@ -2,12 +2,10 @@ use fabro_types::settings::cli::OutputFormat; use fabro_types::settings::run::{ApprovalMode, RunMode, WorktreeMode}; use fabro_types::settings::server::ObjectStoreProvider; -use crate::{ - Combine, ServerSettingsBuilder, SettingsLayer, WorkflowSettingsBuilder, parse_settings_layer, -}; +use crate::{Combine, ServerSettingsBuilder, SettingsLayer, WorkflowSettingsBuilder}; fn parse(source: &str) -> SettingsLayer { - parse_settings_layer(source).expect("fixture should parse") + source.parse::().expect("fixture should parse") } fn embedded_defaults() -> SettingsLayer { diff --git a/lib/crates/fabro-config/src/tests/resolve_server.rs b/lib/crates/fabro-config/src/tests/resolve_server.rs index ec9c6755b..d545786ac 100644 --- a/lib/crates/fabro-config/src/tests/resolve_server.rs +++ b/lib/crates/fabro-config/src/tests/resolve_server.rs @@ -12,10 +12,10 @@ use temp_env::with_var; use crate::resolve::dev_token_auth_enabled; use crate::user::default_storage_dir; -use crate::{ServerSettingsBuilder, SettingsLayer, parse_settings_layer}; +use crate::{ServerSettingsBuilder, SettingsLayer}; fn parse(source: &str) -> SettingsLayer { - let mut layer = parse_settings_layer(source).expect("fixture should parse"); + let mut layer = source.parse::().expect("fixture should parse"); layer.ensure_test_auth_methods(); layer } @@ -37,7 +37,7 @@ fn resolve_errors(error: fabro_config::Error) -> Vec } } -fn render_resolve_errors(error: fabro_config::Error) -> String { +fn render_resolve_error_lines(error: fabro_config::Error) -> String { resolve_errors(error) .into_iter() .map(|error| error.to_string()) @@ -154,8 +154,7 @@ session_sandboxes = true #[test] fn parsing_rejects_inbound_listener_tls_configuration() { - let err = parse_settings_layer( - r#" + let err = r#" _version = 1 [server.listen] @@ -164,8 +163,8 @@ address = "127.0.0.1:32276" [server.listen.tls] cert = "/etc/fabro/server.pem" -"#, - ) +"# + .parse::() .expect_err("listener TLS should be rejected at parse time"); assert!(err.to_string().contains("unknown field `tls`")); @@ -185,7 +184,7 @@ endpoint = "{{ env.S3_ENDPOINT }}" "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file) .expect_err("s3 config without bucket/region should fail"), ); @@ -391,7 +390,7 @@ strategy = "server_url" "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file) .expect_err("server_url webhook strategy should require server.api.url"), ); @@ -413,7 +412,7 @@ strategy = "tailscale_funnel" "#, ); - let rendered = render_resolve_errors(ServerSettingsBuilder::from_layer(&file).expect_err( + let rendered = render_resolve_error_lines(ServerSettingsBuilder::from_layer(&file).expect_err( "configured webhook strategy should require server.integrations.github.app_id", )); @@ -431,7 +430,7 @@ entries = ["10.0.0.0/33"] "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file).expect_err("invalid CIDR should fail"), ); @@ -449,7 +448,7 @@ entries = ["github_meta_hooks"] "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file) .expect_err("github_meta_hooks should be rejected outside github webhooks"), ); @@ -472,7 +471,7 @@ entries = ["10.0.0.0/8"] "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file) .expect_err("unix allowlist without trusted proxies should fail"), ); @@ -495,7 +494,7 @@ entries = ["github_meta_hooks"] "#, ); - let rendered = render_resolve_errors( + let rendered = render_resolve_error_lines( ServerSettingsBuilder::from_layer(&file) .expect_err("unix github webhook allowlist without trusted proxies should fail"), );