mcpx: key scopes by directory identity so a wrong-case cwd shares one instance (#1912)

pkgs/mcpx/internal/config/gitdiscover_darwin_test.go

@@ -34,12 +34,13 @@ func TestGitParityTmpAndPrivateTmp(t *testing.T) {
 }
 
 func TestGitCaseOfAWrongCaseCwdIsKept(t *testing.T) {
-	// On a case-insensitive volume git answers with the on-disk spelling,
-	// because it starts from getcwd(). Native starts from the path it was
-	// given, as the cwd scope always has; getting the on-disk spelling
-	// means F_GETPATH on macOS, which Go reaches only through unsafe calls.
-	// Two spellings of one checkout therefore get two instances -- the
-	// over-isolating direction -- and nothing else differs.
+	// Inverted by the #1912 fix; this test used to pin the bug. Native
+	// discovery still keeps the caller's spelling -- getting the on-disk
+	// spelling means F_GETPATH on macOS, which Go reaches only through
+	// unsafe calls -- but that no longer splits the instance: scope keys
+	// key on directory identity (dev, inode), so the two spellings of one
+	// checkout resolve to one key on the cwd, repo and worktree scopes
+	// alike, where before they got two instances.
 	f := newGitFix(t)
 	r := f.repo("CaseRepo")
 	lower := filepath.Join(filepath.Dir(r), "caserepo")
@@ -57,4 +58,11 @@ func TestGitCaseOfAWrongCaseCwdIsKept(t *testing.T) {
 	if w.worktree != lower || !strings.EqualFold(w.worktree, top) {
 		t.Errorf("native should keep the given spelling and differ only in case: %s vs %s", w.worktree, top)
 	}
+	for _, sc := range []Scope{ScopeCwd, ScopeRepo, ScopeWorktree} {
+		a, _ := sc.Key(CallContext{Cwd: r})
+		b, _ := sc.Key(CallContext{Cwd: lower})
+		if a != b {
+			t.Errorf("scope %s: two spellings of one directory must key together: %q vs %q", sc, a, b)
+		}
+	}
 }

pkgs/mcpx/internal/config/gitsys_other.go

@@ -9,3 +9,9 @@ import "os"
 var gitDevice = func(string) (uint64, error) { return 0, nil }
 
 func accessX(path string) bool { _, err := os.Stat(path); return err == nil }
+
+// dirIdentity has no portable non-unix form: a Windows file index needs a
+// handle API package syscall does not expose. Returning not-ok makes scope
+// keys fall back to the canonical path alone, which is today's behaviour --
+// over-isolation, never accidental sharing.
+var dirIdentity = func(string) (uint64, uint64, bool) { return 0, 0, false }

pkgs/mcpx/internal/config/gitsys_unix.go

@@ -21,3 +21,17 @@ var gitDevice = func(path string) (uint64, error) {
 // accessX is access(path, X_OK), which is how git checks objects/ and refs/:
 // present and searchable, not merely present.
 func accessX(path string) bool { return syscall.Access(path, xOK) == nil }
+
+// dirIdentity is a directory's (device, inode) pair: the identity two
+// spellings of one directory share and two different directories never do,
+// on either kind of volume. A variable, like gitDevice, so tests can put a
+// case-insensitive volume where they cannot mount one.
+var dirIdentity = func(path string) (uint64, uint64, bool) {
+	var st syscall.Stat_t
+	if err := syscall.Stat(path, &st); err != nil {
+		return 0, 0, false
+	}
+	// Dev is int32 on darwin, uint64 on linux; Ino is uint64 on both. Only
+	// equality matters.
+	return uint64(st.Dev), uint64(st.Ino), true
+}

pkgs/mcpx/internal/config/scope.go

@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"sync"
 )
 
 // CallContext is what a caller knows about itself. The daemon resolves a
@@ -64,7 +65,7 @@ func (s Scope) Key(ctx CallContext) (key string, degraded string) {
 		if ctx.Cwd == "" {
 			return fallback(ctx), "cwd is unknown"
 		}
-		return "cwd:" + canonical(ctx.Cwd), ""
+		return "cwd:" + keyPath(ctx.Cwd), ""
 
 	case ScopeRepo, ScopeWorktree:
 		if ctx.Cwd == "" {
@@ -76,13 +77,13 @@ func (s Scope) Key(ctx CallContext) (key string, degraded string) {
 		w, _, err := locateGit(ctx.Cwd)
 		switch {
 		case err != nil:
-			return "cwd:" + canonical(ctx.Cwd), err.Error()
+			return "cwd:" + keyPath(ctx.Cwd), err.Error()
 		case s == ScopeRepo:
-			return "repo:" + w.commonDir, ""
+			return "repo:" + keyPath(w.commonDir), ""
 		case w.worktree != "":
-			return "worktree:" + w.worktree, ""
+			return "worktree:" + keyPath(w.worktree), ""
 		default:
-			return "cwd:" + canonical(ctx.Cwd), "no worktree: " + w.noWorktree
+			return "cwd:" + keyPath(ctx.Cwd), "no worktree: " + w.noWorktree
 		}
 
 	case ScopeSession:
@@ -154,3 +155,52 @@ func canonical(p string) string {
 	}
 	return filepath.Clean(p)
 }
+
+// keyPath is canonical plus directory identity, for use as a scope key
+// (#1912). canonical keeps the caller's spelling, so on a case-insensitive
+// volume (APFS by default, SMB/vfat mounts) /Users/x/Repo and /users/x/repo
+// -- one directory -- keyed separately and spawned a second instance. The
+// error was always over-isolation, never accidental sharing.
+//
+// The fix keys on identity, not spelling: stat the canonical path and look
+// its (device, inode) up in a side table that remembers the first spelling
+// seen for that identity; every spelling of the directory keys to the
+// first. The key stays a plain readable path, which is what `mcpx daemons`
+// and the degraded-scope log print. Two genuinely different directories
+// that differ only by case have different inodes, so a case-sensitive
+// volume can never merge them -- the unsafe direction is impossible.
+//
+// When the stat fails (a deleted or unreadable cwd) or the platform has
+// no identity to give, the canonical path alone is the key: today's
+// behaviour, and still the safe direction. Keys are computed by the
+// daemon and held in memory for its lifetime, never persisted, so an
+// identity that changes across a filesystem rebuild needs no migration;
+// the table holds one entry per distinct directory actually scoped.
+var identitySpellings = struct {
+	sync.Mutex
+	byID map[[2]uint64]string
+}{byID: map[[2]uint64]string{}}
+
+func keyPath(p string) string {
+	c := canonical(p)
+	dev, ino, ok := dirIdentity(c)
+	if !ok {
+		return c
+	}
+	id := [2]uint64{dev, ino}
+	identitySpellings.Lock()
+	defer identitySpellings.Unlock()
+	if first, seen := identitySpellings.byID[id]; seen && first != c {
+		// Reuse is only sound while the remembered spelling still names
+		// this identity. An inode is recycled when its directory is
+		// deleted, and the recycled number must not resurrect the dead
+		// directory's spelling as a live directory's key.
+		if fdev, fino, fok := dirIdentity(first); !fok || fdev != dev || fino != ino {
+			identitySpellings.byID[id] = c
+			return c
+		}
+		return first
+	}
+	identitySpellings.byID[id] = c
+	return c
+}

pkgs/mcpx/internal/config/scope_identity_internal_test.go

@@ -0,0 +1,94 @@
+package config
+
+import "testing"
+
+// #1912, Approach C: scope keys key on directory identity. dirIdentity is
+// stubbed here the way gitDevice is stubbed elsewhere, so both volume kinds
+// are covered on any OS: a case-insensitive volume is exactly the case
+// where two spellings stat to one identity.
+
+func stubIdentity(t *testing.T, ids map[string][2]uint64) {
+	t.Helper()
+	orig := dirIdentity
+	dirIdentity = func(path string) (uint64, uint64, bool) {
+		id, ok := ids[path]
+		if !ok {
+			return 0, 0, false
+		}
+		return id[0], id[1], true
+	}
+	identitySpellings.Lock()
+	saved := identitySpellings.byID
+	identitySpellings.byID = map[[2]uint64]string{}
+	identitySpellings.Unlock()
+	t.Cleanup(func() {
+		dirIdentity = orig
+		identitySpellings.Lock()
+		identitySpellings.byID = saved
+		identitySpellings.Unlock()
+	})
+}
+
+func TestKeyPathTwoSpellingsOfOneDirectoryKeyTogether(t *testing.T) {
+	stubIdentity(t, map[string][2]uint64{
+		"/Users/x/Repo": {7, 42},
+		"/users/x/repo": {7, 42},
+	})
+	a := keyPath("/Users/x/Repo")
+	b := keyPath("/users/x/repo")
+	if a != b {
+		t.Fatalf("two spellings of one directory must key together: %q vs %q", a, b)
+	}
+	if a != "/Users/x/Repo" {
+		t.Fatalf("the key should stay a readable path (the first spelling seen): %q", a)
+	}
+}
+
+func TestKeyPathCaseDifferingDistinctDirectoriesStaySeparate(t *testing.T) {
+	stubIdentity(t, map[string][2]uint64{
+		"/x/Repo": {7, 42},
+		"/x/repo": {7, 43},
+	})
+	if keyPath("/x/Repo") == keyPath("/x/repo") {
+		t.Fatal("genuinely different directories that differ only by case must not share a key")
+	}
+}
+
+func TestKeyPathFallsBackToThePathWhenIdentityIsUnavailable(t *testing.T) {
+	stubIdentity(t, map[string][2]uint64{})
+	if got, want := keyPath("/gone/Repo"), "/gone/Repo"; got != want {
+		t.Fatalf("an unstattable path keys by itself: %q vs %q", got, want)
+	}
+}
+
+func TestKeyPathRecycledIdentityDoesNotResurrectADeadSpelling(t *testing.T) {
+	// An inode is recycled when its directory is deleted. The side table
+	// must notice the remembered spelling no longer stats to the identity
+	// and key the live directory by its own spelling.
+	ids := map[string][2]uint64{"/old/Repo": {7, 42}, "/new/repo": {7, 42}}
+	orig := dirIdentity
+	dirIdentity = func(path string) (uint64, uint64, bool) {
+		id, ok := ids[path]
+		if !ok {
+			return 0, 0, false
+		}
+		return id[0], id[1], true
+	}
+	identitySpellings.Lock()
+	saved := identitySpellings.byID
+	identitySpellings.byID = map[[2]uint64]string{}
+	identitySpellings.Unlock()
+	t.Cleanup(func() {
+		dirIdentity = orig
+		identitySpellings.Lock()
+		identitySpellings.byID = saved
+		identitySpellings.Unlock()
+	})
+	if got := keyPath("/old/Repo"); got != "/old/Repo" {
+		t.Fatalf("first spelling keys by itself: %q", got)
+	}
+	delete(ids, "/old/Repo") // the directory is gone; 7:42 now names /new/repo
+	if got := keyPath("/new/repo"); got != "/new/repo" {
+		t.Fatalf("a recycled identity must not resurrect the dead spelling: %q", got)
+	}
+}

pkgs/mcpx/internal/config/scope_test.go

@@ -272,3 +272,38 @@ func TestOnlyKeysTheCallerMintedAreCallerOwned(t *testing.T) {
 		t.Error("a per-call key is always the caller's")
 	}
 }
+
+func TestCwdScopeCaseDifferingDirectoriesStaySeparate(t *testing.T) {
+	// On a case-sensitive volume these are two directories and must never
+	// share a key (#1912: identity keying makes the unsafe direction
+	// impossible). On a case-insensitive volume the second is the first,
+	// and the two spellings must key together instead.
+	base := t.TempDir()
+	upper := filepath.Join(base, "CaseDir")
+	lower := filepath.Join(base, "casedir")
+	if err := os.Mkdir(upper, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Mkdir(lower, 0o755); err != nil {
+		// The name is taken because the volume already resolved it to
+		// CaseDir -- that is the case-insensitive outcome, tested below.
+		t.Logf("second mkdir: %v", err)
+	}
+	fiU, err := os.Stat(upper)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fiL, err := os.Stat(lower)
+	if err != nil {
+		t.Skip("lower-case spelling does not resolve")
+	}
+	a, _ := config.ScopeCwd.Key(config.CallContext{Cwd: upper, CallID: "c1"})
+	b, _ := config.ScopeCwd.Key(config.CallContext{Cwd: lower, CallID: "c2"})
+	if os.SameFile(fiU, fiL) {
+		if a != b {
+			t.Fatalf("two spellings of one directory must key together: %q vs %q", a, b)
+		}
+	} else if a == b {
+		t.Fatal("case-differing distinct directories must not share a cwd key")
+	}
+}