From 7e9bf8710217b7b9f1bd7fdf546e4c28b4bd09c9 Mon Sep 17 00:00:00 2001 From: Victor Garcia Date: Sat, 8 Aug 2026 02:51:56 -0700 Subject: [PATCH 1/2] fix: a worktree on another branch is not this session's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `locate` took `recorded_branch` but never passed it to `inspect`, so any worktree of the same repository at the recorded path counted as the session's own. Remove a session's worktree, create another at that path on a different branch, resume the session — and the agent commits its work to whatever branch it found. In the feature whose entire purpose is isolation. inspect now requires the checked-out branch to match, alongside the shared git directory and the worktree top level. A detached HEAD deliberately does not match: the recording names a branch, and resuming onto a detached head would leave the work unreachable by that name. Also: a regular file at the recorded path was reported Gone, which sends resume down the recreate path where `git worktree add` fails on the occupied path with an error that never mentions the file. Anything that exists but is not the session's worktree is now Foreign, so resume reports Occupied and names what is in the way. Found by CodeRabbit on #14 and left unresolved when that PR merged. Refs #13 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3 --- crates/cli/src/worktree.rs | 117 ++++++++++++++++++++++++++++++++++--- 1 file changed, 108 insertions(+), 9 deletions(-) diff --git a/crates/cli/src/worktree.rs b/crates/cli/src/worktree.rs index 6fe6b83..b2084c5 100644 --- a/crates/cli/src/worktree.rs +++ b/crates/cli/src/worktree.rs @@ -246,32 +246,65 @@ pub fn locate( let Some((path, branch)) = recorded_path.zip(recorded_branch) else { return Location::Shared; }; - let candidate = inspect(anchor, Path::new(path)); + let candidate = inspect(anchor, Path::new(path), branch); let branch_lives = candidate == Candidate::Gone && repo_root(anchor).is_ok_and(|root| branch_exists(&root, branch)); decide(Some((path, branch)), candidate, branch_lives) } -/// Whether `path` really is a worktree of the repository `anchor` sits in. -/// `is_dir` alone would accept an ordinary directory left at the recorded -/// path — running there is the silent misplacement the recording exists to -/// prevent — so both the shared git directory and the worktree's own top -/// level have to agree. -fn inspect(anchor: &Path, path: &Path) -> Candidate { - if !path.is_dir() { +/// Whether `path` really is *this session's* worktree of the repository +/// `anchor` sits in. Three things have to agree, and each guards a different +/// way of ending up in the wrong tree: +/// +/// - the shared git directory, so a worktree of another repository is not +/// mistaken for this one; +/// - the worktree's own top level, so an ordinary directory left at the +/// recorded path is not run in; +/// - the checked-out branch, so a *different* worktree of the same +/// repository restored at this path does not capture the session. Without +/// this a resumed session commits to whatever branch happens to be there. +/// +/// Anything that exists at the path but is not that worktree is `Foreign`, +/// including a regular file: `Gone` would send resume down the recreate path, +/// where `git worktree add` fails on the occupied path with an error that +/// says nothing about what is actually in the way. +fn inspect(anchor: &Path, path: &Path, branch: &str) -> Candidate { + if !path.exists() { return Candidate::Gone; } + if !path.is_dir() { + return Candidate::Foreign; + } let same_repo = rev_parse_dir(path, "--git-common-dir") .zip(rev_parse_dir(anchor, "--git-common-dir")) .is_some_and(|(candidate, anchor)| candidate == anchor); let is_top_level = repo_root(path).is_ok_and(|top| canonical(&top) == canonical(path)); - if same_repo && is_top_level { + let same_branch = head_branch(path).is_some_and(|head| head == branch); + if same_repo && is_top_level && same_branch { Candidate::Live } else { Candidate::Foreign } } +/// The branch checked out at `path`, or `None` when the worktree is detached +/// or the path is not a worktree at all. A detached HEAD is deliberately not +/// a match: the recording names a branch, and resuming onto a detached head +/// would leave the work unreachable by that name. +fn head_branch(path: &Path) -> Option { + let out = Command::new("git") + .arg("-C") + .arg(path) + .args(["symbolic-ref", "--quiet", "--short", "HEAD"]) + .output() + .ok()?; + if !out.status.success() { + return None; + } + let name = String::from_utf8_lossy(&out.stdout).trim().to_string(); + (!name.is_empty()).then_some(name) +} + fn canonical(path: &Path) -> PathBuf { path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) } @@ -444,6 +477,72 @@ mod tests { ); } + /// The path and the repository can both match while the branch does not. + /// Removing a session's worktree and putting another one from the same + /// repository at that path must not capture the session — resuming into + /// it would commit the agent's work to whatever branch it found. + #[test] + fn a_worktree_of_the_same_repo_on_another_branch_is_not_this_session() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + + create(&root, &path, "bullpen/s1").unwrap(); + assert_eq!( + locate(path.to_str(), Some("bullpen/s1"), &root), + Location::Use(path.clone()) + ); + + // Same path, same repository, different branch — what a user gets by + // clearing the directory and reusing the path. `prune` drops git's + // administrative entry for the deleted worktree, which otherwise + // still claims the path. + std::fs::remove_dir_all(&path).unwrap(); + assert!( + Command::new("git") + .arg("-C") + .arg(&root) + .args(["worktree", "prune"]) + .output() + .unwrap() + .status + .success() + ); + create(&root, &path, "someone-elses-branch").unwrap(); + assert_eq!( + locate(path.to_str(), Some("bullpen/s1"), &root), + Location::Occupied { + path: path.clone(), + branch: "bullpen/s1".into() + } + ); + } + + /// A regular file at the recorded path is in the way, not absent. + /// Reporting it as gone sends resume down the recreate path, where + /// `git worktree add` fails with an error that never mentions the file. + #[test] + fn a_regular_file_at_the_recorded_path_is_occupied_not_gone() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + + // The branch exists, so "gone" would mean Recreate. + create(&root, &path, "bullpen/s1").unwrap(); + std::fs::remove_dir_all(&path).unwrap(); + std::fs::write(&path, "not a worktree").unwrap(); + + assert_eq!( + locate(path.to_str(), Some("bullpen/s1"), &root), + Location::Occupied { + path, + branch: "bullpen/s1".into() + } + ); + } + #[test] fn an_ordinary_checkout_needs_no_write_roots_beyond_itself() { let dir = tempfile::tempdir().unwrap(); From 0b1ac24c3bf2f484d9be07ec22825bed20b201dd Mon Sep 17 00:00:00 2001 From: Victor Garcia Date: Sat, 8 Aug 2026 03:04:14 -0700 Subject: [PATCH 2/2] fix: classify a dangling symlink and a detached HEAD as occupied Two more ways the recorded path can hold something that is not this session's worktree. `Path::exists` follows symlinks, so a dangling link at the recorded path reported itself absent. Resume then took the recreate branch and git refused, because the link does occupy the path. `symlink_metadata` sees the link itself. A detached worktree has no branch to agree with, and accepting one would let a resumed session commit where the recorded branch name can never reach the work again. Worth stating because the obvious shape gets it backwards: `git symbolic-ref -q HEAD` exits nonzero when detached, so treating command failure as agreement silently accepts exactly the case it should refuse. Refs #13 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3 --- crates/cli/src/worktree.rs | 75 +++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/crates/cli/src/worktree.rs b/crates/cli/src/worktree.rs index b2084c5..18d8d7a 100644 --- a/crates/cli/src/worktree.rs +++ b/crates/cli/src/worktree.rs @@ -269,7 +269,10 @@ pub fn locate( /// where `git worktree add` fails on the occupied path with an error that /// says nothing about what is actually in the way. fn inspect(anchor: &Path, path: &Path, branch: &str) -> Candidate { - if !path.exists() { + // `symlink_metadata` rather than `exists`, which follows links: a dangling + // symlink at the recorded path is an obstruction that reports itself + // absent, and `Gone` would send resume into a recreate git then refuses. + if std::fs::symlink_metadata(path).is_err() { return Candidate::Gone; } if !path.is_dir() { @@ -543,6 +546,76 @@ mod tests { ); } + /// A dangling symlink is an obstruction that reports itself absent. + /// `Path::exists` follows the link and says no; `symlink_metadata` sees + /// the link itself. Getting this wrong sends resume into a recreate that + /// git refuses, because something does occupy the path. + #[test] + fn a_dangling_symlink_at_the_recorded_path_is_occupied_not_gone() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + + // The branch exists, so a wrong "gone" would mean Recreate. + create(&root, &path, "bullpen/s1").unwrap(); + std::fs::remove_dir_all(&path).unwrap(); + std::os::unix::fs::symlink(dir.path().join("nowhere"), &path).unwrap(); + assert!(!path.exists(), "the link dangles"); + + assert_eq!( + locate(path.to_str(), Some("bullpen/s1"), &root), + Location::Occupied { + path, + branch: "bullpen/s1".into() + } + ); + } + + /// A detached worktree has no branch to agree with. Accepting it would let + /// a resumed session commit where the recorded branch name can never reach + /// the work again. `git symbolic-ref -q HEAD` exits nonzero when detached, + /// so a check that treats failure as agreement gets this backwards. + #[test] + fn a_detached_worktree_at_the_recorded_path_is_not_this_session() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + + let git = |args: &[&str]| { + assert!( + Command::new("git") + .arg("-C") + .arg(&root) + .args(args) + .output() + .unwrap() + .status + .success(), + "git {args:?}" + ); + }; + create(&root, &path, "bullpen/s1").unwrap(); + std::fs::remove_dir_all(&path).unwrap(); + git(&["worktree", "prune"]); + git(&[ + "worktree", + "add", + "--detach", + path.to_str().unwrap(), + "HEAD", + ]); + + assert_eq!( + locate(path.to_str(), Some("bullpen/s1"), &root), + Location::Occupied { + path, + branch: "bullpen/s1".into() + } + ); + } + #[test] fn an_ordinary_checkout_needs_no_write_roots_beyond_itself() { let dir = tempfile::tempdir().unwrap();