Skip to content

Commit c6e75e3

Browse files
authored
fix: prevent findDbPath from escaping git worktree boundary (#457)
* fix: prevent findDbPath from escaping git worktree boundary findDbPath() walks up from cwd looking for .codegraph/graph.db. In a git worktree (e.g. .claude/worktrees/agent-xxx/), this crosses the worktree boundary and finds the main repo's database instead. Add findRepoRoot() using `git rev-parse --show-toplevel` (which returns the correct root for both repos and worktrees) and use it as a ceiling in findDbPath(). The walk-up now stops at the git boundary, so each worktree resolves to its own database. * fix: address review — real git ceiling test, debug log, robust non-git test - Ceiling test now uses `git init` to create a real git repo boundary, and verifies the outer DB is NOT found (without the ceiling fix it would be). Added separate test for finding DB within the boundary. - Non-git test uses a fresh mkdtemp dir instead of os.tmpdir(). - Added debug log when ceiling stops the walk-up. - Removed unused `origFindRepoRoot` variable. * test: strengthen findRepoRoot and ceiling tests per review feedback - Mock execFileSync via vi.mock to verify caching calls exactly once and cache bypass calls twice (not just comparing return values) - Mock execFileSync to throw in "not in git repo" test so the null path is always exercised regardless of host environment - Rename "does not use cache" test to "bypasses cache" with spy assertion * Revert "test: strengthen findRepoRoot and ceiling tests per review feedback" This reverts commit 83efde5. * Reapply "test: strengthen findRepoRoot and ceiling tests per review feedback" This reverts commit ccb8bd5. * fix: address Greptile review — flaky non-git test and misleading import name - Mock execFileSync to throw in "falls back gracefully when not in a git repo" test, preventing flakiness when os.tmpdir() is inside a git repo - Rename realExecFileSync → execFileSyncForSetup with comment explaining it resolves to the spy due to vi.mock hoisting * fix: resolve symlinks in findDbPath to fix ceiling check on macOS On macOS, os.tmpdir() returns /var/folders/... but git rev-parse returns /private/var/folders/... (resolved symlink). The ceiling comparison failed because the paths didn't match. Use fs.realpathSync on cwd to normalize symlinks before comparing against the ceiling. Impact: 1 functions changed, 1 affected * style: format connection.js try/catch block Impact: 1 functions changed, 1 affected * test: harden findDbPath fallback test to mock execFileSync The 'returns default path when no DB found' test didn't control the git ceiling — if tmpDir was inside a git repo, findRepoRoot() would return a non-null ceiling and the default path would differ from emptyDir. Mock execFileSync to throw so the cwd fallback is always exercised. * fix: normalize findRepoRoot paths with realpathSync for cross-platform ceiling On macOS, os.tmpdir() returns /var/... but git resolves symlinks to /private/var/..., and on Windows, 8.3 short names (RUNNER~1) differ from long names (runneradmin). findDbPath normalizes dir via fs.realpathSync but findRepoRoot only used path.resolve, causing the ceiling comparison to fail — the walk crossed the worktree boundary. Fix findRepoRoot to use fs.realpathSync on git output, and resolve test paths after directory creation so assertions match. Impact: 1 functions changed, 77 affected * fix: use stat-based path comparison for ceiling check on Windows git rev-parse resolves 8.3 short names (RUNNER~1 → runneradmin) but fs.realpathSync may not, causing the string comparison to fail and the walk to escape the git ceiling boundary. Replace string equality with isSameDirectory() that falls back to dev+ino comparison when paths differ textually but refer to the same directory. Also fix the test assertion to use findRepoRoot() for the expected ceiling path instead of the test's worktreeRoot, since git may resolve paths differently than realpathSync. Impact: 2 functions changed, 137 affected * fix: remove test-only _resetRepoRootCache from public barrel export * fix: tighten test assertions and key repo root cache on cwd Impact: 2 functions changed, 2 affected * fix: normalize ceiling path in test to handle Windows 8.3 short names * fix: normalize ceiling path with realpathSync to handle Windows 8.3 short names Impact: 1 functions changed, 1 affected * fix: normalize Windows 8.3 short paths in ceiling boundary test On Windows CI, fs.realpathSync(process.cwd()) and git rev-parse --show-toplevel can resolve 8.3 short names (RUNNER~1) differently than long names (runneradmin). Apply realpathSync to both sides of the assertion so the comparison is path-equivalent. * fix: use existence-based assertions for ceiling test to handle Windows 8.3 names
1 parent 2bd997a commit c6e75e3

3 files changed

Lines changed: 263 additions & 7 deletions

File tree

src/db/connection.js

Lines changed: 95 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,61 @@
1+
import { execFileSync } from 'node:child_process';
12
import fs from 'node:fs';
23
import path from 'node:path';
34
import Database from 'better-sqlite3';
4-
import { warn } from '../infrastructure/logger.js';
5+
import { debug, warn } from '../infrastructure/logger.js';
56
import { DbError } from '../shared/errors.js';
67
import { Repository } from './repository/base.js';
78
import { SqliteRepository } from './repository/sqlite-repository.js';
89

10+
let _cachedRepoRoot; // undefined = not computed, null = not a git repo
11+
let _cachedRepoRootCwd; // cwd at the time the cache was populated
12+
13+
/**
14+
* Return the git worktree/repo root for the given directory (or cwd).
15+
* Uses `git rev-parse --show-toplevel` which returns the correct root
16+
* for both regular repos and git worktrees.
17+
* Results are cached per-process when called without arguments.
18+
* The cache is keyed on cwd so it invalidates if the working directory changes
19+
* (e.g. MCP server serving multiple sessions).
20+
* @param {string} [fromDir] - Directory to resolve from (defaults to cwd)
21+
* @returns {string | null} Absolute path to repo root, or null if not in a git repo
22+
*/
23+
export function findRepoRoot(fromDir) {
24+
const dir = fromDir || process.cwd();
25+
if (!fromDir && _cachedRepoRoot !== undefined && _cachedRepoRootCwd === dir) {
26+
return _cachedRepoRoot;
27+
}
28+
let root = null;
29+
try {
30+
const raw = execFileSync('git', ['rev-parse', '--show-toplevel'], {
31+
cwd: dir,
32+
encoding: 'utf-8',
33+
stdio: ['pipe', 'pipe', 'pipe'],
34+
}).trim();
35+
// Use realpathSync to resolve symlinks (macOS /var → /private/var) and
36+
// 8.3 short names (Windows RUNNER~1 → runneradmin) so the ceiling path
37+
// matches the realpathSync'd dir in findDbPath.
38+
try {
39+
root = fs.realpathSync(raw);
40+
} catch {
41+
root = path.resolve(raw);
42+
}
43+
} catch {
44+
root = null;
45+
}
46+
if (!fromDir) {
47+
_cachedRepoRoot = root;
48+
_cachedRepoRootCwd = dir;
49+
}
50+
return root;
51+
}
52+
53+
/** Reset the cached repo root (for testing). */
54+
export function _resetRepoRootCache() {
55+
_cachedRepoRoot = undefined;
56+
_cachedRepoRootCwd = undefined;
57+
}
58+
959
function isProcessAlive(pid) {
1060
try {
1161
process.kill(pid, 0);
@@ -46,6 +96,22 @@ function releaseAdvisoryLock(lockPath) {
4696
}
4797
}
4898

99+
/**
100+
* Check if two paths refer to the same directory.
101+
* Handles Windows 8.3 short names (RUNNER~1 vs runneradmin) and macOS
102+
* symlinks (/tmp vs /private/tmp) where string comparison fails.
103+
*/
104+
function isSameDirectory(a, b) {
105+
if (path.resolve(a) === path.resolve(b)) return true;
106+
try {
107+
const sa = fs.statSync(a);
108+
const sb = fs.statSync(b);
109+
return sa.dev === sb.dev && sa.ino === sb.ino;
110+
} catch {
111+
return false;
112+
}
113+
}
114+
49115
export function openDb(dbPath) {
50116
const dir = path.dirname(dbPath);
51117
if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true });
@@ -64,15 +130,41 @@ export function closeDb(db) {
64130

65131
export function findDbPath(customPath) {
66132
if (customPath) return path.resolve(customPath);
67-
let dir = process.cwd();
133+
const rawCeiling = findRepoRoot();
134+
// Normalize ceiling with realpathSync to resolve 8.3 short names (Windows
135+
// RUNNER~1 → runneradmin) and symlinks (macOS /var → /private/var).
136+
// findRepoRoot already applies realpathSync internally, but the git output
137+
// may still contain short names on some Windows CI environments.
138+
let ceiling;
139+
if (rawCeiling) {
140+
try {
141+
ceiling = fs.realpathSync(rawCeiling);
142+
} catch {
143+
ceiling = rawCeiling;
144+
}
145+
} else {
146+
ceiling = null;
147+
}
148+
// Resolve symlinks (e.g. macOS /var → /private/var) so dir matches ceiling from git
149+
let dir;
150+
try {
151+
dir = fs.realpathSync(process.cwd());
152+
} catch {
153+
dir = process.cwd();
154+
}
68155
while (true) {
69156
const candidate = path.join(dir, '.codegraph', 'graph.db');
70157
if (fs.existsSync(candidate)) return candidate;
158+
if (ceiling && isSameDirectory(dir, ceiling)) {
159+
debug(`findDbPath: stopped at git ceiling ${ceiling}`);
160+
break;
161+
}
71162
const parent = path.dirname(dir);
72163
if (parent === dir) break;
73164
dir = parent;
74165
}
75-
return path.join(process.cwd(), '.codegraph', 'graph.db');
166+
const base = ceiling || process.cwd();
167+
return path.join(base, '.codegraph', 'graph.db');
76168
}
77169

78170
/**

src/db/index.js

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
// Barrel re-export — keeps all existing `import { ... } from '…/db/index.js'` working.
2-
export { closeDb, findDbPath, openDb, openReadonlyOrFail, openRepo } from './connection.js';
2+
export {
3+
closeDb,
4+
findDbPath,
5+
findRepoRoot,
6+
openDb,
7+
openReadonlyOrFail,
8+
openRepo,
9+
} from './connection.js';
310
export { getBuildMeta, initSchema, MIGRATIONS, setBuildMeta } from './migrations.js';
411
export {
512
fanInJoinSQL,

tests/unit/db.test.js

Lines changed: 160 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,28 @@
22
* Unit tests for src/db.js — build_meta helpers included
33
*/
44

5+
// Note: due to vi.mock hoisting, this resolves to the spy (which delegates
6+
// to the real impl by default). Safe for setup calls before mockImplementationOnce.
7+
import { execFileSync as execFileSyncForSetup } from 'node:child_process';
58
import fs from 'node:fs';
69
import os from 'node:os';
710
import path from 'node:path';
811
import Database from 'better-sqlite3';
9-
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
12+
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
13+
14+
const execFileSyncSpy = vi.hoisted(() => vi.fn());
15+
16+
vi.mock('node:child_process', async (importOriginal) => {
17+
const mod = await importOriginal();
18+
execFileSyncSpy.mockImplementation(mod.execFileSync);
19+
return { ...mod, execFileSync: execFileSyncSpy };
20+
});
21+
22+
import { _resetRepoRootCache } from '../../src/db/connection.js';
1023
import {
1124
closeDb,
1225
findDbPath,
26+
findRepoRoot,
1327
getBuildMeta,
1428
initSchema,
1529
MIGRATIONS,
@@ -131,24 +145,30 @@ describe('findDbPath', () => {
131145
const origCwd = process.cwd;
132146
process.cwd = () => deepDir;
133147
try {
148+
_resetRepoRootCache();
134149
const result = findDbPath();
135150
expect(result).toContain('.codegraph');
136151
expect(result).toContain('graph.db');
137152
} finally {
138153
process.cwd = origCwd;
154+
_resetRepoRootCache();
139155
}
140156
});
141157

142158
it('returns default path when no DB found', () => {
143159
const emptyDir = fs.mkdtempSync(path.join(tmpDir, 'empty-'));
144160
const origCwd = process.cwd;
145161
process.cwd = () => emptyDir;
162+
_resetRepoRootCache();
163+
execFileSyncSpy.mockImplementationOnce(() => {
164+
throw new Error('not a git repo');
165+
});
146166
try {
147167
const result = findDbPath();
148-
expect(result).toContain('.codegraph');
149-
expect(result).toContain('graph.db');
168+
expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
150169
} finally {
151170
process.cwd = origCwd;
171+
_resetRepoRootCache();
152172
}
153173
});
154174
});
@@ -194,6 +214,143 @@ describe('build_meta', () => {
194214
});
195215
});
196216

