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());