Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5d29cd9
fix: prevent findDbPath from escaping git worktree boundary
carlos-alm Mar 16, 2026
deac23e
fix: address review — real git ceiling test, debug log, robust non-gi…
carlos-alm Mar 16, 2026
34820e6
Merge remote-tracking branch 'origin/main' into fix/db-path-worktree-…
carlos-alm Mar 16, 2026
83efde5
test: strengthen findRepoRoot and ceiling tests per review feedback
carlos-alm Mar 16, 2026
ccb8bd5
Revert "test: strengthen findRepoRoot and ceiling tests per review fe…
carlos-alm Mar 16, 2026
73c51a4
Reapply "test: strengthen findRepoRoot and ceiling tests per review f…
carlos-alm Mar 16, 2026
e3c1d4a
fix: address Greptile review — flaky non-git test and misleading impo…
carlos-alm Mar 16, 2026
5b4a5cd
merge: resolve conflict with main in db test imports
carlos-alm Mar 16, 2026
20013d4
fix: resolve symlinks in findDbPath to fix ceiling check on macOS
carlos-alm Mar 16, 2026
7c877d7
style: format connection.js try/catch block
carlos-alm Mar 16, 2026
7e69570
Merge remote-tracking branch 'origin/main' into fix/db-path-worktree-…
carlos-alm Mar 16, 2026
0d0fdd9
test: harden findDbPath fallback test to mock execFileSync
carlos-alm Mar 16, 2026
2fa9dfc
fix: normalize findRepoRoot paths with realpathSync for cross-platfor…
carlos-alm Mar 16, 2026
651c164
fix: use stat-based path comparison for ceiling check on Windows
carlos-alm Mar 16, 2026
5dc707a
fix: remove test-only _resetRepoRootCache from public barrel export
carlos-alm Mar 16, 2026
5a9dd2b
Merge branch 'main' into fix/db-path-worktree-boundary
carlos-alm Mar 16, 2026
b87a12b
fix: tighten test assertions and key repo root cache on cwd
carlos-alm Mar 16, 2026
1301b9f
fix: normalize ceiling path in test to handle Windows 8.3 short names
carlos-alm Mar 16, 2026
25e940b
Merge branch 'main' into fix/db-path-worktree-boundary
carlos-alm Mar 16, 2026
5a77db1
fix: normalize ceiling path with realpathSync to handle Windows 8.3 s…
carlos-alm Mar 16, 2026
de87d18
Merge branch 'fix/db-path-worktree-boundary' of https://github.com/op…
carlos-alm Mar 16, 2026
620128b
fix: normalize Windows 8.3 short paths in ceiling boundary test
carlos-alm Mar 16, 2026
78b791c
fix: use existence-based assertions for ceiling test to handle Window…
carlos-alm Mar 16, 2026
8a41032
Merge remote-tracking branch 'origin/main' into fix/db-path-worktree-…
carlos-alm Mar 16, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 95 additions & 3 deletions src/db/connection.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,61 @@
import { execFileSync } from 'node:child_process';
import fs from 'node:fs';
import path from 'node:path';
import Database from 'better-sqlite3';
import { warn } from '../infrastructure/logger.js';
import { debug, warn } from '../infrastructure/logger.js';
import { DbError } from '../shared/errors.js';
import { Repository } from './repository/base.js';
import { SqliteRepository } from './repository/sqlite-repository.js';

let _cachedRepoRoot; // undefined = not computed, null = not a git repo
let _cachedRepoRootCwd; // cwd at the time the cache was populated

/**
* Return the git worktree/repo root for the given directory (or cwd).
* Uses `git rev-parse --show-toplevel` which returns the correct root
* for both regular repos and git worktrees.
* Results are cached per-process when called without arguments.
* The cache is keyed on cwd so it invalidates if the working directory changes
* (e.g. MCP server serving multiple sessions).
* @param {string} [fromDir] - Directory to resolve from (defaults to cwd)
* @returns {string | null} Absolute path to repo root, or null if not in a git repo
*/
export function findRepoRoot(fromDir) {
const dir = fromDir || process.cwd();
if (!fromDir && _cachedRepoRoot !== undefined && _cachedRepoRootCwd === dir) {
return _cachedRepoRoot;
}
let root = null;
try {
const raw = execFileSync('git', ['rev-parse', '--show-toplevel'], {
cwd: dir,
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'pipe'],
}).trim();
// Use realpathSync to resolve symlinks (macOS /var → /private/var) and
// 8.3 short names (Windows RUNNER~1 → runneradmin) so the ceiling path
// matches the realpathSync'd dir in findDbPath.
try {
root = fs.realpathSync(raw);
} catch {
root = path.resolve(raw);
}
} catch {
root = null;
}
if (!fromDir) {
_cachedRepoRoot = root;
_cachedRepoRootCwd = dir;
}
return root;
}
Comment on lines +23 to +51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale cache if process.cwd() changes after first call

