diff --git a/CHANGELOG.md b/CHANGELOG.md index f65d762..35a0c07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,9 @@ else's branch. Both surfaces share one engine; neither replaces the other. enclosing definition, referenced definitions, and the file's tests. Everything stays inside a byte budget, so context does not grow with the repository. Bodies are read from the working tree rather than stored, so a snippet can - never disagree with the file being reviewed. Adding a language is one entry in + never disagree with the file being reviewed. The graph refreshes itself + incrementally before every review (~0.1s on 647 unchanged files) and builds on + first use, so it can never fall behind the code; `--no-index` opts out. Adding a language is one entry in a table; contributions welcome. - **Cross-file review units.** When a symbol and code that calls it both change diff --git a/apps/tui-cli/src/cli.rs b/apps/tui-cli/src/cli.rs index 10847ae..67f451e 100644 --- a/apps/tui-cli/src/cli.rs +++ b/apps/tui-cli/src/cli.rs @@ -119,6 +119,11 @@ pub struct Cli { #[arg(long)] pub no_cache: bool, + /// Do not refresh the code graph before reviewing. Faster, but findings are + /// judged against whatever the graph last saw. + #[arg(long)] + pub no_index: bool, + /// Ignore `.diffmind/baseline.json` for this run #[arg(long)] pub no_baseline: bool, diff --git a/apps/tui-cli/src/config.rs b/apps/tui-cli/src/config.rs index 4b05245..2b909b3 100644 --- a/apps/tui-cli/src/config.rs +++ b/apps/tui-cli/src/config.rs @@ -39,6 +39,9 @@ pub struct ReviewConfig { /// Same syntax as a rule's `files`: `*.ts`, `**/legacy/**`, or an exact path. #[serde(default)] pub ignore: Option>, + /// Refresh the code graph before each review. On by default — a stale graph + /// reports wrong line ranges, not merely missing ones. + pub auto_index: Option, } #[derive(Debug, Deserialize, Default, Clone)] diff --git a/apps/tui-cli/src/graph/store.rs b/apps/tui-cli/src/graph/store.rs index 7eeedce..ae15c1c 100644 --- a/apps/tui-cli/src/graph/store.rs +++ b/apps/tui-cli/src/graph/store.rs @@ -488,6 +488,50 @@ pub fn refresh(token: &str) -> bool { let _ = std::fs::remove_dir_all(&root); } + /// Why the graph is refreshed before every review rather than on demand. + /// + /// A stale graph does not merely miss new code — its line ranges point at + /// whatever now occupies those lines, and `source` reads the working tree, + /// so the model is handed unrelated code labelled as the enclosing symbol. + /// Confidently wrong context is worse than none. + #[test] + fn a_stale_graph_reports_the_wrong_lines_until_reindexed() { + let root = project("stale"); + write( + &root, + "src/a.rs", + "pub fn target() {\n let secret = 1;\n}\n", + ); + let mut g = Graph::open(&root).unwrap(); + g.index(&root, &|_| {}).unwrap(); + + let before = g.definitions_of("target", None, 1).remove(0); + assert!(before.source(&root, 100).unwrap().contains("let secret")); + + // Someone adds imports at the top — utterly ordinary. + std::thread::sleep(std::time::Duration::from_millis(20)); + write( + &root, + "src/a.rs", + "// added\n// added\n// added\n// added\n// added\npub fn target() {\n let secret = 1;\n}\n", + ); + + let stale = g.definitions_of("target", None, 1).remove(0); + assert!( + !stale.source(&root, 100).unwrap().contains("let secret"), + "this is the failure mode being guarded against" + ); + + // Re-indexing is what makes it right again, and is cheap enough to do + // before every review. + g.index(&root, &|_| {}).unwrap(); + let fresh = g.definitions_of("target", None, 1).remove(0); + assert_eq!(fresh.start_line, 6); + assert!(fresh.source(&root, 100).unwrap().contains("let secret")); + + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn a_deleted_file_drops_out_of_the_graph() { let root = project("deleted"); diff --git a/apps/tui-cli/src/main.rs b/apps/tui-cli/src/main.rs index 3826960..7d07ac6 100644 --- a/apps/tui-cli/src/main.rs +++ b/apps/tui-cli/src/main.rs @@ -103,6 +103,11 @@ fn run() -> Result { // surfaces review exactly the same set of hunks. let (diff, prefilter) = apply_prefilter(&diff, &settings, &project_root)?; + // Refresh the graph before either surface reads it. + if settings.auto_index { + sync_graph(&project_root, settings.format.is_machine_readable()); + } + if prefilter.dropped_everything() { // Distinct from "no changes": there *were* changes, and none of them // were worth a reviewer's attention. Reporting zero findings without @@ -725,6 +730,41 @@ fn read_head(path: &Path) -> std::io::Result { /// a symbol lookup rather than a re-read of `symbols.json`. Returning a closure /// (instead of one context string for the whole diff) is what keeps each /// chunk's cache key independent of the other files in the diff. +/// Bring the code graph up to date before reviewing. +/// +/// Incremental and mtime-keyed: re-checking an unchanged 647-file repository +/// costs about a tenth of a second, which is nothing beside one inference pass. +/// +/// Doing it automatically matters more than the cost. A stale graph does not +/// merely miss new code — it reports **wrong line ranges**, and `Def::source` +/// then reads those lines out of the working tree and hands the model unrelated +/// code labelled as the enclosing function. Confidently wrong context is worse +/// than none, so the default is to never let the graph fall behind. +/// +/// Never fatal: the graph is an optimisation, and a review must still run +/// without it. +fn sync_graph(project_root: &Path, quiet: bool) { + let Ok(mut graph) = Graph::open(project_root) else { + return; + }; + // Only the first build is slow enough to be worth a spinner. + let spinner = (graph.is_empty() && !quiet) + .then(|| make_spinner("Building code graph (first run)...", false)); + + let progress = |n: usize| { + if let Some(s) = &spinner { + s.set_message(format!("Building code graph... {n} files")); + } + }; + if let Err(e) = graph.index(project_root, &progress) { + eprintln!(" ! code graph not refreshed: {e}"); + } + if let Some(s) = spinner { + s.finish_and_clear(); + } + runs::ensure_gitignore(project_root); +} + /// Merge units the code graph says are two halves of one change. Without a /// graph this is the identity function, so behaviour is unchanged. pub fn unit_grouper(project_root: &Path) -> core_engine::analyzer::UnitGrouper { diff --git a/apps/tui-cli/src/settings.rs b/apps/tui-cli/src/settings.rs index b43fe87..ea98533 100644 --- a/apps/tui-cli/src/settings.rs +++ b/apps/tui-cli/src/settings.rs @@ -71,6 +71,8 @@ pub struct Settings { pub debug: bool, /// Extra globs dropped by the pre-filter, from `.diffmind/config.toml`. pub ignore_globs: Vec, + /// Refresh the code graph before reviewing. + pub auto_index: bool, } pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result { @@ -135,6 +137,7 @@ pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result { // *supposed* to differ, and replaying one would be a lie. use_cache: !cli.no_cache && resolve(None, r.cache, true) && temperature == 0.0, ignore_globs: r.ignore.clone().unwrap_or_default(), + auto_index: !cli.no_index && resolve(None, r.auto_index, true), use_baseline: !cli.no_baseline, use_daemon: !cli.no_daemon, debug: cli.debug,