From 3da5b248e7c005ab725669f03beb6a001210e23c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 27 Mar 2026 15:02:27 -0400 Subject: [PATCH] Fix workflow-relative config resolution --- .../fabro-cli/src/commands/config/mod.rs | 35 ++-- .../fabro-cli/src/commands/run/execute.rs | 10 +- lib/crates/fabro-cli/tests/cli.rs | 160 ++++++++++++++++++ lib/crates/fabro-config/src/config.rs | 4 +- 4 files changed, 190 insertions(+), 19 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/config/mod.rs b/lib/crates/fabro-cli/src/commands/config/mod.rs index b997b8c18..6e2dec416 100644 --- a/lib/crates/fabro-cli/src/commands/config/mod.rs +++ b/lib/crates/fabro-cli/src/commands/config/mod.rs @@ -3,7 +3,7 @@ use std::path::Path; use crate::args::{ConfigCommand, ConfigNamespace, ConfigShowArgs}; use anyhow::bail; -use fabro_config::FabroSettings; +use fabro_config::{FabroConfig, FabroSettings}; pub fn dispatch(ns: ConfigNamespace) -> anyhow::Result<()> { match ns.command { @@ -12,27 +12,34 @@ pub fn dispatch(ns: ConfigNamespace) -> anyhow::Result<()> { } fn merged_config(workflow: Option<&Path>) -> anyhow::Result { - let mut config = fabro_config::cli::load_cli_config(None)?; - let cwd = std::env::current_dir()?; - - if let Some((_config_path, project_config)) = - fabro_config::project::discover_project_config(&cwd)? - { - config = config.combine(project_config); - } - if let Some(workflow) = workflow { let (resolved_path, _dot_path, run_config) = crate::commands::run::execute::resolve_workflow_source(workflow)?; + let missing_workflow = run_config.is_none() && !resolved_path.is_file(); + let project_config = fabro_config::project::discover_project_config( + resolved_path.parent().unwrap_or_else(|| Path::new(".")), + )? + .map(|(_, config)| config) + .unwrap_or_default(); + let cli_config = fabro_config::cli::load_cli_config(None)?; + let config = run_config + .unwrap_or_default() + .combine(project_config) + .combine(cli_config); - if let Some(run_config) = run_config { - config = config.combine(run_config); - } else if !resolved_path.is_file() { + if missing_workflow { bail!("Workflow not found: {}", resolved_path.display()); } + + return config.try_into(); } - config.try_into() + let cwd = std::env::current_dir()?; + let project_config = fabro_config::project::discover_project_config(&cwd)? + .map(|(_, config)| config) + .unwrap_or_default(); + let cli_config = fabro_config::cli::load_cli_config(None)?; + FabroConfig::combine(project_config, cli_config).try_into() } pub fn show_command(args: &ConfigShowArgs) -> anyhow::Result<()> { diff --git a/lib/crates/fabro-cli/src/commands/run/execute.rs b/lib/crates/fabro-cli/src/commands/run/execute.rs index 337af1cc8..af503040d 100644 --- a/lib/crates/fabro-cli/src/commands/run/execute.rs +++ b/lib/crates/fabro-cli/src/commands/run/execute.rs @@ -536,14 +536,18 @@ pub(crate) fn load_workflow_source_input( cli_defaults: FabroConfig, apply_project_config: bool, ) -> anyhow::Result { + let (resolved_workflow_path, dot_path, workflow_config) = resolve_workflow_source(workflow)?; let project_config = if apply_project_config { - project_config::discover_project_config(&std::env::current_dir().unwrap_or_default())? - .map(|(_, config)| config) + project_config::discover_project_config( + resolved_workflow_path + .parent() + .unwrap_or_else(|| Path::new(".")), + )? + .map(|(_, config)| config) } else { None }; - let (resolved_workflow_path, dot_path, workflow_config) = resolve_workflow_source(workflow)?; let config = cli_args_config .combine(workflow_config.unwrap_or_default()) .combine(project_config.unwrap_or_default()) diff --git a/lib/crates/fabro-cli/tests/cli.rs b/lib/crates/fabro-cli/tests/cli.rs index dc1d98a95..cef8d1e1b 100644 --- a/lib/crates/fabro-cli/tests/cli.rs +++ b/lib/crates/fabro-cli/tests/cli.rs @@ -137,6 +137,73 @@ SHARED = "run" (home, project) } +fn setup_external_workflow_fixture() -> (tempfile::TempDir, tempfile::TempDir, std::path::PathBuf) { + let home = tempfile::tempdir().unwrap(); + let project = tempfile::tempdir().unwrap(); + let storage_dir = home.path().join("fabro-data"); + + let home_fabro = home.path().join(".fabro"); + std::fs::create_dir_all(&home_fabro).unwrap(); + std::fs::write( + home_fabro.join("cli.toml"), + format!( + r#" +storage_dir = "{}" +auto_approve = true + +[setup] +commands = ["cli-setup"] +"#, + storage_dir.display() + ), + ) + .unwrap(); + + std::fs::write( + project.path().join("fabro.toml"), + r#" +version = 1 + +[setup] +commands = ["project-setup"] + +[sandbox] +preserve = true +"#, + ) + .unwrap(); + + std::fs::write( + project.path().join("workflow.fabro"), + r#" +digraph Test { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + start -> exit +} +"#, + ) + .unwrap(); + + std::fs::write( + project.path().join("workflow.toml"), + r#" +version = 1 +goal = "Ship it" +graph = "workflow.fabro" + +[llm] +model = "claude-sonnet-4-6" + +[setup] +commands = ["workflow-setup"] +"#, + ) + .unwrap(); + + (home, project, storage_dir) +} + // == LLM: prompt ============================================================== #[test] @@ -1496,6 +1563,99 @@ fn config_show_workflow_name_applies_run_overlay_and_deep_merges() { assert_eq!(env.get("SHARED").map(String::as_str), Some("run")); } +#[test] +fn config_show_explicit_workflow_path_uses_workflow_project_layers() { + let (home, project, _storage_dir) = setup_external_workflow_fixture(); + let cwd = tempfile::tempdir().unwrap(); + let workflow = project.path().join("workflow.toml"); + + let output = arc() + .env("HOME", home.path()) + .current_dir(cwd.path()) + .args(["config", "show", workflow.to_str().unwrap()]) + .assert() + .success() + .get_output() + .stdout + .clone(); + + let cfg = parse_config_show(&output); + assert_eq!(cfg.auto_approve, Some(true)); + assert_eq!( + cfg.setup.as_ref().expect("setup config").commands, + vec![ + "workflow-setup".to_string(), + "project-setup".to_string(), + "cli-setup".to_string(), + ] + ); + assert_eq!( + cfg.sandbox.as_ref().expect("sandbox config").preserve, + Some(true) + ); +} + +#[test] +fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { + let (home, project, storage_dir) = setup_external_workflow_fixture(); + let cwd = tempfile::tempdir().unwrap(); + let workflow = project.path().join("workflow.toml"); + let run_id = "external-config-run"; + + arc() + .env("HOME", home.path()) + .current_dir(cwd.path()) + .args([ + "create", + "--dry-run", + "--model", + "gpt-5.2", + "--run-id", + run_id, + workflow.to_str().unwrap(), + ]) + .assert() + .success(); + + let runs_dir = storage_dir.join("runs"); + let run_dir = std::fs::read_dir(&runs_dir) + .unwrap() + .flatten() + .map(|entry| entry.path()) + .find(|path| { + path.is_dir() + && path + .file_name() + .is_some_and(|name| name.to_string_lossy().ends_with(run_id)) + }) + .unwrap_or_else(|| { + panic!( + "expected run directory for {run_id} under {}", + runs_dir.display() + ) + }); + + let run_record: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(run_dir.join("run.json")).unwrap()).unwrap(); + assert_eq!(run_record["config"]["auto_approve"].as_bool(), Some(true)); + assert_eq!( + run_record["config"]["storage_dir"].as_str(), + Some(storage_dir.to_str().unwrap()) + ); + assert_eq!( + run_record["config"]["sandbox"]["preserve"].as_bool(), + Some(true) + ); + assert_eq!( + run_record["config"]["llm"]["model"].as_str(), + Some("gpt-5.2") + ); + assert_eq!( + run_record["config"]["setup"]["commands"], + serde_json::json!(["workflow-setup", "project-setup", "cli-setup"]) + ); +} + #[test] fn config_show_fabro_path_matches_ambient_defaults() { let (home, project) = setup_config_show_fixture(); diff --git a/lib/crates/fabro-config/src/config.rs b/lib/crates/fabro-config/src/config.rs index eccc771ad..40b796671 100644 --- a/lib/crates/fabro-config/src/config.rs +++ b/lib/crates/fabro-config/src/config.rs @@ -139,8 +139,8 @@ impl Combine for FabroConfig { } else if other.hooks.is_empty() { self.hooks } else { - HookConfig { hooks: self.hooks } - .merge(HookConfig { hooks: other.hooks }) + HookConfig { hooks: other.hooks } + .merge(HookConfig { hooks: self.hooks }) .hooks };