diff --git a/scripts/agent-review-bench/cases/1039-qperf-monolithic-perf-module.toml b/scripts/agent-review-bench/cases/1039-qperf-monolithic-perf-module.toml new file mode 100644 index 0000000000..b25c6b2069 --- /dev/null +++ b/scripts/agent-review-bench/cases/1039-qperf-monolithic-perf-module.toml @@ -0,0 +1,15 @@ +id = "1039-qperf-monolithic-perf-module" +pr = 1039 +title = "qperf：单体 perf 模块与巨型输出结构体" +remote = "https://github.com/rcore-os/tgoskits.git" +base = "f0a27a0a47b0bf520a5bad2824e399616428b04a" +head = "4e6d7ecac930eec51c419eb8cea28e396fe82a39" +source = "https://github.com/rcore-os/tgoskits/pull/1039#discussion_r3585803023" +fixed_by = "0bfaa3e6a3d90d81ff7a344e434f1274673ad593" + +[[expected]] +id = "qperf-perf-module-mixes-responsibilities" +path = "scripts/axbuild/src/starry/perf.rs" +line = 56 +severity = "major" +description = "该 PR 将 perf.rs 从 406 行扩展到 2013 行，并把 PerfOutputs 从 6 个字段扩展到 28 个；参数、构建、QEMU/QMP 生命周期、监控、指标、analyzer、ELF 解析和报告输出被集中在同一模块和大结构体中。" 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 659feacdd9..74d1657efd 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 @@ -13,4 +13,3 @@ path = "os/arceos/modules/axtask/src/run_queue.rs" line = 1125 severity = "major" 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 e84b001aa6..ac731766ac 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 @@ -13,4 +13,3 @@ path = "os/StarryOS/kernel/src/syscall/net/opt.rs" line = 521 severity = "major" 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 6998d1d9cc..6ce9c10d7a 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 @@ -13,4 +13,3 @@ path = "drivers/net/realtek-rtl8125/src/hw.rs" line = 550 severity = "major" 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 index 6385d160cd..6c5d67e156 100644 --- a/scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml +++ b/scripts/agent-review-bench/cases/1611-axvisor-qemu-timeout-watchdog.toml @@ -13,4 +13,3 @@ 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/agent-review-bench/schemas/grade.schema.json b/scripts/agent-review-bench/schemas/grade.schema.json index 883f165728..70b33024fa 100644 --- a/scripts/agent-review-bench/schemas/grade.schema.json +++ b/scripts/agent-review-bench/schemas/grade.schema.json @@ -9,14 +9,12 @@ "items": { "type": "object", "additionalProperties": false, - "required": ["expected_id", "finding_index", "reason"], + "required": ["expected_id", "finding_indices", "reason"], "properties": { "expected_id": { "type": "string" }, - "finding_index": { - "anyOf": [ - { "type": "integer", "minimum": 0 }, - { "type": "null" } - ] + "finding_indices": { + "type": "array", + "items": { "type": "integer", "minimum": 0 } }, "reason": { "type": "string" } } diff --git a/scripts/axbuild/src/agent_review_bench/cases.rs b/scripts/axbuild/src/agent_review_bench/cases.rs index d15e746505..2c84d17833 100644 --- a/scripts/axbuild/src/agent_review_bench/cases.rs +++ b/scripts/axbuild/src/agent_review_bench/cases.rs @@ -32,7 +32,6 @@ pub(super) struct ExpectedFinding { pub(super) line: usize, pub(super) severity: Severity, pub(super) description: String, - pub(super) match_if: String, } #[derive(Clone, Copy, Debug, Deserialize, Serialize)] @@ -226,11 +225,8 @@ fn validate_case_schema(case: &BenchCase) -> anyhow::Result<()> { 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 - ); + if expected.description.trim().is_empty() { + bail!("finding `{}` description must not be empty", expected.id); } } Ok(()) @@ -410,7 +406,6 @@ mod tests { line: 1, severity: Severity::Major, description: "sample defect".into(), - match_if: "reviewer identifies sample defect".into(), }], } } diff --git a/scripts/axbuild/src/agent_review_bench/mod.rs b/scripts/axbuild/src/agent_review_bench/mod.rs index 8302b691c1..122f6de742 100644 --- a/scripts/axbuild/src/agent_review_bench/mod.rs +++ b/scripts/axbuild/src/agent_review_bench/mod.rs @@ -48,15 +48,6 @@ pub(crate) struct RunArgs { /// 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, @@ -82,7 +73,7 @@ struct RunSummary { cases: Vec, } -#[derive(Debug, Serialize)] +#[derive(Clone, Debug, Serialize)] struct AgentConfiguration { agent: String, version: String, @@ -144,18 +135,15 @@ async fn run_cases( 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 + let agent = AgentRunner::discover(args.agent)?; + run_cases_with_runner(workspace_root, all_cases, args, &agent).await } -async fn run_cases_with_runners( +async fn run_cases_with_runner( workspace_root: &std::path::Path, all_cases: &[BenchCase], args: RunArgs, - reviewer: &AgentRunner, - grader: &AgentRunner, + agent: &AgentRunner, ) -> anyhow::Result<()> { if args.timeout_secs == 0 { bail!("--timeout-secs must be greater than zero"); @@ -170,9 +158,8 @@ async fn run_cases_with_runners( ) })?; - let (reviewer_options, grader_options) = resolve_agent_options(&args); - let reviewer_version = reviewer.version()?; - let grader_version = grader.version()?; + let options = resolve_agent_options(&args); + let version = agent.version()?; let mut results = Vec::with_capacity(selected.len()); for case in selected { @@ -184,8 +171,8 @@ async fn run_cases_with_runners( let sandbox = ReviewSandbox::create(workspace_root, case)?; let review_path = case_dir.join("review.json"); let review_started = Instant::now(); - reviewer - .review(&sandbox, &review_path, &reviewer_options) + agent + .review(&sandbox, &review_path, &options) .await .with_context(|| format!("reviewer failed for `{}`", case.id))?; let review_seconds = review_started.elapsed().as_secs_f64(); @@ -193,8 +180,8 @@ async fn run_cases_with_runners( let grade_path = case_dir.join("grade.json"); let grade_started = Instant::now(); - grader - .grade(case, &review_path, &grade_path, &grader_options) + agent + .grade(case, &review.findings, &grade_path, &options) .await .with_context(|| format!("grader failed for `{}`", case.id))?; let grade_seconds = grade_started.elapsed().as_secs_f64(); @@ -209,9 +196,8 @@ async fn run_cases_with_runners( results.push(result); } - let reviewer_configuration = - agent_configuration(reviewer, &reviewer_version, &reviewer_options); - let grader_configuration = agent_configuration(grader, &grader_version, &grader_options); + let reviewer_configuration = agent_configuration(agent, &version, &options); + let grader_configuration = reviewer_configuration.clone(); let summary = summarize( reviewer_configuration, grader_configuration, @@ -242,21 +228,12 @@ async fn run_cases_with_runners( Ok(()) } -fn resolve_agent_options(args: &RunArgs) -> (AgentOptions, AgentOptions) { - let reviewer = AgentOptions { +fn resolve_agent_options(args: &RunArgs) -> AgentOptions { + 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( @@ -391,12 +368,6 @@ mod tests { "model with spaces", "--reasoning-effort", "vendor effort", - "--grader-agent", - "codex", - "--grader-model", - "grader \"model\"", - "--grader-reasoning-effort", - "grader effort", ]) .unwrap(); @@ -409,12 +380,17 @@ mod tests { 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 rejects_removed_grader_overrides() { + for option in [ + "--grader-agent", + "--grader-model", + "--grader-reasoning-effort", + ] { + assert!(TestCli::try_parse_from(["bench", "run", option, "value"]).is_err()); + } } #[test] @@ -427,9 +403,6 @@ mod tests { 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] @@ -446,25 +419,22 @@ mod tests { } #[test] - fn grader_options_inherit_reviewer_values() { + fn agent_options_apply_to_review_and_grade() { 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"); + let options = resolve_agent_options(&args); + assert_eq!(options.model.as_deref(), Some("")); + assert_eq!(options.reasoning_effort, "effort with spaces"); + assert_eq!(options.timeout_secs, 7); } #[cfg(unix)] @@ -477,13 +447,12 @@ mod tests { 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"}]}"#, + r#"{"matches":[{"expected_id":"sample-finding","finding_indices":[0],"reason":"same defect"}]}"#, ); - let reviewer = AgentRunner::from_program(AgentKind::Claude, program.clone()); - let grader = AgentRunner::from_program(AgentKind::Codex, program.clone()); + let claude = AgentRunner::from_program(AgentKind::Claude, program.clone()); let args = test_run_args(output.clone()); - run_cases_with_runners(workspace.path(), &[case.clone()], args, &reviewer, &grader) + run_cases_with_runner(workspace.path(), &[case.clone()], args, &claude) .await .unwrap(); @@ -508,30 +477,32 @@ mod tests { 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"); + assert_eq!(summary["grader"], summary["reviewer"]); 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")); + assert_eq!( + captured_args + .matches("You are grading one offline code-review case") + .count(), + 1 + ); + let grader_files = fs::read_to_string(format!("{}.files", program.display())).unwrap(); + assert!(grader_files.contains("known_findings.json")); + assert!(grader_files.contains("candidate_findings.json")); + assert!(!grader_files.contains("review.json")); + assert!(!grader_files.contains("expected.json")); 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 codex = AgentRunner::from_program(AgentKind::Codex, program.clone()); + run_cases_with_runner(workspace.path(), &[case.clone()], reverse_args, &codex) + .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["grader"], reverse_summary["reviewer"]); 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")); @@ -539,15 +510,14 @@ mod tests { write_mock_agent( &program, "not JSON", - r#"{"matches":[{"expected_id":"sample-finding","finding_index":null,"reason":"missed"}]}"#, + r#"{"matches":[{"expected_id":"sample-finding","finding_indices":[],"reason":"missed"}]}"#, ); let invalid_output = workspace.path().join("invalid-artifacts"); - let error = run_cases_with_runners( + let error = run_cases_with_runner( workspace.path(), &[case], test_run_args(invalid_output), - &reviewer, - &grader, + &claude, ) .await .unwrap_err() @@ -605,7 +575,6 @@ mod tests { line: 1, severity: Severity::Major, description: "secret answer".into(), - match_if: "same defect".into(), }], } } @@ -618,9 +587,6 @@ mod tests { 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), @@ -631,12 +597,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\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" + fi\nprintf '%s\\n' \"$@\" >> \"$0.args\"\nmode=review\nif [ -f known_findings.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 ls \ + -1 > \"$0.files\"; 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 b23562b268..75e8d4f93a 100644 --- a/scripts/axbuild/src/agent_review_bench/runner.rs +++ b/scripts/axbuild/src/agent_review_bench/runner.rs @@ -10,17 +10,20 @@ use clap::ValueEnum; use tempfile::tempdir; use tokio::{process::Command, time::timeout}; -use super::{cases::BenchCase, sandbox::ReviewSandbox}; +use super::{cases::BenchCase, sandbox::ReviewSandbox, scoring::ReviewFinding}; 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 \ - 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_PROMPT: &str = + "You are grading one offline code-review case. Read only `known_findings.json` and \ + `candidate_findings.json` in the current directory. For every known finding, decide whether \ + one or more candidate findings identify the same underlying defect or material risk. \ + Different wording, anchors, omitted consequences, and different or absent remediation are \ + acceptable; exact numbers, PR references, and historical context are not required. Candidate \ + findings may jointly cover one known finding. Generic advice, mere proximity, or a different \ + issue does not match. Return exactly one match object per known finding ID. \ + `finding_indices` are zero-based indices in `candidate_findings.json` and must contain every \ + candidate used to support the match, or be empty when missed. Do not inspect any other paths."; const GRADE_SCHEMA: &str = include_str!("../../../agent-review-bench/schemas/grade.schema.json"); const CLAUDE_COMMON_ARGS: &[&str] = &[ @@ -149,14 +152,21 @@ impl AgentRunner { pub(super) async fn grade( &self, case: &BenchCase, - review_path: &Path, + findings: &[ReviewFinding], 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 known_findings = serde_json::to_string_pretty(&case.expected)?; + fs::write( + grader_dir.path().join("known_findings.json"), + known_findings, + )?; + let candidate_findings = serde_json::to_string_pretty(findings)?; + fs::write( + grader_dir.path().join("candidate_findings.json"), + candidate_findings, + )?; 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"); @@ -474,6 +484,16 @@ mod tests { } } + #[test] + fn grader_prompt_limits_context_and_matches_underlying_issues() { + assert!(GRADE_PROMPT.contains("known_findings.json")); + assert!(GRADE_PROMPT.contains("candidate_findings.json")); + assert!(GRADE_PROMPT.contains("same underlying defect or material risk")); + assert!(GRADE_PROMPT.contains("jointly cover one known finding")); + assert!(!GRADE_PROMPT.contains("review.json")); + assert!(!GRADE_PROMPT.contains("match_if")); + } + #[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 9f81dacd3c..3961493642 100644 --- a/scripts/axbuild/src/agent_review_bench/sandbox.rs +++ b/scripts/axbuild/src/agent_review_bench/sandbox.rs @@ -299,7 +299,6 @@ mod tests { line: 1, severity: Severity::Major, description: "secret answer".into(), - match_if: "secret criterion".into(), }], }; diff --git a/scripts/axbuild/src/agent_review_bench/scoring.rs b/scripts/axbuild/src/agent_review_bench/scoring.rs index 494613c038..91e8e35bfc 100644 --- a/scripts/axbuild/src/agent_review_bench/scoring.rs +++ b/scripts/axbuild/src/agent_review_bench/scoring.rs @@ -32,7 +32,7 @@ pub(super) struct GradeOutput { #[serde(deny_unknown_fields)] pub(super) struct FindingMatch { pub(super) expected_id: String, - pub(super) finding_index: Option, + pub(super) finding_indices: Vec, pub(super) reason: String, } @@ -76,7 +76,14 @@ pub(super) fn score_review( finding_match.expected_id ); } - if let Some(index) = finding_match.finding_index { + let mut unique_indices = BTreeSet::new(); + for &index in &finding_match.finding_indices { + if !unique_indices.insert(index) { + bail!( + "grader returned duplicate review finding index {index} for `{}`", + finding_match.expected_id + ); + } review.findings.get(index).with_context(|| { format!( "grader referenced review finding index {index}, but only {} findings exist", @@ -95,12 +102,12 @@ pub(super) fn score_review( let matched_indices = matches .values() - .filter_map(|finding_match| finding_match.finding_index) + .flat_map(|finding_match| finding_match.finding_indices.iter().copied()) .collect::>(); Ok(CaseScore { caught: matches .values() - .filter(|finding_match| finding_match.finding_index.is_some()) + .filter(|finding_match| !finding_match.finding_indices.is_empty()) .count(), expected: case.expected.len(), extra_findings: review.findings.len() - matched_indices.len(), @@ -113,23 +120,27 @@ mod tests { use crate::agent_review_bench::cases::{ExpectedFinding, Severity}; #[test] - fn scores_caught_missed_and_extra_findings() { + fn scores_joint_matches_shared_findings_and_extras() { let case = sample_case(); let review = ReviewOutput { summary: "summary".into(), - findings: vec![finding("caught"), finding("extra")], + findings: vec![ + finding("first part"), + finding("shared part"), + finding("extra"), + ], }; let grade = GradeOutput { matches: vec![ - finding_match("first", Some(0)), - finding_match("second", None), + finding_match("first", &[0, 1]), + finding_match("second", &[1]), ], }; assert_eq!( score_review(&case, &review, &grade).unwrap(), CaseScore { - caught: 1, + caught: 2, expected: 2, extra_findings: 1, } @@ -137,24 +148,88 @@ mod tests { } #[test] - fn rejects_unknown_and_out_of_range_matches() { + fn scores_all_missed_and_zero_candidate_reviews() { + let case = sample_case(); + let missed_review = ReviewOutput { + summary: "summary".into(), + findings: vec![finding("unmatched")], + }; + let missed_grade = GradeOutput { + matches: vec![finding_match("first", &[]), finding_match("second", &[])], + }; + assert_eq!( + score_review(&case, &missed_review, &missed_grade).unwrap(), + CaseScore { + caught: 0, + expected: 2, + extra_findings: 1, + } + ); + + let empty_review = ReviewOutput { + summary: "summary".into(), + findings: Vec::new(), + }; + assert_eq!( + score_review(&case, &empty_review, &missed_grade).unwrap(), + CaseScore { + caught: 0, + expected: 2, + extra_findings: 0, + } + ); + } + + #[test] + fn rejects_unknown_missing_duplicate_and_invalid_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))], + matches: vec![finding_match("unknown", &[0])], }; assert!(score_review(&case, &review, &unknown).is_err()); - let out_of_range = GradeOutput { + let missing = GradeOutput { + matches: vec![finding_match("first", &[0])], + }; + assert!(score_review(&case, &review, &missing).is_err()); + + let duplicate_expected = GradeOutput { matches: vec![ - finding_match("first", Some(2)), - finding_match("second", None), + finding_match("first", &[0]), + finding_match("first", &[0]), + finding_match("second", &[]), ], }; + assert!(score_review(&case, &review, &duplicate_expected).is_err()); + + let out_of_range = GradeOutput { + matches: vec![finding_match("first", &[2]), finding_match("second", &[])], + }; assert!(score_review(&case, &review, &out_of_range).is_err()); + + let duplicate_index = GradeOutput { + matches: vec![ + finding_match("first", &[0, 0]), + finding_match("second", &[]), + ], + }; + assert!(score_review(&case, &review, &duplicate_index).is_err()); + + let empty_reason = GradeOutput { + matches: vec![ + FindingMatch { + expected_id: "first".into(), + finding_indices: vec![0], + reason: " ".into(), + }, + finding_match("second", &[]), + ], + }; + assert!(score_review(&case, &review, &empty_reason).is_err()); } fn sample_case() -> BenchCase { @@ -178,7 +253,6 @@ mod tests { line: 1, severity: Severity::Major, description: "description".into(), - match_if: "criterion".into(), } } @@ -192,10 +266,10 @@ mod tests { } } - fn finding_match(expected_id: &str, finding_index: Option) -> FindingMatch { + fn finding_match(expected_id: &str, finding_indices: &[usize]) -> FindingMatch { FindingMatch { expected_id: expected_id.into(), - finding_index, + finding_indices: finding_indices.to_vec(), reason: "reason".into(), } }