From e20d8d9435f1e365355b8f132cd5b19a831e8734 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 14:38:54 -0400 Subject: [PATCH] route project namespace resolution through workflow builders --- lib/crates/fabro-config/src/builders.rs | 31 +++++++++++++- lib/crates/fabro-config/src/project.rs | 41 ++++++++----------- lib/crates/fabro-config/tests/defaults.rs | 33 +++++++-------- .../fabro-config/tests/resolve_project.rs | 10 +++-- lib/crates/fabro-config/tests/resolve_root.rs | 31 ++++++++------ lib/crates/fabro-config/tests/resolve_run.rs | 11 +++-- .../fabro-config/tests/resolve_workflow.rs | 10 +++-- 7 files changed, 105 insertions(+), 62 deletions(-) diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index 9643055f1..b64b8480e 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -1,7 +1,9 @@ use std::fmt; use std::path::Path; -use fabro_types::settings::{CliLayer, Combine, RunLayer, SettingsLayer}; +use fabro_types::settings::{ + CliLayer, Combine, ProjectNamespace, RunLayer, RunNamespace, SettingsLayer, WorkflowNamespace, +}; use fabro_types::{ServerSettings, UserSettings, WorkflowSettings}; use crate::load::load_settings_path; @@ -224,6 +226,33 @@ impl WorkflowSettingsBuilder { errors, ) } + + pub(crate) fn project_from_layer( + layer: &SettingsLayer, + ) -> std::result::Result { + let layer = apply_builtin_defaults(layer.clone()); + let mut errors = Vec::new(); + let project = resolve_project(&layer.project.clone().unwrap_or_default(), &mut errors); + finish_dense_result(project, errors) + } + + pub(crate) fn workflow_from_layer( + layer: &SettingsLayer, + ) -> std::result::Result { + let layer = apply_builtin_defaults(layer.clone()); + let mut errors = Vec::new(); + let workflow = resolve_workflow(&layer.workflow.clone().unwrap_or_default(), &mut errors); + finish_dense_result(workflow, errors) + } + + pub(crate) fn run_from_layer( + layer: &SettingsLayer, + ) -> std::result::Result { + let layer = apply_builtin_defaults(layer.clone()); + let mut errors = Vec::new(); + let run = resolve_run(&layer.run.clone().unwrap_or_default(), &mut errors); + finish_dense_result(run, errors) + } } fn finish_result(value: T, context: &'static str, errors: Vec) -> Result { diff --git a/lib/crates/fabro-config/src/project.rs b/lib/crates/fabro-config/src/project.rs index d62584669..8ed58a4a8 100644 --- a/lib/crates/fabro-config/src/project.rs +++ b/lib/crates/fabro-config/src/project.rs @@ -16,11 +16,7 @@ use fabro_types::settings::SettingsLayer; use serde::Serialize; use crate::load::load_settings_path; -use crate::parse::parse_settings_layer; -use crate::{ - Error, Result, resolve_project_from_file, resolve_run_from_file, resolve_workflow_from_file, - run, -}; +use crate::{Error, Result, WorkflowSettingsBuilder, run}; const CONFIG_FILENAME: &str = ".fabro/project.toml"; #[derive(Clone, Debug)] @@ -32,19 +28,14 @@ pub struct WorkflowPathResolution { pub workflow_slug: Option, } -/// Parse a project config from a TOML string. -pub fn parse_project_config(content: &str) -> Result { - parse_settings_layer(content).map_err(|err| Error::parse("Failed to parse project config", err)) -} - /// Load a project config from a file path. /// /// Goes through [`load_settings_path`] so that relative `run.goal.file` /// paths are anchored at the directory of `path` at load time. pub fn load_project_config(path: &Path) -> Result { let config = load_settings_path(path)?; - let root = resolve_project_from_file(&config) - .map_err(|errors| Error::resolve("Failed to resolve project settings", errors))? + let root = WorkflowSettingsBuilder::project_from_layer(&config) + .map_err(|errors| Error::resolve("Failed to resolve project settings", errors.into()))? .directory; tracing::debug!(path = %path.display(), root = %root, "Loaded project config"); Ok(config) @@ -94,9 +85,10 @@ pub fn resolve_workflow_path(workflow_path: &Path, cwd: &Path) -> Result { - let workflow = resolve_workflow_from_file(&cfg).map_err(|errors| { - Error::resolve("Failed to resolve workflow settings", errors) - })?; + let workflow = + WorkflowSettingsBuilder::workflow_from_layer(&cfg).map_err(|errors| { + Error::resolve("Failed to resolve workflow settings", errors.into()) + })?; let dot_path = run::resolve_graph_path(&path, &workflow.graph); Ok(WorkflowPathResolution { resolved_workflow_path: path.clone(), @@ -121,7 +113,7 @@ pub fn resolve_workflow_path(workflow_path: &Path, cwd: &Path) -> Result PathBuf { - let Some(work_dir) = resolve_run_from_file(settings) + let Some(work_dir) = WorkflowSettingsBuilder::run_from_layer(settings) .ok() .and_then(|settings| settings.working_dir) .map(|value| value.as_source()) @@ -392,7 +384,7 @@ pub fn resolve_fabro_root(config_path: &Path, config: &SettingsLayer) -> PathBuf let project_dir = config_path .parent() .expect("config_path should have a parent directory"); - let root = resolve_project_from_file(config) + let root = WorkflowSettingsBuilder::project_from_layer(config) .expect("project settings should resolve") .directory; normalize_joined_path(project_dir, Path::new(&root)) @@ -408,14 +400,14 @@ mod tests { #[test] fn parse_minimal_config() { - let config = parse_project_config("_version = 1\n").unwrap(); + let config = crate::parse_settings_layer("_version = 1\n").unwrap(); assert_eq!(config.version, Some(1)); assert!(config.project.is_none()); } #[test] fn parse_with_project_directory() { - let config = parse_project_config( + let config = crate::parse_settings_layer( r#" _version = 1 @@ -425,14 +417,16 @@ directory = "custom/" ) .unwrap(); assert_eq!( - resolve_project_from_file(&config).unwrap().directory, + WorkflowSettingsBuilder::project_from_layer(&config) + .unwrap() + .directory, "custom/" ); } #[test] fn parse_with_run_execution_retros() { - let config = parse_project_config( + let config = crate::parse_settings_layer( " _version = 1 @@ -453,7 +447,8 @@ retros = true #[test] fn parse_rejects_legacy_llm_section() { - let err = parse_project_config("_version = 1\n[llm]\nprovider = \"openai\"\n").unwrap_err(); + let err = crate::parse_settings_layer("_version = 1\n[llm]\nprovider = \"openai\"\n") + .unwrap_err(); let text = format!("{err:#}"); assert!( text.contains("run.model") || text.contains("llm"), @@ -463,7 +458,7 @@ retros = true #[test] fn parse_higher_version_errors() { - let err = parse_project_config("_version = 2\n").unwrap_err(); + let err = crate::parse_settings_layer("_version = 2\n").unwrap_err(); let chain = format!("{err:#}"); assert!( chain.contains("Upgrade") || chain.to_lowercase().contains("version"), diff --git a/lib/crates/fabro-config/tests/defaults.rs b/lib/crates/fabro-config/tests/defaults.rs index e72b97ce1..4e2278790 100644 --- a/lib/crates/fabro-config/tests/defaults.rs +++ b/lib/crates/fabro-config/tests/defaults.rs @@ -1,7 +1,4 @@ -use fabro_config::{ - parse_settings_layer, resolve_run_from_file, resolve_server_from_file, - resolve_workflow_from_file, -}; +use fabro_config::{ServerSettingsBuilder, WorkflowSettingsBuilder, parse_settings_layer}; use fabro_types::settings::cli::OutputFormat; use fabro_types::settings::run::{ApprovalMode, RunMode, WorktreeMode}; use fabro_types::settings::server::ObjectStoreProvider; @@ -98,15 +95,19 @@ fn apply_builtin_defaults_materializes_expected_layer() { #[test] fn resolve_empty_settings_requires_explicit_server_auth_methods() { - let errors = resolve_server_from_file(&SettingsLayer::default()) + let errors = ServerSettingsBuilder::from_layer(&SettingsLayer::default()) .expect_err("empty server settings should fail"); - assert!(errors.iter().any(|error| { - matches!( - error, - fabro_config::ResolveError::Missing { path } if path == "server.auth.methods" - ) - })); + assert!(matches!( + errors, + fabro_config::Error::Resolve { errors, .. } + if errors.iter().any(|error| { + matches!( + error, + fabro_config::ResolveError::Missing { path } if path == "server.auth.methods" + ) + }) + )); } #[test] @@ -123,10 +124,10 @@ mode = "dry_run" "#, ); - let workflow = resolve_workflow_from_file(&layer).expect("workflow settings should resolve"); - let run = resolve_run_from_file(&layer).expect("run settings should resolve"); + let settings = + WorkflowSettingsBuilder::from_layer(&layer).expect("workflow settings should resolve"); - assert_eq!(run.execution.mode, RunMode::DryRun); - assert_eq!(run.execution.approval, ApprovalMode::Prompt); - assert_eq!(workflow.graph, "workflow.fabro"); + assert_eq!(settings.run.execution.mode, RunMode::DryRun); + assert_eq!(settings.run.execution.approval, ApprovalMode::Prompt); + assert_eq!(settings.workflow.graph, "workflow.fabro"); } diff --git a/lib/crates/fabro-config/tests/resolve_project.rs b/lib/crates/fabro-config/tests/resolve_project.rs index 345cf9f53..925485135 100644 --- a/lib/crates/fabro-config/tests/resolve_project.rs +++ b/lib/crates/fabro-config/tests/resolve_project.rs @@ -1,11 +1,13 @@ -use fabro_config::{parse_settings_layer, resolve_project_from_file}; +use fabro_config::{WorkflowSettingsBuilder, parse_settings_layer}; use fabro_types::settings::SettingsLayer; #[test] fn resolves_project_defaults_from_empty_settings() { let settings = SettingsLayer::default(); - let project = resolve_project_from_file(&settings).expect("empty settings should resolve"); + let project = WorkflowSettingsBuilder::from_layer(&settings) + .expect("empty settings should resolve") + .project; assert_eq!(project.directory, "."); assert!(project.name.is_none()); @@ -30,7 +32,9 @@ team = "platform" ) .expect("fixture should parse"); - let project = resolve_project_from_file(&settings).expect("project settings should resolve"); + let project = WorkflowSettingsBuilder::from_layer(&settings) + .expect("project settings should resolve") + .project; assert_eq!(project.name.as_deref(), Some("Acme")); assert_eq!(project.description.as_deref(), Some("Automation")); diff --git a/lib/crates/fabro-config/tests/resolve_root.rs b/lib/crates/fabro-config/tests/resolve_root.rs index 4c48315a1..56e33a0bc 100644 --- a/lib/crates/fabro-config/tests/resolve_root.rs +++ b/lib/crates/fabro-config/tests/resolve_root.rs @@ -1,4 +1,4 @@ -use fabro_config::parse_settings_layer; +use fabro_config::{ServerSettingsBuilder, WorkflowSettingsBuilder, parse_settings_layer}; use fabro_types::settings::run::RunMode; use fabro_types::settings::{InterpString, SettingsLayer}; @@ -83,23 +83,30 @@ name = "gpt-5" "#, ); - let project = fabro_config::resolve_project_from_file(&settings) - .expect("project settings should resolve"); - let workflow = fabro_config::resolve_workflow_from_file(&settings) - .expect("workflow settings should resolve"); + let workflow_settings = + WorkflowSettingsBuilder::from_layer(&settings).expect("workflow settings should resolve"); let server = - fabro_config::resolve_server_from_file(&settings).expect("server settings should resolve"); - let run = fabro_config::resolve_run_from_file(&settings).expect("run settings should resolve"); + ServerSettingsBuilder::from_layer(&settings).expect("server settings should resolve"); - assert_eq!(project.directory, ".fabro"); - assert_eq!(workflow.graph, "graphs/workflow.dot"); - assert_eq!(server.storage.root.as_source(), "/srv/fabro"); + assert_eq!(workflow_settings.project.directory, ".fabro"); + assert_eq!(workflow_settings.workflow.graph, "graphs/workflow.dot"); + assert_eq!(server.server.storage.root.as_source(), "/srv/fabro"); assert_eq!( - run.model.provider.as_ref().map(InterpString::as_source), + workflow_settings + .run + .model + .provider + .as_ref() + .map(InterpString::as_source), Some("openai".to_string()) ); assert_eq!( - run.model.name.as_ref().map(InterpString::as_source), + workflow_settings + .run + .model + .name + .as_ref() + .map(InterpString::as_source), Some("gpt-5".to_string()) ); } diff --git a/lib/crates/fabro-config/tests/resolve_run.rs b/lib/crates/fabro-config/tests/resolve_run.rs index 281916ea1..70bbfd521 100644 --- a/lib/crates/fabro-config/tests/resolve_run.rs +++ b/lib/crates/fabro-config/tests/resolve_run.rs @@ -1,4 +1,4 @@ -use fabro_config::parse_settings_layer; +use fabro_config::{WorkflowSettingsBuilder, parse_settings_layer}; use fabro_types::settings::run::{ApprovalMode, RunGoal, RunMode, WorktreeMode}; use fabro_types::settings::{InterpString, SettingsLayer}; @@ -8,8 +8,9 @@ fn parse(source: &str) -> SettingsLayer { #[test] fn resolves_run_defaults_from_empty_settings() { - let settings = fabro_config::resolve_run_from_file(&SettingsLayer::default()) - .expect("empty settings should resolve"); + let settings = WorkflowSettingsBuilder::from_layer(&SettingsLayer::default()) + .expect("empty settings should resolve") + .run; assert_eq!(settings.execution.mode, RunMode::Normal); assert_eq!(settings.execution.approval, ApprovalMode::Prompt); @@ -38,7 +39,9 @@ name = "sonnet" "#, ); - let settings = fabro_config::resolve_run_from_file(&file).expect("run settings should resolve"); + let settings = WorkflowSettingsBuilder::from_layer(&file) + .expect("run settings should resolve") + .run; match settings.goal { Some(RunGoal::File(path)) => { diff --git a/lib/crates/fabro-config/tests/resolve_workflow.rs b/lib/crates/fabro-config/tests/resolve_workflow.rs index c5746caab..154f0dce2 100644 --- a/lib/crates/fabro-config/tests/resolve_workflow.rs +++ b/lib/crates/fabro-config/tests/resolve_workflow.rs @@ -1,11 +1,13 @@ -use fabro_config::{parse_settings_layer, resolve_workflow_from_file}; +use fabro_config::{WorkflowSettingsBuilder, parse_settings_layer}; use fabro_types::settings::SettingsLayer; #[test] fn resolves_workflow_defaults_from_empty_settings() { let settings = SettingsLayer::default(); - let workflow = resolve_workflow_from_file(&settings).expect("empty settings should resolve"); + let workflow = WorkflowSettingsBuilder::from_layer(&settings) + .expect("empty settings should resolve") + .workflow; assert_eq!(workflow.graph, "workflow.fabro"); assert!(workflow.name.is_none()); @@ -30,7 +32,9 @@ tier = "gold" ) .expect("fixture should parse"); - let workflow = resolve_workflow_from_file(&settings).expect("workflow settings should resolve"); + let workflow = WorkflowSettingsBuilder::from_layer(&settings) + .expect("workflow settings should resolve") + .workflow; assert_eq!(workflow.name.as_deref(), Some("Ship")); assert_eq!(workflow.description.as_deref(), Some("Primary flow"));