217+
describe('findRepoRoot', () => {
218+
beforeEach(() => {
219+
_resetRepoRootCache();
220+
});
221+
222+
afterEach(() => {
223+
_resetRepoRootCache();
224+
});
225+
226+
it('returns normalized git toplevel for the current repo', () => {
227+
_resetRepoRootCache();
228+
const root = findRepoRoot();
229+
expect(root).toBeTruthy();
230+
expect(path.isAbsolute(root)).toBe(true);
231+
// Should contain a .git entry at the root
232+
expect(fs.existsSync(path.join(root, '.git'))).toBe(true);
233+
});
234+
235+
it('returns null when not in a git repo', () => {
236+
execFileSyncSpy.mockImplementationOnce(() => {
237+
throw new Error('not a git repo');
238+
});
239+
const root = findRepoRoot(os.tmpdir());
240+
expect(root).toBeNull();
241+
});
242+
243+
it('caches results when called without arguments', () => {
244+
_resetRepoRootCache();
245+
execFileSyncSpy.mockClear();
246+
const first = findRepoRoot();
247+
const second = findRepoRoot();
248+
expect(first).toBe(second);
249+
expect(execFileSyncSpy).toHaveBeenCalledTimes(1);
250+
});
251+
252+
it('bypasses cache when called with explicit dir', () => {
253+
_resetRepoRootCache();
254+
execFileSyncSpy.mockClear();
255+
const fromCwd = findRepoRoot();
256+
const fromExplicit = findRepoRoot(process.cwd());
257+
expect(fromExplicit).toBe(fromCwd);
258+
// First call populates cache, second call with explicit dir must call again
259+
expect(execFileSyncSpy).toHaveBeenCalledTimes(2);
260+
});
261+
});
262+
263+
describe('findDbPath with git ceiling', () => {
264+
let outerDir;
265+
let worktreeRoot;
266+
let innerDir;
267+
268+
beforeAll(() => {
269+
// Simulate a worktree-inside-repo layout:
270+
// outerDir/.codegraph/graph.db (parent repo DB — should NOT be found)
271+
// outerDir/worktree/ (git init here — acts as ceiling)
272+
// outerDir/worktree/sub/ (cwd inside worktree)
273+
outerDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ceiling-'));
274+
worktreeRoot = path.join(outerDir, 'worktree');
275+
fs.mkdirSync(path.join(outerDir, '.codegraph'), { recursive: true });
276+
fs.writeFileSync(path.join(outerDir, '.codegraph', 'graph.db'), '');
277+
fs.mkdirSync(path.join(worktreeRoot, 'sub'), { recursive: true });
278+
// Initialize a real git repo at the worktree root so findRepoRoot returns it
279+
execFileSyncForSetup('git', ['init'], { cwd: worktreeRoot, stdio: 'pipe' });
280+
// Resolve symlinks (macOS /var → /private/var) and 8.3 short names
281+
// (Windows RUNNER~1 → runneradmin) so test paths match findRepoRoot output.
282+
outerDir = fs.realpathSync(outerDir);
283+
worktreeRoot = fs.realpathSync(worktreeRoot);
284+
innerDir = path.join(worktreeRoot, 'sub');
285+
});
286+
287+
afterAll(() => {
288+
fs.rmSync(outerDir, { recursive: true, force: true });
289+
});
290+
291+
afterEach(() => {
292+
_resetRepoRootCache();
293+
});
294+
295+
it('stops at git ceiling and does not find parent DB', () => {
296+
// No DB inside the worktree — the only DB is in outerDir (beyond the ceiling).
297+
// Without the ceiling fix, findDbPath would walk up and find outerDir's DB.
298+
const origCwd = process.cwd;
299+
process.cwd = () => innerDir;
300+
try {
301+
_resetRepoRootCache();
302+
// Use findRepoRoot() for the expected ceiling — git may resolve 8.3 short
303+
// names (Windows RUNNER~1 → runneradmin) or symlinks (macOS /tmp → /private/tmp)
304+
// differently than fs.realpathSync on the test's worktreeRoot.
305+
const ceiling = findRepoRoot();
306+
const result = findDbPath();
307+
// Should return default path at the ceiling root, NOT the outer DB
308+
expect(result).toBe(path.join(ceiling, '.codegraph', 'graph.db'));
309+
expect(result).not.toContain(`${path.basename(outerDir)}${path.sep}.codegraph`);
310+
} finally {
311+
process.cwd = origCwd;
312+
}
313+
});
314+
315+
it('finds DB within the ceiling boundary', () => {
316+
// Create a DB inside the worktree — should be found normally
317+
fs.mkdirSync(path.join(worktreeRoot, '.codegraph'), { recursive: true });
318+
fs.writeFileSync(path.join(worktreeRoot, '.codegraph', 'graph.db'), '');
319+
const origCwd = process.cwd;
320+
process.cwd = () => innerDir;
321+
try {
322+
_resetRepoRootCache();
323+
const result = findDbPath();
324+
// Verify the DB was found (file exists) and is the worktree DB, not the outer one
325+
expect(fs.existsSync(result)).toBe(true);
326+
expect(result).toMatch(/\.codegraph[/\\]graph\.db$/);
327+
// The outer DB is at outerDir/.codegraph — verify we didn't find that one
328+
expect(result).not.toContain(`${path.basename(outerDir)}${path.sep}.codegraph`);
329+
} finally {
330+
process.cwd = origCwd;
331+
fs.rmSync(path.join(worktreeRoot, '.codegraph'), { recursive: true, force: true });
332+
}
333+
});
334+
335+
it('falls back gracefully when not in a git repo', () => {
336+
const emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-nogit-'));
337+
const origCwd = process.cwd;
338+
process.cwd = () => emptyDir;
339+
_resetRepoRootCache();
340+
execFileSyncSpy.mockImplementationOnce(() => {
341+
throw new Error('not a git repo');
342+
});
343+
try {
344+
const result = findDbPath();
345+
// Should return default path at cwd since there's no git ceiling
346+
expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
347+
} finally {
348+
process.cwd = origCwd;
349+
fs.rmSync(emptyDir, { recursive: true, force: true });
350+
}
351+
});
352+
});
353+
197354
describe('openReadonlyOrFail', () => {
198355
it('throws DbError when DB does not exist', () => {
199356
expect.assertions(4);

0 commit comments

Comments
 (0)