diff --git a/docs/public/workflows/variables.mdx b/docs/public/workflows/variables.mdx index 1b03917a9..9e9857571 100644 --- a/docs/public/workflows/variables.mdx +++ b/docs/public/workflows/variables.mdx @@ -68,7 +68,7 @@ A [command node](/workflows/stages-and-nodes#command) `script` substitutes `{{ g ```dot title="check.fabro" digraph Check { test [shape=parallelogram, script="cargo test -p {{ inputs.crate }} --profile {{ vars.PROFILE }}"] - pr [shape=parallelogram, script="gh pr create --title \"{{ goal }}\""] + pr [shape=parallelogram, script="gh pr create --title {{ goal }}"] } ``` @@ -80,22 +80,30 @@ report [shape=parallelogram, script="kubectl get pod -o go-template='{{ .status. There is no `{% if %}`, no filters, and no loops, and `{{ goal }}` has no dotted form — `{{ goal.title }}` stays literal. Put branching in a [conditional node](/workflows/stages-and-nodes#conditional) or in the shell itself. -Values are substituted as-is, without shell quoting — the script is yours to quote. If a value can contain spaces or shell metacharacters, quote it at the use site. This matters most for `{{ goal }}`, which is free-form prose: +In a shell script, Fabro quotes each substituted value as one shell argument. Put the token where one shell word is valid. Do not add quotes around the token, and do not use a token to inject multiple flags or shell syntax: ```dot -build [shape=parallelogram, script="docker build -t \"{{ inputs.image }}\" ."] +build [shape=parallelogram, script="docker build -t {{ inputs.image }} ."] ``` -Substituted text is never scanned again, so a goal or input containing `{{ ... }}` reaches the shell as literal characters rather than being interpolated a second time. +For `language="python"`, Fabro inserts each value as a quoted string literal. Put the token where a Python expression is valid: + +```dot +report [shape=parallelogram, language="python", script="print({{ goal }})"] +``` + +Substituted text is never scanned again, so a goal or input containing `{{ ... }}` reaches the command as literal characters rather than being interpolated a second time. ### `env` and `secrets` are not substituted -`{{ env.NAME }}` and `{{ secrets.NAME }}` are rejected in a `script` with a validation error. A script runs in a shell, so read environment variables the shell way: +`{{ env.NAME }}` and `{{ secrets.NAME }}` are rejected in a `script` with a validation error. Read environment variables with `$NAME` in a shell script: ```dot deploy [shape=parallelogram, script="deploy --token $DEPLOY_TOKEN"] ``` +In a Python script, read them with `os.environ["NAME"]`. + To make a secret available that way, put it in the [environment's env map](/execution/run-configuration#run-environment-and-environments-slug), where `{{ secrets.* }}` does resolve — at run start, into the sandbox environment rather than into the script text: ```toml title="run.toml" diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index f52079a29..a16e0b0ab 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -7,7 +7,7 @@ use fabro_model::ModelSelectionError; use fabro_template::TemplateError; pub use fabro_types::failure_signature::FailureSignature; pub use fabro_types::outcome::FailureCategory; -use fabro_types::settings::AmbiguousModelRef; +use fabro_types::settings::{AmbiguousModelRef, ResolveError}; use fabro_types::{ExecOutputTail, FailureReason, RunFailure}; use fabro_util::error::{SharedError, collect_causes, collect_chain, render_with_causes}; use fabro_validate::Diagnostic; @@ -263,6 +263,14 @@ pub enum Error { #[error("Validation failed")] ValidationFailed { diagnostics: Vec }, + #[error("Validation error: script interpolation failed in {owner}: {source} ({fix})")] + ScriptInterpolation { + owner: String, + fix: String, + #[source] + source: ResolveError, + }, + #[error("Model selection failed: {0}")] ModelSelection(#[from] ModelSelectionError), @@ -427,6 +435,7 @@ impl Error { .as_ref() .map_or_else(Vec::new, |source| collect_chain(source)), Self::Template { source, .. } => collect_chain(source), + Self::ScriptInterpolation { source, .. } => collect_chain(source), Self::Llm(err) => collect_causes(err), _ => Vec::new(), } @@ -452,6 +461,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::ScriptInterpolation { .. } | Self::ModelSelection(_) | Self::ModelReference(_) | Self::Template { .. } @@ -475,6 +485,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::ScriptInterpolation { .. } | Self::ModelSelection(_) | Self::ModelReference(_) | Self::Template { .. } diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d6637..c7181ef4d 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use super::types::{Parsed, TransformOptions, Transformed}; use crate::error::Error; use crate::transforms::{ - FileInliningTransform, ImportTransform, ModelResolutionTransform, + FileInliningTransform, ImportTransform, ModelResolutionTransform, ScriptInterpolationTransform, StylesheetApplicationTransform, TemplateTransform, Transform, }; @@ -62,6 +62,13 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result run -> exit + }"#, + ); + let parsed = parse( + r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + child [import="./child.fabro"] + exit [shape=Msquare] + start -> child -> exit + }"#, + ) + .unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ + ( + "payload".to_string(), + toml::Value::String("{{ secrets.API_KEY }}".to_string()), + ), + ])), + ..transform_options() + }) + .unwrap(); + + assert_eq!( + transformed.graph.nodes["child.run"] + .attrs + .get("script") + .and_then(AttrValue::as_str), + Some("printf '%s' '{{ secrets.API_KEY }}'") + ); + } + + #[test] + fn imported_script_reports_a_missing_value_once() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("child.fabro"), + r#"digraph child { + start [shape=Mdiamond] + run [shape=parallelogram, script="echo {{ inputs.missing }}"] + exit [shape=Msquare] + start -> run -> exit + }"#, + ); + let parsed = parse( + r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + child [import="./child.fabro"] + exit [shape=Msquare] + start -> child -> exit + }"#, + ) + .unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + render_mode: crate::operations::RenderMode::Structural, + ..transform_options() + }) + .unwrap(); + + let missing_value_diagnostics = transformed + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.rule == TEMPLATE_UNDEFINED_VARIABLE_RULE) + .count(); + assert_eq!( + missing_value_diagnostics, 1, + "{:?}", + transformed.diagnostics + ); + } + #[test] fn transform_interpolates_vars_in_node_prompt() { let dot = r#"digraph Test { diff --git a/lib/components/fabro-workflow/src/transforms/mod.rs b/lib/components/fabro-workflow/src/transforms/mod.rs index 1f6879cda..8246ee750 100644 --- a/lib/components/fabro-workflow/src/transforms/mod.rs +++ b/lib/components/fabro-workflow/src/transforms/mod.rs @@ -22,4 +22,4 @@ pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; pub use stylesheet_application::StylesheetApplicationTransform; -pub use variable_expansion::{RenderMode, TemplateTransform}; +pub use variable_expansion::{RenderMode, ScriptInterpolationTransform, TemplateTransform}; diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index 9384a9b8f..7844c7918 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -1,14 +1,17 @@ +use std::borrow::Cow; use std::collections::HashMap; use std::fmt::Write as _; use std::sync::Arc; -use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_graphviz::graph::{AttrValue, Graph, Node}; use fabro_template::{ TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, TemplateStore, }; -use fabro_types::settings::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; +use fabro_types::settings::interp::Namespace; +use fabro_types::settings::{InterpString, ResolveCtx, ResolveError, ResolveErrorKind}; use fabro_util::error::collect_chain; +use fabro_util::shell; use fabro_validate::{Diagnostic, Severity}; use super::Transform; @@ -223,9 +226,7 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> message, node_id: target.node_id.clone(), edge: target.edge.clone(), - fix: Some(format!( - "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" - )), + fix: Some(input_binding_fix(name)), source_path: location.source_name.or_else(|| target.source_name.clone()), line: location.line, column: location.column, @@ -235,10 +236,12 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> } } -/// Substitutes `{{ inputs.* }}` and `{{ vars.* }}` in a command node `script`. -/// -/// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in a command -/// node `script`. +fn input_binding_fix(name: &str) -> String { + format!("bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`") +} + +/// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in one +/// command node `script`. /// /// Scripts interpolate through [`InterpString`] tokens rather than the /// MiniJinja pass that renders prompts. Shell source is full of brace syntax @@ -252,53 +255,81 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> /// secret would be baked into the `CommandStarted` event that records the /// script verbatim. /// -/// Resolved values are substituted verbatim, not shell-quoted — matching -/// `[[run.prepare.steps]].script`, where the shell snippet is the author's to -/// quote. Callers wanting a value treated as a single argument must quote it -/// in the script. This matters most for `{{ goal }}`, which is free-form prose. -fn interpolate_script( - text: &str, +/// Shell values are quoted as one argument. Python values are quoted as string +/// literals. In both languages the token must stand where one value is valid; +/// callers must not wrap it in another string literal. +fn interpolate_script<'a>( + text: &'a str, ctx: &TemplateContext, + language: &str, render_mode: RenderMode, target: &TemplateRenderTarget, diagnostics: &mut Vec, -) -> Result { +) -> Result, Error> { + if !text.contains("{{") { + return Ok(Cow::Borrowed(text)); + } + + let parsed = InterpString::parse(text); + if parsed.is_literal() { + return Ok(Cow::Borrowed(text)); + } + let mut resolve_ctx = ResolveCtx::new() - .with_inputs(|name| ctx.input(name)) - .with_vars(|name| ctx.var(name)); + .with_inputs(|name| { + ctx.input(name) + .map(|value| quote_script_value(&value, language)) + }) + .with_vars(|name| { + ctx.var(name) + .map(|value| quote_script_value(&value, language)) + }); // The graph goal is rendered before any node attribute, so by here it is // the final text. Substituting it does not re-interpolate: whatever the // goal contains lands in the script as literal characters. if let Some(goal) = ctx.goal() { - resolve_ctx = resolve_ctx.with_goal(goal); + resolve_ctx = resolve_ctx.with_goal(quote_script_value(goal, language)); } - match InterpString::parse(text).resolve_with(&mut resolve_ctx) { - Ok(resolved) => Ok(resolved), + match parsed.resolve_with(&mut resolve_ctx) { + Ok(resolved) => Ok(Cow::Owned(resolved)), // An unbound input or variable is the same authoring gap the prompt // pass reports, so it follows the same mode split: a hard error at // run-create, a diagnostic during `fabro validate`. Err(err) if err.kind == ResolveErrorKind::Missing => match render_mode { - RenderMode::Strict => Err(script_interpolation_error(target, &err)), + RenderMode::Strict => Err(script_interpolation_error(target, err, language)), RenderMode::Structural => { diagnostics.push(script_undefined_variable_diagnostic(&err, target)); // Leave the script in source form. Validation never executes // it, and showing the unresolved token beats emptying it. - Ok(text.to_string()) + Ok(Cow::Borrowed(text)) } }, // An unsupported namespace can never resolve here, however the inputs // are bound, so it fails in both modes — the same treatment // `render_attrs` gives an invalid static reference. - Err(err) => Err(script_interpolation_error(target, &err)), + Err(err) => Err(script_interpolation_error(target, err, language)), } } -fn script_interpolation_error(target: &TemplateRenderTarget, err: &ResolveError) -> Error { - Error::Validation(format!( - "script interpolation failed in {}: {err} ({})", - target.owner, - script_interpolation_fix(err) - )) +fn quote_script_value(value: &str, language: &str) -> String { + if language == "python" { + serde_json::to_string(value).expect("serializing a string to JSON should not fail") + } else { + shell::shell_quote(value) + } +} + +fn script_interpolation_error( + target: &TemplateRenderTarget, + source: ResolveError, + language: &str, +) -> Error { + let fix = script_interpolation_fix(&source, Some(language)); + Error::ScriptInterpolation { + owner: target.owner.clone(), + fix, + source, + } } fn script_undefined_variable_diagnostic( @@ -311,23 +342,29 @@ fn script_undefined_variable_diagnostic( message: format!("{err} in {}", target.owner), node_id: target.node_id.clone(), edge: target.edge.clone(), - fix: Some(script_interpolation_fix(err)), + fix: Some(script_interpolation_fix(err, None)), source_path: target.source_name.clone(), ..Diagnostic::default() } } -fn script_interpolation_fix(err: &ResolveError) -> String { +fn script_interpolation_fix(err: &ResolveError, language: Option<&str>) -> String { let name = &err.name; match err.namespace { - Namespace::Inputs => format!( - "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" - ), + Namespace::Inputs => input_binding_fix(name), Namespace::Vars => format!("set it with `fabro variable set {name} `"), + Namespace::Env if language == Some("python") => format!( + "`script` does not interpolate environment variables; read it in Python as \ + `os.environ[\"{name}\"]` instead" + ), Namespace::Env => format!( "`script` does not interpolate environment variables; read it in the shell as \ `${name}` instead" ), + Namespace::Secrets if language == Some("python") => format!( + "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ + `[environments..env]` and read it in Python as `os.environ[\"{name}\"]`" + ), Namespace::Secrets => format!( "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ `[environments..env]` and read it in the shell as `${name}`" @@ -347,7 +384,8 @@ fn detemplated_attribute_diagnostic(attr_name: &str, target: &TemplateRenderTarg message: format!( "`{attr_name}` in {} is no longer a template; `{{{{ … }}}}` / `{{% … %}}` is treated \ as literal text. Only node `prompt` and graph `goal` support templating, and node \ - `script` supports `{{{{ inputs.* }}}}` / `{{{{ vars.* }}}}` interpolation.", + command `script` supports `{{{{ goal }}}}`, `{{{{ inputs.* }}}}`, and \ + `{{{{ vars.* }}}}` interpolation.", target.owner ), node_id: target.node_id.clone(), @@ -392,8 +430,8 @@ fn goal_self_reference_diagnostic( } } -/// Expands `{{ goal }}` / `{{ inputs.* }}` / `{{ vars.* }}` across all string -/// attributes. +/// Renders graph goals and node prompts, and diagnoses template syntax in +/// attributes that do not support it. pub struct TemplateTransform { pub context: TemplateContext, pub source_name: Option, @@ -476,6 +514,11 @@ impl TemplateTransform { if attr_name == "stack.child_dot_source" { continue; } + // Command scripts use narrow value interpolation in a separate + // one-shot transform after imports are expanded. + if matches!(scope, AttributeScope::Node) && attr_name == "script" { + continue; + } if let Some(kind) = reference_kind_for_attribute(scope, attr_name, text) { validate_static_reference(text, kind) .map_err(|error| Error::Validation(error.to_string()))?; @@ -489,10 +532,6 @@ impl TemplateTransform { // MiniJinja template. *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; - } else if matches!(scope, AttributeScope::Node) && attr_name == "script" { - // `script` interpolates a narrow token set instead — see - // `interpolate_script`. - *text = interpolate_script(text, ctx, render_mode, &target, diagnostics)?; } else if fabro_template::contains_template_syntax(text) { // Every other attribute is no longer a template (`label`, // `model`, `provider`, `speed`, `condition`, edge `label`, @@ -580,6 +619,83 @@ impl Transform for TemplateTransform { } } +/// Interpolates command node scripts once, after import expansion is complete. +/// +/// Keeping this pass separate from [`TemplateTransform`] prevents imported +/// scripts from being scanned once in their source graph and again after they +/// are merged into the root graph. +pub struct ScriptInterpolationTransform { + pub context: TemplateContext, + pub source_name: Option, + pub render_mode: RenderMode, +} + +impl ScriptInterpolationTransform { + fn command_script_language(node: &Node) -> Option<&'static str> { + let is_command = matches!(node.handler_type(), Some("command" | "tool")); + is_command.then(|| { + if node.attrs.get("language").and_then(AttrValue::as_str) == Some("python") { + "python" + } else { + "shell" + } + }) + } + + pub(crate) fn apply_with_diagnostics( + &self, + graph: Graph, + ) -> Result<(Graph, Vec), Error> { + let mut graph = graph; + let mut diagnostics = Vec::new(); + let ctx = self.context.clone().with_goal(graph.goal().to_string()); + + for (node_id, node) in &mut graph.nodes { + let language = Self::command_script_language(node); + let Some(AttrValue::String(text)) = node.attrs.get_mut("script") else { + continue; + }; + let target = TemplateRenderTarget::node_attr( + self.source_name.clone(), + node_id.clone(), + "script", + ) + .with_source_name( + self.source_name + .clone() + .unwrap_or_else(|| "workflow".to_string()), + ); + + if let Some(language) = language { + if let Cow::Owned(resolved) = interpolate_script( + text, + &ctx, + language, + self.render_mode, + &target, + &mut diagnostics, + )? { + *text = resolved; + } + } else if fabro_template::contains_template_syntax(text) { + diagnostics.push(detemplated_attribute_diagnostic("script", &target)); + } + } + + Ok((graph, diagnostics)) + } +} + +impl Transform for ScriptInterpolationTransform { + fn apply(&self, graph: Graph) -> Result { + let (graph, diagnostics) = self.apply_with_diagnostics(graph)?; + if !diagnostics.is_empty() { + return Err(Error::ValidationFailed { diagnostics }); + } + Ok(graph) + } +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -671,6 +787,10 @@ mod tests { .attrs .insert("goal".to_string(), AttrValue::String("Ship it".to_string())); let mut node = Node::new("test"); + node.attrs.insert( + "shape".to_string(), + AttrValue::String("parallelogram".to_string()), + ); node.attrs .insert("script".to_string(), AttrValue::String(script.to_string())); graph.nodes.insert("test".to_string(), node); @@ -681,8 +801,8 @@ mod tests { inputs: &[(&str, toml::Value)], vars: &[(&str, &str)], render_mode: RenderMode, - ) -> TemplateTransform { - TemplateTransform { + ) -> ScriptInterpolationTransform { + ScriptInterpolationTransform { context: TemplateContext::new() .with_inputs( inputs @@ -696,7 +816,6 @@ mod tests { .collect(), ), source_name: None, - source_text: None, render_mode, } } @@ -729,12 +848,58 @@ mod tests { #[test] fn script_substitutes_the_rendered_goal() { - let graph = script_graph("gh pr create --title \"{{ goal }}\""); + let graph = script_graph("gh pr create --title {{ goal }}"); let transform = script_transform(&[], &[], RenderMode::Structural); let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); - assert_eq!(script_of(&graph), "gh pr create --title \"Ship it\""); + assert_eq!(script_of(&graph), "gh pr create --title 'Ship it'"); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn shell_script_quotes_substituted_values_as_one_argument() { + let graph = script_graph("deploy --release {{ inputs.release }}"); + let transform = script_transform( + &[( + "release", + toml::Value::String("stable; touch /tmp/pwned".to_string()), + )], + &[], + RenderMode::Strict, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + script_of(&graph), + "deploy --release 'stable; touch /tmp/pwned'" + ); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn python_script_quotes_substituted_values_as_string_literals() { + let mut graph = script_graph("print({{ inputs.value }})"); + graph.nodes.get_mut("test").unwrap().attrs.insert( + "language".to_string(), + AttrValue::String("python".to_string()), + ); + let transform = script_transform( + &[( + "value", + toml::Value::String("'); __import__('os').system('id'); #".to_string()), + )], + &[], + RenderMode::Strict, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + script_of(&graph), + r#"print("'); __import__('os').system('id'); #")"# + ); assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); } @@ -827,6 +992,12 @@ mod tests { err.to_string().contains("inputs.crate"), "unexpected message: {err}" ); + let source = std::error::Error::source(&err) + .expect("script interpolation errors should preserve ResolveError as their source"); + assert!( + source.to_string().contains("inputs.crate"), + "unexpected source: {source}" + ); } /// A typed input must produce the same text in a script as in a prompt, so @@ -841,16 +1012,25 @@ mod tests { "retry --times {{ inputs.attempts }} --fast {{ inputs.fast }}".to_string(), ), ); - let transform = script_transform( - &[ - ("attempts", toml::Value::Integer(3)), - ("fast", toml::Value::Boolean(true)), - ], - &[], - RenderMode::Structural, - ); - - let (graph, _) = transform.apply_with_diagnostics(graph).unwrap(); + let context = TemplateContext::new().with_inputs(HashMap::from([ + ("attempts".to_string(), toml::Value::Integer(3)), + ("fast".to_string(), toml::Value::Boolean(true)), + ])); + let (graph, _) = TemplateTransform { + context: context.clone(), + source_name: None, + source_text: None, + render_mode: RenderMode::Structural, + } + .apply_with_diagnostics(graph) + .unwrap(); + let (graph, _) = ScriptInterpolationTransform { + context, + source_name: None, + render_mode: RenderMode::Structural, + } + .apply_with_diagnostics(graph) + .unwrap(); assert_eq!(script_of(&graph), "retry --times 3 --fast true"); assert_eq!( @@ -871,9 +1051,12 @@ mod tests { "script".to_string(), AttrValue::String("echo {{ inputs.crate }}".to_string()), ); + let (graph, mut diagnostics) = TemplateTransform::new(HashMap::new()) + .apply_with_diagnostics(graph) + .unwrap(); let transform = script_transform(&[], &[], RenderMode::Structural); - - let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + let (graph, script_diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + diagnostics.extend(script_diagnostics); assert_eq!( graph.attrs.get("script"), @@ -887,6 +1070,32 @@ mod tests { ); } + #[test] + fn non_command_node_script_stays_literal() { + let mut graph = script_graph("echo {{ inputs.value }}"); + graph + .nodes + .get_mut("test") + .unwrap() + .attrs + .insert("shape".to_string(), AttrValue::String("box".to_string())); + let transform = script_transform( + &[("value", toml::Value::String("changed".to_string()))], + &[], + RenderMode::Structural, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), "echo {{ inputs.value }}"); + assert!( + diagnostics + .iter() + .any(|diagnostic| diagnostic.rule == DETEMPLATED_ATTRIBUTE_RULE), + "non-command scripts should keep the literal-template warning: {diagnostics:?}" + ); + } + #[test] fn template_transform_leaves_non_string_attrs_unchanged() { let mut graph = Graph::new("test"); diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index bb4950663..f312e9416 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -816,13 +816,21 @@ mod tests { } #[test] - fn input_and_var_return_none_when_unbound() { + fn workflow_values_return_none_when_unbound() { let ctx = TemplateContext::new(); + assert_eq!(ctx.goal(), None); assert_eq!(ctx.input("missing"), None); assert_eq!(ctx.var("MISSING"), None); } + #[test] + fn goal_returns_the_rendered_value() { + let ctx = TemplateContext::new().with_goal("Ship it"); + + assert_eq!(ctx.goal(), Some("Ship it")); + } + /// `input`/`var` exist so non-template consumers render a value the same /// way a prompt does. Pin that equivalence across the scalar TOML types. #[test] diff --git a/lib/foundation/fabro-types/src/settings/interp.rs b/lib/foundation/fabro-types/src/settings/interp.rs index b91c158ea..1152bd7fd 100644 --- a/lib/foundation/fabro-types/src/settings/interp.rs +++ b/lib/foundation/fabro-types/src/settings/interp.rs @@ -5,8 +5,9 @@ //! scope-determined by the caller through [`ResolveCtx`]: server-scope //! settings provide `env` (and eventually `secrets`), run-scope settings //! additionally provide `vars`, and a command node `script` provides `inputs` -//! and `vars` only. A token whose namespace has no lookup in the resolution -//! context fails loudly rather than passing through as literal text. +//! and `vars` plus the bare `goal` value. A token whose namespace has no lookup +//! in the resolution context fails loudly rather than passing through as +//! literal text. //! //! Most call sites do not wire `inputs`: it is bound where a run's typed //! `[run.inputs]` values are in scope, which is the workflow graph, not @@ -31,7 +32,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use crate::variable::is_env_style_name; /// A config string that may contain `{{ env.NAME }}`, `{{ vars.NAME }}`, -/// `{{ secrets.NAME }}`, or `{{ inputs.NAME }}` tokens. +/// `{{ secrets.NAME }}`, `{{ inputs.NAME }}`, or `{{ goal }}` tokens. #[derive(Debug, Clone, PartialEq, Eq)] pub struct InterpString { segments: Vec, @@ -359,7 +360,7 @@ impl InterpString { /// for the namespaces it does not — their resolution happens later, /// possibly in a different process. /// - /// This is the early, server-side pass (`vars`/`inputs`); late-bound + /// This is the early, server-side pass (`vars`/`inputs`/`goal`); late-bound /// namespaces (`env`/`secrets`) survive in token form for their /// consumption-time [`InterpString::resolve_with`]. pub fn substitute_with(&self, ctx: &mut ResolveCtx<'_>) -> Result { @@ -559,7 +560,7 @@ impl<'de> Deserialize<'de> for InterpString { fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str( "a string, optionally containing {{ env.NAME }}, {{ vars.NAME }}, \ - {{ secrets.NAME }}, or {{ inputs.NAME }} interpolation tokens", + {{ secrets.NAME }}, {{ inputs.NAME }}, or {{ goal }} interpolation tokens", ) } @@ -712,7 +713,8 @@ mod tests { s: InterpString, } - let input = r#"{"s":"{{ env.A }}/{{ vars.B }}/{{ secrets.C }}/{{ inputs.d-key }}"}"#; + let input = + r#"{"s":"{{ env.A }}/{{ vars.B }}/{{ secrets.C }}/{{ inputs.d-key }}/{{ goal }}"}"#; let parsed: Wrap = serde_json::from_str(input).unwrap(); let rendered = serde_json::to_string(&parsed).unwrap(); assert_eq!(rendered, input); @@ -982,5 +984,6 @@ mod tests { assert_eq!(Namespace::Vars.to_string(), "vars"); assert_eq!(Namespace::Secrets.to_string(), "secrets"); assert_eq!(Namespace::Inputs.to_string(), "inputs"); + assert_eq!(Namespace::Goal.to_string(), "goal"); } } diff --git a/lib/foundation/fabro-types/src/settings/mod.rs b/lib/foundation/fabro-types/src/settings/mod.rs index e7c44be84..5bf91e4d9 100644 --- a/lib/foundation/fabro-types/src/settings/mod.rs +++ b/lib/foundation/fabro-types/src/settings/mod.rs @@ -25,7 +25,7 @@ pub use cli::{ CliLoggingSettings, CliNamespace, CliOutputSettings, CliTargetSettings, CliUpdatesSettings, }; pub use duration::{Duration, ParseDurationError}; -pub use interp::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; +pub use interp::{InterpString, ResolveCtx, ResolveError, ResolveErrorKind}; pub use model_ref::{ AmbiguousModelRef, ModelRef, ModelRegistry, ParseModelRefError, ResolvedModelRef, };