From 06d9ef61189348a73f7b7d93db682d8a015a6f9e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 10 Apr 2026 08:49:22 -0400 Subject: [PATCH] refactor(cli): inject user settings layer into build_run_manifest build_run_manifest was reading FABRO_CONFIG env and ~/.fabro/settings.toml internally, which forced its 3 unit tests to use unsafe std::env::set_var to isolate from the developer's real config. This violates the project rule against mutating shared mutable state in tests. Add user_layer: SettingsLayer and user_settings_path: Option to ManifestBuildInput so callers pass the user layer explicitly. - Production callers (graph, preflight, validate, run/create) load via load_settings_user() + active_settings_path(None) at the command boundary. - Tests pass SettingsLayer::default() and None, needing no env access. - Delete all unsafe { set_var/remove_var } blocks and #[allow(unsafe_code)] attributes from the 3 manifest_builder tests. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/graph.rs | 4 ++ .../fabro-cli/src/commands/preflight.rs | 4 ++ .../fabro-cli/src/commands/run/create.rs | 4 ++ lib/crates/fabro-cli/src/commands/validate.rs | 4 ++ lib/crates/fabro-cli/src/manifest_builder.rs | 64 +++++-------------- 5 files changed, 31 insertions(+), 49 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/graph.rs b/lib/crates/fabro-cli/src/commands/graph.rs index caae38781..eeef38768 100644 --- a/lib/crates/fabro-cli/src/commands/graph.rs +++ b/lib/crates/fabro-cli/src/commands/graph.rs @@ -2,6 +2,8 @@ use std::io::Write; use anyhow::bail; use fabro_api::types; +use fabro_config::load::load_settings_user; +use fabro_config::user::active_settings_path; use fabro_types::settings::SettingsLayer; use fabro_util::terminal::Styles; use tracing::debug; @@ -28,6 +30,8 @@ pub(crate) async fn run( args_layer: SettingsLayer::default(), args: None, run_id: None, + user_layer: load_settings_user()?, + user_settings_path: Some(active_settings_path(None)), })?; let client = ctx.server().await?; let preflight = client.run_preflight(built.manifest.clone()).await?; diff --git a/lib/crates/fabro-cli/src/commands/preflight.rs b/lib/crates/fabro-cli/src/commands/preflight.rs index 3216d427e..045c08cbb 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -1,4 +1,6 @@ use anyhow::bail; +use fabro_config::load::load_settings_user; +use fabro_config::user::active_settings_path; use fabro_types::settings::cli::OutputVerbosity; use fabro_util::terminal::Styles; @@ -22,6 +24,8 @@ pub(crate) async fn execute(mut args: PreflightArgs, globals: &GlobalArgs) -> an args_layer: preflight_args_layer(&args)?, args: preflight_manifest_args(&args), run_id: None, + user_layer: load_settings_user()?, + user_settings_path: Some(active_settings_path(None)), })?; let client = ctx.server().await?; let response = client.run_preflight(manifest.manifest).await?; diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index 614385c89..f7a831937 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -3,6 +3,8 @@ use std::path::PathBuf; use crate::args::RunArgs; use crate::command_context::CommandContext; use fabro_config::Storage; +use fabro_config::load::load_settings_user; +use fabro_config::user::active_settings_path; use fabro_types::RunId; use fabro_types::settings::SettingsLayer; use fabro_util::terminal::Styles; @@ -46,6 +48,8 @@ pub(crate) async fn create_run( args_layer: cli_args_config, args: run_manifest_args(args), run_id, + user_layer: load_settings_user()?, + user_settings_path: Some(active_settings_path(None)), })?; let target = user_config::resolve_server_target(&args.target, ctx.machine_settings())?; let client = ctx.server().await?; diff --git a/lib/crates/fabro-cli/src/commands/validate.rs b/lib/crates/fabro-cli/src/commands/validate.rs index 06353d1f8..f35666cfb 100644 --- a/lib/crates/fabro-cli/src/commands/validate.rs +++ b/lib/crates/fabro-cli/src/commands/validate.rs @@ -1,4 +1,6 @@ use anyhow::bail; +use fabro_config::load::load_settings_user; +use fabro_config::user::active_settings_path; use fabro_types::settings::SettingsLayer; use fabro_util::terminal::Styles; @@ -20,6 +22,8 @@ pub(crate) async fn run( args_layer: SettingsLayer::default(), args: None, run_id: None, + user_layer: load_settings_user()?, + user_settings_path: Some(active_settings_path(None)), })?; let client = ctx.server().await?; let response = client.run_preflight(built.manifest).await?; diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 04eed6fcb..632397e74 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -3,11 +3,10 @@ use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result, anyhow}; use fabro_api::types; -use fabro_config::load::{load_settings_for_workflow, load_settings_user}; +use fabro_config::load::load_settings_for_workflow; use fabro_config::merge::combine_files; use fabro_config::project::{self, discover_project_config, resolve_workflow_path}; use fabro_config::run::{parse_run_config, resolve_run_goal}; -use fabro_config::user::active_settings_path; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; use fabro_sandbox::daytona::detect_repo_info; @@ -25,6 +24,12 @@ pub(crate) struct ManifestBuildInput { pub args_layer: SettingsLayer, pub args: Option, pub run_id: Option, + /// User-level settings layer. Production callers load via + /// `load_settings_user()`; tests pass `SettingsLayer::default()`. + pub user_layer: SettingsLayer, + /// Path to the user settings file (for inclusion in + /// `RunManifest.configs`). `None` skips the user config entry. + pub user_settings_path: Option, } #[derive(Debug)] @@ -47,10 +52,9 @@ struct WorkflowScanInput { } pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result { - let user_layer = load_settings_user()?; let workflow_layer = load_settings_for_workflow(&input.workflow, &input.cwd)?; let merged_settings = combine_files( - combine_files(user_layer, workflow_layer), + combine_files(input.user_layer, workflow_layer), input.args_layer.clone(), ); @@ -87,9 +91,7 @@ pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result