mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-11 03:40:05 +00:00
A goal can't reference itself; prompts can reference the goal (#512)
Implements the goal self-reference behavior for interpolation
unification. Independent off `main` — no dependency on the other interp
PRs (touches only the template/goal-render path).
## What changes for users
A graph `goal` is a template that interpolates `{{ inputs.* }}`
(unchanged). A node `prompt` can reference the rendered goal via `{{
goal }}` (unchanged). **New:** a goal can **no longer reference itself**
— `{{ goal }}` *inside* a goal was previously a silent passthrough (left
as the literal text `{{ goal }}`); it's now a clear error.
```
graph [goal="Refine {{ goal }}"] # error: a goal cannot reference itself
work [prompt="Work on {{ goal }}"] # fine: prompts reference the rendered goal
```
## How
- **Structural guarantee:** the goal renders with **no `goal` key in
scope** (`TemplateContext::new().with_inputs(..)` instead of the
`for_input_scan` passthrough), so a self-reference can't resolve.
- **Friendly lint:** before rendering, `resolved_goal` checks the goal
template for a top-level `goal` reference — new
`fabro_template::references_top_level_variable`, backed by MiniJinja
`undeclared_variables` — and emits a dedicated `goal_self_reference`
diagnostic (`Severity::Error`) with a clear message and fix-it, instead
of a generic "undefined variable `goal`". Fails `fabro validate` and
run-create alike.
The goal is resolved in two transform passes (FileInlining +
TemplateTransform); the diagnostic is emitted **once** (FileInlining
discards its goal-resolution diagnostics; TemplateTransform is the
canonical emitter).
## Behavior change (release notes)
A goal containing `{{ goal }}` now **errors** instead of passing through
as literal text. The error message is the migration signal.
## Tests
- `references_top_level_variable` detection
- transform-level rejection (`Severity::Error`)
- single-emission across the two passes
- end-to-end `validate` rejection
- existing goal/prompt tests still green (prompts reference goal; goal
interpolates inputs)
## Verification
- `cargo build --workspace`
- `cargo +nightly clippy --workspace --all-targets -- -D warnings`
- `cargo +nightly fmt --check`
- `cargo nextest run --workspace`: 6800 passed
Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5b3b3b0a6b
commit
882d11288b
6 changed files with 249 additions and 38 deletions
|
|
@ -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([
|
||||
|
|
|
|||
|
|
@ -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<Database> {
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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<Diagnostic>,
|
||||
) -> Result<String, Error> {
|
||||
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<String, TemplateError> {
|
||||
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<String, toml::Value>,
|
||||
|
|
@ -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<TemplateError> {
|
||||
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<String, AttrValue>,
|
||||
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");
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue