From 0a07066e28f81cd8f1b2c6654db8a66e95c0fc0a Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Wed, 8 Jul 2026 07:45:25 -0600 Subject: [PATCH] fix(scripts): resolve benchmark hub targets to a single, kind-filtered node query-benchmark.ts and benchmark.ts each selected "hub"/"mid"/"leaf" call-graph targets by name via a raw SQL query with no `kind` filter and no deterministic tie-break. A hub-candidate name like `buildGraph` could match a local `const { buildGraph } = await import(...)` binding (kind=constant) as easily as the real function definition, and which node won depended on unspecified SQLite row order. benchDiffImpact then ran a second, independently unfiltered query to resolve the hub's file, which could disagree with the first resolution and mutate/diff-impact the wrong file. Extract the shared selection logic into scripts/lib/hub-selection.ts: filter candidates to function/method kinds, add an explicit id-based ORDER BY tie-break, and return the resolved hub's file alongside its name so benchDiffImpact reuses that exact node instead of re-querying. Fixes #1904 --- scripts/benchmark.ts | 22 +----- scripts/lib/hub-selection.ts | 116 +++++++++++++++++++++++++++++ scripts/query-benchmark.ts | 77 ++++--------------- tests/unit/hub-selection.test.ts | 122 +++++++++++++++++++++++++++++++ 4 files changed, 254 insertions(+), 83 deletions(-) create mode 100644 scripts/lib/hub-selection.ts create mode 100644 tests/unit/hub-selection.test.ts diff --git a/scripts/benchmark.ts b/scripts/benchmark.ts index 39e68f33..7a4bab50 100644 --- a/scripts/benchmark.ts +++ b/scripts/benchmark.ts @@ -14,10 +14,10 @@ import fs from 'node:fs'; import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { fileURLToPath } from 'node:url'; -import Database from 'better-sqlite3'; import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js'; import { round1, timeMedian, timeMedianWithValue } from './lib/bench-timing.js'; +import { selectHubTargets } from './lib/hub-selection.js'; // ── Parent process: fork one child per engine, assemble final output ───── if (!isWorker()) { @@ -98,24 +98,6 @@ const QUERY_WARMUP_RUNS = 3; const PROBE_FILE = path.join(root, 'src', 'domain', 'queries.ts'); const BENCH_EXCLUDE = [...resolveBenchmarkExcludes()]; -function selectTargets() { - const db = new Database(dbPath, { readonly: true }); - const rows = db - .prepare( - `SELECT n.name, COUNT(e.id) AS cnt - FROM nodes n - JOIN edges e ON e.source_id = n.id OR e.target_id = n.id - WHERE n.file NOT LIKE '%test%' AND n.file NOT LIKE '%spec%' - GROUP BY n.id - ORDER BY cnt DESC`, - ) - .all(); - db.close(); - - if (rows.length === 0) return { hub: 'buildGraph', leaf: 'median' }; - return { hub: rows[0].name, leaf: rows[rows.length - 1].name }; -} - // Redirect console.log to stderr so only JSON goes to stdout const origLog = console.log; console.log = (...args) => console.error(...args); @@ -191,7 +173,7 @@ try { // ── Query benchmarks ──────────────────────────────────────────────── console.error(` [${engine}] Benchmarking queries...`); -const targets = workerTargets() || selectTargets(); +const targets = workerTargets() || selectHubTargets(dbPath); console.error(` hub=${targets.hub}, leaf=${targets.leaf}`); async function benchQuery(fn, ...args) { diff --git a/scripts/lib/hub-selection.ts b/scripts/lib/hub-selection.ts new file mode 100644 index 00000000..01a03e66 --- /dev/null +++ b/scripts/lib/hub-selection.ts @@ -0,0 +1,116 @@ +/** + * Deterministic hub/mid/leaf target selection for benchmark scripts. + * + * `query-benchmark.ts` and `benchmark.ts` each pick representative call-graph + * nodes ("hub" = well-connected, "mid"/"leaf" = less connected) by `name` to + * drive their timed queries. Both independently duplicated a `selectTargets` + * query that filtered candidates by file path (`NOT LIKE '%test%'`/`'%spec%'`) + * but not by `kind` — so a hub-candidate name like `buildGraph` could resolve + * to a local `const { buildGraph } = await import(...)` binding (`kind = + * 'constant'`) instead of the real `function buildGraph` definition, and + * which node won depended on unspecified SQLite row order (#1904). + * + * This module is the single source of truth for that selection: it filters + * to `HUB_CANDIDATE_KINDS` (mirroring `CALLABLE_SYMBOL_KINDS` in + * `src/shared/kinds.ts`, #1888 — the same "same-name lookup with no other + * signal" hazard) and adds an explicit `ORDER BY id` tie-break so the choice + * is reproducible across builds that insert the same logical nodes in a + * different physical row order (e.g. worker-thread parse completion order). + * + * Callers that need to act on the resolved hub's file (e.g. `benchDiffImpact` + * writing a probe comment into it) should use the `hubFile` this returns + * instead of re-querying `nodes` by name — a second unfiltered query can + * disagree with this one about which physical node "the hub" is. + */ + +import Database from 'better-sqlite3'; + +// Symbol kinds that represent a real invocable definition. Local variable +// and constant bindings must never win hub selection just because they share +// a candidate's name — mirrors CALLABLE_SYMBOL_KINDS in src/shared/kinds.ts. +export const HUB_CANDIDATE_KINDS: readonly string[] = ['function', 'method']; + +export interface HubTargets { + hub: string; + hubFile: string; + mid: string; + leaf: string; +} + +interface NodeRow { + name: string; + file: string; +} + +interface RankedNodeRow extends NodeRow { + cnt: number; +} + +/** + * Selects stable, deterministic hub/mid/leaf targets from a freshly-built + * graph DB at `dbPath`. + * + * `pinnedCandidates` are tried in order (each filtered to + * `HUB_CANDIDATE_KINDS` and non-test files) before falling back to the + * most-connected qualifying node — pinning keeps the "hub" identity stable + * across versions where auto-selection would otherwise drift as files are + * added or removed. + * + * Throws if the graph has no qualifying nodes with edges at all (an empty or + * malformed build), rather than silently returning a name that resolves to + * nothing downstream. + */ +export function selectHubTargets(dbPath: string, pinnedCandidates: readonly string[] = []): HubTargets { + const db = new Database(dbPath, { readonly: true }); + try { + const kindPlaceholders = HUB_CANDIDATE_KINDS.map(() => '?').join(', '); + + let hub: string | null = null; + let hubFile: string | null = null; + for (const candidate of pinnedCandidates) { + const row = db + .prepare( + `SELECT n.name, n.file FROM nodes n + JOIN edges e ON e.source_id = n.id OR e.target_id = n.id + WHERE n.name = ? AND n.kind IN (${kindPlaceholders}) + AND n.file NOT LIKE '%test%' AND n.file NOT LIKE '%spec%' + ORDER BY n.id ASC + LIMIT 1`, + ) + .get(candidate, ...HUB_CANDIDATE_KINDS) as NodeRow | undefined; + if (row) { + hub = row.name; + hubFile = row.file; + break; + } + } + + const rows = db + .prepare( + `SELECT n.id, n.name, n.file, COUNT(e.id) AS cnt + FROM nodes n + JOIN edges e ON e.source_id = n.id OR e.target_id = n.id + WHERE n.kind IN (${kindPlaceholders}) + AND n.file NOT LIKE '%test%' AND n.file NOT LIKE '%spec%' + GROUP BY n.id + ORDER BY cnt DESC, n.id ASC`, + ) + .all(...HUB_CANDIDATE_KINDS) as RankedNodeRow[]; + + if (rows.length === 0) { + throw new Error('No nodes with edges found in graph'); + } + + if (!hub) { + hub = rows[0].name; + hubFile = rows[0].file; + } + + const mid = rows[Math.floor(rows.length / 2)].name; + const leaf = rows[rows.length - 1].name; + + return { hub, hubFile: hubFile as string, mid, leaf }; + } finally { + db.close(); + } +} diff --git a/scripts/query-benchmark.ts b/scripts/query-benchmark.ts index 1e9d7286..3fa39121 100644 --- a/scripts/query-benchmark.ts +++ b/scripts/query-benchmark.ts @@ -15,10 +15,10 @@ import fs from 'node:fs'; import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { fileURLToPath } from 'node:url'; -import Database from 'better-sqlite3'; import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js'; import { round1, timeMedian } from './lib/bench-timing.js'; +import { selectHubTargets, type HubTargets } from './lib/hub-selection.js'; // ── Parent process: fork one child per engine, assemble final output ───── if (!isWorker()) { @@ -123,52 +123,6 @@ const WARMUP_RUNS = 3; // meaningless when barrel/type files get added or removed. const PINNED_HUB_CANDIDATES = ['buildGraph', 'openDb', 'loadConfig']; -function selectTargets() { - const db = new Database(dbPath, { readonly: true }); - try { - - // Try pinned candidates first for a stable hub across versions - let hub = null; - for (const candidate of PINNED_HUB_CANDIDATES) { - const row = db - .prepare( - `SELECT n.name FROM nodes n - JOIN edges e ON e.source_id = n.id OR e.target_id = n.id - WHERE n.name = ? AND n.file NOT LIKE '%test%' AND n.file NOT LIKE '%spec%' - LIMIT 1`, - ) - .get(candidate); - if (row) { - hub = row.name; - break; - } - } - - const rows = db - .prepare( - `SELECT n.name, COUNT(e.id) AS cnt - FROM nodes n - JOIN edges e ON e.source_id = n.id OR e.target_id = n.id - WHERE n.file NOT LIKE '%test%' AND n.file NOT LIKE '%spec%' - GROUP BY n.id - ORDER BY cnt DESC`, - ) - .all(); - - if (rows.length === 0) throw new Error('No nodes with edges found in graph'); - - // Fall back to most-connected if no pinned candidate found - if (!hub) hub = rows[0].name; - - const mid = rows[Math.floor(rows.length / 2)].name; - const leaf = rows[rows.length - 1].name; - return { hub, mid, leaf }; - - } finally { - db.close(); - } -} - async function benchDepths(fn, name, depths) { const result = {}; for (const depth of depths) { @@ -197,21 +151,18 @@ function resolveDbFile(rootDir: string, dbFile: string): string | null { return null; } -async function benchDiffImpact(hubName) { - const db = new Database(dbPath, { readonly: true }); - const row = db - .prepare(`SELECT file FROM nodes WHERE name = ? LIMIT 1`) - .get(hubName); - db.close(); - - if (!row) return { latencyMs: 0, affectedFunctions: 0, affectedFiles: 0 }; - - // row.file is normally relative (e.g. 'src/domain/builder.ts'), but some - // environments store absolute-like paths without the leading '/'. Handle - // both cases so the benchmark works regardless of DB path format. - const hubFile = resolveDbFile(root, row.file); +async function benchDiffImpact(targets: HubTargets) { + // Reuse the exact physical node selectHubTargets already resolved for + // `targets.hub` instead of re-querying `nodes` by name — a second, + // independently unfiltered query can disagree with the first about which + // same-named node "the hub" is (#1904). + // + // targets.hubFile is normally relative (e.g. 'src/domain/builder.ts'), but + // some environments store absolute-like paths without the leading '/'. + // Handle both cases so the benchmark works regardless of DB path format. + const hubFile = resolveDbFile(root, targets.hubFile); if (!hubFile) { - console.error(`[benchDiffImpact] Cannot find hub file for row.file=${row.file}`); + console.error(`[benchDiffImpact] Cannot find hub file for hubFile=${targets.hubFile}`); return { latencyMs: 0, affectedFunctions: 0, affectedFiles: 0 }; } const original = fs.readFileSync(hubFile, 'utf8'); @@ -242,7 +193,7 @@ async function benchDiffImpact(hubName) { if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); await buildGraph(root, { engine, incremental: false, exclude: [...resolveBenchmarkExcludes()] }); -const targets = workerTargets() || selectTargets(); +const targets: HubTargets = workerTargets() || selectHubTargets(dbPath, PINNED_HUB_CANDIDATES); console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); const fnDeps = {}; @@ -256,7 +207,7 @@ fnImpact.depth1Ms = (await benchDepths(fnImpactData, targets.hub, [1])).depth1Ms fnImpact.depth3Ms = (await benchDepths(fnImpactData, targets.hub, [3])).depth3Ms; fnImpact.depth5Ms = (await benchDepths(fnImpactData, targets.hub, [5])).depth5Ms; -const diffImpact = await benchDiffImpact(targets.hub); +const diffImpact = await benchDiffImpact(targets); // Restore console.log for JSON output console.log = origLog; diff --git a/tests/unit/hub-selection.test.ts b/tests/unit/hub-selection.test.ts new file mode 100644 index 00000000..b03dbab6 --- /dev/null +++ b/tests/unit/hub-selection.test.ts @@ -0,0 +1,122 @@ +/** + * Unit tests for scripts/lib/hub-selection.ts + * + * Regression coverage for #1904: benchmark hub-selection queries picked + * non-deterministically among same-named nodes (e.g. a local + * `const { buildGraph } = await import(...)` binding vs. the real + * `function buildGraph` definition) because the underlying SQL had no + * `kind` filter and no explicit ORDER BY tie-break. + */ + +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 { selectHubTargets } from '../../scripts/lib/hub-selection.js'; +import { initSchema } from '../../src/db/index.js'; + +function insertNode(db, name, kind, file, line) { + return db + .prepare('INSERT INTO nodes (name, kind, file, line) VALUES (?, ?, ?, ?)') + .run(name, kind, file, line).lastInsertRowid; +} + +function insertEdge(db, sourceId, targetId, kind) { + db.prepare( + 'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, 1.0, 0)', + ).run(sourceId, targetId, kind); +} + +let tmpDir: string, dbPath: string; + +// Graph shape (mirrors the real #1904 scenario at smaller scale): +// +// constBuildGraph ('buildGraph', kind=constant, scripts/benchmark.ts) — 3 inbound edges +// realBuildGraph ('buildGraph', kind=function, src/domain/graph/builder.ts) — 1 inbound edge +// midHelper (kind=function, src/domain/mid.ts) — 1 inbound edge +// leafHelper (kind=method, src/domain/leaf.ts) — 1 inbound edge +// orchestrator (kind=function, src/cli.ts) — source of all 6 edges above +// +// constBuildGraph has more raw edges (3) than any single real function/method +// node — a query without a `kind` filter would be tempted to rank it highest +// (or, for the pinned-candidate lookup, return it at all just from a +// name match). A correct implementation must exclude it everywhere. +beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-hub-selection-')); + dbPath = path.join(tmpDir, 'graph.db'); + + const db = new Database(dbPath); + initSchema(db); + + const constBuildGraph = insertNode(db, 'buildGraph', 'constant', 'scripts/benchmark.ts', 20); + const realBuildGraph = insertNode( + db, + 'buildGraph', + 'function', + 'src/domain/graph/builder.ts', + 12, + ); + const midHelper = insertNode(db, 'midHelper', 'function', 'src/domain/mid.ts', 5); + const leafHelper = insertNode(db, 'leafHelper', 'method', 'src/domain/leaf.ts', 5); + const orchestrator = insertNode(db, 'orchestrator', 'function', 'src/cli.ts', 1); + + insertEdge(db, orchestrator, constBuildGraph, 'calls'); + insertEdge(db, orchestrator, constBuildGraph, 'calls'); + insertEdge(db, orchestrator, constBuildGraph, 'calls'); + insertEdge(db, orchestrator, realBuildGraph, 'calls'); + insertEdge(db, orchestrator, midHelper, 'calls'); + insertEdge(db, orchestrator, leafHelper, 'calls'); + + db.close(); +}); + +afterAll(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe('selectHubTargets', () => { + it('prefers a callable-kind pinned candidate over a same-named constant binding', () => { + const targets = selectHubTargets(dbPath, ['buildGraph']); + expect(targets.hub).toBe('buildGraph'); + expect(targets.hubFile).toBe('src/domain/graph/builder.ts'); + }); + + it('excludes a constant-kind node from the most-connected fallback even with more raw edges', () => { + // No pinned candidates supplied — falls back to the most-connected + // qualifying (function/method) node. constBuildGraph has 3 edges (more + // than any single function/method node) but must never win because it + // is kind=constant. + const targets = selectHubTargets(dbPath, []); + expect(targets.hub).toBe('orchestrator'); + expect(targets.hubFile).toBe('src/cli.ts'); + }); + + it('selects mid/leaf from the same kind-filtered, edge-ranked ordering', () => { + const targets = selectHubTargets(dbPath, []); + expect(targets.mid).toBe('midHelper'); + expect(targets.leaf).toBe('leafHelper'); + }); + + it('is deterministic across repeated calls against the same DB', () => { + const first = selectHubTargets(dbPath, ['buildGraph']); + const second = selectHubTargets(dbPath, ['buildGraph']); + expect(second).toEqual(first); + }); + + it('throws when the graph has no qualifying nodes with edges', () => { + const emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-hub-selection-empty-')); + const emptyDbPath = path.join(emptyDir, 'graph.db'); + const db = new Database(emptyDbPath); + initSchema(db); + db.close(); + + try { + expect(() => selectHubTargets(emptyDbPath, ['buildGraph'])).toThrow( + 'No nodes with edges found in graph', + ); + } finally { + fs.rmSync(emptyDir, { recursive: true, force: true }); + } + }); +});