Merge pull request #27 from thinkgrid-labs/feat/code-graph

fix(graph): refresh the code graph before every review

Author: akosidencio

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

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,

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<Vec<String>>,
+    /// 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<bool>,
 }
 
 #[derive(Debug, Deserialize, Default, Clone)]

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");

apps/tui-cli/src/main.rs

@@ -103,6 +103,11 @@ fn run() -> Result<i32> {
     // 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<String> {
 /// 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 {

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<String>,
+    /// Refresh the code graph before reviewing.
+    pub auto_index: bool,
 }
 
 pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result<Settings> {
@@ -135,6 +137,7 @@ pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result<Settings> {
         // *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,