diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 95acfb73a71..9c9b831eb74 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2179,6 +2179,7 @@ dependencies = [ "anyhow", "codex-protocol", "codex-utils-path", + "crc32fast", "pretty_assertions", "serde", "serde_json", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 0e4f7cdd29d..e97a654bef1 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -281,6 +281,7 @@ clap = "4" clap_complete = "4" color-eyre = "0.6.3" constant_time_eq = "0.3.1" +crc32fast = "1.5.0" crossbeam-channel = "0.5.15" crypto_box = { version = "0.9.1", features = ["seal"] } crossterm = "0.28.1" @@ -368,7 +369,6 @@ rustls-pki-types = "1.14.0" schemars = "0.8.22" seccompiler = "0.5.0" semver = "1.0" -sentry = "0.46.0" serde = "1" serde_ignored = "0.1.14" serde_json = "1" 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 c3ded6d74fb..f6b69c54d66 100644 --- a/codex-rs/app-server/src/request_processors/turn_processor.rs +++ b/codex-rs/app-server/src/request_processors/turn_processor.rs @@ -1355,7 +1355,11 @@ impl TurnRequestProcessor { let (_, thread) = self.load_thread(&thread_id).await?; let active_review_target = CoreReviewTarget::UncommittedChanges; let active_target = Self::auto_review_target_for_thread(thread.as_ref()).await; - let runs = AutoReviewStore::new(&self.config.codex_home) + let store_scope = active_target + .worktree_path + .as_deref() + .unwrap_or(self.config.cwd.as_path()); + let runs = AutoReviewStore::for_scope(&self.config.codex_home, store_scope) .list_runs() .map_err(|err| internal_error(format!("failed to list auto review runs: {err}")))?; @@ -1435,12 +1439,16 @@ impl TurnRequestProcessor { } let (_, thread) = self.load_thread(&thread_id).await?; - let store = AutoReviewStore::new(&self.config.codex_home); + let active_review_target = CoreReviewTarget::UncommittedChanges; + let active_target = Self::auto_review_target_for_thread(thread.as_ref()).await; + let store_scope = active_target + .worktree_path + .as_deref() + .unwrap_or(self.config.cwd.as_path()); + let store = AutoReviewStore::for_scope(&self.config.codex_home, store_scope); let run = store .load_run(&run_id) .map_err(|_| invalid_request("auto review run not found"))?; - let active_review_target = CoreReviewTarget::UncommittedChanges; - let active_target = Self::auto_review_target_for_thread(thread.as_ref()).await; if run .visible_findings(&active_target, &active_review_target) .into_iter() @@ -1448,12 +1456,14 @@ impl TurnRequestProcessor { { return Err(invalid_request("auto review finding not found")); } - let detail = run.finding_detail(&finding_id, max_bytes).map_err(|err| { - invalid_request(format!( - "failed to read auto review finding detail: {}", - Self::auto_review_detail_error_message(&err.to_string()) - )) - })?; + let detail = store + .finding_detail(&run_id, &finding_id, max_bytes) + .map_err(|err| { + invalid_request(format!( + "failed to read auto review finding detail: {}", + Self::auto_review_detail_error_message(&err.to_string()) + )) + })?; Ok(AutoReviewFindingDetailReadResponse { run_id, diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index d280b0c56cf..7e24c795244 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -398,19 +398,9 @@ async fn review_start_with_detached_delivery_returns_new_thread_id() -> Result<( ) .await??; - let runs_dir = codex_home.path().join("auto-review/runs"); - let mut runs = std::fs::read_dir(&runs_dir)? - .map(|entry| entry.map(|entry| entry.path())) - .collect::, std::io::Error>>()?; + let runs = load_auto_review_runs(codex_home.path())?; assert_eq!(runs.len(), 1, "expected detached review to persist one run"); - let run_id = runs - .pop() - .and_then(|path| { - path.file_stem() - .map(|stem| stem.to_string_lossy().to_string()) - }) - .expect("run file stem"); - let run = AutoReviewStore::new(codex_home.path()).load_run(&run_id)?; + let run = runs.into_iter().next().expect("one run"); assert_eq!(run.status, AutoReviewRunStatus::Completed); assert_eq!(run.source, AutoReviewRunSource::Manual); assert_eq!(run.run_id, turn.id); @@ -592,12 +582,9 @@ async fn auto_review_summary_read_returns_current_summary_and_counts() -> Result timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let thread_id = start_default_thread(&mut mcp).await?; let thread_cwd = std::fs::canonicalize(codex_home.path())?; - AutoReviewStore::new(codex_home.path()).save_run(&sample_auto_review_run( - "run_summary", - "finding_summary", - &thread_cwd, - "Stored body", - ))?; + AutoReviewStore::for_scope(codex_home.path(), &thread_cwd).save_run( + &sample_auto_review_run("run_summary", "finding_summary", &thread_cwd, "Stored body"), + )?; let request_id = mcp .send_auto_review_summary_read_request(AutoReviewSummaryReadParams { thread_id }) @@ -644,16 +631,14 @@ async fn auto_review_summary_read_suppresses_stale_findings_by_default() -> Resu "Current body", ); current_run.completed_at_unix_secs = Some(2); - let mut stale_run = sample_auto_review_run( - "run_stale", - "finding_stale", - &thread_cwd.join("other-worktree"), - "Stale body", - ); + let mut stale_run = + sample_auto_review_run("run_stale", "finding_stale", &thread_cwd, "Stale body"); + stale_run.target.head_sha = Some("old-head".to_string()); stale_run.started_at_unix_secs = 10; stale_run.completed_at_unix_secs = Some(11); - AutoReviewStore::new(codex_home.path()).save_run(¤t_run)?; - AutoReviewStore::new(codex_home.path()).save_run(&stale_run)?; + let store = AutoReviewStore::for_scope(codex_home.path(), &thread_cwd); + store.save_run(¤t_run)?; + store.save_run(&stale_run)?; let request_id = mcp .send_auto_review_summary_read_request(AutoReviewSummaryReadParams { thread_id }) @@ -671,14 +656,14 @@ async fn auto_review_summary_read_suppresses_stale_findings_by_default() -> Resu let latest = summary.latest.expect("latest run summary"); assert_eq!(latest.run_id, "run_stale"); - assert_eq!(latest.freshness, ApiAutoReviewFreshness::Detached); + assert_eq!(latest.freshness, ApiAutoReviewFreshness::Stale); assert_eq!(latest.rendered_findings, 0); assert!(!latest.content.contains("finding_stale")); assert!(summary.status_counts.iter().any(|count| { count.freshness == ApiAutoReviewFreshness::Current && count.target_matches })); assert!(summary.status_counts.iter().any(|count| { - count.freshness == ApiAutoReviewFreshness::Detached && !count.target_matches + count.freshness == ApiAutoReviewFreshness::Stale && !count.target_matches })); assert_eq!( summary @@ -702,12 +687,15 @@ async fn auto_review_finding_detail_read_returns_bounded_detail() -> Result<()> timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let thread_id = start_default_thread(&mut mcp).await?; let thread_cwd = std::fs::canonicalize(codex_home.path())?; - AutoReviewStore::new(codex_home.path()).save_run(&sample_auto_review_run( - "run_detail", - "finding_detail", - &thread_cwd, - &"Use the existing bounded detail store instead of embedding the whole finding. ".repeat(8), - ))?; + AutoReviewStore::for_scope(codex_home.path(), &thread_cwd).save_run( + &sample_auto_review_run( + "run_detail", + "finding_detail", + &thread_cwd, + &"Use the existing bounded detail store instead of embedding the whole finding. " + .repeat(8), + ), + )?; let request_id = mcp .send_auto_review_finding_detail_read_request(AutoReviewFindingDetailReadParams { @@ -767,12 +755,14 @@ async fn auto_review_finding_detail_read_uses_selected_environment_cwd() -> Resu ) .await??; - AutoReviewStore::new(codex_home.path()).save_run(&sample_auto_review_run( - "run_environment_detail", - "environment_finding", - environment_cwd.path(), - "Stored environment body", - ))?; + AutoReviewStore::for_scope(codex_home.path(), environment_cwd.path()).save_run( + &sample_auto_review_run( + "run_environment_detail", + "environment_finding", + environment_cwd.path(), + "Stored environment body", + ), + )?; let request_id = mcp .send_auto_review_finding_detail_read_request(AutoReviewFindingDetailReadParams { @@ -838,12 +828,9 @@ async fn auto_review_finding_detail_read_rejects_unknown_finding_id() -> Result< timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let thread_id = start_default_thread(&mut mcp).await?; let thread_cwd = std::fs::canonicalize(codex_home.path())?; - AutoReviewStore::new(codex_home.path()).save_run(&sample_auto_review_run( - "run_detail", - "finding_detail", - &thread_cwd, - "Stored body", - ))?; + AutoReviewStore::for_scope(codex_home.path(), &thread_cwd).save_run( + &sample_auto_review_run("run_detail", "finding_detail", &thread_cwd, "Stored body"), + )?; let request_id = mcp .send_auto_review_finding_detail_read_request(AutoReviewFindingDetailReadParams { @@ -881,12 +868,14 @@ async fn auto_review_finding_detail_read_rejects_stale_run() -> Result<()> { timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let thread_id = start_default_thread(&mut mcp).await?; let thread_cwd = std::fs::canonicalize(codex_home.path())?; - AutoReviewStore::new(codex_home.path()).save_run(&sample_auto_review_run( - "run_detail", - "finding_detail", - &thread_cwd.join("other-worktree"), - "Stored body", - ))?; + AutoReviewStore::for_scope(codex_home.path(), &thread_cwd).save_run( + &sample_auto_review_run( + "run_detail", + "finding_detail", + &thread_cwd.join("other-worktree"), + "Stored body", + ), + )?; let request_id = mcp .send_auto_review_finding_detail_read_request(AutoReviewFindingDetailReadParams { @@ -974,6 +963,20 @@ fn sample_auto_review_run( } } +fn load_auto_review_runs(codex_home: &std::path::Path) -> Result> { + let review_dir = codex_home.join("state/review"); + if !review_dir.exists() { + return Ok(Vec::new()); + } + let mut runs = Vec::new(); + for entry in std::fs::read_dir(&review_dir)? { + let store_root = entry?.path().join("auto-review"); + runs.extend(AutoReviewStore::from_store_root(store_root).list_runs()?); + } + runs.sort_by(|left, right| left.run_id.cmp(&right.run_id)); + Ok(runs) +} + async fn materialize_thread_rollout(mcp: &mut TestAppServer, thread_id: &str) -> Result<()> { let turn_req = mcp .send_turn_start_request(TurnStartParams { diff --git a/codex-rs/auto-review/Cargo.toml b/codex-rs/auto-review/Cargo.toml index 1fa1cb8f18f..7fd4aca82be 100644 --- a/codex-rs/auto-review/Cargo.toml +++ b/codex-rs/auto-review/Cargo.toml @@ -15,6 +15,7 @@ workspace = true anyhow = { workspace = true } codex-protocol = { workspace = true } codex-utils-path = { workspace = true } +crc32fast = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/auto-review/src/lib.rs b/codex-rs/auto-review/src/lib.rs index 19b5d4f3b50..e371a1eea49 100644 --- a/codex-rs/auto-review/src/lib.rs +++ b/codex-rs/auto-review/src/lib.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::collections::BTreeSet; use std::path::Path; use std::path::PathBuf; @@ -17,7 +18,10 @@ pub const DETAIL_MAX_BYTES: usize = 16384; pub const SCHEMA_VERSION: u32 = 1; const STORE_DIR: &str = "auto-review"; -const RUNS_DIR: &str = "runs"; +const STATE_DIR: &str = "state"; +const REVIEW_DIR: &str = "review"; +const RUNS_FILENAME: &str = "runs.json"; +const OUTPUTS_DIR: &str = "outputs"; const OMITTED_TEMPLATE_PREFIX: &str = "... "; const OMITTED_TEMPLATE_SUFFIX: &str = " more finding(s) omitted"; @@ -27,45 +31,124 @@ pub struct AutoReviewStore { } impl AutoReviewStore { - pub fn new(codex_home: impl AsRef) -> Self { + pub fn for_scope(codex_home: impl AsRef, scope: impl AsRef) -> Self { Self { - root: codex_home.as_ref().join(STORE_DIR), + root: scoped_store_root(codex_home.as_ref(), scope.as_ref()), } } + pub fn from_store_root(root: impl Into) -> Self { + Self { root: root.into() } + } + pub fn save_run(&self, run: &AutoReviewRun) -> Result { validate_run(run)?; - let path = self.run_path(&run.run_id)?; + self.save_output(run)?; + let mut index = self.load_index()?; + index.upsert(run.clone()); + let runs_path = self.runs_path(); + let json = serde_json::to_string_pretty(&index)?; + write_atomically(&runs_path, &format!("{json}\n")).with_context(|| { + format!( + "failed to write auto review runs index {}", + runs_path.display() + ) + })?; + Ok(runs_path) + } + + pub fn load_run(&self, run_id: &str) -> Result { + validate_safe_id(run_id).context("auto review run_id")?; + let Some(run) = self + .load_index()? + .runs + .into_iter() + .find(|run| run.run_id == run_id) + else { + anyhow::bail!("unknown auto review run id: {run_id}"); + }; + validate_run(&run)?; + Ok(run) + } + + pub fn list_runs(&self) -> Result> { + let mut runs = self.load_index()?.runs; + runs.sort_by(|left, right| left.run_id.cmp(&right.run_id)); + Ok(runs) + } + + pub fn finding_detail( + &self, + run_id: &str, + finding_id: &str, + max_bytes: usize, + ) -> Result { + self.load_output(run_id)? + .finding_detail(finding_id, max_bytes) + } + + pub fn output_path(&self, run_id: &str) -> Result { + validate_safe_id(run_id).context("auto review run_id")?; + Ok(self.root.join(OUTPUTS_DIR).join(format!("{run_id}.json"))) + } + + pub fn runs_path(&self) -> PathBuf { + self.root.join(RUNS_FILENAME) + } + + fn load_index(&self) -> Result { + let path = self.runs_path(); + let mut index = 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(|| { + format!("failed to parse auto review runs index {}", path.display()) + })? + } else { + AutoReviewRunsIndex::default() + }; + for run in self.load_output_runs()? { + index.upsert(run); + } + index.validate()?; + Ok(index) + } + + fn save_output(&self, run: &AutoReviewRun) -> Result<()> { + let path = self.output_path(&run.run_id)?; let json = serde_json::to_string_pretty(run)?; write_atomically(&path, &format!("{json}\n")) - .with_context(|| format!("failed to write auto review run {}", run.run_id))?; - Ok(path) + .with_context(|| format!("failed to write auto review output {}", path.display())) } - pub fn load_run(&self, run_id: &str) -> Result { - let path = self.run_path(run_id)?; + fn load_output(&self, run_id: &str) -> Result { + let path = self.output_path(run_id)?; let json = std::fs::read_to_string(&path) - .with_context(|| format!("failed to read auto review run {run_id}"))?; + .with_context(|| format!("failed to read auto review output {run_id}"))?; let run = serde_json::from_str(&json) - .with_context(|| format!("failed to parse auto review run {run_id}"))?; + .with_context(|| format!("failed to parse auto review output {run_id}"))?; validate_run(&run)?; Ok(run) } - pub fn list_runs(&self) -> Result> { - let runs_dir = self.root.join(RUNS_DIR); - if !runs_dir.exists() { + fn load_output_runs(&self) -> Result> { + let outputs_dir = self.root.join(OUTPUTS_DIR); + if !outputs_dir.exists() { return Ok(Vec::new()); } let mut run_ids = Vec::new(); - for entry in std::fs::read_dir(&runs_dir).with_context(|| { - format!("failed to read auto review runs dir {}", runs_dir.display()) + 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 runs dir entry {}", - runs_dir.display() + "failed to read auto review outputs dir entry {}", + outputs_dir.display() ) })?; let path = entry.path(); @@ -82,13 +165,52 @@ impl AutoReviewStore { run_ids.sort(); run_ids .into_iter() - .map(|run_id| self.load_run(&run_id)) + .map(|run_id| self.load_output(&run_id)) .collect() } +} - fn run_path(&self, run_id: &str) -> Result { - validate_safe_id(run_id).context("auto review run_id")?; - Ok(self.root.join(RUNS_DIR).join(format!("{run_id}.json"))) +#[derive(Debug, Clone, Serialize, Deserialize)] +struct AutoReviewRunsIndex { + schema_version: u32, + runs: Vec, +} + +impl Default for AutoReviewRunsIndex { + fn default() -> Self { + Self { + schema_version: SCHEMA_VERSION, + runs: Vec::new(), + } + } +} + +impl AutoReviewRunsIndex { + fn validate(&self) -> Result<()> { + if self.schema_version != SCHEMA_VERSION { + anyhow::bail!( + "unsupported auto review runs index schema version: {}", + self.schema_version + ); + } + let mut run_ids = BTreeSet::new(); + for run in &self.runs { + validate_run(run)?; + if !run_ids.insert(&run.run_id) { + anyhow::bail!("duplicate auto review run id: {}", run.run_id); + } + } + Ok(()) + } + + fn upsert(&mut self, run: AutoReviewRun) { + let mut by_id = self + .runs + .drain(..) + .map(|run| (run.run_id.clone(), run)) + .collect::>(); + by_id.insert(run.run_id.clone(), run); + self.runs = by_id.into_values().collect(); } } @@ -357,6 +479,20 @@ fn validate_safe_id(value: &str) -> Result<()> { anyhow::bail!("contains unsafe path characters: {value:?}") } +fn scoped_store_root(codex_home: &Path, scope: &Path) -> PathBuf { + codex_home + .join(STATE_DIR) + .join(REVIEW_DIR) + .join(repo_key(scope)) + .join(STORE_DIR) +} + +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()); + format!("repo-{key:08x}") +} + fn validate_run(run: &AutoReviewRun) -> Result<()> { if run.schema_version != SCHEMA_VERSION { anyhow::bail!( diff --git a/codex-rs/auto-review/src/lib_tests.rs b/codex-rs/auto-review/src/lib_tests.rs index c4002d90dae..68a5ddc92a9 100644 --- a/codex-rs/auto-review/src/lib_tests.rs +++ b/codex-rs/auto-review/src/lib_tests.rs @@ -20,34 +20,85 @@ use super::SCHEMA_VERSION; #[test] fn save_and_load_run_round_trips_under_codex_home() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; - let store = AutoReviewStore::new(codex_home.path()); + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); let run = sample_run("run_1", vec![sample_finding("f1", "Title")]); let path = store.save_run(&run)?; let loaded = store.load_run("run_1")?; assert_eq!(loaded, run); + assert_eq!(path, store.runs_path()); + assert!(path.ends_with("auto-review/runs.json")); + assert!(store.output_path("run_1")?.exists()); + Ok(()) +} + +#[test] +fn scoped_stores_separate_repos_under_one_home() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope_a = tempfile::tempdir()?; + let scope_b = tempfile::tempdir()?; + let store_a = AutoReviewStore::for_scope(codex_home.path(), scope_a.path()); + let store_b = AutoReviewStore::for_scope(codex_home.path(), scope_b.path()); + + store_a.save_run(&sample_run("run_a", Vec::new()))?; + store_b.save_run(&sample_run("run_b", Vec::new()))?; + + assert_eq!( + store_a + .list_runs()? + .into_iter() + .map(|run| run.run_id) + .collect::>(), + vec!["run_a".to_string()] + ); assert_eq!( - path, - codex_home - .path() - .join("auto-review") - .join("runs") - .join("run_1.json") + store_b + .list_runs()? + .into_iter() + .map(|run| run.run_id) + .collect::>(), + vec!["run_b".to_string()] ); + assert_ne!(store_a.runs_path(), store_b.runs_path()); Ok(()) } #[test] fn list_runs_returns_valid_json_runs_in_id_order() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; - let store = AutoReviewStore::new(codex_home.path()); + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); let run_b = sample_run("run_b", Vec::new()); let run_a = sample_run("run_a", Vec::new()); store.save_run(&run_b)?; store.save_run(&run_a)?; - let runs_dir = codex_home.path().join("auto-review").join("runs"); - std::fs::write(runs_dir.join("ignored.txt"), "ignored")?; + + let runs = store.list_runs()?; + + assert_eq!( + runs.into_iter().map(|run| run.run_id).collect::>(), + vec!["run_a".to_string(), "run_b".to_string()] + ); + Ok(()) +} + +#[test] +fn list_runs_recovers_runs_missing_from_index_but_present_in_outputs() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let run_a = sample_run("run_a", Vec::new()); + let run_b = sample_run("run_b", Vec::new()); + store.save_run(&run_a)?; + store.save_run(&run_b)?; + + let index = serde_json::json!({ + "schema_version": SCHEMA_VERSION, + "runs": [run_b], + }); + std::fs::write(store.runs_path(), format!("{index}\n"))?; let runs = store.list_runs()?; @@ -61,7 +112,8 @@ fn list_runs_returns_valid_json_runs_in_id_order() -> anyhow::Result<()> { #[test] fn list_runs_returns_empty_when_store_is_missing() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; - let runs = AutoReviewStore::new(codex_home.path()).list_runs()?; + let scope = tempfile::tempdir()?; + let runs = AutoReviewStore::for_scope(codex_home.path(), scope.path()).list_runs()?; assert!(runs.is_empty()); Ok(()) @@ -70,31 +122,37 @@ fn list_runs_returns_empty_when_store_is_missing() -> anyhow::Result<()> { #[test] fn load_run_defaults_missing_worktree_diff_fingerprint() -> anyhow::Result<()> { let codex_home = tempfile::tempdir()?; - let store = AutoReviewStore::new(codex_home.path()); - let runs_dir = codex_home.path().join("auto-review").join("runs"); - let path = runs_dir.join("run_1.json"); - std::fs::create_dir_all(&runs_dir)?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let path = store.runs_path(); + let parent = path.parent().expect("runs path should have parent"); + std::fs::create_dir_all(parent)?; std::fs::write( &path, r#"{ "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": [] + "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": [] + } + ] }"#, )?; @@ -107,7 +165,8 @@ fn load_run_defaults_missing_worktree_diff_fingerprint() -> anyhow::Result<()> { #[test] fn unsafe_run_ids_are_rejected() { let codex_home = tempfile::tempdir().expect("tempdir"); - let store = AutoReviewStore::new(codex_home.path()); + let scope = tempfile::tempdir().expect("tempdir"); + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); let run = sample_run("../run", Vec::new()); let error = store.save_run(&run).expect_err("unsafe id should fail"); @@ -118,7 +177,8 @@ fn unsafe_run_ids_are_rejected() { #[test] fn unsafe_and_duplicate_finding_ids_are_rejected() { let codex_home = tempfile::tempdir().expect("tempdir"); - let store = AutoReviewStore::new(codex_home.path()); + let scope = tempfile::tempdir().expect("tempdir"); + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); let unsafe_id = sample_run("run_1", vec![sample_finding("../f1", "Title")]); let duplicate_id = sample_run( "run_1", @@ -143,7 +203,8 @@ fn unsafe_and_duplicate_finding_ids_are_rejected() { #[test] fn unsupported_schema_versions_are_rejected() { let codex_home = tempfile::tempdir().expect("tempdir"); - let store = AutoReviewStore::new(codex_home.path()); + let scope = tempfile::tempdir().expect("tempdir"); + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); let run = AutoReviewRun { schema_version: SCHEMA_VERSION + 1, ..sample_run("run_1", Vec::new()) @@ -421,6 +482,22 @@ fn detail_lookup_returns_bounded_json() -> anyhow::Result<()> { Ok(()) } +#[test] +fn store_detail_lookup_reads_completed_output_sidecar() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let scope = tempfile::tempdir()?; + let store = AutoReviewStore::for_scope(codex_home.path(), scope.path()); + let run = sample_run("run_1", vec![sample_finding("f1", &"x".repeat(200))]); + store.save_run(&run)?; + + let detail = store.finding_detail("run_1", "f1", 120)?; + + assert_eq!(detail.finding_id, "f1"); + assert!(detail.truncated); + assert!(detail.bytes <= 120); + Ok(()) +} + #[test] fn detail_lookup_clamps_to_crate_owned_hard_cap() -> anyhow::Result<()> { let run = sample_run( diff --git a/codex-rs/core/src/context/auto_review_awareness.rs b/codex-rs/core/src/context/auto_review_awareness.rs index db8d33f78e4..2e07b2a6da5 100644 --- a/codex-rs/core/src/context/auto_review_awareness.rs +++ b/codex-rs/core/src/context/auto_review_awareness.rs @@ -76,7 +76,10 @@ async fn build_auto_review_awareness_inner( cwd: &Path, active_snapshot: BackgroundAutoReviewActiveSnapshot, ) -> Result> { - let runs = AutoReviewStore::new(codex_home).list_runs()?; + 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); + let runs = AutoReviewStore::for_scope(codex_home, store_scope).list_runs()?; if runs.is_empty() && active_snapshot.pending_run_id.is_none() && active_snapshot.running_run_id.is_none() @@ -84,8 +87,6 @@ async fn build_auto_review_awareness_inner( return Ok(None); } - let active_review_target = ReviewTarget::UncommittedChanges; - let active_target = collect_auto_review_target(cwd, &active_review_target).await; Ok(render_awareness( &runs, &active_target, 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 fe3cb76c906..c211ada8b7c 100644 --- a/codex-rs/core/src/context/auto_review_awareness_tests.rs +++ b/codex-rs/core/src/context/auto_review_awareness_tests.rs @@ -5,6 +5,7 @@ use codex_auto_review::AutoReviewRun; use codex_auto_review::AutoReviewRunSource; use codex_auto_review::AutoReviewRunStatus; use codex_auto_review::AutoReviewRunTarget; +use codex_auto_review::AutoReviewStore; use codex_protocol::protocol::ReviewCodeLocation; use codex_protocol::protocol::ReviewFinding; use codex_protocol::protocol::ReviewLineRange; @@ -187,9 +188,10 @@ async fn awareness_is_absent_when_store_and_snapshot_are_empty() -> anyhow::Resu async fn awareness_is_absent_when_store_is_unreadable() -> anyhow::Result<()> { let codex_home = TempDir::new()?; let repo = TempDir::new()?; - let runs_dir = codex_home.path().join("auto-review").join("runs"); - std::fs::create_dir_all(&runs_dir)?; - std::fs::write(runs_dir.join("broken.json"), "not json")?; + let store = AutoReviewStore::for_scope(codex_home.path(), repo.path()); + let runs_path = store.runs_path(); + std::fs::create_dir_all(runs_path.parent().expect("runs path parent"))?; + std::fs::write(runs_path, "not json")?; let awareness = build_auto_review_awareness( codex_home.path(), diff --git a/codex-rs/core/src/review_persistence.rs b/codex-rs/core/src/review_persistence.rs index 4e470402dbe..91d20919e76 100644 --- a/codex-rs/core/src/review_persistence.rs +++ b/codex-rs/core/src/review_persistence.rs @@ -28,6 +28,7 @@ pub(crate) const AUTO_REVIEW_INTERRUPTED_ERROR_SUMMARY: &str = pub(crate) struct ReviewPersistenceContext { run_id: String, source: AutoReviewRunSource, + store_scope: PathBuf, target: AutoReviewRunTarget, review_target: ReviewTarget, started_at_unix_secs: i64, @@ -43,12 +44,17 @@ impl ReviewPersistenceContext { model: Option, ) -> Self { let target = collect_auto_review_target(cwd, &review_target).await; + let store_scope = target + .worktree_path + .clone() + .unwrap_or_else(|| cwd.to_path_buf()); Self { run_id, source: match mode { ReviewPersistence::ManualAutoReview => AutoReviewRunSource::Manual, ReviewPersistence::BackgroundAutoReview => AutoReviewRunSource::Background, }, + store_scope, target, review_target, started_at_unix_secs: now_unix_secs(), @@ -154,7 +160,7 @@ impl ReviewPersistenceContext { } else { None }; - let store = AutoReviewStore::new(codex_home); + let store = AutoReviewStore::for_scope(codex_home, &self.store_scope); let _guard = AUTO_REVIEW_RUN_WRITE_LOCK.lock().unwrap_or_else(|err| { tracing::warn!("auto review run write lock was poisoned; continuing"); err.into_inner() @@ -283,7 +289,7 @@ mod tests { persistence.save_cancelled(codex_home.path()); persistence.save_running(codex_home.path()); - let store = AutoReviewStore::new(codex_home.path()); + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); let run = store .load_run("late-running") .expect("load persisted review run"); @@ -308,7 +314,7 @@ mod tests { "background auto review was cancelled by request".to_string(), ); - let store = AutoReviewStore::new(codex_home.path()); + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); let run = store .load_run("custom-cancelled") .expect("load persisted review run"); @@ -335,7 +341,7 @@ mod tests { persistence.save_pending(codex_home.path()); persistence.save_running(codex_home.path()); - let store = AutoReviewStore::new(codex_home.path()); + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); let run = store .load_run("pending-running") .expect("load persisted review run"); @@ -359,7 +365,7 @@ mod tests { persistence.save_skipped(codex_home.path(), "duplicate fingerprint".to_string()); persistence.save_running(codex_home.path()); - let store = AutoReviewStore::new(codex_home.path()); + let store = AutoReviewStore::for_scope(codex_home.path(), cwd.path()); let run = store .load_run("skipped-running") .expect("load persisted review run"); diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 63825368d08..8c19cb0a40d 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -228,8 +228,8 @@ async fn review_op_emits_lifecycle_and_review_output() { "assistant review output contains user_action markup" ); assert!( - !codex_home.path().join("auto-review/runs").exists(), - "default review should not write an auto-review sidecar" + load_auto_review_runs(codex_home.path()).unwrap().is_empty(), + "default review should not write an auto-review run" ); let _codex_home_guard = codex_home; @@ -339,22 +339,8 @@ async fn review_op_with_persistence_writes_auto_review_run() { let _exited = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; - let runs_dir = codex_home.path().join("auto-review/runs"); - let mut runs = std::fs::read_dir(&runs_dir) - .expect("auto review runs dir") - .map(|entry| entry.expect("run dir entry").path()) - .collect::>(); - assert_eq!(runs.len(), 1, "expected exactly one auto review run"); - let run_id = runs - .pop() - .and_then(|path| { - path.file_stem() - .map(|stem| stem.to_string_lossy().to_string()) - }) - .expect("run file stem"); - let run = AutoReviewStore::new(codex_home.path()) - .load_run(&run_id) - .expect("load auto review run"); + let run = load_single_auto_review_run(codex_home.path()).expect("load auto review run"); + let run_id = run.run_id.clone(); assert_eq!(run.run_id, run_id); assert_eq!(run.status, AutoReviewRunStatus::Completed); @@ -404,22 +390,8 @@ async fn review_op_with_persistence_writes_failed_run_without_review_output() { } let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; - let runs_dir = codex_home.path().join("auto-review/runs"); - let mut runs = std::fs::read_dir(&runs_dir) - .expect("auto review runs dir") - .map(|entry| entry.expect("run dir entry").path()) - .collect::>(); - assert_eq!(runs.len(), 1, "expected exactly one auto review run"); - let run_id = runs - .pop() - .and_then(|path| { - path.file_stem() - .map(|stem| stem.to_string_lossy().to_string()) - }) - .expect("run file stem"); - let run = AutoReviewStore::new(codex_home.path()) - .load_run(&run_id) - .expect("load auto review run"); + let run = load_single_auto_review_run(codex_home.path()).expect("load auto review run"); + let run_id = run.run_id.clone(); assert_eq!(run.run_id, run_id); assert_eq!(run.status, AutoReviewRunStatus::Failed); @@ -482,22 +454,8 @@ async fn review_op_with_persistence_writes_cancelled_run_when_interrupted() { let _aborted = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnAborted(_))).await; let _ = gate_completed_tx.send(()); - let runs_dir = codex_home.path().join("auto-review/runs"); - let mut runs = std::fs::read_dir(&runs_dir) - .expect("auto review runs dir") - .map(|entry| entry.expect("run dir entry").path()) - .collect::>(); - assert_eq!(runs.len(), 1, "expected exactly one auto review run"); - let run_id = runs - .pop() - .and_then(|path| { - path.file_stem() - .map(|stem| stem.to_string_lossy().to_string()) - }) - .expect("run file stem"); - let run = AutoReviewStore::new(codex_home.path()) - .load_run(&run_id) - .expect("load auto review run"); + let run = load_single_auto_review_run(codex_home.path()).expect("load auto review run"); + let run_id = run.run_id.clone(); assert_eq!(run.run_id, run_id); assert_eq!(run.status, AutoReviewRunStatus::Cancelled); @@ -796,7 +754,7 @@ async fn auto_review_awareness_injected_for_current_findings() -> anyhow::Result }, }], }; - AutoReviewStore::new(&codex_home).save_run(&run)?; + AutoReviewStore::for_scope(&codex_home, harness.cwd()).save_run(&run)?; let request_log = mount_sse_sequence( harness.server(), vec![sse(vec![ @@ -2112,29 +2070,19 @@ fn load_single_auto_review_run( fn load_auto_review_runs( codex_home: &std::path::Path, ) -> anyhow::Result> { - let runs_dir = codex_home.join("auto-review/runs"); - if !runs_dir.exists() { + let review_dir = codex_home.join("state/review"); + if !review_dir.exists() { return Ok(Vec::new()); } - let mut runs = std::fs::read_dir(&runs_dir) - .map_err(|err| anyhow::anyhow!("auto review runs dir: {err}"))? - .filter_map(|entry| match entry { - Ok(entry) => { - let path = entry.path(); - (path.extension().and_then(|extension| extension.to_str()) == Some("json")) - .then_some(Ok(path)) - } - Err(err) => Some(Err(err)), - }) - .map(|path| { - let path = path?; - let run_id = path - .file_stem() - .map(|stem| stem.to_string_lossy().to_string()) - .ok_or_else(|| anyhow::anyhow!("run file stem"))?; - AutoReviewStore::new(codex_home).load_run(&run_id) - }) - .collect::>>()?; + let mut runs = Vec::new(); + for entry in std::fs::read_dir(&review_dir) + .map_err(|err| anyhow::anyhow!("auto review state dir: {err}"))? + { + let entry = entry?; + let store_root = entry.path().join("auto-review"); + let store = AutoReviewStore::from_store_root(store_root); + runs.extend(store.list_runs()?); + } runs.sort_by(|left, right| left.run_id.cmp(&right.run_id)); Ok(runs) }