From b8326658211322d954f4e2b5110a5913032615ba Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 2 Jun 2026 11:32:31 -0400 Subject: [PATCH 1/3] feat: dedupe auto review runs by diff fingerprint --- code-rs/core/src/review_store.rs | 220 ++++++++++++++++++++++++++ code-rs/tui/src/chatwidget.rs | 263 ++++++++++++++++++++++++++++++- 2 files changed, 477 insertions(+), 6 deletions(-) diff --git a/code-rs/core/src/review_store.rs b/code-rs/core/src/review_store.rs index 3515cc66fcc..6f59a8afd92 100644 --- a/code-rs/core/src/review_store.rs +++ b/code-rs/core/src/review_store.rs @@ -220,6 +220,27 @@ impl AutoReviewRun { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AutoReviewDuplicateDisposition { + Adopt, + ReuseTerminal, + SupersedeTerminal, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AutoReviewDuplicateMatch { + pub run_id: Uuid, + pub status: AutoReviewRunStatus, + pub disposition: AutoReviewDuplicateDisposition, + pub agent_id: Option, + pub snapshot_commit: Option, + pub owner_session_id: Option, + pub worktree_path: Option, + pub branch: Option, + pub finding_count: usize, + pub summary_digest: Option, +} + #[derive(Debug, Clone, Copy)] pub struct AutoReviewLedgerOptions { pub max_bytes: usize, @@ -369,6 +390,89 @@ impl AutoReviewRunStore { Ok(changed) } + pub fn find_duplicate_by_fingerprint( + &self, + diff_fingerprint: &str, + ) -> Option { + self.find_duplicate_by_fingerprint_excluding(diff_fingerprint, None) + } + + pub fn find_duplicate_by_fingerprint_excluding( + &self, + diff_fingerprint: &str, + excluded_run_id: Option, + ) -> Option { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return None; + } + + self.runs + .values() + .filter(|run| Some(run.run_id) != excluded_run_id) + .filter(|run| run.diff_fingerprint.as_deref() == Some(fingerprint)) + .filter(|run| { + !matches!( + run.status, + AutoReviewRunStatus::Lost + | AutoReviewRunStatus::Skipped + | AutoReviewRunStatus::Superseded + ) + }) + .max_by(|left, right| { + duplicate_priority(left) + .cmp(&duplicate_priority(right)) + .then_with(|| left.updated_at.cmp(&right.updated_at)) + .then_with(|| left.created_at.cmp(&right.created_at)) + .then_with(|| left.run_id.cmp(&right.run_id)) + }) + .map(|run| AutoReviewDuplicateMatch { + run_id: run.run_id, + status: run.status, + disposition: duplicate_disposition(run), + agent_id: run.agent_id.clone(), + snapshot_commit: run.snapshot_commit.clone(), + owner_session_id: run.owner_session_id, + worktree_path: run.worktree_path.clone(), + branch: run.branch.clone().or_else(|| run.batch_id.clone()), + finding_count: run.finding_count, + summary_digest: run.summary_digest.clone(), + }) + } + + pub fn mark_superseded_by_fingerprint( + &mut self, + diff_fingerprint: &str, + superseded_by: Uuid, + now: u64, + ) -> io::Result { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return Ok(0); + } + + let mut changed = 0usize; + for run in self.runs.values_mut() { + if run.run_id == superseded_by + || run.diff_fingerprint.as_deref() != Some(fingerprint) + || run.status.is_in_flight() + || run.status == AutoReviewRunStatus::Superseded + || run.finding_count > 0 + || !run.finding_digests.is_empty() + { + continue; + } + run.freshness = AutoReviewFreshness::Superseded; + run.superseded_by = Some(superseded_by); + run.mark_status(AutoReviewRunStatus::Superseded, now); + changed = changed.saturating_add(1); + } + if changed > 0 { + self.save()?; + } + Ok(changed) + } + pub fn write_output(&mut self, run_id: Uuid, output: &ReviewOutputEvent) -> io::Result { if !self.runs.contains_key(&run_id) { return Err(io::Error::new( @@ -409,6 +513,29 @@ impl AutoReviewRunStore { } } +fn duplicate_priority(run: &AutoReviewRun) -> u8 { + if run.status.is_in_flight() { + return 4; + } + if run.finding_count > 0 || !run.finding_digests.is_empty() { + return 3; + } + if run.status == AutoReviewRunStatus::Completed { + return 2; + } + 1 +} + +fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition { + if run.status.is_in_flight() { + AutoReviewDuplicateDisposition::Adopt + } else if run.status == AutoReviewRunStatus::Completed { + AutoReviewDuplicateDisposition::ReuseTerminal + } else { + AutoReviewDuplicateDisposition::SupersedeTerminal + } +} + pub fn auto_review_dir(scope: &Path) -> io::Result { Ok(scoped_review_state_dir(scope)?.join(AUTO_REVIEW_DIR)) } @@ -845,6 +972,99 @@ mod tests { assert!(loaded.get(second_id).is_some()); } + #[test] + #[serial] + fn duplicate_lookup_prefers_in_flight_fingerprint_match() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let completed_id = Uuid::new_v4(); + let mut completed = AutoReviewRun::new(completed_id, AutoReviewRunSource::Tui, 1); + completed.diff_fingerprint = Some("diff:abc".to_string()); + completed.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(completed).unwrap(); + + let live_id = Uuid::new_v4(); + let mut live = AutoReviewRun::new(live_id, AutoReviewRunSource::Tui, 3); + live.diff_fingerprint = Some("diff:abc".to_string()); + live.agent_id = Some("agent-live".to_string()); + live.snapshot_commit = Some("snap-live".to_string()); + live.mark_status(AutoReviewRunStatus::Reviewing, 4); + store.upsert(live).unwrap(); + + let duplicate = store + .find_duplicate_by_fingerprint("diff:abc") + .expect("duplicate"); + assert_eq!(duplicate.run_id, live_id); + assert_eq!(duplicate.disposition, AutoReviewDuplicateDisposition::Adopt); + assert_eq!(duplicate.agent_id.as_deref(), Some("agent-live")); + } + + #[test] + #[serial] + fn duplicate_lookup_reuses_terminal_finding_match() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let run_id = Uuid::new_v4(); + let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1); + run.diff_fingerprint = Some("diff:abc".to_string()); + run.finding_count = 1; + run.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(run).unwrap(); + + let duplicate = store + .find_duplicate_by_fingerprint("diff:abc") + .expect("duplicate"); + assert_eq!(duplicate.run_id, run_id); + assert_eq!( + duplicate.disposition, + AutoReviewDuplicateDisposition::ReuseTerminal + ); + } + + #[test] + #[serial] + fn supersede_by_fingerprint_omits_runs_with_findings() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let new_id = Uuid::new_v4(); + let clean_id = Uuid::new_v4(); + let finding_id = Uuid::new_v4(); + + let mut clean = AutoReviewRun::new(clean_id, AutoReviewRunSource::Tui, 1); + clean.diff_fingerprint = Some("diff:abc".to_string()); + clean.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(clean).unwrap(); + + let mut finding = AutoReviewRun::new(finding_id, AutoReviewRunSource::Tui, 3); + finding.diff_fingerprint = Some("diff:abc".to_string()); + finding.finding_count = 1; + finding.mark_status(AutoReviewRunStatus::Completed, 4); + store.upsert(finding).unwrap(); + + let changed = store + .mark_superseded_by_fingerprint("diff:abc", new_id, 10) + .unwrap(); + assert_eq!(changed, 1); + + let loaded = AutoReviewRunStore::open(repo.path()).unwrap(); + let clean = loaded.get(clean_id).unwrap(); + assert_eq!(clean.status, AutoReviewRunStatus::Superseded); + assert_eq!(clean.superseded_by, Some(new_id)); + + let finding = loaded.get(finding_id).unwrap(); + assert_eq!(finding.status, AutoReviewRunStatus::Completed); + assert_eq!(finding.superseded_by, None); + } + #[test] #[serial] fn run_store_reconciles_orphaned_in_flight_agents() { diff --git a/code-rs/tui/src/chatwidget.rs b/code-rs/tui/src/chatwidget.rs index 7613f9a2448..bdc8584cc81 100644 --- a/code-rs/tui/src/chatwidget.rs +++ b/code-rs/tui/src/chatwidget.rs @@ -5,6 +5,7 @@ use std::collections::hash_map::Entry; use std::collections::HashSet; use std::collections::VecDeque; use std::io; +use std::hash::{Hash, Hasher}; use std::path::{Path, PathBuf}; use std::rc::{Rc, Weak}; use std::sync::Arc; @@ -202,6 +203,7 @@ use code_core::protocol::TokenUsage; use code_core::protocol::TurnDiffEvent; use code_core::protocol::ViewImageToolCallEvent; use code_core::review_store::{ + AutoReviewDuplicateDisposition, AutoReviewFreshness, AutoReviewRun, AutoReviewRunSource, @@ -1652,6 +1654,124 @@ fn format_background_review_scope_from_paths(snapshot_id: &str, paths: &[String] Some(scope.trim_end().to_string()) } +#[derive(Clone, Debug)] +struct AutoReviewScopeMetadata { + scope_note: Option, + diff_excerpt: Option, + diff_fingerprint: Option, + changed_path_count: usize, + listed_paths: Vec, + omitted_path_count: usize, +} + +impl AutoReviewScopeMetadata { + fn empty() -> Self { + Self { + scope_note: None, + diff_excerpt: None, + diff_fingerprint: None, + changed_path_count: 0, + listed_paths: Vec::new(), + omitted_path_count: 0, + } + } +} + +fn auto_review_diff_fingerprint(parent_id: Option<&str>, paths: &[String], diff_text: &str) -> Option { + let normalized_diff = diff_text.trim(); + if normalized_diff.is_empty() { + return None; + } + + let mut normalized_paths = paths + .iter() + .map(|path| path.trim().trim_start_matches("./")) + .filter(|path| !path.is_empty()) + .collect::>(); + normalized_paths.sort_unstable(); + normalized_paths.dedup(); + + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + "auto-review-diff-v1".hash(&mut hasher); + parent_id.unwrap_or_default().hash(&mut hasher); + for path in normalized_paths { + path.hash(&mut hasher); + } + normalized_diff.hash(&mut hasher); + Some(format!("diff:{:016x}", hasher.finish())) +} + +fn auto_review_listed_paths(paths: &[String]) -> Vec { + let mut seen = HashSet::new(); + let mut listed = Vec::new(); + for raw_path in paths { + let normalized = raw_path.trim().trim_start_matches("./"); + if normalized.is_empty() || !seen.insert(normalized.to_string()) { + continue; + } + if should_include_auto_review_scope_path(normalized) { + listed.push(PathBuf::from(normalized)); + } + } + listed +} + +fn persist_auto_review_scope_metadata( + cwd: &Path, + run_id: Uuid, + metadata: &AutoReviewScopeMetadata, +) -> std::io::Result<()> { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + if let Some(run) = store.get_mut(run_id) { + run.diff_fingerprint = metadata.diff_fingerprint.clone(); + run.changed_path_count = metadata.changed_path_count; + run.listed_paths = metadata.listed_paths.clone(); + run.omitted_path_count = metadata.omitted_path_count; + run.mark_activity(now); + } + store.save() +} + +fn find_duplicate_auto_review_run( + cwd: &Path, + diff_fingerprint: &str, + current_run_id: Uuid, +) -> Option { + let store = AutoReviewRunStore::open_existing(cwd).ok().flatten()?; + store.find_duplicate_by_fingerprint_excluding(diff_fingerprint, Some(current_run_id)) +} + +fn mark_auto_review_run_skipped_duplicate( + cwd: &Path, + run_id: Uuid, + duplicate_run_id: Uuid, + diff_fingerprint: Option<&str>, +) -> std::io::Result<()> { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + if let Some(run) = store.get_mut(run_id) { + if let Some(diff_fingerprint) = diff_fingerprint { + run.diff_fingerprint = Some(diff_fingerprint.to_string()); + } + run.freshness = AutoReviewFreshness::Superseded; + run.superseded_by = Some(duplicate_run_id); + run.cancel_reason = Some("duplicate_auto_review_scope".to_string()); + run.mark_status(AutoReviewRunStatus::Skipped, now); + } + store.save() +} + +fn supersede_clean_duplicate_auto_reviews( + cwd: &Path, + diff_fingerprint: &str, + superseded_by: Uuid, +) -> std::io::Result { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + store.mark_superseded_by_fingerprint(diff_fingerprint, superseded_by, now) +} + fn build_background_review_prompt( snapshot_id: &str, scope_note: Option<&str>, @@ -31778,11 +31898,13 @@ async fn run_background_review( .map_err(|err| format!("failed to collect review scope paths: {err}"))? }; if !name_only_output.status.success() { - return Ok::<(Option, Option), String>((None, None)); + return Ok::(AutoReviewScopeMetadata::empty()); } let stdout = String::from_utf8_lossy(&name_only_output.stdout); let paths = stdout.lines().map(str::to_string).collect::>(); let scope_note = format_background_review_scope_from_paths(&snapshot_id, &paths); + let listed_paths = auto_review_listed_paths(&paths); + let omitted_path_count = paths.len().saturating_sub(listed_paths.len()); if paths.len() > AUTO_REVIEW_MAX_CHANGED_PATHS { return Err(format!( @@ -31806,7 +31928,14 @@ async fn run_background_review( .map_err(|err| format!("failed to collect review diff excerpt: {err}"))? }; if !diff_output.status.success() { - return Ok((scope_note, None)); + return Ok(AutoReviewScopeMetadata { + scope_note, + diff_excerpt: None, + diff_fingerprint: None, + changed_path_count: paths.len(), + listed_paths, + omitted_path_count, + }); } let diff_text = String::from_utf8_lossy(&diff_output.stdout); @@ -31826,12 +31955,94 @@ async fn run_background_review( AUTO_REVIEW_DIFF_EXCERPT_CHARS, )) }; - Ok((scope_note, diff_excerpt)) + Ok(AutoReviewScopeMetadata { + scope_note, + diff_excerpt, + diff_fingerprint: auto_review_diff_fingerprint( + (!parent_id.is_empty()).then_some(parent_id.as_str()), + &paths, + &diff_text, + ), + changed_path_count: paths.len(), + listed_paths, + omitted_path_count, + }) } }) .await .map_err(|e| format!("failed to spawn review scope task: {e}"))??; + if let Err(err) = persist_auto_review_scope_metadata(&config.cwd, run_id, &review_scope) { + tracing::warn!(?err, run_id = %run_id, "failed to persist auto-review scope metadata"); + } + + if let Some(diff_fingerprint) = review_scope.diff_fingerprint.as_deref() + && let Some(duplicate) = find_duplicate_auto_review_run(&config.cwd, diff_fingerprint, run_id) + { + if let Err(err) = mark_auto_review_run_skipped_duplicate( + &config.cwd, + run_id, + duplicate.run_id, + Some(diff_fingerprint), + ) { + tracing::warn!(?err, run_id = %run_id, "failed to mark duplicate auto-review run skipped"); + } + + match duplicate.disposition { + AutoReviewDuplicateDisposition::Adopt => { + app_event_tx_clone.send(AppEvent::BackgroundReviewFinished { + run_id, + worktree_path: duplicate.worktree_path.unwrap_or_default(), + branch: duplicate.branch.unwrap_or_default(), + has_findings: false, + findings: 0, + summary: Some("Auto review skipped: equivalent review is already running.".to_string()), + error: None, + agent_id: None, + snapshot: Some(snapshot_id.clone()), + owner_session_id: Some(owner_session_id), + }); + return Ok::<(PathBuf, String, String, String), String>(( + PathBuf::new(), + String::new(), + String::new(), + snapshot_id, + )); + } + AutoReviewDuplicateDisposition::ReuseTerminal => { + app_event_tx_clone.send(AppEvent::BackgroundReviewFinished { + run_id, + worktree_path: duplicate.worktree_path.unwrap_or_default(), + branch: duplicate.branch.unwrap_or_default(), + has_findings: duplicate.finding_count > 0, + findings: duplicate.finding_count, + summary: duplicate.summary_digest.or_else(|| { + Some("Auto review skipped: equivalent review already completed.".to_string()) + }), + error: None, + agent_id: duplicate.agent_id, + snapshot: duplicate.snapshot_commit, + owner_session_id: Some(owner_session_id), + }); + return Ok::<(PathBuf, String, String, String), String>(( + PathBuf::new(), + String::new(), + String::new(), + snapshot_id, + )); + } + AutoReviewDuplicateDisposition::SupersedeTerminal => { + if let Err(err) = supersede_clean_duplicate_auto_reviews( + &config.cwd, + diff_fingerprint, + run_id, + ) { + tracing::warn!(?err, run_id = %run_id, "failed to supersede clean duplicate auto-review runs"); + } + } + } + } + // Attempt to hold the shared review lock; if busy or a previous review // with findings is still surfaced, fall back to a per-request // auto-review worktree to avoid clobbering pending fixes. @@ -31896,8 +32107,8 @@ async fn run_background_review( // Use the /review entrypoint so upstream wiring (model defaults, review formatting) stays intact. let review_prompt = build_background_review_prompt( &snapshot_id, - review_scope.0.as_deref(), - review_scope.1.as_deref(), + review_scope.scope_note.as_deref(), + review_scope.diff_excerpt.as_deref(), turn_context.as_deref(), prompt_token_budget, ); @@ -32751,6 +32962,42 @@ use code_core::protocol::OrderMeta; assert!(scope.contains("excluded by path policy")); } + #[test] + fn auto_review_diff_fingerprint_is_stable_for_equivalent_paths() { + let first = auto_review_diff_fingerprint( + Some("base"), + &["src/lib.rs".to_string(), "./src/main.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + let second = auto_review_diff_fingerprint( + Some("base"), + &["src/main.rs".to_string(), "src/lib.rs".to_string()], + "\n diff --git a/src/lib.rs b/src/lib.rs\n+line\n\n", + ) + .expect("fingerprint"); + + assert_eq!(first, second); + } + + #[test] + fn auto_review_diff_fingerprint_changes_with_base() { + let first = auto_review_diff_fingerprint( + Some("base-a"), + &["src/lib.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + let second = auto_review_diff_fingerprint( + Some("base-b"), + &["src/lib.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + + assert_ne!(first, second); + } + #[test] fn background_review_prompt_prefers_diff_excerpt_over_commit_scope() { let prompt = build_background_review_prompt( @@ -41173,7 +41420,11 @@ impl ChatWidget<'_> { .summary .map(|summary| Self::summarize_plain_review_text(summary, 240)); run.freshness = AutoReviewFreshness::Current; - if let Some(error) = completion.error { + if run.status == AutoReviewRunStatus::Skipped + && run.cancel_reason.as_deref() == Some("duplicate_auto_review_scope") + { + run.mark_status(AutoReviewRunStatus::Skipped, now); + } else if let Some(error) = completion.error { let classification = Self::classify_auto_review_error(error); run.error_class = Some(classification.failure_class.to_string()); run.error_summary = Some(Self::summarize_auto_review_error(error)); From 557be8a9a038fd219d42a4c60051d71f27f42817 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 2 Jun 2026 11:32:31 -0400 Subject: [PATCH 2/3] feat: dedupe auto review runs by diff fingerprint --- code-rs/core/src/review_store.rs | 220 +++++++++++++++++++++++++ code-rs/tui/src/chatwidget.rs | 265 ++++++++++++++++++++++++++++++- 2 files changed, 479 insertions(+), 6 deletions(-) diff --git a/code-rs/core/src/review_store.rs b/code-rs/core/src/review_store.rs index 3515cc66fcc..6f59a8afd92 100644 --- a/code-rs/core/src/review_store.rs +++ b/code-rs/core/src/review_store.rs @@ -220,6 +220,27 @@ impl AutoReviewRun { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AutoReviewDuplicateDisposition { + Adopt, + ReuseTerminal, + SupersedeTerminal, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AutoReviewDuplicateMatch { + pub run_id: Uuid, + pub status: AutoReviewRunStatus, + pub disposition: AutoReviewDuplicateDisposition, + pub agent_id: Option, + pub snapshot_commit: Option, + pub owner_session_id: Option, + pub worktree_path: Option, + pub branch: Option, + pub finding_count: usize, + pub summary_digest: Option, +} + #[derive(Debug, Clone, Copy)] pub struct AutoReviewLedgerOptions { pub max_bytes: usize, @@ -369,6 +390,89 @@ impl AutoReviewRunStore { Ok(changed) } + pub fn find_duplicate_by_fingerprint( + &self, + diff_fingerprint: &str, + ) -> Option { + self.find_duplicate_by_fingerprint_excluding(diff_fingerprint, None) + } + + pub fn find_duplicate_by_fingerprint_excluding( + &self, + diff_fingerprint: &str, + excluded_run_id: Option, + ) -> Option { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return None; + } + + self.runs + .values() + .filter(|run| Some(run.run_id) != excluded_run_id) + .filter(|run| run.diff_fingerprint.as_deref() == Some(fingerprint)) + .filter(|run| { + !matches!( + run.status, + AutoReviewRunStatus::Lost + | AutoReviewRunStatus::Skipped + | AutoReviewRunStatus::Superseded + ) + }) + .max_by(|left, right| { + duplicate_priority(left) + .cmp(&duplicate_priority(right)) + .then_with(|| left.updated_at.cmp(&right.updated_at)) + .then_with(|| left.created_at.cmp(&right.created_at)) + .then_with(|| left.run_id.cmp(&right.run_id)) + }) + .map(|run| AutoReviewDuplicateMatch { + run_id: run.run_id, + status: run.status, + disposition: duplicate_disposition(run), + agent_id: run.agent_id.clone(), + snapshot_commit: run.snapshot_commit.clone(), + owner_session_id: run.owner_session_id, + worktree_path: run.worktree_path.clone(), + branch: run.branch.clone().or_else(|| run.batch_id.clone()), + finding_count: run.finding_count, + summary_digest: run.summary_digest.clone(), + }) + } + + pub fn mark_superseded_by_fingerprint( + &mut self, + diff_fingerprint: &str, + superseded_by: Uuid, + now: u64, + ) -> io::Result { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return Ok(0); + } + + let mut changed = 0usize; + for run in self.runs.values_mut() { + if run.run_id == superseded_by + || run.diff_fingerprint.as_deref() != Some(fingerprint) + || run.status.is_in_flight() + || run.status == AutoReviewRunStatus::Superseded + || run.finding_count > 0 + || !run.finding_digests.is_empty() + { + continue; + } + run.freshness = AutoReviewFreshness::Superseded; + run.superseded_by = Some(superseded_by); + run.mark_status(AutoReviewRunStatus::Superseded, now); + changed = changed.saturating_add(1); + } + if changed > 0 { + self.save()?; + } + Ok(changed) + } + pub fn write_output(&mut self, run_id: Uuid, output: &ReviewOutputEvent) -> io::Result { if !self.runs.contains_key(&run_id) { return Err(io::Error::new( @@ -409,6 +513,29 @@ impl AutoReviewRunStore { } } +fn duplicate_priority(run: &AutoReviewRun) -> u8 { + if run.status.is_in_flight() { + return 4; + } + if run.finding_count > 0 || !run.finding_digests.is_empty() { + return 3; + } + if run.status == AutoReviewRunStatus::Completed { + return 2; + } + 1 +} + +fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition { + if run.status.is_in_flight() { + AutoReviewDuplicateDisposition::Adopt + } else if run.status == AutoReviewRunStatus::Completed { + AutoReviewDuplicateDisposition::ReuseTerminal + } else { + AutoReviewDuplicateDisposition::SupersedeTerminal + } +} + pub fn auto_review_dir(scope: &Path) -> io::Result { Ok(scoped_review_state_dir(scope)?.join(AUTO_REVIEW_DIR)) } @@ -845,6 +972,99 @@ mod tests { assert!(loaded.get(second_id).is_some()); } + #[test] + #[serial] + fn duplicate_lookup_prefers_in_flight_fingerprint_match() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let completed_id = Uuid::new_v4(); + let mut completed = AutoReviewRun::new(completed_id, AutoReviewRunSource::Tui, 1); + completed.diff_fingerprint = Some("diff:abc".to_string()); + completed.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(completed).unwrap(); + + let live_id = Uuid::new_v4(); + let mut live = AutoReviewRun::new(live_id, AutoReviewRunSource::Tui, 3); + live.diff_fingerprint = Some("diff:abc".to_string()); + live.agent_id = Some("agent-live".to_string()); + live.snapshot_commit = Some("snap-live".to_string()); + live.mark_status(AutoReviewRunStatus::Reviewing, 4); + store.upsert(live).unwrap(); + + let duplicate = store + .find_duplicate_by_fingerprint("diff:abc") + .expect("duplicate"); + assert_eq!(duplicate.run_id, live_id); + assert_eq!(duplicate.disposition, AutoReviewDuplicateDisposition::Adopt); + assert_eq!(duplicate.agent_id.as_deref(), Some("agent-live")); + } + + #[test] + #[serial] + fn duplicate_lookup_reuses_terminal_finding_match() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let run_id = Uuid::new_v4(); + let mut run = AutoReviewRun::new(run_id, AutoReviewRunSource::Tui, 1); + run.diff_fingerprint = Some("diff:abc".to_string()); + run.finding_count = 1; + run.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(run).unwrap(); + + let duplicate = store + .find_duplicate_by_fingerprint("diff:abc") + .expect("duplicate"); + assert_eq!(duplicate.run_id, run_id); + assert_eq!( + duplicate.disposition, + AutoReviewDuplicateDisposition::ReuseTerminal + ); + } + + #[test] + #[serial] + fn supersede_by_fingerprint_omits_runs_with_findings() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let new_id = Uuid::new_v4(); + let clean_id = Uuid::new_v4(); + let finding_id = Uuid::new_v4(); + + let mut clean = AutoReviewRun::new(clean_id, AutoReviewRunSource::Tui, 1); + clean.diff_fingerprint = Some("diff:abc".to_string()); + clean.mark_status(AutoReviewRunStatus::Completed, 2); + store.upsert(clean).unwrap(); + + let mut finding = AutoReviewRun::new(finding_id, AutoReviewRunSource::Tui, 3); + finding.diff_fingerprint = Some("diff:abc".to_string()); + finding.finding_count = 1; + finding.mark_status(AutoReviewRunStatus::Completed, 4); + store.upsert(finding).unwrap(); + + let changed = store + .mark_superseded_by_fingerprint("diff:abc", new_id, 10) + .unwrap(); + assert_eq!(changed, 1); + + let loaded = AutoReviewRunStore::open(repo.path()).unwrap(); + let clean = loaded.get(clean_id).unwrap(); + assert_eq!(clean.status, AutoReviewRunStatus::Superseded); + assert_eq!(clean.superseded_by, Some(new_id)); + + let finding = loaded.get(finding_id).unwrap(); + assert_eq!(finding.status, AutoReviewRunStatus::Completed); + assert_eq!(finding.superseded_by, None); + } + #[test] #[serial] fn run_store_reconciles_orphaned_in_flight_agents() { diff --git a/code-rs/tui/src/chatwidget.rs b/code-rs/tui/src/chatwidget.rs index 7613f9a2448..b9e06dc3948 100644 --- a/code-rs/tui/src/chatwidget.rs +++ b/code-rs/tui/src/chatwidget.rs @@ -202,6 +202,7 @@ use code_core::protocol::TokenUsage; use code_core::protocol::TurnDiffEvent; use code_core::protocol::ViewImageToolCallEvent; use code_core::review_store::{ + AutoReviewDuplicateDisposition, AutoReviewFreshness, AutoReviewRun, AutoReviewRunSource, @@ -1652,6 +1653,127 @@ fn format_background_review_scope_from_paths(snapshot_id: &str, paths: &[String] Some(scope.trim_end().to_string()) } +#[derive(Clone, Debug)] +struct AutoReviewScopeMetadata { + scope_note: Option, + diff_excerpt: Option, + diff_fingerprint: Option, + changed_path_count: usize, + listed_paths: Vec, + omitted_path_count: usize, +} + +impl AutoReviewScopeMetadata { + fn empty() -> Self { + Self { + scope_note: None, + diff_excerpt: None, + diff_fingerprint: None, + changed_path_count: 0, + listed_paths: Vec::new(), + omitted_path_count: 0, + } + } +} + +fn auto_review_diff_fingerprint(parent_id: Option<&str>, paths: &[String], diff_text: &str) -> Option { + let normalized_diff = diff_text.trim(); + if normalized_diff.is_empty() { + return None; + } + + let mut normalized_paths = paths + .iter() + .map(|path| path.trim().trim_start_matches("./")) + .filter(|path| !path.is_empty()) + .collect::>(); + normalized_paths.sort_unstable(); + normalized_paths.dedup(); + + let mut hasher = Sha256::new(); + hasher.update(b"auto-review-diff-v2\0"); + hasher.update(parent_id.unwrap_or_default().as_bytes()); + hasher.update(b"\0"); + for path in normalized_paths { + hasher.update(path.as_bytes()); + hasher.update(b"\0"); + } + hasher.update(normalized_diff.as_bytes()); + let digest = hasher.finalize(); + Some(format!("diff:{digest:x}")) +} + +fn auto_review_listed_paths(paths: &[String]) -> Vec { + let mut seen = HashSet::new(); + let mut listed = Vec::new(); + for raw_path in paths { + let normalized = raw_path.trim().trim_start_matches("./"); + if normalized.is_empty() || !seen.insert(normalized.to_string()) { + continue; + } + if should_include_auto_review_scope_path(normalized) { + listed.push(PathBuf::from(normalized)); + } + } + listed +} + +fn persist_auto_review_scope_metadata( + cwd: &Path, + run_id: Uuid, + metadata: &AutoReviewScopeMetadata, +) -> std::io::Result<()> { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + if let Some(run) = store.get_mut(run_id) { + run.diff_fingerprint = metadata.diff_fingerprint.clone(); + run.changed_path_count = metadata.changed_path_count; + run.listed_paths = metadata.listed_paths.clone(); + run.omitted_path_count = metadata.omitted_path_count; + run.mark_activity(now); + } + store.save() +} + +fn find_duplicate_auto_review_run( + cwd: &Path, + diff_fingerprint: &str, + current_run_id: Uuid, +) -> Option { + let store = AutoReviewRunStore::open_existing(cwd).ok().flatten()?; + store.find_duplicate_by_fingerprint_excluding(diff_fingerprint, Some(current_run_id)) +} + +fn mark_auto_review_run_skipped_duplicate( + cwd: &Path, + run_id: Uuid, + duplicate_run_id: Uuid, + diff_fingerprint: Option<&str>, +) -> std::io::Result<()> { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + if let Some(run) = store.get_mut(run_id) { + if let Some(diff_fingerprint) = diff_fingerprint { + run.diff_fingerprint = Some(diff_fingerprint.to_string()); + } + run.freshness = AutoReviewFreshness::Superseded; + run.superseded_by = Some(duplicate_run_id); + run.cancel_reason = Some("duplicate_auto_review_scope".to_string()); + run.mark_status(AutoReviewRunStatus::Skipped, now); + } + store.save() +} + +fn supersede_clean_duplicate_auto_reviews( + cwd: &Path, + diff_fingerprint: &str, + superseded_by: Uuid, +) -> std::io::Result { + let now = unix_now_secs(); + let mut store = AutoReviewRunStore::open(cwd)?; + store.mark_superseded_by_fingerprint(diff_fingerprint, superseded_by, now) +} + fn build_background_review_prompt( snapshot_id: &str, scope_note: Option<&str>, @@ -31778,11 +31900,13 @@ async fn run_background_review( .map_err(|err| format!("failed to collect review scope paths: {err}"))? }; if !name_only_output.status.success() { - return Ok::<(Option, Option), String>((None, None)); + return Ok::(AutoReviewScopeMetadata::empty()); } let stdout = String::from_utf8_lossy(&name_only_output.stdout); let paths = stdout.lines().map(str::to_string).collect::>(); let scope_note = format_background_review_scope_from_paths(&snapshot_id, &paths); + let listed_paths = auto_review_listed_paths(&paths); + let omitted_path_count = paths.len().saturating_sub(listed_paths.len()); if paths.len() > AUTO_REVIEW_MAX_CHANGED_PATHS { return Err(format!( @@ -31806,7 +31930,14 @@ async fn run_background_review( .map_err(|err| format!("failed to collect review diff excerpt: {err}"))? }; if !diff_output.status.success() { - return Ok((scope_note, None)); + return Ok(AutoReviewScopeMetadata { + scope_note, + diff_excerpt: None, + diff_fingerprint: None, + changed_path_count: paths.len(), + listed_paths, + omitted_path_count, + }); } let diff_text = String::from_utf8_lossy(&diff_output.stdout); @@ -31826,12 +31957,94 @@ async fn run_background_review( AUTO_REVIEW_DIFF_EXCERPT_CHARS, )) }; - Ok((scope_note, diff_excerpt)) + Ok(AutoReviewScopeMetadata { + scope_note, + diff_excerpt, + diff_fingerprint: auto_review_diff_fingerprint( + (!parent_id.is_empty()).then_some(parent_id.as_str()), + &paths, + &diff_text, + ), + changed_path_count: paths.len(), + listed_paths, + omitted_path_count, + }) } }) .await .map_err(|e| format!("failed to spawn review scope task: {e}"))??; + if let Err(err) = persist_auto_review_scope_metadata(&config.cwd, run_id, &review_scope) { + tracing::warn!(?err, run_id = %run_id, "failed to persist auto-review scope metadata"); + } + + if let Some(diff_fingerprint) = review_scope.diff_fingerprint.as_deref() + && let Some(duplicate) = find_duplicate_auto_review_run(&config.cwd, diff_fingerprint, run_id) + { + if let Err(err) = mark_auto_review_run_skipped_duplicate( + &config.cwd, + run_id, + duplicate.run_id, + Some(diff_fingerprint), + ) { + tracing::warn!(?err, run_id = %run_id, "failed to mark duplicate auto-review run skipped"); + } + + match duplicate.disposition { + AutoReviewDuplicateDisposition::Adopt => { + app_event_tx_clone.send(AppEvent::BackgroundReviewFinished { + run_id, + worktree_path: duplicate.worktree_path.unwrap_or_default(), + branch: duplicate.branch.unwrap_or_default(), + has_findings: false, + findings: 0, + summary: Some("Auto review skipped: equivalent review is already running.".to_string()), + error: None, + agent_id: None, + snapshot: Some(snapshot_id.clone()), + owner_session_id: Some(owner_session_id), + }); + return Ok::<(PathBuf, String, String, String), String>(( + PathBuf::new(), + String::new(), + String::new(), + snapshot_id, + )); + } + AutoReviewDuplicateDisposition::ReuseTerminal => { + app_event_tx_clone.send(AppEvent::BackgroundReviewFinished { + run_id, + worktree_path: duplicate.worktree_path.unwrap_or_default(), + branch: duplicate.branch.unwrap_or_default(), + has_findings: duplicate.finding_count > 0, + findings: duplicate.finding_count, + summary: duplicate.summary_digest.or_else(|| { + Some("Auto review skipped: equivalent review already completed.".to_string()) + }), + error: None, + agent_id: duplicate.agent_id, + snapshot: duplicate.snapshot_commit, + owner_session_id: Some(owner_session_id), + }); + return Ok::<(PathBuf, String, String, String), String>(( + PathBuf::new(), + String::new(), + String::new(), + snapshot_id, + )); + } + AutoReviewDuplicateDisposition::SupersedeTerminal => { + if let Err(err) = supersede_clean_duplicate_auto_reviews( + &config.cwd, + diff_fingerprint, + run_id, + ) { + tracing::warn!(?err, run_id = %run_id, "failed to supersede clean duplicate auto-review runs"); + } + } + } + } + // Attempt to hold the shared review lock; if busy or a previous review // with findings is still surfaced, fall back to a per-request // auto-review worktree to avoid clobbering pending fixes. @@ -31896,8 +32109,8 @@ async fn run_background_review( // Use the /review entrypoint so upstream wiring (model defaults, review formatting) stays intact. let review_prompt = build_background_review_prompt( &snapshot_id, - review_scope.0.as_deref(), - review_scope.1.as_deref(), + review_scope.scope_note.as_deref(), + review_scope.diff_excerpt.as_deref(), turn_context.as_deref(), prompt_token_budget, ); @@ -32751,6 +32964,42 @@ use code_core::protocol::OrderMeta; assert!(scope.contains("excluded by path policy")); } + #[test] + fn auto_review_diff_fingerprint_is_stable_for_equivalent_paths() { + let first = auto_review_diff_fingerprint( + Some("base"), + &["src/lib.rs".to_string(), "./src/main.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + let second = auto_review_diff_fingerprint( + Some("base"), + &["src/main.rs".to_string(), "src/lib.rs".to_string()], + "\n diff --git a/src/lib.rs b/src/lib.rs\n+line\n\n", + ) + .expect("fingerprint"); + + assert_eq!(first, second); + } + + #[test] + fn auto_review_diff_fingerprint_changes_with_base() { + let first = auto_review_diff_fingerprint( + Some("base-a"), + &["src/lib.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + let second = auto_review_diff_fingerprint( + Some("base-b"), + &["src/lib.rs".to_string()], + "diff --git a/src/lib.rs b/src/lib.rs\n+line\n", + ) + .expect("fingerprint"); + + assert_ne!(first, second); + } + #[test] fn background_review_prompt_prefers_diff_excerpt_over_commit_scope() { let prompt = build_background_review_prompt( @@ -41173,7 +41422,11 @@ impl ChatWidget<'_> { .summary .map(|summary| Self::summarize_plain_review_text(summary, 240)); run.freshness = AutoReviewFreshness::Current; - if let Some(error) = completion.error { + if run.status == AutoReviewRunStatus::Skipped + && run.cancel_reason.as_deref() == Some("duplicate_auto_review_scope") + { + run.mark_status(AutoReviewRunStatus::Skipped, now); + } else if let Some(error) = completion.error { let classification = Self::classify_auto_review_error(error); run.error_class = Some(classification.failure_class.to_string()); run.error_summary = Some(Self::summarize_auto_review_error(error)); From dae7ad3bac507e88f6e2be51f059ea18580bd2bb Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 2 Jun 2026 11:42:21 -0400 Subject: [PATCH 3/3] fix: keep duplicate auto review skips pending --- code-rs/core/src/review_store.rs | 34 ++++++++++++++++++++++-- code-rs/tui/src/chatwidget.rs | 44 ++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/code-rs/core/src/review_store.rs b/code-rs/core/src/review_store.rs index 6f59a8afd92..d7c17fe2f2e 100644 --- a/code-rs/core/src/review_store.rs +++ b/code-rs/core/src/review_store.rs @@ -62,6 +62,13 @@ impl AutoReviewRunStatus { pub fn is_in_flight(self) -> bool { !self.is_terminal() } + + pub fn is_adoptable_duplicate(self) -> bool { + matches!( + self, + AutoReviewRunStatus::Reviewing | AutoReviewRunStatus::Resolving + ) + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] @@ -514,7 +521,7 @@ impl AutoReviewRunStore { } fn duplicate_priority(run: &AutoReviewRun) -> u8 { - if run.status.is_in_flight() { + if run.status.is_adoptable_duplicate() { return 4; } if run.finding_count > 0 || !run.finding_digests.is_empty() { @@ -527,7 +534,7 @@ fn duplicate_priority(run: &AutoReviewRun) -> u8 { } fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition { - if run.status.is_in_flight() { + if run.status.is_adoptable_duplicate() { AutoReviewDuplicateDisposition::Adopt } else if run.status == AutoReviewRunStatus::Completed { AutoReviewDuplicateDisposition::ReuseTerminal @@ -1002,6 +1009,29 @@ mod tests { assert_eq!(duplicate.agent_id.as_deref(), Some("agent-live")); } + #[test] + #[serial] + fn duplicate_lookup_does_not_adopt_pending_fingerprint_match() { + let code_home = TempDir::new().unwrap(); + let repo = TempDir::new().unwrap(); + set_code_home(code_home.path()); + let mut store = AutoReviewRunStore::open(repo.path()).unwrap(); + + let pending_id = Uuid::new_v4(); + let mut pending = AutoReviewRun::new(pending_id, AutoReviewRunSource::Tui, 1); + pending.diff_fingerprint = Some("diff:abc".to_string()); + store.upsert(pending).unwrap(); + + let duplicate = store + .find_duplicate_by_fingerprint("diff:abc") + .expect("duplicate"); + assert_eq!(duplicate.run_id, pending_id); + assert_eq!( + duplicate.disposition, + AutoReviewDuplicateDisposition::SupersedeTerminal + ); + } + #[test] #[serial] fn duplicate_lookup_reuses_terminal_finding_match() { diff --git a/code-rs/tui/src/chatwidget.rs b/code-rs/tui/src/chatwidget.rs index 6a4d8991533..279f471bc96 100644 --- a/code-rs/tui/src/chatwidget.rs +++ b/code-rs/tui/src/chatwidget.rs @@ -5,7 +5,6 @@ use std::collections::hash_map::Entry; use std::collections::HashSet; use std::collections::VecDeque; use std::io; -use std::hash::{Hash, Hasher}; use std::path::{Path, PathBuf}; use std::rc::{Rc, Weak}; use std::sync::Arc; @@ -32002,7 +32001,7 @@ async fn run_background_review( summary: Some("Auto review skipped: equivalent review is already running.".to_string()), error: None, agent_id: None, - snapshot: Some(snapshot_id.clone()), + snapshot: None, owner_session_id: Some(owner_session_id), }); return Ok::<(PathBuf, String, String, String), String>(( @@ -33831,6 +33830,47 @@ use code_core::protocol::OrderMeta; ); } + #[test] + fn duplicate_skipped_background_review_does_not_mark_snapshot_reviewed() { + let mut harness = ChatWidgetHarness::new(); + let chat = harness.chat(); + let session_id = uuid::Uuid::new_v4(); + let run_id = uuid::Uuid::new_v4(); + chat.session_id = Some(session_id); + chat.background_review_run_id = Some(run_id); + chat.background_review = Some(BackgroundReviewState { + worktree_path: PathBuf::new(), + branch: String::new(), + agent_id: None, + snapshot: None, + owner_session_id: Some(session_id), + base: Some(GhostCommit::new("base-before-review".to_string(), None)), + last_seen: Instant::now(), + }); + + chat.on_background_review_finished_for_run( + run_id, + PathBuf::new(), + String::new(), + false, + 0, + Some("Auto review skipped: equivalent review is already running.".to_string()), + None, + None, + None, + Some(session_id), + ); + + assert!(chat.background_review.is_none()); + assert!(chat.auto_review_reviewed_marker.is_none()); + assert_eq!( + chat.pending_auto_review_range + .as_ref() + .map(|pending| pending.base.id()), + Some("base-before-review") + ); + } + #[test] fn background_review_findings_are_forwarded_to_coordinator_resume() { let _rt = enter_test_runtime_guard();