mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-07 08:27:12 +00:00
Limit DOT templates to prompt + goal (#474)
# 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) <noreply@anthropic.com>
This commit is contained in:
parent
3c6ac9e6e1
commit
911e080f3c
2 changed files with 176 additions and 35 deletions
|
|
@ -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<String, Error> {
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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<String, toml::Value>,
|
||||
|
|
@ -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:?}"
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue