diff --git a/Cargo.lock b/Cargo.lock index 0096642..49c00e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1056,7 +1056,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.7.6" +version = "0.7.7" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index a307717..1c97ca4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.7.6" +version = "0.7.7" edition = "2024" description = "Postil: a low-noise AI review gate. Silent on clean PRs, hard gate on real risk." license = "Apache-2.0" diff --git a/src/diff.rs b/src/diff.rs index c560c55..1ce2005 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -6,7 +6,7 @@ //! with its new-file line number. use std::borrow::Cow; -use std::collections::{BTreeSet, HashMap}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::fs::File; use std::io::{Read, Seek, SeekFrom, Write}; use std::ops::RangeInclusive; @@ -408,6 +408,12 @@ pub struct DiffIndex { old_evidence: HashMap<(String, u32), String>, content_policy_evidence: HashMap<(String, u32), String>, rendered_evidence: HashMap<(String, u32), Vec>, + rendered_old_coordinates: HashSet<(String, u32)>, + /// Old-to-new paths for files renamed by the reviewed change. Baseline + /// findings cite the old head, so reconciliation must follow an unchanged + /// evidence line represented in the diff across a rename instead of + /// expiring or misplacing it. + renamed_paths: HashMap, } impl Default for DiffIndex { @@ -422,6 +428,8 @@ impl Default for DiffIndex { old_evidence: HashMap::new(), content_policy_evidence: HashMap::new(), rendered_evidence: HashMap::new(), + rendered_old_coordinates: HashSet::new(), + renamed_paths: HashMap::new(), } } } @@ -430,6 +438,11 @@ impl DiffIndex { pub fn build(diff: &Diff) -> Self { let mut index = Self::default(); for file in &diff.files { + if !file.deleted && file.old_path != file.path { + index + .renamed_paths + .insert(file.old_path.clone(), file.path.clone()); + } if file.binary { continue; } @@ -494,6 +507,9 @@ impl DiffIndex { } self.new_evidence.extend(next.new_evidence); self.old_evidence.extend(next.old_evidence); + self.rendered_old_coordinates + .extend(next.rendered_old_coordinates); + self.renamed_paths.extend(next.renamed_paths); } fn insert_range(&mut self, path: String, range: RangeInclusive, old: bool) { @@ -572,6 +588,13 @@ impl DiffIndex { let Some(path) = current_path.as_ref() else { continue; }; + if let Some(old) = line.strip_prefix("old ") + && let Some(number) = old.split_whitespace().next() + && let Ok(number) = number.parse::() + { + self.rendered_old_coordinates.insert((path.clone(), number)); + continue; + } let Some((number, marked)) = line.trim_start().split_once(' ') else { continue; }; @@ -646,13 +669,105 @@ impl DiffIndex { }) } - pub fn remap_evidence(&self, finding: &crate::envelope::Finding) -> Option { - let evidence = finding.evidence.as_ref()?; - self.new_evidence.iter().find_map(|((path, line), actual)| { - (path == &finding.path && actual == evidence).then_some(*line) + fn nearest_evidence_anchor<'a, I>( + finding: &crate::envelope::Finding, + candidates: I, + ) -> Option<(String, u32)> + where + I: IntoIterator, + { + let evidence = finding.evidence.as_deref()?; + candidates + .into_iter() + .filter(|((path, _), actual)| path == &finding.path && actual.as_str() == evidence) + .map(|((path, line), _)| (path.clone(), *line)) + .min_by_key(|(path, line)| { + ( + (path != &finding.path) as u8, + line.abs_diff(finding.line), + *line, + ) + }) + } + + /// Locate the baseline's exact evidence in the current reviewed head. + /// Duplicate text resolves deterministically to the nearest line, and a + /// pure rename follows the new path. + pub fn remap_current_evidence( + &self, + finding: &crate::envelope::Finding, + ) -> Option<(String, u32)> { + let current_path = self + .renamed_paths + .get(&finding.path) + .unwrap_or(&finding.path); + let mut candidate = finding.clone(); + candidate.path.clone_from(current_path); + Self::nearest_evidence_anchor(&candidate, self.new_evidence.iter()).or_else(|| { + Self::nearest_evidence_anchor(&candidate, self.content_policy_evidence.iter()) + }) + } + + /// Locate exact baseline evidence that a selected model request actually + /// contained. A bounded full review may resolve a reproduced anchor only + /// when that anchor was inside its reviewed evidence. + pub fn remap_reviewed_evidence( + &self, + finding: &crate::envelope::Finding, + ) -> Option<(String, u32)> { + let evidence = finding.evidence.as_deref()?; + let current_path = self + .renamed_paths + .get(&finding.path) + .unwrap_or(&finding.path); + let mut candidates = self + .rendered_evidence + .iter() + .filter(|((path, _), values)| { + path == current_path && values.iter().any(|actual| actual == evidence) + }) + .map(|((path, line), _)| (path.clone(), *line)) + .collect::>(); + if finding.kind == crate::envelope::Kind::ContentPolicy { + candidates.extend( + self.content_policy_evidence + .iter() + .filter(|((path, _), actual)| path == current_path && *actual == evidence) + .map(|((path, line), _)| (path.clone(), *line)), + ); + } + candidates.into_iter().min_by_key(|(path, line)| { + ( + (path != &finding.path) as u8, + line.abs_diff(finding.line), + *line, + ) }) } + /// True only when a selected model request contained the baseline's exact + /// old coordinate or the corresponding current coordinate. This lets a + /// completed bounded review resolve changed evidence it actually saw while + /// keeping changed evidence outside the selected requests open. + pub fn contains_reviewed_baseline_coordinate( + &self, + finding: &crate::envelope::Finding, + ) -> bool { + if self + .rendered_old_coordinates + .contains(&(finding.path.clone(), finding.line)) + { + return true; + } + let current_path = self + .renamed_paths + .get(&finding.path) + .unwrap_or(&finding.path); + self.rendered_evidence + .keys() + .any(|(path, line)| path == current_path && *line == finding.line) + } + pub fn contains(&self, path: &str, line: u32) -> bool { self.ranges .get(path) diff --git a/src/filter.rs b/src/filter.rs index 2fdde6e..dafe2ba 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -212,16 +212,21 @@ pub struct Reconciliation { pub carried: Vec, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReviewTrust { + /// At least one required model request failed or returned unusable output. + Failed, + /// Every selected request completed, but the model saw only a bounded + /// subset of the complete review input. + Bounded, + /// Every source batch in the review input completed successfully. + Exhaustive, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ReconcileScope { - Incremental { - /// A complete incremental review may resolve a changed baseline anchor. - /// Bounded selection cannot prove that an unselected anchor is fixed. - trustworthy: bool, - }, - /// A complete full review replaces the previous review's signal. When the - /// model run is not trustworthy, baseline findings remain carried. - Full { trustworthy: bool }, + Incremental { trust: ReviewTrust }, + Full { trust: ReviewTrust }, } pub const CARRIED_MARKER: &str = "[carried from previous review]"; @@ -284,7 +289,7 @@ fn defect_identity(finding: &Finding) -> (String, crate::envelope::Kind, String, fn touch_addresses(index: &DiffIndex, f: &Finding, scope: ReconcileScope) -> bool { match scope { ReconcileScope::Incremental { .. } => index.contains_old(&f.path, f.line), - ReconcileScope::Full { trustworthy } => trustworthy, + ReconcileScope::Full { trust } => trust == ReviewTrust::Exhaustive, } } @@ -330,19 +335,56 @@ pub fn reconcile( // copy is already in `new_findings` and will reach the gate. continue; } - if let ReconcileScope::Incremental { trustworthy } = scope + if let ReconcileScope::Incremental { trust } = scope && index.contains_old(&f.path, f.line) { if index.old_evidence_matches(f) - && let Some(line) = index.remap_evidence(f) + && let Some((path, line)) = index.remap_current_evidence(f) { let mut carry = f.clone(); + carry.path = path; carry.line = line; carry.end_line = None; push_carried(&mut carried, &mut carried_identities, carry); - } else if trustworthy { + } else if trust == ReviewTrust::Exhaustive + || (trust == ReviewTrust::Bounded + && f.evidence.is_some() + && index.contains_reviewed_baseline_coordinate(f)) + { + resolved.push(f.clone()); + } else { + push_carried(&mut carried, &mut carried_identities, f.clone()); + } + } else if let ReconcileScope::Full { + trust: ReviewTrust::Bounded, + } = scope + { + if let Some((path, line)) = index.remap_current_evidence(f) { + if index.remap_reviewed_evidence(f).as_ref() == Some(&(path.clone(), line)) { + // The selected model input contained this exact current + // anchor and the model did not reproduce it. + resolved.push(f.clone()); + } else { + // The issue's evidence remains in an unselected part of + // the full diff. Keep it open at its current coordinate. + let mut carry = f.clone(); + carry.path = path; + carry.line = line; + carry.end_line = None; + push_carried(&mut carried, &mut carried_identities, carry); + } + } else if f.evidence.is_some() + && f.path != crate::envelope::CHANGE_METADATA_PATH + && index.contains_reviewed_baseline_coordinate(f) + { + // The selected input covered this coordinate and the complete + // current diff no longer contains the exact citation. The + // completed model request did not reproduce the issue. resolved.push(f.clone()); } else { + // Changed evidence outside the selected input, historical + // findings without canonical evidence, and virtual change + // metadata remain open. push_carried(&mut carried, &mut carried_identities, f.clone()); } } else if touch_addresses(index, f, scope) { @@ -642,7 +684,9 @@ mod tests { &baseline, &idx, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.resolved.len(), 1); assert_eq!(rec.resolved[0].path, "a.rs"); @@ -671,7 +715,9 @@ mod tests { &baseline, &idx, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert!(rec.resolved.is_empty()); assert_eq!(rec.carried.len(), 2); @@ -700,7 +746,9 @@ mod tests { &[baseline], &idx, &[fresh], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.carried.len(), 1); assert!(rec.resolved.is_empty()); @@ -715,7 +763,9 @@ mod tests { &baseline, &idx, &new, - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert!(rec.resolved.is_empty()); assert!(rec.carried.is_empty()); @@ -733,7 +783,9 @@ mod tests { &baseline, &idx, &new, - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.resolved.len(), 0); assert_eq!(rec.carried.len(), 1, "baseline Error was dropped"); @@ -752,7 +804,9 @@ mod tests { &baseline, &idx, &new, - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert!(rec.resolved.is_empty()); assert!(rec.carried.is_empty(), "same-issue reflag should supersede"); @@ -770,7 +824,9 @@ mod tests { &[wide], &idx, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.resolved.len(), 0, "wide-span finding falsely resolved"); assert_eq!(rec.carried.len(), 1); @@ -805,7 +861,9 @@ mod tests { &[baseline], &idx, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.resolved.len(), 1); assert!(rec.carried.is_empty()); @@ -821,7 +879,9 @@ mod tests { std::slice::from_ref(&baseline), &shifted, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(remapped.carried.len(), 1); assert_eq!(remapped.carried[0].line, 11); @@ -834,7 +894,9 @@ mod tests { &[baseline], &changed, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert!(expired.carried.is_empty()); assert_eq!(expired.resolved.len(), 1); @@ -843,11 +905,25 @@ mod tests { &[f("a.rs", 10, Severity::Error, 0.9)], &changed, &[], - ReconcileScope::Incremental { trustworthy: false }, + ReconcileScope::Incremental { + trust: ReviewTrust::Bounded, + }, ); assert!(bounded.resolved.is_empty()); assert_eq!(bounded.carried.len(), 1); - assert!(is_carried(&bounded.carried[0])); + + let mut selected_change = changed.clone(); + selected_change.add_rendered_evidence("### a.rs\nold 10 - x\n 10 + y\n"); + let selected = reconcile( + &[f("a.rs", 10, Severity::Error, 0.9)], + &selected_change, + &[], + ReconcileScope::Incremental { + trust: ReviewTrust::Bounded, + }, + ); + assert_eq!(selected.resolved.len(), 1); + assert!(selected.carried.is_empty()); } #[test] @@ -862,7 +938,9 @@ mod tests { &[baseline], &whitespace_change, &[], - ReconcileScope::Incremental { trustworthy: true }, + ReconcileScope::Incremental { + trust: ReviewTrust::Exhaustive, + }, ); assert!(result.carried.is_empty()); @@ -877,12 +955,136 @@ mod tests { &baseline, &idx, &[], - ReconcileScope::Full { trustworthy: true }, + ReconcileScope::Full { + trust: ReviewTrust::Exhaustive, + }, ); assert_eq!(rec.resolved.len(), 1); assert!(rec.carried.is_empty()); } + #[test] + fn bounded_full_review_carries_changed_unselected_baseline_evidence() { + let changed = DiffIndex::build(&diff::parse( + "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -10 +10 @@\n-x\n+y\n", + )); + let baseline = f("a.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &changed, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert!(rec.resolved.is_empty()); + assert_eq!(rec.carried.len(), 1); + } + + #[test] + fn bounded_full_review_resolves_changed_selected_baseline_evidence() { + let mut changed = DiffIndex::build(&diff::parse( + "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -10 +10 @@\n-x\n+y\n", + )); + changed.add_rendered_evidence("### a.rs\nold 10 - x\n 10 + y\n"); + let baseline = f("a.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &changed, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert_eq!(rec.resolved.len(), 1); + assert!(rec.carried.is_empty()); + } + + #[test] + fn bounded_full_review_carries_unselected_current_evidence() { + let shifted = DiffIndex::build(&diff::parse( + "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -10 +11 @@\n x\n", + )); + let baseline = f("a.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &shifted, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert!(rec.resolved.is_empty()); + assert_eq!(rec.carried.len(), 1); + assert_eq!(rec.carried[0].line, 11); + assert!(is_carried(&rec.carried[0])); + } + + #[test] + fn bounded_full_review_resolves_selected_evidence_not_reproduced() { + let mut unchanged = DiffIndex::build(&diff::parse( + "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -10 +10 @@\n x\n", + )); + unchanged.add_rendered_evidence("### a.rs\n@@ starting at line 10 @@\n 10 x\n"); + let baseline = f("a.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &unchanged, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert_eq!(rec.resolved.len(), 1); + assert!(rec.carried.is_empty()); + } + + #[test] + fn bounded_full_review_does_not_confuse_duplicate_selected_evidence() { + let mut duplicate = DiffIndex::build(&diff::parse( + "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -10 +10 @@\n x\n@@ -20 +20 @@\n x\n", + )); + duplicate.add_rendered_evidence("### a.rs\n@@ starting at line 20 @@\n 20 x\n"); + let baseline = f("a.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &duplicate, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert!(rec.resolved.is_empty()); + assert_eq!(rec.carried.len(), 1); + assert_eq!(rec.carried[0].line, 10); + } + + #[test] + fn bounded_full_review_remaps_evidence_across_rename() { + let renamed = DiffIndex::build(&diff::parse( + "diff --git a/old.rs b/new.rs\n--- a/old.rs\n+++ b/new.rs\n@@ -10 +12 @@\n x\n", + )); + let baseline = f("old.rs", 10, Severity::Error, 0.9); + let rec = reconcile( + &[baseline], + &renamed, + &[], + ReconcileScope::Full { + trust: ReviewTrust::Bounded, + }, + ); + + assert!(rec.resolved.is_empty()); + assert_eq!(rec.carried.len(), 1); + assert_eq!(rec.carried[0].path, "new.rs"); + assert_eq!(rec.carried[0].line, 12); + } + #[test] fn failed_full_review_keeps_baseline_findings_open() { let idx = index_for("a.rs", 1, 100); @@ -891,7 +1093,9 @@ mod tests { &baseline, &idx, &[], - ReconcileScope::Full { trustworthy: false }, + ReconcileScope::Full { + trust: ReviewTrust::Failed, + }, ); assert!(rec.resolved.is_empty()); assert_eq!(rec.carried.len(), 1); diff --git a/src/review.rs b/src/review.rs index fd5fbf2..dc6cf33 100644 --- a/src/review.rs +++ b/src/review.rs @@ -404,10 +404,17 @@ async fn run_local(args: &ReviewArgs, cfg: &Config) -> Result { let diff_snapshot = local::acquire(&source).await?; let head_sha = local::head_sha().await; let baseline = load_baseline(args)?; + // This is a fail-closed placeholder, not the completed review's trust. + // review_diff replaces it with Failed, Bounded, or Exhaustive after the + // selected requests finish; an early model failure deliberately keeps it. let scope = if args.since_sha.is_some() { - filter::ReconcileScope::Incremental { trustworthy: false } + filter::ReconcileScope::Incremental { + trust: filter::ReviewTrust::Failed, + } } else { - filter::ReconcileScope::Full { trustworthy: false } + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + } }; let envelope = review_diff( cfg, @@ -728,13 +735,17 @@ async fn remote_review( .map(|diff| { ( diff, - filter::ReconcileScope::Incremental { trustworthy: false }, + filter::ReconcileScope::Incremental { + trust: filter::ReviewTrust::Failed, + }, false, ) })?, Some(_) => ( diff::DiffSnapshot::from_bytes(b"")?, - filter::ReconcileScope::Incremental { trustworthy: false }, + filter::ReconcileScope::Incremental { + trust: filter::ReviewTrust::Failed, + }, false, ), None => ( @@ -747,7 +758,9 @@ async fn remote_review( .await .map_err(crate::forge::classify_review_input_error) .context("diff fetch")?, - filter::ReconcileScope::Full { trustworthy: false }, + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + }, false, ), }; @@ -770,7 +783,9 @@ async fn remote_review( .await .map_err(crate::forge::classify_review_input_error) .context("full diff fallback fetch")?, - filter::ReconcileScope::Full { trustworthy: false }, + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + }, true, ) } else { @@ -1053,7 +1068,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> let mut ungrounded = 0u32; let mut findings: Vec = Vec::new(); let mut suppressed_findings = Vec::new(); - let mut full_review_trustworthy = false; + let mut review_trust = filter::ReviewTrust::Failed; let mut scorer_model: Option = None; let mut scorer_error: Option = None; let mut scorer_disagreements: Option = None; @@ -1115,7 +1130,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> index.add_change_metadata(batches.metadata_count); if batches.count == 0 { model_used = "none (empty diff)".to_string(); - full_review_trustworthy = true; + review_trust = filter::ReviewTrust::Exhaustive; } else { let bounded_candidates = if (args.bounded || crate::config::bounded_review_selection_mode()) @@ -1324,7 +1339,12 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> let first = request_index == 0; let (annotated, user, cross_window_synthesis) = review_batch_prompt(&runtime_prompt_context, batch, first); - index.add_rendered_evidence(&annotated); + // Synthesis digests can ground new cross-window findings, but + // they are lossy summaries rather than direct source coverage. + // Only selected source requests may retire baseline evidence. + if !cross_window_synthesis { + index.add_rendered_evidence(&annotated); + } eprintln!( "postil: reviewing {} request {}/{} ({} bytes)", if cross_window_synthesis { @@ -1500,10 +1520,17 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> )]; } else { // Bounded mode reviews deterministic direct evidence and - // lossy synthesis, not every source batch. It may add new - // findings, but it cannot prove that an unseen baseline - // finding is resolved. - full_review_trustworthy = !batch_failed && !risk_selected_review; + // lossy synthesis, not every source batch. Reconciliation + // carries baseline evidence outside the selected input. + // A changed citation expires only when a completed model + // request covered its coordinate and did not reproduce it. + review_trust = if batch_failed { + filter::ReviewTrust::Failed + } else if risk_selected_review { + filter::ReviewTrust::Bounded + } else { + filter::ReviewTrust::Exhaustive + }; summary = summary_parts.join("\n\n"); let mut kept = outcome.kept; if !kept.is_empty() && cfg.scorer_enabled() { @@ -1609,10 +1636,10 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> let rec = if cfg.enabled { let scope = match scope { filter::ReconcileScope::Incremental { .. } => filter::ReconcileScope::Incremental { - trustworthy: full_review_trustworthy, + trust: review_trust, }, filter::ReconcileScope::Full { .. } => filter::ReconcileScope::Full { - trustworthy: full_review_trustworthy, + trust: review_trust, }, }; filter::reconcile(&baseline, &index, &findings, scope) diff --git a/tests/e2e.rs b/tests/e2e.rs index f31a0a2..b4c9996 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -2390,16 +2390,32 @@ async fn local_bounded_is_explicit_and_default_local_review_remains_exhaustive() } #[tokio::test] -async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_batches() { +async fn bounded_reviews_resolve_changed_prior_evidence_when_selected() { use std::fmt::Write as _; + fn batch_id_containing(prompt: &str, needle: &str) -> usize { + prompt + .split("Batch ") + .skip(1) + .find_map(|block| { + let (id, _) = block.split_once(' ')?; + block.contains(needle).then(|| id.parse::().unwrap()) + }) + .expect("planner manifest contains the baseline path") + } + let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/chat/completions")) .respond_with(|request: &wiremock::Request| { let body = String::from_utf8_lossy(&request.body); if body.contains("select bounded code-review batches") { - ResponseTemplate::new(200).set_body_json(llm_text(r#"{"batchIds":[]}"#)) + let request: Value = serde_json::from_slice(&request.body).unwrap(); + let prompt = request["messages"][1]["content"].as_str().unwrap(); + ResponseTemplate::new(200).set_body_json(llm_text(&format!( + r#"{{"batchIds":[{}]}}"#, + batch_id_containing(prompt, "src/churn-3.rs") + ))) } else { ResponseTemplate::new(200).set_body_json(llm_content(json!([]))) } @@ -2425,6 +2441,11 @@ async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_bat ) .unwrap(); } + writeln!( + diff, + "@@ -100 +230 @@\n-const LATE_ORIGINAL_{file}: &str = \"old\";\n+const LATE_UPDATED_{file}: &str = \"new\";" + ) + .unwrap(); } std::fs::write(&diff_path, diff).unwrap(); @@ -2432,7 +2453,7 @@ async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_bat "version": 1, "summary": "", "silent": false, "findings": [{ "path": "src/churn-3.rs", "line": 1, "severity": "error", "kind": "risk", - "confidence": 0.9, "title": "prior middle finding", "body": "requires direct re-review", + "confidence": 0.9, "title": "prior middle finding", "body": "the cited line must remain current", "evidence": "const ORIGINAL_3: &str = \"old\";" }], "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, @@ -2454,7 +2475,7 @@ async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_bat .arg(&baseline_path) .args(["--output", "json"]) .assert() - .code(1); + .code(0); let envelope: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); assert_eq!(envelope["reviewCoverage"]["mode"], "bounded"); assert!( @@ -2463,14 +2484,10 @@ async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_bat .unwrap() < envelope["reviewCoverage"]["totalBatches"].as_u64().unwrap() ); - assert_eq!(envelope["resolved"], json!([])); - assert_eq!(envelope["findings"][0]["title"], "prior middle finding"); - assert!( - envelope["findings"][0]["body"] - .as_str() - .unwrap() - .starts_with("[carried") - ); + assert_eq!(envelope["resolved"][0]["title"], "prior middle finding"); + assert_eq!(envelope["findings"], json!([])); + assert_eq!(envelope["counts"]["error"], 0); + assert_eq!(envelope["gate"]["failing"], false); let incremental = postil() .current_dir(dir.path()) @@ -2482,26 +2499,65 @@ async fn bounded_reviews_carry_changed_prior_findings_from_non_direct_source_bat .arg(&baseline_path) .args(["--output", "json"]) .assert() - .code(1); + .code(0); let incremental_envelope: Value = serde_json::from_slice(&incremental.get_output().stdout).unwrap(); assert_eq!(incremental_envelope["reviewCoverage"]["mode"], "bounded"); - assert_eq!(incremental_envelope["resolved"], json!([])); assert_eq!( - incremental_envelope["findings"][0]["title"], + incremental_envelope["resolved"][0]["title"], "prior middle finding" ); - let carried_body = incremental_envelope["findings"][0]["body"] - .as_str() - .unwrap(); - assert!(carried_body.starts_with("[carried from previous review]")); - assert_eq!( - carried_body - .matches("[carried from previous review]") - .count(), - 1 + assert_eq!(incremental_envelope["findings"], json!([])); + assert_eq!(incremental_envelope["counts"]["error"], 0); + assert_eq!(incremental_envelope["gate"]["failing"], false); + + let requests = server.received_requests().await.unwrap(); + let unselected_file = (0..7) + .find(|file| { + let marker = format!("LATE_ORIGINAL_{file}"); + requests.iter().all(|request| { + let body = String::from_utf8_lossy(&request.body); + !body.contains("Review this selected source batch independently") + || !body.contains(&marker) + }) + }) + .expect("bounded review leaves at least one source batch unselected"); + let unselected_baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [{ + "path": format!("src/churn-{unselected_file}.rs"), "line": 100, + "severity": "error", "kind": "risk", "confidence": 0.9, + "title": "unselected prior finding", "body": "the cited line must remain current", + "evidence": format!("const LATE_ORIGINAL_{unselected_file}: &str = \"old\";") + }], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "model", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": null, "headSha": null, "sinceSha": null + }); + std::fs::write(&baseline_path, unselected_baseline.to_string()).unwrap(); + let carried = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_DISABLE_SCORER", "1") + .args(["review", "--bounded", "--diff-file"]) + .arg(&diff_path) + .arg("--baseline") + .arg(&baseline_path) + .args(["--output", "json"]) + .assert() + .code(1); + let carried_envelope: Value = serde_json::from_slice(&carried.get_output().stdout).unwrap(); + assert_eq!(carried_envelope["resolved"], json!([])); + assert_eq!(carried_envelope["counts"]["error"], 1); + assert_eq!(carried_envelope["gate"]["failing"], true); + assert!( + carried_envelope["findings"][0]["body"] + .as_str() + .unwrap() + .starts_with("[carried from previous review]") ); - assert_eq!(incremental_envelope["gate"]["failing"], true); } #[tokio::test]