From 679bc6701bf9dada17797cee1920dfec471038e8 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 17 Aug 2026 17:51:46 -0400 Subject: [PATCH] Parse template dependencies whose paths collide with discovery roots Dependency discovery pre-seeded roots into the path-keyed result map and reused that map as the traversal-dedup set, so a loaded include target whose path matched a root was recorded but never parsed (an include chain that reaches the file anchoring a root silently skips its content), and a second root occurrence at an already-seeded path was dropped without parsing. Dedup traversal on the full (path, root, content) occurrence instead, so every distinct authored occurrence is parsed exactly once and identical duplicates parse once. Co-Authored-By: Claude Fable 5 --- .../fabro-template/src/dependency.rs | 32 +++-- lib/foundation/fabro-template/src/lib.rs | 117 ++++++++++++++++++ 2 files changed, 138 insertions(+), 11 deletions(-) diff --git a/lib/foundation/fabro-template/src/dependency.rs b/lib/foundation/fabro-template/src/dependency.rs index fa9629108..289032e38 100644 --- a/lib/foundation/fabro-template/src/dependency.rs +++ b/lib/foundation/fabro-template/src/dependency.rs @@ -80,15 +80,30 @@ pub fn discover_static_dependency_closure( store: &dyn TemplateStore, ) -> Result { let mut sources = HashMap::new(); + // A root and a loaded file can collide on `path` while carrying different + // content (an inline prompt is anchored at its graph file's path), so + // traversal dedup keys on the full occurrence rather than the path: a + // path-keyed check would leave the collided occurrence unparsed. The + // result map stays path-keyed, with the last distinct occurrence winning. + let mut parsed = HashSet::new(); let mut queue = VecDeque::new(); - for source in roots { - if sources - .insert(source.path.clone(), source.clone()) - .is_none() - { + let mut enqueue = |source: TemplateSource, + sources: &mut HashMap, + queue: &mut VecDeque| { + let occurrence = ( + source.path.clone(), + source.root.clone(), + source.content.clone(), + ); + if parsed.insert(occurrence) { + sources.insert(source.path.clone(), source.clone()); queue.push_back(source); } + }; + + for source in roots { + enqueue(source, &mut sources, &mut queue); } while let Some(source) = queue.pop_front() { @@ -106,12 +121,7 @@ pub fn discover_static_dependency_closure( reference: dependency.reference.clone(), } })?; - if sources - .insert(loaded.path.clone(), loaded.clone()) - .is_none() - { - queue.push_back(loaded); - } + enqueue(loaded, &mut sources, &mut queue); } } diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index 8381f5fec..9fc89697b 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -1388,6 +1388,123 @@ mod tests { assert!(matches!(err, TemplateDiscoveryError::Dynamic { .. })); } + #[test] + fn static_dependency_closure_visits_colliding_root_occurrences() { + let roots = [ + TemplateSource::new(manifest_path("workflow.fabro"), manifest_path("."), "valid"), + TemplateSource::new( + manifest_path("workflow.fabro"), + manifest_path("."), + r"{% include inputs.partial %}", + ), + ]; + + let error = + discover_static_dependency_closure(roots, bundle_store(&[]).as_ref()).unwrap_err(); + + assert!(matches!( + error, + TemplateDiscoveryError::Dynamic { parent } + if parent == manifest_path("workflow.fabro") + )); + } + + #[test] + fn static_dependency_closure_parses_dependencies_shadowed_by_root_paths() { + // An inline root anchored at its graph file's path must not shadow the + // file itself when another template includes it: the loaded file + // content still gets parsed. + let roots = [ + TemplateSource::new(manifest_path("workflow.fabro"), manifest_path("."), "valid"), + TemplateSource::new( + manifest_path("goal.md"), + manifest_path("."), + r#"{% include "workflow.fabro" %}"#, + ), + ]; + + let error = discover_static_dependency_closure( + roots, + bundle_store(&[("workflow.fabro", r#"{% include "missing.md" %}"#)]).as_ref(), + ) + .unwrap_err(); + + assert!(matches!( + error, + TemplateDiscoveryError::Missing { parent, reference } + if parent == manifest_path("workflow.fabro") && reference == "missing.md" + )); + } + + #[test] + fn static_dependency_closure_parses_identical_root_occurrences_once() { + struct CountingStore { + inner: Arc, + loads: std::sync::atomic::AtomicUsize, + } + + impl TemplateStore for CountingStore { + fn load( + &self, + parent: &TemplateSource, + reference: &str, + ) -> Result, TemplateLoadError> { + self.loads + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + self.inner.load(parent, reference) + } + } + + let root = TemplateSource::new( + manifest_path("main.md"), + manifest_path("."), + r#"{% include "shared.md" %}"#, + ); + let store = CountingStore { + inner: bundle_store(&[("shared.md", "shared")]), + loads: std::sync::atomic::AtomicUsize::new(0), + }; + + let closure = discover_static_dependency_closure([root.clone(), root], &store).unwrap(); + + assert!(closure.sources.contains_key(&manifest_path("shared.md"))); + assert_eq!(store.loads.load(std::sync::atomic::Ordering::Relaxed), 1); + } + + #[test] + fn static_dependency_closure_deduplicates_loaded_dependencies_across_roots() { + let roots = [ + TemplateSource::new( + manifest_path("first.md"), + manifest_path("."), + r#"{% include "shared.md" %}"#, + ), + TemplateSource::new( + manifest_path("second.md"), + manifest_path("."), + r#"{% include "shared.md" %}"#, + ), + ]; + + let closure = discover_static_dependency_closure( + roots, + bundle_store(&[ + ("shared.md", r#"{% include "nested.md" %}"#), + ("nested.md", "nested"), + ]) + .as_ref(), + ) + .unwrap(); + + assert_eq!( + closure.paths(), + ["first.md", "second.md", "shared.md", "nested.md"] + .into_iter() + .map(manifest_path) + .collect() + ); + } + #[test] fn render_lenient_named_preserves_source_name_for_syntax_errors() { let ctx = TemplateContext::new();