From 47e4d3ff0bac9b101b2ab259e88bb5308d2181b2 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 23 Aug 2026 23:33:39 +0000 Subject: [PATCH 1/2] Recover from an unusable incremental baseline with a full review An incremental review asks GitHub to compare the previously reviewed head against the current one. When the author rebases or force-pushes, the recorded baseline is no longer an ancestor of the head and the compare cannot describe the change; a compare that reaches the documented 300-file response cap cannot describe it either. Both are correct refusals on the incremental path, but treating them as terminal failed the whole run and told the author to retry something that no later attempt can fix, because the baseline commit is gone. The refusals now carry a marker distinguishing "this baseline is unusable" from "the forge is unavailable", and the review falls back in-run to a full review of the same head, mirroring the existing same-head recovery. The incremental path still refuses to pass off a partial compare as coverage, and genuine forge outages still fail the run. --- ARCHITECTURE.md | 4 +- docs/automation.md | 2 + src/forge/github.rs | 55 ++++++++++++++--- src/forge/mod.rs | 48 +++++++++++++++ src/review.rs | 63 +++++++++++++------ tests/e2e.rs | 146 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 292 insertions(+), 26 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 675cb53..e52a949 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -119,7 +119,9 @@ acquire diff --> parse supported lockfiles --> parse + index --> bounded evidenc the head commit, target-branch commit, and computed merge base separately. Truncated transports and metadata mismatches fail closed. GitHub reconstructs full reviews from merge-base/head file content after exhausting the declared - changed-file count, and rejects an ambiguous 300-file incremental compare. Bitbucket exhausts + changed-file count, and rejects an ambiguous 300-file incremental compare. A rejected + incremental compare, whether the head no longer descends from the requested baseline or the + response reached the file cap, falls back in-run to a full review of the same head. Bitbucket exhausts paginated diffstat and reconstructs bounded source content from the compared commits. Azure has no PR-diff endpoint, so it exhausts the authoritative change marker and reconstructs a unified diff from changed-file content with `similar`. The gate check is diff --git a/docs/automation.md b/docs/automation.md index b6c06a6..e51d299 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -20,6 +20,8 @@ postil review \ Postil reviews new commits, carries unresolved findings forward, and marks findings as resolved when the relevant code changes. +When the baseline cannot describe the change, because a rebase or force-push left it off the head's ancestry or the forge truncated the compare, the run reviews the complete change at the same head instead of failing. Retrying such a run cannot help, so the recovery happens in-run. + ## Preview policy changes `postil plan` replays stored envelopes through a candidate configuration without making a model call: diff --git a/src/forge/github.rs b/src/forge/github.rs index fa65113..409399e 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -2915,17 +2915,22 @@ impl Forge for GitHub { "GitHub compare response", ) .await?; - ensure!( - compare.merge_base_commit.sha == since_sha, - "GitHub incremental compare no longer descends from the requested baseline; refusing an incomplete review" - ); + // Both refusals below are properties of the requested baseline rather + // than of the change itself, so they carry the marker that lets the + // caller review the complete change instead of failing the run. + if compare.merge_base_commit.sha != since_sha { + return Err(super::incremental_diff_unavailable( + "GitHub incremental compare no longer descends from the requested baseline; refusing an incomplete review", + )); + } // GitHub documents that compare responses include at most 300 files // and expose no complete file count. Exactly 300 is therefore // ambiguous and must fail closed. - ensure!( - compare.files.len() < 300, - "GitHub compare reached the 300-file response cap; refusing an incomplete incremental review" - ); + if compare.files.len() >= 300 { + return Err(super::incremental_diff_unavailable( + "GitHub compare reached the 300-file response cap; refusing an incomplete incremental review", + )); + } self.build_complete_diff( compare.files, since_sha, @@ -7613,6 +7618,40 @@ mod tests { Err(error) => error, }; assert!(error.to_string().contains("no longer descends")); + assert!(crate::forge::is_incremental_diff_unavailable(&error)); + } + + #[tokio::test] + async fn github_incremental_diff_rejects_a_capped_compare_response() { + let server = MockServer::start().await; + let files: Vec<_> = (0..300) + .map(|index| { + serde_json::json!({ + "filename": format!("src/file{index}.rs"), + "status": "modified", + "changes": 1 + }) + }) + .collect(); + Mock::given(method("GET")) + .and(path("/repos/owner/repo/compare/aaaaaaaa...bbbbbbbb")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "merge_base_commit": {"sha": "aaaaaaaa"}, + "files": files + }))) + .expect(1) + .mount(&server) + .await; + + let error = match test_github(&server) + .fetch_diff_since("aaaaaaaa", "bbbbbbbb") + .await + { + Ok(_) => panic!("capped incremental compare was accepted"), + Err(error) => error, + }; + assert!(error.to_string().contains("300-file response cap")); + assert!(crate::forge::is_incremental_diff_unavailable(&error)); } #[tokio::test] diff --git a/src/forge/mod.rs b/src/forge/mod.rs index ca3f453..6b1222d 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -56,6 +56,31 @@ impl std::fmt::Display for IncompleteReviewInput { impl std::error::Error for IncompleteReviewInput {} +/// Marks a refusal that is specific to the requested incremental baseline: the +/// head no longer descends from it, or the forge truncated the compare. The +/// complete change at the same head is still reviewable, so a caller may +/// recover by falling back to a full review instead of failing the run. +#[derive(Debug)] +pub struct IncrementalDiffUnavailable; + +impl std::fmt::Display for IncrementalDiffUnavailable { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("incremental diff is unavailable for this baseline") + } +} + +impl std::error::Error for IncrementalDiffUnavailable {} + +pub fn incremental_diff_unavailable(message: impl Into) -> anyhow::Error { + anyhow::Error::new(IncrementalDiffUnavailable).context(message.into()) +} + +pub fn is_incremental_diff_unavailable(error: &anyhow::Error) -> bool { + error + .chain() + .any(|cause| cause.downcast_ref::().is_some()) +} + #[derive(Debug)] pub struct RepositoryIdentityFailure(pub String); @@ -2974,6 +2999,29 @@ mod tests { assert!(is_incomplete_review_input(&unauthorized)); } + #[test] + fn incremental_baseline_refusals_stay_recognizable_after_classification() { + let refusal = incremental_diff_unavailable( + "GitHub incremental compare no longer descends from the requested baseline", + ); + assert!(is_incremental_diff_unavailable(&refusal)); + assert!( + refusal + .to_string() + .contains("no longer descends from the requested baseline") + ); + + // The caller inspects the error only after classification wraps it, so + // the marker has to survive the added context layers. + let classified = classify_review_input_error(refusal).context("incremental diff fetch"); + assert!(is_incremental_diff_unavailable(&classified)); + assert!(is_incomplete_review_input(&classified)); + + // A forge outage is not a baseline problem and must still fail the run. + let outage = classify_review_input_error(service_failure("compare fetch failed")); + assert!(!is_incremental_diff_unavailable(&outage)); + } + #[test] fn forge_summary_keeps_incomplete_review_reasons_generic() { for reason in [ diff --git a/src/review.rs b/src/review.rs index a453e11..fcba4da 100644 --- a/src/review.rs +++ b/src/review.rs @@ -988,24 +988,53 @@ async fn remote_review( let has_carryable_baseline = baseline_has_carryable_findings(&baseline); let incremental = args.since_sha.as_deref(); let (diff_snapshot, scope, force_model) = match incremental { - Some(since) if since != head_sha => run_with_hosted_budget( - Some(review_started), - FORGE_READ_TIMEOUT_SECS, - forge.fetch_diff_since(since, head_sha), - "fetching incremental diff", - ) - .await - .map_err(crate::forge::classify_review_input_error) - .context("incremental diff fetch") - .map(|diff| { - ( - diff, - filter::ReconcileScope::Incremental { - trust: filter::ReviewTrust::Failed, - }, - false, + Some(since) if since != head_sha => { + let incremental_diff = run_with_hosted_budget( + Some(review_started), + FORGE_READ_TIMEOUT_SECS, + forge.fetch_diff_since(since, head_sha), + "fetching incremental diff", ) - })?, + .await + .map_err(crate::forge::classify_review_input_error) + .context("incremental diff fetch"); + match incremental_diff { + Ok(diff) => ( + diff, + filter::ReconcileScope::Incremental { + trust: filter::ReviewTrust::Failed, + }, + false, + ), + // The baseline itself is unusable: the head no longer descends + // from it (a rebase or force-push), or the forge truncated the + // compare. Retrying cannot recover a baseline that is gone, and + // the incremental path must keep refusing a diff that would + // understate coverage, so review the complete change at the + // same head instead of failing the run. + Err(error) if crate::forge::is_incremental_diff_unavailable(&error) => { + eprintln!( + "postil: incremental baseline {since} is unusable ({error:#}); reviewing the complete change instead" + ); + ( + run_with_hosted_budget( + Some(review_started), + full_diff_timeout_secs(meta), + forge.fetch_diff(meta), + "fetching full fallback diff", + ) + .await + .map_err(crate::forge::classify_review_input_error) + .context("full diff fallback fetch")?, + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + }, + true, + ) + } + Err(error) => return Err(error), + } + } Some(_) => ( diff::DiffSnapshot::from_bytes(b"")?, filter::ReconcileScope::Incremental { diff --git a/tests/e2e.rs b/tests/e2e.rs index 92cc3c6..af19308 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -12391,6 +12391,152 @@ async fn same_head_without_open_baseline_keeps_empty_diff_noop() { ); } +#[tokio::test] +async fn stale_incremental_baseline_falls_back_to_full_review() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + // A rebase or force-push leaves the recorded baseline off the head's + // ancestry, so the incremental compare cannot describe the change. + Mock::given(method("GET")) + .and(path("/repos/acme/api/compare/cccccccc...aaaaaaaa")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "merge_base_commit": {"sha": "dddddddd"}, + "files": [] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, "base": {"sha": "bbbbbbbb"}, "changed_files": 1 + }))) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--since-sha", + "cccccccc", + "--no-post", + "--output-json", + ]) + .assert() + .code(0); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["findings"], json!([])); + assert!( + !env["findings"] + .as_array() + .unwrap() + .iter() + .any(|finding| finding["path"] == ".postil/model-output"), + "stale baseline produced an operational failure instead of a review" + ); + assert_eq!(env["gate"]["failing"], false); + assert_ne!(env["modelUsed"], "none (empty diff)"); + + let requests = server.received_requests().await.unwrap(); + assert!( + requests + .iter() + .any(|request| request.url.path() == "/repos/acme/api/pulls/7/files"), + "fallback did not acquire the complete change" + ); + let llm_request = requests + .iter() + .find(|request| request.url.path() == "/chat/completions") + .unwrap(); + let body: Value = llm_request.body_json().unwrap(); + assert!( + !body["messages"][1]["content"] + .as_str() + .unwrap() + .contains("This is an INCREMENTAL review"), + "fallback reviewed the change as incremental" + ); +} + +#[tokio::test] +async fn incremental_forge_outage_still_fails_the_review() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/compare/cccccccc...aaaaaaaa")) + .respond_with(ResponseTemplate::new(503)) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path_regex(r"^/repos/acme/api/compare/b+\.\.\.a+$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "merge_base_commit": {"sha": "bbbbbbbb"}, + "files": [] + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7/files")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .expect(0) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, "base": {"sha": "bbbbbbbb"}, "changed_files": 1 + }))) + .mount(&server) + .await; + + let dir = tempfile::tempdir().unwrap(); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--since-sha", + "cccccccc", + "--no-post", + "--output-json", + ]) + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["findings"][0]["path"], ".postil/provider"); + assert_eq!(env["gate"]["failing"], true); +} + #[tokio::test] async fn carried_only_incremental_run_updates_checks_without_posting_review() { let server = MockServer::start().await; From e5c92f431f6854c1f0aedd0e70bc9734f22614ca Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 23 Aug 2026 23:40:18 +0000 Subject: [PATCH 2/2] Report no incremental baseline on a review that measured the full change The review envelope's `sinceSha` names the baseline the review was measured against and is absent on a full review. A run that requested an incremental baseline but could not use it, because the head no longer descends from it or the forge truncated the compare, reviews the complete change instead; echoing the requested baseline back would describe that run as incremental and break the field's contract for every consumer that reads it. The field is now derived from the reconciliation scope the review actually used, which covers the same-head recovery on the same terms. Genuine incremental runs are unaffected. --- docs/automation.md | 2 +- src/review.rs | 7 +++++-- tests/e2e.rs | 6 ++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index e51d299..64518b0 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -20,7 +20,7 @@ postil review \ Postil reviews new commits, carries unresolved findings forward, and marks findings as resolved when the relevant code changes. -When the baseline cannot describe the change, because a rebase or force-push left it off the head's ancestry or the forge truncated the compare, the run reviews the complete change at the same head instead of failing. Retrying such a run cannot help, so the recovery happens in-run. +When the baseline cannot describe the change, because a rebase or force-push left it off the head's ancestry or the forge truncated the compare, the run reviews the complete change at the same head instead of failing. Retrying such a run cannot help, so the recovery happens in-run. `sinceSha` names the baseline a review was measured against, so it is null on any run that reviewed the complete change. ## Preview policy changes diff --git a/src/review.rs b/src/review.rs index fcba4da..b58352b 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1875,7 +1875,7 @@ async fn review_diff_at( cfg, meta, head_sha, - args.since_sha.clone(), + incremental.then(|| args.since_sha.clone()).flatten(), admission, ReviewCoverage { mode: if bounded { @@ -2795,7 +2795,10 @@ async fn review_diff_at( duration_ms: review_started.elapsed().as_millis() as u64, base_sha: meta.map(|m| m.base_sha.clone()), head_sha, - since_sha: args.since_sha.clone(), + // `sinceSha` names the baseline this review was measured against, so it + // is absent on a full review. A requested baseline that the run could + // not use is not the reviewed baseline. + since_sha: incremental.then(|| args.since_sha.clone()).flatten(), }) } diff --git a/tests/e2e.rs b/tests/e2e.rs index af19308..b1336c6 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -12307,6 +12307,7 @@ async fn same_head_with_open_baseline_falls_back_to_full_review() { assert_eq!(env["suppressedFindings"][0]["reason"], "nonActionable"); assert_ne!(env["modelUsed"], "none (empty diff)"); assert_eq!(env["gate"]["failing"], false); + assert_eq!(env["sinceSha"], Value::Null); let requests = server.received_requests().await.unwrap(); assert!( @@ -12456,6 +12457,11 @@ async fn stale_incremental_baseline_falls_back_to_full_review() { ); assert_eq!(env["gate"]["failing"], false); assert_ne!(env["modelUsed"], "none (empty diff)"); + assert_eq!( + env["sinceSha"], + Value::Null, + "a full review reported an incremental baseline it did not measure against" + ); let requests = server.received_requests().await.unwrap(); assert!(