findRepoRoot() (no args) caches its result the first time it's called and returns that value on every subsequent no-arg call — even if process.cwd() has changed in the meantime. In a long-running process (e.g. an MCP server that serves multiple worktree sessions sequentially), a second session with a different cwd would receive the ceiling from the first session. findDbPath() then computes dir from the current process.cwd() but compares it against a ceiling from the old cwd, allowing the walk to escape the expected boundary.

The per-process cache is intentional and documented, and it's benign if each codegraph invocation is a fresh process. However, for the MCP-server use case explicitly called out in the PR, the lifetime of the process spans multiple client connections. A defensive fix would be to key the cache on the cwd value captured at call time:

let _cachedRepoRoot; // undefined = not computed
let _cachedRepoRootCwd; // cwd when the cache was populated

export function findRepoRoot(fromDir) {
  const dir = fromDir || process.cwd();
  if (!fromDir && _cachedRepoRoot !== undefined && _cachedRepoRootCwd === dir) {
    return _cachedRepoRoot;
  }
  // ... rest of implementation ...
  if (!fromDir) {
    _cachedRepoRoot = root;
    _cachedRepoRootCwd = dir;
  }
  return root;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the cache is now keyed on the cwd value at call time. If process.cwd() changes between calls (e.g. MCP server serving multiple sessions), the cache is invalidated and recomputed. _resetRepoRootCache clears both the cached value and the cwd key.


/** Reset the cached repo root (for testing). */
export function _resetRepoRootCache() {
_cachedRepoRoot = undefined;
_cachedRepoRootCwd = undefined;
}

function isProcessAlive(pid) {
try {
process.kill(pid, 0);
Expand Down Expand Up @@ -46,6 +96,22 @@ function releaseAdvisoryLock(lockPath) {
}
}

/**
* Check if two paths refer to the same directory.
* Handles Windows 8.3 short names (RUNNER~1 vs runneradmin) and macOS
* symlinks (/tmp vs /private/tmp) where string comparison fails.
*/
function isSameDirectory(a, b) {
if (path.resolve(a) === path.resolve(b)) return true;
try {
const sa = fs.statSync(a);
const sb = fs.statSync(b);
return sa.dev === sb.dev && sa.ino === sb.ino;
} catch {
return false;
}
}

export function openDb(dbPath) {
const dir = path.dirname(dbPath);
if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true });
Expand All @@ -64,15 +130,41 @@ export function closeDb(db) {

export function findDbPath(customPath) {
if (customPath) return path.resolve(customPath);
let dir = process.cwd();
const rawCeiling = findRepoRoot();
// Normalize ceiling with realpathSync to resolve 8.3 short names (Windows
// RUNNER~1 → runneradmin) and symlinks (macOS /var → /private/var).
// findRepoRoot already applies realpathSync internally, but the git output
// may still contain short names on some Windows CI environments.
let ceiling;
if (rawCeiling) {
try {
ceiling = fs.realpathSync(rawCeiling);
} catch {
ceiling = rawCeiling;
}
} else {
ceiling = null;
}
// Resolve symlinks (e.g. macOS /var → /private/var) so dir matches ceiling from git
let dir;
try {
dir = fs.realpathSync(process.cwd());
} catch {
dir = process.cwd();
}
while (true) {
const candidate = path.join(dir, '.codegraph', 'graph.db');
if (fs.existsSync(candidate)) return candidate;
if (ceiling && isSameDirectory(dir, ceiling)) {
debug(`findDbPath: stopped at git ceiling ${ceiling}`);
break;
}
const parent = path.dirname(dir);
if (parent === dir) break;
dir = parent;
}
return path.join(process.cwd(), '.codegraph', 'graph.db');
const base = ceiling || process.cwd();
return path.join(base, '.codegraph', 'graph.db');
Comment on lines +166 to +167
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undocumented behavior change for non-worktree users

When no existing DB is found during the walk-up, the default creation path has silently changed from process.cwd() to ceiling (the git repo root) for any user inside a git repository. Previously, running codegraph build from /repo/sub/ with no DB would create /repo/sub/.codegraph/graph.db. After this PR, it creates /repo/.codegraph/graph.db.

This is likely the desired long-term behavior, but it's a breaking change for users who intentionally scope their codegraph DB to a project subdirectory rather than the repo root. The PR description only mentions the worktree ceiling fix, not the changed default creation location.

Consider documenting this in the PR description or CHANGELOG, or preserving the previous fallback for non-worktree scenarios:

const base = ceiling || process.cwd();
return path.join(base, '.codegraph', 'graph.db');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on documenting this. Updated the PR description with a Behavior change section explaining that the default DB creation path is now the git repo root instead of process.cwd() when inside a git repo. The process.cwd() fallback is preserved when not in a git repo (line 109: const base = ceiling || process.cwd()).

}

/**
Expand Down
9 changes: 8 additions & 1 deletion src/db/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
// Barrel re-export — keeps all existing `import { ... } from '…/db/index.js'` working.
export { closeDb, findDbPath, openDb, openReadonlyOrFail, openRepo } from './connection.js';
export {
closeDb,
findDbPath,
findRepoRoot,
openDb,
openReadonlyOrFail,
openRepo,
} from './connection.js';
Comment on lines +2 to +9
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-only utility exported through the public barrel

_resetRepoRootCache is a testing helper (signalled by the leading underscore) but is now re-exported from the public barrel src/db/index.js. Any consumer that imports from …/db/index.js will see this function in their IDE completions and bundled types, and could accidentally call it in production code to blow away the per-process cache.

A cleaner approach is to have the test files import directly from the implementation module:

// in tests/unit/db.test.js
import { _resetRepoRootCache, findRepoRoot, ... } from '../../src/db/connection.js';

Then remove _resetRepoRootCache from the barrel export, keeping the public API free of test infrastructure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest push. Removed _resetRepoRootCache from the src/db/index.js barrel export. The test file now imports it directly from ../../src/db/connection.js, keeping the public API free of test infrastructure.

export { getBuildMeta, initSchema, MIGRATIONS, setBuildMeta } from './migrations.js';
export {
fanInJoinSQL,
Expand Down
163 changes: 160 additions & 3 deletions tests/unit/db.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,28 @@
* Unit tests for src/db.js — build_meta helpers included
*/

// Note: due to vi.mock hoisting, this resolves to the spy (which delegates
// to the real impl by default). Safe for setup calls before mockImplementationOnce.
import { execFileSync as execFileSyncForSetup } from 'node:child_process';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import Database from 'better-sqlite3';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';

const execFileSyncSpy = vi.hoisted(() => vi.fn());

vi.mock('node:child_process', async (importOriginal) => {
const mod = await importOriginal();
execFileSyncSpy.mockImplementation(mod.execFileSync);
return { ...mod, execFileSync: execFileSyncSpy };
});

import { _resetRepoRootCache } from '../../src/db/connection.js';
import {
closeDb,
findDbPath,
findRepoRoot,
getBuildMeta,
initSchema,
MIGRATIONS,
Expand Down Expand Up @@ -131,24 +145,30 @@ describe('findDbPath', () => {
const origCwd = process.cwd;
process.cwd = () => deepDir;
try {
_resetRepoRootCache();
const result = findDbPath();
expect(result).toContain('.codegraph');
expect(result).toContain('graph.db');
} finally {
process.cwd = origCwd;
_resetRepoRootCache();
}
});

it('returns default path when no DB found', () => {
const emptyDir = fs.mkdtempSync(path.join(tmpDir, 'empty-'));
const origCwd = process.cwd;
process.cwd = () => emptyDir;
_resetRepoRootCache();
execFileSyncSpy.mockImplementationOnce(() => {
throw new Error('not a git repo');
});
try {
const result = findDbPath();
expect(result).toContain('.codegraph');
expect(result).toContain('graph.db');
expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
} finally {
process.cwd = origCwd;
Comment on lines 155 to 170
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findDbPath fallback test doesn't control the ceiling

This test calls _resetRepoRootCache() but never mocks execFileSync to throw, so findRepoRoot() will call the real git rev-parse --show-toplevel from emptyDir. If the test host's os.tmpdir() is inside a git repo (common in some CI setups), ceiling will be set to that repo root and the default path returned will be <git-root>/.codegraph/graph.db rather than emptyDir/.codegraph/graph.db. The toContain assertions pass either way, so the test silently misses validating the actual fallback location.

The analogous 'falls back gracefully when not in a git repo' test in the new findDbPath with git ceiling describe already solves this correctly with execFileSyncSpy.mockImplementationOnce. The same treatment should be applied here:

Suggested change
it('returns default path when no DB found', () => {
const emptyDir = fs.mkdtempSync(path.join(tmpDir, 'empty-'));
const origCwd = process.cwd;
process.cwd = () => emptyDir;
execFileSyncSpy.mockImplementationOnce(() => {
throw new Error('not a git repo');
});
try {
_resetRepoRootCache();
const result = findDbPath();
expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
} finally {
process.cwd = origCwd;
_resetRepoRootCache();
}
});

This ensures ceiling = null, the process.cwd() fallback is exercised deterministically, and the assertion is precise enough to detect regressions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0d0fdd9. The test now mocks execFileSync to throw (via mockImplementationOnce) so findRepoRoot() always returns null, ensuring the process.cwd() fallback is deterministically exercised. Also tightened the assertion from toContain to toBe with the exact expected path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already fixed in 0d0fdd9 — the test now mocks execFileSync to throw via mockImplementationOnce, ensuring findRepoRoot() always returns null regardless of host environment. The assertion was also tightened from toContain to toBe with the exact expected path.

_resetRepoRootCache();
}
});
});
Expand Down Expand Up @@ -194,6 +214,143 @@ describe('build_meta', () => {
});
});

