From 492aba7fff6370a49a4025d8246e25167ae4bc60 Mon Sep 17 00:00:00 2001 From: Aleksi Asikainen <1086393+salieri@users.noreply.github.com> Date: Sun, 17 May 2026 19:15:45 -0700 Subject: [PATCH] fix: MiniJinja can't find partials (#301) Fixes an issue where use of MiniJinja [`include`](https://jinja.palletsprojects.com/en/stable/templates/#include) control structure (`{% include "filename.ext" %}`) causes a render error `template not found: tried to include non-existing template "filename.ext"` ### Example broken diagram ``` dot digraph ValidatePlan { start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] test_inline_prompt [label="moo" prompt="{% include 'test.tpl.md' %}"] // ^^^^^^^^^^^^^^^^^^^^^^^^^ start -> test_inline_prompt -> exit } ``` ### Fix The core issue was that template rendering knew the source name for diagnostics, but did not have a loader rooted at the prompt/goal file location. Includes therefore failed even when the included file existed next to the rendered file. The fix adds optional loader support to `fabro-template`, then wires workflow rendering to the existing `FileResolver` so includes resolve relative to the file currently being rendered. For `fabro validate`, there was a second manifest-specific problem: validation runs through a bundled manifest, and the manifest builder only bundled explicit `prompt.md` / `goal.md` files, not static MiniJinja `include` dependencies inside those files. The manifest builder now scans prompt/goal template text for literal `{% include "file" %}` / `{% include 'file' %}` references and bundles those files too. Missing or unsafe include names are left for MiniJinja/runtime validation rather than expanding scope. (For clarity: The fix does not support variables or arrays in `include`.) --- Cargo.lock | 2 + Cargo.toml | 2 +- lib/crates/fabro-cli/tests/it/cmd/validate.rs | 35 ++ .../tests/manifest_path_round_trip.rs | 2 +- lib/crates/fabro-manifest/Cargo.toml | 1 + lib/crates/fabro-manifest/src/lib.rs | 338 +++++++++++++- lib/crates/fabro-server/src/run_manifest.rs | 12 +- lib/crates/fabro-template/Cargo.toml | 3 +- lib/crates/fabro-template/src/dependency.rs | 192 ++++++++ lib/crates/fabro-template/src/lib.rs | 437 +++++++++++++++++- lib/crates/fabro-template/src/store.rs | 287 ++++++++++++ lib/crates/fabro-types/src/lib.rs | 2 + .../src/manifest_path.rs | 43 ++ .../fabro-workflow/src/file_resolver.rs | 2 +- lib/crates/fabro-workflow/src/lib.rs | 3 +- .../fabro-workflow/src/operations/create.rs | 105 ++++- .../fabro-workflow/src/operations/start.rs | 6 +- .../fabro-workflow/src/pipeline/types.rs | 3 +- lib/crates/fabro-workflow/src/services.rs | 2 +- .../src/transforms/file_inlining.rs | 113 ++++- .../fabro-workflow/src/transforms/import.rs | 7 +- .../src/transforms/variable_expansion.rs | 35 +- .../fabro-workflow/src/workflow_bundle.rs | 2 +- .../missing_dependency/workflow.fabro | 9 + .../static_dependencies/inline.tpl.md | 1 + .../static_dependencies/prompts/goal.md | 1 + .../static_dependencies/prompts/goal.tpl.md | 1 + .../static_dependencies/prompts/prompt.md | 1 + .../static_dependencies/prompts/prompt.tpl.md | 1 + .../static_dependencies/workflow.fabro | 10 + 30 files changed, 1597 insertions(+), 61 deletions(-) create mode 100644 lib/crates/fabro-template/src/dependency.rs create mode 100644 lib/crates/fabro-template/src/store.rs rename lib/crates/{fabro-workflow => fabro-types}/src/manifest_path.rs (90%) create mode 100644 test/templates/missing_dependency/workflow.fabro create mode 100644 test/templates/static_dependencies/inline.tpl.md create mode 100644 test/templates/static_dependencies/prompts/goal.md create mode 100644 test/templates/static_dependencies/prompts/goal.tpl.md create mode 100644 test/templates/static_dependencies/prompts/prompt.md create mode 100644 test/templates/static_dependencies/prompts/prompt.tpl.md create mode 100644 test/templates/static_dependencies/workflow.fabro diff --git a/Cargo.lock b/Cargo.lock index d05273d9d..dd45f7eee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2081,6 +2081,7 @@ dependencies = [ "fabro-config", "fabro-github", "fabro-graphviz", + "fabro-template", "fabro-types", "fabro-workflow", "git2", @@ -2416,6 +2417,7 @@ name = "fabro-template" version = "0.236.0-nightly.0" dependencies = [ "anyhow", + "fabro-types", "fabro-util", "miette", "minijinja", diff --git a/Cargo.toml b/Cargo.toml index 4cfb73dbe..ac224d899 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -79,7 +79,7 @@ slatedb = { version = "0.11.2", features = ["zstd"] } object_store = { version = "0.12.5", features = ["aws"] } rust-embed = "8" percent-encoding = "2" -minijinja = "2" +minijinja = "=2.19.0" miette = { version = "7.6", features = ["fancy"] } fabro-http = { path = "lib/crates/fabro-http" } fabro-options-metadata = { path = "lib/crates/fabro-options-metadata" } diff --git a/lib/crates/fabro-cli/tests/it/cmd/validate.rs b/lib/crates/fabro-cli/tests/it/cmd/validate.rs index 21bebd5fa..706bd1bf9 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/validate.rs @@ -206,6 +206,41 @@ fn bare_fabro_picks_up_sibling_workflow_toml_inputs() { "); } +#[test] +fn validate_accepts_static_template_dependencies() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("templates/static_dependencies/workflow.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + ----- stderr ----- + Workflow: TemplateIncludes (4 nodes, 3 edges) + Graph: [FIXTURES]/templates/static_dependencies/workflow.fabro + Validation: OK + "); +} + +#[test] +fn validate_reports_missing_template_dependency() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("templates/missing_dependency/workflow.fabro")); + let mut filters = context.filters(); + filters.push(( + r"(?:\.\./)*\.\.\[FIXTURES\]/".to_string(), + "[FIXTURES]/".to_string(), + )); + fabro_snapshot!(filters, cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + × failed to discover template dependencies: missing template dependency `missing.tpl.md` from `[FIXTURES]/templates/missing_dependency/workflow.fabro` + "); +} + #[test] fn invalid() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs index d94f95221..5e8f16cc5 100644 --- a/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs +++ b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs @@ -6,7 +6,7 @@ use std::path::PathBuf; use fabro_manifest::{ManifestBuildInput, build_run_manifest}; -use fabro_workflow::ManifestPath; +use fabro_types::ManifestPath; #[test] fn cli_built_manifest_resolves_user_global_at_path() { diff --git a/lib/crates/fabro-manifest/Cargo.toml b/lib/crates/fabro-manifest/Cargo.toml index 8a8de1fc2..417dc9af0 100644 --- a/lib/crates/fabro-manifest/Cargo.toml +++ b/lib/crates/fabro-manifest/Cargo.toml @@ -18,6 +18,7 @@ fabro-api = { path = "../fabro-api" } fabro-config = { path = "../fabro-config" } fabro-github = { path = "../fabro-github" } fabro-graphviz = { path = "../fabro-graphviz" } +fabro-template = { path = "../fabro-template" } fabro-types = { path = "../fabro-types" } fabro-workflow = { path = "../fabro-workflow" } git2.workspace = true diff --git a/lib/crates/fabro-manifest/src/lib.rs b/lib/crates/fabro-manifest/src/lib.rs index 17d931964..f1bde4e0d 100644 --- a/lib/crates/fabro-manifest/src/lib.rs +++ b/lib/crates/fabro-manifest/src/lib.rs @@ -5,6 +5,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; +use std::sync::Arc; use anyhow::{Context, Result, anyhow}; use fabro_api::types; @@ -16,10 +17,15 @@ use fabro_config::{ }; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; +use fabro_template::{ + BundleTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, TemplateContext, + TemplateRenderMode, TemplateSource, discover_static_dependency_closure, render_source, +}; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; -use fabro_types::{DirtyStatus, GitContext, PreRunPushOutcome, RunId, WorkflowSettings}; -use fabro_workflow::ManifestPath; +use fabro_types::{ + DirtyStatus, GitContext, ManifestPath, PreRunPushOutcome, RunId, WorkflowSettings, +}; use fabro_workflow::git::{ GitSyncStatus, branch_needs_push, head_sha, push_branch_noninteractive, sync_status, }; @@ -124,6 +130,7 @@ pub fn build_sparse_run_overrides(input: RunOverrideInput<'_>) -> Option { cwd: &'a Path, + inputs: HashMap, workflows: HashMap, visited_workflows: HashSet, } @@ -184,6 +191,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let mut context = CollectContext { cwd: &input.cwd, + inputs: workflow_settings.run.inputs.clone(), workflows: HashMap::new(), visited_workflows: HashSet::new(), }; @@ -325,49 +333,97 @@ fn collect_workflow_files( workflow.absolute_dot_path.display() ) })?; + let workflow_base_dir = workflow + .absolute_dot_path + .parent() + .unwrap_or_else(|| Path::new(".")); + let workflow_template_root = manifest_parent_or_dot(&workflow.dot_path)?; if let Some(goal_ref) = graph.attrs.get("goal").and_then(AttrValue::as_str) { if goal_ref.starts_with('@') { - collect_bundled_file( + let bundled = collect_bundled_file( files, - workflow - .absolute_dot_path - .parent() - .unwrap_or_else(|| Path::new(".")), + workflow_base_dir, context.cwd, goal_ref.trim_start_matches('@'), types::ManifestFileRefType::FileInline, manifest_attr_reference_kind(AttributeScope::Graph, "goal", goal_ref)?, Some(workflow.dot_path.clone()), )?; + let source = std::fs::read_to_string(&bundled.absolute_path) + .with_context(|| format!("Failed to read {}", bundled.absolute_path.display()))?; + collect_template_include_files( + files, + context.cwd, + TemplateSource { + path: bundled.path.clone(), + content: source, + }, + &manifest_parent_or_dot(&bundled.path)?, + Some(&bundled.path), + &context.inputs, + )?; + } else { + collect_template_include_files( + files, + context.cwd, + TemplateSource { + path: workflow.dot_path.clone(), + content: goal_ref.to_owned(), + }, + &workflow_template_root, + Some(&workflow.dot_path), + &context.inputs, + )?; } } for node in graph.nodes.values() { if let Some(prompt_ref) = node.attrs.get("prompt").and_then(AttrValue::as_str) { if prompt_ref.starts_with('@') { - collect_bundled_file( + let bundled = collect_bundled_file( files, - workflow - .absolute_dot_path - .parent() - .unwrap_or_else(|| Path::new(".")), + workflow_base_dir, context.cwd, prompt_ref.trim_start_matches('@'), types::ManifestFileRefType::FileInline, manifest_attr_reference_kind(AttributeScope::Node, "prompt", prompt_ref)?, Some(workflow.dot_path.clone()), )?; + let source = + std::fs::read_to_string(&bundled.absolute_path).with_context(|| { + format!("Failed to read {}", bundled.absolute_path.display()) + })?; + collect_template_include_files( + files, + context.cwd, + TemplateSource { + path: bundled.path.clone(), + content: source, + }, + &manifest_parent_or_dot(&bundled.path)?, + Some(&bundled.path), + &context.inputs, + )?; + } else { + collect_template_include_files( + files, + context.cwd, + TemplateSource { + path: workflow.dot_path.clone(), + content: prompt_ref.to_owned(), + }, + &workflow_template_root, + Some(&workflow.dot_path), + &context.inputs, + )?; } } if let Some(import_ref) = node.attrs.get("import").and_then(AttrValue::as_str) { let imported = collect_bundled_file( files, - workflow - .absolute_dot_path - .parent() - .unwrap_or_else(|| Path::new(".")), + workflow_base_dir, context.cwd, import_ref, types::ManifestFileRefType::Import, @@ -397,20 +453,90 @@ fn collect_workflow_files( manifest_attr_reference_kind(AttributeScope::Node, "stack.child_workflow", child_ref)? .validate(child_ref) .map_err(anyhow::Error::new)?; - collect_workflow_entry( - context, - Path::new(child_ref), - workflow - .absolute_dot_path - .parent() - .unwrap_or_else(|| Path::new(".")), - )?; + collect_workflow_entry(context, Path::new(child_ref), workflow_base_dir)?; } } Ok(()) } +fn collect_template_include_files( + files: &mut HashMap, + cwd: &Path, + source: TemplateSource, + template_root: &ManifestPath, + from: Option<&ManifestPath>, + inputs: &HashMap, +) -> Result<()> { + let source_path = source.path.clone(); + let store = FilesystemTemplateStore::new(cwd.to_path_buf(), template_root.clone()); + let closure = discover_static_dependency_closure([source], &store) + .map_err(|err| anyhow!("failed to discover template dependencies: {err}"))?; + verify_recorded_template_dependencies( + &source_path, + &closure, + template_root, + files, + from, + inputs, + )?; + + for (path, source) in closure.sources { + if path == source_path { + continue; + } + let key = path.to_string(); + files + .entry(key) + .or_insert_with(|| types::ManifestFileEntry { + content: source.content, + ref_: types::ManifestFileRef { + from: from.map(std::string::ToString::to_string), + original: path.to_string(), + type_: types::ManifestFileRefType::FileInline, + }, + }); + } + Ok(()) +} + +fn verify_recorded_template_dependencies( + source_path: &ManifestPath, + closure: &fabro_template::TemplateDependencyClosure, + template_root: &ManifestPath, + files: &HashMap, + from: Option<&ManifestPath>, + inputs: &HashMap, +) -> Result<()> { + let Some(source) = closure.sources.get(source_path) else { + return Ok(()); + }; + let mut bundled_files = closure + .sources + .iter() + .map(|(path, source)| (path.clone(), source.content.clone())) + .collect::>(); + for (path, entry) in files { + if let Some(path) = ManifestPath::from_wire(path) { + bundled_files.insert(path, entry.content.clone()); + } + } + let allowed = bundled_files.keys().cloned().collect(); + let store = RecordingTemplateStore::with_allowed( + BundleTemplateStore::new(template_root.clone(), bundled_files), + allowed, + ); + let ctx = TemplateContext::for_input_scan(inputs.clone()); + render_source(source, &ctx, Arc::new(store), TemplateRenderMode::Lenient).with_context( + || { + let from = + from.map_or_else(|| source_path.to_string(), std::string::ToString::to_string); + format!("failed to verify template dependencies for {from}") + }, + )?; + Ok(()) +} + fn manifest_attr_reference_kind( scope: AttributeScope, key: &str, @@ -704,6 +830,12 @@ fn manifest_path_from_absolute(path: &Path, cwd: &Path) -> Result .ok_or_else(|| anyhow!("Failed to compute manifest path for {}", path.display())) } +fn manifest_parent_or_dot(path: &ManifestPath) -> Result { + let parent = path.parent_or_dot().to_string_lossy(); + ManifestPath::from_wire(&parent) + .ok_or_else(|| anyhow!("invalid manifest parent path for {path}: {parent}")) +} + pub fn manifest_args_is_empty(args: &types::ManifestArgs) -> bool { args.auto_approve.is_none() && args.dry_run.is_none() @@ -865,6 +997,164 @@ mod tests { ); } + #[test] + fn build_manifest_bundles_static_minijinja_includes_from_prompts_and_goals() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join(".fabro/workflows/demo"); + std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); + std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r#"digraph Demo { + graph [goal="@prompts/goal.md"] + start [shape=Mdiamond] + exit [shape=Msquare] + file_prompt [prompt="@prompts/plan.md"] + inline_prompt [prompt="{% include 'inline.tpl.md' %}"] + start -> file_prompt -> inline_prompt -> exit + }"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("prompts/goal.md"), + r#"{% include "goal.tpl.md" %}"#, + ) + .unwrap(); + std::fs::write(workflow_dir.join("prompts/goal.tpl.md"), "ship it").unwrap(); + std::fs::write( + workflow_dir.join("prompts/plan.md"), + r#"{% include "plan.tpl.md" %}"#, + ) + .unwrap(); + std::fs::write(workflow_dir.join("prompts/plan.tpl.md"), "plan it").unwrap(); + std::fs::write(workflow_dir.join("inline.tpl.md"), "inline it").unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() + }) + .unwrap(); + + let root = &built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"]; + assert!( + root.files + .contains_key(".fabro/workflows/demo/prompts/goal.tpl.md") + ); + assert!( + root.files + .contains_key(".fabro/workflows/demo/prompts/plan.tpl.md") + ); + assert!( + root.files + .contains_key(".fabro/workflows/demo/inline.tpl.md") + ); + } + + #[test] + fn build_manifest_bundles_static_minijinja_includes_from_all_branches_and_macros() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join(".fabro/workflows/demo"); + std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); + std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r#"digraph Demo { + graph [goal="ship"] + start [shape=Mdiamond] + exit [shape=Msquare] + work [prompt="@prompts/plan.md"] + start -> work -> exit + }"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("prompts/plan.md"), + r#"{% if inputs.use_a %}{% include "a.md" %}{% else %}{% include "b.md" %}{% endif %} +{% from "helpers.md" import render_advanced_prompt %}"#, + ) + .unwrap(); + std::fs::write(workflow_dir.join("prompts/a.md"), "A").unwrap(); + std::fs::write(workflow_dir.join("prompts/b.md"), "B").unwrap(); + std::fs::write( + workflow_dir.join("prompts/helpers.md"), + r#"{% macro render_advanced_prompt() %}{% include "advanced.md" %}{% endmacro %}"#, + ) + .unwrap(); + std::fs::write(workflow_dir.join("prompts/advanced.md"), "advanced").unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() + }) + .unwrap(); + + let root = &built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"]; + for path in [ + ".fabro/workflows/demo/prompts/a.md", + ".fabro/workflows/demo/prompts/b.md", + ".fabro/workflows/demo/prompts/helpers.md", + ".fabro/workflows/demo/prompts/advanced.md", + ] { + assert!(root.files.contains_key(path), "missing {path}"); + } + } + + #[test] + fn build_manifest_rejects_dynamic_minijinja_include_discovery() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join(".fabro/workflows/demo"); + std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); + std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r#"digraph Demo { + graph [goal="ship"] + start [shape=Mdiamond] + exit [shape=Msquare] + work [prompt="@prompts/plan.md"] + start -> work -> exit + }"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("prompts/plan.md"), + r"{% include inputs.partial %}", + ) + .unwrap(); + + let err = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() + }) + .unwrap_err(); + + assert!( + err.to_string().contains("dynamic template dependency"), + "unexpected error: {err:#}" + ); + } + #[test] fn build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index fc66cc401..133482cca 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -30,14 +30,14 @@ use fabro_types::settings::run::{ DaytonaNetworkLayer, DaytonaSettings, DockerSettings, DockerfileSource, RunGoal, RunMode, RunNamespace, }; -use fabro_types::{RunId, WorkflowSettings}; +use fabro_types::{ManifestPath, RunId, WorkflowSettings}; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; +use fabro_workflow::Error as WorkflowError; use fabro_workflow::operations::{CreateRunInput, ValidateInput, WorkflowInput, validate}; use fabro_workflow::pipeline::Validated; use fabro_workflow::run_materialization::materialize_run; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; -use fabro_workflow::{Error as WorkflowError, ManifestPath}; use futures_util::stream::{self, StreamExt}; use tokio::process::Command; use tokio::time; @@ -137,7 +137,11 @@ pub(crate) fn prepare_manifest( .build() .context("failed to resolve manifest settings")?; settings.run.inputs.extend(args_overrides.input_overrides); - if let Some(goal) = manifest.goal.as_ref() { + if let Some(goal) = manifest + .goal + .as_ref() + .filter(|goal| goal.type_ != types::ManifestGoalType::Graph) + { settings.run.goal = Some(RunGoal::Inline(InterpString::parse(&goal.text))); } let title = manifest @@ -2311,7 +2315,7 @@ digraph Demo { //! the strict `SettingsLayer` schema, so unknown fields anywhere in //! the document trip `deny_unknown_fields`. - use fabro_workflow::ManifestPath; + use fabro_types::ManifestPath; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig}; use super::super::root_workflow_run_layer; diff --git a/lib/crates/fabro-template/Cargo.toml b/lib/crates/fabro-template/Cargo.toml index 93dcf1272..92812d4f0 100644 --- a/lib/crates/fabro-template/Cargo.toml +++ b/lib/crates/fabro-template/Cargo.toml @@ -15,8 +15,9 @@ workspace = true [dependencies] anyhow.workspace = true fabro-util = { path = "../fabro-util" } +fabro-types = { path = "../fabro-types" } miette.workspace = true -minijinja = { workspace = true, features = ["debug"] } +minijinja = { workspace = true, features = ["debug", "unstable_machinery"] } serde.workspace = true thiserror.workspace = true toml.workspace = true diff --git a/lib/crates/fabro-template/src/dependency.rs b/lib/crates/fabro-template/src/dependency.rs new file mode 100644 index 000000000..480104e16 --- /dev/null +++ b/lib/crates/fabro-template/src/dependency.rs @@ -0,0 +1,192 @@ +use std::collections::{HashMap, HashSet, VecDeque}; + +use fabro_types::ManifestPath; +use minijinja::machinery::ast::{Expr, Stmt}; +use minijinja::machinery::{self, WhitespaceConfig}; +use minijinja::syntax::SyntaxConfig; +use minijinja::value::Value; +use thiserror::Error; + +use crate::TemplateError; +use crate::store::{TemplateLoadError, TemplateSource, TemplateStore}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum TemplateDependencyKind { + Include, + Extends, + Import, + FromImport, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct TemplateDependency { + pub kind: TemplateDependencyKind, + pub reference: String, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct ExtractedTemplateDependencies { + pub static_references: Vec, + pub dynamic_references: Vec, +} + +#[derive(Debug, Error)] +pub enum TemplateDiscoveryError { + #[error(transparent)] + Parse(#[from] TemplateError), + #[error(transparent)] + Load(#[from] TemplateLoadError), + #[error("missing template dependency `{reference}` from `{parent}`")] + Missing { + parent: ManifestPath, + reference: String, + }, + #[error("dynamic template dependency in `{parent}` must be declared explicitly")] + Dynamic { parent: ManifestPath }, +} + +#[derive(Clone, Debug, Default)] +pub struct TemplateDependencyClosure { + pub sources: HashMap, +} + +impl TemplateDependencyClosure { + #[must_use] + pub fn paths(&self) -> HashSet { + self.sources.keys().cloned().collect() + } +} + +pub fn extract_template_dependencies( + source_name: &str, + source: &str, +) -> Result { + // MiniJinja's AST is behind `unstable_machinery`; keep direct usage in + // this module so future MiniJinja upgrades have a single adapter surface. + let parsed = machinery::parse( + source, + source_name, + SyntaxConfig, + WhitespaceConfig::default(), + ) + .map_err(TemplateError::from)?; + let mut dependencies = ExtractedTemplateDependencies::default(); + collect_stmt_dependencies(&parsed, &mut dependencies); + Ok(dependencies) +} + +pub fn discover_static_dependency_closure( + roots: impl IntoIterator, + store: &dyn TemplateStore, +) -> Result { + let mut sources = HashMap::new(); + let mut queue = VecDeque::new(); + + for source in roots { + if sources + .insert(source.path.clone(), source.clone()) + .is_none() + { + queue.push_back(source); + } + } + + while let Some(source) = queue.pop_front() { + let dependencies = + extract_template_dependencies(&source.path.to_string(), &source.content)?; + if !dependencies.dynamic_references.is_empty() { + return Err(TemplateDiscoveryError::Dynamic { + parent: source.path, + }); + } + for dependency in dependencies.static_references { + let loaded = store + .load(&source.path, &dependency.reference)? + .ok_or_else(|| TemplateDiscoveryError::Missing { + parent: source.path.clone(), + reference: dependency.reference.clone(), + })?; + if sources + .insert(loaded.path.clone(), loaded.clone()) + .is_none() + { + queue.push_back(loaded); + } + } + } + + Ok(TemplateDependencyClosure { sources }) +} + +pub(crate) fn has_loader_dependent_tags( + source_name: &str, + source: &str, +) -> Result, TemplateError> { + let dependencies = extract_template_dependencies(source_name, source)?; + Ok(dependencies + .static_references + .first() + .map(|dependency| dependency.kind) + .or_else(|| dependencies.dynamic_references.first().copied())) +} + +fn collect_stmt_dependencies(stmt: &Stmt<'_>, dependencies: &mut ExtractedTemplateDependencies) { + match stmt { + Stmt::Template(template) => collect_stmt_list(&template.children, dependencies), + Stmt::ForLoop(for_loop) => { + collect_stmt_list(&for_loop.body, dependencies); + collect_stmt_list(&for_loop.else_body, dependencies); + } + Stmt::IfCond(if_cond) => { + collect_stmt_list(&if_cond.true_body, dependencies); + collect_stmt_list(&if_cond.false_body, dependencies); + } + Stmt::WithBlock(with_block) => collect_stmt_list(&with_block.body, dependencies), + Stmt::SetBlock(set_block) => collect_stmt_list(&set_block.body, dependencies), + Stmt::AutoEscape(auto_escape) => collect_stmt_list(&auto_escape.body, dependencies), + Stmt::FilterBlock(filter_block) => collect_stmt_list(&filter_block.body, dependencies), + Stmt::Block(block) => collect_stmt_list(&block.body, dependencies), + Stmt::Include(include) => { + collect_loader_expr(TemplateDependencyKind::Include, &include.name, dependencies); + } + Stmt::Extends(extends) => { + collect_loader_expr(TemplateDependencyKind::Extends, &extends.name, dependencies); + } + Stmt::Import(import) => { + collect_loader_expr(TemplateDependencyKind::Import, &import.expr, dependencies); + } + Stmt::FromImport(from_import) => collect_loader_expr( + TemplateDependencyKind::FromImport, + &from_import.expr, + dependencies, + ), + Stmt::Macro(macro_) => collect_stmt_list(¯o_.body, dependencies), + Stmt::CallBlock(call_block) => collect_stmt_list(&call_block.macro_decl.body, dependencies), + Stmt::EmitExpr(_) | Stmt::EmitRaw(_) | Stmt::Set(_) | Stmt::Do(_) => {} + } +} + +fn collect_stmt_list(stmts: &[Stmt<'_>], dependencies: &mut ExtractedTemplateDependencies) { + for stmt in stmts { + collect_stmt_dependencies(stmt, dependencies); + } +} + +fn collect_loader_expr( + kind: TemplateDependencyKind, + expr: &Expr<'_>, + dependencies: &mut ExtractedTemplateDependencies, +) { + if let Some(reference) = const_string(expr) { + dependencies + .static_references + .push(TemplateDependency { kind, reference }); + } else { + dependencies.dynamic_references.push(kind); + } +} + +fn const_string(expr: &Expr<'_>) -> Option { + let value: Value = expr.as_const()?; + value.as_str().map(ToOwned::to_owned) +} diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index 3dff5e0fe..6216c2f46 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -1,12 +1,43 @@ use std::collections::HashMap; use std::fmt; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; +use fabro_types::ManifestPath; use fabro_util::env::Env; use miette::{LabeledSpan, NamedSource, SourceCode, SourceSpan}; use minijinja::value::{Object, Value}; use minijinja::{AutoEscape, Environment, ErrorKind, UndefinedBehavior}; +mod dependency; +mod store; + +pub use dependency::{ + ExtractedTemplateDependencies, TemplateDependency, TemplateDependencyClosure, + TemplateDependencyKind, TemplateDiscoveryError, discover_static_dependency_closure, + extract_template_dependencies, +}; +pub use store::{ + BundleTemplateStore, CachedTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, + TemplateLoadError, TemplateSource, TemplateStore, +}; + +pub type TemplateLoader = Arc Option + Send + Sync>; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum TemplateRenderMode { + Strict, + Lenient, +} + +impl TemplateRenderMode { + fn undefined_behavior(self) -> UndefinedBehavior { + match self { + Self::Strict => UndefinedBehavior::Strict, + Self::Lenient => UndefinedBehavior::Chainable, + } + } +} + #[derive(Debug, Default, Clone)] pub struct TemplateContext { goal: Option, @@ -117,6 +148,14 @@ where /// boundaries that walk `Error::source()` (anyhow, miette, `collect_chain`). #[derive(Debug)] pub enum TemplateError { + LoaderDependentString { + source_name: Option, + tag: TemplateDependencyKind, + }, + Load { + source_name: Option, + source: Box, + }, Syntax { line: Option, source_name: Option, @@ -147,6 +186,13 @@ pub enum TemplateError { impl fmt::Display for TemplateError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + Self::LoaderDependentString { tag, .. } => { + write!( + f, + "loader-dependent template tag `{tag:?}` requires a rooted template source" + ) + } + Self::Load { .. } => write!(f, "template load error"), Self::Syntax { line, .. } => { write!(f, "template syntax error{}", fmt_location(*line)) } @@ -168,6 +214,8 @@ impl fmt::Display for TemplateError { impl std::error::Error for TemplateError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { + Self::LoaderDependentString { .. } => None, + Self::Load { source, .. } => Some(source.as_ref()), Self::Syntax { source, .. } | Self::UndefinedVariable { source, .. } | Self::Render { source, .. } => Some(source.as_ref()), @@ -243,13 +291,17 @@ impl TemplateError { pub fn expression(&self) -> Option<&str> { match self { Self::UndefinedVariable { expression, .. } => expression.as_deref(), - Self::Syntax { .. } | Self::Render { .. } => None, + Self::LoaderDependentString { .. } + | Self::Load { .. } + | Self::Syntax { .. } + | Self::Render { .. } => None, } } #[must_use] pub fn line(&self) -> Option { match self { + Self::LoaderDependentString { .. } | Self::Load { .. } => None, Self::Syntax { line, .. } | Self::UndefinedVariable { line, .. } | Self::Render { line, .. } => *line, @@ -259,6 +311,9 @@ impl TemplateError { #[must_use] pub fn source_name(&self) -> Option<&str> { match self { + Self::LoaderDependentString { source_name, .. } | Self::Load { source_name, .. } => { + source_name.as_deref() + } Self::Syntax { source_name, .. } | Self::UndefinedVariable { source_name, .. } | Self::Render { source_name, .. } => source_name.as_deref(), @@ -268,6 +323,7 @@ impl TemplateError { #[must_use] pub fn source_text(&self) -> Option<&str> { match self { + Self::LoaderDependentString { .. } | Self::Load { .. } => None, Self::Syntax { source_text, .. } | Self::UndefinedVariable { source_text, .. } | Self::Render { source_text, .. } => source_text.as_deref(), @@ -277,6 +333,7 @@ impl TemplateError { #[must_use] pub fn span(&self) -> Option { match self { + Self::LoaderDependentString { .. } | Self::Load { .. } => None, Self::Syntax { span, .. } | Self::UndefinedVariable { span, .. } | Self::Render { span, .. } => *span, @@ -298,6 +355,7 @@ impl TemplateError { fn source_code_ref(&self) -> Option<&NamedSource> { match self { + Self::LoaderDependentString { .. } | Self::Load { .. } => None, Self::Syntax { source_code, .. } | Self::UndefinedVariable { source_code, .. } | Self::Render { source_code, .. } => source_code.as_deref(), @@ -308,6 +366,8 @@ impl TemplateError { impl miette::Diagnostic for TemplateError { fn code<'a>(&'a self) -> Option> { let code = match self { + Self::LoaderDependentString { .. } => "fabro::template::loader_dependent_string", + Self::Load { .. } => "fabro::template::load", Self::Syntax { .. } => "fabro::template::syntax", Self::UndefinedVariable { .. } => "fabro::template::undefined_variable", Self::Render { .. } => "fabro::template::render", @@ -323,6 +383,8 @@ impl miette::Diagnostic for TemplateError { fn labels(&self) -> Option + '_>> { let span = self.span()?; let label = match self { + Self::LoaderDependentString { .. } => "loader-dependent tag".to_string(), + Self::Load { .. } => "template load".to_string(), Self::UndefinedVariable { expression, .. } => expression.as_ref().map_or_else( || "undefined variable".to_string(), |expr| format!("`{expr}`"), @@ -349,7 +411,7 @@ fn is_plain_text(template: &str) -> bool { } pub fn render(template: &str, ctx: &TemplateContext) -> Result { - render_with(None, template, ctx, UndefinedBehavior::Strict) + render_with(None, template, ctx, UndefinedBehavior::Strict, None) } pub fn render_named( @@ -357,7 +419,28 @@ pub fn render_named( template: &str, ctx: &TemplateContext, ) -> Result { - render_with(Some(name.into()), template, ctx, UndefinedBehavior::Strict) + render_with( + Some(name.into()), + template, + ctx, + UndefinedBehavior::Strict, + None, + ) +} + +pub fn render_named_with_loader( + name: impl Into, + template: &str, + ctx: &TemplateContext, + loader: &TemplateLoader, +) -> Result { + render_with( + Some(name.into()), + template, + ctx, + UndefinedBehavior::Strict, + Some(loader), + ) } /// Render with chainable undefined handling: undefined variables and attribute @@ -365,7 +448,7 @@ pub fn render_named( /// passes (e.g. manifest scanning, `fabro validate` on a bare `.fabro`) where /// the user has not yet bound inputs — strict checking happens elsewhere. pub fn render_lenient(template: &str, ctx: &TemplateContext) -> Result { - render_with(None, template, ctx, UndefinedBehavior::Chainable) + render_with(None, template, ctx, UndefinedBehavior::Chainable, None) } pub fn render_lenient_named( @@ -378,6 +461,22 @@ pub fn render_lenient_named( template, ctx, UndefinedBehavior::Chainable, + None, + ) +} + +pub fn render_lenient_named_with_loader( + name: impl Into, + template: &str, + ctx: &TemplateContext, + loader: &TemplateLoader, +) -> Result { + render_with( + Some(name.into()), + template, + ctx, + UndefinedBehavior::Chainable, + Some(loader), ) } @@ -386,14 +485,22 @@ fn render_with( template: &str, ctx: &TemplateContext, undefined: UndefinedBehavior, + loader: Option<&TemplateLoader>, ) -> Result { if is_plain_text(template) { return Ok(template.to_owned()); } + if loader.is_none() { + reject_loader_dependent_string(name.as_deref(), template)?; + } let mut env = Environment::new(); env.set_undefined_behavior(undefined); env.set_auto_escape_callback(|_| AutoEscape::None); env.set_debug(true); + if let Some(loader) = loader { + let loader = Arc::clone(loader); + env.set_loader(move |name| Ok(loader(name))); + } match name { Some(name) => env.render_named_str(&name, template, ctx.clone().into_value()), None => env.render_str(template, ctx.clone().into_value()), @@ -401,6 +508,88 @@ fn render_with( .map_err(TemplateError::from) } +pub fn render_source( + source: &TemplateSource, + ctx: &TemplateContext, + store: Arc, + mode: TemplateRenderMode, +) -> Result { + render_rooted_source(source, ctx, store, mode.undefined_behavior()) +} + +fn render_rooted_source( + source: &TemplateSource, + ctx: &TemplateContext, + store: Arc, + undefined: UndefinedBehavior, +) -> Result { + if is_plain_text(&source.content) { + return Ok(source.content.clone()); + } + let mut env = Environment::new(); + env.set_undefined_behavior(undefined); + env.set_auto_escape_callback(|_| AutoEscape::None); + env.set_debug(true); + env.set_path_join_callback(|name, parent| joined_template_path(name, parent).into()); + + let load_error = Arc::new(Mutex::new(None)); + let loader_error = Arc::clone(&load_error); + env.set_loader(move |name| { + let parent = ManifestPath::from_wire(".").expect("root manifest path should parse"); + match store.load(&parent, name) { + Ok(source) => Ok(source.map(|source| source.content)), + Err(error) => { + *loader_error + .lock() + .expect("template load error mutex should not be poisoned") = Some(error); + Err(minijinja::Error::new( + ErrorKind::InvalidOperation, + "template load failed", + )) + } + } + }); + + env.render_named_str( + &source.path.to_string(), + &source.content, + ctx.clone().into_value(), + ) + .map_err(|error| { + if let Some(error) = load_error + .lock() + .expect("template load error mutex should not be poisoned") + .take() + { + TemplateError::Load { + source_name: Some(source.path.to_string()), + source: Box::new(error), + } + } else { + TemplateError::from(error) + } + }) +} + +fn joined_template_path(name: &str, parent: &str) -> String { + let Some(parent) = ManifestPath::from_wire(parent) else { + return name.to_owned(); + }; + ManifestPath::from_reference(parent.parent_or_dot(), name) + .map_or_else(|| name.to_owned(), |path| path.to_string()) +} + +fn reject_loader_dependent_string(name: Option<&str>, template: &str) -> Result<(), TemplateError> { + let source_name = name.unwrap_or("string"); + if let Some(tag) = dependency::has_loader_dependent_tags(source_name, template)? { + return Err(TemplateError::LoaderDependentString { + source_name: name.map(ToOwned::to_owned), + tag, + }); + } + Ok(()) +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -410,6 +599,20 @@ mod tests { use super::*; + fn manifest_path(value: &str) -> ManifestPath { + ManifestPath::from_wire(value).expect("path should parse") + } + + fn bundle_store(files: &[(&str, &str)]) -> Arc { + Arc::new(BundleTemplateStore::new( + manifest_path("."), + files + .iter() + .map(|(path, content)| (manifest_path(path), (*content).to_string())) + .collect(), + )) + } + #[test] fn renders_simple_goal_variable() { let ctx = TemplateContext::new().with_goal("Fix bugs"); @@ -523,6 +726,230 @@ mod tests { assert!(span.is_some()); } + #[test] + fn render_named_with_loader_supports_include() { + let ctx = TemplateContext::new(); + let loader: TemplateLoader = + Arc::new(|name| (name == "partial.md").then(|| "included content".to_string())); + + let rendered = + render_named_with_loader("prompt.md", r#"{% include "partial.md" %}"#, &ctx, &loader) + .unwrap(); + + assert_eq!(rendered, "included content"); + } + + #[test] + fn render_source_supports_rooted_include() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("prompts/main.md"), + content: r#"{% include "partial.md" %}"#.to_string(), + }; + + let rendered = render_source( + &source, + &ctx, + bundle_store(&[("prompts/partial.md", "included content")]), + TemplateRenderMode::Strict, + ) + .unwrap(); + + assert_eq!(rendered, "included content"); + } + + #[test] + fn render_source_supports_nested_include() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("prompts/main.md"), + content: r#"{% include "partial.md" %}"#.to_string(), + }; + + let rendered = render_source( + &source, + &ctx, + bundle_store(&[ + ("prompts/partial.md", r#"{% include "nested.md" %}"#), + ("prompts/nested.md", "nested content"), + ]), + TemplateRenderMode::Strict, + ) + .unwrap(); + + assert_eq!(rendered, "nested content"); + } + + #[test] + fn render_source_supports_extends() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("pages/main.md"), + content: r#"{% extends "layout.md" %}{% block body %}Body{% endblock %}"#.to_string(), + }; + + let rendered = render_source( + &source, + &ctx, + bundle_store(&[( + "pages/layout.md", + "prefix:{% block body %}{% endblock %}:suffix", + )]), + TemplateRenderMode::Strict, + ) + .unwrap(); + + assert_eq!(rendered, "prefix:Body:suffix"); + } + + #[test] + fn render_source_supports_import() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("prompts/main.md"), + content: r#"{% import "macros.md" as macros %}{{ macros.greet("Ada") }}"#.to_string(), + }; + + let rendered = render_source( + &source, + &ctx, + bundle_store(&[( + "prompts/macros.md", + r"{% macro greet(name) %}hi {{ name }}{% endmacro %}", + )]), + TemplateRenderMode::Strict, + ) + .unwrap(); + + assert_eq!(rendered, "hi Ada"); + } + + #[test] + fn render_source_supports_from_import() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("prompts/main.md"), + content: r#"{% from "macros.md" import greet %}{{ greet("Ada") }}"#.to_string(), + }; + + let rendered = render_source( + &source, + &ctx, + bundle_store(&[( + "prompts/macros.md", + r"{% macro greet(name) %}hi {{ name }}{% endmacro %}", + )]), + TemplateRenderMode::Strict, + ) + .unwrap(); + + assert_eq!(rendered, "hi Ada"); + } + + #[test] + fn render_source_rejects_unsafe_include() { + let ctx = TemplateContext::new(); + let source = TemplateSource { + path: manifest_path("prompts/main.md"), + content: r#"{% include "../outside.md" %}"#.to_string(), + }; + let store: Arc = Arc::new(BundleTemplateStore::new( + manifest_path("prompts"), + HashMap::new(), + )); + + let err = render_source(&source, &ctx, store, TemplateRenderMode::Strict).unwrap_err(); + + assert!(matches!( + err, + TemplateError::Load { + source, + .. + } if matches!(*source, TemplateLoadError::EscapesRoot { .. }) + )); + } + + #[test] + fn render_named_rejects_loader_dependent_tags_without_root() { + let ctx = TemplateContext::new(); + + let err = render_named("main.md", r#"{% include "partial.md" %}"#, &ctx).unwrap_err(); + + assert!(matches!(err, TemplateError::LoaderDependentString { .. })); + } + + #[test] + fn extractor_ignores_comments_raw_blocks_and_plain_text() { + let source = r#" + {# {% include "comment.md" %} #} + {% raw %}{% include "raw.md" %}{% endraw %} + text {% include "text.md" %} + "#; + + let dependencies = extract_template_dependencies("test.md", source).unwrap(); + + assert_eq!(dependencies.static_references, vec![TemplateDependency { + kind: TemplateDependencyKind::Include, + reference: "text.md".to_string(), + }]); + assert!(dependencies.dynamic_references.is_empty()); + } + + #[test] + fn static_dependency_closure_collects_both_branches() { + let source = TemplateSource { + path: manifest_path("main.md"), + content: r#"{% if inputs.use_a %}{% include "a.md" %}{% else %}{% include "b.md" %}{% endif %}"# + .to_string(), + }; + + let closure = discover_static_dependency_closure( + [source], + bundle_store(&[("a.md", "A"), ("b.md", "B")]).as_ref(), + ) + .unwrap(); + + assert!(closure.sources.contains_key(&manifest_path("a.md"))); + assert!(closure.sources.contains_key(&manifest_path("b.md"))); + } + + #[test] + fn static_dependency_closure_collects_unused_macro_body_dependencies() { + let source = TemplateSource { + path: manifest_path("main.md"), + content: r#"{% from "helpers.md" import render_advanced_prompt %}"#.to_string(), + }; + + let closure = discover_static_dependency_closure( + [source], + bundle_store(&[ + ( + "helpers.md", + r#"{% macro render_advanced_prompt() %}{% include "advanced.md" %}{% endmacro %}"#, + ), + ("advanced.md", "advanced"), + ]) + .as_ref(), + ) + .unwrap(); + + assert!(closure.sources.contains_key(&manifest_path("helpers.md"))); + assert!(closure.sources.contains_key(&manifest_path("advanced.md"))); + } + + #[test] + fn static_dependency_closure_rejects_dynamic_include() { + let source = TemplateSource { + path: manifest_path("main.md"), + content: r"{% include inputs.partial %}".to_string(), + }; + + let err = + discover_static_dependency_closure([source], bundle_store(&[]).as_ref()).unwrap_err(); + + assert!(matches!(err, TemplateDiscoveryError::Dynamic { .. })); + } + #[test] fn render_lenient_named_preserves_source_name_for_syntax_errors() { let ctx = TemplateContext::new(); diff --git a/lib/crates/fabro-template/src/store.rs b/lib/crates/fabro-template/src/store.rs new file mode 100644 index 000000000..60d8c0643 --- /dev/null +++ b/lib/crates/fabro-template/src/store.rs @@ -0,0 +1,287 @@ +use std::collections::{HashMap, HashSet}; +use std::path::{Component, Path, PathBuf}; +use std::sync::{Mutex, MutexGuard}; + +use fabro_types::ManifestPath; +use thiserror::Error; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct TemplateSource { + pub path: ManifestPath, + pub content: String, +} + +pub trait TemplateStore: Send + Sync { + fn load( + &self, + parent: &ManifestPath, + reference: &str, + ) -> Result, TemplateLoadError>; +} + +#[derive(Debug, Error)] +pub enum TemplateLoadError { + #[error("unsafe template reference `{reference}` from `{parent}`")] + UnsafeReference { + parent: ManifestPath, + reference: String, + }, + #[error("template reference `{reference}` from `{parent}` escapes template root `{root}`")] + EscapesRoot { + parent: ManifestPath, + reference: String, + root: ManifestPath, + }, + #[error("failed to read template `{path}`")] + Io { + path: PathBuf, + source: std::io::Error, + }, + #[error("dynamic template dependency `{path}` is not declared as an asset")] + DynamicDependency { path: ManifestPath }, +} + +#[derive(Clone, Debug)] +pub struct FilesystemTemplateStore { + cwd: PathBuf, + root: ManifestPath, +} + +impl FilesystemTemplateStore { + #[must_use] + pub fn new(cwd: impl Into, root: ManifestPath) -> Self { + Self { + cwd: cwd.into(), + root, + } + } +} + +impl TemplateStore for FilesystemTemplateStore { + #[expect( + clippy::disallowed_methods, + reason = "MiniJinja loaders are synchronous, so rooted template stores use sync file I/O" + )] + fn load( + &self, + parent: &ManifestPath, + reference: &str, + ) -> Result, TemplateLoadError> { + let logical = resolve_logical_reference(parent, reference, &self.root)?; + let absolute = self.cwd.join(logical.as_path()); + let root = self.cwd.join(self.root.as_path()); + + let canonical = match absolute.canonicalize() { + Ok(path) if path.is_file() => path, + Ok(_) => return Ok(None), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(error) => { + return Err(TemplateLoadError::Io { + path: absolute, + source: error, + }); + } + }; + let canonical_root = root + .canonicalize() + .map_err(|source| TemplateLoadError::Io { + path: root.clone(), + source, + })?; + if !canonical.starts_with(&canonical_root) { + return Err(TemplateLoadError::EscapesRoot { + parent: parent.clone(), + reference: reference.to_owned(), + root: self.root.clone(), + }); + } + + let content = + std::fs::read_to_string(&canonical).map_err(|source| TemplateLoadError::Io { + path: canonical.clone(), + source, + })?; + Ok(Some(TemplateSource { + path: logical, + content, + })) + } +} + +#[derive(Clone, Debug, Default)] +pub struct BundleTemplateStore { + root: ManifestPath, + files: HashMap, +} + +impl BundleTemplateStore { + #[must_use] + pub fn new(root: ManifestPath, files: HashMap) -> Self { + Self { root, files } + } +} + +impl TemplateStore for BundleTemplateStore { + fn load( + &self, + parent: &ManifestPath, + reference: &str, + ) -> Result, TemplateLoadError> { + let path = resolve_logical_reference(parent, reference, &self.root)?; + Ok(self.files.get(&path).map(|content| TemplateSource { + path, + content: content.clone(), + })) + } +} + +#[derive(Debug)] +pub struct CachedTemplateStore { + inner: T, + cache: Mutex>>, +} + +impl CachedTemplateStore { + #[must_use] + pub fn new(inner: T) -> Self { + Self { + inner, + cache: Mutex::new(HashMap::new()), + } + } +} + +impl TemplateStore for CachedTemplateStore +where + T: TemplateStore, +{ + fn load( + &self, + parent: &ManifestPath, + reference: &str, + ) -> Result, TemplateLoadError> { + let key = (parent.clone(), reference.to_owned()); + if let Some(source) = lock(&self.cache).get(&key).cloned() { + return Ok(source); + } + let source = self.inner.load(parent, reference)?; + lock(&self.cache).insert(key, source.clone()); + Ok(source) + } +} + +#[derive(Debug)] +pub struct RecordingTemplateStore { + inner: T, + loaded: Mutex>, + allowed: Option>, +} + +impl RecordingTemplateStore { + #[must_use] + pub fn new(inner: T) -> Self { + Self { + inner, + loaded: Mutex::new(HashSet::new()), + allowed: None, + } + } + + #[must_use] + pub fn with_allowed(inner: T, allowed: HashSet) -> Self { + Self { + inner, + loaded: Mutex::new(HashSet::new()), + allowed: Some(allowed), + } + } + + #[must_use] + pub fn loaded_paths(&self) -> HashSet { + lock(&self.loaded).clone() + } +} + +impl TemplateStore for RecordingTemplateStore +where + T: TemplateStore, +{ + fn load( + &self, + parent: &ManifestPath, + reference: &str, + ) -> Result, TemplateLoadError> { + let source = self.inner.load(parent, reference)?; + if let Some(source) = source.as_ref() { + if let Some(allowed) = &self.allowed { + if !allowed.contains(&source.path) { + return Err(TemplateLoadError::DynamicDependency { + path: source.path.clone(), + }); + } + } + lock(&self.loaded).insert(source.path.clone()); + } + Ok(source) + } +} + +pub(crate) fn resolve_logical_reference( + parent: &ManifestPath, + reference: &str, + root: &ManifestPath, +) -> Result { + if !is_safe_template_reference(reference) { + return Err(TemplateLoadError::UnsafeReference { + parent: parent.clone(), + reference: reference.to_owned(), + }); + } + let path = + ManifestPath::from_reference(parent.parent_or_dot(), reference).ok_or_else(|| { + TemplateLoadError::UnsafeReference { + parent: parent.clone(), + reference: reference.to_owned(), + } + })?; + if !is_within_root(&path, root) { + return Err(TemplateLoadError::EscapesRoot { + parent: parent.clone(), + reference: reference.to_owned(), + root: root.clone(), + }); + } + Ok(path) +} + +pub(crate) fn is_safe_template_reference(reference: &str) -> bool { + !reference.is_empty() + && !reference.starts_with('~') + && !reference.contains('\\') + && !has_windows_drive_prefix(reference) + && !Path::new(reference).is_absolute() +} + +fn has_windows_drive_prefix(path: &str) -> bool { + let mut chars = path.chars(); + matches!( + (chars.next(), chars.next()), + (Some(first), Some(':')) if first.is_ascii_alphabetic() + ) +} + +fn is_within_root(path: &ManifestPath, root: &ManifestPath) -> bool { + if root.as_path().as_os_str().is_empty() { + return !matches!( + path.as_path().components().next(), + Some(Component::ParentDir) + ); + } + path.starts_with(root) +} + +fn lock(mutex: &Mutex) -> MutexGuard<'_, T> { + mutex + .lock() + .expect("template store mutex should not be poisoned") +} diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index b4d634881..e4fd7b55d 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -14,6 +14,7 @@ pub mod failure_signature; pub mod graph; pub mod interview; pub mod llm_backend; +pub mod manifest_path; pub mod outcome; pub mod principal; pub mod pull_request; @@ -61,6 +62,7 @@ pub use graph::{ }; pub use interview::{InterviewQuestionRecord, QuestionType}; pub use llm_backend::LlmBackend; +pub use manifest_path::{ManifestPath, ManifestPathParseError}; pub use outcome::{ FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageState, }; diff --git a/lib/crates/fabro-workflow/src/manifest_path.rs b/lib/crates/fabro-types/src/manifest_path.rs similarity index 90% rename from lib/crates/fabro-workflow/src/manifest_path.rs rename to lib/crates/fabro-types/src/manifest_path.rs index 44a33808d..89061605a 100644 --- a/lib/crates/fabro-workflow/src/manifest_path.rs +++ b/lib/crates/fabro-types/src/manifest_path.rs @@ -12,6 +12,9 @@ pub struct ManifestPath(PathBuf); impl ManifestPath { #[must_use] pub fn from_reference(current_dir: &Path, reference: &str) -> Option { + if !is_portable_logical_path(reference) { + return None; + } let path = Path::new(reference); if path.is_absolute() || reference.starts_with('~') { return None; @@ -31,6 +34,9 @@ impl ManifestPath { #[must_use] pub fn from_wire(value: &str) -> Option { + if !is_portable_logical_path(value) { + return None; + } Self::from_reference(Path::new("."), value) } @@ -50,6 +56,11 @@ impl ManifestPath { pub fn parent_or_dot(&self) -> &Path { self.0.parent().unwrap_or_else(|| Path::new(".")) } + + #[must_use] + pub fn starts_with(&self, base: &Self) -> bool { + self.0.starts_with(base.as_path()) + } } impl From for PathBuf { @@ -108,6 +119,23 @@ fn normalize_components(path: impl AsRef) -> Option { Some(normalized) } +fn is_portable_logical_path(path: &str) -> bool { + if path.contains('\\') { + return false; + } + let mut chars = path.chars(); + let Some(first) = chars.next() else { + return true; + }; + let Some(second) = chars.next() else { + return true; + }; + if first.is_ascii_alphabetic() && second == ':' { + return false; + } + true +} + fn relative_path_from(path: &Path, base: &Path) -> Option { let path_components = path.components().collect::>(); let base_components = base.components().collect::>(); @@ -160,6 +188,16 @@ mod tests { assert!(ManifestPath::from_reference(Path::new("."), "~/.fabro/workflow.fabro").is_none()); } + #[test] + fn from_reference_rejects_backslash_reference() { + assert!(ManifestPath::from_reference(Path::new("."), "prompts\\goal.md").is_none()); + } + + #[test] + fn from_reference_rejects_windows_drive_reference() { + assert!(ManifestPath::from_reference(Path::new("."), "C:/repo/workflow.fabro").is_none()); + } + #[test] fn from_reference_simple_relative() { let path = ManifestPath::from_reference(Path::new("flows"), "workflow.fabro").unwrap(); @@ -272,6 +310,11 @@ mod tests { assert!(ManifestPath::from_wire("~/.fabro/workflow.fabro").is_none()); } + #[test] + fn from_wire_rejects_backslash_path() { + assert!(ManifestPath::from_wire("foo\\bar").is_none()); + } + #[test] fn from_wire_renormalizes_uncollapsed_curdir() { let path = ManifestPath::from_wire("./foo/./bar").unwrap(); diff --git a/lib/crates/fabro-workflow/src/file_resolver.rs b/lib/crates/fabro-workflow/src/file_resolver.rs index 6ecdbbf00..78bfa1a1a 100644 --- a/lib/crates/fabro-workflow/src/file_resolver.rs +++ b/lib/crates/fabro-workflow/src/file_resolver.rs @@ -6,7 +6,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use crate::ManifestPath; +use fabro_types::ManifestPath; pub trait FileResolver: Send + Sync { fn resolve(&self, current_dir: &Path, reference: &str) -> Option; diff --git a/lib/crates/fabro-workflow/src/lib.rs b/lib/crates/fabro-workflow/src/lib.rs index d7dcc5017..5eb441ff9 100644 --- a/lib/crates/fabro-workflow/src/lib.rs +++ b/lib/crates/fabro-workflow/src/lib.rs @@ -230,7 +230,6 @@ mod hook_context; reason = "The lifecycle module remains crate-visible for tests and pending integrations." )] pub(crate) mod lifecycle; -mod manifest_path; pub(crate) mod node_handler; pub mod operations; pub mod outcome; @@ -247,7 +246,7 @@ pub use billing_rollup::{ billing_rollup_from_projection, }; pub use error::{Error, FailureCategory, FailureSignature, FailureSignatureExt, Result}; -pub use manifest_path::ManifestPath; +pub use fabro_types::ManifestPath; pub use steering_hub::SteeringHub; pub mod run_materialization; pub(crate) mod run_metadata; diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index a086389d9..851420cf2 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -14,12 +14,13 @@ use fabro_model::{Catalog, ProviderId}; use fabro_sandbox::SandboxProvider; use fabro_store::Database; use fabro_types::settings::run::{RunMode, RunNamespace}; -use fabro_types::{ForkSourceRef, GitContext, RunId, RunProvenance, WorkflowSettings}; +use fabro_types::{ + ForkSourceRef, GitContext, ManifestPath, RunId, RunProvenance, WorkflowSettings, +}; use fabro_util::json::normalize_json_value; use tokio::task::spawn_blocking; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; -use crate::ManifestPath; use crate::error::Error; use crate::event::{Event, append_event, to_run_event_at}; use crate::file_resolver::FileResolver; @@ -936,6 +937,55 @@ mod tests { assert_eq!(validated.graph().goal(), "ship it"); } + #[test] + fn validate_from_file_resolves_minijinja_includes_relative_to_prompt_and_goal_files() { + let dir = tempfile::tempdir().unwrap(); + let prompt_dir = dir.path().join("prompts"); + let goal_dir = dir.path().join("goals"); + std::fs::create_dir_all(&prompt_dir).unwrap(); + std::fs::create_dir_all(&goal_dir).unwrap(); + std::fs::write( + prompt_dir.join("prompt.md"), + r#"{% include "prompt.tpl.md" %}"#, + ) + .unwrap(); + std::fs::write(prompt_dir.join("prompt.tpl.md"), "included prompt").unwrap(); + std::fs::write(goal_dir.join("goal.md"), r#"{% include "goal.tpl.md" %}"#).unwrap(); + std::fs::write(goal_dir.join("goal.tpl.md"), "included goal").unwrap(); + + let dot_path = dir.path().join("workflow.fabro"); + std::fs::write( + &dot_path, + r#"digraph Test { + graph [goal="@goals/goal.md"] + start [shape=Mdiamond] + work [prompt="@prompts/prompt.md"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ) + .unwrap(); + + let validated = validate(ValidateInput { + workflow: WorkflowInput::Path(dot_path), + settings: WorkflowSettings::default(), + cwd: dir.path().to_path_buf(), + custom_transforms: Vec::new(), + catalog: test_catalog(), + }) + .unwrap(); + + validated.raise_on_errors().unwrap(); + assert_eq!(validated.graph().goal(), "included goal"); + assert_eq!( + validated.graph().nodes["work"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some("included prompt") + ); + } + #[test] fn validate_from_bundle_resolves_nested_import_files_relative_to_imported_graph() { let validated = validate(ValidateInput { @@ -984,6 +1034,57 @@ mod tests { ); } + #[test] + fn validate_from_bundle_resolves_minijinja_includes_in_prompt_and_goal_files() { + let validated = validate(ValidateInput { + workflow: WorkflowInput::Bundled(BundledWorkflow { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Test { + graph [goal="@goals/goal.md"] + start [shape=Mdiamond] + work [prompt="@prompts/work.md"] + exit [shape=Msquare] + start -> work -> exit + }"# + .to_string(), + config: None, + files: HashMap::from([ + ( + ManifestPath::from_wire("goals/goal.md").unwrap(), + r#"{% include "goal.tpl.md" %}"#.to_string(), + ), + ( + ManifestPath::from_wire("goals/goal.tpl.md").unwrap(), + "Bundled goal".to_string(), + ), + ( + ManifestPath::from_wire("prompts/work.md").unwrap(), + r#"{% include "work.tpl.md" %}"#.to_string(), + ), + ( + ManifestPath::from_wire("prompts/work.tpl.md").unwrap(), + "Bundled prompt".to_string(), + ), + ]), + }), + settings: WorkflowSettings::default(), + cwd: PathBuf::from("."), + custom_transforms: Vec::new(), + catalog: test_catalog(), + }) + .unwrap(); + + validated.raise_on_errors().unwrap(); + assert_eq!(validated.graph().goal(), "Bundled goal"); + assert_eq!( + validated.graph().nodes["work"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some("Bundled prompt") + ); + } + #[tokio::test] async fn create_returns_validation_failed_with_diagnostics() { let dot = r#"digraph Test { diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 58804aa51..8dedc67df 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -15,7 +15,6 @@ use fabro_sandbox::config::{ use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::{DockerSandboxOptions, SandboxProvider, SandboxSpec}; use fabro_static::EnvVars; -use fabro_types::RunId; use fabro_types::settings::run::{ ApprovalMode, DaytonaNetworkLayer, DaytonaSettings, DockerSettings, DockerfileSource as ResolvedDockerfileSource, HookDefinition as ResolvedHookDefinition, @@ -25,12 +24,12 @@ use fabro_types::settings::run::{ RunNamespace as ResolvedRunSettings, TlsMode as ResolvedTlsMode, }; use fabro_types::settings::{InterpString, ModelRegistry, ResolvedModelRef}; +use fabro_types::{ManifestPath, RunId}; use fabro_vault::Vault; use tokio::runtime::Handle; use tokio::sync::RwLock as AsyncRwLock; use tokio_util::sync::CancellationToken; -use crate::ManifestPath; use crate::artifact_upload::ArtifactSink; use crate::context::Context; use crate::error::Error; @@ -1130,11 +1129,10 @@ mod tests { use fabro_store::Database; use fabro_types::settings::ModelRef; use fabro_types::settings::run::RunMode; - use fabro_types::{WorkflowSettings, fixtures}; + use fabro_types::{ManifestPath, WorkflowSettings, fixtures}; use object_store::memory::InMemory; use super::*; - use crate::ManifestPath; use crate::context::Context; use crate::event::{Emitter, EventBody}; use crate::handler::HandlerRegistry; diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index 1db7ba0bc..745924037 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -7,13 +7,12 @@ use fabro_interview::Interviewer; use fabro_mcp::config::McpServerSettings; use fabro_model::{Catalog, FallbackTarget, ProviderId}; use fabro_sandbox::SandboxSpec; -use fabro_types::RunId; use fabro_types::settings::run::{PullRequestSettings, RunModelControls}; +use fabro_types::{ManifestPath, RunId}; use fabro_validate::{Diagnostic, Severity}; use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; -use crate::ManifestPath; use crate::artifact_upload::ArtifactSink; use crate::context::Context; use crate::error::Error; diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index 58198c0b0..aa5e70c2a 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -11,9 +11,9 @@ use fabro_auth::CredentialSource; use fabro_auth::ResolvedCredentials; use fabro_hooks::{HookContext, HookDecision, HookRunner}; use fabro_model::{Catalog, ProviderId}; +use fabro_types::ManifestPath; use tokio_util::sync::CancellationToken; -use crate::ManifestPath; use crate::event::Emitter; use crate::github_token_source::GitHubTokenSource; use crate::handler::HandlerRegistry; diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index 7fb8a69e7..c69a596d2 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph}; -use fabro_template::TemplateContext; +use fabro_template::{TemplateContext, TemplateLoader}; use fabro_validate::Diagnostic; use super::Transform; @@ -34,6 +34,34 @@ pub fn resolve_file_ref( .map_or_else(|| value.to_string(), |resolved| resolved.content)) } +pub(crate) fn template_include_loader( + current_dir: PathBuf, + resolver: Arc, +) -> TemplateLoader { + Arc::new(move |name| { + if !is_safe_include_name(name) { + return None; + } + resolver + .resolve(¤t_dir, name) + .map(|resolved| resolved.content) + }) +} + +fn is_safe_include_name(name: &str) -> bool { + if name.is_empty() || name.starts_with('~') || Path::new(name).is_absolute() { + return false; + } + name.split('/') + .all(|segment| !segment.starts_with('.') && !segment.contains('\\')) +} + +fn parent_dir_or_dot(path: &Path) -> PathBuf { + path.parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .map_or_else(|| PathBuf::from("."), Path::to_path_buf) +} + /// Inlines `@file` references in node prompts and the graph-level goal. pub struct FileInliningTransform { current_dir: PathBuf, @@ -111,7 +139,11 @@ impl FileInliningTransform { node_id.clone(), "prompt", ) - .with_source_text(self.source_text.as_deref(), prompt); + .with_source_text(self.source_text.as_deref(), prompt) + .with_include_loader(Some(template_include_loader( + self.current_dir.clone(), + Arc::clone(&self.resolver), + ))); let rendered = render_template_for_target( prompt, &ctx, @@ -139,7 +171,11 @@ impl FileInliningTransform { }; let ctx = TemplateContext::for_input_scan(self.inputs.clone()); let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") - .with_source_text(self.source_text.as_deref(), goal); + .with_source_text(self.source_text.as_deref(), goal) + .with_include_loader(Some(template_include_loader( + self.current_dir.clone(), + Arc::clone(&self.resolver), + ))); let rendered = render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics)?; let value = self @@ -168,7 +204,11 @@ impl FileInliningTransform { }; let target = owner_target .with_source_name(resolved.path.display().to_string()) - .with_source_text(Some(&resolved.content), &resolved.content); + .with_source_text(Some(&resolved.content), &resolved.content) + .with_include_loader(Some(template_include_loader( + parent_dir_or_dot(&resolved.path), + Arc::clone(&self.resolver), + ))); Ok(Some(render_file_contents( &resolved, ctx, @@ -310,6 +350,71 @@ mod tests { ); } + #[test] + fn file_inlining_transform_resolves_minijinja_includes_for_prompts_and_goal() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(dir.path().join("prompts")).unwrap(); + std::fs::create_dir_all(dir.path().join("goals")).unwrap(); + std::fs::write( + dir.path().join("prompts/work.md"), + r#"{% include "work.tpl.md" %}"#, + ) + .unwrap(); + std::fs::write(dir.path().join("prompts/work.tpl.md"), "file prompt").unwrap(); + std::fs::write(dir.path().join("inline.tpl.md"), "inline prompt").unwrap(); + std::fs::write( + dir.path().join("goals/goal.md"), + r#"{% include "goal.tpl.md" %}"#, + ) + .unwrap(); + std::fs::write(dir.path().join("goals/goal.tpl.md"), "included goal").unwrap(); + + let mut graph = Graph::new("test"); + graph.attrs.insert( + "goal".to_string(), + AttrValue::String("@goals/goal.md".to_string()), + ); + let mut file_prompt = Node::new("file_prompt"); + file_prompt.attrs.insert( + "prompt".to_string(), + AttrValue::String("@prompts/work.md".to_string()), + ); + graph.nodes.insert("file_prompt".to_string(), file_prompt); + let mut inline_prompt = Node::new("inline_prompt"); + inline_prompt.attrs.insert( + "prompt".to_string(), + AttrValue::String(r#"{% include "inline.tpl.md" %}"#.to_string()), + ); + graph + .nodes + .insert("inline_prompt".to_string(), inline_prompt); + + let transform = FileInliningTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + ); + let graph = transform.apply(graph).unwrap(); + + assert_eq!( + graph.nodes["file_prompt"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some("file prompt") + ); + assert_eq!( + graph.nodes["inline_prompt"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some("inline prompt") + ); + assert_eq!( + graph.attrs.get("goal").and_then(AttrValue::as_str), + Some("included goal") + ); + } + #[test] fn resolve_file_ref_expands_tilde() { let home = dirs::home_dir().expect("home dir must exist"); diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index d60e5423b..32a48fc9f 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -7,6 +7,7 @@ use fabro_graphviz::parser; use fabro_template::TemplateContext; use fabro_validate::Diagnostic; +use super::file_inlining::template_include_loader; use super::{FileInliningTransform, Transform}; use crate::error::Error; use crate::file_resolver::{FileResolver, ResolvedFile}; @@ -673,7 +674,11 @@ impl ImportTransform { let path_ctx = TemplateContext::for_input_scan(self.inputs.clone()); let mut ignored_goal_diagnostics = Vec::new(); let goal_target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") - .with_source_text(self.source_text.as_deref(), graph.goal()); + .with_source_text(self.source_text.as_deref(), graph.goal()) + .with_include_loader(Some(template_include_loader( + self.current_dir.clone(), + Arc::clone(&self.resolver), + ))); let parent_goal = render_template_for_target( graph.goal(), &path_ctx, diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index a8232cca7..2af94a1e0 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -2,7 +2,10 @@ use std::collections::HashMap; use std::fmt::Write as _; use fabro_graphviz::graph::{AttrValue, Graph}; -use fabro_template::{TemplateContext, TemplateError, render_lenient_named, render_named}; +use fabro_template::{ + TemplateContext, TemplateError, TemplateLoader, render_lenient_named, + render_lenient_named_with_loader, render_named, render_named_with_loader, +}; use fabro_util::error::collect_chain; use fabro_validate::{Diagnostic, Severity}; @@ -29,7 +32,7 @@ pub enum RenderMode { Structural, } -#[derive(Clone, Debug)] +#[derive(Clone)] pub(crate) struct TemplateRenderTarget { pub source_name: Option, pub source_text: Option, @@ -37,6 +40,7 @@ pub(crate) struct TemplateRenderTarget { pub node_id: Option, pub edge: Option<(String, String)>, pub owner: String, + include_loader: Option, } impl TemplateRenderTarget { @@ -50,6 +54,7 @@ impl TemplateRenderTarget { node_id: None, edge: None, owner: format!("graph attribute `{attr_name}`"), + include_loader: None, } } @@ -68,6 +73,7 @@ impl TemplateRenderTarget { node_id: Some(node_id.clone()), edge: None, owner: format!("node `{node_id}` attribute `{attr_name}`"), + include_loader: None, } } @@ -88,6 +94,7 @@ impl TemplateRenderTarget { node_id: None, edge: Some((from.clone(), to.clone())), owner: format!("edge `{from} -> {to}` attribute `{attr_name}`"), + include_loader: None, } } @@ -104,6 +111,12 @@ impl TemplateRenderTarget { self } + #[must_use] + pub(crate) fn with_include_loader(mut self, include_loader: Option) -> Self { + self.include_loader = include_loader; + self + } + #[must_use] fn template_source_name(&self) -> String { self.source_name @@ -120,15 +133,23 @@ pub(crate) fn render_template_for_target( diagnostics: &mut Vec, ) -> Result { let source_name = target.template_source_name(); + let render_strict = || match target.include_loader.as_ref() { + Some(loader) => render_named_with_loader(source_name.clone(), text, ctx, loader), + None => render_named(source_name.clone(), text, ctx), + }; match render_mode { - RenderMode::Strict => render_named(source_name, text, ctx) - .map_err(|err| template_error_for_target(target, err)), - RenderMode::Structural => match render_named(source_name.clone(), text, ctx) { + RenderMode::Strict => render_strict().map_err(|err| template_error_for_target(target, err)), + RenderMode::Structural => match render_strict() { Ok(rendered) => Ok(rendered), Err(err @ TemplateError::UndefinedVariable { .. }) => { diagnostics.push(template_diagnostic(&err, target)); - render_lenient_named(source_name, text, ctx) - .map_err(|err| template_error_for_target(target, err)) + match target.include_loader.as_ref() { + Some(loader) => { + render_lenient_named_with_loader(source_name, text, ctx, loader) + } + None => render_lenient_named(source_name, text, ctx), + } + .map_err(|err| template_error_for_target(target, err)) } Err(err) => Err(template_error_for_target(target, err)), }, diff --git a/lib/crates/fabro-workflow/src/workflow_bundle.rs b/lib/crates/fabro-workflow/src/workflow_bundle.rs index cebd23ba4..dddcda492 100644 --- a/lib/crates/fabro-workflow/src/workflow_bundle.rs +++ b/lib/crates/fabro-workflow/src/workflow_bundle.rs @@ -2,9 +2,9 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use fabro_types::ManifestPath; use serde::{Deserialize, Serialize}; -use crate::ManifestPath; use crate::file_resolver::{BundleFileResolver, FileResolver}; #[derive(Clone, Debug, Serialize, Deserialize)] diff --git a/test/templates/missing_dependency/workflow.fabro b/test/templates/missing_dependency/workflow.fabro new file mode 100644 index 000000000..85293f1e3 --- /dev/null +++ b/test/templates/missing_dependency/workflow.fabro @@ -0,0 +1,9 @@ +digraph MissingTemplateDependency { + graph [goal="ship"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + inline_prompt [label="Inline Prompt", prompt="{% include 'missing.tpl.md' %}"] + + start -> inline_prompt -> exit +} diff --git a/test/templates/static_dependencies/inline.tpl.md b/test/templates/static_dependencies/inline.tpl.md new file mode 100644 index 000000000..fcdc36581 --- /dev/null +++ b/test/templates/static_dependencies/inline.tpl.md @@ -0,0 +1 @@ +Use the inline include. diff --git a/test/templates/static_dependencies/prompts/goal.md b/test/templates/static_dependencies/prompts/goal.md new file mode 100644 index 000000000..e4aa17916 --- /dev/null +++ b/test/templates/static_dependencies/prompts/goal.md @@ -0,0 +1 @@ +{% include "goal.tpl.md" %} diff --git a/test/templates/static_dependencies/prompts/goal.tpl.md b/test/templates/static_dependencies/prompts/goal.tpl.md new file mode 100644 index 000000000..b8925c5c3 --- /dev/null +++ b/test/templates/static_dependencies/prompts/goal.tpl.md @@ -0,0 +1 @@ +Ship the include fix. diff --git a/test/templates/static_dependencies/prompts/prompt.md b/test/templates/static_dependencies/prompts/prompt.md new file mode 100644 index 000000000..3becdbce2 --- /dev/null +++ b/test/templates/static_dependencies/prompts/prompt.md @@ -0,0 +1 @@ +{% include "prompt.tpl.md" %} diff --git a/test/templates/static_dependencies/prompts/prompt.tpl.md b/test/templates/static_dependencies/prompts/prompt.tpl.md new file mode 100644 index 000000000..6fbe5d1f3 --- /dev/null +++ b/test/templates/static_dependencies/prompts/prompt.tpl.md @@ -0,0 +1 @@ +Use the included prompt. diff --git a/test/templates/static_dependencies/workflow.fabro b/test/templates/static_dependencies/workflow.fabro new file mode 100644 index 000000000..fcbd53343 --- /dev/null +++ b/test/templates/static_dependencies/workflow.fabro @@ -0,0 +1,10 @@ +digraph TemplateIncludes { + graph [goal="@prompts/goal.md"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + file_prompt [label="File Prompt", prompt="@prompts/prompt.md"] + inline_prompt [label="Inline Prompt", prompt="{% include 'inline.tpl.md' %}"] + + start -> file_prompt -> inline_prompt -> exit +}