Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 11 additions & 3 deletions src/db/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,15 +441,18 @@ export function resolveConfigForDbPath(customDbPath?: string): CodegraphConfig {
* Derives rootDir from the resolved DB path so loadConfig reads the right project config.
* Shared by openRepo() and openReadonlyWithNative() so the two call sites can't drift.
*
* Accepts an optional pre-resolved `config` for callers that already loaded it for the
* same `customDbPath` (e.g. withRepo()), avoiding a second findDbPath()/loadConfig() call.
*
* MUST be called before opening any DB handle: loadConfig can throw (e.g. ConfigError
* via resolveSecrets on a malformed llm.apiKeyCommand config), and an already-open
* handle at that point would never be closed.
*/
function resolveDbSettings(
customDbPath: string | undefined,
engineOpt: 'native' | 'wasm' | 'auto' | undefined,
config: CodegraphConfig = resolveConfigForDbPath(customDbPath),
): ResolvedDbSettings {
const config = resolveConfigForDbPath(customDbPath);
// config.build.engine is already populated from CODEGRAPH_ENGINE env by applyEnvOverrides,
// so this covers both the env-var path and the .codegraphrc.json config-file path.
return {
Expand Down Expand Up @@ -573,18 +576,23 @@ function openRepoSqliteFallback(
* When the native engine is available, opens a NativeDatabase (rusqlite) and
* wraps it in NativeRepository. Otherwise falls back to better-sqlite3 via
* SqliteRepository.
*
* `opts.config`, when supplied, is used in place of a fresh
* resolveConfigForDbPath() call — for callers (e.g. withRepo()) that already
* resolved config for the same `customDbPath` and need to avoid a second
* loadConfig() for it.
*/
export function openRepo(
customDbPath?: string,
opts: { repo?: Repository; engine?: 'native' | 'wasm' | 'auto' } = {},
opts: { repo?: Repository; engine?: 'native' | 'wasm' | 'auto'; config?: CodegraphConfig } = {},
): { repo: Repository; close(): void } {
if (opts.repo != null) {
return wrapInjectedRepo(opts.repo);
}

// Respect explicit engine selection: opts.engine > config.build.engine > auto.
// This ensures --engine wasm and benchmark workers bypass the native path.
const { engine, busyTimeoutMs } = resolveDbSettings(customDbPath, opts.engine);
const { engine, busyTimeoutMs } = resolveDbSettings(customDbPath, opts.engine, opts.config);

const native = tryOpenRepoNative(customDbPath, engine, busyTimeoutMs);
if (native) return native;
Expand Down
5 changes: 2 additions & 3 deletions src/domain/analysis/brief.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { Repository } from '../../db/index.js';
import { loadConfig } from '../../infrastructure/config.js';
import { isTestFile } from '../../infrastructure/test-filter.js';
import type { ImportEdgeRow, NodeRow, RelatedNodeRow } from '../../types.js';
import { withRepo } from './query-helpers.js';
Expand Down Expand Up @@ -108,9 +107,9 @@ export function briefData(
customDbPath: string,
opts: { noTests?: boolean; config?: any } = {},
) {
return withRepo(customDbPath, (repo) => {
return withRepo(customDbPath, (repo, dbConfig) => {
const noTests = opts.noTests || false;
const config = opts.config || loadConfig();
const config = opts.config || dbConfig;
const callerDepth = config.analysis?.briefCallerDepth ?? 5;
const importerDepth = config.analysis?.briefImporterDepth ?? 5;
const highRiskCallers = config.analysis?.briefHighRiskCallers ?? 10;
Expand Down
4 changes: 2 additions & 2 deletions src/domain/analysis/fn-impact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,8 @@ export function fnImpactData(
config?: any;
} = {},
) {
return withRepo(customDbPath, (repo) => {
const { noTests, config } = resolveAnalysisOpts(opts);
return withRepo(customDbPath, (repo, dbConfig) => {
const { noTests, config } = resolveAnalysisOpts({ ...opts, config: opts.config ?? dbConfig });
const maxDepth = opts.depth || config.analysis?.fnImpactDepth || 5;
const hc = new Map();

Expand Down
13 changes: 10 additions & 3 deletions src/domain/analysis/query-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
openRepo,
type Repository,
resolveBusyTimeoutMs,
resolveConfigForDbPath,
} from '../../db/index.js';
import { loadConfig } from '../../infrastructure/config.js';
import type { BetterSqlite3Database, CodegraphConfig } from '../../types.js';
Expand All @@ -28,14 +29,20 @@ export function withReadonlyDb<T>(
* Open a Repository (native-first, falling back to better-sqlite3), run `fn`,
* and close on completion. Mirrors `withReadonlyDb` but routes queries through
* the native Rust engine when available.
*
* Resolves the config once and passes it to both `openRepo` (for engine/
* busy-timeout selection) and `fn`, so callers that also need
* `resolveAnalysisOpts` can reuse it as `opts.config` instead of triggering a
* second `loadConfig()` for the same rootDir (issue #1941).
*/
export function withRepo<T>(
customDbPath: string | undefined,
fn: (repo: InstanceType<typeof Repository>) => T,
fn: (repo: InstanceType<typeof Repository>, config: CodegraphConfig) => T,
): T {
const { repo, close } = openRepo(customDbPath);
const config = resolveConfigForDbPath(customDbPath);
const { repo, close } = openRepo(customDbPath, { config });
try {
return fn(repo);
return fn(repo, config);
} finally {
close();
}
Expand Down
167 changes: 167 additions & 0 deletions tests/unit/withrepo-config-resolution.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/**
* Regression tests for issue #1941: `withRepo()` and its `resolveAnalysisOpts`-
* using callers (`fnImpactData`, `briefData`) each triggered a *second*,
* independent `loadConfig()` call for the same `--db` path — one implicitly
* via `openRepo()` -> `resolveDbSettings()` (to pick the engine/busy-timeout),
* and a second one via `resolveAnalysisOpts()` / a direct `loadConfig()` call
* inside the `withRepo()` callback (to read analysis-tuning fields like
* `analysis.fnImpactDepth`/`analysis.briefCallerDepth`).
*
* Beyond the redundant work, the second call resolved its rootDir from
* `process.cwd()` instead of the resolved `--db` path, so a `--db` pointing
* at a different repo than cwd would read the WRONG project's
* `.codegraphrc.json` for those fields (mirrors the `withReadonlyDb` fix
* already applied for `exports.ts`/`context.ts`).
*
* The fix: `withRepo()` resolves config once and threads it through to both
* `openRepo()` (via the new `opts.config`) and the callback, so callers can
* pass `opts.config ?? dbConfig` into `resolveAnalysisOpts` instead of
* triggering a second `loadConfig()`.
*/
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import Database from 'better-sqlite3';
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';

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

// Delegate to the real loadConfig by default; this only counts invocations.
vi.mock('../../src/infrastructure/config.js', async (importOriginal) => {
const mod = await importOriginal<typeof import('../../src/infrastructure/config.js')>();
loadConfigSpy.mockImplementation(mod.loadConfig);
return { ...mod, loadConfig: loadConfigSpy };
});

import { initSchema } from '../../src/db/index.js';
import { briefData } from '../../src/domain/analysis/brief.js';
import { fnImpactData } from '../../src/domain/analysis/fn-impact.js';
import { withRepo } from '../../src/domain/analysis/query-helpers.js';

const CUSTOM_FN_IMPACT_DEPTH = 2;
const CUSTOM_BRIEF_CALLER_DEPTH = 2;

function insertNode(db: Database.Database, name: string, kind: string, file: string, line: number) {
return db
.prepare('INSERT INTO nodes (name, kind, file, line) VALUES (?, ?, ?, ?)')
.run(name, kind, file, line).lastInsertRowid;
}

function insertEdge(
db: Database.Database,
sourceId: number | bigint,
targetId: number | bigint,
kind: string,
) {
db.prepare(
'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, 1.0, 0)',
).run(sourceId, targetId, kind);
}

let tmpDir: string;
let cwdDir: string;
let dbPath: string;

beforeAll(() => {
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-withrepo-config-'));
fs.mkdirSync(path.join(tmpDir, '.git'));
fs.mkdirSync(path.join(tmpDir, '.codegraph'));
dbPath = path.join(tmpDir, '.codegraph', 'graph.db');

// Project config lives next to the DB, not at cwd — proves rootDir is
// derived from `--db`, matching resolveConfigForDbPath()'s contract.
fs.writeFileSync(
path.join(tmpDir, '.codegraphrc.json'),
JSON.stringify({
analysis: {
fnImpactDepth: CUSTOM_FN_IMPACT_DEPTH,
briefCallerDepth: CUSTOM_BRIEF_CALLER_DEPTH,
},
}),
);

const db = new Database(dbPath);
db.pragma('journal_mode = WAL');
initSchema(db);

const fBase = insertNode(db, 'lib/base.js', 'file', 'lib/base.js', 0);
const fCaller = insertNode(db, 'lib/caller.js', 'file', 'lib/caller.js', 0);
insertEdge(db, fCaller, fBase, 'imports');

const target = insertNode(db, 'target', 'function', 'lib/base.js', 5);
const caller = insertNode(db, 'caller', 'function', 'lib/caller.js', 5);
insertEdge(db, caller, target, 'calls');

db.close();

// cwd is a *different*, config-less directory — any loadConfig() call that
// resolves from process.cwd() (instead of the --db path) would silently
// fall back to DEFAULTS instead of picking up the custom depth above.
cwdDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-withrepo-cwd-'));
});

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

beforeEach(() => {
loadConfigSpy.mockClear();
});

describe('withRepo config resolution', () => {
it('resolves config exactly once and passes it to the callback', () => {
const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig);
expect(loadConfigSpy).toHaveBeenCalledTimes(1);
expect(config.analysis?.fnImpactDepth).toBe(CUSTOM_FN_IMPACT_DEPTH);
});
});

describe('fnImpactData (issue #1941)', () => {
it('calls loadConfig exactly once per invocation', () => {
fnImpactData('target', dbPath);
expect(loadConfigSpy).toHaveBeenCalledTimes(1);
});

it('reads analysis.fnImpactDepth from the --db path rootDir, not process.cwd()', () => {
const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(cwdDir);
try {
// depth defaults to config.analysis.fnImpactDepth when opts.depth is unset;
// the BFS at depth 1 already reaches `caller`, so this only distinguishes
// "config resolved" from "config silently defaulted" via the call count above,
// and directly via the resolved config object here.
const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig);
expect(config.analysis?.fnImpactDepth).toBe(CUSTOM_FN_IMPACT_DEPTH);

const data = fnImpactData('target', dbPath) as { results: Array<{ direct: number }> };
expect(data.results).toHaveLength(1);
expect(data.results[0]?.direct).toBe(1);
} finally {
cwdSpy.mockRestore();
}
});

it('opts.config still overrides the resolved db config when the caller supplies one', () => {
fnImpactData('target', dbPath, { config: { analysis: { fnImpactDepth: 99 } } as never });
// The explicit opts.config short-circuits resolveAnalysisOpts's fallback,
// but withRepo's own loadConfig() (for openRepo's engine selection) still runs once.
expect(loadConfigSpy).toHaveBeenCalledTimes(1);
});
});

describe('briefData (issue #1941)', () => {
it('calls loadConfig exactly once per invocation', () => {
briefData('base.js', dbPath);
expect(loadConfigSpy).toHaveBeenCalledTimes(1);
});

it('reads analysis.briefCallerDepth from the --db path rootDir, not process.cwd()', () => {
const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(cwdDir);
try {
const config = withRepo(dbPath, (_repo, dbConfig) => dbConfig);
expect(config.analysis?.briefCallerDepth).toBe(CUSTOM_BRIEF_CALLER_DEPTH);
} finally {
cwdSpy.mockRestore();
}
});
});
Loading