From 7caf972584a0f8ec6e275596efe6548df7774105 Mon Sep 17 00:00:00 2001 From: Drewry Pope Date: Thu, 29 Oct 2026 23:52:38 -0500 Subject: [PATCH] mcpx: key scopes by directory identity so a wrong-case cwd shares one instance (#1912) canonical() keeps the caller's spelling, so on a case-insensitive volume two spellings of one directory produced two scope keys and two server instances across the cwd, repo and worktree scopes. Scope.Key now routes through keyPath(): stat the canonical path, look its (dev, inode) up in a side table remembering the first spelling seen for that identity, and key every spelling to the first. The key stays a plain readable path. Stat failure or a platform without identity falls back to the path alone (today's behaviour, the safe over-isolation direction); distinct directories differing only by case have distinct inodes, so accidental sharing is impossible. Cached spellings are re-validated against the identity before reuse, so a recycled inode cannot resurrect a deleted directory's spelling (caught by the existing suite during development; regression test included). TestGitCaseOfAWrongCaseCwdIsKept is inverted, not deleted: discovery still keeps the spelling, but the cwd/repo/worktree keys for both spellings must now coincide. Portable internal tests stub dirIdentity (the gitDevice pattern) for both volume kinds; fail-before verified by neutering keyPath (new test fails), pass after restore. --- .../config/gitdiscover_darwin_test.go | 20 ++-- pkgs/mcpx/internal/config/gitsys_other.go | 6 ++ pkgs/mcpx/internal/config/gitsys_unix.go | 14 +++ pkgs/mcpx/internal/config/scope.go | 60 +++++++++++- .../config/scope_identity_internal_test.go | 94 +++++++++++++++++++ pkgs/mcpx/internal/config/scope_test.go | 35 +++++++ 6 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 pkgs/mcpx/internal/config/scope_identity_internal_test.go diff --git a/pkgs/mcpx/internal/config/gitdiscover_darwin_test.go b/pkgs/mcpx/internal/config/gitdiscover_darwin_test.go index 690e8b71a..3bcbfcce1 100644 --- a/pkgs/mcpx/internal/config/gitdiscover_darwin_test.go +++ b/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) + } + } } diff --git a/pkgs/mcpx/internal/config/gitsys_other.go b/pkgs/mcpx/internal/config/gitsys_other.go index f15638983..94a065efa 100644 --- a/pkgs/mcpx/internal/config/gitsys_other.go +++ 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 } diff --git a/pkgs/mcpx/internal/config/gitsys_unix.go b/pkgs/mcpx/internal/config/gitsys_unix.go index abb3d0031..9e80b2150 100644 --- a/pkgs/mcpx/internal/config/gitsys_unix.go +++ b/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 +} diff --git a/pkgs/mcpx/internal/config/scope.go b/pkgs/mcpx/internal/config/scope.go index 9cf6f5189..c74ca25ac 100644 --- a/pkgs/mcpx/internal/config/scope.go +++ b/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 +} diff --git a/pkgs/mcpx/internal/config/scope_identity_internal_test.go b/pkgs/mcpx/internal/config/scope_identity_internal_test.go new file mode 100644 index 000000000..6dcb36a34 --- /dev/null +++ b/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) + } +} diff --git a/pkgs/mcpx/internal/config/scope_test.go b/pkgs/mcpx/internal/config/scope_test.go index a808588fe..757876ff2 100644 --- a/pkgs/mcpx/internal/config/scope_test.go +++ b/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") + } +}