From 692a766041ad7886726b35f549d6e83f13e04568 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 14 Aug 2026 04:23:39 +0000 Subject: [PATCH 1/7] fix: clarify unavailable inline placement --- src/forge/github.rs | 11 +++-- src/forge/mod.rs | 115 ++++++++++++++++++++++++++++++++++++++++++-- tests/e2e.rs | 10 ++++ 3 files changed, 129 insertions(+), 7 deletions(-) diff --git a/src/forge/github.rs b/src/forge/github.rs index 92c880c..3b6a3a9 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -3334,7 +3334,9 @@ mod tests { )], ); - let receipt = test_github(&server) + let mut github = test_github(&server); + github.details_url = Some("https://postil.dev/orgs/acme/runs/run-1".into()); + let receipt = github .post_review( &envelope, &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), @@ -3357,8 +3359,11 @@ mod tests { let fallback: serde_json::Value = serde_json::from_slice(&posts[1].body).unwrap(); assert!(fallback.get("comments").is_none()); let summary = fallback["body"].as_str().unwrap(); - assert!(summary.contains("1 finding in review details")); - assert!(summary.contains("inline placement unavailable")); + assert!( + summary + .contains("1 finding could not be placed on the changed lines; see review details") + ); + assert!(!summary.contains("inline placement unavailable")); assert!(!summary.contains("Before the next push")); } diff --git a/src/forge/mod.rs b/src/forge/mod.rs index ed838d6..0dc7e4e 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -991,16 +991,43 @@ pub fn check_summary(envelope: &Envelope, rich: bool, context: SummaryContext) - plural(publication.active_inline, "finding", "findings"), )); } - if publication.summary_only > 0 { + if publication.rejected_inline > 0 { + if publication.summary_only > publication.rejected_inline { + if context.details_url.is_some() { + delivery.push(format!( + "{} {} in review details, including {} that could not be placed on the changed lines", + publication.summary_only, + plural(publication.summary_only, "finding", "findings"), + publication.rejected_inline, + )); + } else { + delivery.push(format!( + "{} {} were not posted inline, including {} that could not be placed on the changed lines", + publication.summary_only, + plural(publication.summary_only, "finding", "findings"), + publication.rejected_inline, + )); + } + } else { + let details_direction = if context.details_url.is_some() { + "; see review details" + } else { + "" + }; + delivery.push(format!( + "{} {} could not be placed on the changed lines{}", + publication.rejected_inline, + plural(publication.rejected_inline, "finding", "findings"), + details_direction, + )); + } + } else if publication.summary_only > 0 { delivery.push(format!( "{} {} in review details", publication.summary_only, plural(publication.summary_only, "finding", "findings"), )); } - if publication.rejected_inline > 0 { - delivery.push("inline placement unavailable".to_string()); - } if !delivery.is_empty() { s.push_str(&delivery.join(" ยท ")); s.push('\n'); @@ -1733,6 +1760,86 @@ mod tests { assert!(!without_details.contains("")); } + #[test] + fn publication_summary_explains_unplaced_findings_without_duplicate_counts() { + let env = envelope_with_findings(vec![finding(), finding()]); + let unplaced_only = check_summary( + &env, + true, + SummaryContext { + details_url: Some("https://postil.dev/orgs/acme/runs/run-1".into()), + publication: Some(ReviewPublicationSummary { + summary_only: 2, + rejected_inline: 2, + ..Default::default() + }), + ..Default::default() + }, + ); + assert!( + unplaced_only.contains( + "2 findings could not be placed on the changed lines; see review details" + ) + ); + assert!(!unplaced_only.contains("inline placement unavailable")); + assert!(!unplaced_only.contains("2 findings in review details")); + + let unplaced_without_details_link = check_summary( + &env, + true, + SummaryContext { + publication: Some(ReviewPublicationSummary { + summary_only: 2, + rejected_inline: 2, + ..Default::default() + }), + ..Default::default() + }, + ); + assert!( + unplaced_without_details_link + .contains("2 findings could not be placed on the changed lines") + ); + assert!(!unplaced_without_details_link.contains("see review details")); + + let mixed = check_summary( + &env, + true, + SummaryContext { + details_url: Some("https://postil.dev/orgs/acme/runs/run-1".into()), + publication: Some(ReviewPublicationSummary { + active_inline: 1, + summary_only: 2, + rejected_inline: 1, + ..Default::default() + }), + ..Default::default() + }, + ); + assert!(mixed.contains( + "2 findings in review details, including 1 that could not be placed on the changed lines" + )); + assert!(mixed.contains("1 finding posted inline")); + assert!(!mixed.contains("inline placement unavailable")); + + let mixed_without_details_link = check_summary( + &env, + true, + SummaryContext { + publication: Some(ReviewPublicationSummary { + summary_only: 2, + rejected_inline: 1, + ..Default::default() + }), + ..Default::default() + }, + ); + assert!(mixed_without_details_link.contains( + "2 findings were not posted inline, including 1 that could not be placed on the changed lines" + )); + assert!(!mixed_without_details_link.contains("review details")); + } + #[test] fn review_coverage_stays_out_of_compact_pull_request_summaries() { let mut env = envelope_with_findings(vec![finding()]); diff --git a/tests/e2e.rs b/tests/e2e.rs index b1cbc9b..bbac49a 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -10329,6 +10329,10 @@ async fn github_unresolved_inline_line_falls_back_once_to_summary_only() { .env("POSTIL_API_BASE", server.uri()) .env("GITHUB_API_URL", server.uri()) .env("GITHUB_TOKEN", "gh-test-token") + .env( + "POSTIL_DETAILS_URL", + "https://postil.dev/orgs/acme/runs/run-1", + ) .args([ "review", "--publish", @@ -10356,6 +10360,12 @@ async fn github_unresolved_inline_line_falls_back_once_to_summary_only() { assert_eq!(review_bodies.len(), 2); assert!(review_bodies[0].get("comments").is_some()); assert!(review_bodies[1].get("comments").is_none()); + let fallback_summary = review_bodies[1]["body"].as_str().unwrap(); + assert!( + fallback_summary + .contains("1 finding could not be placed on the changed lines; see review details") + ); + assert!(!fallback_summary.contains("inline placement unavailable")); } #[tokio::test] From e86dd2c1017f0c9e48fcffa297bf3b6d5a2847bc Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 14 Aug 2026 06:00:35 +0000 Subject: [PATCH 2/7] fix: degrade rejected review comment placement --- src/diff.rs | 75 ++ src/forge/azure.rs | 1 + src/forge/bitbucket.rs | 1 + src/forge/github.rs | 1927 +++++++++++++++++++++++++++++++++++----- src/forge/gitlab.rs | 1 + src/forge/mod.rs | 26 + src/review.rs | 52 +- tests/e2e.rs | 89 +- 8 files changed, 1940 insertions(+), 232 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index 5860973..d5bcaed 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -969,6 +969,30 @@ impl DiffIndex { .is_some_and(|rs| rs.iter().any(|r| r.contains(&line))) } + /// Return the closest groundable new-side line in `path` to `requested`. + /// + /// Each hunk contributes only its clamped endpoint or `requested` itself, + /// so lookup remains proportional to the number of indexed ranges rather + /// than the number of lines they cover. Equal-distance candidates resolve + /// to the lower line. + pub fn nearest_new_side_line(&self, path: &str, requested: u32) -> Option { + if !self.complete { + return None; + } + self.ranges.get(path)?.iter().fold(None, |nearest, range| { + let candidate = requested.max(*range.start()).min(*range.end()); + match nearest { + Some(line) + if (line.abs_diff(requested), line) + <= (candidate.abs_diff(requested), candidate) => + { + Some(line) + } + _ => Some(candidate), + } + }) + } + /// True when both endpoints are on the new side of one diff hunk. Forge /// APIs reject multiline comments that cross hunk boundaries even when the /// individual line numbers both appear elsewhere in the file diff. @@ -6075,6 +6099,20 @@ deleted file mode 100644 -bye diff --git a/img.png b/img.png Binary files a/img.png and b/img.png differ +"; + + const MULTI_HUNK_SAMPLE: &str = "\ +diff --git a/src/multi.rs b/src/multi.rs +--- a/src/multi.rs ++++ b/src/multi.rs +@@ -10 +10,3 @@ + line ten ++added eleven ++added twelve +@@ -20 +20,3 @@ + line twenty ++added twenty-one ++added twenty-two "; #[test] @@ -6353,6 +6391,43 @@ Binary files a/img.png and b/img.png differ assert!(!idx.touches("src/lib.rs", 1, 9)); } + #[test] + fn nearest_new_side_line_returns_the_requested_line_in_range() { + let index = DiffIndex::build(&parse(MULTI_HUNK_SAMPLE)); + + assert_eq!(index.nearest_new_side_line("src/multi.rs", 11), Some(11)); + } + + #[test] + fn nearest_new_side_line_clamps_before_and_after_ranges() { + let index = DiffIndex::build(&parse(MULTI_HUNK_SAMPLE)); + + assert_eq!(index.nearest_new_side_line("src/multi.rs", 1), Some(10)); + assert_eq!(index.nearest_new_side_line("src/multi.rs", 99), Some(22)); + } + + #[test] + fn nearest_new_side_line_breaks_between_range_ties_toward_the_lower_line() { + let index = DiffIndex::build(&parse(MULTI_HUNK_SAMPLE)); + + assert_eq!(index.nearest_new_side_line("src/multi.rs", 16), Some(12)); + } + + #[test] + fn nearest_new_side_line_returns_none_for_a_missing_path() { + let index = DiffIndex::build(&parse(MULTI_HUNK_SAMPLE)); + + assert_eq!(index.nearest_new_side_line("src/missing.rs", 11), None); + } + + #[test] + fn nearest_new_side_line_returns_none_for_deleted_and_binary_files() { + let index = DiffIndex::build(&parse(SAMPLE)); + + assert_eq!(index.nearest_new_side_line("gone.txt", 1), None); + assert_eq!(index.nearest_new_side_line("img.png", 1), None); + } + #[test] fn annotated_render_numbers_new_lines() { let d = parse(SAMPLE); diff --git a/src/forge/azure.rs b/src/forge/azure.rs index a6b488b..eb973eb 100644 --- a/src/forge/azure.rs +++ b/src/forge/azure.rs @@ -465,6 +465,7 @@ impl Forge for Azure { &self, envelope: &Envelope, snapshot: &PrMeta, + _publication_diff: Option<&crate::diff::Diff>, ) -> Result { let findings = &envelope.findings; let receipt = untracked_review_publication_receipt("azure", envelope, &snapshot.head_sha); diff --git a/src/forge/bitbucket.rs b/src/forge/bitbucket.rs index fc9c2ae..37c13b5 100644 --- a/src/forge/bitbucket.rs +++ b/src/forge/bitbucket.rs @@ -510,6 +510,7 @@ impl Forge for Bitbucket { &self, envelope: &Envelope, snapshot: &PrMeta, + _publication_diff: Option<&crate::diff::Diff>, ) -> Result { let findings = &envelope.findings; let receipt = diff --git a/src/forge/github.rs b/src/forge/github.rs index 3b6a3a9..dc4b9af 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -15,12 +15,13 @@ use super::{ ReviewPublicationReceipt, ReviewPublicationSummary, SummaryContext, ThreadKind, check_summary, check_title, only_operational_findings, valid_details_url, }; -use crate::diff::{DiffSnapshot, DiffSpool, WorkspaceBudget}; +use crate::diff::{Diff, DiffIndex, DiffSnapshot, DiffSpool, WorkspaceBudget}; 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; +const FILE_LEVEL_COMMENT_MARKER: &str = ""; // Filtering caps visible findings before forge publication, so one completed // check update always fits GitHub's annotation request limit. Raising the @@ -116,11 +117,34 @@ impl GitHub { ) } + fn review_summary_with_unplaced_findings( + &self, + envelope: &Envelope, + receipt: &ReviewPublicationReceipt, + unplaced: &[&Finding], + ) -> String { + let mut summary = self.review_summary_for_receipt(envelope, receipt); + for finding in unplaced { + let (finding_id, _) = finding_receipt_id(finding); + summary.push_str(&format!( + "\n\nLocation: `{}:{}`\n\n{}", + super::safe_code_text(&finding.path), + finding.line, + append_marker( + &super::finding_comment_body(finding, true), + &finding_marker(&finding_id), + ), + )); + } + summary + } + async fn finalize_review_summary( &self, envelope: &Envelope, receipt: &ReviewPublicationReceipt, marker: &str, + snapshot: &PrMeta, ) -> Result<()> { let review_id = receipt .review_id @@ -131,13 +155,14 @@ impl GitHub { let summary = self.review_summary_for_receipt(envelope, receipt); let body = bounded_review_body(&summary, marker, self.details_url.as_deref()); let response = self - .send_write_retryable( + .send_snapshot_write_retryable( self.request( reqwest::Method::PUT, self.url(&format!("/pulls/{}/reviews/{review_id}", self.pr)), ) .json(&json!({ "body": body })), "review summary update", + snapshot, ) .await?; Self::check_ok(response, "review summary update").await?; @@ -149,9 +174,10 @@ impl GitHub { envelope: &Envelope, receipt: &ReviewPublicationReceipt, marker: &str, + snapshot: &PrMeta, ) { if self - .finalize_review_summary(envelope, receipt, marker) + .finalize_review_summary(envelope, receipt, marker, snapshot) .await .is_err() { @@ -259,7 +285,7 @@ impl GitHub { request: reqwest::RequestBuilder, what: &str, ) -> Result { - self.send_retryable_inner(request, what, false).await + self.send_retryable_inner(request, what, false, None).await } async fn send_write_retryable( @@ -267,7 +293,17 @@ impl GitHub { request: reqwest::RequestBuilder, what: &str, ) -> Result { - self.send_retryable_inner(request, what, true).await + self.send_retryable_inner(request, what, true, None).await + } + + async fn send_snapshot_write_retryable( + &self, + request: reqwest::RequestBuilder, + what: &str, + snapshot: &PrMeta, + ) -> Result { + self.send_retryable_inner(request, what, true, Some(snapshot)) + .await } async fn send_retryable_inner( @@ -275,11 +311,19 @@ impl GitHub { request: reqwest::RequestBuilder, what: &str, fence_each_attempt: bool, + expected_snapshot: Option<&PrMeta>, ) -> Result { const RETRIES: u32 = 2; const TOTAL_BUDGET: Duration = Duration::from_secs(55); let operation_started_at = std::time::Instant::now(); for retry in 0..=RETRIES { + if let Some(expected_snapshot) = expected_snapshot + && !Box::pin(self.snapshot_is_current(expected_snapshot)).await? + { + return Err(anyhow!( + "GitHub {what} skipped because the PR snapshot changed" + )); + } if fence_each_attempt { self.verify_repository_identity_before_write().await?; } @@ -487,11 +531,16 @@ impl GitHub { &self, body: &serde_json::Value, marker: &str, - head_sha: &str, + snapshot: &PrMeta, what: &str, ) -> Result { const RETRIES: u32 = 2; for retry in 0..=RETRIES { + if !self.snapshot_is_current(snapshot).await? { + return Err(anyhow!( + "GitHub review delivery skipped because the PR snapshot changed" + )); + } self.verify_repository_identity_before_write().await?; let response = self .request( @@ -509,7 +558,7 @@ impl GitHub { return Ok(ReviewDelivery::Response(response)); } Ok(response) => { - if let Some(review) = self.find_review(marker, head_sha).await? { + if let Some(review) = self.find_review(marker, &snapshot.head_sha).await? { return Ok(ReviewDelivery::Reconciled(review)); } if retry == RETRIES { @@ -517,7 +566,7 @@ impl GitHub { } } Err(error) => { - if let Some(review) = self.find_review(marker, head_sha).await? { + if let Some(review) = self.find_review(marker, &snapshot.head_sha).await? { return Ok(ReviewDelivery::Reconciled(review)); } if retry == RETRIES { @@ -644,10 +693,13 @@ impl GitHub { /// 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 { + async fn published_finding_comments( + &self, + head_sha: &str, + ) -> std::collections::HashMap { const PAGE_SIZE: usize = 100; const MAX_PAGES: usize = 20; - let mut markers = std::collections::HashSet::new(); + let mut comments_by_marker = std::collections::HashMap::new(); for page in 1..=MAX_PAGES { let request = self.request( reqwest::Method::GET, @@ -657,27 +709,134 @@ impl GitHub { )), ); let Ok(response) = self.send_retryable(request, "inline comment dedup").await else { - return markers; + return comments_by_marker; }; let Ok(response) = Self::check_ok(response, "inline comment dedup").await else { - return markers; + return comments_by_marker; }; let Ok(comments): Result> = super::bounded_response_json(response, "GitHub inline comment dedup").await else { - return markers; + return comments_by_marker; }; let page_len = comments.len(); for comment in comments { - if let Some(marker) = finding_marker_in(&comment.body) { - markers.insert(marker); + if comment.commit_id.as_deref() == Some(head_sha) + && let Some(marker) = finding_marker_in(&comment.body) + { + comments_by_marker.insert(marker, comment); } } if page_len < PAGE_SIZE { break; } } - markers + comments_by_marker + } + + async fn find_review_comment( + &self, + marker: &str, + head_sha: &str, + ) -> Result> { + const PAGE_SIZE: usize = 100; + const MAX_PAGES: usize = 20; + for page in 1..=MAX_PAGES { + let response = self + .send_retryable( + self.request( + reqwest::Method::GET, + self.url(&format!( + "/pulls/{}/comments?per_page={PAGE_SIZE}&page={page}", + self.pr + )), + ), + "file-level review comment reconciliation", + ) + .await?; + let comments: Vec = super::bounded_response_json( + Self::check_ok(response, "file-level review comment reconciliation").await?, + "GitHub file-level review comment reconciliation", + ) + .await?; + let page_len = comments.len(); + if let Some(comment) = comments.into_iter().find(|comment| { + comment.body.contains(marker) && comment.commit_id.as_deref() == Some(head_sha) + }) { + return Ok(Some(comment)); + } + if page_len < PAGE_SIZE { + return Ok(None); + } + } + Err(anyhow!( + "GitHub file-level review comment reconciliation exceeded {MAX_PAGES} pages" + )) + } + + async fn post_file_comment_reconciled( + &self, + body: &serde_json::Value, + marker: &str, + snapshot: &PrMeta, + ) -> Result { + const RETRIES: u32 = 2; + for retry in 0..=RETRIES { + if !self.snapshot_is_current(snapshot).await? { + return Err(anyhow!( + "GitHub file-level review comment delivery skipped because the PR snapshot changed" + )); + } + self.verify_repository_identity_before_write().await?; + let response = self + .request( + reqwest::Method::POST, + self.url(&format!("/pulls/{}/comments", self.pr)), + ) + .json(body) + .send() + .await; + match response { + Ok(response) if response.status().is_success() => { + let comment: PublishedReviewComment = + super::bounded_response_json(response, "GitHub file-level review comment") + .await?; + ensure!( + comment.commit_id.as_deref() == Some(snapshot.head_sha.as_str()), + "GitHub file-level review comment response did not identify the reviewed head" + ); + return Ok(comment); + } + Ok(response) + if github_retryable_response(response.status(), response.headers()) => + { + if let Some(comment) = + self.find_review_comment(marker, &snapshot.head_sha).await? + { + return Ok(comment); + } + if retry == RETRIES { + Self::check_ok(response, "file-level review comment").await?; + } + } + Ok(response) => { + Self::check_ok(response, "file-level review comment").await?; + } + Err(error) => { + if let Some(comment) = + self.find_review_comment(marker, &snapshot.head_sha).await? + { + return Ok(comment); + } + if retry == RETRIES { + return Err(error) + .context("posting file-level review comment after reconciliation"); + } + } + } + tokio::time::sleep(github_transport_retry_delay(retry)).await; + } + unreachable!("bounded GitHub file-level comment loop always returns") } async fn post_comment_reconciled(&self, number: u64, body: &str, marker: &str) -> Result<()> { @@ -1545,6 +1704,8 @@ struct PublishedReviewComment { id: u64, #[serde(default)] body: String, + #[serde(default)] + commit_id: Option, } #[derive(Deserialize)] @@ -1700,10 +1861,11 @@ impl Forge for GitHub { &self, envelope: &Envelope, snapshot: &PrMeta, + publication_diff: Option<&Diff>, ) -> Result { let findings = &envelope.findings; let head_sha = snapshot.head_sha.as_str(); - let planned_receipt = self.plan_review_publication(envelope, snapshot); + let mut planned_receipt = self.plan_review_publication(envelope, snapshot); if only_operational_findings(findings) { return Ok(planned_receipt); } @@ -1724,8 +1886,19 @@ impl Forge for GitHub { // 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 + let published = self.published_finding_comments(head_sha).await; + let mut observed_comment_count = 0usize; + for publication in &mut planned_receipt.findings { + let marker = finding_marker(&publication.finding_id); + let Some(comment) = published.get(&marker) else { + continue; + }; + publication.initial_outcome = FindingPublicationOutcome::Carried; + publication.inline_rejected = false; + publication.comment_id = Some(comment.id.to_string()); + observed_comment_count += 1; + } + let publishable_findings: Vec<_> = findings .iter() // Carried findings already have comments from the previous review. .filter(|f| !filter::is_carried(f)) @@ -1735,34 +1908,33 @@ impl Forge for GitHub { .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!({ - "path": f.path, - "line": f.line, - "side": "RIGHT", - "body": append_marker( - &super::finding_comment_body(f, true), - &finding_marker(&finding_id), - ), - }); - if let Some(end) = f.end_line - && end > f.line - { - c["start_line"] = json!(f.line); - c["line"] = json!(end); - c["start_side"] = json!("RIGHT"); - } - c + !published.contains_key(&finding_marker(&finding_id)) }) .collect(); + let comments: Vec<_> = publishable_findings + .iter() + .map(|finding| initial_review_comment(finding)) + .collect(); let summary = self.review_summary_for_receipt(envelope, &planned_receipt); if comments.is_empty() && summary.is_empty() { return Ok(planned_receipt); } let marker = review_marker(&planned_receipt.receipt_id); + let has_new_summary_finding = findings + .iter() + .any(|finding| !filter::is_carried(finding) && super::is_synthetic_path(&finding.path)); + let all_comments_already_published = observed_comment_count > 0 + && publishable_findings.is_empty() + && !has_new_summary_finding; + if publishable_findings.is_empty() + && all_comments_already_published + && let Some(review) = self.find_review(&marker, head_sha).await? + { + planned_receipt.review_id = review.id.map(|id| id.to_string()); + self.finalize_review_summary_if_possible(envelope, &planned_receipt, &marker, snapshot) + .await; + return Ok(planned_receipt); + } let marked_summary = bounded_review_body(&summary, &marker, self.details_url.as_deref()); let has_planned_inline = planned_receipt .findings @@ -1775,76 +1947,294 @@ impl Forge for GitHub { "comments": comments, }); let delivery = self - .send_review_reconciled(&body, &marker, head_sha, "review post") + .send_review_reconciled(&body, &marker, snapshot, "review post") .await?; - let resp = match delivery { + let mut recovered_partial_review = None; + let response = match delivery { ReviewDelivery::Reconciled(review) => { let receipt = self - .materialize_review_receipt(planned_receipt, review) + .materialize_review_receipt(planned_receipt.clone(), review.clone()) + .await?; + if receipt_covers_findings(&receipt, &publishable_findings) { + if has_planned_inline { + self.finalize_review_summary_if_possible( + envelope, &receipt, &marker, snapshot, + ) + .await; + } + return Ok(receipt); + } + recovered_partial_review = Some(review); + None + } + ReviewDelivery::Response(response) if response.status().is_success() => { + let review: PublishedReview = + super::bounded_response_json(response, "GitHub published review").await?; + let receipt = self + .materialize_review_receipt(planned_receipt.clone(), review.clone()) .await?; - if has_planned_inline { - self.finalize_review_summary_if_possible(envelope, &receipt, &marker) + if receipt_covers_findings(&receipt, &publishable_findings) { + if has_planned_inline { + self.finalize_review_summary_if_possible( + envelope, &receipt, &marker, snapshot, + ) .await; + } + return Ok(receipt); } - return Ok(receipt); + recovered_partial_review = Some(review); + None } - ReviewDelivery::Response(response) => response, + ReviewDelivery::Response(response) => Some(response), }; - if resp.status().is_success() { - let review: PublishedReview = - super::bounded_response_json(resp, "GitHub published review").await?; - let receipt = self - .materialize_review_receipt(planned_receipt, review) - .await?; - if has_planned_inline { - self.finalize_review_summary_if_possible(envelope, &receipt, &marker) - .await; + if let Some(resp) = response { + let status = resp.status(); + let request_id = + github_request_id(resp.headers()).unwrap_or_else(|| "none".to_string()); + if status != reqwest::StatusCode::UNPROCESSABLE_ENTITY { + return Err(anyhow!( + "GitHub review post failed: {status} (request id {request_id})" + )); + } + + let rejection_body = + super::bounded_response_text(resp, "GitHub rejected review").await?; + if !github_review_rejected_line(&rejection_body) { + return Err(anyhow!( + "GitHub review post failed validation: {status} (request id {request_id})" + )); } - return Ok(receipt); + + eprintln!( + "postil: github operation=review-post status=422 category=unresolved-line request_id={} recovery=placement-ladder", + request_id, + ); + } else { + eprintln!( + "postil: github operation=review-reconciliation status=partial recovery=placement-ladder" + ); } - let status = resp.status(); - let request_id = github_request_id(resp.headers()).unwrap_or_else(|| "none".to_string()); - if status != reqwest::StatusCode::UNPROCESSABLE_ENTITY { - return Err(anyhow!( - "GitHub review post failed: {status} (request id {request_id})" - )); + + let owned_publication_diff = if publication_diff.is_none() { + let snapshot = self + .fetch_diff(snapshot) + .await + .context("fetching complete diff for GitHub placement fallback")?; + Some(crate::diff::parse(snapshot.as_str())) + } else { + None + }; + let publication_diff = publication_diff + .or(owned_publication_diff.as_ref()) + .context("GitHub placement fallback is missing the complete pull-request diff")?; + let placement_index = DiffIndex::build(publication_diff); + let mut line_findings = Vec::new(); + let mut file_findings = Vec::new(); + let mut summary_findings = Vec::new(); + for finding in &publishable_findings { + let Some(path) = publication_file_path(publication_diff, &finding.path) else { + summary_findings.push(*finding); + continue; + }; + if let Some(line) = placement_index.nearest_new_side_line(path, finding.line) { + line_findings.push((*finding, path, line)); + } else { + file_findings.push((*finding, path)); + } } - eprintln!( - "postil: github operation=review-post status=422 category=unresolved-line request_id={} recovery=summary-only", - request_id, + let mut fallback_receipt = planned_receipt; + for (finding, _) in &file_findings { + set_publication_outcome( + &mut fallback_receipt, + finding, + FindingPublicationOutcome::FileComment, + false, + )?; + } + for finding in &summary_findings { + set_publication_outcome( + &mut fallback_receipt, + finding, + FindingPublicationOutcome::SummaryOnly, + true, + )?; + } + let fallback_summary = self.review_summary_with_unplaced_findings( + envelope, + &fallback_receipt, + &summary_findings, ); - let rejected_receipt = rejected_inline_receipt(planned_receipt); - let fallback_summary = self.review_summary_for_receipt(envelope, &rejected_receipt); - let summary_only = json!({ - "commit_id": head_sha, - "event": "COMMENT", - "body": bounded_review_body( + let fallback_summary = if summary_findings.is_empty() { + bounded_review_body( if fallback_summary.is_empty() { - "Postil completed the review, but GitHub could not attach its inline comments." + "Postil completed the review." } else { &fallback_summary }, &marker, self.details_url.as_deref(), - ), + ) + } else { + required_review_body(&fallback_summary, &marker)? + }; + let mut fallback_body = json!({ + "commit_id": head_sha, + "event": "COMMENT", + "body": fallback_summary, }); - let fallback = self - .send_review_reconciled(&summary_only, &marker, head_sha, "summary-only review post") - .await?; - match fallback { - ReviewDelivery::Reconciled(review) => { - self.materialize_review_receipt(rejected_receipt, review) - .await + if !line_findings.is_empty() { + fallback_body["comments"] = json!( + line_findings + .iter() + .map(|(finding, path, line)| fallback_line_comment(finding, path, *line)) + .collect::>() + ); + } + let fallback = if let Some(review) = recovered_partial_review { + ReviewDelivery::Reconciled(review) + } else { + match self.find_review(&marker, head_sha).await? { + Some(review) => ReviewDelivery::Reconciled(review), + None => { + self.send_review_reconciled( + &fallback_body, + &marker, + snapshot, + "placement fallback review post", + ) + .await? + } } - ReviewDelivery::Response(response) => { - let response = Self::check_ok(response, "summary-only review post").await?; + }; + let mut receipt = match fallback { + ReviewDelivery::Reconciled(review) => Some( + self.materialize_review_receipt(fallback_receipt.clone(), review) + .await?, + ), + ReviewDelivery::Response(response) if response.status().is_success() => { let review: PublishedReview = - super::bounded_response_json(response, "GitHub summary-only review").await?; - self.materialize_review_receipt(rejected_receipt, review) - .await + super::bounded_response_json(response, "GitHub placement fallback review") + .await?; + Some( + self.materialize_review_receipt(fallback_receipt.clone(), review) + .await?, + ) + } + ReviewDelivery::Response(response) => { + let status = response.status(); + let request_id = + github_request_id(response.headers()).unwrap_or_else(|| "none".to_string()); + let body = + super::bounded_response_text(response, "GitHub rejected fallback review") + .await?; + if status == reqwest::StatusCode::UNPROCESSABLE_ENTITY + && !line_findings.is_empty() + && github_review_rejected_line(&body) + { + eprintln!( + "postil: github operation=placement-fallback status=422 category=unresolved-line request_id={} recovery=file-comments", + request_id, + ); + None + } else { + return Err(anyhow!( + "GitHub placement fallback review failed: {status} (request id {request_id})" + )); + } + } + }; + + if let Some(materialized) = receipt.as_mut() { + for (finding, path, _) in line_findings.drain(..) { + if receipt_has_finding_comment(materialized, finding) { + continue; + } + set_publication_outcome( + materialized, + finding, + FindingPublicationOutcome::FileComment, + false, + )?; + file_findings.push((finding, path)); + } + } + + if receipt.is_none() { + for (finding, path, _) in line_findings.drain(..) { + set_publication_outcome( + &mut fallback_receipt, + finding, + FindingPublicationOutcome::FileComment, + false, + )?; + file_findings.push((finding, path)); } + let fallback_summary = self.review_summary_with_unplaced_findings( + envelope, + &fallback_receipt, + &summary_findings, + ); + let fallback_summary = if summary_findings.is_empty() { + bounded_review_body(&fallback_summary, &marker, self.details_url.as_deref()) + } else { + required_review_body(&fallback_summary, &marker)? + }; + let summary_body = json!({ + "commit_id": head_sha, + "event": "COMMENT", + "body": fallback_summary, + }); + let delivery = self + .send_review_reconciled( + &summary_body, + &marker, + snapshot, + "file-comment fallback review post", + ) + .await?; + let materialized = match delivery { + ReviewDelivery::Reconciled(review) => { + self.materialize_review_receipt(fallback_receipt.clone(), review) + .await? + } + ReviewDelivery::Response(response) => { + let response = + Self::check_ok(response, "file-comment fallback review post").await?; + let review: PublishedReview = super::bounded_response_json( + response, + "GitHub file-comment fallback review", + ) + .await?; + self.materialize_review_receipt(fallback_receipt.clone(), review) + .await? + } + }; + receipt = Some(materialized); + } + + let mut receipt = receipt.context("GitHub placement fallback omitted its receipt")?; + super::write_review_publication_receipt_from_env(&durable_partial_receipt(&receipt))?; + for (finding, path) in file_findings { + let (finding_id, _) = finding_receipt_id(finding); + let finding_marker = finding_marker(&finding_id); + let payload = file_level_comment(finding, path, head_sha); + let comment = self + .post_file_comment_reconciled(&payload, &finding_marker, snapshot) + .await?; + let publication = receipt + .findings + .iter_mut() + .find(|publication| publication.finding_id == finding_id) + .context("GitHub file-level comment omitted its publication receipt")?; + publication.initial_outcome = FindingPublicationOutcome::FileComment; + publication.inline_rejected = false; + publication.comment_id = Some(comment.id.to_string()); + super::write_review_publication_receipt_from_env(&durable_partial_receipt(&receipt))?; } + self.finalize_review_summary_if_possible(envelope, &receipt, &marker, snapshot) + .await; + Ok(receipt) } async fn start_checks(&self, head_sha: &str) -> Result<(String, String)> { @@ -2150,51 +2540,189 @@ fn planned_review_receipt(envelope: &Envelope, head_sha: &str) -> ReviewPublicat } } -fn publication_summary(receipt: &ReviewPublicationReceipt) -> ReviewPublicationSummary { - let mut summary = ReviewPublicationSummary::default(); - for finding in &receipt.findings { - match finding.initial_outcome { - FindingPublicationOutcome::Inline if finding.comment_id.is_some() => { - 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 - | FindingPublicationOutcome::Suppressed - | FindingPublicationOutcome::Unknown => {} - } - if finding.inline_rejected { - summary.rejected_inline += 1; - } +fn initial_review_comment(finding: &Finding) -> serde_json::Value { + let (finding_id, _) = finding_receipt_id(finding); + let mut comment = json!({ + "path": finding.path, + "line": finding.line, + "side": "RIGHT", + "body": append_marker( + &super::finding_comment_body(finding, true), + &finding_marker(&finding_id), + ), + }); + if let Some(end) = finding.end_line + && end > finding.line + { + comment["start_line"] = json!(finding.line); + comment["line"] = json!(end); + comment["start_side"] = json!("RIGHT"); } - summary + comment } -fn rejected_inline_receipt(mut receipt: ReviewPublicationReceipt) -> ReviewPublicationReceipt { - for finding in &mut receipt.findings { - if finding.initial_outcome == FindingPublicationOutcome::Inline { - finding.initial_outcome = FindingPublicationOutcome::SummaryOnly; - finding.inline_rejected = true; - finding.comment_id = None; - } +fn fallback_line_comment(finding: &Finding, path: &str, line: u32) -> serde_json::Value { + let (finding_id, _) = finding_receipt_id(finding); + let mut body = String::new(); + if path != finding.path { + body.push_str(&format!( + "This finding refers to `{}:{}`, outside the changed lines in this file.\n\n", + super::safe_code_text(&finding.path), + finding.line, + )); + } else if line != finding.line { + body.push_str(&format!( + "This finding refers to line {}, outside the changed lines.\n\n", + finding.line, + )); } - receipt + body.push_str(&super::finding_comment_body(finding, true)); + json!({ + "path": path, + "line": line, + "side": "RIGHT", + "body": append_marker(&body, &finding_marker(&finding_id)), + }) } -fn finding_marker(finding_id: &str) -> String { - let hash = Sha256::digest(finding_id.as_bytes()); - format!( - "", - hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] - ) +fn file_level_comment(finding: &Finding, path: &str, head_sha: &str) -> serde_json::Value { + let (finding_id, _) = finding_receipt_id(finding); + let body = format!( + "Original location: `{}:{}`\n\n{}", + super::safe_code_text(&finding.path), + finding.line, + super::finding_comment_body(finding, true), + ); + let body = append_marker(&body, FILE_LEVEL_COMMENT_MARKER); + json!({ + "body": append_marker(&body, &finding_marker(&finding_id)), + "commit_id": head_sha, + "path": path, + "subject_type": "file", + }) } -fn review_marker(receipt_id: &str) -> String { - let mut digest = Sha256::new(); - digest.update(receipt_id.as_bytes()); - let hash = digest.finalize(); +fn publication_file_path<'a>(diff: &'a Diff, finding_path: &str) -> Option<&'a str> { + diff.files + .iter() + .find(|file| file.path == finding_path) + .map(|file| file.path.as_str()) + .or_else(|| { + diff.files + .iter() + .find(|file| !file.deleted && file.old_path == finding_path) + .map(|file| file.path.as_str()) + }) +} + +fn set_publication_outcome( + receipt: &mut ReviewPublicationReceipt, + finding: &Finding, + outcome: FindingPublicationOutcome, + inline_rejected: bool, +) -> Result<()> { + let (finding_id, _) = finding_receipt_id(finding); + let publication = receipt + .findings + .iter_mut() + .find(|publication| publication.finding_id == finding_id) + .context("GitHub placement fallback omitted a finding publication receipt")?; + publication.initial_outcome = outcome; + publication.inline_rejected = inline_rejected; + publication.comment_id = None; + Ok(()) +} + +fn receipt_covers_findings(receipt: &ReviewPublicationReceipt, findings: &[&Finding]) -> bool { + findings + .iter() + .all(|finding| receipt_has_finding_comment(receipt, finding)) +} + +fn receipt_has_finding_comment(receipt: &ReviewPublicationReceipt, finding: &Finding) -> bool { + let (finding_id, _) = finding_receipt_id(finding); + receipt + .findings + .iter() + .find(|publication| publication.finding_id == finding_id) + .is_some_and(|publication| publication.comment_id.is_some()) +} + +fn github_review_rejected_line(body: &str) -> bool { + let normalized = body.to_ascii_lowercase(); + if normalized.contains("line could not be resolved") + || normalized.contains("line must be part of the diff") + { + return true; + } + serde_json::from_str::(body) + .ok() + .and_then(|value| { + value + .get("errors") + .and_then(serde_json::Value::as_array) + .cloned() + }) + .is_some_and(|errors| { + errors.iter().any(|error| { + matches!( + error.get("field").and_then(serde_json::Value::as_str), + Some("line" | "start_line") + ) && error.get("code").and_then(serde_json::Value::as_str) == Some("invalid") + }) + }) +} + +fn publication_summary(receipt: &ReviewPublicationReceipt) -> ReviewPublicationSummary { + let mut summary = ReviewPublicationSummary::default(); + for finding in &receipt.findings { + match finding.initial_outcome { + FindingPublicationOutcome::Inline if finding.comment_id.is_some() => { + summary.active_inline += 1; + } + FindingPublicationOutcome::Inline => {} + FindingPublicationOutcome::FileComment if finding.comment_id.is_some() => { + summary.file_comments += 1; + } + FindingPublicationOutcome::FileComment => {} + FindingPublicationOutcome::CheckAnnotation => summary.summary_only += 1, + FindingPublicationOutcome::SummaryOnly => summary.summary_only += 1, + FindingPublicationOutcome::Carried => summary.carried += 1, + FindingPublicationOutcome::Resolved + | FindingPublicationOutcome::Suppressed + | FindingPublicationOutcome::Unknown => {} + } + if finding.inline_rejected { + summary.rejected_inline += 1; + } + } + summary +} + +fn durable_partial_receipt(receipt: &ReviewPublicationReceipt) -> ReviewPublicationReceipt { + let mut durable = receipt.clone(); + for finding in &mut durable.findings { + if finding.initial_outcome == FindingPublicationOutcome::FileComment + && finding.comment_id.is_none() + { + finding.initial_outcome = FindingPublicationOutcome::Unknown; + } + } + durable +} + +fn finding_marker(finding_id: &str) -> String { + let hash = Sha256::digest(finding_id.as_bytes()); + format!( + "", + hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] + ) +} + +fn review_marker(receipt_id: &str) -> String { + let mut digest = Sha256::new(); + digest.update(receipt_id.as_bytes()); + let hash = digest.finalize(); format!( "", hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] @@ -2238,6 +2766,15 @@ fn bounded_review_body(body: &str, marker: &str, details_url: Option<&str>) -> S append_marker(OVERSIZED_REVIEW_MESSAGE, marker) } +fn required_review_body(body: &str, marker: &str) -> Result { + let marked = append_marker(body, marker); + ensure!( + marked.len() <= MAX_REVIEW_BODY_BYTES, + "GitHub review summary cannot represent every unplaced finding within its size limit" + ); + Ok(marked) +} + fn comment_marker(number: u64, body: &str) -> String { let mut digest = Sha256::new(); digest.update(number.to_be_bytes()); @@ -2340,6 +2877,48 @@ mod tests { } } + fn placement_diff() -> crate::diff::Diff { + crate::diff::parse( + "diff --git a/src/lib.rs b/src/lib.rs\n\ + --- a/src/lib.rs\n\ + +++ b/src/lib.rs\n\ + @@ -10 +10,3 @@\n\ + context\n\ + +added one\n\ + +added two\n", + ) + } + + fn file_only_placement_diff() -> crate::diff::Diff { + crate::diff::parse( + "diff --git a/src/lib.rs b/src/lib.rs\n\ + index 9daeafb..0f15a6e 100644\n\ + Binary files a/src/lib.rs and b/src/lib.rs differ\n", + ) + } + + fn unrelated_placement_diff() -> crate::diff::Diff { + crate::diff::parse( + "diff --git a/src/other.rs b/src/other.rs\n\ + --- a/src/other.rs\n\ + +++ b/src/other.rs\n\ + @@ -1 +1 @@\n\ + -old();\n\ + +new();\n", + ) + } + + fn two_file_placement_diff() -> crate::diff::Diff { + crate::diff::parse( + "diff --git a/src/first.bin b/src/first.bin\n\ + index 9daeafb..0f15a6e 100644\n\ + Binary files a/src/first.bin and b/src/first.bin differ\n\ + diff --git a/src/second.bin b/src/second.bin\n\ + index 9daeafb..0f15a6e 100644\n\ + Binary files a/src/second.bin and b/src/second.bin differ\n", + ) + } + fn repository_search_terms() -> Vec { use crate::envelope::{RepositoryClaim, RepositoryClaimKind}; crate::repository_search::search_terms(std::iter::once(&RepositoryClaim { @@ -2485,6 +3064,7 @@ mod tests { #[tokio::test] async fn github_review_reconciles_before_retrying_uncertain_post() { let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; Mock::given(method("POST")) .and(path("/repos/owner/repo/pulls/1/reviews")) .respond_with(ResponseTemplate::new(500)) @@ -2496,7 +3076,7 @@ mod tests { .respond_with( ResponseTemplate::new(200).set_body_json(serde_json::json!([{ "body": "summary\n\n", - "commit_id": "abcdef12" + "commit_id": "aaaaaaaaaaaa" }])), ) .expect(1) @@ -2505,15 +3085,16 @@ mod tests { let github = test_github(&server); let body = serde_json::json!({ "body": "summary\n\n", - "commit_id": "abcdef12", + "commit_id": "aaaaaaaaaaaa", "event": "COMMENT" }); + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); let response = github .send_review_reconciled( &body, "", - "abcdef12", + &snapshot, "review post", ) .await @@ -2615,7 +3196,6 @@ mod tests { .expect(0) .mount(&server) .await; - let github = test_github(&server); let content = github .fetch_repository_file_if_present("head123", "config/review.toml") @@ -3061,6 +3641,14 @@ mod tests { .await; } + async fn mount_no_existing_review(server: &MockServer) { + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(server) + .await; + } + #[tokio::test] async fn github_receipt_separates_inline_summary_carried_resolved_and_suppressed() { let server = MockServer::start().await; @@ -3117,6 +3705,7 @@ mod tests { .post_review( &envelope, &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + None, ) .await .unwrap(); @@ -3223,6 +3812,7 @@ mod tests { .post_review( &envelope, &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + None, ) .await .unwrap(); @@ -3256,56 +3846,687 @@ mod tests { ); Mock::given(method("POST")) .and(path("/repos/owner/repo/pulls/1/reviews")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "id": 80, - "commit_id": "aaaaaaaaaaaa", - "comments": [{ - "id": 503, - "body": format!("finding\n\n{}", super::finding_marker("inline-1")) - }] + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": 80, + "commit_id": "aaaaaaaaaaaa", + "comments": [{ + "id": 503, + "body": format!("finding\n\n{}", super::finding_marker("inline-1")) + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/repos/owner/repo/pulls/1/reviews/80")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + let mut github = test_github(&server); + github.details_url = Some(format!("https://postil.dev/{}", "x".repeat(60_000))); + + let receipt = github + .post_review( + &envelope, + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + None, + ) + .await + .unwrap(); + + assert_eq!(receipt.findings[0].comment_id.as_deref(), Some("503")); + let requests = server.received_requests().await.unwrap(); + let initial: serde_json::Value = serde_json::from_slice( + &requests + .iter() + .find(|request| request.method == reqwest::Method::POST) + .unwrap() + .body, + ) + .unwrap(); + assert_eq!(initial["comments"].as_array().unwrap().len(), 1); + let body = initial["body"].as_str().unwrap(); + assert!(body.contains(super::OVERSIZED_REVIEW_MESSAGE)); + assert!(!body.contains("Review details")); + assert!(body.contains(" parse supported lockfiles --> parse + index --> bounded evidenc resources, values, versions, paths, and identifiers. One receipt binds those queries to the immutable reviewed head and reports `complete`, `unavailable`, or `exhausted`. Only a complete receipt with no positive counterexample supports a universal claim. - Incomplete repository evidence cannot confirm it, so the original finding remains open. + Incomplete repository evidence cannot confirm a fresh claim, so the claim is suppressed; + a prior-ledger claim remains open until exact-head adjudication resolves it. Every surviving generated candidate, plus applicable baseline candidates during a full rereview, enters one bounded adjudication operation before scoring and publication. The operation admits the complete candidate set or fails closed before provider contact. Its direct-source receipt hashes and scans the complete diff, records deterministic citation occurrence counts, and carries only bounded evidence windows to the model. Adjudication validates exact candidate identities, result completeness, evidence, publication text, - and duplicate primaries. Later and cross-file evidence can refute stale claims, while - unresolved claims remain open. Semantic duplicates collapse across files and kinds - only when one established defect remains; distinct defects sharing a line remain separate. + and duplicate primaries. Later and cross-file evidence can refute stale claims. Fresh + unresolved repository claims are suppressed, ordinary grounded unresolved findings and + prior-ledger claims remain open, and provider or contract failure preserves every candidate. + Semantic duplicates collapse across files and kinds only when one established defect remains; + distinct defects sharing a line remain separate. - `forge/`: trait + GitHub, GitLab, Bitbucket Cloud, and Azure DevOps implementations, with self-managed base URLs where the same API contract applies. Paginated forge metadata has aggregate byte and changed-file bounds. Source responses stream to @@ -266,7 +269,9 @@ additions (violations are `kind: contentPolicy`). Content policy is on by defaul requires the named target and compared value in one searched evidence unit. Query, request, object, tree-depth, byte, deadline, and detailed-match bounds are explicit in the receipt outcome. Public findings state the repository construct and correction without describing - evidence retrieval boundaries or delegating evidence collection to the author. + evidence retrieval boundaries or delegating evidence collection to the author. Fresh + unresolved repository claims are suppressed; prior-ledger claims remain open until an + exact-head adjudication explicitly resolves them. 15. Finding adjudication performs exactly one logical provider operation over every admitted candidate. Candidate identities bind the exact snapshot and semantic finding fields. Adjudication input, output, attempts, deadline, and projected cost are bounded, and results diff --git a/src/adjudication.rs b/src/adjudication.rs index b51d537..1724ae8 100644 --- a/src/adjudication.rs +++ b/src/adjudication.rs @@ -115,6 +115,7 @@ pub(crate) struct AdjudicationCandidate<'a> { pub(crate) struct AdjudicationApplication { pub kept: Vec, pub kept_indices: Vec, + pub unresolved_indices: Vec, pub resolved_indices: Vec, pub suppressed: Vec, } @@ -1288,6 +1289,7 @@ pub(crate) fn apply_results( .collect::>(); let mut kept = Vec::new(); let mut kept_indices = Vec::new(); + let mut unresolved_indices = Vec::new(); let mut resolved_indices = Vec::new(); let mut suppressed = Vec::new(); for (index, (mut finding, id)) in findings.into_iter().zip(candidate_ids).enumerate() { @@ -1308,6 +1310,7 @@ pub(crate) fn apply_results( } (AdjudicationProvenance::Model, AdjudicationDisposition::PreserveUnresolved) => { kept_indices.push(index); + unresolved_indices.push(index); kept.push(finding); } ( @@ -1315,6 +1318,7 @@ pub(crate) fn apply_results( AdjudicationDisposition::PreserveUnresolved, ) => { kept_indices.push(index); + unresolved_indices.push(index); kept.push(finding); } (AdjudicationProvenance::Model, AdjudicationDisposition::SuppressRefuted) => { @@ -1337,6 +1341,7 @@ pub(crate) fn apply_results( Ok(AdjudicationApplication { kept, kept_indices, + unresolved_indices, resolved_indices, suppressed, }) @@ -2658,6 +2663,7 @@ mod tests { assert_eq!(kept.evidence, original.evidence); } assert_eq!(applied.kept_indices, vec![0, 1]); + assert_eq!(applied.unresolved_indices, vec![0, 1]); assert!(applied.resolved_indices.is_empty()); assert!(applied.suppressed.is_empty()); } @@ -2725,6 +2731,7 @@ mod tests { assert_eq!(applied.kept[1].body, unresolved.body); assert_eq!(applied.kept[1].evidence, unresolved.evidence); assert_eq!(applied.kept_indices, vec![0, 2]); + assert_eq!(applied.unresolved_indices, vec![2]); assert_eq!(applied.resolved_indices, vec![1]); assert_eq!( applied.suppressed[0].reason, diff --git a/src/review.rs b/src/review.rs index 33dd414..e5c9c3d 100644 --- a/src/review.rs +++ b/src/review.rs @@ -2352,6 +2352,10 @@ async fn review_diff_at( preserve_unadjudicated_findings(all_adjudication_candidates) } }; + suppress_fresh_unresolved_repository_claims( + &mut application, + fresh_candidate_count, + ); for (candidate_index, finding) in application .kept_indices .iter() @@ -3036,11 +3040,43 @@ fn preserve_unadjudicated_findings( crate::adjudication::AdjudicationApplication { kept_indices: (0..findings.len()).collect(), kept: findings, + unresolved_indices: Vec::new(), resolved_indices: Vec::new(), suppressed: Vec::new(), } } +fn suppress_fresh_unresolved_repository_claims( + application: &mut crate::adjudication::AdjudicationApplication, + fresh_candidate_count: usize, +) { + let mut kept = Vec::with_capacity(application.kept.len()); + let mut kept_indices = Vec::with_capacity(application.kept_indices.len()); + for (candidate_index, finding) in application + .kept_indices + .drain(..) + .zip(application.kept.drain(..)) + { + let unsupported = candidate_index < fresh_candidate_count + && application.unresolved_indices.contains(&candidate_index) + && finding.repository_claim.is_some(); + if unsupported { + application.suppressed.push(SuppressedFinding { + finding, + reason: SuppressionReason::RepositoryClaimUnsupported, + }); + } else { + kept_indices.push(candidate_index); + kept.push(finding); + } + } + application.kept = kept; + application.kept_indices = kept_indices; + application + .unresolved_indices + .retain(|index| application.kept_indices.contains(index)); +} + fn error_envelope( cfg: &Config, err: &anyhow::Error, @@ -3779,6 +3815,57 @@ mod tests { ])); } + #[test] + fn only_fresh_unresolved_repository_claims_are_suppressed() { + let claim = RepositoryClaim { + kind: RepositoryClaimKind::Absence, + resources: vec!["widget".into()], + values: vec![], + versions: vec![], + paths: vec![], + identifiers: vec![], + }; + let mut fresh_repository_claim = finding("src/fresh.rs", 1, "widget is absent"); + fresh_repository_claim.repository_claim = Some(claim.clone()); + let fresh_local_finding = finding("src/local.rs", 2, "authorization is bypassed"); + let mut baseline_repository_claim = finding("src/baseline.rs", 3, "widget is absent"); + baseline_repository_claim.repository_claim = Some(claim); + let mut application = crate::adjudication::AdjudicationApplication { + kept: vec![ + fresh_repository_claim.clone(), + fresh_local_finding.clone(), + baseline_repository_claim.clone(), + ], + kept_indices: vec![0, 1, 2], + unresolved_indices: vec![0, 1, 2], + resolved_indices: vec![], + suppressed: vec![], + }; + + suppress_fresh_unresolved_repository_claims(&mut application, 2); + + assert_eq!(application.kept_indices, vec![1, 2]); + assert_eq!(application.unresolved_indices, vec![1, 2]); + assert_eq!(application.kept.len(), 2); + assert!(same_visible_finding( + &application.kept[0], + &fresh_local_finding + )); + assert!(same_visible_finding( + &application.kept[1], + &baseline_repository_claim + )); + assert_eq!(application.suppressed.len(), 1); + assert!(same_visible_finding( + &application.suppressed[0].finding, + &fresh_repository_claim + )); + assert_eq!( + application.suppressed[0].reason, + SuppressionReason::RepositoryClaimUnsupported + ); + } + #[test] fn adjudication_preserved_baseline_survives_a_deterministic_refutation_receipt() { let snapshot_id = "a".repeat(40); diff --git a/tests/e2e.rs b/tests/e2e.rs index aa4051a..c22b4ca 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -2713,9 +2713,10 @@ async fn adjudication_provider_failure_preserves_findings_and_baseline_blocker() &server, json!([{ "path": "src/auth.rs", "line": 42, "severity": "warn", "kind": "risk", - "confidence": 0.99, "title": "Validate query input", - "body": "The query executes attacker-controlled input without validation.", - "evidence": "exec_query(&token);" + "confidence": 0.99, "title": "Authorization validator is absent", + "body": "The repository does not contain `validate_query_input`.", + "evidence": "exec_query(&token);", + "repositoryContext": {"claim": "absence", "identifiers": ["validate_query_input"]} }]), ) .await; @@ -2778,7 +2779,7 @@ async fn adjudication_provider_failure_preserves_findings_and_baseline_blocker() .as_array() .unwrap() .iter() - .any(|finding| { finding["title"] == "Validate query input" }) + .any(|finding| { finding["title"] == "Authorization validator is absent" }) ); assert!( envelope["findings"] @@ -2827,9 +2828,10 @@ async fn malformed_adjudication_output_blocks_under_advisory_provider_policy() { &server, json!([{ "path": "src/auth.rs", "line": 42, "severity": "warn", "kind": "risk", - "confidence": 0.99, "title": "Validate query input", - "body": "The query executes attacker-controlled input without validation.", - "evidence": "exec_query(&token);" + "confidence": 0.99, "title": "Authorization validator is absent", + "body": "The repository does not contain `validate_query_input`.", + "evidence": "exec_query(&token);", + "repositoryContext": {"claim": "absence", "identifiers": ["validate_query_input"]} }]), ) .await; @@ -2858,7 +2860,8 @@ async fn malformed_adjudication_output_blocks_under_advisory_provider_policy() { .unwrap() .iter() .any(|finding| { - finding["path"] == "src/auth.rs" && finding["title"] == "Validate query input" + finding["path"] == "src/auth.rs" + && finding["title"] == "Authorization validator is absent" }) ); assert!( @@ -11416,7 +11419,7 @@ async fn full_rereview_resolves_false_absence_from_unchanged_repository_source() } #[tokio::test] -async fn fresh_repository_claims_remain_open_for_unavailable_and_exhausted_receipts() { +async fn fresh_unresolved_repository_claims_are_suppressed() { for (name, repository, resources, state) in [ ( "unavailable", @@ -11455,15 +11458,12 @@ async fn fresh_repository_claims_remain_open_for_unavailable_and_exhausted_recei command.arg("--diff-file").arg(&diff); } command.args(["--output", "json"]); - let out = command.assert().code(1); + let out = command.assert().code(0); let envelope: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); assert_eq!(envelope["repositorySearch"]["state"], state, "{name}"); - assert_eq!(envelope["counts"]["suppressed"], 0, "{name}"); - assert_eq!( - envelope["findings"][0]["title"], "Widget dependency is absent", - "{name}" - ); - assert_eq!(envelope["gate"]["failing"], true, "{name}"); + assert_eq!(envelope["counts"]["suppressed"], 1, "{name}"); + assert_eq!(envelope["findings"], json!([]), "{name}"); + assert_eq!(envelope["gate"]["failing"], false, "{name}"); } } From 409b5b582fe7723d4c338f949e3f4c458396598e Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 14 Aug 2026 08:04:17 +0000 Subject: [PATCH 4/7] test(review): cover repository claim refutations --- tests/e2e.rs | 228 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/tests/e2e.rs b/tests/e2e.rs index c22b4ca..e36c15e 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -121,6 +121,43 @@ impl Respond for DefaultAdjudicator { } } +#[derive(Clone, Copy)] +struct RefuteFromReceiptAdjudicator; + +impl Respond for RefuteFromReceiptAdjudicator { + fn respond(&self, request: &Request) -> ResponseTemplate { + let request_body: Value = request.body_json().unwrap(); + let payload: Value = serde_json::from_str( + request_body["messages"] + .as_array() + .and_then(|messages| messages.last()) + .and_then(|message| message["content"].as_str()) + .unwrap(), + ) + .unwrap(); + let candidate = payload["candidates"].as_array().unwrap().first().unwrap(); + let candidate_id = candidate["candidateId"].as_str().unwrap(); + let evidence = payload["diffCorpusReceipt"]["candidateCitations"] + .as_array() + .unwrap() + .iter() + .find(|receipt| receipt["candidateId"] == candidate_id) + .and_then(|receipt| receipt["refutationEvidence"]["source"].as_str()) + .expect("repository claim fixture must expose exact replacement evidence"); + ResponseTemplate::new(200).set_body_json(scorer_text( + &json!([{ + "candidateId": candidate_id, + "status": "refuted", + "revisedTitle": "", + "revisedBody": "", + "evidence": evidence, + "duplicateOf": null + }]) + .to_string(), + )) + } +} + #[derive(Clone, Copy)] struct AllUnresolvedAdjudicator; @@ -11418,6 +11455,197 @@ async fn full_rereview_resolves_false_absence_from_unchanged_repository_source() assert!(envelope.get("repositoryEvidence").is_none()); } +#[tokio::test] +async fn same_hunk_groups_replacement_refutes_false_deletion_claim() { + const PATH: &str = "ansible/group_vars/atlas.yml"; + const OLD_LINE: &str = "groups: [atlas_packages_base]"; + const REPLACEMENT: &str = "groups: [atlas_packages_base, atlas_packages_test]"; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(body_string_contains("single finding adjudicator")) + .respond_with(RefuteFromReceiptAdjudicator) + .with_priority(1) + .mount(&server) + .await; + mock_review( + &server, + json!([{ + "path": PATH, + "line": 120, + "severity": "warn", + "kind": "uncertainty", + "confidence": 0.91, + "title": "The groups entry was deleted", + "body": "The `groups` entry was deleted without its replacement `atlas_packages_test`.", + "evidence": "policy: atlas", + "repositoryContext": { + "claim": "mismatch", + "resources": ["groups"], + "values": ["atlas_packages_test"], + "versions": [], + "paths": [], + "identifiers": [] + } + }]), + ) + .await; + + let directory = tempfile::tempdir().unwrap(); + let diff = directory.path().join("groups-replacement.diff"); + std::fs::write( + &diff, + format!( + "diff --git a/{PATH} b/{PATH}\n--- a/{PATH}\n+++ b/{PATH}\n@@ -120,4 +120,4 @@\n-policy: legacy\n+policy: atlas\n stable: true\n owner: atlas\n-{OLD_LINE}\n+{REPLACEMENT}\n" + ), + ) + .unwrap(); + + let output = postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_DISABLE_SCORER", "1") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .success(); + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + + assert_eq!(envelope["findings"], json!([])); + assert_eq!(envelope["suppressedFindings"][0]["finding"]["path"], PATH); + assert_eq!( + envelope["suppressedFindings"][0]["finding"]["title"], + "The groups entry was deleted" + ); + assert_eq!(envelope["suppressedFindings"][0]["reason"], "nonActionable"); + assert_eq!(envelope["counts"]["warn"], 0); + assert_eq!(envelope["counts"]["suppressed"], 1); + assert_eq!(envelope["gate"]["failing"], false); + + let requests = server.received_requests().await.unwrap(); + let adjudication: Value = requests + .iter() + .find(|request| request_system_contains(request, "single finding adjudicator")) + .unwrap() + .body_json() + .unwrap(); + let payload: Value = serde_json::from_str( + adjudication["messages"].as_array().unwrap().last().unwrap()["content"] + .as_str() + .unwrap(), + ) + .unwrap(); + assert_eq!( + payload["diffCorpusReceipt"]["candidateCitations"][0]["refutationEvidenceComplete"], + true + ); + assert_eq!( + payload["diffCorpusReceipt"]["candidateCitations"][0]["refutationEvidence"]["source"], + REPLACEMENT + ); +} + +#[tokio::test] +async fn cross_file_package_existence_refutes_false_absence_claim() { + const CLAIM_PATH: &str = "ci/package-policy.yml"; + const PACKAGE_PATH: &str = "packages/atlas/manifest.yml"; + const REPLACEMENT: &str = "name: atlas_packages_base"; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .and(body_string_contains("single finding adjudicator")) + .respond_with(RefuteFromReceiptAdjudicator) + .with_priority(1) + .mount(&server) + .await; + mock_review( + &server, + json!([{ + "path": CLAIM_PATH, + "line": 24, + "severity": "warn", + "kind": "uncertainty", + "confidence": 0.88, + "title": "The atlas package is absent", + "body": "The repository does not contain `atlas_packages_base`.", + "evidence": "package: atlas_packages_candidate", + "repositoryContext": { + "claim": "absence", + "resources": ["atlas package"], + "values": ["atlas_packages_base"], + "versions": [], + "paths": [], + "identifiers": [] + } + }]), + ) + .await; + + let directory = tempfile::tempdir().unwrap(); + let diff = directory.path().join("package-existence.diff"); + std::fs::write( + &diff, + format!( + "diff --git a/{CLAIM_PATH} b/{CLAIM_PATH}\n--- a/{CLAIM_PATH}\n+++ b/{CLAIM_PATH}\n@@ -24,1 +24,1 @@\n-package: atlas_packages_legacy\n+package: atlas_packages_candidate\ndiff --git a/{PACKAGE_PATH} b/{PACKAGE_PATH}\n--- a/{PACKAGE_PATH}\n+++ b/{PACKAGE_PATH}\n@@ -1,2 +1,2 @@\n-type: legacy package\n-name: atlas_packages_legacy\n+type: atlas package\n+{REPLACEMENT}\n" + ), + ) + .unwrap(); + + let output = postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("POSTIL_DISABLE_SCORER", "1") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .success(); + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + + assert_eq!(envelope["findings"], json!([])); + assert_eq!( + envelope["suppressedFindings"][0]["finding"]["path"], + CLAIM_PATH + ); + assert_eq!( + envelope["suppressedFindings"][0]["finding"]["title"], + "The atlas package is absent" + ); + assert_eq!(envelope["suppressedFindings"][0]["reason"], "nonActionable"); + assert_eq!(envelope["counts"]["warn"], 0); + assert_eq!(envelope["counts"]["suppressed"], 1); + assert_eq!(envelope["gate"]["failing"], false); + + let requests = server.received_requests().await.unwrap(); + let adjudication: Value = requests + .iter() + .find(|request| request_system_contains(request, "single finding adjudicator")) + .unwrap() + .body_json() + .unwrap(); + let payload: Value = serde_json::from_str( + adjudication["messages"].as_array().unwrap().last().unwrap()["content"] + .as_str() + .unwrap(), + ) + .unwrap(); + assert_eq!( + payload["diffCorpusReceipt"]["candidateCitations"][0]["refutationEvidenceComplete"], + true + ); + assert_eq!( + payload["diffCorpusReceipt"]["candidateCitations"][0]["refutationEvidence"]["path"], + PACKAGE_PATH + ); + assert_eq!( + payload["diffCorpusReceipt"]["candidateCitations"][0]["refutationEvidence"]["source"], + REPLACEMENT + ); +} + #[tokio::test] async fn fresh_unresolved_repository_claims_are_suppressed() { for (name, repository, resources, state) in [ From 498154532d0e99997c09833d1ab54afb7822f145 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 14 Aug 2026 09:10:10 +0000 Subject: [PATCH 5/7] feat(review): emit immutable GitHub publication plans --- src/cli.rs | 94 +++++ src/forge/github.rs | 917 ++++++++++++++++++++++++++++++++++++++------ src/forge/mod.rs | 385 ++++++++++++++++++- src/main.rs | 2 + src/review.rs | 108 ++++-- tests/e2e.rs | 158 ++++++++ 6 files changed, 1505 insertions(+), 159 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 7155fde..dab4cca 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -141,6 +141,21 @@ pub enum Command { /// Leave the merge-gate check pending for a controlling service to complete. #[arg(long, requires = "publish")] defer_gate_check: bool, + /// Write an immutable GitHub publication plan without mutating the forge. + #[arg( + long, + hide = true, + value_name = "PATH", + requires_all = ["repo", "pr", "sha", "base_sha"], + conflicts_with_all = [ + "publish", + "no_post", + "check_run_id", + "gate_check_run_id", + "defer_gate_check" + ] + )] + publication_plan_output: Option, }, /// Reply to an @postil mention on a pull request or issue (interactive bot). Respond { @@ -356,6 +371,85 @@ mod tests { ); } + #[test] + fn publication_plan_is_hidden_snapshot_bound_and_separate_from_mutation() { + let parsed = Cli::try_parse_from([ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + ]) + .unwrap(); + let Command::Review { + publication_plan_output, + publish, + .. + } = parsed.command + else { + panic!("expected review command"); + }; + assert_eq!( + publication_plan_output.as_deref(), + Some(std::path::Path::new("publication-plan.json")) + ); + assert!(!publish); + + let help = Cli::command() + .find_subcommand_mut("review") + .expect("review subcommand") + .render_long_help() + .to_string(); + assert!(!help.contains("publication-plan-output")); + + for extra in [ + vec!["--publish"], + vec!["--no-post"], + vec!["--check-run-id", "901"], + vec!["--gate-check-run-id", "902"], + ] { + let mut arguments = vec![ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + ]; + arguments.extend(extra); + assert!(Cli::try_parse_from(arguments).is_err()); + } + + assert!( + Cli::try_parse_from([ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--publication-plan-output", + "publication-plan.json", + ]) + .is_err() + ); + } + #[test] fn login_accepts_an_optional_org_hint() { let parsed = Cli::try_parse_from(["postil", "login"]).unwrap(); diff --git a/src/forge/github.rs b/src/forge/github.rs index dc4b9af..5e824cb 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -11,9 +11,14 @@ use std::io::Write; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use super::{ - CheckRunIds, CheckState, FindingPublicationOutcome, FindingPublicationReceipt, Forge, PrMeta, - ReviewPublicationReceipt, ReviewPublicationSummary, SummaryContext, ThreadKind, check_summary, - check_title, only_operational_findings, valid_details_url, + CheckRunIds, CheckState, FindingPublicationOutcome, FindingPublicationReceipt, Forge, + GitHubPublicationPlan, GitHubPublicationPlanRequest, PrMeta, PublicationPlanCheckAnnotation, + PublicationPlanFallbackActivation, PublicationPlanFileComment, PublicationPlanFinding, + PublicationPlanFindingFallback, PublicationPlanOperation, PublicationPlanOperationKind, + PublicationPlanRepository, PublicationPlanReviewAction, PublicationPlanReviewAttempt, + PublicationPlanReviewComment, PublicationPlanSnapshot, ReviewPublicationReceipt, + ReviewPublicationSummary, SummaryContext, ThreadKind, check_summary, check_title, + only_operational_findings, valid_details_url, }; use crate::diff::{Diff, DiffIndex, DiffSnapshot, DiffSpool, WorkspaceBudget}; use crate::envelope::{Envelope, Finding, Severity}; @@ -250,6 +255,50 @@ impl GitHub { Ok(()) } + async fn publication_plan_repository_identity(&self) -> Result { + let response = self + .send_retryable( + self.request(reqwest::Method::GET, self.url("")), + "publication-plan repository identity", + ) + .await?; + let identity: RepositoryIdentity = super::bounded_response_json( + Self::check_ok(response, "publication-plan repository identity").await?, + "GitHub publication-plan repository identity", + ) + .await?; + let expected_name = format!("{}/{}", self.owner, self.repo); + ensure!( + identity.id > 0 && identity.full_name.eq_ignore_ascii_case(&expected_name), + "GitHub repository identity changed; refusing publication planning" + ); + if let Some(expected_id) = self.expected_repository_id { + ensure!( + identity.id == expected_id, + "GitHub repository identity changed; refusing publication planning" + ); + } + Ok(identity) + } + + async fn reconcile_published_finding_markers( + &self, + receipt: &mut ReviewPublicationReceipt, + head_sha: &str, + ) -> std::collections::HashMap { + let published = self.published_finding_comments(head_sha).await; + for publication in &mut receipt.findings { + let marker = finding_marker(&publication.finding_id); + let Some(comment) = published.get(&marker) else { + continue; + }; + publication.initial_outcome = FindingPublicationOutcome::Carried; + publication.inline_rejected = false; + publication.comment_id = Some(comment.id.to_string()); + } + published + } + async fn fetch_pr_state(&self) -> Result { let response = self .send_retryable( @@ -1718,12 +1767,85 @@ enum ReviewDelivery { Reconciled(PublishedReview), } -#[derive(Deserialize)] +#[derive(Clone, Deserialize)] struct RepositoryIdentity { id: u64, full_name: String, } +#[derive(Clone)] +struct PlannedCheckOutput { + name: &'static str, + state: CheckState, + title: String, + summary: String, + annotations: Vec, +} + +fn planned_check_outputs( + envelope: &Envelope, + advisory: CheckState, + gate: Option, + annotate_findings: bool, + details_url: Option, +) -> Vec { + let annotations = if annotate_findings { + envelope + .findings + .iter() + .filter(|finding| !filter::is_carried(finding)) + .filter(|finding| !super::is_synthetic_path(&finding.path)) + .map(|finding| { + let publication = crate::envelope::forge_safe_finding_publication_text(finding); + PublicationPlanCheckAnnotation { + path: finding.path.clone(), + start_line: finding.line, + end_line: finding.end_line.unwrap_or(finding.line), + annotation_level: match finding.severity { + Severity::Info => "notice", + Severity::Warn => "warning", + Severity::Error => "failure", + } + .to_string(), + title: publication.title, + message: publication.body, + } + }) + .collect() + } else { + Vec::new() + }; + debug_assert!(annotations.len() <= GITHUB_MAX_ANNOTATIONS_PER_REQUEST); + + let advisory_summary = check_summary( + envelope, + true, + SummaryContext { + details_url, + prevention_hint: false, + prevention_commands: vec![], + publication: None, + }, + ); + let mut checks = vec![PlannedCheckOutput { + name: "postil/review", + state: advisory, + title: super::cap_check_title(&check_title(envelope)), + summary: super::cap_check_summary(&advisory_summary), + annotations, + }]; + if let Some(gate) = gate { + checks.push(PlannedCheckOutput { + name: "postil/gate", + state: gate, + title: super::cap_check_title(gate_title(envelope)), + summary: super::cap_check_summary(&gate_summary(envelope)), + annotations: vec![], + }); + } + checks +} + impl Forge for GitHub { fn rich_markdown(&self) -> bool { true @@ -1743,6 +1865,329 @@ impl Forge for GitHub { planned_review_receipt(envelope, &snapshot.head_sha) } + async fn build_publication_plan( + &self, + request: GitHubPublicationPlanRequest<'_>, + ) -> Result { + let GitHubPublicationPlanRequest { + envelope, + snapshot, + publication_diff, + should_comment, + duplicate_of_baseline, + annotate_findings, + advisory, + gate, + } = request; + ensure!( + self.snapshot_is_current(snapshot).await?, + "GitHub publication planning skipped because the pull request snapshot changed" + ); + let repository = self.publication_plan_repository_identity().await?; + let target_sha = snapshot + .target_sha + .as_deref() + .context("GitHub publication planning requires a target snapshot")?; + let mut receipt = self.plan_review_publication(envelope, snapshot); + if annotate_findings { + receipt.channel = super::ReviewPublicationChannel::CheckAnnotations; + for finding in &mut receipt.findings { + if finding.initial_outcome == FindingPublicationOutcome::Inline { + finding.initial_outcome = FindingPublicationOutcome::CheckAnnotation; + } + } + } else if duplicate_of_baseline { + for finding in &mut receipt.findings { + if matches!( + finding.initial_outcome, + FindingPublicationOutcome::Inline + | FindingPublicationOutcome::FileComment + | FindingPublicationOutcome::SummaryOnly + ) { + finding.initial_outcome = FindingPublicationOutcome::Carried; + } + } + } + + let mut action = PublicationPlanReviewAction::Omit; + let mut initial_attempt = None; + let mut placement_fallback_attempt = None; + let mut summary_fallback_attempt = None; + let mut fallback_intent = + std::collections::HashMap::>::new(); + let mut file_comment_operations = Vec::new(); + + if should_comment && !annotate_findings { + let published = self + .reconcile_published_finding_markers(&mut receipt, &snapshot.head_sha) + .await; + let publishable_findings = publication_plan_publishable_findings(envelope, &published); + let summary = self.review_summary_for_receipt(envelope, &receipt); + let marker = review_marker(&receipt.receipt_id); + let comments = publishable_findings + .iter() + .map(|finding| publication_plan_review_comment(&initial_review_comment(finding))) + .collect::>>()?; + if !comments.is_empty() || !summary.is_empty() { + action = PublicationPlanReviewAction::Publish; + initial_attempt = Some(PublicationPlanReviewAttempt { + body: bounded_review_body(&summary, &marker, self.details_url.as_deref()), + comments, + }); + } + + if !publishable_findings.is_empty() { + let owned_publication_diff = if publication_diff.is_none() { + Some( + self.fetch_diff(snapshot) + .await + .context("fetching complete diff for GitHub publication planning")?, + ) + } else { + None + }; + let parsed_publication_diff = owned_publication_diff + .as_ref() + .map(|diff| crate::diff::parse(diff.as_str())); + let publication_diff = publication_diff + .or(parsed_publication_diff.as_ref()) + .context( + "GitHub publication planning is missing the complete pull-request diff", + )?; + let placement_index = DiffIndex::build(publication_diff); + let mut line_findings = Vec::new(); + let mut file_findings = Vec::new(); + let mut summary_findings = Vec::new(); + for finding in &publishable_findings { + let (finding_id, _) = finding_receipt_id(finding); + let Some(path) = publication_file_path(publication_diff, &finding.path) else { + fallback_intent.insert( + finding_id, + vec![PublicationPlanFindingFallback::SummaryOnly], + ); + summary_findings.push(*finding); + continue; + }; + if let Some(line) = placement_index.nearest_new_side_line(path, finding.line) { + fallback_intent.insert( + finding_id, + vec![ + PublicationPlanFindingFallback::RelocatedInline, + PublicationPlanFindingFallback::FileComment, + ], + ); + line_findings.push((*finding, path, line)); + } else { + fallback_intent.insert( + finding_id, + vec![PublicationPlanFindingFallback::FileComment], + ); + file_findings.push((*finding, path)); + } + } + + let mut fallback_receipt = receipt.clone(); + for (finding, _) in &file_findings { + set_publication_outcome( + &mut fallback_receipt, + finding, + FindingPublicationOutcome::FileComment, + false, + )?; + } + for finding in &summary_findings { + set_publication_outcome( + &mut fallback_receipt, + finding, + FindingPublicationOutcome::SummaryOnly, + true, + )?; + } + let fallback_summary = self.review_summary_with_unplaced_findings( + envelope, + &fallback_receipt, + &summary_findings, + ); + let fallback_summary = if summary_findings.is_empty() { + bounded_review_body( + if fallback_summary.is_empty() { + "Postil completed the review." + } else { + &fallback_summary + }, + &marker, + self.details_url.as_deref(), + ) + } else { + required_review_body(&fallback_summary, &marker)? + }; + placement_fallback_attempt = Some(PublicationPlanReviewAttempt { + body: fallback_summary, + comments: line_findings + .iter() + .map(|(finding, path, line)| { + publication_plan_review_comment(&fallback_line_comment( + finding, path, *line, + )) + }) + .collect::>>()?, + }); + + let mut summary_receipt = fallback_receipt; + for (finding, _, _) in &line_findings { + set_publication_outcome( + &mut summary_receipt, + finding, + FindingPublicationOutcome::FileComment, + false, + )?; + } + if !line_findings.is_empty() { + let summary = self.review_summary_with_unplaced_findings( + envelope, + &summary_receipt, + &summary_findings, + ); + summary_fallback_attempt = Some(PublicationPlanReviewAttempt { + body: if summary_findings.is_empty() { + bounded_review_body(&summary, &marker, self.details_url.as_deref()) + } else { + required_review_body(&summary, &marker)? + }, + comments: vec![], + }); + } + + for (finding, path) in file_findings.into_iter().chain( + line_findings + .into_iter() + .map(|(finding, path, _)| (finding, path)), + ) { + let (finding_id, _) = finding_receipt_id(finding); + let payload = publication_plan_file_comment(&file_level_comment( + finding, + path, + &snapshot.head_sha, + ))?; + file_comment_operations.push(PublicationPlanOperation { + operation_key: publication_plan_operation_key( + repository.id, + self.pr, + &snapshot.head_sha, + "file-comment-fallback", + Some(&finding_id), + ), + desired: PublicationPlanOperationKind::FileCommentFallback { + finding_id, + activation: + PublicationPlanFallbackActivation::CompositeReviewPlacementFailed, + payload, + }, + }); + } + } + } + + let findings = receipt + .findings + .iter() + .map(|publication| { + let finding = publication_plan_finding(envelope, &publication.finding_id)?; + Ok(PublicationPlanFinding { + finding_id: publication.finding_id.clone(), + stable_identity: publication.stable_identity, + path: finding.path.clone(), + line: finding.line, + end_line: finding.end_line, + initial_outcome: publication.initial_outcome, + fallback_intent: fallback_intent + .remove(&publication.finding_id) + .unwrap_or_default(), + }) + }) + .collect::>>()?; + + let mut operations = vec![PublicationPlanOperation { + operation_key: publication_plan_operation_key( + repository.id, + self.pr, + &snapshot.head_sha, + "composite-review", + None, + ), + desired: PublicationPlanOperationKind::CompositeReview { + action, + receipt_id: receipt.receipt_id.clone(), + initial_attempt, + placement_fallback_attempt, + summary_fallback_attempt, + findings, + }, + }]; + operations.append(&mut file_comment_operations); + let conclusion = |state: CheckState| match state { + CheckState::Success => "success", + CheckState::Failure => "failure", + CheckState::Neutral => "neutral", + }; + for check in planned_check_outputs( + envelope, + advisory, + Some(gate), + annotate_findings, + self.details_url.clone(), + ) { + let operation_key = publication_plan_operation_key( + repository.id, + self.pr, + &snapshot.head_sha, + check.name, + None, + ); + let desired = if check.name == "postil/review" { + PublicationPlanOperationKind::AdvisoryCheck { + name: check.name.to_string(), + head_sha: snapshot.head_sha.clone(), + conclusion: conclusion(check.state).to_string(), + title: check.title, + summary: check.summary, + annotations: check.annotations, + details_url: self.details_url.clone(), + } + } else { + PublicationPlanOperationKind::GateCheck { + name: check.name.to_string(), + head_sha: snapshot.head_sha.clone(), + conclusion: conclusion(check.state).to_string(), + title: check.title, + summary: check.summary, + details_url: self.details_url.clone(), + } + }; + operations.push(PublicationPlanOperation { + operation_key, + desired, + }); + } + + GitHubPublicationPlan::new( + PublicationPlanRepository { + id: repository.id, + full_name: repository.full_name, + }, + self.pr, + PublicationPlanSnapshot { + head_sha: snapshot.head_sha.clone(), + merge_base_sha: snapshot.base_sha.clone(), + target_sha: target_sha.to_string(), + pull_request_title_sha256: publication_plan_text_digest(&snapshot.title), + pull_request_body_sha256: publication_plan_text_digest(&snapshot.body), + }, + receipt.receipt_id, + operations, + ) + } + async fn fetch_pr_meta(&self) -> Result { let resp = self .send_retryable( @@ -1886,31 +2331,18 @@ impl Forge for GitHub { // 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_comments(head_sha).await; - let mut observed_comment_count = 0usize; - for publication in &mut planned_receipt.findings { - let marker = finding_marker(&publication.finding_id); - let Some(comment) = published.get(&marker) else { - continue; - }; - publication.initial_outcome = FindingPublicationOutcome::Carried; - publication.inline_rejected = false; - publication.comment_id = Some(comment.id.to_string()); - observed_comment_count += 1; - } - let publishable_findings: Vec<_> = findings + let published = self + .reconcile_published_finding_markers(&mut planned_receipt, head_sha) + .await; + let observed_comment_count = planned_receipt + .findings .iter() - // Carried findings already have comments from the previous review. - .filter(|f| !filter::is_carried(f)) - // Synthetic-path findings (PR description, fail-closed markers) have - // no real file line to anchor an inline comment; they surface only in - // the summary body. - .filter(|f| !super::is_synthetic_path(&f.path)) - .filter(|f| { - let (finding_id, _) = finding_receipt_id(f); - !published.contains_key(&finding_marker(&finding_id)) + .filter(|publication| { + publication.initial_outcome == FindingPublicationOutcome::Carried + && publication.comment_id.is_some() }) - .collect(); + .count(); + let publishable_findings = publication_plan_publishable_findings(envelope, &published); let comments: Vec<_> = publishable_findings .iter() .map(|finding| initial_review_comment(finding)) @@ -2311,96 +2743,63 @@ impl Forge for GitHub { CheckState::Failure => "failure", CheckState::Neutral => "neutral", }; - 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() - } 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((check_ids.gate, gate, "postil/gate", false)); - } + let checks = planned_check_outputs( + envelope, + advisory, + gate, + annotate_findings, + self.details_url.clone(), + ); let mut results = stream::iter(checks.into_iter().enumerate().map( - |(index, (id, state, name, with_annotations))| { - let annotations = &annotations; - async move { - let gate_note = if name == "postil/gate" { - gate_summary(envelope) - } else { - check_summary( - envelope, - true, - SummaryContext { - details_url: self.details_url.clone(), - prevention_hint: false, - prevention_commands: vec![], - publication: None, - }, - ) - }; - let title = if name == "postil/gate" { - gate_title(envelope).to_string() - } else { - check_title(envelope) - }; - let mut output = json!({ - // GitHub rejects title >255 and summary >65535 with HTTP 422, - // which would abort posting both checks. Cap both defensively. - "title": super::cap_check_title(&title), - "summary": super::cap_check_summary(&gate_note), - }); - if annotate_findings && with_annotations && !annotations.is_empty() { - output["annotations"] = json!(annotations); - } - let mut body = json!({ - "status": "completed", - "conclusion": conclusion(state), - "output": output, - }); - self.add_details_url(&mut body); - let result = match self - .send_write_retryable( - self.request( - reqwest::Method::PATCH, - self.url(&format!("/check-runs/{id}")), - ) - .json(&body), - &format!("complete {name}"), + |(index, planned)| async move { + let id = if planned.name == "postil/review" { + check_ids.advisory + } else { + check_ids.gate + }; + let mut output = json!({ + "title": planned.title, + "summary": planned.summary, + }); + if !planned.annotations.is_empty() { + output["annotations"] = json!( + planned + .annotations + .iter() + .map(|annotation| json!({ + "path": annotation.path, + "start_line": annotation.start_line, + "end_line": annotation.end_line, + "annotation_level": annotation.annotation_level, + "title": annotation.title, + "message": annotation.message, + })) + .collect::>() + ); + } + let mut body = json!({ + "status": "completed", + "conclusion": conclusion(planned.state), + "output": output, + }); + self.add_details_url(&mut body); + let result = match self + .send_write_retryable( + self.request( + reqwest::Method::PATCH, + self.url(&format!("/check-runs/{id}")), ) + .json(&body), + &format!("complete {}", planned.name), + ) + .await + { + Ok(response) => Self::check_ok(response, "check-run complete") .await - { - Ok(response) => Self::check_ok(response, "check-run complete") - .await - .map(|_| ()), - Err(error) => Err(error), - }; - (index, name, result) - } + .map(|_| ()), + Err(error) => Err(error), + }; + (index, planned.name, result) }, )) .buffer_unordered(2) @@ -2540,6 +2939,115 @@ fn planned_review_receipt(envelope: &Envelope, head_sha: &str) -> ReviewPublicat } } +fn publication_plan_text_digest(value: &str) -> String { + format!( + "sha256:{}", + crate::repository_search::hex_digest(Sha256::digest(value.as_bytes())) + ) +} + +fn publication_plan_operation_key( + repository_id: u64, + pull_request_number: u64, + head_sha: &str, + kind: &str, + finding_id: Option<&str>, +) -> String { + let mut digest = Sha256::new(); + digest.update(b"github-publication-operation-v1\0"); + digest.update(repository_id.to_be_bytes()); + digest.update(pull_request_number.to_be_bytes()); + digest.update(head_sha.as_bytes()); + digest.update(kind.as_bytes()); + if let Some(finding_id) = finding_id { + digest.update([0]); + digest.update(finding_id.as_bytes()); + } + format!( + "github-publication-v1:{kind}:sha256:{}", + crate::repository_search::hex_digest(digest.finalize()) + ) +} + +fn publication_plan_publishable_findings<'a>( + envelope: &'a Envelope, + published: &std::collections::HashMap, +) -> Vec<&'a Finding> { + envelope + .findings + .iter() + .filter(|finding| !filter::is_carried(finding)) + .filter(|finding| !super::is_synthetic_path(&finding.path)) + .filter(|finding| { + let (finding_id, _) = finding_receipt_id(finding); + !published.contains_key(&finding_marker(&finding_id)) + }) + .collect() +} + +fn publication_plan_finding<'a>(envelope: &'a Envelope, finding_id: &str) -> Result<&'a Finding> { + envelope + .findings + .iter() + .chain(envelope.resolved.iter()) + .chain( + envelope + .suppressed_findings + .iter() + .map(|suppressed| &suppressed.finding), + ) + .find(|finding| finding_receipt_id(finding).0 == finding_id) + .with_context(|| format!("GitHub publication plan omitted finding identity {finding_id}")) +} + +fn publication_plan_review_comment( + value: &serde_json::Value, +) -> Result { + Ok(PublicationPlanReviewComment { + path: value["path"] + .as_str() + .context("GitHub review comment plan omitted its path")? + .to_string(), + line: value["line"] + .as_u64() + .and_then(|line| u32::try_from(line).ok()) + .context("GitHub review comment plan omitted its line")?, + side: value["side"] + .as_str() + .context("GitHub review comment plan omitted its side")? + .to_string(), + start_line: value["start_line"] + .as_u64() + .and_then(|line| u32::try_from(line).ok()), + start_side: value["start_side"].as_str().map(str::to_string), + body: value["body"] + .as_str() + .context("GitHub review comment plan omitted its body")? + .to_string(), + }) +} + +fn publication_plan_file_comment(value: &serde_json::Value) -> Result { + Ok(PublicationPlanFileComment { + body: value["body"] + .as_str() + .context("GitHub file-comment plan omitted its body")? + .to_string(), + commit_id: value["commit_id"] + .as_str() + .context("GitHub file-comment plan omitted its commit id")? + .to_string(), + path: value["path"] + .as_str() + .context("GitHub file-comment plan omitted its path")? + .to_string(), + subject_type: value["subject_type"] + .as_str() + .context("GitHub file-comment plan omitted its subject type")? + .to_string(), + }) +} + fn initial_review_comment(finding: &Finding) -> serde_json::Value { let (finding_id, _) = finding_receipt_id(finding); let mut comment = json!({ @@ -3649,6 +4157,187 @@ mod tests { .await; } + #[tokio::test] + async fn publication_plan_reuses_review_checks_markers_and_the_placement_ladder() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": 42, + "full_name": "owner/repo" + }))) + .expect(1) + .mount(&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!([]))) + .expect(1) + .mount(&server) + .await; + + let mut active = publication_finding( + "active-1", + "src/lib.rs", + "The unchecked value reaches the protected operation.", + ); + active.severity = Severity::Error; + let mut carried = publication_finding( + "carried-1", + "src/carried.rs", + "The carried finding remains open.", + ); + carried.body = format!( + "{} The carried finding remains open.", + crate::filter::CARRIED_MARKER + ); + let summary_only = publication_finding( + "summary-1", + crate::envelope::CHANGE_METADATA_PATH, + "The metadata finding belongs in the summary.", + ); + let resolved = publication_finding( + "resolved-1", + "src/resolved.rs", + "The resolved finding remains identifiable.", + ); + let suppressed = publication_finding( + "suppressed-1", + "src/suppressed.rs", + "The suppressed finding remains identifiable.", + ); + let mut envelope = delivery_envelope_with_findings( + "aaaaaaaaaaaa", + "cccccccccccc", + vec![active, carried, summary_only], + ); + envelope.resolved.push(resolved); + envelope.suppressed_findings.push(SuppressedFinding { + finding: suppressed, + reason: SuppressionReason::BelowConfidence, + }); + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + let plan = test_github(&server) + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + envelope: &envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Failure, + }) + .await + .unwrap(); + let serialized = serde_json::to_value(&plan).unwrap(); + let operations = serialized["operations"].as_array().unwrap(); + assert_eq!(operations[0]["kind"], "compositeReview"); + assert_eq!(operations[0]["action"], "publish"); + assert_eq!(operations[0]["initialAttempt"]["comments"][0]["line"], 7); + assert_eq!( + operations[0]["placementFallbackAttempt"]["comments"][0]["path"], + "src/lib.rs" + ); + assert!(operations[0]["summaryFallbackAttempt"]["body"].is_string()); + let findings = operations[0]["findings"].as_array().unwrap(); + assert_eq!(findings[0]["initialOutcome"], "inline"); + assert_eq!( + findings[0]["fallbackIntent"], + serde_json::json!(["relocatedInline", "fileComment"]) + ); + assert_eq!(findings[1]["initialOutcome"], "carried"); + assert_eq!(findings[2]["initialOutcome"], "summaryOnly"); + assert_eq!(findings[3]["initialOutcome"], "resolved"); + assert_eq!(findings[4]["initialOutcome"], "suppressed"); + assert_eq!(operations[1]["kind"], "fileCommentFallback"); + assert_eq!(operations[1]["findingId"], "active-1"); + assert_eq!( + operations[1]["activation"], + "compositeReviewPlacementFailed" + ); + assert_eq!(operations[2]["kind"], "advisoryCheck"); + assert_eq!(operations[2]["conclusion"], "success"); + assert_eq!(operations[3]["kind"], "gateCheck"); + assert_eq!(operations[3]["conclusion"], "failure"); + + let requests = server.received_requests().await.unwrap(); + assert!(requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + })); + } + + #[tokio::test] + async fn publication_plan_routes_check_annotation_presentation_without_review_delivery() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": 42, + "full_name": "owner/repo" + }))) + .expect(1) + .mount(&server) + .await; + let mut finding = publication_finding( + "annotation-1", + "src/lib.rs", + "The unchecked value reaches the protected operation.", + ); + finding.severity = Severity::Error; + let envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![finding]); + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + let plan = test_github(&server) + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + envelope: &envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: true, + advisory: CheckState::Success, + gate: CheckState::Failure, + }) + .await + .unwrap(); + let operations = serde_json::to_value(plan).unwrap()["operations"] + .as_array() + .unwrap() + .clone(); + assert_eq!(operations.len(), 3); + assert_eq!(operations[0]["kind"], "compositeReview"); + assert_eq!(operations[0]["action"], "omit"); + assert!(operations[0].get("initialAttempt").is_none()); + assert_eq!( + operations[0]["findings"][0]["initialOutcome"], + "checkAnnotation" + ); + assert_eq!(operations[1]["kind"], "advisoryCheck"); + assert_eq!(operations[1]["annotations"][0]["path"], "src/lib.rs"); + assert_eq!(operations[1]["annotations"][0]["startLine"], 7); + assert_eq!(operations[2]["kind"], "gateCheck"); + + let requests = server.received_requests().await.unwrap(); + assert!(requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + })); + } + #[tokio::test] async fn github_receipt_separates_inline_summary_carried_resolved_and_suppressed() { let server = MockServer::start().await; diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 4995969..e9b4cf3 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -461,6 +461,233 @@ pub enum FindingPublicationOutcome { FileComment, } +/// Immutable desired GitHub state emitted for the service-owned publication +/// controller. The digest covers the complete canonical intent except for the +/// digest field itself. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct GitHubPublicationPlan { + pub version: u8, + pub forge: String, + pub repository: PublicationPlanRepository, + pub pull_request_number: u64, + pub reviewed_snapshot: PublicationPlanSnapshot, + pub receipt_id: String, + pub operations: Vec, + pub intent_digest: String, +} + +pub struct GitHubPublicationPlanRequest<'a> { + pub envelope: &'a Envelope, + pub snapshot: &'a PrMeta, + pub publication_diff: Option<&'a crate::diff::Diff>, + pub should_comment: bool, + pub duplicate_of_baseline: bool, + pub annotate_findings: bool, + pub advisory: CheckState, + pub gate: CheckState, +} + +impl GitHubPublicationPlan { + pub const VERSION: u8 = 1; + + pub(crate) fn new( + repository: PublicationPlanRepository, + pull_request_number: u64, + reviewed_snapshot: PublicationPlanSnapshot, + receipt_id: String, + operations: Vec, + ) -> Result { + let mut plan = Self { + version: Self::VERSION, + forge: "github".to_string(), + repository, + pull_request_number, + reviewed_snapshot, + receipt_id, + operations, + intent_digest: String::new(), + }; + plan.intent_digest = plan.canonical_intent_digest()?; + Ok(plan) + } + + fn canonical_intent_digest(&self) -> Result { + #[derive(Serialize)] + #[serde(rename_all = "camelCase")] + struct CanonicalIntent<'a> { + version: u8, + forge: &'a str, + repository: &'a PublicationPlanRepository, + pull_request_number: u64, + reviewed_snapshot: &'a PublicationPlanSnapshot, + receipt_id: &'a str, + operations: &'a [PublicationPlanOperation], + } + + let canonical = serde_json::to_vec(&CanonicalIntent { + version: self.version, + forge: &self.forge, + repository: &self.repository, + pull_request_number: self.pull_request_number, + reviewed_snapshot: &self.reviewed_snapshot, + receipt_id: &self.receipt_id, + operations: &self.operations, + }) + .context("serializing canonical GitHub publication intent")?; + Ok(format!( + "sha256:{}", + crate::repository_search::hex_digest(Sha256::digest(canonical)) + )) + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanRepository { + pub id: u64, + pub full_name: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanSnapshot { + pub head_sha: String, + pub merge_base_sha: String, + pub target_sha: String, + pub pull_request_title_sha256: String, + pub pull_request_body_sha256: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanOperation { + pub operation_key: String, + #[serde(flatten)] + pub desired: PublicationPlanOperationKind, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde( + tag = "kind", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] +pub enum PublicationPlanOperationKind { + CompositeReview { + action: PublicationPlanReviewAction, + receipt_id: String, + #[serde(skip_serializing_if = "Option::is_none")] + initial_attempt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + placement_fallback_attempt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + summary_fallback_attempt: Option, + findings: Vec, + }, + FileCommentFallback { + finding_id: String, + activation: PublicationPlanFallbackActivation, + payload: PublicationPlanFileComment, + }, + AdvisoryCheck { + name: String, + head_sha: String, + conclusion: String, + title: String, + summary: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + annotations: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + details_url: Option, + }, + GateCheck { + name: String, + head_sha: String, + conclusion: String, + title: String, + summary: String, + #[serde(skip_serializing_if = "Option::is_none")] + details_url: Option, + }, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum PublicationPlanReviewAction { + Publish, + Omit, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanReviewAttempt { + pub body: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub comments: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanReviewComment { + pub path: String, + pub line: u32, + pub side: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub start_line: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub start_side: Option, + pub body: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanFileComment { + pub body: String, + pub commit_id: String, + pub path: String, + pub subject_type: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanFinding { + pub finding_id: String, + pub stable_identity: bool, + pub path: String, + pub line: u32, + #[serde(skip_serializing_if = "Option::is_none")] + pub end_line: Option, + pub initial_outcome: FindingPublicationOutcome, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub fallback_intent: Vec, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum PublicationPlanFindingFallback { + RelocatedInline, + FileComment, + SummaryOnly, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum PublicationPlanFallbackActivation { + CompositeReviewPlacementFailed, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PublicationPlanCheckAnnotation { + pub path: String, + pub start_line: u32, + pub end_line: u32, + pub annotation_level: String, + pub title: String, + pub message: String, +} + #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct ReviewPublicationSummary { pub active_inline: usize, @@ -543,23 +770,32 @@ pub fn write_review_publication_receipt_from_env(receipt: &ReviewPublicationRece } fn write_review_publication_receipt(path: &Path, receipt: &ReviewPublicationReceipt) -> Result<()> { + write_private_json_atomically(path, receipt, "publication receipt") +} + +pub fn write_github_publication_plan(path: &Path, plan: &GitHubPublicationPlan) -> Result<()> { + write_private_json_atomically(path, plan, "GitHub publication plan") +} + +fn write_private_json_atomically( + path: &Path, + value: &T, + artifact: &str, +) -> Result<()> { let parent = path .parent() .filter(|parent| !parent.as_os_str().is_empty()) - .ok_or_else(|| anyhow::anyhow!("publication receipt path must name a file"))?; - ensure!( - parent.is_dir(), - "publication receipt directory does not exist" - ); + .ok_or_else(|| anyhow::anyhow!("{artifact} path must name a file"))?; + ensure!(parent.is_dir(), "{artifact} directory does not exist"); if let Ok(metadata) = fs::symlink_metadata(path) { ensure!( !metadata.file_type().is_symlink(), - "publication receipt path must not be a symlink" + "{artifact} path must not be a symlink" ); } let file_name = path .file_name() - .ok_or_else(|| anyhow::anyhow!("publication receipt path must name a file"))?; + .ok_or_else(|| anyhow::anyhow!("{artifact} path must name a file"))?; let temporary = parent.join(format!( ".{}.{}.tmp", file_name.to_string_lossy(), @@ -576,16 +812,19 @@ fn write_review_publication_receipt(path: &Path, receipt: &ReviewPublicationRece let result = (|| -> Result<()> { let mut file = options .open(&temporary) - .context("creating private publication receipt")?; - serde_json::to_writer(&mut file, receipt).context("serializing publication receipt")?; + .with_context(|| format!("creating private {artifact}"))?; + serde_json::to_writer(&mut file, value) + .with_context(|| format!("serializing {artifact}"))?; file.write_all(b"\n") - .context("writing publication receipt")?; - file.sync_all().context("syncing publication receipt")?; - fs::rename(&temporary, path).context("atomically publishing publication receipt")?; + .with_context(|| format!("writing {artifact}"))?; + file.sync_all() + .with_context(|| format!("syncing {artifact}"))?; + fs::rename(&temporary, path) + .with_context(|| format!("atomically publishing {artifact}"))?; #[cfg(unix)] fs::File::open(parent) .and_then(|directory| directory.sync_all()) - .context("syncing publication receipt directory")?; + .with_context(|| format!("syncing {artifact} directory"))?; Ok(()) })(); if result.is_err() { @@ -613,6 +852,12 @@ pub trait Forge { ) -> ReviewPublicationReceipt { untracked_review_publication_receipt("forge", envelope, &snapshot.head_sha) } + async fn build_publication_plan( + &self, + _request: GitHubPublicationPlanRequest<'_>, + ) -> Result { + anyhow::bail!("publication planning is not supported for this forge") + } async fn fetch_pr_meta(&self) -> Result; /// Unified diff of the immutable snapshot returned by `fetch_pr_meta`. /// Implementations must not re-read a moving PR head or target tip. @@ -1291,6 +1536,120 @@ mod tests { } } + fn sample_publication_plan( + head_sha: &str, + review_body: &str, + finding_id: &str, + gate_conclusion: &str, + ) -> GitHubPublicationPlan { + GitHubPublicationPlan::new( + PublicationPlanRepository { + id: 42, + full_name: "acme/api".into(), + }, + 7, + PublicationPlanSnapshot { + head_sha: head_sha.into(), + merge_base_sha: "bbbbbbbb".into(), + target_sha: "cccccccc".into(), + pull_request_title_sha256: "sha256:title".into(), + pull_request_body_sha256: "sha256:body".into(), + }, + "github-review-v2:receipt".into(), + vec![ + PublicationPlanOperation { + operation_key: "review-key".into(), + desired: PublicationPlanOperationKind::CompositeReview { + action: PublicationPlanReviewAction::Publish, + receipt_id: "github-review-v2:receipt".into(), + initial_attempt: Some(PublicationPlanReviewAttempt { + body: review_body.into(), + comments: vec![], + }), + placement_fallback_attempt: None, + summary_fallback_attempt: None, + findings: vec![PublicationPlanFinding { + finding_id: finding_id.into(), + stable_identity: true, + path: "src/lib.rs".into(), + line: 10, + end_line: None, + initial_outcome: FindingPublicationOutcome::Inline, + fallback_intent: vec![PublicationPlanFindingFallback::FileComment], + }], + }, + }, + PublicationPlanOperation { + operation_key: "gate-key".into(), + desired: PublicationPlanOperationKind::GateCheck { + name: "postil/gate".into(), + head_sha: head_sha.into(), + conclusion: gate_conclusion.into(), + title: "Merge gate".into(), + summary: "Gate summary".into(), + details_url: None, + }, + }, + ], + ) + .unwrap() + } + + #[test] + fn publication_plan_is_private_atomic_deterministic_and_intent_bound() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("publication-plan.json"); + let plan = sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "failure"); + write_github_publication_plan(&path, &plan).unwrap(); + let first = std::fs::read(&path).unwrap(); + write_github_publication_plan(&path, &plan).unwrap(); + let second = std::fs::read(&path).unwrap(); + assert_eq!(first, second); + assert_eq!( + serde_json::from_slice::(&first).unwrap(), + plan + ); + assert!(plan.intent_digest.starts_with("sha256:")); + assert_eq!(plan.intent_digest.len(), "sha256:".len() + 64); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } + + for changed in [ + sample_publication_plan("dddddddd", "Review body", "finding-1", "failure"), + sample_publication_plan("aaaaaaaa", "Changed body", "finding-1", "failure"), + sample_publication_plan("aaaaaaaa", "Review body", "finding-2", "failure"), + sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "success"), + ] { + assert_ne!(changed.intent_digest, plan.intent_digest); + } + } + + #[cfg(unix)] + #[test] + fn publication_plan_refuses_symlink_and_directory_targets() { + use std::os::unix::fs::symlink; + + let directory = tempfile::tempdir().unwrap(); + let plan = sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "failure"); + let destination = directory.path().join("destination.json"); + std::fs::write(&destination, "unchanged").unwrap(); + let link = directory.path().join("publication-plan.json"); + symlink(&destination, &link).unwrap(); + let error = write_github_publication_plan(&link, &plan).unwrap_err(); + assert!(error.to_string().contains("must not be a symlink")); + assert_eq!(std::fs::read_to_string(destination).unwrap(), "unchanged"); + + let target_directory = directory.path().join("target-directory"); + std::fs::create_dir(&target_directory).unwrap(); + assert!(write_github_publication_plan(&target_directory, &plan).is_err()); + } + #[tokio::test] async fn source_snapshot_streams_above_page_limit_with_complete_length() { let server = MockServer::start().await; diff --git a/src/main.rs b/src/main.rs index f17db4e..1d69f48 100644 --- a/src/main.rs +++ b/src/main.rs @@ -54,6 +54,7 @@ async fn dispatch(cli: Cli) -> anyhow::Result { publish, no_post, defer_gate_check, + publication_plan_output, } => { let local_mode = staged || base.is_some() || diff_file.is_some(); let kind = match forge { @@ -89,6 +90,7 @@ async fn dispatch(cli: Cli) -> anyhow::Result { bounded, no_post: !publication_enabled(publish, no_post)?, defer_gate_check, + publication_plan_output, }) .await } diff --git a/src/review.rs b/src/review.rs index e5c9c3d..1c612c8 100644 --- a/src/review.rs +++ b/src/review.rs @@ -391,6 +391,7 @@ pub struct ReviewArgs { pub bounded: bool, pub no_post: bool, pub defer_gate_check: bool, + pub publication_plan_output: Option, } impl ReviewArgs { @@ -487,6 +488,26 @@ pub async fn run(args: ReviewArgs) -> Result { if args.output_file.is_some() && args.resolved_output_format().is_none() { return Err(anyhow!("--output-file requires --output or --output-json")); } + if args.publication_plan_output.is_some() { + anyhow::ensure!( + args.forge == ForgeKind::GitHub + && args.repo.is_some() + && args.pr.is_some() + && args.sha.is_some() + && args.base_sha.is_some() + && !args.staged + && args.base.is_none() + && args.diff_file.is_none(), + "--publication-plan-output requires a remote GitHub pull request with explicit --repo, --pr, --sha, and --base-sha" + ); + anyhow::ensure!( + args.no_post + && args.check_run_id.is_none() + && args.gate_check_run_id.is_none() + && !args.defer_gate_check, + "--publication-plan-output cannot be combined with forge mutation options" + ); + } let mut cfg = Config::load(&cwd, args.config.as_deref())?; if let Some(m) = &args.model { cfg.model = m.clone(); @@ -696,24 +717,8 @@ async fn run_remote( envelope, publication_diff, } = review; + let (review_state, gate_state) = remote_check_states(&envelope); let check_completion = if let Some((a, g)) = &checks { - let gate_state = if envelope.gate.failing { - CheckState::Failure - } else { - CheckState::Success - }; - // `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 review_state = if operational { - CheckState::Failure - } else { - CheckState::Success - }; complete_remote_checks( forge, a, @@ -768,17 +773,13 @@ async fn run_remote( Some(&meta), review_started.elapsed().as_millis() as u64, ); + let (review_state, gate_state) = remote_check_states(&envelope); let check_completion = if let Some((a, g)) = &checks { - let gate_state = if envelope.gate.failing { - CheckState::Failure - } else { - CheckState::Success - }; complete_remote_checks( forge, a, g, - CheckState::Failure, + review_state, (!args.defer_gate_check).then_some(gate_state), &envelope, &meta, @@ -834,6 +835,24 @@ fn strict_hosted_github_publication(args: &ReviewArgs) -> bool { crate::config::hosted_mode() && args.forge == ForgeKind::GitHub && !args.no_post } +fn remote_check_states(envelope: &Envelope) -> (CheckState, CheckState) { + let operational = envelope.findings.iter().any(|finding| { + finding.path == crate::envelope::OPERATIONAL_PATH + || finding.path == crate::envelope::PROVIDER_PATH + }); + let advisory = if operational { + CheckState::Failure + } else { + CheckState::Success + }; + let gate = if envelope.gate.failing { + CheckState::Failure + } else { + CheckState::Success + }; + (advisory, gate) +} + async fn require_current_snapshot( forge: &F, expected: &PrMeta, @@ -2786,6 +2805,40 @@ async fn finish( output::print_pretty(&envelope); } + let duplicate_of_baseline = load_baseline(args) + .ok() + .is_some_and(|baseline| visible_finding_sets_equal(&baseline, &envelope.findings)); + let intentional_no_comment = crate::forge::only_operational_findings(&envelope.findings) + || (!envelope.findings.is_empty() && envelope.findings.iter().all(filter::is_carried)); + let should_comment = (!envelope.silent + || matches!(cfg.on_clean, crate::config::OnClean::Comment)) + && !duplicate_of_baseline + && !intentional_no_comment; + + if let Some(path) = &args.publication_plan_output { + let forge = forge.context("publication planning requires a GitHub forge")?; + let PublicationContext { + snapshot: expected_snapshot, + diff: publication_diff, + } = publication.context("publication planning is missing its immutable PR snapshot")?; + let (advisory, gate) = remote_check_states(&envelope); + let plan = forge + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + envelope: &envelope, + snapshot: expected_snapshot, + publication_diff, + should_comment, + duplicate_of_baseline, + annotate_findings: cfg.finding_presentation + == FindingPresentation::CheckAnnotations, + advisory, + gate, + }) + .await?; + crate::forge::write_github_publication_plan(path, &plan)?; + return Ok(if envelope.gate.failing { 1 } else { 0 }); + } + if let Some(forge) = forge && !args.no_post { @@ -2807,15 +2860,6 @@ async fn finish( 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)); - let intentional_no_comment = crate::forge::only_operational_findings(&envelope.findings) - || (!envelope.findings.is_empty() && envelope.findings.iter().all(filter::is_carried)); - let should_comment = (!envelope.silent - || matches!(cfg.on_clean, crate::config::OnClean::Comment)) - && !duplicate_of_baseline - && !intentional_no_comment; if !should_comment { let mut receipt = forge.plan_review_publication(&envelope, expected_snapshot); if duplicate_of_baseline { diff --git a/tests/e2e.rs b/tests/e2e.rs index e36c15e..059b1e8 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -1233,6 +1233,164 @@ async fn remote_review_without_publish_never_mutates_github_even_in_hosted_ci() ); } +#[tokio::test] +async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_forge() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + let directory = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(directory.path(), true); + let first_plan = directory.path().join("publication-plan-1.json"); + let second_plan = directory.path().join("publication-plan-2.json"); + + for plan_path in [&first_plan, &second_plan] { + let envelope_path = plan_path.with_extension("envelope.json"); + postil() + .current_dir(directory.path()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .env("POSTIL_EXPECTED_GITHUB_REPO_ID", "42") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + ]) + .arg(plan_path) + .args(["--output", "json", "--output-file"]) + .arg(envelope_path) + .assert() + .code(0); + } + + let first = std::fs::read(&first_plan).unwrap(); + let second = std::fs::read(&second_plan).unwrap(); + assert_eq!(first, second); + let plan: Value = serde_json::from_slice(&first).unwrap(); + assert_eq!(plan["version"], 1); + assert_eq!(plan["forge"], "github"); + assert_eq!(plan["repository"]["id"], 42); + assert_eq!(plan["repository"]["fullName"], "acme/api"); + assert_eq!(plan["pullRequestNumber"], 7); + assert_eq!(plan["reviewedSnapshot"]["headSha"], "aaaaaaaa"); + assert_eq!(plan["reviewedSnapshot"]["mergeBaseSha"], "bbbbbbbb"); + assert_eq!(plan["reviewedSnapshot"]["targetSha"], "bbbbbbbb"); + assert!( + plan["intentDigest"] + .as_str() + .is_some_and(|digest| digest.starts_with("sha256:") && digest.len() == 71) + ); + assert_eq!(plan["operations"][0]["kind"], "compositeReview"); + assert_eq!(plan["operations"][0]["action"], "publish"); + assert_eq!(plan["operations"][1]["kind"], "advisoryCheck"); + assert_eq!(plan["operations"][1]["conclusion"], "success"); + assert_eq!(plan["operations"][2]["kind"], "gateCheck"); + assert_eq!(plan["operations"][2]["conclusion"], "success"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + std::fs::metadata(&first_plan).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } + + let planning_requests = server.received_requests().await.unwrap(); + assert!( + planning_requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + }), + "publication planning attempted a GitHub mutation: {planning_requests:?}" + ); + + mount_successful_hosted_check_patches(&server).await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "id": 77, + "commit_id": "aaaaaaaa" + }))) + .expect(1) + .mount(&server) + .await; + postil() + .current_dir(directory.path()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--publish", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--check-run-id", + "901", + "--gate-check-run-id", + "902", + "--output", + "json", + "--output-file", + ]) + .arg(directory.path().join("published-envelope.json")) + .assert() + .code(0); + let all_requests = server.received_requests().await.unwrap(); + assert!(all_requests.iter().any(|request| { + matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + })); +} + +#[test] +fn publication_plan_rejects_non_github_and_local_review_modes_before_io() { + let directory = tempfile::tempdir().unwrap(); + for extra in [vec!["--forge", "gitlab"], vec!["--staged"]] { + let mut command = postil(); + command.current_dir(directory.path()).args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + ]); + command + .args(extra) + .assert() + .code(2) + .stderr(predicates::str::contains( + "requires a remote GitHub pull request", + )); + } +} + #[test] fn publication_looking_environment_variables_are_rejected() { for variable in ["POSTIL_PUBLISH", "POSTIL_NO_POST"] { From de4e2c9e255393766aa657fbfacfbafddfcc05e5 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 14 Aug 2026 11:21:25 +0000 Subject: [PATCH 6/7] fix(review): harden publication plan durability --- src/cli.rs | 157 +++- src/forge/github.rs | 2194 +++++++++++++++++++++++++++++++++++++++---- src/forge/mod.rs | 1386 +++++++++++++++++++++++++-- src/main.rs | 17 +- src/review.rs | 47 +- tests/e2e.rs | 232 ++++- 6 files changed, 3717 insertions(+), 316 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index dab4cca..3014326 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -56,6 +56,12 @@ pub enum ForgeArg { #[derive(Subcommand)] #[allow(clippy::large_enum_variant)] // Review carries the full flag set by design. pub enum Command { + /// Probe machine-readable CLI capabilities without external access. + Capabilities { + /// Require and print an exact publication-plan contract identifier. + #[arg(long, value_name = "IDENTIFIER")] + publication_plan_contract: String, + }, /// Print immutable qualification metadata embedded in this binary. #[command(hide = true)] QualificationMetadata, @@ -146,7 +152,14 @@ pub enum Command { long, hide = true, value_name = "PATH", - requires_all = ["repo", "pr", "sha", "base_sha"], + requires_all = [ + "repo", + "pr", + "sha", + "base_sha", + "publication_generation", + "publication_input_identity" + ], conflicts_with_all = [ "publish", "no_post", @@ -156,6 +169,22 @@ pub enum Command { ] )] publication_plan_output: Option, + /// Service-owned generation identity for publication planning. + #[arg( + long, + hide = true, + value_name = "IDENTITY", + requires = "publication_plan_output" + )] + publication_generation: Option, + /// Service-supplied immutable input identity for publication planning. + #[arg( + long, + hide = true, + value_name = "SHA256", + requires = "publication_plan_output" + )] + publication_input_identity: Option, }, /// Reply to an @postil mention on a pull request or issue (interactive bot). Respond { @@ -229,6 +258,15 @@ pub enum Command { Logout, } +pub fn publication_plan_contract_capability(required: &str) -> anyhow::Result<&'static str> { + anyhow::ensure!( + required == crate::forge::GITHUB_PUBLICATION_PLAN_CONTRACT, + "unsupported publication-plan contract {required:?}; supported contract: {}", + crate::forge::GITHUB_PUBLICATION_PLAN_CONTRACT + ); + Ok(crate::forge::GITHUB_PUBLICATION_PLAN_CONTRACT) +} + #[derive(Subcommand)] pub enum HookAction { /// Install a pre-push hook that reviews outgoing commits. @@ -242,7 +280,33 @@ pub enum HookAction { mod tests { use clap::{CommandFactory, Parser}; - use super::{Cli, Command}; + use super::{Cli, Command, publication_plan_contract_capability}; + + const PUBLICATION_INPUT_IDENTITY: &str = + "sha256:1111111111111111111111111111111111111111111111111111111111111111"; + + #[test] + fn publication_plan_capability_requires_the_exact_contract() { + let parsed = Cli::try_parse_from([ + "postil", + "capabilities", + "--publication-plan-contract", + "github-publication-v1", + ]) + .unwrap(); + let Command::Capabilities { + publication_plan_contract, + } = parsed.command + else { + panic!("expected capabilities command"); + }; + assert_eq!(publication_plan_contract, "github-publication-v1"); + assert_eq!( + publication_plan_contract_capability(&publication_plan_contract).unwrap(), + "github-publication-v1" + ); + assert!(publication_plan_contract_capability("github-publication-v2").is_err()); + } #[test] fn review_bounded_is_an_explicit_action_flag_with_concise_help() { @@ -386,10 +450,16 @@ mod tests { "bbbbbbbb", "--publication-plan-output", "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, ]) .unwrap(); let Command::Review { publication_plan_output, + publication_generation, + publication_input_identity, publish, .. } = parsed.command @@ -400,6 +470,11 @@ mod tests { publication_plan_output.as_deref(), Some(std::path::Path::new("publication-plan.json")) ); + assert_eq!(publication_generation.as_deref(), Some("1")); + assert_eq!( + publication_input_identity.as_deref(), + Some(PUBLICATION_INPUT_IDENTITY) + ); assert!(!publish); let help = Cli::command() @@ -428,6 +503,10 @@ mod tests { "bbbbbbbb", "--publication-plan-output", "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, ]; arguments.extend(extra); assert!(Cli::try_parse_from(arguments).is_err()); @@ -445,6 +524,80 @@ mod tests { "aaaaaaaa", "--publication-plan-output", "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, + ]) + .is_err() + ); + assert!( + Cli::try_parse_from([ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + "--publication-generation", + "1", + ]) + .is_err() + ); + assert!( + Cli::try_parse_from([ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, + ]) + .is_err() + ); + assert!( + Cli::try_parse_from([ + "postil", + "review", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, + ]) + .is_err() + ); + assert!( + Cli::try_parse_from([ + "postil", + "review", + "--repo", + "postil-dev/postil", + "--pr", + "1", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, ]) .is_err() ); diff --git a/src/forge/github.rs b/src/forge/github.rs index 5e824cb..fa65113 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -12,13 +12,22 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use super::{ CheckRunIds, CheckState, FindingPublicationOutcome, FindingPublicationReceipt, Forge, - GitHubPublicationPlan, GitHubPublicationPlanRequest, PrMeta, PublicationPlanCheckAnnotation, - PublicationPlanFallbackActivation, PublicationPlanFileComment, PublicationPlanFinding, - PublicationPlanFindingFallback, PublicationPlanOperation, PublicationPlanOperationKind, - PublicationPlanRepository, PublicationPlanReviewAction, PublicationPlanReviewAttempt, - PublicationPlanReviewComment, PublicationPlanSnapshot, ReviewPublicationReceipt, - ReviewPublicationSummary, SummaryContext, ThreadKind, check_summary, check_title, - only_operational_findings, valid_details_url, + GitHubPublicationPlan, GitHubPublicationPlanIdentity, GitHubPublicationPlanRequest, PrMeta, + PublicationPlanActivationCondition, PublicationPlanCheckAnnotation, + PublicationPlanCheckConclusion, PublicationPlanCheckStatus, PublicationPlanDuplicateProvenance, + PublicationPlanFileComment, PublicationPlanFinding, PublicationPlanFindingFallback, + PublicationPlanFindingReconciliation, PublicationPlanGateAnalysis, + PublicationPlanGateOwnership, PublicationPlanLifecycleReceipt, + PublicationPlanMarkerAbsenceGuard, PublicationPlanOperation, + PublicationPlanOperationActivation, PublicationPlanOperationKind, + PublicationPlanOperationReconciliation, PublicationPlanOperationResultField, + PublicationPlanOperationResultReference, PublicationPlanPlacementClassification, + PublicationPlanRepository, PublicationPlanReviewAttemptKind, PublicationPlanReviewComment, + PublicationPlanReviewCreateOutcome, PublicationPlanReviewCreatePayload, + PublicationPlanReviewSummaryCase, PublicationPlanSnapshot, PublicationPlanTerminalOperation, + PublicationPlanTerminalOutcome, ReviewPublicationReceipt, ReviewPublicationSummary, + SummaryContext, ThreadKind, check_summary, check_title, only_operational_findings, + valid_details_url, }; use crate::diff::{Diff, DiffIndex, DiffSnapshot, DiffSpool, WorkspaceBudget}; use crate::envelope::{Envelope, Finding, Severity}; @@ -284,12 +293,18 @@ impl GitHub { async fn reconcile_published_finding_markers( &self, receipt: &mut ReviewPublicationReceipt, + envelope: &Envelope, head_sha: &str, ) -> std::collections::HashMap { let published = self.published_finding_comments(head_sha).await; for publication in &mut receipt.findings { - let marker = finding_marker(&publication.finding_id); - let Some(comment) = published.get(&marker) else { + let Ok(finding) = publication_plan_finding(envelope, &publication.finding_id) else { + continue; + }; + let Some(comment) = finding_marker_candidates(finding) + .iter() + .find_map(|marker| published.get(marker)) + else { continue; }; publication.initial_outcome = FindingPublicationOutcome::Carried; @@ -536,7 +551,12 @@ impl GitHub { unreachable!("bounded check-run create loop always returns") } - async fn find_review(&self, marker: &str, head_sha: &str) -> Result> { + async fn find_review( + &self, + markers: &[String], + head_sha: &str, + allow_correlated_legacy_marker: bool, + ) -> Result> { const PAGE_SIZE: usize = 100; const MAX_PAGES: usize = 20; for page in 1..=MAX_PAGES { @@ -560,10 +580,13 @@ impl GitHub { let page_len = reviews.len(); if let Some(review) = reviews.into_iter().find(|review| { review.commit_id.as_deref() == Some(head_sha) - && review - .body - .as_deref() - .is_some_and(|body| body.contains(marker)) + && review.body.as_deref().is_some_and(|body| { + markers.iter().any(|marker| body.contains(marker)) + || (allow_correlated_legacy_marker + && review_marker_in(body).is_some_and(|marker| { + marker.starts_with("", + crate::repository_search::hex_digest(hash) + ) +} + +fn legacy_finding_marker(finding_id: &str) -> String { let hash = Sha256::digest(finding_id.as_bytes()); format!( "", @@ -3227,20 +4190,52 @@ fn finding_marker(finding_id: &str) -> String { ) } +fn finding_marker_candidates(finding: &Finding) -> Vec { + let (finding_id, _) = finding_receipt_id(finding); + let legacy_id = legacy_finding_receipt_id(finding); + let mut markers = vec![ + finding_marker(&finding_id), + legacy_finding_marker(&finding_id), + ]; + if legacy_id != finding_id { + markers.push(legacy_finding_marker(&legacy_id)); + } + markers +} + fn review_marker(receipt_id: &str) -> String { let mut digest = Sha256::new(); digest.update(receipt_id.as_bytes()); let hash = digest.finalize(); + format!( + "", + crate::repository_search::hex_digest(hash) + ) +} + +fn legacy_review_marker(receipt_id: &str) -> String { + let hash = Sha256::digest(receipt_id.as_bytes()); format!( "", hash[0], hash[1], hash[2], hash[3], hash[4], hash[5] ) } +fn review_marker_in(body: &str) -> Option { + let start = ["")? + start + "-->".len(); + Some(body[start..end].to_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()) } @@ -3253,6 +4248,28 @@ fn append_marker(body: &str, marker: &str) -> String { } } +fn without_finding_marker(body: &str) -> &str { + let Some(marker) = finding_marker_in(body) else { + return body.trim_end(); + }; + body.strip_suffix(&marker) + .map(str::trim_end) + .unwrap_or_else(|| body.trim_end()) +} + +fn publication_plan_body_digest(body: &str) -> String { + publication_plan_text_digest(body) +} + +fn published_comment_for_finding<'a>( + published: &'a std::collections::HashMap, + finding: &Finding, +) -> Option<&'a PublishedReviewComment> { + finding_marker_candidates(finding) + .iter() + .find_map(|marker| published.get(marker)) +} + const MAX_REVIEW_BODY_BYTES: usize = 60_000; const OVERSIZED_REVIEW_MESSAGE: &str = "Review summary omitted because it exceeds GitHub's size limit."; @@ -3305,7 +4322,12 @@ mod tests { use crate::envelope::{ Envelope, Finding, Gate, Kind, Severity, SuppressedFinding, SuppressionReason, Usage, }; - use crate::forge::{CheckRunIds, CheckState, FindingPublicationOutcome, Forge, PrMeta}; + use crate::forge::{ + CheckRunIds, CheckState, FindingPublicationOutcome, Forge, PrMeta, + PublicationPlanActivationCondition, PublicationPlanCheckStatus, PublicationPlanOperation, + PublicationPlanOperationActivation, PublicationPlanOperationKind, + PublicationPlanOperationReconciliation, + }; use reqwest::header::{HeaderMap, HeaderValue}; use std::sync::{ Arc, @@ -3315,6 +4337,24 @@ mod tests { use wiremock::matchers::{method, path, path_regex, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; + const PUBLICATION_INPUT_IDENTITY: &str = + "sha256:1111111111111111111111111111111111111111111111111111111111111111"; + + fn publication_plan_key_scope<'a>( + head_sha: &'a str, + input_identity: &'a str, + review_output_digest: &'a str, + ) -> super::PublicationPlanKeyScope<'a> { + super::PublicationPlanKeyScope { + repository_id: "42", + pull_request_number: "7", + head_sha, + controller_generation: "1", + input_identity, + review_output_digest, + } + } + fn delivery_envelope(head_sha: &str, base_sha: &str) -> Envelope { Envelope { version: 1, @@ -4126,6 +5166,274 @@ mod tests { )); } + #[test] + fn publication_plan_operation_keys_match_every_key_safe_contract_shape() { + let expected = [ + ( + "initial-review-create", + "05d5806e72114f105b5b1e2809be8651811a3e527d89f7455c20dedbf32f24ce", + ), + ( + "relocated-review-create", + "178636b4f224fdfb105833cde04ca2f5b1cf74bf407b9ab2dd23938198467058", + ), + ( + "summary-review-create", + "7ca44380841305baacfac07805bc41c4b308f95acd0278b3e5ed3bfca4f7d8a0", + ), + ( + "file-comment-fallback", + "6e61767bbe33ef6d754950c5bcce236fbeb92df235005db907731caa96e047dc", + ), + ( + "finding-comment-update", + "875e393c65ef65394d4e73148f3369bad46f1d23d064cb906b3a8a69feb38d2f", + ), + ( + "review-summary-update", + "e575a792bbe3c702f35bd4218263a9cf160b841c1e898c8045802530d56cac5f", + ), + ( + "advisory-check-create", + "3be1458de77b6d90c70ead018e3897dddf217dc3bb5c0a68ba51b3f4129556d4", + ), + ( + "advisory-check-complete", + "2c9ec6990843d0012a5489aa99a2fec170326e1a36f48a38fc51a73b42af33d7", + ), + ]; + for (kind, (expected_kind, expected_digest)) in super::PublicationPlanOperationKeyKind::ALL + .into_iter() + .zip(expected) + { + assert_eq!(kind.as_str(), expected_kind); + let finding_id = matches!( + kind, + super::PublicationPlanOperationKeyKind::FileCommentFallback + | super::PublicationPlanOperationKeyKind::FindingCommentUpdate + ) + .then_some("finding-1"); + let key = super::publication_plan_operation_key( + publication_plan_key_scope( + "head-1", + PUBLICATION_INPUT_IDENTITY, + "sha256:2222222222222222222222222222222222222222222222222222222222222222", + ), + kind, + finding_id, + ); + assert_eq!( + key, + format!("github-publication-v1:{expected_kind}:sha256:{expected_digest}") + ); + assert!(!key.contains('/')); + } + } + + #[test] + fn advisory_check_completion_depends_on_creation_and_terminal_review_work() { + let operation = |operation_key: &str| { + PublicationPlanOperation::new( + 0, + operation_key.into(), + vec![], + PublicationPlanOperationActivation { + any_of: vec![PublicationPlanActivationCondition::Always], + }, + PublicationPlanOperationReconciliation { + logical_identity: operation_key.into(), + markers: vec![], + observed_remote_id: None, + exclusive: true, + }, + PublicationPlanOperationKind::AdvisoryCheckCreate { + name: "postil/review".into(), + head_sha: "aaaaaaaaaaaa".into(), + status: PublicationPlanCheckStatus::InProgress, + external_id: "postil:postil/review:aaaaaaaaaaaa".into(), + details_url: None, + }, + ) + .unwrap() + }; + let preceding = vec![ + operation("advisory-create"), + operation("finding-update"), + operation("file-fallback"), + ]; + + assert_eq!( + super::publication_plan_advisory_completion_dependencies( + &preceding, + "advisory-create", + None, + ), + vec!["advisory-create", "finding-update", "file-fallback"] + ); + assert_eq!( + super::publication_plan_advisory_completion_dependencies( + &preceding, + "advisory-create", + Some("review-summary-update"), + ), + vec!["advisory-create", "review-summary-update"] + ); + } + + #[test] + fn same_head_rereviews_bind_remote_identity_to_generation_and_content() { + let envelope = delivery_envelope_with_findings( + "aaaaaaaaaaaa", + "cccccccccccc", + vec![publication_finding( + "finding-1", + "src/lib.rs", + "Original finding body.", + )], + ); + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + let receipt = super::planned_review_receipt(&envelope, &snapshot.head_sha); + let identity = |generation: &str, snapshot: &PrMeta, envelope: &Envelope, gate| { + super::publication_plan_review_output_digest(super::PublicationPlanReviewOutputInput { + controller_generation: generation, + input_identity: PUBLICATION_INPUT_IDENTITY, + repository_id: "42", + pull_request_number: "7", + snapshot, + envelope, + receipt: &receipt, + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate, + details_url: None, + }) + .unwrap() + }; + let baseline = identity("1", &snapshot, &envelope, CheckState::Success); + let mut changed_title = snapshot.clone(); + changed_title.title = "Changed title".into(); + let mut changed_body = snapshot.clone(); + changed_body.body = "Changed pull request body".into(); + let mut changed_finding = envelope.clone(); + changed_finding.findings[0].body = "Changed finding body.".into(); + let mut changed_confidence = envelope.clone(); + changed_confidence.findings[0].confidence = 0.73; + let variants = [ + identity("2", &snapshot, &envelope, CheckState::Success), + identity("1", &changed_title, &envelope, CheckState::Success), + identity("1", &changed_body, &envelope, CheckState::Success), + identity("1", &snapshot, &changed_finding, CheckState::Success), + identity("1", &snapshot, &changed_confidence, CheckState::Success), + identity("1", &snapshot, &envelope, CheckState::Failure), + ]; + for variant in variants { + assert_ne!(variant, baseline); + let variant_scope = publication_plan_key_scope( + &snapshot.head_sha, + PUBLICATION_INPUT_IDENTITY, + &variant, + ); + let baseline_scope = publication_plan_key_scope( + &snapshot.head_sha, + PUBLICATION_INPUT_IDENTITY, + &baseline, + ); + assert_ne!( + super::publication_plan_logical_review_identity(variant_scope), + super::publication_plan_logical_review_identity(baseline_scope) + ); + assert_ne!( + super::publication_plan_operation_key( + variant_scope, + super::PublicationPlanOperationKeyKind::InitialReviewCreate, + None, + ), + super::publication_plan_operation_key( + baseline_scope, + super::PublicationPlanOperationKeyKind::InitialReviewCreate, + None, + ) + ); + } + + let changed_input_identity = + "sha256:3333333333333333333333333333333333333333333333333333333333333333"; + let changed_input_output = + super::publication_plan_review_output_digest(super::PublicationPlanReviewOutputInput { + controller_generation: "1", + input_identity: changed_input_identity, + repository_id: "42", + pull_request_number: "7", + snapshot: &snapshot, + envelope: &envelope, + receipt: &receipt, + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Success, + details_url: None, + }) + .unwrap(); + assert_ne!(changed_input_output, baseline); + assert_ne!( + super::publication_plan_operation_key( + publication_plan_key_scope( + &snapshot.head_sha, + changed_input_identity, + &changed_input_output, + ), + super::PublicationPlanOperationKeyKind::InitialReviewCreate, + None, + ), + super::publication_plan_operation_key( + publication_plan_key_scope( + &snapshot.head_sha, + PUBLICATION_INPUT_IDENTITY, + &baseline, + ), + super::PublicationPlanOperationKeyKind::InitialReviewCreate, + None, + ) + ); + } + + #[test] + fn durable_markers_are_strong_and_accept_released_compatibility_shapes() { + let finding = publication_finding("finding-1", "src/lib.rs", "Body."); + let finding_markers = super::finding_marker_candidates(&finding); + assert_eq!(finding_markers.len(), 2); + assert!(finding_markers[0].starts_with("".len() + 64 + ); + assert!(finding_markers[1].starts_with("".len() + 12 + ); + + let current_receipt = super::planned_review_receipt( + &delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![finding]), + "aaaaaaaaaaaa", + ); + let current_review_marker = super::review_marker(¤t_receipt.receipt_id); + assert!(current_review_marker.starts_with("".len() + 64 + ); + let released_marker = super::legacy_review_marker("github-review-v2:0123456789ab"); + assert!(released_marker.starts_with("".len() + 12 + ); + } + async fn mount_current_delivery_snapshot(server: &MockServer) { Mock::given(method("GET")) .and(path("/repos/owner/repo/pulls/1")) @@ -4161,19 +5469,20 @@ mod tests { async fn publication_plan_reuses_review_checks_markers_and_the_placement_ladder() { let server = MockServer::start().await; mount_current_delivery_snapshot(&server).await; + mount_no_existing_review(&server).await; Mock::given(method("GET")) .and(path("/repos/owner/repo")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "id": 42, "full_name": "owner/repo" }))) - .expect(1) + .expect(2) .mount(&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!([]))) - .expect(1) + .expect(2) .mount(&server) .await; @@ -4218,8 +5527,15 @@ mod tests { reason: SuppressionReason::BelowConfidence, }); let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); - let plan = test_github(&server) + let planner = || { + let mut github = test_github(&server); + github.details_url = Some("https://postil.dev/orgs/acme/runs/run-1".into()); + github + }; + let plan = planner() .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "1", + input_identity: PUBLICATION_INPUT_IDENTITY, envelope: &envelope, snapshot: &snapshot, publication_diff: Some(&placement_diff()), @@ -4231,36 +5547,520 @@ mod tests { }) .await .unwrap(); + let mut reordered_envelope = envelope.clone(); + reordered_envelope.findings.reverse(); + reordered_envelope.resolved.reverse(); + reordered_envelope.suppressed_findings.reverse(); + let reordered_plan = planner() + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "1", + input_identity: PUBLICATION_INPUT_IDENTITY, + envelope: &reordered_envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Failure, + }) + .await + .unwrap(); + assert_eq!( + serde_json::to_vec(&plan).unwrap(), + serde_json::to_vec(&reordered_plan).unwrap(), + "provider finding order must not affect publication intent" + ); let serialized = serde_json::to_value(&plan).unwrap(); let operations = serialized["operations"].as_array().unwrap(); - assert_eq!(operations[0]["kind"], "compositeReview"); - assert_eq!(operations[0]["action"], "publish"); - assert_eq!(operations[0]["initialAttempt"]["comments"][0]["line"], 7); assert_eq!( - operations[0]["placementFallbackAttempt"]["comments"][0]["path"], + operations + .iter() + .map(|operation| operation["kind"].as_str().unwrap()) + .collect::>(), + vec![ + "advisoryCheckCreate", + "reviewCreate", + "reviewCreate", + "reviewCreate", + "fileCommentFallback", + "reviewSummaryUpdate", + "advisoryCheckComplete", + ] + ); + assert_eq!(operations[0]["ordinal"], 1); + assert_eq!(operations[1]["ordinal"], 2); + assert_eq!(operations[2]["ordinal"], 3); + assert_eq!(operations[0]["name"], "postil/review"); + assert_eq!(operations[0]["headSha"], "aaaaaaaaaaaa"); + assert_eq!(operations[0]["status"], "in_progress"); + assert_eq!( + operations[0]["externalId"], + "postil:run-1:postil/review:aaaaaaaaaaaa" + ); + assert_eq!( + operations[0]["detailsUrl"], + "https://postil.dev/orgs/acme/runs/run-1" + ); + assert_eq!(operations[1]["attempt"], "initial"); + assert_eq!(operations[2]["attempt"], "relocatedInline"); + assert_eq!(operations[3]["attempt"], "summaryOnly"); + assert_eq!(operations[1]["payload"]["comments"][0]["line"], 7); + assert_eq!( + operations[2]["payload"]["comments"][0]["path"], "src/lib.rs" ); - assert!(operations[0]["summaryFallbackAttempt"]["body"].is_string()); - let findings = operations[0]["findings"].as_array().unwrap(); + assert!(operations[3]["payload"]["body"].is_string()); + let logical_review_identity = operations[1]["logicalReviewIdentity"].as_str().unwrap(); + for operation in &operations[1..4] { + assert_eq!(operation["logicalReviewIdentity"], logical_review_identity); + assert_eq!( + operation["reconciliation"]["logicalIdentity"], + logical_review_identity + ); + assert_eq!(operation["reconciliation"]["exclusive"], true); + assert_eq!( + operation["reconciliation"]["markers"] + .as_array() + .unwrap() + .len(), + 2 + ); + } + assert_eq!( + operations[2]["activation"]["anyOf"][0]["condition"], + "semanticPlacementRejected" + ); + assert_eq!(operations[2]["activation"]["anyOf"][0]["httpStatus"], 422); + assert_eq!( + operations[2]["activation"]["anyOf"][0]["classification"], + "invalidReviewCommentPlacement" + ); + assert_eq!( + operations[2]["activation"]["anyOf"][0]["markerAbsence"], + operations[1]["activation"]["anyOf"][0]["guard"] + ); + assert_eq!( + operations[3]["activation"]["anyOf"][0]["markerAbsence"], + operations[1]["activation"]["anyOf"][0]["guard"] + ); + let findings = serialized["lifecycleReceipt"]["findings"] + .as_array() + .unwrap(); + assert_eq!( + findings + .iter() + .map(|finding| finding["findingId"].as_str().unwrap()) + .collect::>(), + vec![ + "active-1", + "carried-1", + "resolved-1", + "summary-1", + "suppressed-1" + ] + ); assert_eq!(findings[0]["initialOutcome"], "inline"); assert_eq!( findings[0]["fallbackIntent"], serde_json::json!(["relocatedInline", "fileComment"]) ); assert_eq!(findings[1]["initialOutcome"], "carried"); - assert_eq!(findings[2]["initialOutcome"], "summaryOnly"); - assert_eq!(findings[3]["initialOutcome"], "resolved"); + assert_eq!(findings[2]["initialOutcome"], "resolved"); + assert_eq!(findings[3]["initialOutcome"], "summaryOnly"); assert_eq!(findings[4]["initialOutcome"], "suppressed"); - assert_eq!(operations[1]["kind"], "fileCommentFallback"); - assert_eq!(operations[1]["findingId"], "active-1"); + assert_eq!(findings[4]["suppressionReason"], "belowConfidence"); + assert!( + serialized["lifecycleReceipt"]["digest"] + .as_str() + .unwrap() + .starts_with("sha256:") + ); + assert_eq!(operations[4]["findingId"], "active-1"); + assert_eq!( + operations[4]["activation"]["anyOf"][0]["condition"], + "semanticPlacementRejected" + ); + assert_eq!( + operations[4]["activation"]["anyOf"][1]["condition"], + "partialReviewObserved" + ); + assert_eq!( + operations[4]["activation"]["anyOf"][1]["findingMarkerAbsence"]["markers"], + operations[4]["reconciliation"]["markers"] + ); + assert_eq!( + operations[5]["terminalOperations"][0]["findingId"], + "active-1" + ); + assert_eq!( + operations[5]["terminalOperations"][0]["requiresRemoteId"], + true + ); + assert_eq!(operations[5]["cases"][0]["fileCommentCount"], 0); + assert_eq!(operations[5]["cases"][1]["fileCommentCount"], 1); + assert_eq!( + operations[5]["cases"][1]["selectedReviewOutcomes"], + serde_json::json!(["partialObserved"]) + ); + assert_eq!(operations[6]["name"], "postil/review"); + assert_eq!(operations[6]["conclusion"], "success"); + assert_eq!( + operations[6]["dependencies"], + serde_json::json!([ + operations[0]["operationKey"].clone(), + operations[5]["operationKey"].clone() + ]) + ); + assert_eq!( + operations[6]["createdCheck"]["dependencyOperationKey"], + operations[0]["operationKey"] + ); + assert_eq!(operations[6]["createdCheck"]["resultField"], "remoteId"); + assert_eq!(serialized["repository"]["id"], "42"); + assert_eq!(serialized["pullRequestNumber"], "1"); + assert_eq!(serialized["gateAnalysis"]["ownership"], "service"); + assert_eq!(serialized["gateAnalysis"]["authoritative"], false); + assert_eq!( + serialized["gateAnalysis"]["organizationGateModeRequired"], + true + ); + assert_eq!(serialized["gateAnalysis"]["name"], "postil/gate"); + assert_eq!(serialized["gateAnalysis"]["analyzedConclusion"], "failure"); + assert!(operations.iter().all(|operation| { + operation["kind"] != "gateCheck" && operation["name"] != "postil/gate" + })); + assert_eq!(plan.recompute_intent_digest().unwrap(), plan.intent_digest); + + let requests = server.received_requests().await.unwrap(); + assert!(requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + })); + } + + #[tokio::test] + async fn publication_plan_recovers_a_partial_review_without_an_alternative_create() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + let first = publication_finding( + "first", + "src/lib.rs", + "The first finding was observed in the partial review.", + ); + let second = publication_finding( + "second", + "src/lib.rs", + "The second finding is missing from the partial review.", + ); + let envelope = delivery_envelope_with_findings( + "aaaaaaaaaaaa", + "cccccccccccc", + vec![second, first.clone()], + ); + let legacy_review_marker = "".to_string(); + let first_body = super::initial_review_comment(&first)["body"] + .as_str() + .unwrap() + .to_string(); + Mock::given(method("GET")) + .and(path("/repos/owner/repo")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": 42, + "full_name": "owner/repo" + }))) + .expect(2) + .mount(&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": 501, + "body": first_body, + "commit_id": "aaaaaaaaaaaa" + }])), + ) + .expect(2) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!([{ + "id": 77, + "body": legacy_review_marker.clone(), + "commit_id": "aaaaaaaaaaaa" + }])), + ) + .expect(2) + .mount(&server) + .await; + + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + let build = || async { + test_github(&server) + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "3", + input_identity: PUBLICATION_INPUT_IDENTITY, + envelope: &envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Success, + }) + .await + .unwrap() + }; + let first_plan = build().await; + let second_plan = build().await; + assert_eq!( + serde_json::to_vec(&first_plan).unwrap(), + serde_json::to_vec(&second_plan).unwrap() + ); + let plan = serde_json::to_value(first_plan).unwrap(); + let findings = plan["lifecycleReceipt"]["findings"].as_array().unwrap(); + assert_eq!(findings[0]["findingId"], "first"); + assert_eq!(findings[0]["observedCommentId"], "501"); + assert_eq!(findings[0]["observedOutcome"], "inline"); + assert_eq!(findings[0]["reconciliation"], "retain"); + assert_eq!(findings[1]["findingId"], "second"); + assert!(findings[1].get("observedCommentId").is_none()); + assert_eq!(plan["lifecycleReceipt"]["observedReviewId"], "77"); + + let operations = plan["operations"].as_array().unwrap(); + let review_operations = &operations[1..4]; + let logical_identity = review_operations[0]["logicalReviewIdentity"].clone(); + let shared_markers = review_operations[0]["reconciliation"]["markers"].clone(); + assert!( + shared_markers + .as_array() + .unwrap() + .contains(&serde_json::json!(legacy_review_marker)) + ); + let exact_marker_guard = review_operations[0]["activation"]["anyOf"][0]["guard"].clone(); + for operation in review_operations { + assert_eq!(operation["logicalReviewIdentity"], logical_identity); + assert_eq!(operation["reconciliation"]["markers"], shared_markers); + assert_eq!(operation["reconciliation"]["observedRemoteId"], "77"); + assert_eq!(operation["reconciliation"]["exclusive"], true); + } + assert_eq!( + review_operations[1]["activation"]["anyOf"][0]["markerAbsence"], + exact_marker_guard + ); + assert_eq!( + review_operations[2]["activation"]["anyOf"][0]["markerAbsence"], + exact_marker_guard + ); + + let file_fallback = operations + .iter() + .find(|operation| operation["kind"] == "fileCommentFallback") + .unwrap(); + assert_eq!(file_fallback["findingId"], "second"); + let partial_conditions = file_fallback["activation"]["anyOf"] + .as_array() + .unwrap() + .iter() + .filter(|condition| condition["condition"] == "partialReviewObserved") + .collect::>(); + assert_eq!(partial_conditions.len(), 2); + for condition in partial_conditions { + assert_eq!(condition["reviewMarkers"], shared_markers); + assert_eq!( + condition["findingMarkerAbsence"]["markers"], + file_fallback["reconciliation"]["markers"] + ); + assert_eq!(condition["findingMarkerAbsence"]["required"], true); + } + + let final_summary = operations + .iter() + .find(|operation| operation["kind"] == "reviewSummaryUpdate") + .unwrap(); + assert!( + final_summary["dependencies"] + .as_array() + .unwrap() + .contains(&file_fallback["operationKey"]) + ); + assert_eq!( + final_summary["terminalOperations"] + .as_array() + .unwrap() + .len(), + 1 + ); assert_eq!( - operations[1]["activation"], - "compositeReviewPlacementFailed" + final_summary["terminalOperations"][0]["findingId"], + "second" + ); + let partial_case = final_summary["cases"] + .as_array() + .unwrap() + .iter() + .find(|case| { + case["selectedReviewOperationKey"] == review_operations[0]["operationKey"] + && case["selectedReviewOutcomes"] == serde_json::json!(["partialObserved"]) + && case["fileCommentCount"] == 1 + }) + .expect("partial observation has a truthful final summary case"); + assert!( + partial_case["body"] + .as_str() + .unwrap() + .contains("file-level") + ); + + let requests = server.received_requests().await.unwrap(); + assert!(requests.iter().all(|request| { + !matches!( + request.method, + wiremock::http::Method::POST + | wiremock::http::Method::PATCH + | wiremock::http::Method::PUT + | wiremock::http::Method::DELETE + ) + })); + } + + #[tokio::test] + async fn publication_plan_updates_stale_same_head_finding_content() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + let finding = publication_finding( + "stable-finding", + "src/lib.rs", + "The desired finding body is current.", + ); + let envelope = + delivery_envelope_with_findings("aaaaaaaaaaaa", "cccccccccccc", vec![finding.clone()]); + let receipt = super::planned_review_receipt(&envelope, "aaaaaaaaaaaa"); + let review_marker = super::review_marker(&receipt.receipt_id); + let stale_body = super::append_marker( + "**Stale finding title**\n\nThe old finding prose is obsolete.", + &super::finding_marker("stable-finding"), + ); + Mock::given(method("GET")) + .and(path("/repos/owner/repo")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": 42, + "full_name": "owner/repo" + }))) + .expect(2) + .mount(&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": 501, + "body": stale_body, + "commit_id": "aaaaaaaaaaaa" + }])), + ) + .expect(2) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1/reviews")) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!([{ + "id": 77, + "body": review_marker, + "commit_id": "aaaaaaaaaaaa" + }])), + ) + .expect(2) + .mount(&server) + .await; + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + let first_plan = test_github(&server) + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "4", + input_identity: PUBLICATION_INPUT_IDENTITY, + envelope: &envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Success, + }) + .await + .unwrap(); + let mut changed_envelope = envelope.clone(); + changed_envelope.findings[0].body = "A later same-head finding body is desired.".into(); + let changed_plan = test_github(&server) + .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "4", + input_identity: PUBLICATION_INPUT_IDENTITY, + envelope: &changed_envelope, + snapshot: &snapshot, + publication_diff: Some(&placement_diff()), + should_comment: true, + duplicate_of_baseline: false, + annotate_findings: false, + advisory: CheckState::Success, + gate: CheckState::Success, + }) + .await + .unwrap(); + + let first = serde_json::to_value(&first_plan).unwrap(); + let changed = serde_json::to_value(&changed_plan).unwrap(); + let finding_receipt = &first["lifecycleReceipt"]["findings"][0]; + assert_eq!(finding_receipt["findingId"], "stable-finding"); + assert_eq!(finding_receipt["observedCommentId"], "501"); + assert_eq!(finding_receipt["reconciliation"], "replace"); + assert_ne!( + finding_receipt["desiredBodySha256"], + finding_receipt["observedBodySha256"] + ); + let update = first["operations"] + .as_array() + .unwrap() + .iter() + .find(|operation| operation["kind"] == "findingCommentUpdate") + .unwrap(); + assert_eq!(update["findingId"], "stable-finding"); + assert_eq!(update["observedCommentId"], "501"); + assert_eq!(update["reconciliation"]["observedRemoteId"], "501"); + assert_eq!(update["expectedMarkers"].as_array().unwrap().len(), 2); + let final_summary = first["operations"] + .as_array() + .unwrap() + .iter() + .find(|operation| operation["kind"] == "reviewSummaryUpdate") + .unwrap(); + assert!( + final_summary["dependencies"] + .as_array() + .unwrap() + .contains(&update["operationKey"]) ); - assert_eq!(operations[2]["kind"], "advisoryCheck"); - assert_eq!(operations[2]["conclusion"], "success"); - assert_eq!(operations[3]["kind"], "gateCheck"); - assert_eq!(operations[3]["conclusion"], "failure"); + + let changed_update = changed["operations"] + .as_array() + .unwrap() + .iter() + .find(|operation| operation["kind"] == "findingCommentUpdate") + .unwrap(); + assert_eq!(first["inputIdentity"], changed["inputIdentity"]); + assert_ne!(first["reviewOutputDigest"], changed["reviewOutputDigest"]); + assert_ne!(update["operationKey"], changed_update["operationKey"]); + assert_ne!(update["desiredDigest"], changed_update["desiredDigest"]); + assert_ne!(first["intentDigest"], changed["intentDigest"]); let requests = server.received_requests().await.unwrap(); assert!(requests.iter().all(|request| { @@ -4298,6 +6098,8 @@ mod tests { let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); let plan = test_github(&server) .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: "1", + input_identity: PUBLICATION_INPUT_IDENTITY, envelope: &envelope, snapshot: &snapshot, publication_diff: Some(&placement_diff()), @@ -4313,18 +6115,20 @@ mod tests { .as_array() .unwrap() .clone(); - assert_eq!(operations.len(), 3); - assert_eq!(operations[0]["kind"], "compositeReview"); - assert_eq!(operations[0]["action"], "omit"); - assert!(operations[0].get("initialAttempt").is_none()); - assert_eq!( - operations[0]["findings"][0]["initialOutcome"], - "checkAnnotation" - ); - assert_eq!(operations[1]["kind"], "advisoryCheck"); + assert_eq!(operations.len(), 2); + assert_eq!(operations[0]["kind"], "advisoryCheckCreate"); + assert_eq!(operations[1]["kind"], "advisoryCheckComplete"); assert_eq!(operations[1]["annotations"][0]["path"], "src/lib.rs"); assert_eq!(operations[1]["annotations"][0]["startLine"], 7); - assert_eq!(operations[2]["kind"], "gateCheck"); + assert_eq!( + operations[1]["dependencies"], + serde_json::json!([operations[0]["operationKey"].clone()]) + ); + assert!( + operations + .iter() + .all(|operation| operation["kind"] != "gateCheck") + ); let requests = server.received_requests().await.unwrap(); assert!(requests.iter().all(|request| { @@ -4578,7 +6382,7 @@ mod tests { let body = initial["body"].as_str().unwrap(); assert!(body.contains(super::OVERSIZED_REVIEW_MESSAGE)); assert!(!body.contains("Review details")); - assert!(body.contains("".into(), + compatible_markers: vec![], + desired_body: "Finding body".into(), + desired_body_sha256: "sha256:body".into(), + observed_comment_id: None, + observed_body_sha256: None, + observed_outcome: None, + reconciliation: PublicationPlanFindingReconciliation::Create, + suppression_reason: None, + duplicate_provenance: PublicationPlanDuplicateProvenance::None, + }; + let lifecycle_receipt = PublicationPlanLifecycleReceipt::new( + format!("sha256:{}", "1".repeat(64)), + ReviewPublicationChannel::ReviewComments, + "github-review-v2:receipt".into(), + vec![], + None, + false, + vec![finding.clone()], + ) + .unwrap(); + let review_operation = PublicationPlanOperation::new( + 1, + "review-key".into(), + vec![], + PublicationPlanOperationActivation { + any_of: vec![PublicationPlanActivationCondition::Always], }, - 7, - PublicationPlanSnapshot { - head_sha: head_sha.into(), - merge_base_sha: "bbbbbbbb".into(), - target_sha: "cccccccc".into(), - pull_request_title_sha256: "sha256:title".into(), - pull_request_body_sha256: "sha256:body".into(), + PublicationPlanOperationReconciliation { + logical_identity: "review-identity".into(), + markers: vec!["".into()], + observed_remote_id: None, + exclusive: true, }, - "github-review-v2:receipt".into(), - vec![ - PublicationPlanOperation { - operation_key: "review-key".into(), - desired: PublicationPlanOperationKind::CompositeReview { - action: PublicationPlanReviewAction::Publish, - receipt_id: "github-review-v2:receipt".into(), - initial_attempt: Some(PublicationPlanReviewAttempt { - body: review_body.into(), - comments: vec![], - }), - placement_fallback_attempt: None, - summary_fallback_attempt: None, - findings: vec![PublicationPlanFinding { - finding_id: finding_id.into(), - stable_identity: true, - path: "src/lib.rs".into(), - line: 10, - end_line: None, - initial_outcome: FindingPublicationOutcome::Inline, - fallback_intent: vec![PublicationPlanFindingFallback::FileComment], - }], - }, + PublicationPlanOperationKind::ReviewCreate { + attempt: PublicationPlanReviewAttemptKind::Initial, + logical_review_identity: "review-identity".into(), + payload: PublicationPlanReviewCreatePayload { + commit_id: head_sha.into(), + event: "COMMENT".into(), + body: review_body.into(), + comments: vec![], }, - PublicationPlanOperation { - operation_key: "gate-key".into(), - desired: PublicationPlanOperationKind::GateCheck { - name: "postil/gate".into(), - head_sha: head_sha.into(), - conclusion: gate_conclusion.into(), - title: "Merge gate".into(), - summary: "Gate summary".into(), - details_url: None, - }, + }, + ) + .unwrap(); + GitHubPublicationPlan::new( + GitHubPublicationPlanIdentity { + controller_generation: "1".into(), + input_identity: format!("sha256:{}", "1".repeat(64)), + review_output_digest: format!("sha256:{}", "2".repeat(64)), + repository: PublicationPlanRepository { + id: "42".into(), + full_name: "acme/api".into(), }, - ], + pull_request_number: "7".into(), + reviewed_snapshot: PublicationPlanSnapshot { + head_sha: head_sha.into(), + merge_base_sha: "bbbbbbbb".into(), + target_sha: "cccccccc".into(), + pull_request_title_sha256: "sha256:title".into(), + pull_request_body_sha256: "sha256:body".into(), + }, + }, + lifecycle_receipt, + vec![review_operation], + PublicationPlanGateAnalysis { + ownership: PublicationPlanGateOwnership::Service, + authoritative: false, + organization_gate_mode_required: true, + name: "postil/gate".into(), + head_sha: head_sha.into(), + analyzed_conclusion: gate_conclusion, + title: "Merge gate".into(), + summary: "Gate summary".into(), + details_url: None, + }, ) .unwrap() } + fn rebuild_publication_plan( + template: &GitHubPublicationPlan, + operations: Vec, + ) -> Result { + GitHubPublicationPlan::new( + GitHubPublicationPlanIdentity { + controller_generation: template.controller_generation.clone(), + input_identity: template.input_identity.clone(), + review_output_digest: template.review_output_digest.clone(), + repository: template.repository.clone(), + pull_request_number: template.pull_request_number.clone(), + reviewed_snapshot: template.reviewed_snapshot.clone(), + }, + template.lifecycle_receipt.clone(), + operations, + template.gate_analysis.clone(), + ) + } + + fn dependent_publication_operation( + template: &PublicationPlanOperation, + operation_key: &str, + dependency_operation_key: &str, + ) -> PublicationPlanOperation { + let mut operation = template.clone(); + operation.ordinal = 2; + operation.operation_key = operation_key.into(); + operation.dependencies = vec![dependency_operation_key.into()]; + operation.activation = PublicationPlanOperationActivation { + any_of: vec![ + PublicationPlanActivationCondition::ReviewSelectionTerminal { + selected_review_operation_keys: vec![dependency_operation_key.into()], + }, + ], + }; + operation.reconciliation.logical_identity = format!("{operation_key}-identity"); + operation + } + + fn bounded_publication_operations( + template: &PublicationPlanOperation, + operation_count: usize, + dependency_edge_count: usize, + ) -> Vec { + let mut remaining_edges = dependency_edge_count; + let mut operations = Vec::with_capacity(operation_count); + for index in 0..operation_count { + let operation_key = format!("operation-{index}"); + let edge_count = remaining_edges.min(index); + let dependencies = (0..edge_count) + .map(|dependency_index| format!("operation-{dependency_index}")) + .collect(); + remaining_edges -= edge_count; + + let mut operation = template.clone(); + operation.ordinal = u32::try_from(index + 1).unwrap(); + operation.operation_key = operation_key.clone(); + operation.dependencies = dependencies; + operation.activation = PublicationPlanOperationActivation { + any_of: vec![PublicationPlanActivationCondition::Always], + }; + operation.reconciliation.logical_identity = format!("{operation_key}-identity"); + operations.push(operation); + } + assert_eq!(remaining_edges, 0, "fixture has enough dependency capacity"); + operations + } + #[test] fn publication_plan_is_private_atomic_deterministic_and_intent_bound() { let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("publication-plan.json"); - let plan = sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "failure"); + let plan = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ); write_github_publication_plan(&path, &plan).unwrap(); let first = std::fs::read(&path).unwrap(); write_github_publication_plan(&path, &plan).unwrap(); let second = std::fs::read(&path).unwrap(); assert_eq!(first, second); + let mut piped = Vec::new(); + write_github_publication_plan_to_writer(&mut piped, &plan).unwrap(); + assert_eq!(piped, first); + assert_eq!(piped.last(), Some(&b'\n')); assert_eq!( serde_json::from_slice::(&first).unwrap(), plan ); + let serialized: serde_json::Value = serde_json::from_slice(&first).unwrap(); + assert_eq!(serialized["repository"]["id"], "42"); + assert_eq!(serialized["pullRequestNumber"], "7"); + assert_eq!( + serialized["inputIdentity"], + format!("sha256:{}", "1".repeat(64)) + ); + assert_eq!( + serialized["reviewOutputDigest"], + format!("sha256:{}", "2".repeat(64)) + ); + assert_eq!(serialized["operationCount"], 1); + assert_eq!(plan.operation_count, plan.operations.len() as u32); + assert_eq!( + plan.recompute_operation_manifest_digest().unwrap(), + plan.operation_manifest_digest + ); assert!(plan.intent_digest.starts_with("sha256:")); assert_eq!(plan.intent_digest.len(), "sha256:".len() + 64); + assert_eq!(plan.recompute_intent_digest().unwrap(), plan.intent_digest); + assert_eq!( + plan.lifecycle_receipt.recompute_digest().unwrap(), + plan.lifecycle_receipt.digest + ); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -1621,13 +2345,466 @@ mod tests { } for changed in [ - sample_publication_plan("dddddddd", "Review body", "finding-1", "failure"), - sample_publication_plan("aaaaaaaa", "Changed body", "finding-1", "failure"), - sample_publication_plan("aaaaaaaa", "Review body", "finding-2", "failure"), - sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "success"), + sample_publication_plan( + "dddddddd", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ), + sample_publication_plan( + "aaaaaaaa", + "Changed body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ), + sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-2", + PublicationPlanCheckConclusion::Failure, + ), + sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Success, + ), ] { assert_ne!(changed.intent_digest, plan.intent_digest); } + + let mut stored_digest_only = plan.clone(); + stored_digest_only.intent_digest = "sha256:untrusted".into(); + assert_eq!( + stored_digest_only.recompute_intent_digest().unwrap(), + plan.intent_digest, + "the stored intent digest is outside its own canonical boundary" + ); + let mut repository_changed = plan.clone(); + repository_changed.repository.id = "43".into(); + assert_ne!( + repository_changed.recompute_intent_digest().unwrap(), + plan.intent_digest + ); + let mut pull_request_changed = plan.clone(); + pull_request_changed.pull_request_number = "8".into(); + assert_ne!( + pull_request_changed.recompute_intent_digest().unwrap(), + plan.intent_digest + ); + let mut lifecycle_changed = plan.clone(); + lifecycle_changed.lifecycle_receipt.findings[0].desired_body = "Changed finding".into(); + assert_ne!( + lifecycle_changed + .lifecycle_receipt + .recompute_digest() + .unwrap(), + plan.lifecycle_receipt.digest + ); + assert_ne!( + lifecycle_changed.recompute_intent_digest().unwrap(), + plan.intent_digest + ); + let mut operation_changed = plan.clone(); + let PublicationPlanOperationKind::ReviewCreate { payload, .. } = + &mut operation_changed.operations[0].desired + else { + panic!("sample plan must contain a review create"); + }; + payload.body = "Changed operation payload".into(); + assert_ne!( + operation_changed + .recompute_operation_manifest_digest() + .unwrap(), + plan.operation_manifest_digest + ); + let mut stored_manifest_only = plan.clone(); + stored_manifest_only.operation_manifest_digest = "sha256:untrusted".into(); + assert_eq!( + stored_manifest_only + .recompute_operation_manifest_digest() + .unwrap(), + plan.operation_manifest_digest, + "the stored manifest digest is outside its own canonical boundary" + ); + assert_ne!( + stored_manifest_only.recompute_intent_digest().unwrap(), + plan.intent_digest, + "the manifest seal is inside the plan intent boundary" + ); + } + + #[test] + fn publication_plan_rejects_noncanonical_numeric_identifiers() { + for repository_id in ["", "0", "01", "9223372036854775808", "18446744073709551615"] { + assert!(ensure_publication_decimal_identifier("repository id", repository_id).is_err()); + } + for pull_request_number in ["", "0", "07", "-7"] { + assert!( + ensure_publication_decimal_identifier("pull request number", pull_request_number) + .is_err() + ); + } + assert!( + ensure_publication_decimal_identifier("repository id", "9223372036854775807").is_ok() + ); + assert!( + ensure_publication_sha256_identity( + "input identity", + &format!("sha256:{}", "a".repeat(64)), + ) + .is_ok() + ); + for identity in [ + "", + "sha256:", + &format!("sha256:{}", "a".repeat(63)), + &format!("sha256:{}", "A".repeat(64)), + &format!("sha256:{}", "g".repeat(64)), + ] { + assert!(ensure_publication_sha256_identity("input identity", identity).is_err()); + } + + let plan = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ); + assert!( + GitHubPublicationPlan::new( + GitHubPublicationPlanIdentity { + controller_generation: "generation-1".into(), + input_identity: plan.input_identity.clone(), + review_output_digest: plan.review_output_digest.clone(), + repository: plan.repository.clone(), + pull_request_number: plan.pull_request_number.clone(), + reviewed_snapshot: plan.reviewed_snapshot.clone(), + }, + plan.lifecycle_receipt.clone(), + plan.operations.clone(), + plan.gate_analysis.clone(), + ) + .is_err() + ); + + assert!( + PublicationPlanLifecycleReceipt::new( + plan.input_identity.clone(), + plan.lifecycle_receipt.channel, + plan.lifecycle_receipt.receipt_id.clone(), + plan.lifecycle_receipt.compatible_receipt_ids.clone(), + Some("9223372036854775808".into()), + plan.lifecycle_receipt.duplicate_of_baseline, + plan.lifecycle_receipt.findings.clone(), + ) + .is_err() + ); + let mut finding_with_oversized_id = plan.lifecycle_receipt.findings[0].clone(); + finding_with_oversized_id.observed_comment_id = Some("9223372036854775808".into()); + assert!( + PublicationPlanLifecycleReceipt::new( + plan.input_identity.clone(), + plan.lifecycle_receipt.channel, + plan.lifecycle_receipt.receipt_id.clone(), + vec![], + None, + false, + vec![finding_with_oversized_id], + ) + .is_err() + ); + } + + #[test] + fn publication_plan_constructor_accepts_only_a_sealed_topological_operation_graph() { + let template = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Success, + ); + let first = template.operations[0].clone(); + let second = dependent_publication_operation(&first, "summary-key", "review-key"); + let valid = rebuild_publication_plan(&template, vec![first.clone(), second.clone()]) + .expect("a contiguous topological operation graph is valid"); + assert_eq!(valid.operation_count, 2); + assert_eq!( + valid.recompute_operation_manifest_digest().unwrap(), + valid.operation_manifest_digest + ); + assert_eq!( + valid.recompute_intent_digest().unwrap(), + valid.intent_digest + ); + + let mut duplicate = second.clone(); + duplicate.operation_key = first.operation_key.clone(); + assert!( + rebuild_publication_plan(&template, vec![first.clone(), duplicate]) + .unwrap_err() + .to_string() + .contains("keys must be unique") + ); + + let mut missing = second.clone(); + missing.dependencies = vec!["missing-key".into()]; + missing.activation = PublicationPlanOperationActivation { + any_of: vec![PublicationPlanActivationCondition::Always], + }; + assert!( + rebuild_publication_plan(&template, vec![first.clone(), missing]) + .unwrap_err() + .to_string() + .contains("depends on missing operation") + ); + + let mut forward_first = first.clone(); + forward_first.dependencies = vec![second.operation_key.clone()]; + assert!( + rebuild_publication_plan(&template, vec![forward_first, second.clone()]) + .unwrap_err() + .to_string() + .contains("forward, self, or cyclic dependency") + ); + + let mut cyclic_first = first.clone(); + cyclic_first.dependencies = vec![second.operation_key.clone()]; + let mut cyclic_second = second.clone(); + cyclic_second.dependencies = vec![first.operation_key.clone()]; + assert!( + rebuild_publication_plan(&template, vec![cyclic_first, cyclic_second]) + .unwrap_err() + .to_string() + .contains("forward, self, or cyclic dependency") + ); + + let mut self_dependent = first.clone(); + self_dependent.dependencies = vec![first.operation_key.clone()]; + assert!( + rebuild_publication_plan(&template, vec![self_dependent]) + .unwrap_err() + .to_string() + .contains("forward, self, or cyclic dependency") + ); + + let mut undeclared_activation = second.clone(); + undeclared_activation.dependencies.clear(); + assert!( + rebuild_publication_plan(&template, vec![first.clone(), undeclared_activation]) + .unwrap_err() + .to_string() + .contains("references undeclared dependency") + ); + + let mut empty_activation = first.clone(); + empty_activation.activation.any_of.clear(); + assert!( + rebuild_publication_plan(&template, vec![empty_activation]) + .unwrap_err() + .to_string() + .contains("require at least one activation condition") + ); + + let mut stale_desired_digest = first.clone(); + let PublicationPlanOperationKind::ReviewCreate { payload, .. } = + &mut stale_desired_digest.desired + else { + panic!("sample plan must contain a review create"); + }; + payload.body = "Changed without resealing".into(); + assert!( + rebuild_publication_plan(&template, vec![stale_desired_digest]) + .unwrap_err() + .to_string() + .contains("desired digest does not match") + ); + + let advisory_completion = PublicationPlanOperation::new( + 2, + "advisory-complete".into(), + vec![], + PublicationPlanOperationActivation { + any_of: vec![PublicationPlanActivationCondition::Always], + }, + PublicationPlanOperationReconciliation { + logical_identity: "advisory-complete".into(), + markers: vec![], + observed_remote_id: None, + exclusive: true, + }, + PublicationPlanOperationKind::AdvisoryCheckComplete { + name: "postil/review".into(), + head_sha: "aaaaaaaa".into(), + created_check: PublicationPlanOperationResultReference { + dependency_operation_key: first.operation_key.clone(), + result_field: PublicationPlanOperationResultField::RemoteId, + }, + conclusion: PublicationPlanCheckConclusion::Success, + title: "Review".into(), + summary: "Review complete.".into(), + annotations: vec![], + details_url: None, + }, + ) + .unwrap(); + assert!( + rebuild_publication_plan(&template, vec![first.clone(), advisory_completion]) + .unwrap_err() + .to_string() + .contains("undeclared create dependency") + ); + + let mut ordinal_zero = first.clone(); + ordinal_zero.ordinal = 0; + assert!( + rebuild_publication_plan(&template, vec![ordinal_zero]) + .unwrap_err() + .to_string() + .contains("contiguous and one-based") + ); + + let mut ordinal_gap = second; + ordinal_gap.ordinal = 3; + assert!( + rebuild_publication_plan(&template, vec![first, ordinal_gap]) + .unwrap_err() + .to_string() + .contains("contiguous and one-based") + ); + } + + #[test] + fn publication_plan_enforces_operation_and_dependency_edge_bounds() { + let template = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Success, + ); + let operation_template = &template.operations[0]; + + let maximum_operations = bounded_publication_operations( + operation_template, + GitHubPublicationPlan::MAX_OPERATIONS, + 0, + ); + assert_eq!( + rebuild_publication_plan(&template, maximum_operations) + .unwrap() + .operation_count, + 126 + ); + let too_many_operations = bounded_publication_operations( + operation_template, + GitHubPublicationPlan::MAX_OPERATIONS + 1, + 0, + ); + assert!( + rebuild_publication_plan(&template, too_many_operations) + .unwrap_err() + .to_string() + .contains("operation count must not exceed 126") + ); + + let maximum_edges = bounded_publication_operations( + operation_template, + GitHubPublicationPlan::MAX_OPERATIONS, + GitHubPublicationPlan::MAX_DEPENDENCY_EDGES, + ); + assert!(rebuild_publication_plan(&template, maximum_edges).is_ok()); + let too_many_edges = bounded_publication_operations( + operation_template, + GitHubPublicationPlan::MAX_OPERATIONS, + GitHubPublicationPlan::MAX_DEPENDENCY_EDGES + 1, + ); + assert!( + rebuild_publication_plan(&template, too_many_edges) + .unwrap_err() + .to_string() + .contains("dependency edge count must not exceed 1024") + ); + } + + #[test] + fn publication_plan_check_wire_values_are_closed_enums() { + assert_eq!( + serde_json::to_value(PublicationPlanCheckStatus::InProgress).unwrap(), + serde_json::json!("in_progress") + ); + assert_eq!( + serde_json::to_value(PublicationPlanCheckConclusion::Success).unwrap(), + serde_json::json!("success") + ); + assert_eq!( + serde_json::to_value(PublicationPlanCheckConclusion::Failure).unwrap(), + serde_json::json!("failure") + ); + assert_eq!( + serde_json::to_value(PublicationPlanCheckConclusion::Neutral).unwrap(), + serde_json::json!("neutral") + ); + assert!(serde_json::from_str::("\"queued\"").is_err()); + assert!(serde_json::from_str::("\"cancelled\"").is_err()); + } + + #[test] + fn publication_operation_manifest_seals_complete_ordered_records() { + let plan = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ); + let baseline = plan.operation_manifest_digest.clone(); + let assert_changed = |changed: &GitHubPublicationPlan| { + assert_ne!( + changed.recompute_operation_manifest_digest().unwrap(), + baseline + ); + }; + + let mut changed = plan.clone(); + changed.operations[0].ordinal = 2; + assert_changed(&changed); + + let mut changed = plan.clone(); + changed.operations[0].operation_key.push_str("-changed"); + assert_changed(&changed); + + let mut changed = plan.clone(); + changed.operations[0] + .dependencies + .push("dependency-key".into()); + assert_changed(&changed); + + let mut changed = plan.clone(); + changed.operations[0].activation.any_of.clear(); + assert_changed(&changed); + + let mut changed = plan.clone(); + changed.operations[0] + .reconciliation + .logical_identity + .push_str("-changed"); + assert_changed(&changed); + + let mut changed = plan.clone(); + changed.operations[0].desired_digest = "sha256:changed".into(); + assert_changed(&changed); + + let mut changed = plan.clone(); + let PublicationPlanOperationKind::ReviewCreate { + attempt, + logical_review_identity, + .. + } = &mut changed.operations[0].desired + else { + panic!("sample plan must contain a review create"); + }; + *attempt = PublicationPlanReviewAttemptKind::SummaryOnly; + logical_review_identity.push_str("-changed"); + assert_changed(&changed); } #[cfg(unix)] @@ -1636,7 +2813,12 @@ mod tests { use std::os::unix::fs::symlink; let directory = tempfile::tempdir().unwrap(); - let plan = sample_publication_plan("aaaaaaaa", "Review body", "finding-1", "failure"); + let plan = sample_publication_plan( + "aaaaaaaa", + "Review body", + "finding-1", + PublicationPlanCheckConclusion::Failure, + ); let destination = directory.path().join("destination.json"); std::fs::write(&destination, "unchanged").unwrap(); let link = directory.path().join("publication-plan.json"); diff --git a/src/main.rs b/src/main.rs index 1d69f48..9354f36 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,9 @@ use clap::Parser; #[cfg(feature = "qualification-candidate")] use postil_cli::attribution; -use postil_cli::cli::{Cli, Command, ForgeArg, HookAction, publication_enabled}; +use postil_cli::cli::{ + Cli, Command, ForgeArg, HookAction, publication_enabled, publication_plan_contract_capability, +}; use postil_cli::config::{Config, qualification_metadata, starter_config}; use postil_cli::review::{ForgeKind, ReviewArgs}; use postil_cli::{doctor, hook, login, plan, respond, review}; @@ -22,6 +24,15 @@ async fn main() { async fn dispatch(cli: Cli) -> anyhow::Result { match cli.command { + Command::Capabilities { + publication_plan_contract, + } => { + println!( + "{}", + publication_plan_contract_capability(&publication_plan_contract)? + ); + Ok(0) + } Command::QualificationMetadata => { println!("{}", serde_json::to_string(&qualification_metadata())?); Ok(0) @@ -55,6 +66,8 @@ async fn dispatch(cli: Cli) -> anyhow::Result { no_post, defer_gate_check, publication_plan_output, + publication_generation, + publication_input_identity, } => { let local_mode = staged || base.is_some() || diff_file.is_some(); let kind = match forge { @@ -91,6 +104,8 @@ async fn dispatch(cli: Cli) -> anyhow::Result { no_post: !publication_enabled(publish, no_post)?, defer_gate_check, publication_plan_output, + publication_generation, + publication_input_identity, }) .await } diff --git a/src/review.rs b/src/review.rs index 1c612c8..bc07d7c 100644 --- a/src/review.rs +++ b/src/review.rs @@ -392,6 +392,8 @@ pub struct ReviewArgs { pub no_post: bool, pub defer_gate_check: bool, pub publication_plan_output: Option, + pub publication_generation: Option, + pub publication_input_identity: Option, } impl ReviewArgs { @@ -507,6 +509,27 @@ pub async fn run(args: ReviewArgs) -> Result { && !args.defer_gate_check, "--publication-plan-output cannot be combined with forge mutation options" ); + let generation = args + .publication_generation + .as_deref() + .context("--publication-plan-output requires --publication-generation")?; + crate::forge::ensure_publication_decimal_identifier("controller generation", generation) + .context("invalid --publication-generation")?; + let input_identity = args + .publication_input_identity + .as_deref() + .context("--publication-plan-output requires --publication-input-identity")?; + crate::forge::ensure_publication_sha256_identity("input identity", input_identity) + .context("invalid --publication-input-identity")?; + } else { + anyhow::ensure!( + args.publication_generation.is_none(), + "--publication-generation requires --publication-plan-output" + ); + anyhow::ensure!( + args.publication_input_identity.is_none(), + "--publication-input-identity requires --publication-plan-output" + ); } let mut cfg = Config::load(&cwd, args.config.as_deref())?; if let Some(m) = &args.model { @@ -2790,6 +2813,8 @@ async fn finish( publication: Option>, strict_publication: bool, ) -> Result { + let publication_plan_to_stdout = + args.publication_plan_output.as_deref() == Some(Path::new("-")); // Persist artifacts before any forge I/O: a posting hiccup must not // discard the completed review's SARIF or envelope output. if let Some(path) = &args.sarif { @@ -2798,10 +2823,14 @@ async fn finish( .with_context(|| format!("writing SARIF to {}", path.display()))?; } - if let Some(format) = args.resolved_output_format() { + if let Some(format) = args.resolved_output_format() + && (args.output_file.is_some() || !publication_plan_to_stdout) + { output::write_envelope(&envelope, format, args.output_file.as_deref())?; } - if args.resolved_output_format().is_none() || args.output_file.is_some() { + if !publication_plan_to_stdout + && (args.resolved_output_format().is_none() || args.output_file.is_some()) + { output::print_pretty(&envelope); } @@ -2824,6 +2853,14 @@ async fn finish( let (advisory, gate) = remote_check_states(&envelope); let plan = forge .build_publication_plan(crate::forge::GitHubPublicationPlanRequest { + controller_generation: args + .publication_generation + .as_deref() + .context("publication planning is missing its generation identity")?, + input_identity: args + .publication_input_identity + .as_deref() + .context("publication planning is missing its input identity")?, envelope: &envelope, snapshot: expected_snapshot, publication_diff, @@ -2835,7 +2872,11 @@ async fn finish( gate, }) .await?; - crate::forge::write_github_publication_plan(path, &plan)?; + if publication_plan_to_stdout { + crate::forge::write_github_publication_plan_to_writer(std::io::stdout().lock(), &plan)?; + } else { + crate::forge::write_github_publication_plan(path, &plan)?; + } return Ok(if envelope.gate.failing { 1 } else { 0 }); } diff --git a/tests/e2e.rs b/tests/e2e.rs index 059b1e8..ccb6388 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -11,6 +11,9 @@ use serde_json::{Value, json}; use wiremock::matchers::{body_string_contains, header, method, path, path_regex, query_param}; use wiremock::{Mock, MockServer as WireMockServer, Request, Respond, ResponseTemplate}; +const PUBLICATION_INPUT_IDENTITY: &str = + "sha256:1111111111111111111111111111111111111111111111111111111111111111"; + struct MockServer(WireMockServer); impl std::ops::Deref for MockServer { @@ -1238,13 +1241,30 @@ async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_fo let server = MockServer::start().await; mount_github_complete_diff(&server, 7).await; mount_static_github_pr(&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; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .mount(&server) + .await; let directory = tempfile::tempdir().unwrap(); disable_review_for_hosted_publication(directory.path(), true); - let first_plan = directory.path().join("publication-plan-1.json"); + let first_plan_name = "publication-plan-1.json"; + let first_plan = directory.path().join(first_plan_name); let second_plan = directory.path().join("publication-plan-2.json"); + let second_envelope = directory.path().join("publication-plan-2.envelope.json"); - for plan_path in [&first_plan, &second_plan] { - let envelope_path = plan_path.with_extension("envelope.json"); + for (plan_path, envelope_path) in [ + ( + std::path::Path::new(first_plan_name), + std::path::Path::new("publication-plan-1.envelope.json"), + ), + (second_plan.as_path(), second_envelope.as_path()), + ] { postil() .current_dir(directory.path()) .env("GITHUB_API_URL", server.uri()) @@ -1263,6 +1283,8 @@ async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_fo "--publication-plan-output", ]) .arg(plan_path) + .args(["--publication-generation", "1"]) + .args(["--publication-input-identity", PUBLICATION_INPUT_IDENTITY]) .args(["--output", "json", "--output-file"]) .arg(envelope_path) .assert() @@ -1272,12 +1294,71 @@ async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_fo let first = std::fs::read(&first_plan).unwrap(); let second = std::fs::read(&second_plan).unwrap(); assert_eq!(first, second); + let piped = postil() + .current_dir(directory.path()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .env("POSTIL_EXPECTED_GITHUB_REPO_ID", "42") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "-", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, + "--output", + "json", + ]) + .assert() + .code(0) + .get_output() + .stdout + .clone(); + assert_eq!(piped, first); + assert_eq!(piped.last(), Some(&b'\n')); + assert!(!directory.path().join("-").exists()); + for envelope_path in [ + directory.path().join("publication-plan-1.envelope.json"), + second_envelope, + ] { + assert!( + !std::fs::read_to_string(envelope_path) + .unwrap() + .contains(PUBLICATION_INPUT_IDENTITY), + "the service-supplied input identity must remain private to the plan artifact" + ); + } + assert!(!std::fs::read_dir(directory.path()).unwrap().any(|entry| { + let name = entry.unwrap().file_name(); + let name = name.to_string_lossy(); + name.starts_with(".publication-plan-1.json.") && name.ends_with(".tmp") + })); let plan: Value = serde_json::from_slice(&first).unwrap(); assert_eq!(plan["version"], 1); assert_eq!(plan["forge"], "github"); - assert_eq!(plan["repository"]["id"], 42); + assert_eq!(plan["controllerGeneration"], "1"); + assert_eq!(plan["inputIdentity"], PUBLICATION_INPUT_IDENTITY); + assert_eq!( + plan["lifecycleReceipt"]["inputIdentity"], + PUBLICATION_INPUT_IDENTITY + ); + assert!( + plan["reviewOutputDigest"] + .as_str() + .is_some_and(|digest| digest.starts_with("sha256:") && digest.len() == 71) + ); + assert_eq!(plan["repository"]["id"], "42"); assert_eq!(plan["repository"]["fullName"], "acme/api"); - assert_eq!(plan["pullRequestNumber"], 7); + assert_eq!(plan["pullRequestNumber"], "7"); assert_eq!(plan["reviewedSnapshot"]["headSha"], "aaaaaaaa"); assert_eq!(plan["reviewedSnapshot"]["mergeBaseSha"], "bbbbbbbb"); assert_eq!(plan["reviewedSnapshot"]["targetSha"], "bbbbbbbb"); @@ -1286,12 +1367,48 @@ async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_fo .as_str() .is_some_and(|digest| digest.starts_with("sha256:") && digest.len() == 71) ); - assert_eq!(plan["operations"][0]["kind"], "compositeReview"); - assert_eq!(plan["operations"][0]["action"], "publish"); - assert_eq!(plan["operations"][1]["kind"], "advisoryCheck"); - assert_eq!(plan["operations"][1]["conclusion"], "success"); - assert_eq!(plan["operations"][2]["kind"], "gateCheck"); - assert_eq!(plan["operations"][2]["conclusion"], "success"); + assert_eq!( + plan["operationCount"], + plan["operations"].as_array().unwrap().len() + ); + assert!( + plan["operationManifestDigest"] + .as_str() + .is_some_and(|digest| digest.starts_with("sha256:") && digest.len() == 71) + ); + assert_eq!(plan["operations"][0]["kind"], "advisoryCheckCreate"); + assert_eq!(plan["operations"][0]["name"], "postil/review"); + assert_eq!(plan["operations"][1]["kind"], "reviewCreate"); + assert_eq!(plan["operations"][1]["attempt"], "initial"); + assert_eq!(plan["operations"][2]["kind"], "reviewSummaryUpdate"); + assert_eq!(plan["operations"][3]["kind"], "advisoryCheckComplete"); + assert_eq!(plan["operations"][3]["name"], "postil/review"); + assert_eq!(plan["operations"][3]["conclusion"], "success"); + assert_eq!( + plan["operations"][3]["createdCheck"]["dependencyOperationKey"], + plan["operations"][0]["operationKey"] + ); + assert_eq!( + plan["operations"][3]["dependencies"], + json!([ + plan["operations"][0]["operationKey"].clone(), + plan["operations"][2]["operationKey"].clone() + ]) + ); + assert!( + plan["operations"] + .as_array() + .unwrap() + .iter() + .all(|operation| { + operation["kind"] != "gateCheck" && operation["name"] != "postil/gate" + }) + ); + assert_eq!(plan["gateAnalysis"]["ownership"], "service"); + assert_eq!(plan["gateAnalysis"]["authoritative"], false); + assert_eq!(plan["gateAnalysis"]["organizationGateModeRequired"], true); + assert_eq!(plan["gateAnalysis"]["name"], "postil/gate"); + assert_eq!(plan["gateAnalysis"]["analyzedConclusion"], "success"); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -1363,6 +1480,42 @@ async fn github_publication_plan_is_byte_stable_private_and_never_mutates_the_fo })); } +#[test] +fn publication_plan_capability_probe_is_exact_and_external_io_free() { + let directory = tempfile::tempdir().unwrap(); + postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", "http://127.0.0.1:1") + .env("GITHUB_API_URL", "http://127.0.0.1:1") + .args([ + "capabilities", + "--publication-plan-contract", + "github-publication-v1", + ]) + .assert() + .code(0) + .stdout("github-publication-v1\n") + .stderr(""); + assert_eq!(std::fs::read_dir(directory.path()).unwrap().count(), 0); + + postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", "http://127.0.0.1:1") + .env("GITHUB_API_URL", "http://127.0.0.1:1") + .args([ + "capabilities", + "--publication-plan-contract", + "github-publication-v2", + ]) + .assert() + .code(2) + .stdout("") + .stderr(predicates::str::contains( + "supported contract: github-publication-v1", + )); + assert_eq!(std::fs::read_dir(directory.path()).unwrap().count(), 0); +} + #[test] fn publication_plan_rejects_non_github_and_local_review_modes_before_io() { let directory = tempfile::tempdir().unwrap(); @@ -1380,6 +1533,10 @@ fn publication_plan_rejects_non_github_and_local_review_modes_before_io() { "bbbbbbbb", "--publication-plan-output", "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + PUBLICATION_INPUT_IDENTITY, ]); command .args(extra) @@ -1389,6 +1546,37 @@ fn publication_plan_rejects_non_github_and_local_review_modes_before_io() { "requires a remote GitHub pull request", )); } + + for invalid_identity in [ + "sha256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", + "sha256:1", + "not-a-digest", + ] { + postil() + .current_dir(directory.path()) + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--base-sha", + "bbbbbbbb", + "--publication-plan-output", + "publication-plan.json", + "--publication-generation", + "1", + "--publication-input-identity", + invalid_identity, + ]) + .assert() + .code(2) + .stderr(predicates::str::contains( + "invalid --publication-input-identity", + )); + } } #[test] @@ -10567,7 +10755,8 @@ async fn github_unresolved_inline_line_retries_on_a_changed_line() { .await; let dir = tempfile::tempdir().unwrap(); - let receipt_path = dir.path().join("publication-receipt.json"); + let receipt_name = "publication-receipt.json"; + let receipt_path = dir.path().join(receipt_name); let out = postil() .current_dir(dir.path()) .env("POSTIL_API_BASE", server.uri()) @@ -10577,7 +10766,7 @@ async fn github_unresolved_inline_line_retries_on_a_changed_line() { "POSTIL_DETAILS_URL", "https://postil.dev/orgs/acme/runs/run-1", ) - .env("POSTIL_PUBLICATION_RECEIPT_PATH", &receipt_path) + .env("POSTIL_PUBLICATION_RECEIPT_PATH", receipt_name) .args([ "review", "--publish", @@ -10622,6 +10811,23 @@ async fn github_unresolved_inline_line_retries_on_a_changed_line() { assert_eq!(receipt["reviewId"], "77"); assert_eq!(receipt["findings"][0]["initialOutcome"], "inline"); assert_eq!(receipt["findings"][0]["commentId"], "500"); + assert!(!std::fs::read_dir(dir.path()).unwrap().any(|entry| { + let name = entry.unwrap().file_name(); + let name = name.to_string_lossy(); + name.starts_with(".publication-receipt.json.") && name.ends_with(".tmp") + })); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + std::fs::metadata(&receipt_path) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o600 + ); + } } #[tokio::test] From a0ec5cd0786777fd6ed8545a7cc5bc72cc63d0da Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sat, 15 Aug 2026 01:53:55 +0000 Subject: [PATCH 7/7] chore(release): prepare v0.8.17 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 960f736..9108fb7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1104,7 +1104,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.8.16" +version = "0.8.17" dependencies = [ "aho-corasick", "anyhow", diff --git a/Cargo.toml b/Cargo.toml index a49ac65..aeddbcb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.8.16" +version = "0.8.17" edition = "2024" description = "Postil: a low-noise AI review gate. Silent on clean PRs, hard gate on real risk." license = "Apache-2.0"