scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-29 06:36:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

refactor: simplify CLI input override plumbing

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-05-06 10:29:48 -04:00
parent 05ac998c6d
commit 9db5740491
No known key found for this signature in database
12 changed files with 89 additions and 193 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
lib/crates/fabro-cli/src/commands/graph.rs
View file

@ -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?;

14
lib/crates/fabro-cli/src/commands/preflight.rs
View file

@ -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..."));

12
lib/crates/fabro-cli/src/commands/validate.rs
View file

@ -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);

107
lib/crates/fabro-cli/src/manifest_builder.rs
View file

@ -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<BuiltManifest> {
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<String, toml::Value>,
) -> Result<String> {
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();

12
lib/crates/fabro-cli/tests/manifest_path_round_trip.rs
View file

@ -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();

95
lib/crates/fabro-config/src/input_overrides.rs
View file

@ -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<toml::Value, InputOve
}
let document = format!("value = {raw_value}");
match document.parse::<toml::Value>() {
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::<toml::Table>() 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<String, toml::Value>,
) {
settings.run.inputs.extend(input_overrides);
}
#[cfg(test)]
mod tests {
use super::*;
use crate::WorkflowSettingsBuilder;
fn parse_one(raw: &str) -> Result<toml::Value, InputOverrideParseError> {
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<toml::Value, InputOverrideParseError> {
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()))
);
}
}

2
lib/crates/fabro-config/src/lib.rs
View file

@ -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,

4
lib/crates/fabro-server/src/run_manifest.rs
View file

@ -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)));
}

8
lib/crates/fabro-template/src/lib.rs
View file

@ -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<String, toml::Value>) -> Self {
Self::new().with_goal("{{ goal }}").with_inputs(inputs)
}
#[must_use]
pub fn with_env_lookup<E>(mut self, env: &E) -> Self
where

9
lib/crates/fabro-workflow/src/operations/create.rs
View file

@ -304,13 +304,8 @@ pub(super) fn preprocess_and_validate(
goal_override: Option<&str>,
) -> Result<Validated, Error> {
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);

4
lib/crates/fabro-workflow/src/transforms/import.rs
View file

@ -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())))?;

4
lib/crates/fabro-workflow/src/transforms/variable_expansion.rs
View file

@ -25,9 +25,7 @@ impl TemplateTransform {
}
fn resolved_goal(&self, graph: &Graph) -> Result<String, Error> {
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)?)
}
}