diff --git a/README.md b/README.md index b984460..693aa2f 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ export GITHUB_TOKEN=... postil review --repo owner/repository --pr 123 --publish ``` -`--publish` is required for any forge write. Without it, the CLI fetches the pull request and reports locally. Published runs create inline findings and separate `postil/review` and `postil/gate` checks. Mark only `postil/gate` as required in branch protection. +`--publish` is required for any forge write. Without it, the CLI fetches the pull request and reports locally. Published runs create separate `postil/review` and `postil/gate` checks. Findings appear in one batched review by default; GitHub repositories can set `review.findingPresentation: checkAnnotations` to put them on the advisory check instead. Mark only `postil/gate` as required in branch protection. For GitHub Actions, use [`postil-action`](https://github.com/postil-dev/postil-action). Hosted GitHub reviews are available at [postil.dev/install](https://postil.dev/install). diff --git a/docs/configuration.md b/docs/configuration.md index d84a5fc..0afddda 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -23,6 +23,7 @@ reviewer: focus: [security, concurrency] review: onClean: skip + findingPresentation: reviewComments # reviewComments or checkAnnotations (GitHub only) uncertaintyResolution: true # resolve uncertainty findings from referenced repository files conciseFindings: true # compress over-long finding bodies before rendering gate: diff --git a/docs/forges.md b/docs/forges.md index 36a30a0..83cd921 100644 --- a/docs/forges.md +++ b/docs/forges.md @@ -14,6 +14,8 @@ postil review --repo owner/repository --pr 123 --publish `GITHUB_API_URL` selects a GitHub Enterprise Server API. GitHub review delivery needs pull-request and check-run write access. +Findings appear in one batched review by default. Set `review.findingPresentation: checkAnnotations` to publish them only as annotations on `postil/review`. Both modes retain the advisory and gate check summaries and conclusions. + Automated callers can bind a run to an observed pull-request snapshot with `--sha ` and `--base-sha `. Before each GitHub write, Postil verifies the head commit, target-branch commit, and merge base from the acquired review snapshot. A changed value suppresses delivery. ## GitLab diff --git a/src/config.rs b/src/config.rs index 49430ff..036603b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -870,6 +870,26 @@ pub enum OnClean { Comment, } +/// Where GitHub renders individual findings. Check runs always retain their +/// status and summary; this setting selects one detailed finding surface. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum FindingPresentation { + /// Publish findings as one batched pull-request review with inline comments. + ReviewComments, + /// Publish findings as annotations on the advisory check run. + CheckAnnotations, +} + +impl FindingPresentation { + pub fn as_str(self) -> &'static str { + match self { + Self::ReviewComments => "reviewComments", + Self::CheckAnnotations => "checkAnnotations", + } + } +} + /// What the gate does when the review cannot complete (model outage, rate-limit /// exhaustion). Default is fail closed. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] @@ -878,7 +898,7 @@ pub enum OnError { /// Operational error fails the gate. Nothing merges on a broken review. Block, /// Provider outage passes the gate (advisory only): an outage does not - /// freeze every merge in the org; the review check goes neutral. Unusable + /// freeze every merge in the org; the review check still fails. Unusable /// model output still blocks because that class is attacker-influenceable. Advisory, } @@ -924,6 +944,7 @@ pub struct Config { pub tone: String, pub focus: Vec, pub on_clean: OnClean, + pub finding_presentation: FindingPresentation, pub uncertainty_resolution: bool, pub concise_findings: bool, pub gate_fail_on: GateLevel, @@ -970,6 +991,7 @@ impl Default for Config { tone: "concise, dry, lightly sardonic, never hostile; no praise or filler".to_string(), focus: Vec::new(), on_clean: OnClean::Skip, + finding_presentation: FindingPresentation::ReviewComments, uncertainty_resolution: true, concise_findings: true, gate_fail_on: GateLevel::Severity(Severity::Error), @@ -1019,6 +1041,7 @@ pub struct ReviewerSection { #[serde(deny_unknown_fields, rename_all = "camelCase")] pub struct ReviewSection { pub on_clean: Option, + pub finding_presentation: Option, pub uncertainty_resolution: Option, pub concise_findings: Option, } @@ -1168,6 +1191,9 @@ impl Config { if let Some(oc) = r.on_clean { self.on_clean = oc; } + if let Some(presentation) = r.finding_presentation { + self.finding_presentation = presentation; + } if let Some(enabled) = r.uncertainty_resolution { self.uncertainty_resolution = enabled; } @@ -1932,6 +1958,7 @@ reviewer: review: onClean: skip # skip = stay silent on clean PRs (default) | comment + # findingPresentation: reviewComments # reviewComments (default) | checkAnnotations (GitHub only) # Reviews fetch referenced repository files to resolve uncertainty findings # by default. Uncomment this explicit opt-out to disable that pass. # uncertaintyResolution: false @@ -1945,7 +1972,7 @@ gate: - humanEscalation # genuine owner/product decisions only; concrete bugs remain risk # onError: block # block (default, fail closed) | advisory: gate outcome when # # the review itself errors (model outage). advisory keeps an - # # outage from freezing merges; the review check goes neutral, not green. + # # outage from freezing merges; the review check still fails. model: name: __DEFAULT_MODEL__ @@ -2271,6 +2298,7 @@ scorer = { enabled = true, default_model = "provider/scorer", fallback = "provid let c = Config::default(); assert_eq!(c.min_confidence, 0.6); assert_eq!(c.on_clean, OnClean::Skip); + assert_eq!(c.finding_presentation, FindingPresentation::ReviewComments); assert!(c.uncertainty_resolution); assert!(c.concise_findings); assert!(matches!( @@ -2288,6 +2316,18 @@ scorer = { enabled = true, default_model = "provider/scorer", fallback = "provid assert!(config.uncertainty_resolution); } + #[test] + fn file_selects_check_annotation_finding_presentation() { + let file: FileConfig = + yaml_serde::from_str("review:\n findingPresentation: checkAnnotations\n").unwrap(); + let mut config = Config::default(); + config.apply_file_inner(file, false, false).unwrap(); + assert_eq!( + config.finding_presentation, + FindingPresentation::CheckAnnotations + ); + } + #[test] fn file_disables_concise_findings() { let file: FileConfig = yaml_serde::from_str("review:\n conciseFindings: false\n").unwrap(); diff --git a/src/forge/azure.rs b/src/forge/azure.rs index 62121ff..a6b488b 100644 --- a/src/forge/azure.rs +++ b/src/forge/azure.rs @@ -17,8 +17,8 @@ use serde_json::json; use std::io::Write; use super::{ - CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, check_title, - untracked_review_publication_receipt, + CheckRunIds, CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, + check_title, untracked_review_publication_receipt, }; use crate::diff::{DiffSnapshot, DiffSpool, WorkspaceBudget}; use crate::envelope::{Envelope, Finding}; @@ -527,12 +527,12 @@ impl Forge for Azure { async fn complete_checks( &self, - _advisory_id: &str, - _gate_id: &str, + _check_ids: CheckRunIds<'_>, advisory: CheckState, gate: Option, envelope: &Envelope, snapshot: &PrMeta, + _annotate_findings: bool, ) -> Result<()> { if !self.snapshot_is_current(snapshot).await? { eprintln!("postil: azure status delivery skipped because the pull request changed"); diff --git a/src/forge/bitbucket.rs b/src/forge/bitbucket.rs index a9fe54f..fc9c2ae 100644 --- a/src/forge/bitbucket.rs +++ b/src/forge/bitbucket.rs @@ -21,8 +21,8 @@ use sha2::Digest; use std::io::Write; use super::{ - CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, check_title, - untracked_review_publication_receipt, + CheckRunIds, CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, + check_title, untracked_review_publication_receipt, }; use crate::diff::{DiffSnapshot, DiffSpool, WorkspaceBudget}; use crate::envelope::{Envelope, Finding}; @@ -575,12 +575,12 @@ impl Forge for Bitbucket { async fn complete_checks( &self, - _advisory_id: &str, - _gate_id: &str, + _check_ids: CheckRunIds<'_>, advisory: CheckState, gate: Option, envelope: &Envelope, snapshot: &PrMeta, + _annotate_findings: bool, ) -> Result<()> { if !self.snapshot_is_current(snapshot).await? { eprintln!("postil: bitbucket status delivery skipped because the pull request changed"); diff --git a/src/forge/github.rs b/src/forge/github.rs index 048e3a8..59e6037 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -11,7 +11,7 @@ use std::io::Write; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use super::{ - CheckState, FindingPublicationOutcome, FindingPublicationReceipt, Forge, PrMeta, + CheckRunIds, CheckState, FindingPublicationOutcome, FindingPublicationReceipt, Forge, PrMeta, ReviewPublicationReceipt, ReviewPublicationSummary, SummaryContext, ThreadKind, check_summary, check_title, only_operational_findings, valid_details_url, }; @@ -20,6 +20,12 @@ use crate::envelope::{Envelope, Finding, Severity}; use crate::filter; pub const EXPECTED_REPOSITORY_ID_ENV: &str = "POSTIL_EXPECTED_GITHUB_REPO_ID"; +const GITHUB_MAX_ANNOTATIONS_PER_REQUEST: usize = 50; + +// Filtering caps visible findings before forge publication, so one completed +// check update always fits GitHub's annotation request limit. Raising the +// product cap requires implementing multi-request annotation delivery first. +const _: () = assert!(crate::config::MAX_FINDINGS <= GITHUB_MAX_ANNOTATIONS_PER_REQUEST); pub struct GitHub { http: reqwest::Client, @@ -1449,12 +1455,12 @@ impl Forge for GitHub { async fn complete_checks( &self, - advisory_id: &str, - gate_id: &str, + check_ids: CheckRunIds<'_>, advisory: CheckState, gate: Option, envelope: &Envelope, snapshot: &PrMeta, + annotate_findings: bool, ) -> Result<()> { if !self.snapshot_is_current(snapshot).await? { return Err(anyhow!( @@ -1469,32 +1475,39 @@ impl Forge for GitHub { CheckState::Failure => "failure", CheckState::Neutral => "neutral", }; - let annotations: Vec<_> = envelope - .findings - .iter() - // Synthetic-path findings have no real file line to annotate; they - // are already carried in the check-run summary body. - .filter(|f| !super::is_synthetic_path(&f.path)) - .take(50) // GitHub caps annotations per request at 50. - .map(|f| { - let publication = crate::envelope::forge_safe_finding_publication_text(f); - json!({ - "path": f.path, - "start_line": f.line, - "end_line": f.end_line.unwrap_or(f.line), - "annotation_level": match f.severity { - Severity::Info => "notice", - Severity::Warn => "warning", - Severity::Error => "failure", - }, - "title": publication.title, - "message": publication.body, + let annotations: Vec<_> = if annotate_findings { + envelope + .findings + .iter() + // Carried findings remain visible on the review that introduced + // them. Re-annotating can also target a stale line range. + .filter(|f| !filter::is_carried(f)) + // Synthetic-path findings have no real file line to annotate; + // they are already carried in the check-run summary body. + .filter(|f| !super::is_synthetic_path(&f.path)) + .map(|f| { + let publication = crate::envelope::forge_safe_finding_publication_text(f); + json!({ + "path": f.path, + "start_line": f.line, + "end_line": f.end_line.unwrap_or(f.line), + "annotation_level": match f.severity { + Severity::Info => "notice", + Severity::Warn => "warning", + Severity::Error => "failure", + }, + "title": publication.title, + "message": publication.body, + }) }) - }) - .collect(); - let mut checks = vec![(advisory_id, advisory, "postil/review", true)]; + .collect() + } else { + Vec::new() + }; + debug_assert!(annotations.len() <= GITHUB_MAX_ANNOTATIONS_PER_REQUEST); + let mut checks = vec![(check_ids.advisory, advisory, "postil/review", true)]; if let Some(gate) = gate { - checks.push((gate_id, gate, "postil/gate", false)); + checks.push((check_ids.gate, gate, "postil/gate", false)); } let mut results = stream::iter(checks.into_iter().enumerate().map( |(index, (id, state, name, with_annotations))| { @@ -1525,7 +1538,7 @@ impl Forge for GitHub { "title": super::cap_check_title(&title), "summary": super::cap_check_summary(&gate_note), }); - if with_annotations && !annotations.is_empty() { + if annotate_findings && with_annotations && !annotations.is_empty() { output["annotations"] = json!(annotations); } let mut body = json!({ @@ -1672,7 +1685,7 @@ fn planned_review_receipt(envelope: &Envelope, head_sha: &str) -> ReviewPublicat })); let mut digest = Sha256::new(); - digest.update(b"github-review-receipt-v1\0"); + digest.update(b"github-review-receipt-v2\0"); digest.update(head_sha.as_bytes()); for finding in &findings { digest.update(finding.finding_id.as_bytes()); @@ -1681,8 +1694,9 @@ fn planned_review_receipt(envelope: &Envelope, head_sha: &str) -> ReviewPublicat let hash = digest.finalize(); ReviewPublicationReceipt { version: ReviewPublicationReceipt::VERSION, + channel: super::ReviewPublicationChannel::ReviewComments, receipt_id: format!( - "github-review-v1:{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", + "github-review-v2:{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] ), review_id: None, @@ -1698,6 +1712,7 @@ fn publication_summary(receipt: &ReviewPublicationReceipt) -> ReviewPublicationS summary.active_inline += 1; } FindingPublicationOutcome::Inline => {} + FindingPublicationOutcome::CheckAnnotation => summary.summary_only += 1, FindingPublicationOutcome::SummaryOnly => summary.summary_only += 1, FindingPublicationOutcome::Carried => summary.carried += 1, FindingPublicationOutcome::Resolved @@ -1790,7 +1805,7 @@ mod tests { use crate::envelope::{ Envelope, Finding, Gate, Kind, Severity, SuppressedFinding, SuppressionReason, Usage, }; - use crate::forge::{CheckState, FindingPublicationOutcome, Forge, PrMeta}; + use crate::forge::{CheckRunIds, CheckState, FindingPublicationOutcome, Forge, PrMeta}; use reqwest::header::{HeaderMap, HeaderValue}; use std::sync::{ Arc, @@ -2192,8 +2207,12 @@ mod tests { .await .unwrap(); - assert_eq!(receipt.version, 1); - assert!(receipt.receipt_id.starts_with("github-review-v1:")); + assert_eq!(receipt.version, 2); + assert_eq!( + receipt.channel, + super::super::ReviewPublicationChannel::ReviewComments + ); + assert!(receipt.receipt_id.starts_with("github-review-v2:")); assert_eq!(receipt.review_id.as_deref(), Some("77")); let outcome = |id: &str| { receipt @@ -2544,12 +2563,15 @@ mod tests { let error = test_github(&server) .complete_checks( - "11", - "12", + CheckRunIds { + advisory: "11", + gate: "12", + }, CheckState::Success, Some(CheckState::Success), &delivery_envelope("aaaaaaaaaaaa", "cccccccccccc"), &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + false, ) .await .unwrap_err(); @@ -2578,12 +2600,15 @@ mod tests { let result = tokio::time::timeout( Duration::from_millis(100), github.complete_checks( - "11", - "12", + CheckRunIds { + advisory: "11", + gate: "12", + }, CheckState::Success, Some(CheckState::Success), &envelope, &snapshot, + false, ), ) .await; @@ -2950,12 +2975,15 @@ mod tests { assert!(review_error.to_string().contains("PR snapshot changed")); let check_error = github .complete_checks( - "11", - "12", + CheckRunIds { + advisory: "11", + gate: "12", + }, CheckState::Success, Some(CheckState::Success), &delivery_envelope("aaaaaaaaaaaa", "cccccccccccc"), &snapshot, + false, ) .await .unwrap_err(); @@ -3024,12 +3052,15 @@ mod tests { assert!(review_error.to_string().contains("PR snapshot changed")); let check_error = github .complete_checks( - "11", - "12", + CheckRunIds { + advisory: "11", + gate: "12", + }, CheckState::Success, Some(CheckState::Success), &envelope, &snapshot, + false, ) .await .unwrap_err(); @@ -3127,7 +3158,7 @@ mod tests { } #[tokio::test] - async fn github_review_and_check_annotations_preserve_valid_model_prose() { + async fn github_finding_presentations_are_exclusive_and_preserve_valid_model_prose() { let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/repos/owner/repo/pulls/1")) @@ -3165,7 +3196,7 @@ mod tests { .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) .mount(&server) .await; - for id in ["11", "12"] { + for id in ["11", "12", "13", "14"] { Mock::given(method("PATCH")) .and(path(format!("/repos/owner/repo/check-runs/{id}"))) .respond_with(ResponseTemplate::new(200)) @@ -3238,12 +3269,29 @@ mod tests { .unwrap(); github .complete_checks( - "11", - "12", + CheckRunIds { + advisory: "11", + gate: "12", + }, CheckState::Failure, Some(CheckState::Failure), &envelope, &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + false, + ) + .await + .unwrap(); + github + .complete_checks( + CheckRunIds { + advisory: "13", + gate: "14", + }, + CheckState::Failure, + Some(CheckState::Failure), + &envelope, + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + true, ) .await .unwrap(); @@ -3260,10 +3308,22 @@ mod tests { let inline = review_body["comments"][0]["body"].as_str().unwrap(); assert!(inline.contains(&finding.body)); - let advisory = requests + let review_comment_advisory = requests .iter() .find(|request| request.url.path().ends_with("/check-runs/11")) .unwrap(); + let review_comment_check_body: serde_json::Value = + serde_json::from_slice(&review_comment_advisory.body).unwrap(); + assert!( + review_comment_check_body["output"] + .get("annotations") + .is_none() + ); + + let advisory = requests + .iter() + .find(|request| request.url.path().ends_with("/check-runs/13")) + .unwrap(); let check_body: serde_json::Value = serde_json::from_slice(&advisory.body).unwrap(); let annotation = &check_body["output"]["annotations"][0]; let title = annotation["title"].as_str().unwrap(); diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index 323e177..6846daa 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -10,8 +10,8 @@ use serde_json::json; use std::io::Write; use super::{ - CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, check_title, - untracked_review_publication_receipt, + CheckRunIds, CheckState, Forge, PrMeta, ReviewPublicationReceipt, ThreadKind, check_summary, + check_title, untracked_review_publication_receipt, }; use crate::diff::{DiffSnapshot, DiffSpool, WorkspaceBudget}; use crate::envelope::{Envelope, Finding}; @@ -547,12 +547,12 @@ impl Forge for GitLab { async fn complete_checks( &self, - _advisory_id: &str, - _gate_id: &str, + _check_ids: CheckRunIds<'_>, advisory: CheckState, gate: Option, envelope: &Envelope, snapshot: &PrMeta, + _annotate_findings: bool, ) -> Result<()> { let current = self.mr().await?; if !mr_matches_snapshot(¤t, snapshot) { diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 35ee9e0..f79a624 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -377,6 +377,12 @@ pub enum CheckState { Neutral, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CheckRunIds<'a> { + pub advisory: &'a str, + pub gate: &'a str, +} + /// What a `respond` thread number points at. GitHub's issues API covers both, /// so it ignores this; GitLab/Bitbucket/Azure key issues and PRs on different /// endpoints, so they branch on it. @@ -391,10 +397,18 @@ pub enum ThreadKind { /// Versioned result of one review publication attempt. Finding outcomes are /// keyed by the envelope's stable finding ID so the hosted service can join /// the immutable delivery result to later thread lifecycle observations. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum ReviewPublicationChannel { + ReviewComments, + CheckAnnotations, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ReviewPublicationReceipt { pub version: u8, + pub channel: ReviewPublicationChannel, pub receipt_id: String, #[serde(skip_serializing_if = "Option::is_none")] pub review_id: Option, @@ -402,7 +416,7 @@ pub struct ReviewPublicationReceipt { } impl ReviewPublicationReceipt { - pub const VERSION: u8 = 1; + pub const VERSION: u8 = 2; } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -434,6 +448,7 @@ fn is_true(value: &bool) -> bool { #[serde(rename_all = "camelCase")] pub enum FindingPublicationOutcome { Inline, + CheckAnnotation, SummaryOnly, Carried, Resolved, @@ -493,7 +508,7 @@ pub fn untracked_review_publication_receipt( }); } let mut digest = Sha256::new(); - digest.update(b"review-receipt-v1\0"); + digest.update(b"review-receipt-v2\0"); digest.update(forge.as_bytes()); digest.update(head_sha.as_bytes()); for finding in &findings { @@ -502,8 +517,9 @@ pub fn untracked_review_publication_receipt( let hash = digest.finalize(); ReviewPublicationReceipt { version: ReviewPublicationReceipt::VERSION, + channel: ReviewPublicationChannel::ReviewComments, receipt_id: format!( - "{forge}-review-v1:{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", + "{forge}-review-v2:{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] ), review_id: None, @@ -614,12 +630,12 @@ pub trait Forge { /// while the acquired snapshot remains current. async fn complete_checks( &self, - advisory_id: &str, - gate_id: &str, + check_ids: CheckRunIds<'_>, advisory: CheckState, gate: Option, envelope: &Envelope, snapshot: &PrMeta, + annotate_findings: bool, ) -> Result<()>; /// Confirm that publication still targets the snapshot that was reviewed. @@ -1198,7 +1214,8 @@ mod tests { let path = directory.path().join("publication.json"); let receipt = ReviewPublicationReceipt { version: ReviewPublicationReceipt::VERSION, - receipt_id: "github-review-v1:test".into(), + channel: ReviewPublicationChannel::ReviewComments, + receipt_id: "github-review-v2:test".into(), review_id: Some("77".into()), findings: vec![FindingPublicationReceipt { finding_id: "finding-1".into(), diff --git a/src/main.rs b/src/main.rs index 60d7565..132cefd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -147,6 +147,10 @@ async fn dispatch(cli: Cli) -> anyhow::Result { postil_cli::config::OnClean::Comment => "comment", } ); + println!( + "review.findingPresentation: {}", + cfg.finding_presentation.as_str() + ); println!( "review.uncertaintyResolution: {}", cfg.uncertainty_resolution diff --git a/src/review.rs b/src/review.rs index f1f3193..7a7428e 100644 --- a/src/review.rs +++ b/src/review.rs @@ -6,7 +6,7 @@ use std::time::{Duration, Instant}; use anyhow::{Context, Result, anyhow}; use futures::StreamExt; -use crate::config::{Config, GateLevel, OnError}; +use crate::config::{Config, FindingPresentation, GateLevel, OnError}; use crate::diff; use crate::durable_plan::{DurablePlanRegistrar, DurableReviewPlan}; use crate::envelope::{ @@ -389,6 +389,14 @@ pub async fn run(args: ReviewArgs) -> Result { cfg.gate_fail_on = GateLevel::parse(fo).ok_or_else(|| anyhow!("invalid --fail-on {fo:?}"))?; } + if !args.no_post + && cfg.finding_presentation == FindingPresentation::CheckAnnotations + && args.forge != ForgeKind::GitHub + { + return Err(anyhow!( + "review.findingPresentation checkAnnotations requires GitHub publication" + )); + } match args.forge { ForgeKind::Local => run_local(&args, &cfg, &cwd).await, @@ -527,6 +535,11 @@ async fn run_remote( if crate::forge::is_repository_identity_failure(&e) { return Err(e).context("creating check runs"); } + if cfg.finding_presentation == FindingPresentation::CheckAnnotations { + return Err(e).context( + "creating check runs required by review.findingPresentation checkAnnotations", + ); + } // CI tokens without checks:write still get review + exit code. eprintln!("postil: cannot create check runs ({e:#}); continuing without"); None @@ -554,16 +567,15 @@ async fn run_remote( } else { CheckState::Success }; - // An operational failure inside the review (provider outage, - // unusable output) must stay visible on the advisory check. - // green-on-green would make an outage look like a clean pass - // when the gate stands aside under `gate.onError: advisory`. + // `postil/review` reports whether a review verdict exists. It + // fails when inference is operationally incomplete, independent + // of whether `postil/gate` is configured to stand aside. let operational = envelope.findings.iter().any(|f| { f.path == crate::envelope::OPERATIONAL_PATH || f.path == crate::envelope::PROVIDER_PATH }); - let advisory_state = if operational { - CheckState::Neutral + let review_state = if operational { + CheckState::Failure } else { CheckState::Success }; @@ -571,10 +583,11 @@ async fn run_remote( forge, a, g, - advisory_state, + review_state, (!args.defer_gate_check).then_some(gate_state), &envelope, &meta, + cfg.finding_presentation == FindingPresentation::CheckAnnotations, review_started, ) .await @@ -602,8 +615,8 @@ async fn run_remote( eprintln!("postil: review failed before completion ({e:#})"); // Fail closed by default: an errored run must never read as a silent // pass. `gate.onError: advisory` opts a repo out of blocking on - // operational errors (provider outage). The advisory check still - // shows the error; only the gate stands aside. + // operational errors (provider outage). The review check still + // fails truthfully; only the merge gate stands aside. // // Build the error envelope and route it through the SAME output path // (finish) as a successful run: emitting the envelope/SARIF and @@ -627,10 +640,11 @@ async fn run_remote( forge, a, g, - CheckState::Neutral, + CheckState::Failure, (!args.defer_gate_check).then_some(gate_state), &envelope, &meta, + cfg.finding_presentation == FindingPresentation::CheckAnnotations, review_started, ) .await @@ -705,13 +719,24 @@ async fn complete_remote_checks( gate: Option, envelope: &Envelope, snapshot: &PrMeta, + annotate_findings: bool, review_started: Instant, ) -> Result<()> { require_current_snapshot(forge, snapshot, review_started, "check completion").await?; run_with_hosted_budget( Some(review_started), CHECK_COMPLETION_TIMEOUT_SECS, - forge.complete_checks(advisory_id, gate_id, advisory, gate, envelope, snapshot), + forge.complete_checks( + crate::forge::CheckRunIds { + advisory: advisory_id, + gate: gate_id, + }, + advisory, + gate, + envelope, + snapshot, + annotate_findings, + ), "completing check runs", ) .await @@ -1962,7 +1987,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> // Operational findings (model unreachable/unusable) fail the gate by default // and fail closed. `gate.onError: advisory` lets the gate stand aside on a // provider outage so a blip does not freeze every merge; the finding still - // shows in the output and the advisory check goes neutral. Unusable model + // shows in the output and the review check fails. Unusable model // output (OPERATIONAL_PATH) never bypasses the gate: a malicious diff can // induce it via prompt injection. let advisory_on_error = cfg.gate_on_error == OnError::Advisory; @@ -2102,6 +2127,20 @@ async fn finish( { let expected_snapshot = expected_snapshot.context("remote publication is missing its immutable PR snapshot")?; + if cfg.finding_presentation == FindingPresentation::CheckAnnotations { + let mut receipt = forge.plan_review_publication(&envelope, expected_snapshot); + receipt.channel = crate::forge::ReviewPublicationChannel::CheckAnnotations; + for finding in &mut receipt.findings { + if finding.initial_outcome == crate::forge::FindingPublicationOutcome::Inline { + finding.initial_outcome = + crate::forge::FindingPublicationOutcome::CheckAnnotation; + finding.inline_rejected = false; + finding.comment_id = None; + } + } + crate::forge::write_review_publication_receipt_from_env(&receipt)?; + return Ok(if envelope.gate.failing { 1 } else { 0 }); + } let duplicate_of_baseline = load_baseline(args) .ok() .is_some_and(|baseline| visible_finding_sets_equal(&baseline, &envelope.findings)); diff --git a/tests/e2e.rs b/tests/e2e.rs index a0c4b93..b441659 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -6065,14 +6065,14 @@ async fn advisory_on_error_lets_gate_stand_aside() { .map(|r| r.body_json().unwrap()) .collect(); assert_eq!(patches.len(), 2); - // The gate stands aside (success) but the outage stays visible: the - // advisory check goes neutral, never green-on-green. + // The gate stands aside (success), while the review check fails so the + // outage remains visible. let mut conclusions: Vec<&str> = patches .iter() .map(|p| p["conclusion"].as_str().unwrap()) .collect(); conclusions.sort_unstable(); - assert_eq!(conclusions, vec!["neutral", "success"]); + assert_eq!(conclusions, vec!["failure", "success"]); } // Shared setup for the pre-review diff-fetch-failure tests: PR meta succeeds, @@ -6160,7 +6160,7 @@ async fn diff_fetch_failure_advisory_emits_envelope_and_exits_zero() { assert_eq!(env["gate"]["failing"], false); assert_eq!(env["headSha"], "aaaaaaaaaaaa"); - // The advisory check went neutral, the gate success. + // The review check failed, while the gate stood aside with success. let reqs = server.received_requests().await.unwrap(); let conclusions: Vec = reqs .iter() @@ -6172,7 +6172,7 @@ async fn diff_fetch_failure_advisory_emits_envelope_and_exits_zero() { .to_string() }) .collect(); - assert_eq!(conclusions, vec!["neutral", "success"]); + assert_eq!(conclusions, vec!["failure", "success"]); } #[tokio::test] @@ -8580,6 +8580,11 @@ async fn github_flow_posts_review_and_completes_both_checks() { .collect(); assert!(conclusions.contains(&"success")); assert!(conclusions.contains(&"failure")); + assert!( + patches + .iter() + .all(|patch| patch["output"].get("annotations").is_none()) + ); let gate_patch = patches .iter() .find(|patch| patch["conclusion"] == "failure") @@ -8622,6 +8627,74 @@ async fn github_flow_posts_review_and_completes_both_checks() { summary.contains("[Review details](https://postil.dev/orgs/acme/runs/review-7)") ); + std::fs::write( + dir.path().join(".postil.yaml"), + "review:\n findingPresentation: checkAnnotations\n", + ) + .unwrap(); + let annotation_receipt_path = dir.path().join("annotation-receipt.json"); + let request_count_before_annotations = reqs.len(); + postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .env("POSTIL_PUBLICATION_RECEIPT_PATH", &annotation_receipt_path) + .args([ + "review", + "--publish", + "--repo", + "acme/api", + "--pr", + "7", + "--output-json", + ]) + .assert() + .code(1); + let annotation_receipt: Value = + serde_json::from_slice(&std::fs::read(&annotation_receipt_path).unwrap()).unwrap(); + assert_eq!(annotation_receipt["version"], 2); + assert_eq!(annotation_receipt["channel"], "checkAnnotations"); + assert!(annotation_receipt.get("reviewId").is_none()); + assert_eq!( + annotation_receipt["findings"][0]["initialOutcome"], + "checkAnnotation" + ); + assert!(annotation_receipt["findings"][0].get("commentId").is_none()); + let annotation_requests = server.received_requests().await.unwrap(); + let annotation_requests = &annotation_requests[request_count_before_annotations..]; + assert!(!annotation_requests.iter().any(|request| { + request.method == wiremock::http::Method::POST + && request.url.path() == "/repos/acme/api/pulls/7/reviews" + })); + let annotation_patches = annotation_requests + .iter() + .filter(|request| request.method == wiremock::http::Method::PATCH) + .map(|request| request.body_json::().unwrap()) + .collect::>(); + assert_eq!(annotation_patches.len(), 2); + let advisory_annotation = annotation_patches + .iter() + .find(|patch| patch["output"].get("annotations").is_some()) + .expect("advisory check annotation"); + assert_eq!( + advisory_annotation["output"]["annotations"][0]["path"], + "src/auth.rs" + ); + assert_eq!( + advisory_annotation["output"]["annotations"][0]["start_line"], + 41 + ); + assert_eq!( + annotation_patches + .iter() + .filter(|patch| patch["output"].get("annotations").is_some()) + .count(), + 1 + ); + std::fs::remove_file(dir.path().join(".postil.yaml")).unwrap(); + let request_count_before_deferred_gate = server.received_requests().await.unwrap().len(); + let out = postil() .current_dir(dir.path()) .env("POSTIL_API_BASE", server.uri()) @@ -8644,10 +8717,9 @@ async fn github_flow_posts_review_and_completes_both_checks() { assert_eq!(env["gate"]["failing"], true); let reqs = server.received_requests().await.unwrap(); - let conclusions: Vec = reqs + let conclusions: Vec = reqs[request_count_before_deferred_gate..] .iter() .filter(|request| request.method == wiremock::http::Method::PATCH) - .skip(2) .map(|request| { request.body_json::().unwrap()["conclusion"] .as_str()