diff --git a/codex-rs/auto-review/src/lib.rs b/codex-rs/auto-review/src/lib.rs index c69c77b3c262..19c0729bd3da 100644 --- a/codex-rs/auto-review/src/lib.rs +++ b/codex-rs/auto-review/src/lib.rs @@ -115,6 +115,21 @@ impl AutoReviewStore { &self, diff_fingerprint: &str, superseded_by: &str, + ) -> Result { + self.mark_superseded_by_fingerprint_with_target( + diff_fingerprint, + superseded_by, + /*active_branch*/ None, + /*active_head*/ None, + ) + } + + pub fn mark_superseded_by_fingerprint_with_target( + &self, + diff_fingerprint: &str, + superseded_by: &str, + active_branch: Option<&str>, + active_head: Option<&str>, ) -> Result { let fingerprint = diff_fingerprint.trim(); if fingerprint.is_empty() { @@ -125,6 +140,7 @@ impl AutoReviewStore { for run in self.load_index()?.runs { if run.run_id == superseded_by || run.target.worktree_diff_fingerprint.as_deref() != Some(fingerprint) + || !duplicate_target_is_reusable(&run, active_branch, active_head) { continue; } @@ -185,6 +201,24 @@ impl AutoReviewStore { active_branch: Option<&str>, active_head: Option<&str>, ) -> Result> { + self.find_duplicate_by_fingerprint_with_target_and_filter( + diff_fingerprint, + active_branch, + active_head, + |_| true, + ) + } + + pub fn find_duplicate_by_fingerprint_with_target_and_filter( + &self, + diff_fingerprint: &str, + active_branch: Option<&str>, + active_head: Option<&str>, + is_eligible: F, + ) -> Result> + where + F: Fn(&AutoReviewDuplicateMatch) -> bool, + { let fingerprint = diff_fingerprint.trim(); if fingerprint.is_empty() { return Ok(None); @@ -203,20 +237,24 @@ impl AutoReviewStore { ) }) .filter(|run| duplicate_target_is_reusable(run, active_branch, active_head)) + .filter_map(|run| { + let duplicate = AutoReviewDuplicateMatch { + run_id: run.run_id.clone(), + status: run.status.clone(), + disposition: duplicate_disposition(&run), + finding_count: run.findings.len(), + model: run.model.clone(), + }; + is_eligible(&duplicate).then_some((run, duplicate)) + }) .max_by(|left, right| { - duplicate_priority(left) - .cmp(&duplicate_priority(right)) + duplicate_priority(&left.0) + .cmp(&duplicate_priority(&right.0)) .then_with(|| { - auto_review_run_sort_key(left).cmp(&auto_review_run_sort_key(right)) + auto_review_run_sort_key(&left.0).cmp(&auto_review_run_sort_key(&right.0)) }) }) - .map(|run| AutoReviewDuplicateMatch { - run_id: run.run_id.clone(), - status: run.status.clone(), - disposition: duplicate_disposition(&run), - finding_count: run.findings.len(), - model: run.model, - })) + .map(|(_run, duplicate)| duplicate)) } pub fn load_run(&self, run_id: &str) -> Result { diff --git a/codex-rs/auto-review/src/lib_tests.rs b/codex-rs/auto-review/src/lib_tests.rs index ee6ceee7a943..25760dcb7443 100644 --- a/codex-rs/auto-review/src/lib_tests.rs +++ b/codex-rs/auto-review/src/lib_tests.rs @@ -621,6 +621,40 @@ fn duplicate_lookup_prefers_adoptable_in_flight_match() -> anyhow::Result<()> { Ok(()) } +#[test] +fn duplicate_lookup_filter_can_fall_back_from_ineligible_adopt_to_completed() -> anyhow::Result<()> +{ + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&AutoReviewRun { + target: target_with_fingerprint("sha256:abc"), + ..sample_run("completed", vec![sample_finding("f1", "Keep")]) + })?; + store.save_run(&AutoReviewRun { + status: AutoReviewRunStatus::Reviewing, + completed_at_unix_secs: None, + target: target_with_fingerprint("sha256:abc"), + ..sample_run("unowned_reviewing", Vec::new()) + })?; + + let duplicate = store + .find_duplicate_by_fingerprint_with_target_and_filter( + "sha256:abc", + None, + None, + |duplicate| duplicate.disposition != AutoReviewDuplicateDisposition::Adopt, + )? + .expect("eligible completed duplicate"); + + assert_eq!(duplicate.run_id, "completed"); + assert_eq!( + duplicate.disposition, + AutoReviewDuplicateDisposition::ReuseTerminal + ); + Ok(()) +} + #[test] fn duplicate_lookup_can_filter_stale_target_matches() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; @@ -743,6 +777,49 @@ fn mark_superseded_by_fingerprint_only_supersedes_matching_scope() -> anyhow::Re Ok(()) } +#[test] +fn mark_superseded_by_fingerprint_with_target_preserves_stale_head() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&AutoReviewRun { + target: AutoReviewRunTarget { + branch: Some("main".to_string()), + head_sha: Some("old-head".to_string()), + worktree_diff_fingerprint: Some("sha256:abc".to_string()), + ..sample_target("main", "old-head", "/repo") + }, + ..sample_run("old_head", Vec::new()) + })?; + store.save_run(&AutoReviewRun { + target: AutoReviewRunTarget { + branch: Some("main".to_string()), + head_sha: Some("new-head".to_string()), + worktree_diff_fingerprint: Some("sha256:abc".to_string()), + ..sample_target("main", "new-head", "/repo") + }, + ..sample_run("new_head", Vec::new()) + })?; + + let changed = store.mark_superseded_by_fingerprint_with_target( + "sha256:abc", + "new_run", + Some("main"), + Some("new-head"), + )?; + + assert_eq!(changed, 1); + assert_eq!( + store.load_run("old_head")?.status, + AutoReviewRunStatus::Completed + ); + assert_eq!( + store.load_run("new_head")?.status, + AutoReviewRunStatus::Superseded + ); + Ok(()) +} + #[test] fn reconcile_orphaned_in_flight_marks_lost() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; diff --git a/codex-rs/core/src/review_persistence.rs b/codex-rs/core/src/review_persistence.rs index 76cb62f27322..d35a5919d099 100644 --- a/codex-rs/core/src/review_persistence.rs +++ b/codex-rs/core/src/review_persistence.rs @@ -2,6 +2,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Mutex; +use codex_auto_review::AutoReviewDuplicateMatch; use codex_auto_review::AutoReviewFindingRecord; use codex_auto_review::AutoReviewRun; use codex_auto_review::AutoReviewRunFreshness; @@ -75,6 +76,18 @@ impl ReviewPersistenceContext { &self.run_id } + pub(crate) fn store_scope(&self) -> &Path { + &self.store_scope + } + + pub(crate) fn target(&self) -> &AutoReviewRunTarget { + &self.target + } + + pub(crate) fn worktree_diff_fingerprint(&self) -> Option<&str> { + self.target.worktree_diff_fingerprint.as_deref() + } + pub(crate) fn review_target(&self) -> &ReviewTarget { &self.review_target } @@ -148,12 +161,53 @@ impl ReviewPersistenceContext { ) } + pub(crate) fn save_skipped_duplicate( + &self, + codex_home: impl AsRef, + duplicate: &AutoReviewDuplicateMatch, + ) -> bool { + let error_summary = format!( + "equivalent background auto review already exists: {}", + duplicate.run_id + ); + self.save_run_with_metadata( + codex_home, + AutoReviewRunStatus::Skipped, + /*output*/ None, + Some(error_summary), + AutoReviewRunFreshness::Superseded, + Some(duplicate.run_id.clone()), + Some("duplicate_auto_review_scope".to_string()), + ) + } + fn save_run( &self, codex_home: impl AsRef, status: AutoReviewRunStatus, output: Option<&ReviewOutputEvent>, error_summary: Option, + ) -> bool { + self.save_run_with_metadata( + codex_home, + status, + output, + error_summary, + AutoReviewRunFreshness::Current, + None, + None, + ) + } + + fn save_run_with_metadata( + &self, + codex_home: impl AsRef, + status: AutoReviewRunStatus, + output: Option<&ReviewOutputEvent>, + error_summary: Option, + freshness: AutoReviewRunFreshness, + superseded_by: Option, + cancel_reason: Option, ) -> bool { let codex_home = codex_home.as_ref(); let completed_at_unix_secs = if is_terminal_status(&status) { @@ -182,15 +236,15 @@ impl ReviewPersistenceContext { schema_version: SCHEMA_VERSION, run_id: self.run_id.clone(), status, - freshness: AutoReviewRunFreshness::Current, + freshness, source: self.source.clone(), target: self.target.clone(), review_target: self.review_target.clone(), started_at_unix_secs: self.started_at_unix_secs, completed_at_unix_secs, model: self.model.clone(), - superseded_by: None, - cancel_reason: None, + superseded_by, + cancel_reason, error_summary, findings: output .map(|output| finding_records(&output.findings)) @@ -373,4 +427,43 @@ mod tests { assert_eq!(run.status, AutoReviewRunStatus::Skipped); assert_eq!(run.error_summary.as_deref(), Some("duplicate fingerprint")); } + + #[tokio::test] + async fn save_skipped_duplicate_records_duplicate_metadata() { + let codex_home = TempDir::new().expect("create temp codex home"); + let cwd = TempDir::new().expect("create temp cwd"); + let persistence = ReviewPersistenceContext::new( + "duplicate-candidate".to_string(), + ReviewPersistence::BackgroundAutoReview, + ReviewTarget::UncommittedChanges, + cwd.path(), + Some("test-model".to_string()), + ) + .await; + let duplicate = AutoReviewDuplicateMatch { + run_id: "existing-run".to_string(), + status: AutoReviewRunStatus::Completed, + disposition: codex_auto_review::AutoReviewDuplicateDisposition::ReuseTerminal, + finding_count: 1, + model: Some("test-model".to_string()), + }; + + persistence.save_skipped_duplicate(codex_home.path(), &duplicate); + + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); + let run = store + .load_run("duplicate-candidate") + .expect("load persisted duplicate skip"); + assert_eq!(run.status, AutoReviewRunStatus::Skipped); + assert_eq!(run.freshness, AutoReviewRunFreshness::Superseded); + assert_eq!(run.superseded_by.as_deref(), Some("existing-run")); + assert_eq!( + run.cancel_reason.as_deref(), + Some("duplicate_auto_review_scope") + ); + assert_eq!( + run.error_summary.as_deref(), + Some("equivalent background auto review already exists: existing-run") + ); + } } diff --git a/codex-rs/core/src/session/background_auto_review.rs b/codex-rs/core/src/session/background_auto_review.rs index b5837b39bdb6..74d3dce73146 100644 --- a/codex-rs/core/src/session/background_auto_review.rs +++ b/codex-rs/core/src/session/background_auto_review.rs @@ -1,6 +1,9 @@ use std::sync::Arc; use std::time::Duration; +use codex_auto_review::AutoReviewDuplicateDisposition; +use codex_auto_review::AutoReviewDuplicateMatch; +use codex_auto_review::AutoReviewStore; use codex_git_utils::get_worktree_diff_byte_count; use codex_git_utils::get_worktree_diff_fingerprint; use codex_protocol::protocol::BackgroundAutoReviewControlAction; @@ -109,6 +112,24 @@ impl Session { Some(model), ) .await; + if let Some(duplicate) = self + .resolve_durable_background_auto_review_duplicate(&persistence) + .await + { + self.record_skipped_duplicate_background_auto_review( + &persistence, + schedule.generation, + &schedule.fingerprint, + &duplicate, + ) + .await; + debug!( + duplicate_run_id = %duplicate.run_id, + duplicate_status = ?duplicate.status, + "background auto review skipped: equivalent durable review already exists" + ); + return; + } if !self .record_pending_background_auto_review( schedule.generation, @@ -137,6 +158,8 @@ impl Session { ) .await; } + self.supersede_clean_durable_background_auto_review_duplicates(&persistence) + .await; let sess = Arc::clone(&self); tokio::spawn(async move { @@ -379,6 +402,101 @@ impl Session { } } + async fn record_skipped_duplicate_background_auto_review( + self: &Arc, + persistence: &ReviewPersistenceContext, + generation: u64, + fingerprint: &str, + duplicate: &AutoReviewDuplicateMatch, + ) { + let codex_home = self.codex_home().await; + let error_summary = format!( + "equivalent background auto review already exists: {}", + duplicate.run_id + ); + let mut state = self.state.lock().await; + state + .background_auto_review + .clear_pending_review(generation, fingerprint); + drop(state); + if persistence.save_skipped_duplicate(codex_home, duplicate) { + record_background_review_status( + Arc::clone(self), + persistence, + BackgroundAutoReviewStatus::Skipped, + Some(error_summary), + ) + .await; + } + } + + async fn resolve_durable_background_auto_review_duplicate( + &self, + persistence: &ReviewPersistenceContext, + ) -> Option { + let fingerprint = persistence.worktree_diff_fingerprint()?; + let codex_home = self.codex_home().await; + let store = AutoReviewStore::for_scope(codex_home, persistence.store_scope()); + let active_run_ids = { + let state = self.state.lock().await; + state.background_auto_review.active_snapshot() + }; + let duplicate = match store.find_duplicate_by_fingerprint_with_target_and_filter( + fingerprint, + persistence.target().branch.as_deref(), + persistence.target().head_sha.as_deref(), + |duplicate| match duplicate.disposition { + AutoReviewDuplicateDisposition::ReuseTerminal => true, + AutoReviewDuplicateDisposition::Adopt => { + active_run_ids.pending_run_id.as_deref() == Some(duplicate.run_id.as_str()) + || active_run_ids.running_run_id.as_deref() + == Some(duplicate.run_id.as_str()) + } + AutoReviewDuplicateDisposition::SupersedeTerminal => false, + }, + ) { + Ok(duplicate) => duplicate, + Err(err) => { + warn!(error = %err, "failed to query durable background auto review duplicates"); + None + } + }?; + if duplicate.run_id == persistence.run_id() { + return None; + } + Some(duplicate) + } + + async fn supersede_clean_durable_background_auto_review_duplicates( + &self, + persistence: &ReviewPersistenceContext, + ) { + let Some(fingerprint) = persistence.worktree_diff_fingerprint() else { + return; + }; + let codex_home = self.codex_home().await; + let store = AutoReviewStore::for_scope(codex_home, persistence.store_scope()); + match store.mark_superseded_by_fingerprint_with_target( + fingerprint, + persistence.run_id(), + persistence.target().branch.as_deref(), + persistence.target().head_sha.as_deref(), + ) { + Ok(changed) => { + if changed > 0 { + debug!( + run_id = %persistence.run_id(), + changed, + "superseded clean durable background auto review duplicates" + ); + } + } + Err(err) => { + warn!(error = %err, "failed to supersede durable background auto review duplicates"); + } + } + } + async fn record_started_background_auto_review_if_idle( &self, generation: u64, diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 0dae48505d69..c6f429f865e0 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -1,6 +1,7 @@ use codex_auto_review::AutoReviewRunSource; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewStore; +use codex_auto_review::SCHEMA_VERSION as AUTO_REVIEW_SCHEMA_VERSION; use codex_core::CodexThread; use codex_core::REVIEW_PROMPT; use codex_core::config::Config; @@ -1315,6 +1316,126 @@ async fn automatic_background_review_suppresses_unchanged_dirty_diff() -> anyhow Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn automatic_background_review_reuses_completed_durable_duplicate() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let patch = "*** Begin Patch\n*** Add File: auto_background_review_duplicate.txt\n+reuse durable review\n*** End Patch"; + let foreground_tool = vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_custom_tool_call("auto-bg-duplicate", patch), + ev_completed("resp-1"), + ]), + }]; + let foreground_complete = vec![StreamingSseChunk { + gate: None, + body: responses::sse(vec![ + ev_assistant_message("msg-1", "patch applied"), + ev_completed("resp-2"), + ]), + }]; + let (server, _completions) = + start_streaming_sse_server(vec![foreground_tool, foreground_complete]).await; + let codex_home = Arc::new(TempDir::new()?); + let test = test_codex() + .with_home(codex_home.clone()) + .build_with_streaming_server(&server) + .await?; + init_git_repo(test.cwd_path()); + + std::fs::write( + test.cwd_path().join("auto_background_review_duplicate.txt"), + "reuse durable review\n", + )?; + let fingerprint = expected_worktree_diff_fingerprint(test.cwd_path()).await; + std::fs::remove_file(test.cwd_path().join("auto_background_review_duplicate.txt"))?; + let existing = codex_auto_review::AutoReviewRun { + schema_version: AUTO_REVIEW_SCHEMA_VERSION, + run_id: "existing_duplicate_review".to_string(), + status: AutoReviewRunStatus::Completed, + freshness: codex_auto_review::AutoReviewRunFreshness::Current, + source: AutoReviewRunSource::Background, + target: codex_auto_review::AutoReviewRunTarget { + branch: Some("main".to_string()), + head_sha: current_git_head(test.cwd_path()), + base_sha: None, + worktree_path: Some(test.cwd_path().to_path_buf()), + worktree_diff_fingerprint: Some(fingerprint.clone()), + }, + review_target: ReviewTarget::UncommittedChanges, + started_at_unix_secs: 1, + completed_at_unix_secs: Some(2), + model: Some("gpt-test".to_string()), + superseded_by: None, + cancel_reason: None, + error_summary: None, + findings: vec![codex_auto_review::AutoReviewFindingRecord { + finding_id: "f1".to_string(), + finding: ReviewFinding { + title: "Existing durable finding".to_string(), + body: "existing durable finding body".to_string(), + confidence_score: 0.9, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: test + .cwd_path() + .join("auto_background_review_duplicate.txt"), + line_range: ReviewLineRange { start: 1, end: 1 }, + }, + }, + }], + }; + AutoReviewStore::for_scope(codex_home.path(), test.cwd_path()).save_run(&existing)?; + + test.submit_turn("create a file with an already reviewed diff") + .await?; + + let skipped_status = wait_for_background_auto_review_status( + test.codex.as_ref(), + BackgroundAutoReviewStatus::Skipped, + None, + ) + .await; + assert_eq!( + skipped_status.error_summary.as_deref(), + Some("equivalent background auto review already exists: existing_duplicate_review") + ); + server + .assert_request_count_stays(2, Duration::from_millis(2500)) + .await; + + let runs = load_auto_review_runs(codex_home.path())?; + assert_eq!(runs.len(), 2); + let skipped = runs + .iter() + .find(|run| run.run_id == skipped_status.run_id) + .expect("expected duplicate candidate run"); + assert_eq!(skipped.status, AutoReviewRunStatus::Skipped); + assert_eq!( + skipped.superseded_by.as_deref(), + Some("existing_duplicate_review") + ); + assert_eq!( + skipped.cancel_reason.as_deref(), + Some("duplicate_auto_review_scope") + ); + let completed = runs + .iter() + .find(|run| run.run_id == "existing_duplicate_review") + .expect("expected completed duplicate to remain visible"); + assert_eq!(completed.status, AutoReviewRunStatus::Completed); + assert_eq!( + completed.target.worktree_diff_fingerprint.as_deref(), + Some(fingerprint.as_str()) + ); + + let _codex_home_guard = codex_home; + server.shutdown().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn automatic_background_review_debounce_ignores_superseded_diff() -> anyhow::Result<()> { skip_if_no_network!(Ok(()));