From aa8cc8f129d6daedbfdc6d61f9c037d0469d28a9 Mon Sep 17 00:00:00 2001 From: shiny-code-bot Date: Wed, 17 Jun 2026 23:34:43 -0400 Subject: [PATCH] Fix auto-review store migration awareness --- codex-rs/auto-review/src/lib.rs | 127 ++++++++++++-- codex-rs/auto-review/src/lib_tests.rs | 161 ++++++++++++++++++ .../core/src/context/auto_review_awareness.rs | 7 + 3 files changed, 285 insertions(+), 10 deletions(-) diff --git a/codex-rs/auto-review/src/lib.rs b/codex-rs/auto-review/src/lib.rs index e371a1eea498..0d35c8d8f995 100644 --- a/codex-rs/auto-review/src/lib.rs +++ b/codex-rs/auto-review/src/lib.rs @@ -20,6 +20,7 @@ pub const SCHEMA_VERSION: u32 = 1; const STORE_DIR: &str = "auto-review"; const STATE_DIR: &str = "state"; const REVIEW_DIR: &str = "review"; +const LEGACY_RUNS_DIR: &str = "runs"; const RUNS_FILENAME: &str = "runs.json"; const OUTPUTS_DIR: &str = "outputs"; const OMITTED_TEMPLATE_PREFIX: &str = "... "; @@ -28,17 +29,28 @@ const OMITTED_TEMPLATE_SUFFIX: &str = " more finding(s) omitted"; #[derive(Debug, Clone)] pub struct AutoReviewStore { root: PathBuf, + legacy_root: Option, } impl AutoReviewStore { pub fn for_scope(codex_home: impl AsRef, scope: impl AsRef) -> Self { + let codex_home = codex_home.as_ref(); Self { - root: scoped_store_root(codex_home.as_ref(), scope.as_ref()), + root: scoped_store_root(codex_home, scope.as_ref()), + legacy_root: Some(legacy_store_root(codex_home)), } } pub fn from_store_root(root: impl Into) -> Self { - Self { root: root.into() } + Self { + root: root.into(), + legacy_root: None, + } + } + + pub fn has_store_files(codex_home: impl AsRef) -> bool { + let codex_home = codex_home.as_ref(); + has_loadable_legacy_run(codex_home) || has_scoped_store_files(codex_home) } pub fn save_run(&self, run: &AutoReviewRun) -> Result { @@ -83,8 +95,7 @@ impl AutoReviewStore { finding_id: &str, max_bytes: usize, ) -> Result { - self.load_output(run_id)? - .finding_detail(finding_id, max_bytes) + self.load_run(run_id)?.finding_detail(finding_id, max_bytes) } pub fn output_path(&self, run_id: &str) -> Result { @@ -98,16 +109,21 @@ impl AutoReviewStore { fn load_index(&self) -> Result { let path = self.runs_path(); - let mut index = if path.exists() { + let mut index = AutoReviewRunsIndex::default(); + for run in self.load_legacy_runs()? { + index.upsert(run); + } + if path.exists() { let json = std::fs::read_to_string(&path).with_context(|| { format!("failed to read auto review runs index {}", path.display()) })?; - serde_json::from_str(&json).with_context(|| { + let parsed: AutoReviewRunsIndex = serde_json::from_str(&json).with_context(|| { format!("failed to parse auto review runs index {}", path.display()) - })? - } else { - AutoReviewRunsIndex::default() - }; + })?; + for run in parsed.runs { + index.upsert(run); + } + } for run in self.load_output_runs()? { index.upsert(run); } @@ -168,6 +184,61 @@ impl AutoReviewStore { .map(|run_id| self.load_output(&run_id)) .collect() } + + fn legacy_run_path(&self, run_id: &str) -> Result> { + validate_safe_id(run_id).context("auto review run_id")?; + Ok(self + .legacy_root + .as_ref() + .map(|root| root.join(LEGACY_RUNS_DIR).join(format!("{run_id}.json")))) + } + + fn load_legacy_run(&self, run_id: &str) -> Result { + let Some(path) = self.legacy_run_path(run_id)? else { + anyhow::bail!("legacy auto review store is unavailable"); + }; + let json = std::fs::read_to_string(&path) + .with_context(|| format!("failed to read legacy auto review run {run_id}"))?; + let run = serde_json::from_str(&json) + .with_context(|| format!("failed to parse legacy auto review run {run_id}"))?; + validate_run(&run)?; + Ok(run) + } + + fn load_legacy_runs(&self) -> Result> { + let Some(root) = &self.legacy_root else { + return Ok(Vec::new()); + }; + let runs_dir = root.join(LEGACY_RUNS_DIR); + if !runs_dir.exists() { + return Ok(Vec::new()); + } + + let mut run_ids = Vec::new(); + let Ok(entries) = std::fs::read_dir(&runs_dir) else { + return Ok(Vec::new()); + }; + + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().and_then(|ext| ext.to_str()) != Some("json") { + continue; + } + let Some(run_id) = path.file_stem().and_then(|stem| stem.to_str()) else { + continue; + }; + if validate_safe_id(run_id).is_ok() { + run_ids.push(run_id.to_string()); + } + } + + run_ids.sort(); + let runs = run_ids + .into_iter() + .filter_map(|run_id| self.load_legacy_run(&run_id).ok()) + .collect(); + Ok(runs) + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -487,6 +558,42 @@ fn scoped_store_root(codex_home: &Path, scope: &Path) -> PathBuf { .join(STORE_DIR) } +fn legacy_store_root(codex_home: &Path) -> PathBuf { + codex_home.join(STORE_DIR) +} + +fn has_loadable_legacy_run(codex_home: &Path) -> bool { + let store = AutoReviewStore { + root: scoped_store_root(codex_home, codex_home), + legacy_root: Some(legacy_store_root(codex_home)), + }; + store.load_legacy_runs().is_ok_and(|runs| !runs.is_empty()) +} + +fn has_scoped_store_files(codex_home: &Path) -> bool { + let review_dir = codex_home.join(STATE_DIR).join(REVIEW_DIR); + let Ok(entries) = std::fs::read_dir(&review_dir) else { + return false; + }; + + for entry in entries.flatten() { + let store_root = entry.path().join(STORE_DIR); + if store_root.join(RUNS_FILENAME).exists() || has_json_file(&store_root.join(OUTPUTS_DIR)) { + return true; + } + } + false +} + +fn has_json_file(dir: &Path) -> bool { + let Ok(entries) = std::fs::read_dir(dir) else { + return false; + }; + entries + .flatten() + .any(|entry| entry.path().extension().and_then(|ext| ext.to_str()) == Some("json")) +} + fn repo_key(scope: &Path) -> String { let normalized_scope = scope.canonicalize().unwrap_or_else(|_| scope.to_path_buf()); let key = crc32fast::hash(normalized_scope.to_string_lossy().as_bytes()); diff --git a/codex-rs/auto-review/src/lib_tests.rs b/codex-rs/auto-review/src/lib_tests.rs index 68a5ddc92a9e..e22ff281eebe 100644 --- a/codex-rs/auto-review/src/lib_tests.rs +++ b/codex-rs/auto-review/src/lib_tests.rs @@ -119,6 +119,167 @@ fn list_runs_returns_empty_when_store_is_missing() -> anyhow::Result<()> { Ok(()) } +#[test] +fn scoped_store_reads_legacy_unscoped_runs() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let legacy_run = sample_run("legacy_run", vec![sample_finding("f1", "Legacy")]); + let legacy_dir = codex_home.path().join("auto-review").join("runs"); + std::fs::create_dir_all(&legacy_dir)?; + std::fs::write( + legacy_dir.join("legacy_run.json"), + format!("{}\n", serde_json::to_string_pretty(&legacy_run)?), + )?; + + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let runs = store.list_runs()?; + let loaded = store.load_run("legacy_run")?; + let detail = store.finding_detail("legacy_run", "f1", 1024)?; + + assert_eq!(runs, vec![legacy_run.clone()]); + assert_eq!(loaded, legacy_run); + assert!(detail.content.contains("Legacy")); + Ok(()) +} + +#[test] +fn scoped_store_prefers_new_runs_over_legacy_runs() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let legacy_run = sample_run("same_run", vec![sample_finding("f1", "Legacy")]); + let new_run = sample_run("same_run", vec![sample_finding("f1", "New")]); + let legacy_dir = codex_home.path().join("auto-review").join("runs"); + std::fs::create_dir_all(&legacy_dir)?; + std::fs::write( + legacy_dir.join("same_run.json"), + format!("{}\n", serde_json::to_string_pretty(&legacy_run)?), + )?; + + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&new_run)?; + + assert_eq!(store.load_run("same_run")?, new_run); + Ok(()) +} + +#[test] +fn detects_new_or_legacy_store_files() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + + assert!(!AutoReviewStore::has_store_files(codex_home.path())); + + std::fs::create_dir_all( + codex_home + .path() + .join("state") + .join("review") + .join("repo-empty") + .join("auto-review") + .join("outputs"), + )?; + assert!(!AutoReviewStore::has_store_files(codex_home.path())); + + std::fs::create_dir_all(codex_home.path().join("auto-review").join("runs"))?; + assert!(!AutoReviewStore::has_store_files(codex_home.path())); + std::fs::write( + codex_home + .path() + .join("auto-review") + .join("runs") + .join("bad_run.json"), + "not json\n", + )?; + std::fs::write( + codex_home + .path() + .join("auto-review") + .join("runs") + .join("bad run.json"), + "not json\n", + )?; + assert!(!AutoReviewStore::has_store_files(codex_home.path())); + std::fs::write( + codex_home + .path() + .join("auto-review") + .join("runs") + .join("legacy_run.json"), + format!( + "{}\n", + serde_json::to_string_pretty(&sample_run("legacy_run", Vec::new()))? + ), + )?; + assert!(AutoReviewStore::has_store_files(codex_home.path())); + + let codex_home = tempfile::tempdir()?; + AutoReviewStore::for_scope(codex_home.path(), scope.path()) + .save_run(&sample_run("run_1", Vec::new()))?; + + assert!(AutoReviewStore::has_store_files(codex_home.path())); + Ok(()) +} + +#[test] +fn detail_lookup_uses_new_index_before_legacy_run() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let legacy_run = sample_run("same_run", vec![sample_finding("f1", "Legacy")]); + let new_run = sample_run("same_run", vec![sample_finding("f1", "New")]); + let legacy_dir = codex_home.path().join("auto-review").join("runs"); + std::fs::create_dir_all(&legacy_dir)?; + std::fs::write( + legacy_dir.join("same_run.json"), + format!("{}\n", serde_json::to_string_pretty(&legacy_run)?), + )?; + + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let index = serde_json::json!({ + "schema_version": SCHEMA_VERSION, + "runs": [new_run], + }); + 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 detail = store.finding_detail("same_run", "f1", 1024)?; + + assert!(detail.content.contains("New")); + assert!(!detail.content.contains("Legacy")); + Ok(()) +} + +#[test] +fn corrupt_legacy_run_does_not_hide_new_store() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let new_run = sample_run("new_run", vec![sample_finding("f1", "New")]); + let legacy_dir = codex_home.path().join("auto-review").join("runs"); + std::fs::create_dir_all(&legacy_dir)?; + std::fs::write(legacy_dir.join("bad_run.json"), "not json\n")?; + std::fs::write(legacy_dir.join("bad run.json"), "not json\n")?; + + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + store.save_run(&new_run)?; + + assert_eq!( + store + .list_runs()? + .into_iter() + .map(|run| run.run_id) + .collect::>(), + vec!["new_run".to_string()] + ); + assert_eq!(store.load_run("new_run")?.run_id, "new_run"); + assert!( + store + .finding_detail("new_run", "f1", 1024)? + .content + .contains("New") + ); + Ok(()) +} + #[test] fn load_run_defaults_missing_worktree_diff_fingerprint() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; diff --git a/codex-rs/core/src/context/auto_review_awareness.rs b/codex-rs/core/src/context/auto_review_awareness.rs index 2e07b2a6da50..ce935e77a0cb 100644 --- a/codex-rs/core/src/context/auto_review_awareness.rs +++ b/codex-rs/core/src/context/auto_review_awareness.rs @@ -76,6 +76,13 @@ async fn build_auto_review_awareness_inner( cwd: &Path, active_snapshot: BackgroundAutoReviewActiveSnapshot, ) -> Result> { + if active_snapshot.pending_run_id.is_none() + && active_snapshot.running_run_id.is_none() + && !AutoReviewStore::has_store_files(codex_home) + { + return Ok(None); + } + let active_review_target = ReviewTarget::UncommittedChanges; let active_target = collect_auto_review_target(cwd, &active_review_target).await; let store_scope = active_target.worktree_path.as_deref().unwrap_or(cwd);