From 911e080f3ceb916fa628bf7a88530d0237e1adce Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 5 Jun 2026 11:22:38 -0400 Subject: [PATCH] Limit DOT templates to prompt + goal (#474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Limit DOT templates to prompt + goal Part of unifying config interpolation in Fabro. Per the field taxonomy, full MiniJinja templates (`ImportableTemplate`) should be limited to **`prompt` (node) and `goal` (graph)** — the content fields that legitimately need `{{ inputs.* }}` / `{{ goal }}`. Every other graph/node/edge attribute should be a plain value, not a Turing-complete template. This is a **behavior-reducing** slice and is **independent of the InterpString foundation PR** (it touches the MiniJinja/template-engine path, not the `InterpString` config path), so it branches off `main` and can be reviewed on its own. ## What changes - `TemplateTransform::render_attrs` still renders node `prompt` (unchanged) and the graph `goal` (rendered separately, as before), but **no longer renders** `label`, `model`, `provider`, `speed`, edge `label`, or `condition`. Those are left as literal text. - When a now-demoted attribute still contains `{{ … }}` / `{% … %}`, a `detemplated_attribute` **warning** is emitted so authors can migrate (the syntax is now literal, not rendered). - `condition` keeps its dedicated routing-expression evaluator (`evaluate_condition` / `parse_condition_expr`); only the Jinja pre-render is removed, so routing still works exactly as before. - `output_schema` becomes a string-or-`@file` value, not a template: `FileInliningTransform` still resolves an `@file` reference but loads its contents **verbatim**, and neither the inline value nor the loaded file is MiniJinja-rendered. `prompt` and `goal` are unaffected — both inline and `@file` forms are still MiniJinja-rendered (the `@` only selects whether the template is in-band or loaded from a file). ## Behavior change `{{ … }}` in a demoted attribute (`label`/`model`/`provider`/`speed`/ `condition`/`output_schema`) is now **literal text** instead of being rendered. A parse-time `detemplated_attribute` warning flags any remaining occurrences so they're not silently dropped. This was rarely a sensible thing to do anyway (e.g. `label = "{{ goal }}"` would splat the entire goal into a short display label). ## Verification - `cargo build --workspace` - `cargo nextest run -p fabro-workflow` → 1164 passed - `cargo +nightly fmt --check --all` - `cargo +nightly clippy --workspace --all-targets -- -D warnings` → clean - No pending `insta` snapshots ## Tests - `template_transform_renders_prompt_and_leaves_other_attrs_literal` — `prompt` still renders; node/graph/edge `label` stay literal; one migration warning per demoted label. - `file_inlining_transform_does_not_render_templates_in_output_schema` and `file_inlining_transform_loads_output_schema_file_verbatim` — `output_schema` inline and `@file` contents are used verbatim, no Jinja. Co-authored-by: Claude Opus 4.8 (1M context) --- .../src/transforms/file_inlining.rs | 134 +++++++++++++++--- .../src/transforms/variable_expansion.rs | 77 ++++++++-- 2 files changed, 176 insertions(+), 35 deletions(-) diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index 942ba4af6..fad3b42e4 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -192,45 +192,52 @@ impl FileInliningTransform { .with_inputs(self.inputs.clone()); for (node_id, node) in &mut graph.nodes { - for attr_name in ["prompt", "output_schema"] { - let Some(AttrValue::String(attr_value)) = node.attrs.get(attr_name) else { - continue; - }; + // `prompt` is an importable template: MiniJinja-render the value, + // then inline any `@file` reference (whose contents are rendered + // too). Clone up front so the immutable borrow ends before we + // re-insert. + let prompt = match node.attrs.get("prompt") { + Some(AttrValue::String(value)) => Some(value.clone()), + _ => None, + }; + if let Some(attr_value) = prompt { let target = TemplateRenderTarget::node_attr( self.source_name.clone(), node_id.clone(), - attr_name, + "prompt", ) - .with_source_origin(self.source_text.as_deref(), attr_value) + .with_source_origin(self.source_text.as_deref(), &attr_value) .with_template_store(template_render_store( &self.current_dir, Arc::clone(&self.resolver), self.source_name.as_deref(), - attr_value, + &attr_value, )?); let rendered = render_template_for_target( - attr_value, + &attr_value, &ctx, self.render_mode, &target, &mut diagnostics, )?; - let value = match self.render_resolved_file_ref( - &rendered, - &ctx, - target, - &mut diagnostics, - )? { - Some(value) => value, - None if attr_name == "output_schema" && rendered.starts_with('@') => { - return Err(Error::Validation(format!( - "node '{node_id}' output_schema has unresolved file reference: {rendered}" - ))); - } - None => rendered, - }; + let value = self + .render_resolved_file_ref(&rendered, &ctx, target, &mut diagnostics)? + .unwrap_or(rendered); node.attrs - .insert(attr_name.to_string(), AttrValue::String(value)); + .insert("prompt".to_string(), AttrValue::String(value)); + } + + // `output_schema` is NOT a template: an inline JSON string is used + // verbatim, and an `@file` reference is loaded verbatim. Neither the + // value nor the loaded contents are MiniJinja-rendered. + let output_schema = match node.attrs.get("output_schema") { + Some(AttrValue::String(value)) => Some(value.clone()), + _ => None, + }; + if let Some(attr_value) = output_schema { + let value = self.resolve_output_schema_ref(node_id, &attr_value)?; + node.attrs + .insert("output_schema".to_string(), AttrValue::String(value)); } } @@ -294,6 +301,24 @@ impl FileInliningTransform { )?)) } + /// Resolve an `output_schema` value. An inline JSON string is returned + /// as-is; an `@file` reference is loaded verbatim. Unlike `prompt`, + /// `output_schema` is not a template, so neither the value nor the loaded + /// file contents are MiniJinja-rendered. + fn resolve_output_schema_ref(&self, node_id: &str, value: &str) -> Result { + let Some(path_str) = value.strip_prefix('@') else { + return Ok(value.to_string()); + }; + validate_static_reference(path_str, ReferenceKind::FileInline) + .map_err(|error| Error::Validation(error.to_string()))?; + let Some(resolved) = self.resolver.resolve(&self.current_dir, path_str) else { + return Err(Error::Validation(format!( + "node '{node_id}' output_schema has unresolved file reference: {value}" + ))); + }; + Ok(resolved.content) + } + fn template_root_for_resolved_file(&self, path: &Path) -> PathBuf { let parent = parent_dir_or_dot(path); if self.current_dir.is_absolute() && path.starts_with(&self.current_dir) { @@ -537,6 +562,69 @@ mod tests { ); } + #[test] + fn file_inlining_transform_does_not_render_templates_in_output_schema() { + let dir = tempfile::tempdir().unwrap(); + let mut graph = Graph::new("test"); + graph.attrs.insert( + "goal".to_string(), + AttrValue::String("Fix bugs".to_string()), + ); + let mut node = Node::new("emit"); + // `output_schema` is not a template: `{{ goal }}` must stay literal. + node.attrs.insert( + "output_schema".to_string(), + AttrValue::String(r#"{"title": "{{ goal }}"}"#.to_string()), + ); + graph.nodes.insert("emit".to_string(), node); + + let transform = FileInliningTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + ); + let graph = transform.apply(graph).unwrap(); + + assert_eq!( + graph.nodes["emit"] + .attrs + .get("output_schema") + .and_then(AttrValue::as_str), + Some(r#"{"title": "{{ goal }}"}"#) + ); + } + + #[test] + fn file_inlining_transform_loads_output_schema_file_verbatim() { + let dir = tempfile::tempdir().unwrap(); + // File contents contain template syntax that must NOT be rendered. + std::fs::write( + dir.path().join("schema.json"), + r#"{"kind": "{{ inputs.kind }}"}"#, + ) + .unwrap(); + let mut graph = Graph::new("test"); + let mut node = Node::new("emit"); + node.attrs.insert( + "output_schema".to_string(), + AttrValue::String("@schema.json".to_string()), + ); + graph.nodes.insert("emit".to_string(), node); + + let transform = FileInliningTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + ); + let graph = transform.apply(graph).unwrap(); + + assert_eq!( + graph.nodes["emit"] + .attrs + .get("output_schema") + .and_then(AttrValue::as_str), + Some(r#"{"kind": "{{ inputs.kind }}"}"#) + ); + } + #[test] fn file_inlining_transform_reports_unresolved_output_schema_reference() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index 1891e5a47..2a3a131f9 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -223,6 +223,36 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> } } +const DETEMPLATED_ATTRIBUTE_RULE: &str = "detemplated_attribute"; + +/// True when `text` contains MiniJinja template syntax (`{{ … }}` or +/// `{% … %}`). +fn contains_template_syntax(text: &str) -> bool { + text.contains("{{") || text.contains("{%") +} + +/// Warning emitted when an attribute that is no longer a template still +/// contains template syntax — the syntax is now treated as literal text. +fn detemplated_attribute_diagnostic(attr_name: &str, target: &TemplateRenderTarget) -> Diagnostic { + Diagnostic { + rule: DETEMPLATED_ATTRIBUTE_RULE.to_owned(), + severity: Severity::Warning, + message: format!( + "`{attr_name}` in {} is no longer a template; `{{{{ … }}}}` / `{{% … %}}` is treated \ + as literal text. Only node `prompt` and graph `goal` support templating.", + target.owner + ), + node_id: target.node_id.clone(), + edge: target.edge.clone(), + fix: Some(format!( + "remove the template syntax from `{attr_name}`, or move the dynamic value into a \ + `prompt`/`goal`" + )), + source_path: target.source_name.clone(), + ..Diagnostic::default() + } +} + /// Expands `{{ goal }}` / `{{ inputs.* }}` across all string attributes. pub struct TemplateTransform { pub inputs: HashMap, @@ -271,6 +301,8 @@ impl TemplateTransform { ) -> Result<(), Error> { for (attr_name, value) in attrs { if let AttrValue::String(text) = value { + // The graph `goal` is rendered separately and must not be + // re-rendered here. if matches!(scope, AttributeScope::Graph) && attr_name == "goal" { continue; } @@ -285,7 +317,16 @@ impl TemplateTransform { let target = owner_for_attr(attr_name) .with_source_name(source_name.cloned().unwrap_or_else(|| "workflow".into())) .with_source_origin(source_text, text); - *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; + if matches!(scope, AttributeScope::Node) && attr_name == "prompt" { + // `prompt` is the only templated node attribute. + *text = + render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; + } else if contains_template_syntax(text) { + // Every other attribute is no longer a template (`label`, + // `model`, `provider`, `speed`, `condition`, edge `label`, + // …): leave it literal and warn so authors can migrate. + diagnostics.push(detemplated_attribute_diagnostic(attr_name, &target)); + } } } Ok(()) @@ -378,7 +419,7 @@ mod tests { use super::*; #[test] - fn template_transform_replaces_goal_and_inputs_across_string_attrs() { + fn template_transform_renders_prompt_and_leaves_other_attrs_literal() { let mut graph = Graph::new("test"); graph.attrs.insert( "goal".to_string(), @@ -419,25 +460,37 @@ mod tests { toml::Value::String("hello".to_string()), ), ])); - let graph = transform.apply(graph).unwrap(); + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); - let prompt = graph.nodes["plan"] - .attrs - .get("prompt") - .and_then(AttrValue::as_str) - .unwrap(); - assert_eq!(prompt, "Achieve: Fix bugs now"); + // `prompt` is the only templated attribute and is still rendered. + assert_eq!( + graph.nodes["plan"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some("Achieve: Fix bugs now") + ); + // `label` (node, graph, edge) is no longer a template: left literal. assert_eq!( graph.nodes["plan"].attrs.get("label"), - Some(&AttrValue::String("Planner".to_string())) + Some(&AttrValue::String("{{ inputs.name }}".to_string())) ); assert_eq!( graph.attrs.get("label"), - Some(&AttrValue::String("Workflow: Fix bugs".to_string())) + Some(&AttrValue::String("Workflow: {{ goal }}".to_string())) ); assert_eq!( graph.edges[0].attrs.get("label"), - Some(&AttrValue::String("hello".to_string())) + Some(&AttrValue::String("{{ inputs.greeting }}".to_string())) + ); + // Each demoted `label` still containing template syntax warns. + let detemplated = diagnostics + .iter() + .filter(|d| d.rule == DETEMPLATED_ATTRIBUTE_RULE) + .count(); + assert_eq!( + detemplated, 3, + "expected a migration warning per demoted label, got: {diagnostics:?}" ); }