From 64dbdf250072a2acb47115746e7cdc8af2020f35 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 20:49:30 -0400 Subject: [PATCH] fix(ci): resolve workspace test and lint regressions --- lib/crates/fabro-cli/src/command_context.rs | 23 +++--- lib/crates/fabro-cli/src/commands/exec.rs | 6 +- .../fabro-cli/src/commands/uninstall.rs | 5 +- lib/crates/fabro-cli/src/local_server.rs | 17 ++-- lib/crates/fabro-cli/src/manifest_builder.rs | 2 +- lib/crates/fabro-cli/src/user_config.rs | 78 +++++++++++++++++-- lib/crates/fabro-config/src/builders.rs | 65 +++++++++++++++- lib/crates/fabro-server/src/run_manifest.rs | 5 +- lib/crates/fabro-server/src/serve.rs | 2 +- lib/crates/fabro-server/src/server.rs | 13 +--- lib/crates/fabro-server/tests/it/api/tcp.rs | 13 ++-- lib/crates/fabro-server/tests/it/helpers.rs | 23 +++--- lib/crates/fabro-store/src/run_state.rs | 2 +- .../fabro-workflow/src/operations/source.rs | 5 +- .../fabro-workflow/src/operations/start.rs | 8 +- 15 files changed, 190 insertions(+), 77 deletions(-) diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index 8be555870..4e79dbf48 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -199,16 +199,16 @@ fn load_merged_settings( Some(cli_layer), )?, }; - resolve_command_settings(loaded_settings) + Ok(resolve_command_settings(loaded_settings)) } -fn resolve_command_settings(loaded_settings: LoadedSettings) -> Result { - Ok(ResolvedCommandSettings { +fn resolve_command_settings(loaded_settings: LoadedSettings) -> ResolvedCommandSettings { + ResolvedCommandSettings { storage_dir: loaded_settings.storage_dir, run_settings: loaded_settings.run_settings, server_settings: loaded_settings.server_settings, user_settings: loaded_settings.user_settings, - }) + } } #[cfg(test)] @@ -238,8 +238,7 @@ mod tests { 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, @@ -283,8 +282,7 @@ root = "/srv/fabro/default" Some(&cli_layer), ) .expect("base settings should resolve"), - ) - .expect("base settings should merge"); + ); let connection_settings = resolve_command_settings( user_config::load_resolved_settings_from_toml( r#" @@ -297,8 +295,7 @@ root = "/srv/fabro/default" Some(&cli_layer), ) .expect("connection settings should resolve"), - ) - .expect("connection settings should merge"); + ); assert_eq!( base_settings.user_settings, @@ -339,8 +336,7 @@ root = "/srv/fabro" Some(&CliLayer::default()), ) .expect("settings should resolve"), - ) - .expect("settings should merge"); + ); assert_eq!(resolved.storage_dir, PathBuf::from("/srv/fabro")); assert!(resolved.run_settings.is_ok()); @@ -362,8 +358,7 @@ command = ["demo-mcp"] Some(&CliLayer::default()), ) .expect("settings should resolve"), - ) - .expect("settings should merge"); + ); let run_settings = resolved.run_settings.expect("run settings should resolve"); assert!(run_settings.agent.mcps.contains_key("demo")); diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index b871b4056..78578838f 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -304,12 +304,12 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu // v2 MCPs live under `cli.exec.agent.mcps` (owner-specific) or // `run.agent.mcps`. For `fabro exec` we use the cli.exec path, falling // back to run.agent.mcps if unset. - let mcp_servers: Vec = if !cli.exec.agent.mcps.is_empty() { - cli.exec.agent.mcps.values().cloned().collect() - } else { + let mcp_servers: Vec = if cli.exec.agent.mcps.is_empty() { ctx.run_settings() .map(|settings| settings.agent.mcps.values().cloned().collect()) .unwrap_or_default() + } else { + cli.exec.agent.mcps.values().cloned().collect() }; if let Some(target) = server_target { tracing::info!(transport = "server", "Agent session starting"); diff --git a/lib/crates/fabro-cli/src/commands/uninstall.rs b/lib/crates/fabro-cli/src/commands/uninstall.rs index 2e660d84b..f29f3b73e 100644 --- a/lib/crates/fabro-cli/src/commands/uninstall.rs +++ b/lib/crates/fabro-cli/src/commands/uninstall.rs @@ -59,8 +59,9 @@ pub(crate) async fn run_uninstall(args: &UninstallArgs, ctx: &CommandContext) -> let storage_dir = local_server::LocalServerConfig::load_with_storage_dir(None) .ok() - .map(|settings| settings.storage_dir().to_path_buf()) - .unwrap_or_else(user_config::default_storage_dir); + .map_or_else(user_config::default_storage_dir, |settings| { + settings.storage_dir().to_path_buf() + }); let inventory = build_inventory(&home_root, &storage_dir)?; diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 14988105c..21f6cf427 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -4,6 +4,8 @@ use std::path::{Path, PathBuf}; use anyhow::Result; use fabro_config::bind::BindRequest; +use fabro_config::user::default_storage_dir; +use fabro_server::serve::resolve_bind_request_from_server_settings; use fabro_types::ServerSettings; use fabro_types::settings::{InterpString, ServerAuthMethod}; @@ -58,7 +60,7 @@ impl LocalServerConfig { .server_settings .as_ref() .map_err(|err| anyhow::anyhow!("{err}"))?; - fabro_server::serve::resolve_bind_request_from_server_settings(settings, cli_override) + resolve_bind_request_from_server_settings(settings, cli_override) } } @@ -72,11 +74,10 @@ fn storage_dir_from_toml_with_lookup( ) -> Result { let document: toml::Value = toml::from_str(source) .map_err(|err| anyhow::anyhow!("failed to parse settings file: {err}"))?; - let storage_root = string_at_path(&document, &["server", "storage", "root"]) - .map(|root| InterpString::parse(&root)) - .unwrap_or_else(|| { - InterpString::parse(&fabro_config::user::default_storage_dir().to_string_lossy()) - }); + let storage_root = string_at_path(&document, &["server", "storage", "root"]).map_or_else( + || InterpString::parse(&default_storage_dir().to_string_lossy()), + |root| InterpString::parse(&root), + ); let resolved_root = storage_root .resolve(lookup) .map_err(|err| anyhow::anyhow!("failed to resolve {}: {err}", storage_root.as_source()))?; @@ -95,6 +96,8 @@ fn string_at_path(document: &toml::Value, path: &[&str]) -> Option { mod tests { use std::path::PathBuf; + use fabro_config::user::default_storage_dir; + use super::{storage_dir_from_toml, storage_dir_from_toml_with_lookup}; #[test] @@ -116,7 +119,7 @@ root = "/srv/fabro" fn storage_dir_from_toml_defaults_without_auth_methods() { let path = storage_dir_from_toml("_version = 1\n").expect("default storage dir"); - assert_eq!(path, fabro_config::user::default_storage_dir()); + assert_eq!(path, default_storage_dir()); } #[test] diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 1f41459ef..e325c0309 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -352,7 +352,7 @@ fn collect_workflow_config_files( .map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?; let run = document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))? .unwrap_or_default(); diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index fa279d06e..078eab5c7 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -3,10 +3,10 @@ use std::str::FromStr; use anyhow::Result; pub(crate) use fabro_client::ServerTarget; -use fabro_config::user::default_socket_path; -pub(crate) use fabro_config::user::{active_settings_path, default_storage_dir}; +pub(crate) use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, default_storage_dir}; +use fabro_config::user::{default_settings_path, default_socket_path}; use fabro_config::{ - CliLayer, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, load_config_file, + CliLayer, ParseError, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, }; use fabro_types::settings::cli::CliTargetSettings; use fabro_types::settings::{CliNamespace, InterpString, RunNamespace}; @@ -52,7 +52,40 @@ pub(crate) fn load_resolved_settings( } fn load_settings_document(config_path: Option<&Path>) -> anyhow::Result { - let table: toml::Table = load_config_file(config_path, "settings.toml")?; + load_settings_document_with_lookup(config_path, |name| std::env::var_os(name)) +} + +#[expect( + clippy::disallowed_methods, + reason = "sync settings load during CLI startup; not on a Tokio path" +)] +fn load_settings_document_with_lookup( + config_path: Option<&Path>, + lookup: impl Fn(&str) -> Option, +) -> anyhow::Result { + let config_path = config_path + .map(Path::to_path_buf) + .or_else(|| lookup(FABRO_CONFIG_ENV).map(PathBuf::from)); + + let path = if let Some(path) = config_path { + path + } else { + let default_path = default_settings_path(); + if !default_path.is_file() { + return Ok(toml::Value::Table(toml::Table::new())); + } + default_path + }; + + let contents = std::fs::read_to_string(&path) + .map_err(|source| fabro_config::Error::read_file(&path, source))?; + let table: toml::Table = toml::from_str(&contents).map_err(|source| { + fabro_config::Error::parse_file( + "Failed to parse settings file", + &path, + ParseError::Toml(source.to_string()), + ) + })?; Ok(toml::Value::Table(table)) } @@ -104,9 +137,10 @@ fn storage_dir_from_document_with_lookup( return Ok(dir.to_path_buf()); } - let storage_root = string_at_path(document, &["server", "storage", "root"]) - .map(|root| InterpString::parse(&root)) - .unwrap_or_else(|| InterpString::parse(&default_storage_dir().to_string_lossy())); + let storage_root = string_at_path(document, &["server", "storage", "root"]).map_or_else( + || InterpString::parse(&default_storage_dir().to_string_lossy()), + |root| InterpString::parse(&root), + ); let resolved_root = storage_root.resolve(lookup)?; Ok(PathBuf::from(resolved_root.value)) } @@ -378,4 +412,34 @@ root = "{{ env.FABRO_STORAGE_ROOT }}" temp.path() ); } + + #[test] + #[expect( + clippy::disallowed_methods, + reason = "unit test writes a temporary settings fixture with sync std::fs::write" + )] + fn load_settings_document_uses_fabro_config_env_for_storage_root() { + let dir = tempfile::tempdir().unwrap(); + let config_path = dir.path().join("settings.toml"); + std::fs::write( + &config_path, + r#" +_version = 1 + +[server.storage] +root = "/srv/fabro" +"#, + ) + .unwrap(); + + let document = load_settings_document_with_lookup(None, |_| { + Some(config_path.clone().into_os_string()) + }) + .expect("settings document should load"); + + assert_eq!( + storage_dir_from_document(&document, None).unwrap(), + PathBuf::from("/srv/fabro") + ); + } } diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index f3639b137..ab6b041f5 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -283,7 +283,7 @@ impl WorkflowSettingsBuilder { #[must_use] pub(crate) fn args_layer(mut self, layer: SettingsLayer) -> Self { - self.args = layer; + self.args = layer.combine(self.args); self } @@ -458,9 +458,14 @@ fn finish_dense_result( #[cfg(test)] mod tests { - use fabro_types::settings::run::RunMode; + use std::collections::HashMap; - use super::RunSettingsBuilder; + use fabro_types::settings::InterpString; + use fabro_types::settings::cli::OutputVerbosity; + use fabro_types::settings::run::{ApprovalMode, RunMode}; + + use super::{RunSettingsBuilder, WorkflowSettingsBuilder}; + use crate::{CliLayer, CliOutputLayer, ReplaceMap, RunExecutionLayer, RunLayer, RunModelLayer}; #[test] fn run_settings_builder_resolves_run_namespace() { @@ -481,4 +486,58 @@ command = ["demo-mcp"] assert_eq!(settings.execution.mode, RunMode::DryRun); assert!(settings.agent.mcps.contains_key("demo")); } + + #[test] + fn workflow_builder_preserves_run_overrides_when_cli_overrides_are_added() { + let settings = WorkflowSettingsBuilder::new() + .run_overrides(RunLayer { + metadata: ReplaceMap::from(HashMap::from([("env".to_string(), "cli".to_string())])), + model: Some(RunModelLayer { + provider: Some(InterpString::parse("openai")), + name: Some(InterpString::parse("gpt-5")), + fallbacks: Vec::new(), + }), + execution: Some(RunExecutionLayer { + mode: Some(RunMode::DryRun), + approval: Some(ApprovalMode::Auto), + retros: Some(false), + }), + ..RunLayer::default() + }) + .cli_overrides(CliLayer { + output: Some(CliOutputLayer { + verbosity: Some(OutputVerbosity::Verbose), + ..CliOutputLayer::default() + }), + ..CliLayer::default() + }) + .build() + .expect("settings should resolve"); + + assert_eq!( + settings.run.metadata.get("env").map(String::as_str), + Some("cli") + ); + assert_eq!( + settings + .run + .model + .provider + .as_ref() + .map(InterpString::as_source), + Some("openai".to_string()) + ); + assert_eq!( + settings + .run + .model + .name + .as_ref() + .map(InterpString::as_source), + Some("gpt-5".to_string()) + ); + assert_eq!(settings.run.execution.mode, RunMode::DryRun); + assert_eq!(settings.run.execution.approval, ApprovalMode::Auto); + assert!(!settings.run.execution.retros); + } } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 5d94df8c6..c5cc7b87d 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -55,6 +55,7 @@ struct ManifestSettingsOverrides { cli: Option, } +#[cfg(test)] pub(crate) fn manifest_run_defaults(run: Option<&RunLayer>) -> RunLayer { run.cloned().unwrap_or_default() } @@ -226,7 +227,7 @@ fn root_workflow_run_layer( .map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?; let mut run = document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))? .unwrap_or_default(); @@ -975,7 +976,7 @@ mod tests { let mut document: toml::Table = source.parse().expect("v2 fixture should parse"); document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .expect("run settings should parse") .unwrap_or_default() diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index f1d9944d0..1ecfb2ede 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -1067,7 +1067,7 @@ mod tests { let mut document: toml::Table = source.parse().expect("v2 fixture should parse"); document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .expect("run settings should parse") .unwrap_or_default() diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index cd2e17365..4ff628b2f 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2658,10 +2658,6 @@ pub(crate) fn create_test_app_state_with_runtime_settings_and_session_key( } #[cfg(test)] -#[expect( - clippy::disallowed_methods, - reason = "test helper writes a fixture server.env with sync std::fs::write" -)] pub(crate) fn create_test_app_state_with_session_key( server_settings: ServerSettings, manifest_run_defaults: RunLayer, @@ -7491,7 +7487,7 @@ mod tests { let mut document: toml::Table = source.parse().expect("run defaults should parse"); document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .expect("run defaults should parse") .unwrap_or_default() @@ -11045,11 +11041,8 @@ timeout = "30s" #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn concurrency_limit_respected() { - let state = create_app_state_with_options( - default_test_server_settings(), - RunLayer::default(), - 1, - ); + let state = + create_app_state_with_options(default_test_server_settings(), RunLayer::default(), 1); let app = test_app_with_scheduler(Arc::clone(&state)); // Create and start two runs with max_concurrent_runs=1 diff --git a/lib/crates/fabro-server/tests/it/api/tcp.rs b/lib/crates/fabro-server/tests/it/api/tcp.rs index 0b21583e3..733211846 100644 --- a/lib/crates/fabro-server/tests/it/api/tcp.rs +++ b/lib/crates/fabro-server/tests/it/api/tcp.rs @@ -106,14 +106,11 @@ async fn spawn_served_listener( .await }); - let bind = match rx.await { - Ok(bind) => bind, - Err(_) => { - let result = handle - .await - .expect("server task should not panic before reporting readiness"); - panic!("server should report its bind address: {result:?}"); - } + let Ok(bind) = rx.await else { + let result = handle + .await + .expect("server task should not panic before reporting readiness"); + panic!("server should report its bind address: {result:?}"); }; (handle, bind, tempdir) } diff --git a/lib/crates/fabro-server/tests/it/helpers.rs b/lib/crates/fabro-server/tests/it/helpers.rs index 667ca826d..c1c062c73 100644 --- a/lib/crates/fabro-server/tests/it/helpers.rs +++ b/lib/crates/fabro-server/tests/it/helpers.rs @@ -62,7 +62,7 @@ pub(crate) fn settings_from_toml(source: &str) -> TestAppSettings { ensure_test_auth_methods(&mut document); let manifest_run_defaults = document .remove("run") - .map(|value| value.try_into::()) + .map(toml::Value::try_into::) .transpose() .expect("test run settings should parse") .unwrap_or_default(); @@ -93,17 +93,18 @@ pub(crate) fn test_app_state_with_options( } pub(crate) fn test_settings() -> TestAppSettings { - let mut settings = TestAppSettings::default(); - settings.manifest_run_defaults = RunLayer { - sandbox: Some(RunSandboxLayer { - local: Some(LocalSandboxLayer { - worktree_mode: Some(WorktreeMode::Never), + TestAppSettings { + manifest_run_defaults: RunLayer { + sandbox: Some(RunSandboxLayer { + local: Some(LocalSandboxLayer { + worktree_mode: Some(WorktreeMode::Never), + }), + ..RunSandboxLayer::default() }), - ..RunSandboxLayer::default() - }), - ..RunLayer::default() - }; - settings + ..RunLayer::default() + }, + ..TestAppSettings::default() + } } pub(crate) fn test_app_with_scheduler(state: Arc) -> axum::Router { diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index b4c3f39bf..126201ec2 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -647,7 +647,7 @@ mod tests { let state: RunProjection = serde_json::from_value(serde_json::json!({ "spec": { "run_id": "01JW6A7VNFZSFF0SKXJG29Z2M3", - "settings": { "_version": 1 }, + "settings": WorkflowSettings::default(), "graph": { "name": "ship", "nodes": {}, "edges": [], "attrs": {} }, "workflow_slug": "demo", "working_directory": "/tmp/project", diff --git a/lib/crates/fabro-workflow/src/operations/source.rs b/lib/crates/fabro-workflow/src/operations/source.rs index e0bc5d969..051818597 100644 --- a/lib/crates/fabro-workflow/src/operations/source.rs +++ b/lib/crates/fabro-workflow/src/operations/source.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::Context; -use fabro_config::project::resolve_working_directory_from_run; +use fabro_config::project::{resolve_workflow_path, resolve_working_directory_from_run}; use fabro_config::run::resolve_run_goal_from_namespace; use fabro_types::WorkflowSettings; @@ -65,8 +65,7 @@ fn workflow_slug_from_path(workflow_path: &Path) -> Option { pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result { match request.workflow { WorkflowInput::Path(workflow_path) => { - let resolution = - fabro_config::project::resolve_workflow_path(&workflow_path, &request.cwd)?; + let resolution = resolve_workflow_path(&workflow_path, &request.cwd)?; let settings = request.settings; let raw_source = std::fs::read_to_string(&resolution.dot_path) .with_context(|| format!("Failed to read {}", resolution.dot_path.display()))?; diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index f65d90452..ff21b2e8f 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -311,7 +311,7 @@ impl RunSession { let resolved = &settings.run; - let sandbox_provider = resolve_sandbox_provider(&resolved)?; + let sandbox_provider = resolve_sandbox_provider(resolved)?; let sandbox_provider = if resolved.execution.mode == RunMode::DryRun && !sandbox_provider.is_local() { SandboxProvider::Local @@ -366,7 +366,7 @@ impl RunSession { None => None, }; SandboxSpec::Daytona { - config: resolve_daytona_config(&resolved).unwrap_or_default(), + config: resolve_daytona_config(resolved).unwrap_or_default(), github_app: services.github_app.clone(), run_id: Some(record.run_id), clone_branch: detected_base_branch.or_else(|| record.base_branch.clone()), @@ -434,7 +434,7 @@ impl RunSession { artifact_sink: services.artifact_sink, git, github_app: services.github_app.clone(), - worktree_mode: Some(resolve_worktree_mode(&resolved)), + worktree_mode: Some(resolve_worktree_mode(resolved)), registry_override: services.registry_override, retro_enabled: resolved.execution.retros && project_config::is_retro_enabled(), preserve_sandbox: resolved.sandbox.preserve, @@ -971,8 +971,8 @@ mod tests { use chrono::Utc; use fabro_config::{RunExecutionLayer, RunLayer, WorkflowSettingsBuilder}; use fabro_store::Database; - use fabro_types::{WorkflowSettings, fixtures}; use fabro_types::settings::run::RunMode; + use fabro_types::{WorkflowSettings, fixtures}; use object_store::memory::InMemory; use super::*;