mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
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>
This commit is contained in:
parent
178320e7a5
commit
79e44262aa
1 changed files with 90 additions and 14 deletions
|
|
@ -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())]);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue