From 8a522c2a7527757df53c99d1b302fcfd7dd97db8 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 19 Jul 2026 20:56:10 +0000 Subject: [PATCH] Fail hosted runs when GitHub publication is incomplete --- src/forge/github.rs | 266 ++++++++++++++++++++++-------- src/review.rs | 329 ++++++++++++++++++++++++------------- tests/e2e.rs | 390 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 805 insertions(+), 180 deletions(-) diff --git a/src/forge/github.rs b/src/forge/github.rs index 1d1de45..2bd48ae 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -1109,13 +1109,12 @@ impl Forge for GitHub { return Ok(()); } if !self.snapshot_is_current(snapshot).await? { - eprintln!( - "postil: github review delivery skipped because the PR snapshot changed reviewed_head={} reviewed_target={} reviewed_merge_base={}", + return Err(anyhow!( + "GitHub review delivery skipped because the PR snapshot changed reviewed_head={} reviewed_target={} reviewed_merge_base={}", short_sha(head_sha), short_sha(snapshot.target_sha.as_deref().unwrap_or("unknown")), short_sha(&snapshot.base_sha), - ); - return Ok(()); + )); } let comments: Vec<_> = findings .iter() @@ -1254,13 +1253,12 @@ impl Forge for GitHub { snapshot: &PrMeta, ) -> Result<()> { if !self.snapshot_is_current(snapshot).await? { - eprintln!( - "postil: github check delivery skipped because the PR snapshot changed reviewed_head={} reviewed_target={} reviewed_merge_base={}", + return Err(anyhow!( + "GitHub check delivery skipped because the PR snapshot changed reviewed_head={} reviewed_target={} reviewed_merge_base={}", short_sha(&snapshot.head_sha), short_sha(snapshot.target_sha.as_deref().unwrap_or("unknown")), short_sha(&snapshot.base_sha), - ); - return Ok(()); + )); } let conclusion = |s: CheckState| match s { CheckState::Success => "success", @@ -1294,51 +1292,82 @@ impl Forge for GitHub { if let Some(gate) = gate { checks.push((gate_id, gate, "postil/gate", false)); } - for (id, state, name, with_annotations) in checks { - 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![], - }, - ) - }; - 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 with_annotations && !annotations.is_empty() { - output["annotations"] = json!(annotations); + 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![], + }, + ) + }; + 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 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}"), + ) + .await + { + Ok(response) => Self::check_ok(response, "check-run complete") + .await + .map(|_| ()), + Err(error) => Err(error), + }; + (index, name, result) + } + }, + )) + .buffer_unordered(2) + .collect::>() + .await; + results.sort_by_key(|(index, _, _)| *index); + let mut failures = Vec::new(); + for (_, name, result) in results { + if let Err(error) = result { + if super::is_repository_identity_failure(&error) { + return Err(error); + } + failures.push(format!("{name}: {error:#}")); } - let mut body = json!({ - "status": "completed", - "conclusion": conclusion(state), - "output": output, - }); - self.add_details_url(&mut body); - let resp = self - .send_write_retryable( - self.request( - reqwest::Method::PATCH, - self.url(&format!("/check-runs/{id}")), - ) - .json(&body), - &format!("complete {name}"), - ) - .await?; - Self::check_ok(resp, "check-run complete").await?; + } + if !failures.is_empty() { + return Err(anyhow!( + "{} GitHub check completion(s) failed: {}", + failures.len(), + failures.join("; ") + )); } Ok(()) } @@ -1641,6 +1670,109 @@ mod tests { } } + async fn mount_current_delivery_snapshot(server: &MockServer) { + Mock::given(method("GET")) + .and(path("/repos/owner/repo/pulls/1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "title": "t", "body": "b", "state": "open", "merged": false, + "head": {"sha": "aaaaaaaaaaaa"}, + "base": {"sha": "bbbbbbbbbbbb"}, + "changed_files": 1 + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path( + "/repos/owner/repo/compare/bbbbbbbbbbbb...aaaaaaaaaaaa", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "merge_base_commit": {"sha": "cccccccccccc"}, + "files": [] + }))) + .mount(server) + .await; + } + + #[tokio::test] + async fn github_check_completion_attempts_the_gate_after_an_advisory_patch_fails() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + Mock::given(method("PATCH")) + .and(path("/repos/owner/repo/check-runs/11")) + .respond_with(ResponseTemplate::new(422).set_body_string("invalid advisory patch")) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/owner/repo/check-runs/12")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .expect(1) + .mount(&server) + .await; + + let error = test_github(&server) + .complete_checks( + "11", + "12", + CheckState::Success, + Some(CheckState::Success), + &delivery_envelope("aaaaaaaaaaaa", "cccccccccccc"), + &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), + ) + .await + .unwrap_err(); + + assert!(error.to_string().contains("postil/review")); + assert!(!error.to_string().contains("postil/gate")); + } + + #[tokio::test] + async fn github_check_completion_starts_both_patches_before_an_outer_timeout() { + let server = MockServer::start().await; + mount_current_delivery_snapshot(&server).await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/owner/repo/check-runs/(11|12)$")) + .respond_with( + ResponseTemplate::new(200) + .set_delay(Duration::from_secs(1)) + .set_body_json(serde_json::json!({})), + ) + .mount(&server) + .await; + let github = test_github(&server); + let envelope = delivery_envelope("aaaaaaaaaaaa", "cccccccccccc"); + let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); + + let result = tokio::time::timeout( + Duration::from_millis(100), + github.complete_checks( + "11", + "12", + CheckState::Success, + Some(CheckState::Success), + &envelope, + &snapshot, + ), + ) + .await; + assert!(result.is_err()); + + let requests = server.received_requests().await.unwrap(); + let mut patched = requests + .iter() + .filter(|request| request.method == reqwest::Method::PATCH) + .map(|request| request.url.path().to_string()) + .collect::>(); + patched.sort(); + assert_eq!( + patched, + vec![ + "/repos/owner/repo/check-runs/11", + "/repos/owner/repo/check-runs/12" + ] + ); + } + fn fenced_test_github(server: &MockServer, expected_repository_id: u64) -> GitHub { GitHub { expected_repository_id: Some(expected_repository_id), @@ -1916,14 +2048,15 @@ mod tests { id: None, }; - github + let error = github .post_review( "Summary", &[finding], &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), ) .await - .unwrap(); + .unwrap_err(); + assert!(error.to_string().contains("PR snapshot changed")); } #[tokio::test] @@ -1979,11 +2112,12 @@ mod tests { let github = test_github(&server); let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); - github + let review_error = github .post_review("Summary", &[finding], &snapshot) .await - .unwrap(); - github + .unwrap_err(); + assert!(review_error.to_string().contains("PR snapshot changed")); + let check_error = github .complete_checks( "11", "12", @@ -1993,7 +2127,8 @@ mod tests { &snapshot, ) .await - .unwrap(); + .unwrap_err(); + assert!(check_error.to_string().contains("PR snapshot changed")); } #[tokio::test] @@ -2053,11 +2188,12 @@ mod tests { let envelope = delivery_envelope("aaaaaaaaaaaa", "cccccccccccc"); let snapshot = delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"); - github + let review_error = github .post_review("Summary", &[finding], &snapshot) .await - .unwrap(); - github + .unwrap_err(); + assert!(review_error.to_string().contains("PR snapshot changed")); + let check_error = github .complete_checks( "11", "12", @@ -2067,7 +2203,8 @@ mod tests { &snapshot, ) .await - .unwrap(); + .unwrap_err(); + assert!(check_error.to_string().contains("PR snapshot changed")); } #[tokio::test] @@ -2148,14 +2285,15 @@ mod tests { id: None, }; - github + let error = github .post_review( "Summary", &[finding], &delivery_snapshot("aaaaaaaaaaaa", "bbbbbbbbbbbb", "cccccccccccc"), ) .await - .unwrap(); + .unwrap_err(); + assert!(error.to_string().contains("PR snapshot changed")); } #[tokio::test] diff --git a/src/review.rs b/src/review.rs index 3cbc83d..65a488d 100644 --- a/src/review.rs +++ b/src/review.rs @@ -64,29 +64,14 @@ async fn snapshot_is_current( forge: &F, expected: &PrMeta, review_started: Instant, -) -> bool { - match run_with_hosted_budget( +) -> Result { + run_with_hosted_budget( Some(review_started), FORGE_READ_TIMEOUT_SECS, forge.snapshot_is_current(expected), "verifying pull request snapshot before publication", ) .await - { - Ok(true) => true, - Ok(false) => { - eprintln!( - "postil: publication skipped because the pull request snapshot changed after review" - ); - false - } - Err(error) => { - eprintln!( - "postil: publication skipped because snapshot freshness could not be verified ({error:#})" - ); - false - } - } } fn conservative_context_tokens(model: &str) -> usize { @@ -356,7 +341,7 @@ async fn run_local(args: &ReviewArgs, cfg: &Config) -> Result { }, ) .await?; - finish(args, cfg, envelope, None::<&GitHub>, None, None).await + finish(args, cfg, envelope, None::<&GitHub>, None, None, false).await } async fn run_remote( @@ -366,6 +351,7 @@ async fn run_remote( repo: &str, ) -> Result { let review_started = std::time::Instant::now(); + let strict_publication = strict_hosted_github_publication(args); let meta = run_with_hosted_budget( Some(review_started), FORGE_READ_TIMEOUT_SECS, @@ -431,9 +417,7 @@ async fn run_remote( .await; match result { Ok(envelope) => { - if let Some((a, g)) = &checks - && snapshot_is_current(forge, &meta, review_started).await - { + let check_completion = if let Some((a, g)) = &checks { let gate_state = if envelope.gate.failing { CheckState::Failure } else { @@ -452,40 +436,36 @@ async fn run_remote( } else { CheckState::Success }; - // A transient forge outage here (rate limit, timeout) must not - // discard the review this far along: log and keep going so the - // envelope/SARIF output and exit code below still land. Mirrors - // the Err(e) arm below, which best-effort's this same call. - let completed = run_with_hosted_budget( - Some(review_started), - CHECK_COMPLETION_TIMEOUT_SECS, - forge.complete_checks( - a, - g, - advisory_state, - (!args.defer_gate_check).then_some(gate_state), - &envelope, - &meta, - ), - "completing check runs", + complete_remote_checks( + forge, + a, + g, + advisory_state, + (!args.defer_gate_check).then_some(gate_state), + &envelope, + &meta, + review_started, ) - .await; - if let Err(e) = completed { - if crate::forge::is_repository_identity_failure(&e) { - return Err(e); - } - eprintln!("postil: could not update check runs ({e:#})"); - } - } - finish( + .await + } else { + Ok(()) + }; + let check_failure = retain_publication_failure( + strict_publication, + check_completion, + "could not update check runs", + )?; + let finish_result = finish( args, cfg, envelope, Some(forge), Some(review_started), Some(&meta), + strict_publication, ) - .await + .await; + combine_required_publication(check_failure, finish_result) } Err(e) => { eprintln!("postil: review failed before completion ({e:#})"); @@ -506,66 +486,137 @@ async fn run_remote( &meta, review_started.elapsed().as_millis() as u64, ); - if let Some((a, g)) = &checks - && snapshot_is_current(forge, &meta, review_started).await - { + let check_completion = if let Some((a, g)) = &checks { let gate_state = if envelope.gate.failing { CheckState::Failure } else { CheckState::Success }; - let completion = run_with_hosted_budget( - Some(review_started), - CHECK_COMPLETION_TIMEOUT_SECS, - forge.complete_checks( - a, - g, - CheckState::Neutral, - (!args.defer_gate_check).then_some(gate_state), - &envelope, - &meta, - ), - "completing check runs", + complete_remote_checks( + forge, + a, + g, + CheckState::Neutral, + (!args.defer_gate_check).then_some(gate_state), + &envelope, + &meta, + review_started, ) - .await; - if let Err(error) = completion - && crate::forge::is_repository_identity_failure(&error) - { - return Err(error); - } - } - // Emit envelope/SARIF and derive the exit code from the gate. - // `finish` itself already downgrades forge posting failures - // (complete_checks/post_review) to warnings on stderr without - // touching its return value, so this only remains as a fallback - // for a local I/O failure inside `finish` (e.g. writing SARIF) on - // this already-errored path; even that must not mask the derived - // exit code, so it is downgraded to the gate-derived code rather - // than propagated as exit 2. + .await + } else { + Ok(()) + }; + let check_failure = retain_publication_failure( + strict_publication, + check_completion, + "could not update check runs", + )?; + // Emit the envelope and SARIF before delivery. Hosted GitHub runs + // require every applicable publication step; other invocations + // retain their gate-derived result when the forge is unavailable. let code = if envelope.gate.failing { 1 } else { 0 }; - match finish( + let finish_result = finish( args, cfg, envelope, Some(forge), Some(review_started), Some(&meta), + strict_publication, ) - .await - { + .await; + let finish_result = match finish_result { Ok(c) => Ok(c), Err(post_err) => { if crate::forge::is_repository_identity_failure(&post_err) { return Err(post_err); } - eprintln!("postil: could not post the error review ({post_err:#})"); - Ok(code) + if strict_publication { + Err(post_err) + } else { + eprintln!("postil: could not post the error review ({post_err:#})"); + Ok(code) + } } - } + }; + combine_required_publication(check_failure, finish_result) } } } +fn strict_hosted_github_publication(args: &ReviewArgs) -> bool { + crate::config::hosted_mode() && args.forge == ForgeKind::GitHub && !args.no_post +} + +async fn require_current_snapshot( + forge: &F, + expected: &PrMeta, + review_started: Instant, + publication: &str, +) -> Result<()> { + match snapshot_is_current(forge, expected, review_started).await { + Ok(true) => Ok(()), + Ok(false) => Err(anyhow!( + "{publication} skipped because the pull request snapshot changed after review" + )), + Err(error) => Err(error).with_context(|| { + format!("{publication} skipped because snapshot freshness could not be verified") + }), + } +} + +#[allow(clippy::too_many_arguments)] +async fn complete_remote_checks( + forge: &F, + advisory_id: &str, + gate_id: &str, + advisory: CheckState, + gate: Option, + envelope: &Envelope, + snapshot: &PrMeta, + review_started: Instant, +) -> Result<()> { + require_current_snapshot(forge, snapshot, review_started, "check completion").await?; + run_with_hosted_budget( + Some(review_started), + CHECK_COMPLETION_TIMEOUT_SECS, + forge.complete_checks(advisory_id, gate_id, advisory, gate, envelope, snapshot), + "completing check runs", + ) + .await +} + +fn retain_publication_failure( + required: bool, + result: Result<()>, + warning: &str, +) -> Result> { + match result { + Ok(()) => Ok(None), + Err(error) if required => Ok(Some(error)), + Err(error) if crate::forge::is_repository_identity_failure(&error) => Err(error), + Err(error) => { + eprintln!("postil: {warning} ({error:#})"); + Ok(None) + } + } +} + +fn combine_required_publication( + check_failure: Option, + finish_result: Result, +) -> Result { + match (check_failure, finish_result) { + (None, result) => result, + (Some(check_error), Ok(_)) => { + Err(check_error).context("required hosted check publication failed") + } + (Some(check_error), Err(review_error)) => Err(anyhow!( + "required hosted publication failed: check completion: {check_error:#}; review delivery: {review_error:#}" + )), + } +} + async fn remote_review( args: &ReviewArgs, cfg: &Config, @@ -1622,6 +1673,7 @@ async fn finish( forge: Option<&F>, hosted_budget_started_at: Option, expected_snapshot: Option<&PrMeta>, + strict_publication: bool, ) -> Result { // Persist artifacts before any forge I/O: a posting hiccup must not // discard the completed review's SARIF or envelope output. @@ -1643,45 +1695,65 @@ async fn finish( { let expected_snapshot = expected_snapshot.context("remote publication is missing its immutable PR snapshot")?; - let current = if let Some(started_at) = hosted_budget_started_at { - snapshot_is_current(forge, expected_snapshot, started_at).await - } else { - forge - .snapshot_is_current(expected_snapshot) - .await - .unwrap_or(false) - }; - if !current { - eprintln!("postil: review comment skipped because freshness is not proven"); - 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; - if should_comment { - let summary = forge.review_summary(&envelope); - // A posting failure here (rate limit, transient 5xx, network blip) - // must not discard a review that already computed and persisted its - // envelope/SARIF/stdout output above. Log it and keep going because the - // exit code always derives from the gate below, never from whether - // the forge comment made it out. - let posted = run_with_hosted_budget( - hosted_budget_started_at, - REVIEW_POST_TIMEOUT_SECS, - forge.post_review(&summary, &envelope.findings, expected_snapshot), - "posting review comment", - ) - .await; - if let Err(e) = posted { - if crate::forge::is_repository_identity_failure(&e) { - return Err(e); - } - eprintln!("postil: could not post review comment ({e:#})"); + && !duplicate_of_baseline + && !intentional_no_comment; + if !should_comment { + return Ok(if envelope.gate.failing { 1 } else { 0 }); + } + let freshness = if let Some(started_at) = hosted_budget_started_at { + snapshot_is_current(forge, expected_snapshot, started_at).await + } else { + forge.snapshot_is_current(expected_snapshot).await + }; + match freshness { + Ok(true) => {} + Ok(false) if strict_publication => { + return Err(anyhow!( + "required review publication skipped because the pull request snapshot changed after review" + )); + } + Ok(false) => { + eprintln!( + "postil: publication skipped because the pull request snapshot changed after review" + ); + return Ok(if envelope.gate.failing { 1 } else { 0 }); + } + Err(error) if strict_publication => { + return Err(error).context( + "required review publication skipped because snapshot freshness could not be verified", + ); + } + Err(error) => { + eprintln!( + "postil: publication skipped because snapshot freshness could not be verified ({error:#})" + ); + return Ok(if envelope.gate.failing { 1 } else { 0 }); } } + let summary = forge.review_summary(&envelope); + let posted = run_with_hosted_budget( + hosted_budget_started_at, + REVIEW_POST_TIMEOUT_SECS, + forge.post_review(&summary, &envelope.findings, expected_snapshot), + "posting review comment", + ) + .await; + if let Err(e) = posted { + if crate::forge::is_repository_identity_failure(&e) { + return Err(e); + } + if strict_publication { + return Err(e).context("required hosted review publication failed"); + } + eprintln!("postil: could not post review comment ({e:#})"); + } } Ok(if envelope.gate.failing { 1 } else { 0 }) } @@ -2123,6 +2195,33 @@ mod tests { ); } + #[tokio::test] + async fn required_check_completion_timeout_overrides_a_gate_derived_success() { + let started_at = Instant::now() - Duration::from_secs(HOSTED_WORKER_WATCHDOG_SECS) + + Duration::from_millis(20); + let completion = run_with_hosted_budget( + Some(started_at), + CHECK_COMPLETION_TIMEOUT_SECS, + async { + tokio::time::sleep(Duration::from_secs(1)).await; + Ok(()) + }, + "completing check runs", + ) + .await; + let retained = retain_publication_failure(true, completion, "fixture warning") + .unwrap() + .expect("strict hosted publication retains the timeout"); + let error = combine_required_publication(Some(retained), Ok(0)).unwrap_err(); + + assert!(format!("{error:#}").contains("completing check runs timed out")); + assert!( + error + .to_string() + .contains("required hosted check publication failed") + ); + } + #[test] fn stable_ids_do_not_share_duplicate_buckets_across_path_line_boundaries() { let head_sha = "0123456789abcdef0123456789abcdef01234567"; diff --git a/tests/e2e.rs b/tests/e2e.rs index 8492a9a..be1541f 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -1,8 +1,8 @@ //! End-to-end tests: the real binary against mocked LLM and forge endpoints. use std::collections::BTreeMap; -use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; use assert_cmd::Command; #[cfg(feature = "qualification-candidate")] @@ -280,6 +280,26 @@ struct GitHubHeadRaceResponder { calls: Arc, } +#[derive(Clone)] +struct GitHubLateHeadRaceResponder { + calls: Arc, +} + +#[derive(Clone)] +struct GitHubFreshnessFailureResponder { + calls: Arc, +} + +#[derive(Clone)] +struct AmbiguousReviewPostResponder { + published_body: Arc>>, +} + +#[derive(Clone)] +struct ReconciledReviewListResponder { + published_body: Arc>>, +} + struct OutputBudgetResponder; #[derive(Clone)] @@ -414,6 +434,64 @@ impl Respond for GitHubHeadRaceResponder { } } +impl Respond for GitHubLateHeadRaceResponder { + fn respond(&self, _request: &Request) -> ResponseTemplate { + let call = self.calls.fetch_add(1, Ordering::SeqCst); + let head = if call < 4 { "aaaaaaaa" } else { "cccccccc" }; + ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", + "body": "b", + "state": "open", "merged": false, + "head": {"sha": head}, + "base": {"sha": "bbbbbbbb"}, + "changed_files": 1 + })) + } +} + +impl Respond for GitHubFreshnessFailureResponder { + fn respond(&self, _request: &Request) -> ResponseTemplate { + if self.calls.fetch_add(1, Ordering::SeqCst) < 2 { + ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", + "body": "b", + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, + "base": {"sha": "bbbbbbbb"}, + "changed_files": 1 + })) + } else { + ResponseTemplate::new(500).set_body_string("temporary PR lookup failure") + } + } +} + +impl Respond for AmbiguousReviewPostResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body = request.body_json::().unwrap()["body"] + .as_str() + .unwrap() + .to_string(); + *self.published_body.lock().unwrap() = Some(body); + ResponseTemplate::new(500).set_body_string("ambiguous upstream response") + } +} + +impl Respond for ReconciledReviewListResponder { + fn respond(&self, _request: &Request) -> ResponseTemplate { + let body = self + .published_body + .lock() + .unwrap() + .clone() + .unwrap_or_default(); + ResponseTemplate::new(200).set_body_json(json!([{ + "body": body, + "commit_id": "aaaaaaaa" + }])) + } +} + async fn mount_github_complete_diff(server: &MockServer, pr: u64) { Mock::given(method("GET")) .and(path_regex(r"^/repos/acme/api/compare/b+\.\.\.a+$")) @@ -439,6 +517,76 @@ async fn mount_github_complete_diff(server: &MockServer, pr: u64) { .await; } +fn hosted_publish_command(directory: &std::path::Path, server: &MockServer) -> Command { + let mut command = postil(); + command + .current_dir(directory) + .env("POSTIL_HOSTED_MODE", "1") + .env("POSTIL_PROVISIONAL_HOSTED_ROSTER", "1") + .env("POSTIL_EXPECTED_GITHUB_REPO_ID", "42") + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--publish", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--check-run-id", + "901", + "--gate-check-run-id", + "902", + "--output-json", + ]); + command +} + +fn disable_review_for_hosted_publication(directory: &std::path::Path, comment_on_clean: bool) { + let on_clean = if comment_on_clean { + "review:\n onClean: comment\n" + } else { + "" + }; + std::fs::write( + directory.join(".postil.yaml"), + format!("enabled: false\n{on_clean}"), + ) + .unwrap(); +} + +async fn mount_static_github_pr(server: &MockServer) { + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": "b", + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, + "base": {"sha": "bbbbbbbb"}, + "changed_files": 1 + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "id": 42, + "full_name": "acme/api" + }))) + .mount(server) + .await; +} + +async fn mount_successful_hosted_check_patches(server: &MockServer) { + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/(901|902)$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(server) + .await; +} + #[derive(Clone)] struct GitHubLargeLockfileResponder; @@ -6546,6 +6694,246 @@ async fn forge_post_failure_on_success_path_keeps_gate_derived_exit_code() { ); } +#[tokio::test] +async fn hosted_review_post_failure_is_operational_after_the_envelope_is_persisted() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + mount_successful_hosted_check_patches(&server).await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(500).set_body_string("temporary review failure")) + .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 dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), true); + let output = hosted_publish_command(dir.path(), &server).assert().code(2); + + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap_or_else(|e| { + panic!( + "hosted review did not emit its envelope: {e}; stderr: {}", + String::from_utf8_lossy(&output.get_output().stderr) + ) + }); + assert_eq!(envelope["modelUsed"], "none (disabled by config)"); + assert_eq!(envelope["gate"]["failing"], false); + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!(stderr.contains("required hosted review publication failed")); + + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::PATCH) + .count(), + 2 + ); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::POST + && request.url.path() == "/repos/acme/api/pulls/7/reviews") + .count(), + 3, + "each ambiguous failure must be reconciled before the bounded retry" + ); +} + +#[tokio::test] +async fn hosted_marker_reconciliation_accepts_an_ambiguous_review_post_without_a_duplicate() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + mount_successful_hosted_check_patches(&server).await; + let published_body = Arc::new(Mutex::new(None)); + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(AmbiguousReviewPostResponder { + published_body: published_body.clone(), + }) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ReconciledReviewListResponder { published_body }) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), true); + hosted_publish_command(dir.path(), &server).assert().code(0); + + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::POST + && request.url.path() == "/repos/acme/api/pulls/7/reviews") + .count(), + 1, + "marker reconciliation must prevent a duplicate review" + ); +} + +#[tokio::test] +async fn hosted_check_patch_failures_are_operational_and_do_not_hide_the_envelope() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/(901|902)$")) + .respond_with(ResponseTemplate::new(500).set_body_string("temporary check failure")) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), false); + let output = hosted_publish_command(dir.path(), &server).assert().code(2); + + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap_or_else(|e| { + panic!( + "hosted review did not emit its envelope: {e}; stderr: {}", + String::from_utf8_lossy(&output.get_output().stderr) + ) + }); + assert_eq!(envelope["modelUsed"], "none (disabled by config)"); + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!(stderr.contains("required hosted check publication failed")); + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests + .iter() + .filter(|request| request.method == wiremock::http::Method::PATCH) + .count(), + 6, + "both required checks receive the complete bounded retry sequence" + ); +} + +#[tokio::test] +async fn hosted_freshness_lookup_failure_is_operational_before_any_write() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(GitHubFreshnessFailureResponder { + calls: Arc::new(AtomicUsize::new(0)), + }) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/(901|902)$")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), false); + let output = hosted_publish_command(dir.path(), &server).assert().code(2); + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!(stderr.contains("snapshot freshness could not be verified")); +} + +#[tokio::test] +async fn hosted_stale_head_race_is_operational_and_suppresses_all_writes() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(GitHubHeadRaceResponder { + calls: Arc::new(AtomicUsize::new(0)), + }) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/(901|902)$")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), false); + let output = hosted_publish_command(dir.path(), &server).assert().code(2); + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!(stderr.contains("pull request snapshot changed after review")); +} + +#[tokio::test] +async fn hosted_silent_review_requires_checks_but_not_a_pr_comment() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + mount_static_github_pr(&server).await; + mount_successful_hosted_check_patches(&server).await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), false); + hosted_publish_command(dir.path(), &server).assert().code(0); +} + +#[tokio::test] +async fn hosted_silent_review_does_not_require_a_later_comment_freshness_check() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + let calls = Arc::new(AtomicUsize::new(0)); + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(GitHubLateHeadRaceResponder { + calls: calls.clone(), + }) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "id": 42, + "full_name": "acme/api" + }))) + .mount(&server) + .await; + mount_successful_hosted_check_patches(&server).await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + disable_review_for_hosted_publication(dir.path(), false); + hosted_publish_command(dir.path(), &server).assert().code(0); + assert_eq!( + calls.load(Ordering::SeqCst), + 4, + "a no-comment result must stop after the required check delivery freshness reads" + ); +} + #[tokio::test] async fn github_unresolved_inline_line_falls_back_once_to_summary_only() { let server = MockServer::start().await;