From 9db57404918b260ab7de8f24b402f7b983ca6cca Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 6 May 2026 10:29:48 -0400 Subject: [PATCH] refactor: simplify CLI input override plumbing Apply review-driven cleanups on top of the input-overrides feature: extract a shared TemplateContext::for_input_scan helper for the "render inputs but defer goal" idiom (4 callers), inline the one-line apply_input_overrides wrapper, drop a redundant clone, and derive Default on ManifestBuildInput so call sites use ..Default::default() instead of spelling every field. Also collapse dead code in parse_input_value and tighten the test helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/commands/graph.rs | 11 +- .../fabro-cli/src/commands/preflight.rs | 14 +-- lib/crates/fabro-cli/src/commands/validate.rs | 12 +- lib/crates/fabro-cli/src/manifest_builder.rs | 107 +++++------------- .../tests/manifest_path_round_trip.rs | 12 +- .../fabro-config/src/input_overrides.rs | 95 +++++----------- lib/crates/fabro-config/src/lib.rs | 2 +- lib/crates/fabro-server/src/run_manifest.rs | 4 +- lib/crates/fabro-template/src/lib.rs | 8 ++ .../fabro-workflow/src/operations/create.rs | 9 +- .../fabro-workflow/src/transforms/import.rs | 4 +- .../src/transforms/variable_expansion.rs | 4 +- 12 files changed, 89 insertions(+), 193 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/graph.rs b/lib/crates/fabro-cli/src/commands/graph.rs index 99d6a8283..dbfae41fc 100644 --- a/lib/crates/fabro-cli/src/commands/graph.rs +++ b/lib/crates/fabro-cli/src/commands/graph.rs @@ -7,7 +7,6 @@ reason = "sync CLI `graph` command: blocking std::io::stdout is the intended output mechanism" )] -use std::collections::HashMap; use std::io::Write; use anyhow::{Context, bail}; @@ -34,14 +33,10 @@ pub(crate) async fn run( let printer = base_ctx.printer(); let ctx = base_ctx.with_target(&args.target)?; let built = build_run_manifest(ManifestBuildInput { - workflow: args.workflow.clone(), - cwd: ctx.cwd().to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, + workflow: args.workflow.clone(), + cwd: ctx.cwd().to_path_buf(), user_settings_path: Some(active_settings_path(None)), + ..Default::default() })?; 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 5c56e5cf3..a3d1dbdf6 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -22,14 +22,14 @@ pub(crate) async fn execute( let cli_args_config = preflight_args_overrides(&args)?; let manifest = build_run_manifest(ManifestBuildInput { - workflow: args.workflow.clone(), - cwd: ctx.cwd().to_path_buf(), - run_overrides: cli_args_config.run, - cli_overrides: cli_args_config.cli, - input_overrides: cli_args_config.input_overrides, - args: preflight_manifest_args(&args), - run_id: None, + workflow: args.workflow.clone(), + cwd: ctx.cwd().to_path_buf(), + run_overrides: cli_args_config.run, + cli_overrides: cli_args_config.cli, + input_overrides: cli_args_config.input_overrides, + args: preflight_manifest_args(&args), user_settings_path: Some(active_settings_path(None)), + ..Default::default() })?; let spinner = (!ctx.json_output()).then(|| cyan_spinner("Running checks...")); diff --git a/lib/crates/fabro-cli/src/commands/validate.rs b/lib/crates/fabro-cli/src/commands/validate.rs index 48eee409b..74495fadf 100644 --- a/lib/crates/fabro-cli/src/commands/validate.rs +++ b/lib/crates/fabro-cli/src/commands/validate.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use anyhow::bail; use fabro_config::RunLayer; use fabro_config::user::active_settings_path; @@ -19,14 +17,10 @@ pub(crate) fn run( ) -> anyhow::Result<()> { let printer = base_ctx.printer(); let built = build_run_manifest(ManifestBuildInput { - workflow: args.workflow.clone(), - cwd: base_ctx.cwd().to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, + workflow: args.workflow.clone(), + cwd: base_ctx.cwd().to_path_buf(), user_settings_path: Some(active_settings_path(None)), + ..Default::default() })?; let response = manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 29b24bb29..6e66543f2 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -10,9 +10,7 @@ use anyhow::{Context, Result, anyhow}; use fabro_api::types; use fabro_config::project::{self, discover_project_config, resolve_workflow_path}; use fabro_config::run::{resolve_run_goal_from_layer, resolve_run_goal_from_namespace}; -use fabro_config::{ - CliLayer, DaytonaDockerfileLayer, RunLayer, WorkflowSettingsBuilder, apply_input_overrides, -}; +use fabro_config::{CliLayer, DaytonaDockerfileLayer, RunLayer, WorkflowSettingsBuilder}; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; use fabro_template::{TemplateContext, render as render_template}; @@ -25,7 +23,7 @@ use fabro_workflow::git::{ use crate::args::{PreflightArgs, RunArgs}; -#[derive(Debug)] +#[derive(Debug, Default)] pub struct ManifestBuildInput { pub workflow: PathBuf, pub cwd: PathBuf, @@ -97,7 +95,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let mut workflow_settings = workflow_settings_builder .build() .context("failed to resolve manifest settings")?; - apply_input_overrides(&mut workflow_settings, input.input_overrides.clone()); + workflow_settings.run.inputs.extend(input.input_overrides); let target_path = root_resolution.dot_path.clone(); let target_manifest_path = manifest_path_from_absolute(&target_path, &input.cwd)?; let target_key = target_manifest_path.to_string(); @@ -375,13 +373,8 @@ fn render_workflow_scan_source( path: &Path, inputs: &HashMap, ) -> Result { - render_template( - source, - &TemplateContext::new() - .with_goal("{{ goal }}") - .with_inputs(inputs.clone()), - ) - .with_context(|| format!("Failed to render {} for manifest scanning", path.display())) + render_template(source, &TemplateContext::for_input_scan(inputs.clone())) + .with_context(|| format!("Failed to render {} for manifest scanning", path.display())) } fn collect_workflow_config_files( @@ -731,14 +724,9 @@ mod tests { .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: project.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() }) .unwrap(); @@ -814,11 +802,9 @@ mod tests { .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: project.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::from([ + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + input_overrides: HashMap::from([ ( "prompt_file".to_string(), toml::Value::String("plan.md".to_string()), @@ -832,9 +818,7 @@ mod tests { toml::Value::String("child".to_string()), ), ]), - args: None, - run_id: None, - user_settings_path: None, + ..Default::default() }) .unwrap(); @@ -884,17 +868,13 @@ mod tests { std::fs::write(workflow_dir.join("prompts/goal.md"), "ship it").unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: project.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::from([( + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + input_overrides: HashMap::from([( "goal_file".to_string(), toml::Value::String("goal.md".to_string()), )]), - args: None, - run_id: None, - user_settings_path: None, + ..Default::default() }) .unwrap(); @@ -948,14 +928,9 @@ file = "prompts/goal.md" .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: project.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() }) .unwrap(); @@ -1002,14 +977,9 @@ file = "prompts/goal.md" .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: project.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() }) .unwrap(); @@ -1068,14 +1038,9 @@ working_dir = "repos/target" .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: workspace.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: workspace.to_path_buf(), + ..Default::default() }) .unwrap(); @@ -1124,14 +1089,9 @@ repository = "target" .unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: workspace.to_path_buf(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: workspace.to_path_buf(), + ..Default::default() }) .unwrap(); @@ -1195,14 +1155,9 @@ exit 1 temp_env::with_var("PATH", Some(path), || { temp_env::with_var("FABRO_PROMPT_ENV_LOG", Some(helper_log.as_os_str()), || { let built = build_run_manifest(ManifestBuildInput { - workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), - cwd: workspace.clone(), - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: workspace.clone(), + ..Default::default() }) .unwrap(); diff --git a/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs index c5dcd9e87..9cf4c50f8 100644 --- a/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs +++ b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs @@ -3,7 +3,6 @@ reason = "Sync temp fixture writes keep this manifest round-trip test simple and isolated." )] -use std::collections::HashMap; use std::path::PathBuf; use fabro_cli::{ManifestBuildInput, build_run_manifest}; @@ -30,14 +29,9 @@ fn cli_built_manifest_resolves_user_global_at_path() { std::fs::write(workflow_dir.join("prompts/hello.md"), "hello from bundle").unwrap(); let built = build_run_manifest(ManifestBuildInput { - workflow: workflow_dir.join("workflow.fabro"), - cwd: project, - run_overrides: None, - cli_overrides: None, - input_overrides: HashMap::new(), - args: None, - run_id: None, - user_settings_path: None, + workflow: workflow_dir.join("workflow.fabro"), + cwd: project, + ..Default::default() }) .unwrap(); diff --git a/lib/crates/fabro-config/src/input_overrides.rs b/lib/crates/fabro-config/src/input_overrides.rs index 76ad0b88c..f2edef7f4 100644 --- a/lib/crates/fabro-config/src/input_overrides.rs +++ b/lib/crates/fabro-config/src/input_overrides.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; -use fabro_types::WorkflowSettings; use thiserror::Error; #[derive(Debug, Error, Clone, PartialEq, Eq)] @@ -32,22 +31,19 @@ fn parse_input_value(key: &str, raw_value: &str) -> Result() { - Ok(value) => { - let value = value - .get("value") - .cloned() - .unwrap_or_else(|| toml::Value::String(raw_value.to_string())); - if let Some(kind) = unsupported_kind(&value) { - return Err(InputOverrideParseError::UnsupportedValue { - key: key.to_string(), - kind, - }); - } - Ok(value) - } - Err(_) => Ok(toml::Value::String(raw_value.to_string())), + let Ok(mut table) = document.parse::() else { + return Ok(toml::Value::String(raw_value.to_string())); + }; + let value = table + .remove("value") + .expect("`value` key was just written to the document"); + if let Some(kind) = unsupported_kind(&value) { + return Err(InputOverrideParseError::UnsupportedValue { + key: key.to_string(), + kind, + }); } + Ok(value) } pub fn parse_input_overrides( @@ -66,30 +62,19 @@ pub fn parse_input_overrides( Ok(parsed) } -pub fn apply_input_overrides( - settings: &mut WorkflowSettings, - input_overrides: HashMap, -) { - settings.run.inputs.extend(input_overrides); -} - #[cfg(test)] mod tests { use super::*; - use crate::WorkflowSettingsBuilder; - fn parse_one(raw: &str) -> Result { - parse_input_overrides(&[raw.to_string()]).map(|mut parsed| { - parsed - .remove(raw.split_once('=').map_or("", |(key, _)| key)) - .expect("input should be present") - }) + fn parse_one(key: &str, raw_value: &str) -> Result { + parse_input_overrides(&[format!("{key}={raw_value}")]) + .map(|mut parsed| parsed.remove(key).expect("input should be present")) } #[test] fn empty_value_is_empty_string() { assert_eq!( - parse_one("foo=").unwrap(), + parse_one("foo", "").unwrap(), toml::Value::String(String::new()) ); } @@ -97,7 +82,7 @@ mod tests { #[test] fn bare_value_falls_back_to_string() { assert_eq!( - parse_one("foo=bar").unwrap(), + parse_one("foo", "bar").unwrap(), toml::Value::String("bar".to_string()) ); } @@ -105,29 +90,32 @@ mod tests { #[test] fn quoted_toml_string_is_accepted() { assert_eq!( - parse_one("foo=\"bar\"").unwrap(), + parse_one("foo", "\"bar\"").unwrap(), toml::Value::String("bar".to_string()) ); } #[test] fn boolean_is_accepted() { - assert_eq!(parse_one("foo=false").unwrap(), toml::Value::Boolean(false)); + assert_eq!( + parse_one("foo", "false").unwrap(), + toml::Value::Boolean(false) + ); } #[test] fn integer_is_accepted() { - assert_eq!(parse_one("foo=3").unwrap(), toml::Value::Integer(3)); + assert_eq!(parse_one("foo", "3").unwrap(), toml::Value::Integer(3)); } #[test] fn float_is_accepted() { - assert_eq!(parse_one("foo=0.75").unwrap(), toml::Value::Float(0.75)); + assert_eq!(parse_one("foo", "0.75").unwrap(), toml::Value::Float(0.75)); } #[test] fn datetime_is_rejected() { - let err = parse_one("foo=2026-05-06").unwrap_err(); + let err = parse_one("foo", "2026-05-06").unwrap_err(); assert_eq!(err, InputOverrideParseError::UnsupportedValue { key: "foo".to_string(), kind: "datetime", @@ -138,7 +126,7 @@ mod tests { #[test] fn array_is_rejected() { - let err = parse_one("foo=[1]").unwrap_err(); + let err = parse_one("foo", "[1]").unwrap_err(); assert_eq!(err, InputOverrideParseError::UnsupportedValue { key: "foo".to_string(), kind: "array", @@ -148,7 +136,7 @@ mod tests { #[test] fn inline_table_is_rejected() { - let err = parse_one("foo={a=1}").unwrap_err(); + let err = parse_one("foo", "{a=1}").unwrap_err(); assert_eq!(err, InputOverrideParseError::UnsupportedValue { key: "foo".to_string(), kind: "inline table", @@ -180,33 +168,4 @@ mod tests { Some(&toml::Value::String("second".to_string())) ); } - - #[test] - fn apply_preserves_unrelated_existing_inputs() { - let mut settings = WorkflowSettingsBuilder::from_toml( - r#" -_version = 1 - -[run.inputs] -keep = "project" -override = "project" -"#, - ) - .unwrap(); - let input_overrides = HashMap::from([( - "override".to_string(), - toml::Value::String("cli".to_string()), - )]); - - apply_input_overrides(&mut settings, input_overrides); - - assert_eq!( - settings.run.inputs.get("keep"), - Some(&toml::Value::String("project".to_string())) - ); - assert_eq!( - settings.run.inputs.get("override"), - Some(&toml::Value::String("cli".to_string())) - ); - } } diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 20bfb8966..73dc06989 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -36,7 +36,7 @@ pub use builders::{ pub use error::{Error, Result}; pub use fabro_util::path::expand_tilde; pub use home::Home; -pub use input_overrides::{InputOverrideParseError, apply_input_overrides, parse_input_overrides}; +pub use input_overrides::{InputOverrideParseError, parse_input_overrides}; pub use layers::{ CliAuthLayer, CliExecAgentLayer, CliExecLayer, CliExecModelLayer, CliLayer, CliLoggingLayer, CliOutputLayer, CliTargetLayer, CliUpdatesLayer, DaytonaDockerfileLayer, DaytonaSandboxLayer, diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 49bb7b099..23e3830b2 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -11,7 +11,7 @@ use fabro_config::run::parse_run_layer_from_settings_toml; use fabro_config::{ CliLayer, CliOutputLayer, DaytonaDockerfileLayer, DockerSandboxLayer, LocalSandboxLayer, ReplaceMap, RunExecutionLayer, RunLayer, RunModelLayer, RunSandboxLayer, - WorkflowSettingsBuilder, apply_input_overrides, parse_input_overrides, + WorkflowSettingsBuilder, parse_input_overrides, }; use fabro_graphviz::graph::{Graph, is_llm_handler_type}; use fabro_graphviz::render::apply_direction; @@ -125,7 +125,7 @@ pub(crate) fn prepare_manifest( let mut settings = workflow_settings_builder .build() .context("failed to resolve manifest settings")?; - apply_input_overrides(&mut settings, args_overrides.input_overrides); + settings.run.inputs.extend(args_overrides.input_overrides); if let Some(goal) = manifest.goal.as_ref() { settings.run.goal = Some(RunGoal::Inline(InterpString::parse(&goal.text))); } diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index 99b917430..7bec0f6a0 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -32,6 +32,14 @@ impl TemplateContext { self } + /// Context that interpolates inputs but leaves `{{ goal }}` as a literal + /// pass-through — used for structural pre-rendering before the goal is + /// known (e.g. manifest scanning, import resolution). + #[must_use] + pub fn for_input_scan(inputs: HashMap) -> Self { + Self::new().with_goal("{{ goal }}").with_inputs(inputs) + } + #[must_use] pub fn with_env_lookup(mut self, env: &E) -> Self where diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index a573b77c5..8872b792e 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -304,13 +304,8 @@ pub(super) fn preprocess_and_validate( goal_override: Option<&str>, ) -> Result { let inputs = run_inputs(settings); - let source = render_template( - dot_source, - &TemplateContext::new() - .with_goal("{{ goal }}") - .with_inputs(inputs.clone()), - ) - .map_err(|error| Error::Parse(format!("template expansion failed: {error}")))?; + let source = render_template(dot_source, &TemplateContext::for_input_scan(inputs.clone())) + .map_err(|error| Error::Parse(format!("template expansion failed: {error}")))?; let mut parsed = pipeline::parse(&source)?; apply_goal_override(&mut parsed.graph, goal_override); diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 3ab0b9d5d..464692846 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -155,9 +155,7 @@ impl ImportTransform { Self::with_import_stack(import_stack, resolved_file.path.clone(), |import_stack| { let rendered_source = render_template( &resolved_file.content, - &TemplateContext::new() - .with_goal("{{ goal }}") - .with_inputs(self.inputs.clone()), + &TemplateContext::for_input_scan(self.inputs.clone()), ) .map_err(|error| ImportPrepareError::Hard(Error::Validation(error.to_string())))?; diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index 3549177d3..854050eaf 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -25,9 +25,7 @@ impl TemplateTransform { } fn resolved_goal(&self, graph: &Graph) -> Result { - let ctx = TemplateContext::new() - .with_goal("{{ goal }}") - .with_inputs(self.inputs.clone()); + let ctx = TemplateContext::for_input_scan(self.inputs.clone()); Ok(render_template(graph.goal(), &ctx)?) } }