Wire durable auto-review runtime dedupe

codex-rs/auto-review/src/lib.rs

@@ -115,6 +115,21 @@ impl AutoReviewStore {
         &self,
         diff_fingerprint: &str,
         superseded_by: &str,
+    ) -> Result<usize> {
+        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<usize> {
         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<Option<AutoReviewDuplicateMatch>> {
+        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<F>(
+        &self,
+        diff_fingerprint: &str,
+        active_branch: Option<&str>,
+        active_head: Option<&str>,
+        is_eligible: F,
+    ) -> Result<Option<AutoReviewDuplicateMatch>>
+    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<AutoReviewRun> {

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()?;

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<Path>,
+        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<Path>,
         status: AutoReviewRunStatus,
         output: Option<&ReviewOutputEvent>,
         error_summary: Option<String>,
+    ) -> 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<Path>,
+        status: AutoReviewRunStatus,
+        output: Option<&ReviewOutputEvent>,
+        error_summary: Option<String>,
+        freshness: AutoReviewRunFreshness,
+        superseded_by: Option<String>,
+        cancel_reason: Option<String>,
     ) -> 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")
+        );
+    }
 }