diff --git a/.github/scripts/post-merge-queue-ejection-notice.mjs b/.github/scripts/post-merge-queue-ejection-notice.mjs new file mode 100644 index 000000000000..3706fffdd3d7 --- /dev/null +++ b/.github/scripts/post-merge-queue-ejection-notice.mjs @@ -0,0 +1,135 @@ +#!/usr/bin/env node +/** + * BLO-28886 ask 2: make a merge-queue ejection visible on the pull request. + * + * A queue entry runs the full suite on a temporary + * `gh-readonly-queue/master/pr--` branch. When that run fails the + * entry is ejected, and the PR carries NO trace of it: queue-branch runs do not + * appear in the pull request's `statusCheckRollup`, so the PR keeps reading + * CLEAN/green on every surface while its queue entry is dead. GitHub emits no + * wake on ejection either, so the authoring agent's run is long over and the + * human who enqueued it is never told. + * + * Measured cost of that invisibility (BLO-28886, 2026-09-17): #1853 sat out of + * the queue for 17h43m, fully green, with nobody re-enqueuing it, and the only + * question that would have found it was "which open PRs have a + * `removed_from_merge_queue` event and are not currently in the queue?" — + * which nobody was asking. Each ejection is therefore an independent chance of + * PERMANENT abandonment, not merely a lost hour. At the ~26% terminal + * failure rate re-derived on that issue, that is the dominant cost. + * + * This posts one comment on the ejected PR naming the failing lanes and linking + * the queue run, which turns an invisible ejection into a diagnosable one. + * + * FAILURE POSTURE — read before changing an exit code. This runs from a + * `workflow_run` listener, decoupled from the queue, so it can neither eject an + * entry nor gate anything. It still exits 0 on every non-delivery path it can + * (not a queue ref, unparseable ref, no failing lanes): a notifier that goes red + * is noise on a run that is already reporting someone else's failure. It exits + * non-zero only when the comment POST itself fails, because that is the one case + * where staying silent reproduces the exact invisibility the script exists to + * remove. + */ + +/** + * A queue ref is `gh-readonly-queue//pr--`. The base + * branch may itself contain slashes, and the trailing SHA is hex, so anchor on + * the LAST `/pr-` segment rather than splitting on `/`. + */ +export function parseQueueRef(ref) { + if (typeof ref !== 'string' || !ref.startsWith('gh-readonly-queue/')) return null; + const match = /\/pr-(\d+)-[0-9a-f]+$/.exec(ref); + if (!match) return null; + return { prNumber: Number(match[1]) }; +} + +export function buildEjectionComment({ failedJobs, runUrl, runId, headSha }) { + const shards = failedJobs.length + ? failedJobs.map((name) => `- \`${name}\``).join('\n') + : '- _(no job reported `failure`; see the run for cancelled or killed lanes)_'; + + return [ + '### ⚠️ This PR was ejected from the merge queue', + '', + 'Its queue entry ran the full suite on a temporary `gh-readonly-queue/…` branch and that run failed,', + 'so GitHub removed the entry. **This failure does not appear in this PR\'s check rollup** — the PR above', + 'still reads green, because queue-branch runs are reported against the queue branch, not against the PR.', + '', + '**Failing job(s) in the queue run:**', + '', + shards, + '', + `Queue run: ${runUrl} (\`${runId}\`, candidate head \`${String(headSha).slice(0, 12)}\`)`, + '', + 'If the failure is unrelated to this diff, re-queue the PR — BLO-28886 tracks the underlying', + 'nondeterminism, and the failing shard varies run to run. If it is related, push a fix first.', + '', + 'Posted by `.github/workflows/merge-queue-ejection-notice.yml` (BLO-28886 ask 2).', + ].join('\n'); +} + +async function gh(path, token) { + const response = await fetch(`https://api.github.com${path}`, { + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + }, + signal: AbortSignal.timeout(15_000), + }); + if (!response.ok) throw new Error(`GET ${path} -> ${response.status}`); + return response.json(); +} + +async function main() { + const repo = process.env.GITHUB_REPOSITORY; + const token = process.env.GH_TOKEN; + const headBranch = process.env.RUN_HEAD_BRANCH ?? ''; + const runId = process.env.RUN_ID ?? ''; + const runUrl = process.env.RUN_URL ?? ''; + const headSha = process.env.RUN_HEAD_SHA ?? ''; + + const parsed = parseQueueRef(headBranch); + if (!parsed) { + console.log(`::notice::"${headBranch}" is not a merge-queue ref; nothing to report.`); + return; + } + + const jobs = await gh(`/repos/${repo}/actions/runs/${runId}/jobs?per_page=100`, token); + const failedJobs = (jobs.jobs ?? []) + .filter((job) => job.conclusion === 'failure') + .map((job) => job.name); + + const body = buildEjectionComment({ failedJobs, runUrl, runId, headSha }); + + const response = await fetch( + `https://api.github.com/repos/${repo}/issues/${parsed.prNumber}/comments`, + { + method: 'POST', + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + 'content-type': 'application/json', + }, + body: JSON.stringify({ body }), + signal: AbortSignal.timeout(15_000), + }, + ); + + if (!response.ok) { + const text = await response.text().catch(() => ''); + console.error( + `::error::Could not post the ejection notice on #${parsed.prNumber}: ` + + `${response.status} ${text.slice(0, 300)}`, + ); + process.exitCode = 1; + return; + } + + console.log( + `Posted ejection notice on #${parsed.prNumber} naming ${failedJobs.length} failing job(s).`, + ); +} + +if (import.meta.url === `file://${process.argv[1]}`) { + main(); +} diff --git a/.github/scripts/tests/post-merge-queue-ejection-notice.test.mjs b/.github/scripts/tests/post-merge-queue-ejection-notice.test.mjs new file mode 100644 index 000000000000..b403c6d17682 --- /dev/null +++ b/.github/scripts/tests/post-merge-queue-ejection-notice.test.mjs @@ -0,0 +1,121 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { test } from "node:test"; + +import { + buildEjectionComment, + parseQueueRef, +} from "../post-merge-queue-ejection-notice.mjs"; + +// BLO-28886 ask 2. Everything here pins a property that is unobservable until a +// real ejection has already been missed — which is the failure this notifier +// exists to remove, so it cannot be the thing that detects its own regression. + +test("parses the PR number out of a real queue ref", () => { + assert.deepEqual( + parseQueueRef( + "gh-readonly-queue/master/pr-1411-1ae5079c2da3e352f2a26b42f0c332a1c8eeda63", + ), + { prNumber: 1411 }, + ); +}); + +// The base branch is a path segment and may itself contain slashes, so a naive +// `split('/')[2]` or a non-anchored match reads the wrong segment and the notice +// lands on an unrelated PR — worse than not posting at all. +test("anchors on the last /pr- segment, not the third path segment", () => { + assert.deepEqual( + parseQueueRef("gh-readonly-queue/release/v2/pr-1853-9feaa97d0abc1234"), + { prNumber: 1853 }, + ); +}); + +// A non-queue ref must be a silent no-op: this listener sees every completed PR +// run, and the overwhelming majority are ordinary pull_request runs. +test("returns null for refs that are not merge-queue candidates", () => { + for (const ref of [ + "master", + "staff/blo-28886", + "gh-readonly-queue/master/pr-abc-1ae5079c", + "gh-readonly-queue/master/nope-1411-1ae5079c", + "", + undefined, + null, + ]) { + assert.equal(parseQueueRef(ref), null, `expected null for ${String(ref)}`); + } +}); + +// The `gh-readonly-queue/` prefix check is load-bearing on its own: the trailing +// `/pr--` shape is not unique to the queue, and an ordinary branch that +// happens to match it would make this notifier post an ejection notice onto an +// unrelated PR that was never ejected. Found by mutation-testing — deleting the +// prefix check passed the whole suite until this case existed. +test("a non-queue branch shaped like a queue ref is still rejected", () => { + for (const ref of [ + "backport/pr-1411-1ae5079c", + "gh-readonly-queue-lookalike/master/pr-1411-1ae5079c", + "pr-1411-1ae5079c", + ]) { + assert.equal(parseQueueRef(ref), null, `expected null for ${ref}`); + } +}); + +test("names every failing job in the comment", () => { + const body = buildEjectionComment({ + failedJobs: ["General tests (server 4/4)", "General tests (workspaces-b)"], + runUrl: "https://github.com/Blockcast/paperclip/actions/runs/32224860384", + runId: "32224860384", + headSha: "1ae5079c2da3e352f2a26b42f0c332a1c8eeda63", + }); + + assert.match(body, /General tests \(server 4\/4\)/); + assert.match(body, /General tests \(workspaces-b\)/); + assert.match(body, /actions\/runs\/32224860384/); + // The reader's most likely wrong inference is "my PR is green, so this is + // stale". Saying why the rollup disagrees is the whole point of the comment. + assert.match(body, /check rollup/i); +}); + +// A run can be ejected with zero `failure` jobs (a lane killed mid-run reports +// `cancelled`). Emitting an empty bullet list would read as "nothing failed", +// which is the same false-green this notifier exists to prevent. +test("says so explicitly when no job reported failure", () => { + const body = buildEjectionComment({ + failedJobs: [], + runUrl: "https://example.invalid/run", + runId: "1", + headSha: "abc123def456", + }); + + assert.match(body, /no job reported/i); +}); + +// The trust-boundary argument in the workflow header is load-bearing and is not +// enforced by anything else: a later edit adding `ref:` to the checkout would +// run the queue candidate's code with `pull-requests: write`, silently turning +// this notifier into a privilege-escalation path for any queued diff. +test("the workflow never checks out the queue candidate", () => { + const workflow = readFileSync( + new URL("../../workflows/merge-queue-ejection-notice.yml", import.meta.url), + "utf8", + ); + + // Strip comment lines before matching. The header deliberately QUOTES the + // anti-pattern it forbids, so a `doesNotMatch` over the raw file fails on its + // own documentation — the same trap recorded in + // scripts/__tests__/merge-group-concurrency.test.mjs. + const directives = workflow + .split("\n") + .filter((line) => !/^\s*#/.test(line)) + .join("\n"); + + assert.match(directives, /pull-requests: write/); + assert.doesNotMatch( + directives, + /ref:\s*\$\{\{\s*github\.event\.workflow_run\.head_sha/, + "checking out the triggering run's head would run candidate code with a write token", + ); + // Not a merge_group check: it must not be able to eject an entry. + assert.doesNotMatch(directives, /^\s*merge_group:/m); +}); diff --git a/.github/workflows/merge-queue-ejection-notice.yml b/.github/workflows/merge-queue-ejection-notice.yml new file mode 100644 index 000000000000..45c969f297dc --- /dev/null +++ b/.github/workflows/merge-queue-ejection-notice.yml @@ -0,0 +1,124 @@ +name: Merge-queue ejection notice + +# BLO-28886 ask 2: surface a queue-branch failure on the pull request it ejected. +# +# A merge-queue entry re-runs the full suite on a temporary +# `gh-readonly-queue/master/pr--` branch. Those runs are reported +# against the QUEUE BRANCH, so they never appear in 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, green, +# with nobody re-enqueuing it. See BLO-28886 for the measurement. +# +# WHY A SEPARATE `workflow_run` WORKFLOW RATHER THAN A STEP IN pr.yml — two +# independent reasons, either one sufficient: +# +# 1. TRUST BOUNDARY. `pr.yml`'s `verify` job checks out the QUEUE CANDIDATE, +# i.e. the diff under review. Giving that job `pull-requests: write` would +# hand every queued PR a write token it could drive by editing the script +# or the workflow in its own diff. `comment-review-gate-merge-group.yml` +# already refuses that same trade for `statuses: write`. A `workflow_run` +# listener runs the DEFAULT BRANCH's copy of this file regardless of what +# the candidate contains, so the write scope is never exposed to it. +# 2. EJECTION SURFACE. Every job added to a merge_group run is another way for +# an entry to die under ALLGREEN. This workflow is not a merge_group check +# and cannot gate, delay, or eject anything. +# +# ⚠️ UNVERIFIED DELIVERY ASSUMPTION — do not assume a silent notifier is a quiet +# one. GitHub does not document whether `workflow_run` is delivered when the +# triggering run was itself started by `merge_group`, and the failure mode is +# silent: no run, no comment, no error, indistinguishable from "no ejections +# happened". That is the exact invisibility this workflow exists to remove, so +# it must be checked rather than assumed. +# +# It is checked two ways. `workflow_dispatch` below replays this logic against +# any past run id without waiting for a real ejection, which proves the parsing, +# the job query and the comment POST end to end. It does NOT prove automatic +# delivery — for that, after this merges, take the next `PR` run with +# `event=merge_group` and `conclusion=failure` and confirm a run of this +# workflow exists for it: +# +# gh run list -R Blockcast/paperclip --workflow="Merge-queue ejection notice" +# +# If automatic delivery turns out not to happen, the fallback is a scheduled +# sweep over `removed_from_merge_queue` timeline events (the detector recorded +# on BLO-28886), not a merge_group job. + +on: + workflow_run: + workflows: ["PR"] + types: [completed] + # Replays the notice for one past run, so delivery and formatting can be + # verified without waiting for a real ejection. + workflow_dispatch: + inputs: + run_id: + description: "PR-workflow run id to replay the ejection notice for" + required: true + type: string + +permissions: + contents: read + +jobs: + notice: + # Only a FAILED run on a queue branch ejected anything. `cancelled` is queue + # supersession or a re-stage and carries no verdict (BLO-23194), so it is + # deliberately not reported — announcing it would train readers to ignore + # this comment. The dispatch path skips the filter so a replay can target + # any run. + if: >- + ${{ github.event_name == 'workflow_dispatch' || + (github.event.workflow_run.conclusion == 'failure' && + startsWith(github.event.workflow_run.head_branch, 'gh-readonly-queue/')) }} + # NOT arc-merge-queue: that pool's admission hook refuses any workflow other + # than pr.yml before checkout (see comment-review-gate-merge-group.yml). + # arc-light is the precedented pool for this repo's non-pr.yml CI jobs. + runs-on: arc-light + timeout-minutes: 10 + permissions: + contents: read + # The run's per-job conclusions, to name the failing shard. + actions: read + # The comment itself. Safe here and only here: this job never runs + # candidate code (see the header). + pull-requests: write + steps: + # No `ref:` — a `workflow_run` job checks out the default branch by + # default, which is the property the trust-boundary argument rests on. + # Do not add `ref: ${{ github.event.workflow_run.head_sha }}`: that would + # run the queue candidate's code with the write token above. + - name: Checkout default branch (never queue-candidate code) + uses: actions/checkout@v6 + + - name: Resolve the run being reported + id: run + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then + run_json="$(gh api "repos/${{ github.repository }}/actions/runs/${{ inputs.run_id }}")" + { + echo "id=$(jq -r .id <<<"$run_json")" + echo "head_branch=$(jq -r .head_branch <<<"$run_json")" + echo "head_sha=$(jq -r .head_sha <<<"$run_json")" + echo "url=$(jq -r .html_url <<<"$run_json")" + } >>"$GITHUB_OUTPUT" + else + { + echo "id=${{ github.event.workflow_run.id }}" + echo "head_branch=${{ github.event.workflow_run.head_branch }}" + echo "head_sha=${{ github.event.workflow_run.head_sha }}" + echo "url=${{ github.event.workflow_run.html_url }}" + } >>"$GITHUB_OUTPUT" + fi + + - name: Post the ejection notice on the pull request + env: + GH_TOKEN: ${{ github.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + RUN_ID: ${{ steps.run.outputs.id }} + RUN_HEAD_BRANCH: ${{ steps.run.outputs.head_branch }} + RUN_HEAD_SHA: ${{ steps.run.outputs.head_sha }} + RUN_URL: ${{ steps.run.outputs.url }} + run: node ./.github/scripts/post-merge-queue-ejection-notice.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 69e97e0e4626..c3fcb04ab86b 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -402,6 +402,20 @@ jobs: run: node --test ./.github/scripts/tests/check-ffmpeg-muxer.test.mjs timeout-minutes: 1 + # BLO-28886 ask 2. The ejection notifier only ever runs from a + # `workflow_run` listener, after a merge-queue entry has already failed — + # so every property below is unobservable until an ejection has already + # been missed, which is the exact failure it exists to remove. Two of them + # cannot be re-checked after the fact at all: that a non-queue branch + # shaped like a queue ref is rejected (otherwise the notice lands on an + # unrelated PR), and that the workflow never checks out the queue + # candidate (it holds `pull-requests: write`, so a `ref:` pointing at the + # triggering run would hand that token to any queued diff). + - name: Test merge-queue ejection notice (BLO-28886) + if: ${{ !cancelled() }} + run: node --test ./.github/scripts/tests/post-merge-queue-ejection-notice.test.mjs + timeout-minutes: 1 + - name: Test production environment protection guard (BLO-22329) if: ${{ !cancelled() }} run: |