Skip to content

Recover from an unusable incremental baseline with a full review - #163

Merged
morgaesis merged 2 commits into
mainfrom
fix/incremental-fallback-full
Aug 24, 2026
Merged

morgaesis merged 2 commits into
mainfrom
fix/incremental-fallback-full

Conversation

@morgaesis

Copy link
Copy Markdown
Contributor

An incremental review whose baseline has been rebased away currently fails the run. The GitHub compare guard is right to refuse a diff whose merge base no longer equals the requested baseline, and right to refuse a compare that hit the documented 300-file response cap, because either would let an incomplete diff masquerade as coverage. But the refusal was terminal: the review ended with a "Review incomplete" error envelope telling the author to retry after the forge can supply the change, which it never can, since the rebased-away commits are gone for good. In hosted production this failed a review on every rebase or force-push of an active pull request.

Both refusals now carry a typed marker, and the review falls back in-run to fetching and reviewing the complete change, mirroring the existing same-head recovery: full reconciliation scope, one stderr line naming the unusable baseline. Genuine forge service failures, connect errors and timeouts still fail the run unchanged, and the incremental guards themselves are untouched.

The envelope's sinceSha is now derived from the reconciliation scope the run actually used rather than from the requested baseline, so any run that measured the full change reports sinceSha null, as the envelope contract documents. This also corrects the same-head recovery, which promoted to full scope through the same path and kept the stale field.

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.
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.
@morgaesis
morgaesis force-pushed the fix/incremental-fallback-full branch from ba7a358 to e5c92f4 Compare August 24, 2026 00:28
@morgaesis
morgaesis merged commit cb2f160 into main Aug 24, 2026
10 checks passed
@morgaesis
morgaesis deleted the fix/incremental-fallback-full branch August 24, 2026 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant