From 237253151413490fb0f27cc9a6cbed3b5a3cf0b7 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 14:57:43 +0800 Subject: [PATCH] Audit that every merge on main went through the merge queue (#689) #650 (6438cb2b) and #680 (6b9820bb) landed as build commits whose Merge gate run had failed, so "only the queue merges" has been a convention that nothing checks. audit-merges.mjs walks the last commits on main and prints `bypassed queue: # (merge_group: failure|absent)` when the landed commit carries no successful gate run of its own, `direct commit: ` for a push that came from no pull request, and exits 1 when it found one. Read-only. The gate run is matched by workflow, not by "some merge_group run succeeded": on #680 the Test run of the very entry the gate refused is green, so the looser test would call that bypass a queue merge. Fixes #689 Docs-not-needed: adds a read-only .github/scripts check and its cases; no subsystem doc owns .github/. --- .github/scripts/audit-merges.mjs | 125 +++++++++++++++++ .github/scripts/audit-merges.test.mjs | 130 ++++++++++++++++++ .../scripts/fixtures/audit-merges/absent.json | 8 ++ .../fixtures/audit-merges/bypassed.json | 9 ++ .../scripts/fixtures/audit-merges/direct.json | 8 ++ .../scripts/fixtures/audit-merges/queued.json | 9 ++ 6 files changed, 289 insertions(+) create mode 100644 .github/scripts/audit-merges.mjs create mode 100644 .github/scripts/audit-merges.test.mjs create mode 100644 .github/scripts/fixtures/audit-merges/absent.json create mode 100644 .github/scripts/fixtures/audit-merges/bypassed.json create mode 100644 .github/scripts/fixtures/audit-merges/direct.json create mode 100644 .github/scripts/fixtures/audit-merges/queued.json diff --git a/.github/scripts/audit-merges.mjs b/.github/scripts/audit-merges.mjs new file mode 100644 index 00000000..b1387e04 --- /dev/null +++ b/.github/scripts/audit-merges.mjs @@ -0,0 +1,125 @@ +// Audit that every merge on main went through the merge queue (#689). +// +// GH_REPO=owner/repo GH_TOKEN=... node audit-merges.mjs [--limit 50] [--repo owner/repo] +// +// The merge gate judges a queue entry, and only the queue can turn that entry into a commit on +// `main`. That second half was a convention, and it has been broken: `#650` (`6438cb2b`) and `#680` +// (`6b9820bb`) are build commits whose `Merge gate` run failed, and `main` was nevertheless pushed +// to them (both carry `merged_by: Birfy`). A real queue merge — `#672` (`93e52347`), `#674` +// (`9393d558`) — leaves a **successful** `Merge gate` run whose `head_sha` is the commit that landed. +// +// So the difference is machine-checkable. This script walks the last `--limit` commits on `main`, +// asks which pull request each came from, and asks for the `merge_group` runs on that commit: +// - a commit with a successful `Merge gate` run on it went through the queue: no output; +// - a commit whose only `Merge gate` run on it failed (or which has none) prints +// `bypassed queue: # (merge_group: failure|absent)` and the run exits 1; +// - a commit that came from no pull request prints `direct commit: ` and also exits 1: +// a direct push to `main` did not go through the queue either. It is a separate defect, so it +// gets a separate line rather than being counted as a bypass. +// +// The gate run is identified by its workflow, not by "some `merge_group` run succeeded": one queue +// entry runs several workflows (`Test`, `Merge gate`, …), and on `#680` the `Test` run of the very +// entry the gate refused is green. Matching a green `Test` run would call that bypass a queue merge. +// +// Read-only: it reads commits, pull requests and workflow runs, and writes nothing — no comment, no +// label, no merge, no ref push. Token scope: `contents:read`, `pulls:read`, `actions:read`. +import { execFileSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' + +const SHORT = 12 +// The gate workflow that a queue entry has to pass. `path` is what the runs API carries for the +// workflow; `name` is the fallback for fixtures and for API versions that omit it. +export const GATE_PATH = '.github/workflows/merge-gate.yml' +export const GATE_NAME = 'Merge gate' + +export const short = (sha) => String(sha ?? '').slice(0, SHORT) +export const isGateRun = (run) => run?.path === GATE_PATH || run?.name === GATE_NAME + +// The successful gate run for this exact commit, or null. `head_sha` is the entry's built commit — +// the commit the queue would fast-forward `main` to — so an entry built on top of an older `main` +// says nothing about this commit. +export function queueRun(sha, runs) { + const target = String(sha ?? '').toLowerCase() + return (runs ?? []).find((run) => String(run?.head_sha ?? '').toLowerCase() === target && isGateRun(run) && run?.conclusion === 'success') ?? null +} + +// Whether the gate ran on this commit at all: it separates "the gate said no" from "nobody built a +// queue entry this commit could have come from". +export const gateRan = (sha, runs) => { + const target = String(sha ?? '').toLowerCase() + return (runs ?? []).some((run) => String(run?.head_sha ?? '').toLowerCase() === target && isGateRun(run)) +} + +// The pull request a commit came from. A merge commit names its own pull request; a squash merge +// carries one too. Prefer the pull request whose `merge_commit_sha` is this commit, because a +// cherry-pick can list more than one. +export function pullOf(sha, pulls) { + const list = (pulls ?? []).filter((p) => p?.number) + return list.find((p) => String(p.merge_commit_sha ?? '').toLowerCase() === String(sha).toLowerCase()) ?? list[0] ?? null +} + +// One finding for one commit, or null when the commit went through the queue. The line is the whole +// interface: the Maintainer patrol and a hand run both read it, so its shape is fixed. +export function auditCommit({ sha, pulls = [], runs = [] }) { + const pull = pullOf(sha, pulls) + if (!pull) return { sha, ok: false, line: `direct commit: ${sha}` } + if (queueRun(sha, runs)) return { sha, ok: true, pr: pull.number } + const state = gateRan(sha, runs) ? 'failure' : 'absent' + return { sha, ok: false, pr: pull.number, line: `bypassed queue: #${pull.number} ${short(sha)} (merge_group: ${state})` } +} + +// The walk, over data already fetched. `pullsFor(sha)` and `runsFor(sha)` are the two calls per +// commit; keeping them injected is what lets a test drive the whole audit from fixtures. +export function audit({ commits, pullsFor, runsFor }) { + const findings = [] + for (const commit of commits) { + const sha = String(typeof commit === 'string' ? commit : commit?.sha ?? '') + if (!sha) continue + const finding = auditCommit({ sha, pulls: pullsFor(sha) ?? [], runs: runsFor(sha) ?? [] }) + if (!finding.ok) findings.push(finding) + } + return findings +} + +export const exitCodeFor = (findings) => (findings.length ? 1 : 0) + +const realGh = (...args) => execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 256 << 20 }) + +function arg(name) { + const i = process.argv.indexOf(name) + return i > 0 ? process.argv[i + 1] : undefined +} + +// The entry point a workflow or a hand run calls. `api`, `env` and `out` are injectable so a test +// drives the whole walk from fixtures without the network. +export function main({ api, gh = realGh, env = process.env, repo: repoOption, limit: limitOption } = {}) { + const fetchJson = api ?? ((path) => JSON.parse(gh('api', path))) + const repo = repoOption || env.GH_REPO || arg('--repo') + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + const limit = Number(limitOption ?? arg('--limit') ?? env.LIMIT ?? 50) + if (!Number.isInteger(limit) || limit < 1) throw new Error(`--limit is not a positive integer: ${limit}`) + + const base = `repos/${repo}` + const commits = fetchJson(`${base}/commits?sha=main&per_page=${limit}`) + if (!Array.isArray(commits)) throw new Error('the commits API did not return a list') + const findings = audit({ + commits, + pullsFor: (sha) => fetchJson(`${base}/commits/${sha}/pulls?per_page=100`) ?? [], + // The runs endpoint answers `{ total_count, workflow_runs }`; only the runs matter here. + runsFor: (sha) => fetchJson(`${base}/actions/runs?event=merge_group&head_sha=${sha}&per_page=100`)?.workflow_runs ?? [], + }) + for (const finding of findings) console.log(finding.line) + const bypassed = findings.filter((f) => f.pr).length + console.error(`audit-merges: ${commits.length} commits on main, ${bypassed} bypassed the queue, ${findings.length - bypassed} direct`) + return { findings, bypassed, direct: findings.length - bypassed, commits: commits.length, ok: findings.length === 0 } +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + try { + const result = main() + process.exit(exitCodeFor(result.findings)) + } catch (e) { + console.error(`audit-merges: ${e.message}`) + process.exit(1) + } +} diff --git a/.github/scripts/audit-merges.test.mjs b/.github/scripts/audit-merges.test.mjs new file mode 100644 index 00000000..90bfd285 --- /dev/null +++ b/.github/scripts/audit-merges.test.mjs @@ -0,0 +1,130 @@ +// audit-merges: a merge counts as a queue merge only when the gate itself passed on the commit that +// landed (#689). The fixtures carry one commit/PR/run set per shape; the walk is driven from them. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { audit, auditCommit, exitCodeFor, main, queueRun } from './audit-merges.mjs' + +const fixture = (name) => JSON.parse(readFileSync(new URL(`./fixtures/audit-merges/${name}.json`, import.meta.url), 'utf8')) +const one = (name) => { + const f = fixture(name) + return auditCommit({ sha: f.sha, pulls: f.pulls, runs: f.runs }) +} + +test('a green Merge gate run on the landed commit is a queue merge, and prints nothing', () => { + const finding = one('queued') + assert.equal(finding.ok, true) + assert.equal(finding.pr, 674) + assert.equal(exitCodeFor([]), 0) +}) + +// The whole check exists because of this shape: #680 landed as 6b9820bb while the gate refused that +// entry, and the entry's `Test` run was still green. Reading "some merge_group run succeeded" would +// call the bypass a queue merge, which is why the run is matched by workflow. +test('a failed Merge gate run on the landed commit is a bypass, even when Test of the same entry is green', () => { + const f = fixture('bypassed') + assert.equal(queueRun(f.sha, f.runs), null, 'the green Test run of the same entry is not the gate passing') + const finding = auditCommit({ sha: f.sha, pulls: f.pulls, runs: f.runs }) + assert.equal(finding.ok, false) + assert.equal(finding.line, `bypassed queue: #680 ${f.sha.slice(0, 12)} (merge_group: failure)`) + assert.equal(exitCodeFor([finding]), 1) +}) + +test('a merge the gate never judged says absent, not failure', () => { + const finding = one('absent') + assert.equal(finding.line, `bypassed queue: #622 ${fixture('absent').sha.slice(0, 12)} (merge_group: absent)`) +}) + +test('a push that came from no pull request is its own defect, not a bypass', () => { + const f = fixture('direct') + const finding = auditCommit({ sha: f.sha, pulls: f.pulls, runs: f.runs }) + assert.equal(finding.line, `direct commit: ${f.sha}`) + assert.doesNotMatch(finding.line, /bypassed queue/) +}) + +test('a green gate run on a different commit says nothing about this one', () => { + const f = fixture('queued') + const other = { ...f.runs[0], head_sha: 'ab'.repeat(20) } + const finding = auditCommit({ sha: f.sha, pulls: f.pulls, runs: [other] }) + assert.equal(finding.line, `bypassed queue: #674 ${f.sha.slice(0, 12)} (merge_group: absent)`) +}) + +test('a gate run that did not conclude success (cancelled) is a failure', () => { + const f = fixture('queued') + const cancelled = { ...f.runs[0], conclusion: 'cancelled' } + const finding = auditCommit({ sha: f.sha, pulls: f.pulls, runs: [cancelled] }) + assert.equal(finding.line, `bypassed queue: #674 ${f.sha.slice(0, 12)} (merge_group: failure)`) +}) + +test('the walk prints one line per merge that did not go through the queue, in commit order', () => { + const queued = fixture('queued') + const bypassed = fixture('bypassed') + const direct = fixture('direct') + const bySha = new Map([queued, bypassed, direct].map((f) => [f.sha, f])) + const paths = [] + const api = (path) => { + paths.push(path) + if (path === 'repos/o/r/commits?sha=main&per_page=50') return [queued, bypassed, direct].map((f) => ({ sha: f.sha })) + const pulls = /^repos\/o\/r\/commits\/([0-9a-f]+)\/pulls\?per_page=100$/.exec(path) + if (pulls) return bySha.get(pulls[1]).pulls + const runs = /^repos\/o\/r\/actions\/runs\?event=merge_group&head_sha=([0-9a-f]+)&per_page=100$/.exec(path) + if (runs) return { total_count: bySha.get(runs[1]).runs.length, workflow_runs: bySha.get(runs[1]).runs } + throw new Error(`unexpected path ${path}`) + } + const lines = [] + const errors = [] + const [log, error] = [console.log, console.error] + console.log = (line) => lines.push(line) + console.error = (line) => errors.push(line) + let result + try { + result = main({ api, repo: 'o/r' }) + } finally { + console.log = log + console.error = error + } + assert.deepEqual(lines, [ + `bypassed queue: #680 ${bypassed.sha.slice(0, 12)} (merge_group: failure)`, + `direct commit: ${direct.sha}`, + ]) + assert.equal(result.bypassed, 1) + assert.equal(result.direct, 1) + assert.equal(exitCodeFor(result.findings), 1) + assert.match(errors.join('\n'), /3 commits on main, 1 bypassed the queue, 1 direct/) + assert.ok(paths.some((p) => p.includes(`head_sha=${queued.sha}`)), 'the queued commit is audited too, it just finds nothing') +}) + +test('the walk is clean when every merge went through the queue', () => { + const queued = fixture('queued') + const api = (path) => { + if (path === 'repos/o/r/commits?sha=main&per_page=50') return [{ sha: queued.sha }] + if (path === `repos/o/r/commits/${queued.sha}/pulls?per_page=100`) return queued.pulls + if (path === `repos/o/r/actions/runs?event=merge_group&head_sha=${queued.sha}&per_page=100`) return { workflow_runs: queued.runs } + throw new Error(`unexpected path ${path}`) + } + const logs = [] + const [log, error] = [console.log, console.error] + console.log = (line) => logs.push(line) + console.error = () => {} + let result + try { + result = main({ api, repo: 'o/r' }) + } finally { + console.log = log + console.error = error + } + assert.deepEqual(logs, []) + assert.deepEqual(result.findings, []) + assert.equal(exitCodeFor(result.findings), 0) +}) + +test('audit() keeps commit order and skips a commit with no sha', () => { + const queued = fixture('queued') + const bypassed = fixture('bypassed') + const findings = audit({ + commits: [{ sha: bypassed.sha }, { sha: queued.sha }, {}], + pullsFor: (sha) => (sha === queued.sha ? queued.pulls : bypassed.pulls), + runsFor: (sha) => (sha === queued.sha ? queued.runs : bypassed.runs), + }) + assert.deepEqual(findings.map((f) => f.pr), [680]) +}) diff --git a/.github/scripts/fixtures/audit-merges/absent.json b/.github/scripts/fixtures/audit-merges/absent.json new file mode 100644 index 00000000..31c9b065 --- /dev/null +++ b/.github/scripts/fixtures/audit-merges/absent.json @@ -0,0 +1,8 @@ +{ + "note": "A shape, not a merge in this repository's history: a merge commit whose only merge_group runs belong to other workflows, so the gate never judged it.", + "sha": "1f4024be0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", + "pulls": [{ "number": 622, "merge_commit_sha": "1f4024be0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f" }], + "runs": [ + { "id": 39700000001, "name": "Test", "path": ".github/workflows/test.yml", "event": "merge_group", "head_sha": "1f4024be0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f", "conclusion": "success" } + ] +} diff --git a/.github/scripts/fixtures/audit-merges/bypassed.json b/.github/scripts/fixtures/audit-merges/bypassed.json new file mode 100644 index 00000000..b29e1cdf --- /dev/null +++ b/.github/scripts/fixtures/audit-merges/bypassed.json @@ -0,0 +1,9 @@ +{ + "note": "The bypass: #680 landed as 6b9820bb, its Merge gate run on that commit failed (39730969259) and the Test run of the same entry is green (39730969253) — a green Test run must not be read as the gate passing.", + "sha": "6b9820bbd75304a6eb41e90e045fc58596dd1a41", + "pulls": [{ "number": 680, "merge_commit_sha": "6b9820bbd75304a6eb41e90e045fc58596dd1a41" }], + "runs": [ + { "id": 39730969259, "name": "Merge gate", "path": ".github/workflows/merge-gate.yml", "event": "merge_group", "head_sha": "6b9820bbd75304a6eb41e90e045fc58596dd1a41", "conclusion": "failure" }, + { "id": 39730969253, "name": "Test", "path": ".github/workflows/test.yml", "event": "merge_group", "head_sha": "6b9820bbd75304a6eb41e90e045fc58596dd1a41", "conclusion": "success" } + ] +} diff --git a/.github/scripts/fixtures/audit-merges/direct.json b/.github/scripts/fixtures/audit-merges/direct.json new file mode 100644 index 00000000..2076ba6b --- /dev/null +++ b/.github/scripts/fixtures/audit-merges/direct.json @@ -0,0 +1,8 @@ +{ + "note": "A push to main that came from no pull request: reported as its own defect, not as a queue bypass.", + "sha": "0d1e2f3a4b5c6d7e8f9012345678abcdef012345", + "pulls": [], + "runs": [ + { "id": 39700000002, "name": "Test", "path": ".github/workflows/test.yml", "event": "push", "head_sha": "0d1e2f3a4b5c6d7e8f9012345678abcdef012345", "conclusion": "success" } + ] +} diff --git a/.github/scripts/fixtures/audit-merges/queued.json b/.github/scripts/fixtures/audit-merges/queued.json new file mode 100644 index 00000000..0fb059ee --- /dev/null +++ b/.github/scripts/fixtures/audit-merges/queued.json @@ -0,0 +1,9 @@ +{ + "note": "A real queue merge: #674 landed as 9393d558 and its Merge gate run on that commit is green (run 39722858008).", + "sha": "9393d5586c2a06a806c04fb99cc09c4d71ebf161", + "pulls": [{ "number": 674, "merge_commit_sha": "9393d5586c2a06a806c04fb99cc09c4d71ebf161" }], + "runs": [ + { "id": 39722858008, "name": "Merge gate", "path": ".github/workflows/merge-gate.yml", "event": "merge_group", "head_sha": "9393d5586c2a06a806c04fb99cc09c4d71ebf161", "conclusion": "success" }, + { "id": 39722858269, "name": "Test", "path": ".github/workflows/test.yml", "event": "merge_group", "head_sha": "9393d5586c2a06a806c04fb99cc09c4d71ebf161", "conclusion": "success" } + ] +}