From 79e44262aa230b21fae212a5fe40eb1537c4caa2 Mon Sep 17 00:00:00 2001
From: Scott Werner <stwerner@vt.edu>
Date: Thu, 13 Aug 2026 10:37:19 -0400
Subject: [PATCH] Detect workflow path collisions hidden by sort order

The adjacent-pair scan over the byte-sorted path list missed
file/directory collisions whenever a sibling path sorted between the
ancestor and its descendant (any byte below '/' after the shared
prefix, e.g. "assets.txt" between "assets" and "assets/item.txt").
Replace it with an exhaustive ancestor-prefix lookup over a path set,
which also catches equal paths across files and workflow dependencies.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 .../fabro-types/src/workflow_version.rs       | 104 +++++++++++++++---
 1 file changed, 90 insertions(+), 14 deletions(-)

diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs
index c0b35f8ad..cba3a04c1 100644
--- a/lib/foundation/fabro-types/src/workflow_version.rs
+++ b/lib/foundation/fabro-types/src/workflow_version.rs
@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 use std::fmt;
 use std::marker::PhantomData;
 
@@ -135,23 +135,27 @@ impl WorkflowVersion {
     fn validate_path_collisions(&self) -> Result<(), WorkflowVersionShapeError> {
         // Keys are unique within each map, so equality can only collide
         // across files and workflow dependencies.
-        let mut paths = self
-            .files
-            .keys()
-            .chain(self.workflow_dependencies.keys())
-            .collect::<Vec<_>>();
-        paths.sort_unstable();
-        for pair in paths.windows(2) {
-            let [first, second] = pair else {
-                unreachable!("a two-item window must contain two paths")
-            };
-            if first == second || first.is_ancestor_of(second) {
+        let mut by_text =
+            HashMap::with_capacity(self.files.len() + self.workflow_dependencies.len());
+        for path in self.files.keys().chain(self.workflow_dependencies.keys()) {
+            if let Some(existing) = by_text.insert(path.as_str(), path) {
                 return Err(WorkflowVersionShapeError::PathCollision {
-                    first:  (*first).clone(),
-                    second: (*second).clone(),
+                    first:  existing.clone(),
+                    second: path.clone(),
                 });
             }
         }
+        for path in self.files.keys().chain(self.workflow_dependencies.keys()) {
+            let text = path.as_str();
+            for (index, _) in text.match_indices('/') {
+                if let Some(ancestor) = by_text.get(&text[..index]) {
+                    return Err(WorkflowVersionShapeError::PathCollision {
+                        first:  (*ancestor).clone(),
+                        second: path.clone(),
+                    });
+                }
+            }
+        }
         Ok(())
     }
 }
@@ -295,6 +299,78 @@ mod tests {
         ));
     }
 
+    #[test]
+    fn rejects_ancestor_collisions_hidden_by_sort_order() {
+        // `assets.txt` sorts between `assets` and `assets/item.txt` because
+        // '.' precedes '/', so an adjacent-pair scan over the sorted list
+        // would miss this collision.
+        let error = WorkflowVersion::new(
+            path("workflow.fabro"),
+            BTreeMap::from([
+                (path("workflow.fabro"), "digraph W {}".to_string()),
+                (path("assets"), "file".to_string()),
+                (path("assets.txt"), "sibling".to_string()),
+                (path("assets/item.txt"), "nested".to_string()),
+            ]),
+            BTreeMap::new(),
+        )
+        .unwrap_err();
+        assert!(matches!(
+            error,
+            WorkflowVersionShapeError::PathCollision { first, second }
+                if first.as_str() == "assets" && second.as_str() == "assets/item.txt"
+        ));
+
+        assert!(
+            WorkflowVersion::new(
+                path("workflow.fabro"),
+                BTreeMap::from([
+                    (path("workflow.fabro"), "digraph W {}".to_string()),
+                    (path("assets.txt"), "sibling".to_string()),
+                    (path("assets/item.txt"), "nested".to_string()),
+                ]),
+                BTreeMap::new(),
+            )
+            .is_ok()
+        );
+    }
+
+    #[test]
+    fn rejects_collisions_across_files_and_workflow_dependencies() {
+        let dependency_id = WorkflowVersionId::from(BlobHash::new(b"child"));
+
+        let equal = WorkflowVersion::new(
+            path("workflow.fabro"),
+            BTreeMap::from([
+                (path("workflow.fabro"), "digraph W {}".to_string()),
+                (path("child.fabro"), "digraph C {}".to_string()),
+            ]),
+            BTreeMap::from([(path("child.fabro"), dependency_id)]),
+        )
+        .unwrap_err();
+        assert!(matches!(
+            equal,
+            WorkflowVersionShapeError::PathCollision { first, second }
+                if first == second && first.as_str() == "child.fabro"
+        ));
+
+        let ancestor = WorkflowVersion::new(
+            path("workflow.fabro"),
+            BTreeMap::from([
+                (path("workflow.fabro"), "digraph W {}".to_string()),
+                (path("libs"), "file".to_string()),
+                (path("libs.md"), "sibling".to_string()),
+            ]),
+            BTreeMap::from([(path("libs/child.fabro"), dependency_id)]),
+        )
+        .unwrap_err();
+        assert!(matches!(
+            ancestor,
+            WorkflowVersionShapeError::PathCollision { first, second }
+                if first.as_str() == "libs" && second.as_str() == "libs/child.fabro"
+        ));
+    }
+
     #[test]
     fn enforces_file_count_file_size_and_canonical_size_boundaries() {
         let mut files = BTreeMap::from([(path("workflow.fabro"), "digraph W {}".to_string())]);