From fcf3152daf76081340312e2ce6977d654d7e2aa0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 20:41:00 -0400 Subject: [PATCH] fix(config): route project/workflow loaders through ConfigLayer::load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Stage 6 audit caught that `load_project_config` and `load_run_config` bypassed `ConfigLayer::load` and called `parse_project_config` / `ConfigLayer::parse` directly. As a result, `resolve_goal_file_paths` — which rewrites relative `[run.goal] file = "..."` paths to absolute against the declaring file's directory — only fired for `~/.fabro/settings.toml`, never for `fabro.toml` or `workflow.toml`. That meant a project author writing [run.goal] file = "prompts/goal.md" would have the relative path survive all the way to consume time and get resolved against the run's `working_directory` instead of the config-file directory, contradicting the agreed "config-file rooted" rule and breaking the most common case. Both loaders now delegate to `ConfigLayer::load(path)`, which performs the load-time rewrite. The user-settings path was already correct. ## Tests - `load_project_config_rewrites_relative_goal_file_path` - `load_run_config_rewrites_relative_goal_file_path` - `load_run_config_leaves_absolute_goal_file_untouched` - `build_manifest_resolves_relative_goal_file_in_project_config` — end-to-end via `build_run_manifest`, asserting the absolute path lands in `manifest.goal.path` and the file contents land in `manifest.goal.text`. - `build_manifest_resolves_relative_goal_file_in_workflow_config` — same shape but exercising `workflow.toml`-declared goal files, which resolve relative to the much deeper workflow directory rather than the project root. 3,787 workspace tests pass (was 3,782, +5 new). `cargo fmt --check --all` and `cargo clippy --workspace -- -D warnings` are clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/manifest_builder.rs | 128 +++++++++++++++++++ lib/crates/fabro-config/src/project.rs | 31 ++++- lib/crates/fabro-config/src/run.rs | 58 ++++++++- 3 files changed, 210 insertions(+), 7 deletions(-) diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 716e77023..d88eb2599 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -660,4 +660,132 @@ mod tests { .contains_key("fabro/workflows/child/workflow.fabro") ); } + + /// A relative `[run.goal] file = "..."` declared in `fabro.toml` must + /// resolve against the directory of `fabro.toml`, not against the + /// invocation cwd. We exercise this by invoking from a subdirectory + /// below the project root. + #[test] + #[allow(unsafe_code, clippy::allow_attributes)] + fn build_manifest_resolves_relative_goal_file_in_project_config() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join("fabro/workflows/demo"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + std::fs::create_dir_all(project.join("prompts")).unwrap(); + + std::fs::write( + project.join("fabro.toml"), + r#"_version = 1 + +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + std::fs::write(project.join("prompts/goal.md"), "ship from project root").unwrap(); + + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + + let sandboxed_settings = temp.path().join("empty-settings.toml"); + std::fs::write(&sandboxed_settings, "_version = 1\n").unwrap(); + // SAFETY: single-threaded unit test body. + unsafe { + std::env::set_var("FABRO_CONFIG", &sandboxed_settings); + } + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from("fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + args_layer: ConfigLayer::default(), + args: None, + run_id: None, + }) + .unwrap(); + + // SAFETY: single-threaded unit test body. + unsafe { + std::env::remove_var("FABRO_CONFIG"); + } + + let goal = built.manifest.goal.expect("manifest goal should be set"); + assert_eq!(goal.text, "ship from project root"); + assert_eq!(goal.type_, types::ManifestGoalType::File); + let resolved = goal.path.expect("file goal must carry a path"); + let expected = project.join("prompts").join("goal.md"); + assert_eq!(PathBuf::from(resolved), expected); + } + + /// A relative `[run.goal] file = "..."` declared in `workflow.toml` + /// must resolve against the directory of `workflow.toml`, not against + /// the invocation cwd or project root. + #[test] + #[allow(unsafe_code, clippy::allow_attributes)] + fn build_manifest_resolves_relative_goal_file_in_workflow_config() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join("fabro/workflows/demo"); + std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); + + std::fs::write(project.join("fabro.toml"), "_version = 1\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + r#"_version = 1 + +[workflow] +graph = "workflow.fabro" + +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("prompts/goal.md"), + "ship from workflow dir", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + + let sandboxed_settings = temp.path().join("empty-settings.toml"); + std::fs::write(&sandboxed_settings, "_version = 1\n").unwrap(); + // SAFETY: single-threaded unit test body. + unsafe { + std::env::set_var("FABRO_CONFIG", &sandboxed_settings); + } + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from("fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + args_layer: ConfigLayer::default(), + args: None, + run_id: None, + }) + .unwrap(); + + // SAFETY: single-threaded unit test body. + unsafe { + std::env::remove_var("FABRO_CONFIG"); + } + + let goal = built.manifest.goal.expect("manifest goal should be set"); + assert_eq!(goal.text, "ship from workflow dir"); + assert_eq!(goal.type_, types::ManifestGoalType::File); + let resolved = goal.path.expect("file goal must carry a path"); + let expected = workflow_dir.join("prompts").join("goal.md"); + assert_eq!(PathBuf::from(resolved), expected); + } } diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index eb7e569a6..2c49cf426 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -33,10 +33,11 @@ pub fn parse_project_config(content: &str) -> anyhow::Result { } /// Load a project config from a file path. +/// +/// Goes through [`ConfigLayer::load`] so that relative `run.goal.file` +/// paths are anchored at the directory of `path` at load time. pub fn load_project_config(path: &Path) -> anyhow::Result { - let content = std::fs::read_to_string(path) - .with_context(|| format!("Failed to read {}", path.display()))?; - let config = parse_project_config(&content)?; + let config = ConfigLayer::load(path).context("Failed to parse project config")?; let root = config .as_v2() .project @@ -477,4 +478,28 @@ retros = true assert_eq!(found_path, tmp.path().join("fabro.toml")); assert_eq!(config.as_v2().version, Some(1)); } + + #[test] + fn load_project_config_rewrites_relative_goal_file_path() { + use fabro_types::settings::run::RunGoalLayer; + + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("fabro.toml"); + fs::write( + &path, + r#"_version = 1 + +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + + let config = load_project_config(&path).unwrap(); + let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else { + panic!("expected file variant"); + }; + let expected = tmp.path().join("prompts").join("goal.md"); + assert_eq!(file.as_source(), expected.to_string_lossy()); + } } diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index a5c015173..33fac41c4 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -18,11 +18,10 @@ pub fn parse_run_config(contents: &str) -> anyhow::Result { /// Load and parse a run config from a TOML file. /// -/// Returns the v2-backed `ConfigLayer`. +/// Goes through [`ConfigLayer::load`] so that relative `run.goal.file` +/// paths are anchored at the directory of `path` at load time. pub fn load_run_config(path: &Path) -> anyhow::Result { - let content = std::fs::read_to_string(path) - .with_context(|| format!("Failed to read {}", path.display()))?; - ConfigLayer::parse(&content) + ConfigLayer::load(path) .with_context(|| format!("Failed to parse workflow config at {}", path.display())) } @@ -34,3 +33,54 @@ pub fn resolve_graph_path(workflow_toml: &Path, graph_relative: &str) -> PathBuf .unwrap_or_else(|| Path::new(".")) .join(graph_relative) } + +#[cfg(test)] +mod tests { + use super::*; + use fabro_types::settings::run::RunGoalLayer; + + #[test] + fn load_run_config_rewrites_relative_goal_file_path() { + let tmp = tempfile::tempdir().unwrap(); + let workflow_dir = tmp.path().join("fabro").join("workflows").join("demo"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + let workflow_toml = workflow_dir.join("workflow.toml"); + std::fs::write( + &workflow_toml, + r#"_version = 1 + +[run.goal] +file = "prompts/goal.md" +"#, + ) + .unwrap(); + + let config = load_run_config(&workflow_toml).unwrap(); + let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else { + panic!("expected file variant"); + }; + let expected = workflow_dir.join("prompts").join("goal.md"); + assert_eq!(file.as_source(), expected.to_string_lossy()); + } + + #[test] + fn load_run_config_leaves_absolute_goal_file_untouched() { + let tmp = tempfile::tempdir().unwrap(); + let workflow_toml = tmp.path().join("workflow.toml"); + std::fs::write( + &workflow_toml, + r#"_version = 1 + +[run.goal] +file = "/etc/fabro/goal.md" +"#, + ) + .unwrap(); + + let config = load_run_config(&workflow_toml).unwrap(); + let Some(RunGoalLayer::File { file }) = config.as_v2().run_goal_layer() else { + panic!("expected file variant"); + }; + assert_eq!(file.as_source(), "/etc/fabro/goal.md"); + } +}