session: no git command runs without a directory, and the checkout guard reads a linked worktree's own share

docs/changes/unreleased/1099-no-git-command-without-a-directory.md

@@ -0,0 +1,14 @@
+---
+kind: fixed
+title: No git command runs without a directory, and the checkout guard reads a linked worktree's own share
+pr: 1099
+surface: [engine]
+invalidates:
+  - "`worktreeDirtIn` and `UnsavedEditsNote` built `git -C <dir>` themselves, and `git -C \"\"` is a no-op, so an empty directory ran the command where the process stood. Both go through `gitWith` now (`gitContext` is the same call under a caller's context), where the refusal of an empty directory and the pinned environment live once."
+  - "`taskTree.releaseLanded` removed a worktree and deleted its branch with no empty-root guard and without the root lock. It refuses a tree with no root or directory and takes the root lock, as `releaseKept` does."
+  - "The session package's checkout guard read branches and worktree registrations for the whole shared clone, so from a linked worktree a sibling's task/* churn was reported as the run's damage, and its live-codeaf probe walked `/proc`, which does not exist on darwin. From a linked worktree the branch reading is skipped and the worktree reading keeps only registrations under the checkout; the probe reads the process table."
+---
+
+A command with no directory runs wherever the process happens to be. Every
+spelling of a git call in this package is a wrapper of one function that
+refuses that, and the two that had gone round it are wrappers of it now.

internal/session/groundladder.go

