fix(mcp): disambiguate duplicate-name repo resolution for worktrees (#1753)

* fix(mcp): disambiguate duplicate-name repo resolution for worktrees

When multiple indexed repos share the same registry name (main checkout plus linked worktrees), MCP tools no longer silently pick the first sibling. Resolution prefers the repo matching process.cwd()'s git root, throws RegistryAmbiguousTargetError when still ambiguous, and uses canonical path matching aligned with the CLI registry.

Fixes #1658. Complements worktree detect_changes fixes in #1654/#1691.

* fix(mcp): refresh registry on duplicate-name ambiguity before failing

resolveRepo now retries resolveRepoFromCache after RegistryAmbiguousTargetError so stale in-memory siblings clear when the registry changes. Adds detect_changes callTool ambiguity test, registry-refresh regression test, pickRepoHandleForCwd MCP cwd doc, and temp-dir cleanup in #1658 fixtures.

* chore(autofix): apply prettier + eslint fixes via /autofix command

* fix(mcp): PR #1753 review follow-ups + collision-id case bug

Address Findings 3-6 from the production-readiness review on PR #1753,
plus a latent bug surfaced while writing the F5 regression test:

- F3: drop the no-op `try { ... } catch (err) { throw err; }` wrapper
  around the miss-path retry in `resolveRepo`; the catch only re-threw.
- F4: rewrite the misleading "child/repo" example on the relative-path
  tier — `child/repo` would be classified as path-like and never reach
  this branch. Comment now describes bare, separator-free names
  resolved against `process.cwd()`.
- F5: add regression test for the stable hashed-id tier so a duplicate
  sibling can be reached by its `<name>-<hash>` id. Writing this test
  exposed that `repoId()` produced a mixed-case base64url suffix while
  `resolveRepoFromCache` lowercased the param before the Map lookup, so
  collision ids with any uppercase byte in the hash were unreachable.
  Fix: lowercase the hash in `repoId` so it survives `paramLower`.
- F6: add regression test asserting two repos sharing a name prefix
  (`project-a`, `project-b`) cause `resolveRepo("project")` to reject
  as not-found rather than silently returning the first partial match.

* refactor(mcp): tighten PR #1753 follow-up tests + pin hash length

Address three P2 maintainability findings from the ce-code-review pass
on commit aa7f2050:

- Export `REPO_ID_HASH_LENGTH` from local-backend.ts and use it in both
  `repoId()` and the hashed-id test. Closes the silent-drift hole where
  the test's inline formula could fall out of sync with the source
  without any signal.
- Extract `makeSharedPrefixFixture(nameA, nameB)` next to
  `makeDuplicateNameFixture`. Centralises the temp-dir + `.gitnexus`
  scaffolding + `duplicateFixtureDirs.push()` cleanup contract so
  future callers can't drop the cleanup step.
- Reorder the hashed-id test's comment block so the intentional-coupling
  rationale leads, before the description of the formula being mirrored.

* chore(autofix): apply prettier + eslint fixes via /autofix command

* chore: re-run CI

---------

Co-authored-by: Test <test@example.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
Gergő Magyar
2026-05-21 19:21:25 +01:00
committed by GitHub
co-authored by Test github-actions[bot]
parent df2ed009ce
commit 231ad71d40
2 changed files with 286 additions and 23 deletions
+118 -17
View File
@@ -31,6 +31,8 @@ import { realpathSync } from 'fs';
import {
listRegisteredRepos,
cleanupOldKuzuFiles,
canonicalizePath,
RegistryAmbiguousTargetError,
type RegistryEntry,
} from '../../storage/repo-manager.js';
import { GroupService, type GroupToolPort } from '../../core/group/service.js';
@@ -295,6 +297,13 @@ export function resolveWorktreeCwd(repoPath: string, launchCwd: string): string
return repoPath;
}
/**
* Length of the base64url path hash appended to a colliding repo id.
* Exported so tests can pin the suffix shape without re-deriving the
* literal; see `repoId()` and the hashed-id resolution tier (#1658).
*/
export const REPO_ID_HASH_LENGTH = 6;
export class LocalBackend {
private repos: Map<string, RepoHandle> = new Map();
private contextCache: Map<string, CodebaseContext> = new Map();
@@ -423,7 +432,13 @@ export class LocalBackend {
for (const [id, handle] of this.repos) {
if (id === base && handle.repoPath !== path.resolve(repoPath)) {
// Collision — use path hash
const hash = Buffer.from(repoPath).toString('base64url').slice(0, 6);
// Lowercase the hash so it survives the `paramLower` lookup in
// resolveRepoFromCache — base64url retains mixed case, but the id
// tier compares against `repoParam.toLowerCase()` (#1658 follow-up).
const hash = Buffer.from(repoPath)
.toString('base64url')
.slice(0, REPO_ID_HASH_LENGTH)
.toLowerCase();
return `${base}-${hash}`;
}
}
@@ -442,7 +457,19 @@ export class LocalBackend {
* while the MCP server was running.
*/
async resolveRepo(repoParam?: string): Promise<RepoHandle> {
const result = this.resolveRepoFromCache(repoParam);
let refreshedAfterAmbiguity = false;
let result: RepoHandle | null;
try {
result = this.resolveRepoFromCache(repoParam);
} catch (err) {
if (!(err instanceof RegistryAmbiguousTargetError)) throw err;
// Stale in-memory duplicate siblings can linger after unregister; refresh
// once before re-throwing so a resolved registry can disambiguate (#1658).
await this.refreshRepos();
refreshedAfterAmbiguity = true;
result = this.resolveRepoFromCache(repoParam);
}
if (result) {
// Issue: silent graph drift across sibling clones.
// If the caller's cwd lives in a *different* on-disk clone of
@@ -456,8 +483,10 @@ export class LocalBackend {
return result;
}
// Miss — refresh registry and try once more
await this.refreshRepos();
// Miss — refresh registry and try once more (skip if already refreshed above)
if (!refreshedAfterAmbiguity) {
await this.refreshRepos();
}
const retried = this.resolveRepoFromCache(repoParam);
if (retried) {
this.maybeWarnSiblingDrift(retried).catch(() => {});
@@ -492,27 +521,66 @@ export class LocalBackend {
/**
* Try to resolve a repo from the in-memory cache. Returns null on miss.
* Throws {@link RegistryAmbiguousTargetError} when `repoParam` matches
* multiple handles by name and cwd cannot disambiguate (#1658).
*/
private resolveRepoFromCache(repoParam?: string): RepoHandle | null {
if (this.repos.size === 0) return null;
if (repoParam) {
const paramLower = repoParam.toLowerCase();
// Match by id
const looksLikePath =
path.isAbsolute(repoParam) || repoParam.includes(path.sep) || repoParam.includes('/');
const resolvePathMatch = (): RepoHandle | undefined => {
const canonicalTarget = canonicalizePath(repoParam);
return [...this.repos.values()].find((handle) => {
const stored = canonicalizePath(handle.repoPath);
return process.platform === 'win32'
? stored.toLowerCase() === canonicalTarget.toLowerCase()
: stored === canonicalTarget;
});
};
// Path-like params first (absolute or contains separators) — aligns with
// resolveRegistryEntry (#829). Bare aliases such as ".tmp-repro-mini" must
// not be resolved via path.resolve(cwd) before duplicate-name handling.
if (looksLikePath) {
const pathMatch = resolvePathMatch();
if (pathMatch) return pathMatch;
}
// Exact name before id — the first duplicate sibling keeps id === name
// (e.g. id "shared"), so a name lookup must not be captured by the id tier.
const nameMatches = [...this.repos.values()].filter(
(handle) => handle.name.toLowerCase() === paramLower,
);
if (nameMatches.length === 1) return nameMatches[0];
if (nameMatches.length > 1) {
const cwdPick = this.pickRepoHandleForCwd(nameMatches);
if (cwdPick) return cwdPick;
throw new RegistryAmbiguousTargetError(
repoParam,
nameMatches.map((h) => this.handleToRegistryEntry(h)),
);
}
// Stable hashed id (e.g. "shared-abc123") from repoId() collision suffix
if (this.repos.has(paramLower)) return this.repos.get(paramLower)!;
// Match by name (case-insensitive)
for (const handle of this.repos.values()) {
if (handle.name.toLowerCase() === paramLower) return handle;
}
// Match by path (substring)
const resolved = path.resolve(repoParam);
for (const handle of this.repos.values()) {
if (handle.repoPath === resolved) return handle;
}
// Match by partial name
for (const handle of this.repos.values()) {
if (handle.name.toLowerCase().includes(paramLower)) return handle;
// Bare name resolved as a cwd-relative path (e.g. "myrepo" against process.cwd()),
// after name/id tiers. Path-like strings with separators were handled at the top.
if (!looksLikePath) {
const pathMatch = resolvePathMatch();
if (pathMatch) return pathMatch;
}
// Partial name — only when unambiguous
const partialMatches = [...this.repos.values()].filter((handle) =>
handle.name.toLowerCase().includes(paramLower),
);
if (partialMatches.length === 1) return partialMatches[0];
return null;
}
@@ -523,6 +591,39 @@ export class LocalBackend {
return null; // Multiple repos, no param — ambiguous
}
/**
* Prefer the indexed repo whose path matches the git root of process.cwd().
*
* In MCP stdio server mode, `process.cwd()` is the server's launch directory,
* not the agent client's cwd. If the server was started from an unrelated
* directory, `getGitRoot` returns null and duplicate-name resolution throws
* {@link RegistryAmbiguousTargetError} — callers should pass an absolute path.
*/
private pickRepoHandleForCwd(candidates: RepoHandle[]): RepoHandle | null {
const cwdRoot = getGitRoot(process.cwd());
if (!cwdRoot) return null;
const canonicalCwd = canonicalizePath(cwdRoot);
const cwdMatches = candidates.filter((handle) => {
const stored = canonicalizePath(handle.repoPath);
return process.platform === 'win32'
? stored.toLowerCase() === canonicalCwd.toLowerCase()
: stored === canonicalCwd;
});
return cwdMatches.length === 1 ? cwdMatches[0] : null;
}
private handleToRegistryEntry(handle: RepoHandle): RegistryEntry {
return {
name: handle.name,
path: handle.repoPath,
storagePath: handle.storagePath,
indexedAt: handle.indexedAt,
lastCommit: handle.lastCommit,
stats: handle.stats,
remoteUrl: handle.remoteUrl,
};
}
// ─── Lazy LadybugDB Init ────────────────────────────────────────────
private async ensureInitialized(repoId: string): Promise<void> {
+168 -6
View File
@@ -8,6 +8,9 @@
* the dispatch and error handling logic in isolation.
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
import os from 'os';
import path from 'path';
// We need to mock the LadybugDB adapter and repo-manager BEFORE importing LocalBackend.
// local-backend.ts imports from core/lbug/pool-adapter.js; the mcp/core/lbug-adapter.js
@@ -37,11 +40,15 @@ vi.mock('../../src/mcp/core/lbug-adapter.js', async (importOriginal) => {
return { ...actual, ...lbugMocks };
});
vi.mock('../../src/storage/repo-manager.js', () => ({
listRegisteredRepos: vi.fn().mockResolvedValue([]),
cleanupOldKuzuFiles: vi.fn().mockResolvedValue({ found: false, needsReindex: false }),
findSiblingClones: vi.fn().mockResolvedValue([]),
}));
vi.mock('../../src/storage/repo-manager.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/storage/repo-manager.js')>();
return {
...actual,
listRegisteredRepos: vi.fn().mockResolvedValue([]),
cleanupOldKuzuFiles: vi.fn().mockResolvedValue({ found: false, needsReindex: false }),
findSiblingClones: vi.fn().mockResolvedValue([]),
};
});
// `core/git-staleness` is also imported by `local-backend.ts` (for
// `checkStaleness` and `checkCwdMatch`). Stub it out here so unit
@@ -52,6 +59,14 @@ vi.mock('../../src/core/git-staleness.js', () => ({
checkCwdMatch: vi.fn().mockResolvedValue({ match: 'none' }),
}));
vi.mock('../../src/storage/git.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/storage/git.js')>();
return {
...actual,
getGitRoot: vi.fn().mockReturnValue(null),
};
});
vi.mock('../../src/core/platform/capabilities.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/core/platform/capabilities.js')>();
return {
@@ -70,8 +85,9 @@ vi.mock('../../src/mcp/core/embedder.js', () => ({
getEmbeddingDims: vi.fn().mockReturnValue(384),
}));
import { LocalBackend } from '../../src/mcp/local/local-backend.js';
import { LocalBackend, REPO_ID_HASH_LENGTH } from '../../src/mcp/local/local-backend.js';
import { listRegisteredRepos, cleanupOldKuzuFiles } from '../../src/storage/repo-manager.js';
import { getGitRoot } from '../../src/storage/git.js';
import { _captureLogger } from '../../src/core/logger.js';
import {
initLbug,
@@ -112,6 +128,56 @@ function setupNoRepos() {
(listRegisteredRepos as any).mockResolvedValue([]);
}
const duplicateFixtureDirs: string[] = [];
function makeDuplicateNameFixture() {
const mainDir = mkdtempSync(path.join(os.tmpdir(), 'gnx-shared-main-'));
const wtDir = mkdtempSync(path.join(os.tmpdir(), 'gnx-shared-wt-'));
duplicateFixtureDirs.push(mainDir, wtDir);
for (const dir of [mainDir, wtDir]) {
const storagePath = path.join(dir, '.gitnexus');
mkdirSync(path.join(storagePath, 'lbug'), { recursive: true });
writeFileSync(path.join(storagePath, 'meta.json'), '{}');
}
return {
mainDir,
wtDir,
entries: [
{
...MOCK_REPO_ENTRY,
name: 'shared',
path: mainDir,
storagePath: path.join(mainDir, '.gitnexus'),
},
{
...MOCK_REPO_ENTRY,
name: 'shared',
path: wtDir,
storagePath: path.join(wtDir, '.gitnexus'),
},
],
};
}
function makeSharedPrefixFixture(nameA: string, nameB: string) {
const dirA = mkdtempSync(path.join(os.tmpdir(), `gnx-${nameA}-`));
const dirB = mkdtempSync(path.join(os.tmpdir(), `gnx-${nameB}-`));
duplicateFixtureDirs.push(dirA, dirB);
for (const dir of [dirA, dirB]) {
const storagePath = path.join(dir, '.gitnexus');
mkdirSync(path.join(storagePath, 'lbug'), { recursive: true });
writeFileSync(path.join(storagePath, 'meta.json'), '{}');
}
return {
dirA,
dirB,
entries: [
{ ...MOCK_REPO_ENTRY, name: nameA, path: dirA, storagePath: path.join(dirA, '.gitnexus') },
{ ...MOCK_REPO_ENTRY, name: nameB, path: dirB, storagePath: path.join(dirB, '.gitnexus') },
],
};
}
// ─── LocalBackend lifecycle ──────────────────────────────────────────
describe('LocalBackend.init', () => {
@@ -783,9 +849,16 @@ describe('LocalBackend.resolveRepo', () => {
beforeEach(async () => {
vi.clearAllMocks();
(getGitRoot as any).mockReturnValue(null);
backend = new LocalBackend();
});
afterEach(() => {
for (const dir of duplicateFixtureDirs.splice(0)) {
rmSync(dir, { recursive: true, force: true });
}
});
it('resolves single repo without param', async () => {
setupSingleRepo();
await backend.init();
@@ -829,6 +902,95 @@ describe('LocalBackend.resolveRepo', () => {
);
});
it('prefers duplicate-name repo matching process.cwd() git root (#1658)', async () => {
const { wtDir, entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(wtDir);
await backend.init();
(executeParameterized as any).mockResolvedValue([]);
await backend.callTool('query', { query: 'test', repo: 'shared' });
const resolved = await backend.resolveRepo('shared');
expect(resolved.repoPath).toBe(wtDir);
});
it('throws RegistryAmbiguousTargetError when duplicate name cannot be disambiguated (#1658)', async () => {
const { entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(null);
await backend.init();
await expect(backend.resolveRepo('shared')).rejects.toThrow(/Multiple registered repos match/);
await expect(backend.resolveRepo('shared')).rejects.toThrow(/absolute path/i);
});
it('resolves duplicate-name repos by absolute path before name (#1658)', async () => {
const { mainDir, wtDir, entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(mainDir);
await backend.init();
(executeParameterized as any).mockResolvedValue([]);
const resolved = await backend.resolveRepo(wtDir);
expect(resolved.repoPath).toBe(wtDir);
});
it('does not treat a bare duplicate alias as a relative path (#1658)', async () => {
const { entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(null);
await backend.init();
await expect(backend.resolveRepo('shared')).rejects.toThrow(/Multiple registered repos match/);
});
it('refreshes registry after ambiguity when duplicates are removed (#1658)', async () => {
const { mainDir, entries } = makeDuplicateNameFixture();
const singleEntry = [entries[0]];
(listRegisteredRepos as any).mockResolvedValueOnce(entries).mockResolvedValueOnce(singleEntry);
(getGitRoot as any).mockReturnValue(null);
await backend.init();
const resolved = await backend.resolveRepo('shared');
expect(resolved.repoPath).toBe(mainDir);
});
it('detect_changes surfaces RegistryAmbiguousTargetError on duplicate repo name (#1658)', async () => {
const { entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(null);
await backend.init();
await expect(
backend.callTool('detect_changes', { scope: 'unstaged', repo: 'shared' }),
).rejects.toThrow(/Multiple registered repos match/);
});
it('resolves second duplicate-name repo by its stable hashed id (#1658)', async () => {
const { wtDir, entries } = makeDuplicateNameFixture();
(listRegisteredRepos as any).mockResolvedValue(entries);
// Couples this test to repoId's suffix formula on purpose — if repoId changes
// its suffix, this assertion should fail and force a re-review of the hashed-id
// resolution tier. Mirrors LocalBackend.repoId: base64url(repoPath) sliced to
// REPO_ID_HASH_LENGTH and lowercased so it survives the paramLower lookup in
// resolveRepoFromCache.
const wtId = `shared-${Buffer.from(wtDir)
.toString('base64url')
.slice(0, REPO_ID_HASH_LENGTH)
.toLowerCase()}`;
await backend.init();
const resolved = await backend.resolveRepo(wtId);
expect(resolved.repoPath).toBe(wtDir);
});
it('does not silently return first partial match for ambiguous prefix (#1658)', async () => {
const { dirA, entries } = makeSharedPrefixFixture('project-a', 'project-b');
(listRegisteredRepos as any).mockResolvedValue(entries);
(getGitRoot as any).mockReturnValue(null);
await backend.init();
await expect(backend.resolveRepo('project')).rejects.toThrow(/Repository "project" not found/);
// Sanity: exact names still resolve unambiguously against the same fixture.
const exact = await backend.resolveRepo('project-a');
expect(exact.name).toBe('project-a');
expect(exact.repoPath).toBe(dirA);
});
it('resolves repo case-insensitively', async () => {
setupSingleRepo();
await backend.init();