From 74e2c3597c4d70c9cbade0b00b4aba967a3b4645 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 25 Aug 2026 18:54:06 -0400 Subject: [PATCH] Simplify model stylesheet template plumbing Apply cleanup review findings on the model stylesheet template branch: - Move the root-only stylesheet rule into visit_graph_references via a GraphPosition parameter, so the bundler and workflow-version stop re-implementing the entrypoint guard with duplicated match arms - Let ModelStylesheetTemplateTransform build its own template store and skip the pass entirely when the graph has no stylesheet; drop its dead Transform impl and the template_render_store re-export - Parse fix-message namespaces with the typed Namespace enum, share the vars/goal fix strings with script_interpolation_fix, and replace the attribute_name magic-string check with a restricted-namespace fix the stylesheet transform sets on its own render target - Drop template_render_store's content parameter; the store's render always overwrites it before rendering - Trim redundant tests and add a transform_options() helper in pipeline/validate.rs tests Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019FBHEs42qNHDeKmsqTSDSQ --- .../fabro-manifest/src/workflow_bundler.rs | 66 +++++---------- .../fabro-workflow-version/src/lib.rs | 19 ++--- .../fabro-workflow/src/pipeline/transform.rs | 80 ++++--------------- .../fabro-workflow/src/pipeline/validate.rs | 34 ++++---- .../src/transforms/file_inlining.rs | 7 +- .../fabro-workflow/src/transforms/import.rs | 1 - .../fabro-workflow/src/transforms/mod.rs | 1 - .../transforms/model_stylesheet_template.rs | 80 +++++++------------ .../src/transforms/variable_expansion.rs | 62 ++++++++------ lib/foundation/fabro-template/src/lib.rs | 4 +- .../fabro-template/src/static_reference.rs | 57 ++++++++++--- 11 files changed, 183 insertions(+), 228 deletions(-) diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 2fc7842e2..e1907c37b 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -8,9 +8,9 @@ use fabro_config::project::WorkflowLocation; use fabro_config::{EnvironmentDockerfileLayer, EnvironmentImageLayer, SettingsLayer}; use fabro_graphviz::parser; use fabro_template::{ - BundleTemplateStore, FilesystemTemplateStore, GraphReference, GraphReferenceError, - RecordingTemplateStore, TemplateContext, TemplateDependencyClosure, TemplateRenderMode, - TemplateSource, validate_static_reference, visit_graph_references, + BundleTemplateStore, FilesystemTemplateStore, GraphPosition, GraphReference, + GraphReferenceError, RecordingTemplateStore, TemplateContext, TemplateDependencyClosure, + TemplateRenderMode, TemplateSource, validate_static_reference, visit_graph_references, }; use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; @@ -87,7 +87,12 @@ impl<'a> WorkflowBundler<'a> { .ok_or_else(|| anyhow!("invalid manifest workflow config path: {}", config.path))?; self.collect_config_dockerfile(&config_path, &config.source, &mut files)?; } - self.collect_workflow_files(&scan, &mut files, &mut visited_imports, true)?; + self.collect_workflow_files( + &scan, + &mut files, + &mut visited_imports, + GraphPosition::Entrypoint, + )?; self.workflows .insert(dot_key.clone(), types::ManifestWorkflow { @@ -123,7 +128,7 @@ impl<'a> WorkflowBundler<'a> { workflow: &WorkflowScanInput, files: &mut HashMap, visited_imports: &mut HashSet, - collect_model_stylesheet: bool, + position: GraphPosition, ) -> Result<()> { let graph = parser::parse(&workflow.source) .with_context(|| format!("Failed to parse {}", workflow.absolute_dot_path.display()))?; @@ -138,7 +143,7 @@ impl<'a> WorkflowBundler<'a> { let mut imports = Vec::new(); let mut children = Vec::new(); - visit_graph_references(&graph, |reference| -> Result<()> { + visit_graph_references(&graph, position, |reference| -> Result<()> { match reference { GraphReference::GoalFile { reference } => { let bundled = self.collect_bundled_file( @@ -152,17 +157,9 @@ impl<'a> WorkflowBundler<'a> { self.collect_bundled_template_includes(files, &bundled, &workflow_template_root) } GraphReference::GoalInline { content } - | GraphReference::InlinePrompt { content } => self.collect_template_include_files( - files, - TemplateSource::new( - workflow.dot_path.clone(), - workflow_template_root.clone(), - content.to_owned(), - ), - Some(&workflow.dot_path), - ), - GraphReference::ModelStylesheetInline { content } if collect_model_stylesheet => { - self.collect_template_include_files( + | GraphReference::InlinePrompt { content } + | GraphReference::ModelStylesheetInline { content } => self + .collect_template_include_files( files, TemplateSource::new( workflow.dot_path.clone(), @@ -170,9 +167,7 @@ impl<'a> WorkflowBundler<'a> { content.to_owned(), ), Some(&workflow.dot_path), - ) - } - GraphReference::ModelStylesheetInline { .. } => Ok(()), + ), GraphReference::FileInline { key, reference } => { let bundled = self.collect_bundled_file( files, @@ -225,7 +220,12 @@ impl<'a> WorkflowBundler<'a> { dot_path: imported.path, source: imported_source, }; - self.collect_workflow_files(&imported_scan, files, visited_imports, false)?; + self.collect_workflow_files( + &imported_scan, + files, + visited_imports, + GraphPosition::Imported, + )?; } } for child in children { @@ -520,30 +520,6 @@ mod tests { files["styles/nested.css"].content, "* { reasoning_effort: low; }" ); - - let store = BundleTemplateStore::new( - files - .iter() - .map(|(path, entry)| { - ( - ManifestPath::from_wire(path).expect("bundled path should parse"), - entry.content.clone(), - ) - }) - .collect(), - ); - let rendered = fabro_template::render_source( - &TemplateSource::new( - ManifestPath::from_wire("workflow.fabro").unwrap(), - ManifestPath::from_wire(".").unwrap(), - "{% include 'styles/base.css' %}", - ), - &TemplateContext::new().for_model_stylesheet(), - Arc::new(store), - TemplateRenderMode::Strict, - ) - .expect("bundled stylesheet should render"); - assert_eq!(rendered, "* { reasoning_effort: low; }"); } #[test] diff --git a/lib/components/fabro-workflow-version/src/lib.rs b/lib/components/fabro-workflow-version/src/lib.rs index 7b6524a79..ff0bd1e1a 100644 --- a/lib/components/fabro-workflow-version/src/lib.rs +++ b/lib/components/fabro-workflow-version/src/lib.rs @@ -14,7 +14,7 @@ use fabro_config::{ }; use fabro_graphviz::parser; use fabro_template::{ - BundleTemplateStore, GraphReference, GraphReferenceError, StaticReferenceError, + BundleTemplateStore, GraphPosition, GraphReference, GraphReferenceError, StaticReferenceError, TemplateDiscoveryError, TemplateSource, discover_static_dependency_closure, validate_static_reference, visit_graph_references, }; @@ -278,9 +278,13 @@ fn validate_graph_closure( path: path.clone(), source, })?; - let is_entrypoint = &path == version.entrypoint(); + let position = if &path == version.entrypoint() { + GraphPosition::Entrypoint + } else { + GraphPosition::Imported + }; - visit_graph_references(&graph, |reference| match reference { + visit_graph_references(&graph, position, |reference| match reference { GraphReference::GoalFile { reference } => { let target = resolve_reference(&path, ReferenceKind::GraphGoalFile, reference)?; let content = @@ -288,15 +292,12 @@ fn validate_graph_closure( template_roots.push(&target, content); Ok(()) } - GraphReference::GoalInline { content } | GraphReference::InlinePrompt { content } => { + GraphReference::GoalInline { content } + | GraphReference::InlinePrompt { content } + | GraphReference::ModelStylesheetInline { content } => { template_roots.push(&path, content); Ok(()) } - GraphReference::ModelStylesheetInline { content } if is_entrypoint => { - template_roots.push(&path, content); - Ok(()) - } - GraphReference::ModelStylesheetInline { .. } => Ok(()), GraphReference::Import { reference } => { let target = resolve_reference(&path, ReferenceKind::Import, reference)?; require_file(version, &path, ReferenceKind::Import, target.clone())?; diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index 390fff2b9..542d52c43 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -5,7 +5,6 @@ use crate::error::Error; use crate::transforms::{ FileInliningTransform, ImportTransform, ModelStylesheetTemplateTransform, ScriptInterpolationTransform, StylesheetApplicationTransform, TemplateTransform, Transform, - template_render_store, }; /// TRANSFORM phase: apply built-in and custom transforms to a parsed graph. @@ -63,23 +62,23 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result low -> high -> exit - }"#; - let parsed = parse(dot).unwrap(); - let transformed = transform(parsed, &TransformOptions { - template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ - ( - "efforts".to_string(), - toml::Value::Array(vec![ - toml::Value::String("low".to_string()), - toml::Value::String("high".to_string()), - ]), - ), - ])), - ..transform_options() - }) - .unwrap(); - - assert_eq!( - transformed.graph.nodes["low"] - .attrs - .get("reasoning_effort") - .and_then(AttrValue::as_str), - Some("low") - ); - assert_eq!( - transformed.graph.nodes["high"] - .attrs - .get("reasoning_effort") - .and_then(AttrValue::as_str), - Some("high") - ); - } - #[test] fn transform_renders_model_stylesheet_static_include() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index 5123da35c..3c550f5df 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -37,9 +37,8 @@ mod tests { use crate::pipeline::transform; use crate::pipeline::types::TransformOptions; - fn run_pipeline(dot: &str) -> Validated { - let parsed = parse(dot).unwrap(); - let transformed = transform::transform(parsed, &TransformOptions { + fn transform_options() -> TransformOptions { + TransformOptions { current_dir: None, file_resolver: None, template_context: fabro_template::TemplateContext::new(), @@ -47,8 +46,12 @@ mod tests { render_mode: crate::operations::RenderMode::Strict, custom_transforms: vec![], model_resolution: None, - }) - .unwrap(); + } + } + + fn run_pipeline(dot: &str) -> Validated { + let parsed = parse(dot).unwrap(); + let transformed = transform::transform(parsed, &transform_options()).unwrap(); validate(transformed, None, &[]) } @@ -118,18 +121,15 @@ mod tests { start -> exit }"#; let transformed = transform::transform(parse(dot).unwrap(), &TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ + template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ ( "declaration".to_string(), toml::Value::String("garbage garbage".to_string()), ), ])), - source_name: Some("workflow.fabro".to_string()), - render_mode: crate::operations::RenderMode::Structural, - custom_transforms: vec![], - model_resolution: None, + source_name: Some("workflow.fabro".to_string()), + render_mode: crate::operations::RenderMode::Structural, + ..transform_options() }) .unwrap(); let validated = validate(transformed, None, &[]); @@ -153,13 +153,9 @@ mod tests { start -> exit }"#; let transformed = transform::transform(parse(dot).unwrap(), &TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new(), - source_name: Some("workflow.fabro".to_string()), - render_mode: crate::operations::RenderMode::Structural, - custom_transforms: vec![], - model_resolution: None, + source_name: Some("workflow.fabro".to_string()), + render_mode: crate::operations::RenderMode::Structural, + ..transform_options() }) .unwrap(); let validated = validate(transformed, None, &[]); diff --git a/lib/components/fabro-workflow/src/transforms/file_inlining.rs b/lib/components/fabro-workflow/src/transforms/file_inlining.rs index ff599824b..565edf7d8 100644 --- a/lib/components/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/components/fabro-workflow/src/transforms/file_inlining.rs @@ -24,13 +24,14 @@ pub(crate) fn template_render_store( current_dir: &Path, resolver: Arc, source_name: Option<&str>, - content: &str, ) -> Result { let root = template_root_for_current_dir(current_dir)?; let source_path = template_source_path_for_current_dir(current_dir, source_name, &root)?; let base_dir = template_store_base_dir(current_dir); + // The store's render substitutes the text being rendered, so the source + // carries only its path and template root. Ok(TemplateRenderStore::new( - TemplateSource::new(source_path, root, content.to_owned()), + TemplateSource::new(source_path, root, String::new()), Arc::new(FileResolverTemplateStore::new(base_dir, resolver)), )) } @@ -184,7 +185,6 @@ impl FileInliningTransform { &self.current_dir, Arc::clone(&self.resolver), self.source_name.as_deref(), - &attr_value, )?); let rendered = render_template_for_target( &attr_value, @@ -232,7 +232,6 @@ impl FileInliningTransform { &self.current_dir, Arc::clone(&self.resolver), self.source_name.as_deref(), - goal, )?); let rendered = render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics)?; diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index 6433a6c1b..6317f1778 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -686,7 +686,6 @@ impl ImportTransform { &self.current_dir, Arc::clone(&self.resolver), self.source_name.as_deref(), - graph.goal(), )?); let parent_goal = render_template_for_target( graph.goal(), diff --git a/lib/components/fabro-workflow/src/transforms/mod.rs b/lib/components/fabro-workflow/src/transforms/mod.rs index 86a400055..d9ec04868 100644 --- a/lib/components/fabro-workflow/src/transforms/mod.rs +++ b/lib/components/fabro-workflow/src/transforms/mod.rs @@ -19,7 +19,6 @@ mod stylesheet_application; pub mod variable_expansion; pub use file_inlining::FileInliningTransform; -pub(crate) use file_inlining::template_render_store; pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub(crate) use model_stylesheet_template::ModelStylesheetTemplateTransform; diff --git a/lib/components/fabro-workflow/src/transforms/model_stylesheet_template.rs b/lib/components/fabro-workflow/src/transforms/model_stylesheet_template.rs index ff213e249..7071091d4 100644 --- a/lib/components/fabro-workflow/src/transforms/model_stylesheet_template.rs +++ b/lib/components/fabro-workflow/src/transforms/model_stylesheet_template.rs @@ -1,50 +1,33 @@ +use std::path::PathBuf; +use std::sync::Arc; + use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_template::TemplateContext; use fabro_validate::Diagnostic; -use super::Transform; +use super::file_inlining::template_render_store; use super::variable_expansion::{ - RenderMode, TemplateRenderOutcome, TemplateRenderStore, TemplateRenderTarget, - render_template_for_target_outcome, + RenderMode, TemplateRenderOutcome, TemplateRenderTarget, render_template_for_target_outcome, }; use crate::error::Error; +use crate::file_resolver::FileResolver; /// Renders the root graph's `model_stylesheet` with its restricted template /// context after imports are expanded and before stylesheet parsing. pub(crate) struct ModelStylesheetTemplateTransform { - context: TemplateContext, - source_name: Option, - source_text: Option, - render_mode: RenderMode, - template_store: Option, + pub context: TemplateContext, + pub source_name: Option, + pub source_text: Option, + pub render_mode: RenderMode, + /// Enables `{% include %}` resolution; without it the stylesheet renders + /// from its inline text alone. + pub file_resolution: Option<(PathBuf, Arc)>, } impl ModelStylesheetTemplateTransform { - #[must_use] - pub(crate) fn new( - context: TemplateContext, - source_name: Option, - source_text: Option, - render_mode: RenderMode, - ) -> Self { - Self { - context, - source_name, - source_text, - render_mode, - template_store: None, - } - } - - #[must_use] - pub(crate) fn with_template_store(mut self, template_store: TemplateRenderStore) -> Self { - self.template_store = Some(template_store); - self - } - pub(crate) fn apply_with_diagnostics( &self, - graph: Graph, + mut graph: Graph, ) -> Result<(Graph, Vec), Error> { let stylesheet = graph.model_stylesheet(); if stylesheet.is_empty() { @@ -53,9 +36,17 @@ impl ModelStylesheetTemplateTransform { let mut target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "model_stylesheet") - .with_source_origin(self.source_text.as_deref(), stylesheet); - if let Some(template_store) = self.template_store.clone() { - target = target.with_template_store(template_store); + .with_source_origin(self.source_text.as_deref(), stylesheet) + .with_restricted_namespace_fix( + "`model_stylesheet` templates expose only `inputs` and `vars`; use one of \ + those values or a MiniJinja local value", + ); + if let Some((current_dir, resolver)) = &self.file_resolution { + target = target.with_template_store(template_render_store( + current_dir, + Arc::clone(resolver), + self.source_name.as_deref(), + )?); } let mut diagnostics = Vec::new(); @@ -73,7 +64,6 @@ impl ModelStylesheetTemplateTransform { TemplateRenderOutcome::Unresolved => String::new(), }; - let mut graph = graph; graph .attrs .insert("model_stylesheet".to_string(), AttrValue::String(rendered)); @@ -81,17 +71,6 @@ impl ModelStylesheetTemplateTransform { } } -impl Transform for ModelStylesheetTemplateTransform { - fn apply(&self, graph: Graph) -> Result { - let (graph, diagnostics) = self.apply_with_diagnostics(graph)?; - if diagnostics.is_empty() { - Ok(graph) - } else { - Err(Error::ValidationFailed { diagnostics }) - } - } -} - #[cfg(test)] mod tests { use std::collections::HashMap; @@ -115,14 +94,15 @@ mod tests { stylesheet: &str, render_mode: RenderMode, ) -> Result<(Graph, Vec), Error> { - ModelStylesheetTemplateTransform::new( + ModelStylesheetTemplateTransform { context, - Some("workflow.fabro".to_string()), - Some(format!( + source_name: Some("workflow.fabro".to_string()), + source_text: Some(format!( "digraph Test {{ graph [model_stylesheet=\"{stylesheet}\"] }}" )), render_mode, - ) + file_resolution: None, + } .apply_with_diagnostics(graph_with_stylesheet(stylesheet)) } diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index 2f1c1d124..a5872b2b3 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -38,13 +38,15 @@ pub enum RenderMode { #[derive(Clone)] pub(crate) struct TemplateRenderTarget { - pub source_name: Option, - pub node_id: Option, - pub edge: Option<(String, String)>, - pub owner: String, - attribute_name: String, - source_origin: Option, - template_store: Option, + pub source_name: Option, + pub node_id: Option, + pub edge: Option<(String, String)>, + pub owner: String, + /// Fix text for undefined variables outside `inputs`/`vars`, set by + /// targets whose template context is a restricted projection. + restricted_namespace_fix: Option, + source_origin: Option, + template_store: Option, } #[derive(Clone)] @@ -84,7 +86,7 @@ impl TemplateRenderTarget { node_id: None, edge: None, owner: format!("graph attribute `{attr_name}`"), - attribute_name: attr_name, + restricted_namespace_fix: None, source_origin: None, template_store: None, } @@ -103,7 +105,7 @@ impl TemplateRenderTarget { node_id: Some(node_id.clone()), edge: None, owner: format!("node `{node_id}` attribute `{attr_name}`"), - attribute_name: attr_name, + restricted_namespace_fix: None, source_origin: None, template_store: None, } @@ -124,7 +126,7 @@ impl TemplateRenderTarget { node_id: None, edge: Some((from.clone(), to.clone())), owner: format!("edge `{from} -> {to}` attribute `{attr_name}`"), - attribute_name: attr_name, + restricted_namespace_fix: None, source_origin: None, template_store: None, } @@ -150,6 +152,12 @@ impl TemplateRenderTarget { self } + #[must_use] + pub(crate) fn with_restricted_namespace_fix(mut self, fix: impl Into) -> Self { + self.restricted_namespace_fix = Some(fix.into()); + self + } + #[must_use] fn template_source_name(&self) -> String { self.source_name @@ -261,21 +269,21 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> fn template_variable_fix(expression: Option<&str>, target: &TemplateRenderTarget) -> String { let mut parts = expression.unwrap_or_default().split('.'); - let namespace = parts.next().unwrap_or_default(); + let namespace = parts.next().unwrap_or_default().parse::(); let name = parts.next().unwrap_or(""); - match namespace { - "inputs" => input_binding_fix(name), - "vars" => format!("set it with `fabro variable set {name} `"), - _ if target.attribute_name == "model_stylesheet" => { - "`model_stylesheet` templates expose only `inputs` and `vars`; use one of those values or a MiniJinja local value" - .to_string() + match (namespace, &target.restricted_namespace_fix) { + (Ok(Namespace::Inputs), _) => input_binding_fix(name), + (Ok(Namespace::Vars), _) => variable_binding_fix(name), + (_, Some(fix)) => fix.clone(), + (Ok(Namespace::Goal), None) => GOAL_BINDING_FIX.to_string(), + (Ok(namespace), None) => format!("`{namespace}` is not available in workflow templates"), + (Err(_), None) => { + format!( + "define `{}` in the template context", + expression.unwrap_or("the value") + ) } - "goal" => "set a graph `goal` on the workflow".to_string(), - "env" | "secrets" => { - format!("`{namespace}` is not available in workflow templates") - } - _ => format!("define `{}` in the template context", expression.unwrap_or("the value")), } } @@ -283,6 +291,12 @@ fn input_binding_fix(name: &str) -> String { format!("bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`") } +fn variable_binding_fix(name: &str) -> String { + format!("set it with `fabro variable set {name} `") +} + +const GOAL_BINDING_FIX: &str = "set a graph `goal` on the workflow"; + /// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in one /// command node `script`. /// @@ -395,7 +409,7 @@ fn script_interpolation_fix(err: &ResolveError, language: Option<&str>) -> Strin let name = &err.name; match err.namespace { Namespace::Inputs => input_binding_fix(name), - Namespace::Vars => format!("set it with `fabro variable set {name} `"), + Namespace::Vars => variable_binding_fix(name), Namespace::Env if language == Some("python") => format!( "`script` does not interpolate environment variables; read it in Python as \ `os.environ[\"{name}\"]` instead" @@ -412,7 +426,7 @@ fn script_interpolation_fix(err: &ResolveError, language: Option<&str>) -> Strin "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ `[environments..env]` and read it in the shell as `${name}`" ), - Namespace::Goal => "set a graph `goal` on the workflow".to_string(), + Namespace::Goal => GOAL_BINDING_FIX.to_string(), } } diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index 27cf3e71a..0c3c4ba78 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -17,8 +17,8 @@ pub use dependency::{ extract_template_dependencies, }; pub use static_reference::{ - GraphReference, GraphReferenceError, StaticReferenceError, validate_static_reference, - visit_graph_references, + GraphPosition, GraphReference, GraphReferenceError, StaticReferenceError, + validate_static_reference, visit_graph_references, }; pub use store::{ BundleTemplateStore, CachedTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, diff --git a/lib/foundation/fabro-template/src/static_reference.rs b/lib/foundation/fabro-template/src/static_reference.rs index a15deb6ad..6dc2a310d 100644 --- a/lib/foundation/fabro-template/src/static_reference.rs +++ b/lib/foundation/fabro-template/src/static_reference.rs @@ -68,11 +68,11 @@ pub enum GraphReference<'graph> { GoalFile { reference: &'graph str }, /// A non-`@` graph `goal`: inline template content. GoalInline { content: &'graph str }, - /// The root graph's inline `model_stylesheet` template content. + /// The entrypoint graph's inline `model_stylesheet` template content. /// - /// Consumers that recurse through imported graphs decide whether the - /// visited graph is the workflow entrypoint before treating this as a - /// template root. + /// Emitted only when the walked graph is [`GraphPosition::Entrypoint`]; + /// imported stylesheets are ignored at runtime, so they are never + /// template roots. ModelStylesheetInline { content: &'graph str }, /// `node [import=""]` — another graph file to walk. Import { reference: &'graph str }, @@ -97,13 +97,28 @@ pub enum GraphReferenceError { Visit(E), } +/// Whether the walked graph is the workflow's entrypoint or was reached +/// through an `import`/`stack.child_workflow` reference. +/// +/// Position-dependent reference semantics (today: `model_stylesheet` is a +/// template root only on the entrypoint) live in the walker, so every +/// consumer applies the same rule. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum GraphPosition { + Entrypoint, + Imported, +} + /// Walk every static file reference and inline template in one parsed graph, /// validating that file references are template-free before emitting them. /// /// The walker covers a single graph; recursion into `Import` targets and /// resolution of references against a file source are the consumer's job. +/// `position` tells the walker whether this graph is the workflow entrypoint, +/// which gates position-dependent references such as `model_stylesheet`. pub fn visit_graph_references<'graph, E>( graph: &'graph Graph, + position: GraphPosition, mut visit: impl FnMut(GraphReference<'graph>) -> Result<(), E>, ) -> Result<(), GraphReferenceError> { let goal = graph.goal(); @@ -119,7 +134,7 @@ pub fn visit_graph_references<'graph, E>( } let model_stylesheet = graph.model_stylesheet(); - if !model_stylesheet.is_empty() { + if position == GraphPosition::Entrypoint && !model_stylesheet.is_empty() { visit(GraphReference::ModelStylesheetInline { content: model_stylesheet, }) @@ -166,7 +181,7 @@ mod tests { use fabro_types::graph::{AttrValue, Graph, Node, ReferenceKind}; - use super::{GraphReference, GraphReferenceError, validate_static_reference}; + use super::{GraphPosition, GraphReference, GraphReferenceError, validate_static_reference}; #[test] fn static_reference_rejects_template_syntax() { @@ -221,6 +236,7 @@ mod tests { let mut seen = BTreeSet::new(); super::visit_graph_references( &graph, + GraphPosition::Entrypoint, |reference| -> Result<(), std::convert::Infallible> { seen.insert(match reference { GraphReference::GoalFile { reference } => format!("goal-file:{reference}"), @@ -253,6 +269,24 @@ mod tests { ); } + #[test] + fn imported_graphs_do_not_emit_model_stylesheet() { + let mut graph = Graph::new("test"); + graph.attrs.insert( + "model_stylesheet".to_string(), + AttrValue::String("* { reasoning_effort: low; }".to_string()), + ); + + super::visit_graph_references( + &graph, + GraphPosition::Imported, + |reference| -> Result<(), std::convert::Infallible> { + panic!("imported graph emitted {reference:?}") + }, + ) + .unwrap(); + } + #[test] fn rejects_template_syntax_in_references_before_visiting() { let mut graph = Graph::new("test"); @@ -261,11 +295,14 @@ mod tests { node_with("imported", &[("import", "graphs/{{ name }}.fabro")]), ); - let error = - super::visit_graph_references(&graph, |_| -> Result<(), std::convert::Infallible> { + let error = super::visit_graph_references( + &graph, + GraphPosition::Entrypoint, + |_| -> Result<(), std::convert::Infallible> { panic!("references with template syntax must not be visited") - }) - .unwrap_err(); + }, + ) + .unwrap_err(); assert!(matches!(error, GraphReferenceError::StaticReference(_))); } }