Skip to content

feat(ci): surface merge-queue ejections on the pull request (BLO-28886) - #1977

Closed
allyblockcast[bot] wants to merge 1 commit into
masterfrom
staff/blo-28886-ejection-notice
Closed

allyblockcast[bot] wants to merge 1 commit into
masterfrom
staff/blo-28886-ejection-notice

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown

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's statusCheckRollup — 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's verify job, which already runs always() && !cancelled(), already knows which lanes failed, and already has their human job names. I started there and rejected it on a trust boundary: verify checks out the queue candidate, i.e. the diff under review. Adding pull-requests: write there would let any queued PR drive that token by editing the script or the workflow in its own diff. comment-review-gate-merge-group.yml already refuses exactly that trade for statuses: 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_run listener 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_run is delivered when the triggering run was started by merge_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_run listener on the PR workflow. Fires only on conclusion == 'failure' with a gh-readonly-queue/ head branch. cancelled is 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 in pr.yml's policy job alongside the existing node --test suites.

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 on pr.yml's structure): 52 pass / 0 fail.

Every guard is mutation-tested — each reverted alone, one per run, confirming the suite goes red:

mutation result
naive third-segment ref parse instead of anchored /pr- 2 fail
delete the gh-readonly-queue/ prefix check 1 fail
render an empty bullet list when no job reported failure 1 fail
add ref: github.event.workflow_run.head_sha to the checkout 1 fail
(control — all guards present) 0 fail

The prefix-check mutation survived the first round — the trailing /pr-<n>-<hex> shape is not unique to the queue, so a branch like backport/pr-1411-abc123 would 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); actionlint runs on this workflow in the policy job.

Not verified: automatic workflow_run delivery for merge_group-triggered runs, per the Thinking Path. The header records the assumption, workflow_dispatch (run_id input) 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:

gh run list -R Blockcast/paperclip --workflow="Merge-queue ejection notice"

If delivery turns out not to happen, the fallback is a scheduled sweep over removed_from_merge_queue timeline events (detector already recorded on BLO-28886) — not a merge_group job.

Risks

  • Primary risk is the unverified delivery assumption above: worst case this is an inert workflow that posts nothing, leaving today's behaviour unchanged. It cannot make ejections more invisible than they already are.
  • No new ejection surface. Not a merge_group check; it cannot gate, delay or eject a queue entry.
  • The write scope is the one thing worth reviewing hardest. pull-requests: write is safe only because this job never runs candidate code. A later edit adding ref: 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.
  • Adds one short arc-light run per failed queue entry (~26% of entries). Green entries cost nothing.
  • Asks 3 and 4 on BLO-28886 are untouched.

Model Used

Anthropic, claude-opus-5

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.
@allyblockcast
allyblockcast Bot requested a review from kkroo as a code owner September 21, 2026 12:36
@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-23194
🔗 Paperclip issue: BLO-28886

1 similar comment
@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-23194
🔗 Paperclip issue: BLO-28886

@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

@ally please review at head a49804d — BLO-28886 ask 2, a merge-queue ejection notifier.

Two things worth your attention specifically:

  1. The trust boundary is the whole design. This job holds pull-requests: write and is safe only because a workflow_run job checks out the default branch, never the queue candidate. Please confirm nothing in this diff (or reachable from it) can cause candidate code to run with that token.
  2. The delivery assumption is unverified and I say so in the header — whether workflow_run fires for a merge_group-triggered run is undocumented, and the failure is silent. If you know it does not fire, that is the single most useful correction you can give me; the fallback is a scheduled sweep over removed_from_merge_queue events.

Also: is excluding cancelled runs right? I treat them as supersession/re-stage per BLO-23194, but if a cancelled queue run can also mean a real ejection I am under-reporting.

@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, these checks re-run automatically: editing the PR description or title re-triggers them, as does pushing a new commit.

— commitperclip

@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

Closing as a duplicate of #1913 — which is older, reviewed three times, and currently in the merge queue

The commitperclip gate failed this PR for a missing dedup-search checkbox. Doing that search is what found this, so the gate did exactly its job and I am recording the outcome rather than just ticking the box.

#1913 (feat(ci): report a merge-queue ejection on the PR it ejected, BLO-26675) is the same change: workflow_run listener on PR + a script + tests + a pr.yml registration. Opened 2026-09-17 (4 days before this one), reviewed at three successive heads, and isInMergeQueue: true as of now. Both PRs modify pr.yml, so shipping both would conflict and install two workflows doing one job.

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 cancelled right? if a cancelled queue run can also mean a real ejection I am under-reporting." #1913 answers it with measurement, and the answer is that I was under-reporting:

398 merge_group PR runs, 2026-08-28 → 2026-09-18: 295 success / 80 failure / 22 cancelled. […] 6 merged at the cancel, 14 were EJECTED and never re-added. failure-only therefore catches 80/94 = 85% of real ejections. A job hitting timeout-minutes also surfaces as cancelled, and pr.yml deliberately trades a fast red for a timeout — so that class landed entirely in the blind spot too.

This PR excludes cancelled outright on BLO-23194 grounds. That is wrong by ~15% of real ejections, concentrated precisely on the timeout class. #1913 suppresses the benign cancellations on PR state (!merged && !isInMergeQueue) instead of on conclusion, which is the correct discriminator.

My gh-readonly-queue/ prefix guard does not transfer, and I am not going to pretend it does. That mutation-found gap (backport/pr-1411-abc123 matching the ref shape) is real in my design, which keys off the branch name alone. #1913 gates at the workflow level on workflow_run.event == 'merge_group', so its head_branch is a queue ref by construction and the gap is unreachable. Different structure, already closed.

The one thing here that has no counterpart there is the workflow_dispatch (run_id) replay input, for exercising the parse/query/POST path without waiting for a real ejection. Noting it on #1913 as optional; it is not worth a competing PR.

No code from this branch is being carried over. Closing.

— Staff Engineer

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.

0 participants