From 5708c31a55cff773331a65d6ac97aa49227c0354 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 21 Oct 2026 08:22:15 +0300 Subject: [PATCH 1/4] fix(repo,vfs): refuse a snapshot that holds a path as both file and directory (#278) An absent protected or out-of-view file is carried back verbatim at commit, merge/pick/rebase completion, ws harvest and ws refresh. A directory created at the same path in the checkout then produced a tree with a blob and a tree under one name. The one tree builder now refuses it with FileDirCollision { file, child } before writing any tree; no ref moves. Across commits a path may still change kind (#247). --- CLAUDE.md | 8 + crates/repo/src/file_dir_collision.rs | 209 ++++++++++++++++++++ crates/repo/src/lib.rs | 2 + crates/vfs/src/lib.rs | 82 +++++++- docs/adr/0025-protected-merge-and-replay.md | 8 + docs/adr/0034-sparse-checkouts.md | 23 +++ 6 files changed, 329 insertions(+), 3 deletions(-) create mode 100644 crates/repo/src/file_dir_collision.rs diff --git a/CLAUDE.md b/CLAUDE.md index 6e06b598..1c84c7f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -339,6 +339,14 @@ transport-adjacent. The rest, imperatively: away and retry. A directory holding only tracked files is not such a case: a file/directory change at one path crosses switch, undo, abort and `--continue`, and the emptied directories are cleared (#247). `materialize` checks every such directory before its first removal, so a refusal changes nothing and an abort stays retryable; a forward op also refuses a user's empty directory there (it cannot tell it from a husk), while `materialize` itself prunes empty ones. (On a case-insensitive checkout, replacing `foo/a` with a file `Foo` is refused: the check does not follow the case alias. Fail-closed.) On a sparse checkout the same holds for a path the result adds outside the view (a later commit would take the user's file instead, #248); there any directory at the path refuses, tracked contents or not. Merging into an unborn branch and the conflict stop of a merge into a private branch are not guarded (except that a private stop refuses a `.theirs` sidecar that would land on an existing file). +- **A snapshot never holds a path as both a file and a directory (#278).** The + one tree builder (`scl_vfs::Vfs::build_root`) refuses it with + `FileDirCollision { file, child }` before writing any tree. It matters because + an absent protected or out-of-view file is carried back verbatim: creating a + directory of that name in a checkout without the key (or outside the view) + makes the commit, merge completion, `sc ws harvest` or `sc ws refresh` fail + until the directory is removed (ADR-0034 refinement 9). Across commits a path + may still change kind (#247). - **Every ref-moving or commit-creating op (including all secret/protect policy ops) is guarded against an in-progress merge/pick/rebase** — finish or abort the in-progress op first. A push into a repo with a stopped op is diff --git a/crates/repo/src/file_dir_collision.rs b/crates/repo/src/file_dir_collision.rs new file mode 100644 index 00000000..4a7d6a9c --- /dev/null +++ b/crates/repo/src/file_dir_collision.rs @@ -0,0 +1,209 @@ +//! #278: a snapshot may never hold a path as both a file and the parent +//! directory of another path. The absent-path carry (protected content the +//! committer cannot read, paths outside the sparse view) used to be able to put +//! a stale file next to a directory the user or agent created at the same path. +//! Every commit path ends in the one tree builder, which now refuses; nothing +//! moves a ref when it does. + +use std::path::{Path, PathBuf}; + +use crate::error::Error; +use crate::repo::Repo; +use crate::worktree; + +fn tmp_root(tag: &str) -> PathBuf { + let p = std::env::temp_dir().join(format!("scl-fdc-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&p); + std::fs::create_dir_all(&p).unwrap(); + p +} + +fn put(root: &Path, rel: &str, body: &str) { + let full = root.join(rel); + std::fs::create_dir_all(full.parent().unwrap()).unwrap(); + std::fs::write(full, body).unwrap(); +} + +/// The error names both the file and the path that needs it to be a directory. +fn assert_collision(err: &Error, file: &str, child: &str) { + let msg = err.to_string(); + assert!( + matches!(err, Error::Vfs(scl_vfs::Error::FileDirCollision { .. })), + "wanted a file/directory collision, got {err:?}" + ); + assert!( + msg.contains(file) && msg.contains(child), + "message must name both paths: {msg}" + ); +} + +/// A repo whose `main` holds a protected file `secret` (and `keep.txt`), plus +/// a checkout where `secret` is absent: the state of a committer with no key. +/// Returns the repo, its root and the tip, with the working tree clean. +fn keyless_checkout(tag: &str) -> (Repo, PathBuf, scl_core::ObjectId) { + let root = tmp_root(tag); + let repo = Repo::init(&root).unwrap(); + let (_alice_sk, alice_pk) = scl_crypto::generate_keypair(); + let (mallory_sk, _) = scl_crypto::generate_keypair(); + repo.test_set_protected_prefix("secret/", &[alice_pk]) + .unwrap(); + repo.branch("other").unwrap(); + put(&root, "secret", "hunter2"); + put(&root, "keep.txt", "k\n"); + let c1 = repo.commit("me", "base").unwrap(); + repo.switch_with_identity("other", Some(&mallory_sk)) + .unwrap(); + let skipped = repo + .switch_with_identity("main", Some(&mallory_sk)) + .unwrap(); + assert_eq!(skipped, vec!["secret".to_string()]); + assert!(!root.join("secret").exists()); + (repo, root, c1) +} + +fn cleanup(repo: Repo, root: &Path) { + drop(repo); + std::fs::remove_dir_all(root).unwrap(); +} + +#[test] +fn commit_refuses_a_directory_over_an_absent_protected_file() { + let (repo, root, c1) = keyless_checkout("commit-dir"); + put(&root, "secret/notes.txt", "mine\n"); + let err = repo.commit("mallory", "dir").unwrap_err(); + assert_collision(&err, "secret", "secret/notes.txt"); + assert_eq!(repo.head_tip().unwrap(), Some(c1), "no ref moved"); + cleanup(repo, &root); +} + +#[test] +fn commit_refuses_a_file_over_an_absent_protected_directory() { + let root = tmp_root("commit-file"); + let repo = Repo::init(&root).unwrap(); + let (_sk, alice_pk) = scl_crypto::generate_keypair(); + let (mallory_sk, _) = scl_crypto::generate_keypair(); + repo.test_set_protected_prefix("vault/", &[alice_pk]) + .unwrap(); + repo.branch("other").unwrap(); + put(&root, "vault/sub/a.txt", "a"); + let c1 = repo.commit("me", "base").unwrap(); + repo.switch_with_identity("other", Some(&mallory_sk)) + .unwrap(); + repo.switch_with_identity("main", Some(&mallory_sk)) + .unwrap(); + // `vault/sub` is absent for mallory, who creates a plain *file* of that name. + let _ = std::fs::remove_dir_all(root.join("vault")); + put(&root, "vault/sub", "not a dir\n"); + let err = repo.commit("mallory", "file").unwrap_err(); + assert_collision(&err, "vault/sub", "vault/sub/a.txt"); + assert_eq!(repo.head_tip().unwrap(), Some(c1)); + cleanup(repo, &root); +} + +#[test] +fn commit_refuses_a_directory_over_an_out_of_sparse_file() { + let root = tmp_root("sparse"); + let repo = Repo::init(&root).unwrap(); + put(&root, "src/a.txt", "a"); + put(&root, "docs", "a file\n"); + let c1 = repo.commit("me", "base").unwrap(); + repo.set_sparse(&["src/".to_string()], None).unwrap(); + assert!(!root.join("docs").exists()); + put(&root, "docs/x.txt", "x\n"); + let err = repo.commit("me", "dir").unwrap_err(); + assert_collision(&err, "docs", "docs/x.txt"); + assert_eq!(repo.head_tip().unwrap(), Some(c1)); + cleanup(repo, &root); +} + +#[test] +fn a_real_file_to_directory_transition_still_commits() { + // #247: across commits a path may change kind; only one snapshot may not + // hold both. + let root = tmp_root("transition"); + let repo = Repo::init(&root).unwrap(); + put(&root, "x", "file\n"); + repo.commit("me", "file").unwrap(); + std::fs::remove_file(root.join("x")).unwrap(); + put(&root, "x/inner.txt", "dir\n"); + let c2 = repo.commit("me", "dir").unwrap(); + let root_id = repo.snapshot(&c2).unwrap().root; + let files = { + let a = repo.vfs_handle().store(); + let mut s = a.lock().unwrap(); + worktree::tree_file_entries_with_perms(&mut s, root_id).unwrap() + }; + assert_eq!(files.keys().collect::>(), vec!["x/inner.txt"]); + cleanup(repo, &root); +} + +#[test] +fn merge_completion_refuses_a_directory_over_a_carried_protected_file() { + let (repo, root, _c1) = keyless_checkout("merge"); + // Both sides edit keep.txt, so the merge stops; `secret` is carried absent. + repo.branch("feature").unwrap(); + put(&root, "keep.txt", "ours\n"); + let ours = repo.commit("mallory", "ours").unwrap(); + let (mallory_sk, _) = scl_crypto::generate_keypair(); + repo.switch_with_identity("feature", Some(&mallory_sk)) + .unwrap(); + put(&root, "keep.txt", "theirs\n"); + repo.commit("mallory", "theirs").unwrap(); + repo.switch_with_identity("main", Some(&mallory_sk)) + .unwrap(); + assert!(matches!( + repo.merge_with_identity("feature", "mallory", None), + Err(Error::MergeConflicts(1)) + )); + put(&root, "keep.txt", "resolved\n"); + put(&root, "secret/notes.txt", "mine\n"); + let err = repo.commit("mallory", "merge").unwrap_err(); + assert_collision(&err, "secret", "secret/notes.txt"); + assert_eq!(repo.head_tip().unwrap(), Some(ours), "no ref moved"); + cleanup(repo, &root); +} + +#[test] +fn ws_harvest_refuses_a_directory_over_an_absent_protected_file() { + let (repo, root, c1) = keyless_checkout("ws"); + // Forked without a key, so `secret` is absent from the workspace. + let s = repo.ws_fork(None, 1, "t", None).unwrap(); + let dir = s.workspaces[0].dir.clone(); + assert!(!dir.join("secret").exists()); + put(&dir, "secret/agent.txt", "agent\n"); + let err = repo.ws_harvest(None, None, "t", None).unwrap_err(); + assert_collision(&err, "secret", "secret/agent.txt"); + assert_eq!(repo.head_tip().unwrap(), Some(c1), "no ref moved"); + assert!( + crate::refs::resolve_tip(repo.layout(), "ws-1") + .unwrap() + .is_none(), + "no candidate branch left behind" + ); + // The session is still open: removing the directory lets it harvest. + std::fs::remove_dir_all(dir.join("secret")).unwrap(); + put(&dir, "other.txt", "ok\n"); + let out = repo.ws_harvest(None, None, "t", None).unwrap(); + assert_eq!(out.len(), 1); + cleanup(repo, &root); +} + +#[test] +fn ws_refresh_refuses_before_any_write() { + let (repo, root, c1) = keyless_checkout("refresh"); + let s = repo.ws_fork(None, 1, "t", None).unwrap(); + let dir = s.workspaces[0].dir.clone(); + put(&dir, "secret/agent.txt", "agent\n"); + put(&root, "host.txt", "h\n"); + repo.commit("me", "host moves on").unwrap(); + let err = repo.ws_refresh(None, None, None).unwrap_err(); + assert_collision(&err, "secret", "secret/agent.txt"); + let after = repo.ws_session(None).unwrap().unwrap(); + assert_eq!(after.base_snapshot, c1, "base not moved"); + assert!( + !dir.join("host.txt").exists(), + "nothing laid into the checkout" + ); + assert!(after.pending_refresh.is_none()); + cleanup(repo, &root); +} diff --git a/crates/repo/src/lib.rs b/crates/repo/src/lib.rs index dac39c8b..4dd9c4f0 100644 --- a/crates/repo/src/lib.rs +++ b/crates/repo/src/lib.rs @@ -9,6 +9,8 @@ pub(crate) mod conflict_plaintext; pub(crate) mod conflicts; pub mod diff3; pub mod error; +#[cfg(test)] +mod file_dir_collision; pub mod fill; pub mod gc; pub mod git_marks; diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index e0cabd8e..0dfef375 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -23,6 +23,10 @@ pub enum Error { NotFound(String), #[error("io error: {0}")] Io(#[from] std::io::Error), + /// A snapshot would hold `file` as a file and also `child` beneath it as a + /// directory (#278). Raised before any tree object is written. + #[error("{file} would be both a file and a directory in one snapshot (it is also the parent of {child}); a protected or out-of-view file that is absent from this checkout cannot be replaced by a directory of the same name, so remove the directory or restore the file")] + FileDirCollision { file: String, child: String }, } pub type Result = std::result::Result; @@ -156,7 +160,7 @@ impl Repo { map.insert(normalize(path), (id, *mode, *perms)); } } - self.build_subtree_inner(&map, &BTreeMap::new(), "") + self.build_root(&map, &BTreeMap::new()) } /// Like [`write_tree_with_perms`][Self::write_tree_with_perms], and also @@ -182,7 +186,7 @@ impl Repo { .iter() .map(|(p, e)| (normalize(p), e.clone())) .collect(); - self.build_subtree_inner(&map, &carried, "") + self.build_root(&map, &carried) } /// Fork an in-memory worktree from a base snapshot. Cheap: allocates only an @@ -209,7 +213,25 @@ impl Repo { .iter() .map(|(p, (id, mode))| (p.clone(), (*id, *mode, 0u8))) .collect(); - self.build_subtree_inner(&with_perms, &BTreeMap::new(), "") + self.build_root(&with_perms, &BTreeMap::new()) + } + + /// The one entry to tree assembly: refuses a path that is both a file and + /// the parent directory of another (#278) before any tree object is + /// written, then builds the root. Within ONE snapshot only; a path may + /// change kind between commits. + fn build_root( + &self, + files: &BTreeMap, + carried: &BTreeMap, + ) -> Result { + let paths: Vec<&str> = files + .keys() + .map(String::as_str) + .chain(carried.keys().map(String::as_str)) + .collect(); + check_file_dir_collisions(&paths)?; + self.build_subtree_inner(files, carried, "") } /// The single tree builder: reads each blob entry's `perms` byte from the map @@ -276,6 +298,25 @@ impl Repo { } } +/// Error if any path is a proper ancestor directory of another path in the set +/// (a name that would be a blob and a tree at once). Sorted order puts every +/// descendant of `p` right after it, so checking neighbours is not enough +/// (`a`, `a-b`, `a/c`); look each ancestor up instead. +fn check_file_dir_collisions(paths: &[&str]) -> Result<()> { + let set: std::collections::BTreeSet<&str> = paths.iter().copied().collect(); + for p in &set { + for (i, _) in p.match_indices('/') { + if set.contains(&p[..i]) { + return Err(Error::FileDirCollision { + file: p[..i].to_string(), + child: (*p).to_string(), + }); + } + } + } + Ok(()) +} + /// A copy-on-write overlay entry. enum Overlay { Written(Arc<[u8]>, FileMode), @@ -545,6 +586,41 @@ mod tests { Repo::new(Store::new(StoreConfig::default())) } + #[test] + fn a_path_cannot_be_both_file_and_directory_in_one_snapshot() { + let r = repo(); + let f = |p: &str| (p.to_string(), b"x".to_vec(), FileMode::FILE, 0u8); + // Not adjacent in sorted order: `a`, `a-b`, `a/c`. + let err = r + .write_tree_with_perms(&[f("a"), f("a-b"), f("a/c")]) + .unwrap_err(); + assert!( + matches!(&err, Error::FileDirCollision { file, child } if file == "a" && child == "a/c"), + "{err:?}" + ); + assert!(r + .write_tree(&[ + ("x/y".into(), vec![], FileMode::FILE), + ("x".into(), vec![], FileMode::FILE) + ]) + .is_err()); + // A carried entry collides with a built file the same way. + let carried = TreeEntry { + name: String::new(), + kind: EntryKind::Blob, + id: Object::blob(b"c".to_vec()).id(), + mode: FileMode::FILE, + perms: scl_core::PROTECTED, + }; + assert!(r + .write_tree_with_carried(&[f("s/x")], &[("s".into(), carried)]) + .is_err()); + // Siblings that merely share a name prefix are fine. + assert!(r + .write_tree_with_perms(&[f("a"), f("a-b"), f("a.b/c"), f("ab/c")]) + .is_ok()); + } + #[test] fn write_tree_with_carried_places_subtrees_verbatim_beside_built_ones() { let r = repo(); diff --git a/docs/adr/0025-protected-merge-and-replay.md b/docs/adr/0025-protected-merge-and-replay.md index 47fef424..a2e1cb56 100644 --- a/docs/adr/0025-protected-merge-and-replay.md +++ b/docs/adr/0025-protected-merge-and-replay.md @@ -136,3 +136,11 @@ deferred. `crypto`" rule), `crates/crypto` re-exports `zeroize::Zeroizing` as a type alias — the dependency itself stays quarantined; only the type name crosses the boundary. +8. **A carried protected path and a directory at the same path are refused + (#278).** Completion carries an absent still-protected path from the decided + root (refinement in ADR-0034 and #238). If the user then created a directory + with that name, or a file where the decided tree has a protected directory, + the snapshot would hold both. The commit fails with a typed + `FileDirCollision` naming both paths and moves nothing; the protected + ciphertext is never dropped and never replaced by plaintext. See ADR-0034 + refinement 9. diff --git a/docs/adr/0034-sparse-checkouts.md b/docs/adr/0034-sparse-checkouts.md index 26cd6d25..b6e5397e 100644 --- a/docs/adr/0034-sparse-checkouts.md +++ b/docs/adr/0034-sparse-checkouts.md @@ -343,3 +343,26 @@ every claim below is checked against the shipped code, not the plan. indistinguishable from the stop's own write and goes; a failed unlink other than "not found" is now an error instead of being swallowed. Pinned by `user_file_safety.rs` (merge, pick, rebase) and `private_abort.rs`. +9. **A carried absent path never sits beside a directory of the same name + (#278).** The absent-path carry (a still-protected path the committer has + no key for, a path outside the sparse view) cannot tell "absent because + skipped" from "absent because replaced", so it brings the tip's file back + even when the checkout now holds a directory at that path (or a file where + the tip had a protected directory). Before, the commit wrote a tree with a + blob and a tree under one name. Every snapshot now goes through one tree + builder (`scl_vfs::Vfs::build_root`) that refuses a path that is both a file + and the parent of another path in the same snapshot, with + `scl_vfs::Error::FileDirCollision { file, child }` naming both, before any + tree object is written; the ref does not move. We refuse instead of letting + the directory win: dropping the carry would destroy ciphertext the committer + cannot read (the rule that makes absent protected paths carry at all), and + the same holds for an out-of-view file. The fix is the user's: remove the + directory, or restore the file (`sc switch` with a key, `sparse set` + widening). The check sees one snapshot only, so a file-to-directory change + across commits (#247) is untouched. It covers plain commit, + amend, merge/pick/rebase completion (`snapshot_files` ends in it), `sc ws + harvest` (the error surfaces with no candidate branch and the session still + open), `sc ws refresh` (an error before any write) and `sc work` (that + agent's harvest result is the error). Private branches build their sealed + trees separately and do not carry absent paths, so they are not affected. + Pinned by `crates/repo/src/file_dir_collision.rs`. From 99b7631d6cc28be6066db8fb9a8587f4a8b04f11 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 21 Oct 2026 08:41:08 +0300 Subject: [PATCH 2/4] fix(repo): run the file/directory collision check in the private-branch tree builder (#278) Review finding: commit_private and publish carry out-of-view and protected paths through build_tree_objects, which had no check. Also corrects the ADR and CLAUDE.md wording and documents the partial-clone graft limit. --- CLAUDE.md | 2 +- crates/repo/src/file_dir_collision.rs | 25 +++++++++++++++++++++++++ crates/repo/src/private.rs | 13 +++++++++---- crates/repo/src/repo.rs | 2 +- crates/vfs/src/lib.rs | 26 ++++++++++++++------------ docs/adr/0034-sparse-checkouts.md | 19 +++++++++++++++---- 6 files changed, 65 insertions(+), 22 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1c84c7f2..65d89195 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -340,7 +340,7 @@ transport-adjacent. The rest, imperatively: outside the view (a later commit would take the user's file instead, #248); there any directory at the path refuses, tracked contents or not. Merging into an unborn branch and the conflict stop of a merge into a private branch are not guarded (except that a private stop refuses a `.theirs` sidecar that would land on an existing file). - **A snapshot never holds a path as both a file and a directory (#278).** The - one tree builder (`scl_vfs::Vfs::build_root`) refuses it with + one tree builder (`scl_vfs::Repo::build_root`, and `private::build_tree_objects` for private branches) refuses it with `FileDirCollision { file, child }` before writing any tree. It matters because an absent protected or out-of-view file is carried back verbatim: creating a directory of that name in a checkout without the key (or outside the view) diff --git a/crates/repo/src/file_dir_collision.rs b/crates/repo/src/file_dir_collision.rs index 4a7d6a9c..70aa347e 100644 --- a/crates/repo/src/file_dir_collision.rs +++ b/crates/repo/src/file_dir_collision.rs @@ -207,3 +207,28 @@ fn ws_refresh_refuses_before_any_write() { assert!(after.pending_refresh.is_none()); cleanup(repo, &root); } + +#[test] +fn private_commit_refuses_a_directory_over_an_out_of_sparse_file() { + // A private commit carries out-of-view paths through its own tree builder. + let root = tmp_root("private"); + let repo = Repo::init(&root).unwrap(); + put(&root, "src/a.txt", "a"); + put(&root, "docs", "a file\n"); + repo.commit("me", "base").unwrap(); + let (sk, _) = scl_crypto::generate_keypair(); + repo.set_sparse(&["src/".to_string()], Some(&sk)).unwrap(); + repo.branch_private("fix", &sk, &[], &[]).unwrap(); + repo.switch_with_identity("fix", Some(&sk)).unwrap(); + assert!(!root.join("docs").exists()); + let before = crate::refs::resolve_tip(repo.layout(), "fix").unwrap(); + put(&root, "docs/x.txt", "x\n"); + let err = repo.commit_private("me", "dir", Some(&sk)).unwrap_err(); + assert_collision(&err, "docs", "docs/x.txt"); + assert_eq!( + crate::refs::resolve_tip(repo.layout(), "fix").unwrap(), + before, + "private ref did not move" + ); + cleanup(repo, &root); +} diff --git a/crates/repo/src/private.rs b/crates/repo/src/private.rs index d9f99410..ad15349b 100644 --- a/crates/repo/src/private.rs +++ b/crates/repo/src/private.rs @@ -998,7 +998,7 @@ impl Repo { let reachable: BTreeSet = entries.values().map(|(id, _, _)| *id).collect(); protection.wrapped.retain(|id, _| reachable.contains(id)); - let (root, trees) = build_tree_objects(&entries); + let (root, trees) = build_tree_objects(&entries)?; let snapshot = Object::Snapshot(Snapshot { root, parents, @@ -1098,12 +1098,17 @@ impl Repo { /// touching any store: returns the root tree id plus every tree object built /// (the sealing loop decides which of them are new). Pure — the inner world's /// analogue of `vfs::write_tree_with_perms`. +/// +/// Refuses a path that is both a file and the parent of another (#278), like +/// the public builder: a private commit carries absent protected and +/// out-of-view paths too. pub(crate) fn build_tree_objects( entries: &BTreeMap, -) -> (ObjectId, Vec) { +) -> Result<(ObjectId, Vec)> { + scl_vfs::check_file_dir_collisions(entries.keys().map(String::as_str))?; let mut trees = Vec::new(); let root = build_subtree(entries, "", &mut trees); - (root, trees) + Ok((root, trees)) } fn build_subtree( @@ -1889,7 +1894,7 @@ impl Repo { published.insert(path.clone(), (blob.id(), *mode, 0)); to_put.push(blob); } - let (root, trees) = build_tree_objects(&published); + let (root, trees) = build_tree_objects(&published)?; to_put.extend(trees); let parents: Vec = snap .parents diff --git a/crates/repo/src/repo.rs b/crates/repo/src/repo.rs index a8b43a10..09881820 100644 --- a/crates/repo/src/repo.rs +++ b/crates/repo/src/repo.rs @@ -3160,7 +3160,7 @@ mod tests { let id = store.put(scl_core::Object::blob(body)).unwrap(); entries.insert(path.to_string(), (id, scl_core::FileMode(0o644), 0)); } - let (tree, objs) = crate::private::build_tree_objects(&entries); + let (tree, objs) = crate::private::build_tree_objects(&entries).unwrap(); for o in objs { store.put(o).unwrap(); } diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index 0dfef375..e2a3e7f6 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -25,7 +25,7 @@ pub enum Error { Io(#[from] std::io::Error), /// A snapshot would hold `file` as a file and also `child` beneath it as a /// directory (#278). Raised before any tree object is written. - #[error("{file} would be both a file and a directory in one snapshot (it is also the parent of {child}); a protected or out-of-view file that is absent from this checkout cannot be replaced by a directory of the same name, so remove the directory or restore the file")] + #[error("{file} would be both a file and a directory in one snapshot (it is also the parent of {child}); a protected or out-of-view path that is absent from this checkout cannot be swapped between file and directory, so remove the conflicting path or restore the missing one")] FileDirCollision { file: String, child: String }, } pub type Result = std::result::Result; @@ -168,7 +168,7 @@ impl Repo { /// `path` with its id, mode and perms untouched and nothing below it read. /// Ancestor directories are built from `files` and the carried paths /// together. A carried path that is also a file, or the ancestor of one, is - /// the caller's collision to reject first (`merge::check_path_collisions`). + /// a collision `build_root` refuses with [`Error::FileDirCollision`]. pub fn write_tree_with_carried( &self, files: &[(String, Vec, FileMode, u8)], @@ -225,12 +225,12 @@ impl Repo { files: &BTreeMap, carried: &BTreeMap, ) -> Result { - let paths: Vec<&str> = files - .keys() - .map(String::as_str) - .chain(carried.keys().map(String::as_str)) - .collect(); - check_file_dir_collisions(&paths)?; + check_file_dir_collisions( + files + .keys() + .map(String::as_str) + .chain(carried.keys().map(String::as_str)), + )?; self.build_subtree_inner(files, carried, "") } @@ -300,10 +300,12 @@ impl Repo { /// Error if any path is a proper ancestor directory of another path in the set /// (a name that would be a blob and a tree at once). Sorted order puts every -/// descendant of `p` right after it, so checking neighbours is not enough -/// (`a`, `a-b`, `a/c`); look each ancestor up instead. -fn check_file_dir_collisions(paths: &[&str]) -> Result<()> { - let set: std::collections::BTreeSet<&str> = paths.iter().copied().collect(); +/// descendant of `p` after other names (`a`, `a-b`, `a/c`), so checking +/// neighbours is not enough; look each ancestor up instead. Public so the +/// private-branch tree builder, which assembles sealed trees itself, applies +/// the same rule (#278). +pub fn check_file_dir_collisions<'a>(paths: impl IntoIterator) -> Result<()> { + let set: std::collections::BTreeSet<&str> = paths.into_iter().collect(); for p in &set { for (i, _) in p.match_indices('/') { if set.contains(&p[..i]) { diff --git a/docs/adr/0034-sparse-checkouts.md b/docs/adr/0034-sparse-checkouts.md index b6e5397e..ef4a8f69 100644 --- a/docs/adr/0034-sparse-checkouts.md +++ b/docs/adr/0034-sparse-checkouts.md @@ -350,7 +350,7 @@ every claim below is checked against the shipped code, not the plan. even when the checkout now holds a directory at that path (or a file where the tip had a protected directory). Before, the commit wrote a tree with a blob and a tree under one name. Every snapshot now goes through one tree - builder (`scl_vfs::Vfs::build_root`) that refuses a path that is both a file + builder (`scl_vfs::Repo::build_root`) that refuses a path that is both a file and the parent of another path in the same snapshot, with `scl_vfs::Error::FileDirCollision { file, child }` naming both, before any tree object is written; the ref does not move. We refuse instead of letting @@ -363,6 +363,17 @@ every claim below is checked against the shipped code, not the plan. amend, merge/pick/rebase completion (`snapshot_files` ends in it), `sc ws harvest` (the error surfaces with no candidate branch and the session still open), `sc ws refresh` (an error before any write) and `sc work` (that - agent's harvest result is the error). Private branches build their sealed - trees separately and do not carry absent paths, so they are not affected. - Pinned by `crates/repo/src/file_dir_collision.rs`. + agent's harvest result is the error). Private branches carry absent + out-of-view and protected paths too and assemble sealed trees in + `private::build_tree_objects`, which runs the same check + (`scl_vfs::check_file_dir_collisions`) before building. A workspace that + collides stops `sc ws harvest` with the error (workspaces already landed + stay landed; the failing one stays live and can be fixed in place), unlike a + scanner hit, which is a per-workspace outcome. Limit: on a partial clone + the out-of-view graft (`graft_out_of_sparse`) runs after the builder and + resolves such a collision by itself; it is not covered by this check. + Snapshots written before this check may already hold the collision; they + still read, but a commit or replay that rebuilds their tree is refused. + Pinned for plain commit, merge completion, `ws harvest`, `ws refresh` and a + private commit by `crates/repo/src/file_dir_collision.rs`; amend, pick and + rebase completion share `snapshot_files` and are covered by construction. From ef57c079e4321e3974766b3082f2ce20929e90af Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 21 Oct 2026 08:47:52 +0300 Subject: [PATCH 3/4] fix(vfs): catch a built file and a carried entry at the same path; assert test cleanup (#278) --- crates/repo/src/file_dir_collision.rs | 1 + crates/vfs/src/lib.rs | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/repo/src/file_dir_collision.rs b/crates/repo/src/file_dir_collision.rs index 70aa347e..25de1931 100644 --- a/crates/repo/src/file_dir_collision.rs +++ b/crates/repo/src/file_dir_collision.rs @@ -64,6 +64,7 @@ fn keyless_checkout(tag: &str) -> (Repo, PathBuf, scl_core::ObjectId) { fn cleanup(repo: Repo, root: &Path) { drop(repo); std::fs::remove_dir_all(root).unwrap(); + assert!(!root.exists(), "test root must be removed"); } #[test] diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index e2a3e7f6..1bdd6ae9 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -305,7 +305,17 @@ impl Repo { /// private-branch tree builder, which assembles sealed trees itself, applies /// the same rule (#278). pub fn check_file_dir_collisions<'a>(paths: impl IntoIterator) -> Result<()> { - let set: std::collections::BTreeSet<&str> = paths.into_iter().collect(); + let mut set = std::collections::BTreeSet::new(); + for p in paths { + // The same path twice (a built file and a carried entry): the tree + // would hold two entries of one name. Reported with `file == child`. + if !set.insert(p) { + return Err(Error::FileDirCollision { + file: p.to_string(), + child: p.to_string(), + }); + } + } for p in &set { for (i, _) in p.match_indices('/') { if set.contains(&p[..i]) { @@ -615,7 +625,11 @@ mod tests { perms: scl_core::PROTECTED, }; assert!(r - .write_tree_with_carried(&[f("s/x")], &[("s".into(), carried)]) + .write_tree_with_carried(&[f("s/x")], &[("s".into(), carried.clone())]) + .is_err()); + // The same path as a built file and a carried entry is a collision too. + assert!(r + .write_tree_with_carried(&[f("s")], &[("s".into(), carried)]) .is_err()); // Siblings that merely share a name prefix are fine. assert!(r From bbd871304de475297e59d9934e2facd70e71b57c Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 21 Oct 2026 08:57:24 +0300 Subject: [PATCH 4/4] docs: name both tree builders and the publish limit (#278) --- CLAUDE.md | 2 +- docs/adr/0034-sparse-checkouts.md | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 65d89195..abb51843 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -340,7 +340,7 @@ transport-adjacent. The rest, imperatively: outside the view (a later commit would take the user's file instead, #248); there any directory at the path refuses, tracked contents or not. Merging into an unborn branch and the conflict stop of a merge into a private branch are not guarded (except that a private stop refuses a `.theirs` sidecar that would land on an existing file). - **A snapshot never holds a path as both a file and a directory (#278).** The - one tree builder (`scl_vfs::Repo::build_root`, and `private::build_tree_objects` for private branches) refuses it with + tree builders (`scl_vfs::Repo::build_root`, and `private::build_tree_objects` for private branches) refuse it with `FileDirCollision { file, child }` before writing any tree. It matters because an absent protected or out-of-view file is carried back verbatim: creating a directory of that name in a checkout without the key (or outside the view) diff --git a/docs/adr/0034-sparse-checkouts.md b/docs/adr/0034-sparse-checkouts.md index ef4a8f69..426338fb 100644 --- a/docs/adr/0034-sparse-checkouts.md +++ b/docs/adr/0034-sparse-checkouts.md @@ -373,7 +373,9 @@ every claim below is checked against the shipped code, not the plan. the out-of-view graft (`graft_out_of_sparse`) runs after the builder and resolves such a collision by itself; it is not covered by this check. Snapshots written before this check may already hold the collision; they - still read, but a commit or replay that rebuilds their tree is refused. + still read, but a commit or replay that rebuilds their tree is refused, and + `sc branch publish` (which rebuilds every inner snapshot) refuses a private + branch whose history holds one. Pinned for plain commit, merge completion, `ws harvest`, `ws refresh` and a private commit by `crates/repo/src/file_dir_collision.rs`; amend, pick and rebase completion share `snapshot_files` and are covered by construction.