Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions apps/tui-cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions apps/tui-cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
44 changes: 44 additions & 0 deletions apps/tui-cli/src/graph/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
40 changes: 40 additions & 0 deletions apps/tui-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions apps/tui-cli/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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,
Expand Down
Loading