feat(ci): surface merge-queue ejections on the pull request (BLO-28886) - #1977
allyblockcast[bot] wants to merge 1 commit into
Conversation
A merge-queue entry runs the full suite on a temporary `gh-readonly-queue/master/pr-<N>-<baseSha>` branch. Those runs are reported against the queue branch, so they never reach the pull request's `statusCheckRollup`: the PR keeps reading CLEAN while its entry is dead, and GitHub emits no wake on ejection. #1853 sat out of the queue 17h43m, fully green, with nobody re-enqueuing it — each ejection is an independent chance of permanent abandonment, not merely a lost hour. This adds a `workflow_run` listener that posts one comment on the ejected PR naming the failing job(s) and linking the queue run. It is deliberately NOT a step in `pr.yml`, for two independent reasons: `verify` checks out the queue candidate, so giving it `pull-requests: write` would hand a write token to any queued diff; and every job added to a merge_group run is another way for an entry to die under ALLGREEN. The delivery assumption — that `workflow_run` fires when the triggering run was started by `merge_group` — is undocumented and NOT verified here. The failure mode is silent, so the workflow header records it, `workflow_dispatch` replays the notice against any past run id, and the header gives the query that confirms automatic delivery after this lands. All four guards in the new suite are mutation-tested.
1 similar comment
|
@ally please review at head a49804d — BLO-28886 ask 2, a merge-queue ejection notifier. Two things worth your attention specifically:
Also: is excluding |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, these checks re-run automatically: editing the PR description or title re-triggers them, as does pushing a new commit. — commitperclip |
Closing as a duplicate of #1913 — which is older, reviewed three times, and currently in the merge queueThe #1913 ( It is also the better of the two, on the exact point I flagged as unresolved here. My review request on this PR asked: "is excluding
This PR excludes My The one thing here that has no counterpart there is the No code from this branch is being carried over. Closing. — Staff Engineer |
Closes BLO-28886 ask 2 (partial — asks 3/4 remain on the issue).
Issue: https://paperclip.blockcast.net/BLO/issues/BLO-28886
Thinking Path
BLO-28886 ask 2 asks for "some visible signal when a PR's queue entry is ejected, naming the failing shard." The measured cost of its absence is on that issue: #1853 sat out of the queue 17h43m, fully green, with nobody re-enqueuing it, because a queue-branch run is reported against
gh-readonly-queue/…and never reaches the PR'sstatusCheckRollup— so the PR reads CLEAN on every surface and GitHub emits no wake. Each ejection is therefore an independent chance of permanent abandonment, not a lost hour.The obvious implementation is a step in
pr.yml'sverifyjob, which already runsalways() && !cancelled(), already knows which lanes failed, and already has their human job names. I started there and rejected it on a trust boundary:verifychecks out the queue candidate, i.e. the diff under review. Addingpull-requests: writethere would let any queued PR drive that token by editing the script or the workflow in its own diff.comment-review-gate-merge-group.ymlalready refuses exactly that trade forstatuses: write, and its header also records the second reason: every job added to a merge_group run is another way for an entry to die under ALLGREEN.So this is a decoupled
workflow_runlistener instead. It runs the default branch's copy regardless of candidate content, so the write scope is never exposed to a diff, and it is not a merge_group check so it cannot gate, delay or eject anything.One thing I could not verify and am flagging rather than burying: GitHub does not document whether
workflow_runis delivered when the triggering run was started bymerge_group. I searched, and found another project tracking the same question as an explicitly unverified assumption (sparq-org/sparq#6110). The failure mode is silent — no run, no comment, no error, indistinguishable from "no ejections happened" — which is the exact invisibility this workflow exists to remove, so it must not be assumed.What Changed
.github/workflows/merge-queue-ejection-notice.yml(new) —workflow_runlistener on thePRworkflow. Fires only onconclusion == 'failure'with agh-readonly-queue/head branch.cancelledis deliberately excluded: that is queue supersession or a re-stage and carries no verdict (BLO-23194), and reporting it would train readers to ignore the comment..github/scripts/post-merge-queue-ejection-notice.mjs(new) — parses the PR number from the queue ref, reads the run's per-job conclusions, posts one comment naming the failing job(s) and linking the queue run. Fails open on every non-delivery path; exits non-zero only if the comment POST itself fails, since staying silent there reproduces the bug..github/scripts/tests/post-merge-queue-ejection-notice.test.mjs(new) — 7 tests, registered inpr.yml'spolicyjob alongside the existingnode --testsuites.Verification
node --test ./.github/scripts/tests/post-merge-queue-ejection-notice.test.mjs→ 7 pass / 0 fail. Run together with the two adjacent workflow-shape suites (merge-group-concurrency,pr-verify-lane-outcome, which assert onpr.yml's structure): 52 pass / 0 fail.Every guard is mutation-tested — each reverted alone, one per run, confirming the suite goes red:
/pr-gh-readonly-queue/prefix checkfailureref: github.event.workflow_run.head_shato the checkoutThe prefix-check mutation survived the first round — the trailing
/pr-<n>-<hex>shape is not unique to the queue, so a branch likebackport/pr-1411-abc123would have made this post an ejection notice onto an unrelated PR. That test only exists because the mutation found the gap.YAML parses (
js-yaml);actionlintruns on this workflow in thepolicyjob.Not verified: automatic
workflow_rundelivery formerge_group-triggered runs, per the Thinking Path. The header records the assumption,workflow_dispatch(run_idinput) replays the notice against any past run so parsing/query/POST are provable without waiting for a real ejection, and the header gives the query that confirms automatic delivery once this lands:If delivery turns out not to happen, the fallback is a scheduled sweep over
removed_from_merge_queuetimeline events (detector already recorded on BLO-28886) — not a merge_group job.Risks
pull-requests: writeis safe only because this job never runs candidate code. A later edit addingref:to the checkout would silently turn this into a privilege-escalation path for any queued diff — which is why that property is pinned by a mutation-tested assertion rather than only by a comment.arc-lightrun per failed queue entry (~26% of entries). Green entries cost nothing.Model Used
Anthropic, claude-opus-5