From 6143d810aec5503cb69506ad2c53a7be503c9f48 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Thu, 13 Aug 2026 18:53:35 -0600 Subject: [PATCH] fix(dataflow): barrel-only tie-break misclassifies hybrid files, drops their call edges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #2339: `is_barrel_file`/`isBarrelFile` classified a file as barrel-only using `reexports >= ownDefs`. A file with exactly one reexport and exactly one own definition hit that `>=` and got misclassified as barrel-only, even though it's a genuine hybrid (real logic plus a reexport), not a pure barrel. Once misclassified, the call-edge builder skips ALL of that file's own outgoing call/receiver edges, so on any incremental build where the file gets pulled into the barrel-candidate reparse (because something imports from it), its own call edges are silently dropped and never re-emitted. Changed the tie-break to strict `>` in both engines. This is orthogonal to #1848's fix, which scopes *which files* are even eligible for this check (only transient barrel-candidate reparses, never a file genuinely part of the current build's changed set) — not the comparison itself; verified no existing test exercises the exact reexports==ownDefs tie, so this couldn't have silently broken an already-covered case. Also fixes a second, previously-hidden bug discovered while writing this issue's regression test: the JS/WASM engine's `reparseBarrelFiles` deleted a barrel candidate's outgoing edges without first clearing `dataflow` rows that reference them via `call_edge_id`, so the delete threw a FOREIGN KEY constraint error whenever the candidate's own call edges were also tracked by interprocedural dataflow — silently swallowed by the surrounding try/catch's `debug()` call (a no-op unless verbose), aborting the entire barrel reparse for that file. The Rust engine already carries this exact fix (#979); the JS engine never got the mirror. Without it, the new regression test couldn't even reach the `>=`/`>` code path on the WASM engine, since barrel reparsing silently failed before ever classifying the file. Confirmed the fix is necessary by verifying the test fails without it (edges genuinely go missing) and passes with it. New tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts, dual-engine (mirrors issue-1174-chained-barrel-incremental.test.ts's structure): a hybrid.js at the exact reexports==ownDefs==1 tie, whose own call edge to helper.js must survive an incremental rebuild that pulls it into the barrel-candidate reparse. Confirmed this fails without either fix and passes with both. tests/integration/issue-1174-chained-barrel- incremental.test.ts (parser.js has 1 reexport vs. 4 defs, never at the tie) is unaffected — still passes. Closes #2339 docs check acknowledged: internal engine bug fix + regression test, no new user-facing behavior or CLI surface change. Impact: 6 functions changed, 0 affected --- .../src/domain/graph/builder/pipeline.rs | 4 +- .../graph/builder/stages/import_edges.rs | 19 ++- .../graph/builder/stages/resolve-imports.ts | 40 ++++- .../issue-2339-barrel-tiebreak/app.js | 5 + .../issue-2339-barrel-tiebreak/helper.js | 3 + .../issue-2339-barrel-tiebreak/hybrid.js | 12 ++ .../issue-2339-barrel-tiebreak/other.js | 3 + ...e-2339-barrel-tiebreak-incremental.test.ts | 147 ++++++++++++++++++ 8 files changed, 220 insertions(+), 13 deletions(-) create mode 100644 tests/fixtures/issue-2339-barrel-tiebreak/app.js create mode 100644 tests/fixtures/issue-2339-barrel-tiebreak/helper.js create mode 100644 tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js create mode 100644 tests/fixtures/issue-2339-barrel-tiebreak/other.js create mode 100644 tests/integration/issue-2339-barrel-tiebreak-incremental.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 ba9c732eb..288f907c1 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -744,8 +744,8 @@ pub fn run_pipeline( // `barrel_candidates_added` (empty on full builds) rather than every key // in `file_symbols` — a file that's genuinely part of this build's // changed set must always get its own non-reexport imports emitted, - // regardless of whether it happens to satisfy the reexports>=ownDefs - // heuristic (#1848). + // regardless of whether it happens to satisfy the reexports-outnumber- + // ownDefs heuristic (#1848, #2339). import_ctx.reexport_map = import_edges::build_reexport_map(&import_ctx); import_ctx.barrel_only_files = import_edges::detect_barrel_only_files(&import_ctx, &barrel_candidates_added); 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 ae7eaca0b..77bf147a3 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 @@ -29,7 +29,7 @@ pub struct ImportEdgeContext { pub batch_resolved: HashMap, /// Map of relPath -> reexport entries. pub reexport_map: HashMap>, - /// Set of files that are barrel-only (reexport count >= definition count). + /// Set of files that are barrel-only (reexport count > definition count). pub barrel_only_files: HashSet, /// Parsed symbols per relative path. pub file_symbols: BTreeMap, @@ -72,7 +72,16 @@ impl ImportEdgeContext { ) } - /// Check if a file is a barrel file (reexport count >= definition count). + /// Check if a file is a barrel file (reexport count strictly exceeds + /// definition count). Strict `>`, not `>=` (issue #2339): a file with + /// exactly one reexport and one own definition is a genuine hybrid (real + /// logic plus a reexport), not a pure barrel — `>=` misclassified it as + /// barrel-only, silently dropping its own outgoing call/receiver edges + /// whenever it got pulled into Stage 6b's barrel-candidate reparse. This + /// is orthogonal to #1848's fix, which scopes *which files* are even + /// eligible for this check (only transient barrel-candidate reparses, + /// never a file genuinely part of this build's changed set) — not the + /// comparison itself. pub fn is_barrel_file(&self, rel_path: &str) -> bool { let symbols = match self.file_symbols.get(rel_path) { Some(s) => s, @@ -86,7 +95,7 @@ impl ImportEdgeContext { if reexport_count == 0 { return false; } - reexport_count >= symbols.definitions.len() + reexport_count > symbols.definitions.len() } /// Recursively resolve a barrel export to its actual source file. @@ -1066,8 +1075,8 @@ mod tests { /// Regression test for #1848: `detect_barrel_only_files` must only /// classify files present in the supplied candidate list, even when a - /// file outside that list also satisfies the reexports>=ownDefs - /// heuristic. `run_pipeline` relies on this scoping to keep a + /// file outside that list also satisfies the reexports-outnumber- + /// ownDefs heuristic. `run_pipeline` relies on this scoping to keep a /// genuinely-changed (or, on a full build, every) file's own /// non-reexport imports from ever being dropped — only files /// transiently side-loaded by `reparse_barrel_candidates` for barrel diff --git a/src/domain/graph/builder/stages/resolve-imports.ts b/src/domain/graph/builder/stages/resolve-imports.ts index 6d6e78466..9c32d97c8 100644 --- a/src/domain/graph/builder/stages/resolve-imports.ts +++ b/src/domain/graph/builder/stages/resolve-imports.ts @@ -120,11 +120,11 @@ function findBarrelCandidates( * them for the next level of barrel candidates. * * A re-parsed file is marked `barrel-only` only when it really is one (the - * `isBarrelFile` check — reexports >= ownDefs). The previous unconditional - * `.add(relPath)` caused hybrid barrels with many local defs (e.g. a file - * with one `export type ... from` and dozens of internal functions) to drop - * all their non-reexport imports in build-edges, since the barrel-only branch - * skips them (#1174). + * `isBarrelFile` check — reexports strictly outnumber ownDefs, #2339). The + * previous unconditional `.add(relPath)` caused hybrid barrels with many + * local defs (e.g. a file with one `export type ... from` and dozens of + * internal functions) to drop all their non-reexport imports in + * build-edges, since the barrel-only branch skips them (#1174). */ async function reparseBarrelFiles( ctx: PipelineContext, @@ -146,6 +146,22 @@ async function reparseBarrelFiles( // candidates are merged here *after* insertNodes, so wiping those kinds // would permanently drop them (mirrors the Rust orchestrator's Stage 6b // delete in domain/graph/builder/pipeline.rs). + // + // Clear dataflow rows that reference these outgoing edges via call_edge_id + // BEFORE deleting the edges — avoids a FOREIGN KEY constraint failure when + // `PRAGMA foreign_keys` is on (`dataflow.call_edge_id REFERENCES edges.id`). + // Discovered while writing #2339's regression test: this delete previously + // threw on every barrel candidate whose own call edges were also tracked by + // interprocedural dataflow, and the exception was silently swallowed by the + // catch below (only surfaced via `debug()`, which is a no-op unless + // verbose), so the barrel simply never got reparsed — the Rust engine + // already had this exact fix (#979); the JS engine never got the mirror. + const deleteReferencingDataflow = db.prepare( + `DELETE FROM dataflow WHERE call_edge_id IN ( + SELECT id FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?) + AND kind NOT IN ('contains', 'parameter_of') + )`, + ); const deleteOutgoingEdges = db.prepare( `DELETE FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?) AND kind NOT IN ('contains', 'parameter_of')`, @@ -155,6 +171,7 @@ async function reparseBarrelFiles( try { const barrelSymbols = await parseFilesAuto(barrelPaths, rootDir, engineOpts); for (const [relPath, fileSym] of barrelSymbols) { + deleteReferencingDataflow.run(relPath); deleteOutgoingEdges.run(relPath); fileSymbols.set(relPath, fileSym); if (isBarrelFile(ctx, relPath)) { @@ -330,13 +347,24 @@ export function getResolved(ctx: PipelineContext, absFile: string, importSource: return resolveImportPath(absFile, importSource, ctx.rootDir, ctx.aliases, ctx.allFiles); } +/** + * A file is a barrel file when its reexport count strictly exceeds its + * definition count. Strict `>`, not `>=` (issue #2339): a file with exactly + * one reexport and one own definition is a genuine hybrid (real logic plus + * a reexport), not a pure barrel — `>=` misclassified it as barrel-only, + * silently dropping its own outgoing call/receiver edges whenever it got + * pulled into `reparseBarrelFiles`' transient reparse. Orthogonal to + * #1848's fix, which scopes *which files* are even eligible for this check + * (only transient barrel-candidate reparses, never a file genuinely part of + * this build's changed set) — not the comparison itself. + */ export function isBarrelFile(ctx: PipelineContext, relPath: string): boolean { const symbols = ctx.fileSymbols.get(relPath); if (!symbols) return false; const reexports = symbols.imports.filter((imp) => imp.reexport); if (reexports.length === 0) return false; const ownDefs = symbols.definitions.length; - return reexports.length >= ownDefs; + return reexports.length > ownDefs; } /** Check if a re-export source directly defines the symbol. */ diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/app.js b/tests/fixtures/issue-2339-barrel-tiebreak/app.js new file mode 100644 index 000000000..d99e243b6 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/app.js @@ -0,0 +1,5 @@ +import { doWork } from './hybrid.js'; + +export function main(input) { + return doWork(input); +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/helper.js b/tests/fixtures/issue-2339-barrel-tiebreak/helper.js new file mode 100644 index 000000000..da257aaa6 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/helper.js @@ -0,0 +1,3 @@ +export function helperFn(input) { + return input; +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js b/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js new file mode 100644 index 000000000..a5a7bd57d --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js @@ -0,0 +1,12 @@ +// Hybrid file at the exact tie boundary (issue #2339): exactly one reexport +// and exactly one own definition. Under the old `reexports >= ownDefs` +// heuristic this was misclassified as barrel-only, so its own outgoing call +// edge to helper.js was silently dropped whenever this file got reparsed as +// a Stage 6b barrel candidate on an incremental rebuild. +export { Named } from './other.js'; + +import { helperFn } from './helper.js'; + +export function doWork(input) { + return helperFn(input); +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/other.js b/tests/fixtures/issue-2339-barrel-tiebreak/other.js new file mode 100644 index 000000000..a0685f496 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/other.js @@ -0,0 +1,3 @@ +export function Named() { + return 'named'; +} diff --git a/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts b/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts new file mode 100644 index 000000000..18e3ed781 --- /dev/null +++ b/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts @@ -0,0 +1,147 @@ +/** + * Regression for #2339: `detect_barrel_only_files` / `isBarrelFile` used + * `reexports >= ownDefs` to decide whether a file is a pure barrel. A file + * with exactly one reexport and exactly one own definition hit that `>=` + * and got misclassified as barrel-only — even though it's a genuine hybrid + * (real logic plus a reexport), not a pure barrel. Once misclassified, + * `build_and_insert_call_edges` (and its JS equivalent) skip emitting ANY + * of that file's own outgoing call edges, so on any incremental build where + * the file gets pulled into Stage 6b's barrel-candidate reparse (because + * something imports the reexported symbol from it), its own call edges are + * silently dropped and never re-emitted — even on an otherwise-correct + * build. + * + * Fixture shape: + * + * app.js + * └─ imports `doWork` from hybrid.js + * + * hybrid.js (exact tie: 1 reexport + 1 own def) + * ├─ `export { Named } from './other.js'` + * └─ `doWork()` calls `helperFn` from helper.js + * + * other.js (defines the reexported symbol) + * helper.js (defines the function hybrid.js's own def calls) + * + * Before the fix, editing app.js triggered a reparse of hybrid.js (it has + * one reexport edge in the DB, so the orchestrator flags it as a barrel + * candidate every incremental build), and the `>=` tie caused it to be + * (re)classified barrel-only, dropping the doWork -> helperFn call edge. + * Mirrors tests/integration/issue-1174-chained-barrel-incremental.test.ts's + * structure (that fixture's hybrid file has 1 reexport vs. 4 defs, so it + * never exercised this exact tie). + */ + +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 type { EngineMode } from '../../src/types.js'; + +const FIXTURE_DIR = path.join(import.meta.dirname, '..', 'fixtures', 'issue-2339-barrel-tiebreak'); + +function copyDirSync(src: string, dest: string) { + fs.mkdirSync(dest, { recursive: true }); + for (const entry of fs.readdirSync(src, { withFileTypes: true })) { + const s = path.join(src, entry.name); + const d = path.join(dest, entry.name); + if (entry.isDirectory()) copyDirSync(s, d); + else fs.copyFileSync(s, d); + } +} + +interface EdgeRow { + source_file: string; + source_name: string; + target_file: string; + target_name: string; + kind: string; +} + +function readEdges(dbPath: string): EdgeRow[] { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.file AS source_file, n1.name AS source_name, + n2.file AS target_file, n2.name AS target_name, e.kind + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + ORDER BY n1.file, n1.name, n2.file, n2.name, e.kind`, + ) + .all() as EdgeRow[]; + } finally { + db.close(); + } +} + +const ENGINES: EngineMode[] = ['wasm', 'native']; + +describe.each(ENGINES)('Issue #2339 barrel reexports==ownDefs tie-break parity (%s)', (engine) => { + let fullEdges: EdgeRow[]; + let incrEdges: EdgeRow[]; + let tmpBase: string; + + beforeAll(async () => { + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), `codegraph-2339-${engine}-`)); + const fullDir = path.join(tmpBase, 'full'); + const incrDir = path.join(tmpBase, 'incr'); + copyDirSync(FIXTURE_DIR, fullDir); + copyDirSync(FIXTURE_DIR, incrDir); + + // Establish baseline on the incremental copy + await buildGraph(incrDir, { incremental: false, skipRegistry: true, engine }); + + // Mutate app.js (the only "changed" file) on both copies + const mutate = (dir: string) => { + fs.appendFileSync(path.join(dir, 'app.js'), '\n// touch\n'); + }; + mutate(fullDir); + mutate(incrDir); + + // Full build on the full copy + await buildGraph(fullDir, { incremental: false, skipRegistry: true, engine }); + // Incremental rebuild on the incr copy + await buildGraph(incrDir, { incremental: true, skipRegistry: true, engine }); + + fullEdges = readEdges(path.join(fullDir, '.codegraph', 'graph.db')); + incrEdges = readEdges(path.join(incrDir, '.codegraph', 'graph.db')); + }, 90_000); + + afterAll(() => { + if (tmpBase) fs.rmSync(tmpBase, { recursive: true, force: true }); + }); + + it('emits the doWork -> helperFn call edge on full build (hybrid.js is not barrel-only)', () => { + const callEdge = fullEdges.filter( + (e) => + e.source_file === 'hybrid.js' && + e.source_name === 'doWork' && + e.target_file === 'helper.js' && + e.target_name === 'helperFn' && + e.kind === 'calls', + ); + expect(callEdge.length).toBeGreaterThan(0); + }); + + it('the doWork -> helperFn call edge survives the incremental rebuild', () => { + const callEdge = incrEdges.filter( + (e) => + e.source_file === 'hybrid.js' && + e.source_name === 'doWork' && + e.target_file === 'helper.js' && + e.target_name === 'helperFn' && + e.kind === 'calls', + ); + expect(callEdge.length).toBeGreaterThan(0); + }); + + it('call edge count matches full rebuild', () => { + const fullCalls = fullEdges.filter((e) => e.kind === 'calls'); + const incrCalls = incrEdges.filter((e) => e.kind === 'calls'); + expect(incrCalls.length).toBe(fullCalls.length); + }); +});