Fix auto-review store migration awareness

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<PathBuf>,
 }
 
 impl AutoReviewStore {
     pub fn for_scope(codex_home: impl AsRef<Path>, scope: impl AsRef<Path>) -> 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<PathBuf>) -> Self {
-        Self { root: root.into() }
+        Self {
+            root: root.into(),
+            legacy_root: None,
+        }
+    }
+
+    pub fn has_store_files(codex_home: impl AsRef<Path>) -> 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<PathBuf> {
@@ -83,8 +95,7 @@ impl AutoReviewStore {
         finding_id: &str,
         max_bytes: usize,
     ) -> Result<AutoReviewDetail> {
-        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<PathBuf> {
@@ -98,16 +109,21 @@ impl AutoReviewStore {
 
     fn load_index(&self) -> Result<AutoReviewRunsIndex> {
         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<Option<PathBuf>> {
+        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<AutoReviewRun> {
+        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<Vec<AutoReviewRun>> {
+        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());

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

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<Option<AutoReviewAwareness>> {
+    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);