diff --git a/codex-rs/app-server/src/request_processors/turn_processor.rs b/codex-rs/app-server/src/request_processors/turn_processor.rs index f6b69c54d66b..c29996b9e171 100644 --- a/codex-rs/app-server/src/request_processors/turn_processor.rs +++ b/codex-rs/app-server/src/request_processors/turn_processor.rs @@ -415,11 +415,16 @@ impl TurnRequestProcessor { fn api_auto_review_status(status: AutoReviewRunStatus) -> ApiBackgroundAutoReviewStatus { match status { AutoReviewRunStatus::Pending => ApiBackgroundAutoReviewStatus::Pending, - AutoReviewRunStatus::Running => ApiBackgroundAutoReviewStatus::Running, + AutoReviewRunStatus::Snapshotting + | AutoReviewRunStatus::Running + | AutoReviewRunStatus::Reviewing + | AutoReviewRunStatus::Resolving => ApiBackgroundAutoReviewStatus::Running, AutoReviewRunStatus::Completed => ApiBackgroundAutoReviewStatus::Completed, AutoReviewRunStatus::Failed => ApiBackgroundAutoReviewStatus::Failed, AutoReviewRunStatus::Cancelled => ApiBackgroundAutoReviewStatus::Cancelled, + AutoReviewRunStatus::Superseded => ApiBackgroundAutoReviewStatus::Skipped, AutoReviewRunStatus::Skipped => ApiBackgroundAutoReviewStatus::Skipped, + AutoReviewRunStatus::Lost => ApiBackgroundAutoReviewStatus::Cancelled, } } diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 7e24c795244d..3e657ec79a00 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -934,6 +934,7 @@ fn sample_auto_review_run( schema_version: SCHEMA_VERSION, run_id: run_id.to_string(), status: AutoReviewRunStatus::Completed, + freshness: codex_auto_review::AutoReviewRunFreshness::Current, source: AutoReviewRunSource::Background, target: AutoReviewRunTarget { branch: None, @@ -946,6 +947,8 @@ fn sample_auto_review_run( started_at_unix_secs: 1, completed_at_unix_secs: Some(2), model: Some("review-model".to_string()), + superseded_by: None, + cancel_reason: None, error_summary: None, findings: vec![AutoReviewFindingRecord { finding_id: finding_id.to_string(), diff --git a/codex-rs/auto-review/src/lib.rs b/codex-rs/auto-review/src/lib.rs index 0d35c8d8f995..c69c77b3c262 100644 --- a/codex-rs/auto-review/src/lib.rs +++ b/codex-rs/auto-review/src/lib.rs @@ -32,6 +32,22 @@ pub struct AutoReviewStore { legacy_root: Option, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AutoReviewDuplicateMatch { + pub run_id: String, + pub status: AutoReviewRunStatus, + pub disposition: AutoReviewDuplicateDisposition, + pub finding_count: usize, + pub model: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AutoReviewDuplicateDisposition { + Adopt, + ReuseTerminal, + SupersedeTerminal, +} + impl AutoReviewStore { pub fn for_scope(codex_home: impl AsRef, scope: impl AsRef) -> Self { let codex_home = codex_home.as_ref(); @@ -55,7 +71,6 @@ impl AutoReviewStore { pub fn save_run(&self, run: &AutoReviewRun) -> Result { validate_run(run)?; - self.save_output(run)?; let mut index = self.load_index()?; index.upsert(run.clone()); let runs_path = self.runs_path(); @@ -66,9 +81,144 @@ impl AutoReviewStore { runs_path.display() ) })?; + let _ = self.save_output(run); Ok(runs_path) } + pub fn mark_superseded(&self, run_id: &str, superseded_by: &str) -> Result { + validate_safe_id(run_id).context("auto review run_id")?; + validate_safe_id(superseded_by).context("auto review superseded_by")?; + let mut index = self.load_index()?; + let Some(run) = index.runs.iter_mut().find(|run| run.run_id == run_id) else { + return Ok(false); + }; + if run.run_id == superseded_by + || run.status.is_in_flight() + || run.status == AutoReviewRunStatus::Superseded + || !run.findings.is_empty() + || run.error_summary.is_some() + || run.cancel_reason.is_some() + { + return Ok(false); + } + run.status = AutoReviewRunStatus::Superseded; + run.freshness = AutoReviewRunFreshness::Superseded; + run.completed_at_unix_secs = run + .completed_at_unix_secs + .or(Some(run.started_at_unix_secs)); + run.superseded_by = Some(superseded_by.to_string()); + self.save_run(&run.clone())?; + Ok(true) + } + + pub fn mark_superseded_by_fingerprint( + &self, + diff_fingerprint: &str, + superseded_by: &str, + ) -> Result { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return Ok(0); + } + validate_safe_id(superseded_by).context("auto review superseded_by")?; + let mut changed = 0; + for run in self.load_index()?.runs { + if run.run_id == superseded_by + || run.target.worktree_diff_fingerprint.as_deref() != Some(fingerprint) + { + continue; + } + changed += usize::from(self.mark_superseded(&run.run_id, superseded_by)?); + } + Ok(changed) + } + + pub fn reconcile_orphaned_in_flight( + &self, + live_run_ids: I, + now_unix_secs: i64, + ) -> Result + where + I: IntoIterator, + I::Item: AsRef, + { + let live_run_ids = live_run_ids + .into_iter() + .map(|run_id| run_id.as_ref().to_string()) + .collect::>(); + let mut index = self.load_index()?; + let mut changed = Vec::new(); + for run in &mut index.runs { + if !run.status.is_in_flight() { + continue; + } + if live_run_ids.contains(&run.run_id) { + continue; + } + run.status = AutoReviewRunStatus::Lost; + run.freshness = AutoReviewRunFreshness::Lost; + run.completed_at_unix_secs = Some(now_unix_secs); + run.cancel_reason = Some("agent_missing_after_restart".to_string()); + changed.push(run.clone()); + } + let changed_count = changed.len(); + for run in changed { + self.save_run(&run)?; + } + Ok(changed_count) + } + + pub fn find_duplicate_by_fingerprint( + &self, + diff_fingerprint: &str, + ) -> Result> { + self.find_duplicate_by_fingerprint_with_target( + diff_fingerprint, + /*active_branch*/ None, + /*active_head*/ None, + ) + } + + pub fn find_duplicate_by_fingerprint_with_target( + &self, + diff_fingerprint: &str, + active_branch: Option<&str>, + active_head: Option<&str>, + ) -> Result> { + let fingerprint = diff_fingerprint.trim(); + if fingerprint.is_empty() { + return Ok(None); + } + Ok(self + .load_index()? + .runs + .into_iter() + .filter(|run| run.target.worktree_diff_fingerprint.as_deref() == Some(fingerprint)) + .filter(|run| { + !matches!( + run.status, + AutoReviewRunStatus::Lost + | AutoReviewRunStatus::Skipped + | AutoReviewRunStatus::Superseded + ) + }) + .filter(|run| duplicate_target_is_reusable(run, active_branch, active_head)) + .max_by(|left, right| { + duplicate_priority(left) + .cmp(&duplicate_priority(right)) + .then_with(|| { + auto_review_run_sort_key(left).cmp(&auto_review_run_sort_key(right)) + }) + }) + .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, + })) + } + pub fn load_run(&self, run_id: &str) -> Result { validate_safe_id(run_id).context("auto review run_id")?; let Some(run) = self @@ -124,8 +274,8 @@ impl AutoReviewStore { index.upsert(run); } } - for run in self.load_output_runs()? { - index.upsert(run); + for run in self.load_output_runs() { + index.upsert_if_absent(run); } index.validate()?; Ok(index) @@ -148,25 +298,17 @@ impl AutoReviewStore { Ok(run) } - fn load_output_runs(&self) -> Result> { + fn load_output_runs(&self) -> Vec { let outputs_dir = self.root.join(OUTPUTS_DIR); if !outputs_dir.exists() { - return Ok(Vec::new()); + return Vec::new(); } let mut run_ids = Vec::new(); - for entry in std::fs::read_dir(&outputs_dir).with_context(|| { - format!( - "failed to read auto review outputs dir {}", - outputs_dir.display() - ) - })? { - let entry = entry.with_context(|| { - format!( - "failed to read auto review outputs dir entry {}", - outputs_dir.display() - ) - })?; + let Ok(entries) = std::fs::read_dir(&outputs_dir) else { + return Vec::new(); + }; + for entry in entries.flatten() { let path = entry.path(); if path.extension().and_then(|ext| ext.to_str()) != Some("json") { continue; @@ -174,14 +316,18 @@ impl AutoReviewStore { let Some(run_id) = path.file_stem().and_then(|stem| stem.to_str()) else { continue; }; - validate_safe_id(run_id).context("auto review run_id")?; - run_ids.push(run_id.to_string()); + if validate_safe_id(run_id).is_ok() { + run_ids.push(run_id.to_string()); + } } run_ids.sort(); run_ids .into_iter() - .map(|run_id| self.load_output(&run_id)) + .filter_map(|run_id| match self.load_output(&run_id) { + Ok(run) => Some(run), + Err(_) => None, + }) .collect() } @@ -283,6 +429,17 @@ impl AutoReviewRunsIndex { by_id.insert(run.run_id.clone(), run); self.runs = by_id.into_values().collect(); } + + fn upsert_if_absent(&mut self, run: AutoReviewRun) { + if self + .runs + .iter() + .any(|existing| existing.run_id == run.run_id) + { + return; + } + self.upsert(run); + } } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] @@ -291,12 +448,18 @@ pub struct AutoReviewRun { pub schema_version: u32, pub run_id: String, pub status: AutoReviewRunStatus, + #[serde(default)] + pub freshness: AutoReviewRunFreshness, pub source: AutoReviewRunSource, pub target: AutoReviewRunTarget, pub review_target: ReviewTarget, pub started_at_unix_secs: i64, pub completed_at_unix_secs: Option, pub model: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub superseded_by: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub cancel_reason: Option, pub error_summary: Option, pub findings: Vec, } @@ -320,6 +483,12 @@ impl AutoReviewRun { } pub fn freshness(&self, active_target: &AutoReviewRunTarget) -> AutoReviewFreshness { + if self.freshness == AutoReviewRunFreshness::Lost { + return AutoReviewFreshness::Stale; + } + if self.freshness == AutoReviewRunFreshness::Superseded { + return AutoReviewFreshness::Stale; + } self.target.freshness(active_target) } @@ -376,11 +545,51 @@ impl AutoReviewRun { #[serde(rename_all = "snake_case")] pub enum AutoReviewRunStatus { Pending, + Snapshotting, Running, + Reviewing, + Resolving, Completed, Failed, Cancelled, + Superseded, Skipped, + Lost, +} + +impl AutoReviewRunStatus { + pub fn is_terminal(&self) -> bool { + matches!( + self, + Self::Completed + | Self::Failed + | Self::Cancelled + | Self::Superseded + | Self::Skipped + | Self::Lost + ) + } + + pub fn is_in_flight(&self) -> bool { + !self.is_terminal() + } + + pub fn is_adoptable_duplicate(&self) -> bool { + matches!(self, Self::Running | Self::Reviewing | Self::Resolving) + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)] +#[serde(rename_all = "snake_case")] +pub enum AutoReviewRunFreshness { + Current, + LongRunning, + Inactive, + Superseded, + Obsolete, + Lost, + #[default] + Unknown, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -483,6 +692,58 @@ fn review_target_matches(stored: &ReviewTarget, active: &ReviewTarget) -> bool { } } +fn duplicate_priority(run: &AutoReviewRun) -> u8 { + if run.status.is_adoptable_duplicate() { + return 4; + } + if !run.findings.is_empty() { + return 3; + } + if run.status == AutoReviewRunStatus::Completed { + return 2; + } + 1 +} + +fn duplicate_disposition(run: &AutoReviewRun) -> AutoReviewDuplicateDisposition { + if run.status.is_adoptable_duplicate() { + AutoReviewDuplicateDisposition::Adopt + } else if run.status == AutoReviewRunStatus::Completed { + AutoReviewDuplicateDisposition::ReuseTerminal + } else { + AutoReviewDuplicateDisposition::SupersedeTerminal + } +} + +fn duplicate_target_is_reusable( + run: &AutoReviewRun, + active_branch: Option<&str>, + active_head: Option<&str>, +) -> bool { + if active_head.and_then(non_empty_str).is_none() { + return true; + } + if let Some(active_branch) = active_branch.and_then(non_empty_str) + && run.target.branch.as_deref() != Some(active_branch) + { + return false; + } + run.target.head_sha.as_deref() == active_head.and_then(non_empty_str) +} + +fn non_empty_str(value: &str) -> Option<&str> { + let value = value.trim(); + (!value.is_empty()).then_some(value) +} + +fn auto_review_run_sort_key(run: &AutoReviewRun) -> (i64, &str) { + ( + run.completed_at_unix_secs + .unwrap_or(run.started_at_unix_secs), + run.run_id.as_str(), + ) +} + fn render_summary(findings: Vec<&AutoReviewFindingRecord>) -> AutoReviewSummary { if findings.is_empty() { return AutoReviewSummary { diff --git a/codex-rs/auto-review/src/lib_tests.rs b/codex-rs/auto-review/src/lib_tests.rs index e22ff281eebe..ee6ceee7a943 100644 --- a/codex-rs/auto-review/src/lib_tests.rs +++ b/codex-rs/auto-review/src/lib_tests.rs @@ -7,9 +7,11 @@ use codex_protocol::protocol::ReviewTarget; use pretty_assertions::assert_eq; use super::AutoReviewDetail; +use super::AutoReviewDuplicateDisposition; use super::AutoReviewFindingRecord; use super::AutoReviewFreshness; use super::AutoReviewRun; +use super::AutoReviewRunFreshness; use super::AutoReviewRunSource; use super::AutoReviewRunStatus; use super::AutoReviewRunTarget; @@ -109,6 +111,59 @@ fn list_runs_recovers_runs_missing_from_index_but_present_in_outputs() -> anyhow Ok(()) } +#[test] +fn sidecar_does_not_override_index_run() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let indexed = AutoReviewRun { + status: AutoReviewRunStatus::Running, + completed_at_unix_secs: None, + findings: Vec::new(), + ..sample_run("run_1", Vec::new()) + }; + let sidecar = AutoReviewRun { + status: AutoReviewRunStatus::Completed, + findings: vec![sample_finding("f1", "Sidecar")], + ..sample_run("run_1", Vec::new()) + }; + let index = serde_json::json!({ + "schema_version": SCHEMA_VERSION, + "runs": [indexed], + }); + let runs_path = store.runs_path(); + std::fs::create_dir_all(runs_path.parent().expect("runs path parent"))?; + std::fs::write(&runs_path, format!("{index}\n"))?; + let output_path = store.output_path("run_1")?; + std::fs::create_dir_all(output_path.parent().expect("output path parent"))?; + std::fs::write(&output_path, serde_json::to_string_pretty(&sidecar)?)?; + + let loaded = store.load_run("run_1")?; + + assert_eq!(loaded.status, AutoReviewRunStatus::Running); + assert!(loaded.findings.is_empty()); + Ok(()) +} + +#[test] +fn corrupt_sidecar_does_not_block_store_listing() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&sample_run("run_1", Vec::new()))?; + let bad_path = store.output_path("bad_run")?; + std::fs::create_dir_all(bad_path.parent().expect("output path parent"))?; + std::fs::write(&bad_path, "not json\n")?; + + let runs = store.list_runs()?; + + assert_eq!( + runs.into_iter().map(|run| run.run_id).collect::>(), + vec!["run_1".to_string()] + ); + Ok(()) +} + #[test] fn list_runs_returns_empty_when_store_is_missing() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; @@ -323,6 +378,50 @@ fn load_run_defaults_missing_worktree_diff_fingerprint() -> anyhow::Result<()> { Ok(()) } +#[test] +fn load_run_defaults_missing_lifecycle_fields() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let path = store.runs_path(); + std::fs::create_dir_all(path.parent().expect("runs path parent"))?; + std::fs::write( + &path, + r#"{ + "schema_version": 1, + "runs": [ + { + "schema_version": 1, + "run_id": "run_1", + "status": "completed", + "source": "manual", + "target": { + "branch": "main", + "head_sha": "head-2", + "base_sha": "base-1", + "worktree_path": "/repo" + }, + "review_target": { + "type": "uncommittedChanges" + }, + "started_at_unix_secs": 1, + "completed_at_unix_secs": 2, + "model": "gpt-test", + "error_summary": null, + "findings": [] + } + ] +}"#, + )?; + + let loaded = store.load_run("run_1")?; + + assert_eq!(loaded.freshness, AutoReviewRunFreshness::Unknown); + assert_eq!(loaded.superseded_by, None); + assert_eq!(loaded.cancel_reason, None); + Ok(()) +} + #[test] fn unsafe_run_ids_are_rejected() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -472,10 +571,15 @@ fn visible_findings_require_completed_run_status() { for status in [ AutoReviewRunStatus::Pending, + AutoReviewRunStatus::Snapshotting, AutoReviewRunStatus::Running, + AutoReviewRunStatus::Reviewing, + AutoReviewRunStatus::Resolving, AutoReviewRunStatus::Failed, AutoReviewRunStatus::Cancelled, + AutoReviewRunStatus::Superseded, AutoReviewRunStatus::Skipped, + AutoReviewRunStatus::Lost, ] { let run = AutoReviewRun { status, @@ -490,6 +594,236 @@ fn visible_findings_require_completed_run_status() { } } +#[test] +fn duplicate_lookup_prefers_adoptable_in_flight_match() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let completed = AutoReviewRun { + target: target_with_fingerprint("sha256:abc"), + ..sample_run("completed", Vec::new()) + }; + let reviewing = AutoReviewRun { + status: AutoReviewRunStatus::Reviewing, + completed_at_unix_secs: None, + target: target_with_fingerprint("sha256:abc"), + ..sample_run("reviewing", Vec::new()) + }; + store.save_run(&completed)?; + store.save_run(&reviewing)?; + + let duplicate = store + .find_duplicate_by_fingerprint("sha256:abc")? + .expect("duplicate match"); + + assert_eq!(duplicate.run_id, "reviewing"); + assert_eq!(duplicate.disposition, AutoReviewDuplicateDisposition::Adopt); + Ok(()) +} + +#[test] +fn duplicate_lookup_can_filter_stale_target_matches() -> 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", Vec::new()) + })?; + + assert!( + store + .find_duplicate_by_fingerprint_with_target( + "sha256:abc", + Some("main"), + Some("new-head") + )? + .is_none() + ); + assert!( + store + .find_duplicate_by_fingerprint_with_target( + "sha256:abc", + Some("main"), + Some("old-head") + )? + .is_some() + ); + Ok(()) +} + +#[test] +fn duplicate_lookup_reports_pending_but_ignores_lost_skipped_and_superseded() -> 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 { + status: AutoReviewRunStatus::Pending, + target: target_with_fingerprint("sha256:pending"), + ..sample_run("pending", Vec::new()) + })?; + for (run_id, status) in [ + ("lost", AutoReviewRunStatus::Lost), + ("skipped", AutoReviewRunStatus::Skipped), + ("superseded", AutoReviewRunStatus::Superseded), + ] { + store.save_run(&AutoReviewRun { + status, + target: target_with_fingerprint("sha256:abc"), + ..sample_run(run_id, Vec::new()) + })?; + } + + assert_eq!(store.find_duplicate_by_fingerprint("sha256:abc")?, None); + let duplicate = store + .find_duplicate_by_fingerprint("sha256:pending")? + .expect("pending duplicate match"); + assert_eq!(duplicate.run_id, "pending"); + assert_eq!( + duplicate.disposition, + AutoReviewDuplicateDisposition::SupersedeTerminal + ); + Ok(()) +} + +#[test] +fn mark_superseded_preserves_runs_with_evidence() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&sample_run("clean", Vec::new()))?; + store.save_run(&sample_run( + "with_finding", + vec![sample_finding("f1", "Keep")], + ))?; + + assert!(store.mark_superseded("clean", "new_run")?); + assert!(!store.mark_superseded("with_finding", "new_run")?); + + let clean = store.load_run("clean")?; + let with_finding = store.load_run("with_finding")?; + assert_eq!(clean.status, AutoReviewRunStatus::Superseded); + assert_eq!(clean.freshness, AutoReviewRunFreshness::Superseded); + assert_eq!(clean.superseded_by.as_deref(), Some("new_run")); + assert_eq!(with_finding.status, AutoReviewRunStatus::Completed); + Ok(()) +} + +#[test] +fn mark_superseded_by_fingerprint_only_supersedes_matching_scope() -> 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("same_scope", Vec::new()) + })?; + store.save_run(&AutoReviewRun { + target: target_with_fingerprint("sha256:other"), + ..sample_run("other_scope", Vec::new()) + })?; + + let changed = store.mark_superseded_by_fingerprint("sha256:abc", "new_run")?; + + assert_eq!(changed, 1); + assert_eq!( + store.load_run("same_scope")?.status, + AutoReviewRunStatus::Superseded + ); + assert_eq!( + store.load_run("other_scope")?.status, + AutoReviewRunStatus::Completed + ); + Ok(()) +} + +#[test] +fn reconcile_orphaned_in_flight_marks_lost() -> 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 { + status: AutoReviewRunStatus::Reviewing, + completed_at_unix_secs: None, + ..sample_run("reviewing", Vec::new()) + })?; + store.save_run(&sample_run("completed", Vec::new()))?; + + let changed = store.reconcile_orphaned_in_flight(std::iter::empty::<&str>(), 42)?; + + assert_eq!(changed, 1); + let lost = store.load_run("reviewing")?; + assert_eq!(lost.status, AutoReviewRunStatus::Lost); + assert_eq!(lost.freshness, AutoReviewRunFreshness::Lost); + assert_eq!(lost.completed_at_unix_secs, Some(42)); + assert_eq!( + lost.cancel_reason.as_deref(), + Some("agent_missing_after_restart") + ); + assert_eq!( + store.load_run("completed")?.status, + AutoReviewRunStatus::Completed + ); + Ok(()) +} + +#[test] +fn reconcile_orphaned_in_flight_preserves_live_runs() -> 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 { + status: AutoReviewRunStatus::Reviewing, + completed_at_unix_secs: None, + ..sample_run("live", Vec::new()) + })?; + store.save_run(&AutoReviewRun { + status: AutoReviewRunStatus::Reviewing, + completed_at_unix_secs: None, + ..sample_run("orphan", Vec::new()) + })?; + + let changed = store.reconcile_orphaned_in_flight(["live"], 42)?; + + assert_eq!(changed, 1); + assert_eq!( + store.load_run("live")?.status, + AutoReviewRunStatus::Reviewing + ); + assert_eq!(store.load_run("orphan")?.status, AutoReviewRunStatus::Lost); + Ok(()) +} + +#[test] +fn lifecycle_freshness_overrides_matching_target_freshness() { + let active = sample_target("main", "head-2", "/repo"); + for (freshness, status) in [ + (AutoReviewRunFreshness::Lost, AutoReviewRunStatus::Lost), + ( + AutoReviewRunFreshness::Superseded, + AutoReviewRunStatus::Superseded, + ), + ] { + let run = AutoReviewRun { + freshness, + status, + ..sample_run("run_1", vec![sample_finding("f1", "Title")]) + }; + + assert_eq!(run.freshness(&active), AutoReviewFreshness::Stale); + assert!( + run.visible_findings(&active, &ReviewTarget::UncommittedChanges) + .is_empty() + ); + } +} + #[test] fn finding_detail_requires_completed_run_status() { let run = AutoReviewRun { @@ -702,17 +1036,27 @@ fn sample_run(run_id: &str, findings: Vec) -> AutoRevie schema_version: SCHEMA_VERSION, run_id: run_id.to_string(), status: AutoReviewRunStatus::Completed, + freshness: AutoReviewRunFreshness::Current, source: AutoReviewRunSource::Manual, target: sample_target("main", "head-2", "/repo"), 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, } } +fn target_with_fingerprint(fingerprint: &str) -> AutoReviewRunTarget { + AutoReviewRunTarget { + worktree_diff_fingerprint: Some(fingerprint.to_string()), + ..sample_target("main", "head-2", "/repo") + } +} + fn sample_target(branch: &str, head_sha: &str, worktree_path: &str) -> AutoReviewRunTarget { AutoReviewRunTarget { branch: Some(branch.to_string()), diff --git a/codex-rs/core/src/context/auto_review_awareness.rs b/codex-rs/core/src/context/auto_review_awareness.rs index ce935e77a0cb..6ed29adece1e 100644 --- a/codex-rs/core/src/context/auto_review_awareness.rs +++ b/codex-rs/core/src/context/auto_review_awareness.rs @@ -202,11 +202,16 @@ fn status_count_key( }; let status = match &run.status { AutoReviewRunStatus::Pending => "pending", + AutoReviewRunStatus::Snapshotting => "snapshotting", AutoReviewRunStatus::Running => "running", + AutoReviewRunStatus::Reviewing => "reviewing", + AutoReviewRunStatus::Resolving => "resolving", AutoReviewRunStatus::Completed => "completed", AutoReviewRunStatus::Failed => "failed", AutoReviewRunStatus::Cancelled => "cancelled", + AutoReviewRunStatus::Superseded => "superseded", AutoReviewRunStatus::Skipped => "skipped", + AutoReviewRunStatus::Lost => "lost", }; let source = match &run.source { AutoReviewRunSource::Manual => "manual", diff --git a/codex-rs/core/src/context/auto_review_awareness_tests.rs b/codex-rs/core/src/context/auto_review_awareness_tests.rs index c211ada8b7c2..f79d400bf2fe 100644 --- a/codex-rs/core/src/context/auto_review_awareness_tests.rs +++ b/codex-rs/core/src/context/auto_review_awareness_tests.rs @@ -213,12 +213,15 @@ fn sample_run( schema_version: codex_auto_review::SCHEMA_VERSION, run_id: run_id.to_string(), status, + freshness: codex_auto_review::AutoReviewRunFreshness::Current, source: AutoReviewRunSource::Background, target: sample_target("main", "head-2", "/repo"), 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, } diff --git a/codex-rs/core/src/review_persistence.rs b/codex-rs/core/src/review_persistence.rs index 91d20919e76c..76cb62f27322 100644 --- a/codex-rs/core/src/review_persistence.rs +++ b/codex-rs/core/src/review_persistence.rs @@ -4,6 +4,7 @@ use std::sync::Mutex; use codex_auto_review::AutoReviewFindingRecord; use codex_auto_review::AutoReviewRun; +use codex_auto_review::AutoReviewRunFreshness; use codex_auto_review::AutoReviewRunSource; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewRunTarget; @@ -181,12 +182,15 @@ impl ReviewPersistenceContext { schema_version: SCHEMA_VERSION, run_id: self.run_id.clone(), status, + freshness: AutoReviewRunFreshness::Current, 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, error_summary, findings: output .map(|output| finding_records(&output.findings)) @@ -205,10 +209,7 @@ impl ReviewPersistenceContext { } fn is_terminal_status(status: &AutoReviewRunStatus) -> bool { - !matches!( - status, - AutoReviewRunStatus::Pending | AutoReviewRunStatus::Running - ) + status.is_terminal() } pub(crate) async fn collect_auto_review_target( diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 8c19cb0a40dc..0dae48505d69 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -727,6 +727,7 @@ async fn auto_review_awareness_injected_for_current_findings() -> anyhow::Result schema_version: codex_auto_review::SCHEMA_VERSION, run_id: "awareness_run".to_string(), status: AutoReviewRunStatus::Completed, + freshness: codex_auto_review::AutoReviewRunFreshness::Current, source: AutoReviewRunSource::Background, target: codex_auto_review::AutoReviewRunTarget { branch: Some("main".to_string()), @@ -739,6 +740,8 @@ async fn auto_review_awareness_injected_for_current_findings() -> anyhow::Result 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(),