Repository navigation
mcpx: key scopes by directory identity so a wrong-case cwd shares one instance (#1912) - #2563
Merged
Merged
Conversation
… 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.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1912. Queue task 3190 (triage task 2807 confirmed the bug live on main). Design decision (Approach C, side-table variant) recorded on #1912 before implementation.
What changed
Scope.Key(cwd, repo, worktree scopes) now routes its path through a newkeyPath()inpkgs/mcpx/internal/config/scope.go: canonicalize as before,os.Statthe result, and look its(dev, inode)up in a process-wide side table that remembers the first canonical spelling seen for that identity. Every spelling of one directory keys to the first spelling — one key, one instance, on all three scopes.dirIdentityadded besidegitDeviceingitsys_unix.go(syscall.Stat_t, no new dependency) /gitsys_other.go(no identity available → fallback).Statfailure (deleted/unreadable cwd) or no platform identity → canonical path alone. That is the safe over-isolation direction; two genuinely different directories differing only by case have different inodes, so accidental sharing is impossible.mcpx daemonsoutput unaffected).Found during development
The first version of the side table failed an existing test: a temp directory's inode was recycled after deletion and the table resurrected the dead directory's spelling as a live directory's key.
keyPathnow re-validates a remembered spelling against the identity before reusing it and replaces stale entries;TestKeyPathRecycledIdentityDoesNotResurrectADeadSpellingpins this.Tests
TestGitCaseOfAWrongCaseCwdIsKeptinverted, not deleted: discovery still keeps the caller's spelling; the test now asserts cwd/repo/worktree keys for both spellings coincide (darwin-gated, as before; cross-checked here withGOOS=darwin go vet).dirIdentity(thegitDevicepattern) to cover both volume kinds on any OS: two spellings/one identity key together; case-differing distinct directories stay separate; unstattable path falls back to itself; recycled identity does not resurrect a dead spelling.scope_test.go: case-differing directories stay separate on case-sensitive volumes and key together where the volume resolves them to one directory.keyPathneutered to ignore identity,TestKeyPathTwoSpellingsOfOneDirectoryKeyTogetherfails ("/Users/x/Repo" vs "/users/x/repo"); restored, it passes.go test ./internal/config/ -count=1PASS; daemon degraded-scope tests PASS. Note: the fullinternal/daemonpackage test binary getssignal: terminatedon the laptop — reproduced identically on a cleanorigin/mainworktree, so it is a pre-existing laptop environment issue, not this change; CI is authoritative.Open questions from #1912 (key persistence across restarts, container bind mounts, #88 interaction) are answered in the design-decision comment on the issue.
Need help on this PR? Tag
@codesmith-botwith what you need.