From 83fc1602ea3d0549dc77d529d2f28bd2b49da9f9 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 17:09:21 -0400 Subject: [PATCH] route cli settings loads through dense config --- lib/crates/fabro-cli/src/command_context.rs | 124 ++++++++---------- lib/crates/fabro-cli/src/local_server.rs | 70 +++++----- lib/crates/fabro-cli/src/user_config.rs | 137 ++++++++++++++------ 3 files changed, 185 insertions(+), 146 deletions(-) diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index b52d02515..289ba63b7 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -2,9 +2,8 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::{Context as _, Result, bail}; -use fabro_config::{RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder}; +use fabro_types::settings::RunNamespace; use fabro_types::settings::cli::{CliLayer, OutputFormat, OutputVerbosity}; -use fabro_types::settings::{Combine, RunNamespace, SettingsLayer}; use fabro_types::{ServerSettings, UserSettings}; use fabro_util::printer::Printer; use tokio::sync::OnceCell; @@ -13,6 +12,7 @@ use crate::args::{ ServerConnectionArgs, ServerTargetArgs, printer_from_verbosity, require_no_json_override, }; use crate::server_client::Client; +use crate::user_config::LoadedSettings; use crate::{server_client, user_config}; #[derive(Clone, Debug)] @@ -185,42 +185,28 @@ fn load_merged_settings( cli_layer: &CliLayer, server_mode: &ServerMode, ) -> Result { - let disk_settings = match server_mode { - ServerMode::None | ServerMode::ByTarget { .. } => user_config::load_settings()?, + let loaded_settings = match server_mode { + ServerMode::None | ServerMode::ByTarget { .. } => { + user_config::load_resolved_settings(None, None, Some(cli_layer))? + } ServerMode::ByStorageDir { storage_dir_override, .. - } => user_config::load_settings_with_storage_dir(storage_dir_override.as_deref())?, + } => user_config::load_resolved_settings( + None, + storage_dir_override.as_deref(), + Some(cli_layer), + )?, }; - merge_settings_layer(disk_settings, cli_layer) + resolve_command_settings(loaded_settings) } -fn merge_settings_layer( - disk_settings: SettingsLayer, - cli_layer: &CliLayer, -) -> Result { - let storage_dir = crate::local_server::storage_dir(&disk_settings)?; - let run_settings = RunSettingsBuilder::from_layer(&disk_settings).map_err(|err| { - // Keep command context tolerant even when unrelated run defaults - // do not resolve cleanly. - err.to_string() - }); - let server_settings = ServerSettingsBuilder::from_layer(&disk_settings).map_err(|err| { - // Keep storage-dir and CLI-target resolution tolerant even when full - // server resolution would reject a partial local settings file. - err.to_string() - }); - let merged_settings = SettingsLayer { - cli: Some(cli_layer.clone()), - ..SettingsLayer::default() - } - .combine(disk_settings); - let user_settings = UserSettingsBuilder::from_layer(&merged_settings)?; +fn resolve_command_settings(loaded_settings: LoadedSettings) -> Result { Ok(ResolvedCommandSettings { - storage_dir, - run_settings, - server_settings, - user_settings, + storage_dir: loaded_settings.storage_dir, + run_settings: loaded_settings.run_settings, + server_settings: loaded_settings.server_settings, + user_settings: loaded_settings.user_settings, }) } @@ -228,14 +214,12 @@ fn merge_settings_layer( mod tests { use std::path::PathBuf; - use fabro_config::parse_settings_layer; - use fabro_types::settings::InterpString; use fabro_types::settings::cli::{CliLayer, CliOutputLayer, OutputFormat, OutputVerbosity}; - use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; use fabro_util::printer::Printer; use tokio::sync::OnceCell; - use super::{CommandContext, ServerMode, merge_settings_layer}; + use super::{CommandContext, ServerMode, resolve_command_settings}; + use crate::user_config; fn cli_layer_with_json_and_verbose() -> CliLayer { CliLayer { @@ -249,9 +233,11 @@ mod tests { fn synthetic_context(process_local_json: bool, printer: Printer) -> CommandContext { let cli_layer = cli_layer_with_json_and_verbose(); - let resolved_settings = - merge_settings_layer(parse_settings_layer("_version = 1\n").unwrap(), &cli_layer) - .expect("settings should merge"); + let resolved_settings = resolve_command_settings( + user_config::load_resolved_settings_from_toml("_version = 1\n", None, Some(&cli_layer)) + .expect("settings should resolve"), + ) + .expect("settings should merge"); CommandContext { printer, process_local_json, @@ -267,18 +253,6 @@ mod tests { } } - fn with_storage_dir_override( - mut layer: fabro_types::settings::SettingsLayer, - path: &std::path::Path, - ) -> fabro_types::settings::SettingsLayer { - let server = layer.server.get_or_insert_with(ServerLayer::default); - let storage = server - .storage - .get_or_insert_with(ServerStorageLayer::default); - storage.root = Some(InterpString::parse(&path.display().to_string())); - layer - } - #[test] fn context_exposes_resolved_output_and_explicit_json_state() { let ctx = synthetic_context(true, Printer::Default); @@ -295,24 +269,34 @@ mod tests { #[test] fn storage_dir_override_only_changes_storage_root_in_merged_settings() { let cli_layer = cli_layer_with_json_and_verbose(); - let base_disk_settings = parse_settings_layer( - r#" + let base_settings = resolve_command_settings( + user_config::load_resolved_settings_from_toml( + r#" _version = 1 [server.storage] root = "/srv/fabro/default" "#, + None, + Some(&cli_layer), + ) + .expect("base settings should resolve"), ) - .expect("settings fixture should parse"); - let override_disk_settings = with_storage_dir_override( - base_disk_settings.clone(), - std::path::Path::new("/srv/fabro/override"), - ); + .expect("base settings should merge"); + let connection_settings = resolve_command_settings( + user_config::load_resolved_settings_from_toml( + r#" +_version = 1 - let base_settings = merge_settings_layer(base_disk_settings, &cli_layer) - .expect("base settings should merge"); - let connection_settings = merge_settings_layer(override_disk_settings, &cli_layer) - .expect("connection settings should merge"); +[server.storage] +root = "/srv/fabro/default" +"#, + Some(std::path::Path::new("/srv/fabro/override")), + Some(&cli_layer), + ) + .expect("connection settings should resolve"), + ) + .expect("connection settings should merge"); assert_eq!( base_settings.user_settings, @@ -341,17 +325,18 @@ root = "/srv/fabro/default" #[test] fn storage_dir_stays_available_when_server_settings_do_not_resolve() { - let resolved = merge_settings_layer( - parse_settings_layer( + let resolved = resolve_command_settings( + user_config::load_resolved_settings_from_toml( r#" _version = 1 [server.storage] root = "/srv/fabro" "#, + None, + Some(&CliLayer::default()), ) - .expect("settings fixture should parse"), - &CliLayer::default(), + .expect("settings should resolve"), ) .expect("settings should merge"); @@ -362,8 +347,8 @@ root = "/srv/fabro" #[test] fn run_settings_include_run_agent_mcps() { - let resolved = merge_settings_layer( - parse_settings_layer( + let resolved = resolve_command_settings( + user_config::load_resolved_settings_from_toml( r#" _version = 1 @@ -371,9 +356,10 @@ _version = 1 type = "stdio" command = ["demo-mcp"] "#, + None, + Some(&CliLayer::default()), ) - .expect("settings fixture should parse"), - &CliLayer::default(), + .expect("settings should resolve"), ) .expect("settings should merge"); diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 2dcb35629..17f4599dd 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -1,14 +1,9 @@ //! Helpers for CLI code that manages the local Fabro server on this host. -//! -//! This module is the only generic CLI lifecycle surface allowed to read -//! `[server.*]` settings. User-facing CLI commands outside same-host server -//! lifecycle should not call into it. use std::path::{Path, PathBuf}; use anyhow::Result; use fabro_config::bind::BindRequest; -use fabro_config::{ServerSettingsBuilder, parse_settings_layer}; use fabro_types::ServerSettings; use fabro_types::settings::{ServerAuthMethod, SettingsLayer}; @@ -23,34 +18,27 @@ pub(crate) struct LocalServerConfig { impl LocalServerConfig { pub(crate) fn load(config_path: Option<&Path>, storage_dir: Option<&Path>) -> Result { - let settings = - user_config::load_settings_with_config_and_storage_dir(config_path, storage_dir)?; - Self::from_layer(&settings) + let settings = user_config::load_resolved_settings(config_path, storage_dir, None)?; + Ok(Self::from_loaded_settings(settings)) } pub(crate) fn load_with_storage_dir(storage_dir: Option<&Path>) -> Result { - let settings = user_config::load_settings_with_storage_dir(storage_dir)?; - Self::from_layer(&settings) + let settings = user_config::load_resolved_settings(None, storage_dir, None)?; + Ok(Self::from_loaded_settings(settings)) } - fn from_layer(settings: &SettingsLayer) -> Result { - let storage_dir = storage_dir(settings)?; - let config_log_level = settings - .server - .as_ref() - .and_then(|server| server.logging.as_ref()) - .and_then(|logging| logging.level.clone()); - let server_settings = resolved_server_settings(settings).map_err(|err| err.to_string()); + fn from_loaded_settings(settings: user_config::LoadedSettings) -> Self { + let server_settings = settings.server_settings; let auth_methods = server_settings .as_ref() .map(|resolved| resolved.server.auth.methods.clone()) .unwrap_or_default(); - Ok(Self { - storage_dir, + Self { + storage_dir: settings.storage_dir, auth_methods, - config_log_level, + config_log_level: settings.config_log_level, server_settings, - }) + } } pub(crate) fn storage_dir(&self) -> &Path { @@ -75,20 +63,16 @@ impl LocalServerConfig { } pub(crate) fn storage_dir_from_toml(source: &str) -> Result { - let settings = parse_settings_layer(source) - .map_err(|err| anyhow::anyhow!("failed to parse settings file: {err}"))?; - storage_dir(&settings) + storage_dir_from_toml_with_lookup(source, &|name| std::env::var(name).ok()) } -pub(crate) fn storage_dir(settings: &SettingsLayer) -> Result { - storage_dir_with_lookup(settings, &|name| std::env::var(name).ok()) -} - -pub(crate) fn storage_dir_with_lookup( - settings: &SettingsLayer, +fn storage_dir_from_toml_with_lookup( + source: &str, lookup: &dyn Fn(&str) -> Option, ) -> Result { - let storage_root = settings + let layer: SettingsLayer = toml::from_str(source) + .map_err(|err| anyhow::anyhow!("failed to parse settings file: {err}"))?; + let storage_root = layer .server .as_ref() .and_then(|server| server.storage.as_ref()) @@ -104,15 +88,11 @@ pub(crate) fn storage_dir_with_lookup( Ok(PathBuf::from(resolved_root.value)) } -fn resolved_server_settings(settings: &SettingsLayer) -> Result { - ServerSettingsBuilder::from_layer(settings).map_err(Into::into) -} - #[cfg(test)] mod tests { use std::path::PathBuf; - use super::storage_dir_from_toml; + use super::{storage_dir_from_toml, storage_dir_from_toml_with_lookup}; #[test] fn storage_dir_from_toml_reads_explicit_root_without_full_server_resolution() { @@ -135,4 +115,20 @@ root = "/srv/fabro" assert_eq!(path, fabro_config::user::default_storage_dir()); } + + #[test] + fn storage_dir_from_toml_resolves_env_interpolation() { + let path = storage_dir_from_toml_with_lookup( + r#" +_version = 1 + +[server.storage] +root = "{{ env.FABRO_STORAGE_ROOT }}" +"#, + &|name| (name == "FABRO_STORAGE_ROOT").then_some("/srv/fabro".to_string()), + ) + .expect("storage root should resolve"); + + assert_eq!(path, PathBuf::from("/srv/fabro")); + } } diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index b9c28e3f0..ea7b304de 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -1,52 +1,101 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str::FromStr; use anyhow::Result; pub(crate) use fabro_client::ServerTarget; pub(crate) use fabro_config::user::{active_settings_path, default_storage_dir}; use fabro_config::user::{default_socket_path, load_settings_config}; -use fabro_types::UserSettings; -use fabro_types::settings::cli::CliTargetSettings; -use fabro_types::settings::{CliNamespace, SettingsLayer}; +use fabro_config::{RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder}; +use fabro_types::settings::cli::{CliLayer, CliTargetSettings}; +use fabro_types::settings::{CliNamespace, Combine, RunNamespace, SettingsLayer}; +use fabro_types::{ServerSettings, UserSettings}; use fabro_util::version::FABRO_VERSION; use tracing::debug; use crate::args::ServerTargetArgs; -pub(crate) fn load_settings() -> anyhow::Result { - load_settings_with_config_and_storage_dir(None, None) +pub(crate) struct LoadedSettings { + pub(crate) storage_dir: PathBuf, + pub(crate) config_log_level: Option, + pub(crate) run_settings: std::result::Result, + pub(crate) server_settings: std::result::Result, + pub(crate) user_settings: UserSettings, } -pub(crate) fn load_settings_with_storage_dir( - storage_dir: Option<&Path>, -) -> anyhow::Result { - load_settings_with_config_and_storage_dir(None, storage_dir) -} - -pub(crate) fn load_settings_with_config_and_storage_dir( +pub(crate) fn load_resolved_settings( config_path: Option<&Path>, storage_dir: Option<&Path>, -) -> anyhow::Result { + cli_layer: Option<&CliLayer>, +) -> anyhow::Result { let layer = load_settings_config(config_path)?; - Ok(apply_storage_dir_override(layer, storage_dir)) + resolve_loaded_settings(layer, storage_dir, cli_layer) } -fn apply_storage_dir_override( - mut layer: SettingsLayer, +fn resolve_loaded_settings( + layer: SettingsLayer, storage_dir: Option<&Path>, -) -> SettingsLayer { - use fabro_types::settings::InterpString; - use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; + cli_layer: Option<&CliLayer>, +) -> anyhow::Result { + let storage_override = storage_dir.map(Path::to_path_buf); + let storage_dir = storage_dir_from_layer(&layer, storage_dir)?; + let config_log_level = layer + .server + .as_ref() + .and_then(|server| server.logging.as_ref()) + .and_then(|logging| logging.level.clone()); + let run_settings = RunSettingsBuilder::from_layer(&layer).map_err(|err| err.to_string()); + let server_settings = ServerSettingsBuilder::from_layer(&layer) + .map(|settings| match storage_override.as_deref() { + Some(dir) => settings.with_storage_override(dir), + None => settings, + }) + .map_err(|err| err.to_string()); + let user_settings_layer = if let Some(cli_layer) = cli_layer { + SettingsLayer { + cli: Some(cli_layer.clone()), + ..SettingsLayer::default() + } + .combine(layer.clone()) + } else { + layer.clone() + }; + let user_settings = UserSettingsBuilder::from_layer(&user_settings_layer)?; + Ok(LoadedSettings { + storage_dir, + config_log_level, + run_settings, + server_settings, + user_settings, + }) +} + +fn storage_dir_from_layer( + layer: &SettingsLayer, + storage_dir: Option<&Path>, +) -> anyhow::Result { + storage_dir_from_layer_with_lookup(layer, storage_dir, &|name| std::env::var(name).ok()) +} + +fn storage_dir_from_layer_with_lookup( + layer: &SettingsLayer, + storage_dir: Option<&Path>, + lookup: &dyn Fn(&str) -> Option, +) -> anyhow::Result { if let Some(dir) = storage_dir { - let server = layer.server.get_or_insert_with(ServerLayer::default); - let storage = server - .storage - .get_or_insert_with(ServerStorageLayer::default); - storage.root = Some(InterpString::parse(&dir.display().to_string())); + return Ok(dir.to_path_buf()); } - layer + let storage_root = layer + .server + .as_ref() + .and_then(|server| server.storage.as_ref()) + .and_then(|storage| storage.root.clone()) + .unwrap_or_else(|| { + fabro_types::settings::InterpString::parse(&default_storage_dir().to_string_lossy()) + }); + let resolved_root = storage_root.resolve(lookup)?; + Ok(PathBuf::from(resolved_root.value)) } /// Pull the resolved CLI target configuration out of `[cli.target]`. @@ -102,17 +151,27 @@ pub(crate) fn cli_http_client_builder() -> fabro_http::HttpClientBuilder { fabro_http::HttpClientBuilder::new().user_agent(format!("fabro-cli/{FABRO_VERSION}")) } +#[cfg(test)] +pub(crate) fn load_resolved_settings_from_toml( + source: &str, + storage_dir: Option<&Path>, + cli_layer: Option<&CliLayer>, +) -> anyhow::Result { + let layer: SettingsLayer = toml::from_str(source) + .map_err(|err| anyhow::anyhow!("failed to parse settings file: {err}"))?; + resolve_loaded_settings(layer, storage_dir, cli_layer) +} + #[cfg(test)] mod tests { use std::path::PathBuf; + use fabro_config::UserSettingsBuilder; use fabro_config::user::default_storage_dir; - use fabro_config::{UserSettingsBuilder, parse_settings_layer}; use fabro_types::UserSettings; use super::*; use crate::args::ServerTargetArgs; - use crate::local_server; fn server_target_args(value: Option<&str>) -> ServerTargetArgs { ServerTargetArgs { @@ -124,10 +183,6 @@ mod tests { UserSettingsBuilder::from_toml(source).expect("fixture should resolve") } - fn parse_layer(source: &str) -> SettingsLayer { - parse_settings_layer(source).expect("fixture should parse") - } - #[test] fn exec_has_no_server_target_by_default() { assert_eq!(exec_server_target(&server_target_args(None)).unwrap(), None); @@ -234,45 +289,47 @@ url = "https://config.example.com" #[test] fn storage_dir_defaults_without_server_auth_methods() { - let settings = SettingsLayer::default(); + let layer = SettingsLayer::default(); assert_eq!( - local_server::storage_dir(&settings).unwrap(), + storage_dir_from_layer(&layer, None).unwrap(), default_storage_dir() ); } #[test] fn storage_dir_uses_explicit_server_storage_root() { - let settings = parse_layer( + let layer: SettingsLayer = toml::from_str( r#" _version = 1 [server.storage] root = "/srv/fabro" "#, - ); + ) + .expect("fixture should parse"); assert_eq!( - local_server::storage_dir(&settings).unwrap(), + storage_dir_from_layer(&layer, None).unwrap(), PathBuf::from("/srv/fabro") ); } #[test] fn storage_dir_resolves_env_interpolated_root() { - let settings = parse_layer( + let layer: SettingsLayer = toml::from_str( r#" _version = 1 [server.storage] root = "{{ env.FABRO_STORAGE_ROOT }}" "#, - ); + ) + .expect("fixture should parse"); let temp = tempfile::tempdir().unwrap(); assert_eq!( - local_server::storage_dir_with_lookup(&settings, &|name| { + storage_dir_from_layer_with_lookup(&layer, None, &|name| { (name == "FABRO_STORAGE_ROOT").then(|| temp.path().display().to_string()) }) .unwrap(),