From 54abe7336b3274d8f3cd57e3037a0782dfae8fbb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 07:54:52 +0000 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20CI=20green,=20perf,=20dead=20branch,=20one-shot=20t?= =?UTF-8?q?est?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/cc2259f7-94e4-4243-aaa9-e03b7c632d32 --- gitnexus/src/core/git-staleness.ts | 24 ++--- gitnexus/src/mcp/local/local-backend.ts | 97 +++++++++++++------ gitnexus/test/unit/calltool-dispatch.test.ts | 39 ++++++++ .../test/unit/sibling-clone-drift.test.ts | 8 +- 4 files changed, 119 insertions(+), 49 deletions(-) diff --git a/gitnexus/src/core/git-staleness.ts b/gitnexus/src/core/git-staleness.ts index 919732e39..93e556ab5 100644 --- a/gitnexus/src/core/git-staleness.ts +++ b/gitnexus/src/core/git-staleness.ts @@ -5,11 +5,7 @@ import { execFileSync } from 'node:child_process'; import path from 'path'; -import { - readRegistry, - type RegistryEntry, - type CwdMatch, -} from '../storage/repo-manager.js'; +import { readRegistry, type RegistryEntry, type CwdMatch } from '../storage/repo-manager.js'; import { getGitRoot, getCurrentCommit, getRemoteUrl } from '../storage/git.js'; export interface StalenessInfo { @@ -54,15 +50,11 @@ export function checkStaleness(repoPath: string, lastCommit: string): StalenessI function commitsAheadOfIndexed(siblingPath: string, indexedCommit: string): number | undefined { if (!indexedCommit) return undefined; try { - const result = execFileSync( - 'git', - ['rev-list', '--count', `${indexedCommit}..HEAD`], - { - cwd: siblingPath, - encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], - }, - ).trim(); + const result = execFileSync('git', ['rev-list', '--count', `${indexedCommit}..HEAD`], { + cwd: siblingPath, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).trim(); return parseInt(result, 10) || 0; } catch { return undefined; @@ -117,7 +109,9 @@ export async function checkCwdMatch(cwd: string): Promise { const cwdRemote = getRemoteUrl(cwdGitRoot); if (!cwdRemote) return { match: 'none' }; - const sibling = entries.find((e) => e.remoteUrl === cwdRemote && norm(e.path) !== norm(cwdGitRoot)); + const sibling = entries.find( + (e) => e.remoteUrl === cwdRemote && norm(e.path) !== norm(cwdGitRoot), + ); if (!sibling) return { match: 'none' }; const cwdHead = getCurrentCommit(cwdGitRoot) || undefined; diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index e1710a0aa..f8a049eb3 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -25,7 +25,6 @@ import { parseDiffHunks, type FileDiff } from '../../storage/git.js'; import { listRegisteredRepos, cleanupOldKuzuFiles, - findSiblingClones, type RegistryEntry, } from '../../storage/repo-manager.js'; import { GroupService, type GroupToolPort } from '../../core/group/service.js'; @@ -522,31 +521,48 @@ export class LocalBackend { > { await this.refreshRepos(); const handles = [...this.repos.values()]; - return Promise.all( - handles.map(async (h) => { - const stale = checkStaleness(h.repoPath, h.lastCommit); - const siblings = await findSiblingClones(h.remoteUrl, h.repoPath); - return { - name: h.name, - path: h.repoPath, - indexedAt: h.indexedAt, - lastCommit: h.lastCommit, - remoteUrl: h.remoteUrl, - stats: h.stats, - staleness: stale.isStale - ? { commitsBehind: stale.commitsBehind, hint: stale.hint } + + // Pre-group registered handles by `remoteUrl` so the sibling + // lookup is O(1) per handle. We reuse the in-memory `this.repos` + // (already populated by `refreshRepos`) instead of doing a fresh + // `readRegistry()` per entry — that would be N file reads for N + // registered repos. + const isWin = process.platform === 'win32'; + const norm = (p: string) => (isWin ? path.resolve(p).toLowerCase() : path.resolve(p)); + const byRemote = new Map(); + for (const h of handles) { + if (!h.remoteUrl) continue; + const list = byRemote.get(h.remoteUrl) ?? []; + list.push(h); + byRemote.set(h.remoteUrl, list); + } + + return handles.map((h) => { + const stale = checkStaleness(h.repoPath, h.lastCommit); + const selfNorm = norm(h.repoPath); + const siblings = h.remoteUrl + ? (byRemote.get(h.remoteUrl) ?? []).filter((e) => norm(e.repoPath) !== selfNorm) + : []; + return { + name: h.name, + path: h.repoPath, + indexedAt: h.indexedAt, + lastCommit: h.lastCommit, + remoteUrl: h.remoteUrl, + stats: h.stats, + staleness: stale.isStale + ? { commitsBehind: stale.commitsBehind, hint: stale.hint } + : undefined, + siblings: + siblings.length > 0 + ? siblings.map((s) => ({ + name: s.name, + path: s.repoPath, + lastCommit: s.lastCommit, + })) : undefined, - siblings: - siblings.length > 0 - ? siblings.map((s) => ({ - name: s.name, - path: s.path, - lastCommit: s.lastCommit, - })) - : undefined, - }; - }), - ); + }; + }); } /** @@ -558,7 +574,18 @@ export class LocalBackend { * graph may be stale relative to what's actually on disk under their * cwd. Silent on path matches and on repos without a remote URL. * + * Limitation: in MCP stdio server mode `process.cwd()` is the + * server's CWD at start time, *not* the agent client's CWD. The + * warning therefore only fires when the MCP server itself was + * launched from inside a sibling clone (typical for `npx gitnexus + * serve` from a polecat workspace). Surfacing the client's CWD + * would require a per-tool-call `cwd` parameter — out of scope for + * the current MCP contract. + * * Pure side-effect (stderr); never affects the returned handle. + * After the first computation for a given (repo, cwd) pair the + * result is cached so subsequent `resolveRepo()` calls don't + * re-shell-out to git. */ private async maybeWarnSiblingDrift(handle: RepoHandle): Promise { if (!handle.remoteUrl) return; @@ -568,6 +595,14 @@ export class LocalBackend { } catch { return; } + // Early-exit cache: keyed on (repo, cwd) BEFORE any git shellout. + // After the first call for a given cwd, this short-circuits the + // up-to-four `execSync`/`execFileSync` calls inside `checkCwdMatch` + // — important for MCP-server mode where `process.cwd()` is constant + // and `resolveRepo` runs on every tool call. + const cacheKey = `${handle.id}|${cwd}`; + if (this.warnedSiblingDrift.has(cacheKey)) return; + const match = await checkCwdMatch(cwd); if ( match.match !== 'sibling-by-remote' || @@ -576,16 +611,14 @@ export class LocalBackend { match.entry.path !== handle.repoPath || !match.hint ) { + // Cache "nothing to warn about" outcomes too — `checkCwdMatch` + // is deterministic for a fixed (registry, cwd) pair, so re-running + // it yields nothing new. + this.warnedSiblingDrift.add(cacheKey); return; } - // Only warn when the sibling has actually drifted (or drift is - // unknown). If both clones are on the indexed commit, skip the - // noise — the caller is fine. - if (match.cwdHead && match.cwdHead === handle.lastCommit) return; - const key = `${handle.id}|${match.cwdGitRoot}`; - if (this.warnedSiblingDrift.has(key)) return; - this.warnedSiblingDrift.add(key); + this.warnedSiblingDrift.add(cacheKey); console.error(`GitNexus: ${match.hint}`); } diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index fa8df474a..d57bd9051 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -757,6 +757,45 @@ describe('LocalBackend.resolveRepo', () => { // listRegisteredRepos should have been called again expect(listRegisteredRepos).toHaveBeenCalledTimes(2); // once in init, once in refreshRepos }); + + it('emits sibling-clone drift warning exactly once per (repo, cwd) pair', async () => { + // Regression guard for the one-shot stderr warning emitted when + // the caller's cwd is in a sibling clone of the resolved index. + // The cache must short-circuit BOTH `console.error` and the + // underlying `checkCwdMatch` git shellouts on subsequent calls. + const { checkCwdMatch } = await import('../../src/core/git-staleness.js'); + (listRegisteredRepos as any).mockResolvedValue([ + { ...MOCK_REPO_ENTRY, remoteUrl: 'https://example.com/foo/bar' }, + ]); + (checkCwdMatch as any).mockResolvedValue({ + match: 'sibling-by-remote', + entry: { ...MOCK_REPO_ENTRY, remoteUrl: 'https://example.com/foo/bar' }, + cwdGitRoot: '/tmp/sibling-clone', + cwdHead: 'feedface', + hint: '⚠️ stale sibling clone', + }); + + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + await backend.init(); + + // Three resolveRepo invocations from the same cwd: + await backend.callTool('list_repos', {}); // resolveRepo not called for list_repos + // Use a real resolveRepo path: + await backend.resolveRepo(); + await backend.resolveRepo(); + await backend.resolveRepo(); + + const drift = errSpy.mock.calls.filter((c) => String(c[0]).includes('stale sibling clone')); + expect(drift).toHaveLength(1); + // checkCwdMatch should also only run once — the cache check + // happens BEFORE the shellout-heavy match call. + expect(checkCwdMatch).toHaveBeenCalledTimes(1); + } finally { + errSpy.mockRestore(); + (checkCwdMatch as any).mockResolvedValue({ match: 'none' }); + } + }); }); // ─── getContext ────────────────────────────────────────────────────── diff --git a/gitnexus/test/unit/sibling-clone-drift.test.ts b/gitnexus/test/unit/sibling-clone-drift.test.ts index b858b3440..83fe3294e 100644 --- a/gitnexus/test/unit/sibling-clone-drift.test.ts +++ b/gitnexus/test/unit/sibling-clone-drift.test.ts @@ -16,7 +16,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import path from 'path'; -import fs from 'fs/promises'; +import fs from 'fs'; import { execSync } from 'child_process'; import { registerRepo, @@ -199,8 +199,12 @@ describe('checkCwdMatch', () => { const m = await checkCwdMatch(sibling.dbPath); expect(m.match).toBe('sibling-by-remote'); + // `git rev-parse --show-toplevel` returns the realpath of the + // worktree, which on macOS resolves the `/var → /private/var` + // symlink and on Windows expands short 8.3 names. Compare + // against `fs.realpathSync` so the assertion is portable. expect(m.entry?.path).toBe(path.resolve(indexed.dbPath)); - expect(m.cwdGitRoot).toBe(path.resolve(sibling.dbPath)); + expect(m.cwdGitRoot).toBe(fs.realpathSync(sibling.dbPath)); expect(m.hint).toBeTruthy(); } finally { await indexed.cleanup();