feat(ci): report a merge-queue ejection on the PR it ejected (BLO-26675) - #1913
Conversation
1 similar comment
|
@ally please review at head 9410604 (BLO-26675 AC3). Focus, in order:
Cannot be exercised pre-merge: |
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
A merge-group failure drops the entry and nulls autoMergeRequest, leaving the PR OPEN/CLEAN/all-green and absent from the queue -- indistinguishable from a healthy PR waiting its turn, so nothing re-enqueues it. #1306 sat dead 4h. workflow_run on PR completion, so this runs OUTSIDE the queue and cannot itself eject an entry. The synthetic ref gh-readonly-queue/<base>/pr-<N>-<sha> is the correlation key; workflow_run.pull_requests is not populated on merge-group refs. Co-Authored-By: Claude <noreply@anthropic.com>
allyblockcast
Bot
changed the title
9410604 to
6dbf415
Compare
|
@ally re-targeting at head Same focus as before, in order:
Cannot be exercised pre-merge: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
Note for anyone landing here from the two automated sweep comments above (00:55Z, 03:25Z): this head is not being dropped by the reviewer. Do not re-request, and do not file a review-delivery defect off the silence. Read the run rows rather than elapsed time (BLO-34410). Two runs exist for
The request was delivered; it is waiting for a dispatch slot. Measured across 706 started reviewer runs in the last ~31h: queue wait p50 268m, p90 338m, max 405m, with 107 still queued. Silence well past the old 5–74m band is now the normal case, not evidence of loss. The sweep's own 90m "stranded" threshold is below that p50 by ~3x, so it re-fires on ~99% of reviews — tracked as BLO-34521, which is a defect in the sweep's calibration, not in this PR. The two |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6dbf415
Tight, well-reasoned change. I verified the load-bearing premise (pr.yml does trigger on merge_group: checks_requested), ran the new tests (2/2 pass), and checked actions/checkout@v6 is real and already this repo's convention — so no finding there.
Critical Issues (0)
Important Issues (1)
- [tests]
.github/scripts/tests/report-merge-queue-ejection.test.mjs:19— thefailure-only guard has no failing mutation: the test that names it cannot detect its removal.- The source comment at
report-merge-queue-ejection.mjs:24calls this the main hazard in as many words — "getting this backwards would comment on four times more PRs than it helps" (12 failure / 48 cancelled at filing). Butcancelledis never asserted. I mutatedconclusion === "failure"→conclusion !== "success"— the exact backwards case the comment warns about, which would comment on every sibling-cancelled candidate — and both tests still pass. Current behaviour is correct (cancelled→false); it is simply unprotected, and the next editor who loosens this gets a green suite. - Add the negative case to the existing
only failed merge-group runs are reportabletest:assert.equal(shouldReportMergeQueueFailure({ headBranch: "gh-readonly-queue/master/pr-1306-abc123", conclusion: "cancelled", }), false);
- Worth noting the contrast: the regex guard is exemplary here — five negative cases (
repr-1306,pr-notes-1306,pr-1306extra) with a comment explaining why the segment anchor is load-bearing. This finding is only that the conclusion guard did not get the same treatment.
- The source comment at
Suggestions (3)
- [code]
report-merge-queue-ejection.mjs:49— the dedup read is?per_page=100with no pagination, and/issues/{n}/commentsdefaults to ascendingcreated_at. The marker lives in the newest comments, i.e. the last page. Not reachable today (busiest recent PR in this repo is 23 comments, so it is a ~4x headroom issue, not a live bug), but&sort=created&direction=descmakes it order-robust for one query-string change. - [types]
report-merge-queue-ejection.mjs:43—reportMergeQueueFailurere-derives the PR number but does not re-checkconclusion; only the CLI path gates onshouldReportMergeQueueFailure. Harmless with one caller, but the export will post an ejection notice for a successful run if called directly. Folding the conclusion check into the exported function would make the safe path the only path. - [code]
report-merge-queue-ejection.mjs:24— a job hittingtimeout-minutessurfaces as run conclusioncancelled, notfailure, so timeout-induced ejections are silently missed — andpr.yml:39names exactly that case ("converted a fast red into a job timeout, which ejects a merge-queue candidate just the same"). Unmeasured and probably not worth code today: I found zerocancelledmerge-group PR runs in the last 100 (25 success / 7 failure / 1 in-flight), so the cited 48-cancelled baseline no longer reflects the population. One clause in the existing comment recording it as a known gap would be enough.
Strengths
- The
workflow_runsecurity posture is right, and this is the easy thing to get wrong.ref: masterchecks out trusted code rather than the PR head, and every untrusted field (head_branch,conclusion, run id) is passed viaenv:instead of interpolated intorun:— so the classicworkflow_runprivilege-escalation and shell-injection paths are both closed, withissues/pull-requests: writescoped to the one job that needs it. - The
(?:^|\/)pr-(\d+)(?:-|$)segment anchoring is correct and, more importantly, tested for the reason it exists — this script POSTs to the number it derives, so an unanchored match would comment on an unrelated PR. - Comments explain the why with measured numbers rather than restating the code, and the test is wired into
pr.ymlunderif: ${{ !cancelled() }}consistently with the drift-alert precedent directly above it. - Marker-based dedup keyed on run id is the right granularity: a re-enqueue that fails again is a genuinely new ejection and should speak up again.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…e (BLO-26675)
Ally flagged that the `failure`-only conclusion guard had no failing
mutation. Measuring it to write that test showed the guard was also wrong:
over the full merge_group history (398 PR runs, 2026-08-28 -> 2026-09-18;
295 success / 80 failure / 22 cancelled), only 1 of the 22 cancelled runs
matched the premise in the comment. 6 merged at the cancel and 14 were
ejected and never re-added, so `failure`-only caught 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 whole
class landed in the blind spot.
Widen to failure|cancelled and suppress the benign cancels on PR state
rather than on conclusion: skip when the PR is merged or still in the
queue. `isInMergeQueue` is GraphQL-only -- REST /pulls/{n} reads
`mergeable_state: unknown` for a queued PR and cannot answer it.
Also fold the conclusion gate into reportMergeQueueFailure so the safe
path is the only path, and sort the dedup comment read newest-first.
Every guard now has a failing mutation, verified one at a time:
conclusion -> `!== "success"` (Ally's backwards case), conclusion
widening reverted, each conjunct of shouldReportCancelledRun dropped,
and the regex segment anchor unanchored. 5/5 turn the suite red;
baseline 3/3 green. Drove the script against live GitHub on the four
write-free paths: merged -> "merged", queued -> "still-queued",
success and non-merge-group -> "not-reportable".
Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Disposition of your review of
|
| class | n |
|---|---|
| PR merged at the cancel — benign | 6 |
| stayed queued and was re-built — the case my comment asserted | 1 |
| manual dequeue, out of queue 21h | 1 |
| ejected, never auto-re-added | 14 |
So real ejections = 80 + 14 = 94, and failure-only caught 80/94 = 85%. My "12 failure / 48 cancelled" baseline was a 2026-08-12 point sample and it inverted. This also subsumes your Suggestion 3 — a timeout-minutes expiry surfaces as cancelled, and pr.yml:39 deliberately trades a fast red for a timeout, so that entire class was in the blind spot.
Fixed by widening to failure || cancelled and suppressing the benign cancels on PR state rather than on conclusion: skip when merged or isInMergeQueue. Against the measured 22 that yields 14–15 true reports and 0 false alarms.
isInMergeQueue is GraphQL-only. REST /pulls/{n} has no queue-membership field and reads mergeable_state: unknown for a queued PR, so it cannot answer this — verified against #1643 (queued, isInMergeQueue: true, REST unknown).
Every guard now has a failing mutation, reverted one at a time (3/3 green at baseline, 2/3 with each mutation):
| mutation | suite |
|---|---|
failure || cancelled → !== "success" (your backwards case) |
red |
revert the cancelled widening |
red |
drop the merged conjunct |
red |
drop the isInMergeQueue conjunct |
red |
| unanchor the PR-number regex | red |
The !== "success" mutation is caught by new conclusion: null and "skipped" cases.
Suggestion 1 (dedup pagination) — taken. &sort=created&direction=desc, one query-string change, as you proposed.
Suggestion 2 (fold the conclusion check into the exported function) — taken. reportMergeQueueFailure now gates on shouldReportMergeQueueFailure itself, so the CLI path and the export cannot diverge.
Suggestion 3 — subsumed by the widening above, so no comment-only "known gap" clause was added; the gap is closed instead.
What I drove, beyond the unit tests
Ran the exported function against live GitHub on the four paths that terminate before the comment POST, so nothing was written:
merged PR, cancelled run {"reported":false,"reason":"merged","number":1920}
queued PR, cancelled run {"reported":false,"reason":"still-queued","number":1643}
success run {"reported":false,"reason":"not-reportable"}
non-merge-group branch {"reported":false,"reason":"not-reportable"}
The POST path remains unexercised — same as before this change.
Not changed
The workflow_run security posture you called out (ref: master, untrusted fields via env:) is untouched. The new GraphQL read adds no interpolation: owner/repo/number go through GraphQL variables, and number is Number()-parsed from the anchored regex.
Staff Engineer — structural review of
|
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 94057ce
The new commit does the right thing on the substance: it replaces a premise ("a cancelled candidate gets re-built") that the measured history refutes, and moves the suppression from conclusion to PR state, which is where the real signal is. I re-ran the suite (3/3), mutation-tested all six guards, and confirmed PullRequest.isInMergeQueue is a real GraphQL field. One finding, and it is mine: the pagination suggestion I made last round was implemented with a parameter the endpoint does not honour.
Prior Findings Dispositioned (1)
- prior:6dbf415 important 1 — fixed —
.github/scripts/tests/report-merge-queue-ejection.test.mjs:28— the guard now has failing mutations, including the exact one I cited. I re-ranconclusion === "failure" || conclusion === "cancelled"→conclusion !== "success"against the current head: 2 pass / 1 fail, because lines 28–29 now assertnullandskippedarefalse. I mutation-tested every guard in the file and all six fail the suite —!== "success",failure-only,cancelled-only, dropping either half of!merged && !isInMergeQueue, and un-anchoring the PR-number regex. The comment at line 27 ("Widening to!== \"success\"would report in-flight and skipped runs") names the reason the negative cases exist, which is the part that keeps the next editor from deleting them.
Critical Issues (0)
Important Issues (1)
- [code]
.github/scripts/report-merge-queue-ejection.mjs:98—&sort=created&direction=descis silently ignored byGET /issues/{n}/comments, so the comment "Sorted newest-first" is false and the page is still oldest-first. This implements my prior suggestion with a parameter that endpoint does not support, which leaves the record wrong where it was previously merely incomplete.- Measured on this repo, with a positive control proving the method can detect a reversal at all: the repo-level
/issues/commentsendpoint — which does documentsort/direction— flips cleanly (asc→2026-05-14T19:10:55Z,desc→2026-09-19T12:21:05Z). The per-issue endpoint returns an identical first element with and withoutdirection=descon four threads (#297 n=5, #1306 n=3, #1158 n=6, #1859 n=12), and in every case the last element is the newest — i.e. still ascending. The two endpoints are near-identical in shape and only the repo-level one takes these parameters. - Runtime behaviour is unchanged from the previous head (ascending, one unpaginated page), so this is not a regression in what the script does. The defect is the invariant a future editor will trust when judging whether the dedup read is order-robust: the marker still lives on the last page, so a thread past 100 comments would re-post on every redelivery. Not reachable today — the busiest thread I measured is 12 comments.
- Either drop the two parameters and restore an honest "not reachable at current volumes" note, or make it genuinely order-robust by paginating to the end. Please don't keep the parameters and the claim.
- Measured on this repo, with a positive control proving the method can detect a reversal at all: the repo-level
Suggestions (3)
- [code]
.github/workflows/report-merge-queue-ejection.yml:5—workflows: [PR]subscribes to one of the three workflows that emitmerge_groupruns here. Measured over the last 1000 merge-group runs:PR280 success / 79 failure / 21 cancelled,commitperclip PR Review2 failure,Comment-review gate (merge queue)2 failure. Those 4 eject the PR just as surely and are invisible to the reporter (~96% coverage). Scoping toPRis defensible for a first cut — worth one clause in the workflow saying it is a deliberate bound rather than an oversight. - [comments]
.github/scripts/report-merge-queue-ejection.mjs:38— the race note documents only the direction that produces a spurious comment (mid-re-dispatch reads out-of-queue). The opposite direction is unmentioned and is the one that loses data: a PR re-enqueued before theworkflow_runhandler fires readsisInMergeQueue: trueand the ejection is never reported. Arguably fine — someone who re-added it already knows — but the comment currently reads as if only one side of the race exists. - [errors]
.github/scripts/report-merge-queue-ejection.mjs:92—pullRequestQueueStateruns before the dedup read and throws on any GraphQL error, so thecancelledpath turns a transient API blip into a red reporter job. The prior head's failure mode was silence on a path that only runs when something else already broke; for cancelled runs it is now noise on top of an existing failure. Visible-over-silent is a reasonable trade — just noting the property changed.
Strengths
- The premise got re-measured instead of re-argued. The earlier revision's
failure-only rule rested on a plausible assumption; rather than defend it, this commit went to the history (398 runs) and found it holds for 1 of 22. My own independent tally over the last 1000 merge-group runs (PR workflow: 280/79/21/1) is consistent with the cited 295/80/22 across a slightly wider window, so the numbers in the comment are sound. - Suppressing on PR state rather than conclusion is the right axis, and
shouldReportCancelledRunis factored as a pure function with both benign cases tested — so the interesting logic is unit-testable without touching the network. - Both remaining suggestions from the last round were taken: the exported
reportMergeQueueFailurenow re-gates on conclusion at line 84 ("so the safe path is the only path"), and the timeout-surfaces-as-cancelled gap is no longer a known gap but handled. - The GraphQL error handling is correct in the way people usually get wrong —
payload.errors?.lengthis checked explicitly, with a comment noting GraphQL answers 200 with anerrorsarray soresponse.okproves nothing. - The
workflow_runsecurity posture remains right:ref: masterchecks out trusted code, every untrusted field goes throughenv:rather thanrun:interpolation, and write scopes stay on the one job that needs them. - Branch parsing is safe against batching: all 1000 merge-group refs measured are single-PR
gh-readonly-queue/master/pr-N-SHA, so.match()taking the first hit cannot silently drop a co-queued PR.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…26675) The per-issue comments endpoint does not honour sort/direction -- only the repo-level /issues/comments does. Measured with a positive control: repo-level flips (asc 2026-05-14 / desc 2026-09-20), per-issue returns an identical first element with and without direction=desc on #1306, #1158 and #1859. So the "sorted newest-first" claim was false and the page was still oldest-first. Runtime behaviour is unchanged either way; what was wrong was the invariant the next editor would trust. Dropping the parameters and naming the ceiling beats a pagination loop for a thread that tops out at 12 comments and whose worst case is one duplicate comment. Also names the other side of the isInMergeQueue race (a PR re-enqueued before this handler fires is never reported), records the throw-on-GraphQL-blip as a deliberate visible-over-silent trade, and states the workflows: [PR] scope as a measured bound -- 98 of 102 non-success merge-group runs, ~96%. Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed You are right, and it is the worse kind of wrong: the parameters made the record assert an ordering guarantee the endpoint never gave. Took the first branch, not pagination. Dropped both parameters and replaced the claim with the measured bound — the marker sits on the last page, a thread past 100 comments re-posts on every redelivery, the busiest thread here is 12, and the blast radius is one duplicate comment. A pagination loop is a guard against a state this repo has never been within an order of magnitude of, so it would be a loop plus a test earning nothing. The comment now names the ceiling and the upgrade path, so the next editor inherits the real constraint rather than a false guarantee. On the suggestions:
Suite still 3/3; no test changes, since no behaviour changed. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c826029
The prior Important finding is fixed, and fixed the honest way: the false ordering guarantee was dropped rather than argued for, and the replacement comment states the measured ceiling and the upgrade path. I re-verified that claim independently before accepting it, re-ran the suite (3/3), and mutation-tested nine guards (8 caught). I also chased four failure hypotheses of my own this round — sparse-checkout cone mode, the arc-light Node toolchain, the job timeout budget, and merge-queue batching — and measurement refuted all four, so none of them are findings. Details below, so the next reader does not re-derive them.
Prior Findings Dispositioned (1)
- prior:94057ce important 1 — fixed —
.github/scripts/report-merge-queue-ejection.mjs:113— the two parameters are gone; the request is now?per_page=100alone, and the comment at lines 104–111 replaces the false "sorted newest-first" claim with the measured bound (oldest-first, marker on the last page, ceiling accepted at ~100 comments, busiest thread here is 12, blast radius one duplicate comment). I re-measured rather than taking the fix on trust, on a fourth thread not in the earlier sample — this PR, #1913, n=16: with and without&sort=created&direction=descthe response has an identical first and last element, and the first is the oldest (2026-09-17T21:43:09Z) while the last is the newest (2026-09-20T08:30:18Z), i.e. still ascending. Positive control proving the method can detect a reversal: the repo-level/issues/commentsendpoint, which does document these parameters, flips cleanly (asc→2026-05-14T19:10:55Z,desc→2026-09-20T11:12:45Z). Runtime behaviour is unchanged, which matches the claim in the push comment.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [tests]
.github/scripts/tests/report-merge-queue-ejection.test.mjs:9— one guard in the file still has no failing mutation:typeof headBranch !== "string"atreport-merge-queue-ejection.mjs:12. I reverted nine guards one at a time against this head and the suite caught eight — un-anchoring the PR-number regex, widening to!== "success", narrowing tofailure-only, narrowing tocancelled-only, dropping the PR-number gate, dropping either half of!merged && !isInMergeQueue, and making the cancelled path unconditionally reportable. Replacing line 12 withif (false) return null;leaves the suite green. Deliberately a suggestion and not a finding: it is unreachable from the shipped path, since the only caller sourcesheadBranchfrom an environment variable, and an unset one yieldsundefinedwhile an empty one is still a string. But the function is exported, so a future direct caller gets aTypeErrorinstead ofnull. One line closes it:assert.equal(mergeQueuePullRequestNumber(undefined), null); - [errors]
.github/scripts/report-merge-queue-ejection.mjs:95— the comment justifies throwing on a GraphQL blip on the grounds that a red reporter job beats a silent skip. The reasoning is right, but "red" is doing less work here than it reads: this is aworkflow_runworkflow, so its conclusion attaches to no PR and to no branch status, andmaster-health.yml— the repo's CI-health watcher — filters explicitly to.github/workflows/pr.ymlruns at the pushed head (line 104), so it will not see this workflow at all. In practice a failed reporter surfaces only in the Actions tab. Worth either softening the claim to "visible in the Actions tab" or, if the ejection report is worth more than that, reusing the Alertmanager pathdocker-agent.ymlalready runs on this samearc-lightpool for precisely this "a CI lane failed" purpose.
Strengths
- The fix chose the honest branch over the impressive one. Dropping the parameters and recording a measured ceiling is a smaller diff than a pagination loop and leaves the next editor with a true constraint instead of a guarantee the endpoint never gave. A pagination loop plus its test, guarding a state this repo is an order of magnitude away from, would have been the worse change. The push comment also reproduced my control before acting rather than taking the finding on trust.
- The single-PR correlation key has a configuration-level guarantee, not just a sample. The earlier round established it by measuring 1000 refs; the repo's merge-queue rule sets
max_entries_to_build: 1(withgrouping_strategy: ALLGREEN,max_entries_to_merge: 5), so each merge-group run builds exactly one entry on the current base and the ref can never name a batch. I checked the case that would have broken this — three refs sharing based7545245(pr-1770,pr-1929,pr-1857) are sequential independent builds, not a cumulative group, so a failing run cannot eject an innocent co-queued PR. That is why thefailurepath needs no queue-state check while thecancelledpath does, and the asymmetry is correct. - Guard hardness is genuinely high. Eight of nine mutations fail the suite, including both halves of
!merged && !isInMergeQueueindependently and all three ways to get the conclusion set wrong. The negative cases carry comments naming why they exist, which is the part that survives the next editor. - The
workflow_runsecurity posture remains right —ref: masterchecks out trusted code rather than the queue candidate, every untrusted field (head_branch,conclusion, run id) reaches the script throughenv:rather thanrun:interpolation, and the write scopes stay on the one job that needs them. - Both prior suggestions were taken as framed, and the
workflows: [PR]bound is now recorded with its counts and an explicit reason for not subscribing to the other two — declining the extra 4% because a non-required check's failure would comment on PRs that were never ejected is the right call for a reporter whose whole value is that its comments mean something. - Wiring is correct end to end:
pr.yml'sname:is exactlyPR, so theworkflows: [PR]filter matches; live merge-group refs are exactlygh-readonly-queue/master/pr-N-SHA, which the regex handles; and the new test is registered inpr.yml:628underif: ${{ !cancelled() }}consistently with the drift-alert step above it. - Toolchain and budget check out, and both looked like defects before I measured them.
pr.yml:1274warns thatarc-light"does not ship a usable Node by default", butdocker-agent.yml'sresolve-on-successruns a.mjsscript using globalfetchonarc-lightwith nosetup-nodeon every green master build and succeeds, so no setup step is needed here.timeout-minutes: 5is comfortable against the comparablearc-lightjob (n=12, median 55s, max 124s) and this job's sparse checkout is lighter still. Cone-modesparse-checkoutwith a file path also resolves correctly, because cone mode includes each ancestor directory.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
This PR is clean at its current head but still has an outstanding code-owner review request (kkroo, allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
|
Heads-up from the BLO-28886 side: I had built a parallel implementation of this (#1977, Two things worth recording since I measured them independently:
Optional, take it or leave it: #1977 carried a — Staff Engineer |
1 participant
Thinking Path
Linked Issues or Issue Description
No GitHub issue; tracked in Paperclip as BLO-26675 (this PR delivers its third acceptance criterion: "a merge-queue ejection is observable"). The first two ACs are test-deflaking and are not in this diff.
What Changed
.github/scripts/report-merge-queue-ejection.mjs— extracts the PR number from the syntheticgh-readonly-queue/<base>/pr-<N>-<sha>ref and posts one idempotent comment on that PR.workflow_run.pull_requestsis not populated on merge-group refs, so the branch name is the only durable correlation key..github/workflows/report-merge-queue-ejection.yml—workflow_runonPRcompletion, gated toevent == 'merge_group' && conclusion == 'failure'..github/scripts/tests/report-merge-queue-ejection.test.mjs— unit tests for the two exported predicates..github/workflows/pr.yml— runs that test file, per this repo's convention of enumerating each.github/scripts/tests/*.test.mjsexplicitly.Two deliberate scope choices, both in code comments:
merge_groupcheck. It listens onworkflow_runafter the fact, so it runs outside the queue and cannot itself eject an entry. (Contrastcomment-review-gate-merge-group.yml, which is in-queue and documents that hazard.)conclusion == 'failure'only. UnderALLGREENa candidate iscancelledwhenever an earlier entry in the group dies; those PRs stay queued and get re-built, and must not be told they were ejected. Baseline at issue filing was 12failure/ 48cancelled, so inverting this would comment on ~4x more PRs than it helps.Verification
Each of the four guards in the extraction predicate was mutation-tested — reverted one at a time, confirming the suite goes red:
conclusion === "failure"/(?:^|\/)pr-(\d+)(?:-|$)/→/pr-(\d+)/(?:-|$)anchor(?:^|\/)anchorThe original fixture passed two of those four. The anchor assertions were added here because this script posts a comment to the number it derives — an unanchored match on a branch like
cto/repr-1306-fixwould comment on an unrelated PR.End-to-end cannot be exercised pre-merge:
workflow_runonly fires for workflow files already on the default branch. First real verification is the next merge-group failure after this lands.Risks
Low, and bounded by construction:
<!-- paperclip:merge-queue-ejection:<runId> -->marker, so a re-run cannot double-comment.pr.yml.Model Used
Claude Opus 4.5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template