feat(github): revalidate stored check state at merge-queue admission - #977
Draft
aparajon wants to merge 2 commits into
Draft
feat(github): revalidate stored check state at merge-queue admission#977aparajon wants to merge 2 commits into
aparajon wants to merge 2 commits into
Conversation
A PR's required check is evaluated on the synthetic merge-group commit after it enters the merge queue, but SchemaBot posted an unconditional pass there — so a preflight hold landing on the PR head after queue entry could not stop the merge. Re-fold the queued PR's stored check state at admission instead: identify the PR from the merge-group ref, post success only when nothing blocks, and post action_required when a sibling apply's hold (or the PR's own in-flight apply) blocks, so the queue ejects the PR until its checks turn green again. Admission fails closed: an unidentifiable merge-group ref posts a blocking check, and a storage read failure retries the delivery rather than guessing a verdict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the admission check blocks a queued pull request, GitHub removes it from the merge queue and never re-adds it on its own — and the blocking Check Run lives on the synthetic merge-group commit, invisible from the PR page. Without guidance the author watches their merge silently vanish. Post a comment on the ejected pull request naming the blocked databases, explaining that held checks re-plan on their own when the in-flight apply settles, and spelling out the one manual step: queue the pull request again once its checks are green. The comment is idempotent per queue attempt — a hidden marker keyed on the merge-group head SHA deduplicates webhook redeliveries, while a later re-queue gets a fresh comment at the bottom of the timeline. A posting failure retries the delivery; the already-posted checks reconcile idempotently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters
Once a pull request enters a merge queue, branch protection stops evaluating required checks on the PR head and evaluates them on the synthetic merge-group commit instead. SchemaBot posted an unconditional pass on that commit, on the rationale that the PR-head check already gated queue entry — so anything that turned blocking after entry had no way to stop the merge: a preflight hold from a sibling apply, a failed apply, a fail-closed re-plan.
"Sibling apply", throughout: two open pull requests that declare changes to the same database in the same environment are siblings. When an apply starts from one of them, the preflight (#940, #941) flips every sibling's stored check to a hold —
apply_in_flight_on_target— until the apply settles, because each sibling's plan was computed against a schema that is now changing underneath it. A sibling apply is that in-flight apply, seen from the held PR's perspective.The same unconditional pass also launders a check that went stale before entry. Confirmed against a running deployment: a pull request whose aggregate check went green kept that green — same Check Run, same completion timestamp — while a second pull request applied a change to the same database, because every stored check update is keyed to its own pull request and nothing fans out to siblings. The queue then admits it.
#942 closed the last first-party gap on the PR head and left merge-time revalidation as future work. This is that work, and it is what makes the merge queue a real gate rather than a formality. Stack 8/8, on top of #942.
What it does
postMergeGroupAdmissionChecksreplaces the unconditional pass with a re-fold of the queued pull request's stored check state. It identifies the pull request from the merge-group ref, loads its stored checks, and for each aggregate target this instance gates posts success only when nothing blocks for that environment.action_requiredinstead, and the queue removes the pull request. Because the queue never re-adds a pull request on its own — and the blocking Check Run lives on the synthetic merge-group commit, where the author won't find it — the block also posts a guidance comment on the ejected pull request (preview below). It is marker-deduped per queue attempt, keyed on the merge-group head SHA: a webhook redelivery never double-posts, while a later re-queue gets a fresh comment at the bottom of the timeline. A comment-post failure retries the delivery; the already-posted checks reconcile idempotently.postPassingAggregateChecksbecomespostAggregateChecks(conclusion, onEach)plus a single-targetpostAggregateCheck. The default-branch push callers stay unconditional, which is still correct — those commits have already merged.schemabot.merge_gate.merge_group_admission_blocked_total. Any sustained rate means pull requests are reaching the queue and then being ejected, which points at holds landing later than expected rather than at this check.From webhook to verdict
The
merge_groupwebhook names the queued pull request in its ref —pr-481-…→ pull request 481 (a made-up example; parsed from the tail segment, so base branches containing slashes still work; a ref that doesn't fit posts a blocking check rather than guessing):{ "action": "checks_requested", "merge_group": { "head_sha": "0d4f0c9e2ab…", "head_ref": "refs/heads/gh-readonly-queue/main/pr-481-9f8e7d6c5b4…" }, "repository": { "full_name": "acme/schema" } }Admission then reads that pull request's stored checks — the same rows every other gate reads — and posts the verdict on the merge-group commit:
Before / after
Two open pull requests change the same database:
Before — the queue's question gets an unconditional yes:
After — the queue's question is answered from PR B's stored check state, where PR A's apply already left a hold:
What the ejected pull request sees
The guidance comment posted on PR B, at the bottom of its timeline:
How it moves us toward the northstar
The gate's guarantee has been "a passing check means the schema change is applied and verified." Until now that held on the PR head and evaporated at the queue, which is precisely where the merge decision is made. With this, the stored state is authoritative at every point a merge can happen, and the check that gates the merge is the one derived from it.
Worth stating what remains outside this change: a hold landing after admission passes but before the queue merges is still uncovered, since the merge-group Check Run is already recorded by then. The natural follow-up is for the preflight render to flip recorded merge-group Check Runs alongside PR-head ones. Separately, whether a stale PR-head check can arise at all depends on the repository requiring branches to be up to date before merging — a code-host setting SchemaBot neither enforces nor verifies, and worth auditing independently of this change.
The chain: #867 (storage) → #868 (drive-tail recording) → #866 (settle re-plan processor) → #939 (request kinds + hold storage) → #940 (preflight hold fan-out) → #941 (apply-start gate) → #942 (plan-time holds) → this PR (merge-queue admission). Merges bottom-up; each PR retargets to
mainas its base merges.🤖 Generated with Claude Code