fix(mcp): address review feedback — CI green, perf, dead branch, one-shot test

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/cc2259f7-94e4-4243-aaa9-e03b7c632d32

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<CwdMatch> {
   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;

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<string, RepoHandle[]>();
+    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<void> {
     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}`);
   }
 

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 ──────────────────────────────────────────────────────

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();