@@ -1147,6 +1147,19 @@ func (t taskTree) carryBranchHomeLocked() (string, error) {
 // session's own and goes with the work it carried; what furrow is told is that
 // the record may be forgotten ([taskTree.dropUniverse]).
 func (t taskTree) releaseLanded() {
+	// THE SAME DOOR [taskTree.releaseKept] closes: a tree whose ground was never
+	// made has nowhere a worktree could be registered and no repository to ask,
+	// and a command with no directory runs in the process's own directory — so
+	// the removal below would take a working copy out of somebody else's
+	// checkout and the branch deletion would name a branch in it.
+	if strings.TrimSpace(t.root) == "" || strings.TrimSpace(t.dir) == "" {
+		return
+	}
+	// THE ROOT LOCK IS THE CALLER'S. Every road that reaches here is a landing
+	// that already holds it ([taskTree.carryBranchHome]'s span, task_run.go),
+	// the same way [taskTree.releaseKeptLocked] is reached, and the lock is not
+	// re-entrant: taking it again here waited on itself for the whole of a
+	// suite's timeout.
 	if t.ownRepository() {
 		_ = os.RemoveAll(t.dir)
 	} else if _, err := git(t.root, "worktree", "remove", t.dir); err != nil {

internal/session/hermetic_checkout_test.go

@@ -4,7 +4,9 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"sort"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -66,16 +68,28 @@ import (
 // do not care what anything is called and still catch the commit and the files,
 // which is the shape that actually cost somebody an afternoon.
 //
+// AND A LINKED WORKTREE OF A SHARED CLONE READS LESS STILL. Branches belong to
+// the clone's common directory, not to the working copy they were read from, so
+// from a linked worktree every sibling's task/* churn — a branch cut for their
+// own run, a branch deleted when their run landed — would look exactly like
+// this run's damage, and the branch reading is skipped outright there. The
+// worktree reading narrows instead of skipping: registrations under this root
+// can still be this suite's, and a sibling's registration elsewhere in the
+// clone cannot.
+//
 // It cannot see a content change to a file that was already dirty before the
 // run — hashing the whole checkout each run costs more than it is worth — and
 // the incident's shape, new commits, new files, new branches and new worktrees,
 // is what it catches.
 
 // checkout is what the tree looked like at a moment: the commit it stood on,
 // the paths git considered dirty or unknown, the branches it had, and the
-// worktrees registered against it.
+// worktrees registered against it. When the tree is a linked worktree of a
+// shared clone, branches is nil — the reading is skipped, not empty — and the
+// worktrees are only the ones registered under the tree itself.
 type checkout struct {
 	root      string
+	linked    bool
 	head      string
 	dirty     map[string]bool
 	branches  map[string]bool
@@ -99,12 +113,38 @@ func watchTheCheckout() checkout {
 	if watched.head, err = gitLine(root, "rev-parse", "HEAD"); err != nil {
 		return checkout{}
 	}
+	watched.linked = isLinkedWorktree(root)
+	if !watched.linked {
+		watched.branches = branchNames(root)
+	}
 	watched.dirty = porcelain(root)
-	watched.branches = branchNames(root)
-	watched.worktrees = worktreePaths(root)
+	watched.worktrees = worktreePaths(root, watched.linked)
 	return watched
 }
 
+// isLinkedWorktree says whether the working copy at root is one worktree among
+// several of one shared clone, which `rev-parse` answers by naming two
+// different directories: the repository this copy carries its state in, and
+// the common one every sibling of the same clone points at. A plain checkout
+// answers the same directory twice.
+func isLinkedWorktree(root string) bool {
+	gitDir, err := gitLine(root, "rev-parse", "--git-dir")
+	if err != nil || gitDir == "" {
+		return false
+	}
+	commonDir, err := gitLine(root, "rev-parse", "--git-common-dir")
+	if err != nil || commonDir == "" {
+		return false
+	}
+	if !filepath.IsAbs(gitDir) { // a plain checkout answers relative to root
+		gitDir = filepath.Join(root, gitDir)
+	}
+	if !filepath.IsAbs(commonDir) {
+		commonDir = filepath.Join(root, commonDir)
+	}
+	return filepath.Clean(gitDir) != filepath.Clean(commonDir)
+}
+
 // moved says how the checkout differs from what it was, and "" when it does
 // not. A watch that was never taken never reports.
 func (c checkout) moved() string {
@@ -127,33 +167,40 @@ func (c checkout) moved() string {
 	if moved := differing(c.dirty, porcelain(c.root)); len(moved) > 0 {
 		said = append(said, "these paths are not what they were before the run:\n\t"+strings.Join(moved, "\n\t"))
 	}
-	// A BRANCH IS A WRITE THAT MOVES NOTHING. `task/do-the-thing-<hex>` cut in
-	// the person's repository leaves HEAD exactly where it was and leaves the
-	// porcelain empty, and it is still the suite committing into a tree it does
-	// not own — the merge that would have moved HEAD is the only part that did
-	// not happen yet.
-	//
-	// AND A LIVE codeaf CREATES THE SAME BRANCHES. When another codeaf is using
-	// this checkout and TMPDIR is outside it, branch and worktree churn is
-	// ambient the way journal growth is (hermetic_test.go) — report the head
-	// and the porcelain, which a neighbour's ordinary work does not move, and
-	// leave the shared refs alone. The #578 TempDir-inside-checkout case still
-	// reports everything, because that is when the suite itself can mint them.
-	ignoreAmbientRefs := liveCodeafUsingCheckout(c.root) && !tempDirInsideCheckout(c.root)
-	if !ignoreAmbientRefs {
-		if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 {
-			said = append(said, "these branches are not what they were before the run:\n\t"+strings.Join(moved, "\n\t"))
-		}
-		// AND A WORKTREE IS A WRITE GIT IS TOLD TO IGNORE. The ground ladder puts a
-		// task's tree under `.codeaf/`, which .gitignore covers, so the porcelain
-		// above stays silent about a whole second checkout sitting in the person's
-		// tree. The registration is not ignorable: git keeps it, so it is asked for.
-		if moved := differing(c.worktrees, worktreePaths(c.root)); len(moved) > 0 {
+	// BRANCHES AND WORKTREES BELONG TO THE WHOLE SHARED CLONE the checkout was
+	// read from, and a LINKED worktree reads even less than the ambient case:
+	// every sibling of the same clone churns the same task/* refs this suite
+	// could not have written, so from a linked worktree the branch reading is
+	// skipped outright and the worktree reading keeps only the registrations
+	// under the checkout itself. The head and the porcelain above still watch
+	// everything, whatever the checkout is.
+	if c.linked {
+		if moved := differing(c.worktrees, worktreePaths(c.root, true)); len(moved) > 0 {
 			said = append(said, "these worktrees are not what they were before the run:\n\t"+strings.Join(moved, "\n\t"))
 		}
-	} else if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 ||
-		len(differing(c.worktrees, worktreePaths(c.root))) > 0 {
-		fmt.Fprintf(os.Stderr, "checkout guard: another codeaf is using %s; ignoring task branch/worktree churn (head and porcelain still watched)\n", c.root)
+	} else {
+		// AND A LIVE codeaf CREATES THE SAME BRANCHES. When another codeaf is using
+		// this checkout and TMPDIR is outside it, branch and worktree churn is
+		// ambient the way journal growth is (hermetic_test.go) — report the head
+		// and the porcelain, which a neighbour's ordinary work does not move, and
+		// leave the shared refs alone. The #578 TempDir-inside-checkout case still
+		// reports everything, because that is when the suite itself can mint them.
+		ignoreAmbientRefs := liveCodeafUsingCheckout(c.root) && !tempDirInsideCheckout(c.root)
+		if !ignoreAmbientRefs {
+			if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 {
+				said = append(said, "these branches are not what they were before the run:\n\t"+strings.Join(moved, "\n\t"))
+			}
+			// AND A WORKTREE IS A WRITE GIT IS TOLD TO IGNORE. The ground ladder puts a
+			// task's tree under `.codeaf/`, which .gitignore covers, so the porcelain
+			// above stays silent about a whole second checkout sitting in the person's
+			// tree. The registration is not ignorable: git keeps it, so it is asked for.
+			if moved := differing(c.worktrees, worktreePaths(c.root, false)); len(moved) > 0 {
+				said = append(said, "these worktrees are not what they were before the run:\n\t"+strings.Join(moved, "\n\t"))
+			}
+		} else if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 ||
+			len(differing(c.worktrees, worktreePaths(c.root, false))) > 0 {
+			fmt.Fprintf(os.Stderr, "checkout guard: another codeaf is using %s; ignoring task branch/worktree churn (head and porcelain still watched)\n", c.root)
+		}
 	}
 	if len(said) == 0 {
 		return ""
@@ -167,7 +214,7 @@ func (c checkout) moved() string {
 		"`rev-parse --show-toplevel` answers with this checkout (task_run.go's repositoryRoot, " +
 		"#578). Which one it was, this guard cannot tell you; if the run had GOTMPDIR or TMPDIR " +
 		"pointing inside a checkout, suspect the second. Undo the above before pushing.\n" +
-		"(a second checkout running this same suite beside you cannot cause this; a test that names no directory can, and so can a temporary directory that is not outside your tree. A live codeaf using this checkout is ignored for branch/worktree churn when TMPDIR is outside the tree.)"
+		"(a second checkout running this same suite beside you cannot cause this; a test that names no directory can, and so can a temporary directory that is not outside your tree. A live codeaf using this checkout is ignored for branch/worktree churn when TMPDIR is outside the tree. When this checkout is a linked worktree of a shared clone, branch churn is not reported at all and the worktree reading covers only registrations under the checkout, because both belong to the clone the siblings share — a leak that only cut a task/ branch from inside a linked worktree slips past here, and the head and the porcelain above are what catch it.)"
 }
 
 // differing names every line that is in one listing and not the other, marked
@@ -249,22 +296,35 @@ func harnessBranch(name string) bool {
 	return strings.HasPrefix(name, harnessBranchPrefix)
 }
 
-// worktreePaths is the set of worktree directories registered against root that
-// THIS SUITE could have registered — including one it hid under an ignored
-// folder, which is the whole reason this reading exists.
+// worktreePaths is the set of worktree directories registered against root
+// that THIS SUITE could have registered — including one it hid under an ignored
+// folder, which is the whole reason this reading exists. For a linked worktree
+// it is asked with underRoot, because the listing answers for the whole shared
+// clone and only the registrations under the checkout itself can be the
+// suite's.
 //
 // ONLY THE PATHS ARE KEPT. The plain listing carries each worktree's head
 // beside its path, so keeping the whole line would report a head move a second
 // time, in different words, from a set that is meant to answer a different
 // question. The porcelain form is asked for so the path and the branch can be
 // read separately, and an unreadable tree is an empty set as everywhere else
 // here.
-func worktreePaths(root string) map[string]bool {
+func worktreePaths(root string, underRoot bool) map[string]bool {
 	out, err := gitLines(root, "worktree", "list", "--porcelain")
 	if err != nil {
 		return map[string]bool{}
 	}
-	return harnessWorktrees(root, out)
+	found := harnessWorktrees(root, out)
+	if !underRoot {
+		return found
+	}
+	under := strings.TrimSuffix(root, "/") + "/"
+	for path := range found {
+		if !strings.HasPrefix(path, under) {
+			delete(found, path)
+		}
+	}
+	return found
 }
 
 // harnessWorktrees reads `git worktree list --porcelain` — one record per
@@ -440,58 +500,104 @@ func TestTheCheckoutGuardReportsOnlyWhatThisRunCouldHaveWritten(t *testing.T) {
 	}
 }
 
+// TestTheCheckoutGuardDoesNotReportASiblingWorktreesBranch is the shared-clone
+// case, run for real: a linked worktree stands beside a sibling in one clone,
+// the sibling cuts and takes away task/ branches and registers a worktree of
+// its own while "the run" is watched, and none of it may read as this run's
+// damage — branches belong to the clone's common directory, and the sibling's
+// registrations stand outside the watched tree. The second half proves the
+// churn is real: watched from the main checkout, where the refs are this
+// checkout's own, the same branch deletion is reported.
+func TestTheCheckoutGuardDoesNotReportASiblingWorktreesBranch(t *testing.T) {
+	shared := newTestRepo(t)
+	linked := filepath.Join(shared, "linked")
+	sibling := filepath.Join(shared, "sibling")
+	mustGit(t, shared, "worktree", "add", "-b", "task/this-run-0a1b2c", linked)
+	mustGit(t, shared, "worktree", "add", "-b", "task/sibling-run-9f8e7d", sibling)
+	// A branch the sibling will take away during the run, seen only through
+	// the clone's shared refs.
+	mustGit(t, shared, "branch", "task/sibling-gone-554433")
+
+	t.Chdir(linked)
+	watched := watchTheCheckout()
+	if !watched.linked {
+		t.Fatal("the linked worktree was not read as linked, so this test is not standing where it means to")
+	}
+	// THE SAME REFS READ FROM THE MAIN CHECKOUT, before the churn, are this
+	// checkout's own — that watch is the contrast the narrowing has to hold
+	// against.
+	head, _ := gitLine(shared, "rev-parse", "HEAD")
+	watchedMain := checkout{root: shared, head: head,
+		dirty: porcelain(shared), branches: branchNames(shared), worktrees: worktreePaths(shared, false)}
+
+	mustGit(t, sibling, "branch", "task/sibling-cuts-778899")
+	mustGit(t, sibling, "branch", "-D", "task/sibling-gone-554433")
+	mustGit(t, shared, "worktree", "add", "-b", "task/sibling-tree-112233", filepath.Join(shared, "sibling-tree"))
+
+	if said := watched.moved(); said != "" {
+		t.Fatalf("a sibling worktree's churn was reported as this run's damage:\n%s", said)
+	}
+
+	// AND THE SAME CHURN IS REPORTED FROM THE MAIN CHECKOUT — the narrowing
+	// above is tied to the linked worktree, not to the branch having been
+	// someone else's.
+	if said := watchedMain.moved(); !strings.Contains(said, "gone: task/sibling-gone-554433") {
+		t.Fatalf("from the main checkout the sibling's deleted branch was not reported:\n%s", said)
+	}
+}
+
 // liveCodeafUsingCheckout reports whether another codeaf process looks like it
 // is working in this repository right now — a chat, a tick, a hosted session —
 // whose task/ branches and `.codeaf/` worktrees would otherwise look exactly
 // like the suite's own leak.
 //
-// IT IS BEST-EFFORT AND FAILS OPEN TO "NO". A machine without /proc, a binary
+// THE QUESTION IS ASKED OF THE PROCESS TABLE, which exists wherever this
+// program does: any other process whose command line names a worktree of the
+// repository root belongs to is taken for a live codeaf. The worktrees come
+// from `git worktree list --porcelain` — the checkout itself, and every tree
+// registered against the same clone. A /proc walk was tried first and
+// abandoned: it exists on Linux only, so on darwin the answer was always
+// "no", and the ambient reading never engaged on the machine this suite is
+// written on.
+//
+// IT IS BEST-EFFORT AND FAILS OPEN TO "NO". A machine without ps, a binary
 // renamed something else, or a sandbox that hides other processes simply keeps
 // the stricter reading; a false negative is a red that a person can re-run with
 // codeaf closed, and a false positive would hide a real suite write.
 func liveCodeafUsingCheckout(root string) bool {
-	under := strings.TrimSuffix(root, "/") + "/"
-	entries, err := os.ReadDir("/proc")
+	lines, err := gitLines(root, "worktree", "list", "--porcelain")
 	if err != nil {
 		return false
 	}
-	self := os.Getpid()
-	for _, entry := range entries {
-		if !entry.IsDir() {
-			continue
-		}
-		name := entry.Name()
-		if len(name) == 0 || name[0] < '0' || name[0] > '9' {
-			continue
-		}
-		var pid int
-		for i := 0; i < len(name); i++ {
-			if name[i] < '0' || name[i] > '9' {
-				pid = -1
-				break
-			}
-			pid = pid*10 + int(name[i]-'0')
+	places := map[string]bool{root: true}
+	for _, line := range lines {
+		if rest, ok := strings.CutPrefix(line, "worktree "); ok {
+			places[strings.TrimSpace(rest)] = true
 		}
-		if pid <= 0 || pid == self {
+	}
+	out, err := exec.Command("ps", "-axo", "pid=,command=").Output()
+	if err != nil {
+		return false
+	}
+	self := os.Getpid()
+	for _, line := range strings.Split(string(out), "\n") {
+		pid, command, found := strings.Cut(strings.TrimSpace(line), " ")
+		if !found {
 			continue
 		}
-		cmdline, err := os.ReadFile("/proc/" + name + "/cmdline")
-		if err != nil {
+		// The suite's own test binary stands IN a worktree but does not name one
+		// on its command line, so it never reads as a neighbour here.
+		own, err := strconv.Atoi(strings.TrimSpace(pid))
+		if err != nil || own <= 0 || own == self {
 			continue
 		}
-		joined := strings.ReplaceAll(string(cmdline), "\x00", " ")
-		if !strings.Contains(joined, "codeaf") {
+		if !strings.Contains(command, "codeaf") {
 			continue
 		}
-		// The suite's own test binary and helpers name codeaf too (`go test`,
-		// `codeaf-session-test-home`). A live chat is the binary itself, or a
-		// tick/host child, with the checkout in its cwd or its arguments.
-		cwd, err := os.Readlink("/proc/" + name + "/cwd")
-		if err == nil && (cwd == root || strings.HasPrefix(cwd, under)) {
-			return true
-		}
-		if strings.Contains(joined, root) {
-			return true
+		for place := range places {
+			if strings.Contains(command, place) {
+				return true
+			}
 		}
 	}
 	return false