From 30982c6d385572d89a4a4ad006bc072875f77a19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Sun, 3 May 2026 09:17:12 +0100 Subject: [PATCH] fix(mcp): avoid git from non-repo cwd in sibling cwd match (#1138) (#1293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): avoid git shellout from non-repo cwd for sibling match checkCwdMatch used getGitRoot(cwd), which runs git rev-parse from the launch cwd (often \C:\Users\gergo in MCP stdio). Resolve the cwd git root via ancestor .git checks first, then keep existing remote-based sibling logic. Fixes #1138 Co-authored-by: Cursor * test(mcp): address PR #1293 review follow-ups Three test gaps flagged by review on the #1138 fix: - sibling-clone-drift.test.ts: the existing "non-git cwd" test only asserted match=none, which the pre-fix code also returned (by silently failing the spawn). Wrap child_process / node:child_process with passthrough vi.fn() spies and assert no execSync/execFileSync call is recorded when checkCwdMatch runs against a non-git cwd, so a regression that re-introduces the spawn fails loudly. - git.test.ts: add coverage for findGitRootByDotGit's three untested inputs — a `.git` FILE (linked worktree / submodule), a path that does not exist, and a file path inside a repo (must walk from the parent dir). Each asserts no subprocess was spawned. No production code changes. Test additions only. --------- Co-authored-by: Cursor --- gitnexus/src/core/git-staleness.ts | 9 ++- gitnexus/src/storage/git.ts | 28 +++++++ gitnexus/test/unit/git.test.ts | 74 ++++++++++++++++++- .../test/unit/sibling-clone-drift.test.ts | 57 +++++++++++++- 4 files changed, 161 insertions(+), 7 deletions(-) diff --git a/gitnexus/src/core/git-staleness.ts b/gitnexus/src/core/git-staleness.ts index 93e556ab5..96f70ddd6 100644 --- a/gitnexus/src/core/git-staleness.ts +++ b/gitnexus/src/core/git-staleness.ts @@ -6,7 +6,7 @@ import { execFileSync } from 'node:child_process'; import path from 'path'; import { readRegistry, type RegistryEntry, type CwdMatch } from '../storage/repo-manager.js'; -import { getGitRoot, getCurrentCommit, getRemoteUrl } from '../storage/git.js'; +import { findGitRootByDotGit, getCurrentCommit, getRemoteUrl } from '../storage/git.js'; export interface StalenessInfo { isStale: boolean; @@ -101,9 +101,10 @@ export async function checkCwdMatch(cwd: string): Promise { } if (bestPath) return { match: 'path', entry: bestPath }; - // 2) Sibling-by-remote: locate the cwd's git root, get its remote - // URL, and look for any registered entry with the same fingerprint. - const cwdGitRoot = getGitRoot(cwdResolved); + // 2) Sibling-by-remote: locate the cwd's git root using only ancestor + // `.git` checks before shelling out. This keeps MCP startup from + // running git in an unrelated launch cwd such as $HOME (#1138). + const cwdGitRoot = findGitRootByDotGit(cwdResolved); if (!cwdGitRoot) return { match: 'none' }; const cwdRemote = getRemoteUrl(cwdGitRoot); diff --git a/gitnexus/src/storage/git.ts b/gitnexus/src/storage/git.ts index 8e0d6e555..9105a4da3 100644 --- a/gitnexus/src/storage/git.ts +++ b/gitnexus/src/storage/git.ts @@ -93,6 +93,34 @@ export const getGitRoot = (fromPath: string): string | null => { return null; } }; + +/** + * Find a git root by checking only `.git` entries on the ancestor chain. + * + * Unlike `getGitRoot`, this does not spawn `git`, so MCP can cheaply decide + * whether a launch cwd is a worktree before running any subprocess there. + */ +export const findGitRootByDotGit = (fromPath: string): string | null => { + let current = path.resolve(fromPath); + try { + if (!statSync(current).isDirectory()) { + current = path.dirname(current); + } + } catch { + return null; + } + + while (true) { + try { + statSync(path.join(current, '.git')); + return current; + } catch { + const parent = path.dirname(current); + if (parent === current) return null; + current = parent; + } + } +}; /** * Check whether a directory contains a .git entry (file or folder). * diff --git a/gitnexus/test/unit/git.test.ts b/gitnexus/test/unit/git.test.ts index 1e13facda..af339ce69 100644 --- a/gitnexus/test/unit/git.test.ts +++ b/gitnexus/test/unit/git.test.ts @@ -1,6 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { execSync } from 'child_process'; -import { isGitRepo, getCurrentCommit, getGitRoot } from '../../src/storage/git.js'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { + isGitRepo, + getCurrentCommit, + getGitRoot, + findGitRootByDotGit, +} from '../../src/storage/git.js'; // Mock child_process.execSync vi.mock('child_process', () => ({ @@ -92,4 +100,68 @@ describe('git utilities', () => { expect(result!.trim()).toBe(result); }); }); + + describe('findGitRootByDotGit', () => { + it('finds an ancestor .git directory without spawning git', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-dotgit-')); + try { + fs.mkdirSync(path.join(tmpDir, '.git')); + const nested = path.join(tmpDir, 'packages', 'app'); + fs.mkdirSync(nested, { recursive: true }); + + expect(findGitRootByDotGit(nested)).toBe(path.resolve(tmpDir)); + expect(mockExecSync).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('returns null outside a git worktree without spawning git', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-nonrepo-')); + try { + expect(findGitRootByDotGit(tmpDir)).toBeNull(); + expect(mockExecSync).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + // Linked worktrees and submodules use a `.git` file (not directory) that + // points at the real gitdir. statSync succeeds for both, so the ancestor + // walk should treat such roots identically to ordinary repos. + it('treats a .git file (linked worktree) as a valid root', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-worktree-')); + try { + fs.writeFileSync(path.join(tmpDir, '.git'), 'gitdir: /fake/worktrees/wt\n'); + const nested = path.join(tmpDir, 'src', 'pkg'); + fs.mkdirSync(nested, { recursive: true }); + + expect(findGitRootByDotGit(nested)).toBe(path.resolve(tmpDir)); + expect(mockExecSync).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('returns null when the input path does not exist', () => { + const missing = path.join(os.tmpdir(), `gitnexus-missing-${Date.now()}-${Math.random()}`); + expect(findGitRootByDotGit(missing)).toBeNull(); + expect(mockExecSync).not.toHaveBeenCalled(); + }); + + it('walks from a file input by starting at its parent directory', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-fileinput-')); + try { + fs.mkdirSync(path.join(tmpDir, '.git')); + const filePath = path.join(tmpDir, 'pkg', 'index.ts'); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, 'export {};\n'); + + expect(findGitRootByDotGit(filePath)).toBe(path.resolve(tmpDir)); + expect(mockExecSync).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + }); }); diff --git a/gitnexus/test/unit/sibling-clone-drift.test.ts b/gitnexus/test/unit/sibling-clone-drift.test.ts index cd063ceec..7f5b7339b 100644 --- a/gitnexus/test/unit/sibling-clone-drift.test.ts +++ b/gitnexus/test/unit/sibling-clone-drift.test.ts @@ -14,9 +14,24 @@ * `checkCwdMatch` API. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import path from 'path'; -import { execSync } from 'child_process'; +import { execSync, execFileSync } from 'child_process'; + +// Wrap child_process exports in spies that pass through to the real +// implementation. Test setup (initRepoWithCommit, etc.) keeps working +// against real git; individual tests can clear + assert call counts to +// prove no subprocess was launched. Both module specifiers are mocked +// because git-staleness.ts imports from 'node:child_process' while this +// file (and storage/git.ts) import from 'child_process'. +vi.mock('child_process', async () => { + const actual = await vi.importActual('child_process'); + return { ...actual, execSync: vi.fn(actual.execSync), execFileSync: vi.fn(actual.execFileSync) }; +}); +vi.mock('node:child_process', async () => { + const actual = await vi.importActual('node:child_process'); + return { ...actual, execSync: vi.fn(actual.execSync), execFileSync: vi.fn(actual.execFileSync) }; +}); import { registerRepo, readRegistry, @@ -233,6 +248,44 @@ describe('checkCwdMatch', () => { } }); + it('returns match=none for a non-git cwd before resolving sibling remotes', async () => { + const indexed = await createTempDir('cwd-home-indexed-'); + const nonGitCwd = await createTempDir('cwd-home-non-git-'); + try { + const indexedHead = initRepoWithCommit(indexed.dbPath, 'https://example.com/foo/bar'); + await registerRepo(indexed.dbPath, { + repoPath: indexed.dbPath, + lastCommit: indexedHead, + indexedAt: new Date().toISOString(), + remoteUrl: 'https://example.com/foo/bar', + }); + + // Clear after setup so we only count subprocess calls made by + // checkCwdMatch itself. The fix in #1138 must guarantee that no + // git subprocess is launched when the cwd is outside any .git + // ancestor — a return value of 'none' alone does not prove that + // (the pre-fix code also returned 'none', just by failing the + // spawn). The mocked module exports are vi.fn() wrappers that + // pass through to real implementations; clearing tracks only the + // calls made by checkCwdMatch below. + vi.mocked(execSync).mockClear(); + vi.mocked(execFileSync).mockClear(); + const nodeCp = await import('node:child_process'); + vi.mocked(nodeCp.execSync).mockClear(); + vi.mocked(nodeCp.execFileSync).mockClear(); + + const m = await checkCwdMatch(nonGitCwd.dbPath); + expect(m.match).toBe('none'); + expect(execSync).not.toHaveBeenCalled(); + expect(execFileSync).not.toHaveBeenCalled(); + expect(nodeCp.execSync).not.toHaveBeenCalled(); + expect(nodeCp.execFileSync).not.toHaveBeenCalled(); + } finally { + await indexed.cleanup(); + await nonGitCwd.cleanup(); + } + }); + it('reports sibling-by-remote with a stale hint when cwd HEAD has advanced', async () => { // Polecat-style scenario from the issue: index at path A, query // from cwd=path B (same repo), get a warning rather than