From 605e713d09581cfe7cf3295238f5e182ced7bd68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E7=9D=BF?= Date: Wed, 15 Jul 2026 12:22:37 +0800 Subject: [PATCH 1/4] feat(axbuild): add agent review benchmark --- ...axtask-deferred-wake-local-reschedule.toml | 16 + .../1505-ipv6-pktinfo-family-validation.toml | 16 + ...8125-regression-covers-constants-only.toml | 16 + scripts/agent-review-bench/reviewer.md | 18 + .../schemas/grade.schema.json | 26 + .../schemas/review.schema.json | 27 + .../axbuild/src/agent_review_bench/cases.rs | 445 +++++++++++++++ .../axbuild/src/agent_review_bench/codex.rs | 235 ++++++++ scripts/axbuild/src/agent_review_bench/mod.rs | 532 ++++++++++++++++++ .../axbuild/src/agent_review_bench/sandbox.rs | 292 ++++++++++ .../axbuild/src/agent_review_bench/scoring.rs | 202 +++++++ scripts/axbuild/src/lib.rs | 7 + 12 files changed, 1832 insertions(+) create mode 100644 scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml create mode 100644 scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml create mode 100644 scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml create mode 100644 scripts/agent-review-bench/reviewer.md create mode 100644 scripts/agent-review-bench/schemas/grade.schema.json create mode 100644 scripts/agent-review-bench/schemas/review.schema.json create mode 100644 scripts/axbuild/src/agent_review_bench/cases.rs create mode 100644 scripts/axbuild/src/agent_review_bench/codex.rs create mode 100644 scripts/axbuild/src/agent_review_bench/mod.rs create mode 100644 scripts/axbuild/src/agent_review_bench/sandbox.rs create mode 100644 scripts/axbuild/src/agent_review_bench/scoring.rs diff --git a/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml b/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml new file mode 100644 index 0000000000..2f5027a8e6 --- /dev/null +++ b/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml @@ -0,0 +1,16 @@ +id = "1495-axtask-deferred-wake-local-reschedule" +pr = 1495 +title = "fix(axtask): defer cross-core wake instead of spinning on remote on_cpu" +remote = "https://github.com/rcore-os/tgoskits.git" +base = "baf4aa42e883b78156011636d189f348409ff4c1" +head = "c74237d2c10754b1341e03f495caafd261cf8b7e" +source = "https://github.com/rcore-os/tgoskits/pull/1495#discussion_r3518853256" +fixed_by = "bf3b71dd4622be7436431ed6138fade0b06955fa" + +[[expected]] +id = "axtask-owner-drain-needs-local-reschedule" +path = "os/arceos/modules/axtask/src/run_queue.rs" +line = 1125 +severity = "major" +description = "When the owner CPU drains a deferred wake, it always calls kick_remote_cpu(target). select_wake_run_queue can choose the same owner CPU as target, and that self-kick is a no-op. The runnable task can therefore miss the reschedule that the original remote waker supplied and remain queued while the owner enters idle or until a later tick." +match_if = "A finding must identify that the deferred-wake drain loses required scheduling notification when target is the current/owner CPU because kick_remote_cpu is ineffective for a self-target, and require an explicit local reschedule path or equivalent preserved handoff state." diff --git a/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml b/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml new file mode 100644 index 0000000000..b7e98ac7f5 --- /dev/null +++ b/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml @@ -0,0 +1,16 @@ +id = "1505-ipv6-pktinfo-family-validation" +pr = 1505 +title = "fix(starry): accept read-only mmap fdatasync and IP_PKTINFO/IPV6 pktinfo sockopts" +remote = "https://github.com/rcore-os/tgoskits.git" +base = "882e07fd2d1581a8a9882d397946db25a974bd15" +head = "dfde82ef6b58b6944613d1421bec297327e5f7da" +source = "https://github.com/rcore-os/tgoskits/pull/1505#discussion_r3526033263" +fixed_by = "0ec97c6b71dd63783db596f599b97b4142f31851" + +[[expected]] +id = "ipv6-pktinfo-bypasses-socket-family-check" +path = "os/StarryOS/kernel/src/syscall/net/opt.rs" +line = 521 +severity = "major" +description = "The new IPV6_RECVPKTINFO/IPV6_PKTINFO fast paths return success before ensure_ipv6_socket is called. An AF_INET socket can consequently use an IPPROTO_IPV6 option successfully, unlike Linux and the existing IPv6 option paths, which reject the family mismatch with ENOPROTOOPT. The getsockopt path has the same issue." +match_if = "A finding must flag that the newly accepted IPv6 pktinfo options bypass IPv6 socket-family validation for AF_INET sockets in set and/or get handling, and require ensure_ipv6_socket or equivalent validation before success." diff --git a/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml b/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml new file mode 100644 index 0000000000..72437fc3c7 --- /dev/null +++ b/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml @@ -0,0 +1,16 @@ +id = "1583-rtl8125-regression-covers-constants-only" +pr = 1583 +title = "fix(realtek-rtl8125): advertise gigabit autonegotiation" +remote = "https://github.com/rcore-os/tgoskits.git" +base = "ba27d7b061b47d22b16a53c847b1a4abc2bece71" +head = "214ccd50c9df852efaa74822341294defa92e001" +source = "https://github.com/rcore-os/tgoskits/pull/1583#discussion_r3567595139" +fixed_by = "7b37887e616abdb4b6e98c1e07f533607f51e2b7" + +[[expected]] +id = "rtl8125-regression-does-not-exercise-phy-sequence" +path = "drivers/net/realtek-rtl8125/src/hw.rs" +line = 550 +severity = "major" +description = "The regression test only checks relationships among the newly added constants. It never calls hw_phy_config or observes PHY reads and writes, so deleting the production MII_CTRL1000 and MII_BMCR configuration still leaves the test passing and restores the original autonegotiation bug." +match_if = "A finding must identify that the added test validates constants rather than the production PHY configuration behavior, so it cannot fail when the new page selection/advertisement/restart sequence is removed, and require a recording/mock PHY boundary or equivalent deterministic production-path regression." diff --git a/scripts/agent-review-bench/reviewer.md b/scripts/agent-review-bench/reviewer.md new file mode 100644 index 0000000000..0afb7216fc --- /dev/null +++ b/scripts/agent-review-bench/reviewer.md @@ -0,0 +1,18 @@ +# Offline Review Contract + +Review only the committed change between `bench-base` and `HEAD` in this repository. + +- This is an offline benchmark snapshot, not a live GitHub pull request. Do not use the network, + `gh`, GitHub APIs, external repositories, or paths outside this repository. Do not submit a + review, comment, issue, branch, commit, or reviewer request. +- Do not modify files. Use read-only source inspection and Git history/diff commands only; do not + build or run tests that write artifacts. +- Read and apply the repository `AGENTS.md` and every file under `book/guideline/` as the current + review standards. Do not invoke the side-effectful `review-single-pr` workflow. +- Review the change for actionable correctness, concurrency, security/soundness, hardware or ABI + semantics, maintainability, deterministic regression coverage, test discovery/wiring, and + documentation/user-facing compatibility. +- Report only issues introduced by this change. Each finding must explain the concrete failure, + its impact, and the expected fix direction, and must point to a changed line on the `HEAD` side. +- Return the JSON object required by the supplied output schema. Use an empty `findings` array when + no actionable issue exists. diff --git a/scripts/agent-review-bench/schemas/grade.schema.json b/scripts/agent-review-bench/schemas/grade.schema.json new file mode 100644 index 0000000000..883f165728 --- /dev/null +++ b/scripts/agent-review-bench/schemas/grade.schema.json @@ -0,0 +1,26 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "additionalProperties": false, + "required": ["matches"], + "properties": { + "matches": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["expected_id", "finding_index", "reason"], + "properties": { + "expected_id": { "type": "string" }, + "finding_index": { + "anyOf": [ + { "type": "integer", "minimum": 0 }, + { "type": "null" } + ] + }, + "reason": { "type": "string" } + } + } + } + } +} diff --git a/scripts/agent-review-bench/schemas/review.schema.json b/scripts/agent-review-bench/schemas/review.schema.json new file mode 100644 index 0000000000..b0c7259915 --- /dev/null +++ b/scripts/agent-review-bench/schemas/review.schema.json @@ -0,0 +1,27 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "additionalProperties": false, + "required": ["summary", "findings"], + "properties": { + "summary": { "type": "string" }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["title", "body", "path", "line", "severity"], + "properties": { + "title": { "type": "string" }, + "body": { "type": "string" }, + "path": { "type": "string" }, + "line": { "type": "integer", "minimum": 1 }, + "severity": { + "type": "string", + "enum": ["critical", "major", "minor", "nit"] + } + } + } + } + } +} diff --git a/scripts/axbuild/src/agent_review_bench/cases.rs b/scripts/axbuild/src/agent_review_bench/cases.rs new file mode 100644 index 0000000000..058dafba70 --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/cases.rs @@ -0,0 +1,445 @@ +use std::{ + collections::BTreeSet, + fs, + path::{Component, Path}, + process::Command, +}; + +use anyhow::{Context, bail}; +use serde::{Deserialize, Serialize}; + +const CASES_DIR: &str = "scripts/agent-review-bench/cases"; + +#[derive(Clone, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(super) struct BenchCase { + pub(super) id: String, + pub(super) pr: u64, + pub(super) title: String, + pub(super) remote: String, + pub(super) base: String, + pub(super) head: String, + pub(super) source: String, + pub(super) fixed_by: String, + pub(super) expected: Vec, +} + +#[derive(Clone, Debug, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct ExpectedFinding { + pub(super) id: String, + pub(super) path: String, + pub(super) line: usize, + pub(super) severity: Severity, + pub(super) description: String, + pub(super) match_if: String, +} + +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +#[serde(rename_all = "lowercase")] +pub(super) enum Severity { + Critical, + Major, + Minor, + Nit, +} + +pub(super) fn load_cases(workspace_root: &Path) -> anyhow::Result> { + let cases_dir = workspace_root.join(CASES_DIR); + let mut paths = fs::read_dir(&cases_dir) + .with_context(|| { + format!( + "failed to read benchmark cases from {}", + cases_dir.display() + ) + })? + .map(|entry| entry.map(|entry| entry.path())) + .collect::, _>>()?; + paths.retain(|path| { + path.extension() + .is_some_and(|extension| extension == "toml") + }); + paths.sort(); + + let mut cases = Vec::with_capacity(paths.len()); + for path in paths { + let text = fs::read_to_string(&path) + .with_context(|| format!("failed to read {}", path.display()))?; + let case = toml::from_str::(&text) + .with_context(|| format!("invalid benchmark case {}", path.display()))?; + validate_case_schema(&case) + .with_context(|| format!("invalid benchmark case {}", path.display()))?; + cases.push(case); + } + validate_unique_ids(&cases)?; + if cases.is_empty() { + bail!( + "no benchmark case TOML files found in {}", + cases_dir.display() + ); + } + Ok(cases) +} + +pub(super) fn select_cases<'a>( + cases: &'a [BenchCase], + case_ids: &[String], + prs: &[u64], +) -> anyhow::Result> { + if case_ids.is_empty() && prs.is_empty() { + return Ok(cases.iter().collect()); + } + + let requested_ids = case_ids.iter().map(String::as_str).collect::>(); + let requested_prs = prs.iter().copied().collect::>(); + let selected = cases + .iter() + .filter(|case| requested_ids.contains(case.id.as_str()) || requested_prs.contains(&case.pr)) + .collect::>(); + + let found_ids = selected + .iter() + .map(|case| case.id.as_str()) + .collect::>(); + let found_prs = selected.iter().map(|case| case.pr).collect::>(); + let missing_ids = requested_ids + .difference(&found_ids) + .copied() + .collect::>(); + let missing_prs = requested_prs + .difference(&found_prs) + .copied() + .collect::>(); + if !missing_ids.is_empty() || !missing_prs.is_empty() { + bail!( + "unknown benchmark selectors: case IDs [{}], PRs [{}]", + missing_ids.join(", "), + missing_prs + .iter() + .map(u64::to_string) + .collect::>() + .join(", ") + ); + } + Ok(selected) +} + +pub(super) fn prepare_case(workspace_root: &Path, case: &BenchCase) -> anyhow::Result<()> { + ensure_commit(workspace_root, case, &case.base)?; + ensure_commit(workspace_root, case, &case.head)?; + ensure_commit(workspace_root, case, &case.fixed_by)?; + ensure_ancestor(workspace_root, &case.base, &case.head)?; + + let changed_paths = git_lines( + workspace_root, + &["diff", "--name-only", &case.base, &case.head, "--"], + )? + .into_iter() + .collect::>(); + for expected in &case.expected { + if !changed_paths.contains(&expected.path) { + bail!( + "expected finding `{}` targets `{}`, which is not changed by {}..{}", + expected.id, + expected.path, + case.base, + case.head + ); + } + let object = format!("{}:{}", case.head, expected.path); + let content = git_output(workspace_root, &["show", &object])?; + let line_count = content.lines().count(); + if expected.line > line_count { + bail!( + "expected finding `{}` line {} exceeds `{}` line count {} at {}", + expected.id, + expected.line, + expected.path, + line_count, + case.head + ); + } + if !line_is_added(workspace_root, case, expected)? { + bail!( + "expected finding `{}` line {} is not an added HEAD-side line in `{}`", + expected.id, + expected.line, + expected.path + ); + } + } + Ok(()) +} + +fn validate_case_schema(case: &BenchCase) -> anyhow::Result<()> { + if !valid_id(&case.id) { + bail!("case id must contain only lowercase ASCII letters, digits, and hyphens"); + } + if case.pr == 0 { + bail!("PR number must be greater than zero"); + } + for (name, value) in [ + ("title", case.title.as_str()), + ("remote", case.remote.as_str()), + ("source", case.source.as_str()), + ] { + if value.trim().is_empty() { + bail!("{name} must not be empty"); + } + } + if !(case.remote.starts_with("https://") || case.remote.starts_with("git@")) { + bail!("remote must be an https:// or git@ fetch URL"); + } + for (name, sha) in [ + ("base", case.base.as_str()), + ("head", case.head.as_str()), + ("fixed_by", case.fixed_by.as_str()), + ] { + if !valid_sha(sha) { + bail!("{name} must be a full 40-character lowercase hexadecimal SHA"); + } + } + if case.base == case.head { + bail!("base and head must differ"); + } + if case.expected.is_empty() { + bail!("at least one expected finding is required"); + } + + let mut finding_ids = BTreeSet::new(); + for expected in &case.expected { + if !valid_id(&expected.id) { + bail!("finding id `{}` is invalid", expected.id); + } + if !finding_ids.insert(expected.id.as_str()) { + bail!("duplicate finding id `{}`", expected.id); + } + let finding_path = Path::new(&expected.path); + if expected.path.is_empty() + || finding_path.is_absolute() + || !finding_path + .components() + .all(|component| matches!(component, Component::Normal(_))) + { + bail!("finding `{}` path must be repository-relative", expected.id); + } + if expected.line == 0 { + bail!("finding `{}` line must be greater than zero", expected.id); + } + if expected.description.trim().is_empty() || expected.match_if.trim().is_empty() { + bail!( + "finding `{}` description and match_if must not be empty", + expected.id + ); + } + } + Ok(()) +} + +fn validate_unique_ids(cases: &[BenchCase]) -> anyhow::Result<()> { + let mut case_ids = BTreeSet::new(); + let mut finding_ids = BTreeSet::new(); + for case in cases { + if !case_ids.insert(case.id.as_str()) { + bail!("duplicate benchmark case id `{}`", case.id); + } + for expected in &case.expected { + if !finding_ids.insert(expected.id.as_str()) { + bail!("duplicate benchmark finding id `{}`", expected.id); + } + } + } + Ok(()) +} + +fn valid_id(value: &str) -> bool { + !value.is_empty() + && value + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') +} + +fn valid_sha(value: &str) -> bool { + value.len() == 40 + && value + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) +} + +fn ensure_commit(workspace_root: &Path, case: &BenchCase, sha: &str) -> anyhow::Result<()> { + if git_status( + workspace_root, + &["cat-file", "-e", &format!("{sha}^{{commit}}")], + )? { + return Ok(()); + } + let direct_fetch = Command::new("git") + .current_dir(workspace_root) + .args(["fetch", "--no-tags", &case.remote, sha]) + .output() + .with_context(|| format!("failed to fetch commit {sha} from {}", case.remote))?; + if !direct_fetch.status.success() { + let pull_ref = format!("refs/pull/{}/head", case.pr); + let pull_fetch = Command::new("git") + .current_dir(workspace_root) + .args(["fetch", "--no-tags", &case.remote, &pull_ref]) + .output() + .with_context(|| format!("failed to fetch {pull_ref} from {}", case.remote))?; + if !pull_fetch.status.success() { + bail!( + "could not fetch commit {sha} directly or through {pull_ref}: direct fetch: {}; \ + PR fetch: {}", + String::from_utf8_lossy(&direct_fetch.stderr).trim(), + String::from_utf8_lossy(&pull_fetch.stderr).trim() + ); + } + } + if !git_status( + workspace_root, + &["cat-file", "-e", &format!("{sha}^{{commit}}")], + )? { + bail!("fetched object {sha} is not a commit"); + } + Ok(()) +} + +fn line_is_added( + workspace_root: &Path, + case: &BenchCase, + expected: &ExpectedFinding, +) -> anyhow::Result { + let diff = git_output( + workspace_root, + &[ + "diff", + "--unified=0", + &case.base, + &case.head, + "--", + &expected.path, + ], + )?; + for line in diff.lines().filter(|line| line.starts_with("@@ ")) { + let Some(head_range) = line + .split_ascii_whitespace() + .find(|field| field.starts_with('+')) + else { + continue; + }; + let range = head_range.trim_start_matches('+'); + let (start, count) = match range.split_once(',') { + Some((start, count)) => (start, count), + None => (range, "1"), + }; + let start = start + .parse::() + .with_context(|| format!("invalid diff hunk start `{start}`"))?; + let count = count + .parse::() + .with_context(|| format!("invalid diff hunk count `{count}`"))?; + if count > 0 && (start..start + count).contains(&expected.line) { + return Ok(true); + } + } + Ok(false) +} + +fn ensure_ancestor(workspace_root: &Path, base: &str, head: &str) -> anyhow::Result<()> { + if git_status(workspace_root, &["merge-base", "--is-ancestor", base, head])? { + Ok(()) + } else { + bail!("base {base} is not an ancestor of head {head}") + } +} + +fn git_lines(workspace_root: &Path, args: &[&str]) -> anyhow::Result> { + Ok(git_output(workspace_root, args)? + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(str::to_string) + .collect()) +} + +fn git_output(workspace_root: &Path, args: &[&str]) -> anyhow::Result { + let output = Command::new("git") + .current_dir(workspace_root) + .args(args) + .output() + .with_context(|| format!("failed to run git {}", args.join(" ")))?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!( + "git {} exited with status {}: {}", + args.join(" "), + output.status, + stderr.trim() + ); + } + String::from_utf8(output.stdout).context("git output was not UTF-8") +} + +fn git_status(workspace_root: &Path, args: &[&str]) -> anyhow::Result { + let status = Command::new("git") + .current_dir(workspace_root) + .args(args) + .status() + .with_context(|| format!("failed to run git {}", args.join(" ")))?; + Ok(status.success()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sample_case() -> BenchCase { + BenchCase { + id: "0001-sample".into(), + pr: 1, + title: "sample".into(), + remote: "https://github.com/example/repo.git".into(), + base: "a".repeat(40), + head: "b".repeat(40), + source: "https://github.com/example/repo/pull/1".into(), + fixed_by: "c".repeat(40), + expected: vec![ExpectedFinding { + id: "sample-finding".into(), + path: "src/lib.rs".into(), + line: 1, + severity: Severity::Major, + description: "sample defect".into(), + match_if: "reviewer identifies sample defect".into(), + }], + } + } + + #[test] + fn validates_well_formed_case() { + validate_case_schema(&sample_case()).unwrap(); + } + + #[test] + fn rejects_short_sha() { + let mut case = sample_case(); + case.head = "abc".into(); + assert!(validate_case_schema(&case).is_err()); + } + + #[test] + fn selectors_form_a_union() { + let first = sample_case(); + let mut second = sample_case(); + second.id = "0002-second".into(); + second.pr = 2; + second.expected[0].id = "second-finding".into(); + let cases = [first, second]; + + let selected = select_cases(&cases, &["0001-sample".into()], &[2]).unwrap(); + assert_eq!(selected.len(), 2); + } + + #[test] + fn rejects_unknown_selector() { + assert!(select_cases(&[sample_case()], &["missing".into()], &[]).is_err()); + } +} diff --git a/scripts/axbuild/src/agent_review_bench/codex.rs b/scripts/axbuild/src/agent_review_bench/codex.rs new file mode 100644 index 0000000000..32f0ff9373 --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/codex.rs @@ -0,0 +1,235 @@ +use std::{ + fs, + path::{Path, PathBuf}, + process::Stdio, + time::Duration, +}; + +use anyhow::{Context, bail}; +use tempfile::tempdir; +use tokio::{process::Command, time::timeout}; + +use super::{cases::BenchCase, sandbox::ReviewSandbox}; + +const REVIEW_PROMPT: &str = + "You are performing an offline code review benchmark. Review only the committed changes from \ + `bench-base` to `HEAD` in the current repository. First read `AGENTS.md`, every file under \ + `book/guideline/`, and `.agent-review-context/reviewer.md`, then follow those rules. Inspect \ + the diff and relevant in-repository context for actionable defects. Do not use the network, \ + GitHub, paths outside this repository, or write operations. Return only the JSON object \ + required by the output schema."; +const GRADE_PROMPT: &str = "You are grading an offline code review. Read `expected.json` and \ + `review.json` in the current directory. For every expected item, \ + decide whether one review finding satisfies its `match_if` criterion \ + at the stated code location. Return exactly one match object per \ + expected ID. Use the zero-based index in `review.findings` when \ + caught, or null when missed. Wording may differ, but nearby or \ + generic comments do not count. Do not inspect any other paths."; +const GRADE_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/grade.schema.json"); + +pub(super) struct CodexOptions { + pub(super) model: Option, + pub(super) reasoning_effort: String, + pub(super) timeout_secs: u64, +} + +pub(super) struct CodexRunner { + program: PathBuf, +} + +impl CodexRunner { + pub(super) fn discover() -> anyhow::Result { + let runner = Self { + program: PathBuf::from("codex"), + }; + runner + .version() + .context("Codex CLI is unavailable or not authenticated for local use")?; + Ok(runner) + } + + pub(super) fn version(&self) -> anyhow::Result { + let output = std::process::Command::new(&self.program) + .arg("--version") + .output() + .with_context(|| format!("failed to execute {} --version", self.program.display()))?; + if !output.status.success() { + bail!( + "{} --version exited with {}", + self.program.display(), + output.status + ); + } + let version = String::from_utf8(output.stdout).context("Codex version was not UTF-8")?; + let version = version.trim(); + if version.is_empty() { + bail!("Codex CLI returned an empty version"); + } + Ok(version.to_string()) + } + + pub(super) async fn review( + &self, + sandbox: &ReviewSandbox, + artifact_path: &Path, + options: &CodexOptions, + ) -> anyhow::Result<()> { + let temporary_output = sandbox.temporary_review_output(); + remove_if_exists(&temporary_output)?; + let mut command = Command::new(&self.program); + command + .current_dir(sandbox.repo()) + .args(["exec", "--ephemeral", "--sandbox", "read-only"]) + .args(["-c", "approval_policy=\"never\""]) + .args(["-c", "sandbox_workspace_write.network_access=false"]) + .arg("--output-schema") + .arg(sandbox.review_schema()) + .arg("--output-last-message") + .arg(&temporary_output); + apply_model_options(&mut command, options); + command.arg(REVIEW_PROMPT); + + run_process(&mut command, options.timeout_secs, "Codex reviewer").await?; + copy_nonempty_output(&temporary_output, artifact_path, "reviewer") + } + + pub(super) async fn grade( + &self, + case: &BenchCase, + review_path: &Path, + artifact_path: &Path, + options: &CodexOptions, + ) -> anyhow::Result<()> { + let grader_dir = tempdir().context("failed to create isolated grader directory")?; + let grader_review = grader_dir.path().join("review.json"); + fs::copy(review_path, &grader_review)?; + let expected = serde_json::to_string_pretty(&case.expected)?; + fs::write(grader_dir.path().join("expected.json"), expected)?; + let schema_path = grader_dir.path().join("grade.schema.json"); + fs::write(&schema_path, GRADE_SCHEMA)?; + let temporary_output = grader_dir.path().join("grade.json"); + + let mut command = Command::new(&self.program); + command + .args(["exec", "--skip-git-repo-check", "--ephemeral"]) + .args(["--sandbox", "read-only"]) + .args(["-c", "approval_policy=\"never\""]) + .args(["-c", "sandbox_workspace_write.network_access=false"]) + .arg("--cd") + .arg(grader_dir.path()) + .arg("--output-schema") + .arg(&schema_path) + .arg("--output-last-message") + .arg(&temporary_output); + apply_model_options(&mut command, options); + command.arg(GRADE_PROMPT); + + run_process(&mut command, options.timeout_secs, "Codex grader").await?; + copy_nonempty_output(&temporary_output, artifact_path, "grader") + } + + #[cfg(test)] + pub(super) fn from_program(program: PathBuf) -> Self { + Self { program } + } +} + +fn apply_model_options(command: &mut Command, options: &CodexOptions) { + if let Some(model) = &options.model { + command.args(["--model", model]); + } + command.args([ + "-c", + &format!("model_reasoning_effort=\"{}\"", options.reasoning_effort), + ]); +} + +async fn run_process( + command: &mut Command, + timeout_secs: u64, + description: &str, +) -> anyhow::Result<()> { + command + .stdin(Stdio::null()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()) + .kill_on_drop(true); + let mut child = command + .spawn() + .with_context(|| format!("failed to spawn {description}"))?; + let status = match timeout(Duration::from_secs(timeout_secs), child.wait()).await { + Ok(status) => status.with_context(|| format!("failed to wait for {description}"))?, + Err(_) => { + child + .kill() + .await + .with_context(|| format!("failed to terminate timed-out {description}"))?; + let _ = child.wait().await; + bail!("{description} timed out after {timeout_secs} seconds"); + } + }; + if status.success() { + Ok(()) + } else { + bail!("{description} exited with status {status}") + } +} + +fn copy_nonempty_output(source: &Path, destination: &Path, role: &str) -> anyhow::Result<()> { + let metadata = fs::metadata(source) + .with_context(|| format!("{role} did not create {}", source.display()))?; + if metadata.len() == 0 { + bail!("{role} created an empty output file"); + } + fs::copy(source, destination).with_context(|| { + format!( + "failed to copy {role} output from {} to {}", + source.display(), + destination.display() + ) + })?; + Ok(()) +} + +fn remove_if_exists(path: &Path) -> anyhow::Result<()> { + match fs::remove_file(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err).with_context(|| format!("failed to remove {}", path.display())), + } +} + +#[cfg(test)] +mod tests { + use std::{fs, os::unix::fs::PermissionsExt}; + + use tempfile::tempdir; + + use super::*; + + #[test] + fn reads_version_from_injected_codex_program() { + let temp = tempdir().unwrap(); + let program = temp.path().join("mock-codex"); + fs::write(&program, "#!/bin/sh\necho 'codex-cli test-version'\n").unwrap(); + fs::set_permissions(&program, fs::Permissions::from_mode(0o755)).unwrap(); + + let runner = CodexRunner::from_program(program); + assert_eq!(runner.version().unwrap(), "codex-cli test-version"); + } + + #[tokio::test] + async fn reports_nonzero_and_timeout_processes() { + let mut failure = Command::new("sh"); + failure.args(["-c", "exit 7"]); + assert!(run_process(&mut failure, 1, "mock failure").await.is_err()); + + let mut slow = Command::new("sh"); + slow.args(["-c", "sleep 2"]); + let error = run_process(&mut slow, 1, "mock timeout") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("timed out")); + } +} diff --git a/scripts/axbuild/src/agent_review_bench/mod.rs b/scripts/axbuild/src/agent_review_bench/mod.rs new file mode 100644 index 0000000000..8aebc5b5c3 --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/mod.rs @@ -0,0 +1,532 @@ +//! Offline agent-review benchmark orchestration. + +use std::{fs, path::PathBuf, time::Instant}; + +use anyhow::{Context, bail}; +use chrono::Utc; +use clap::{Args, Subcommand, ValueEnum}; +use serde::Serialize; + +use crate::context; + +mod cases; +mod codex; +mod sandbox; +mod scoring; + +use cases::{BenchCase, load_cases, prepare_case, select_cases}; +use codex::{CodexOptions, CodexRunner}; +use sandbox::ReviewSandbox; +use scoring::{CaseScore, GradeOutput, ReviewOutput, score_review}; + +const DEFAULT_TIMEOUT_SECS: u64 = 1800; + +#[derive(Subcommand, Debug)] +pub(crate) enum Command { + /// List configured benchmark cases without fetching commits + List, + /// Validate cases and their historical Git snapshots without invoking Codex + Check, + /// Review and grade selected cases sequentially + Run(RunArgs), +} + +#[derive(Args, Clone, Debug, PartialEq, Eq)] +pub(crate) struct RunArgs { + /// Select a case by exact ID; repeat to select more cases + #[arg(long = "case", value_name = "ID")] + cases: Vec, + /// Select every case sourced from a PR; repeat to select more PRs + #[arg(long = "pr", value_name = "NUMBER")] + prs: Vec, + /// Override the Codex model; omit to inherit the user's Codex configuration + #[arg(long)] + model: Option, + /// Reasoning effort used by both reviewer and grader + #[arg(long, value_enum, default_value_t = ReasoningEffort::High)] + reasoning_effort: ReasoningEffort, + /// Timeout for each reviewer or grader invocation + #[arg(long, default_value_t = DEFAULT_TIMEOUT_SECS)] + timeout_secs: u64, + /// Fail when total recall is below this percentage + #[arg(long, value_parser = clap::value_parser!(u8).range(0..=100))] + min_recall: Option, + /// Artifact directory; relative paths are resolved from the workspace root + #[arg(long)] + output: Option, +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, ValueEnum)] +enum ReasoningEffort { + Minimal, + Low, + Medium, + #[default] + High, + Xhigh, +} + +impl ReasoningEffort { + fn as_str(self) -> &'static str { + match self { + Self::Minimal => "minimal", + Self::Low => "low", + Self::Medium => "medium", + Self::High => "high", + Self::Xhigh => "xhigh", + } + } +} + +#[derive(Debug, Serialize)] +struct RunSummary { + generated_at: String, + codex_version: String, + model: Option, + reasoning_effort: String, + timeout_secs: u64, + min_recall: Option, + caught: usize, + expected: usize, + recall_percent: f64, + extra_findings: usize, + cases: Vec, +} + +#[derive(Debug, Serialize)] +struct CaseResult { + case_id: String, + pr: u64, + caught: usize, + expected: usize, + recall_percent: f64, + extra_findings: usize, + review_seconds: f64, + grade_seconds: f64, +} + +pub(crate) async fn execute(command: Command) -> anyhow::Result<()> { + let workspace_root = context::workspace_root_path()?; + let all_cases = load_cases(&workspace_root)?; + + match command { + Command::List => list_cases(&all_cases), + Command::Check => check_cases(&workspace_root, &all_cases), + Command::Run(args) => run_cases(&workspace_root, &all_cases, args).await, + } +} + +fn list_cases(cases: &[BenchCase]) -> anyhow::Result<()> { + for case in cases { + println!( + "{}\tPR #{}\t{} expected\t{}", + case.id, + case.pr, + case.expected.len(), + case.title + ); + } + Ok(()) +} + +fn check_cases(workspace_root: &std::path::Path, cases: &[BenchCase]) -> anyhow::Result<()> { + for case in cases { + prepare_case(workspace_root, case) + .with_context(|| format!("benchmark case `{}` is invalid", case.id))?; + println!("checked {} (PR #{})", case.id, case.pr); + } + println!("OK: {} agent-review benchmark cases", cases.len()); + Ok(()) +} + +async fn run_cases( + workspace_root: &std::path::Path, + all_cases: &[BenchCase], + args: RunArgs, +) -> anyhow::Result<()> { + let runner = CodexRunner::discover()?; + run_cases_with_runner(workspace_root, all_cases, args, &runner).await +} + +async fn run_cases_with_runner( + workspace_root: &std::path::Path, + all_cases: &[BenchCase], + args: RunArgs, + runner: &CodexRunner, +) -> anyhow::Result<()> { + if args.timeout_secs == 0 { + bail!("--timeout-secs must be greater than zero"); + } + if args.model.as_deref().is_some_and(str::is_empty) { + bail!("--model must not be empty"); + } + + let selected = select_cases(all_cases, &args.cases, &args.prs)?; + let output_dir = resolve_output_dir(workspace_root, args.output.as_deref()); + fs::create_dir_all(&output_dir).with_context(|| { + format!( + "failed to create benchmark artifact directory {}", + output_dir.display() + ) + })?; + + let codex_version = runner.version()?; + let options = CodexOptions { + model: args.model.clone(), + reasoning_effort: args.reasoning_effort.as_str().to_string(), + timeout_secs: args.timeout_secs, + }; + + let mut results = Vec::with_capacity(selected.len()); + for case in selected { + println!("reviewing {} (PR #{})", case.id, case.pr); + prepare_case(workspace_root, case)?; + let case_dir = output_dir.join(&case.id); + fs::create_dir_all(&case_dir)?; + + let sandbox = ReviewSandbox::create(workspace_root, case)?; + let review_path = case_dir.join("review.json"); + let review_started = Instant::now(); + runner + .review(&sandbox, &review_path, &options) + .await + .with_context(|| format!("reviewer failed for `{}`", case.id))?; + let review_seconds = review_started.elapsed().as_secs_f64(); + let review = read_json::(&review_path)?; + + let grade_path = case_dir.join("grade.json"); + let grade_started = Instant::now(); + runner + .grade(case, &review_path, &grade_path, &options) + .await + .with_context(|| format!("grader failed for `{}`", case.id))?; + let grade_seconds = grade_started.elapsed().as_secs_f64(); + let grade = read_json::(&grade_path)?; + let score = score_review(case, &review, &grade)?; + let result = case_result(case, score, review_seconds, grade_seconds); + write_json(&case_dir.join("result.json"), &result)?; + println!( + " recall {}/{} ({:.1}%), extra findings {}", + result.caught, result.expected, result.recall_percent, result.extra_findings + ); + results.push(result); + } + + let summary = summarize(&codex_version, &options, args.min_recall, results); + write_json(&output_dir.join("summary.json"), &summary)?; + println!( + "agent-review recall: {}/{} ({:.1}%); extra findings: {}; artifacts: {}", + summary.caught, + summary.expected, + summary.recall_percent, + summary.extra_findings, + output_dir.display() + ); + + if recall_gate_failed(summary.recall_percent, args.min_recall) { + let min_recall = args + .min_recall + .expect("a failed recall gate must have a threshold"); + bail!( + "agent-review recall {:.1}% is below the requested {}% gate", + summary.recall_percent, + min_recall + ); + } + Ok(()) +} + +fn resolve_output_dir( + workspace_root: &std::path::Path, + output: Option<&std::path::Path>, +) -> PathBuf { + if let Some(output) = output { + return if output.is_absolute() { + output.to_path_buf() + } else { + workspace_root.join(output) + }; + } + let run_id = Utc::now().format("%Y%m%d-%H%M%S-%3fZ"); + workspace_root + .join("target/agent-review-bench") + .join(run_id.to_string()) +} + +fn case_result( + case: &BenchCase, + score: CaseScore, + review_seconds: f64, + grade_seconds: f64, +) -> CaseResult { + CaseResult { + case_id: case.id.clone(), + pr: case.pr, + caught: score.caught, + expected: score.expected, + recall_percent: percentage(score.caught, score.expected), + extra_findings: score.extra_findings, + review_seconds, + grade_seconds, + } +} + +fn summarize( + codex_version: &str, + options: &CodexOptions, + min_recall: Option, + cases: Vec, +) -> RunSummary { + let caught = cases.iter().map(|case| case.caught).sum(); + let expected = cases.iter().map(|case| case.expected).sum(); + let extra_findings = cases.iter().map(|case| case.extra_findings).sum(); + RunSummary { + generated_at: Utc::now().to_rfc3339(), + codex_version: codex_version.to_string(), + model: options.model.clone(), + reasoning_effort: options.reasoning_effort.clone(), + timeout_secs: options.timeout_secs, + min_recall, + caught, + expected, + recall_percent: percentage(caught, expected), + extra_findings, + cases, + } +} + +fn recall_gate_failed(recall_percent: f64, min_recall: Option) -> bool { + min_recall.is_some_and(|minimum| recall_percent < f64::from(minimum)) +} + +fn percentage(numerator: usize, denominator: usize) -> f64 { + if denominator == 0 { + 0.0 + } else { + 100.0 * numerator as f64 / denominator as f64 + } +} + +fn read_json(path: &std::path::Path) -> anyhow::Result { + let text = + fs::read_to_string(path).with_context(|| format!("failed to read {}", path.display()))?; + serde_json::from_str(&text).with_context(|| format!("invalid JSON in {}", path.display())) +} + +fn write_json(path: &std::path::Path, value: &impl Serialize) -> anyhow::Result<()> { + let mut text = serde_json::to_string_pretty(value)?; + text.push('\n'); + fs::write(path, text).with_context(|| format!("failed to write {}", path.display())) +} + +#[cfg(test)] +mod tests { + #[cfg(unix)] + use std::{fs, os::unix::fs::PermissionsExt, path::Path, process::Command as ProcessCommand}; + + use clap::Parser; + + use super::*; + #[cfg(unix)] + use crate::agent_review_bench::cases::{ExpectedFinding, Severity}; + + #[derive(Parser)] + struct TestCli { + #[command(subcommand)] + command: Command, + } + + #[test] + fn parses_run_selectors_and_gate() { + let cli = TestCli::try_parse_from([ + "bench", + "run", + "--case", + "case-a", + "--case", + "case-b", + "--pr", + "1495", + "--min-recall", + "80", + "--reasoning-effort", + "medium", + ]) + .unwrap(); + + let Command::Run(args) = cli.command else { + panic!("expected run command"); + }; + assert_eq!(args.cases, ["case-a", "case-b"]); + assert_eq!(args.prs, [1495]); + assert_eq!(args.min_recall, Some(80)); + assert_eq!(args.reasoning_effort, ReasoningEffort::Medium); + } + + #[test] + fn percentage_handles_empty_and_non_empty_totals() { + assert_eq!(percentage(0, 0), 0.0); + assert_eq!(percentage(1, 2), 50.0); + } + + #[test] + fn recall_gate_is_opt_in_and_inclusive() { + assert!(!recall_gate_failed(0.0, None)); + assert!(!recall_gate_failed(80.0, Some(80))); + assert!(recall_gate_failed(79.9, Some(80))); + } + + #[cfg(unix)] + #[tokio::test] + async fn mock_codex_writes_artifacts_and_rejects_invalid_json() { + let workspace = tempfile::tempdir().unwrap(); + let case = create_test_case(workspace.path()); + let output = workspace.path().join("artifacts"); + let program = workspace.path().join("mock-codex"); + write_mock_codex( + &program, + r#"{"summary":"found one issue","findings":[{"title":"caught","body":"body","path":"src/lib.rs","line":1,"severity":"major"},{"title":"extra","body":"body","path":"src/lib.rs","line":1,"severity":"minor"}]}"#, + r#"{"matches":[{"expected_id":"sample-finding","finding_index":0,"reason":"same defect"}]}"#, + ); + let runner = CodexRunner::from_program(program.clone()); + let args = test_run_args(output.clone()); + + run_cases_with_runner(workspace.path(), &[case.clone()], args, &runner) + .await + .unwrap(); + + let case_dir = output.join(&case.id); + for artifact in [ + case_dir.join("review.json"), + case_dir.join("grade.json"), + case_dir.join("result.json"), + output.join("summary.json"), + ] { + assert!( + artifact.is_file(), + "missing artifact {}", + artifact.display() + ); + } + let summary = read_json::(&output.join("summary.json")).unwrap(); + assert_eq!(summary["caught"], 1); + assert_eq!(summary["expected"], 1); + assert_eq!(summary["extra_findings"], 1); + assert_eq!(summary["timeout_secs"], 2); + + write_mock_codex( + &program, + "not JSON", + r#"{"matches":[{"expected_id":"sample-finding","finding_index":null,"reason":"missed"}]}"#, + ); + let invalid_output = workspace.path().join("invalid-artifacts"); + let error = run_cases_with_runner( + workspace.path(), + &[case], + test_run_args(invalid_output), + &runner, + ) + .await + .unwrap_err() + .to_string(); + assert!(error.contains("invalid JSON")); + } + + #[cfg(unix)] + fn create_test_case(workspace: &Path) -> BenchCase { + fs::write(workspace.join("AGENTS.md"), "current rules\n").unwrap(); + fs::create_dir_all(workspace.join("book/guideline")).unwrap(); + fs::write( + workspace.join("book/guideline/code-quality.md"), + "current guideline\n", + ) + .unwrap(); + git(workspace, &["init", "--quiet"]); + git(workspace, &["config", "user.name", "test"]); + git(workspace, &["config", "user.email", "test@invalid"]); + fs::create_dir_all(workspace.join("src")).unwrap(); + fs::write(workspace.join("src/lib.rs"), "pub fn value() -> u8 { 1 }\n").unwrap(); + git(workspace, &["add", "--all"]); + git(workspace, &["commit", "--quiet", "-m", "base"]); + let base = git_output(workspace, &["rev-parse", "HEAD"]); + + fs::write(workspace.join("src/lib.rs"), "pub fn value() -> u8 { 2 }\n").unwrap(); + git(workspace, &["add", "--all"]); + git(workspace, &["commit", "--quiet", "-m", "head"]); + let head = git_output(workspace, &["rev-parse", "HEAD"]); + + fs::write(workspace.join("fixed.txt"), "fixed\n").unwrap(); + git(workspace, &["add", "--all"]); + git(workspace, &["commit", "--quiet", "-m", "fix"]); + let fixed_by = git_output(workspace, &["rev-parse", "HEAD"]); + + BenchCase { + id: "0001-sample".into(), + pr: 1, + title: "sample change".into(), + remote: "https://example.invalid/repo.git".into(), + base, + head, + source: "secret source".into(), + fixed_by, + expected: vec![ExpectedFinding { + id: "sample-finding".into(), + path: "src/lib.rs".into(), + line: 1, + severity: Severity::Major, + description: "secret answer".into(), + match_if: "same defect".into(), + }], + } + } + + #[cfg(unix)] + fn test_run_args(output: PathBuf) -> RunArgs { + RunArgs { + cases: Vec::new(), + prs: Vec::new(), + model: Some("mock-model".into()), + reasoning_effort: ReasoningEffort::High, + timeout_secs: 2, + min_recall: None, + output: Some(output), + } + } + + #[cfg(unix)] + fn write_mock_codex(program: &Path, review: &str, grade: &str) { + let script = format!( + "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo 'codex-cli mock'; exit 0; \ + fi\nmode=review\nif [ -f expected.json ]; then mode=grade; fi\noutput=\nwhile [ \ + \"$#\" -gt 0 ]; do\ncase \"$1\" in\n--cd) shift; mode=grade \ + ;;\n--output-last-message|-o) shift; output=$1 ;;\nesac\nshift\ndone\nif [ \"$mode\" \ + = review ]; then printf '%s\\n' '{review}' > \"$output\"; else printf '%s\\n' \ + '{grade}' > \"$output\"; fi\n" + ); + fs::write(program, script).unwrap(); + fs::set_permissions(program, fs::Permissions::from_mode(0o755)).unwrap(); + } + + #[cfg(unix)] + fn git(workspace: &Path, args: &[&str]) { + assert!( + ProcessCommand::new("git") + .current_dir(workspace) + .args(args) + .status() + .unwrap() + .success() + ); + } + + #[cfg(unix)] + fn git_output(workspace: &Path, args: &[&str]) -> String { + let output = ProcessCommand::new("git") + .current_dir(workspace) + .args(args) + .output() + .unwrap(); + assert!(output.status.success()); + String::from_utf8(output.stdout).unwrap().trim().into() + } +} diff --git a/scripts/axbuild/src/agent_review_bench/sandbox.rs b/scripts/axbuild/src/agent_review_bench/sandbox.rs new file mode 100644 index 0000000000..69a1970485 --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/sandbox.rs @@ -0,0 +1,292 @@ +use std::{ + ffi::OsStr, + fs, + path::{Path, PathBuf}, + process::Command, +}; + +use anyhow::{Context, bail}; +use tempfile::TempDir; +use walkdir::WalkDir; + +use super::cases::BenchCase; + +const REVIEW_CONTRACT: &str = include_str!("../../../agent-review-bench/reviewer.md"); +const REVIEW_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/review.schema.json"); + +pub(super) struct ReviewSandbox { + _root: TempDir, + repo: PathBuf, +} + +impl ReviewSandbox { + pub(super) fn create(workspace_root: &Path, case: &BenchCase) -> anyhow::Result { + let root = tempfile::Builder::new() + .prefix("tgos-agent-review-") + .tempdir() + .context("failed to create review sandbox")?; + let repo = root.path().join("repo"); + fs::create_dir(&repo)?; + + extract_snapshot(workspace_root, &case.base, root.path(), &repo)?; + overlay_current_review_context(workspace_root, &repo)?; + initialize_repo(&repo)?; + commit_all(&repo, "benchmark base")?; + git(&repo, ["branch", "bench-base"])?; + + clear_worktree(&repo)?; + extract_snapshot(workspace_root, &case.head, root.path(), &repo)?; + overlay_current_review_context(workspace_root, &repo)?; + commit_all(&repo, &case.title)?; + ensure_review_diff(&repo)?; + ensure_standalone_git_dir(&repo)?; + + Ok(Self { _root: root, repo }) + } + + pub(super) fn repo(&self) -> &Path { + &self.repo + } + + pub(super) fn review_schema(&self) -> PathBuf { + self.repo.join(".agent-review-context/review.schema.json") + } + + pub(super) fn temporary_review_output(&self) -> PathBuf { + self.repo + .parent() + .expect("sandbox repository must have a parent") + .join("review.json") + } +} + +fn extract_snapshot( + workspace_root: &Path, + revision: &str, + scratch_root: &Path, + destination: &Path, +) -> anyhow::Result<()> { + let archive_path = scratch_root.join(format!("{revision}.tar")); + let status = Command::new("git") + .current_dir(workspace_root) + .arg("archive") + .arg("--format=tar") + .arg(format!("--output={}", archive_path.display())) + .arg(revision) + .status() + .with_context(|| format!("failed to archive revision {revision}"))?; + if !status.success() { + bail!("git archive exited with status {status} for revision {revision}"); + } + + let archive_file = fs::File::open(&archive_path)?; + tar::Archive::new(archive_file) + .unpack(destination) + .with_context(|| format!("failed to unpack revision {revision}"))?; + fs::remove_file(archive_path)?; + Ok(()) +} + +fn overlay_current_review_context(workspace_root: &Path, repo: &Path) -> anyhow::Result<()> { + fs::copy(workspace_root.join("AGENTS.md"), repo.join("AGENTS.md")) + .context("failed to copy current AGENTS.md into review sandbox")?; + + let guideline_destination = repo.join("book/guideline"); + if guideline_destination.exists() { + fs::remove_dir_all(&guideline_destination)?; + } + copy_tree( + &workspace_root.join("book/guideline"), + &guideline_destination, + )?; + + let context_dir = repo.join(".agent-review-context"); + fs::create_dir_all(&context_dir)?; + fs::write(context_dir.join("reviewer.md"), REVIEW_CONTRACT)?; + fs::write(context_dir.join("review.schema.json"), REVIEW_SCHEMA)?; + Ok(()) +} + +fn copy_tree(source: &Path, destination: &Path) -> anyhow::Result<()> { + for entry in WalkDir::new(source) { + let entry = entry?; + let relative = entry.path().strip_prefix(source)?; + let target = destination.join(relative); + if entry.file_type().is_dir() { + fs::create_dir_all(&target)?; + } else if entry.file_type().is_file() { + fs::copy(entry.path(), &target).with_context(|| { + format!( + "failed to copy review context {} to {}", + entry.path().display(), + target.display() + ) + })?; + } else { + bail!( + "unsupported review-context file type at {}", + entry.path().display() + ); + } + } + Ok(()) +} + +fn initialize_repo(repo: &Path) -> anyhow::Result<()> { + git(repo, ["init", "--quiet"])?; + git(repo, ["config", "user.name", "TGOS Review Benchmark"])?; + git(repo, ["config", "user.email", "review-benchmark@invalid"])?; + Ok(()) +} + +fn commit_all(repo: &Path, message: &str) -> anyhow::Result<()> { + git(repo, ["add", "--all"])?; + git(repo, ["commit", "--quiet", "--message", message])?; + Ok(()) +} + +fn clear_worktree(repo: &Path) -> anyhow::Result<()> { + for entry in fs::read_dir(repo)? { + let entry = entry?; + if entry.file_name() == OsStr::new(".git") { + continue; + } + let path = entry.path(); + if entry.file_type()?.is_dir() { + fs::remove_dir_all(path)?; + } else { + fs::remove_file(path)?; + } + } + Ok(()) +} + +fn ensure_review_diff(repo: &Path) -> anyhow::Result<()> { + let status = Command::new("git") + .current_dir(repo) + .args(["diff", "--quiet", "bench-base", "HEAD", "--"]) + .status()?; + match status.code() { + Some(1) => Ok(()), + Some(0) => bail!("synthetic review repository has an empty diff"), + _ => bail!("git diff exited with status {status}"), + } +} + +fn ensure_standalone_git_dir(repo: &Path) -> anyhow::Result<()> { + let git_dir = repo.join(".git"); + if !git_dir.is_dir() { + bail!("review sandbox .git is not a standalone directory"); + } + let alternates = git_dir.join("objects/info/alternates"); + if alternates.exists() { + bail!("review sandbox unexpectedly references external Git objects"); + } + Ok(()) +} + +fn git(repo: &Path, args: I) -> anyhow::Result<()> +where + I: IntoIterator, + S: AsRef, +{ + let status = Command::new("git") + .current_dir(repo) + .args(args) + .status() + .context("failed to spawn git")?; + if status.success() { + Ok(()) + } else { + bail!("git exited with status {status} in {}", repo.display()) + } +} + +#[cfg(test)] +mod tests { + use std::process::Command; + + use tempfile::tempdir; + + use super::*; + use crate::agent_review_bench::cases::{ExpectedFinding, Severity}; + + #[test] + fn creates_standalone_two_commit_repository_without_ground_truth() { + let workspace = tempdir().unwrap(); + fs::write(workspace.path().join("AGENTS.md"), "current rules\n").unwrap(); + fs::create_dir_all(workspace.path().join("book/guideline")).unwrap(); + fs::write( + workspace.path().join("book/guideline/code-quality.md"), + "current guideline\n", + ) + .unwrap(); + git(workspace.path(), ["init", "--quiet"]).unwrap(); + git(workspace.path(), ["config", "user.name", "test"]).unwrap(); + git(workspace.path(), ["config", "user.email", "test@invalid"]).unwrap(); + fs::create_dir_all(workspace.path().join("src")).unwrap(); + fs::write( + workspace.path().join("src/lib.rs"), + "pub fn value() -> u8 { 1 }\n", + ) + .unwrap(); + commit_all(workspace.path(), "base").unwrap(); + let base = rev_parse(workspace.path(), "HEAD"); + fs::write( + workspace.path().join("src/lib.rs"), + "pub fn value() -> u8 { 2 }\n", + ) + .unwrap(); + commit_all(workspace.path(), "head").unwrap(); + let head = rev_parse(workspace.path(), "HEAD"); + + let case = BenchCase { + id: "0001-sample".into(), + pr: 1, + title: "sample change".into(), + remote: "https://github.com/example/repo.git".into(), + base, + head, + source: "secret source".into(), + fixed_by: "a".repeat(40), + expected: vec![ExpectedFinding { + id: "secret-finding".into(), + path: "src/lib.rs".into(), + line: 1, + severity: Severity::Major, + description: "secret answer".into(), + match_if: "secret criterion".into(), + }], + }; + + let sandbox = ReviewSandbox::create(workspace.path(), &case).unwrap(); + assert!(sandbox.repo().join(".git").is_dir()); + assert!(!sandbox.repo().join(".git/objects/info/alternates").exists()); + assert_eq!( + fs::read_to_string(sandbox.repo().join("AGENTS.md")).unwrap(), + "current rules\n" + ); + let diff = Command::new("git") + .current_dir(sandbox.repo()) + .args(["diff", "bench-base", "HEAD", "--", "src/lib.rs"]) + .output() + .unwrap(); + let diff = String::from_utf8(diff.stdout).unwrap(); + assert!(diff.contains("value() -> u8 { 2 }")); + assert!(!WalkDir::new(sandbox.repo()).into_iter().any(|entry| { + entry + .ok() + .and_then(|entry| fs::read_to_string(entry.path()).ok()) + .is_some_and(|text| text.contains("secret answer")) + })); + } + + fn rev_parse(repo: &Path, revision: &str) -> String { + let output = Command::new("git") + .current_dir(repo) + .args(["rev-parse", revision]) + .output() + .unwrap(); + String::from_utf8(output.stdout).unwrap().trim().to_string() + } +} diff --git a/scripts/axbuild/src/agent_review_bench/scoring.rs b/scripts/axbuild/src/agent_review_bench/scoring.rs new file mode 100644 index 0000000000..494613c038 --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/scoring.rs @@ -0,0 +1,202 @@ +use std::collections::{BTreeMap, BTreeSet}; + +use anyhow::{Context, bail}; +use serde::{Deserialize, Serialize}; + +use super::cases::BenchCase; + +#[derive(Debug, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct ReviewOutput { + pub(super) summary: String, + pub(super) findings: Vec, +} + +#[derive(Debug, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct ReviewFinding { + pub(super) title: String, + pub(super) body: String, + pub(super) path: String, + pub(super) line: usize, + pub(super) severity: String, +} + +#[derive(Debug, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct GradeOutput { + pub(super) matches: Vec, +} + +#[derive(Debug, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct FindingMatch { + pub(super) expected_id: String, + pub(super) finding_index: Option, + pub(super) reason: String, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) struct CaseScore { + pub(super) caught: usize, + pub(super) expected: usize, + pub(super) extra_findings: usize, +} + +pub(super) fn score_review( + case: &BenchCase, + review: &ReviewOutput, + grade: &GradeOutput, +) -> anyhow::Result { + let expected_ids = case + .expected + .iter() + .map(|expected| expected.id.as_str()) + .collect::>(); + let mut matches = BTreeMap::new(); + for finding_match in &grade.matches { + if !expected_ids.contains(finding_match.expected_id.as_str()) { + bail!( + "grader returned unknown expected finding `{}`", + finding_match.expected_id + ); + } + if matches + .insert(finding_match.expected_id.as_str(), finding_match) + .is_some() + { + bail!( + "grader returned duplicate match for `{}`", + finding_match.expected_id + ); + } + if finding_match.reason.trim().is_empty() { + bail!( + "grader returned an empty reason for `{}`", + finding_match.expected_id + ); + } + if let Some(index) = finding_match.finding_index { + review.findings.get(index).with_context(|| { + format!( + "grader referenced review finding index {index}, but only {} findings exist", + review.findings.len() + ) + })?; + } + } + if matches.len() != expected_ids.len() { + let missing = expected_ids + .into_iter() + .filter(|id| !matches.contains_key(id)) + .collect::>(); + bail!("grader omitted expected finding(s): {}", missing.join(", ")); + } + + let matched_indices = matches + .values() + .filter_map(|finding_match| finding_match.finding_index) + .collect::>(); + Ok(CaseScore { + caught: matches + .values() + .filter(|finding_match| finding_match.finding_index.is_some()) + .count(), + expected: case.expected.len(), + extra_findings: review.findings.len() - matched_indices.len(), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::agent_review_bench::cases::{ExpectedFinding, Severity}; + + #[test] + fn scores_caught_missed_and_extra_findings() { + let case = sample_case(); + let review = ReviewOutput { + summary: "summary".into(), + findings: vec![finding("caught"), finding("extra")], + }; + let grade = GradeOutput { + matches: vec![ + finding_match("first", Some(0)), + finding_match("second", None), + ], + }; + + assert_eq!( + score_review(&case, &review, &grade).unwrap(), + CaseScore { + caught: 1, + expected: 2, + extra_findings: 1, + } + ); + } + + #[test] + fn rejects_unknown_and_out_of_range_matches() { + let case = sample_case(); + let review = ReviewOutput { + summary: "summary".into(), + findings: vec![finding("caught")], + }; + let unknown = GradeOutput { + matches: vec![finding_match("unknown", Some(0))], + }; + assert!(score_review(&case, &review, &unknown).is_err()); + + let out_of_range = GradeOutput { + matches: vec![ + finding_match("first", Some(2)), + finding_match("second", None), + ], + }; + assert!(score_review(&case, &review, &out_of_range).is_err()); + } + + fn sample_case() -> BenchCase { + BenchCase { + id: "0001-sample".into(), + pr: 1, + title: "sample".into(), + remote: "https://example.invalid/repo.git".into(), + base: "a".repeat(40), + head: "b".repeat(40), + source: "source".into(), + fixed_by: "c".repeat(40), + expected: vec![expected("first"), expected("second")], + } + } + + fn expected(id: &str) -> ExpectedFinding { + ExpectedFinding { + id: id.into(), + path: "src/lib.rs".into(), + line: 1, + severity: Severity::Major, + description: "description".into(), + match_if: "criterion".into(), + } + } + + fn finding(title: &str) -> ReviewFinding { + ReviewFinding { + title: title.into(), + body: "body".into(), + path: "src/lib.rs".into(), + line: 1, + severity: "major".into(), + } + } + + fn finding_match(expected_id: &str, finding_index: Option) -> FindingMatch { + FindingMatch { + expected_id: expected_id.into(), + finding_index, + reason: "reason".into(), + } + } +} diff --git a/scripts/axbuild/src/lib.rs b/scripts/axbuild/src/lib.rs index 1630391c76..d18ffe8e21 100644 --- a/scripts/axbuild/src/lib.rs +++ b/scripts/axbuild/src/lib.rs @@ -5,6 +5,7 @@ use clap::{Args, Parser, Subcommand}; use crate::{arceos::ArceOS, axloader::Axloader, axvisor::Axvisor, starry::Starry}; +mod agent_review_bench; pub mod arceos; pub mod axloader; pub mod axvisor; @@ -51,6 +52,11 @@ pub(crate) struct SyncLintArgs { #[derive(Subcommand)] enum Commands { + /// Run offline Codex review benchmarks from historical PR snapshots + AgentReviewBench { + #[command(subcommand)] + command: agent_review_bench::Command, + }, /// Run std tests for the configured workspace package whitelist Test, /// Run kernel axtest targets through QEMU or a remote board @@ -102,6 +108,7 @@ pub async fn run() -> anyhow::Result<()> { async fn run_root_cli(cli: Cli) -> anyhow::Result<()> { match cli.command { + Commands::AgentReviewBench { command } => agent_review_bench::execute(command).await, Commands::Test => test::std::run_std_test_command(), Commands::Ktest(args) => ktest::run(args).await, Commands::Clippy(args) => { From 86407b95e77bcbb11523b69ab525be319d5f7887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E7=9D=BF?= Date: Wed, 15 Jul 2026 13:43:26 +0800 Subject: [PATCH 2/4] feat(axbuild): support Claude Code review benchmarks --- .../axbuild/src/agent_review_bench/codex.rs | 235 --------- scripts/axbuild/src/agent_review_bench/mod.rs | 271 +++++++--- .../axbuild/src/agent_review_bench/runner.rs | 482 ++++++++++++++++++ 3 files changed, 679 insertions(+), 309 deletions(-) delete mode 100644 scripts/axbuild/src/agent_review_bench/codex.rs create mode 100644 scripts/axbuild/src/agent_review_bench/runner.rs diff --git a/scripts/axbuild/src/agent_review_bench/codex.rs b/scripts/axbuild/src/agent_review_bench/codex.rs deleted file mode 100644 index 32f0ff9373..0000000000 --- a/scripts/axbuild/src/agent_review_bench/codex.rs +++ /dev/null @@ -1,235 +0,0 @@ -use std::{ - fs, - path::{Path, PathBuf}, - process::Stdio, - time::Duration, -}; - -use anyhow::{Context, bail}; -use tempfile::tempdir; -use tokio::{process::Command, time::timeout}; - -use super::{cases::BenchCase, sandbox::ReviewSandbox}; - -const REVIEW_PROMPT: &str = - "You are performing an offline code review benchmark. Review only the committed changes from \ - `bench-base` to `HEAD` in the current repository. First read `AGENTS.md`, every file under \ - `book/guideline/`, and `.agent-review-context/reviewer.md`, then follow those rules. Inspect \ - the diff and relevant in-repository context for actionable defects. Do not use the network, \ - GitHub, paths outside this repository, or write operations. Return only the JSON object \ - required by the output schema."; -const GRADE_PROMPT: &str = "You are grading an offline code review. Read `expected.json` and \ - `review.json` in the current directory. For every expected item, \ - decide whether one review finding satisfies its `match_if` criterion \ - at the stated code location. Return exactly one match object per \ - expected ID. Use the zero-based index in `review.findings` when \ - caught, or null when missed. Wording may differ, but nearby or \ - generic comments do not count. Do not inspect any other paths."; -const GRADE_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/grade.schema.json"); - -pub(super) struct CodexOptions { - pub(super) model: Option, - pub(super) reasoning_effort: String, - pub(super) timeout_secs: u64, -} - -pub(super) struct CodexRunner { - program: PathBuf, -} - -impl CodexRunner { - pub(super) fn discover() -> anyhow::Result { - let runner = Self { - program: PathBuf::from("codex"), - }; - runner - .version() - .context("Codex CLI is unavailable or not authenticated for local use")?; - Ok(runner) - } - - pub(super) fn version(&self) -> anyhow::Result { - let output = std::process::Command::new(&self.program) - .arg("--version") - .output() - .with_context(|| format!("failed to execute {} --version", self.program.display()))?; - if !output.status.success() { - bail!( - "{} --version exited with {}", - self.program.display(), - output.status - ); - } - let version = String::from_utf8(output.stdout).context("Codex version was not UTF-8")?; - let version = version.trim(); - if version.is_empty() { - bail!("Codex CLI returned an empty version"); - } - Ok(version.to_string()) - } - - pub(super) async fn review( - &self, - sandbox: &ReviewSandbox, - artifact_path: &Path, - options: &CodexOptions, - ) -> anyhow::Result<()> { - let temporary_output = sandbox.temporary_review_output(); - remove_if_exists(&temporary_output)?; - let mut command = Command::new(&self.program); - command - .current_dir(sandbox.repo()) - .args(["exec", "--ephemeral", "--sandbox", "read-only"]) - .args(["-c", "approval_policy=\"never\""]) - .args(["-c", "sandbox_workspace_write.network_access=false"]) - .arg("--output-schema") - .arg(sandbox.review_schema()) - .arg("--output-last-message") - .arg(&temporary_output); - apply_model_options(&mut command, options); - command.arg(REVIEW_PROMPT); - - run_process(&mut command, options.timeout_secs, "Codex reviewer").await?; - copy_nonempty_output(&temporary_output, artifact_path, "reviewer") - } - - pub(super) async fn grade( - &self, - case: &BenchCase, - review_path: &Path, - artifact_path: &Path, - options: &CodexOptions, - ) -> anyhow::Result<()> { - let grader_dir = tempdir().context("failed to create isolated grader directory")?; - let grader_review = grader_dir.path().join("review.json"); - fs::copy(review_path, &grader_review)?; - let expected = serde_json::to_string_pretty(&case.expected)?; - fs::write(grader_dir.path().join("expected.json"), expected)?; - let schema_path = grader_dir.path().join("grade.schema.json"); - fs::write(&schema_path, GRADE_SCHEMA)?; - let temporary_output = grader_dir.path().join("grade.json"); - - let mut command = Command::new(&self.program); - command - .args(["exec", "--skip-git-repo-check", "--ephemeral"]) - .args(["--sandbox", "read-only"]) - .args(["-c", "approval_policy=\"never\""]) - .args(["-c", "sandbox_workspace_write.network_access=false"]) - .arg("--cd") - .arg(grader_dir.path()) - .arg("--output-schema") - .arg(&schema_path) - .arg("--output-last-message") - .arg(&temporary_output); - apply_model_options(&mut command, options); - command.arg(GRADE_PROMPT); - - run_process(&mut command, options.timeout_secs, "Codex grader").await?; - copy_nonempty_output(&temporary_output, artifact_path, "grader") - } - - #[cfg(test)] - pub(super) fn from_program(program: PathBuf) -> Self { - Self { program } - } -} - -fn apply_model_options(command: &mut Command, options: &CodexOptions) { - if let Some(model) = &options.model { - command.args(["--model", model]); - } - command.args([ - "-c", - &format!("model_reasoning_effort=\"{}\"", options.reasoning_effort), - ]); -} - -async fn run_process( - command: &mut Command, - timeout_secs: u64, - description: &str, -) -> anyhow::Result<()> { - command - .stdin(Stdio::null()) - .stdout(Stdio::inherit()) - .stderr(Stdio::inherit()) - .kill_on_drop(true); - let mut child = command - .spawn() - .with_context(|| format!("failed to spawn {description}"))?; - let status = match timeout(Duration::from_secs(timeout_secs), child.wait()).await { - Ok(status) => status.with_context(|| format!("failed to wait for {description}"))?, - Err(_) => { - child - .kill() - .await - .with_context(|| format!("failed to terminate timed-out {description}"))?; - let _ = child.wait().await; - bail!("{description} timed out after {timeout_secs} seconds"); - } - }; - if status.success() { - Ok(()) - } else { - bail!("{description} exited with status {status}") - } -} - -fn copy_nonempty_output(source: &Path, destination: &Path, role: &str) -> anyhow::Result<()> { - let metadata = fs::metadata(source) - .with_context(|| format!("{role} did not create {}", source.display()))?; - if metadata.len() == 0 { - bail!("{role} created an empty output file"); - } - fs::copy(source, destination).with_context(|| { - format!( - "failed to copy {role} output from {} to {}", - source.display(), - destination.display() - ) - })?; - Ok(()) -} - -fn remove_if_exists(path: &Path) -> anyhow::Result<()> { - match fs::remove_file(path) { - Ok(()) => Ok(()), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(err) => Err(err).with_context(|| format!("failed to remove {}", path.display())), - } -} - -#[cfg(test)] -mod tests { - use std::{fs, os::unix::fs::PermissionsExt}; - - use tempfile::tempdir; - - use super::*; - - #[test] - fn reads_version_from_injected_codex_program() { - let temp = tempdir().unwrap(); - let program = temp.path().join("mock-codex"); - fs::write(&program, "#!/bin/sh\necho 'codex-cli test-version'\n").unwrap(); - fs::set_permissions(&program, fs::Permissions::from_mode(0o755)).unwrap(); - - let runner = CodexRunner::from_program(program); - assert_eq!(runner.version().unwrap(), "codex-cli test-version"); - } - - #[tokio::test] - async fn reports_nonzero_and_timeout_processes() { - let mut failure = Command::new("sh"); - failure.args(["-c", "exit 7"]); - assert!(run_process(&mut failure, 1, "mock failure").await.is_err()); - - let mut slow = Command::new("sh"); - slow.args(["-c", "sleep 2"]); - let error = run_process(&mut slow, 1, "mock timeout") - .await - .unwrap_err() - .to_string(); - assert!(error.contains("timed out")); - } -} diff --git a/scripts/axbuild/src/agent_review_bench/mod.rs b/scripts/axbuild/src/agent_review_bench/mod.rs index 8aebc5b5c3..4bfaa011d2 100644 --- a/scripts/axbuild/src/agent_review_bench/mod.rs +++ b/scripts/axbuild/src/agent_review_bench/mod.rs @@ -4,18 +4,18 @@ use std::{fs, path::PathBuf, time::Instant}; use anyhow::{Context, bail}; use chrono::Utc; -use clap::{Args, Subcommand, ValueEnum}; +use clap::{Args, Subcommand}; use serde::Serialize; use crate::context; mod cases; -mod codex; +mod runner; mod sandbox; mod scoring; use cases::{BenchCase, load_cases, prepare_case, select_cases}; -use codex::{CodexOptions, CodexRunner}; +use runner::{AgentKind, AgentOptions, AgentRunner}; use sandbox::ReviewSandbox; use scoring::{CaseScore, GradeOutput, ReviewOutput, score_review}; @@ -25,7 +25,7 @@ const DEFAULT_TIMEOUT_SECS: u64 = 1800; pub(crate) enum Command { /// List configured benchmark cases without fetching commits List, - /// Validate cases and their historical Git snapshots without invoking Codex + /// Validate cases and their historical Git snapshots without invoking an agent CLI Check, /// Review and grade selected cases sequentially Run(RunArgs), @@ -39,12 +39,24 @@ pub(crate) struct RunArgs { /// Select every case sourced from a PR; repeat to select more PRs #[arg(long = "pr", value_name = "NUMBER")] prs: Vec, - /// Override the Codex model; omit to inherit the user's Codex configuration + /// CLI used for review + #[arg(long, value_enum, default_value_t = AgentKind::Codex)] + agent: AgentKind, + /// Model passed unchanged to the reviewer CLI; omit to inherit its configuration #[arg(long)] model: Option, - /// Reasoning effort used by both reviewer and grader - #[arg(long, value_enum, default_value_t = ReasoningEffort::High)] - reasoning_effort: ReasoningEffort, + /// Reasoning effort passed unchanged to the reviewer CLI + #[arg(long, default_value = "high")] + reasoning_effort: String, + /// CLI used for grading; defaults to the reviewer CLI + #[arg(long, value_enum)] + grader_agent: Option, + /// Model passed unchanged to the grader CLI; defaults to --model + #[arg(long)] + grader_model: Option, + /// Reasoning effort passed unchanged to the grader CLI; defaults to reviewer effort + #[arg(long)] + grader_reasoning_effort: Option, /// Timeout for each reviewer or grader invocation #[arg(long, default_value_t = DEFAULT_TIMEOUT_SECS)] timeout_secs: u64, @@ -56,34 +68,11 @@ pub(crate) struct RunArgs { output: Option, } -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, ValueEnum)] -enum ReasoningEffort { - Minimal, - Low, - Medium, - #[default] - High, - Xhigh, -} - -impl ReasoningEffort { - fn as_str(self) -> &'static str { - match self { - Self::Minimal => "minimal", - Self::Low => "low", - Self::Medium => "medium", - Self::High => "high", - Self::Xhigh => "xhigh", - } - } -} - #[derive(Debug, Serialize)] struct RunSummary { generated_at: String, - codex_version: String, - model: Option, - reasoning_effort: String, + reviewer: AgentConfiguration, + grader: AgentConfiguration, timeout_secs: u64, min_recall: Option, caught: usize, @@ -93,6 +82,14 @@ struct RunSummary { cases: Vec, } +#[derive(Debug, Serialize)] +struct AgentConfiguration { + agent: String, + version: String, + model: Option, + reasoning_effort: String, +} + #[derive(Debug, Serialize)] struct CaseResult { case_id: String, @@ -144,22 +141,25 @@ async fn run_cases( all_cases: &[BenchCase], args: RunArgs, ) -> anyhow::Result<()> { - let runner = CodexRunner::discover()?; - run_cases_with_runner(workspace_root, all_cases, args, &runner).await + if args.timeout_secs == 0 { + bail!("--timeout-secs must be greater than zero"); + } + let grader_kind = args.grader_agent.unwrap_or(args.agent); + let reviewer = AgentRunner::discover(args.agent)?; + let grader = AgentRunner::discover(grader_kind)?; + run_cases_with_runners(workspace_root, all_cases, args, &reviewer, &grader).await } -async fn run_cases_with_runner( +async fn run_cases_with_runners( workspace_root: &std::path::Path, all_cases: &[BenchCase], args: RunArgs, - runner: &CodexRunner, + reviewer: &AgentRunner, + grader: &AgentRunner, ) -> anyhow::Result<()> { if args.timeout_secs == 0 { bail!("--timeout-secs must be greater than zero"); } - if args.model.as_deref().is_some_and(str::is_empty) { - bail!("--model must not be empty"); - } let selected = select_cases(all_cases, &args.cases, &args.prs)?; let output_dir = resolve_output_dir(workspace_root, args.output.as_deref()); @@ -170,12 +170,9 @@ async fn run_cases_with_runner( ) })?; - let codex_version = runner.version()?; - let options = CodexOptions { - model: args.model.clone(), - reasoning_effort: args.reasoning_effort.as_str().to_string(), - timeout_secs: args.timeout_secs, - }; + let (reviewer_options, grader_options) = resolve_agent_options(&args); + let reviewer_version = reviewer.version()?; + let grader_version = grader.version()?; let mut results = Vec::with_capacity(selected.len()); for case in selected { @@ -187,8 +184,8 @@ async fn run_cases_with_runner( let sandbox = ReviewSandbox::create(workspace_root, case)?; let review_path = case_dir.join("review.json"); let review_started = Instant::now(); - runner - .review(&sandbox, &review_path, &options) + reviewer + .review(&sandbox, &review_path, &reviewer_options) .await .with_context(|| format!("reviewer failed for `{}`", case.id))?; let review_seconds = review_started.elapsed().as_secs_f64(); @@ -196,8 +193,8 @@ async fn run_cases_with_runner( let grade_path = case_dir.join("grade.json"); let grade_started = Instant::now(); - runner - .grade(case, &review_path, &grade_path, &options) + grader + .grade(case, &review_path, &grade_path, &grader_options) .await .with_context(|| format!("grader failed for `{}`", case.id))?; let grade_seconds = grade_started.elapsed().as_secs_f64(); @@ -212,7 +209,16 @@ async fn run_cases_with_runner( results.push(result); } - let summary = summarize(&codex_version, &options, args.min_recall, results); + let reviewer_configuration = + agent_configuration(reviewer, &reviewer_version, &reviewer_options); + let grader_configuration = agent_configuration(grader, &grader_version, &grader_options); + let summary = summarize( + reviewer_configuration, + grader_configuration, + args.timeout_secs, + args.min_recall, + results, + ); write_json(&output_dir.join("summary.json"), &summary)?; println!( "agent-review recall: {}/{} ({:.1}%); extra findings: {}; artifacts: {}", @@ -236,6 +242,23 @@ async fn run_cases_with_runner( Ok(()) } +fn resolve_agent_options(args: &RunArgs) -> (AgentOptions, AgentOptions) { + let reviewer = AgentOptions { + model: args.model.clone(), + reasoning_effort: args.reasoning_effort.clone(), + timeout_secs: args.timeout_secs, + }; + let grader = AgentOptions { + model: args.grader_model.clone().or_else(|| args.model.clone()), + reasoning_effort: args + .grader_reasoning_effort + .clone() + .unwrap_or_else(|| args.reasoning_effort.clone()), + timeout_secs: args.timeout_secs, + }; + (reviewer, grader) +} + fn resolve_output_dir( workspace_root: &std::path::Path, output: Option<&std::path::Path>, @@ -272,8 +295,9 @@ fn case_result( } fn summarize( - codex_version: &str, - options: &CodexOptions, + reviewer: AgentConfiguration, + grader: AgentConfiguration, + timeout_secs: u64, min_recall: Option, cases: Vec, ) -> RunSummary { @@ -282,10 +306,9 @@ fn summarize( let extra_findings = cases.iter().map(|case| case.extra_findings).sum(); RunSummary { generated_at: Utc::now().to_rfc3339(), - codex_version: codex_version.to_string(), - model: options.model.clone(), - reasoning_effort: options.reasoning_effort.clone(), - timeout_secs: options.timeout_secs, + reviewer, + grader, + timeout_secs, min_recall, caught, expected, @@ -295,6 +318,19 @@ fn summarize( } } +fn agent_configuration( + runner: &AgentRunner, + version: &str, + options: &AgentOptions, +) -> AgentConfiguration { + AgentConfiguration { + agent: runner.kind().as_str().to_string(), + version: version.to_string(), + model: options.model.clone(), + reasoning_effort: options.reasoning_effort.clone(), + } +} + fn recall_gate_failed(recall_percent: f64, min_recall: Option) -> bool { min_recall.is_some_and(|minimum| recall_percent < f64::from(minimum)) } @@ -349,8 +385,18 @@ mod tests { "1495", "--min-recall", "80", + "--agent", + "claude", + "--model", + "model with spaces", "--reasoning-effort", - "medium", + "vendor effort", + "--grader-agent", + "codex", + "--grader-model", + "grader \"model\"", + "--grader-reasoning-effort", + "grader effort", ]) .unwrap(); @@ -360,7 +406,30 @@ mod tests { assert_eq!(args.cases, ["case-a", "case-b"]); assert_eq!(args.prs, [1495]); assert_eq!(args.min_recall, Some(80)); - assert_eq!(args.reasoning_effort, ReasoningEffort::Medium); + assert_eq!(args.agent, AgentKind::Claude); + assert_eq!(args.model.as_deref(), Some("model with spaces")); + assert_eq!(args.reasoning_effort, "vendor effort"); + assert_eq!(args.grader_agent, Some(AgentKind::Codex)); + assert_eq!(args.grader_model.as_deref(), Some("grader \"model\"")); + assert_eq!( + args.grader_reasoning_effort.as_deref(), + Some("grader effort") + ); + } + + #[test] + fn parses_default_agent_configuration() { + let cli = TestCli::try_parse_from(["bench", "run"]).unwrap(); + let Command::Run(args) = cli.command else { + panic!("expected run command"); + }; + + assert_eq!(args.agent, AgentKind::Codex); + assert_eq!(args.model, None); + assert_eq!(args.reasoning_effort, "high"); + assert_eq!(args.grader_agent, None); + assert_eq!(args.grader_model, None); + assert_eq!(args.grader_reasoning_effort, None); } #[test] @@ -376,22 +445,45 @@ mod tests { assert!(recall_gate_failed(79.9, Some(80))); } + #[test] + fn grader_options_inherit_reviewer_values() { + let args = RunArgs { + cases: Vec::new(), + prs: Vec::new(), + agent: AgentKind::Claude, + model: Some(String::new()), + reasoning_effort: "effort with spaces".into(), + grader_agent: None, + grader_model: None, + grader_reasoning_effort: None, + timeout_secs: 7, + min_recall: None, + output: None, + }; + + let (reviewer, grader) = resolve_agent_options(&args); + assert_eq!(reviewer, grader); + assert_eq!(grader.model.as_deref(), Some("")); + assert_eq!(grader.reasoning_effort, "effort with spaces"); + } + #[cfg(unix)] #[tokio::test] - async fn mock_codex_writes_artifacts_and_rejects_invalid_json() { + async fn mock_agents_write_artifacts_and_reject_invalid_json() { let workspace = tempfile::tempdir().unwrap(); let case = create_test_case(workspace.path()); let output = workspace.path().join("artifacts"); let program = workspace.path().join("mock-codex"); - write_mock_codex( + write_mock_agent( &program, r#"{"summary":"found one issue","findings":[{"title":"caught","body":"body","path":"src/lib.rs","line":1,"severity":"major"},{"title":"extra","body":"body","path":"src/lib.rs","line":1,"severity":"minor"}]}"#, r#"{"matches":[{"expected_id":"sample-finding","finding_index":0,"reason":"same defect"}]}"#, ); - let runner = CodexRunner::from_program(program.clone()); + let reviewer = AgentRunner::from_program(AgentKind::Claude, program.clone()); + let grader = AgentRunner::from_program(AgentKind::Codex, program.clone()); let args = test_run_args(output.clone()); - run_cases_with_runner(workspace.path(), &[case.clone()], args, &runner) + run_cases_with_runners(workspace.path(), &[case.clone()], args, &reviewer, &grader) .await .unwrap(); @@ -412,24 +504,50 @@ mod tests { assert_eq!(summary["caught"], 1); assert_eq!(summary["expected"], 1); assert_eq!(summary["extra_findings"], 1); - assert_eq!(summary["timeout_secs"], 2); + assert_eq!(summary["timeout_secs"], 30); + assert_eq!(summary["reviewer"]["agent"], "claude"); + assert_eq!(summary["reviewer"]["model"], "mock model"); + assert_eq!(summary["reviewer"]["reasoning_effort"], "custom effort"); + assert_eq!(summary["grader"]["agent"], "codex"); + assert_eq!(summary["grader"]["model"], "grader model"); + assert_eq!(summary["grader"]["reasoning_effort"], "grader effort"); + + let reverse_output = workspace.path().join("reverse-artifacts"); + let mut reverse_args = test_run_args(reverse_output.clone()); + reverse_args.agent = AgentKind::Codex; + reverse_args.grader_agent = Some(AgentKind::Claude); + run_cases_with_runners( + workspace.path(), + &[case.clone()], + reverse_args, + &grader, + &reviewer, + ) + .await + .unwrap(); + let reverse_summary = + read_json::(&reverse_output.join("summary.json")).unwrap(); + assert_eq!(reverse_summary["reviewer"]["agent"], "codex"); + assert_eq!(reverse_summary["grader"]["agent"], "claude"); + assert_eq!(reverse_summary["caught"], 1); - write_mock_codex( + write_mock_agent( &program, "not JSON", r#"{"matches":[{"expected_id":"sample-finding","finding_index":null,"reason":"missed"}]}"#, ); let invalid_output = workspace.path().join("invalid-artifacts"); - let error = run_cases_with_runner( + let error = run_cases_with_runners( workspace.path(), &[case], test_run_args(invalid_output), - &runner, + &reviewer, + &grader, ) .await .unwrap_err() .to_string(); - assert!(error.contains("invalid JSON")); + assert!(error.contains("invalid JSON"), "unexpected error: {error}"); } #[cfg(unix)] @@ -485,23 +603,28 @@ mod tests { RunArgs { cases: Vec::new(), prs: Vec::new(), - model: Some("mock-model".into()), - reasoning_effort: ReasoningEffort::High, - timeout_secs: 2, + agent: AgentKind::Claude, + model: Some("mock model".into()), + reasoning_effort: "custom effort".into(), + grader_agent: Some(AgentKind::Codex), + grader_model: Some("grader model".into()), + grader_reasoning_effort: Some("grader effort".into()), + timeout_secs: 30, min_recall: None, output: Some(output), } } #[cfg(unix)] - fn write_mock_codex(program: &Path, review: &str, grade: &str) { + fn write_mock_agent(program: &Path, review: &str, grade: &str) { let script = format!( "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo 'codex-cli mock'; exit 0; \ fi\nmode=review\nif [ -f expected.json ]; then mode=grade; fi\noutput=\nwhile [ \ \"$#\" -gt 0 ]; do\ncase \"$1\" in\n--cd) shift; mode=grade \ ;;\n--output-last-message|-o) shift; output=$1 ;;\nesac\nshift\ndone\nif [ \"$mode\" \ - = review ]; then printf '%s\\n' '{review}' > \"$output\"; else printf '%s\\n' \ - '{grade}' > \"$output\"; fi\n" + = review ]; then payload='{review}'; else payload='{grade}'; fi\nif [ -n \"$output\" \ + ]; then printf '%s\\n' \"$payload\" > \"$output\"; else printf '%s\\n' \"$payload\"; \ + fi\n" ); fs::write(program, script).unwrap(); fs::set_permissions(program, fs::Permissions::from_mode(0o755)).unwrap(); diff --git a/scripts/axbuild/src/agent_review_bench/runner.rs b/scripts/axbuild/src/agent_review_bench/runner.rs new file mode 100644 index 0000000000..643be0036a --- /dev/null +++ b/scripts/axbuild/src/agent_review_bench/runner.rs @@ -0,0 +1,482 @@ +use std::{ + fs, + path::{Path, PathBuf}, + process::Stdio, + time::Duration, +}; + +use anyhow::{Context, bail}; +use clap::ValueEnum; +use tempfile::tempdir; +use tokio::{process::Command, time::timeout}; + +use super::{cases::BenchCase, sandbox::ReviewSandbox}; + +const REVIEW_PROMPT: &str = + "You are performing an offline code review benchmark. Review only the committed changes from \ + `bench-base` to `HEAD` in the current repository. First read `AGENTS.md`, every file under \ + `book/guideline/`, and `.agent-review-context/reviewer.md`, then follow those rules. Inspect \ + the diff and relevant in-repository context for actionable defects. Do not use the network, \ + GitHub, paths outside this repository, or write operations. Return only the JSON object \ + required by the output schema."; +const GRADE_PROMPT: &str = "You are grading an offline code review. Read `expected.json` and \ + `review.json` in the current directory. For every expected item, \ + decide whether one review finding satisfies its `match_if` criterion \ + at the stated code location. Return exactly one match object per \ + expected ID. Use the zero-based index in `review.findings` when \ + caught, or null when missed. Wording may differ, but nearby or \ + generic comments do not count. Do not inspect any other paths."; +const GRADE_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/grade.schema.json"); + +const CLAUDE_COMMON_ARGS: &[&str] = &[ + "-p", + "--safe-mode", + "--no-session-persistence", + "--strict-mcp-config", + "--mcp-config", + r#"{"mcpServers":{}}"#, + "--no-chrome", + "--permission-mode", + "dontAsk", +]; +const CLAUDE_REVIEW_TOOLS: &str = "Bash,Read,Glob,Grep"; +const CLAUDE_REVIEW_ALLOWED_TOOLS: &[&str] = &[ + "Read", + "Glob", + "Grep", + "Bash(git diff *)", + "Bash(git show *)", + "Bash(git status *)", + "Bash(git log *)", + "Bash(git grep *)", +]; +const CLAUDE_GRADE_TOOLS: &str = "Read"; + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, ValueEnum)] +pub(super) enum AgentKind { + #[default] + Codex, + Claude, +} + +impl AgentKind { + pub(super) fn as_str(self) -> &'static str { + match self { + Self::Codex => "codex", + Self::Claude => "claude", + } + } + + fn program(self) -> &'static str { + self.as_str() + } +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct AgentOptions { + pub(super) model: Option, + pub(super) reasoning_effort: String, + pub(super) timeout_secs: u64, +} + +pub(super) struct AgentRunner { + kind: AgentKind, + program: PathBuf, +} + +impl AgentRunner { + pub(super) fn discover(kind: AgentKind) -> anyhow::Result { + let runner = Self { + kind, + program: PathBuf::from(kind.program()), + }; + runner + .version() + .with_context(|| format!("{} CLI is unavailable", kind.as_str()))?; + Ok(runner) + } + + pub(super) fn kind(&self) -> AgentKind { + self.kind + } + + pub(super) fn version(&self) -> anyhow::Result { + let output = std::process::Command::new(&self.program) + .arg("--version") + .output() + .with_context(|| format!("failed to execute {} --version", self.program.display()))?; + if !output.status.success() { + bail!( + "{} --version exited with {}", + self.program.display(), + output.status + ); + } + let version = String::from_utf8(output.stdout).context("agent version was not UTF-8")?; + let version = version.trim(); + if version.is_empty() { + bail!("{} CLI returned an empty version", self.kind.as_str()); + } + Ok(version.to_string()) + } + + pub(super) async fn review( + &self, + sandbox: &ReviewSandbox, + artifact_path: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + match self.kind { + AgentKind::Codex => { + self.review_with_codex(sandbox, artifact_path, options) + .await + } + AgentKind::Claude => { + self.review_with_claude(sandbox, artifact_path, options) + .await + } + } + } + + pub(super) async fn grade( + &self, + case: &BenchCase, + review_path: &Path, + artifact_path: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + let grader_dir = tempdir().context("failed to create isolated grader directory")?; + fs::copy(review_path, grader_dir.path().join("review.json"))?; + let expected = serde_json::to_string_pretty(&case.expected)?; + fs::write(grader_dir.path().join("expected.json"), expected)?; + let schema_path = grader_dir.path().join("grade.schema.json"); + fs::write(&schema_path, GRADE_SCHEMA)?; + let temporary_output = grader_dir.path().join("grade.json"); + + match self.kind { + AgentKind::Codex => { + self.grade_with_codex(grader_dir.path(), &schema_path, &temporary_output, options) + .await?; + } + AgentKind::Claude => { + self.grade_with_claude(grader_dir.path(), &temporary_output, options) + .await?; + } + } + copy_nonempty_output(&temporary_output, artifact_path, "grader") + } + + async fn review_with_codex( + &self, + sandbox: &ReviewSandbox, + artifact_path: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + let temporary_output = sandbox.temporary_review_output(); + remove_if_exists(&temporary_output)?; + let mut command = Command::new(&self.program); + command + .current_dir(sandbox.repo()) + .args(["exec", "--ephemeral", "--sandbox", "read-only"]) + .args(["-c", "approval_policy=\"never\""]) + .args(["-c", "sandbox_workspace_write.network_access=false"]) + .arg("--output-schema") + .arg(sandbox.review_schema()) + .arg("--output-last-message") + .arg(&temporary_output) + .stdout(Stdio::inherit()); + apply_codex_options(&mut command, options); + command.arg(REVIEW_PROMPT); + + run_process(&mut command, options.timeout_secs, "Codex reviewer").await?; + copy_nonempty_output(&temporary_output, artifact_path, "reviewer") + } + + async fn review_with_claude( + &self, + sandbox: &ReviewSandbox, + artifact_path: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + let temporary_output = sandbox.temporary_review_output(); + remove_if_exists(&temporary_output)?; + let output = fs::File::create(&temporary_output)?; + let schema = claude_schema(&fs::read_to_string(sandbox.review_schema())?)?; + let mut command = Command::new(&self.program); + command + .current_dir(sandbox.repo()) + .args(CLAUDE_COMMON_ARGS) + .args(["--tools", CLAUDE_REVIEW_TOOLS]) + .arg("--allowedTools") + .args(CLAUDE_REVIEW_ALLOWED_TOOLS) + .args(["--json-schema", &schema]) + .stdout(Stdio::from(output)); + apply_claude_options(&mut command, options); + command.arg(REVIEW_PROMPT); + + run_process(&mut command, options.timeout_secs, "Claude reviewer").await?; + copy_nonempty_output(&temporary_output, artifact_path, "reviewer") + } + + async fn grade_with_codex( + &self, + grader_dir: &Path, + schema_path: &Path, + temporary_output: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + let mut command = Command::new(&self.program); + command + .args(["exec", "--skip-git-repo-check", "--ephemeral"]) + .args(["--sandbox", "read-only"]) + .args(["-c", "approval_policy=\"never\""]) + .args(["-c", "sandbox_workspace_write.network_access=false"]) + .arg("--cd") + .arg(grader_dir) + .arg("--output-schema") + .arg(schema_path) + .arg("--output-last-message") + .arg(temporary_output) + .stdout(Stdio::inherit()); + apply_codex_options(&mut command, options); + command.arg(GRADE_PROMPT); + + run_process(&mut command, options.timeout_secs, "Codex grader").await + } + + async fn grade_with_claude( + &self, + grader_dir: &Path, + temporary_output: &Path, + options: &AgentOptions, + ) -> anyhow::Result<()> { + let output = fs::File::create(temporary_output)?; + let schema = claude_schema(GRADE_SCHEMA)?; + let mut command = Command::new(&self.program); + command + .current_dir(grader_dir) + .args(CLAUDE_COMMON_ARGS) + .args(["--tools", CLAUDE_GRADE_TOOLS]) + .args(["--allowedTools", "Read"]) + .args(["--json-schema", &schema]) + .stdout(Stdio::from(output)); + apply_claude_options(&mut command, options); + command.arg(GRADE_PROMPT); + + run_process(&mut command, options.timeout_secs, "Claude grader").await + } + + #[cfg(test)] + pub(super) fn from_program(kind: AgentKind, program: PathBuf) -> Self { + Self { kind, program } + } +} + +fn apply_codex_options(command: &mut Command, options: &AgentOptions) { + if let Some(model) = &options.model { + command.args(["--model", model]); + } + let effort = toml::Value::String(options.reasoning_effort.clone()).to_string(); + command.args(["-c", &format!("model_reasoning_effort={effort}")]); +} + +fn apply_claude_options(command: &mut Command, options: &AgentOptions) { + if let Some(model) = &options.model { + command.args(["--model", model]); + } + command.args(["--effort", &options.reasoning_effort]); +} + +fn claude_schema(schema: &str) -> anyhow::Result { + let mut schema = serde_json::from_str::(schema) + .context("invalid JSON Schema supplied to Claude")?; + let object = schema + .as_object_mut() + .context("Claude JSON Schema must be an object")?; + object.remove("$schema"); + serde_json::to_string(&schema).context("failed to serialize Claude JSON Schema") +} + +async fn run_process( + command: &mut Command, + timeout_secs: u64, + description: &str, +) -> anyhow::Result<()> { + command + .stdin(Stdio::null()) + .stderr(Stdio::inherit()) + .kill_on_drop(true); + let mut child = command + .spawn() + .with_context(|| format!("failed to spawn {description}"))?; + let status = match timeout(Duration::from_secs(timeout_secs), child.wait()).await { + Ok(status) => status.with_context(|| format!("failed to wait for {description}"))?, + Err(_) => { + child + .kill() + .await + .with_context(|| format!("failed to terminate timed-out {description}"))?; + let _ = child.wait().await; + bail!("{description} timed out after {timeout_secs} seconds"); + } + }; + if status.success() { + Ok(()) + } else { + bail!("{description} exited with status {status}") + } +} + +fn copy_nonempty_output(source: &Path, destination: &Path, role: &str) -> anyhow::Result<()> { + let metadata = fs::metadata(source) + .with_context(|| format!("{role} did not create {}", source.display()))?; + if metadata.len() == 0 { + bail!("{role} created an empty output file"); + } + fs::copy(source, destination).with_context(|| { + format!( + "failed to copy {role} output from {} to {}", + source.display(), + destination.display() + ) + })?; + Ok(()) +} + +fn remove_if_exists(path: &Path) -> anyhow::Result<()> { + match fs::remove_file(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err).with_context(|| format!("failed to remove {}", path.display())), + } +} + +#[cfg(all(test, unix))] +mod tests { + use std::{ffi::OsStr, fs, os::unix::fs::PermissionsExt}; + + use tempfile::tempdir; + + use super::*; + + #[test] + fn reads_version_from_injected_agent_program() { + let temp = tempdir().unwrap(); + let program = temp.path().join("mock-agent"); + fs::write(&program, "#!/bin/sh\necho 'agent test-version'\n").unwrap(); + fs::set_permissions(&program, fs::Permissions::from_mode(0o755)).unwrap(); + + let runner = AgentRunner::from_program(AgentKind::Claude, program); + assert_eq!(runner.version().unwrap(), "agent test-version"); + } + + #[test] + fn model_and_effort_are_passed_as_single_argv_values() { + let options = AgentOptions { + model: Some("model with \"quotes\"; $(touch ignored)".into()), + reasoning_effort: "vendor effort 'x' $HOME\\next".into(), + timeout_secs: 1, + }; + + let mut codex = Command::new("codex"); + apply_codex_options(&mut codex, &options); + let codex_args = args(&codex); + assert_eq!( + codex_args[0..2], + ["--model", "model with \"quotes\"; $(touch ignored)"] + ); + assert_eq!(codex_args[2], "-c"); + let config = codex_args[3] + .strip_prefix("model_reasoning_effort=") + .unwrap(); + assert_eq!( + config.parse::().unwrap(), + toml::Value::String("vendor effort 'x' $HOME\\next".into()) + ); + + let mut claude = Command::new("claude"); + apply_claude_options(&mut claude, &options); + assert_eq!( + args(&claude), + [ + "--model", + "model with \"quotes\"; $(touch ignored)", + "--effort", + "vendor effort 'x' $HOME\\next" + ] + ); + + let empty = AgentOptions { + model: Some(String::new()), + reasoning_effort: String::new(), + timeout_secs: 1, + }; + let mut claude = Command::new("claude"); + apply_claude_options(&mut claude, &empty); + assert_eq!(args(&claude), ["--model", "", "--effort", ""]); + + let omitted = AgentOptions { + model: None, + reasoning_effort: "high".into(), + timeout_secs: 1, + }; + let mut claude = Command::new("claude"); + apply_claude_options(&mut claude, &omitted); + assert_eq!(args(&claude), ["--effort", "high"]); + } + + #[test] + fn claude_tool_sets_are_read_only_and_offline() { + assert_eq!(CLAUDE_GRADE_TOOLS, "Read"); + assert!( + CLAUDE_REVIEW_TOOLS + .split(',') + .all(|tool| matches!(tool, "Bash" | "Read" | "Glob" | "Grep")) + ); + assert!(CLAUDE_REVIEW_ALLOWED_TOOLS.iter().all(|tool| { + !tool.contains("Write") + && !tool.contains("Edit") + && !tool.contains("WebFetch") + && !tool.contains("WebSearch") + })); + assert!(CLAUDE_COMMON_ARGS.contains(&"--safe-mode")); + assert!(CLAUDE_COMMON_ARGS.contains(&"--strict-mcp-config")); + assert!(CLAUDE_COMMON_ARGS.contains(&r#"{"mcpServers":{}}"#)); + assert!(CLAUDE_COMMON_ARGS.contains(&"--no-chrome")); + } + + #[test] + fn claude_schema_removes_unsupported_draft_metadata() { + let schema = claude_schema( + r#"{"$schema":"https://json-schema.org/draft/2020-12/schema","type":"object"}"#, + ) + .unwrap(); + let schema = serde_json::from_str::(&schema).unwrap(); + assert_eq!(schema["type"], "object"); + assert!(schema.get("$schema").is_none()); + } + + #[tokio::test] + async fn reports_nonzero_and_timeout_processes() { + let mut failure = Command::new("sh"); + failure.args(["-c", "exit 7"]); + assert!(run_process(&mut failure, 1, "mock failure").await.is_err()); + + let mut slow = Command::new("sh"); + slow.args(["-c", "sleep 2"]); + let error = run_process(&mut slow, 1, "mock timeout") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("timed out")); + } + + fn args(command: &Command) -> Vec<&str> { + command + .as_std() + .get_args() + .map(OsStr::to_str) + .collect::>>() + .unwrap() + } +} From 31fdb88481676a19f366355ad8d408b6487704a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E7=9D=BF?= Date: Wed, 15 Jul 2026 14:17:53 +0800 Subject: [PATCH 3/4] refactor(axbuild): invoke review bench through project skill --- .claude/skills/review-single-pr/SKILL.md | 26 ++++++++ scripts/agent-review-bench/reviewer.md | 23 +++---- scripts/axbuild/src/agent_review_bench/mod.rs | 24 +++++-- .../axbuild/src/agent_review_bench/runner.rs | 51 ++++++++++---- .../axbuild/src/agent_review_bench/sandbox.rs | 66 +++++++++++++++++++ 5 files changed, 158 insertions(+), 32 deletions(-) diff --git a/.claude/skills/review-single-pr/SKILL.md b/.claude/skills/review-single-pr/SKILL.md index 07d72c563e..663b2f36b7 100644 --- a/.claude/skills/review-single-pr/SKILL.md +++ b/.claude/skills/review-single-pr/SKILL.md @@ -17,6 +17,32 @@ After reading the full skill and all files under `book/guideline/`, create a rev When requirements overlap, apply the stricter rule. If skipping a requirement is necessary because it is inapplicable or impossible, record the concrete reason and evidence in the review body or user summary. +## Offline Benchmark Mode + +Use this mode only when the skill is invoked with the exact `offline-benchmark` argument and the +repository contains `.agent-review-context/reviewer.md`. The context file is the authoritative +environment and output contract for this mode. If either condition is absent, follow the normal +online workflow in the rest of this skill. + +In offline benchmark mode, treat the committed change from `bench-base` to `HEAD` as the one change +under review. Read the complete skill, `AGENTS.md`, every file under `book/guideline/`, the offline +contract, and the supplied output schema before judging the change. Apply the review-focus, +test-quality, blocking-finding, hardware/ABI, security/soundness, maintainability, and documentation +requirements from this skill to the local diff and relevant in-repository context. + +The benchmark repository intentionally has no live PR identity. Mark PR metadata and intake, review +threads, remote CI, related-open-PR searches, worktree creation, merge-conflict repair, network +semantic-source research, command execution for validation, GitHub review submission, reviewer +assignment, and remote cleanup as not applicable. Do not infer a PR number, inspect paths outside +the repository, access the network or GitHub, modify files, create commits or branches, or run +builds and tests. Use only read-only repository inspection and the Git history/diff commands allowed +by the harness. + +Return only the JSON object required by `.agent-review-context/review.schema.json`. Report findings +introduced by `bench-base..HEAD`, anchor each finding to a changed line on the `HEAD` side, and use +an empty `findings` array when no actionable issue exists. Do not submit or draft GitHub-facing +review text in this mode. + ## Goal Perform a focused review of exactly one PR, using an isolated worktree and local validation before submitting a GitHub review. The review must also decide whether the PR duplicates existing base-branch functionality or overlaps with other open PRs. After the review decision is submitted, assign suitable human reviewers from `.github/MAINTAINERS.md` when the PR still needs domain follow-up. The normal outcome is either `APPROVE` when no blocking issue remains, or `REQUEST_CHANGES` with Chinese inline comments when the PR has correctness, standards, duplication, test, or CI coverage problems. diff --git a/scripts/agent-review-bench/reviewer.md b/scripts/agent-review-bench/reviewer.md index 0afb7216fc..42ca853fa3 100644 --- a/scripts/agent-review-bench/reviewer.md +++ b/scripts/agent-review-bench/reviewer.md @@ -1,18 +1,11 @@ # Offline Review Contract -Review only the committed change between `bench-base` and `HEAD` in this repository. +This file marks the repository as the isolated `review-single-pr offline-benchmark` environment. -- This is an offline benchmark snapshot, not a live GitHub pull request. Do not use the network, - `gh`, GitHub APIs, external repositories, or paths outside this repository. Do not submit a - review, comment, issue, branch, commit, or reviewer request. -- Do not modify files. Use read-only source inspection and Git history/diff commands only; do not - build or run tests that write artifacts. -- Read and apply the repository `AGENTS.md` and every file under `book/guideline/` as the current - review standards. Do not invoke the side-effectful `review-single-pr` workflow. -- Review the change for actionable correctness, concurrency, security/soundness, hardware or ABI - semantics, maintainability, deterministic regression coverage, test discovery/wiring, and - documentation/user-facing compatibility. -- Report only issues introduced by this change. Each finding must explain the concrete failure, - its impact, and the expected fix direction, and must point to a changed line on the `HEAD` side. -- Return the JSON object required by the supplied output schema. Use an empty `findings` array when - no actionable issue exists. +- The review target is the committed change between `bench-base` and `HEAD` in this repository. +- There is no live PR identity or external context. Network access, GitHub operations, paths outside + this repository, writes, builds, and tests are unavailable. +- The current `review-single-pr` skill, `AGENTS.md`, `book/guideline/`, and + `.agent-review-context/review.schema.json` are committed on both sides of the synthetic change and + therefore are review instructions rather than part of the target diff. +- The final response must be only the JSON object required by the supplied output schema. diff --git a/scripts/axbuild/src/agent_review_bench/mod.rs b/scripts/axbuild/src/agent_review_bench/mod.rs index 4bfaa011d2..8302b691c1 100644 --- a/scripts/axbuild/src/agent_review_bench/mod.rs +++ b/scripts/axbuild/src/agent_review_bench/mod.rs @@ -511,6 +511,9 @@ mod tests { assert_eq!(summary["grader"]["agent"], "codex"); assert_eq!(summary["grader"]["model"], "grader model"); assert_eq!(summary["grader"]["reasoning_effort"], "grader effort"); + let captured_args = fs::read_to_string(format!("{}.args", program.display())).unwrap(); + assert!(captured_args.contains("/review-single-pr offline-benchmark")); + assert!(!captured_args.contains("You are performing an offline code review benchmark")); let reverse_output = workspace.path().join("reverse-artifacts"); let mut reverse_args = test_run_args(reverse_output.clone()); @@ -530,6 +533,8 @@ mod tests { assert_eq!(reverse_summary["reviewer"]["agent"], "codex"); assert_eq!(reverse_summary["grader"]["agent"], "claude"); assert_eq!(reverse_summary["caught"], 1); + let captured_args = fs::read_to_string(format!("{}.args", program.display())).unwrap(); + assert!(captured_args.contains("$review-single-pr offline-benchmark")); write_mock_agent( &program, @@ -553,12 +558,19 @@ mod tests { #[cfg(unix)] fn create_test_case(workspace: &Path) -> BenchCase { fs::write(workspace.join("AGENTS.md"), "current rules\n").unwrap(); + fs::write(workspace.join("CLAUDE.md"), "see AGENTS.md\n").unwrap(); fs::create_dir_all(workspace.join("book/guideline")).unwrap(); fs::write( workspace.join("book/guideline/code-quality.md"), "current guideline\n", ) .unwrap(); + fs::create_dir_all(workspace.join(".claude/skills/review-single-pr")).unwrap(); + fs::write( + workspace.join(".claude/skills/review-single-pr/SKILL.md"), + "current review skill\n", + ) + .unwrap(); git(workspace, &["init", "--quiet"]); git(workspace, &["config", "user.name", "test"]); git(workspace, &["config", "user.email", "test@invalid"]); @@ -619,12 +631,12 @@ mod tests { fn write_mock_agent(program: &Path, review: &str, grade: &str) { let script = format!( "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo 'codex-cli mock'; exit 0; \ - fi\nmode=review\nif [ -f expected.json ]; then mode=grade; fi\noutput=\nwhile [ \ - \"$#\" -gt 0 ]; do\ncase \"$1\" in\n--cd) shift; mode=grade \ - ;;\n--output-last-message|-o) shift; output=$1 ;;\nesac\nshift\ndone\nif [ \"$mode\" \ - = review ]; then payload='{review}'; else payload='{grade}'; fi\nif [ -n \"$output\" \ - ]; then printf '%s\\n' \"$payload\" > \"$output\"; else printf '%s\\n' \"$payload\"; \ - fi\n" + fi\nprintf '%s\\n' \"$@\" >> \"$0.args\"\nmode=review\nif [ -f expected.json ]; then \ + mode=grade; fi\noutput=\nwhile [ \"$#\" -gt 0 ]; do\ncase \"$1\" in\n--cd) shift; \ + mode=grade ;;\n--output-last-message|-o) shift; output=$1 ;;\nesac\nshift\ndone\nif \ + [ \"$mode\" = review ]; then payload='{review}'; else payload='{grade}'; fi\nif [ -n \ + \"$output\" ]; then printf '%s\\n' \"$payload\" > \"$output\"; else printf '%s\\n' \ + \"$payload\"; fi\n" ); fs::write(program, script).unwrap(); fs::set_permissions(program, fs::Permissions::from_mode(0o755)).unwrap(); diff --git a/scripts/axbuild/src/agent_review_bench/runner.rs b/scripts/axbuild/src/agent_review_bench/runner.rs index 643be0036a..b23562b268 100644 --- a/scripts/axbuild/src/agent_review_bench/runner.rs +++ b/scripts/axbuild/src/agent_review_bench/runner.rs @@ -12,13 +12,8 @@ use tokio::{process::Command, time::timeout}; use super::{cases::BenchCase, sandbox::ReviewSandbox}; -const REVIEW_PROMPT: &str = - "You are performing an offline code review benchmark. Review only the committed changes from \ - `bench-base` to `HEAD` in the current repository. First read `AGENTS.md`, every file under \ - `book/guideline/`, and `.agent-review-context/reviewer.md`, then follow those rules. Inspect \ - the diff and relevant in-repository context for actionable defects. Do not use the network, \ - GitHub, paths outside this repository, or write operations. Return only the JSON object \ - required by the output schema."; +const CODEX_REVIEW_PROMPT: &str = "$review-single-pr offline-benchmark"; +const CLAUDE_REVIEW_PROMPT: &str = "/review-single-pr offline-benchmark"; const GRADE_PROMPT: &str = "You are grading an offline code review. Read `expected.json` and \ `review.json` in the current directory. For every expected item, \ decide whether one review finding satisfies its `match_if` criterion \ @@ -30,7 +25,6 @@ const GRADE_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/gra const CLAUDE_COMMON_ARGS: &[&str] = &[ "-p", - "--safe-mode", "--no-session-persistence", "--strict-mcp-config", "--mcp-config", @@ -39,6 +33,20 @@ const CLAUDE_COMMON_ARGS: &[&str] = &[ "--permission-mode", "dontAsk", ]; +const CLAUDE_REVIEW_SETTING_SOURCES: &str = "user,project"; +const CLAUDE_REVIEW_SETTINGS: &str = r#"{ + "disableAllHooks": true, + "disableAgentView": true, + "disableSkillShellExecution": true, + "env": { + "CLAUDE_CODE_DISABLE_AUTO_MEMORY": "1", + "CLAUDE_CODE_DISABLE_BACKGROUND_TASKS": "1", + "CLAUDE_CODE_DISABLE_CLAUDE_MDS": "1", + "CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC": "1", + "CLAUDE_CODE_DISABLE_OFFICIAL_MARKETPLACE_AUTOINSTALL": "1" + } +}"#; +const CLAUDE_GRADE_ARGS: &[&str] = &["--safe-mode"]; const CLAUDE_REVIEW_TOOLS: &str = "Bash,Read,Glob,Grep"; const CLAUDE_REVIEW_ALLOWED_TOOLS: &[&str] = &[ "Read", @@ -186,7 +194,7 @@ impl AgentRunner { .arg(&temporary_output) .stdout(Stdio::inherit()); apply_codex_options(&mut command, options); - command.arg(REVIEW_PROMPT); + command.arg(CODEX_REVIEW_PROMPT); run_process(&mut command, options.timeout_secs, "Codex reviewer").await?; copy_nonempty_output(&temporary_output, artifact_path, "reviewer") @@ -206,13 +214,15 @@ impl AgentRunner { command .current_dir(sandbox.repo()) .args(CLAUDE_COMMON_ARGS) + .args(["--setting-sources", CLAUDE_REVIEW_SETTING_SOURCES]) + .args(["--settings", CLAUDE_REVIEW_SETTINGS]) .args(["--tools", CLAUDE_REVIEW_TOOLS]) .arg("--allowedTools") .args(CLAUDE_REVIEW_ALLOWED_TOOLS) .args(["--json-schema", &schema]) .stdout(Stdio::from(output)); apply_claude_options(&mut command, options); - command.arg(REVIEW_PROMPT); + command.arg(CLAUDE_REVIEW_PROMPT); run_process(&mut command, options.timeout_secs, "Claude reviewer").await?; copy_nonempty_output(&temporary_output, artifact_path, "reviewer") @@ -256,6 +266,7 @@ impl AgentRunner { command .current_dir(grader_dir) .args(CLAUDE_COMMON_ARGS) + .args(CLAUDE_GRADE_ARGS) .args(["--tools", CLAUDE_GRADE_TOOLS]) .args(["--allowedTools", "Read"]) .args(["--json-schema", &schema]) @@ -439,12 +450,30 @@ mod tests { && !tool.contains("WebFetch") && !tool.contains("WebSearch") })); - assert!(CLAUDE_COMMON_ARGS.contains(&"--safe-mode")); + assert!(!CLAUDE_COMMON_ARGS.contains(&"--safe-mode")); + assert!(CLAUDE_GRADE_ARGS.contains(&"--safe-mode")); + assert_eq!(CLAUDE_REVIEW_SETTING_SOURCES, "user,project"); + let settings = serde_json::from_str::(CLAUDE_REVIEW_SETTINGS).unwrap(); + assert_eq!(settings["disableAllHooks"], true); + assert_eq!(settings["disableAgentView"], true); + assert_eq!(settings["disableSkillShellExecution"], true); + assert_eq!(settings["env"]["CLAUDE_CODE_DISABLE_CLAUDE_MDS"], "1"); assert!(CLAUDE_COMMON_ARGS.contains(&"--strict-mcp-config")); assert!(CLAUDE_COMMON_ARGS.contains(&r#"{"mcpServers":{}}"#)); assert!(CLAUDE_COMMON_ARGS.contains(&"--no-chrome")); } + #[test] + fn reviewer_prompts_only_invoke_the_project_skill() { + assert_eq!(CODEX_REVIEW_PROMPT, "$review-single-pr offline-benchmark"); + assert_eq!(CLAUDE_REVIEW_PROMPT, "/review-single-pr offline-benchmark"); + for prompt in [CODEX_REVIEW_PROMPT, CLAUDE_REVIEW_PROMPT] { + assert!(!prompt.contains("correctness")); + assert!(!prompt.contains("GitHub")); + assert!(!prompt.contains("bench-base")); + } + } + #[test] fn claude_schema_removes_unsupported_draft_metadata() { let schema = claude_schema( diff --git a/scripts/axbuild/src/agent_review_bench/sandbox.rs b/scripts/axbuild/src/agent_review_bench/sandbox.rs index 69a1970485..9f81dacd3c 100644 --- a/scripts/axbuild/src/agent_review_bench/sandbox.rs +++ b/scripts/axbuild/src/agent_review_bench/sandbox.rs @@ -13,6 +13,7 @@ use super::cases::BenchCase; const REVIEW_CONTRACT: &str = include_str!("../../../agent-review-bench/reviewer.md"); const REVIEW_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/review.schema.json"); +const REVIEW_SKILL_PATH: &str = ".claude/skills/review-single-pr/SKILL.md"; pub(super) struct ReviewSandbox { _root: TempDir, @@ -90,6 +91,8 @@ fn extract_snapshot( fn overlay_current_review_context(workspace_root: &Path, repo: &Path) -> anyhow::Result<()> { fs::copy(workspace_root.join("AGENTS.md"), repo.join("AGENTS.md")) .context("failed to copy current AGENTS.md into review sandbox")?; + fs::copy(workspace_root.join("CLAUDE.md"), repo.join("CLAUDE.md")) + .context("failed to copy current CLAUDE.md into review sandbox")?; let guideline_destination = repo.join("book/guideline"); if guideline_destination.exists() { @@ -100,6 +103,16 @@ fn overlay_current_review_context(workspace_root: &Path, repo: &Path) -> anyhow: &guideline_destination, )?; + let current_skill = workspace_root.join(REVIEW_SKILL_PATH); + replace_review_skill( + ¤t_skill, + &repo.join(".claude/skills/review-single-pr"), + )?; + replace_review_skill( + ¤t_skill, + &repo.join(".agents/skills/review-single-pr"), + )?; + let context_dir = repo.join(".agent-review-context"); fs::create_dir_all(&context_dir)?; fs::write(context_dir.join("reviewer.md"), REVIEW_CONTRACT)?; @@ -107,6 +120,28 @@ fn overlay_current_review_context(workspace_root: &Path, repo: &Path) -> anyhow: Ok(()) } +fn replace_review_skill(source: &Path, destination_dir: &Path) -> anyhow::Result<()> { + if destination_dir.exists() { + fs::remove_dir_all(destination_dir)?; + } + copy_file(source, &destination_dir.join("SKILL.md")) +} + +fn copy_file(source: &Path, destination: &Path) -> anyhow::Result<()> { + let parent = destination + .parent() + .context("review-context destination has no parent directory")?; + fs::create_dir_all(parent)?; + fs::copy(source, destination).with_context(|| { + format!( + "failed to copy review context {} to {}", + source.display(), + destination.display() + ) + })?; + Ok(()) +} + fn copy_tree(source: &Path, destination: &Path) -> anyhow::Result<()> { for entry in WalkDir::new(source) { let entry = entry?; @@ -215,12 +250,21 @@ mod tests { fn creates_standalone_two_commit_repository_without_ground_truth() { let workspace = tempdir().unwrap(); fs::write(workspace.path().join("AGENTS.md"), "current rules\n").unwrap(); + fs::write(workspace.path().join("CLAUDE.md"), "see AGENTS.md\n").unwrap(); fs::create_dir_all(workspace.path().join("book/guideline")).unwrap(); fs::write( workspace.path().join("book/guideline/code-quality.md"), "current guideline\n", ) .unwrap(); + fs::create_dir_all(workspace.path().join(".claude/skills/review-single-pr")).unwrap(); + fs::write( + workspace + .path() + .join(".claude/skills/review-single-pr/SKILL.md"), + "current review skill\n", + ) + .unwrap(); git(workspace.path(), ["init", "--quiet"]).unwrap(); git(workspace.path(), ["config", "user.name", "test"]).unwrap(); git(workspace.path(), ["config", "user.email", "test@invalid"]).unwrap(); @@ -266,6 +310,19 @@ mod tests { fs::read_to_string(sandbox.repo().join("AGENTS.md")).unwrap(), "current rules\n" ); + assert_eq!( + fs::read_to_string(sandbox.repo().join("CLAUDE.md")).unwrap(), + "see AGENTS.md\n" + ); + for skill_path in [ + ".claude/skills/review-single-pr/SKILL.md", + ".agents/skills/review-single-pr/SKILL.md", + ] { + assert_eq!( + fs::read_to_string(sandbox.repo().join(skill_path)).unwrap(), + "current review skill\n" + ); + } let diff = Command::new("git") .current_dir(sandbox.repo()) .args(["diff", "bench-base", "HEAD", "--", "src/lib.rs"]) @@ -273,6 +330,15 @@ mod tests { .unwrap(); let diff = String::from_utf8(diff.stdout).unwrap(); assert!(diff.contains("value() -> u8 { 2 }")); + let changed_paths = Command::new("git") + .current_dir(sandbox.repo()) + .args(["diff", "--name-only", "bench-base", "HEAD"]) + .output() + .unwrap(); + assert_eq!( + String::from_utf8(changed_paths.stdout).unwrap(), + "src/lib.rs\n" + ); assert!(!WalkDir::new(sandbox.repo()).into_iter().any(|entry| { entry .ok() From 8c6abfd5104949110567be1fe906f803848d0060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E7=9D=BF?= Date: Wed, 15 Jul 2026 15:36:03 +0800 Subject: [PATCH 4/4] test(agent-review-bench): add qemu timeout regression case --- ...axtask-deferred-wake-local-reschedule.toml | 6 +- .../1505-ipv6-pktinfo-family-validation.toml | 6 +- ...8125-regression-covers-constants-only.toml | 6 +- .../1611-axvisor-qemu-timeout-watchdog.toml | 16 ++++ .../axbuild/src/agent_review_bench/cases.rs | 89 +++++++++++++++++-- 5 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml diff --git a/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml b/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml index 2f5027a8e6..659feacdd9 100644 --- a/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml +++ b/scripts/agent-review-bench/cases/1495-axtask-deferred-wake-local-reschedule.toml @@ -1,6 +1,6 @@ id = "1495-axtask-deferred-wake-local-reschedule" pr = 1495 -title = "fix(axtask): defer cross-core wake instead of spinning on remote on_cpu" +title = "修复 axtask：延迟跨核唤醒，避免在远端 on_cpu 上自旋" remote = "https://github.com/rcore-os/tgoskits.git" base = "baf4aa42e883b78156011636d189f348409ff4c1" head = "c74237d2c10754b1341e03f495caafd261cf8b7e" @@ -12,5 +12,5 @@ id = "axtask-owner-drain-needs-local-reschedule" path = "os/arceos/modules/axtask/src/run_queue.rs" line = 1125 severity = "major" -description = "When the owner CPU drains a deferred wake, it always calls kick_remote_cpu(target). select_wake_run_queue can choose the same owner CPU as target, and that self-kick is a no-op. The runnable task can therefore miss the reschedule that the original remote waker supplied and remain queued while the owner enters idle or until a later tick." -match_if = "A finding must identify that the deferred-wake drain loses required scheduling notification when target is the current/owner CPU because kick_remote_cpu is ineffective for a self-target, and require an explicit local reschedule path or equivalent preserved handoff state." +description = "owner CPU 清空 deferred wake 时总是调用 kick_remote_cpu(target)。select_wake_run_queue 可能再次选择同一个 owner CPU 作为 target，而这种 self-kick 不会产生作用。因此，可运行任务会丢失原始远端唤醒者提供的 reschedule 通知，并可能在 owner 进入 idle 后一直留在队列中，直到后续 tick 才得到调度。" +match_if = "评审必须指出：deferred-wake drain 在 target 为当前或 owner CPU 时会丢失必要的调度通知，因为 kick_remote_cpu 对 self-target 无效；并要求显式触发本地 reschedule，或用等价机制保留 handoff 状态。" diff --git a/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml b/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml index b7e98ac7f5..e84b001aa6 100644 --- a/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml +++ b/scripts/agent-review-bench/cases/1505-ipv6-pktinfo-family-validation.toml @@ -1,6 +1,6 @@ id = "1505-ipv6-pktinfo-family-validation" pr = 1505 -title = "fix(starry): accept read-only mmap fdatasync and IP_PKTINFO/IPV6 pktinfo sockopts" +title = "修复 Starry：支持只读 mmap 的 fdatasync 和 IP_PKTINFO/IPV6 pktinfo 套接字选项" remote = "https://github.com/rcore-os/tgoskits.git" base = "882e07fd2d1581a8a9882d397946db25a974bd15" head = "dfde82ef6b58b6944613d1421bec297327e5f7da" @@ -12,5 +12,5 @@ id = "ipv6-pktinfo-bypasses-socket-family-check" path = "os/StarryOS/kernel/src/syscall/net/opt.rs" line = 521 severity = "major" -description = "The new IPV6_RECVPKTINFO/IPV6_PKTINFO fast paths return success before ensure_ipv6_socket is called. An AF_INET socket can consequently use an IPPROTO_IPV6 option successfully, unlike Linux and the existing IPv6 option paths, which reject the family mismatch with ENOPROTOOPT. The getsockopt path has the same issue." -match_if = "A finding must flag that the newly accepted IPv6 pktinfo options bypass IPv6 socket-family validation for AF_INET sockets in set and/or get handling, and require ensure_ipv6_socket or equivalent validation before success." +description = "新的 IPV6_RECVPKTINFO/IPV6_PKTINFO 快速路径在调用 ensure_ipv6_socket 前直接返回成功。因此，AF_INET 套接字可以成功使用 IPPROTO_IPV6 选项；这与 Linux 以及现有 IPv6 选项路径不一致，后者会对地址族不匹配返回 ENOPROTOOPT。getsockopt 路径存在同样问题。" +match_if = "评审必须指出：新接受的 IPv6 pktinfo 选项在 set 和/或 get 路径中绕过 IPv6 套接字地址族校验，导致 AF_INET 套接字被错误接受；并要求在成功返回前调用 ensure_ipv6_socket 或执行等价校验。" diff --git a/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml b/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml index 72437fc3c7..6998d1d9cc 100644 --- a/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml +++ b/scripts/agent-review-bench/cases/1583-rtl8125-regression-covers-constants-only.toml @@ -1,6 +1,6 @@ id = "1583-rtl8125-regression-covers-constants-only" pr = 1583 -title = "fix(realtek-rtl8125): advertise gigabit autonegotiation" +title = "修复 realtek-rtl8125：通告千兆自协商能力" remote = "https://github.com/rcore-os/tgoskits.git" base = "ba27d7b061b47d22b16a53c847b1a4abc2bece71" head = "214ccd50c9df852efaa74822341294defa92e001" @@ -12,5 +12,5 @@ id = "rtl8125-regression-does-not-exercise-phy-sequence" path = "drivers/net/realtek-rtl8125/src/hw.rs" line = 550 severity = "major" -description = "The regression test only checks relationships among the newly added constants. It never calls hw_phy_config or observes PHY reads and writes, so deleting the production MII_CTRL1000 and MII_BMCR configuration still leaves the test passing and restores the original autonegotiation bug." -match_if = "A finding must identify that the added test validates constants rather than the production PHY configuration behavior, so it cannot fail when the new page selection/advertisement/restart sequence is removed, and require a recording/mock PHY boundary or equivalent deterministic production-path regression." +description = "该回归测试只检查新常量之间的关系，从未调用 hw_phy_config，也未观察 PHY 读写。因此，即使删除生产代码中的 MII_CTRL1000 和 MII_BMCR 配置，测试仍会通过，原来的自协商缺陷也会复现。" +match_if = "评审必须指出：新增测试只验证常量，而没有覆盖生产 PHY 配置行为，所以移除新增的页选择、通告和重启序列后测试仍不会失败；并要求通过记录型或 mock PHY 边界，或等价方式建立确定性的生产路径回归测试。" diff --git a/scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml b/scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml new file mode 100644 index 0000000000..6385d160cd --- /dev/null +++ b/scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml @@ -0,0 +1,16 @@ +id = "1611-axvisor-qemu-timeout-watchdog" +pr = 1611 +title = "QEMU：移除 SVM 和 VMX 冒烟测试的显式超时" +remote = "https://github.com/rcore-os/tgoskits.git" +base = "dbbe0e06576fd1446c014d4edea3aa480505b5e9" +head = "6a4a23fe147e02d46c66df1d2fca8269098c2fba" +source = "https://github.com/rcore-os/tgoskits/pull/1611#discussion_r3583879562" +fixed_by = "a1a6ed0ce4ac56e006280cca268973578bd7a89d" + +[[expected]] +id = "axvisor-nested-qemu-watchdog-disabled" +path = "test-suit/axvisor/normal/qemu/smoke/qemu-x86_64-svm.toml" +line = 25 +severity = "major" +description = "SVM 与 VMX 嵌套虚拟化冒烟配置删掉了仅有的有限 timeout。解析后 QemuConfig.timeout 变为 None，测试运行器因而不再安装 watchdog；一旦 QEMU 启动、guest shell 或结束条件卡住，任务会无限等待。慢速 runner 已可通过提高基线值或 timeout scale 放宽，不应通过删除超时规避失败。" +match_if = "评审必须指出：SVM 和 VMX 配置删除 timeout 会使 watchdog 失效，并导致 QEMU 或 guest shell 卡住时无限等待；同时要求为两者恢复有限超时，允许调整基线值或使用 timeout scale。只泛泛建议增加超时或只讨论测试速度不算命中。" diff --git a/scripts/axbuild/src/agent_review_bench/cases.rs b/scripts/axbuild/src/agent_review_bench/cases.rs index 058dafba70..d15e746505 100644 --- a/scripts/axbuild/src/agent_review_bench/cases.rs +++ b/scripts/axbuild/src/agent_review_bench/cases.rs @@ -159,9 +159,9 @@ pub(super) fn prepare_case(workspace_root: &Path, case: &BenchCase) -> anyhow::R case.head ); } - if !line_is_added(workspace_root, case, expected)? { + if !line_is_in_head_hunk(workspace_root, case, expected)? { bail!( - "expected finding `{}` line {} is not an added HEAD-side line in `{}`", + "expected finding `{}` line {} is not a HEAD-side line in a changed hunk of `{}`", expected.id, expected.line, expected.path @@ -303,7 +303,7 @@ fn ensure_commit(workspace_root: &Path, case: &BenchCase, sha: &str) -> anyhow:: Ok(()) } -fn line_is_added( +fn line_is_in_head_hunk( workspace_root: &Path, case: &BenchCase, expected: &ExpectedFinding, @@ -312,7 +312,7 @@ fn line_is_added( workspace_root, &[ "diff", - "--unified=0", + "--unified=1", &case.base, &case.head, "--", @@ -390,6 +390,8 @@ fn git_status(workspace_root: &Path, args: &[&str]) -> anyhow::Result { #[cfg(test)] mod tests { + use tempfile::tempdir; + use super::*; fn sample_case() -> BenchCase { @@ -426,7 +428,7 @@ mod tests { } #[test] - fn selectors_form_a_union() { + fn selectors_form_a_deduplicated_union() { let first = sample_case(); let mut second = sample_case(); second.id = "0002-second".into(); @@ -434,7 +436,12 @@ mod tests { second.expected[0].id = "second-finding".into(); let cases = [first, second]; - let selected = select_cases(&cases, &["0001-sample".into()], &[2]).unwrap(); + let selected = select_cases( + &cases, + &["0001-sample".into(), "0001-sample".into()], + &[2, 2], + ) + .unwrap(); assert_eq!(selected.len(), 2); } @@ -442,4 +449,74 @@ mod tests { fn rejects_unknown_selector() { assert!(select_cases(&[sample_case()], &["missing".into()], &[]).is_err()); } + + #[test] + fn accepts_head_context_line_adjacent_to_deletion() { + let (repo, case) = case_with_file_change( + "setting = true\ntimeout = 300\nfail_regex = []\n", + "setting = true\nfail_regex = []\n", + 2, + ); + + assert!(line_is_in_head_hunk(repo.path(), &case, &case.expected[0]).unwrap()); + } + + #[test] + fn accepts_added_head_line() { + let (repo, case) = case_with_file_change( + "setting = true\nfail_regex = []\n", + "setting = true\ntimeout = 300\nfail_regex = []\n", + 2, + ); + + assert!(line_is_in_head_hunk(repo.path(), &case, &case.expected[0]).unwrap()); + } + + #[test] + fn rejects_unchanged_head_line_outside_diff_hunk() { + let (repo, case) = case_with_file_change( + "setting = true\ntimeout = 300\nfirst = 1\nsecond = 2\nthird = 3\n", + "setting = true\nfirst = 1\nsecond = 2\nthird = 3\n", + 4, + ); + + assert!(!line_is_in_head_hunk(repo.path(), &case, &case.expected[0]).unwrap()); + } + + fn case_with_file_change( + base_content: &str, + head_content: &str, + expected_line: usize, + ) -> (tempfile::TempDir, BenchCase) { + let repo = tempdir().unwrap(); + initialize_repo(repo.path()); + let base = commit_file(repo.path(), base_content, "base"); + let head = commit_file(repo.path(), head_content, "change file"); + let mut case = sample_case(); + case.base = base; + case.head = head; + case.expected[0].path = "case.toml".into(); + case.expected[0].line = expected_line; + (repo, case) + } + + fn initialize_repo(repo: &Path) { + git_output(repo, &["init", "--quiet"]).unwrap(); + git_output(repo, &["config", "user.name", "Agent Review Bench"]).unwrap(); + git_output( + repo, + &["config", "user.email", "agent-review-bench@example.com"], + ) + .unwrap(); + } + + fn commit_file(repo: &Path, content: &str, message: &str) -> String { + fs::write(repo.join("case.toml"), content).unwrap(); + git_output(repo, &["add", "case.toml"]).unwrap(); + git_output(repo, &["commit", "--quiet", "-m", message]).unwrap(); + git_output(repo, &["rev-parse", "HEAD"]) + .unwrap() + .trim() + .to_string() + } }