Skip to content

refactor: deduplicate BFS impact traversal and centralize config loading #463

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.

Sign up for GitHub

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

Merged
carlos-alm merged 11 commits into from
Mar 16, 2026
Merged

refactor: deduplicate BFS impact traversal and centralize config loading #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d1dd922
refactor: deduplicate BFS impact traversal and centralize config loading
carlos-alm Mar 16, 2026
2a51c29
fix: filter minConfidence for Repository path and simplify alias
carlos-alm Mar 16, 2026
6215e46
Merge branch 'main' into refactor/deduplicate-bfs-and-config
carlos-alm Mar 16, 2026
dd5e2e5
Merge branch 'main' into refactor/deduplicate-bfs-and-config
carlos-alm Mar 16, 2026
4fe685f
test: add InMemoryRepository tests for dependency builder and documen…
carlos-alm Mar 16, 2026
cc8bc26
Merge branch 'main' into refactor/deduplicate-bfs-and-config
carlos-alm Mar 16, 2026
404c2e9
docs: tighten bfsTransitiveCallers db param type to better-sqlite3 Da…
carlos-alm Mar 16, 2026
f24b29b
Merge branch 'main' into refactor/deduplicate-bfs-and-config
carlos-alm Mar 16, 2026
5e5f356
docs: document opts.config cwd vs db-root trade-off in checkData
carlos-alm Mar 16, 2026
05b0ba4
merge: resolve conflict with origin/main in dependency builder
carlos-alm Mar 16, 2026
d6b80f2
Merge branch 'main' into refactor/deduplicate-bfs-and-config
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
1 change: 1 addition & 0 deletions src/cli/commands/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const command = {
kind: opts.kind,
noTests: ctx.resolveNoTests(opts),
json: opts.json,
config: ctx.config,
});
},
};
3 changes: 3 additions & 0 deletions src/cli/commands/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const command = {
limit: opts.limit ? parseInt(opts.limit, 10) : undefined,
offset: opts.offset ? parseInt(opts.offset, 10) : undefined,
ndjson: opts.ndjson,
config: ctx.config,
});
return;
}
Expand All @@ -56,6 +57,7 @@ export const command = {
depth: opts.depth ? parseInt(opts.depth, 10) : undefined,
noTests: ctx.resolveNoTests(opts),
json: opts.json,
config: ctx.config,
});

if (opts.rules) {
Expand All @@ -73,6 +75,7 @@ export const command = {
limit: opts.limit ? parseInt(opts.limit, 10) : undefined,
offset: opts.offset ? parseInt(opts.offset, 10) : undefined,
ndjson: opts.ndjson,
config: ctx.config,
});
}
},
Expand Down
1 change: 1 addition & 0 deletions src/cli/commands/complexity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export const command = {
noTests: ctx.resolveNoTests(opts),
json: opts.json,
ndjson: opts.ndjson,
config: ctx.config,
});
},
};
1 change: 1 addition & 0 deletions src/cli/commands/diff-impact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const command = {
limit: opts.limit ? parseInt(opts.limit, 10) : undefined,
offset: opts.offset ? parseInt(opts.offset, 10) : undefined,
ndjson: opts.ndjson,
config: ctx.config,
});
},
};
2 changes: 1 addition & 1 deletion src/db/repository/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export class Repository {
throw new Error('not implemented');
}

