From 2b6a00d2a80d4f03e3080dd55c4f3fb7b8a086e3 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 11:24:15 -0600 Subject: [PATCH 01/12] fix: scope codegraph batch complexity targets to file paths BATCH_COMMANDS entries with sig: 'dbOnly' (currently only complexity) always wrote their target into opts.target. complexityData treats opts.target as a symbol-name filter and opts.file as the file-path filter, so batch complexity never matched anything and the function list silently fell back to empty with a whole-repo summary regardless of which file was requested. Add an optional targetKey on BatchCommandEntry so each dbOnly command can declare which opts key its targets map to, and set it to 'file' for complexity. batchData and multiBatchData now route the target through the declared key instead of assuming opts.target. Fixes #1721 Impact: 4 functions changed, 6 affected --- src/features/batch.ts | 28 ++++++++++++++-- tests/integration/batch.test.ts | 57 ++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/features/batch.ts b/src/features/batch.ts index 12a6aefb0..d8f343e34 100644 --- a/src/features/batch.ts +++ b/src/features/batch.ts @@ -18,6 +18,15 @@ type BatchSig = 'name' | 'target' | 'file' | 'dbOnly'; interface BatchCommandEntry { fn: (...args: unknown[]) => unknown; sig: BatchSig; + /** + * For `sig: 'dbOnly'` commands only: the `opts` key each target should be + * written to before calling `fn(customDbPath, opts)`. Defaults to `'target'` + * (a symbol/name filter). Commands whose underlying query function expects a + * *file path* target (rather than a symbol name) — e.g. `complexityData`, + * which treats `opts.target` as a name filter and `opts.file` as the file + * scope — must override this so the batch target lands in the right filter. + */ + targetKey?: string; } export const BATCH_COMMANDS: Record = { @@ -31,9 +40,22 @@ export const BATCH_COMMANDS: Record = { exports: { fn: exportsData as (...args: unknown[]) => unknown, sig: 'file' }, flow: { fn: flowData as (...args: unknown[]) => unknown, sig: 'name' }, dataflow: { fn: dataflowData as (...args: unknown[]) => unknown, sig: 'name' }, - complexity: { fn: complexityData as (...args: unknown[]) => unknown, sig: 'dbOnly' }, + complexity: { + fn: complexityData as (...args: unknown[]) => unknown, + sig: 'dbOnly', + targetKey: 'file', + }, }; +/** Resolve the opts key a `dbOnly` command's target should be written to. */ +function dbOnlyTargetOpts( + entry: BatchCommandEntry, + opts: Record, + target: string, +): Record { + return { ...opts, [entry.targetKey ?? 'target']: target }; +} + interface BatchResultOk { target: string; ok: true; @@ -77,7 +99,7 @@ export function batchData( try { let data: unknown; if (entry.sig === 'dbOnly') { - data = entry.fn(customDbPath, { ...opts, target }); + data = entry.fn(customDbPath, dbOnlyTargetOpts(entry, opts, target)); } else { data = entry.fn(target, customDbPath, opts); } @@ -166,7 +188,7 @@ export function multiBatchData( try { let data: unknown; if (entry.sig === 'dbOnly') { - data = entry.fn(customDbPath, { ...merged, target }); + data = entry.fn(customDbPath, dbOnlyTargetOpts(entry, merged, target)); } else { data = entry.fn(target, customDbPath, merged); } diff --git a/tests/integration/batch.test.ts b/tests/integration/batch.test.ts index 0dc44e3b0..8d84ea344 100644 --- a/tests/integration/batch.test.ts +++ b/tests/integration/batch.test.ts @@ -39,6 +39,12 @@ function insertEdge(db, sourceId, targetId, kind = 'calls', confidence = 1.0) { ).run(sourceId, targetId, kind, confidence); } +function insertComplexity(db, nodeId, cognitive, cyclomatic, maxNesting = 0) { + db.prepare( + 'INSERT INTO function_complexity (node_id, cognitive, cyclomatic, max_nesting) VALUES (?, ?, ?, ?)', + ).run(nodeId, cognitive, cyclomatic, maxNesting); +} + // ─── Fixture DB ──────────────────────────────────────────────────────── let tmpDir: string, dbPath: string; @@ -71,6 +77,13 @@ beforeAll(() => { insertEdge(db, fnRoute, fnAuth); insertEdge(db, fnRoute, fnFormat); + // Complexity data, distinct per file, so batch complexity can be asserted + // to scope results to the requested file rather than the whole repo. + insertComplexity(db, fnAuth, 5, 2); + insertComplexity(db, fnValidate, 3, 1); + insertComplexity(db, fnRoute, 10, 4); + insertComplexity(db, fnFormat, 1, 1); + db.close(); }); @@ -197,12 +210,46 @@ describe('batchData — edge cases', () => { // ─── complexity (dbOnly sig) ───────────────────────────────────────── describe('batchData — complexity (dbOnly signature)', () => { - test('complexity command uses target as opts.target', () => { - const data = batchData('complexity', ['authenticate'], dbPath); - expect(data.total).toBe(1); - // complexityData returns functions array — it won't error for unknown targets + test('complexity command scopes each target to a file, not a symbol name (#1721)', () => { + // Regression test for #1721: batch previously forwarded the file-path + // target as opts.target (a symbol-name filter), so complexityData never + // matched anything and silently fell back to identical, unfiltered + // whole-repo results for every target. It must land in opts.file instead. + const data = batchData('complexity', ['src/auth.js', 'src/routes.js'], dbPath); + expect(data.total).toBe(2); + expect(data.succeeded).toBe(2); + + const auth = data.results.find((r) => r.target === 'src/auth.js'); + const routes = data.results.find((r) => r.target === 'src/routes.js'); + expect(auth.ok).toBe(true); + expect(routes.ok).toBe(true); + + // src/auth.js has authenticate (cognitive 5) + validateToken (cognitive 3) + const authNames = auth.data.functions.map((f) => f.name).sort(); + expect(authNames).toEqual(['authenticate', 'validateToken']); + + // src/routes.js has only handleRoute (cognitive 10) + const routeNames = routes.data.functions.map((f) => f.name); + expect(routeNames).toEqual(['handleRoute']); + + // The two targets must not collapse into identical, unfiltered results. + // (Note: `summary` is a whole-repo health metric independent of the file + // filter, by design of complexityData — see #1807 for a separate, + // pre-existing gap where it should but doesn't reflect the file scope. + // The bug this test guards against is that `functions` came back empty + // for every target because the file path was routed into a symbol-name + // filter instead of the file filter.) + expect(auth.data.functions).not.toEqual(routes.data.functions); + }); + + test('complexity command still accepts a bare symbol name via opts.target passthrough', () => { + // BATCH_COMMANDS commands other than complexity keep symbol-name targets; + // this guards that complexity's own --target-style filtering (via shared + // opts, not the batch target) continues to work for direct callers. + const data = batchData('complexity', ['src/auth.js'], dbPath, { target: 'valid' }); expect(data.results[0].ok).toBe(true); - expect(data.results[0].data).toHaveProperty('functions'); + const names = data.results[0].data.functions.map((f) => f.name); + expect(names).toEqual(['validateToken']); }); }); From b28051ab3b220a745a6bf7b012b68fbe948716be Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:06:28 -0600 Subject: [PATCH 02/12] fix: exclude parameters and interface/type members from dead-role classification codegraph roles --role dead flagged every function parameter as dead-leaf and every interface/type member as dead-unresolved, regardless of actual usage. Root cause: the role classifier's fan-in-based "no callers = dead" heuristic is meaningless for these two symbol categories, since neither can ever have inbound call edges by construction: - Parameters: `kind = 'parameter'` nodes were unconditionally force-assigned dead-leaf via a fast-path bypass in classifyNodeRolesFull/Incremental (TS) and do_classify_full/do_classify_incremental (native), before any fan-in was even computed. A parameter's liveness is a local dataflow question (is it referenced within its own function body), not a call-graph reachability question, so this produced ~90% noise in --role dead output. - Interface/type members: every language extractor qualifies interface/type members as `Owner.member` top-level definitions (mirroring class method qualification), and they never receive inbound call edges (they're consumed via type annotations, not calls). Lacking any recognition of this, they fell through the normal fan-in/fan-out path and landed in dead-unresolved. Fix: - Parameters are now fully excluded from role classification (role stays NULL), the same treatment already given to file/directory nodes. - Interface/type members are now recognized in the classifier by resolving the Owner.member prefix against same-file TYPE_DEF_KINDS declarations (interface/type/struct/enum/trait/record) and classified `leaf` unconditionally. Class methods use the identical Owner.member naming convention but are unaffected since `class` is not in TYPE_DEF_KINDS, so real dead methods/functions remain detected. Applied to both the TS/WASM classifier (graph/classifiers/roles.ts, features/structure.ts) and the native Rust classifier (graph/classifiers/roles.rs) per the dual-engine parity requirement; verified both engines produce identical results end-to-end against this repo's own graph. Fixes #1723 Impact: 6 functions changed, 5 affected --- .../src/graph/classifiers/roles.rs | 88 ++++++++++++-- src/features/structure.ts | 17 ++- src/graph/classifiers/roles.ts | 85 ++++++++++++- tests/graph/classifiers/roles.test.ts | 113 ++++++++++++++++++ tests/unit/roles.test.ts | 69 +++++++++++ 5 files changed, 356 insertions(+), 16 deletions(-) diff --git a/crates/codegraph-core/src/graph/classifiers/roles.rs b/crates/codegraph-core/src/graph/classifiers/roles.rs index c45184650..1b45f03bc 100644 --- a/crates/codegraph-core/src/graph/classifiers/roles.rs +++ b/crates/codegraph-core/src/graph/classifiers/roles.rs @@ -106,6 +106,55 @@ fn median(sorted: &[u32]) -> f64 { } } +/// Compute, per file, the set of symbol names that are `TYPE_DEF_KINDS`-kind +/// declarations (interface/type/struct/enum/trait/record). Mirrors JS +/// `computeTypeDefNamesByFile` in `graph/classifiers/roles.ts`. +fn compute_type_def_names_by_file( + rows: &[(i64, String, String, String, u32, u32)], +) -> HashMap> { + let mut by_file: HashMap> = HashMap::new(); + for (_id, name, kind, file, _fan_in, _fan_out) in rows { + if TYPE_DEF_KINDS.iter().any(|k| *k == kind.as_str()) { + by_file + .entry(file.clone()) + .or_default() + .insert(name.clone()); + } + } + by_file +} + +/// True when `name`/`kind` is a `method`/`property` member of an interface/type +/// declared in the same file — e.g. TS `interface Foo { bar: string }` extracts +/// `bar` as a top-level `method`-kind definition named `Foo.bar` (#1723). Every +/// language extractor qualifies interface/type members as `Owner.member` +/// (mirroring class method qualification), so the owner name is recovered from +/// the prefix before the first `.` and looked up against same-file +/// `TYPE_DEF_KINDS` declarations. Class methods use the identical `Owner.member` +/// convention but are unaffected here because `class` is not in `TYPE_DEF_KINDS` +/// — they remain subject to normal dead-code detection. +/// +/// These members can never gain inbound call edges by construction, so a +/// `fan_in == 0` reading carries zero dead-code signal for them, unlike a real +/// function/method where it does. Mirrors JS `isTypeDeclarationMember`. +fn is_type_declaration_member( + name: &str, + kind: &str, + file: &str, + type_def_names_by_file: &HashMap>, +) -> bool { + if kind != "method" && kind != "property" { + return false; + } + let Some(dot_idx) = name.find('.') else { + return false; + }; + let owner_name = &name[..dot_idx]; + type_def_names_by_file + .get(file) + .is_some_and(|names| names.contains(owner_name)) +} + /// Dead sub-role classification matching JS `classifyDeadSubRole`. fn classify_dead_sub_role(_name: &str, kind: &str, file: &str) -> &'static str { // Leaf kinds @@ -126,6 +175,7 @@ fn classify_dead_sub_role(_name: &str, kind: &str, file: &str) -> &'static str { } /// Classify a single node into a role. +#[allow(clippy::too_many_arguments)] fn classify_node( name: &str, kind: &str, @@ -135,9 +185,16 @@ fn classify_node( is_exported: bool, production_fan_in: u32, has_active_file_siblings: bool, + is_type_member: bool, median_fan_in: f64, median_fan_out: f64, ) -> &'static str { + // Interface/type members (#1723) — never subject to call-graph dead-code + // detection, regardless of fan-in/fan-out/export status. + if is_type_member { + return "leaf"; + } + // Framework entry if FRAMEWORK_ENTRY_PREFIXES.iter().any(|p| name.starts_with(p)) { return "entry"; @@ -267,10 +324,14 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result dead-leaf + // 1. Property kind (class/struct fields) -> dead-leaf. + // `parameter` is deliberately NOT included here (#1723): a parameter's liveness + // is a local dataflow question, not a call-graph reachability question, so + // "no incoming call edges" carries zero dead-code signal for it. Parameters are + // also excluded from the main rows query below, so they never receive a role + // at all — the same treatment as `file`/`directory` nodes. let leaf_ids: Vec = { - let mut stmt = - tx.prepare("SELECT id FROM nodes WHERE kind IN ('parameter', 'property')")?; + let mut stmt = tx.prepare("SELECT id FROM nodes WHERE kind = 'property'")?; let rows = stmt.query_map([], |row| row.get::<_, i64>(0))?; rows.filter_map(|r| r.ok()).collect() }; @@ -408,6 +469,11 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result> = HashMap::new(); @@ -423,6 +489,7 @@ pub(crate) fn do_classify_full(conn: &Connection) -> rusqlite::Result, prod_fan_in: &HashMap, active_files: &std::collections::HashSet, called_active_files: &std::collections::HashSet, + type_def_names_by_file: &HashMap>, median_fan_in: f64, median_fan_out: f64, ids_by_role: &mut HashMap<&'static str, Vec>, @@ -573,6 +642,7 @@ fn classify_rows( } else { false }; + let is_type_member = is_type_declaration_member(name, kind, file, type_def_names_by_file); let role = classify_node( name, kind, @@ -582,6 +652,7 @@ fn classify_rows( is_exported, prod_fi, has_active_siblings, + is_type_member, median_fan_in, median_fan_out, ); @@ -629,16 +700,15 @@ fn find_neighbour_files( } /// Query leaf kind node IDs and callable node rows for a set of files. +/// `parameter` is intentionally excluded from the leaf query (#1723) — see +/// `do_classify_full`'s leaf_ids comment for the rationale. fn query_nodes_for_files( tx: &rusqlite::Transaction, files: &[&str], ) -> rusqlite::Result<(Vec, Vec<(i64, String, String, String, u32, u32)>)> { let ph: String = files.iter().map(|_| "?").collect::>().join(","); - let leaf_sql = format!( - "SELECT id FROM nodes WHERE kind IN ('parameter', 'property') AND file IN ({})", - ph - ); + let leaf_sql = format!("SELECT id FROM nodes WHERE kind = 'property' AND file IN ({})", ph); let leaf_ids: Vec = { let mut stmt = tx.prepare(&leaf_sql)?; for (i, f) in files.iter().enumerate() { @@ -804,6 +874,9 @@ pub(crate) fn do_classify_incremental( let (active_files, called_active_files) = compute_active_files(&rows); + // Compute interface/type owner names per file (#1723) — see do_classify_full. + let type_def_names_by_file = compute_type_def_names_by_file(&rows); + let mut ids_by_role: HashMap<&str, Vec> = HashMap::new(); if !leaf_ids.is_empty() { @@ -818,6 +891,7 @@ pub(crate) fn do_classify_incremental( &prod_fan_in, &active_files, &called_active_files, + &type_def_names_by_file, median_fan_in, median_fan_out, &mut ids_by_role, diff --git a/src/features/structure.ts b/src/features/structure.ts index 74b88ef4a..85f3afff9 100644 --- a/src/features/structure.ts +++ b/src/features/structure.ts @@ -777,13 +777,20 @@ function writeMedianCache( } function classifyNodeRolesFull(db: BetterSqlite3Database, emptySummary: RoleSummary): RoleSummary { - // Leaf kinds (parameter, property) can never have callers/callees. + // Property kind (class/struct fields) can never have callers/callees. // Classify them directly as dead-leaf without the expensive fan-in/fan-out JOINs. + // + // `parameter` is deliberately NOT included here (#1723): a parameter's liveness + // is a local dataflow question (is it referenced within its own function body), + // not a call-graph reachability question, so "no incoming call edges" carries + // zero dead-code signal for it. Parameters are also excluded from the main + // query below, so they never receive a role at all — the same treatment as + // `file`/`directory` nodes. const leafRows = db .prepare( `SELECT n.id FROM nodes n - WHERE n.kind IN ('parameter', 'property')`, + WHERE n.kind = 'property'`, ) .all() as { id: number }[]; @@ -967,11 +974,13 @@ function classifyNodeRolesIncremental( globalMedians = computed; } - // 2a. Leaf kinds (parameter, property) in affected files — always dead-leaf + // 2a. Property kind (class/struct fields) in affected files — always dead-leaf. + // `parameter` is intentionally excluded (#1723) — see classifyNodeRolesFull for + // the rationale. Parameters stay unclassified (role = NULL) after the reset below. const leafRows = db .prepare( `SELECT n.id FROM nodes n - WHERE n.kind IN ('parameter', 'property') + WHERE n.kind = 'property' AND n.file IN (${placeholders})`, ) .all(...allAffectedFiles) as { id: number }[]; diff --git a/src/graph/classifiers/roles.ts b/src/graph/classifiers/roles.ts index 2b17c33ba..fe278ff31 100644 --- a/src/graph/classifiers/roles.ts +++ b/src/graph/classifiers/roles.ts @@ -4,10 +4,23 @@ * Roles: entry, core, utility, adapter, leaf, dead-*, test-only * * Dead sub-categories refine the coarse "dead" bucket: - * dead-leaf — parameters, properties, constants (leaf nodes by definition) + * dead-leaf — properties, constants (leaf nodes by definition) * dead-entry — framework dispatch: CLI commands, MCP tools, event handlers * dead-ffi — cross-language FFI boundaries (e.g. Rust napi-rs bindings) * dead-unresolved — genuinely unreferenced callables (the real dead code) + * + * `parameter`-kind nodes never reach this module in production — callers + * (`features/structure.ts`, native `graph/classifiers/roles.rs`) exclude them + * entirely, leaving `role` unset, the same treatment as `file`/`directory` + * nodes. A parameter's liveness is a local dataflow question (is it referenced + * within its own function body), not a call-graph reachability question, so + * "no incoming call edges" carries zero dead-code signal for it (#1723). + * + * `method`/`property`-kind members of an interface/type declaration (e.g. + * `interface Foo { bar: string }`) DO reach this module, but are recognized by + * `isTypeDeclarationMember` and classified `leaf` unconditionally — they can + * never gain inbound call edges by construction, so call-graph reachability + * doesn't apply to them either (#1723). */ import type { DeadSubRole, Role } from '../../types.js'; @@ -53,6 +66,56 @@ export interface ClassifiableNode { file?: string; } +/** + * Compute, per file, the set of symbol names that are `TYPE_DEF_KINDS`-kind + * declarations (interface/type/struct/enum/trait/record). Used by + * `isTypeDeclarationMember` to recognize `Owner.member`-qualified nodes whose + * owner is a type-level declaration rather than a class. + */ +function computeTypeDefNamesByFile(nodes: RoleClassificationNode[]): Map> { + const byFile = new Map>(); + for (const n of nodes) { + if (n.file && n.kind && TYPE_DEF_KINDS.has(n.kind)) { + let names = byFile.get(n.file); + if (!names) { + names = new Set(); + byFile.set(n.file, names); + } + names.add(n.name); + } + } + return byFile; +} + +/** + * True when `node` is a `method`/`property`-kind member of an interface/type + * declared in the same file — e.g. TS `interface Foo { bar: string }` extracts + * `bar` as a top-level `method`-kind definition named `Foo.bar` (#1723). Every + * language extractor qualifies interface/type members as `Owner.member` + * (mirroring class method qualification), so the owner name is recovered from + * the prefix before the first `.` and looked up against same-file + * `TYPE_DEF_KINDS` declarations. Class methods use the identical `Owner.member` + * convention but are unaffected here because `class` is not in `TYPE_DEF_KINDS` + * — they remain subject to normal dead-code detection. + * + * These members can never gain inbound call edges by construction — they are + * consumed via type annotations and structural typing, never calls — so a + * `fanIn === 0` reading carries zero dead-code signal for them, unlike a real + * function/method where it does. Call-edge-based reachability just doesn't + * apply to type-level declarations, so they must never be judged dead by it. + */ +function isTypeDeclarationMember( + node: RoleClassificationNode, + typeDefNamesByFile: Map>, +): boolean { + if (node.kind !== 'method' && node.kind !== 'property') return false; + if (!node.file) return false; + const dotIdx = node.name.indexOf('.'); + if (dotIdx === -1) return false; + const ownerName = node.name.slice(0, dotIdx); + return typeDefNamesByFile.get(node.file)?.has(ownerName) ?? false; +} + /** * Refine a "dead" classification into a sub-category. */ @@ -176,10 +239,21 @@ function classifyByFanShape(highIn: boolean, highOut: boolean): Role { /** * Apply role-classification rules to a single node. - * Order matters — framework entries are tagged first, then dead/test cases, - * then the fan-in/fan-out shape decides among the structural roles. + * Order matters — type-level members are ruled out first (they can never be + * judged by call-graph reachability at all), then framework entries, then + * dead/test cases, then the fan-in/fan-out shape decides among the structural + * roles. */ -function classifyNodeRole(node: RoleClassificationNode, medFanIn: number, medFanOut: number): Role { +function classifyNodeRole( + node: RoleClassificationNode, + medFanIn: number, + medFanOut: number, + typeDefNamesByFile: Map>, +): Role { + // Interface/type members (#1723) — never subject to call-graph dead-code + // detection, regardless of fan-in/fan-out/export status. + if (isTypeDeclarationMember(node, typeDefNamesByFile)) return 'leaf'; + if (FRAMEWORK_ENTRY_PREFIXES.some((p) => node.name.startsWith(p))) return 'entry'; if (node.fanIn === 0) { @@ -217,10 +291,11 @@ export function classifyRoles( if (nodes.length === 0) return new Map(); const { fanIn: medFanIn, fanOut: medFanOut } = medianOverrides ?? computeFanMedians(nodes); + const typeDefNamesByFile = computeTypeDefNamesByFile(nodes); const result = new Map(); for (const node of nodes) { - result.set(node.id, classifyNodeRole(node, medFanIn, medFanOut)); + result.set(node.id, classifyNodeRole(node, medFanIn, medFanOut, typeDefNamesByFile)); } return result; } diff --git a/tests/graph/classifiers/roles.test.ts b/tests/graph/classifiers/roles.test.ts index 03ae894c7..b1de630ee 100644 --- a/tests/graph/classifiers/roles.test.ts +++ b/tests/graph/classifiers/roles.test.ts @@ -515,4 +515,117 @@ describe('classifyRoles', () => { const roles = classifyRoles(nodes); expect(roles.get('1')).toBe('dead-unresolved'); }); + + // ── Interface/type member exemption (#1723) ───────────────────────── + + it('classifies interface method-signature member as leaf, not dead', () => { + // TS `interface Foo { bar(): void }` extracts `bar` as a top-level + // `method`-kind definition named `Foo.bar`. It can never gain an inbound + // call edge (nothing "calls" a type-level declaration), so fanIn === 0 + // carries zero dead-code signal here — unlike a real function/method. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'interface', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Foo.bar', + kind: 'method', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('classifies type-alias property-signature member as leaf, not dead', () => { + // TS `type Foo = { bar: string }` — a property-kind member of a `type` owner. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'type', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Foo.bar', + kind: 'property', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('classifies interface member as leaf even when the interface is isolated (no active siblings)', () => { + // Unlike bare TYPE_DEF_KINDS nodes (which need hasActiveFileSiblings to + // avoid being marked dead), members are exempt unconditionally — "no call + // edges" is a structural certainty for them, not merely a likelihood. + const nodes = [ + { + id: '1', + name: 'Isolated', + kind: 'interface', + file: 'src/isolated.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + { + id: '2', + name: 'Isolated.onlyMember', + kind: 'method', + file: 'src/isolated.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('leaf'); + }); + + it('does not exempt class methods sharing the Owner.member naming convention', () => { + // Class methods are qualified identically to interface members + // (`ClassName.method`), but `class` is not in TYPE_DEF_KINDS — they must + // remain subject to normal dead-code detection. + const nodes = [ + { + id: '1', + name: 'Foo', + kind: 'class', + file: 'src/a.ts', + fanIn: 5, + fanOut: 0, + isExported: true, + }, + { + id: '2', + name: 'Foo.deadMethod', + kind: 'method', + file: 'src/a.ts', + fanIn: 0, + fanOut: 0, + isExported: false, + }, + ]; + const roles = classifyRoles(nodes); + expect(roles.get('2')).toBe('dead-unresolved'); + }); }); diff --git a/tests/unit/roles.test.ts b/tests/unit/roles.test.ts index 110e1c7bd..88d5ee1e0 100644 --- a/tests/unit/roles.test.ts +++ b/tests/unit/roles.test.ts @@ -380,4 +380,73 @@ describe('classifyNodeRoles', () => { // Should be entry (exported, fan-in 0), not dead-unresolved expect(role.role).toBe('entry'); }); + + // ── Parameters and interface members are not dead-code targets (#1723) ── + + it('excludes parameter-kind nodes from role classification entirely', () => { + // A parameter's liveness is a local dataflow question (is it referenced + // within its own function body), not a call-graph reachability question. + // "No incoming call edges" is guaranteed for every parameter regardless of + // usage, so parameters must never receive a `dead-*` role — or any role at + // all, the same treatment as file/directory nodes. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + const fn = insertNode('findChild', 'function', 'src/helpers.ts', 26); + insertNode('node', 'parameter', 'src/helpers.ts', 26); + insertNode('type', 'parameter', 'src/helpers.ts', 26); + // An external caller so findChild itself is not incidentally dead too — + // isolates the assertion to parameter handling specifically. + const caller = insertNode('caller', 'function', 'other.ts', 1); + insertEdge(caller, fn, 'calls'); + + classifyNodeRoles(db); + + const paramRoles = db.prepare("SELECT role FROM nodes WHERE kind = 'parameter'").all() as { + role: string | null; + }[]; + expect(paramRoles).toHaveLength(2); + for (const row of paramRoles) { + expect(row.role).toBeNull(); + } + }); + + it('does not classify interface members as dead', () => { + // TS `interface ExtractParametersOptions { typeMap?: Map<...> }` extracts + // `typeMap` as a top-level `method`-kind definition named + // `ExtractParametersOptions.typeMap` (property-signature members are + // currently mislabeled `method` by the extractor — tracked separately). + // It has no callers by construction; it must not be flagged dead. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + insertNode('ExtractParametersOptions', 'interface', 'src/helpers.ts', 359); + insertNode('ExtractParametersOptions.typeMap', 'method', 'src/helpers.ts', 361); + + classifyNodeRoles(db); + + const role = db + .prepare("SELECT role FROM nodes WHERE name = 'ExtractParametersOptions.typeMap'") + .get() as { role: string | null }; + expect(role.role).toBe('leaf'); + }); + + it('regression: used parameters, interface members, and a genuinely dead function are classified correctly together', () => { + // Mirrors the exact issue #1723 repro: `findChild(node, type)` in + // src/extractors/helpers.ts, alongside an interface and a truly dead function. + insertNode('src/helpers.ts', 'file', 'src/helpers.ts', 0); + const findChildFn = insertNode('findChild', 'function', 'src/helpers.ts', 26); + insertNode('node', 'parameter', 'src/helpers.ts', 26); + insertNode('type', 'parameter', 'src/helpers.ts', 26); + insertNode('ChaContext', 'interface', 'src/helpers.ts', 18); + insertNode('ChaContext.implementors', 'method', 'src/helpers.ts', 20); + insertNode('trulyDeadHelper', 'function', 'src/helpers.ts', 400); + + const caller = insertNode('caller', 'function', 'other.ts', 1); + insertEdge(caller, findChildFn, 'calls'); + + classifyNodeRoles(db); + const getRole = (name) => db.prepare('SELECT role FROM nodes WHERE name = ?').get(name)?.role; + + expect(getRole('node')).toBeNull(); + expect(getRole('type')).toBeNull(); + expect(getRole('ChaContext.implementors')).toBe('leaf'); + expect(getRole('trulyDeadHelper')).toBe('dead-unresolved'); + }); }); From 17fbcba97423e96693bfc2b689b090abd4ea45ff Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:25:38 -0600 Subject: [PATCH 03/12] fix: credit import-type usages as exports consumers codegraph exports --json (and the --unused dead-export filter it feeds) reported zero consumers for interfaces/types that are demonstrably imported and used elsewhere via `import type { X }`, even though `codegraph deps ` correctly showed the importing file in `importedBy`. Example: ChaContext in src/domain/graph/builder/cha.ts is imported (type-only) by build-edges.ts and native-orchestrator.ts, but `codegraph exports` showed consumerCount: 0. Root cause: exportsFileImpl's per-symbol consumers query (domain/analysis/exports.ts) only looked at kind = 'calls' edges. The builder already emits a symbol-level `imports-type` edge for `import type { X }` statements (source = importing file node, target = the specific imported symbol -- see emitTypeOnlySymbolEdges in build-edges.ts/incremental.ts), which `codegraph deps` reads from, but the exports consumer query never looked at this edge kind. Role classification (features/structure.ts) already includes 'imports-type' in its fan-in formula, so `codegraph roles --role dead` was unaffected -- this was purely a gap in exports's independent consumer list. Fix: widen the consumers query to `kind IN ('calls', 'imports-type')`, matching the edge-kind set already used everywhere else in the codebase for cross-file usage credit (structure.ts, graph-enrichment.ts, boundaries.ts, dependencies.ts). No native Rust changes needed -- domain/analysis/exports.ts is pure query-layer code that reads the already-built edges table and has no engine-specific mirror. Deliberately does NOT add `extends`/`implements`: investigation found those edges are resolved by symbol name only, with no file/import scoping (buildClassHierarchyEdges in both build-edges.ts/incremental.ts and the native emit_hierarchy_edges), so they link same-named declarations across unrelated files (verified: this repo's own graph has false `implements`/`extends` edges between unrelated fixture classes across languages and a `Repository` interface in src/types.ts). Filed as #1812. Also filed #1813 for a related but distinct gap: `import { type X }` inline per-specifier modifiers aren't tracked as type-only in either engine's extractor, so such X still gets no credit even after this fix. Added tests/integration/exports.test.ts coverage: an interface consumed only via a symbol-level `imports-type` edge gets consumerCount >= 1 and is excluded from --unused, while a genuinely unreferenced interface still shows 0 consumers. Verified against this repo's own graph: `codegraph exports src/domain/graph/builder/cha.ts -T --json` now credits ChaContext with 2 consumers (build-edges.ts, native-orchestrator.ts). Full test suite (201 files, 3359 tests) and lint pass clean. docs check acknowledged: internal bug fix to exports consumer computation, no new feature/language/CLI surface/architecture change -- README.md, CLAUDE.md, and ROADMAP.md are unaffected. Fixes #1724 Impact: 1 functions changed, 3 affected --- src/domain/analysis/exports.ts | 15 ++++++- tests/integration/exports.test.ts | 73 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/domain/analysis/exports.ts b/src/domain/analysis/exports.ts index 62f97c434..3c9710df6 100644 --- a/src/domain/analysis/exports.ts +++ b/src/domain/analysis/exports.ts @@ -164,11 +164,24 @@ function exportsFileImpl( "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", ) : null; + // Consumers include real call/construct edges plus `imports-type` edges — + // the symbol-level edge emitted for `import type { X }` statements (source + // is the importing *file* node, since the import statement references the + // type rather than a specific function). Without this, interfaces/types + // that are only ever used as type annotations are misclassified as dead + // exports even though `codegraph deps` already reports the importing file + // via this same edge (#1724). + // + // `extends`/`implements` edges are deliberately NOT included here: they are + // resolved by symbol name only, with no file/import scoping (see + // buildClassHierarchyEdges), so they can link same-named declarations + // across unrelated files — crediting them as consumers would surface false + // positives instead of fixing false negatives. const consumersStmt = cachedStmt( _consumersStmtCache, db, `SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id - WHERE e.target_id = ? AND e.kind = 'calls'`, + WHERE e.target_id = ? AND e.kind IN ('calls', 'imports-type')`, ); const reexportsFromStmt = cachedStmt( _reexportsFromStmtCache, diff --git a/tests/integration/exports.test.ts b/tests/integration/exports.test.ts index 467621280..f869ec115 100644 --- a/tests/integration/exports.test.ts +++ b/tests/integration/exports.test.ts @@ -240,3 +240,76 @@ describe('exportsData', () => { expect(data.totalReexportedUnused).toBe(0); }); }); + +// ─── import type / type-only consumer crediting (#1724) ────────────────── +// +// Regression coverage for: interfaces/types that are only ever consumed via +// `import type { X }` (never called/constructed) were reported as zero- +// consumer dead exports, even though the builder already emits a +// symbol-level `imports-type` edge (source = importing file node, target = +// the specific imported symbol) for exactly this case. `codegraph deps` +// already surfaced the file-level import correctly; `exportsData`'s +// per-symbol consumer query only looked at `kind = 'calls'` and missed it. + +describe('exportsData — import type consumer crediting (#1724)', () => { + let tmpDir2: string, dbPath2: string; + + beforeAll(() => { + tmpDir2 = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-exports-typeonly-')); + fs.mkdirSync(path.join(tmpDir2, '.codegraph')); + dbPath2 = path.join(tmpDir2, '.codegraph', 'graph.db'); + + const db = new Database(dbPath2); + db.pragma('journal_mode = WAL'); + initSchema(db); + + // File nodes + insertNode(db, 'types.ts', 'file', 'types.ts', 0); + const fConsumer = insertNode(db, 'consumer.ts', 'file', 'consumer.ts', 0); + + // Interface exported from types.ts, referenced elsewhere only via a type + // annotation (never called/constructed). + const configIface = insertNode(db, 'Config', 'interface', 'types.ts', 1); + // Interface exported from types.ts, genuinely never referenced anywhere. + const unusedIface = insertNode(db, 'Unused', 'interface', 'types.ts', 10); + + const markExported = db.prepare('UPDATE nodes SET exported = 1 WHERE id = ?'); + markExported.run(configIface); + markExported.run(unusedIface); + + // consumer.ts does `import type { Config } from './types'`. The builder + // emits the symbol-level edge with the importing *file* as source (see + // emitTypeOnlySymbolEdges in domain/graph/builder/stages/build-edges.ts + // and incremental.ts) since the import statement — not a specific + // function — is what references the type. + insertEdge(db, fConsumer, configIface, 'imports-type'); + + db.close(); + }); + + afterAll(() => { + if (tmpDir2) fs.rmSync(tmpDir2, { recursive: true, force: true }); + }); + + test('interface consumed only via `import type` is credited with a consumer', () => { + const data = exportsData('types.ts', dbPath2); + const config = data.results.find((r) => r.name === 'Config'); + expect(config).toBeDefined(); + expect(config.consumerCount).toBe(1); + expect(config.consumers.length).toBe(1); + expect(config.consumers[0].file).toBe('consumer.ts'); + }); + + test('interface consumed only via `import type` is excluded from --unused', () => { + const data = exportsData('types.ts', dbPath2, { unused: true }); + expect(data.results.find((r) => r.name === 'Config')).toBeUndefined(); + }); + + test('interface with no references anywhere is still classified unused', () => { + const data = exportsData('types.ts', dbPath2, { unused: true }); + const unused = data.results.find((r) => r.name === 'Unused'); + expect(unused).toBeDefined(); + expect(unused.consumerCount).toBe(0); + expect(unused.consumers).toEqual([]); + }); +}); From 26918bb90ebad99480933cf54adeb5a1cf7e336c Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 13:56:17 -0600 Subject: [PATCH 04/12] fix: prevent fn-impact/query crash when -f/--file is passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI's `-f/--file` option is a repeatable Commander accumulator (collectFile) that always produces a string[], even on first use. The native composite bindings backing fn-impact (findNodesWithFanIn) and query (fnDeps) forwarded that array straight into napi-rs functions whose Rust signatures only accepted a single String, so any use of -f/--file crashed with "Failed to convert JavaScript value ... into rust type `String`" regardless of engine defaults. context worked only because its code path never touches the native repository for symbol lookup. Widen the native Rust signatures (find_nodes_with_fan_in, fn_deps) to accept Vec and build an OR-of-LIKE clause for multiple files, mirroring buildFileConditionSQL/NodeQuery.fileFilter on the JS side. Normalize the file option before calling into the native binding on the TS side, and thread the widened string | string[] type through QueryOpts, fnDeps's opts, and the call chain down to findMatchingNodes. This also makes repeated -f/--file genuinely multi-file end to end for fn-impact/query, matching context's existing behavior, instead of crashing on any use. findNodesByScope shares QueryOpts but has no CLI caller and its native binding still only accepts one file; it now takes the first value defensively rather than crash or fail to type-check. Fixes #1726 docs check acknowledged: pure bug fix, no new CLI options/languages/ architecture — the -f/--file "repeatable" docs are now accurate rather than needing correction. Impact: 14 functions changed, 34 affected --- .../src/db/repository/graph_read.rs | 47 +++++++++++-- src/db/repository/base.ts | 2 +- src/db/repository/native-repository.ts | 17 +++-- src/domain/analysis/dependencies.ts | 2 +- src/domain/analysis/fn-impact.ts | 2 +- src/domain/analysis/symbol-lookup.ts | 7 +- src/presentation/queries-cli/impact.ts | 2 +- src/types.ts | 9 +-- tests/integration/cli.test.ts | 66 +++++++++++++++++++ tests/integration/queries.test.ts | 47 +++++++++++++ 10 files changed, 179 insertions(+), 22 deletions(-) diff --git a/crates/codegraph-core/src/db/repository/graph_read.rs b/crates/codegraph-core/src/db/repository/graph_read.rs index b55bec051..5f36382e8 100644 --- a/crates/codegraph-core/src/db/repository/graph_read.rs +++ b/crates/codegraph-core/src/db/repository/graph_read.rs @@ -28,6 +28,41 @@ fn escape_like(s: &str) -> String { out } +/// Append an `AND (col LIKE ? [OR col LIKE ? ...])` fragment for a multi-value +/// file filter and push the corresponding escaped LIKE params. Mirrors +/// `buildFileConditionSQL()` / `NodeQuery.fileFilter()` in +/// `src/db/query-builder.ts`, which is why callers must accept `Vec` +/// (repeatable `-f/--file`) rather than a single `String`. No-op when `files` +/// is empty. Advances `*idx` past the placeholders it consumes. +fn push_file_filter( + sql: &mut String, + params_v: &mut Vec>, + idx: &mut usize, + column: &str, + files: &[String], +) { + if files.is_empty() { + return; + } + let start = *idx; + if files.len() == 1 { + sql.push_str(&format!(" AND {column} LIKE ?{start} ESCAPE '\\'")); + params_v.push(Box::new(format!("%{}%", escape_like(&files[0])))); + *idx += 1; + return; + } + let clauses: Vec = files + .iter() + .enumerate() + .map(|(i, _)| format!("{column} LIKE ?{} ESCAPE '\\'", start + i)) + .collect(); + sql.push_str(&format!(" AND ({})", clauses.join(" OR "))); + for f in files { + params_v.push(Box::new(format!("%{}%", escape_like(f)))); + } + *idx += files.len(); +} + /// Check if a file path looks like a test file (mirrors `isTestFile` in JS). fn is_test_file(file: &str) -> bool { file.contains(".test.") @@ -143,7 +178,7 @@ struct FnDepsCallerWithId { fn build_fn_deps_match_query( name: &str, kind: Option<&str>, - file: Option<&str>, + file: Option<&[String]>, ) -> (String, Vec>) { let default_kinds: Vec = vec![ "function".to_string(), @@ -180,8 +215,7 @@ fn build_fn_deps_match_query( idx += kinds.len(); } if let Some(f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - params_v.push(Box::new(format!("%{}%", escape_like(f)))); + push_file_filter(&mut sql, &mut params_v, &mut idx, "n.file", f); } (sql, params_v) @@ -1086,7 +1120,7 @@ impl NativeDatabase { &self, name_pattern: String, kinds: Option>, - file: Option, + file: Option>, ) -> napi::Result> { let conn = self.conn()?; @@ -1112,8 +1146,7 @@ impl NativeDatabase { } } if let Some(ref f) = file { - sql.push_str(&format!(" AND n.file LIKE ?{idx} ESCAPE '\\'")); - param_values.push(Box::new(format!("%{}%", escape_like(f)))); + push_file_filter(&mut sql, &mut param_values, &mut idx, "n.file", f); } let mut stmt = conn @@ -2114,7 +2147,7 @@ impl NativeDatabase { name: String, depth: Option, no_tests: Option, - file: Option, + file: Option>, kind: Option, ) -> napi::Result { let conn = self.conn()?; diff --git a/src/db/repository/base.ts b/src/db/repository/base.ts index 4c1546c2e..9ab137a86 100644 --- a/src/db/repository/base.ts +++ b/src/db/repository/base.ts @@ -231,7 +231,7 @@ export class Repository implements IRepository { */ fnDeps( _name: string, - _opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + _opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): FnDepsResult | null { return null; } diff --git a/src/db/repository/native-repository.ts b/src/db/repository/native-repository.ts index a822c6912..202b3bb74 100644 --- a/src/db/repository/native-repository.ts +++ b/src/db/repository/native-repository.ts @@ -45,6 +45,7 @@ import type { TriageNodeRow, TriageQueryOpts, } from '../../types.js'; +import { normalizeFileFilter } from '../query-builder.js'; import { type FnDepsCallerNode, type FnDepsEntry, @@ -267,8 +268,9 @@ export class NativeRepository extends Repository { } findNodesWithFanIn(namePattern: string, opts: QueryOpts = {}): NodeRowWithFanIn[] { + const files = normalizeFileFilter(opts.file); return this.#ndb - .findNodesWithFanIn(namePattern, opts.kinds ?? null, opts.file ?? null) + .findNodesWithFanIn(namePattern, opts.kinds ?? null, files.length > 0 ? files : null) .map(toNodeRowWithFanIn); } @@ -301,9 +303,11 @@ export class NativeRepository extends Repository { } findNodesByScope(scopeName: string, opts: QueryOpts = {}): NodeRow[] { - return this.#ndb - .findNodesByScope(scopeName, opts.kind ?? null, opts.file ?? null) - .map(toNodeRow); + // Unlike findNodesWithFanIn/fnDeps, this native binding only accepts a single + // file (no CLI command currently wires -f/--file to it); take the first value + // rather than crash if a caller ever passes the repeatable-flag array form. + const [file] = normalizeFileFilter(opts.file); + return this.#ndb.findNodesByScope(scopeName, opts.kind ?? null, file ?? null).map(toNodeRow); } findNodeByQualifiedName(qualifiedName: string, opts: { file?: string } = {}): NodeRow[] { @@ -526,14 +530,15 @@ export class NativeRepository extends Repository { // ── Composite queries ────────────────────────────────────────────── fnDeps( name: string, - opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): FnDepsResult | null { if (typeof this.#ndb.fnDeps !== 'function') return null; + const files = normalizeFileFilter(opts?.file); const raw = this.#ndb.fnDeps( name, opts?.depth ?? undefined, opts?.noTests ?? undefined, - opts?.file ?? undefined, + files.length > 0 ? files : undefined, opts?.kind ?? undefined, ); // Convert from native format (transitive_callers as array of groups) diff --git a/src/domain/analysis/dependencies.ts b/src/domain/analysis/dependencies.ts index 4a759e6e1..e602f0ab6 100644 --- a/src/domain/analysis/dependencies.ts +++ b/src/domain/analysis/dependencies.ts @@ -175,7 +175,7 @@ export function fnDepsData( opts: { depth?: number; noTests?: boolean; - file?: string; + file?: string | string[]; kind?: string; limit?: number; offset?: number; diff --git a/src/domain/analysis/fn-impact.ts b/src/domain/analysis/fn-impact.ts index 148cf5b59..d7cfc2901 100644 --- a/src/domain/analysis/fn-impact.ts +++ b/src/domain/analysis/fn-impact.ts @@ -268,7 +268,7 @@ export function fnImpactData( opts: { depth?: number; noTests?: boolean; - file?: string; + file?: string | string[]; kind?: string; includeImplementors?: boolean; limit?: number; diff --git a/src/domain/analysis/symbol-lookup.ts b/src/domain/analysis/symbol-lookup.ts index 5009d043a..065c11f4e 100644 --- a/src/domain/analysis/symbol-lookup.ts +++ b/src/domain/analysis/symbol-lookup.ts @@ -38,7 +38,12 @@ const FUNCTION_KINDS: SymbolKind[] = ['function', 'method', 'class', 'constant'] export function findMatchingNodes( dbOrRepo: BetterSqlite3Database | InstanceType, name: string, - opts: { noTests?: boolean; file?: string; kind?: string; kinds?: readonly string[] } = {}, + opts: { + noTests?: boolean; + file?: string | string[]; + kind?: string; + kinds?: readonly string[]; + } = {}, ): Array { const kinds = ( opts.kind ? [opts.kind] : opts.kinds?.length ? [...opts.kinds] : FUNCTION_KINDS diff --git a/src/presentation/queries-cli/impact.ts b/src/presentation/queries-cli/impact.ts index e9eafe052..94b1fa64d 100644 --- a/src/presentation/queries-cli/impact.ts +++ b/src/presentation/queries-cli/impact.ts @@ -118,7 +118,7 @@ interface OutputOpts { format?: string; staged?: boolean; ref?: string; - file?: string; + file?: string | string[]; kind?: string; [key: string]: unknown; } diff --git a/src/types.ts b/src/types.ts index 980713528..8374a2a0c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -265,7 +265,8 @@ export interface ComplexityMetrics { export interface QueryOpts { kind?: SymbolKind; kinds?: SymbolKind[]; - file?: string; + /** Partial-match file filter. Repeatable on the CLI (`-f/--file`), so a single value arrives as a one-element array. */ + file?: string | string[]; noTests?: boolean; } @@ -346,7 +347,7 @@ export interface Repository { // ── Composite queries ────────────────────────────────────────────── fnDeps( name: string, - opts?: { depth?: number; noTests?: boolean; file?: string; kind?: string }, + opts?: { depth?: number; noTests?: boolean; file?: string | string[]; kind?: string }, ): { name: string; results: Array<{ @@ -2496,7 +2497,7 @@ export interface NativeDatabase { findNodesWithFanIn( namePattern: string, kinds: string[] | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, ): NativeNodeRowWithFanIn[]; findNodesForTriage( kind: string | null | undefined, @@ -2553,7 +2554,7 @@ export interface NativeDatabase { name: string, depth: number | null | undefined, noTests: boolean | null | undefined, - file: string | null | undefined, + file: string[] | null | undefined, kind: string | null | undefined, ): NativeFnDepsResult; diff --git a/tests/integration/cli.test.ts b/tests/integration/cli.test.ts index f61b69153..28c3a0a76 100644 --- a/tests/integration/cli.test.ts +++ b/tests/integration/cli.test.ts @@ -110,6 +110,40 @@ describe('CLI smoke tests', () => { expect(data).toHaveProperty('results'); }); + // Regression test for #1726: `-f/--file` is a repeatable Commander option + // (collectFile) that always produces a string[], even for a single use. + // query's native composite path used to forward that array straight into + // a napi binding typed for a single String and crash. + test('query -f scopes results to a single file without crashing', () => { + const out = run('query', 'add', '-f', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('query --file (long form) scopes results to a single file without crashing', () => { + const out = run('query', 'add', '--file', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('query with a single -f excludes non-matching files', () => { + // `add` is only defined in math.js, so scoping to utils.js must be empty. + const out = run('query', 'add', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results).toHaveLength(0); + }); + + test('query supports repeated -f (multi-file scoping)', () => { + // "square" substring-matches square() in math.js and sumOfSquares() in utils.js. + const out = run('query', 'square', '-f', 'math.js', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + const names = data.results.map((r) => r.name); + expect(names).toContain('square'); + expect(names).toContain('sumOfSquares'); + }); + // ─── Fn-Impact ─────────────────────────────────────────────────────── test('fn-impact --json returns valid JSON with results', () => { const out = run('fn-impact', 'add', '--db', dbPath, '--json'); @@ -117,6 +151,38 @@ describe('CLI smoke tests', () => { expect(data).toHaveProperty('results'); }); + // Regression test for #1726 (see query -f test above for full context). + test('fn-impact -f scopes results to a single file without crashing', () => { + const out = run('fn-impact', 'add', '-f', 'math.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results.length).toBeGreaterThan(0); + for (const r of data.results) expect(r.file).toContain('math.js'); + }); + + test('fn-impact with a single -f excludes non-matching files', () => { + const out = run('fn-impact', 'add', '-f', 'utils.js', '--db', dbPath, '--json'); + const data = JSON.parse(out); + expect(data.results).toHaveLength(0); + }); + + test('fn-impact supports repeated -f (multi-file scoping)', () => { + const out = run( + 'fn-impact', + 'square', + '-f', + 'math.js', + '-f', + 'utils.js', + '--db', + dbPath, + '--json', + ); + const data = JSON.parse(out); + const names = data.results.map((r) => r.name); + expect(names).toContain('square'); + expect(names).toContain('sumOfSquares'); + }); + // ─── Query (path mode, formerly path) ──────────────────────────────── test('query --path --json returns valid JSON with path info', () => { const out = run('query', 'sumOfSquares', '--path', 'add', '--db', dbPath, '--json'); diff --git a/tests/integration/queries.test.ts b/tests/integration/queries.test.ts index 11fe0190c..c76d4ac86 100644 --- a/tests/integration/queries.test.ts +++ b/tests/integration/queries.test.ts @@ -361,6 +361,31 @@ describe('fnDepsData', () => { expect(names).not.toContain('authMiddleware'); }); + // Regression test for #1726: the CLI's `-f/--file` option is a repeatable + // Commander accumulator (collectFile) that always produces a string[], even + // for a single use. The native composite fnDeps() path used to forward that + // array straight into a napi binding typed for a single String and crash + // with "Failed to convert JavaScript value ... into rust type `String`". + test('--file as a single-element array (real CLI shape) scopes to a single file', () => { + const data = fnDepsData('auth', dbPath, { file: ['auth.js'] }); + for (const r of data.results) { + expect(r.file).toContain('auth.js'); + } + const names = data.results.map((r) => r.name); + expect(names).not.toContain('authMiddleware'); + }); + + test('--file with multiple values (repeated -f) scopes across all of them', () => { + const data = fnDepsData('auth', dbPath, { file: ['auth.js', 'middleware.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); // auth.js + expect(names).toContain('authMiddleware'); // middleware.js + // routes.js/utils.js should still be excluded + for (const r of data.results) { + expect(['auth.js', 'middleware.js']).toContain(r.file); + } + }); + test('--kind method filters to methods only', () => { // All fixtures are functions, so filtering by method should return empty const data = fnDepsData('auth', dbPath, { kind: 'method' }); @@ -414,6 +439,28 @@ describe('fnImpactData', () => { expect(r.direct + r.transitive).toBe(r.totalDependents); }); + // Regression test for #1726 — see the equivalent fnDepsData test above for + // full context. fnImpactData's native path (findNodesWithFanIn) forwards + // the file filter used to resolve the *matched root symbol(s)* (not their + // callers) into rusqlite, and used to crash on the array shape the CLI + // always sends. + test('--file as a single-element array scopes matched root symbols to that file', () => { + const data = fnImpactData('auth', dbPath, { file: ['auth.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); + expect(names).not.toContain('authMiddleware'); // middleware.js + for (const r of data.results) expect(r.file).toBe('auth.js'); + }); + + test('--file with multiple values scopes matched root symbols across all of them', () => { + const data = fnImpactData('auth', dbPath, { file: ['auth.js', 'middleware.js'] }); + const names = data.results.map((r) => r.name); + expect(names).toContain('authenticate'); // auth.js + expect(names).toContain('authMiddleware'); // middleware.js + // preAuthenticate (utils.js) also substring-matches "auth" — must stay excluded + for (const r of data.results) expect(['auth.js', 'middleware.js']).toContain(r.file); + }); + test('direct and transitive are 0 for a function with no callers', () => { // handleRoute is the root of the call graph — nothing calls it const data = fnImpactData('handleRoute', dbPath); From be6432cda8580230d9ed0401ceb7bc5720abc281 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 14:29:26 -0600 Subject: [PATCH 05/12] fix: correct exported-symbol detection for literal and object-literal exports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two compounding root causes made `where --file`'s `exported` array (and `codegraph exports`) unreliable for entire classes of exports: 1. The JS/TS export-statement handler (WASM query path, WASM walk path, and the mirrored native Rust extractor) only recognized `export function`, `export class`, `export interface`, and `export type` declarations. It never matched `lexical_declaration`/`variable_declaration`, so `export const/let/var …` was never added to the extractor's export list — regardless of the initializer's shape — leaving the `exported=1` DB column permanently unset for every exported constant in the codebase (including ones that happened to look "correct", like a `new Set(...)`-initialized constant referenced as a call argument elsewhere). 2. `where --file`'s `exported` list ignored the `exported` DB column entirely and computed membership from `findCrossFileCallTargets` (symbols targeted by a cross-file `calls` edge) — a heuristic that only "worked" by coincidence for symbols that happened to be passed as call arguments elsewhere. `codegraph exports` already had the right idea (prefer the `exported` column, fall back to the heuristic only for pre-migration DBs) but duplicated that logic locally instead of sharing it. Fixes: - src/extractors/javascript.ts: extract the function/class/interface/type kind map plus a new lexical/variable-declaration branch into a shared `collectExportedDeclarations`, used by both `handleExportCapture` (query path) and `handleExportStmt` (walk path) so they can't drift apart again. Declarator values are classified with the same predicate already used to build the matching Definition (function-valued -> kind 'function', literal/array/object/new-expression-valued `const` -> kind 'constant'). - crates/codegraph-core/src/extractors/javascript.rs: mirror the same fix in `handle_export_declaration` via a new `collect_exported_var_declarations`. - src/db/repository/nodes.ts: add `findExportedNodesByFile`, the single shared implementation of "prefer exported=1, fall back to cross-file calls for pre-migration DBs" — re-exported through db/repository/index.ts and db/index.ts. - src/domain/analysis/exports.ts: use the shared helper instead of a locally duplicated hasExportedCol/exportedNodesStmt implementation. - src/domain/analysis/symbol-lookup.ts: `whereFileImpl` now uses the shared helper instead of `findCrossFileCallTargets` directly, so `where --file` and `exports` agree on what "exported" means. Tests: - tests/parsers/javascript.test.ts: unit coverage for bare-literal, new Set(...), object-literal-with-methods, and arrow-function exported consts, plus a non-exported-const negative case and a function/class regression guard for the shared-helper refactor. - tests/engines/query-walk-parity.test.ts, tests/engines/parity.test.ts: cross-path/cross-engine regression cases for the same shapes. - tests/integration/queries.test.ts: new fixture node with exported=1 but zero incoming edges of any kind, proving `where --file`'s exported list no longer depends on cross-file call presence. No user-facing command/language/architecture changes, so README/CLAUDE.md/ ROADMAP.md are unaffected — docs check acknowledged. Fixes #1728 Impact: 8 functions changed, 17 affected --- .../src/extractors/javascript.rs | 53 +++++++++++ src/db/index.ts | 1 + src/db/repository/index.ts | 1 + src/db/repository/nodes.ts | 52 +++++++++++ src/domain/analysis/exports.ts | 52 +---------- src/domain/analysis/symbol-lookup.ts | 6 +- src/extractors/javascript.ts | 92 ++++++++++++------- tests/engines/parity.test.ts | 22 +++++ tests/engines/query-walk-parity.test.ts | 18 ++++ tests/integration/queries.test.ts | 25 ++++- tests/parsers/javascript.test.ts | 63 +++++++++++++ 11 files changed, 298 insertions(+), 87 deletions(-) diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 5a9292c7e..21d91a8c6 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1700,6 +1700,10 @@ fn handle_export_declaration(node: &Node, decl: &Node, source: &[u8], symbols: & "class_declaration" | "abstract_class_declaration" => ("class", "name"), "interface_declaration" => ("interface", "name"), "type_alias_declaration" => ("type", "name"), + "lexical_declaration" | "variable_declaration" => { + collect_exported_var_declarations(node, decl, source, symbols); + return; + } _ => return, }; if let Some(n) = decl.child_by_field_name(field) { @@ -1711,6 +1715,55 @@ fn handle_export_declaration(node: &Node, decl: &Node, source: &[u8], symbols: & } } +/// Push `ExportInfo` entries for `export const/let/var …`, one per declarator. +/// +/// Named function/class/interface/type declarations carry their own `name` +/// field (handled above); a lexical/variable declaration doesn't, so each +/// declarator's value is classified the same way `handle_var_decl` classifies +/// it when creating the matching `Definition`: function-valued declarators +/// become kind "function", `const` declarators with a literal/array/object/ +/// new-expression value (per `is_js_literal`) become kind "constant". +/// Mirrors the WASM/TS extractor's `collectExportedDeclarations`. +/// +/// This predicate must stay identical to `handle_var_decl`'s: `insert_nodes.rs` +/// marks `exported = 1` by matching (name, kind, file, line) against +/// already-inserted definition rows, so a mismatched kind here silently +/// no-ops the UPDATE instead of marking the symbol exported (#1728). +fn collect_exported_var_declarations( + node: &Node, + decl: &Node, + source: &[u8], + symbols: &mut FileSymbols, +) { + let is_const = decl + .child(0) + .map(|c| node_text(&c, source) == "const") + .unwrap_or(false); + let line = start_line(node); + for i in 0..decl.child_count() { + let Some(declarator) = decl.child(i) else { continue }; + if declarator.kind() != "variable_declarator" { continue; } + let name_n = declarator.child_by_field_name("name"); + let value_n = declarator.child_by_field_name("value"); + let (Some(name_n), Some(value_n)) = (name_n, value_n) else { continue }; + if name_n.kind() != "identifier" { continue; } + let vt = value_n.kind(); + if vt == "arrow_function" || vt == "function_expression" || vt == "function" || vt == "generator_function" { + symbols.exports.push(ExportInfo { + name: node_text(&name_n, source).to_string(), + kind: "function".to_string(), + line, + }); + } else if is_const && is_js_literal(&value_n) { + symbols.exports.push(ExportInfo { + name: node_text(&name_n, source).to_string(), + kind: "constant".to_string(), + line, + }); + } + } +} + fn handle_reexport(node: &Node, source_node: &Node, source: &[u8], symbols: &mut FileSymbols) { let mod_path = node_text(source_node, source) .replace(&['\'', '"'][..], ""); diff --git a/src/db/index.ts b/src/db/index.ts index 65b81de9c..f08a37677 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -39,6 +39,7 @@ export { findCallers, findCrossFileCallTargets, findDistinctCallers, + findExportedNodesByFile, findFileNodes, findImplementors, findImportDependents, diff --git a/src/db/repository/index.ts b/src/db/repository/index.ts index a7011806a..5fd5d1ff9 100644 --- a/src/db/repository/index.ts +++ b/src/db/repository/index.ts @@ -35,6 +35,7 @@ export { countEdges, countFiles, countNodes, + findExportedNodesByFile, findFileNodes, findNodeById, findNodeByQualifiedName, diff --git a/src/db/repository/nodes.ts b/src/db/repository/nodes.ts index b9465383e..cab617a5d 100644 --- a/src/db/repository/nodes.ts +++ b/src/db/repository/nodes.ts @@ -15,6 +15,7 @@ import type { } from '../../types.js'; import { buildFileConditionSQL, NodeQuery } from '../query-builder.js'; import { cachedStmt } from './cached-stmt.js'; +import { findCrossFileCallTargets } from './edges.js'; // ─── Query-builder based lookups (moved from src/db/repository.js) ───── @@ -179,6 +180,57 @@ export function findNodesByFile(db: BetterSqlite3Database, file: string): NodeRo ).all(file); } +/** Cache the schema probe for the `exported` column per db handle. */ +const _hasExportedColCache: WeakMap = new WeakMap(); + +const _findExportedNodesByFileStmt: StmtCache = new WeakMap(); + +/** + * Detect whether this DB's `nodes` table has the `exported` column populated + * (older DBs built before it existed fall back to a cross-file-calls + * heuristic). Cached per db handle to avoid re-probing on every call. + */ +function hasExportedColumn(db: BetterSqlite3Database): boolean { + if (_hasExportedColCache.has(db)) return _hasExportedColCache.get(db)!; + let has = false; + try { + db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true); + has = true; + } catch { + /* older DB predates the exported column */ + } + _hasExportedColCache.set(db, has); + return has; +} + +/** + * Find nodes in `file` that are part of its exported surface. + * + * Prefers the `exported = 1` column — set by the extractor at build time from + * the file's actual `export` statements, the authoritative "was this + * declared as exported" signal — and only falls back to + * `findCrossFileCallTargets` (symbols targeted by a cross-file `calls` edge) + * for DBs built before the column existed. + * + * Shared by `codegraph exports` and `codegraph where --file` so both commands + * agree on what "exported" means; before this was extracted, `where --file` + * used the cross-file-calls heuristic unconditionally, which misses any + * exported symbol that's only ever read as a value (not called) from another + * file — e.g. an exported constant used in a comparison (#1728). + */ +export function findExportedNodesByFile(db: BetterSqlite3Database, file: string): NodeRow[] { + if (hasExportedColumn(db)) { + return cachedStmt( + _findExportedNodesByFileStmt, + db, + "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", + ).all(file); + } + const symbols = findNodesByFile(db, file); + const exportedIds = findCrossFileCallTargets(db, file); + return symbols.filter((s) => exportedIds.has(s.id)); +} + /** * Find file-kind nodes matching a LIKE pattern. */ diff --git a/src/domain/analysis/exports.ts b/src/domain/analysis/exports.ts index 3c9710df6..f4780f8a3 100644 --- a/src/domain/analysis/exports.ts +++ b/src/domain/analysis/exports.ts @@ -1,12 +1,11 @@ import path from 'node:path'; import { - findCrossFileCallTargets, findDbPath, + findExportedNodesByFile, findFileNodes, findNodesByFile, } from '../../db/index.js'; import { cachedStmt } from '../../db/repository/cached-stmt.js'; -import { debug } from '../../infrastructure/logger.js'; import { isTestFile } from '../../infrastructure/test-filter.js'; import { createFileLinesReader, @@ -17,10 +16,6 @@ import { paginateResult } from '../../shared/paginate.js'; import type { BetterSqlite3Database, NodeRow, StmtCache } from '../../types.js'; import { resolveAnalysisOpts, withReadonlyDb } from './query-helpers.js'; -/** Cache the schema probe for the `exported` column per db handle. */ -const _hasExportedColCache: WeakMap = new WeakMap(); - -const _exportedNodesStmtCache: StmtCache = new WeakMap(); const _consumersStmtCache: StmtCache<{ name: string; file: string; line: number }> = new WeakMap(); const _reexportsFromStmtCache: StmtCache<{ file: string }> = new WeakMap(); const _reexportsToStmtCache: StmtCache<{ file: string }> = new WeakMap(); @@ -104,8 +99,6 @@ function collectReexportedSymbols( db: BetterSqlite3Database, fileNodeId: number, reexportsToStmt: ReturnType, - exportedNodesStmt: ReturnType | null, - hasExportedCol: boolean, getFileLines: (file: string) => string[] | null, buildSymbolResult: (s: NodeRow, fileLines: string[] | null) => any, ) { @@ -113,14 +106,7 @@ function collectReexportedSymbols( const reexportedSymbols: Array & { originFile: string }> = []; for (const reexTarget of reexportTargets) { - let targetExported: NodeRow[]; - if (hasExportedCol) { - targetExported = exportedNodesStmt!.all(reexTarget.file) as NodeRow[]; - } else { - const targetSymbols = findNodesByFile(db, reexTarget.file) as NodeRow[]; - const exportedIds = findCrossFileCallTargets(db, reexTarget.file) as Set; - targetExported = targetSymbols.filter((s) => exportedIds.has(s.id)); - } + const targetExported = findExportedNodesByFile(db, reexTarget.file); for (const s of targetExported) { reexportedSymbols.push({ ...buildSymbolResult(s, getFileLines(reexTarget.file)), @@ -142,28 +128,6 @@ function exportsFileImpl( const fileNodes = findFileNodes(db, `%${target}%`) as NodeRow[]; if (fileNodes.length === 0) return []; - // Detect whether exported column exists (cached per db handle) - let hasExportedCol: boolean; - if (_hasExportedColCache.has(db)) { - hasExportedCol = _hasExportedColCache.get(db)!; - } else { - hasExportedCol = false; - try { - db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true); - hasExportedCol = true; - } catch (e: unknown) { - debug(`exported column not available, using fallback: ${(e as Error).message}`); - } - _hasExportedColCache.set(db, hasExportedCol); - } - - const exportedNodesStmt = hasExportedCol - ? cachedStmt( - _exportedNodesStmtCache, - db, - "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", - ) - : null; // Consumers include real call/construct edges plus `imports-type` edges — // the symbol-level edge emitted for `import type { X }` statements (source // is the importing *file* node, since the import statement references the @@ -199,15 +163,7 @@ function exportsFileImpl( return fileNodes.map((fn) => { const symbols = findNodesByFile(db, fn.file) as NodeRow[]; - let exported: NodeRow[]; - if (hasExportedCol) { - // Use the exported column populated during build - exported = exportedNodesStmt!.all(fn.file) as NodeRow[]; - } else { - // Fallback: symbols that have incoming calls from other files - const exportedIds = findCrossFileCallTargets(db, fn.file) as Set; - exported = symbols.filter((s) => exportedIds.has(s.id)); - } + const exported = findExportedNodesByFile(db, fn.file); const internalCount = symbols.length - exported.length; const buildSymbolResult = (s: NodeRow, fileLines: string[] | null) => { @@ -244,8 +200,6 @@ function exportsFileImpl( db, fn.id, reexportsToStmt, - exportedNodesStmt, - hasExportedCol, getFileLines, buildSymbolResult, ); diff --git a/src/domain/analysis/symbol-lookup.ts b/src/domain/analysis/symbol-lookup.ts index 065c11f4e..90a39934e 100644 --- a/src/domain/analysis/symbol-lookup.ts +++ b/src/domain/analysis/symbol-lookup.ts @@ -3,7 +3,7 @@ import { findAllIncomingEdges, findAllOutgoingEdges, findCallers, - findCrossFileCallTargets, + findExportedNodesByFile, findFileNodes, findImportSources, findImportTargets, @@ -177,9 +177,7 @@ function whereFileImpl(db: BetterSqlite3Database, target: string) { const importedBy = (findImportSources(db, fn.id) as ImportEdgeRow[]).map((r) => r.file); - const exportedIds = findCrossFileCallTargets(db, fn.file) as Set; - - const exported = symbols.filter((s) => exportedIds.has(s.id)).map((s) => s.name); + const exported = findExportedNodesByFile(db, fn.file).map((s) => s.name); return { file: fn.file, diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index 4d189fa4c..7c4b1ea9e 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -195,6 +195,64 @@ function handleMethodCapture(c: Record, definitions: Def }); } +/** Node types whose own `name` field is the exported symbol's name. */ +const EXPORT_DECL_KIND: Record = { + function_declaration: 'function', + generator_function_declaration: 'function', + class_declaration: 'class', + abstract_class_declaration: 'class', + interface_declaration: 'interface', + type_alias_declaration: 'type', +}; + +/** + * Push Export entries for the declaration wrapped by an `export` statement. + * Shared by both extraction paths (query-based `handleExportCapture` and + * walk-based `handleExportStmt`) so they can't drift apart on what counts as + * an export — see the "two code paths" gotcha for this extractor. + * + * Named function/class/interface/type declarations carry their own `name` + * field. `export const/let/var …` has no such field — each declarator's value + * is classified the same way `handleVariableDeclarator` classifies it when + * building the matching Definition (function-valued → kind 'function', + * literal/array/object/new-expression-valued `const` → kind 'constant'). + * This predicate must stay identical to the Definition-building one: the + * exported=1 UPDATE it feeds matches DB rows by (name, kind, file, line), so + * a mismatched kind silently no-ops instead of marking the symbol exported (#1728). + */ +function collectExportedDeclarations( + decl: TreeSitterNode, + exportLine: number, + exps: Export[], +): void { + const kind = EXPORT_DECL_KIND[decl.type]; + if (kind) { + const n = decl.childForFieldName('name'); + if (n) exps.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); + return; + } + if (decl.type !== 'lexical_declaration' && decl.type !== 'variable_declaration') return; + const isConst = decl.text.startsWith('const '); + for (let i = 0; i < decl.childCount; i++) { + const declarator = decl.child(i); + if (declarator?.type !== 'variable_declarator') continue; + const nameN = declarator.childForFieldName('name'); + const valueN = declarator.childForFieldName('value'); + if (nameN?.type !== 'identifier' || !valueN) continue; + const valType = valueN.type; + if ( + valType === 'arrow_function' || + valType === 'function_expression' || + valType === 'function' || + valType === 'generator_function' + ) { + exps.push({ name: nameN.text, kind: 'function', line: exportLine }); + } else if (isConst && isConstantValue(valueN)) { + exps.push({ name: nameN.text, kind: 'constant', line: exportLine }); + } + } +} + /** Handle export_statement capture. */ function handleExportCapture( c: Record, @@ -203,22 +261,7 @@ function handleExportCapture( ): void { const exportLine = nodeStartLine(c.exp_node!); const decl = c.exp_node!.childForFieldName('declaration'); - if (decl) { - const declType = decl.type; - const kindMap: Record = { - function_declaration: 'function', - generator_function_declaration: 'function', - class_declaration: 'class', - abstract_class_declaration: 'class', - interface_declaration: 'interface', - type_alias_declaration: 'type', - }; - const kind = kindMap[declType]; - if (kind) { - const n = decl.childForFieldName('name'); - if (n) exps.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); - } - } + if (decl) collectExportedDeclarations(decl, exportLine, exps); const source = c.exp_node!.childForFieldName('source') || findChild(c.exp_node!, 'string'); if (source && !decl) { const modPath = source.text.replace(/['"]/g, ''); @@ -1448,22 +1491,7 @@ function handleImportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { function handleExportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { const exportLine = nodeStartLine(node); const decl = node.childForFieldName('declaration'); - if (decl) { - const declType = decl.type; - const kindMap: Record = { - function_declaration: 'function', - generator_function_declaration: 'function', - class_declaration: 'class', - abstract_class_declaration: 'class', - interface_declaration: 'interface', - type_alias_declaration: 'type', - }; - const kind = kindMap[declType]; - if (kind) { - const n = decl.childForFieldName('name'); - if (n) ctx.exports.push({ name: n.text, kind: kind as Export['kind'], line: exportLine }); - } - } + if (decl) collectExportedDeclarations(decl, exportLine, ctx.exports); const source = node.childForFieldName('source') || findChild(node, 'string'); if (source && !decl) { const modPath = source.text.replace(/['"]/g, ''); diff --git a/tests/engines/parity.test.ts b/tests/engines/parity.test.ts index 679207e79..f9c5b5c4d 100644 --- a/tests/engines/parity.test.ts +++ b/tests/engines/parity.test.ts @@ -144,6 +144,28 @@ add(1, 2); const MAX_RETRIES = 3; const APP_NAME = "codegraph"; const add = (a, b) => a + b; +`, + }, + { + // Regression guard for #1728: `export const …` must be recognized as an + // export in both engines regardless of the initializer's shape — a bare + // literal, a `new Expr(...)` call, and an arrow function must all + // produce a matching exports entry, and a non-exported const must not. + // + // The object-literal-with-methods shape (e.g. `export const command = { + // execute() {} }`, issue #1728's other repro) is deliberately not + // included here: both engines already agree it's exported (see + // tests/parsers/javascript.test.ts and tests/engines/query-walk-parity.test.ts), + // but they emit its qualified/unqualified method definitions in different + // relative array order — a separate, pre-existing, content-neutral gap + // (see #1818) that this test's unsorted `normalize()` would otherwise trip on. + name: 'JavaScript — exported constants of varying initializer shapes', + file: 'exported-const.js', + code: ` +export const MAX_WALK_DEPTH = 200; +export const PUNCTUATION_TOKENS = new Set([',', ';']); +export const add = (a, b) => a + b; +const INTERNAL = 'not exported'; `, }, { diff --git a/tests/engines/query-walk-parity.test.ts b/tests/engines/query-walk-parity.test.ts index 2b1f3e787..37f660264 100644 --- a/tests/engines/query-walk-parity.test.ts +++ b/tests/engines/query-walk-parity.test.ts @@ -170,6 +170,24 @@ export function serve(port) { listen(port); } export class Server { start() { this.init(); } } +`, + }, + { + // Regression guard for #1728: `export const …` must be recognized as an + // export regardless of the initializer's shape (bare literal, new-expression, + // object-literal-with-methods, arrow function) — both extraction paths must + // agree on the resulting (name, kind, line) exports entries. + name: 'exported const declarations of varying initializer shapes', + file: 'test.js', + code: ` +export const MAX_WALK_DEPTH = 200; +export const PUNCTUATION_TOKENS = new Set([',', ';']); +export const add = (a, b) => a + b; +export const command = { + name: 'info', + execute(args, opts, ctx) {}, +}; +const INTERNAL = 'not exported'; `, }, { diff --git a/tests/integration/queries.test.ts b/tests/integration/queries.test.ts index c76d4ac86..38672ced0 100644 --- a/tests/integration/queries.test.ts +++ b/tests/integration/queries.test.ts @@ -84,6 +84,14 @@ beforeAll(() => { const formatResponse = insertNode(db, 'formatResponse', 'function', 'utils.js', 1); const preAuthenticate = insertNode(db, 'preAuthenticate', 'function', 'utils.js', 10); + // Exported constant with zero incoming edges of any kind — regression fixture + // for #1728: "exported" must reflect the DB's exported=1 column (set by the + // extractor from the source's `export` keyword), not "has an incoming + // cross-file `calls` edge." A value-only export like this is never the + // target of a calls edge, so a cross-file-calls heuristic incorrectly + // omitted it from `where --file`'s exported list. + const apiVersion = insertNode(db, 'API_VERSION', 'constant', 'middleware.js', 1); + // Test file + function const fTest = insertNode(db, 'auth.test.js', 'file', 'auth.test.js', 0); const testAuth = insertNode(db, 'testAuthenticate', 'function', 'auth.test.js', 5); @@ -125,7 +133,14 @@ beforeAll(() => { // Mark exported symbols (those declared as exports in source) const markExported = db.prepare('UPDATE nodes SET exported = 1 WHERE id = ?'); - for (const id of [authenticate, validateToken, authMiddleware, formatResponse, preAuthenticate]) { + for (const id of [ + authenticate, + validateToken, + authMiddleware, + formatResponse, + preAuthenticate, + apiVersion, + ]) { markExported.run(id); } @@ -767,8 +782,14 @@ describe('whereData', () => { test('file: exported list', () => { const data = whereData('middleware.js', dbPath, { file: true }); const r = data.results[0]; - // authMiddleware is called from routes.js (cross-file) + // authMiddleware has exported=1 in the DB (and, incidentally, is also + // called from routes.js — but presence in `exported` must not depend on + // that; see the next assertion). expect(r.exported).toContain('authMiddleware'); + // API_VERSION has exported=1 but no incoming edges of any kind (#1728): + // "exported" reflects the DB's exported=1 column, not "has an incoming + // cross-file `calls` edge" — a value-only export is never call-targeted. + expect(r.exported).toContain('API_VERSION'); }); test('file: empty for unknown', () => { diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index b31040e2a..c825df2a9 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -1482,4 +1482,67 @@ describe('JavaScript parser', () => { ); }); }); + + describe('export-list detection for `export const/let/var …` (#1728)', () => { + it('lists named exported function/class declarations (refactor regression guard)', () => { + const symbols = parseJS(`export function greet() {}\nexport class Widget {}`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'greet', kind: 'function' }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'Widget', kind: 'class' }), + ); + }); + + it('lists an exported const with a bare numeric-literal initializer (repro 1)', () => { + const symbols = parseJS(`export const MAX_WALK_DEPTH = 200;`); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'MAX_WALK_DEPTH', kind: 'constant', line: 1 }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'MAX_WALK_DEPTH', kind: 'constant', line: 1 }), + ); + }); + + it('lists an exported const initialized with new Set(...) (sibling regression guard)', () => { + const symbols = parseJS(`export const PUNCTUATION_TOKENS = new Set([',', ';']);`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'PUNCTUATION_TOKENS', kind: 'constant', line: 1 }), + ); + }); + + it('lists an exported object-literal-with-methods const, without independently exporting its methods (repro 2)', () => { + const symbols = parseJS( + `export const command = {\n name: 'info',\n execute(args, opts, ctx) {},\n};`, + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'command', kind: 'constant', line: 1 }), + ); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'command.execute', kind: 'function' }), + ); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'command', kind: 'constant', line: 1 }), + ); + // Qualified child methods aren't independently listed as exports — mirrors + // how `Foo.method` isn't exported when only `export class Foo` is (only the + // top-level declared name is; see the class-method exported=0 convention). + expect(symbols.exports.some((e) => e.name === 'command.execute')).toBe(false); + }); + + it('lists an exported arrow-function const with kind "function"', () => { + const symbols = parseJS(`export const add = (a, b) => a + b;`); + expect(symbols.exports).toContainEqual( + expect.objectContaining({ name: 'add', kind: 'function', line: 1 }), + ); + }); + + it('does not list a non-exported const', () => { + const symbols = parseJS(`const INTERNAL = 42;`); + expect(symbols.definitions).toContainEqual( + expect.objectContaining({ name: 'INTERNAL', kind: 'constant' }), + ); + expect(symbols.exports.some((e) => e.name === 'INTERNAL')).toBe(false); + }); + }); }); From 4ead84062278ec31e1d08b26fa962b4026e98c4b Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 14:56:16 -0600 Subject: [PATCH 06/12] fix: exclude primitive type keywords from ast --kind string matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tree-sitter-typescript's predefined_type production (string, number, boolean, ... primitive type keywords) lexes its keyword as an anonymous grammar token whose type string is identical to the named `string` node type used for real string literals. Both engines matched by node type alone, so `codegraph ast --kind string` spuriously matched bare `string` type annotations (interface fields, parameter types, return types) as if they were string literals. Add an isNamed/is_named() guard scoped to the JS/TS/TSX family on both engines: WASM via astRequiresNamedNode() in ast-analysis/rules/index.ts feeding the shared resolveAstKind() in ast-store-visitor.ts, native via a match guard in javascript.rs's walk_ast_nodes_depth. Genuine string, template, and string-literal-type nodes are always named and unaffected. No user-facing command/language/architecture changes, so README/CLAUDE.md/ ROADMAP.md are unaffected — docs check acknowledged. Fixes #1729 Impact: 8 functions changed, 13 affected --- .../src/extractors/javascript.rs | 9 +- src/ast-analysis/engine.ts | 2 + src/ast-analysis/rules/index.ts | 20 ++ .../visitors/ast-store-visitor.ts | 15 +- src/domain/wasm-worker-entry.ts | 2 + src/features/ast.ts | 2 + src/types.ts | 10 + tests/engines/ast-parity.test.ts | 2 + tests/parsers/ast-nodes.test.ts | 229 ++++++++++++++++++ 9 files changed, 288 insertions(+), 3 deletions(-) diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 21d91a8c6..5c343e820 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1896,7 +1896,14 @@ fn walk_ast_nodes_depth(node: &Node, source: &[u8], ast_nodes: &mut Vec } return; } - "string" | "template_string" => { + // Guard on `is_named()`: tree-sitter-typescript's `predefined_type` + // production (the `string`/`number`/`boolean`/... primitive type + // keywords) lexes its keyword as an anonymous token whose `kind()` + // string is identical to the *named* `string` literal node type. + // Without this guard, `name: string` type annotations are + // misclassified as string-literal ast_nodes (#1729). Mirrors the + // WASM-side guard in `ast-store-visitor.ts::resolveAstKind`. + "string" | "template_string" if node.is_named() => { let raw = node_text(node, source); // Strip quotes to get content let content = raw diff --git a/src/ast-analysis/engine.ts b/src/ast-analysis/engine.ts index 6edc65413..706df0161 100644 --- a/src/ast-analysis/engine.ts +++ b/src/ast-analysis/engine.ts @@ -45,6 +45,7 @@ import { computeLOCMetrics, computeMaintainabilityIndex } from './metrics.js'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, CFG_RULES, COMPLEXITY_RULES, @@ -475,6 +476,7 @@ function setupAstVisitor( nodeIdMap, stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); } diff --git a/src/ast-analysis/rules/index.ts b/src/ast-analysis/rules/index.ts index 01a599eb4..298ce6fb0 100644 --- a/src/ast-analysis/rules/index.ts +++ b/src/ast-analysis/rules/index.ts @@ -378,3 +378,23 @@ export function astStopRecurseKinds(langId: string): ReadonlySet { } return EMPTY_STOP_RECURSE; } + +// ─── Per-language "named-node-required" guard ──────────────────────────── +// +// tree-sitter-typescript's `predefined_type` production (the `string`, +// `number`, `boolean`, `void`, ... primitive type keywords) lexes its +// keyword as an anonymous grammar token whose `type` string is identical to +// the *named* `string` node type used for real string literals — the +// grammar's own node-types.json lists both a `"string", named: true` entry +// (the literal) and a `"string", named: false` entry (the keyword token). +// Matching by `node.type` alone therefore misclassifies `name: string` +// type annotations as string-literal ast_nodes (#1729). Mirrors the native +// `is_named()` guard in `extractors/javascript.rs::walk_ast_nodes_depth`. +// +// Scoped to the JS family: no other bundled grammar currently maps an +// AST_TYPE_MAPS key that collides with an anonymous token of the same name. +const JS_REQUIRES_NAMED_NODE: ReadonlySet = new Set(['javascript', 'typescript', 'tsx']); + +export function astRequiresNamedNode(langId: string): boolean { + return JS_REQUIRES_NAMED_NODE.has(langId); +} diff --git a/src/ast-analysis/visitors/ast-store-visitor.ts b/src/ast-analysis/visitors/ast-store-visitor.ts index dd63515be..963840f9f 100644 --- a/src/ast-analysis/visitors/ast-store-visitor.ts +++ b/src/ast-analysis/visitors/ast-store-visitor.ts @@ -273,9 +273,19 @@ function collectNode(ctx: CollectCtx, node: TreeSitterNode, kind: string): void * Object() function), which then crashes the worker boundary with * "function Object() { [native code] } could not be cloned" when the * resulting astNodes row is structured-cloned back to the main thread. + * + * When `requireNamedNode` is set, anonymous grammar tokens are rejected even + * if their `type` string matches a mapped key — e.g. TypeScript's + * `predefined_type` keyword (`string`, `number`, ...) lexes as an unnamed + * token whose type collides with the named `string` literal node (#1729). */ -function resolveAstKind(node: TreeSitterNode, astTypeMap: Record): string | null { +function resolveAstKind( + node: TreeSitterNode, + astTypeMap: Record, + requireNamedNode: boolean, +): string | null { if (!Object.hasOwn(astTypeMap, node.type)) return null; + if (requireNamedNode && !node.isNamed) return null; return astTypeMap[node.type] || null; } @@ -286,6 +296,7 @@ export function createAstStoreVisitor( nodeIdMap: Map, stringConfig: AstStringConfig = DEFAULT_STRING_CONFIG, stopRecurseKinds: ReadonlySet = new Set(), + requireNamedNode = false, ): Visitor { const newTypes = newTypesFor(astTypeMap); // When nodeIdMap is empty, parentNodeId resolution is wasted work — the @@ -311,7 +322,7 @@ export function createAstStoreVisitor( // unrelated subtree. The parent call's skipChildren handles the intended case. if (ctx.matched.has(node.id)) return; - const kind = resolveAstKind(node, astTypeMap); + const kind = resolveAstKind(node, astTypeMap, requireNamedNode); if (!kind) return; collectNode(ctx, node, kind); diff --git a/src/domain/wasm-worker-entry.ts b/src/domain/wasm-worker-entry.ts index d55f5f7e6..2e02b4ed8 100644 --- a/src/domain/wasm-worker-entry.ts +++ b/src/domain/wasm-worker-entry.ts @@ -30,6 +30,7 @@ import { computeLOCMetrics, computeMaintainabilityIndex } from '../ast-analysis/ import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, CFG_RULES, COMPLEXITY_RULES, @@ -606,6 +607,7 @@ function buildAstVisitor( new Map(), stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); } diff --git a/src/features/ast.ts b/src/features/ast.ts index da9aff195..d227b4bcc 100644 --- a/src/features/ast.ts +++ b/src/features/ast.ts @@ -2,6 +2,7 @@ import path from 'node:path'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, } from '../ast-analysis/rules/index.js'; import { buildExtensionSet } from '../ast-analysis/shared.js'; @@ -244,6 +245,7 @@ function walkAst( nodeIdMap, stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); const results = walkWithVisitors(rootNode, [visitor], langId); const collected = (results['ast-store'] || []) as AstRow[]; diff --git a/src/types.ts b/src/types.ts index 8374a2a0c..81fafbe03 100644 --- a/src/types.ts +++ b/src/types.ts @@ -777,6 +777,16 @@ export interface TreeSitterNode { id: number; type: string; text: string; + /** + * Whether this node is a named grammar production vs. an anonymous token + * (keyword/punctuation). Tree-sitter grammars can define an anonymous + * token whose text matches a *named* node's type string — e.g. + * tree-sitter-typescript's `predefined_type` wraps an anonymous `string` + * keyword token that collides with the named `string` literal node type + * (#1729). Consumers matching by `type` alone must also check `isNamed` + * to avoid misclassifying keyword tokens as literal/expression nodes. + */ + isNamed: boolean; startPosition: { row: number; column: number }; endPosition: { row: number; column: number }; childCount: number; diff --git a/tests/engines/ast-parity.test.ts b/tests/engines/ast-parity.test.ts index a73c5852f..4e4e50192 100644 --- a/tests/engines/ast-parity.test.ts +++ b/tests/engines/ast-parity.test.ts @@ -11,6 +11,7 @@ import { beforeAll, describe, expect, it } from 'vitest'; import { AST_STRING_CONFIGS, AST_TYPE_MAPS, + astRequiresNamedNode, astStopRecurseKinds, } from '../../src/ast-analysis/rules/index.js'; import { walkWithVisitors } from '../../src/ast-analysis/visitor.js'; @@ -420,6 +421,7 @@ let run () = new Map(), stringConfig, astStopRecurseKinds(langId), + astRequiresNamedNode(langId), ); const results = walkWithVisitors(tree.rootNode as any, [visitor], langId); const rows = (results['ast-store'] || []) as unknown[]; diff --git a/tests/parsers/ast-nodes.test.ts b/tests/parsers/ast-nodes.test.ts index 4ebd79f7d..b6d31a503 100644 --- a/tests/parsers/ast-nodes.test.ts +++ b/tests/parsers/ast-nodes.test.ts @@ -182,6 +182,168 @@ describe('buildAstNodes — JS extraction', () => { }); }); +// ─── TypeScript fixture (#1729: predefined_type keyword false-positives) ── +// +// tree-sitter-typescript's `predefined_type` production (the `string`, +// `number`, `boolean`, ... primitive type keywords) lexes its keyword as an +// anonymous token whose `type` string collides with the *named* `string` +// literal node type. `--kind string` must only match the latter. + +const TS_FIXTURE_CODE = ` +interface Person { + name: string; + age: number; +} + +type UserId = "user-id-literal"; + +function greet(name: string): string { + return \`Hello, \${name}!\`; +} + +function processItems(items: string[]): void { + console.log(items); +} + +import { helper } from './helper.js'; + +const greeting = "hello world"; +`; + +describe('buildAstNodes — TypeScript extraction (#1729)', () => { + let tsTmpDir: string, tsDb: any; + + function queryTsAstNodes(kind: string) { + return tsDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + tsTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-ts-extract-')); + const srcDir = path.join(tsTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(tsTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.ts'); + fs.writeFileSync(fixturePath, TS_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], tsTmpDir, { engine: 'wasm' }); + const symbols = allSymbols.get('src/fixture.ts'); + if (!symbols) throw new Error('Failed to parse TS fixture file'); + + const dbPath = path.join(tsTmpDir, '.codegraph', 'graph.db'); + tsDb = new Database(dbPath); + tsDb.pragma('journal_mode = WAL'); + initSchema(tsDb); + + const insertNode = tsDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.ts', def.line, def.endLine); + } + + await buildAstNodes(tsDb, allSymbols, tsTmpDir); + }); + + afterAll(() => { + if (tsDb) tsDb.close(); + fs.rmSync(tsTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string', () => { + // `name: string;` (line 3) must never surface as a bare, unquoted "string" + // row — genuine literals are always quoted in `text` (e.g. "hello world"). + const nodes = queryTsAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify parameter or return type annotations as kind:string', () => { + // `greet(name: string): string` (param + return type, line 9) must not + // contribute bare "string" rows. + const nodes = queryTsAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('does not misclassify array-of-primitive parameter type as kind:string', () => { + // `processItems(items: string[])` (line 13) wraps predefined_type in + // array_type — must not contribute a bare "string" row either. + const nodes = queryTsAstNodes('string'); + expect(nodes.some((n) => n.line === 13)).toBe(false); + }); + + test('still captures genuine string literals, template literals, and string-literal types', () => { + const nodes = queryTsAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names).toContain('user-id-literal'); // type UserId = "user-id-literal" + expect(names).toContain('./helper.js'); // import source path + expect(names).toContain('hello world'); // genuine string literal + expect(nodes.some((n) => n.text?.startsWith('`Hello, '))).toBe(true); // template literal + }); + + test('captures exactly the 4 genuine literals — no keyword false-positives', () => { + const nodes = queryTsAstNodes('string'); + expect(nodes.length).toBe(4); + }); +}); + +// ─── TSX fixture (#1729) — shares JS_AST_TYPES + grammar family with TS ─── + +const TSX_FIXTURE_CODE = ` +interface Props { + label: string; +} + +function Greeting(props: Props) { + return
{"hello world"}
; +} +`; + +describe('buildAstNodes — TSX extraction (#1729)', () => { + let tsxTmpDir: string, tsxDb: any; + + beforeAll(async () => { + tsxTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-tsx-extract-')); + const srcDir = path.join(tsxTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(tsxTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.tsx'); + fs.writeFileSync(fixturePath, TSX_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], tsxTmpDir, { engine: 'wasm' }); + const symbols = allSymbols.get('src/fixture.tsx'); + if (!symbols) throw new Error('Failed to parse TSX fixture file'); + + const dbPath = path.join(tsxTmpDir, '.codegraph', 'graph.db'); + tsxDb = new Database(dbPath); + tsxDb.pragma('journal_mode = WAL'); + initSchema(tsxDb); + + const insertNode = tsxDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.tsx', def.line, def.endLine); + } + + await buildAstNodes(tsxDb, allSymbols, tsxTmpDir); + }); + + afterAll(() => { + if (tsxDb) tsxDb.close(); + fs.rmSync(tsxTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string, still captures JSX-embedded literal', () => { + const nodes = tsxDb + .prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line') + .all('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + expect(nodes.length).toBe(1); + expect(nodes[0].name).toBe('hello world'); + }); +}); + // ─── Native engine AST node extraction ─────────────────────────────── // Check if native addon is available AND supports ast_nodes. @@ -308,3 +470,70 @@ describe.skipIf(!canTestNative)('buildAstNodes — native engine', () => { expect(withParent.length).toBeGreaterThan(0); }); }); + +// ─── Native engine: TypeScript predefined_type false-positives (#1729) ──── + +describe.skipIf(!canTestNative)('buildAstNodes — native TypeScript extraction (#1729)', () => { + let nativeTsTmpDir: string, nativeTsDb: any; + + function queryNativeTsAstNodes(kind: string) { + return nativeTsDb.prepare('SELECT * FROM ast_nodes WHERE kind = ? ORDER BY line').all(kind); + } + + beforeAll(async () => { + nativeTsTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-ast-ts-native-')); + const srcDir = path.join(nativeTsTmpDir, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(path.join(nativeTsTmpDir, '.codegraph')); + + const fixturePath = path.join(srcDir, 'fixture.ts'); + fs.writeFileSync(fixturePath, TS_FIXTURE_CODE); + + const allSymbols = await parseFilesAuto([fixturePath], nativeTsTmpDir, { engine: 'native' }); + const symbols = allSymbols.get('src/fixture.ts'); + if (!symbols) throw new Error('Failed to parse TS fixture file with native engine'); + + const dbPath = path.join(nativeTsTmpDir, '.codegraph', 'graph.db'); + nativeTsDb = new Database(dbPath); + nativeTsDb.pragma('journal_mode = WAL'); + initSchema(nativeTsDb); + + const insertNode = nativeTsDb.prepare( + 'INSERT INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + for (const def of symbols.definitions) { + insertNode.run(def.name, def.kind, 'src/fixture.ts', def.line, def.endLine); + } + + await buildAstNodes(nativeTsDb, allSymbols, nativeTsTmpDir); + }); + + afterAll(() => { + if (nativeTsDb) nativeTsDb.close(); + if (nativeTsTmpDir) fs.rmSync(nativeTsTmpDir, { recursive: true, force: true }); + }); + + test('does not misclassify interface field type annotation as kind:string', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.some((n) => n.text === 'string')).toBe(false); + }); + + test('does not misclassify parameter, return, or array-element type annotations as kind:string', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.filter((n) => n.text === 'string').length).toBe(0); + }); + + test('still captures genuine string literals, template literals, and string-literal types', () => { + const nodes = queryNativeTsAstNodes('string'); + const names = nodes.map((n) => n.name); + expect(names).toContain('user-id-literal'); + expect(names).toContain('./helper.js'); + expect(names).toContain('hello world'); + expect(nodes.some((n) => n.text?.startsWith('`Hello, '))).toBe(true); + }); + + test('captures exactly the 4 genuine literals — no keyword false-positives', () => { + const nodes = queryNativeTsAstNodes('string'); + expect(nodes.length).toBe(4); + }); +}); From f6326bff7b3925ce80edf0e6ca0271b5efbdc0e7 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 15:45:20 -0600 Subject: [PATCH 07/12] fix: resolve call edges through renamed import specifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codegraph exports reported zero consumers for an exported function/symbol that is genuinely imported and used elsewhere, whenever the importing file renamed the binding at the import site (import { X as Y } from '...'). Example: collectFiles in src/domain/graph/builder/helpers.ts is imported (as collectFilesUtil) and called by collect-files.ts and native-orchestrator.ts, but `codegraph exports helpers.ts -T --json` showed consumerCount: 0. Root cause (extractor, both engines): extractImportNames (javascript.ts) and its mirrored scan_import_names_depth (javascript.rs) handled import_specifier nodes with `node.childForFieldName('name') || childForFieldName('alias')`. Per the tree-sitter grammar, `name` is *always* present on import_specifier (the name as declared by the source module); `alias` only exists for `X as Y` and holds the *local* binding actually referenced by call sites. Preferring `name` unconditionally meant the local alias was silently dropped for every renamed import — `imp.names` recorded "collectFiles" instead of "collectFilesUtil", so `importedNames` (keyed by call-site text) never had a matching entry and the call fell through unresolved. Fixing only the extractor was not sufficient: once `imp.names` correctly holds the local alias, `resolveCallTargets` (call-resolver.ts) / resolve_call_targets (build_edges.rs) still searched the *target file* for a symbol literally named "collectFilesUtil" — which doesn't exist there (only "collectFiles" does). This required threading a second piece of information end-to-end: for each renamed specifier, the local alias's *original* exported name, so target-file/barrel lookups search by the right name while importedNames stays keyed by the call-site text. Changes (both engines, full-build + incremental/native-orchestrator + native-hybrid paths): - types.ts / types.rs: new `Import.renamedImports` / `Import.renamed_imports` field — { local, imported } pairs, populated only for specifiers that actually rename a binding. - extractors/javascript.ts, .rs: extractImportNames / extract_import_names_with_renames now prefer `alias` (local binding) for import_specifier and record the rename pair. export_specifier is deliberately left unchanged (see scope notes below). Fixed in both the walk path (handleImportStmt) and the query/WASM-worker path (handleImportCapture) on the TS side. - build-edges.ts, build_edges.rs, import_edges.rs, pipeline.rs, call-resolver.ts, incremental.ts: buildImportedNamesMap / collect_imported_names_for_file now also produce a local-alias -> original-name map; resolveCallTargets / resolve_call_targets consult it to search the target file by the original name. Applied consistently to the primary call-resolution path, the Object.defineProperty post-pass, points-to alias resolution, barrel edge / import-type edge emission, and cross-file return-type propagation — every place that previously assumed a call site's local name equals the target file's declared name. Native Rust: touched and verified. Rebuilt locally via `npx napi build --platform --release` (crates/codegraph-core), codesigned (`codesign --sign - --force`), and installed over the loaded node_modules/@optave/codegraph-darwin-arm64/codegraph-core.node so the fix was exercised by the actual native engine, not the prebuilt npm binary. `cargo test --lib`: 427 passed. Tests added: - tests/parsers/javascript.test.ts: extractor-level coverage for the local-name/rename-pair extraction, a mixed renamed+non-renamed specifier list, and confirmation export_specifier is unaffected. - crates/codegraph-core/src/extractors/javascript.rs: matching Rust unit tests. - tests/integration/issue-1730-renamed-import-consumer.test.ts: new end-to-end test building real files through buildGraph() on both wasm and native engines, asserting the calls edge exists, no edge is created against a nonexistent "collectFilesUtil" symbol, and `codegraph exports` credits the consumer — on both engines. tests/integration/exports.test.ts was not extended: that file hand-inserts DB rows and only exercises the query layer, which is unaffected by this bug (the missing piece was edge creation, not the consumer query added for #1724). Verified against this repo's own graph (native engine, rebuilt): `codegraph exports src/domain/graph/builder/helpers.ts -T --json` now credits collectFiles with 2 consumers (collect-files.ts, native-orchestrator.ts); WASM engine agrees exactly. Without -T, also picks up tests/unit/builder.test.ts via the (unrenamed) builder.ts barrel re-export, which already worked pre-fix and was only hidden by -T in the original repro. Scope notes — filed as separate issues rather than expanding this fix: - #1823: export { X as Y } from '...' (barrel re-export with rename) is not tracked — resolveBarrelExport/resolve_barrel_export key off the original name, and export_specifier extraction was deliberately left unchanged here since barrel/reexport tracing is a distinct mechanism. - #1824: dynamic import destructuring rename (const { a: b } = await import(...)) has the same class of bug in extractDynamicImportNames, a different extraction function. - #1825: a renamed import used as a call receiver (import { X as Y }; Y.method()) still fails to resolve — confirmed empirically (no calls edge created) — a different resolution strategy (resolveByReceiver/resolveViaDirectQualifiedMethod in resolver/strategy.ts) than the one this fix addresses. Full test suite (202 files, 3398 tests) and lint pass clean. docs check acknowledged: internal resolver/extractor bug fix, no new feature/language/CLI surface/architecture change -- README.md, CLAUDE.md, and ROADMAP.md are unaffected. Fixes #1730 Impact: 29 functions changed, 47 affected --- .../src/domain/graph/builder/pipeline.rs | 38 +++-- .../graph/builder/stages/build_edges.rs | 38 ++++- .../graph/builder/stages/import_edges.rs | 51 ++++--- .../src/extractors/javascript.rs | 102 ++++++++++++- crates/codegraph-core/src/types.rs | 20 +++ src/domain/graph/builder/call-resolver.ts | 11 +- src/domain/graph/builder/incremental.ts | 89 +++++++++--- .../graph/builder/stages/build-edges.ts | 134 +++++++++++++----- src/extractors/javascript.ts | 47 +++++- src/types.ts | 11 ++ ...issue-1730-renamed-import-consumer.test.ts | 125 ++++++++++++++++ tests/parsers/javascript.test.ts | 46 ++++++ 12 files changed, 618 insertions(+), 94 deletions(-) create mode 100644 tests/integration/issue-1730-renamed-import-consumer.test.ts diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index 53f05601d..f191b9f66 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -1277,17 +1277,22 @@ fn load_file_node_id_map(conn: &Connection) -> HashMap { /// Resolve a file's imports to the list of `ImportedName` entries the edge /// builder consumes. Walks barrel chains to the ultimate definition file so /// the edge builder's name-lookup can find the right target (#976 P1). +/// +/// For renamed specifiers (`import { X as Y }`), `ImportedName.imported` +/// carries the original name (X) so `resolve_call_targets` can look it up in +/// the target file instead of the local alias (Y), which only exists in the +/// importing file (#1730). fn collect_imported_names_for_file( abs_str: &str, symbols: &FileSymbols, import_ctx: &crate::domain::graph::builder::stages::import_edges::ImportEdgeContext, ) -> Vec { use crate::domain::graph::builder::stages::build_edges::ImportedName; + use crate::domain::graph::builder::stages::import_edges::import_name_pairs; let mut imported_names: Vec = Vec::new(); for imp in &symbols.imports { let resolved_path = import_ctx.get_resolved(abs_str, &imp.source); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name).to_string(); + for (local, original) in import_name_pairs(imp) { // CJS require bindings are included in imported_names so the receiver-edge // resolver treats them as import artifacts (not locally-defined symbols). // We use an empty target_file so the import-aware call-target lookup @@ -1295,20 +1300,25 @@ fn collect_imported_names_for_file( // through to the same-file shadow node — matching WASM call-resolution // behaviour where CJS bindings are not in importedNamesMap (#1678). if imp.cjs_require.unwrap_or(false) { - imported_names.push(ImportedName { name: clean_name, file: String::new() }); + imported_names.push(ImportedName { + name: local, + file: String::new(), + imported: None, + }); continue; } let mut target_file = resolved_path.clone(); if import_ctx.is_barrel_file(&resolved_path) { let mut visited = HashSet::new(); if let Some(actual) = - import_ctx.resolve_barrel_export(&resolved_path, &clean_name, &mut visited) + import_ctx.resolve_barrel_export(&resolved_path, &original, &mut visited) { target_file = actual; } } imported_names.push(ImportedName { - name: clean_name, + imported: if original != local { Some(original) } else { None }, + name: local, file: target_file, }); } @@ -1411,8 +1421,14 @@ fn inject_return_types_for_file( let imported_names = collect_imported_names_for_file(abs_str, symbols, import_ctx); // Later entries overwrite earlier ones on duplicate names — same as the // HashMap collect in build_call_edges. - let imported_map: HashMap = - imported_names.into_iter().map(|e| (e.name, e.file)).collect(); + let mut imported_map: HashMap = HashMap::new(); + let mut imported_original_map: HashMap = HashMap::new(); + for e in imported_names { + if let Some(original) = e.imported { + imported_original_map.insert(e.name.clone(), original); + } + imported_map.insert(e.name, e.file); + } let mut injections: Vec = Vec::new(); let mut injected: HashSet = HashSet::new(); @@ -1429,7 +1445,13 @@ fn inject_return_types_for_file( global_return_types.get(&format!("{receiver}.{}", ca.callee_name)) } None => imported_map.get(&ca.callee_name).and_then(|from| { - return_type_index.get(from).and_then(|m| m.get(&ca.callee_name)) + // The return-type index for the imported file is keyed by the + // function's own declared name — use the original (pre-rename) + // name when the callee is a renamed import binding (#1730). + let callee_original_name = imported_original_map + .get(&ca.callee_name) + .unwrap_or(&ca.callee_name); + return_type_index.get(from).and_then(|m| m.get(callee_original_name)) }), }; diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs index d86c42872..8d14f7e4d 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs @@ -46,6 +46,12 @@ pub struct CallInfo { pub struct ImportedName { pub name: String, pub file: String, + /// For renamed specifiers (`import { X as Y }`): the original name + /// exported by `file` (X), when it differs from `name` (the local + /// binding Y). `resolve_call_targets` looks this up in `file` instead of + /// `name` — the renamed local alias only exists in the importing file, + /// not in `file` itself (#1730). + pub imported: Option, } #[napi(object)] @@ -384,6 +390,7 @@ struct PtsAliasCtx<'a> { is_dynamic: u32, rel_path: &'a str, imported_names: &'a HashMap<&'a str, &'a str>, + imported_original_names: &'a HashMap<&'a str, &'a str>, type_map: &'a HashMap<&'a str, (&'a str, f64)>, } @@ -399,6 +406,7 @@ fn emit_pts_alias_edges<'a>( ) { for alias in resolve_via_points_to(alias_ctx.lookup_name, alias_ctx.pts) { let alias_imported_from = alias_ctx.imported_names.get(alias).copied(); + let alias_imported_original_name = alias_ctx.imported_original_names.get(alias).copied(); let alias_call = CallInfo { name: alias.to_string(), line: alias_ctx.call_line, @@ -409,6 +417,7 @@ fn emit_pts_alias_edges<'a>( }; let mut alias_targets = resolve_call_targets( ctx, &alias_call, alias_ctx.rel_path, alias_imported_from, alias_ctx.type_map, alias_ctx.caller_name, + alias_imported_original_name, ); sort_targets_by_confidence(&mut alias_targets, alias_ctx.rel_path, alias_imported_from); for t in &alias_targets { @@ -458,6 +467,11 @@ struct FileContext<'a> { rel_path: &'a str, file_node_id: u32, imported_names: HashMap<&'a str, &'a str>, + /// Local import alias -> original exported name, for renamed specifiers + /// (`import { X as Y }`) only — entries where local === original are + /// omitted. Consulted by `resolve_call_targets` so a call to the local + /// alias resolves against the correct exported symbol (#1730). + imported_original_names: HashMap<&'a str, &'a str>, type_map: HashMap<&'a str, (&'a str, f64)>, defs_with_ids: Vec>, pts_map: Option>>, @@ -567,6 +581,10 @@ fn build_file_context<'a>( .imported_names.iter() .map(|im| (im.name.as_str(), im.file.as_str())) .collect(); + let imported_original_names: HashMap<&str, &str> = file_input + .imported_names.iter() + .filter_map(|im| im.imported.as_deref().map(|orig| (im.name.as_str(), orig))) + .collect(); let type_map = build_type_map(file_input); let file_nodes: Vec<&NodeInfo> = all_nodes.iter().filter(|n| n.file == rel_path).collect(); let defs_with_ids: Vec = file_input.definitions.iter().map(|d| { @@ -590,6 +608,7 @@ fn build_file_context<'a>( rel_path, file_node_id: file_input.file_node_id, imported_names, + imported_original_names, type_map, defs_with_ids, pts_map, @@ -655,6 +674,7 @@ fn emit_no_receiver_pts_edges<'a>( is_dynamic, rel_path: fc.rel_path, imported_names: &fc.imported_names, + imported_original_names: &fc.imported_original_names, type_map: &fc.type_map, }, seen_edges, @@ -697,6 +717,7 @@ fn emit_receiver_pts_edges<'a>( is_dynamic, rel_path: fc.rel_path, imported_names: &fc.imported_names, + imported_original_names: &fc.imported_original_names, type_map: &fc.type_map, }, seen_edges, @@ -734,8 +755,11 @@ fn process_file<'a>( let (caller_id, caller_name) = find_enclosing_caller(&fc.defs_with_ids, call.line, fc.file_node_id); let is_dynamic = if call.dynamic.unwrap_or(false) { 1u32 } else { 0u32 }; let imported_from = fc.imported_names.get(call.name.as_str()).copied(); + let imported_original_name = fc.imported_original_names.get(call.name.as_str()).copied(); - let mut targets = resolve_call_targets(ctx, call, fc.rel_path, imported_from, &fc.type_map, caller_name); + let mut targets = resolve_call_targets( + ctx, call, fc.rel_path, imported_from, &fc.type_map, caller_name, imported_original_name, + ); sort_targets_by_confidence(&mut targets, fc.rel_path, imported_from); emit_call_edges(&targets, caller_id, is_dynamic, fc.rel_path, imported_from, &mut seen_edges, &mut pts_edge_map, edges); @@ -882,6 +906,7 @@ fn resolve_call_targets<'a>( imported_from: Option<&str>, type_map: &HashMap<&str, (&str, f64)>, caller_name: &str, + imported_original_name: Option<&str>, ) -> Vec<&'a NodeInfo> { // Flagged dynamic calls use synthetic names like "". Short-circuit // so they never accidentally match a real symbol via name lookup. @@ -889,10 +914,15 @@ fn resolve_call_targets<'a>( return vec![]; } + // When the call site uses a renamed import binding (`import { X as Y }`), + // the imported file's actual symbol is declared under the *original* name + // (X) — look that up instead of the local alias the call site wrote (#1730). + let target_name = imported_original_name.unwrap_or(call.name.as_str()); + // 1. Import-aware resolution if let Some(imp_file) = imported_from { let targets = ctx.nodes_by_name_and_file - .get(&(call.name.as_str(), imp_file)) + .get(&(target_name, imp_file)) .cloned().unwrap_or_default(); if !targets.is_empty() { return targets; } } @@ -2081,7 +2111,7 @@ mod call_edge_tests { ); // Mark `Calculator` as an imported name so the resolver treats the // same-file kind="function" node as an import artifact and falls through. - file.imported_names = vec![ImportedName { name: "Calculator".to_string(), file: "utils.js".to_string() }]; + file.imported_names = vec![ImportedName { name: "Calculator".to_string(), file: "utils.js".to_string(), imported: None }]; let edges = build_call_edges(vec![file], all_nodes, vec![]); @@ -2464,7 +2494,7 @@ mod call_edge_tests { vec![], vec![], ); - file.imported_names = vec![ImportedName { name: "e4".to_string(), file: "other.js".to_string() }]; + file.imported_names = vec![ImportedName { name: "e4".to_string(), file: "other.js".to_string(), imported: None }]; file.param_bindings = Some(vec![ParamBinding { callee: "f3".to_string(), arg_index: 0, diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs index fd09d4a2f..c8f0487e8 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs @@ -115,6 +115,29 @@ impl BarrelContext for ImportEdgeContext { } } +/// Pairs each locally-bound name from an import statement with its original +/// (pre-rename) exported name — identical to the local name unless the +/// specifier renames a binding (`import { X as Y }`). Barrel tracing and +/// target-file symbol lookups must search using the *original* name — the +/// renamed local alias only exists in the importing file, not in the file +/// being imported from (#1730). Mirrors `importNamePairs` in build-edges.ts. +pub(crate) fn import_name_pairs(imp: &crate::types::Import) -> Vec<(String, String)> { + let mut original_name_for: HashMap<&str, &str> = HashMap::new(); + if let Some(renamed) = &imp.renamed_imports { + for r in renamed { + original_name_for.insert(&r.local, &r.imported); + } + } + imp.names + .iter() + .map(|name| { + let local = name.strip_prefix("* as ").unwrap_or(name); + let original = original_name_for.get(local).copied().unwrap_or(local); + (local.to_string(), original.to_string()) + }) + .collect() +} + /// Build the reexport map from parsed file symbols. pub fn build_reexport_map(ctx: &ImportEdgeContext) -> HashMap> { let mut reexport_map = HashMap::new(); @@ -249,18 +272,17 @@ fn collect_type_only_lookup_pairs(ctx: &ImportEdgeContext) -> HashSet<(String, S continue; } let resolved_path = ctx.get_resolved(abs_str, &imp.source); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut target_file = resolved_path.clone(); if ctx.is_barrel_file(&resolved_path) { let mut visited = HashSet::new(); if let Some(actual) = - ctx.resolve_barrel_export(&resolved_path, clean_name, &mut visited) + ctx.resolve_barrel_export(&resolved_path, &original, &mut visited) { target_file = actual; } } - pairs.insert((clean_name.to_string(), target_file)); + pairs.insert((original, target_file)); } } } @@ -303,18 +325,16 @@ fn emit_type_only_symbol_rows( if !imp.type_only.unwrap_or(false) { return; } - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut target_file = resolved_path.to_string(); if ctx.is_barrel_file(resolved_path) { let mut visited = HashSet::new(); - if let Some(actual) = - ctx.resolve_barrel_export(resolved_path, clean_name, &mut visited) + if let Some(actual) = ctx.resolve_barrel_export(resolved_path, &original, &mut visited) { target_file = actual; } } - if let Some(&sym_id) = symbol_node_ids.get(&(clean_name.to_string(), target_file)) { + if let Some(&sym_id) = symbol_node_ids.get(&(original, target_file)) { edges.push(EdgeRow { source_id: file_node_id, target_id: sym_id, @@ -347,14 +367,13 @@ fn emit_barrel_through_rows( _ => "imports", }; let mut resolved_sources: HashSet = HashSet::new(); - for name in &imp.names { - let clean_name = name.strip_prefix("* as ").unwrap_or(name); + for (_local, original) in import_name_pairs(imp) { let mut visited = HashSet::new(); - let actual_source = - match ctx.resolve_barrel_export(resolved_path, clean_name, &mut visited) { - Some(s) => s, - None => continue, - }; + let actual_source = match ctx.resolve_barrel_export(resolved_path, &original, &mut visited) + { + Some(s) => s, + None => continue, + }; if actual_source == resolved_path || !resolved_sources.insert(actual_source.clone()) { continue; } diff --git a/crates/codegraph-core/src/extractors/javascript.rs b/crates/codegraph-core/src/extractors/javascript.rs index 5c343e820..daf2ff9d3 100644 --- a/crates/codegraph-core/src/extractors/javascript.rs +++ b/crates/codegraph-core/src/extractors/javascript.rs @@ -1672,11 +1672,15 @@ fn handle_import_stmt(node: &Node, source: &[u8], symbols: &mut FileSymbols) { if let Some(source_node) = source_node { let mod_path = node_text(&source_node, source) .replace(&['\'', '"'][..], ""); - let names = extract_import_names(node, source); + let mut renamed_imports = Vec::new(); + let names = extract_import_names_with_renames(node, source, &mut renamed_imports); let mut imp = Import::new(mod_path, names, start_line(node)); if is_type_only { imp.type_only = Some(true); } + if !renamed_imports.is_empty() { + imp.renamed_imports = Some(renamed_imports); + } symbols.imports.push(imp); } } @@ -2972,20 +2976,80 @@ fn extract_rest_identifier(rest_node: &Node, source: &[u8], names: &mut Vec Vec { let mut names = Vec::new(); - scan_import_names(node, source, &mut names); + let mut renamed = Vec::new(); + scan_import_names(node, source, &mut names, &mut renamed); + names +} + +/// Extract import names and collect `{ local, imported }` pairs for +/// `import_specifier` nodes that rename a binding (`import { X as Y }`). +/// Mirrors `extractImportNames`'s `renamedOut` parameter in +/// src/extractors/javascript.ts (#1730). +fn extract_import_names_with_renames( + node: &Node, + source: &[u8], + renamed_out: &mut Vec, +) -> Vec { + let mut names = Vec::new(); + scan_import_names(node, source, &mut names, renamed_out); names } -fn scan_import_names(node: &Node, source: &[u8], names: &mut Vec) { - scan_import_names_depth(node, source, names, 0); +fn scan_import_names( + node: &Node, + source: &[u8], + names: &mut Vec, + renamed_out: &mut Vec, +) { + scan_import_names_depth(node, source, names, renamed_out, 0); } -fn scan_import_names_depth(node: &Node, source: &[u8], names: &mut Vec, depth: usize) { +/// Grammar note (see tree-sitter-javascript): for `import_specifier`, the +/// `name` field is *always* present — it holds the name as declared by the +/// source module. `alias` is only present for `X as Y` and holds the *local* +/// binding actually referenced by call sites in this file. Preferring `name` +/// unconditionally (as this function used to, and as the `export_specifier` +/// branch below still deliberately does — see its comment) silently drops the +/// local alias for every renamed import: call sites use `Y`, not `X` (#1730). +fn scan_import_names_depth( + node: &Node, + source: &[u8], + names: &mut Vec, + renamed_out: &mut Vec, + depth: usize, +) { if depth >= MAX_WALK_DEPTH { return; } match node.kind() { - "import_specifier" | "export_specifier" => { + "import_specifier" => { + let source_name_node = node.child_by_field_name("name"); + let alias_node = node.child_by_field_name("alias"); + let local_node = alias_node.or(source_name_node); + if let Some(local_node) = local_node { + names.push(node_text(&local_node, source).to_string()); + if let (Some(alias), Some(source_name)) = (alias_node, source_name_node) { + let alias_text = node_text(&alias, source); + let source_text = node_text(&source_name, source); + if alias_text != source_text { + renamed_out.push(RenamedImport { + local: alias_text.to_string(), + imported: source_text.to_string(), + }); + } + } + } else { + names.push(node_text(node, source).to_string()); + } + } + "export_specifier" => { + // export_specifier's `name` is the local declaration being (re-)exported; + // `alias` is the external name it's exposed as. Barrel/reexport tracing + // keys off the *original* declaration name, so this branch is + // deliberately left picking `name` first — do not unify with the + // import_specifier branch above. Rename-aware barrel tracing for + // `export { X as Y } from …` is a distinct, separate gap (#1730 + // investigation). let name_node = node .child_by_field_name("name") .or_else(|| node.child_by_field_name("alias")); @@ -3009,7 +3073,7 @@ fn scan_import_names_depth(node: &Node, source: &[u8], names: &mut Vec, } for i in 0..node.child_count() { if let Some(child) = node.child(i) { - scan_import_names_depth(&child, source, names, depth + 1); + scan_import_names_depth(&child, source, names, renamed_out, depth + 1); } } } @@ -3659,6 +3723,30 @@ mod tests { assert_eq!(s.imports[0].names, vec!["readFile"]); } + /// Regression test for #1730: `import { X as Y }` must record the *local* + /// binding (Y) in `names` — that's what call sites reference — plus the + /// `{ local: Y, imported: X }` pair in `renamed_imports` so call-edge + /// resolution can recover the original exported name X. + #[test] + fn renamed_import_records_local_name_and_rename_pair() { + let s = parse_js("import { collectFiles as collectFilesUtil } from './helpers';"); + assert_eq!(s.imports.len(), 1); + assert_eq!(s.imports[0].names, vec!["collectFilesUtil"]); + let renamed = s.imports[0] + .renamed_imports + .as_ref() + .expect("renamed_imports should be populated for a renamed specifier"); + assert_eq!(renamed.len(), 1); + assert_eq!(renamed[0].local, "collectFilesUtil"); + assert_eq!(renamed[0].imported, "collectFiles"); + } + + #[test] + fn non_renamed_import_has_no_renamed_imports() { + let s = parse_js("import { readFile } from 'fs';"); + assert!(s.imports[0].renamed_imports.is_none()); + } + #[test] fn finds_calls() { let s = parse_js("function f() { console.log('hi'); foo(); }"); diff --git a/crates/codegraph-core/src/types.rs b/crates/codegraph-core/src/types.rs index a1c3ccdc2..e41c50034 100644 --- a/crates/codegraph-core/src/types.rs +++ b/crates/codegraph-core/src/types.rs @@ -114,6 +114,16 @@ pub struct Call { pub key_expr: Option, } +/// `import { X as Y }`: the local binding name (Y) paired with the original +/// name exported by the source module (X). Mirrors TS `Import.renamedImports`. +/// See `Import.renamed_imports` for why this is needed (#1730). +#[napi(object)] +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RenamedImport { + pub local: String, + pub imported: String, +} + #[napi(object)] #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Import { @@ -125,6 +135,15 @@ pub struct Import { pub reexport: Option, #[napi(js_name = "wildcardReexport")] pub wildcard_reexport: Option, + /// For `import { X as Y }` specifiers: the local binding name (Y) mapped to + /// the original name exported by the source module (X). `names` always + /// carries the local (post-rename) binding — this field lets call-edge + /// resolution recover the *original* symbol name to look up in the + /// imported file when a call site uses the local alias (#1730). Only + /// populated for specifiers that actually rename a binding. Mirrors TS + /// `Import.renamedImports`. + #[napi(js_name = "renamedImports")] + pub renamed_imports: Option>, // Language-specific flags #[napi(js_name = "pythonImport")] pub python_import: Option, @@ -168,6 +187,7 @@ impl Import { type_only: None, reexport: None, wildcard_reexport: None, + renamed_imports: None, python_import: None, go_import: None, rust_use: None, diff --git a/src/domain/graph/builder/call-resolver.ts b/src/domain/graph/builder/call-resolver.ts index b39a158a7..68a5a2589 100644 --- a/src/domain/graph/builder/call-resolver.ts +++ b/src/domain/graph/builder/call-resolver.ts @@ -217,6 +217,7 @@ export function resolveCallTargets( importedNames: Map, typeMap: Map, callerName?: string | null, + importedOriginalNames?: ReadonlyMap, ): { targets: Array<{ id: number; file: string }>; importedFrom: string | undefined } { // Flagged dynamic calls use synthetic names like ''. Short-circuit // so they never accidentally match a real symbol via lookup.byName. @@ -225,14 +226,18 @@ export function resolveCallTargets( } const importedFrom = importedNames.get(call.name); + // When the call site uses a renamed import binding (`import { X as Y }`), + // the imported file's actual symbol is declared under the *original* name + // (X) — look that up instead of the local alias the call site wrote (#1730). + const targetName = importedOriginalNames?.get(call.name) ?? call.name; let targets: ReadonlyArray<{ id: number; file: string }> | undefined; if (importedFrom) { - targets = lookup.byNameAndFile(call.name, importedFrom); + targets = lookup.byNameAndFile(targetName, importedFrom); if (targets.length === 0 && lookup.isBarrel(importedFrom)) { - const actualSource = lookup.resolveBarrel(importedFrom, call.name); + const actualSource = lookup.resolveBarrel(importedFrom, targetName); if (actualSource) { - targets = lookup.byNameAndFile(call.name, actualSource); + targets = lookup.byNameAndFile(targetName, actualSource); } } } diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 863ef00f7..909230fb4 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -16,6 +16,7 @@ import type { BetterSqlite3Database, EngineOpts, ExtractorOutput, + Import, PathAliases, SqliteStatement, } from '../../../types.js'; @@ -209,8 +210,22 @@ function rebuildReverseDepEdges( aliases, skipBarrel ? null : db, ); - const importedNames = buildImportedNamesMap(symbols, rootDir, depRelPath, aliases, db); - edgesAdded += buildCallEdges(db, stmts, depRelPath, symbols, fileNodeRow, importedNames); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + symbols, + rootDir, + depRelPath, + aliases, + db, + ); + edgesAdded += buildCallEdges( + db, + stmts, + depRelPath, + symbols, + fileNodeRow, + importedNames, + importedOriginalNames, + ); edgesAdded += buildClassHierarchyEdges(stmts, depRelPath, symbols); return edgesAdded; } @@ -304,6 +319,23 @@ function resolveBarrelTarget( return null; } +/** + * Pairs each locally-bound name from an import statement with its original + * (pre-rename) exported name — identical to the local name unless the + * specifier renames a binding (`import { X as Y }`). Barrel tracing and + * target-file symbol lookups must search using the *original* name — the + * renamed local alias only exists in the importing file, not in the file + * being imported from (#1730). Mirrors `importNamePairs` in build-edges.ts. + */ +function importNamePairs(imp: Import): Array<{ local: string; original: string }> { + const originalNameFor = new Map(); + for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); + return imp.names.map((name) => { + const local = name.replace(/^\*\s+as\s+/, ''); + return { local, original: originalNameFor.get(local) ?? local }; + }); +} + /** * Resolve barrel imports for a single import statement and create edges to actual source files. * Shared by buildImportEdges (primary file) and Pass 2 of the reverse-dep cascade. @@ -318,9 +350,8 @@ function resolveBarrelImportEdges( let edgesAdded = 0; if (!isBarrelFile(db, resolvedPath)) return edgesAdded; const resolvedSources = new Set(); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - const actualSource = resolveBarrelTarget(db, resolvedPath, cleanName); + for (const { original } of importNamePairs(imp)) { + const actualSource = resolveBarrelTarget(db, resolvedPath, original); if (actualSource && actualSource !== resolvedPath && !resolvedSources.has(actualSource)) { resolvedSources.add(actualSource); const actualRow = stmts.getNodeId.get(actualSource, 'file', actualSource, 0); @@ -343,14 +374,13 @@ function emitTypeOnlySymbolEdges( fileNodeId: number, ): number { let edgesAdded = 0; - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (db && isBarrelFile(db, resolvedPath)) { - const actual = resolveBarrelTarget(db, resolvedPath, cleanName); + const actual = resolveBarrelTarget(db, resolvedPath, original); if (actual) targetFile = actual; } - const candidates = stmts.findNodeInFile.all(cleanName, targetFile) as Array<{ + const candidates = stmts.findNodeInFile.all(original, targetFile) as Array<{ id: number; file: string; }>; @@ -413,14 +443,23 @@ function buildImportEdges( return edgesAdded; } +/** + * Mirrors the full-build `buildImportedNamesMap` in build-edges.ts: maps each + * locally-bound import name to its defining file (`importedNames`), plus, for + * renamed specifiers (`import { X as Y }`), the *original* exported name + * (`importedOriginalNames`, keyed by local name Y). Barrel tracing and the + * downstream target-file symbol lookup must use the original name — the + * renamed local alias only exists in the importing file (#1730). + */ function buildImportedNamesMap( symbols: ExtractorOutput, rootDir: string, relPath: string, aliases: PathAliases, db: BetterSqlite3Database, -): Map { +): { importedNames: Map; importedOriginalNames: Map } { const importedNames = new Map(); + const importedOriginalNames = new Map(); for (const imp of symbols.imports) { const resolvedPath = resolveImportPath( path.join(rootDir, relPath), @@ -428,21 +467,21 @@ function buildImportedNamesMap( rootDir, aliases, ); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { local, original } of importNamePairs(imp)) { // Mirror full-build's `buildImportedNamesMap`: follow barrel re-exports so // `importedNames` maps to the *defining* file, not the barrel. This ensures // `computeConfidence` gets `importedFrom === targetFile` and returns 1.0 // instead of the cross-directory fallback (0.3). let targetFile = resolvedPath; if (isBarrelFile(db, resolvedPath)) { - const actual = resolveBarrelTarget(db, resolvedPath, cleanName); + const actual = resolveBarrelTarget(db, resolvedPath, original); if (actual) targetFile = actual; } - importedNames.set(cleanName, targetFile); + importedNames.set(local, targetFile); + if (original !== local) importedOriginalNames.set(local, original); } } - return importedNames; + return { importedNames, importedOriginalNames }; } // ── Class hierarchy edges ─────────────────────────────────────────────── @@ -690,6 +729,7 @@ function buildCallEdges( symbols: ExtractorOutput, fileNodeRow: { id: number }, importedNames: Map, + importedOriginalNames?: ReadonlyMap, ): number { const typeMap = buildIncrementalTypeMap(symbols); const seenCallEdges = new Set(); @@ -707,6 +747,7 @@ function buildCallEdges( importedNames, typeMap, caller.callerName, + importedOriginalNames, ); const targets = applyThisReceiverFallbacks( @@ -773,8 +814,22 @@ function rebuildEdgesForTargetFile( let edgesAdded = buildContainmentEdges(db, stmts, relPath, symbols); edgesAdded += rebuildDirContainment(db, stmts, relPath); edgesAdded += buildImportEdges(stmts, relPath, symbols, rootDir, fileNodeRow.id, aliases, db); - const importedNames = buildImportedNamesMap(symbols, rootDir, relPath, aliases, db); - edgesAdded += buildCallEdges(db, stmts, relPath, symbols, fileNodeRow, importedNames); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + symbols, + rootDir, + relPath, + aliases, + db, + ); + edgesAdded += buildCallEdges( + db, + stmts, + relPath, + symbols, + fileNodeRow, + importedNames, + importedOriginalNames, + ); edgesAdded += buildClassHierarchyEdges(stmts, relPath, symbols); return edgesAdded; } diff --git a/src/domain/graph/builder/stages/build-edges.ts b/src/domain/graph/builder/stages/build-edges.ts index 011ce246b..33ebafa84 100644 --- a/src/domain/graph/builder/stages/build-edges.ts +++ b/src/domain/graph/builder/stages/build-edges.ts @@ -88,7 +88,7 @@ interface NativeFileEntry { params?: string[]; }>; calls: Call[]; - importedNames: Array<{ name: string; file: string }>; + importedNames: Array<{ name: string; file: string; imported?: string }>; classes: ClassRelation[]; typeMap: Array<{ name: string; typeName: string; confidence: number }>; /** Phase 8.3: function-reference bindings for pts analysis. */ @@ -148,6 +148,23 @@ function importEdgeKind(imp: Import): string { return 'imports'; } +/** + * Pairs each locally-bound name from an import statement with its original + * (pre-rename) exported name — identical to the local name unless the + * specifier renames a binding (`import { X as Y }`). Barrel tracing and + * target-file symbol lookups must search using the *original* name — the + * renamed local alias only exists in the importing file, not in the file + * being imported from (#1730). + */ +function importNamePairs(imp: Import): Array<{ local: string; original: string }> { + const originalNameFor = new Map(); + for (const r of imp.renamedImports ?? []) originalNameFor.set(r.local, r.imported); + return imp.names.map((name) => { + const local = name.replace(/^\*\s+as\s+/, ''); + return { local, original: originalNameFor.get(local) ?? local }; + }); +} + /** * For a `import type` statement, emit symbol-level `imports-type` edges so * the target symbols get fan-in credit and aren't classified as dead code. @@ -160,14 +177,13 @@ function emitTypeOnlySymbolEdges( allEdgeRows: EdgeRowTuple[], ): void { if (!ctx.nodesByNameAndFile) return; - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (isBarrelFile(ctx, resolvedPath)) { - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, original); if (actual) targetFile = actual; } - const candidates = ctx.nodesByNameAndFile.get(`${cleanName}|${targetFile}`); + const candidates = ctx.nodesByNameAndFile.get(`${original}|${targetFile}`); if (candidates && candidates.length > 0) { allEdgeRows.push([fileNodeId, candidates[0]!.id, 'imports-type', 1.0, 0, null, null]); } @@ -233,9 +249,8 @@ function buildBarrelEdges( edgeRows: EdgeRowTuple[], ): void { const resolvedSources = new Set(); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - const actualSource = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + for (const { original } of importNamePairs(imp)) { + const actualSource = resolveBarrelExportCached(ctx, resolvedPath, original); if (actualSource && actualSource !== resolvedPath && !resolvedSources.has(actualSource)) { resolvedSources.add(actualSource); const actualRow = getNodeIdStmt.get(actualSource, 'file', actualSource, 0); @@ -474,7 +489,12 @@ function propagateReturnTypesAcrossFiles( // file rather than the barrel. This means returnTypeIndex.get(importedFrom) // now finds entries it previously missed, improving cross-file return-type // propagation through re-export chains (Phase 8.2 improvement). - const importedNamesMap = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames: importedNamesMap, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); for (const ca of symbols.callAssignments) { if (symbols.typeMap.has(ca.varName)) continue; // already resolved locally @@ -484,7 +504,11 @@ function propagateReturnTypesAcrossFiles( returnEntry = globalReturnTypeMap.get(`${ca.receiverTypeName}.${ca.calleeName}`); } else { const importedFrom = importedNamesMap.get(ca.calleeName); - if (importedFrom) returnEntry = returnTypeIndex.get(importedFrom)?.get(ca.calleeName); + // The return-type index for the imported file is keyed by the + // function's own declared name — use the original (pre-rename) name + // when the call-assignment's callee is a renamed import binding (#1730). + const calleeOriginalName = importedOriginalNames.get(ca.calleeName) ?? ca.calleeName; + if (importedFrom) returnEntry = returnTypeIndex.get(importedFrom)?.get(calleeOriginalName); } if (returnEntry) { @@ -639,7 +663,12 @@ function buildDefinePropertyPostPass( const fileNodeRow = getNodeIdStmt.get(relPath, 'file', relPath, 0); if (!fileNodeRow) continue; - const importedNames = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); const typeMap: Map = symbols.typeMap || new Map(); const definePropertyReceivers = symbols.definePropertyReceivers!; @@ -660,6 +689,7 @@ function buildDefinePropertyPostPass( importedNames, typeMap as Map, caller.callerName, + importedOriginalNames, ); if (directTargets.length > 0) continue; @@ -789,21 +819,28 @@ function buildImportedNamesForNative( relPath: string, symbols: ExtractorOutput, rootDir: string, -): Array<{ name: string; file: string }> { - const importedNames: Array<{ name: string; file: string }> = []; +): Array<{ name: string; file: string; imported?: string }> { + const importedNames: Array<{ name: string; file: string; imported?: string }> = []; // Process dynamic imports first (lower priority), then static imports // (higher priority). Rust HashMap::collect keeps the last entry per key, // so static imports win when both contribute the same name. const addImports = (imp: (typeof symbols.imports)[number]) => { const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); + for (const { local, original } of importNamePairs(imp)) { let targetFile = resolvedPath; if (isBarrelFile(ctx, resolvedPath)) { - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, original); if (actual) targetFile = actual; } - importedNames.push({ name: cleanName, file: targetFile }); + // `imported` carries the original (pre-rename) exported name so the + // native resolver can look it up in `targetFile` instead of the local + // alias, which only exists in this file (#1730). Omitted when unrenamed. + const entry: { name: string; file: string; imported?: string } = { + name: local, + file: targetFile, + }; + if (original !== local) entry.imported = original; + importedNames.push(entry); } }; for (const imp of symbols.imports) { @@ -831,7 +868,12 @@ function buildCallEdgesJS( const fileNodeRow = getNodeIdStmt.get(relPath, 'file', relPath, 0); if (!fileNodeRow) continue; - const importedNames = buildImportedNamesMap(ctx, relPath, symbols, rootDir); + const { importedNames, importedOriginalNames } = buildImportedNamesMap( + ctx, + relPath, + symbols, + rootDir, + ); const typeMap: Map = new Map( symbols.typeMap instanceof Map ? symbols.typeMap : [], ); @@ -887,48 +929,58 @@ function buildCallEdgesJS( ptsMap, chaCtx, importArtifactNames, + importedOriginalNames, ); buildClassHierarchyEdges(ctx, relPath, symbols, allEdgeRows); } } +/** + * Maps each locally-bound import name in `relPath` to the file it comes from + * (`importedNames`), plus, for renamed specifiers (`import { X as Y }`), the + * *original* exported name (`importedOriginalNames`, keyed by local name Y). + * + * Barrel tracing and downstream target-file symbol lookups must search using + * the original name — the renamed local alias only exists in the importing + * file, not in the file being imported from (#1730). + */ function buildImportedNamesMap( ctx: PipelineContext, relPath: string, symbols: ExtractorOutput, rootDir: string, -): Map { +): { importedNames: Map; importedOriginalNames: Map } { const importedNames = new Map(); - // Process dynamic imports first (lower priority), then static imports - // (higher priority). Static imports represent direct bindings while dynamic - // imports often use aliased destructuring (`{ foo: bar } = await import(…)`). - // When both contribute the same name, the static binding is authoritative. - // + const importedOriginalNames = new Map(); // Phase 8.4: trace through barrel files so that symbol names map to their // actual definition file, not the re-exporting barrel. Mirrors the tracing // already done in buildImportedNamesForNative (the native path). - const traceBarrel = (resolvedPath: string, cleanName: string): string => { + const traceBarrel = (resolvedPath: string, originalName: string): string => { if (!isBarrelFile(ctx, resolvedPath)) return resolvedPath; - const actual = resolveBarrelExportCached(ctx, resolvedPath, cleanName); + const actual = resolveBarrelExportCached(ctx, resolvedPath, originalName); return actual ?? resolvedPath; }; + const addImportNames = (imp: (typeof symbols.imports)[number], resolvedPath: string) => { + for (const { local, original } of importNamePairs(imp)) { + importedNames.set(local, traceBarrel(resolvedPath, original)); + if (original !== local) importedOriginalNames.set(local, original); + } + }; + // Process dynamic imports first (lower priority), then static imports + // (higher priority). Static imports represent direct bindings while dynamic + // imports often use aliased destructuring (`{ foo: bar } = await import(…)`). + // When both contribute the same name, the static binding is authoritative. for (const imp of symbols.imports) { if (!imp.dynamicImport) continue; const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - importedNames.set(cleanName, traceBarrel(resolvedPath, cleanName)); - } + addImportNames(imp, resolvedPath); } for (const imp of symbols.imports) { if (imp.dynamicImport) continue; const resolvedPath = getResolved(ctx, path.join(rootDir, relPath), imp.source); - for (const name of imp.names) { - const cleanName = name.replace(/^\*\s+as\s+/, ''); - importedNames.set(cleanName, traceBarrel(resolvedPath, cleanName)); - } + addImportNames(imp, resolvedPath); } - return importedNames; + return { importedNames, importedOriginalNames }; } /** @@ -1226,6 +1278,7 @@ function resolveFallbackTargets( lookup: CallNodeLookup, typeMap: Map, definePropertyReceivers: Map | undefined, + importedOriginalNames?: ReadonlyMap, ): { targets: ReadonlyArray<{ id: number; file: string; kind?: string }>; importedFrom: string | null | undefined; @@ -1245,6 +1298,7 @@ function resolveFallbackTargets( importedNames, typeMap as Map, caller.callerName, + importedOriginalNames, ); // Fallback strategies, applied in order until one yields a match. Each @@ -1403,6 +1457,7 @@ function emitPtsNoReceiverEdges( seenCallEdges: Set, ptsEdgeRows: Map, allEdgeRows: EdgeRowTuple[], + importedOriginalNames?: ReadonlyMap, ): void { const scopedPtsKey = caller.callerName != null ? `${caller.callerName}::${call.name}` : null; // Module-level calls (callerName === null) use the '' sentinel emitted by @@ -1445,6 +1500,8 @@ function emitPtsNoReceiverEdges( relPath, importedNames, typeMap as Map, + undefined, + importedOriginalNames, ); const sortedAliasTargets = aliasTargets.length > 1 @@ -1488,6 +1545,7 @@ function emitPtsReceiverEdges( seenCallEdges: Set, ptsEdgeRows: Map, allEdgeRows: EdgeRowTuple[], + importedOriginalNames?: ReadonlyMap, ): void { const receiverKey = `${call.receiver}.${call.name}`; if (!ptsMap.has(receiverKey)) return; @@ -1499,6 +1557,8 @@ function emitPtsReceiverEdges( relPath, importedNames, typeMap as Map, + undefined, + importedOriginalNames, ); const sortedAliasTargets = aliasTargets.length > 1 @@ -1621,6 +1681,7 @@ function buildFileCallEdges( ptsMap?: PointsToMap | null, chaCtx?: ChaContext, importArtifactNames?: ReadonlyMap, + importedOriginalNames?: ReadonlyMap, ): void { // Tracks edges that were inserted by the pts fallback (edgeKey → allEdgeRows index). // Kept separate from seenCallEdges so that a subsequent direct-call edge for the same @@ -1656,6 +1717,7 @@ function buildFileCallEdges( lookup, typeMap, symbols.definePropertyReceivers, + importedOriginalNames, ); // Step 2: Emit direct-call edges (upgrades any pending pts edge in-place). @@ -1687,6 +1749,7 @@ function buildFileCallEdges( seenCallEdges, ptsEdgeRows, allEdgeRows, + importedOriginalNames, ); } @@ -1712,6 +1775,7 @@ function buildFileCallEdges( seenCallEdges, ptsEdgeRows, allEdgeRows, + importedOriginalNames, ); } diff --git a/src/extractors/javascript.ts b/src/extractors/javascript.ts index 7c4b1ea9e..65326e366 100644 --- a/src/extractors/javascript.ts +++ b/src/extractors/javascript.ts @@ -311,12 +311,14 @@ function handleImportCapture(c: Record, imports: Import[ const impNode = c.imp_node!; const isTypeOnly = impNode.text.startsWith('import type'); const modPath = c.imp_source!.text.replace(/['"]/g, ''); - const names = extractImportNames(impNode); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractImportNames(impNode, renamedImports); imports.push({ source: modPath, names, line: nodeStartLine(impNode), typeOnly: isTypeOnly, + ...(renamedImports.length > 0 ? { renamedImports } : {}), }); } @@ -1478,12 +1480,14 @@ function handleImportStmt(node: TreeSitterNode, ctx: ExtractorOutput): void { const source = node.childForFieldName('source') || findChild(node, 'string'); if (source) { const modPath = source.text.replace(/['"]/g, ''); - const names = extractImportNames(node); + const renamedImports: Array<{ local: string; imported: string }> = []; + const names = extractImportNames(node, renamedImports); ctx.imports.push({ source: modPath, names, line: nodeStartLine(node), typeOnly: isTypeOnly, + ...(renamedImports.length > 0 ? { renamedImports } : {}), }); } } @@ -3843,10 +3847,45 @@ function findParentClass(node: TreeSitterNode): string | null { return findParentNode(node, JS_CLASS_TYPES); } -function extractImportNames(node: TreeSitterNode): string[] { +/** + * Extract the local binding names introduced by an import/export statement. + * + * `renamedOut`, when passed, collects `{ local, imported }` pairs for + * `import_specifier` nodes that rename a binding (`import { X as Y }`). + * + * Grammar note (see tree-sitter-javascript): for `import_specifier`, the + * `name` field is *always* present — it holds the name as declared by the + * source module. `alias` is only present for `X as Y` and holds the *local* + * binding actually referenced by call sites in this file. Preferring `name` + * unconditionally (as this function used to, and as the `export_specifier` + * branch below still deliberately does — see its comment) silently drops the + * local alias for every renamed import: call sites use `Y`, not `X` (#1730). + */ +function extractImportNames( + node: TreeSitterNode, + renamedOut?: Array<{ local: string; imported: string }>, +): string[] { const names: string[] = []; function scan(n: TreeSitterNode): void { - if (n.type === 'import_specifier' || n.type === 'export_specifier') { + if (n.type === 'import_specifier') { + const sourceNameNode = n.childForFieldName('name'); + const aliasNode = n.childForFieldName('alias'); + const localNode = aliasNode || sourceNameNode; + if (localNode) { + names.push(localNode.text); + if (aliasNode && sourceNameNode && aliasNode.text !== sourceNameNode.text) { + renamedOut?.push({ local: aliasNode.text, imported: sourceNameNode.text }); + } + } else { + names.push(n.text); + } + } else if (n.type === 'export_specifier') { + // export_specifier's `name` is the local declaration being (re-)exported; + // `alias` is the external name it's exposed as. Barrel/reexport tracing + // (resolveBarrelExport) keys off the *original* declaration name, so this + // branch is deliberately left picking `name` first — do not unify with + // the import_specifier branch above. Rename-aware barrel tracing for + // `export { X as Y } from …` is a distinct, separate gap (#1730 investigation). const nameNode = n.childForFieldName('name') || n.childForFieldName('alias'); if (nameNode) names.push(nameNode.text); else names.push(n.text); diff --git a/src/types.ts b/src/types.ts index 81fafbe03..96f7d2d46 100644 --- a/src/types.ts +++ b/src/types.ts @@ -510,6 +510,17 @@ export interface Import { reexport?: boolean; wildcardReexport?: boolean; dynamicImport?: boolean; + /** + * For `import { X as Y }` specifiers: the local binding name (Y) mapped to + * the original name exported by the source module (X). `names` always + * carries the local (post-rename) binding — this field lets call-edge + * resolution recover the *original* symbol name to look up in the imported + * file when a call site uses the local alias (#1730). Only populated for + * specifiers that actually rename a binding; entries where local === source + * name are omitted. Not populated for `export { X as Y } from …` reexports + * — barrel/reexport tracing is a distinct mechanism (see resolveBarrelExport). + */ + renamedImports?: Array<{ local: string; imported: string }>; // Language-specific flags (mutually exclusive at runtime) pythonImport?: boolean; goImport?: boolean; diff --git a/tests/integration/issue-1730-renamed-import-consumer.test.ts b/tests/integration/issue-1730-renamed-import-consumer.test.ts new file mode 100644 index 000000000..4d43843c0 --- /dev/null +++ b/tests/integration/issue-1730-renamed-import-consumer.test.ts @@ -0,0 +1,125 @@ +/** + * Regression test for #1730: consumer resolution must follow a call site + * through a renamed import specifier (`import { X as Y } from '...'`) back + * to the original exported symbol. + * + * Setup: two files. + * - helpers.js: `export function collectFiles() {...}` + * - consumer.js: `import { collectFiles as collectFilesUtil } from './helpers.js';` + * calls `collectFilesUtil()` from an exported function. + * + * Before the fix, the extractor recorded the *original* exported name + * (`collectFiles`) instead of the local alias actually used at the call site + * (`collectFilesUtil`) — so `importedNames` never had a key matching the call + * site text, and no `calls` edge was ever created. `codegraph exports + * helpers.js` reported zero consumers for a genuinely-used export. + * + * Verified on both engines — this is resolver/extractor logic mirrored in + * `crates/codegraph-core/`, so WASM and native must agree (#1730 root cause + * was duplicated in both). + */ + +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 { buildGraph } from '../../src/domain/graph/builder.js'; +import { exportsData } from '../../src/domain/queries.js'; + +const FILE_HELPERS = ` +export function collectFiles() { + return ['a.js', 'b.js']; +} +`; + +const FILE_CONSUMER = ` +import { collectFiles as collectFilesUtil } from './helpers.js'; + +export function useIt() { + return collectFilesUtil(); +} +`; + +let tmpWasm: string; +let tmpNative: string; + +beforeAll(async () => { + tmpWasm = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1730-wasm-')); + fs.writeFileSync(path.join(tmpWasm, 'helpers.js'), FILE_HELPERS); + fs.writeFileSync(path.join(tmpWasm, 'consumer.js'), FILE_CONSUMER); + + tmpNative = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1730-native-')); + fs.writeFileSync(path.join(tmpNative, 'helpers.js'), FILE_HELPERS); + fs.writeFileSync(path.join(tmpNative, 'consumer.js'), FILE_CONSUMER); + + await Promise.all([ + buildGraph(tmpWasm, { incremental: false, skipRegistry: true, engine: 'wasm' }), + buildGraph(tmpNative, { incremental: false, skipRegistry: true, engine: 'native' }), + ]); +}); + +afterAll(() => { + fs.rmSync(tmpWasm, { recursive: true, force: true }); + fs.rmSync(tmpNative, { recursive: true, force: true }); +}); + +function getCallEdges(dbPath: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.name AS src, n1.file AS src_file, n2.name AS tgt, n2.file AS tgt_file + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind = 'calls' + ORDER BY n1.name, n2.name`, + ) + .all() as Array<{ src: string; src_file: string; tgt: string; tgt_file: string }>; + } finally { + db.close(); + } +} + +describe('call-edge resolution through a renamed import specifier (#1730)', () => { + it('WASM: useIt -> collectFiles calls edge exists (resolved through the rename)', () => { + const edges = getCallEdges(path.join(tmpWasm, '.codegraph', 'graph.db')); + const edge = edges.find((e) => e.src === 'useIt' && e.tgt === 'collectFiles'); + expect(edge).toBeDefined(); + expect(edge?.tgt_file).toBe('helpers.js'); + }); + + it('Native: useIt -> collectFiles calls edge exists (resolved through the rename)', () => { + const edges = getCallEdges(path.join(tmpNative, '.codegraph', 'graph.db')); + const edge = edges.find((e) => e.src === 'useIt' && e.tgt === 'collectFiles'); + expect(edge).toBeDefined(); + expect(edge?.tgt_file).toBe('helpers.js'); + }); + + it('no spurious edge is created against a nonexistent "collectFilesUtil" symbol', () => { + for (const dbPath of [ + path.join(tmpWasm, '.codegraph', 'graph.db'), + path.join(tmpNative, '.codegraph', 'graph.db'), + ]) { + const edges = getCallEdges(dbPath); + expect(edges.find((e) => e.tgt === 'collectFilesUtil')).toBeUndefined(); + } + }); + + it('WASM: codegraph exports credits collectFiles with the renamed-import consumer', () => { + const data = exportsData('helpers.js', path.join(tmpWasm, '.codegraph', 'graph.db')); + const collectFiles = data.results.find((r: { name: string }) => r.name === 'collectFiles'); + expect(collectFiles).toBeDefined(); + expect(collectFiles.consumerCount).toBeGreaterThanOrEqual(1); + expect(collectFiles.consumers.map((c: { name: string }) => c.name)).toContain('useIt'); + }); + + it('Native: codegraph exports credits collectFiles with the renamed-import consumer', () => { + const data = exportsData('helpers.js', path.join(tmpNative, '.codegraph', 'graph.db')); + const collectFiles = data.results.find((r: { name: string }) => r.name === 'collectFiles'); + expect(collectFiles).toBeDefined(); + expect(collectFiles.consumerCount).toBeGreaterThanOrEqual(1); + expect(collectFiles.consumers.map((c: { name: string }) => c.name)).toContain('useIt'); + }); +}); diff --git a/tests/parsers/javascript.test.ts b/tests/parsers/javascript.test.ts index c825df2a9..5f9994fbe 100644 --- a/tests/parsers/javascript.test.ts +++ b/tests/parsers/javascript.test.ts @@ -130,6 +130,52 @@ describe('JavaScript parser', () => { expect(symbols.imports[0].names).toContain('bar'); }); + // Regression coverage for #1730: `import { X as Y }` must record the local + // binding (Y) — what call sites actually reference — in `names`, plus the + // `{ local, imported }` rename pair so call-edge resolution can recover the + // original exported symbol (X) when a call site uses the local alias. + describe('renamed import specifiers (#1730)', () => { + it('records the local alias, not the source name, in imports[].names', () => { + const symbols = parseJS(`import { collectFiles as collectFilesUtil } from './helpers';`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].names).toEqual(['collectFilesUtil']); + }); + + it('records the local -> original rename pair in renamedImports', () => { + const symbols = parseJS(`import { collectFiles as collectFilesUtil } from './helpers';`); + expect(symbols.imports[0].renamedImports).toEqual([ + { local: 'collectFilesUtil', imported: 'collectFiles' }, + ]); + }); + + it('does not set renamedImports for non-renamed specifiers', () => { + const symbols = parseJS(`import { foo, bar } from './baz';`); + expect(symbols.imports[0].renamedImports).toBeUndefined(); + }); + + it('handles a mix of renamed and non-renamed specifiers in one statement', () => { + const symbols = parseJS( + `import { foo, collectFiles as collectFilesUtil, bar } from './mixed';`, + ); + expect(symbols.imports[0].names).toEqual(['foo', 'collectFilesUtil', 'bar']); + expect(symbols.imports[0].renamedImports).toEqual([ + { local: 'collectFilesUtil', imported: 'collectFiles' }, + ]); + }); + + it('does not apply rename tracking to export_specifier (reexport) statements', () => { + // export_specifier semantics differ (name = local declaration being + // re-exported, alias = external name) — barrel/reexport tracing keys off + // the original declaration name, so this is intentionally left as-is. + // See resolveBarrelExport / issues filed from the #1730 investigation. + const symbols = parseJS(`export { collectFiles as friendlyName } from './helpers';`); + expect(symbols.imports).toHaveLength(1); + expect(symbols.imports[0].reexport).toBe(true); + expect(symbols.imports[0].names).toEqual(['collectFiles']); + expect(symbols.imports[0].renamedImports).toBeUndefined(); + }); + }); + it('extracts call expressions', () => { const symbols = parseJS(`import { foo } from './bar'; foo(); baz();`); expect(symbols.calls).toContainEqual(expect.objectContaining({ name: 'foo' })); From 590f5608c32ab10300b5e49a38f5c39e728aa935 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 16:26:03 -0600 Subject: [PATCH 08/12] fix: couple file_hashes updates with edge regeneration in incremental builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit insertNodes committed file_hashes for changed files in the same transaction as node insertion, before resolveImports/buildEdges rebuilt those files' edges (a separate, later stage/transaction) — in both the JS/WASM pipeline and the native Rust orchestrator. Any exception, crash, or interruption between the two left a hash that claimed a file was "up to date" while its edges still reflected an older revision. Since change-detection trusts file_hashes exclusively, that divergence was never self-healed by later builds. Defer the file_hashes commit so it only runs once edges have been rebuilt to match: - insert-nodes.ts: insertNodes no longer writes file_hashes for changed files; new commitFileHashes() does it, called from pipeline.ts after buildEdges. - insert_nodes.rs / pipeline.rs / connection.rs: do_insert_nodes no longer upserts file_hashes (only removed-file cleanup, which has no coupling risk); new commit_file_hashes() runs after Stage 7 (edges). Watch-mode's rebuildFile never wrote file_hashes at all, leaving it permanently stale after every edit — also fixed by writing/deleting the hash once a file's edges have been rebuilt or the file is deleted. Adds tests/integration/issue-1731-hash-edge-coupling.test.ts: a fault-injection test that throws inside buildEdges mid-incremental-build and asserts the hash does not advance until edges genuinely match (and that a retry self-heals), plus coverage for rebuildFile keeping file_hashes in sync with its edge rebuilds. Fixes #1731 Impact: 8 functions changed, 13 affected --- crates/codegraph-core/src/db/connection.rs | 16 +- .../src/domain/graph/builder/pipeline.rs | 25 +- .../graph/builder/stages/insert_nodes.rs | 98 ++++--- src/domain/graph/builder/incremental.ts | 26 +- src/domain/graph/builder/pipeline.ts | 10 +- .../graph/builder/stages/insert-nodes.ts | 152 ++++++----- src/domain/graph/watcher.ts | 4 + .../issue-1259-watch-call-resolution.test.ts | 4 + ...issue-1370-incremental-caller-name.test.ts | 4 + .../issue-1731-hash-edge-coupling.test.ts | 251 ++++++++++++++++++ .../integration/watcher-fk-embeddings.test.ts | 4 + tests/integration/watcher-rebuild.test.ts | 4 + 12 files changed, 497 insertions(+), 101 deletions(-) create mode 100644 tests/integration/issue-1731-hash-edge-coupling.test.ts diff --git a/crates/codegraph-core/src/db/connection.rs b/crates/codegraph-core/src/db/connection.rs index f75c34c3d..a41237b83 100644 --- a/crates/codegraph-core/src/db/connection.rs +++ b/crates/codegraph-core/src/db/connection.rs @@ -857,6 +857,14 @@ impl NativeDatabase { /// Batches are received as `serde_json::Value` and deserialized via serde so /// that `null` visibility values map to `None` instead of crashing napi's /// `Option` object conversion (#709). + /// + /// `file_hashes` is committed in its own transaction, separate from node + /// insertion (#1731) — callers that need edge-consistent hashes (i.e. the + /// standard incremental build pipeline) should pass an empty array here + /// and commit hashes themselves once resolveImports/buildEdges have + /// finished rebuilding the affected files' edges (see + /// `insertNodes.commitFileHashes` on the JS side, or + /// `insert_nodes::commit_file_hashes` for the all-Rust orchestrator). #[napi(ts_args_type = "batches: Array<{ file: string; definitions: Array<{ name: string; kind: string; line: number; endLine?: number; visibility?: string; children: Array<{ name: string; kind: string; line: number; endLine?: number; visibility?: string }> }>; exports: Array<{ name: string; kind: string; line: number }> }>, fileHashes: FileHashEntry[], removedFiles: string[]")] pub fn bulk_insert_nodes( &self, @@ -869,9 +877,13 @@ impl NativeDatabase { napi::Error::from_reason(format!("bulk_insert_nodes: invalid batches: {e}")) })?; let conn = self.conn()?; - Ok(insert_nodes::do_insert_nodes(conn, &batches, &file_hashes, &removed_files) + let insert_ok = insert_nodes::do_insert_nodes(conn, &batches, &removed_files) .inspect_err(|e| eprintln!("[NativeDatabase] bulk_insert_nodes failed: {e}")) - .is_ok()) + .is_ok(); + let hashes_ok = insert_nodes::commit_file_hashes(conn, &file_hashes) + .inspect_err(|e| eprintln!("[NativeDatabase] bulk_insert_nodes hash commit failed: {e}")) + .is_ok(); + Ok(insert_ok && hashes_ok) } /// Bulk-insert edge rows using chunked multi-value INSERT statements. diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index f191b9f66..ea4442e34 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -9,10 +9,13 @@ //! 2. Collect files (with gitignore + extension filter) //! 3. Detect changes (tiered: journal/mtime/hash) //! 4. Parse files in parallel (existing `parallel::parse_files_parallel`) -//! 5. Insert nodes (existing `insert_nodes::do_insert_nodes`) +//! 5. Insert nodes (existing `insert_nodes::do_insert_nodes`) — file_hashes +//! for changed files is NOT written here; see step 7 //! 6. Resolve imports (existing `resolve::resolve_imports_batch`) //! 6b. Re-parse barrel candidates (incremental only) -//! 7. Build import edges + call edges + barrel resolution +//! 7. Build import edges + call edges + barrel resolution, then commit +//! file_hashes for changed files (`insert_nodes::commit_file_hashes`) now +//! that their edges match this revision (#1731) //! 8. Structure metrics + role classification //! 9. Finalize (metadata, journal) @@ -505,13 +508,20 @@ pub fn run_pipeline( timing.parse_ms = t0.elapsed().as_secs_f64() * 1000.0; // ── Stage 5: Insert nodes ────────────────────────────────────────── + // file_hashes for these files is deliberately NOT written here — only + // node/edge-adjacent (contains, parameter_of) data plus removed-file hash + // cleanup. Committing a changed file's hash this early (before Stage 7 + // rebuilds its import/call edges) would let the hash claim "up to date" + // even if edge-building later fails or is interrupted, permanently + // desyncing file_hashes from the edges it's supposed to gate re-parsing + // on (#1731). The hash is committed at the end of Stage 7 instead, once + // edges genuinely match this revision — see `commit_file_hashes` below. let t0 = Instant::now(); let insert_batches = build_insert_batches(&file_symbols); let file_hashes = build_file_hash_entries(&parse_changes); let _ = crate::domain::graph::builder::stages::insert_nodes::do_insert_nodes( conn, &insert_batches, - &file_hashes, &change_result.removed, ); detect_changes::heal_metadata(conn, &change_result.metadata_updates); @@ -564,6 +574,15 @@ pub fn run_pipeline( build_and_insert_call_edges(conn, &file_symbols, &import_ctx, !change_result.is_full_build); reconnect_saved_reverse_dep_edges(conn, &saved_reverse_dep_edges); + + // Now that edges reflect this revision, commit file_hashes for the + // changed files (#1731). Deferred from Stage 5 — see the comment there. + if let Err(e) = crate::domain::graph::builder::stages::insert_nodes::commit_file_hashes( + conn, + &file_hashes, + ) { + eprintln!("[codegraph] commit_file_hashes failed: {e}"); + } timing.edges_ms = t0.elapsed().as_secs_f64() * 1000.0; // ── Stage 8: Structure + roles ───────────────────────────────────── diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs b/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs index 69b4519ab..bb81b8782 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/insert_nodes.rs @@ -94,17 +94,41 @@ fn query_node_ids( Ok(map) } +/// Insert nodes/children/containment edges, plus hash cleanup for removed +/// files. `file_hashes` for CHANGED files is intentionally NOT written here — +/// see [`commit_file_hashes`] below (#1731): committing a file's hash this +/// early (before import/call edges are built) would let the hash claim +/// "up to date" even if edge-building later fails or is interrupted. Deleting +/// a removed file's hash has no such coupling risk (the file has no edges to +/// keep in sync with), so `removed_files` is still handled immediately. pub(crate) fn do_insert_nodes( conn: &Connection, batches: &[InsertNodesBatch], - file_hashes: &[FileHashEntry], removed_files: &[String], ) -> rusqlite::Result<()> { let tx = conn.unchecked_transaction()?; insert_file_nodes(&tx, batches)?; let (contains_edges, param_of_edges) = insert_symbol_nodes(&tx, batches)?; upsert_node_batch(&tx, &contains_edges, ¶m_of_edges)?; - upsert_file_hashes(&tx, file_hashes, removed_files)?; + delete_removed_file_hashes(&tx, removed_files)?; + tx.commit() +} + +/// Commit `file_hashes` for changed files. Called separately from +/// [`do_insert_nodes`], strictly AFTER import/call edges have been rebuilt +/// for these files (#1731) — see that function's doc comment and the call +/// site in `pipeline.rs` Stage 7. Own transaction: node insertion and hash +/// commit are two distinct steps in the coupling this fix establishes, not +/// one atomic unit. +pub(crate) fn commit_file_hashes( + conn: &Connection, + file_hashes: &[FileHashEntry], +) -> rusqlite::Result<()> { + if file_hashes.is_empty() { + return Ok(()); + } + let tx = conn.unchecked_transaction()?; + upsert_file_hashes(&tx, file_hashes)?; tx.commit() } @@ -297,43 +321,57 @@ fn upsert_node_batch( Ok(()) } -/// Phase 4: upsert file hashes and remove hashes for deleted files. No-ops -/// gracefully when the `file_hashes` table has not been created yet (e.g. -/// during the initial schema migration). +/// Returns true iff the `file_hashes` table exists. Both halves of the old +/// combined upsert/delete function (now [`upsert_file_hashes`] and +/// [`delete_removed_file_hashes`]) no-op gracefully when it's absent — e.g. +/// during the initial schema migration. +fn has_file_hashes_table(tx: &rusqlite::Transaction) -> bool { + tx.prepare("SELECT 1 FROM sqlite_master WHERE type='table' AND name='file_hashes'") + .and_then(|mut s| s.query_row([], |_| Ok(true))) + .unwrap_or(false) +} + +/// Phase 4a: upsert file hashes for changed files. Split out from the +/// removed-files delete (below) so the two can be committed at different +/// points in the pipeline — see [`commit_file_hashes`] (#1731). fn upsert_file_hashes( tx: &rusqlite::Transaction, file_hashes: &[FileHashEntry], - removed_files: &[String], ) -> rusqlite::Result<()> { - let has_file_hashes = tx - .prepare("SELECT 1 FROM sqlite_master WHERE type='table' AND name='file_hashes'") - .and_then(|mut s| s.query_row([], |_| Ok(true))) - .unwrap_or(false); - - if !has_file_hashes { + if file_hashes.is_empty() || !has_file_hashes_table(tx) { return Ok(()); } - { - let mut upsert = tx.prepare_cached( - "INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) \ - VALUES (?1, ?2, ?3, ?4)", - )?; - for entry in file_hashes { - upsert.execute(params![ - &entry.file, - &entry.hash, - entry.mtime as i64, - entry.size as i64 - ])?; - } + let mut upsert = tx.prepare_cached( + "INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) \ + VALUES (?1, ?2, ?3, ?4)", + )?; + for entry in file_hashes { + upsert.execute(params![ + &entry.file, + &entry.hash, + entry.mtime as i64, + entry.size as i64 + ])?; } - if !removed_files.is_empty() { - let mut delete = tx.prepare_cached("DELETE FROM file_hashes WHERE file = ?1")?; - for file in removed_files { - delete.execute(params![file])?; - } + Ok(()) +} + +/// Phase 4b: remove file_hashes rows for deleted files. Safe to run +/// immediately alongside node insertion (unlike the upsert above) — a +/// removed file has no edges that need to stay in sync with its hash. +fn delete_removed_file_hashes( + tx: &rusqlite::Transaction, + removed_files: &[String], +) -> rusqlite::Result<()> { + if removed_files.is_empty() || !has_file_hashes_table(tx) { + return Ok(()); + } + + let mut delete = tx.prepare_cached("DELETE FROM file_hashes WHERE file = ?1")?; + for file in removed_files { + delete.execute(params![file])?; } Ok(()) diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 909230fb4..cde9e3a78 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -29,7 +29,7 @@ import { resolveReceiverEdge, resolveSameClassQualifiedMethod, } from './call-resolver.js'; -import { BUILTIN_RECEIVERS, readFileSafe } from './helpers.js'; +import { BUILTIN_RECEIVERS, fileHash, fileStat, readFileSafe } from './helpers.js'; // ── Local types ───────────────────────────────────────────────────────── @@ -42,6 +42,14 @@ export interface IncrementalStmts { listSymbols: { all: (...params: unknown[]) => unknown[] }; findNodeInFile: { all: (...params: unknown[]) => unknown[] }; findNodeByName: { all: (...params: unknown[]) => unknown[] }; + /** + * Upsert a `file_hashes` row: `(relPath, hash, mtime, size)`. Called only + * after a file's edges have been fully rebuilt (#1731) — see the call site + * in `rebuildFile` for why this can't happen any earlier. + */ + upsertFileHash: { run: (...params: unknown[]) => unknown }; + /** Delete a `file_hashes` row for a file removed from disk. */ + deleteFileHash: { run: (...params: unknown[]) => unknown }; } interface RebuildResult { @@ -955,6 +963,10 @@ export async function rebuildFile( if (!fs.existsSync(filePath)) { if (cache) (cache as { remove(p: string): void }).remove(filePath); + // The file no longer exists, so it has no edges to keep in sync with a + // hash — delete it immediately (mirrors the full-build removed-file path + // in insertNodes.ts, which is likewise unconditional). + stmts.deleteFileHash.run(relPath); return buildDeletionResult(relPath, oldNodes, edgesBefore, oldSymbols, diffSymbols); } @@ -999,6 +1011,18 @@ export async function rebuildFile( // their edges unchanged. const totalEdgesBefore = edgesBefore + reverseDepsEdgesBefore; + // Commit file_hashes now that relPath's edges have been fully rebuilt to + // match `code` (#1731). Writing this any earlier — or not at all, as + // before this fix — would leave file_hashes stale relative to the edges + // rebuildEdgesForTargetFile just wrote, so the next full/incremental + // `codegraph build` would either redundantly reprocess an already-correct + // file (stale-hash direction) or, combined with other divergent writers, + // risk trusting a hash that doesn't actually reflect these edges. + const stat = fileStat(filePath); + if (stat) { + stmts.upsertFileHash.run(relPath, fileHash(code), stat.mtime, stat.size); + } + const symbolDiff = diffSymbols ? diffSymbols(oldSymbols, newSymbols) : null; const event = oldNodes === 0 ? 'added' : 'modified'; diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index 28e4eb0df..7f3bf09b8 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -39,7 +39,7 @@ import { buildStructure } from './stages/build-structure.js'; import { collectFiles } from './stages/collect-files.js'; import { detectChanges, detectNoChanges } from './stages/detect-changes.js'; import { finalize } from './stages/finalize.js'; -import { insertNodes } from './stages/insert-nodes.js'; +import { commitFileHashes, insertNodes } from './stages/insert-nodes.js'; import { closeNativeDb, refreshJsDb, @@ -319,6 +319,14 @@ async function runPipelineStages(ctx: PipelineContext): Promise { await resolveImports(ctx); await buildEdges(ctx); + + // Commit file_hashes for changed files now that their edges have been + // rebuilt to match (#1731) — see commitFileHashes() for the rationale. + // Placed before buildStructure/runAnalyses so a failure there still + // benefits from a hash that accurately reflects the (now-consistent) + // node+edge state committed so far. + commitFileHashes(ctx); + await buildStructure(ctx); // Reopen nativeDb for feature modules (ast, cfg, complexity, dataflow). diff --git a/src/domain/graph/builder/stages/insert-nodes.ts b/src/domain/graph/builder/stages/insert-nodes.ts index c0459a2c5..0d786d249 100644 --- a/src/domain/graph/builder/stages/insert-nodes.ts +++ b/src/domain/graph/builder/stages/insert-nodes.ts @@ -2,11 +2,16 @@ * Stage: insertNodes * * Batch-inserts file nodes, definitions, exports, children, and contains/parameter_of edges. - * Updates file hashes for incremental builds. * * When the native engine is available, delegates all SQLite writes to Rust via * `bulkInsertNodes` — eliminating JS↔C boundary overhead. Falls back to the * JS implementation on failure or when native is unavailable. + * + * Does NOT write file_hashes for changed files (only removed-file cleanup, + * which carries no coupling risk — see commitFileHashes below). Hashes for + * changed files are committed later, once resolveImports/buildEdges have + * finished rebuilding their edges, so a hash can never claim a file is + * "up to date" while its edges still reflect an older revision (#1731). */ import path from 'node:path'; import { performance } from 'node:perf_hooks'; @@ -19,7 +24,6 @@ import type { ExtractorOutput, FileToParse, MetadataUpdate, - SqliteStatement, } from '../../../../types.js'; import type { PipelineContext } from '../context.js'; import { @@ -218,28 +222,26 @@ export function buildFileHashes( function tryNativeInsert(ctx: PipelineContext): boolean { if (!ctx.nativeDb?.bulkInsertNodes) return false; - const { allSymbols, filesToParse, metadataUpdates, rootDir, removed } = ctx; + const { allSymbols, removed } = ctx; const batches = marshalSymbolBatches(allSymbols); - const precomputedData = new Map(); - for (const item of filesToParse) { - if (item.relPath) precomputedData.set(item.relPath, item as PrecomputedFileData); - } - const fileHashes = buildFileHashes(filesToParse, precomputedData, metadataUpdates, rootDir); - - // In native-first mode (single rusqlite connection), no WAL dance is needed. - // In dual-connection mode, checkpoint JS side before native write, then - // checkpoint native side after (#696, #709, #715, #717). + // file_hashes is intentionally NOT written here. Committing a file's hash + // this early (before resolveImports/buildEdges have run) would let the + // hash claim "up to date" even if edge-building later throws or is + // interrupted — see commitFileHashes below, called once edges are in + // place (#1731). `removed` is still passed through: deleting a removed + // file's hash has no such coupling risk (the file has no edges to keep + // in sync with). let result: boolean; if (ctx.nativeFirstProxy) { - result = ctx.nativeDb!.bulkInsertNodes(batches, fileHashes, removed); + result = ctx.nativeDb!.bulkInsertNodes(batches, [], removed); } else { try { if (ctx.db) { ctx.db.pragma('wal_checkpoint(TRUNCATE)'); } - result = ctx.nativeDb!.bulkInsertNodes(batches, fileHashes, removed); + result = ctx.nativeDb!.bulkInsertNodes(batches, [], removed); } finally { try { ctx.nativeDb?.exec('PRAGMA wal_checkpoint(TRUNCATE)'); @@ -380,36 +382,10 @@ function insertChildrenAndEdges( batchInsertEdges(db, edgeRows); } -// ── JS fallback: Phase 4 ──────────────────────────────────────────── - -function updateFileHashes( - _db: BetterSqlite3Database, - filesToParse: FileToParse[], - precomputedData: Map, - metadataUpdates: MetadataUpdate[], - rootDir: string, - upsertHash: SqliteStatement | null, -): void { - if (!upsertHash) return; - - // Iterate every collected file (#1068): files that produced zero symbols - // (empty, parser no-op, or grammar-missing optional language) still need a - // hash row, otherwise the next no-op rebuild's fast-skip pre-flight rejects. - for (const record of iterFileHashRecords( - filesToParse, - precomputedData, - metadataUpdates, - rootDir, - 'updateFileHashes', - )) { - upsertHash.run(record.file, record.hash, record.mtime, record.size); - } -} - // ── Main entry point ──────────────────────────────────────────────── export async function insertNodes(ctx: PipelineContext): Promise { - const { allSymbols, filesToParse, metadataUpdates, rootDir, removed } = ctx; + const { allSymbols, removed } = ctx; // Populate fileSymbols before any DB writes (used by later stages) for (const [relPath, symbols] of allSymbols) { @@ -423,7 +399,9 @@ export async function insertNodes(ctx: PipelineContext): Promise { try { if (tryNativeInsert(ctx)) { ctx.timing.insertMs = performance.now() - t0; - // Removed-file hash cleanup is handled inside the native call + // Removed-file hash cleanup is handled inside the native call. + // Content-changed files' hashes are committed later by + // commitFileHashes(), once their edges exist (#1731). return; } } catch (e) { @@ -431,36 +409,82 @@ export async function insertNodes(ctx: PipelineContext): Promise { } } - // JS fallback - const precomputedData = new Map(); - for (const item of filesToParse) { - if (item.relPath) precomputedData.set(item.relPath, item as PrecomputedFileData); - } - - let upsertHash: SqliteStatement | null; - try { - upsertHash = ctx.db.prepare( - 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', - ); - } catch (e) { - debug(`insertNodes: file_hashes prepare failed (table may not exist): ${toErrorMessage(e)}`); - upsertHash = null; - } - + // JS fallback — node/edge insertion only. file_hashes for changed files is + // intentionally NOT written here: see commitFileHashes() below (#1731). const insertAll = ctx.db.transaction(() => { insertDefinitionsAndExports(ctx.db, allSymbols); insertChildrenAndEdges(ctx.db, allSymbols); - updateFileHashes(ctx.db, filesToParse, precomputedData, metadataUpdates, rootDir, upsertHash); }); insertAll(); ctx.timing.insertMs = performance.now() - t0; - // Clean up removed file hashes - if (upsertHash && removed.length > 0) { - const deleteHash = ctx.db.prepare('DELETE FROM file_hashes WHERE file = ?'); - for (const relPath of removed) { - deleteHash.run(relPath); + // Clean up removed file hashes. Safe to do immediately (unlike the upsert + // path): a removed file has no edges that need to stay in sync with its hash. + if (removed.length > 0) { + try { + const deleteHash = ctx.db.prepare('DELETE FROM file_hashes WHERE file = ?'); + for (const relPath of removed) { + deleteHash.run(relPath); + } + } catch (e) { + debug( + `insertNodes: removed-file hash cleanup failed (table may not exist): ${toErrorMessage(e)}`, + ); + } + } +} + +// ── Deferred file_hashes commit ────────────────────────────────────── + +/** + * Commit `file_hashes` for every changed/parsed file, plus metadata-only + * healing entries. Called by the pipeline strictly AFTER resolveImports and + * buildEdges have finished rebuilding those files' edges. + * + * This is the fix for #1731: previously, `insertNodes` wrote file_hashes in + * the same transaction as node insertion — BEFORE resolveImports/buildEdges + * ran. Any exception, crash, or interruption between that write and the + * (separate) edge-building transaction(s) left the DB with a hash that + * claimed "up to date" while edges still reflected the previous revision (or + * were missing entirely) — and since change-detection trusts file_hashes + * exclusively, that divergence was never self-healed by later builds. + * + * Deferring the write here restores the invariant: a file's hash only ever + * advances once its edges have been rebuilt to match. If anything upstream + * throws before this point, the hash keeps its old value, so the next build + * correctly detects the file as still needing (re)processing. + */ +export function commitFileHashes(ctx: PipelineContext): void { + const { filesToParse, metadataUpdates, rootDir } = ctx; + + const precomputedData = new Map(); + for (const item of filesToParse) { + if (item.relPath) precomputedData.set(item.relPath, item as PrecomputedFileData); + } + const fileHashes = buildFileHashes(filesToParse, precomputedData, metadataUpdates, rootDir); + if (fileHashes.length === 0) return; + + if (ctx.engineName === 'native' && ctx.nativeDb?.healFileMetadata) { + try { + ctx.nativeDb.healFileMetadata(fileHashes); + } catch (e) { + debug(`commitFileHashes: native healFileMetadata failed: ${toErrorMessage(e)}`); } + return; + } + + try { + const upsertHash = ctx.db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ); + const commitTx = ctx.db.transaction(() => { + for (const record of fileHashes) { + upsertHash.run(record.file, record.hash, record.mtime, record.size); + } + }); + commitTx(); + } catch (e) { + debug(`commitFileHashes: file_hashes write failed (table may not exist): ${toErrorMessage(e)}`); } } diff --git a/src/domain/graph/watcher.ts b/src/domain/graph/watcher.ts index 703a45247..97b86d066 100644 --- a/src/domain/graph/watcher.ts +++ b/src/domain/graph/watcher.ts @@ -44,6 +44,10 @@ function prepareWatcherStatements(db: ReturnType): IncrementalStm "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1259-watch-call-resolution.test.ts b/tests/integration/issue-1259-watch-call-resolution.test.ts index affbf8c85..78e563c33 100644 --- a/tests/integration/issue-1259-watch-call-resolution.test.ts +++ b/tests/integration/issue-1259-watch-call-resolution.test.ts @@ -99,6 +99,10 @@ function makeStmts(db: ReturnType) { "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1370-incremental-caller-name.test.ts b/tests/integration/issue-1370-incremental-caller-name.test.ts index 8751d449b..ecdca2079 100644 --- a/tests/integration/issue-1370-incremental-caller-name.test.ts +++ b/tests/integration/issue-1370-incremental-caller-name.test.ts @@ -95,6 +95,10 @@ function makeStmts(db: ReturnType) { "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } diff --git a/tests/integration/issue-1731-hash-edge-coupling.test.ts b/tests/integration/issue-1731-hash-edge-coupling.test.ts new file mode 100644 index 000000000..0fee9b45b --- /dev/null +++ b/tests/integration/issue-1731-hash-edge-coupling.test.ts @@ -0,0 +1,251 @@ +/** + * Regression test for #1731: `file_hashes` must never claim a file is + * "up to date" while its edges still reflect an older revision. + * + * Root cause: `insertNodes` committed `file_hashes` for changed files in the + * same transaction as node insertion — BEFORE `resolveImports`/`buildEdges` + * had rebuilt those files' edges (a separate, later stage/transaction). Any + * exception thrown while rebuilding edges left the DB with a hash that + * matched the file's CURRENT on-disk content while its edges still reflected + * the PREVIOUS content (or were missing entirely, since the old edges had + * already been purged). Because change-detection trusts `file_hashes` + * exclusively, that divergence was never self-healed by later builds — the + * file would be silently skipped forever, permanently showing stale/missing + * edges via `codegraph deps` / `where --file`. + * + * The fix defers the `file_hashes` commit so it only happens once + * `resolveImports`/`buildEdges` have finished rebuilding a file's edges + * (`commitFileHashes` in `insert-nodes.ts`, called from the pipeline after + * `buildEdges`). Separately, watch-mode's `rebuildFile` never wrote + * `file_hashes` at all — also fixed here, coupling the write to a + * successful edge rebuild. + * + * This file has two suites: + * 1. Fault-injects an exception inside `buildEdges` during an incremental + * build (the adversarial case that would have caught the original bug) + * and asserts the hash does not advance until edges genuinely match. + * 2. Exercises `rebuildFile` (the watch-mode path) directly and asserts it + * now keeps `file_hashes` in sync with the edges it rebuilds. + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { getNodeId as getNodeIdQuery, initSchema, openDb } from '../../src/db/index.js'; +import { fileHash } from '../../src/domain/graph/builder/helpers.js'; +import { rebuildFile } from '../../src/domain/graph/builder/incremental.js'; +import { buildGraph } from '../../src/domain/graph/builder.js'; + +// ── Fault injection for suite 1 ────────────────────────────────────────── +// A `vi.hoisted` object is used (rather than a plain module-scope `let`) so +// the mutable flag is visible both inside the hoisted `vi.mock` factory and +// in the test body that arms/disarms it. +const injection = vi.hoisted(() => ({ armed: false })); + +vi.mock('../../src/domain/graph/builder/stages/build-edges.js', async (importOriginal) => { + const mod = + await importOriginal(); + return { + ...mod, + buildEdges: async (ctx: Parameters[0]) => { + if (injection.armed) { + injection.armed = false; + throw new Error('simulated buildEdges failure (#1731 regression test)'); + } + return mod.buildEdges(ctx); + }, + }; +}); + +// ── Shared fixture helpers ──────────────────────────────────────────────── + +/** Writes a.js importing from whichever of b.js/c.js are named in `imports`. */ +function writeProject(dir: string, imports: string[]) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'b.js'), 'export function b() { return 1; }\n'); + fs.writeFileSync(path.join(dir, 'c.js'), 'export function c() { return 2; }\n'); + const importLines = imports.map((m) => `import { ${m} } from './${m}.js';`).join('\n'); + const body = imports.map((m) => `${m}()`).join(' + ') || '0'; + fs.writeFileSync( + path.join(dir, 'a.js'), + `${importLines}\nexport function run() { return ${body}; }\n`, + ); +} + +function readFileHashRow(dbPath: string, file: string): { hash: string } | undefined { + const db = new Database(dbPath, { readonly: true }); + try { + return db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file) as + | { hash: string } + | undefined; + } finally { + db.close(); + } +} + +function readImportEdgeTargets(dbPath: string, file: string): string[] { + const db = new Database(dbPath, { readonly: true }); + try { + return ( + db + .prepare( + `SELECT n2.file AS tgt FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE n1.file = ? AND n1.kind = 'file' AND e.kind = 'imports' + ORDER BY n2.file`, + ) + .all(file) as Array<{ tgt: string }> + ).map((r) => r.tgt); + } finally { + db.close(); + } +} + +// ── Suite 1: incremental build pipeline ────────────────────────────────── + +describe('Issue #1731: file_hashes/edges coupling survives a mid-build failure', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1731-')); + injection.armed = false; + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('does not commit file_hashes when buildEdges throws mid-incremental-build, and self-heals on retry', async () => { + writeProject(tmpDir, ['b', 'c']); + // engine: 'wasm' pins this to the JS pipeline (insertNodes/resolveImports/ + // buildEdges as separate stages) — the code path this fix changes. The + // native orchestrator runs the equivalent pipeline entirely in Rust and + // isn't reachable via a mocked JS module. + await buildGraph(tmpDir, { incremental: false, skipRegistry: true, engine: 'wasm' }); + + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const originalHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(originalHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js', 'c.js']); + + // Edit a.js to drop its import of c.js. + writeProject(tmpDir, ['b']); + const editedHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(editedHash).not.toBe(originalHash); + + // Arm the fault injection and run the incremental build — it must reject + // partway through, after node insertion but before edges are rebuilt. + injection.armed = true; + await expect( + buildGraph(tmpDir, { incremental: true, skipRegistry: true, engine: 'wasm' }), + ).rejects.toThrow(/simulated buildEdges failure/); + + // The hash must NOT have advanced to match the edited content. If it + // had (the pre-fix behavior), the file would be permanently stuck: the + // next build would see the hash as "up to date" and skip reprocessing + // it forever, leaving its edges stale or missing. + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(originalHash); + + // Retry without fault injection — the resulting hash/content mismatch + // must be detected so the file gets fully reprocessed. + injection.armed = false; + await buildGraph(tmpDir, { incremental: true, skipRegistry: true, engine: 'wasm' }); + + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(editedHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js']); + }); +}); + +// ── Suite 2: watch-mode rebuildFile ─────────────────────────────────────── + +function makeStmts(db: ReturnType) { + return { + insertNode: db.prepare( + 'INSERT OR IGNORE INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ), + getNodeId: { + get: (name: string, kind: string, file: string, line: number) => { + const id = getNodeIdQuery(db, name, kind, file, line); + return id != null ? { id } : undefined; + }, + }, + insertEdge: db.prepare( + 'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, ?, ?)', + ), + countNodes: db.prepare('SELECT COUNT(*) as c FROM nodes WHERE file = ?'), + countEdges: db.prepare( + 'SELECT COUNT(*) as c FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?)', + ), + findNodeInFile: db.prepare( + "SELECT id, kind, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant') AND file = ?", + ), + findNodeByName: db.prepare( + "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", + ), + listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), + }; +} + +describe('Issue #1731: watch-mode rebuildFile keeps file_hashes in sync with edges', () => { + let tmpDir: string; + let dbPath: string; + + beforeEach(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1731-watch-')); + writeProject(tmpDir, ['b', 'c']); + await buildGraph(tmpDir, { incremental: false, skipRegistry: true, engine: 'wasm' }); + dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('updates file_hashes to match the edited content after a rebuild, not just the edges', async () => { + const originalHash = readFileHashRow(dbPath, 'a.js')?.hash; + expect(originalHash).toBeTruthy(); + + // Edit a.js to drop its import of c.js, then rebuild via the watcher's + // single-file path. + writeProject(tmpDir, ['b']); + const editedHash = fileHash(fs.readFileSync(path.join(tmpDir, 'a.js'), 'utf-8')); + expect(editedHash).not.toBe(originalHash); + + const db = openDb(dbPath); + try { + initSchema(db); + const stmts = makeStmts(db); + await rebuildFile(db, tmpDir, path.join(tmpDir, 'a.js'), stmts, { engine: 'wasm' }, null); + } finally { + db.close(); + } + + // Before the fix, rebuildFile rebuilt edges correctly but never touched + // file_hashes at all, leaving it permanently stuck at originalHash. + expect(readFileHashRow(dbPath, 'a.js')?.hash).toBe(editedHash); + expect(readImportEdgeTargets(dbPath, 'a.js')).toEqual(['b.js']); + }); + + it('removes the file_hashes row when the file is deleted', async () => { + expect(readFileHashRow(dbPath, 'a.js')).toBeTruthy(); + + fs.rmSync(path.join(tmpDir, 'a.js')); + + const db = openDb(dbPath); + try { + initSchema(db); + const stmts = makeStmts(db); + await rebuildFile(db, tmpDir, path.join(tmpDir, 'a.js'), stmts, { engine: 'wasm' }, null); + } finally { + db.close(); + } + + expect(readFileHashRow(dbPath, 'a.js')).toBeUndefined(); + }); +}); diff --git a/tests/integration/watcher-fk-embeddings.test.ts b/tests/integration/watcher-fk-embeddings.test.ts index 6d75cf55d..2bac2c16d 100644 --- a/tests/integration/watcher-fk-embeddings.test.ts +++ b/tests/integration/watcher-fk-embeddings.test.ts @@ -54,6 +54,10 @@ function makeStmts(db: Database.Database): Parameters[3] { "SELECT id, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), } as Parameters[3]; } diff --git a/tests/integration/watcher-rebuild.test.ts b/tests/integration/watcher-rebuild.test.ts index 95c286a72..5b92d486d 100644 --- a/tests/integration/watcher-rebuild.test.ts +++ b/tests/integration/watcher-rebuild.test.ts @@ -77,6 +77,10 @@ function makeStmts(db) { "SELECT id, kind, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", ), listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), }; } From d3ea4a498d329627cc14141131a23962a4aeba45 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 16:41:09 -0600 Subject: [PATCH 09/12] fix: compare signature-change diffs using new-file line ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkNoSignatureChanges compared def.line (post-change, new-file coordinates, from the rebuilt graph) against diff.oldRanges (pre-change, old-file coordinates). Any diff that shifts line counts before a given point in a file — virtually all deletions/insertions — could make an untouched symbol's new line number coincidentally fall inside the old hunk's range, producing a false "signature change" violation. checkMaxBlastRadius already used diff.changedRanges (new-file positions) against the same db; checkNoSignatureChanges now does the same. The parameter is renamed from oldRanges to changedRanges to make the expected coordinate space explicit. diff.oldRanges remains computed by parseDiffOutput and is still exercised directly by its own tests. No README/CLAUDE.md/ROADMAP updates needed — internal predicate logic fix, no new commands, languages, or architecture changes (docs check acknowledged). Fixes #1732 Fixes #1737 Impact: 2 functions changed, 5 affected --- src/features/check.ts | 14 ++++- tests/integration/check.test.ts | 97 +++++++++++++++++++++++++++++---- 2 files changed, 96 insertions(+), 15 deletions(-) diff --git a/src/features/check.ts b/src/features/check.ts index 2e9fc9df6..26f057e98 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -300,14 +300,22 @@ interface SignatureResult { violations: SignatureViolation[]; } +/** + * `db` reflects the current working-tree (post-change) file content, so + * `nodes.line` values are in new-file coordinates. `changedRanges` must be + * in the same coordinate space — i.e. `diff.changedRanges`, not + * `diff.oldRanges` (which is pre-change/old-file and would only line up + * with `db` by coincidence once a hunk changes the file's total line + * count). See issues #1732 and #1737. + */ export function checkNoSignatureChanges( db: BetterSqlite3Database, - oldRanges: Map, + changedRanges: Map, noTests: boolean, ): SignatureResult { const violations: SignatureViolation[] = []; - for (const [file, ranges] of oldRanges) { + for (const [file, ranges] of changedRanges) { if (ranges.length === 0) continue; if (noTests && isTestFile(file)) continue; @@ -491,7 +499,7 @@ function runPredicates( if (flags.enableSignatures) { predicates.push({ name: 'signatures', - ...checkNoSignatureChanges(db, diff.oldRanges, noTests), + ...checkNoSignatureChanges(db, diff.changedRanges, noTests), }); } if (flags.enableBoundaries) { diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index 3fc78fbbc..aa3546fa8 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -281,23 +281,23 @@ describe('checkMaxBlastRadius', () => { describe('checkNoSignatureChanges', () => { test('passes for body-only changes', () => { // add is at line 1. Changing lines 3-5 (body only) should pass - const oldRanges = new Map([['src/math.js', [{ start: 3, end: 5 }]]]); - const result = checkNoSignatureChanges(db, oldRanges, false); + const changedRanges = new Map([['src/math.js', [{ start: 3, end: 5 }]]]); + const result = checkNoSignatureChanges(db, changedRanges, false); expect(result.passed).toBe(true); }); test('fails when declaration line is in a changed hunk', () => { // add is at line 1. Changing lines 1-2 (includes declaration) should fail - const oldRanges = new Map([['src/math.js', [{ start: 1, end: 2 }]]]); - const result = checkNoSignatureChanges(db, oldRanges, false); + const changedRanges = new Map([['src/math.js', [{ start: 1, end: 2 }]]]); + const result = checkNoSignatureChanges(db, changedRanges, false); expect(result.passed).toBe(false); expect(result.violations.length).toBeGreaterThanOrEqual(1); expect(result.violations[0].name).toBe('add'); }); test('skips test files when noTests is true', () => { - const oldRanges = new Map([['tests/math.test.js', [{ start: 1, end: 5 }]]]); - const result = checkNoSignatureChanges(db, oldRanges, true); + const changedRanges = new Map([['tests/math.test.js', [{ start: 1, end: 5 }]]]); + const result = checkNoSignatureChanges(db, changedRanges, true); expect(result.passed).toBe(true); }); @@ -306,15 +306,15 @@ describe('checkNoSignatureChanges', () => { // exact "adopt shared helper, drop the file-local duplicate" pattern // grind performs — must not trip this check: every caller of a // private helper lives in the same file and is already part of the diff. - const oldRanges = new Map([['src/math.js', [{ start: 14, end: 16 }]]]); - const result = checkNoSignatureChanges(db, oldRanges, false); + const changedRanges = new Map([['src/math.js', [{ start: 14, end: 16 }]]]); + const result = checkNoSignatureChanges(db, changedRanges, false); expect(result.passed).toBe(true); }); test('still flags an exported symbol even when a private symbol shares the file', () => { // Sanity check that the exported-only filter doesn't accidentally // suppress real violations on exported declarations in the same file. - const oldRanges = new Map([ + const changedRanges = new Map([ [ 'src/math.js', [ @@ -323,7 +323,7 @@ describe('checkNoSignatureChanges', () => { ], ], ]); - const result = checkNoSignatureChanges(db, oldRanges, false); + const result = checkNoSignatureChanges(db, changedRanges, false); expect(result.passed).toBe(false); expect(result.violations.map((v) => v.name)).toEqual(['add']); }); @@ -348,12 +348,85 @@ describe('checkNoSignatureChanges', () => { ' }', // context, old line 9 ].join('\n'); - const { oldRanges } = parseDiffOutput(diff); + const { oldRanges, changedRanges } = parseDiffOutput(diff); + // parseDiffOutput's old-side tracking is still verified directly here... expect(oldRanges.get('src/math.js')).toEqual([{ start: 6, end: 6 }]); - const result = checkNoSignatureChanges(db, oldRanges, false); + // ...but checkNoSignatureChanges itself is driven by changedRanges + // (new-file coordinates). This hunk has no added lines, so there is + // nothing to compare against and multiply is correctly left alone. + const result = checkNoSignatureChanges(db, changedRanges, false); expect(result.passed).toBe(true); }); + + test('regression: coordinate-space mismatch does not falsely flag an untouched function after a shifting deletion (issues #1732, #1737)', () => { + // Real-world repro shape: an exported function sits right after an + // 11-line block that gets deleted outright. The db reflects the + // POST-change file (the block is gone, the function shifted up 11 + // lines), while the diff's old-side range still spans the pre-change + // line numbers. Before the fix, checkNoSignatureChanges was wired to + // `oldRanges`, so the function's new (post-change) line would + // coincidentally fall inside the old hunk's numeric range and get + // falsely flagged, even though its body is byte-for-byte identical. + insertNode(db, 'isPidAlive', 'function', 'src/coordshift.js', 2, 4, 1); + + const diff = [ + '--- a/src/coordshift.js', + '+++ b/src/coordshift.js', + '@@ -2,11 +1,0 @@', + '-function helperToRemove() {', + '- return 1;', + '-}', + '-', + '-function alsoUnused() {', + '- return 2;', + '-}', + '-', + '-function oneMoreUnused() {', + '- return 3;', + '-}', + ].join('\n'); + + const parsed = parseDiffOutput(diff); + // Pure deletion: the old side covers exactly the 11 removed lines; the + // new side has nothing added, so changedRanges is empty for this file. + expect(parsed.oldRanges.get('src/coordshift.js')).toEqual([{ start: 2, end: 12 }]); + expect(parsed.changedRanges.get('src/coordshift.js')).toEqual([]); + + // Prove the regression is real: had the call site still passed + // oldRanges, isPidAlive's post-change line (2) falls inside the old + // range [2, 12] and would be wrongly flagged. + const buggyResult = checkNoSignatureChanges(db, parsed.oldRanges, false); + expect(buggyResult.passed).toBe(false); + expect(buggyResult.violations.map((v) => v.name)).toContain('isPidAlive'); + + // The fix: changedRanges is empty for a pure deletion, so there is + // nothing to compare against and isPidAlive is correctly left alone. + const fixedResult = checkNoSignatureChanges(db, parsed.changedRanges, false); + expect(fixedResult.passed).toBe(true); + }); + + test('a genuinely touched exported function IS flagged via changedRanges (issues #1732, #1737)', () => { + // Complements the untouched-function case above: when a hunk actually + // replaces a declaration line, the post-change db line falls inside + // the *new*-side range and must still be flagged. + insertNode(db, 'someExportedFn', 'function', 'src/coordshift2.js', 1, 3, 1); + + const diff = [ + '--- a/src/coordshift2.js', + '+++ b/src/coordshift2.js', + '@@ -1,1 +1,1 @@', + '-function someExportedFn() {', + '+function someExportedFn(extra) {', + ].join('\n'); + + const { changedRanges } = parseDiffOutput(diff); + expect(changedRanges.get('src/coordshift2.js')).toEqual([{ start: 1, end: 1 }]); + + const result = checkNoSignatureChanges(db, changedRanges, false); + expect(result.passed).toBe(false); + expect(result.violations.map((v) => v.name)).toContain('someExportedFn'); + }); }); // ─── checkNoBoundaryViolations ──────────────────────────────────────── From c30681218d221e8f99f5a57bb6c205cd61c8de2f Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 17:54:07 -0600 Subject: [PATCH 10/12] feat: add environment doctor check for stale native binary and missing WASM grammars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every git worktree gets its own untracked node_modules/ and grammars/, so a worktree set up before a host Node upgrade (or where npm install was interrupted) can end up with a better-sqlite3 binary compiled for the wrong Node ABI, or an incomplete grammars/ directory — both fail deep inside a build or test run with confusing, unrelated-looking errors rather than a clear diagnosis. Adds src/infrastructure/doctor.ts with two checks whose decision logic is pure and unit-tested in isolation from the I/O: - native ABI compatibility, via a real require() attempt - grammar completeness against the full LANGUAGE_REGISTRY, split by the registry's own required flag: a missing required (JS/TS/TSX) grammar fails the check, but a missing optional grammar only warns. Non-required parsers are designed to fail gracefully at runtime (per this repo's own CLAUDE.md), so a worktree missing one rarely-used language's grammar must stay able to run npm test, not get hard-blocked before a single test starts. scripts/doctor.ts is the CLI entry point (npm run doctor, optionally with --fix for a scoped, worktree-local repair covering both blocking and non-blocking findings) and is also wired as pretest so npm test fails fast on a genuinely blocking problem instead of a wall of unrelated failures. Fixes #1733 docs check acknowledged: CLAUDE.md already covers the new npm run doctor command and infrastructure/doctor.ts (added in this same change); README.md does not document npm-run dev scripts (only the shipped codegraph CLI), and ROADMAP.md has no phase this bug-fix-sized change affects. Impact: 6 functions changed, 3 affected --- CLAUDE.md | 2 + CONTRIBUTING.md | 11 ++ package.json | 2 + scripts/doctor.ts | 112 +++++++++++++ src/domain/parser.ts | 10 +- src/infrastructure/doctor.ts | 291 ++++++++++++++++++++++++++++++++ tests/unit/doctor.test.ts | 316 +++++++++++++++++++++++++++++++++++ 7 files changed, 743 insertions(+), 1 deletion(-) create mode 100644 scripts/doctor.ts create mode 100644 src/infrastructure/doctor.ts create mode 100644 tests/unit/doctor.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index cb147dac9..f309cace4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,6 +74,7 @@ npm run test:coverage # Coverage report npx vitest run tests/parsers/javascript.test.ts # Single test file npx vitest run -t "finds cycles" # Single test by name npm run build:wasm # Rebuild WASM grammars from devDeps (built automatically on npm install) +npm run doctor # Check this worktree for a stale native binary / missing WASM grammars (runs automatically via pretest) ``` **Linter/Formatter:** [Biome](https://biomejs.dev/) — config in `biome.json`, scoped to `src/` and `tests/`. @@ -103,6 +104,7 @@ Source is TypeScript in `src/`, compiled via `tsup`. The Rust native engine live | `shared/paginate.ts` | Pagination helpers for bounded query results | | **`infrastructure/`** | **Platform and I/O plumbing** | | `infrastructure/config.ts` | `.codegraphrc.json` loading, env overrides, `apiKeyCommand` secret resolution | +| `infrastructure/doctor.ts` | Environment health checks — stale `better-sqlite3` native ABI, incomplete `grammars/`; see `npm run doctor` | | `infrastructure/logger.ts` | Structured logging (`warn`, `debug`, `info`, `error`) | | `infrastructure/native.ts` | Native napi-rs addon loader with WASM fallback | | `infrastructure/registry.ts` | Global repo registry (`~/.codegraph/registry.json`) for multi-repo MCP | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d6d883298..c51b8b470 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -17,6 +17,16 @@ npm test # run the full test suite **Requirements:** Node.js >= 20 +**Working in multiple git worktrees?** Each worktree gets its own untracked +`node_modules/` and `grammars/` — neither is shared via git — so every fresh +`git worktree add` needs its own `npm install`. A worktree set up before a +host Node upgrade, or where `npm install` was interrupted, can be left with a +`better-sqlite3` binary compiled for the wrong Node ABI or an incomplete +`grammars/` directory; both fail in confusing ways deep inside a build or test +run. Run `npm run doctor` to check (or `npm run doctor -- --fix` to repair +in place, scoped to the current worktree) — it also runs automatically before +`npm test` via `pretest`. + ## Contributor License Agreement (CLA) All contributors must sign the [Contributor License Agreement](CLA.md) before @@ -88,6 +98,7 @@ npm run test:coverage # Coverage report npx vitest run tests/parsers/go.test.js # Single test file npx vitest run -t "finds cycles" # Single test by name npm run build:wasm # Rebuild WASM grammars +npm run doctor # Check for a stale native binary / missing WASM grammars ``` ## Branch Naming Convention diff --git a/package.json b/package.json index 1d51ba687..0ea841824 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,8 @@ "build:wasm": "node scripts/node-ts.js scripts/build-wasm.ts", "typecheck": "tsc --noEmit", "verify-imports": "node scripts/node-ts.js scripts/verify-imports.ts", + "doctor": "node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/doctor.ts", + "pretest": "npm run doctor", "test": "vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", diff --git a/scripts/doctor.ts b/scripts/doctor.ts new file mode 100644 index 000000000..1dfeb6674 --- /dev/null +++ b/scripts/doctor.ts @@ -0,0 +1,112 @@ +#!/usr/bin/env node +/** + * codegraph environment doctor (issue #1733). + * + * Detects two classes of silent per-worktree environment drift: a stale + * better-sqlite3 native binary (ABI mismatch after a Node upgrade) and an + * incomplete grammars/ directory (interrupted or skipped `npm run build:wasm`). + * Both are untracked, worktree-local state — see CLAUDE.md "Parallel + * Sessions" — so every fresh `git worktree add` needs this checked at least + * once, and a long-lived worktree needs it re-checked after a host Node + * upgrade. + * + * Usage: + * npm run doctor # report only; exits 1 only on a blocking ('fail') check + * npm run doctor -- --fix # also run the scoped fix command(s), then re-check + * + * A missing *optional*-language grammar reports as a non-blocking WARN, not + * FAIL — this repo's own parsers are designed to degrade gracefully when a + * non-required grammar is unavailable (see CLAUDE.md), so `npm test` must + * stay runnable in that case, just with narrower language coverage. + * + * Also wired as the `pretest` lifecycle script (report-only, no --fix) so + * `npm test` fails fast with one actionable message instead of a wall of + * unrelated-looking failures scattered across the suite — but only for a + * genuinely blocking problem (stale native binary, missing required grammar). + * + * `--fix` is opt-in rather than automatic: the fixes themselves (`npm rebuild + * better-sqlite3`, `npm run build:wasm`) can take anywhere from seconds to + * over a minute, so running them unattended on every `npm test` would add + * unpredictable latency to a hot dev-loop command. Detect-and-report is the + * safe default; healing is one explicit flag away. Both fix commands always + * run with cwd pinned to this script's own repo root (never process.cwd()), + * so they can never touch a different worktree or a global install. + */ +import { execFileSync } from 'node:child_process'; +import os from 'node:os'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { runDoctorChecks } from '../src/infrastructure/doctor.ts'; + +// doctor.ts's DoctorCheck/DoctorReport interfaces are intentionally not +// exported (nothing outside that module constructs one independently), so +// this derives the shapes structurally rather than importing them by name. +type DoctorReport = ReturnType; +type DoctorCheck = DoctorReport['checks'][number]; + +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); + +// npm on Windows is npm.cmd; Node refuses to spawn .cmd/.bat without a shell. +const NPM_SHELL = os.platform() === 'win32'; + +const STATUS_MARK: Record = { + ok: 'OK ', + warn: 'WARN', + fail: 'FAIL', +}; + +function printReport(report: DoctorReport): void { + for (const check of report.checks) { + console.log(`[${STATUS_MARK[check.status]}] ${check.label}: ${check.detail}`); + if (check.status !== 'ok' && check.fixCommand) { + console.log(` fix: ${check.fixCommand}`); + } + } +} + +/** Run a non-ok check's fix command, scoped to this repo's own root. */ +function runFix(check: DoctorCheck): void { + if (!check.fixCommand) return; + const [cmd, ...args] = check.fixCommand.split(' '); + if (!cmd) return; + console.log(`\n> ${check.fixCommand}`); + try { + execFileSync(cmd, args, { cwd: repoRoot, stdio: 'inherit', shell: NPM_SHELL }); + } catch (err) { + console.error(` fix command failed: ${(err as Error).message}`); + } +} + +const shouldFix = process.argv.includes('--fix'); + +let report = runDoctorChecks(); +printReport(report); + +// --fix repairs anything with something to fix — a non-blocking 'warn' (e.g. +// a missing optional grammar) is still worth fixing when explicitly asked, +// even though it wouldn't block pretest/npm test on its own. +const needsFix = report.checks.filter((c) => c.status !== 'ok'); +if (shouldFix && needsFix.length > 0) { + console.log(`\n--fix passed — attempting scoped repairs in ${repoRoot}`); + for (const check of needsFix) runFix(check); + + console.log('\nRe-checking...'); + report = runDoctorChecks(); + printReport(report); +} + +if (!report.ok) { + console.error('\ncodegraph doctor: environment is NOT healthy — see fix command(s) above.'); + if (!shouldFix) { + console.error('Re-run with --fix to attempt an automatic, worktree-scoped repair:'); + console.error(' npm run doctor -- --fix'); + } + process.exit(1); +} + +const hasWarnings = report.checks.some((c) => c.status === 'warn'); +console.log( + hasWarnings + ? '\ncodegraph doctor: environment healthy (non-blocking warnings above).' + : '\ncodegraph doctor: environment healthy.', +); diff --git a/src/domain/parser.ts b/src/domain/parser.ts index 2042c2099..5208222c2 100644 --- a/src/domain/parser.ts +++ b/src/domain/parser.ts @@ -105,8 +105,16 @@ import { const __dirname = path.dirname(fileURLToPath(import.meta.url)); +/** + * Absolute path to the `grammars/` directory holding pre-built WASM grammar + * files. Exported so other modules (e.g. `infrastructure/doctor.ts`) can + * verify installation completeness without duplicating this relative-path + * computation. + */ +export const GRAMMARS_DIR = path.join(__dirname, '..', '..', 'grammars'); + function grammarPath(name: string): string { - return path.join(__dirname, '..', '..', 'grammars', name); + return path.join(GRAMMARS_DIR, name); } let _initialized: boolean = false; diff --git a/src/infrastructure/doctor.ts b/src/infrastructure/doctor.ts new file mode 100644 index 000000000..c5f3ea1ba --- /dev/null +++ b/src/infrastructure/doctor.ts @@ -0,0 +1,291 @@ +/** + * Environment health checks ("doctor") for a codegraph checkout. + * + * Every git worktree (see CLAUDE.md "Parallel Sessions") gets its own + * untracked `node_modules/` and `grammars/` — neither is shared via git, so + * every fresh `git worktree add` needs its own `npm install`. Two classes of + * drift are both silent until something fails deep inside a build or test + * run, surfacing as a cryptic native-module stack trace or a swallowed parse + * failure rather than a clear diagnosis (issue #1733): + * + * 1. `better-sqlite3`'s compiled `.node` binary is a classic V8/NAN addon + * (not N-API), so it is tied to the exact Node ABI + * (`process.versions.modules`) it was compiled under. Upgrading Node in + * place without `npm rebuild` leaves a stale binary that throws on load + * — and better-sqlite3 sits on the hot path for nearly every command + * (see `db/builder/pipeline.ts`), so this one failure looks like almost + * everything is broken. + * 2. `grammars/*.wasm` is populated by `npm run build:wasm` (via the + * `prepare` lifecycle script) from tree-sitter grammar devDependencies. + * A worktree set up before that step finished — or where it failed + * partway — is left with only a partial grammar set. + * + * Grammar completeness is NOT all-or-nothing: `LANGUAGE_REGISTRY` marks only + * JS/TS/TSX as `required: true` — every other language is designed to fail + * gracefully at runtime when its grammar is missing (see this repo's own + * CLAUDE.md: "Non-required parsers ... fail gracefully if their WASM grammar + * is unavailable"). A worktree that can't fetch one optional grammar's + * devDependency (e.g. a sandboxed environment and a `git+ssh` grammar + * package) should still be able to build/test with reduced language coverage + * — not get hard-blocked before a single test runs. So `checkWasmGrammars` + * only fails (blocks `pretest`) when a *required* grammar is missing; a + * missing optional grammar is surfaced as a non-blocking 'warn'. + * + * Design: each check's *decision* logic is a pure function (`parseAbiMismatchError`, + * `findMissingGrammars`) that takes plain data and is trivial to unit test with + * fake inputs. The public `checkXxx` functions wrap that logic with the real + * I/O (a `require()` attempt, a directory listing) behind an injectable + * parameter, so tests can simulate a broken environment without touching this + * worktree's real native binary or grammars/ directory. + * + * This module only detects — it never mutates the environment. `scripts/doctor.ts` + * is the CLI entry point that also knows how to *fix* what this module reports. + */ +import { readdirSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { GRAMMARS_DIR, LANGUAGE_REGISTRY } from '../domain/parser.js'; +import type { LanguageRegistryEntry } from '../types.js'; + +const _require = createRequire(import.meta.url); + +/** + * 'ok' — nothing to report. 'warn' — non-blocking (e.g. an optional grammar + * is missing); does not fail the check or the overall report. 'fail' — + * blocking; fails the overall report and `pretest`. + */ +type DoctorCheckStatus = 'ok' | 'warn' | 'fail'; + +/** + * Result of a single doctor check. Not exported by name — consumers (e.g. + * `scripts/doctor.ts`) type against `DoctorReport['checks'][number]` / + * `ReturnType` structurally rather than importing + * this interface, since nothing outside this module currently constructs a + * `DoctorCheck` independently of calling `checkBetterSqlite3Abi` / + * `checkWasmGrammars` / `runDoctorChecks`. + */ +interface DoctorCheck { + /** Stable machine-readable id, e.g. 'better-sqlite3-abi'. */ + id: string; + /** Human-readable label for report output. */ + label: string; + status: DoctorCheckStatus; + /** One-line human-readable explanation of the result. */ + detail: string; + /** Shell command that resolves this failure/warning. Absent when status is 'ok'. */ + fixCommand?: string; +} + +/** Aggregate result of running every doctor check. Not exported — see DoctorCheck. */ +interface DoctorReport { + /** False only when some check's status is 'fail' — a 'warn' never flips this. */ + ok: boolean; + checks: DoctorCheck[]; +} + +/** + * Parsed ABI numbers from a Node native-addon load-failure error message. + * Not exported — it's only ever consumed as `parseAbiMismatchError`'s return + * value; callers destructure the fields rather than naming the type. + */ +interface AbiMismatchInfo { + /** NODE_MODULE_VERSION the binary was compiled against. */ + compiledVersion: number; + /** NODE_MODULE_VERSION the current Node process requires. */ + requiredVersion: number; +} + +// Node's native-addon ABI mismatch error has been stable for many major +// versions, e.g.: +// "The module '/path/to/better_sqlite3.node' +// was compiled against a different Node.js version using +// NODE_MODULE_VERSION 127. This version of Node.js requires +// NODE_MODULE_VERSION 131. Please try re-compiling or re-installing +// the module (for instance, using `npm rebuild` or `npm install`)." +// Detection never depends on this regex matching — any thrown error from +// loadModule() is already treated as "broken" by checkBetterSqlite3Abi. +// This only extracts the two version numbers for a friendlier message; if a +// future Node release rewords the message, the check still fails correctly, +// it just falls back to the raw error text instead of the parsed numbers. +const ABI_MISMATCH_RE = /NODE_MODULE_VERSION (\d+)\.[\s\S]*?NODE_MODULE_VERSION (\d+)/; + +/** + * Parse Node's native-addon ABI mismatch error text into structured version + * numbers. Returns null for any message that doesn't match the known format + * (e.g. a missing-module error, a permission error, or reworded future text). + */ +export function parseAbiMismatchError(message: string): AbiMismatchInfo | null { + const match = ABI_MISMATCH_RE.exec(message); + if (!match) return null; + return { compiledVersion: Number(match[1]), requiredVersion: Number(match[2]) }; +} + +/** + * Check whether better-sqlite3's compiled native binary loads under the + * current Node process. + * + * Requiring it is the only reliable way to learn a prebuilt `.node` file's + * ABI compatibility — Node exposes no static introspection API for a + * candidate binary's compiled-against version — so this performs a real (but + * fast, in-process, side-effect-free beyond module caching) require() rather + * than shelling out to a subprocess. + * + * `loadModule` is injectable so tests can simulate a stale binary, a missing + * install, or a healthy load without touching this worktree's real + * node_modules/. + */ +export function checkBetterSqlite3Abi( + loadModule: () => unknown = () => _require('better-sqlite3'), +): DoctorCheck { + const id = 'better-sqlite3-abi'; + const label = 'better-sqlite3 native binary'; + try { + loadModule(); + return { + id, + label, + status: 'ok', + detail: `loads cleanly under Node ${process.version} (ABI ${process.versions.modules})`, + }; + } catch (err) { + const code = (err as NodeJS.ErrnoException)?.code; + const message = err instanceof Error ? err.message : String(err); + + if (code === 'MODULE_NOT_FOUND') { + return { + id, + label, + status: 'fail', + detail: 'better-sqlite3 is not installed in this worktree', + fixCommand: 'npm install', + }; + } + + const mismatch = parseAbiMismatchError(message); + const detail = mismatch + ? `compiled for NODE_MODULE_VERSION ${mismatch.compiledVersion}, but Node ${process.version} requires ${mismatch.requiredVersion} — likely a stale binary from before a Node upgrade` + : `failed to load: ${message.split('\n')[0]}`; + return { id, label, status: 'fail', detail, fixCommand: 'npm rebuild better-sqlite3' }; + } +} + +/** + * The subset of a language registry entry the grammar-completeness check + * needs. Not exported — `checkWasmGrammars`'s default parameter is the real + * `LANGUAGE_REGISTRY`; this narrower shape only exists so callers (tests) can + * pass minimal fake entries without fabricating an `extractor`/`extensions`. + */ +type GrammarRegistryEntry = Pick; + +/** + * Pure: given the language registry and the set of grammar filenames that + * actually exist on disk, return the entries whose `.wasm` file is missing. + */ +export function findMissingGrammars( + registry: readonly GrammarRegistryEntry[], + existingFiles: ReadonlySet, +): GrammarRegistryEntry[] { + return registry.filter((entry) => !existingFiles.has(entry.grammarFile)); +} + +/** List `.wasm` filenames present in the real `grammars/` directory. */ +function listInstalledGrammarFiles(): ReadonlySet { + try { + return new Set(readdirSync(GRAMMARS_DIR).filter((f) => f.endsWith('.wasm'))); + } catch { + return new Set(); // grammars/ doesn't exist at all + } +} + +/** Render up to `max` grammar filenames, with a "+N more" suffix beyond that. */ +function sampleList(entries: readonly GrammarRegistryEntry[], max = 5): string { + const sample = entries + .slice(0, max) + .map((e) => e.grammarFile) + .join(', '); + const suffix = entries.length > max ? `, +${entries.length - max} more` : ''; + return `${sample}${suffix}`; +} + +/** + * Check that every language in the registry has its WASM grammar file + * present on disk. Checks the *full* registry (all 30+ languages), not just + * the `required` tier (JS/TS/TSX) that `isWasmAvailable()` in `domain/parser.ts` + * gates parser startup on — a worktree can start up fine with only the + * required grammars while silently missing most language support. + * + * A missing `required` grammar (JS/TS/TSX) is a hard failure — the parser + * can't function without it. A missing *optional* grammar is reported as a + * non-blocking warning: non-required parsers are designed to fail gracefully + * at runtime (see the module doc comment), so this must never flip `status` + * to 'fail' on its own — that would incorrectly block `pretest`/`npm test` + * for every worktree missing even one rarely-used language's grammar. + * + * Both `registry` and `listGrammarFiles` are injectable so tests can supply a + * fake registry and a fake "what's on disk" set without touching the real + * grammars/ directory. + */ +export function checkWasmGrammars( + registry: readonly GrammarRegistryEntry[] = LANGUAGE_REGISTRY, + listGrammarFiles: () => ReadonlySet = listInstalledGrammarFiles, +): DoctorCheck { + const id = 'wasm-grammars'; + const label = 'WASM tree-sitter grammars'; + const existing = listGrammarFiles(); + const missing = findMissingGrammars(registry, existing); + + if (missing.length === 0) { + return { + id, + label, + status: 'ok', + detail: `all ${registry.length} grammar files present in grammars/`, + }; + } + + const missingRequired = missing.filter((e) => e.required); + const missingOptional = missing.filter((e) => !e.required); + + if (missingRequired.length > 0) { + const optionalNote = + missingOptional.length > 0 + ? `; ${missingOptional.length} optional grammar file(s) also missing` + : ''; + return { + id, + label, + status: 'fail', + detail: + `${missingRequired.length} required grammar file(s) missing ` + + `(${sampleList(missingRequired)})${optionalNote}`, + fixCommand: 'npm run build:wasm', + }; + } + + // Only optional grammars are missing — all required (JS/TS/TSX) grammars + // are present, so parsing isn't broken, just narrower. Non-blocking. + return { + id, + label, + status: 'warn', + detail: + `all required grammar files present; ${missingOptional.length} optional ` + + `grammar file(s) missing, non-blocking (${sampleList(missingOptional)})`, + fixCommand: 'npm run build:wasm', + }; +} + +/** + * Run every doctor check and aggregate the result. Fast and read-only — safe + * to call frequently (e.g. from a `pretest` hook) since neither check spawns + * a subprocess or mutates the environment. + * + * `checks` is injectable (defaulting to the two real checks) so the + * ok/'fail'-only aggregation rule — a 'warn' must never flip the overall + * report unhealthy — can be unit tested directly with fake check results, + * independent of the real native binary or grammars/ directory. + */ +export function runDoctorChecks( + checks: readonly DoctorCheck[] = [checkBetterSqlite3Abi(), checkWasmGrammars()], +): DoctorReport { + return { ok: checks.every((c) => c.status !== 'fail'), checks: [...checks] }; +} diff --git a/tests/unit/doctor.test.ts b/tests/unit/doctor.test.ts new file mode 100644 index 000000000..1cb19a9ed --- /dev/null +++ b/tests/unit/doctor.test.ts @@ -0,0 +1,316 @@ +/** + * Unit tests for src/infrastructure/doctor.ts (issue #1733). + * + * Covers the two failure classes reported against a stale worktree: a + * better-sqlite3 native binary compiled for an older Node ABI, and a + * grammars/ directory missing most of its .wasm files. Every check function + * takes its I/O (require(), directory listing) as an injectable parameter, + * so these tests simulate both broken states with fakes — no real native + * binary or grammars/ file is touched or modified. + * + * Grammar-completeness tests specifically cover the required-vs-optional + * split: a missing *required* (JS/TS/TSX) grammar must fail the check and the + * overall report (blocking `pretest`), but a missing *optional* grammar must + * only warn — never flip the report unhealthy — per this repo's own + * CLAUDE.md ("Non-required parsers ... fail gracefully if their WASM grammar + * is unavailable"). + */ +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { LANGUAGE_REGISTRY } from '../../src/domain/parser.js'; +import { + checkBetterSqlite3Abi, + checkWasmGrammars, + findMissingGrammars, + parseAbiMismatchError, + runDoctorChecks, +} from '../../src/infrastructure/doctor.js'; + +describe('parseAbiMismatchError', () => { + it('parses the real Node native-addon ABI mismatch message', () => { + const message = `The module '/repo/node_modules/better-sqlite3/build/Release/better_sqlite3.node' +was compiled against a different Node.js version using +NODE_MODULE_VERSION 137. This version of Node.js requires +NODE_MODULE_VERSION 147. Please try re-compiling or re-installing +the module (for instance, using \`npm rebuild\` or \`npm install\`).`; + + expect(parseAbiMismatchError(message)).toEqual({ + compiledVersion: 137, + requiredVersion: 147, + }); + }); + + it('returns null for unrelated error text', () => { + expect(parseAbiMismatchError("Cannot find module 'better-sqlite3'")).toBeNull(); + expect(parseAbiMismatchError('Segmentation fault (core dumped)')).toBeNull(); + expect(parseAbiMismatchError('')).toBeNull(); + }); + + it('returns null when only one NODE_MODULE_VERSION mention is present', () => { + expect(parseAbiMismatchError('NODE_MODULE_VERSION 137 only mentioned once')).toBeNull(); + }); +}); + +describe('checkBetterSqlite3Abi', () => { + it('reports ok when loadModule succeeds', () => { + const result = checkBetterSqlite3Abi(() => ({ Database: class {} })); + expect(result.status).toBe('ok'); + expect(result.id).toBe('better-sqlite3-abi'); + expect(result.fixCommand).toBeUndefined(); + expect(result.detail).toContain(process.version); + }); + + it('reports a parsed ABI mismatch with the rebuild fix command', () => { + const message = `The module '/repo/node_modules/better-sqlite3/build/Release/better_sqlite3.node' +was compiled against a different Node.js version using +NODE_MODULE_VERSION 137. This version of Node.js requires +NODE_MODULE_VERSION 147. Please try re-compiling or re-installing +the module (for instance, using \`npm rebuild\` or \`npm install\`).`; + const result = checkBetterSqlite3Abi(() => { + throw new Error(message); + }); + + expect(result.status).toBe('fail'); + expect(result.fixCommand).toBe('npm rebuild better-sqlite3'); + expect(result.detail).toContain('137'); + expect(result.detail).toContain('147'); + }); + + it('reports a missing install with the npm install fix command', () => { + const result = checkBetterSqlite3Abi(() => { + const err = Object.assign(new Error("Cannot find module 'better-sqlite3'"), { + code: 'MODULE_NOT_FOUND', + }); + throw err; + }); + + expect(result.status).toBe('fail'); + expect(result.fixCommand).toBe('npm install'); + expect(result.detail).toMatch(/not installed/i); + }); + + it('falls back to the raw message and a generic rebuild fix when the error is unrecognized', () => { + // Simulates a future Node version rewording the ABI-mismatch message — + // detection must still fail closed rather than silently reporting healthy. + const result = checkBetterSqlite3Abi(() => { + throw new Error('Segmentation fault (core dumped)'); + }); + + expect(result.status).toBe('fail'); + expect(result.fixCommand).toBe('npm rebuild better-sqlite3'); + expect(result.detail).toContain('Segmentation fault'); + }); + + it('handles non-Error throws gracefully', () => { + const result = checkBetterSqlite3Abi(() => { + throw 'boom'; + }); + expect(result.status).toBe('fail'); + expect(result.detail).toContain('boom'); + }); +}); + +describe('findMissingGrammars (pure)', () => { + const registry = [ + { id: 'javascript', grammarFile: 'tree-sitter-javascript.wasm', required: true }, + { id: 'python', grammarFile: 'tree-sitter-python.wasm', required: false }, + { id: 'rust', grammarFile: 'tree-sitter-rust.wasm', required: false }, + ]; + + it('returns entries whose grammar file is absent from the existing set', () => { + const missing = findMissingGrammars(registry, new Set(['tree-sitter-javascript.wasm'])); + expect(missing.map((m) => m.id)).toEqual(['python', 'rust']); + }); + + it('returns an empty array when every grammar file is present', () => { + const all = new Set(registry.map((r) => r.grammarFile)); + expect(findMissingGrammars(registry, all)).toEqual([]); + }); + + it('returns the full registry when nothing is installed', () => { + expect(findMissingGrammars(registry, new Set())).toEqual(registry); + }); + + it('does not mutate its inputs', () => { + const existing = new Set(['tree-sitter-javascript.wasm']); + const registryCopy = [...registry]; + findMissingGrammars(registry, existing); + expect(registry).toEqual(registryCopy); + expect(existing.size).toBe(1); + }); +}); + +describe('checkWasmGrammars (fake registry + fake listGrammarFiles)', () => { + // Mirrors the real LANGUAGE_REGISTRY shape: a small required tier (like + // JS/TS/TSX) plus a larger optional tier (like every other language). + const registry = [ + { id: 'javascript', grammarFile: 'tree-sitter-javascript.wasm', required: true }, + { id: 'typescript', grammarFile: 'tree-sitter-typescript.wasm', required: true }, + { id: 'python', grammarFile: 'tree-sitter-python.wasm', required: false }, + { id: 'rust', grammarFile: 'tree-sitter-rust.wasm', required: false }, + { id: 'go', grammarFile: 'tree-sitter-go.wasm', required: false }, + { id: 'java', grammarFile: 'tree-sitter-java.wasm', required: false }, + { id: 'ruby', grammarFile: 'tree-sitter-ruby.wasm', required: false }, + { id: 'php', grammarFile: 'tree-sitter-php.wasm', required: false }, + ]; + const requiredFiles = registry.filter((r) => r.required).map((r) => r.grammarFile); + + it('reports ok when every grammar file is present', () => { + const result = checkWasmGrammars(registry, () => new Set(registry.map((r) => r.grammarFile))); + expect(result.status).toBe('ok'); + expect(result.id).toBe('wasm-grammars'); + expect(result.detail).toBe(`all ${registry.length} grammar files present in grammars/`); + expect(result.fixCommand).toBeUndefined(); + }); + + it('reports warn — never fail — when only optional grammars are missing and all required are present', () => { + // All required (javascript, typescript) present; all 6 optional missing. + const result = checkWasmGrammars(registry, () => new Set(requiredFiles)); + expect(result.status).toBe('warn'); + expect(result.status).not.toBe('fail'); + expect(result.fixCommand).toBe('npm run build:wasm'); + expect(result.detail).toContain('all required grammar files present'); + expect(result.detail).toContain('6 optional'); + expect(result.detail).toContain('non-blocking'); + // 6 missing optional entries, truncated sample: 5 shown + "+1 more". + expect(result.detail).toContain('+1 more'); + }); + + it('reports fail when a required grammar is missing, even if it is the only one missing', () => { + const present = new Set(registry.map((r) => r.grammarFile)); + present.delete('tree-sitter-javascript.wasm'); // drop one required grammar + const result = checkWasmGrammars(registry, () => present); + + expect(result.status).toBe('fail'); + expect(result.fixCommand).toBe('npm run build:wasm'); + expect(result.detail).toContain('1 required grammar file(s) missing'); + expect(result.detail).toContain('tree-sitter-javascript.wasm'); + // No optional grammars are missing in this scenario. + expect(result.detail).not.toContain('optional grammar file(s) also missing'); + }); + + it('reports fail and still mentions optional grammars missing alongside a required one', () => { + // Only typescript present: javascript (required) AND all 6 optional missing. + const result = checkWasmGrammars(registry, () => new Set(['tree-sitter-typescript.wasm'])); + + expect(result.status).toBe('fail'); + expect(result.detail).toContain('1 required grammar file(s) missing'); + expect(result.detail).toContain('tree-sitter-javascript.wasm'); + expect(result.detail).toContain('6 optional grammar file(s) also missing'); + }); + + it('reports fail when the grammars directory does not exist (empty set)', () => { + const result = checkWasmGrammars(registry, () => new Set()); + expect(result.status).toBe('fail'); + expect(result.detail).toContain(`${requiredFiles.length} required grammar file(s) missing`); + }); +}); + +describe('checkWasmGrammars (real fs I/O against a temp directory, real LANGUAGE_REGISTRY)', () => { + let tmpDir: string; + + afterEach(() => { + if (tmpDir) rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('detects a healthy directory containing every registered grammar file', () => { + tmpDir = mkdtempSync(path.join(os.tmpdir(), 'cg-doctor-grammars-healthy-')); + for (const entry of LANGUAGE_REGISTRY) { + writeFileSync(path.join(tmpDir, entry.grammarFile), ''); + } + + const result = checkWasmGrammars(LANGUAGE_REGISTRY, () => new Set(readdirSync(tmpDir))); + expect(result.status).toBe('ok'); + expect(result.detail).toBe( + `all ${LANGUAGE_REGISTRY.length} grammar files present in grammars/`, + ); + }); + + it('detects a near-empty directory (mirrors the reported worktree: 1 of ~40 files) — required grammars missing, so it fails', () => { + tmpDir = mkdtempSync(path.join(os.tmpdir(), 'cg-doctor-grammars-broken-')); + // Only the erlang (optional) grammar present, exactly as in the original + // bug report — JS/TS/TSX (required) are among the 35 missing files, so + // this must still be a hard failure, not just a warning. + writeFileSync(path.join(tmpDir, 'tree-sitter-erlang.wasm'), ''); + + const result = checkWasmGrammars(LANGUAGE_REGISTRY, () => new Set(readdirSync(tmpDir))); + expect(result.status).toBe('fail'); + expect(result.fixCommand).toBe('npm run build:wasm'); + expect(result.detail).toContain('required grammar file(s) missing'); + expect(result.detail).toContain('tree-sitter-javascript.wasm'); + }); + + it('detects a directory missing exactly one required grammar', () => { + tmpDir = mkdtempSync(path.join(os.tmpdir(), 'cg-doctor-grammars-partial-')); + const [, ...rest] = LANGUAGE_REGISTRY; // drop the first entry (javascript, required) + for (const entry of rest) { + writeFileSync(path.join(tmpDir, entry.grammarFile), ''); + } + + const result = checkWasmGrammars(LANGUAGE_REGISTRY, () => new Set(readdirSync(tmpDir))); + expect(result.status).toBe('fail'); + expect(result.detail).toContain('tree-sitter-javascript.wasm'); + expect(result.detail).toContain('1 required grammar file(s) missing'); + }); + + it('detects a directory missing only an optional grammar (all required present) — warns, does not fail', () => { + tmpDir = mkdtempSync(path.join(os.tmpdir(), 'cg-doctor-grammars-optional-gap-')); + // tree-sitter-erlang.wasm is an optional (required: false) grammar. + for (const entry of LANGUAGE_REGISTRY) { + if (entry.grammarFile === 'tree-sitter-erlang.wasm') continue; + writeFileSync(path.join(tmpDir, entry.grammarFile), ''); + } + + const result = checkWasmGrammars(LANGUAGE_REGISTRY, () => new Set(readdirSync(tmpDir))); + expect(result.status).toBe('warn'); + expect(result.status).not.toBe('fail'); + expect(result.detail).toContain('tree-sitter-erlang.wasm'); + expect(result.detail).toContain('non-blocking'); + }); +}); + +describe('runDoctorChecks', () => { + it('a non-blocking warn check does not flip the overall report unhealthy', () => { + const report = runDoctorChecks([ + { id: 'a', label: 'A', status: 'ok', detail: 'fine' }, + { id: 'b', label: 'B', status: 'warn', detail: 'minor issue, non-blocking' }, + ]); + expect(report.ok).toBe(true); + }); + + it('a fail check flips the overall report unhealthy, even alongside ok/warn checks', () => { + const report = runDoctorChecks([ + { id: 'a', label: 'A', status: 'ok', detail: 'fine' }, + { id: 'b', label: 'B', status: 'warn', detail: 'minor issue, non-blocking' }, + { id: 'c', label: 'C', status: 'fail', detail: 'broken' }, + ]); + expect(report.ok).toBe(false); + }); + + it('an all-ok report is healthy', () => { + const report = runDoctorChecks([{ id: 'a', label: 'A', status: 'ok', detail: 'fine' }]); + expect(report.ok).toBe(true); + }); + + it('runs both real checks against the real environment and returns a well-formed report', () => { + // Uses the real defaults (real require('better-sqlite3'), real grammars/ + // directory) — by the time this test runs, `pretest` has already gated + // `npm test` on a healthy environment, but this asserts shape rather than + // hard-coding a specific status so a single-file run on a mid-repair + // machine doesn't fail on an unrelated assertion. + const report = runDoctorChecks(); + + expect(typeof report.ok).toBe('boolean'); + expect(report.checks).toHaveLength(2); + expect(report.checks.map((c) => c.id)).toEqual(['better-sqlite3-abi', 'wasm-grammars']); + for (const check of report.checks) { + expect(['ok', 'warn', 'fail']).toContain(check.status); + expect(typeof check.label).toBe('string'); + expect(typeof check.detail).toBe('string'); + expect(check.detail.length).toBeGreaterThan(0); + } + expect(report.ok).toBe(report.checks.every((c) => c.status !== 'fail')); + }); +}); From 9702c4b6fd7910029e61069ffaab1a2d33c2385c Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 21:13:29 -0600 Subject: [PATCH 11/12] fix: wire doctor into pretest:watch and pretest:coverage hooks (#1834) --- package.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/package.json b/package.json index 0ea841824..d018511ab 100644 --- a/package.json +++ b/package.json @@ -86,7 +86,9 @@ "doctor": "node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/doctor.ts", "pretest": "npm run doctor", "test": "vitest run", + "pretest:watch": "npm run doctor", "test:watch": "vitest", + "pretest:coverage": "npm run doctor", "test:coverage": "vitest run --coverage", "test:regression-guard": "vitest run tests/benchmarks/regression-guard.test.ts", "lint": "biome check src/ tests/", From b1c4aac18ebb5466427b9d97df2be9992ad8885e Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Sun, 5 Jul 2026 21:13:35 -0600 Subject: [PATCH 12/12] fix: don't crash doctor if the parser module (web-tree-sitter) fails to load (#1834) Impact: 2 functions changed, 2 affected --- src/infrastructure/doctor.ts | 46 ++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/src/infrastructure/doctor.ts b/src/infrastructure/doctor.ts index c5f3ea1ba..d14ed62d4 100644 --- a/src/infrastructure/doctor.ts +++ b/src/infrastructure/doctor.ts @@ -43,11 +43,31 @@ */ import { readdirSync } from 'node:fs'; import { createRequire } from 'node:module'; -import { GRAMMARS_DIR, LANGUAGE_REGISTRY } from '../domain/parser.js'; import type { LanguageRegistryEntry } from '../types.js'; const _require = createRequire(import.meta.url); +/** + * `domain/parser.js` eagerly imports `web-tree-sitter` at module scope, so a + * fully broken `npm install` (not just a stale `better-sqlite3` binary) + * would otherwise crash this entire module with an unhandled + * `MODULE_NOT_FOUND` before a single doctor check can run — defeating the + * point of a tool whose job is to turn opaque startup crashes into a clear + * diagnostic (#1733). Loaded once via top-level await so `checkWasmGrammars` + * stays synchronous for callers/tests; a load failure is surfaced as a + * doctor check row instead of propagating. + */ +let parserModule: { + GRAMMARS_DIR: string; + LANGUAGE_REGISTRY: readonly LanguageRegistryEntry[]; +} | null = null; +let parserModuleError: string | null = null; +try { + parserModule = await import('../domain/parser.js'); +} catch (e) { + parserModuleError = e instanceof Error ? e.message : String(e); +} + /** * 'ok' — nothing to report. 'warn' — non-blocking (e.g. an optional grammar * is missing); does not fail the check or the overall report. 'fail' — @@ -189,8 +209,9 @@ export function findMissingGrammars( /** List `.wasm` filenames present in the real `grammars/` directory. */ function listInstalledGrammarFiles(): ReadonlySet { + if (!parserModule) return new Set(); // parser module failed to load — see checkWasmGrammars try { - return new Set(readdirSync(GRAMMARS_DIR).filter((f) => f.endsWith('.wasm'))); + return new Set(readdirSync(parserModule.GRAMMARS_DIR).filter((f) => f.endsWith('.wasm'))); } catch { return new Set(); // grammars/ doesn't exist at all } @@ -222,14 +243,31 @@ function sampleList(entries: readonly GrammarRegistryEntry[], max = 5): string { * * Both `registry` and `listGrammarFiles` are injectable so tests can supply a * fake registry and a fake "what's on disk" set without touching the real - * grammars/ directory. + * grammars/ directory. `registry` defaults to the real `LANGUAGE_REGISTRY` + * unless the `domain/parser.js` module itself failed to load (see the + * top-level-await block above), in which case this reports a clear 'fail' + * instead of silently claiming zero missing grammars. */ export function checkWasmGrammars( - registry: readonly GrammarRegistryEntry[] = LANGUAGE_REGISTRY, + registry?: readonly GrammarRegistryEntry[], listGrammarFiles: () => ReadonlySet = listInstalledGrammarFiles, ): DoctorCheck { const id = 'wasm-grammars'; const label = 'WASM tree-sitter grammars'; + + if (registry === undefined) { + if (!parserModule) { + return { + id, + label, + status: 'fail', + detail: `cannot check grammar completeness — parser module failed to load: ${parserModuleError}`, + fixCommand: 'npm install', + }; + } + registry = parserModule.LANGUAGE_REGISTRY; + } + const existing = listGrammarFiles(); const missing = findMissingGrammars(registry, existing);