diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index 9b1bee2e9..2d2471457 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -286,6 +286,11 @@ fn reconnect_saved_reverse_dep_edges( /// structure rebuild based on the same gates as the JS pipeline. The change /// set is read from `file_symbols.keys()` because only truly-changed files /// are present (reverse-deps are reconnected, not re-parsed). +/// +/// `removed_files` is threaded through separately from `parse_changes_len` +/// (which only counts re-parsed files) so the fast path's directory-metrics +/// refresh also covers files deleted from a directory, not just files added +/// or modified within it (#1738). fn run_structure_phase( conn: &Connection, file_symbols: &BTreeMap, @@ -293,6 +298,7 @@ fn run_structure_phase( root_dir: &str, line_count_map: &HashMap, parse_changes_len: usize, + removed_files: &[String], is_full_build: bool, ) { let changed_files: Vec = file_symbols.keys().cloned().collect(); @@ -303,6 +309,7 @@ fn run_structure_phase( if use_fast_path { structure::update_changed_file_metrics(conn, &changed_files, line_count_map, file_symbols); + structure::refresh_affected_directory_metrics(conn, &changed_files, removed_files); } else { let changed_for_structure: Option> = if is_full_build { None @@ -603,6 +610,7 @@ pub fn run_pipeline( root_dir, &line_count_map, parse_changes.len(), + &change_result.removed, change_result.is_full_build, ); timing.structure_ms = t0.elapsed().as_secs_f64() * 1000.0; diff --git a/crates/codegraph-core/src/features/structure.rs b/crates/codegraph-core/src/features/structure.rs index ebcf6d62c..f2fbe5d66 100644 --- a/crates/codegraph-core/src/features/structure.rs +++ b/crates/codegraph-core/src/features/structure.rs @@ -141,6 +141,256 @@ pub fn get_existing_file_count(conn: &Connection) -> i64 { .unwrap_or(0) } +/// Directories connected to `dir` via a live import/imports-type edge in +/// either direction — the cross-directory neighbours whose own fan-in/out +/// may have shifted even though none of their files changed. Used by +/// `refresh_affected_directory_metrics` to expand its affected-directory set +/// by exactly one hop. +fn find_neighbor_files(conn: &Connection, dir: &str) -> Vec { + let lo = format!("{dir}/"); + let hi = format!("{dir}0"); + let mut stmt = match conn.prepare( + "SELECT n2.file AS other FROM edges e \ + JOIN nodes n1 ON e.source_id = n1.id JOIN nodes n2 ON e.target_id = n2.id \ + WHERE e.kind IN ('imports', 'imports-type') AND n1.file != n2.file \ + AND n1.file >= ?1 AND n1.file < ?2 \ + AND NOT (n2.file >= ?1 AND n2.file < ?2) \ + UNION \ + SELECT n1.file AS other FROM edges e \ + JOIN nodes n1 ON e.source_id = n1.id JOIN nodes n2 ON e.target_id = n2.id \ + WHERE e.kind IN ('imports', 'imports-type') AND n1.file != n2.file \ + AND n2.file >= ?1 AND n2.file < ?2 \ + AND NOT (n1.file >= ?1 AND n1.file < ?2)", + ) { + Ok(s) => s, + Err(_) => return Vec::new(), + }; + let result = match stmt.query_map(rusqlite::params![lo, hi], |row| row.get::<_, String>(0)) { + Ok(rows) => rows.flatten().collect(), + Err(_) => Vec::new(), + }; + result +} + +/// Targeted directory-metrics refresh for the small-incremental fast path. +/// +/// `update_changed_file_metrics` only ever touches per-file `node_metrics` +/// rows — it never looks at directories. Any file added to, removed from, or +/// edited within a directory left that directory's +/// fileCount/symbolCount/fanIn/fanOut/cohesion stale until the next full +/// rebuild (#1738), and a file added under a brand-new directory never even +/// got a directory node or a `contains` edge from its parent. +/// +/// This recomputes metrics for the ancestor directories of the files that +/// changed in this build (added, removed, or modified), PLUS any directory +/// reachable from the touched files' *immediate* (most specific) directory +/// via a live cross-directory import edge — a changed file that gains (or +/// loses) an import into a sibling package shifts that package's +/// fan-in/fan-out/cohesion even though none of its own files were touched. +/// +/// The neighbor-discovery step is seeded only from each touched file's +/// leaf directory, not from every ancestor up to root — `find_neighbor_files` +/// is bounded by the number of import edges crossing ONE directory's +/// boundary, which for a broad, near-root "container" directory (e.g. `src`, +/// which transitively contains most of the repo) is effectively the size of +/// the whole codebase, not "path depth". Seeding from every ancestor turned +/// a single touched file into 50+ affected directories on this repo's own +/// `src/` tree — a measured 70-90ms hit on the "1-file rebuild" benchmark +/// (#1738 follow-up). Ancestor rollup (ancestors' own aggregates still get +/// recomputed) is unaffected; only the expensive cross-directory neighbor +/// lookup is scoped down. +/// +/// Removed files need no edge/node cleanup of their own — the purge step +/// already deleted their nodes and every edge referencing them (including +/// their old `contains` edge) earlier in the pipeline; only their ancestor +/// directories' aggregates need recomputing here. Note this expansion can't +/// reach a directory whose ONLY relationship to the touched set was an edge +/// to/from a file that was JUST removed — that edge is already gone by the +/// time this runs, so there's nothing left to discover it from (tracked +/// separately, see #1738 follow-up). +pub fn refresh_affected_directory_metrics( + conn: &Connection, + changed_files: &[String], + removed_files: &[String], +) { + let mut touched: Vec = Vec::with_capacity(changed_files.len() + removed_files.len()); + touched.extend_from_slice(changed_files); + touched.extend_from_slice(removed_files); + let mut affected_dirs = get_ancestor_dirs(&touched); + if affected_dirs.is_empty() { + return; + } + + // Seed neighbor-discovery from each touched file's own leaf directory + // only — NOT every entry in `affected_dirs` (which includes the full + // ancestor chain up to root). See the function doc comment: expanding + // from a broad ancestor like `src` is effectively repo-wide. + let leaf_dirs: HashSet = touched.iter().filter_map(|f| parent_dir(f)).collect(); + for dir in &leaf_dirs { + let neighbor_files = find_neighbor_files(conn, dir); + for ancestor in get_ancestor_dirs(&neighbor_files) { + affected_dirs.insert(ancestor); + } + } + + let tx = match conn.unchecked_transaction() { + Ok(tx) => tx, + Err(_) => return, + }; + + // 1. Ensure directory nodes exist for the whole affected ancestor chain — + // handles a file added under a brand-new (possibly multi-level) directory. + { + let mut insert_dir = match tx.prepare( + "INSERT OR IGNORE INTO nodes (name, kind, file, line, end_line) VALUES (?, 'directory', ?, 0, NULL)", + ) { + Ok(s) => s, + Err(_) => return, + }; + for dir in &affected_dirs { + let _ = insert_dir.execute(rusqlite::params![dir, dir]); + } + } + + { + let mut insert_edge = match tx.prepare( + "INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) \ + SELECT ?, ?, 'contains', 1.0, 0 \ + WHERE NOT EXISTS (SELECT 1 FROM edges WHERE source_id = ? AND target_id = ? AND kind = 'contains')", + ) { + Ok(s) => s, + Err(_) => return, + }; + + // 2. Wire dir -> parent-dir contains edges for the chain. + for dir in &affected_dirs { + if let Some(parent) = parent_dir(dir) { + if let (Some(parent_id), Some(dir_id)) = ( + get_node_id(&tx, &parent, "directory", &parent, 0), + get_node_id(&tx, dir, "directory", dir, 0), + ) { + let _ = + insert_edge.execute(rusqlite::params![parent_id, dir_id, parent_id, dir_id]); + } + } + } + + // 3. Wire dir -> file contains edges for changed (added/modified) files. + // Removed files' nodes and edges are already purged upstream. + for rel_path in changed_files { + if let Some(dir) = parent_dir(rel_path) { + if let (Some(dir_id), Some(file_id)) = ( + get_node_id(&tx, &dir, "directory", &dir, 0), + get_node_id(&tx, rel_path, "file", rel_path, 0), + ) { + let _ = + insert_edge.execute(rusqlite::params![dir_id, file_id, dir_id, file_id]); + } + } + } + } + + // 4. Recompute each affected directory's metrics from the live DB state. + { + // fileCount/symbolCount: transitive counts under `dir`, matching + // compute_directory_metrics below. `file >= dir/ AND file < dir0` is + // an index-friendly prefix-range scan equivalent to `file LIKE + // 'dir/%'` — '0' (0x30) is the character immediately after '/' + // (0x2F), so this bound matches exactly the paths nested under `dir`. + let mut count_files = match tx.prepare( + "SELECT COUNT(*) FROM nodes WHERE kind = 'file' AND file >= ?1 AND file < ?2", + ) { + Ok(s) => s, + Err(_) => return, + }; + let mut count_symbols = match tx.prepare( + "SELECT COUNT(*) FROM nodes WHERE kind != 'file' AND kind != 'directory' AND file >= ?1 AND file < ?2", + ) { + Ok(s) => s, + Err(_) => return, + }; + // Edges sourced from a file inside dir: intra (target also inside dir) vs fan-out. + let mut outbound = match tx.prepare( + "SELECT \ + COALESCE(SUM(CASE WHEN n2.file >= ?1 AND n2.file < ?2 THEN 1 ELSE 0 END), 0), \ + COUNT(*) \ + FROM edges e \ + JOIN nodes n1 ON e.source_id = n1.id \ + JOIN nodes n2 ON e.target_id = n2.id \ + WHERE e.kind IN ('imports', 'imports-type') \ + AND n1.file != n2.file \ + AND n2.kind = 'file' \ + AND n1.file >= ?1 AND n1.file < ?2", + ) { + Ok(s) => s, + Err(_) => return, + }; + // Edges targeting a file inside dir, sourced from a file outside dir (fan-in only). + let mut inbound = match tx.prepare( + "SELECT COUNT(*) \ + FROM edges e \ + JOIN nodes n1 ON e.source_id = n1.id \ + JOIN nodes n2 ON e.target_id = n2.id \ + WHERE e.kind IN ('imports', 'imports-type') \ + AND n1.file != n2.file \ + AND n2.kind = 'file' \ + AND n2.file >= ?1 AND n2.file < ?2 \ + AND NOT (n1.file >= ?1 AND n1.file < ?2)", + ) { + Ok(s) => s, + Err(_) => return, + }; + let mut upsert = match tx.prepare( + "INSERT OR REPLACE INTO node_metrics \ + (node_id, line_count, symbol_count, import_count, export_count, fan_in, fan_out, cohesion, file_count) \ + VALUES (?, NULL, ?, NULL, NULL, ?, ?, ?, ?)", + ) { + Ok(s) => s, + Err(_) => return, + }; + + for dir in &affected_dirs { + let dir_id = match get_node_id(&tx, dir, "directory", dir, 0) { + Some(id) => id, + None => continue, + }; + let lo = format!("{dir}/"); + let hi = format!("{dir}0"); + + let file_count: i64 = count_files + .query_row(rusqlite::params![lo, hi], |r| r.get(0)) + .unwrap_or(0); + let symbol_count: i64 = count_symbols + .query_row(rusqlite::params![lo, hi], |r| r.get(0)) + .unwrap_or(0); + let (intra, total): (i64, i64) = outbound + .query_row(rusqlite::params![lo, hi], |r| Ok((r.get(0)?, r.get(1)?))) + .unwrap_or((0, 0)); + let fan_out = total - intra; + let fan_in: i64 = inbound + .query_row(rusqlite::params![lo, hi], |r| r.get(0)) + .unwrap_or(0); + let total_edges = intra + fan_in + fan_out; + let cohesion: Option = if total_edges > 0 { + Some(intra as f64 / total_edges as f64) + } else { + None + }; + + let _ = upsert.execute(rusqlite::params![ + dir_id, + symbol_count, + fan_in, + fan_out, + cohesion, + file_count + ]); + } + } + + let _ = tx.commit(); +} + // ── Full structure computation ────────────────────────────────────────── /// Normalize a path to use forward slashes only. diff --git a/src/domain/graph/builder/stages/build-structure.ts b/src/domain/graph/builder/stages/build-structure.ts index 144537dfe..dcf8de6ad 100644 --- a/src/domain/graph/builder/stages/build-structure.ts +++ b/src/domain/graph/builder/stages/build-structure.ts @@ -6,7 +6,7 @@ import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { debug } from '../../../../infrastructure/logger.js'; -import { normalizePath } from '../../../../shared/constants.js'; +import { getAncestorDirs, normalizePath } from '../../../../shared/constants.js'; import type { ExtractorOutput } from '../../../../types.js'; import type { PipelineContext } from '../context.js'; import { readFileSafe } from '../helpers.js'; @@ -55,6 +55,7 @@ async function buildDirectoryStructure( ): Promise { if (useSmallIncrementalFastPath) { updateChangedFileMetrics(ctx, changedFileList!); + refreshAffectedDirectoryMetrics(ctx, changedFileList!, ctx.removed ?? []); return; } @@ -185,8 +186,9 @@ export async function buildStructure(ctx: PipelineContext): Promise { * (~8ms) and full structure rebuild (~15ms), replacing them with per-file * indexed queries (~1-2ms total for 1-5 files). * - * Directory metrics are not recomputed — a 1-5 file change won't - * meaningfully alter directory-level cohesion or symbol counts. + * Directory-level metrics are handled separately by + * `refreshAffectedDirectoryMetrics` below — this function only ever touches + * per-file rows. */ function updateChangedFileMetrics(ctx: PipelineContext, changedFiles: string[]): void { const { db } = ctx; @@ -250,6 +252,209 @@ function updateChangedFileMetrics(ctx: PipelineContext, changedFiles: string[]): debug(`Structure (fast path): updated metrics for ${changedFiles.length} files`); } +/** + * Targeted directory-metrics refresh for the small-incremental fast path. + * + * `updateChangedFileMetrics` only ever touches per-file `node_metrics` rows — + * it never looks at directories. Any file added to, removed from, or edited + * within a directory left that directory's fileCount/symbolCount/fanIn/ + * fanOut/cohesion stale until the next full rebuild (#1738), and a file added + * under a brand-new directory never even got a directory node or a `contains` + * edge from its parent. + * + * This recomputes metrics for the ancestor directories of the files that + * changed in this build (added, removed, or modified), PLUS any directory + * reachable from the touched files' *immediate* (most specific) directory + * via a live cross-directory import edge — a changed file that gains (or + * loses) an import into a sibling package shifts that package's + * fan-in/fan-out/cohesion even though none of its own files were touched. + * + * The neighbor-discovery step is seeded only from each touched file's leaf + * directory, not from every ancestor up to root — the neighbor query is + * bounded by the number of import edges crossing ONE directory's boundary, + * which for a broad, near-root "container" directory (e.g. `src`, which + * transitively contains most of the repo) is effectively the size of the + * whole codebase, not "path depth". Seeding from every ancestor turned a + * single touched file into 50+ affected directories on this repo's own + * `src/` tree — a measured 70-90ms hit on the "1-file rebuild" benchmark + * (#1738 follow-up, mirrors the same fix in structure.rs). Ancestor rollup + * (ancestors' own aggregates still get recomputed) is unaffected; only the + * expensive cross-directory neighbor lookup is scoped down. + * + * Removed files need no edge/node cleanup of their own — `purgeFilesData` + * already deleted their nodes and every edge referencing them (including + * their old `contains` edge) earlier in the pipeline; only their ancestor + * directories' aggregates need recomputing here. Note this expansion can't + * reach a directory whose ONLY relationship to the touched set was an edge + * to/from a file that was JUST removed — that edge is already gone by the + * time this runs, so there's nothing left to discover it from (tracked + * separately, see #1738 follow-up). + */ +function refreshAffectedDirectoryMetrics( + ctx: PipelineContext, + changedFiles: string[], + removedFiles: string[], +): void { + const { db } = ctx; + const affectedDirs = getAncestorDirs([...changedFiles, ...removedFiles]); + if (affectedDirs.size === 0) return; + + const getDirId = db.prepare( + "SELECT id FROM nodes WHERE name = ? AND kind = 'directory' AND file = ? AND line = 0", + ); + // Directories connected to `dir` via a live import/imports-type edge in + // either direction — the cross-directory neighbours whose own fan-in/out + // may have shifted even though none of their files changed. + const neighborFiles = db.prepare(` + SELECT n2.file AS other FROM edges e + JOIN nodes n1 ON e.source_id = n1.id JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind IN ('imports', 'imports-type') AND n1.file != n2.file + AND n1.file >= @lo AND n1.file < @hi + AND NOT (n2.file >= @lo AND n2.file < @hi) + UNION + SELECT n1.file AS other FROM edges e + JOIN nodes n1 ON e.source_id = n1.id JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind IN ('imports', 'imports-type') AND n1.file != n2.file + AND n2.file >= @lo AND n2.file < @hi + AND NOT (n1.file >= @lo AND n1.file < @hi) + `); + // Seed neighbor-discovery from each touched file's own leaf directory + // only — NOT every entry in `affectedDirs` (which includes the full + // ancestor chain up to root). See the function doc comment: expanding + // from a broad ancestor like `src` is effectively repo-wide. + const leafDirs = new Set(); + for (const f of [...changedFiles, ...removedFiles]) { + const d = normalizePath(path.dirname(f)); + if (d && d !== '.') leafDirs.add(d); + } + for (const dir of leafDirs) { + const otherFiles = neighborFiles.all({ lo: `${dir}/`, hi: `${dir}0` }) as Array<{ + other: string; + }>; + for (const ancestor of getAncestorDirs(otherFiles.map((r) => r.other))) { + affectedDirs.add(ancestor); + } + } + + const insertDirNode = db.prepare( + 'INSERT OR IGNORE INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ); + const getFileId = db.prepare( + "SELECT id FROM nodes WHERE name = ? AND kind = 'file' AND file = ? AND line = 0", + ); + const insertContainsIfMissing = db.prepare(` + INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) + SELECT ?, ?, 'contains', 1.0, 0 + WHERE NOT EXISTS ( + SELECT 1 FROM edges WHERE source_id = ? AND target_id = ? AND kind = 'contains' + ) + `); + // fileCount/symbolCount: transitive counts under `dir`, matching + // computeDirectoryMetrics in features/structure.ts. `file >= dir/ AND + // file < dir0` is an index-friendly prefix-range scan equivalent to + // `file LIKE 'dir/%'` — '0' (0x30) is the character immediately after + // '/' (0x2F), so this bound matches exactly the paths nested under `dir`. + const countFiles = db.prepare( + "SELECT COUNT(*) AS c FROM nodes WHERE kind = 'file' AND file >= ? AND file < ?", + ); + const countSymbols = db.prepare( + "SELECT COUNT(*) AS c FROM nodes WHERE kind != 'file' AND kind != 'directory' AND file >= ? AND file < ?", + ); + // Edges sourced from a file inside dir: intra (target also inside dir) vs fan-out. + const outboundEdges = db.prepare(` + SELECT + COALESCE(SUM(CASE WHEN n2.file >= @lo AND n2.file < @hi THEN 1 ELSE 0 END), 0) AS intra, + COUNT(*) AS total + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind IN ('imports', 'imports-type') + AND n1.file != n2.file + AND n2.kind = 'file' + AND n1.file >= @lo AND n1.file < @hi + `); + // Edges targeting a file inside dir, sourced from a file outside dir (fan-in only). + const inboundEdges = db.prepare(` + SELECT COUNT(*) AS c + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind IN ('imports', 'imports-type') + AND n1.file != n2.file + AND n2.kind = 'file' + AND n2.file >= @lo AND n2.file < @hi + AND NOT (n1.file >= @lo AND n1.file < @hi) + `); + const upsertMetric = db.prepare(` + INSERT OR REPLACE INTO node_metrics + (node_id, line_count, symbol_count, import_count, export_count, fan_in, fan_out, cohesion, file_count) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + `); + + db.transaction(() => { + // Ensure directory nodes exist for the whole affected ancestor chain — + // handles a file added under a brand-new (possibly multi-level) directory. + for (const dir of affectedDirs) { + insertDirNode.run(dir, 'directory', dir, 0, null); + } + + // Wire parent-dir -> child-dir contains edges for the chain. + for (const dir of affectedDirs) { + const parent = normalizePath(path.dirname(dir)); + if (!parent || parent === '.' || parent === dir) continue; + const parentRow = getDirId.get(parent, parent) as { id: number } | undefined; + const childRow = getDirId.get(dir, dir) as { id: number } | undefined; + if (parentRow && childRow) { + insertContainsIfMissing.run(parentRow.id, childRow.id, parentRow.id, childRow.id); + } + } + + // Wire dir -> file contains edges for changed (added/modified) files. + // Removed files' nodes and edges are already purged upstream. + for (const relPath of changedFiles) { + const dir = normalizePath(path.dirname(relPath)); + if (!dir || dir === '.') continue; + const dirRow = getDirId.get(dir, dir) as { id: number } | undefined; + const fileRow = getFileId.get(relPath, relPath) as { id: number } | undefined; + if (dirRow && fileRow) { + insertContainsIfMissing.run(dirRow.id, fileRow.id, dirRow.id, fileRow.id); + } + } + + // Recompute each affected directory's metrics from the live DB state. + for (const dir of affectedDirs) { + const dirRow = getDirId.get(dir, dir) as { id: number } | undefined; + if (!dirRow) continue; + + const lo = `${dir}/`; + const hi = `${dir}0`; + const fileCount = (countFiles.get(lo, hi) as { c: number }).c; + const symbolCount = (countSymbols.get(lo, hi) as { c: number }).c; + const out = outboundEdges.get({ lo, hi }) as { intra: number; total: number }; + const fanOut = out.total - out.intra; + const fanIn = (inboundEdges.get({ lo, hi }) as { c: number }).c; + const totalEdges = out.intra + fanIn + fanOut; + const cohesion = totalEdges > 0 ? out.intra / totalEdges : null; + + upsertMetric.run( + dirRow.id, + null, + symbolCount, + null, + null, + fanIn, + fanOut, + cohesion, + fileCount, + ); + } + })(); + + debug( + `Structure (fast path): refreshed metrics for ${affectedDirs.size} affected director${affectedDirs.size === 1 ? 'y' : 'ies'}`, + ); +} + // ── Full incremental DB load (medium/large changes) ────────────────────── function loadUnchangedFilesFromDb(ctx: PipelineContext): void { diff --git a/src/features/structure.ts b/src/features/structure.ts index 85f3afff9..885b1102d 100644 --- a/src/features/structure.ts +++ b/src/features/structure.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { getBuildMeta, getNodeId, setBuildMeta, testFilterSQL } from '../db/index.js'; import { debug } from '../infrastructure/logger.js'; -import { normalizePath } from '../shared/constants.js'; +import { getAncestorDirs, normalizePath } from '../shared/constants.js'; import type { BetterSqlite3Database } from '../types.js'; // ─── Build-time helpers ─────────────────────────────────────────────── @@ -17,18 +17,6 @@ interface FileSymbolData { calls?: unknown[]; } -function getAncestorDirs(filePaths: string[]): Set { - const dirs = new Set(); - for (const f of filePaths) { - let d = normalizePath(path.dirname(f)); - while (d && d !== '.') { - dirs.add(d); - d = normalizePath(path.dirname(d)); - } - } - return dirs; -} - function cleanupPreviousData( db: BetterSqlite3Database, getNodeIdStmt: NodeIdStmt, diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 9dc88056e..bff1cab4c 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -77,3 +77,23 @@ export function isSupportedFile(filePath: string): boolean { export function normalizePath(filePath: string): string { return filePath.split(path.sep).join('/'); } + +/** + * Walk every ancestor directory of each given file path (not just the direct + * parent) and return the union across all files. Shared by the full + * directory-structure build (`features/structure.ts`) and the incremental + * fast path (`domain/graph/builder/stages/build-structure.ts`), which both + * need to scope work to exactly the directories whose file composition may + * have changed (#1738). + */ +export function getAncestorDirs(filePaths: Iterable): Set { + const dirs = new Set(); + for (const f of filePaths) { + let d = normalizePath(path.dirname(f)); + while (d && d !== '.') { + dirs.add(d); + d = normalizePath(path.dirname(d)); + } + } + return dirs; +} diff --git a/tests/integration/issue-1738-structure-metrics-incremental.test.ts b/tests/integration/issue-1738-structure-metrics-incremental.test.ts new file mode 100644 index 000000000..0086446f0 --- /dev/null +++ b/tests/integration/issue-1738-structure-metrics-incremental.test.ts @@ -0,0 +1,277 @@ +/** + * Regression test for #1738: `codegraph structure --depth 2 --json` reported + * stale fileCount/symbolCount/fanIn/cohesion/density for a directory after an + * INCREMENTAL rebuild added or removed a file in it. A full rebuild + * (`--no-incremental`) always produced the correct numbers. + * + * Root cause: the small-incremental fast path (`updateChangedFileMetrics` in + * `domain/graph/builder/stages/build-structure.ts`, mirrored by + * `update_changed_file_metrics` in `crates/codegraph-core/src/features/ + * structure.rs`) only ever updated per-FILE `node_metrics` rows. It never + * touched directories at all — no directory-metrics recompute, no `contains` + * edge for the new file, and no directory node for a brand-new directory. + * This path triggers whenever an incremental build touches at most + * `smallFilesThreshold` (5) files and the repo already has more than 20 + * files — i.e. almost every normal edit-and-rebuild cycle on a non-trivial + * repo, including a pure-removal build (0 parsed files, which trivially + * satisfies the "<=5" gate). + * + * Fixed by `refreshAffectedDirectoryMetrics` / + * `refresh_affected_directory_metrics`, which recomputes metrics (and wires + * up any missing directory nodes/contains edges) for the ancestor + * directories of the files touched by the incremental build, PLUS any + * directory reachable from them via a live cross-directory import edge (a + * changed file gaining/losing an import into a sibling package shifts that + * package's fan-in/fan-out too, even though none of its own files changed) + * — cheap because it's bounded by (changed files x path depth) rather than + * the size of the repo. See #1839 for a narrower residual gap this does not + * cover: a directory whose only link to the touched set was an edge to/from + * a file that was itself just removed (that edge's evidence is gone by the + * time the refresh runs). + * + * Strategy: build a fixture with >20 files (crossing the fast-path's + * `existingFileCount > 20` gate), mutate it incrementally, then diff the + * resulting directory metrics against a from-scratch full build of the exact + * same final file set — the full build is what the issue itself uses as + * "ground truth" (full rebuild fixes it). + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { structureData } from '../../src/features/structure.js'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; + +// Small-incremental fast path requires existingFileCount > 20 — keep a +// healthy margin above that boundary. +const BASE_FILE_COUNT = 24; +const DIR = 'src/pkg'; + +function fileContent(i: number): string { + return `export function fn${i}() { return ${i}; }\n`; +} + +/** Write `count` standalone (no cross-imports) files into `/src/pkg`. */ +function writeBaseFixture(root: string, count: number, skip: Set = new Set()): void { + fs.mkdirSync(path.join(root, DIR), { recursive: true }); + for (let i = 0; i < count; i++) { + if (skip.has(i)) continue; + fs.writeFileSync(path.join(root, DIR, `file${i}.js`), fileContent(i)); + } +} + +interface DirSnapshot { + fileCount: number; + symbolCount: number; + fanIn: number; + fanOut: number; + cohesion: number | null; + fileNames: string[]; +} + +function snapshotDir(dbPath: string, dirName: string): DirSnapshot { + const data = structureData(dbPath, { directory: dirName, full: true }); + const entry = data.directories.find((d) => d.directory === dirName); + expect(entry, `${dirName} directory missing from structureData output`).toBeDefined(); + return { + fileCount: entry!.fileCount, + symbolCount: entry!.symbolCount, + fanIn: entry!.fanIn, + fanOut: entry!.fanOut, + cohesion: entry!.cohesion, + fileNames: entry!.files.map((f) => f.file).sort(), + }; +} + +function runScenario(engine: 'wasm' | 'native'): void { + describe(`directory structure metrics after incremental rebuild (#1738) — ${engine}`, () => { + let incrDir: string; + const tmpDirs: string[] = []; + const incrDbPath = () => path.join(incrDir, '.codegraph', 'graph.db'); + + function mkTmp(prefix: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + tmpDirs.push(dir); + return dir; + } + + beforeAll(async () => { + incrDir = mkTmp(`cg-1738-incr-${engine}-`); + writeBaseFixture(incrDir, BASE_FILE_COUNT); + await buildGraph(incrDir, { engine, incremental: false, skipRegistry: true }); + }, 60_000); + + afterAll(() => { + for (const dir of tmpDirs) fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('baseline full build reports the expected file/symbol counts', () => { + const snap = snapshotDir(incrDbPath(), DIR); + expect(snap.fileCount).toBe(BASE_FILE_COUNT); + expect(snap.symbolCount).toBe(BASE_FILE_COUNT); + }); + + it('reflects a newly added file after an incremental rebuild (matches a full rebuild of the same file set)', async () => { + // Mutate the incremental repo: add one new file to the existing directory. + // A single added file stays within smallFilesThreshold (5), so this + // exercises the fast path. + fs.writeFileSync( + path.join(incrDir, DIR, 'new-file.js'), + "export function brandNew() { return 'new'; }\n", + ); + await buildGraph(incrDir, { engine, skipRegistry: true }); // incremental (default) + const incremental = snapshotDir(incrDbPath(), DIR); + + // Ground truth: an independent repo built in one full pass with the + // identical final file set (BASE_FILE_COUNT existing files + new-file.js). + const refDir = mkTmp(`cg-1738-ref-add-${engine}-`); + writeBaseFixture(refDir, BASE_FILE_COUNT); + fs.writeFileSync( + path.join(refDir, DIR, 'new-file.js'), + "export function brandNew() { return 'new'; }\n", + ); + await buildGraph(refDir, { engine, incremental: false, skipRegistry: true }); + const reference = snapshotDir(path.join(refDir, '.codegraph', 'graph.db'), DIR); + + expect( + incremental.fileCount, + 'directory fileCount is stale after incremental rebuild added a file (#1738)', + ).toBe(reference.fileCount); + expect(incremental.symbolCount).toBe(reference.symbolCount); + expect(incremental.fanIn).toBe(reference.fanIn); + expect(incremental.fanOut).toBe(reference.fanOut); + expect(incremental.cohesion).toBe(reference.cohesion); + expect( + incremental.fileNames, + 'new file is missing a contains edge from its parent directory (#1738)', + ).toEqual(reference.fileNames); + expect(incremental.fileNames).toContain(`${DIR}/new-file.js`); + }, 60_000); + + it('reflects a removed file after an incremental rebuild (matches a full rebuild of the same file set)', async () => { + // Mutate the incremental repo further: remove one pre-existing file. + // Zero re-parsed files also stays within smallFilesThreshold, so this + // exercises the fast path's pure-removal case. + fs.rmSync(path.join(incrDir, DIR, 'file0.js')); + await buildGraph(incrDir, { engine, skipRegistry: true }); + const incremental = snapshotDir(incrDbPath(), DIR); + + // Ground truth: BASE_FILE_COUNT files minus file0, plus new-file.js + // (added by the previous test), built from scratch in one full pass. + const refDir = mkTmp(`cg-1738-ref-remove-${engine}-`); + writeBaseFixture(refDir, BASE_FILE_COUNT, new Set([0])); + fs.writeFileSync( + path.join(refDir, DIR, 'new-file.js'), + "export function brandNew() { return 'new'; }\n", + ); + await buildGraph(refDir, { engine, incremental: false, skipRegistry: true }); + const reference = snapshotDir(path.join(refDir, '.codegraph', 'graph.db'), DIR); + + expect( + incremental.fileCount, + 'directory fileCount is stale after incremental rebuild removed a file (#1738)', + ).toBe(reference.fileCount); + expect(incremental.symbolCount).toBe(reference.symbolCount); + expect(incremental.fileNames).toEqual(reference.fileNames); + expect(incremental.fileNames).not.toContain(`${DIR}/file0.js`); + }, 60_000); + + it('creates a directory node and metrics for a file added under a brand-new nested directory', async () => { + // A file under a directory that has never existed before must get a + // directory node (and ancestor contains edges) created for it, not + // just a metrics update to a pre-existing row. + fs.mkdirSync(path.join(incrDir, DIR, 'newdir', 'nested'), { recursive: true }); + fs.writeFileSync( + path.join(incrDir, DIR, 'newdir', 'nested', 'deep.js'), + 'export function deepFn() { return 1; }\n', + ); + await buildGraph(incrDir, { engine, skipRegistry: true }); + + const nestedSnap = snapshotDir(incrDbPath(), `${DIR}/newdir/nested`); + expect(nestedSnap.fileCount).toBe(1); + expect(nestedSnap.symbolCount).toBe(1); + expect(nestedSnap.fileNames).toEqual([`${DIR}/newdir/nested/deep.js`]); + + // The ancestor's transitive fileCount must include the nested file too. + const parentSnap = snapshotDir(incrDbPath(), DIR); + expect(parentSnap.fileCount).toBeGreaterThanOrEqual( + BASE_FILE_COUNT /* original */ - + 1 /* file0 removed */ + + 1 /* new-file.js */ + + 1 /* deep.js */, + ); + }, 60_000); + + it("reflects a cross-directory import gained by a changed file on the OTHER (untouched) directory's fan-in/fan-out", async () => { + // A neighbor directory can have zero files of its own added, removed, + // or modified and still need its fan-in/fan-out/cohesion refreshed, + // because a changed file elsewhere gained or lost a cross-directory + // import edge touching it. + const crossDir = mkTmp(`cg-1738-cross-${engine}-`); + fs.mkdirSync(path.join(crossDir, 'src', 'pkgA'), { recursive: true }); + fs.mkdirSync(path.join(crossDir, 'src', 'pkgB'), { recursive: true }); + writeBaseFixture(crossDir, BASE_FILE_COUNT); // padding to cross the fast-path gate + fs.writeFileSync( + path.join(crossDir, 'src', 'pkgA', 'a1.js'), + "import { a2 } from './a2.js';\nexport function a1() { return a2(); }\n", + ); + fs.writeFileSync( + path.join(crossDir, 'src', 'pkgA', 'a2.js'), + 'export function a2() { return 2; }\n', + ); + fs.writeFileSync( + path.join(crossDir, 'src', 'pkgB', 'b1.js'), + 'export function b1() { return 1; }\n', + ); + await buildGraph(crossDir, { engine, incremental: false, skipRegistry: true }); + + const crossDbPath = () => path.join(crossDir, '.codegraph', 'graph.db'); + const baseline = snapshotDir(crossDbPath(), 'src/pkgB'); + expect(baseline.fanIn).toBe(0); + + // pkgA/a1.js (already-existing, gets modified) now ALSO imports + // pkgB/b1.js — pkgB itself has no file of its own touched. + fs.writeFileSync( + path.join(crossDir, 'src', 'pkgA', 'a1.js'), + "import { a2 } from './a2.js';\nimport { b1 } from '../pkgB/b1.js';\nexport function a1() { return a2() + b1(); }\n", + ); + await buildGraph(crossDir, { engine, skipRegistry: true }); + const incremental = snapshotDir(crossDbPath(), 'src/pkgB'); + + // Ground truth: an independent from-scratch full build of the same + // final source. + const refDir = mkTmp(`cg-1738-cross-ref-${engine}-`); + fs.mkdirSync(path.join(refDir, 'src', 'pkgA'), { recursive: true }); + fs.mkdirSync(path.join(refDir, 'src', 'pkgB'), { recursive: true }); + writeBaseFixture(refDir, BASE_FILE_COUNT); + fs.writeFileSync( + path.join(refDir, 'src', 'pkgA', 'a1.js'), + "import { a2 } from './a2.js';\nimport { b1 } from '../pkgB/b1.js';\nexport function a1() { return a2() + b1(); }\n", + ); + fs.writeFileSync( + path.join(refDir, 'src', 'pkgA', 'a2.js'), + 'export function a2() { return 2; }\n', + ); + fs.writeFileSync( + path.join(refDir, 'src', 'pkgB', 'b1.js'), + 'export function b1() { return 1; }\n', + ); + await buildGraph(refDir, { engine, incremental: false, skipRegistry: true }); + const reference = snapshotDir(path.join(refDir, '.codegraph', 'graph.db'), 'src/pkgB'); + + expect( + incremental.fanIn, + "pkgB's fanIn is stale — a cross-directory import gained by pkgA was not reflected on its neighbor (#1738)", + ).toBe(reference.fanIn); + expect(incremental.fanIn).toBe(1); + expect(incremental.cohesion).toBe(reference.cohesion); + }, 60_000); + }); +} + +runScenario('wasm'); + +describe.skipIf(!isNativeAvailable())('native engine coverage', () => { + runScenario('native'); +});