diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index a3cc2175f..16e5a53c2 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -524,6 +524,24 @@ pub fn contains_template_syntax(template: &str) -> bool { template.contains("{{") || template.contains("{%") || template.contains("{#") } +/// Whether `template` references `name` as a top-level variable. +/// +/// Used by the goal self-reference lint: a graph `goal` may not reference +/// itself. Returns `false` for templates that fail to parse — syntax errors +/// surface through the normal render path with proper diagnostics. +#[must_use] +pub fn references_top_level_variable(template: &str, name: &str) -> bool { + if is_plain_text(template) || !template.contains(name) { + return false; + } + let mut env = Environment::new(); + if env.add_template("__variable_scan__", template).is_err() { + return false; + } + env.get_template("__variable_scan__") + .is_ok_and(|tmpl| tmpl.undeclared_variables(false).contains(name)) +} + /// Returns `true` when the string contains no MiniJinja delimiters and can /// be returned as-is without paying for a full template parse+render cycle. fn is_plain_text(template: &str) -> bool { @@ -798,6 +816,24 @@ mod tests { assert_eq!(rendered, "Goal: Fix bugs"); } + #[test] + fn references_top_level_variable_detects_goal_self_reference() { + assert!(references_top_level_variable("Do {{ goal }} now", "goal")); + assert!(references_top_level_variable("{{ goal.title }}", "goal")); + assert!(!references_top_level_variable( + "Fix {{ inputs.bug }}", + "goal" + )); + assert!(!references_top_level_variable( + "plain text, no goal token", + "goal" + )); + assert!(!references_top_level_variable( + "{# {{ goal }} is commented out #}", + "goal" + )); + } + #[test] fn renders_typed_input_values() { let ctx = TemplateContext::new().with_inputs(HashMap::from([ diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index a7437eeaa..64b25bc49 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -430,7 +430,7 @@ mod tests { use super::*; use crate::operations::{ValidateInput, validate}; - use crate::pipeline::types::TEMPLATE_UNDEFINED_VARIABLE_RULE; + use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; use crate::workflow_bundle::BundledWorkflow; fn memory_store() -> Arc { Arc::new(Database::new( @@ -497,6 +497,36 @@ mod tests { assert!(validated.graph().find_exit_node().is_some()); } + #[test] + fn validate_rejects_goal_self_reference() { + // A goal can't reference itself; a prompt can reference the goal. + let dot = r#"digraph Test { + graph [goal="Refine {{ goal }}"] + start [shape=Mdiamond] + work [prompt="Work on {{ goal }}"] + exit [shape=Msquare] + start -> work -> exit + }"#; + let validated = validate_dot(dot, WorkflowSettings::default()); + + assert!( + validated.has_errors(), + "goal self-reference should fail validation" + ); + let self_ref: Vec<_> = validated + .diagnostics() + .iter() + .filter(|d| d.rule == GOAL_SELF_REFERENCE_RULE) + .collect(); + assert_eq!( + self_ref.len(), + 1, + "expected one goal self-reference diagnostic, got: {:?}", + validated.diagnostics() + ); + assert_eq!(self_ref[0].severity, Severity::Error); + } + #[test] fn validate_with_unbound_inputs_warns_but_succeeds() { let dot = r#"digraph Test { diff --git a/lib/crates/fabro-workflow/src/pipeline/transform.rs b/lib/crates/fabro-workflow/src/pipeline/transform.rs index 5d9945916..85a90e91a 100644 --- a/lib/crates/fabro-workflow/src/pipeline/transform.rs +++ b/lib/crates/fabro-workflow/src/pipeline/transform.rs @@ -91,6 +91,7 @@ mod tests { use super::*; use crate::file_resolver::FilesystemFileResolver; use crate::pipeline::parse::parse; + use crate::pipeline::types::GOAL_SELF_REFERENCE_RULE; fn write_file(path: &Path, contents: &str) { if let Some(parent) = path.parent() { @@ -237,4 +238,41 @@ mod tests { Some(&AttrValue::String("claude-sonnet-4-6".into())) ); } + + #[test] + fn transform_reports_goal_self_reference_once_across_passes() { + // FileInlining renders the goal for prompt context, but TemplateTransform + // is the only pass that should emit the self-reference diagnostic. + let dir = tempfile::tempdir().unwrap(); + let parsed = parse( + r#"digraph Test { + graph [goal="Improve on {{ goal }}"] + start [shape=Mdiamond] + work [prompt="Do the work"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ) + .unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + inputs: HashMap::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Structural, + custom_transforms: vec![], + catalog: test_catalog(), + }) + .unwrap(); + + let self_ref = transformed + .diagnostics + .iter() + .filter(|d| d.rule == GOAL_SELF_REFERENCE_RULE) + .count(); + assert_eq!( + self_ref, 1, + "goal self-reference should be reported exactly once across transform passes" + ); + } } diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index 541daebe1..8e7a25cac 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -50,6 +50,9 @@ pub struct Transformed { /// Lint rule name attached to diagnostics for undefined template variables. pub const TEMPLATE_UNDEFINED_VARIABLE_RULE: &str = "template_undefined_variable"; +/// Lint rule name attached to diagnostics for graph goal self-references. +pub(crate) const GOAL_SELF_REFERENCE_RULE: &str = "goal_self_reference"; + /// Output of the VALIDATE phase. Always produced (even with errors). /// Caller inspects diagnostics and decides whether to proceed. /// Graph is read-only — use accessors, not direct field access. diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index fad3b42e4..cc45fc77a 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -12,8 +12,7 @@ use crate::error::Error; use crate::file_resolver::{FileResolver, FileResolverTemplateStore, ResolvedFile}; use crate::static_reference::{ReferenceKind, validate_static_reference}; use crate::transforms::variable_expansion::{ - RenderMode, TemplateRenderStore, TemplateRenderTarget, TemplateTransform, - render_template_for_target, + RenderMode, TemplateRenderStore, TemplateRenderTarget, render_template_for_target, }; /// Resolve a potential `@path` file reference. @@ -179,13 +178,10 @@ impl FileInliningTransform { let resolved_goal = match &self.goal_override { Some(goal) => goal.clone(), - None => TemplateTransform { - inputs: self.inputs.clone(), - source_name: self.source_name.clone(), - source_text: self.source_text.clone(), - render_mode: self.render_mode, - } - .resolved_goal(&graph, &mut diagnostics)?, + // `inline_graph_goal` has already rendered inputs and inlined any + // goal file reference for prompt context. The later TemplateTransform + // pass owns canonical goal validation diagnostics. + None => graph.goal().to_string(), }; let ctx = TemplateContext::new() .with_goal(resolved_goal) diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index 2a3a131f9..9a6fda263 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -5,14 +5,14 @@ use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_template::{ TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, - TemplateStore, render_named_with_origin, render_source, + TemplateStore, }; use fabro_util::error::collect_chain; use fabro_validate::{Diagnostic, Severity}; use super::Transform; use crate::error::Error; -use crate::pipeline::types::TEMPLATE_UNDEFINED_VARIABLE_RULE; +use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; use crate::static_reference::{ AttributeScope, ReferenceKind, reference_kind_for_attribute, validate_static_reference, }; @@ -67,7 +67,7 @@ impl TemplateRenderStore { None => self.source.clone(), }; text.clone_into(&mut source.content); - render_source(&source, ctx, Arc::clone(&self.store), mode) + fabro_template::render_source(&source, ctx, Arc::clone(&self.store), mode) } } @@ -158,31 +158,42 @@ pub(crate) fn render_template_for_target( target: &TemplateRenderTarget, diagnostics: &mut Vec, ) -> Result { - let source_name = target.template_source_name(); - let render_with_mode = |mode| match target.template_store.as_ref() { + match render_mode { + RenderMode::Strict => { + render_template_with_mode(text, ctx, TemplateRenderMode::Strict, target) + .map_err(|err| template_error_for_target(target, err)) + } + RenderMode::Structural => { + match render_template_with_mode(text, ctx, TemplateRenderMode::Strict, target) { + Ok(rendered) => Ok(rendered), + Err(err @ TemplateError::UndefinedVariable { .. }) => { + diagnostics.push(template_diagnostic(&err, target)); + render_template_with_mode(text, ctx, TemplateRenderMode::Lenient, target) + .map_err(|err| template_error_for_target(target, err)) + } + Err(err) => Err(template_error_for_target(target, err)), + } + } + } +} + +fn render_template_with_mode( + text: &str, + ctx: &TemplateContext, + mode: TemplateRenderMode, + target: &TemplateRenderTarget, +) -> Result { + match target.template_store.as_ref() { Some(template_store) => { template_store.render(text, ctx, mode, target.source_origin.as_ref()) } - None => render_named_with_origin( - source_name.clone(), + None => fabro_template::render_named_with_origin( + target.template_source_name(), text, ctx, mode, target.source_origin.as_ref(), ), - }; - match render_mode { - RenderMode::Strict => render_with_mode(TemplateRenderMode::Strict) - .map_err(|err| template_error_for_target(target, err)), - RenderMode::Structural => match render_with_mode(TemplateRenderMode::Strict) { - Ok(rendered) => Ok(rendered), - Err(err @ TemplateError::UndefinedVariable { .. }) => { - diagnostics.push(template_diagnostic(&err, target)); - render_with_mode(TemplateRenderMode::Lenient) - .map_err(|err| template_error_for_target(target, err)) - } - Err(err) => Err(template_error_for_target(target, err)), - }, } } @@ -225,12 +236,6 @@ 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 { @@ -253,6 +258,37 @@ fn detemplated_attribute_diagnostic(attr_name: &str, target: &TemplateRenderTarg } } +/// Error emitted when the graph `goal` references `{{ goal }}` — a goal cannot +/// reference itself. Prompts may reference the rendered goal; the goal renders +/// without `goal` in scope, so a self-reference is always a mistake. +fn goal_self_reference_diagnostic( + target: &TemplateRenderTarget, + error: Option<&TemplateError>, +) -> Diagnostic { + let location = error.map(TemplateError::location).unwrap_or_default(); + Diagnostic { + rule: GOAL_SELF_REFERENCE_RULE.to_owned(), + severity: Severity::Error, + message: format!( + "the graph `goal` cannot reference itself (`{{{{ goal }}}}`) in {}", + target.owner + ), + node_id: target.node_id.clone(), + edge: target.edge.clone(), + fix: Some( + "remove the `{{ goal }}` reference from the goal; a node `prompt` can reference the \ + goal instead" + .to_string(), + ), + source_path: location.source_name.or_else(|| target.source_name.clone()), + line: location.line, + column: location.column, + span_start: location.span_start, + span_len: location.span_len, + ..Diagnostic::default() + } +} + /// Expands `{{ goal }}` / `{{ inputs.* }}` across all string attributes. pub struct TemplateTransform { pub inputs: HashMap, @@ -283,12 +319,39 @@ impl TemplateTransform { .map_err(|error| Error::Validation(error.to_string()))?; return Ok(goal.to_string()); } - let ctx = TemplateContext::for_input_scan(self.inputs.clone()); let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") .with_source_origin(self.source_text.as_deref(), goal); + // The goal renders with no `goal` in scope, so it cannot reference + // itself. Flag the self-reference with a friendly diagnostic before the + // render would otherwise produce a generic "undefined variable `goal`". + if fabro_template::references_top_level_variable(goal, "goal") { + let location_error = self.goal_self_reference_location(goal, &target); + diagnostics.push(goal_self_reference_diagnostic( + &target, + location_error.as_ref(), + )); + return Ok(goal.to_string()); + } + let ctx = TemplateContext::new().with_inputs(self.inputs.clone()); render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics) } + fn goal_self_reference_location( + &self, + goal: &str, + target: &TemplateRenderTarget, + ) -> Option { + let ctx = TemplateContext::new().with_inputs(self.inputs.clone()); + match render_template_with_mode(goal, &ctx, TemplateRenderMode::Strict, target) { + Err(err @ TemplateError::UndefinedVariable { .. }) + if err.expression() == Some("goal") => + { + Some(err) + } + _ => None, + } + } + fn render_attrs( attrs: &mut HashMap, ctx: &TemplateContext, @@ -321,7 +384,7 @@ impl TemplateTransform { // `prompt` is the only templated node attribute. *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; - } else if contains_template_syntax(text) { + } else if fabro_template::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. @@ -532,6 +595,51 @@ mod tests { assert_eq!(prompt, "Goal: "); } + #[test] + fn template_transform_rejects_goal_self_reference() { + let source = r#"digraph Test { + graph [goal="Improve on {{ goal }}"] + }"#; + let mut graph = Graph::new("test"); + graph.attrs.insert( + "goal".to_string(), + AttrValue::String("Improve on {{ goal }}".to_string()), + ); + let mut node = Node::new("plan"); + node.attrs.insert( + "prompt".to_string(), + AttrValue::String("Work: {{ goal }}".to_string()), + ); + graph.nodes.insert("plan".to_string(), node); + + let transform = TemplateTransform { + inputs: HashMap::new(), + source_name: Some("workflow.fabro".to_string()), + source_text: Some(source.to_string()), + render_mode: RenderMode::Structural, + }; + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + let self_ref: Vec<_> = diagnostics + .iter() + .filter(|d| d.rule == GOAL_SELF_REFERENCE_RULE) + .collect(); + assert_eq!( + self_ref.len(), + 1, + "expected one goal_self_reference diagnostic" + ); + assert_eq!(self_ref[0].severity, Severity::Error); + assert!(self_ref[0].message.contains("cannot reference itself")); + assert_eq!(self_ref[0].source_path.as_deref(), Some("workflow.fabro")); + assert_eq!(self_ref[0].line, Some(2)); + assert!(self_ref[0].span_start.is_some()); + assert_eq!( + graph.attrs.get("goal").and_then(AttrValue::as_str), + Some("Improve on {{ goal }}") + ); + } + #[test] fn template_transform_warns_on_undefined_variable() { let mut graph = Graph::new("test");