diff --git a/crates/codegraph-core/src/db/repository/graph_read.rs b/crates/codegraph-core/src/db/repository/graph_read.rs index 5f36382e8..aa21de34e 100644 --- a/crates/codegraph-core/src/db/repository/graph_read.rs +++ b/crates/codegraph-core/src/db/repository/graph_read.rs @@ -1675,8 +1675,15 @@ impl NativeDatabase { .map(|k| format!("'{k}'")) .collect::>() .join(","); + // ORDER BY id: without an explicit order, SQLite's row order for a + // bare WHERE scan is unspecified — it happened to track physical/ + // insertion order, which is only deterministic now that the build + // pipeline inserts nodes in a fixed (BTreeMap-sorted) order (#1734). + // Sorting explicitly here removes the dependency on that unspecified + // behavior so downstream consumers (e.g. community detection) build + // the same graph on every run regardless of how rows are stored. let sql = format!( - "SELECT id, name, kind, file FROM nodes WHERE kind IN ({kinds_sql})" + "SELECT id, name, kind, file FROM nodes WHERE kind IN ({kinds_sql}) ORDER BY id" ); let mut stmt = conn .prepare_cached(&sql) @@ -1701,7 +1708,8 @@ impl NativeDatabase { let conn = self.conn()?; let mut stmt = conn .prepare_cached( - "SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls'", + "SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls' \ + ORDER BY source_id, target_id", ) .map_err(|e| napi::Error::from_reason(format!("get_call_edges prepare: {e}")))?; let rows = stmt @@ -1721,8 +1729,10 @@ impl NativeDatabase { #[napi] pub fn get_file_nodes_all(&self) -> napi::Result> { let conn = self.conn()?; + // ORDER BY id — see the comment in get_callable_nodes for why an + // explicit order matters for build-to-build determinism (#1734). let mut stmt = conn - .prepare_cached("SELECT id, name, file FROM nodes WHERE kind = 'file'") + .prepare_cached("SELECT id, name, file FROM nodes WHERE kind = 'file' ORDER BY id") .map_err(|e| napi::Error::from_reason(format!("get_file_nodes_all prepare: {e}")))?; let rows = stmt .query_map([], |row| { @@ -1743,7 +1753,8 @@ impl NativeDatabase { let conn = self.conn()?; let mut stmt = conn .prepare_cached( - "SELECT source_id, target_id FROM edges WHERE kind IN ('imports','imports-type')", + "SELECT source_id, target_id FROM edges WHERE kind IN ('imports','imports-type') \ + ORDER BY source_id, target_id", ) .map_err(|e| napi::Error::from_reason(format!("get_import_edges prepare: {e}")))?; let rows = stmt diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index ea4442e34..9b1bee2e9 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -33,7 +33,7 @@ use crate::features::structure; use crate::types::{FileSymbols, ImportResolutionInput, TypeMapEntry}; use rusqlite::Connection; use serde::Serialize; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::Path; use std::time::Instant; @@ -218,12 +218,12 @@ fn parse_and_index_files( root_dir: &str, include_dataflow: bool, include_ast: bool, -) -> HashMap { +) -> BTreeMap { let files_to_parse: Vec = parse_changes.iter().map(|c| c.abs_path.clone()).collect(); let parsed = parallel::parse_files_parallel(&files_to_parse, root_dir, include_dataflow, include_ast); - let mut file_symbols: HashMap = HashMap::new(); + let mut file_symbols: BTreeMap = BTreeMap::new(); for mut sym in parsed { let rel = relative_path(root_dir, &sym.file); sym.file = rel.clone(); @@ -235,7 +235,7 @@ fn parse_and_index_files( /// Build the batched import-resolution input set and run resolution, returning /// `(batch_resolved, known_files)`. Mirrors stage 6 of `run_pipeline`. fn resolve_pipeline_imports( - file_symbols: &HashMap, + file_symbols: &BTreeMap, collect_files: &[String], root_dir: &str, napi_aliases: &crate::types::PathAliases, @@ -288,7 +288,7 @@ fn reconnect_saved_reverse_dep_edges( /// are present (reverse-deps are reconnected, not re-parsed). fn run_structure_phase( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, collect_directories: &HashSet, root_dir: &str, line_count_map: &HashMap, @@ -326,7 +326,7 @@ fn run_structure_phase( /// nodes are gone (#1027). fn run_role_classification( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, removal_reverse_deps: Vec, is_full_build: bool, ) { @@ -367,7 +367,7 @@ struct AnalysisPersistenceResult { /// analysis scope. fn run_analysis_persistence( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, analysis_scope: Option<&Vec>, opts: &BuildOpts, include_ast: bool, @@ -765,7 +765,7 @@ fn reparse_barrel_candidates( root_dir: &str, napi_aliases: &crate::types::PathAliases, known_files: &HashSet, - file_symbols: &mut HashMap, + file_symbols: &mut BTreeMap, batch_resolved: &mut HashMap, ) { // Find all barrel files from DB (files that have 'reexports' edges) @@ -892,7 +892,7 @@ fn collect_imported_barrel_candidates( from_files: &[String], batch_resolved: &HashMap, barrel_files_in_db: &HashSet, - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) -> Vec { let mut out = Vec::new(); for rel_path in from_files { @@ -926,7 +926,7 @@ fn collect_reexport_from_barrels( conn: &Connection, root_dir: &str, changed_files: &[String], - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) -> Vec { let mut out = Vec::new(); let mut stmt = match conn.prepare( @@ -1018,7 +1018,7 @@ fn check_version_mismatch(conn: &Connection) -> bool { /// Build InsertNodesBatch from parsed file symbols. fn build_insert_batches( - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) -> Vec { file_symbols .iter() @@ -1155,7 +1155,7 @@ const EDGE_NODE_KIND_FILTER: &str = "kind IN ('function','method','class','inter /// ultimate definition files barrel chains resolve to. Mirrors the JS /// `relevantFiles` accumulation in `loadNodes` (#976, greptile P1). fn compute_edge_relevant_files( - file_symbols: &HashMap, + file_symbols: &BTreeMap, import_ctx: &crate::domain::graph::builder::stages::import_edges::ImportEdgeContext, ) -> HashSet { let mut relevant_files: HashSet = file_symbols.keys().cloned().collect(); @@ -1192,7 +1192,7 @@ fn compute_edge_relevant_files( /// `Vec` suitable for the native edge builder. fn load_edge_node_set( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, import_ctx: &crate::domain::graph::builder::stages::import_edges::ImportEdgeContext, is_incremental: bool, ) -> Vec { @@ -1354,7 +1354,7 @@ fn collect_imported_names_for_file( /// so method calls and receiver edges on that variable resolve. Must run /// before `build_and_insert_call_edges`. fn propagate_return_types_across_files( - file_symbols: &mut HashMap, + file_symbols: &mut BTreeMap, import_ctx: &ImportEdgeContext, ) { use crate::domain::graph::builder::stages::build_edges::PROPAGATION_HOP_PENALTY; @@ -1386,7 +1386,7 @@ fn propagate_return_types_across_files( /// - `global_return_types`: flat map for qualified `Type.method` lookups; higher /// confidence wins, tie-break is deterministic (paths visited in sorted order). fn build_return_type_index( - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) -> ( HashMap>, HashMap, @@ -1511,7 +1511,7 @@ fn insert_call_edge_rows(conn: &Connection, edges: &[crate::domain::graph::build /// Full builds always load every node — there is no smaller set anyway. fn build_and_insert_call_edges( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, import_ctx: &ImportEdgeContext, is_incremental: bool, ) { @@ -1669,7 +1669,7 @@ fn build_analysis_node_map( /// Convert FileSymbols AST nodes to FileAstBatch format for `ast::do_insert_ast_nodes`. fn build_ast_batches( - file_symbols: &HashMap, + file_symbols: &BTreeMap, analysis_files: &HashSet<&str>, ) -> Vec { let mut batches = Vec::new(); @@ -1698,7 +1698,7 @@ fn build_ast_batches( /// Write complexity metrics from parsed definitions to the `function_complexity` table. fn write_complexity( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, analysis_files: &HashSet<&str>, node_id_map: &HashMap<(String, String, u32), i64>, ) -> bool { @@ -1777,7 +1777,7 @@ fn write_complexity( /// Write CFG blocks and edges from parsed definitions to DB tables. fn write_cfg( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, analysis_files: &HashSet<&str>, node_id_map: &HashMap<(String, String, u32), i64>, ) -> bool { @@ -1882,7 +1882,7 @@ fn write_def_cfg( /// `makeNodeResolver` logic (prefer same-file match, fall back to global). fn write_dataflow( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, analysis_files: &HashSet<&str>, ) -> bool { let tx = match conn.unchecked_transaction() { @@ -2050,7 +2050,7 @@ mod tests { use super::*; use crate::types::{Import, PathAliases}; - fn make_import_ctx(file_symbols: &HashMap) -> ImportEdgeContext { + fn make_import_ctx(file_symbols: &BTreeMap) -> ImportEdgeContext { let mut batch_resolved = HashMap::new(); batch_resolved.insert("/repo/driver.js|./service.js".to_string(), "service.js".to_string()); ImportEdgeContext { @@ -2089,7 +2089,7 @@ mod tests { receiver_type_name: None, }); - let mut file_symbols = HashMap::new(); + let mut file_symbols = BTreeMap::new(); file_symbols.insert("service.js".to_string(), service); file_symbols.insert("driver.js".to_string(), driver); let import_ctx = make_import_ctx(&file_symbols); @@ -2120,7 +2120,7 @@ mod tests { receiver_type_name: Some("Factory".to_string()), }); - let mut file_symbols = HashMap::new(); + let mut file_symbols = BTreeMap::new(); file_symbols.insert("factory.js".to_string(), factory); file_symbols.insert("driver.js".to_string(), driver); let import_ctx = make_import_ctx(&file_symbols); @@ -2151,7 +2151,7 @@ mod tests { receiver_type_name: None, }); - let mut file_symbols = HashMap::new(); + let mut file_symbols = BTreeMap::new(); file_symbols.insert("service.js".to_string(), service); file_symbols.insert("driver.js".to_string(), driver); let import_ctx = make_import_ctx(&file_symbols); 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 c8f0487e8..9f1f97d2b 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 @@ -8,7 +8,7 @@ use crate::domain::graph::builder::barrel_resolution::{self, BarrelContext, Reex use crate::domain::graph::resolve; use crate::types::{FileSymbols, PathAliases}; use rusqlite::Connection; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::Path; /// A resolved reexport entry for a barrel file. @@ -28,7 +28,7 @@ pub struct ImportEdgeContext { /// Set of files that are barrel-only (reexport count >= definition count). pub barrel_only_files: HashSet, /// Parsed symbols per relative path. - pub file_symbols: HashMap, + pub file_symbols: BTreeMap, /// Root directory. pub root_dir: String, /// Path aliases. @@ -601,7 +601,7 @@ mod tests { #[test] fn barrel_detection() { - let mut file_symbols = HashMap::new(); + let mut file_symbols = BTreeMap::new(); // 1 def, 2 reexports → barrel file_symbols.insert( "src/index.ts".to_string(), diff --git a/crates/codegraph-core/src/features/structure.rs b/crates/codegraph-core/src/features/structure.rs index 7b9897cb7..ebcf6d62c 100644 --- a/crates/codegraph-core/src/features/structure.rs +++ b/crates/codegraph-core/src/features/structure.rs @@ -9,7 +9,7 @@ use crate::types::FileSymbols; use rusqlite::Connection; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; /// Per-file metrics to upsert into node_metrics. #[derive(Debug, Clone)] @@ -25,7 +25,7 @@ pub struct FileMetrics { /// Build line count map from parsed file symbols. pub fn build_line_count_map( - file_symbols: &HashMap, + file_symbols: &BTreeMap, root_dir: &str, ) -> HashMap { let mut map = HashMap::new(); @@ -50,7 +50,7 @@ pub fn update_changed_file_metrics( conn: &Connection, changed_files: &[String], line_count_map: &HashMap, - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) { if changed_files.is_empty() { return; @@ -250,7 +250,7 @@ struct ImportEdge { /// and contains-edge insertion to affected directories only. pub fn build_full_structure( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, discovered_dirs: &HashSet, root_dir: &str, line_count_map: &HashMap, @@ -408,7 +408,7 @@ fn load_file_paths_in_dirs(conn: &Connection, dirs: &HashSet) -> Vec, + file_symbols: &BTreeMap, all_file_paths: &[String], affected_dirs: Option<&HashSet>, ) { @@ -508,7 +508,7 @@ fn restore_unchanged_dir_edges( fn insert_contains_edges( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, all_dirs: &HashSet, changed_files: Option<&[String]>, ) { @@ -595,7 +595,7 @@ fn compute_import_edge_maps( fn compute_file_metrics( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, line_count_map: &HashMap, fan_in_map: &HashMap, fan_out_map: &HashMap, @@ -718,7 +718,7 @@ fn record_file_in_ancestor_dirs<'a>( fn build_dir_files_map<'a>( all_dirs: &'a HashSet, all_db_files: &'a [String], - file_symbols: &'a HashMap, + file_symbols: &'a BTreeMap, ) -> HashMap<&'a str, Vec<&'a str>> { let mut dir_files: HashMap<&str, Vec<&str>> = HashMap::new(); for dir in all_dirs { @@ -831,7 +831,7 @@ fn count_distinct_definitions(sym: &FileSymbols) -> i64 { fn compute_dir_symbol_counts<'a>( dir_files: &HashMap<&'a str, Vec<&'a str>>, db_symbol_counts: &HashMap, - file_symbols: &HashMap, + file_symbols: &BTreeMap, ) -> HashMap<&'a str, i64> { let mut dir_symbol_counts: HashMap<&str, i64> = HashMap::new(); for (dir, files) in dir_files { @@ -897,7 +897,7 @@ fn write_directory_metric_rows( fn compute_directory_metrics( conn: &Connection, - file_symbols: &HashMap, + file_symbols: &BTreeMap, all_dirs: &HashSet, import_edges: &[ImportEdge], ) { @@ -920,7 +920,7 @@ mod tests { #[test] fn line_count_map_from_symbols() { - let mut file_symbols = HashMap::new(); + let mut file_symbols = BTreeMap::new(); let mut sym = FileSymbols::new("src/a.ts".to_string()); sym.line_count = Some(42); file_symbols.insert("src/a.ts".to_string(), sym.clone()); diff --git a/crates/codegraph-core/src/graph/algorithms/louvain.rs b/crates/codegraph-core/src/graph/algorithms/louvain.rs index 165a5c27b..eb36ffe24 100644 --- a/crates/codegraph-core/src/graph/algorithms/louvain.rs +++ b/crates/codegraph-core/src/graph/algorithms/louvain.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use crate::shared::constants::{ DEFAULT_RANDOM_SEED, LOUVAIN_MAX_LEVELS, LOUVAIN_MAX_PASSES, LOUVAIN_MIN_GAIN, @@ -48,9 +48,16 @@ pub fn louvain_communities( } /// Internal state for the Louvain multi-level loop. +/// +/// `cur_edges` uses `BTreeMap` (not `HashMap`) so that iteration order is +/// deterministic across process runs. Rust's default `HashMap` hasher is +/// randomly seeded per-process (DoS resistance), so iterating a `HashMap` +/// here would silently reorder the adjacency list built in +/// `local_move_phase` on every run, changing which local optimum the greedy +/// local-move phase converges to even with a fixed `rng_state` seed (#1734). struct LouvainState { cur_n: usize, - cur_edges: HashMap<(usize, usize), f64>, + cur_edges: BTreeMap<(usize, usize), f64>, cur_degree: Vec, original_community: Vec, rng_state: u32, @@ -61,15 +68,17 @@ fn louvain_init( edges: &[GraphEdge], node_ids: &[String], seed: u32, -) -> (HashMap<(usize, usize), f64>, f64, LouvainState) { +) -> (BTreeMap<(usize, usize), f64>, f64, LouvainState) { let n = node_ids.len(); let mut id_to_idx: HashMap<&str, usize> = HashMap::with_capacity(n); for (i, id) in node_ids.iter().enumerate() { id_to_idx.insert(id.as_str(), i); } - // Build undirected weighted edge list (deduplicate, merge parallel edges) - let mut edge_map: HashMap<(usize, usize), f64> = HashMap::new(); + // Build undirected weighted edge list (deduplicate, merge parallel edges). + // BTreeMap keeps this deterministically ordered by (src, tgt) — see the + // `LouvainState.cur_edges` doc comment above for why this matters. + let mut edge_map: BTreeMap<(usize, usize), f64> = BTreeMap::new(); for edge in edges { if let (Some(&src), Some(&tgt)) = ( id_to_idx.get(edge.source.as_str()), @@ -139,13 +148,21 @@ fn local_move_phase( } let mut any_moved = false; + // BTreeMap (not HashMap) so the best-move scan below visits candidate + // communities in a fixed, deterministic order — otherwise a genuine tie + // in `gain` would be broken by Rust's per-process-randomized HashMap + // iteration order instead of a reproducible rule (#1734). Hoisted out of + // the node loop and cleared per-iteration instead of reallocated, since + // `cur_n * LOUVAIN_MAX_PASSES` fresh allocations would otherwise show up + // on very high-degree hub nodes. + let mut comm_w: BTreeMap = BTreeMap::new(); for _pass in 0..LOUVAIN_MAX_PASSES { let mut pass_moved = false; for &node in &order { let node_comm = level_comm[node]; let node_deg = state.cur_degree[node]; - let mut comm_w: HashMap = HashMap::new(); + comm_w.clear(); for &(neighbor, w) in &adj[node] { *comm_w.entry(level_comm[neighbor]).or_insert(0.0) += w; } @@ -216,7 +233,7 @@ fn aggregation_phase( } // Build coarse graph for next level - let mut coarse_edge_map: HashMap<(usize, usize), f64> = HashMap::new(); + let mut coarse_edge_map: BTreeMap<(usize, usize), f64> = BTreeMap::new(); for (&(src, tgt), &w) in &state.cur_edges { let cu = level_comm[src]; let cv = level_comm[tgt]; @@ -241,7 +258,7 @@ fn aggregation_phase( /// Compute final modularity score: Q = sum_c [ L_c / m - gamma * (k_c / 2m)^2 ] fn compute_modularity( - edge_map: &HashMap<(usize, usize), f64>, + edge_map: &BTreeMap<(usize, usize), f64>, original_community: &[usize], total_weight: f64, resolution: f64, @@ -398,4 +415,65 @@ mod tests { assert_eq!(map["a"], map["b"]); assert_eq!(map["b"], map["c"]); } + + /// Regression test for #1734: `codegraph communities --drift` produced + /// different modularity/community assignments across separate full + /// rebuilds of byte-identical source. Root cause: `local_move_phase` + /// accumulated per-candidate-community weights in a `HashMap`, whose + /// iteration order is randomized per-process — so a genuine tie in + /// modularity gain between candidate communities was broken by hashmap + /// bucket order instead of a reproducible rule, even with a fixed + /// `random_seed`. Fixed by switching `cur_edges`/`comm_w` to `BTreeMap`. + /// + /// This graph is symmetric by construction — three disjoint triangles + /// plus a bridge node connected with equal weight to one member of each + /// triangle — so moving the bridge node into any of the three triangles + /// yields the exact same modularity gain, forcing a genuine tie on every + /// run of the local-move phase. + #[test] + fn test_louvain_deterministic_across_repeated_calls_with_tie() { + let edges = vec![ + edge("a1", "a2"), + edge("a2", "a3"), + edge("a3", "a1"), + edge("b1", "b2"), + edge("b2", "b3"), + edge("b3", "b1"), + edge("c1", "c2"), + edge("c2", "c3"), + edge("c3", "c1"), + edge("x", "a1"), + edge("x", "b1"), + edge("x", "c1"), + ]; + let nodes: Vec = vec!["a1", "a2", "a3", "b1", "b2", "b3", "c1", "c2", "c3", "x"] + .into_iter() + .map(String::from) + .collect(); + + let mut snapshots: Vec<(String, f64)> = Vec::new(); + for _ in 0..30 { + let result = louvain_communities(edges.clone(), nodes.clone(), Some(1.0), Some(42)); + let mut pairs: Vec = result + .assignments + .iter() + .map(|a| format!("{}:{}", a.node, a.community)) + .collect(); + pairs.sort(); + snapshots.push((pairs.join(","), result.modularity)); + } + + let first = &snapshots[0]; + for (i, snapshot) in snapshots.iter().enumerate().skip(1) { + assert_eq!( + snapshot.0, first.0, + "run {i} produced a different assignment than run 0 — \ + tie-breaking is not deterministic" + ); + assert_eq!( + snapshot.1, first.1, + "run {i} produced a different modularity than run 0" + ); + } + } } diff --git a/src/db/repository/graph-read.ts b/src/db/repository/graph-read.ts index 5dc0e9180..3c5ed1b3f 100644 --- a/src/db/repository/graph-read.ts +++ b/src/db/repository/graph-read.ts @@ -19,44 +19,50 @@ const CALLABLE_KINDS_SQL = CORE_SYMBOL_KINDS.map((k: string) => `'${k}'`).join(' /** * Get callable nodes (all core symbol kinds) for graph construction. + * + * `ORDER BY id` — without an explicit order, SQLite's row order for a bare + * WHERE scan is unspecified. Consumers (e.g. community detection's graph + * builder) rely on a stable iteration order for run-to-run determinism, so + * sort explicitly rather than depending on incidental physical/insertion + * order (#1734). Mirrors `get_callable_nodes` in the native `graph_read.rs`. */ export function getCallableNodes(db: BetterSqlite3Database): CallableNodeRow[] { return cachedStmt( _getCallableNodesStmt, db, - `SELECT id, name, kind, file FROM nodes WHERE kind IN (${CALLABLE_KINDS_SQL})`, + `SELECT id, name, kind, file FROM nodes WHERE kind IN (${CALLABLE_KINDS_SQL}) ORDER BY id`, ).all(); } /** - * Get all 'calls' edges. + * Get all 'calls' edges. Ordered for determinism — see `getCallableNodes`. */ export function getCallEdges(db: BetterSqlite3Database): CallEdgeRow[] { return cachedStmt( _getCallEdgesStmt, db, - "SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls'", + "SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls' ORDER BY source_id, target_id", ).all(); } /** - * Get all file-kind nodes. + * Get all file-kind nodes. Ordered for determinism — see `getCallableNodes`. */ export function getFileNodesAll(db: BetterSqlite3Database): FileNodeRow[] { return cachedStmt( _getFileNodesAllStmt, db, - "SELECT id, name, file FROM nodes WHERE kind = 'file'", + "SELECT id, name, file FROM nodes WHERE kind = 'file' ORDER BY id", ).all(); } /** - * Get all import edges. + * Get all import edges. Ordered for determinism — see `getCallableNodes`. */ export function getImportEdges(db: BetterSqlite3Database): ImportGraphEdgeRow[] { return cachedStmt( _getImportEdgesStmt, db, - "SELECT source_id, target_id FROM edges WHERE kind IN ('imports','imports-type')", + "SELECT source_id, target_id FROM edges WHERE kind IN ('imports','imports-type') ORDER BY source_id, target_id", ).all(); } diff --git a/tests/graph/algorithms/louvain.test.ts b/tests/graph/algorithms/louvain.test.ts index d7bcc8110..0002873e0 100644 --- a/tests/graph/algorithms/louvain.test.ts +++ b/tests/graph/algorithms/louvain.test.ts @@ -47,6 +47,97 @@ describe('louvainCommunities', () => { expect(modularity).toBe(0); }); + // Regression test for #1734: `codegraph communities --drift` produced + // different modularity/community assignments across separate full rebuilds + // of byte-identical source. Root cause: the native Rust local-move phase + // accumulated per-candidate-community weights in a `std::collections::HashMap` + // (randomized per-process hasher), so a genuine tie in modularity gain + // between two or more candidate communities was broken by hashmap iteration + // order instead of a reproducible rule. Fixed by switching the relevant + // maps to `BTreeMap` (deterministic, sorted iteration). + // + // This graph is symmetric by construction: three disjoint triangles, plus a + // bridge node connected with equal weight to one member of each triangle. + // Moving the bridge node into any of the three triangles yields the exact + // same modularity gain, forcing the local-move phase to break a genuine + // tie on every run. + describe('determinism (#1734)', () => { + function buildTieGraph(): CodeGraph { + const g = new CodeGraph(); + g.addEdge('a1', 'a2'); + g.addEdge('a2', 'a3'); + g.addEdge('a3', 'a1'); + g.addEdge('b1', 'b2'); + g.addEdge('b2', 'b3'); + g.addEdge('b3', 'b1'); + g.addEdge('c1', 'c2'); + g.addEdge('c2', 'c3'); + g.addEdge('c3', 'c1'); + // Bridge node tied equally between all three triangles. + g.addEdge('x', 'a1'); + g.addEdge('x', 'b1'); + g.addEdge('x', 'c1'); + return g; + } + + /** Sorted "node:community" pairs — stable snapshot for deep-equal comparison. */ + function snapshotAssignments(assignments: Map): string[] { + return [...assignments.entries()].map(([node, community]) => `${node}:${community}`).sort(); + } + + it('produces byte-identical modularity and assignments across repeated runs', () => { + const runs = Array.from({ length: 20 }, () => { + const g = buildTieGraph(); + return louvainCommunities(g); + }); + + const firstModularity = runs[0]!.modularity; + const firstAssignments = snapshotAssignments(runs[0]!.assignments); + + for (const run of runs.slice(1)) { + expect(run.modularity).toBe(firstModularity); + expect(snapshotAssignments(run.assignments)).toEqual(firstAssignments); + } + }); + + // Note: `buildTieGraph()` above is *symmetric by design* — the bridge + // node's three-way tie means a different edge-insertion order can validly + // land it in a different (but equally optimal) community. That is + // expected Louvain behavior, not a bug, so it is not asserted here. + // Order-independence is only a meaningful invariant when the optimal + // partition is unambiguous, as below. + it('produces an equivalent partition regardless of edge insertion order (unambiguous graph)', () => { + function buildUnambiguousGraph(edges: Array<[string, string]>): CodeGraph { + const g = new CodeGraph(); + for (const [src, tgt] of edges) g.addEdge(src, tgt); + return g; + } + + // Two tightly-connected triangles joined by a single weak bridge edge — + // the best partition (two triangles) is unambiguous, so insertion order + // must not change which nodes end up grouped together. + const edges: Array<[string, string]> = [ + ['a', 'b'], + ['b', 'c'], + ['c', 'a'], + ['x', 'y'], + ['y', 'z'], + ['z', 'x'], + ['c', 'x'], + ]; + + const forwardResult = louvainCommunities(buildUnambiguousGraph(edges)); + const reversedResult = louvainCommunities(buildUnambiguousGraph(edges.slice().reverse())); + + expect(reversedResult.modularity).toBe(forwardResult.modularity); + expect(reversedResult.assignments.get('a')).toBe(reversedResult.assignments.get('b')); + expect(reversedResult.assignments.get('b')).toBe(reversedResult.assignments.get('c')); + expect(reversedResult.assignments.get('x')).toBe(reversedResult.assignments.get('y')); + expect(reversedResult.assignments.get('y')).toBe(reversedResult.assignments.get('z')); + expect(reversedResult.assignments.get('a')).not.toBe(reversedResult.assignments.get('x')); + }); + }); + describe('Leiden-knob parity logging', () => { let stderrSpy: ReturnType; diff --git a/tests/integration/issue-1734-communities-determinism.test.ts b/tests/integration/issue-1734-communities-determinism.test.ts new file mode 100644 index 000000000..17edc0415 --- /dev/null +++ b/tests/integration/issue-1734-communities-determinism.test.ts @@ -0,0 +1,117 @@ +/** + * Regression test for #1734: `codegraph communities --drift` produced + * different modularity/community assignments across separate full rebuilds + * of byte-identical source code. + * + * Root causes (both fixed): + * 1. The native build pipeline collected parsed file symbols into a + * `std::collections::HashMap` (`crates/codegraph-core/.../pipeline.rs`), + * whose iteration order is randomized per-process. That order drove + * node/edge insertion order into SQLite, so the same file could get a + * different autoincrement `id` — and hence a different position in the + * in-memory graph — on every rebuild. + * 2. The native Louvain local-move phase (`graph/algorithms/louvain.rs`) + * accumulated per-candidate-community weights in a `HashMap`, so a + * genuine tie in modularity gain was broken by hashmap iteration order + * instead of a reproducible rule, even with a fixed random seed. + * + * This test builds a small but non-trivial fixture (three tightly-connected + * clusters bridged by one file that imports equally from each) into two + * independent full rebuilds and asserts the community-detection output is + * identical between them — exercising the real end-to-end pipeline rather + * than just the in-memory algorithm. + */ +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/pipeline.js'; +import { communitiesData } from '../../src/features/communities.js'; + +let tmpDir: string; +const dbPathA = () => path.join(tmpDir, 'a.db'); +const dbPathB = () => path.join(tmpDir, 'b.db'); + +function clusterFile(clusterName: string, index: number, peers: number[]): string { + const imports = peers + .map((p) => `import { ${clusterName}${p} } from './${clusterName}${p}.js';`) + .join('\n'); + const uses = peers.map((p) => ` ${clusterName}${p}();`).join('\n'); + return `${imports} +export function ${clusterName}${index}() { +${uses} + return ${index}; +} +`; +} + +beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-communities-determinism-')); + + // Three fully-connected triangles (clusters A, B, C) — each file imports + // both peers in its own cluster. + for (const cluster of ['a', 'b', 'c']) { + for (let i = 1; i <= 3; i++) { + const peers = [1, 2, 3].filter((p) => p !== i); + fs.writeFileSync(path.join(tmpDir, `${cluster}${i}.js`), clusterFile(cluster, i, peers)); + } + } + + // Bridge file imports equally from one member of each cluster — symmetric + // three-way tie, mirroring the unit-level regression test in + // tests/graph/algorithms/louvain.test.ts. + fs.writeFileSync( + path.join(tmpDir, 'bridge.js'), + `import { a1 } from './a1.js'; +import { b1 } from './b1.js'; +import { c1 } from './c1.js'; +export function bridge() { + a1(); + b1(); + c1(); +} +`, + ); + fs.writeFileSync(path.join(tmpDir, 'package.json'), '{"name":"test"}'); + + // Two independent full rebuilds of the identical source, into separate DBs. + await buildGraph(tmpDir, { dbPath: dbPathA(), incremental: false }); + await buildGraph(tmpDir, { dbPath: dbPathB(), incremental: false }); +}); + +afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe('communities determinism across independent full rebuilds (#1734)', () => { + it('produces identical modularity and community structure', () => { + const resultA = communitiesData(dbPathA(), { noTests: true }); + const resultB = communitiesData(dbPathB(), { noTests: true }); + + expect(resultA.modularity).toBe(resultB.modularity); + expect((resultA.summary as { communityCount: number }).communityCount).toBe( + (resultB.summary as { communityCount: number }).communityCount, + ); + expect((resultA.summary as { driftScore: number }).driftScore).toBe( + (resultB.summary as { driftScore: number }).driftScore, + ); + + // Compare full community structure (files grouped per community, not raw + // numeric community IDs — those are arbitrary labels and may legitimately + // differ in assignment order between independent runs even when the + // underlying grouping is identical). + type CommunityShape = { members?: Array<{ file: string }> }; + const toFileSets = (data: Record): string[][] => + (data.communities as CommunityShape[]) + .map((c) => (c.members ?? []).map((m) => m.file).sort()) + .sort((x, y) => x.join(',').localeCompare(y.join(','))); + + expect(toFileSets(resultA)).toEqual(toFileSets(resultB)); + + // Drift output (the exact shape returned by `communities --drift`) must + // also match byte-for-byte. + const driftA = communitiesData(dbPathA(), { noTests: true, drift: true }); + const driftB = communitiesData(dbPathB(), { noTests: true, drift: true }); + expect(driftA).toEqual(driftB); + }); +});