mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FBHEs42qNHDeKmsqTSDSQ
This commit is contained in:
parent
a522414bdc
commit
74e2c3597c
11 changed files with 183 additions and 228 deletions
|
|
@ -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<String, types::ManifestFileEntry>,
|
||||
visited_imports: &mut HashSet<String>,
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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())?;
|
||||
|
|
|
|||
|
|
@ -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<Transform
|
|||
}
|
||||
.apply_with_diagnostics(graph)?;
|
||||
diagnostics.extend(transform_diagnostics);
|
||||
let mut stylesheet_transform = ModelStylesheetTemplateTransform::new(
|
||||
options.template_context.clone(),
|
||||
options.source_name.clone(),
|
||||
Some(source.clone()),
|
||||
options.render_mode,
|
||||
);
|
||||
if let (Some(current_dir), Some(file_resolver)) = (&options.current_dir, &options.file_resolver)
|
||||
{
|
||||
stylesheet_transform = stylesheet_transform.with_template_store(template_render_store(
|
||||
current_dir,
|
||||
Arc::clone(file_resolver),
|
||||
options.source_name.as_deref(),
|
||||
graph.model_stylesheet(),
|
||||
)?);
|
||||
}
|
||||
let (graph, transform_diagnostics) = stylesheet_transform.apply_with_diagnostics(graph)?;
|
||||
diagnostics.extend(transform_diagnostics);
|
||||
let graph = if graph.model_stylesheet().is_empty() {
|
||||
graph
|
||||
} else {
|
||||
let (graph, transform_diagnostics) = ModelStylesheetTemplateTransform {
|
||||
context: options.template_context.clone(),
|
||||
source_name: options.source_name.clone(),
|
||||
source_text: Some(source.clone()),
|
||||
render_mode: options.render_mode,
|
||||
file_resolution: options
|
||||
.current_dir
|
||||
.clone()
|
||||
.zip(options.file_resolver.clone()),
|
||||
}
|
||||
.apply_with_diagnostics(graph)?;
|
||||
diagnostics.extend(transform_diagnostics);
|
||||
graph
|
||||
};
|
||||
let (graph, transform_diagnostics) = ScriptInterpolationTransform {
|
||||
context: options.template_context.clone(),
|
||||
source_name: options.source_name.clone(),
|
||||
|
|
@ -251,51 +250,6 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn transform_applies_rules_emitted_by_model_stylesheet_loop() {
|
||||
let dot = r#"digraph Test {
|
||||
graph [model_stylesheet="
|
||||
{% for effort in inputs.efforts %}
|
||||
.tier-{{ loop.index }} { reasoning_effort: {{ effort }}; }
|
||||
{% endfor %}
|
||||
"]
|
||||
start [shape=Mdiamond]
|
||||
low [prompt="Low", class="tier-1"]
|
||||
high [prompt="High", class="tier-2"]
|
||||
exit [shape=Msquare]
|
||||
start -> 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();
|
||||
|
|
|
|||
|
|
@ -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, &[]);
|
||||
|
|
|
|||
|
|
@ -24,13 +24,14 @@ pub(crate) fn template_render_store(
|
|||
current_dir: &Path,
|
||||
resolver: Arc<dyn FileResolver>,
|
||||
source_name: Option<&str>,
|
||||
content: &str,
|
||||
) -> Result<TemplateRenderStore, Error> {
|
||||
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)?;
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
source_text: Option<String>,
|
||||
render_mode: RenderMode,
|
||||
template_store: Option<TemplateRenderStore>,
|
||||
pub context: TemplateContext,
|
||||
pub source_name: Option<String>,
|
||||
pub source_text: Option<String>,
|
||||
pub render_mode: RenderMode,
|
||||
/// Enables `{% include %}` resolution; without it the stylesheet renders
|
||||
/// from its inline text alone.
|
||||
pub file_resolution: Option<(PathBuf, Arc<dyn FileResolver>)>,
|
||||
}
|
||||
|
||||
impl ModelStylesheetTemplateTransform {
|
||||
#[must_use]
|
||||
pub(crate) fn new(
|
||||
context: TemplateContext,
|
||||
source_name: Option<String>,
|
||||
source_text: Option<String>,
|
||||
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<Diagnostic>), 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<Graph, Error> {
|
||||
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<Diagnostic>), 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))
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -38,13 +38,15 @@ pub enum RenderMode {
|
|||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct TemplateRenderTarget {
|
||||
pub source_name: Option<String>,
|
||||
pub node_id: Option<String>,
|
||||
pub edge: Option<(String, String)>,
|
||||
pub owner: String,
|
||||
attribute_name: String,
|
||||
source_origin: Option<TemplateSourceOrigin>,
|
||||
template_store: Option<TemplateRenderStore>,
|
||||
pub source_name: Option<String>,
|
||||
pub node_id: Option<String>,
|
||||
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<String>,
|
||||
source_origin: Option<TemplateSourceOrigin>,
|
||||
template_store: Option<TemplateRenderStore>,
|
||||
}
|
||||
|
||||
#[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<String>) -> 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::<Namespace>();
|
||||
let name = parts.next().unwrap_or("<name>");
|
||||
|
||||
match namespace {
|
||||
"inputs" => input_binding_fix(name),
|
||||
"vars" => format!("set it with `fabro variable set {name} <value>`"),
|
||||
_ 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}=<value>`")
|
||||
}
|
||||
|
||||
fn variable_binding_fix(name: &str) -> String {
|
||||
format!("set it with `fabro variable set {name} <value>`")
|
||||
}
|
||||
|
||||
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} <value>`"),
|
||||
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.<slug>.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(),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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="<reference>"]` — another graph file to walk.
|
||||
Import { reference: &'graph str },
|
||||
|
|
@ -97,13 +97,28 @@ pub enum GraphReferenceError<E> {
|
|||
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<E>> {
|
||||
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(_)));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue