Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions docs/automation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
55 changes: 47 additions & 8 deletions src/forge/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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]
Expand Down
48 changes: 48 additions & 0 deletions src/forge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>) -> 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::<IncrementalDiffUnavailable>().is_some())
}

#[derive(Debug)]
pub struct RepositoryIdentityFailure(pub String);

Expand Down Expand Up @@ -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 [
Expand Down
70 changes: 51 additions & 19 deletions src/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -988,24 +988,53 @@ async fn remote_review<F: Forge>(
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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
})
}

Expand Down
Loading