From 31e0118d6fa4d86e21f239e95181af0688bba81d Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Thu, 30 Jul 2026 17:02:46 +0000 Subject: [PATCH] Check a finding's citation against the code it names A grounded finding proves its cited line exists and that its quoted evidence is that line's text. It does not prove the finding is about that line. A review of an Ansible playbook reported that a password task had dropped no_log, cited a line sixteen rows below where that task sits, and then compared the pull request description against the misreading and accused its author of describing the change falsely. Both claims blocked the merge; both were wrong. Four post-model passes address the shapes behind that: Anchor corroboration suppresses a finding when the constructs its prose names sit elsewhere on the same path, or when they are too far apart for the relationship it asserts to exist at one place in the file. A finding that names nothing the diff can locate is left alone, so silence never becomes a verdict. A content-policy claim can no longer outrank the finding it argues from. It inherits that finding's confidence as a ceiling, and falls with it when the only reading it rests on was suppressed as mis-anchored. Findings that make one claim about several locations collapse into one finding that names them all, instead of one comment per file. An uncertainty finding whose body only asks the author to confirm something loses its blocking severity. It runs after uncertainty resolution, so a finding that went and checked keeps what it earned, and after policy thresholds, so the question stays visible rather than dropping below a warn floor. Publication gains cross-run dedup: a second review of an unchanged head re-detects what the first found, and those findings arrive fresh rather than carried, so the carry filter cannot see them. Their markers already on the pull request can. A run that cannot read the existing comments posts everything it found. Live benchmark against the recorded baseline: detection 96.5 to 98.2 percent, gate verdict correctness 76.8 to 78.6 percent, false findings unchanged at 4, mean cost per case down. The hermetic suite passes 70 of 70 cases. --- src/diff.rs | 33 ++ src/envelope.rs | 12 + src/filter.rs | 819 +++++++++++++++++++++++++++++++++++++++++++- src/forge/github.rs | 288 +++++++++++++++- src/forge/mod.rs | 5 + src/review.rs | 5 + 6 files changed, 1158 insertions(+), 4 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 3ae6853..3c0106a 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -894,6 +894,39 @@ impl DiffIndex { .is_some_and(|rs| rs.iter().any(|r| *r.start() <= end && start <= *r.end())) } + /// New-side lines on `path` whose canonical text contains `needle`, in + /// ascending order. This is the same text the model was shown, so a lookup + /// answers "where in this file does the construct the finding names + /// actually appear" without re-reading the worktree. + /// + /// The comparison ignores case. Callers use this to decide whether to + /// distrust a finding, so a case difference between prose and source must + /// count as corroboration rather than as evidence against the anchor. + pub fn new_side_lines_containing(&self, path: &str, needle: &str) -> Vec { + if needle.is_empty() { + return Vec::new(); + } + let needle = needle.to_ascii_lowercase(); + let mut lines: Vec = self + .new_evidence + .iter() + .filter(|((candidate, _), text)| { + candidate == path && text.to_ascii_lowercase().contains(&needle) + }) + .map(|((_, line), _)| *line) + .collect(); + lines.sort_unstable(); + lines + } + + /// True when the diff carries any new-side text for `path`. A path with no + /// new-side coverage cannot corroborate or contradict an anchor. + pub fn has_new_side_text(&self, path: &str) -> bool { + self.new_evidence + .keys() + .any(|(candidate, _)| candidate == path) + } + pub fn is_empty(&self) -> bool { self.ranges.is_empty() } diff --git a/src/envelope.rs b/src/envelope.rs index 940bdfa..766d34d 100644 --- a/src/envelope.rs +++ b/src/envelope.rs @@ -482,6 +482,18 @@ pub enum SuppressionReason { BelowSeverity, BelowConfidence, MaxFindings, + /// The finding's prose names a construct that the diff places elsewhere on + /// the same path than the line it cites. A reader cannot verify a claim + /// against the wrong code, so the finding is not publishable as written. + AnchorMismatch, + /// The finding restates a claim that another, retained finding already + /// makes about a different location. Only the retained copy is published, + /// and it names the other affected locations. + DuplicateRootCause, + /// A content-policy claim built on top of a finding that was itself + /// suppressed as mis-anchored. It inherits the misreading and cannot stand + /// on its own. + DerivedFromSuppressed, } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/src/filter.rs b/src/filter.rs index d45143e..25256ee 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -53,9 +53,39 @@ pub fn apply(cfg: &Config, index: &DiffIndex, mut findings: Vec) -> Res let ungrounded = (before - findings.len()) as u32; let all_ungrounded = had_any && findings.is_empty(); + let mut suppressed_findings = Vec::new(); + + // Anchor corroboration. Grounding proves the cited line exists and that the + // quoted evidence is its text; it does not prove the finding is ABOUT that + // line. A model that reasons correctly about one construct and cites + // another produces a claim no reader can check against the code in front of + // them, and any later rule that builds on the citation inherits the error. + let mismatched: Vec = { + let mut mismatched = Vec::new(); + findings.retain(|f| { + if anchor_verdict(index, f) == AnchorVerdict::Mismatched { + mismatched.push(f.clone()); + suppressed_findings.push(SuppressedFinding { + finding: f.clone(), + reason: SuppressionReason::AnchorMismatch, + }); + false + } else { + true + } + }); + mismatched + }; + + // Content-policy claims are second-order: they compare the PR's own prose + // against what the diff does. When the "what the diff does" half came from a + // finding that misread the diff, the content-policy claim is an accusation + // built on that misreading, so it falls with it rather than outliving it at + // a higher confidence than the finding it rests on. + apply_derived_confidence(&mut findings, &mismatched, &mut suppressed_findings); + // Policy suppression. let ignore = build_ignore_set(&cfg.ignore)?; - let mut suppressed_findings = Vec::new(); findings.retain(|f| { let reason = if deterministically_non_actionable(f) { Some(SuppressionReason::NonActionable) @@ -79,6 +109,12 @@ pub fn apply(cfg: &Config, index: &DiffIndex, mut findings: Vec) -> Res } }); + // One observation replicated across every file it touches is one finding, + // not one per file. Collapsing here rather than at the model boundary keeps + // the collapse honest: the retained copy names the other locations, so the + // reader still learns the full extent of the issue. + collapse_shared_root_cause(&mut findings, &mut suppressed_findings); + // Highest severity first, then confidence; cap to maxFindings. findings.sort_by(|a, b| { b.severity @@ -106,6 +142,454 @@ pub fn apply(cfg: &Config, index: &DiffIndex, mut findings: Vec) -> Res }) } +/// How far from its cited line a named construct may sit and still corroborate +/// the anchor. Wide enough to span a function signature, a systemd unit stanza, +/// or a YAML task block, so a finding that pins the top of a construct and +/// discusses a key further down still corroborates. +const ANCHOR_CORROBORATION_WINDOW: u32 = 12; + +/// Shortest quoted span treated as a locatable construct. Below this, a token +/// matches too much of any file to say anything about where a finding belongs. +const MIN_CONSTRUCT_CHARS: usize = 4; + +/// Longest quoted span treated as a locatable construct. A longer quote is +/// prose being quoted, not an identifier being named. +const MAX_CONSTRUCT_CHARS: usize = 80; + +/// Most words a delimited span may carry and still be a name rather than a +/// sentence. Long enough for a descriptive task or test name. +const MAX_CONSTRUCT_WORDS: usize = 6; + +/// How much prose a finding must carry before two copies of it can be judged +/// the same observation. A real finding states a claim in sentences; a pair +/// that matches on less than this has not earned a collapse. +const MIN_ROOT_CAUSE_TOKENS: usize = 12; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AnchorVerdict { + /// A construct the finding names sits at or near the cited line. + Corroborated, + /// Every construct the finding names sits elsewhere on the same path. + Mismatched, + /// The finding names nothing the diff can locate, so the anchor can be + /// neither confirmed nor contradicted. Grounding already passed; leave it. + Unlocatable, +} + +/// Decide whether a finding's own prose corroborates the line it cites. +/// +/// Two things have to hold for a citation to be checkable. The constructs the +/// finding names must reach the anchor, and they must sit close enough together +/// that the relationship the finding asserts between them exists at one place +/// in the file. A finding fails the first when it simply wrote down the wrong +/// number. It fails the second when it correctly read several constructs and +/// then described them as one — the production case, where a password task, its +/// `no_log`, and an unrelated task's `diff: false` were reported as a single +/// edit sixteen lines apart. +/// +/// Neither failure is inferred from silence. A finding that names nothing the +/// diff can locate is `Unlocatable` and stands on the grounding check alone. +fn anchor_verdict(index: &DiffIndex, finding: &Finding) -> AnchorVerdict { + if crate::envelope::is_reserved_anchor(&finding.path) || !index.has_new_side_text(&finding.path) + { + return AnchorVerdict::Unlocatable; + } + let anchor_end = finding.end_line.unwrap_or(finding.line); + // Read each construct at the occurrence most favourable to the finding: the + // one nearest its anchor. If the claim does not hold together even there, it + // does not hold together anywhere. + let mut located: Vec = Vec::new(); + for construct in named_constructs(finding) { + let lines = index.new_side_lines_containing(&finding.path, &construct); + if let Some(nearest) = lines + .into_iter() + .min_by_key(|line| line.abs_diff(finding.line).min(line.abs_diff(anchor_end))) + { + located.push(nearest); + } + } + if located.is_empty() { + return AnchorVerdict::Unlocatable; + } + + let low = finding.line.saturating_sub(ANCHOR_CORROBORATION_WINDOW); + let high = anchor_end.saturating_add(ANCHOR_CORROBORATION_WINDOW); + if !located.iter().any(|line| (low..=high).contains(line)) { + return AnchorVerdict::Mismatched; + } + + let nearest = *located.iter().min().expect("located is non-empty"); + let furthest = *located.iter().max().expect("located is non-empty"); + if furthest - nearest > ANCHOR_CORROBORATION_WINDOW { + return AnchorVerdict::Mismatched; + } + AnchorVerdict::Corroborated +} + +/// Constructs a finding names in its own prose: the spans it puts in backticks +/// or double quotes that look like code rather than like English. +fn named_constructs(finding: &Finding) -> Vec { + let mut constructs = Vec::new(); + let own_basename = finding.path.rsplit('/').next().unwrap_or(&finding.path); + for text in [finding.title.as_str(), finding.body.as_str()] { + // Backticks and quotes are not equivalent. A backticked span is a + // literal being named — an identifier, a key, a task title. A + // double-quoted span is as often a quoted sentence, so it has to look + // like code before it counts. + for (delimiter, prose_allowed) in [('`', true), ('"', false)] { + let mut parts = text.split(delimiter); + // A split on a paired delimiter alternates outside/inside; only the + // odd indices are quoted spans, and a trailing unpaired delimiter + // leaves a final outside part that is correctly skipped. + parts.next(); + while let Some(span) = parts.next() { + if parts.next().is_none() { + break; + } + let span = span.trim(); + if !is_locatable_construct(span, prose_allowed) { + continue; + } + // The finding's own path is metadata about where it points, not + // a construct at the line it points to. + if span == finding.path || span == own_basename { + continue; + } + if !constructs.iter().any(|existing| existing == span) { + constructs.push(span.to_string()); + } + } + } + } + constructs +} + +/// Whether a delimited span names something the diff could contain. +/// +/// `prose_allowed` relaxes the code-shape test for spans the author marked as +/// literals. A named Ansible task or test case carries no punctuation and no +/// camel case, and rejecting it loses the one construct that pins where a +/// finding belongs. The relaxation is safe because an extracted span only ever +/// matters when the diff actually contains it. +fn is_locatable_construct(span: &str, prose_allowed: bool) -> bool { + let chars = span.chars().count(); + if !(MIN_CONSTRUCT_CHARS..=MAX_CONSTRUCT_CHARS).contains(&chars) { + return false; + } + let words = span.split_whitespace().count(); + if span.contains('\n') || words > MAX_CONSTRUCT_WORDS { + return false; + } + if prose_allowed { + return true; + } + let code_punctuation = span + .chars() + .any(|c| matches!(c, '_' | '.' | ':' | '/' | '-' | '=' | '(' | '[' | '{')); + let has_digit = span.chars().any(|c| c.is_ascii_digit()); + let inner_capital = span + .chars() + .zip(span.chars().skip(1)) + .any(|(previous, next)| previous.is_ascii_lowercase() && next.is_ascii_uppercase()); + code_punctuation || has_digit || inner_capital +} + +/// Cap a content-policy finding's confidence at that of the code finding it +/// argues from, and drop it outright when every finding it argues from was +/// suppressed as mis-anchored. +/// +/// A content-policy finding says "the PR description claims X but the change +/// does Y". The "does Y" half is only ever as reliable as the reading of the +/// diff that produced it. Left uncapped, a 0.60-confidence misreading becomes a +/// 0.95-confidence accusation that the author described their own change +/// falsely, which is both the most damaging thing this reviewer can say and the +/// claim it has the least independent basis for. +fn apply_derived_confidence( + findings: &mut Vec, + mismatched: &[Finding], + suppressed_findings: &mut Vec, +) { + let sources: Vec<(String, f64)> = findings + .iter() + .filter(|f| f.kind != crate::envelope::Kind::ContentPolicy) + .map(|f| (f.path.clone(), f.confidence)) + .collect(); + let mismatched_paths: Vec = mismatched + .iter() + .filter(|f| f.kind != crate::envelope::Kind::ContentPolicy) + .map(|f| f.path.clone()) + .collect(); + + let mut derived_only_from_suppressed = Vec::new(); + for finding in findings.iter_mut() { + if finding.kind != crate::envelope::Kind::ContentPolicy { + continue; + } + let ceiling = sources + .iter() + .filter(|(path, _)| references_path(finding, path)) + .map(|(_, confidence)| *confidence) + .fold(f64::NEG_INFINITY, f64::max); + if ceiling.is_finite() { + finding.confidence = finding.confidence.min(ceiling); + continue; + } + // No surviving finding backs this claim. If the claim points at a path + // whose only finding was suppressed as mis-anchored, it is that + // misreading restated as an accusation. + if mismatched_paths + .iter() + .any(|path| references_path(finding, path)) + { + derived_only_from_suppressed.push(finding.clone()); + } + } + if derived_only_from_suppressed.is_empty() { + return; + } + findings.retain(|finding| { + let dropped = derived_only_from_suppressed + .iter() + .any(|candidate| finding_identity(candidate) == finding_identity(finding)); + if dropped { + suppressed_findings.push(SuppressedFinding { + finding: finding.clone(), + reason: SuppressionReason::DerivedFromSuppressed, + }); + } + !dropped + }); +} + +/// Whether a finding's prose points at `path`, by full path or by basename. +fn references_path(finding: &Finding, path: &str) -> bool { + if crate::envelope::is_reserved_anchor(path) { + return false; + } + let basename = path.rsplit('/').next().unwrap_or(path); + let haystack = format!("{} {}", finding.title, finding.body).to_ascii_lowercase(); + haystack.contains(&path.to_ascii_lowercase()) + || (basename.contains('.') && haystack.contains(&basename.to_ascii_lowercase())) +} + +fn finding_identity(finding: &Finding) -> (String, u32, String, String) { + ( + finding.path.clone(), + finding.line, + finding.title.clone(), + finding.body.clone(), + ) +} + +/// Collapse findings that make the same claim about different locations into +/// one finding that names them all. +/// +/// The retained copy is the most confident, breaking ties toward the earliest +/// location so the choice is stable across runs. +fn collapse_shared_root_cause( + findings: &mut Vec, + suppressed_findings: &mut Vec, +) { + use std::collections::HashMap; + + let mut groups: HashMap> = HashMap::new(); + let mut order: Vec = Vec::new(); + for (position, finding) in findings.iter().enumerate() { + if crate::envelope::is_reserved_anchor(&finding.path) || is_carried(finding) { + continue; + } + let Some(key) = root_cause_key(finding) else { + continue; + }; + if groups.entry(key.clone()).or_default().is_empty() { + order.push(key.clone()); + } + groups + .get_mut(&key) + .expect("group just inserted") + .push(position); + } + + let mut drop_positions = std::collections::HashSet::new(); + for key in &order { + let positions = &groups[key]; + if positions.len() < 2 { + continue; + } + let keep = *positions + .iter() + .min_by(|left, right| { + let a = &findings[**left]; + let b = &findings[**right]; + b.confidence + .total_cmp(&a.confidence) + .then_with(|| a.path.cmp(&b.path)) + .then_with(|| a.line.cmp(&b.line)) + }) + .expect("group is non-empty"); + let mut others: Vec = positions + .iter() + .filter(|position| **position != keep) + .map(|position| format!("{}:{}", findings[*position].path, findings[*position].line)) + .collect(); + others.sort(); + for position in positions { + if *position != keep { + drop_positions.insert(*position); + } + } + annotate_shared_locations(&mut findings[keep], &others); + } + if drop_positions.is_empty() { + return; + } + let mut position = 0; + findings.retain(|finding| { + let dropped = drop_positions.contains(&position); + position += 1; + if dropped { + suppressed_findings.push(SuppressedFinding { + finding: finding.clone(), + reason: SuppressionReason::DuplicateRootCause, + }); + } + !dropped + }); +} + +/// The claim a finding makes, with the location it makes it about removed. +/// Two findings share a root cause when only their locations differ. +/// +/// Returns `None` for prose too short to identify a claim. Two findings that +/// agree on a handful of words have not been shown to be the same observation, +/// and collapsing them would hide a real second defect. +fn root_cause_key(finding: &Finding) -> Option { + let prose = format!("{}\n{}", finding.title, finding.body); + let mut normalized = String::with_capacity(prose.len()); + let mut tokens = 0usize; + for token in prose.split_whitespace() { + tokens += 1; + let trimmed = token.trim_matches(|c: char| !c.is_alphanumeric()); + let path_like = trimmed.contains('/') + || (trimmed.contains('.') + && trimmed.rsplit('.').next().is_some_and(|extension| { + !extension.is_empty() && extension.chars().all(|c| c.is_ascii_alphabetic()) + })); + if path_like { + normalized.push_str(" "); + } else if trimmed.chars().any(|c| c.is_ascii_digit()) { + normalized.push_str(" "); + } else { + normalized.push_str(&token.to_ascii_lowercase()); + normalized.push(' '); + } + } + if tokens < MIN_ROOT_CAUSE_TOKENS { + return None; + } + Some(format!( + "{}|{}|{}", + finding.kind.as_str(), + finding.severity.as_str(), + normalized.trim() + )) +} + +/// Append the other locations sharing this finding's cause, but never at the +/// cost of the finding's publishability: forge sinks must not repair prose, so +/// a note that would push the body past the publication limits is dropped +/// rather than truncated. +fn annotate_shared_locations(finding: &mut Finding, others: &[String]) { + if others.is_empty() { + return; + } + let note = format!( + "\n\nThe same issue appears at {}.", + others + .iter() + .map(String::as_str) + .collect::>() + .join(", ") + ); + let mut annotated = finding.clone(); + annotated.body = format!("{}{note}", finding.body); + if crate::envelope::validate_finding_publication(&annotated).is_ok() { + finding.body = annotated.body; + } +} + +/// Strip blocking severity from `uncertainty` findings that only ask the author +/// to check something. +/// +/// This runs last, after uncertainty resolution has had its chance to turn a +/// question into an evidenced claim and after policy thresholds have been +/// applied to the severity the model asked for. The order matters in both +/// directions: resolving first means a finding that did the work keeps its +/// severity, and demoting after thresholds means the question stays visible +/// instead of dropping below a `warn` floor and disappearing. +pub fn demote_deferred_verification(findings: &mut [Finding]) { + for finding in findings { + if defers_verification_to_the_author(finding) { + finding.severity = crate::envelope::Severity::Info; + } + } +} + +/// Whether an `uncertainty` finding only asks the author to check something, +/// without saying what the reviewer itself checked. +/// +/// "Confirm that X is always created" is a question, not a finding. It costs +/// the author the verification work the reviewer was supposed to do, and it +/// does so at a severity that can gate a merge. A finding that reports what it +/// looked at ("the diff adds no other caller") is doing the work and keeps its +/// severity. +fn defers_verification_to_the_author(finding: &Finding) -> bool { + if finding.kind != crate::envelope::Kind::Uncertainty + || finding.severity == crate::envelope::Severity::Info + || crate::envelope::is_reserved_anchor(&finding.path) + { + return false; + } + // The ask is read from the body alone. A title is a headline and is + // routinely imperative ("Verify the caller contract") even for a finding + // whose body then goes and establishes the answer; demoting on the headline + // would punish exactly the findings that did the work. + let body = finding.body.to_ascii_lowercase(); + let prose = format!("{} {}", finding.title, finding.body).to_ascii_lowercase(); + let asks_the_author = [ + "confirm that", + "confirm the", + "please confirm", + "verify that", + "verify the", + "please verify", + "double-check", + "make sure that", + "ensure that", + "check whether", + "check that", + ] + .iter() + .any(|marker| body.contains(marker)); + if !asks_the_author { + return false; + } + let states_what_it_checked = [ + "the diff shows", + "the diff adds", + "the diff does not", + "the diff contains no", + "no other caller", + "no other reference", + "searched", + "the only caller", + "elsewhere in this change", + ] + .iter() + .any(|marker| prose.contains(marker)); + !states_what_it_checked +} + fn deterministically_non_actionable(finding: &Finding) -> bool { let title = finding.title.to_ascii_lowercase(); let body = finding.body.to_ascii_lowercase(); @@ -1120,4 +1604,337 @@ mod tests { assert!(rec.resolved.is_empty()); assert_eq!(rec.carried.len(), 1); } + + /// The Ansible playbook from the production misattribution: a + /// password-switching task with `no_log: true` sits at 965..972, and an + /// unrelated rclone-config task with `diff: false` sits at 979..982. + fn playbook_index() -> DiffIndex { + let mut text = String::from( + "diff --git a/ansible/playbooks/backup.yml b/ansible/playbooks/backup.yml\n\ + --- a/ansible/playbooks/backup.yml\n\ + +++ b/ansible/playbooks/backup.yml\n\ + @@ -960,24 +960,24 @@\n", + ); + for line in 960..=990 { + let content = match line { + 965 => " - name: Switch RGW admin password".to_string(), + 972 => " no_log: true".to_string(), + 979 => " - name: Write rclone config".to_string(), + 981 => " diff: false".to_string(), + other => format!(" key_{other}: value"), + }; + text.push('+'); + text.push_str(&content); + text.push('\n'); + } + DiffIndex::build(&diff::parse(&text)) + } + + fn playbook_finding(line: u32, title: &str, body: &str) -> Finding { + let mut finding = f("ansible/playbooks/backup.yml", line, Severity::Error, 0.6); + finding.title = title.into(); + finding.body = body.into(); + finding.evidence = Some(match line { + 981 => " diff: false".to_string(), + other => format!(" key_{other}: value"), + }); + finding + } + + #[test] + fn a_finding_citing_a_line_its_named_construct_does_not_sit_on_is_suppressed() { + // Reproduces the production defect: the model reasoned about the + // password task at 965 and cited 981, where an unrelated task lives. + // Grounding passes because the evidence really is line 981's text. + let idx = playbook_index(); + let cfg = Config::default(); + let finding = playbook_finding( + 981, + "Password task drops `no_log: true`", + "The `Switch RGW admin password` task replaces `no_log: true` with \ + `diff: false`, so the new password is written to the job log in \ + plaintext on every run of this playbook.", + ); + + let out = apply(&cfg, &idx, vec![finding]).unwrap(); + + assert!( + out.kept.is_empty(), + "a mis-anchored finding is not published" + ); + assert_eq!( + out.ungrounded, 0, + "it grounded; the anchor is what is wrong" + ); + assert_eq!(out.suppressed_findings.len(), 1); + assert_eq!( + out.suppressed_findings[0].reason, + SuppressionReason::AnchorMismatch + ); + } + + #[test] + fn a_finding_citing_the_line_its_named_construct_sits_on_is_kept() { + let idx = playbook_index(); + let cfg = Config::default(); + let mut finding = playbook_finding( + 972, + "Password task drops `no_log: true`", + "The `Switch RGW admin password` task no longer sets `no_log: true`, \ + so the new password is written to the job log in plaintext.", + ); + finding.evidence = Some(" no_log: true".to_string()); + + let out = apply(&cfg, &idx, vec![finding]).unwrap(); + + assert_eq!(out.kept.len(), 1); + assert!(out.suppressed_findings.is_empty()); + } + + #[test] + fn a_finding_naming_nothing_the_diff_can_locate_keeps_its_anchor() { + // Without a locatable construct there is no evidence either way, and + // grounding has already done its job. Silence must not become a verdict. + let idx = playbook_index(); + let cfg = Config::default(); + let finding = playbook_finding( + 981, + "This task is not idempotent", + "Re-running the playbook rewrites the file unconditionally, so every \ + run reports a change even when nothing differs.", + ); + + let out = apply(&cfg, &idx, vec![finding]).unwrap(); + + assert_eq!(out.kept.len(), 1); + } + + #[test] + fn a_content_policy_claim_built_on_a_mis_anchored_finding_falls_with_it() { + // The second production error-severity finding: the PR description was + // compared against the misreading and its author accused of describing + // the change falsely. + let mut idx = playbook_index(); + idx.add_content_policy_evidence( + crate::envelope::PR_DESCRIPTION_PATH, + "### .postil/pr-description\n 1 Backup hardening\n 2 Keeps no_log on the password task.\n", + ); + let cfg = Config::default(); + let mis_anchored = playbook_finding( + 981, + "Password task drops `no_log: true`", + "The `Switch RGW admin password` task replaces `no_log: true` with \ + `diff: false`, exposing the password in the job log.", + ); + let mut derived = f( + crate::envelope::PR_DESCRIPTION_PATH, + 2, + Severity::Error, + 0.95, + ); + derived.kind = Kind::ContentPolicy; + derived.title = "PR description contradicts the change".into(); + derived.body = "The description states no_log is kept, but \ + ansible/playbooks/backup.yml removes it." + .into(); + derived.evidence = Some("Keeps no_log on the password task.".into()); + + let out = apply(&cfg, &idx, vec![mis_anchored, derived]).unwrap(); + + assert!(out.kept.is_empty(), "neither error-severity claim survives"); + let reasons: Vec<_> = out + .suppressed_findings + .iter() + .map(|entry| entry.reason) + .collect(); + assert!(reasons.contains(&SuppressionReason::AnchorMismatch)); + assert!(reasons.contains(&SuppressionReason::DerivedFromSuppressed)); + } + + #[test] + fn a_content_policy_claim_cannot_outrank_the_finding_it_argues_from() { + let mut idx = playbook_index(); + idx.add_content_policy_evidence( + crate::envelope::PR_DESCRIPTION_PATH, + "### .postil/pr-description\n 1 Backup hardening\n 2 Keeps no_log on the password task.\n", + ); + let mut cfg = Config::default(); + cfg.min_confidence = 0.0; + let mut source = playbook_finding( + 972, + "Password task drops `no_log: true`", + "The `Switch RGW admin password` task no longer sets `no_log: true`.", + ); + source.evidence = Some(" no_log: true".to_string()); + source.confidence = 0.55; + let mut derived = f( + crate::envelope::PR_DESCRIPTION_PATH, + 2, + Severity::Error, + 0.95, + ); + derived.kind = Kind::ContentPolicy; + derived.title = "PR description contradicts the change".into(); + derived.body = "The description states no_log is kept, but \ + ansible/playbooks/backup.yml removes it." + .into(); + derived.evidence = Some("Keeps no_log on the password task.".into()); + + let out = apply(&cfg, &idx, vec![source, derived]).unwrap(); + + let content_policy = out + .kept + .iter() + .find(|finding| finding.kind == Kind::ContentPolicy) + .expect("the claim survives, at its source's confidence"); + assert!( + (content_policy.confidence - 0.55).abs() < f64::EPSILON, + "confidence was {}, expected the source's 0.55", + content_policy.confidence + ); + } + + #[test] + fn one_observation_repeated_across_files_becomes_one_finding() { + // Production shape: five systemd unit templates, one claim, bodies + // differing only by filename. + let units = [ + "backup-asset.service.j2", + "backup-integrity.service.j2", + "backup-prune.service.j2", + "backup-restore-test.service.j2", + "backup-tenant-data-maintenance.service.j2", + ]; + let mut text = String::new(); + let mut findings = Vec::new(); + for (position, unit) in units.iter().enumerate() { + let path = format!("ansible/roles/backup/templates/{unit}"); + text.push_str(&format!( + "diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -10,1 +10,1 @@\n+EnvironmentFile=/etc/backup.env\n", + )); + let mut finding = f(&path, 10, Severity::Warn, 0.6); + finding.kind = Kind::Risk; + finding.title = format!("EnvironmentFile in {unit} is not optional"); + finding.body = format!( + "In {unit} the EnvironmentFile directive has no leading dash, so \ + systemd refuses to start the unit when the file is absent rather \ + than continuing with the environment unset." + ); + finding.evidence = Some("EnvironmentFile=/etc/backup.env".into()); + finding.confidence = 0.6 + position as f64 * 0.01; + findings.push(finding); + } + let idx = DiffIndex::build(&diff::parse(&text)); + let cfg = Config::default(); + + let out = apply(&cfg, &idx, findings).unwrap(); + + assert_eq!(out.kept.len(), 1, "one claim, one finding"); + assert_eq!(out.suppressed_findings.len(), 4); + assert!( + out.suppressed_findings + .iter() + .all(|entry| entry.reason == SuppressionReason::DuplicateRootCause) + ); + let body = &out.kept[0].body; + for unit in &units { + if !out.kept[0].path.ends_with(unit) { + assert!( + body.contains(unit), + "the retained finding names {unit} as also affected" + ); + } + } + } + + #[test] + fn findings_with_too_little_prose_to_compare_are_not_collapsed() { + let mut text = String::new(); + let mut findings = Vec::new(); + for path in ["a.rs", "b.rs"] { + text.push_str(&format!( + "diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -1,1 +1,1 @@\n+x\n", + )); + let mut finding = f(path, 1, Severity::Warn, 0.9); + finding.title = "Unchecked index".into(); + finding.body = "This can panic.".into(); + findings.push(finding); + } + let idx = DiffIndex::build(&diff::parse(&text)); + let cfg = Config::default(); + + let out = apply(&cfg, &idx, findings).unwrap(); + + assert_eq!( + out.kept.len(), + 2, + "two short findings are still two findings" + ); + } + + fn uncertainty(title: &str, body: &str) -> Finding { + let mut finding = f("values.cluster.yaml", 675, Severity::Warn, 0.6); + finding.kind = Kind::Uncertainty; + finding.title = title.into(); + finding.body = body.into(); + finding + } + + #[test] + fn an_uncertainty_finding_that_only_asks_the_author_to_check_stops_blocking() { + let mut findings = vec![uncertainty( + "rgwConfig may not be a recognized field", + "Verify that rgwConfig is a field the chart recognizes; if it is not, \ + the block is silently ignored.", + )]; + + demote_deferred_verification(&mut findings); + + assert_eq!( + findings[0].severity, + Severity::Info, + "the question stays visible but cannot carry a blocking severity" + ); + } + + #[test] + fn an_uncertainty_finding_that_reports_what_it_checked_keeps_its_severity() { + let mut findings = vec![uncertainty( + "rgwConfig is not a recognized field", + "The diff adds no schema entry for rgwConfig and no other reference to \ + it, so verify that the chart consumes it.", + )]; + + demote_deferred_verification(&mut findings); + + assert_eq!(findings[0].severity, Severity::Warn); + } + + #[test] + fn demotion_leaves_other_kinds_and_reserved_anchors_alone() { + let mut risk = uncertainty( + "Confirm that the cache is warmed", + "Confirm that the cache is warmed before the first request.", + ); + risk.kind = Kind::Risk; + let mut operational = uncertainty( + "Review incomplete", + "Confirm that the full diff was reviewed.", + ); + operational.path = crate::envelope::OPERATIONAL_PATH.into(); + let mut findings = vec![risk, operational]; + + demote_deferred_verification(&mut findings); + + assert_eq!( + findings[0].severity, + Severity::Warn, + "only uncertainty demotes" + ); + assert_eq!( + findings[1].severity, + Severity::Warn, + "an operational anchor states the run's own limits" + ); + } } diff --git a/src/forge/github.rs b/src/forge/github.rs index 59e6037..6497242 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -632,6 +632,54 @@ impl GitHub { )) } + /// Finding markers already carried by inline comments on this pull request. + /// + /// A finding's id is a hash over the head SHA, kind, path, line, and title, + /// so an identical marker is the same finding at the same place on the same + /// commit. Two reviews of one head — a push review followed by an `@postil` + /// mention, say — legitimately re-detect the same issue, and without this + /// the second run posts a second copy of every comment the first already + /// left. + /// + /// Failure here is not a publication failure. A review that cannot read the + /// existing comments posts everything it found, which is the behaviour this + /// dedup replaces. + async fn published_finding_markers(&self) -> std::collections::HashSet { + const PAGE_SIZE: usize = 100; + const MAX_PAGES: usize = 20; + let mut markers = std::collections::HashSet::new(); + for page in 1..=MAX_PAGES { + let request = self.request( + reqwest::Method::GET, + self.url(&format!( + "/pulls/{}/comments?per_page={PAGE_SIZE}&page={page}", + self.pr + )), + ); + let Ok(response) = self.send_retryable(request, "inline comment dedup").await else { + return markers; + }; + let Ok(response) = Self::check_ok(response, "inline comment dedup").await else { + return markers; + }; + let Ok(comments): Result> = + super::bounded_response_json(response, "GitHub inline comment dedup").await + else { + return markers; + }; + let page_len = comments.len(); + for comment in comments { + if let Some(marker) = finding_marker_in(&comment.body) { + markers.insert(marker); + } + } + if page_len < PAGE_SIZE { + break; + } + } + markers + } + async fn post_comment_reconciled(&self, number: u64, body: &str, marker: &str) -> Result<()> { const RETRIES: u32 = 2; for retry in 0..=RETRIES { @@ -1283,6 +1331,10 @@ impl Forge for GitHub { short_sha(&snapshot.base_sha), )); } + // A re-review of an unchanged head re-detects what the last review + // found. Those findings arrive fresh rather than carried, so the carry + // filter above cannot see them; their markers already on the PR can. + let published = self.published_finding_markers().await; let comments: Vec<_> = findings .iter() // Carried findings already have comments from the previous review. @@ -1291,6 +1343,10 @@ impl Forge for GitHub { // no real file line to anchor an inline comment; they surface only in // the summary body. .filter(|f| !super::is_synthetic_path(&f.path)) + .filter(|f| { + let (finding_id, _) = finding_receipt_id(f); + !published.contains(&finding_marker(&finding_id)) + }) .map(|f| { let (finding_id, _) = finding_receipt_id(f); let mut c = json!({ @@ -1755,6 +1811,14 @@ fn review_marker(receipt_id: &str) -> String { ) } +/// The finding marker a published comment body ends with, if any. +fn finding_marker_in(body: &str) -> Option { + const OPEN: &str = "")? + start + "-->".len(); + Some(body[start..end].to_string()) +} + fn append_marker(body: &str, marker: &str) -> String { if body.trim().is_empty() { marker.to_string() @@ -1798,9 +1862,10 @@ fn comment_marker(number: u64, body: &str) -> String { #[cfg(test)] mod tests { use super::{ - EXPECTED_REPOSITORY_ID_ENV, GitHub, PullFile, gate_summary, github_retry_delay_at, - github_retryable_response, github_transport_retry_delay, only_operational_findings, - valid_details_url, validate_pull_file, + EXPECTED_REPOSITORY_ID_ENV, GitHub, PullFile, finding_marker, finding_marker_in, + finding_receipt_id, gate_summary, github_retry_delay_at, github_retryable_response, + github_transport_retry_delay, only_operational_findings, valid_details_url, + validate_pull_file, }; use crate::envelope::{ Envelope, Finding, Gate, Kind, Severity, SuppressedFinding, SuppressionReason, Usage, @@ -3471,4 +3536,221 @@ mod tests { assert!(!summary.contains("provider")); assert!(!summary.contains("timeout")); } + + fn dedup_finding() -> Finding { + Finding { + path: "values.cluster.yaml".into(), + line: 675, + end_line: None, + severity: Severity::Warn, + kind: Kind::Uncertainty, + confidence: 0.6, + generator_confidence: None, + scorer_confidence: None, + generator_kind: None, + scorer_kind: None, + scorer_reason: None, + title: "rgwConfig may not be a recognized field".into(), + body: "The chart may silently ignore this block.".into(), + evidence: None, + id: None, + } + } + + fn dedup_github(server: &MockServer) -> GitHub { + GitHub { + http: reqwest::Client::new(), + api_base: server.uri(), + details_url: None, + token: "test-token".into(), + owner: "owner".into(), + repo: "repo".into(), + pr: 1, + expected_repository_id: None, + } + } + + async fn mount_dedup_snapshot(server: &MockServer) { + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "title": "t", "body": "b", "state": "open", "merged": false, + "head": {"sha": "aaaaaaaaaaaa"}, "base": {"sha": "bbbbbbbbbbbb"}, + "changed_files": 1 + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path( + "/repos/owner/repo/compare/bbbbbbbbbbbb...aaaaaaaaaaaa", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "merge_base_commit": {"sha": "cccccccccccc"}, "files": [] + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/reviews/11/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; + } + + /// A second review of an unchanged head re-detects what the first found. + /// Those findings are fresh, not carried, so only the marker already on the + /// pull request can tell the run its comment is a duplicate. + #[tokio::test] + async fn github_does_not_repost_an_inline_comment_already_on_the_pull_request() { + let server = MockServer::start().await; + mount_dedup_snapshot(&server).await; + let finding = dedup_finding(); + let envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![finding.clone()]); + let github = dedup_github(&server); + let (finding_id, _) = finding_receipt_id( + envelope + .findings + .first() + .expect("one finding in the envelope"), + ); + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/comments")) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!([{ + "id": 7, + "body": format!("**{}**\n\n{}", finding.title, finding_marker(&finding_id)), + }])), + ) + .mount(&server) + .await; + let posted = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let captured = posted.clone(); + Mock::given(method("POST")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with(move |request: &wiremock::Request| { + captured + .lock() + .expect("capture lock") + .push(request.body_json().expect("review body")); + ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": 11})) + }) + .mount(&server) + .await; + + github + .post_review( + &envelope, + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + ) + .await + .expect("the review still posts its summary"); + + let bodies = posted.lock().expect("capture lock"); + let body = bodies.first().expect("one review posted"); + assert_eq!( + body["comments"].as_array().map_or(0, Vec::len), + 0, + "the duplicate inline comment is not reposted" + ); + } + + #[tokio::test] + async fn github_posts_an_inline_comment_the_pull_request_does_not_carry() { + let server = MockServer::start().await; + mount_dedup_snapshot(&server).await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/comments")) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!([{ + "id": 7, + "body": "**Some other finding**\n\n", + }])), + ) + .mount(&server) + .await; + let posted = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let captured = posted.clone(); + Mock::given(method("POST")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with(move |request: &wiremock::Request| { + captured + .lock() + .expect("capture lock") + .push(request.body_json().expect("review body")); + ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": 11})) + }) + .mount(&server) + .await; + let envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![dedup_finding()]); + + dedup_github(&server) + .post_review( + &envelope, + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + ) + .await + .expect("review posts"); + + let bodies = posted.lock().expect("capture lock"); + let body = bodies.first().expect("one review posted"); + assert_eq!( + body["comments"].as_array().map_or(0, Vec::len), + 1, + "an unrelated marker must not suppress a real finding" + ); + } + + #[tokio::test] + async fn github_publishes_normally_when_existing_comments_cannot_be_read() { + // Dedup is an improvement on top of publication, never a gate on it. + let server = MockServer::start().await; + mount_dedup_snapshot(&server).await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/comments")) + .respond_with(ResponseTemplate::new(500)) + .mount(&server) + .await; + let posted = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let captured = posted.clone(); + Mock::given(method("POST")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with(move |request: &wiremock::Request| { + captured + .lock() + .expect("capture lock") + .push(request.body_json().expect("review body")); + ResponseTemplate::new(200).set_body_json(serde_json::json!({"id": 11})) + }) + .mount(&server) + .await; + let envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![dedup_finding()]); + + dedup_github(&server) + .post_review( + &envelope, + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + ) + .await + .expect("review posts"); + + let bodies = posted.lock().expect("capture lock"); + assert_eq!( + bodies.first().expect("one review posted")["comments"] + .as_array() + .map_or(0, Vec::len), + 1 + ); + } + + #[test] + fn finding_marker_is_read_back_from_a_published_comment_body() { + let marker = finding_marker("finding-id"); + assert_eq!( + finding_marker_in(&format!("**Title**\n\nBody text.\n\n{marker}")), + Some(marker) + ); + assert_eq!(finding_marker_in("no marker here"), None); + } } diff --git a/src/forge/mod.rs b/src/forge/mod.rs index f79a624..2571b3e 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -1135,6 +1135,11 @@ fn suppression_reason(reason: SuppressionReason) -> &'static str { SuppressionReason::BelowSeverity => "below the configured severity threshold", SuppressionReason::BelowConfidence => "below the configured confidence threshold", SuppressionReason::MaxFindings => "outside the configured finding cap", + SuppressionReason::AnchorMismatch => "cites a line the named construct does not sit on", + SuppressionReason::DuplicateRootCause => { + "restates a retained finding about another location" + } + SuppressionReason::DerivedFromSuppressed => "built on a finding suppressed as mis-anchored", } } diff --git a/src/review.rs b/src/review.rs index 7a7428e..4049e98 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1953,6 +1953,11 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> } } + // A question the reviewer never answered cannot block a merge. This runs + // after uncertainty resolution so a finding that went and checked keeps the + // severity it earned. + crate::filter::demote_deferred_verification(&mut findings); + // Fresh metadata IDs must exist before reconciliation: synthetic line // numbers are presentation positions, not issue identity. generate_finding_ids(&mut findings, head_sha.as_deref());