diff --git a/src/diff.rs b/src/diff.rs index 1e81378..74b35f5 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -10,6 +10,8 @@ use std::ops::RangeInclusive; #[derive(Debug, Clone)] pub struct Hunk { + pub old_start: u32, + pub old_count: u32, pub new_start: u32, pub new_count: u32, /// Raw hunk lines including leading ' ', '+', '-'. @@ -17,6 +19,10 @@ pub struct Hunk { } impl Hunk { + pub fn old_range(&self) -> RangeInclusive { + self.old_start..=self.old_start + self.old_count.saturating_sub(1) + } + pub fn new_range(&self) -> RangeInclusive { // A zero-count hunk (pure deletion) still anchors at new_start. self.new_start..=self.new_start + self.new_count.saturating_sub(1) @@ -25,6 +31,9 @@ impl Hunk { #[derive(Debug, Clone)] pub struct FileDiff { + /// Old-side path ("a/" stripped). Used to reconcile findings anchored to + /// the previously reviewed head, including across renames and deletions. + pub old_path: String, /// New-side path ("b/" stripped). Deleted files keep the old path. pub path: String, pub deleted: bool, @@ -47,6 +56,7 @@ impl Diff { #[derive(Debug, Default)] pub struct DiffIndex { ranges: HashMap>>, + old_ranges: HashMap>>, /// Reserved synthetic-path line ranges that only content-policy findings may /// ground against (e.g. the rendered PR title/description). Kept separate /// from `ranges` so a non-content-policy finding cannot exploit them. @@ -56,8 +66,20 @@ pub struct DiffIndex { impl DiffIndex { pub fn build(diff: &Diff) -> Self { let mut ranges: HashMap>> = HashMap::new(); + let mut old_ranges: HashMap>> = HashMap::new(); for file in &diff.files { - if file.deleted || file.binary { + if file.binary { + continue; + } + for hunk in &file.hunks { + if hunk.old_count > 0 { + old_ranges + .entry(file.old_path.clone()) + .or_default() + .push(hunk.old_range()); + } + } + if file.deleted { continue; } let entry = ranges.entry(file.path.clone()).or_default(); @@ -69,6 +91,7 @@ impl DiffIndex { } DiffIndex { ranges, + old_ranges, content_policy_ranges: HashMap::new(), } } @@ -98,6 +121,16 @@ impl DiffIndex { .is_some_and(|rs| rs.iter().any(|r| r.contains(&line))) } + /// True when `(path, line)` falls on the old side of a changed hunk. A + /// baseline finding cites the previously reviewed head, so incremental + /// reconciliation must use this coordinate space rather than new-file + /// grounding ranges. + pub fn contains_old(&self, path: &str, line: u32) -> bool { + self.old_ranges + .get(path) + .is_some_and(|rs| rs.iter().any(|r| r.contains(&line))) + } + /// True when the diff touches any line in [start, end] of `path`. /// Used for baseline reconciliation in incremental reviews. pub fn touches(&self, path: &str, start: u32, end: u32) -> bool { @@ -161,12 +194,18 @@ pub fn parse(text: &str) -> Diff { } // Seed the path from the header (binary diffs have no +++/--- lines); // the +++/--- lines that follow refine it for renames. - let seeded = rest + let (old_path, path) = rest .rsplit_once(" b/") - .map(|(_, b)| b.trim().to_string()) + .map(|(a, b)| { + ( + strip_prefix_ab(a).to_string(), + strip_prefix_ab(b).to_string(), + ) + }) .unwrap_or_default(); current = Some(FileDiff { - path: seeded, + old_path, + path, deleted: false, binary: false, hunks: Vec::new(), @@ -180,12 +219,14 @@ pub fn parse(text: &str) -> Diff { } } } else if let Some(rest) = line.strip_prefix("--- ") { - // Keep the old path as a fallback for deletions (+++ /dev/null case). if let Some(f) = current.as_mut() - && f.path.is_empty() && rest != "/dev/null" { - f.path = strip_prefix_ab(rest).to_string(); + f.old_path = strip_prefix_ab(rest).to_string(); + // Keep the old path as a fallback for deletions (+++ /dev/null). + if f.path.is_empty() { + f.path = f.old_path.clone(); + } } } else if line.starts_with("Binary files ") || line.starts_with("GIT binary patch") { if let Some(f) = current.as_mut() { @@ -193,8 +234,10 @@ pub fn parse(text: &str) -> Diff { } } else if let Some(header) = line.strip_prefix("@@ ") { flush_hunk(&mut current, &mut current_hunk); - if let Some((new_start, new_count, old_count)) = parse_hunk_header(header) { + if let Some((old_start, old_count, new_start, new_count)) = parse_hunk_header(header) { current_hunk = Some(Hunk { + old_start, + old_count, new_start, new_count, lines: Vec::new(), @@ -244,28 +287,26 @@ fn strip_prefix_ab(path: &str) -> &str { } /// "@@ -l,c +l,c @@ ctx" minus the leading "@@ ". Returns -/// (new_start, new_count, old_count). old_count defaults to 1 when the header -/// omits it (single-line hunk) and to 0 when the old side is absent. -fn parse_hunk_header(header: &str) -> Option<(u32, u32, u32)> { - let count_of = |token: &str| -> Option { +/// (old_start, old_count, new_start, new_count). Counts default to 1 when a +/// header omits them (single-line hunk). +fn parse_hunk_header(header: &str) -> Option<(u32, u32, u32, u32)> { + let range_of = |token: &str| -> Option<(u32, u32)> { let spec = &token[1..]; match spec.split_once(',') { - Some((_, c)) => c.parse().ok(), - None => Some(1), + Some((start, count)) => Some((start.parse().ok()?, count.parse().ok()?)), + None => Some((spec.parse().ok()?, 1)), } }; - let plus = header.split_whitespace().find(|t| t.starts_with('+'))?; - let spec = &plus[1..]; - let (new_start, new_count) = match spec.split_once(',') { - Some((s, c)) => (s.parse().ok()?, c.parse().ok()?), - None => (spec.parse().ok()?, 1), - }; - let old_count = header + let (old_start, old_count) = header .split_whitespace() .find(|t| t.starts_with('-')) - .and_then(count_of) - .unwrap_or(0); - Some((new_start, new_count, old_count)) + .and_then(range_of) + .unwrap_or((0, 0)); + let (new_start, new_count) = header + .split_whitespace() + .find(|t| t.starts_with('+')) + .and_then(range_of)?; + Some((old_start, old_count, new_start, new_count)) } /// Render the diff for the model with new-file line numbers on every line that @@ -376,7 +417,10 @@ Binary files a/img.png and b/img.png differ let d = parse(SAMPLE); assert_eq!(d.files.len(), 3); assert_eq!(d.files[0].path, "src/lib.rs"); + assert_eq!(d.files[0].old_path, "src/lib.rs"); assert_eq!(d.files[0].hunks.len(), 1); + assert_eq!(d.files[0].hunks[0].old_start, 10); + assert_eq!(d.files[0].hunks[0].old_count, 4); assert_eq!(d.files[0].hunks[0].new_start, 10); assert_eq!(d.files[0].hunks[0].new_count, 5); assert!(d.files[1].deleted); @@ -394,6 +438,10 @@ Binary files a/img.png and b/img.png differ assert!(!idx.contains("src/lib.rs", 9)); assert!(!idx.contains("gone.txt", 1)); assert!(!idx.contains("img.png", 1)); + assert!(idx.contains_old("src/lib.rs", 10)); + assert!(idx.contains_old("src/lib.rs", 13)); + assert!(!idx.contains_old("src/lib.rs", 14)); + assert!(idx.contains_old("gone.txt", 1)); assert!(idx.touches("src/lib.rs", 1, 10)); assert!(!idx.touches("src/lib.rs", 1, 9)); } @@ -428,6 +476,7 @@ Binary files a/img.png and b/img.png differ let mut diff = Diff::default(); for i in 0..5000 { diff.files.push(FileDiff { + old_path: format!("path/to/generated/file_{i:05}.bin"), path: format!("path/to/generated/file_{i:05}.bin"), deleted: false, binary: true, @@ -488,9 +537,30 @@ Binary files a/img.png and b/img.png differ #[test] fn hunk_header_without_count() { - assert_eq!(parse_hunk_header("-1 +5 @@"), Some((5, 1, 1))); - assert_eq!(parse_hunk_header("-1,2 +3,4 @@"), Some((3, 4, 2))); - assert_eq!(parse_hunk_header("-0,0 +1,3 @@"), Some((1, 3, 0))); + assert_eq!(parse_hunk_header("-1 +5 @@"), Some((1, 1, 5, 1))); + assert_eq!(parse_hunk_header("-1,2 +3,4 @@"), Some((1, 2, 3, 4))); + assert_eq!(parse_hunk_header("-0,0 +1,3 @@"), Some((0, 0, 1, 3))); + } + + #[test] + fn old_side_index_uses_pre_change_path_for_renames() { + let text = "\ +diff --git a/old.rs b/new.rs +similarity index 80% +rename from old.rs +rename to new.rs +--- a/old.rs ++++ b/new.rs +@@ -9,2 +9,2 @@ +-old ++new + context +"; + let idx = DiffIndex::build(&parse(text)); + assert!(idx.contains_old("old.rs", 9)); + assert!(!idx.contains_old("new.rs", 9)); + assert!(idx.contains("new.rs", 9)); + assert!(!idx.contains("old.rs", 9)); } // A bare blank line separating two concatenated file diffs must not be diff --git a/src/filter.rs b/src/filter.rs index b5cea79..a3a54e8 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -92,6 +92,22 @@ pub struct Reconciliation { pub carried: Vec, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReconcileScope { + Incremental, + /// A complete full review replaces the previous review's signal. When the + /// model run is not trustworthy, baseline findings remain carried. + Full { + trustworthy: bool, + }, +} + +pub const CARRIED_MARKER: &str = "[carried from previous review]"; + +pub fn is_carried(finding: &Finding) -> bool { + finding.body.starts_with(CARRIED_MARKER) +} + /// How close (in lines) a new finding must be to a baseline finding to be /// considered "the same spot". Kept small; proximity alone is not enough to /// supersede (see `supersedes`). @@ -111,7 +127,7 @@ fn supersedes(base: &Finding, new: &Finding) -> bool { && new.severity >= base.severity } -/// Whether the incremental diff plausibly ADDRESSES the baseline finding (so it +/// Whether the reviewed diff plausibly ADDRESSES the baseline finding (so it /// can be declared resolved). Interval overlap (`touches`) is too loose: a /// finding with a wide `end_line` span (e.g. 5..40) would be resolved by a /// single one-line edit anywhere inside it, even if the bug is untouched. We @@ -121,14 +137,23 @@ fn supersedes(base: &Finding, new: &Finding) -> bool { /// next run is the real confirmation we lack — but it removes the worst /// false-resolve (wide-span / distant-touch) and fails closed (carry) when the /// edit landed elsewhere in the span. -fn touch_addresses(incremental_index: &DiffIndex, f: &Finding) -> bool { - incremental_index.contains(&f.path, f.line) +/// +/// Incremental baselines cite the OLD head, so their anchors must be checked +/// against old-side hunk coordinates. A trustworthy full review is +/// authoritative over the complete PR and resolves any baseline issue the +/// fresh model run did not reproduce. +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, + } } pub fn reconcile( baseline: &[Finding], - incremental_index: &DiffIndex, + index: &DiffIndex, new_findings: &[Finding], + scope: ReconcileScope, ) -> Reconciliation { let mut resolved = Vec::new(); let mut carried = Vec::new(); @@ -144,20 +169,19 @@ pub fn reconcile( // copy is already in `new_findings` and will reach the gate. continue; } - if touch_addresses(incremental_index, f) { - // The author edited the exact line the issue was pinned to and the - // model did not re-flag it this run: treat as resolved. (Imperfect — - // a non-fixing edit to that line will also resolve it — but the - // anchor-line requirement is far tighter than span overlap, and a - // full re-review would re-detect a still-broken issue.) + if touch_addresses(index, f, scope) { + // An incremental edit touched the old-head anchor, or a trustworthy + // full review did not reproduce the issue: treat it as resolved. + // Incremental touch is imperfect because a non-fixing edit can also + // resolve it, but a full re-review re-detects a still-broken issue. resolved.push(f.clone()); } else { // Not superseded and the anchor line was not touched: the issue // persists. Carry it forward (fail-closed) so an unrelated nearby // finding or a distant in-span edit cannot clear the gate. let mut carry = f.clone(); - if !carry.body.contains("[carried from previous review]") { - carry.body = format!("[carried from previous review]\n\n{}", carry.body); + if !is_carried(&carry) { + carry.body = format!("{CARRIED_MARKER}\n\n{}", carry.body); } carried.push(carry); } @@ -187,7 +211,7 @@ mod tests { fn index_for(path: &str, start: u32, count: u32) -> DiffIndex { let text = format!( - "diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -1,{count} +{start},{count} @@\n{}", + "diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -{start},{count} +{start},{count} @@\n{}", "+x\n".repeat(count as usize) ); DiffIndex::build(&diff::parse(&text)) @@ -315,7 +339,7 @@ mod tests { f("b.rs", 5, Severity::Warn, 0.8), // untouched → carried f(".postil/model-output", 1, Severity::Error, 1.0), // synthetic → dropped ]; - let rec = reconcile(&baseline, &idx, &[]); + let rec = reconcile(&baseline, &idx, &[], ReconcileScope::Incremental); assert_eq!(rec.resolved.len(), 1); assert_eq!(rec.resolved[0].path, "a.rs"); assert_eq!(rec.carried.len(), 1); @@ -328,7 +352,7 @@ mod tests { let idx = index_for("a.rs", 10, 3); let baseline = vec![f("a.rs", 11, Severity::Error, 0.9)]; let new = vec![f("a.rs", 12, Severity::Error, 0.95)]; - let rec = reconcile(&baseline, &idx, &new); + let rec = reconcile(&baseline, &idx, &new, ReconcileScope::Incremental); assert!(rec.resolved.is_empty()); assert!(rec.carried.is_empty()); } @@ -341,7 +365,7 @@ mod tests { let idx = index_for("z.rs", 100, 1); // incremental diff is elsewhere let baseline = vec![f("a.rs", 10, Severity::Error, 0.9)]; let new = vec![f("a.rs", 12, Severity::Info, 0.9)]; // unrelated, lower sev - let rec = reconcile(&baseline, &idx, &new); + let rec = reconcile(&baseline, &idx, &new, ReconcileScope::Incremental); assert_eq!(rec.resolved.len(), 0); assert_eq!(rec.carried.len(), 1, "baseline Error was dropped"); assert_eq!(rec.carried[0].severity, Severity::Error); @@ -355,7 +379,7 @@ mod tests { let idx = index_for("z.rs", 100, 1); let baseline = vec![f("a.rs", 10, Severity::Warn, 0.9)]; let new = vec![f("a.rs", 11, Severity::Error, 0.9)]; - let rec = reconcile(&baseline, &idx, &new); + let rec = reconcile(&baseline, &idx, &new, ReconcileScope::Incremental); assert!(rec.resolved.is_empty()); assert!(rec.carried.is_empty(), "same-issue reflag should supersede"); } @@ -368,9 +392,66 @@ mod tests { let idx = index_for("a.rs", 30, 1); // touches only a.rs:30 let mut wide = f("a.rs", 5, Severity::Error, 0.9); wide.end_line = Some(40); - let rec = reconcile(&[wide], &idx, &[]); + let rec = reconcile(&[wide], &idx, &[], ReconcileScope::Incremental); assert_eq!(rec.resolved.len(), 0, "wide-span finding falsely resolved"); assert_eq!(rec.carried.len(), 1); assert_eq!(rec.carried[0].severity, Severity::Error); } + + #[test] + fn reconcile_uses_old_head_coordinates_for_rewritten_anchor() { + // PR 314 regression: the baseline finding cited old-head line 99. The + // rewrite touched it in an old-side 93..109 hunk which moved to new-side + // 133..159. Looking for new-side line 99 misses the edit and carries a + // stale finding forever. + let text = format!( + "diff --git a/src/components/code-copy-enhancer.tsx b/src/components/code-copy-enhancer.tsx\n\ + --- a/src/components/code-copy-enhancer.tsx\n\ + +++ b/src/components/code-copy-enhancer.tsx\n\ + @@ -93,17 +133,27 @@ export function CodeCopyEnhancer() {{\n{}{}", + "-old line\n".repeat(17), + "+new line\n".repeat(27) + ); + let idx = DiffIndex::build(&diff::parse(&text)); + assert!(idx.contains_old("src/components/code-copy-enhancer.tsx", 99)); + assert!(!idx.contains("src/components/code-copy-enhancer.tsx", 99)); + + let baseline = f( + "src/components/code-copy-enhancer.tsx", + 99, + Severity::Error, + 0.9, + ); + let rec = reconcile(&[baseline], &idx, &[], ReconcileScope::Incremental); + assert_eq!(rec.resolved.len(), 1); + assert!(rec.carried.is_empty()); + } + + #[test] + fn trustworthy_full_review_resolves_findings_it_does_not_reproduce() { + let idx = index_for("other.rs", 1, 1); + let baseline = vec![f("a.rs", 99, Severity::Error, 0.9)]; + let rec = reconcile( + &baseline, + &idx, + &[], + ReconcileScope::Full { trustworthy: true }, + ); + assert_eq!(rec.resolved.len(), 1); + assert!(rec.carried.is_empty()); + } + + #[test] + fn failed_full_review_keeps_baseline_findings_open() { + let idx = index_for("a.rs", 1, 100); + let baseline = vec![f("a.rs", 99, Severity::Error, 0.9)]; + let rec = reconcile( + &baseline, + &idx, + &[], + ReconcileScope::Full { trustworthy: false }, + ); + assert!(rec.resolved.is_empty()); + assert_eq!(rec.carried.len(), 1); + } } diff --git a/src/forge/github.rs b/src/forge/github.rs index b409620..a7138a1 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -6,6 +6,7 @@ use serde_json::json; use super::{CheckState, Forge, PrMeta, ThreadKind, check_summary, check_title, wrap_plain_text}; use crate::envelope::{Envelope, Finding, Severity}; +use crate::filter; pub struct GitHub { http: reqwest::Client, @@ -184,10 +185,16 @@ impl Forge for GitHub { } async fn post_review(&self, summary: &str, findings: &[Finding], head_sha: &str) -> Result<()> { + // Every carried finding is already visible in an earlier Postil review. + // Check-runs still receive the complete envelope, but posting the same + // visible set as another PR review is duplicate noise. + if !findings.is_empty() && findings.iter().all(filter::is_carried) { + return Ok(()); + } let comments: Vec<_> = findings .iter() // Carried findings already have comments from the previous review. - .filter(|f| !f.body.starts_with("[carried from previous review]")) + .filter(|f| !filter::is_carried(f)) // Synthetic-path findings (PR description, fail-closed markers) have // no real file line to anchor an inline comment; they surface only in // the summary body. diff --git a/src/review.rs b/src/review.rs index 463da17..3d0f9ef 100644 --- a/src/review.rs +++ b/src/review.rs @@ -54,6 +54,16 @@ pub struct ReviewArgs { pub no_post: bool, } +struct ReviewInput<'a> { + diff_text: &'a str, + meta: Option<&'a PrMeta>, + head_sha: Option, + repo: Option<&'a str>, + baseline: Vec, + scope: filter::ReconcileScope, + force_model: bool, +} + pub async fn run(args: ReviewArgs) -> Result { let cwd = std::env::current_dir()?; let mut cfg = Config::load(&cwd, args.config.as_deref())?; @@ -116,7 +126,26 @@ async fn run_local(args: &ReviewArgs, cfg: &Config) -> Result { }; let diff_text = local::acquire(&source).await?; let head_sha = local::head_sha().await; - let envelope = review_diff(cfg, &diff_text, None, args, head_sha, None).await?; + let baseline = load_baseline(args)?; + let scope = if args.since_sha.is_some() { + filter::ReconcileScope::Incremental + } else { + filter::ReconcileScope::Full { trustworthy: false } + }; + let envelope = review_diff( + cfg, + args, + ReviewInput { + diff_text: &diff_text, + meta: None, + head_sha, + repo: None, + baseline, + scope, + force_model: false, + }, + ) + .await?; finish(args, cfg, envelope, None::<&GitHub>).await } @@ -255,55 +284,91 @@ async fn remote_review( head_sha: &str, prefetched_diff: Option>, ) -> Result { + let baseline = load_baseline(args)?; + let has_carryable_baseline = baseline.iter().any(|f| !f.path.starts_with(".postil/")); let incremental = args.since_sha.as_deref(); - let diff_text = match incremental { + let (diff_text, scope, force_model) = match incremental { Some(since) if since != head_sha => forge .fetch_diff_since(since, head_sha) .await - .context("incremental diff fetch")?, - Some(_) => String::new(), - None => match prefetched_diff { - Some(diff) => diff.context("diff fetch")?, - None => forge.fetch_diff().await.context("diff fetch")?, - }, + .context("incremental diff fetch") + .map(|diff| (diff, filter::ReconcileScope::Incremental, false))?, + Some(_) => (String::new(), filter::ReconcileScope::Incremental, false), + None => ( + match prefetched_diff { + Some(diff) => diff.context("diff fetch")?, + None => forge.fetch_diff().await.context("diff fetch")?, + }, + filter::ReconcileScope::Full { trustworthy: false }, + false, + ), + }; + + // A same-head re-run has no incremental diff. If a real baseline finding + // remains open, an empty run can never clear it, so retry as a full review. + // Empty incremental runs without carryable findings remain model-free. + let (diff_text, scope, force_model) = if cfg.enabled + && has_carryable_baseline + && matches!(scope, filter::ReconcileScope::Incremental) + && diff_text.trim().is_empty() + { + ( + forge + .fetch_diff() + .await + .context("full diff fallback fetch")?, + filter::ReconcileScope::Full { trustworthy: false }, + true, + ) + } else { + (diff_text, scope, force_model) }; review_diff( cfg, - &diff_text, - Some(meta), args, - Some(head_sha.to_string()), - Some(repo), + ReviewInput { + diff_text: &diff_text, + meta: Some(meta), + head_sha: Some(head_sha.to_string()), + repo: Some(repo), + baseline, + scope, + force_model, + }, ) .await } -/// Core engine: diff text in, envelope out. No forge I/O. -async fn review_diff( - cfg: &Config, - diff_text: &str, - meta: Option<&PrMeta>, - args: &ReviewArgs, - head_sha: Option, - repo: Option<&str>, -) -> Result { - let baseline: Vec = match &args.baseline { +fn load_baseline(args: &ReviewArgs) -> Result> { + match &args.baseline { Some(path) => { let raw = std::fs::read_to_string(path) .with_context(|| format!("reading baseline {}", path.display()))?; let env: Envelope = serde_json::from_str(&raw).context("parsing baseline envelope")?; - env.findings + Ok(env.findings) } - None => Vec::new(), - }; + None => Ok(Vec::new()), + } +} +/// Core engine: diff text in, envelope out. No forge I/O. +async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> Result { + let ReviewInput { + diff_text, + meta, + head_sha, + repo, + baseline, + scope, + force_model, + } = input; let review_started = std::time::Instant::now(); // Cap the raw diff before parsing so an oversized fetched diff cannot force // unbounded parse work; a cut here forces the truncated path below. let (diff_text, raw_truncated) = diff::cap_raw_diff(diff_text, MAX_RAW_DIFF_BYTES); let parsed = diff::parse(diff_text); let mut index = DiffIndex::build(&parsed); - let incremental = args.since_sha.is_some(); + let incremental = matches!(scope, filter::ReconcileScope::Incremental); // When content policy is active, render the PR title/description as a // numbered, groundable block and register its line range so a title/body @@ -329,13 +394,14 @@ async fn review_diff( let mut suppressed = 0u32; let mut ungrounded = 0u32; let mut findings: Vec = Vec::new(); + let mut full_review_trustworthy = false; // Run the model when there is a diff to review, or when content policy is // active and there is a PR title/description to review (an empty diff should // still get its prose checked). if !cfg.enabled { model_used = "none (disabled by config)".to_string(); - } else if !parsed.is_empty() || pr_desc_lines > 0 { + } else if force_model || !parsed.is_empty() || pr_desc_lines > 0 { let (annotated, render_truncated) = diff::render_annotated(&parsed, MAX_DIFF_BYTES); // Either the raw input was capped or the rendered output hit the limit; // both mean the model did not see the full change. @@ -380,6 +446,7 @@ async fn review_diff( &model_review.summary, )]; } else { + full_review_trustworthy = true; summary = model_review.summary; findings = outcome.kept; } @@ -401,6 +468,7 @@ async fn review_diff( // A truncated review must never read as a full pass: the unreviewed // tail is surfaced as an explicit uncertainty finding. if truncated { + full_review_trustworthy = false; findings.push(Finding { path: ".postil/diff".to_string(), line: 1, @@ -425,7 +493,13 @@ async fn review_diff( // review off there is no fresh signal to reconcile against, so honoring the // disable means dropping the baseline carry-forward too. let rec = if cfg.enabled { - filter::reconcile(&baseline, &index, &findings) + let scope = match scope { + filter::ReconcileScope::Incremental => filter::ReconcileScope::Incremental, + filter::ReconcileScope::Full { .. } => filter::ReconcileScope::Full { + trustworthy: full_review_trustworthy, + }, + }; + filter::reconcile(&baseline, &index, &findings, scope) } else { filter::Reconciliation { resolved: vec![], @@ -494,8 +568,12 @@ async fn finish( if let Some(forge) = forge && !args.no_post { - let should_comment = - !envelope.silent || matches!(cfg.on_clean, crate::config::OnClean::Comment); + let duplicate_of_baseline = load_baseline(args) + .ok() + .is_some_and(|baseline| visible_finding_sets_equal(&baseline, &envelope.findings)); + let should_comment = (!envelope.silent + || matches!(cfg.on_clean, crate::config::OnClean::Comment)) + && !duplicate_of_baseline; if should_comment { let rich = forge.rich_markdown(); let summary = if envelope.silent { @@ -522,6 +600,50 @@ async fn finish( Ok(if envelope.gate.failing { 1 } else { 0 }) } +fn visible_finding_sets_equal(previous: &[Finding], current: &[Finding]) -> bool { + // Synthetic findings represent this run's operational state and must stay + // visible even if an earlier run failed in the same way. + if previous.is_empty() + || current.is_empty() + || previous.len() != current.len() + || previous + .iter() + .chain(current) + .any(|f| f.path.starts_with(".postil/")) + { + return false; + } + + let mut matched = vec![false; current.len()]; + previous.iter().all(|old| { + current + .iter() + .enumerate() + .find(|(i, new)| !matched[*i] && same_visible_finding(old, new)) + .is_some_and(|(i, _)| { + matched[i] = true; + true + }) + }) +} + +fn same_visible_finding(a: &Finding, b: &Finding) -> bool { + a.path == b.path + && a.line == b.line + && a.end_line == b.end_line + && a.severity == b.severity + && a.kind == b.kind + && a.confidence.to_bits() == b.confidence.to_bits() + && a.title == b.title + && visible_body(&a.body) == visible_body(&b.body) +} + +fn visible_body(body: &str) -> &str { + body.strip_prefix(filter::CARRIED_MARKER) + .map(str::trim_start) + .unwrap_or(body) +} + fn error_envelope( cfg: &Config, err: &anyhow::Error, @@ -564,3 +686,45 @@ fn error_envelope( since_sha: None, } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::envelope::{Kind, Severity}; + + fn finding(path: &str, line: u32, body: &str) -> Finding { + Finding { + path: path.to_string(), + line, + end_line: None, + severity: Severity::Error, + kind: Kind::Risk, + confidence: 0.9, + title: "Finding".to_string(), + body: body.to_string(), + } + } + + #[test] + fn visible_finding_comparison_ignores_carry_marker_and_order() { + let previous = vec![finding("a.rs", 10, "first"), finding("b.rs", 20, "second")]; + let current = vec![ + finding("b.rs", 20, "[carried from previous review]\n\nsecond"), + finding("a.rs", 10, "[carried from previous review]\n\nfirst"), + ]; + assert!(visible_finding_sets_equal(&previous, ¤t)); + } + + #[test] + fn visible_finding_comparison_detects_changes_and_fresh_synthetic_state() { + let previous = vec![finding("a.rs", 10, "first")]; + assert!(!visible_finding_sets_equal( + &previous, + &[finding("a.rs", 11, "first")] + )); + assert!(!visible_finding_sets_equal( + &[crate::envelope::provider_error_finding("old")], + &[crate::envelope::provider_error_finding("old")] + )); + } +} diff --git a/tests/e2e.rs b/tests/e2e.rs index 1441d57..77c95c1 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -1248,6 +1248,316 @@ async fn incremental_review_resolves_and_carries_baseline_findings() { assert_eq!(env["sinceSha"], "abc123"); } +#[tokio::test] +async fn same_head_with_open_baseline_falls_back_to_full_review() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .and(header("Accept", "application/vnd.github.v3.diff")) + .respond_with(ResponseTemplate::new(200).set_body_string(DIFF)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "head": {"sha": "h1"}, "base": {"sha": "b1"} + }))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/\d+$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .expect(2) + .mount(&server) + .await; + + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [ + {"path": "src/auth.rs", "line": 41, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "old auth bug", "body": "fixed now"} + ], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": "b1", "headSha": "h1", "sinceSha": null + }); + let dir = tempfile::tempdir().unwrap(); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args(["review", "--repo", "acme/api", "--pr", "7"]) + .args(["--sha", "h1", "--since-sha", "h1", "--baseline"]) + .arg(&baseline_path) + .args([ + "--check-run-id", + "11", + "--gate-check-run-id", + "12", + "--output-json", + ]) + .assert() + .code(0); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["resolved"][0]["title"], "old auth bug"); + assert_eq!(env["findings"], json!([])); + assert_ne!(env["modelUsed"], "none (empty diff)"); + assert_eq!(env["gate"]["failing"], false); + + let requests = server.received_requests().await.unwrap(); + assert!( + !requests + .iter() + .any(|request| request.url.path().contains("/compare/")) + ); + assert!( + !requests + .iter() + .any(|request| request.url.path().ends_with("/reviews")) + ); + let llm_request = requests + .iter() + .find(|request| request.url.path() == "/chat/completions") + .unwrap(); + let body: Value = llm_request.body_json().unwrap(); + assert!( + !body["messages"][1]["content"] + .as_str() + .unwrap() + .contains("This is an INCREMENTAL review") + ); +} + +#[tokio::test] +async fn same_head_without_open_baseline_keeps_empty_diff_noop() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(0) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "head": {"sha": "h1"}, "base": {"sha": "b1"} + }))) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "h1", + "--since-sha", + "h1", + "--no-post", + "--output-json", + ]) + .assert() + .code(0); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["modelUsed"], "none (empty diff)"); + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests.len(), + 1, + "empty no-op fetched more than PR metadata" + ); +} + +#[tokio::test] +async fn carried_only_incremental_run_updates_checks_without_posting_review() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/compare/old...h1")) + .respond_with(ResponseTemplate::new(200).set_body_string(DIFF)) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "head": {"sha": "h1"}, "base": {"sha": "b1"} + }))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/\d+$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [ + {"path": "src/db.rs", "line": 10, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "still broken", "body": "not addressed"} + ], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": "b1", "headSha": "old", "sinceSha": null + }); + let dir = tempfile::tempdir().unwrap(); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args(["review", "--repo", "acme/api", "--pr", "7"]) + .args(["--sha", "h1", "--since-sha", "old", "--baseline"]) + .arg(&baseline_path) + .args([ + "--check-run-id", + "11", + "--gate-check-run-id", + "12", + "--output-json", + ]) + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert!( + env["findings"][0]["body"] + .as_str() + .unwrap() + .starts_with("[carried from previous review]") + ); + assert_eq!(env["gate"]["failing"], true); + + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::PATCH) + .count(), + 2 + ); + assert!( + !requests + .iter() + .any(|request| request.url.path().ends_with("/reviews")) + ); +} + +#[tokio::test] +async fn identical_fresh_finding_set_does_not_post_duplicate_review() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(llm_content(json!([finding_at(41, "error", 0.95)]))), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/compare/old...h1")) + .respond_with(ResponseTemplate::new(200).set_body_string(DIFF)) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "head": {"sha": "h1"}, "base": {"sha": "b1"} + }))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/\d+$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [{ + "path": "src/auth.rs", "line": 41, "severity": "error", "kind": "risk", + "confidence": 0.95, "title": "Unsanitized input reaches query", + "body": "user_input flows into exec_query without sanitization." + }], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": "b1", "headSha": "old", "sinceSha": null + }); + let dir = tempfile::tempdir().unwrap(); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args(["review", "--repo", "acme/api", "--pr", "7"]) + .args(["--sha", "h1", "--since-sha", "old", "--baseline"]) + .arg(&baseline_path) + .args([ + "--check-run-id", + "11", + "--gate-check-run-id", + "12", + "--output-json", + ]) + .assert() + .code(1); + + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::PATCH) + .count(), + 2 + ); + assert!( + !requests + .iter() + .any(|request| request.url.path().ends_with("/reviews")) + ); +} + #[tokio::test] async fn disabled_review_does_not_carry_baseline_findings() { // M2 regression: a repo with `enabled: false` plus a supplied baseline must