diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index d1297ea17..331cffb2c 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -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 = new Map(); private contextCache: Map = 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 { - 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 { diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 8c69a23d5..9596d6b7a 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -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(); + 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(); + return { + ...actual, + getGitRoot: vi.fn().mockReturnValue(null), + }; +}); + vi.mock('../../src/core/platform/capabilities.js', async (importOriginal) => { const actual = await importOriginal(); 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();