/** @returns {{ source_id: number, target_id: number }[]} */
/** @returns {{ source_id: number, target_id: number, confidence: number|null }[]} */
getCallEdges() {
throw new Error('not implemented');
}
Expand Down
4 changes: 2 additions & 2 deletions src/db/repository/graph-read.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ export function getCallableNodes(db) {
/**
* Get all 'calls' edges.
* @param {object} db
* @returns {{ source_id: number, target_id: number }[]}
* @returns {{ source_id: number, target_id: number, confidence: number|null }[]}
*/
export function getCallEdges(db) {
return cachedStmt(
_getCallEdgesStmt,
db,
"SELECT source_id, target_id FROM edges WHERE kind = 'calls'",
"SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls'",
).all();
}

Expand Down
2 changes: 1 addition & 1 deletion src/db/repository/in-memory-repository.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ export class InMemoryRepository extends Repository {
getCallEdges() {
return [...this.#edges.values()]
.filter((e) => e.kind === 'calls')
.map((e) => ({ source_id: e.source_id, target_id: e.target_id }));
.map((e) => ({ source_id: e.source_id, target_id: e.target_id, confidence: e.confidence }));
}

getFileNodesAll() {
Expand Down
103 changes: 53 additions & 50 deletions src/domain/analysis/impact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,42 @@ import { normalizeSymbol } from '../../shared/normalize.js';
import { paginateResult } from '../../shared/paginate.js';
import { findMatchingNodes } from './symbol-lookup.js';

// ─── Shared BFS: transitive callers ────────────────────────────────────

/**
* BFS traversal to find transitive callers of a node.
*
* @param {import('better-sqlite3').Database} db - Open read-only SQLite database handle (not a Repository)
* @param {number} startId - Starting node ID
* @param {{ noTests?: boolean, maxDepth?: number, onVisit?: (caller: object, parentId: number, depth: number) => void }} options
* @returns {{ totalDependents: number, levels: Record<number, Array<{name:string, kind:string, file:string, line:number}>> }}
*/
Comment on lines +23 to +30
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

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

JSDoc db type overly broad for an exported function

bfsTransitiveCallers is now part of the public API (export function). The JSDoc describes db only as {object} - Open SQLite database handle, but the function's body calls findDistinctCallers(db, fid) from ../../db/index.js, which relies on cachedStmt (a WeakMap-backed prepared-statement cache) and therefore requires a raw better-sqlite3 handle — a Repository instance would fail at runtime here.

Tightening the type annotation and adding a note prevents future callers from accidentally passing a Repository:

Suggested change
/**
* BFS traversal to find transitive callers of a node.
*
* @param {object} db - Open SQLite database handle
* @param {number} startId - Starting node ID
* @param {{ noTests?: boolean, maxDepth?: number, onVisit?: (caller: object, parentId: number, depth: number) => void }} options
* @returns {{ totalDependents: number, levels: Record<number, Array<{name:string, kind:string, file:string, line:number}>> }}
*/
* @param {import('better-sqlite3').Database} db - Open read-only SQLite database handle (not a Repository)

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in 404c2e9 — tightened the JSDoc type from generic object to import(better-sqlite3).Database with a note that Repository instances are not accepted.

export function bfsTransitiveCallers(db, startId, { noTests = false, maxDepth = 3, onVisit } = {}) {
const visited = new Set([startId]);
const levels = {};
let frontier = [startId];

for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const fid of frontier) {
const callers = findDistinctCallers(db, fid);
for (const c of callers) {
if (!visited.has(c.id) && (!noTests || !isTestFile(c.file))) {
visited.add(c.id);
nextFrontier.push(c.id);
if (!levels[d]) levels[d] = [];
levels[d].push({ name: c.name, kind: c.kind, file: c.file, line: c.line });
if (onVisit) onVisit(c, fid, d);
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}

return { totalDependents: visited.size - 1, levels };
}

export function impactAnalysisData(file, customDbPath, opts = {}) {
const db = openReadonlyOrFail(customDbPath);
try {
Expand Down Expand Up @@ -82,31 +118,11 @@ export function fnImpactData(name, customDbPath, opts = {}) {
}

const results = nodes.map((node) => {
const visited = new Set([node.id]);
const levels = {};
let frontier = [node.id];

for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const fid of frontier) {
const callers = findDistinctCallers(db, fid);
for (const c of callers) {
if (!visited.has(c.id) && (!noTests || !isTestFile(c.file))) {
visited.add(c.id);
nextFrontier.push(c.id);
if (!levels[d]) levels[d] = [];
levels[d].push({ name: c.name, kind: c.kind, file: c.file, line: c.line });
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}

const { levels, totalDependents } = bfsTransitiveCallers(db, node.id, { noTests, maxDepth });
return {
...normalizeSymbol(node, db, hc),
levels,
totalDependents: visited.size - 1,
totalDependents,
};
});

Expand Down Expand Up @@ -232,40 +248,27 @@ export function diffImpactData(customDbPath, opts = {}) {

const allAffected = new Set();
const functionResults = affectedFunctions.map((fn) => {
const visited = new Set([fn.id]);
let frontier = [fn.id];
let totalCallers = 0;
const levels = {};
const edges = [];
const idToKey = new Map();
idToKey.set(fn.id, `${fn.file}::${fn.name}:${fn.line}`);
for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const fid of frontier) {
const callers = findDistinctCallers(db, fid);
for (const c of callers) {
if (!visited.has(c.id) && (!noTests || !isTestFile(c.file))) {
visited.add(c.id);
nextFrontier.push(c.id);
allAffected.add(`${c.file}:${c.name}`);
const callerKey = `${c.file}::${c.name}:${c.line}`;
idToKey.set(c.id, callerKey);
if (!levels[d]) levels[d] = [];
levels[d].push({ name: c.name, kind: c.kind, file: c.file, line: c.line });
edges.push({ from: idToKey.get(fid), to: callerKey });
totalCallers++;
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}

const { levels, totalDependents } = bfsTransitiveCallers(db, fn.id, {
noTests,
maxDepth,
onVisit(c, parentId) {
allAffected.add(`${c.file}:${c.name}`);
const callerKey = `${c.file}::${c.name}:${c.line}`;
idToKey.set(c.id, callerKey);
edges.push({ from: idToKey.get(parentId), to: callerKey });
},
});

return {
name: fn.name,
kind: fn.kind,
file: fn.file,
line: fn.line,
transitiveCallers: totalCallers,
transitiveCallers: totalDependents,
levels,
edges,
};
Expand Down Expand Up @@ -310,8 +313,8 @@ export function diffImpactData(customDbPath, opts = {}) {
let boundaryViolations = [];
let boundaryViolationCount = 0;
try {
const config = loadConfig(repoRoot);
const boundaryConfig = config.manifesto?.boundaries;
const cfg = opts.config || loadConfig(repoRoot);
const boundaryConfig = cfg.manifesto?.boundaries;
if (boundaryConfig) {
const result = evaluateBoundaries(db, boundaryConfig, {
scopeFiles: [...changedRanges.keys()],
Expand Down
52 changes: 12 additions & 40 deletions src/features/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import path from 'node:path';
import { openReadonlyOrFail } from '../db/index.js';
import { bfsTransitiveCallers } from '../domain/analysis/impact.js';
import { explainData } from '../domain/queries.js';
import { loadConfig } from '../infrastructure/config.js';
import { isTestFile } from '../infrastructure/test-filter.js';
Expand All @@ -17,11 +18,15 @@ import { RULE_DEFS } from './manifesto.js';

const FUNCTION_RULES = RULE_DEFS.filter((d) => d.level === 'function');

function resolveThresholds(customDbPath) {
function resolveThresholds(customDbPath, config) {
try {
const dbDir = path.dirname(customDbPath);
const repoRoot = path.resolve(dbDir, '..');
const cfg = loadConfig(repoRoot);
const cfg =
config ||
(() => {
const dbDir = path.dirname(customDbPath);
const repoRoot = path.resolve(dbDir, '..');
return loadConfig(repoRoot);
})();
const userRules = cfg.manifesto || {};
const resolved = {};
for (const def of FUNCTION_RULES) {
Expand Down Expand Up @@ -70,39 +75,6 @@ function checkBreaches(row, thresholds) {
return breaches;
}

// ─── BFS impact (inline, same algorithm as fnImpactData) ────────────

function computeImpact(db, nodeId, noTests, maxDepth) {
const visited = new Set([nodeId]);
const levels = {};
let frontier = [nodeId];

for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const fid of frontier) {
const callers = db
.prepare(
`SELECT DISTINCT n.id, n.name, n.kind, n.file, n.line
FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'calls'`,
)
.all(fid);
for (const c of callers) {
if (!visited.has(c.id) && (!noTests || !isTestFile(c.file))) {
visited.add(c.id);
nextFrontier.push(c.id);
if (!levels[d]) levels[d] = [];
levels[d].push({ name: c.name, kind: c.kind, file: c.file, line: c.line });
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}

return { totalDependents: visited.size - 1, levels };
}

// ─── Phase 4.4 fields (graceful null fallback) ─────────────────────

function readPhase44(db, nodeId) {
Expand Down Expand Up @@ -147,7 +119,7 @@ export function auditData(target, customDbPath, opts = {}) {

// 2. Open DB for enrichment
const db = openReadonlyOrFail(customDbPath);
const thresholds = resolveThresholds(customDbPath);
const thresholds = resolveThresholds(customDbPath, opts.config);

let functions;
try {
Expand Down Expand Up @@ -189,7 +161,7 @@ function enrichFunction(db, r, noTests, maxDepth, thresholds) {
const nodeId = nodeRow?.id;
const health = nodeId ? buildHealth(db, nodeId, thresholds) : defaultHealth();
const impact = nodeId
? computeImpact(db, nodeId, noTests, maxDepth)
? bfsTransitiveCallers(db, nodeId, { noTests, maxDepth })
: { totalDependents: 0, levels: {} };
const phase44 = nodeId
? readPhase44(db, nodeId)
Expand Down Expand Up @@ -260,7 +232,7 @@ function enrichSymbol(db, sym, file, noTests, maxDepth, thresholds) {

const health = nodeId ? buildHealth(db, nodeId, thresholds) : defaultHealth();
const impact = nodeId
? computeImpact(db, nodeId, noTests, maxDepth)
? bfsTransitiveCallers(db, nodeId, { noTests, maxDepth })
: { totalDependents: 0, levels: {} };
const phase44 = nodeId
? readPhase44(db, nodeId)
Expand Down
35 changes: 9 additions & 26 deletions src/features/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { execFileSync } from 'node:child_process';
import fs from 'node:fs';
import path from 'node:path';
import { findDbPath, openReadonlyOrFail } from '../db/index.js';
import { bfsTransitiveCallers } from '../domain/analysis/impact.js';
import { findCycles } from '../domain/graph/cycles.js';
import { loadConfig } from '../infrastructure/config.js';
import { isTestFile } from '../infrastructure/test-filter.js';
Expand Down Expand Up @@ -96,31 +97,10 @@ export function checkMaxBlastRadius(db, changedRanges, threshold, noTests, maxDe
}
if (!overlaps) continue;

// BFS transitive callers
const visited = new Set([def.id]);
let frontier = [def.id];
let totalCallers = 0;
for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const fid of frontier) {
const callers = db
.prepare(
`SELECT DISTINCT n.id, n.name, n.kind, n.file, n.line
FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'calls'`,
)
.all(fid);
for (const c of callers) {
if (!visited.has(c.id) && (!noTests || !isTestFile(c.file))) {
visited.add(c.id);
nextFrontier.push(c.id);
totalCallers++;
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}
const { totalDependents: totalCallers } = bfsTransitiveCallers(db, def.id, {
noTests,
maxDepth,
});

if (totalCallers > maxFound) maxFound = totalCallers;
if (totalCallers > threshold) {
Expand Down Expand Up @@ -240,7 +220,10 @@ export function checkData(customDbPath, opts = {}) {
const maxDepth = opts.depth || 3;

// Load config defaults for check predicates
const config = loadConfig(repoRoot);
// NOTE: opts.config is loaded from process.cwd() at startup (via CLI context),
// which may differ from the DB's parent repo root when --db points to an external
// project. This is an acceptable trade-off to avoid duplicate I/O on the hot path.
const config = opts.config || loadConfig(repoRoot);
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

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

opts.config origin may differ from old loadConfig(repoRoot) fallback

Previously checkData always loaded the config relative to the database file's parent directory:

// old
const config = loadConfig(repoRoot); // repoRoot = dirname(dbPath)/..

Now the CLI path passes ctx.config, which is loadConfig(process.cwd()) — loaded once at startup from the shell's working directory. These are equivalent when the user invokes codegraph from the repo root, but will silently diverge if --db points to a database outside the current working tree (e.g. codegraph check --db /other/project/.codegraph/graph.db). In that scenario the check predicates will read thresholds from the caller's project config instead of the target project's config.

The same observation applies to diffImpactData in src/domain/analysis/impact.js. This is an acceptable trade-off for the hot path (avoids duplicate I/O), but it's worth documenting with a comment so future maintainers understand why the two sources can differ.

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026

Choose a reason for hiding this comment

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

Added a comment documenting the trade-off — opts.config is loaded from process.cwd() at startup, which may differ from the DB's parent repo root when --db points externally. Accepted trade-off to avoid duplicate I/O.

const checkConfig = config.check || {};

// Resolve which predicates are enabled: CLI flags ?? config ?? built-in defaults
Expand Down
2 changes: 1 addition & 1 deletion src/features/complexity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ export function complexityData(customDbPath, opts = {}) {
const kindFilter = opts.kind || null;

// Load thresholds from config
const config = loadConfig(process.cwd());
const config = opts.config || loadConfig(process.cwd());
const thresholds = config.manifesto?.rules || {
cognitive: { warn: 15, fail: null },
cyclomatic: { warn: 10, fail: null },
Expand Down
2 changes: 1 addition & 1 deletion src/features/manifesto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ export function manifestoData(customDbPath, opts = {}) {
const db = openReadonlyOrFail(customDbPath);

try {
const config = loadConfig(process.cwd());
const config = opts.config || loadConfig(process.cwd());
const rules = resolveRules(config.manifesto?.rules);

const violations = [];
Expand Down
Loading
Loading