Skip to content

Align auto-review store layout #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shiny-code-bot merged 1 commit into from
Jun 18, 2026
Merged

Align auto-review store layout #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion codex-rs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
30 changes: 20 additions & 10 deletions codex-rs/app-server/src/request_processors/turn_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")))?;

Expand Down Expand Up @@ -1435,25 +1439,31 @@ 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()
.all(|finding| finding.finding_id != finding_id)
{
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,
Expand Down
107 changes: 55 additions & 52 deletions codex-rs/app-server/tests/suite/v2/review.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Result<Vec<_>, 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);
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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(&current_run)?;
AutoReviewStore::new(codex_home.path()).save_run(&stale_run)?;
let store = AutoReviewStore::for_scope(codex_home.path(), &thread_cwd);
store.save_run(&current_run)?;
store.save_run(&stale_run)?;

let request_id = mcp
.send_auto_review_summary_read_request(AutoReviewSummaryReadParams { thread_id })
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -974,6 +963,20 @@ fn sample_auto_review_run(
}
}

fn load_auto_review_runs(codex_home: &std::path::Path) -> Result<Vec<AutoReviewRun>> {
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 {
Expand Down
1 change: 1 addition & 0 deletions codex-rs/auto-review/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down
Loading
Loading