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..64518b0 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. `sinceSha` names the baseline a review was measured against, so it is null on any run that reviewed the complete change. + ## 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..b58352b 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 { @@ -1846,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 { @@ -2766,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 92cc3c6..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!( @@ -12391,6 +12392,157 @@ 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)"); + 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!( + 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;