describe('findRepoRoot', () => {
beforeEach(() => {
_resetRepoRootCache();
});

afterEach(() => {
_resetRepoRootCache();
});

it('returns normalized git toplevel for the current repo', () => {
_resetRepoRootCache();
const root = findRepoRoot();
expect(root).toBeTruthy();
expect(path.isAbsolute(root)).toBe(true);
// Should contain a .git entry at the root
expect(fs.existsSync(path.join(root, '.git'))).toBe(true);
});

it('returns null when not in a git repo', () => {
execFileSyncSpy.mockImplementationOnce(() => {
throw new Error('not a git repo');
});
const root = findRepoRoot(os.tmpdir());
expect(root).toBeNull();
});
Comment on lines +235 to +241
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test silently passes without asserting

If os.tmpdir() happens to be inside a git repo (possible on some CI environments or developer machines), this test passes without making any assertion at all — the if block is skipped. Consider using vi.spyOn to mock execFileSync to throw, ensuring the null path is always exercised regardless of the host environment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 83efde5. Now uses vi.mock('node:child_process') with mockImplementationOnce to force execFileSync to throw, ensuring the null path is always exercised regardless of host environment.


it('caches results when called without arguments', () => {
_resetRepoRootCache();
execFileSyncSpy.mockClear();
const first = findRepoRoot();
const second = findRepoRoot();
expect(first).toBe(second);
expect(execFileSyncSpy).toHaveBeenCalledTimes(1);
});
Comment on lines +243 to +250
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching test does not actually verify caching

JavaScript string primitives are compared by value, not reference, so expect(first).toBe(second) passes even if execFileSync is called twice and returns two distinct string objects with the same content. The test does not prove that the cache is being used.

To truly verify the caching behaviour, spy on execFileSync and assert it was called exactly once:

it('caches results when called without arguments', () => {
  _resetRepoRootCache();
  const spy = vi.spyOn(childProcess, 'execFileSync');
  const first = findRepoRoot();
  const second = findRepoRoot();
  expect(first).toBe(second);
  expect(spy).toHaveBeenCalledTimes(1); // cached on second call
  spy.mockRestore();
});

This requires importing node:child_process as a namespace (import * as childProcess from 'node:child_process') so the spy can intercept the module-level binding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 83efde5. The caching test now uses vi.mock('node:child_process') to create a spy on execFileSync, clears it before the test, and asserts toHaveBeenCalledTimes(1) — proving the second call hits the cache.


it('bypasses cache when called with explicit dir', () => {
_resetRepoRootCache();
execFileSyncSpy.mockClear();
const fromCwd = findRepoRoot();
const fromExplicit = findRepoRoot(process.cwd());
expect(fromExplicit).toBe(fromCwd);
// First call populates cache, second call with explicit dir must call again
expect(execFileSyncSpy).toHaveBeenCalledTimes(2);
});
});

