fix: evaluate backport approval from live labels to prevent check race - #422
Merged
Conversation
On manually-opened backport PRs, the shared pull_request handler evaluated the Backport Approval Enforcement verdict from the webhook payload's label snapshot, which is frozen at delivery time and can never contain the backport/requested label that updateManualBackport adds mid-handler. A slow invocation carrying a stale payload could then complete the check run last with 'Backport Approval Not Required', and the labeled-event safety net no-op'd whenever the run was already queued, so the stale conclusion stood and auto-merge could proceed without approval. - Evaluate the approval verdict from live labels fetched via the API (labelExistsOnPR) instead of the payload's label array. - Replace the queued no-op with an idempotent write of the pending state, so a labeled/unlabeled event corrects a stale conclusion instead of returning early. - Deduplicate check-run creation: queueBackportApprovalCheck now re-lists check runs for the head SHA and resets the existing run to queued by id instead of creating another run. - When backport/approved is removed, keep the check pending after re-adding backport/requested rather than stamping success.
jkleinsc
approved these changes
Aug 19, 2026
jkleinsc
marked this pull request as ready for review
August 19, 2026 18:14
VerteDinde
approved these changes
Aug 19, 2026
dsanders11
pushed a commit
that referenced
this pull request
Aug 25, 2026
…ting them (#423) * fix: supersede completed backport approval check runs instead of updating them The Checks API treats a completed check run as terminal: a PATCH asking to move it back to 'queued' succeeds and applies the output, but silently keeps the old status and conclusion. The dedupe path added in #422 therefore could not un-green a run that had already concluded success - the labeled-event reset only rewrote the output text, leaving a green check whose output read 'Needs Backport Approval' (observed on electron/electron#53035). - queueBackportApprovalCheck now only updates an existing run in place while it is still pending; a completed run is superseded by a fresh queued run, which branch protection consults as the latest run per name. - The opened-event evaluation no longer concludes 'not required' for backport PRs authored by trop itself: trop adds labels in a separate API call shortly after creating the PR, so the live labels are still empty in that window. The check stays queued until the labeled events that follow trop's own label writes settle the verdict. * fix: keep opened backport approval check pending for all declared backports The opened-event guard special-cased backports authored by trop's bot user, but the window it protects against is not author-specific: a declared backport's labels are always written by trop itself - backportImpl labels trop-created backports in a separate API call after opening them, and updateManualBackport labels manually-opened backports with at least the base-ref label - so labels can land after the opened delivery for any author, and webhook deliveries can be delayed or reordered besides. Gate the guard on the author-agnostic property that actually matters: whether the PR body declares 'Backport of #N'. Every declared backport is guaranteed at least one trop-written label and therefore a labeled event that settles the verdict, so opened can safely leave the check pending. PRs without a declaration (e.g. fast-track PRs targeting release branches) get no guaranteed labeled event and would hang queued forever, so opened still concludes 'not required' for them. Also skip the re-queue when the check has already concluded: a labeled delivery processed before a late opened delivery has already settled the verdict from the same live labels, and superseding that concluded run would leave a queued check no follow-up event ever completes. --------- Co-authored-by: Claude <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
3 participants
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.
Requested by John Kleinschmidt · Slack thread
Before: on a manually-opened backport PR, concurrent webhook deliveries could create duplicate "Backport Approval Enforcement" check runs, and a slow invocation holding a stale label snapshot could stamp the latest run "Backport Approval Not Required" — even though trop itself had just added
backport/requested 🗳— letting auto-merge proceed without approval. This is exactly what happened on electron/electron#52973.After: the verdict is evaluated from the PR's live labels at write time, a
labeled/unlabeledevent always writes the correct state to the existing run (including completing/resetting one that is alreadyqueued), and duplicate check runs are no longer created.This makes the "Backport Approval Enforcement" check evaluate against live labels and write idempotently to a single check run, closing the race that let a backport requiring approval merge with a green check.
How
src/index.ts): the approval verdict (backport/approved ✅/backport/requested 🗳) is now read vialabelExistsOnPR(issues.listLabelsOnIssue) rather thancontext.payload.pull_request.labels. The payload is frozen at delivery time, so for a manual backport it can never contain thebackport/requested 🗳label thatupdateManualBackportadds mid-handler — the very invocation that requested approval would conclude "Not Required".src/index.ts): the pending branch previously returned early when the run's status was alreadyqueued, trusting a snapshot that a concurrent stale invocation could later overwrite. It now always (re-)asserts the pending state idempotently, so a stale conclusion gets corrected instead of standing forever. Relatedly, whenbackport/approved ✅is removed andbackport/requested 🗳is re-added, the check now goes back to pending rather than briefly concluding success.src/utils/checks-util.ts):queueBackportApprovalCheckre-lists the check runs for the head SHA immediately before writing and, if a run named "Backport Approval Enforcement" already exists, resets it toqueuedby run id instead of creating another run (six duplicates were created within 11 seconds during the incident). This narrows the check-then-create race substantially; fully atomic creation isn't possible with the Checks API, but with the verdict now computed from live labels a rare duplicate can no longer carry a stale conclusion.backport/requested 🗳is only present in live labels (post-snapshot) staying pending instead of concluding "Not Required", (2) alabeledevent on an already-queuedrun re-asserting the pending state instead of no-op'ing, (3) alabeledevent completing aqueuedrun whenbackport/approved ✅is added, and (4)queueBackportApprovalCheckupdating an existing run instead of creating a duplicate.The approval-request gate itself (
shouldRequestBackportApproval, added in #417) is unchanged — this bug is in how the check run's conclusion was computed and written, not in the gate.Refs: electron/electron#52973, #417
Generated by Claude Code