From 40f53873e49f932a6cae0b5d8cd74e9f39568ecf Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 17 Mar 2026 15:33:04 -0400 Subject: [PATCH] Deduplicate config loading and run-config merging - Extract generic load_config_file(path, filename) helper, reducing load_cli_config and load_server_config to one-liners. - Rewrite WorkflowRunConfig::apply_defaults to delegate to RunDefaults::merge_overlay, eliminating ~90 lines of duplicate deep-merge logic. Both methods now share the same code path. - Fix bug where RunDefaults::merge_overlay silently dropped the ssh sandbox config from overlays (the ssh field was never merged). Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-config/src/cli.rs | 19 +-- lib/crates/fabro-config/src/lib.rs | 29 +++++ lib/crates/fabro-config/src/run.rs | 169 +++++--------------------- lib/crates/fabro-config/src/server.rs | 19 +-- 4 files changed, 63 insertions(+), 173 deletions(-) diff --git a/lib/crates/fabro-config/src/cli.rs b/lib/crates/fabro-config/src/cli.rs index 95af2cb7f..4c58f1c59 100644 --- a/lib/crates/fabro-config/src/cli.rs +++ b/lib/crates/fabro-config/src/cli.rs @@ -1,7 +1,6 @@ use std::path::{Path, PathBuf}; use serde::Deserialize; -use tracing::debug; use crate::run::RunDefaults; @@ -119,23 +118,7 @@ impl CliConfig { /// Load CLI config from an explicit path or `~/.fabro/cli.toml`, returning defaults if the /// default file doesn't exist. An explicit path that doesn't exist is an error. pub fn load_cli_config(path: Option<&Path>) -> anyhow::Result { - if let Some(explicit) = path { - debug!(path = %explicit.display(), "Loading CLI config from explicit path"); - let contents = std::fs::read_to_string(explicit)?; - return Ok(toml::from_str(&contents)?); - } - - let Some(home) = dirs::home_dir() else { - debug!("No home directory found, using default CLI config"); - return Ok(CliConfig::default()); - }; - let default_path = home.join(".fabro").join("cli.toml"); - debug!(path = %default_path.display(), "Loading CLI config"); - match std::fs::read_to_string(&default_path) { - Ok(contents) => Ok(toml::from_str(&contents)?), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(CliConfig::default()), - Err(e) => Err(e.into()), - } + crate::load_config_file(path, "cli.toml") } #[cfg(test)] diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 41a13815e..b5b338bd4 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -7,3 +7,32 @@ pub mod sandbox; pub mod server; pub use fabro_util::path::expand_tilde; + +use std::path::Path; + +/// Load a TOML config from an explicit path or `~/.fabro/{filename}`. +/// +/// Returns `T::default()` when no explicit path is given and the default file +/// doesn't exist. An explicit path that doesn't exist is an error. +pub fn load_config_file(path: Option<&Path>, filename: &str) -> anyhow::Result +where + T: Default + serde::de::DeserializeOwned, +{ + if let Some(explicit) = path { + tracing::debug!(path = %explicit.display(), "Loading config from explicit path"); + let contents = std::fs::read_to_string(explicit)?; + return Ok(toml::from_str(&contents)?); + } + + let Some(home) = dirs::home_dir() else { + tracing::debug!("No home directory found, using default config"); + return Ok(T::default()); + }; + let default_path = home.join(".fabro").join(filename); + tracing::debug!(path = %default_path.display(), "Loading config"); + match std::fs::read_to_string(&default_path) { + Ok(contents) => Ok(toml::from_str(&contents)?), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(T::default()), + Err(e) => Err(e.into()), + } +} diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 672df8198..3b4b33543 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -125,142 +125,36 @@ impl WorkflowRunConfig { /// Each field uses the first non-`None` value (task config wins). /// Vars are merged: defaults first, then task config overwrites. pub fn apply_defaults(&mut self, defaults: &RunDefaults) { - if self.work_dir.is_none() { - self.work_dir = defaults.work_dir.clone(); - } + // Build a RunDefaults from the task's current fields (the "overlay"), + // then merge it on top of the server defaults (the "base"). + // This gives "task wins" semantics via merge_overlay's "overlay wins". + let task_overlay = RunDefaults { + work_dir: self.work_dir.take(), + llm: self.llm.take(), + setup: self.setup.take(), + sandbox: self.sandbox.take(), + vars: self.vars.take(), + checkpoint: std::mem::take(&mut self.checkpoint), + pull_request: self.pull_request.take(), + assets: self.assets.take(), + hooks: std::mem::take(&mut self.hooks), + mcp_servers: std::mem::take(&mut self.mcp_servers), + github: self.github.take(), + }; + let mut merged = defaults.clone(); + merged.merge_overlay(task_overlay); - match (&mut self.llm, &defaults.llm) { - (Some(task), Some(default)) => { - if task.model.is_none() { - task.model = default.model.clone(); - } - if task.provider.is_none() { - task.provider = default.provider.clone(); - } - if task.fallbacks.is_none() { - task.fallbacks = default.fallbacks.clone(); - } - } - (None, Some(_)) => self.llm = defaults.llm.clone(), - _ => {} - } - - match (&mut self.setup, &defaults.setup) { - (Some(task), Some(default)) => { - if task.timeout_ms.is_none() { - task.timeout_ms = default.timeout_ms; - } - } - (None, Some(_)) => self.setup = defaults.setup.clone(), - _ => {} - } - - match (&mut self.sandbox, &defaults.sandbox) { - (Some(task), Some(default)) => { - if task.provider.is_none() { - task.provider = default.provider.clone(); - } - if task.preserve.is_none() { - task.preserve = default.preserve; - } - if task.devcontainer.is_none() { - task.devcontainer = default.devcontainer; - } - if task.local.is_none() { - task.local = default.local.clone(); - } - match (&mut task.daytona, &default.daytona) { - (Some(task_d), Some(default_d)) => { - if task_d.auto_stop_interval.is_none() { - task_d.auto_stop_interval = default_d.auto_stop_interval; - } - if task_d.snapshot.is_none() { - task_d.snapshot = default_d.snapshot.clone(); - } - if let Some(ref default_labels) = default_d.labels { - let mut merged = default_labels.clone(); - if let Some(ref task_labels) = task_d.labels { - merged.extend(task_labels.clone()); - } - task_d.labels = Some(merged); - } - if task_d.network.is_none() { - task_d.network = default_d.network.clone(); - } - } - (None, Some(_)) => task.daytona = default.daytona.clone(), - _ => {} - } - #[cfg(feature = "exedev")] - match (&mut task.exe, &default.exe) { - (Some(task_e), Some(default_e)) => { - if task_e.image.is_none() { - task_e.image = default_e.image.clone(); - } - } - (None, Some(_)) => task.exe = default.exe.clone(), - _ => {} - } - if task.ssh.is_none() { - task.ssh = default.ssh.clone(); - } - if let Some(ref default_env) = default.env { - let mut merged = default_env.clone(); - if let Some(ref task_env) = task.env { - merged.extend(task_env.clone()); - } - task.env = Some(merged); - } - } - (None, Some(_)) => self.sandbox = defaults.sandbox.clone(), - _ => {} - } - - if let Some(ref default_vars) = defaults.vars { - let mut merged = default_vars.clone(); - if let Some(ref task_vars) = self.vars { - merged.extend(task_vars.clone()); - } - self.vars = Some(merged); - } - - if !defaults.checkpoint.exclude_globs.is_empty() { - let mut merged = defaults.checkpoint.exclude_globs.clone(); - merged.append(&mut self.checkpoint.exclude_globs.clone()); - merged.sort(); - merged.dedup(); - self.checkpoint.exclude_globs = merged; - } - - if self.pull_request.is_none() { - self.pull_request = defaults.pull_request.clone(); - } - - if self.assets.is_none() { - self.assets = defaults.assets.clone(); - } - - // Merge hooks: defaults as base, workflow overrides by name - if !defaults.hooks.is_empty() { - let base = HookConfig { - hooks: defaults.hooks.clone(), - }; - let overlay = HookConfig { - hooks: std::mem::take(&mut self.hooks), - }; - self.hooks = base.merge(overlay).hooks; - } - - // Merge mcp_servers: defaults as base, workflow overrides by key - if !defaults.mcp_servers.is_empty() { - let mut merged = defaults.mcp_servers.clone(); - merged.extend(std::mem::take(&mut self.mcp_servers)); - self.mcp_servers = merged; - } - - if self.github.is_none() { - self.github = defaults.github.clone(); - } + self.work_dir = merged.work_dir; + self.llm = merged.llm; + self.setup = merged.setup; + self.sandbox = merged.sandbox; + self.vars = merged.vars; + self.checkpoint = merged.checkpoint; + self.pull_request = merged.pull_request; + self.assets = merged.assets; + self.hooks = merged.hooks; + self.mcp_servers = merged.mcp_servers; + self.github = merged.github; } } @@ -268,8 +162,6 @@ impl RunDefaults { /// Merge an overlay on top of this base. The overlay takes precedence /// for simple fields; compound fields (vars, hooks, mcp_servers) are /// deep-merged with the overlay winning on collision. - /// - /// Uses the same deep-merge semantics as `WorkflowRunConfig::apply_defaults`. pub fn merge_overlay(&mut self, overlay: RunDefaults) { if overlay.work_dir.is_some() { self.work_dir = overlay.work_dir; @@ -345,6 +237,9 @@ impl RunDefaults { (None, Some(over_e)) => base.exe = Some(over_e), _ => {} } + if over.ssh.is_some() { + base.ssh = over.ssh; + } if let Some(over_env) = over.env { let mut merged = base.env.take().unwrap_or_default(); merged.extend(over_env); diff --git a/lib/crates/fabro-config/src/server.rs b/lib/crates/fabro-config/src/server.rs index 8bfd35b1e..029f814b2 100644 --- a/lib/crates/fabro-config/src/server.rs +++ b/lib/crates/fabro-config/src/server.rs @@ -1,7 +1,6 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; -use tracing::debug; use crate::run::RunDefaults; @@ -150,23 +149,7 @@ pub struct ServerConfig { /// Load server config from an explicit path or `~/.fabro/server.toml`, returning defaults if the /// default file doesn't exist. An explicit path that doesn't exist is an error. pub fn load_server_config(path: Option<&Path>) -> anyhow::Result { - if let Some(explicit) = path { - debug!(path = %explicit.display(), "Loading server config from explicit path"); - let contents = std::fs::read_to_string(explicit)?; - return Ok(toml::from_str(&contents)?); - } - - let Some(home) = dirs::home_dir() else { - debug!("No home directory found, using default server config"); - return Ok(ServerConfig::default()); - }; - let default_path = home.join(".fabro").join("server.toml"); - debug!(path = %default_path.display(), "Loading server config"); - match std::fs::read_to_string(&default_path) { - Ok(contents) => Ok(toml::from_str(&contents)?), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(ServerConfig::default()), - Err(e) => Err(e.into()), - } + crate::load_config_file(path, "server.toml") } /// Resolve the data directory: config value > default `~/.fabro`.