-
Notifications
You must be signed in to change notification settings - Fork 2
fix: prevent findDbPath from escaping git worktree boundary #457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5d29cd9
deac23e
34820e6
83efde5
ccb8bd5
73c51a4
e3c1d4a
5b4a5cd
20013d4
7c877d7
7e69570
0d0fdd9
2fa9dfc
651c164
5dc707a
5a9dd2b
b87a12b
1301b9f
25e940b
5a77db1
de87d18
620128b
78b791c
8a41032
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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, | ||||||||||
|
|
@@ -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; | ||||||||||
| _resetRepoRootCache(); | ||||||||||
| } | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
|
|
@@ -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(); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('caches results when called without arguments', () => { | ||||||||||
| _resetRepoRootCache(); | ||||||||||
| execFileSyncSpy.mockClear(); | ||||||||||
| const first = findRepoRoot(); | ||||||||||
| const second = findRepoRoot(); | ||||||||||
| expect(first).toBe(second); | ||||||||||
| expect(execFileSyncSpy).toHaveBeenCalledTimes(1); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test assertion will fail on macOS when
These are not equal, so the test fails. The production code in
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }); | ||||||||||
| } | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| 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 }); | ||||||||||
| } | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| describe('openReadonlyOrFail', () => { | ||||||||||
| it('throws DbError when DB does not exist', () => { | ||||||||||
| expect.assertions(4); | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
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()toceiling(the git repo root) for any user inside a git repository. Previously, runningcodegraph buildfrom/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:
There was a problem hiding this comment.
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. Theprocess.cwd()fallback is preserved when not in a git repo (line 109:const base = ceiling || process.cwd()).