describe('findDbPath with git ceiling', () => {
let outerDir;
let worktreeRoot;
let innerDir;

beforeAll(() => {
// Simulate a worktree-inside-repo layout:
// outerDir/.codegraph/graph.db (parent repo DB — should NOT be found)
// outerDir/worktree/ (git init here — acts as ceiling)
// outerDir/worktree/sub/ (cwd inside worktree)
outerDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ceiling-'));
worktreeRoot = path.join(outerDir, 'worktree');
fs.mkdirSync(path.join(outerDir, '.codegraph'), { recursive: true });
fs.writeFileSync(path.join(outerDir, '.codegraph', 'graph.db'), '');
fs.mkdirSync(path.join(worktreeRoot, 'sub'), { recursive: true });
// Initialize a real git repo at the worktree root so findRepoRoot returns it
execFileSyncForSetup('git', ['init'], { cwd: worktreeRoot, stdio: 'pipe' });
// Resolve symlinks (macOS /var → /private/var) and 8.3 short names
// (Windows RUNNER~1 → runneradmin) so test paths match findRepoRoot output.
outerDir = fs.realpathSync(outerDir);
worktreeRoot = fs.realpathSync(worktreeRoot);
innerDir = path.join(worktreeRoot, 'sub');
});

afterAll(() => {
fs.rmSync(outerDir, { recursive: true, force: true });
});

afterEach(() => {
_resetRepoRootCache();
});

it('stops at git ceiling and does not find parent DB', () => {
// No DB inside the worktree — the only DB is in outerDir (beyond the ceiling).
// Without the ceiling fix, findDbPath would walk up and find outerDir's DB.
const origCwd = process.cwd;
process.cwd = () => innerDir;
try {
_resetRepoRootCache();
Comment on lines +300 to +301
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test assertion will fail on macOS when /tmp is the temp dir

result is built from ceiling, which is the real (symlink-resolved) path returned by git rev-parse --show-toplevel. On macOS, if os.tmpdir() resolves through /tmp (which is a symlink to /private/tmp), git will return /private/tmp/cg-ceiling-xxx/worktree while worktreeRoot is still /tmp/cg-ceiling-xxx/worktree. The toBe assertion then compares:

  • actual: /private/tmp/cg-ceiling-xxx/worktree/.codegraph/graph.db
  • expected: /tmp/cg-ceiling-xxx/worktree/.codegraph/graph.db

These are not equal, so the test fails. The production code in connection.js is actually correct — realpathSync is applied to dir precisely to ensure ceiling (from git) and dir agree on symlinks. The test expectation just needs to match the same resolved form:

Suggested change
try {
_resetRepoRootCache();
expect(result).toBe(path.join(fs.realpathSync(worktreeRoot), '.codegraph', 'graph.db'));
expect(result).not.toContain(`${path.basename(outerDir)}${path.sep}.codegraph`);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 651c164. The test now uses \ for the expected ceiling instead of the test's , so the assertion always matches the path form that \ uses internally. This handles both the macOS symlink case (\ → ) and the Windows 8.3 short name case (\ → ). Additionally, the production code now uses a stat-based \ comparison (dev+ino fallback) instead of string equality for the ceiling check, which is robust against all path representation differences.

// Use findRepoRoot() for the expected ceiling — git may resolve 8.3 short
// names (Windows RUNNER~1 → runneradmin) or symlinks (macOS /tmp → /private/tmp)
// differently than fs.realpathSync on the test's worktreeRoot.
const ceiling = findRepoRoot();
const result = findDbPath();
// Should return default path at the ceiling root, NOT the outer DB
expect(result).toBe(path.join(ceiling, '.codegraph', 'graph.db'));
expect(result).not.toContain(`${path.basename(outerDir)}${path.sep}.codegraph`);
} finally {
process.cwd = origCwd;
}
});

it('finds DB within the ceiling boundary', () => {
// Create a DB inside the worktree — should be found normally
fs.mkdirSync(path.join(worktreeRoot, '.codegraph'), { recursive: true });
fs.writeFileSync(path.join(worktreeRoot, '.codegraph', 'graph.db'), '');
const origCwd = process.cwd;
process.cwd = () => innerDir;
try {
_resetRepoRootCache();
const result = findDbPath();
// Verify the DB was found (file exists) and is the worktree DB, not the outer one
expect(fs.existsSync(result)).toBe(true);
expect(result).toMatch(/\.codegraph[/\\]graph\.db$/);
// The outer DB is at outerDir/.codegraph — verify we didn't find that one
expect(result).not.toContain(`${path.basename(outerDir)}${path.sep}.codegraph`);
} finally {
process.cwd = origCwd;
fs.rmSync(path.join(worktreeRoot, '.codegraph'), { recursive: true, force: true });
}
});
Comment on lines +315 to +333
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loose assertions don't validate the found DB location precisely

The two toContain assertions verify that the result string contains 'worktree' and '.codegraph', but they don't confirm the DB was found at worktreeRoot rather than innerDir. A path like path.join(innerDir, '.codegraph', 'graph.db') would also satisfy both checks. Every other assertion in this describe block uses toBe with an exact path. Consider using the same here for consistency and precision:

Suggested change
it('finds DB within the ceiling boundary', () => {
// Create a DB inside the worktree — should be found normally
fs.mkdirSync(path.join(worktreeRoot, '.codegraph'), { recursive: true });
fs.writeFileSync(path.join(worktreeRoot, '.codegraph', 'graph.db'), '');
const origCwd = process.cwd;
process.cwd = () => innerDir;
try {
_resetRepoRootCache();
const result = findDbPath();
expect(result).toContain('worktree');
expect(result).toContain('.codegraph');
} finally {
process.cwd = origCwd;
fs.rmSync(path.join(worktreeRoot, '.codegraph'), { recursive: true, force: true });
}
});
it('finds DB within the ceiling boundary', () => {
// Create a DB inside the worktree — should be found normally
fs.mkdirSync(path.join(worktreeRoot, '.codegraph'), { recursive: true });
fs.writeFileSync(path.join(worktreeRoot, '.codegraph', 'graph.db'), '');
const origCwd = process.cwd;
process.cwd = () => innerDir;
try {
_resetRepoRootCache();
const result = findDbPath();
const ceiling = findRepoRoot();
expect(result).toBe(path.join(ceiling, '.codegraph', 'graph.db'));
} finally {
process.cwd = origCwd;
fs.rmSync(path.join(worktreeRoot, '.codegraph'), { recursive: true, force: true });
}
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — tightened the assertion to use toBe with the exact ceiling-based path, consistent with the rest of the describe block.


it('falls back gracefully when not in a git repo', () => {
const emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-nogit-'));
const origCwd = process.cwd;
process.cwd = () => emptyDir;
_resetRepoRootCache();
execFileSyncSpy.mockImplementationOnce(() => {
throw new Error('not a git repo');
});
try {
const result = findDbPath();
// Should return default path at cwd since there's no git ceiling
expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
} finally {
process.cwd = origCwd;
fs.rmSync(emptyDir, { recursive: true, force: true });
}
});
Comment on lines +335 to +351
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky non-git fallback test — same issue that was fixed in findRepoRoot

This test creates emptyDir inside os.tmpdir() but never mocks execFileSync to throw. The spy's default implementation calls the real execFileSync, so if the test host's os.tmpdir() happens to be inside a git repository (possible on some CI setups or Docker images), findRepoRoot() will return a non-null ceiling. findDbPath() would then return ceiling/.codegraph/graph.db instead of emptyDir/.codegraph/graph.db, making the expect(result).toBe(path.join(emptyDir, ...)) assertion fail.

The analogous test in findRepoRoot ('returns null when not in a git repo') was already fixed by using mockImplementationOnce to force execFileSync to throw. The same fix should be applied here:

it('falls back gracefully when not in a git repo', () => {
  const emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-nogit-'));
  const origCwd = process.cwd;
  process.cwd = () => emptyDir;
  _resetRepoRootCache();
  execFileSyncSpy.mockImplementationOnce(() => {
    throw new Error('not a git repo');
  });
  try {
    const result = findDbPath();
    expect(result).toBe(path.join(emptyDir, '.codegraph', 'graph.db'));
  } finally {
    process.cwd = origCwd;
    fs.rmSync(emptyDir, { recursive: true, force: true });
  }
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3c1d4a. Added \ before the test, ensuring \ always returns null regardless of host environment.

});

describe('openReadonlyOrFail', () => {
it('throws DbError when DB does not exist', () => {
expect.assertions(4);
Expand Down
Loading