From a226979a5b6cbb90fb39d5549f1e5fdc13a1b5d0 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 19 Jul 2026 17:06:09 +0000 Subject: [PATCH] Fix multi-batch review validation retries --- src/diff.rs | 86 +++++++++++++++++++- src/llm.rs | 2 +- src/review.rs | 175 ++++++++++++++++++++++++++++++++-------- tests/e2e.rs | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 445 insertions(+), 36 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 3004e1d..c560c55 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -3433,7 +3433,24 @@ pub fn review_batch_canonical_evidence( evidence: Option<&str>, ) -> Option { let evidence = evidence.filter(|value| !value.trim().is_empty())?; + let payloads = review_batch_evidence_payloads(annotated, path, line); + if let Some(exact) = payloads.iter().find(|payload| **payload == evidence) { + return Some((*exact).to_string()); + } + let trimmed_matches = payloads + .into_iter() + .filter(|payload| payload.trim() == evidence.trim()) + .collect::>(); + let first = *trimmed_matches.first()?; + trimmed_matches + .iter() + .all(|payload| *payload == first) + .then(|| first.to_string()) +} + +fn review_batch_evidence_payloads<'a>(annotated: &'a str, path: &str, line: u32) -> Vec<&'a str> { let mut current_path: Option<&str> = None; + let mut payloads = Vec::new(); for rendered in annotated.lines() { if let Some(header) = rendered.strip_prefix("### ") { current_path = Some(prompt_header_path(header)); @@ -3451,11 +3468,27 @@ pub fn review_batch_canonical_evidence( let payload = marked .strip_prefix("+ ") .or_else(|| marked.strip_prefix(" ")); - if let Some(payload) = payload.filter(|value| value.trim() == evidence.trim()) { - return Some(payload.to_string()); + if let Some(payload) = payload.filter(|value| !value.trim().is_empty()) { + payloads.push(payload); } } - None + payloads +} + +/// Resolve a prompt citation to the exact non-empty new-side text the model +/// must copy. This is exposed to the correction prompt, while final acceptance +/// continues to require an exact match through `review_batch_canonical_evidence`. +pub fn review_batch_expected_evidence(annotated: &str, path: &str, line: u32) -> Option { + let payloads = review_batch_evidence_payloads(annotated, path, line); + let first = *payloads.first()?; + payloads + .iter() + .all(|payload| *payload == first) + .then(|| first.to_string()) +} + +pub fn review_batch_has_evidence_anchor(annotated: &str, path: &str, line: u32) -> bool { + !review_batch_evidence_payloads(annotated, path, line).is_empty() } /// Render a bounded local window around a citation from the exact evidence a @@ -4135,12 +4168,59 @@ Binary files a/img.png and b/img.png differ .as_deref(), Some(" indented replacement") ); + assert_eq!( + review_batch_expected_evidence(batch, "src/a.rs", 10).as_deref(), + Some("replacement command") + ); + assert_eq!( + review_batch_expected_evidence(batch, "src/a.rs", 13).as_deref(), + Some(" indented replacement") + ); + assert_eq!(review_batch_expected_evidence(batch, "src/a.rs", 11), None); + assert_eq!(review_batch_expected_evidence(batch, "src/a.rs", 99), None); assert!(!review_batch_contains_exact_evidence( batch, "src/a.rs", 13, Some("indented replacement") )); + + let repeated = "### src/a.rs\n@@ first @@\n 13 + first slice\n@@ second @@\n 13 + second slice\n"; + assert_eq!( + review_batch_expected_evidence(repeated, "src/a.rs", 13), + None + ); + assert!(review_batch_has_evidence_anchor(repeated, "src/a.rs", 13)); + assert_eq!( + review_batch_canonical_evidence(repeated, "src/a.rs", 13, Some("second slice")) + .as_deref(), + Some("second slice") + ); + + let whitespace_distinct = "### src/a.rs\n@@ first @@\n 13 + changed();\n@@ second @@\n 13 + changed();\n"; + assert_eq!( + review_batch_canonical_evidence( + whitespace_distinct, + "src/a.rs", + 13, + Some("changed();") + ), + None + ); + assert_eq!( + review_batch_canonical_evidence( + whitespace_distinct, + "src/a.rs", + 13, + Some(" changed();") + ) + .as_deref(), + Some(" changed();") + ); + assert_eq!( + review_batch_expected_evidence(whitespace_distinct, "src/a.rs", 13), + None + ); } #[test] diff --git a/src/llm.rs b/src/llm.rs index 5224ed0..ed4382c 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -608,7 +608,7 @@ fn review_semantic_retry_user(user: &str, previous: &str) -> String { fn review_validation_retry_user(user: &str, reason: &str) -> String { format!( - "{user}\n\n[Correction] The previous response was unusable ({reason}). Retry once. Every finding must cite an exact path and new-file line displayed in the review input. Return only the corrected review JSON." + "{user}\n\n[Correction] The previous response was unusable: {reason}. Correct that exact contract failure. Preserve a finding only when the review input supports it; otherwise retract it. Return only the corrected review JSON." ) } diff --git a/src/review.rs b/src/review.rs index 0a20dea..3cbc83d 100644 --- a/src/review.rs +++ b/src/review.rs @@ -103,6 +103,65 @@ fn conservative_context_tokens(model: &str) -> usize { } } +fn review_batch_validation_reason( + finding: &Finding, + annotated: &str, + content_policy_prompt: Option<&str>, +) -> Option { + if let Err(reason) = crate::envelope::validate_finding_publication(finding) { + return Some(format!( + "finding at {}:{} violates the publication contract: {reason}", + finding.path, finding.line + )); + } + + if diff::review_batch_contains_exact_evidence( + annotated, + &finding.path, + finding.line, + finding.evidence.as_deref(), + ) || content_policy_prompt.is_some_and(|prompt| { + diff::review_batch_contains_exact_evidence( + prompt, + &finding.path, + finding.line, + finding.evidence.as_deref(), + ) + }) { + return None; + } + + let evidence_source = content_policy_prompt + .filter(|prompt| { + diff::review_batch_has_evidence_anchor(prompt, &finding.path, finding.line) + }) + .or_else(|| { + diff::review_batch_has_evidence_anchor(annotated, &finding.path, finding.line) + .then_some(annotated) + }); + + let Some(evidence_source) = evidence_source else { + return Some(format!( + "finding at {}:{} does not cite a non-empty new-side line displayed in this review input; retract it or cite a displayed new-side line", + finding.path, finding.line + )); + }; + let Some(expected) = + diff::review_batch_expected_evidence(evidence_source, &finding.path, finding.line) + else { + return Some(format!( + "finding at {}:{} has multiple displayed new-side evidence strings; copy the exact supporting string or retract it", + finding.path, finding.line + )); + }; + Some(format!( + "finding at {}:{} must set `evidence` to the exact JSON string {}", + finding.path, + finding.line, + serde_json::to_string(&expected).expect("evidence string is JSON-serializable") + )) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ForgeKind { GitHub, @@ -1147,31 +1206,20 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> let validation_user = user.clone(); match client .review_validated(cfg, &system, &user, move |review| { - let invalid = review.findings.iter().find(|finding| { - let grounded = diff::review_batch_contains_exact_evidence( - &validation_annotated, - &finding.path, - finding.line, - finding.evidence.as_deref(), - ) || (first - && finding.kind == crate::envelope::Kind::ContentPolicy - && diff::review_batch_contains_exact_evidence( - &validation_user, - &finding.path, - finding.line, - finding.evidence.as_deref(), - )); - crate::envelope::validate_finding_publication(finding).is_err() - || !grounded - }); - if let Some(finding) = invalid { - Err(format!( - "finding at {}:{} has invalid publication text or lacks exact new-side evidence", - finding.path, finding.line - )) - } else { - Ok(()) - } + review + .findings + .iter() + .find_map(|finding| { + review_batch_validation_reason( + finding, + &validation_annotated, + (first + && finding.kind + == crate::envelope::Kind::ContentPolicy) + .then_some(validation_user.as_str()), + ) + }) + .map_or(Ok(()), Err) }) .await { @@ -1210,15 +1258,13 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> finding.evidence.as_deref(), ) .or_else(|| { - (first - && finding.kind - == crate::envelope::Kind::ContentPolicy) + (first && finding.kind == crate::envelope::Kind::ContentPolicy) .then(|| { diff::review_batch_canonical_evidence( - &user, - &finding.path, - finding.line, - finding.evidence.as_deref(), + &user, + &finding.path, + finding.line, + finding.evidence.as_deref(), ) }) .flatten() @@ -2279,4 +2325,69 @@ mod tests { assert_eq!(findings[0].confidence, 0.49); assert_eq!(findings[0].kind, Kind::Uncertainty); } + + #[test] + fn batch_validation_exposes_exact_evidence_for_one_correction() { + let annotated = "### src/lib.rs\n@@ fixture @@\n 7 + changed();\n"; + let mut finding = finding("src/lib.rs", 7, "This change is unsafe."); + finding.evidence = Some("changed approximately".to_string()); + + let reason = review_batch_validation_reason(&finding, annotated, None).unwrap(); + assert_eq!( + reason, + "finding at src/lib.rs:7 must set `evidence` to the exact JSON string \" changed();\"" + ); + + finding.evidence = Some(" changed();".to_string()); + assert_eq!( + review_batch_validation_reason(&finding, annotated, None), + None + ); + } + + #[test] + fn batch_validation_reports_publication_failure_before_grounding() { + let annotated = "### src/lib.rs\n@@ fixture @@\n 7 + changed();\n"; + let mut finding = finding("src/lib.rs", 7, "This sentence is cut off"); + finding.evidence = Some("changed();".to_string()); + + assert_eq!( + review_batch_validation_reason(&finding, annotated, None).as_deref(), + Some( + "finding at src/lib.rs:7 violates the publication contract: finding body must end with sentence punctuation" + ) + ); + } + + #[test] + fn content_policy_evidence_may_come_from_policy_prompt_on_an_overlapping_line() { + let annotated = "### .postil/content-policy.md\n@@ diff @@\n 7 + repository text\n"; + let policy = "### .postil/content-policy.md\n@@ policy @@\n 7 + policy text\n"; + let mut finding = finding( + ".postil/content-policy.md", + 7, + "The proposed text violates the configured policy.", + ); + finding.kind = Kind::ContentPolicy; + finding.evidence = Some("policy text".to_string()); + + assert_eq!( + review_batch_validation_reason(&finding, annotated, Some(policy)), + None + ); + } + + #[test] + fn batch_validation_does_not_choose_between_distinct_duplicate_slices() { + let annotated = "### src/lib.rs\n@@ first @@\n 7 + first slice\n@@ second @@\n 7 + second slice\n"; + let mut finding = finding("src/lib.rs", 7, "This change is unsafe."); + finding.evidence = Some("approximate evidence".to_string()); + + assert_eq!( + review_batch_validation_reason(&finding, annotated, None).as_deref(), + Some( + "finding at src/lib.rs:7 has multiple displayed new-side evidence strings; copy the exact supporting string or retract it" + ) + ); + } } diff --git a/tests/e2e.rs b/tests/e2e.rs index eb4ea1d..8492a9a 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -293,6 +293,57 @@ struct SequentialReviewResponder { responses: Arc>, } +#[cfg(feature = "qualification-candidate")] +#[derive(Clone)] +struct ExactEvidenceRetryResponder { + calls: Arc, + prompt_marker: &'static str, + path: &'static str, + line: u32, + evidence: &'static str, +} + +#[cfg(feature = "qualification-candidate")] +impl Respond for ExactEvidenceRetryResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body: Value = request.body_json().unwrap(); + let system = body["messages"][0]["content"].as_str().unwrap(); + let user = body["messages"][1]["content"].as_str().unwrap(); + if system.contains("select bounded code-review batches") { + return ResponseTemplate::new(200).set_body_json(json!({ + "choices": [{"finish_reason": "stop", "message": {"content": "{\"batchIds\":[]}"}}], + "usage": {"prompt_tokens": 100, "completion_tokens": 10, "cost": 0.000001} + })); + } + if !user.contains(self.prompt_marker) { + return ResponseTemplate::new(200).set_body_json(llm_content(json!([]))); + } + + self.calls.fetch_add(1, Ordering::SeqCst); + let correction = user.contains("[Correction]"); + if correction { + let expected = format!( + "must set `evidence` to the exact JSON string {}", + serde_json::to_string(self.evidence).unwrap() + ); + assert!( + user.contains(&expected), + "correction did not include source-exact evidence: {user}" + ); + } + ResponseTemplate::new(200).set_body_json(llm_content(json!([{ + "path": self.path, + "line": self.line, + "severity": "warn", + "kind": "risk", + "confidence": 0.95, + "title": "Keep the validated value", + "body": "The sink uses the unvalidated input. Pass the validated value instead.", + "evidence": if correction { self.evidence } else { "approximate evidence" } + }]))) + } +} + impl Respond for SequentialReviewResponder { fn respond(&self, _request: &Request) -> ResponseTemplate { let index = self.calls.fetch_add(1, Ordering::SeqCst); @@ -2244,6 +2295,96 @@ async fn final_synthesis_detects_cross_batch_validation_sink_relationship() { ); } +#[cfg(feature = "qualification-candidate")] +#[tokio::test] +async fn bounded_synthesis_repairs_source_exact_evidence_without_relaxing_validation() { + use std::fmt::Write as _; + + let server = MockServer::start().await; + let correction_calls = Arc::new(AtomicUsize::new(0)); + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ExactEvidenceRetryResponder { + calls: correction_calls.clone(), + prompt_marker: "dangerous_sink(original)", + path: "src/sink.rs", + line: 1100, + evidence: "dangerous_sink(original);", + }) + .mount(&server) + .await; + + let mut source = String::new(); + for (path_name, marker) in [ + ( + "src/validate.rs", + "let validated = validate_pair(left, right);", + ), + ("src/sink.rs", "dangerous_sink(original);"), + ] { + writeln!(source, "diff --git a/{path_name} b/{path_name}").unwrap(); + writeln!( + source, + "--- /dev/null\n+++ b/{path_name}\n@@ -0,0 +1,2201 @@" + ) + .unwrap(); + for line in 1..=2201 { + if line == 1100 { + writeln!(source, "+{marker}").unwrap(); + } else { + writeln!( + source, + "+let padding_{line:04} = trusted; // {}", + "x".repeat(1_000) + ) + .unwrap(); + } + } + } + + let dir = tempfile::tempdir().unwrap(); + let diff = dir.path().join("bounded-synthesis.diff"); + std::fs::write(&diff, source).unwrap(); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_ALLOW_PRIVATE_API_BASE", "1") + .env("GITHUB_API_URL", server.uri()) + .env("CI", "true") + .env("POSTIL_BENCH_FORCE_BOUNDED_SELECTION", "1") + .env("POSTIL_DISABLE_SCORER", "1") + .env("REVIEW_MODEL", "fixture/model") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .success(); + + let envelope: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + assert_eq!(envelope["reviewCoverage"]["mode"], "bounded"); + assert_eq!(envelope["findings"][0]["path"], "src/sink.rs"); + assert_eq!( + envelope["findings"][0]["evidence"], + "dangerous_sink(original);" + ); + assert_eq!(correction_calls.load(Ordering::SeqCst), 2); + + let requests = server.received_requests().await.unwrap(); + let review_requests = requests + .iter() + .filter(|request| { + request.body_json::().unwrap()["messages"][0]["content"] + .as_str() + .is_some_and(|system| !system.contains("select bounded code-review batches")) + }) + .collect::>(); + assert!(review_requests.len() >= 2); + assert!(review_requests.iter().all(|request| { + let body: Value = request.body_json().unwrap(); + body["max_tokens"] == 8_000 && body.get("response_format").is_none() + })); +} + #[tokio::test] async fn oversized_line_tail_remains_reviewable() { let server = MockServer::start().await; @@ -2446,6 +2587,83 @@ async fn staged_diff_compacts_large_generated_noise_and_reviews_late_source() { ); } +#[tokio::test] +async fn staged_review_cascades_after_bad_grounding_without_publishing() { + let server = MockServer::start().await; + let mut invalid = finding_at(41, "warn", 0.95); + invalid["evidence"] = json!("approximately the changed line"); + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(body_string_contains("primary-model")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([invalid])))) + .expect(2) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(body_string_contains("backup-model")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(llm_content(json!([finding_at(41, "warn", 0.95)]))), + ) + .expect(1) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + assert!( + std::process::Command::new("git") + .args(["init", "--quiet"]) + .current_dir(dir.path()) + .status() + .unwrap() + .success() + ); + std::fs::create_dir(dir.path().join("src")).unwrap(); + std::fs::write( + dir.path().join("src/auth.rs"), + format!( + "{}let token = format!(\"{{}}\", user_input);\nexec_query(&token);\n", + "\n".repeat(40) + ), + ) + .unwrap(); + assert!( + std::process::Command::new("git") + .args(["add", "src/auth.rs"]) + .current_dir(dir.path()) + .status() + .unwrap() + .success() + ); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("POSTIL_DISABLE_SCORER", "1") + .env("REVIEW_MODEL", "primary-model") + .env("REVIEW_MODEL_CASCADE", "backup-model") + .args(["review", "--staged", "--output", "json"]) + .assert() + .success(); + let envelope: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + assert_eq!(envelope["findings"][0]["path"], "src/auth.rs"); + assert_eq!(envelope["modelUsed"], "backup-model"); + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 3); + assert!( + requests + .iter() + .all(|request| request.url.path() == "/chat/completions") + ); + assert!(requests.iter().all(|request| { + let body: Value = request.body_json().unwrap(); + body["max_tokens"] == 8_000 && body.get("response_format").is_none() + })); +} + #[tokio::test] async fn lockfile_only_diff_is_reviewed_from_compact_dependency_evidence() { let server = MockServer::start().await;