diff --git a/Cargo.lock b/Cargo.lock index d2a3632..90841e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -117,9 +117,9 @@ checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" [[package]] name = "aws-lc-rs" -version = "1.17.0" +version = "1.18.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5ec2f1fc3ec205783a5da9a7e6c1509cc69dedf09a1949e412c1e18469326d00" +checksum = "b281d307588d634de920874890732659e2e7672f72b5e10e81badc1a8a83621e" dependencies = [ "aws-lc-sys", "zeroize", @@ -127,14 +127,15 @@ dependencies = [ [[package]] name = "aws-lc-sys" -version = "0.41.0" +version = "0.45.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a2f9779ce85b93ab6170dd940ad0169b5766ff848247aff13bb788b832fe3f4" +checksum = "9bff6c3b54fad79a2e60b8102caf565819711497c1f5f092f49508e2f5c31b27" dependencies = [ "cc", "cmake", "dunce", "fs_extra", + "pkg-config", ] [[package]] @@ -1343,6 +1344,12 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" +[[package]] +name = "pkg-config" +version = "0.3.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f6b464fbc74e149a392436b17d523f769e057cb6877f6a5c4618bc6f11800548" + [[package]] name = "portable-atomic" version = "1.15.0" @@ -1718,9 +1725,9 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.40" +version = "0.23.45" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef86cd5876211988985292b91c96a8f2d298df24e75989a43a3c73f2d4d8168b" +checksum = "0d41d731c7d2f962d1ccc364cec258de3c0e93b38c2fb3ba97ac74513048d634" dependencies = [ "aws-lc-rs", "once_cell", @@ -1781,9 +1788,9 @@ checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" [[package]] name = "rustls-webpki" -version = "0.103.13" +version = "0.103.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "61c429a8649f110dddef65e2a5ad240f747e85f7758a6bccc7e5777bd33f756e" +checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" dependencies = [ "aws-lc-rs", "ring", @@ -2087,7 +2094,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.3.4", + "getrandom 0.4.3", "once_cell", "rustix", "windows-sys 0.61.2", diff --git a/src/forge/github.rs b/src/forge/github.rs index 31344d5..bbeb33b 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -309,6 +309,12 @@ impl GitHub { ) -> std::collections::HashMap { let published = self.published_finding_comments(head_sha).await; for publication in &mut receipt.findings { + if matches!( + publication.initial_outcome, + FindingPublicationOutcome::Resolved | FindingPublicationOutcome::Suppressed + ) { + continue; + } let Ok(finding) = publication_plan_finding(envelope, &publication.finding_id) else { continue; }; @@ -8590,6 +8596,66 @@ mod tests { assert_eq!(pr_reads.load(std::sync::atomic::Ordering::SeqCst), 2); } + #[tokio::test] + async fn lifecycle_identity_receipts_keep_active_and_terminal_states_exclusive() { + let active = publication_finding("active", "src/lib.rs", "An active concern remains."); + let retired = publication_finding("retired", "src/lib.rs", "A retired concern."); + let suppressed = publication_finding("suppressed", "src/lib.rs", "A suppressed concern."); + let mut envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![active]); + envelope.resolved = vec![retired]; + envelope.suppressed_findings = vec![crate::envelope::SuppressedFinding { + finding: suppressed, + reason: crate::envelope::SuppressionReason::DuplicateRootCause, + }]; + let mut receipt = super::planned_review_receipt(&envelope, "aaaaaaaaaaaa"); + assert_eq!(receipt.findings.len(), 3); + assert_eq!( + receipt.findings[0].initial_outcome, + FindingPublicationOutcome::Inline + ); + assert_eq!( + receipt.findings[1].initial_outcome, + FindingPublicationOutcome::Resolved + ); + assert_eq!( + crate::forge::untracked_review_publication_receipt("gitlab", &envelope, "aaaaaaaaaaaa") + .findings + .len(), + 3 + ); + + let server = MockServer::start().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": 501, "commit_id": "aaaaaaaaaaaa", "body": super::finding_marker("active")}, + {"id": 502, "commit_id": "aaaaaaaaaaaa", "body": super::finding_marker("retired")}, + {"id": 503, "commit_id": "aaaaaaaaaaaa", "body": super::finding_marker("suppressed")} + ]))) + .expect(1) + .mount(&server) + .await; + dedup_github(&server) + .reconcile_published_finding_markers(&mut receipt, &envelope, "aaaaaaaaaaaa") + .await; + assert_eq!( + receipt.findings[0].initial_outcome, + FindingPublicationOutcome::Carried + ); + assert_eq!(receipt.findings[0].comment_id.as_deref(), Some("501")); + assert_eq!( + receipt.findings[1].initial_outcome, + FindingPublicationOutcome::Resolved + ); + assert!(receipt.findings[1].comment_id.is_none()); + assert_eq!( + receipt.findings[2].initial_outcome, + FindingPublicationOutcome::Suppressed + ); + assert!(receipt.findings[2].comment_id.is_none()); + } + /// 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. diff --git a/src/review.rs b/src/review.rs index f275bcf..b5d350f 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1337,6 +1337,78 @@ fn generate_finding_ids(findings: &mut [Finding], head_sha: Option<&str>) { } } +/// A publication identity has one lifecycle state. Active evidence takes +/// precedence over retirement of another adjudication candidate for that ID. +fn reconcile_finding_identities( + cfg: &Config, + head_sha: Option<&str>, + findings: &mut Vec, + resolved: &mut Vec, + suppressed: &mut Vec, +) -> usize { + generate_finding_ids(findings, head_sha); + generate_finding_ids(resolved, head_sha); + for entry in suppressed.iter_mut() { + generate_finding_ids(std::slice::from_mut(&mut entry.finding), head_sha); + } + let blocking_kinds = cfg + .block_on_kinds + .iter() + .map(|kind| kind.as_str().to_string()) + .collect::>(); + let priority = |finding: &Finding| { + ( + crate::envelope::finding_blocks_gate( + finding, + cfg.gate_fail_on.as_str(), + &blocking_kinds, + finding.path == crate::envelope::PROVIDER_PATH + && cfg.gate_on_error == OnError::Advisory, + ), + !finding.machine_claim_deferred, + finding.severity, + ) + }; + let mut positions = HashMap::::new(); + let mut active = Vec::::new(); + for finding in findings.drain(..) { + if let Some(id) = finding.id.as_ref() { + if let Some(&position) = positions.get(id) { + let previous = &active[position]; + // Keep an intact candidate, including its evidence and scorer + // provenance. Selecting by severity alone can erase a kind + // blocker or promote a deferred machine claim. + let order = priority(&finding) + .cmp(&priority(previous)) + .then(finding.confidence.total_cmp(&previous.confidence)) + .then_with(|| filter::is_carried(previous).cmp(&filter::is_carried(&finding))); + if order.is_gt() { + active[position] = finding; + } + continue; + } + positions.insert(id.clone(), active.len()); + } + active.push(finding); + } + *findings = active; + let mut seen = positions + .into_keys() + .collect::>(); + // Adjudication emits a suppression alongside each retired baseline. + // Retain the resolution for its thread lifecycle and count it only there. + resolved.retain(|finding| finding.id.as_ref().is_none_or(|id| seen.insert(id.clone()))); + let previous_suppressed = suppressed.len(); + suppressed.retain(|entry| { + entry + .finding + .id + .as_ref() + .is_none_or(|id| seen.insert(id.clone())) + }); + previous_suppressed - suppressed.len() +} + fn scorer_inputs( finding_batches: &[String], evidence_corpus: &[String], @@ -2983,6 +3055,18 @@ async fn review_diff_at( findings.push(failure); } + let mut resolved = rec.resolved; + resolved.extend(adjudication_resolved); + resolved.extend(machine_application.resolved); + let removed_suppressions = reconcile_finding_identities( + cfg, + head_sha.as_deref(), + &mut findings, + &mut resolved, + &mut suppressed_findings, + ); + suppressed = suppressed.saturating_sub(removed_suppressions as u32); + for finding in findings .iter() .filter(|finding| crate::envelope::is_ephemeral_anchor(&finding.path)) @@ -3026,14 +3110,8 @@ async fn review_diff_at( counts.ungrounded = ungrounded; let buckets = Envelope::buckets_of(&findings); - // Generate stable IDs for findings - generate_finding_ids(&mut findings, head_sha.as_deref()); model_usage.sort_by_key(|entry| entry.call_ordinal.unwrap_or(u32::MAX)); - let mut resolved = rec.resolved; - resolved.extend(adjudication_resolved); - resolved.extend(machine_application.resolved); - Ok(Envelope { version: 1, summary, @@ -4668,6 +4746,147 @@ mod tests { ); } + #[test] + fn lifecycle_identity_keeps_blocking_evidence_and_excludes_terminal_copies() { + let cfg = Config::default(); + let mut blocker = finding("src/auth.rs", 7, "The guard is bypassed."); + blocker.evidence = Some("execute();".into()); + blocker.scorer_reason = Some("The guard is absent before execution.".into()); + let mut weaker = blocker.clone(); + weaker.severity = Severity::Warn; + weaker.confidence = 0.99; + weaker.body = "A weaker assessment of the guard.".into(); + for reverse in [false, true] { + let mut findings = vec![weaker.clone(), blocker.clone()]; + if reverse { + findings.reverse(); + } + let mut resolved = vec![blocker.clone()]; + let mut suppressed = vec![SuppressedFinding { + finding: blocker.clone(), + reason: SuppressionReason::DuplicateRootCause, + }]; + assert_eq!( + reconcile_finding_identities( + &cfg, + None, + &mut findings, + &mut resolved, + &mut suppressed, + ), + 1 + ); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].body, blocker.body); + assert_eq!(findings[0].scorer_reason, blocker.scorer_reason); + assert_eq!(findings[0].evidence, blocker.evidence); + assert!(crate::envelope::finding_blocks_gate( + &findings[0], + "error", + &[], + false + )); + assert!(resolved.is_empty()); + assert!(suppressed.is_empty()); + } + } + + #[test] + fn lifecycle_identity_preserves_kind_and_supported_machine_claim_blockers() { + let cfg = Config { + block_on_kinds: vec![Kind::HumanEscalation], + ..Config::default() + }; + let mut blocker = finding("src/auth.rs", 7, "The supported concern needs review."); + blocker.id = Some("same-lifecycle".into()); + blocker.kind = Kind::HumanEscalation; + blocker.severity = Severity::Warn; + blocker.confidence = 0.4; + for deferred in [false, true] { + let mut nonblocking = blocker.clone(); + nonblocking.severity = Severity::Error; + nonblocking.confidence = if deferred { 0.99 } else { 0.2 }; + nonblocking.machine_claim_deferred = deferred; + let mut findings = vec![nonblocking, blocker.clone()]; + reconcile_finding_identities( + &cfg, + None, + &mut findings, + &mut Vec::new(), + &mut Vec::new(), + ); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].severity, Severity::Warn); + assert!(!findings[0].machine_claim_deferred); + assert!(crate::envelope::finding_blocks_gate( + &findings[0], + "error", + &["humanEscalation".into()], + false, + )); + } + } + + #[test] + fn lifecycle_identity_keeps_distinct_duplicate_suppressions_separate_from_active_evidence() { + let mut active = finding("src/auth.rs", 7, "The guard is bypassed."); + active.id = Some("active-concern".into()); + let mut duplicate = active.clone(); + duplicate.id = Some("duplicate-candidate".into()); + let mut findings = vec![active]; + let mut resolved = Vec::new(); + let mut suppressed = vec![SuppressedFinding { + finding: duplicate, + reason: SuppressionReason::DuplicateRootCause, + }]; + assert_eq!( + reconcile_finding_identities( + &Config::default(), + None, + &mut findings, + &mut resolved, + &mut suppressed, + ), + 0 + ); + assert_eq!(findings.len(), 1); + assert_eq!(suppressed.len(), 1); + assert_eq!(suppressed[0].reason, SuppressionReason::DuplicateRootCause); + assert!(resolved.is_empty()); + assert!(crate::envelope::finding_blocks_gate( + &findings[0], + "error", + &[], + false + )); + } + + #[test] + fn lifecycle_identity_deduplicates_terminal_states_without_merging_distinct_evidence() { + let mut first = finding("src/auth.rs", 7, "First source operation."); + first.evidence = Some("first();".into()); + let mut second = first.clone(); + second.evidence = Some("second();".into()); + let mut findings = vec![first.clone(), second]; + let retired = finding("src/retired.rs", 9, "Retired concern."); + let mut resolved = vec![retired.clone(), retired.clone()]; + let mut suppressed = vec![SuppressedFinding { + finding: retired, + reason: SuppressionReason::NonActionable, + }]; + reconcile_finding_identities( + &Config::default(), + Some("head"), + &mut findings, + &mut resolved, + &mut suppressed, + ); + assert_eq!(findings.len(), 2); + assert_ne!(findings[0].id, findings[1].id); + assert_eq!(resolved.len(), 1); + assert!(suppressed.is_empty()); + } + #[test] fn stable_ids_do_not_share_duplicate_buckets_across_path_line_boundaries() { let head_sha = "0123456789abcdef0123456789abcdef01234567"; diff --git a/tests/e2e.rs b/tests/e2e.rs index 87f6d11..54da0d4 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -13851,7 +13851,7 @@ async fn pending_refutation_recovery_commits_baseline_resolution_only_after_scor envelope["resolved"][0]["title"], "Legacy API has no callers" ); - assert_eq!(envelope["counts"]["suppressed"], 2); + assert_eq!(envelope["counts"]["suppressed"], 1); assert_eq!( envelope["suppressedFindings"] .as_array() @@ -13859,7 +13859,7 @@ async fn pending_refutation_recovery_commits_baseline_resolution_only_after_scor .iter() .map(|entry| entry["finding"]["title"].as_str().unwrap()) .collect::>(), - vec!["Legacy API has no callers", "Preserve login audit"] + vec!["Preserve login audit"] ); } else { assert_eq!(envelope["resolved"], json!([])); @@ -14430,6 +14430,153 @@ async fn oversized_adjudication_payload_preserves_findings_without_aborting_revi ); } +#[tokio::test] +async fn lifecycle_identity_reconciles_duplicate_baseline_adjudication() { + use sha2::{Digest, Sha256}; + + for duplicate_baseline in [true, false] { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + mount_successful_hosted_check_patches(&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)) + .with_priority(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .mount(&server) + .await; + let (published_review, published_comments) = published_review_responders(); + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(published_review) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7/reviews/77/comments")) + .respond_with(published_comments) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/repos/acme/api/pulls/7/reviews/77")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + let title = "Authorization guard remains bypassed"; + let body = "The authorization guard is bypassed before query execution."; + let fresh = json!({ + "path": "src/auth.rs", "line": 42, "severity": "error", "kind": "risk", + "confidence": 0.95, "title": title, "body": body, + "evidence": "exec_query(&token);" + }); + mock_review(&server, json!([fresh.clone()])).await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(body_string_contains("single finding adjudicator")) + .respond_with(move |request: &Request| { + let request: Value = request.body_json().unwrap(); + let payload: Value = serde_json::from_str( + request["messages"].as_array().unwrap().last().unwrap()["content"] + .as_str() + .unwrap(), + ) + .unwrap(); + let candidates = payload["candidates"].as_array().unwrap(); + assert_eq!(candidates.len(), 2); + let results = candidates + .iter() + .enumerate() + .map(|(index, candidate)| { + json!({ + "candidateId": candidate["candidateId"], + "status": "confirmed", + "revisedTitle": title, "revisedBody": body, + "evidence": "exec_query(&token);", + "duplicateOf": if index == 1 && duplicate_baseline { + candidates[0]["candidateId"].clone() + } else { Value::Null } + }) + }) + .collect::>(); + ResponseTemplate::new(200).set_body_json(scorer_text(&json!(results).to_string())) + }) + .with_priority(1) + .expect(1) + .mount(&server) + .await; + let identity = format!( + "evidence\0risk\0src/auth.rs\0{}\0exec_query(&token);", + title.to_lowercase() + ); + let id = Sha256::digest(identity.as_bytes()) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::(); + let mut previous = fresh; + previous["id"] = json!(id); + previous["confidence"] = json!(0.9); + previous["body"] = json!("The same query bypasses the authorization guard."); + let baseline = json!({ + "version": 1, "summary": "", "silent": false, "findings": [previous], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], "gate": {"failOn": "error", "failing": true}, + "modelUsed": "model", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": null, "headSha": null, "sinceSha": null + }); + let directory = tempfile::tempdir().unwrap(); + let baseline_path = directory.path().join("baseline.json"); + let receipt_path = directory.path().join("receipt.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + let output = postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_DISABLE_SCORER", "1") + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", format!("fixture-{}", std::process::id())) + .env("POSTIL_PUBLICATION_RECEIPT_PATH", &receipt_path) + .args([ + "review", + "--publish", + "--repo", + "acme/api", + "--pr", + "7", + "--check-run-id", + "901", + "--gate-check-run-id", + "902", + ]) + .arg("--baseline") + .arg(baseline_path) + .args(["--output", "json"]) + .assert() + .code(1); + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + assert_eq!(envelope["findings"].as_array().unwrap().len(), 1); + assert_eq!(envelope["findings"][0]["id"], id); + assert_eq!(envelope["findings"][0]["body"], body); + assert_eq!(envelope["findings"][0]["confidence"], 0.95); + assert_eq!(envelope["resolved"], json!([])); + assert!(envelope.get("suppressedFindings").is_none()); + assert_eq!(envelope["counts"]["error"], 1); + assert_eq!(envelope["counts"]["suppressed"], 0); + assert_eq!(envelope["confidenceBuckets"], json!([0, 0, 0, 0, 1])); + assert_eq!(envelope["gate"]["failing"], true); + assert_model_usage_matches_aggregate(&envelope); + let receipt: Value = serde_json::from_slice(&std::fs::read(receipt_path).unwrap()).unwrap(); + assert_eq!(receipt["findings"].as_array().unwrap().len(), 1); + assert_eq!(receipt["findings"][0]["findingId"], id); + assert_eq!(receipt["findings"][0]["initialOutcome"], "inline"); + assert_eq!(receipt["findings"][0]["commentId"], "500"); + } +} + #[tokio::test] async fn scorer_cannot_suppress_an_unresolved_full_rereview_baseline() { for status in ["unresolved", "refuted"] {