From d5d667bd047855536085117e92655980031020c0 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 15:57:37 +0800 Subject: [PATCH] audit-merges: a watermark, and a read-only patrol that runs it The audit was correct and unusable: it failed on every bypass in the history that predates merge-gate becoming a required check, so it was red on its first run and would stay red until that history slid out of the window, and nothing ran it at all (#697). --since takes a sha (resolved to its committer date) or an ISO date and starts the walk there: a finding at or before the watermark prints as "earlier bypass" / "earlier direct commit" and does not affect the exit code, so exit 1 now means a bypass newer than the watermark. With no --since the behaviour is unchanged, which is what a hand run wants. A boundary commit counts as earlier, and an unreadable --since is an error rather than a silent unfiltered walk. The new read-only patrol (contents/actions/pull-requests: read) runs the script daily and takes its watermark from the head_sha of its own previous successful run, so no file has to be committed and the job cannot write anything. A first run has no previous run and baselines the history it finds, which makes the first green the watermark. The header comment is corrected too: it said a pass named the merge commit "was pushed to main", which is more than the data shows, and it offered an `out` parameter that does not exist. Fixes #697 --- .github/scripts/audit-merges.mjs | 106 ++++++++++++++++---- .github/scripts/audit-merges.test.mjs | 139 ++++++++++++++++++++++++++ .github/workflows/audit-merges.yml | 63 ++++++++++++ docs/autonomous-dev.md | 7 ++ 4 files changed, 295 insertions(+), 20 deletions(-) create mode 100644 .github/workflows/audit-merges.yml diff --git a/.github/scripts/audit-merges.mjs b/.github/scripts/audit-merges.mjs index b1387e040..f9f302d10 100644 --- a/.github/scripts/audit-merges.mjs +++ b/.github/scripts/audit-merges.mjs @@ -1,12 +1,12 @@ -// Audit that every merge on main went through the merge queue (#689). +// Audit that every merge on main went through the merge queue (#689, #697). // -// GH_REPO=owner/repo GH_TOKEN=... node audit-merges.mjs [--limit 50] [--repo owner/repo] +// GH_REPO=owner/repo GH_TOKEN=... node audit-merges.mjs [--limit 50] [--since | --baseline] [--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. +// The merge gate judges a queue entry, and only the queue should turn that entry into a commit on +// `main`. That second half is still a convention, and it has been broken: `#650` (`6438cb2b`) and +// `#680` (`6b9820bb`) are commits that a pull request names as its merge commit, while the +// `Merge gate` run of the queue entry it built failed. A real queue merge — `#672` (`93e52347`), +// `#674` (`9393d558`) — leaves a **successful** `Merge gate` run whose `head_sha` is that commit. // // 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: @@ -21,6 +21,18 @@ // 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. // +// **The watermark (#697).** This history contains bypasses from before `merge-gate` became a +// required check, so a check that reports all of them is red on its first run and stays red until +// they slide out of the window — a check nobody reads. `--since ` moves the start of +// the walk: a sha is resolved to its committer date, the commits list is fetched from that date, and +// a finding at or before the watermark prints as `earlier bypass: …` / `earlier direct commit: …` +// and does not affect the exit code. Only a finding newer than the watermark fails the run, so red +// means "new". With no `--since` everything is a finding, which is what a hand run wants. +// +// `--baseline` is the first run of a patrol that has no previous run: it walks the window unfiltered +// and marks every finding `earlier`, so the history it finds becomes the watermark instead of a red +// first run. `--since` and `--baseline` are exclusive — one starts at a point, the other sets one. +// // 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' @@ -59,29 +71,56 @@ export function pullOf(sha, pulls) { } // 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. +// interface: the patrol workflow 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})` } + return { sha, ok: false, pr: pull.number, state, line: `bypassed queue: #${pull.number} ${short(sha)} (merge_group: ${state})` } +} + +// The committer date of a commit in the shape `GET /commits` returns it, in milliseconds, or null +// when the object carries no readable date (a test may pass a bare `{ sha }`). +export const commitMs = (commit) => { + const raw = commit?.commit?.committer?.date ?? commit?.commit?.author?.date + if (raw === undefined || raw === null) return null + const ms = Date.parse(raw) + return Number.isNaN(ms) ? null : ms } +// A finding at or before the watermark. It stays in the report — the history is the record — but it +// never fails the run, so `exit 1` always means "something merged after the watermark bypassed". +const earlierLine = (finding) => + finding.pr + ? `earlier bypass: #${finding.pr} ${short(finding.sha)} (merge_group: ${finding.state})` + : `earlier direct commit: ${finding.sha}` + // 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 }) { +// commit; keeping them injected is what lets a test drive the whole audit from fixtures. `since` is +// an ISO timestamp (or milliseconds): findings at or before it are marked `earlier` and never fail. +export function audit({ commits, pullsFor, runsFor, since, baseline = false }) { + const sinceMs = + since === undefined || since === null || since === '' ? null : typeof since === 'number' ? since : Date.parse(since) + if (sinceMs !== null && Number.isNaN(sinceMs)) throw new Error(`--since is not an ISO date: ${since}`) 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) + if (finding.ok) continue + const ms = typeof commit === 'string' ? null : commitMs(commit) + if (baseline || (sinceMs !== null && ms !== null && ms <= sinceMs)) { + finding.earlier = true + finding.line = earlierLine(finding) + } + findings.push(finding) } return findings } -export const exitCodeFor = (findings) => (findings.length ? 1 : 0) +// Only a finding newer than the watermark fails: an all-earlier report is the baseline, not a defect. +export const exitCodeFor = (findings) => ((findings ?? []).some((f) => !f.earlier) ? 1 : 0) const realGh = (...args) => execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 256 << 20 }) @@ -90,9 +129,26 @@ function arg(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 } = {}) { +// `--since` takes either a sha (resolved to that commit's committer date) or an ISO date. Resolving +// it here keeps the walk one request with `since=`, and lets the patrol pass the `head_sha` of its +// own previous successful run without a file having to be committed. +export function resolveSince(fetchJson, base, raw) { + if (raw === undefined || raw === null || raw === '') return null + const value = String(raw).trim() + if (/^[0-9a-f]{4,40}$/i.test(value)) { + const commit = fetchJson(`${base}/commits/${value}`) + const iso = commit?.commit?.committer?.date ?? commit?.commit?.author?.date + if (!iso) throw new Error(`--since sha ${value} has no committer date`) + return new Date(iso).toISOString() + } + const ms = Date.parse(value) + if (Number.isNaN(ms)) throw new Error(`--since is not a sha or an ISO date: ${value}`) + return new Date(ms).toISOString() +} + +// The entry point a workflow or a hand run calls. `api`, `gh`, `env` and the explicit options 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, since: sinceOption, baseline: baselineOption } = {}) { 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') @@ -100,18 +156,28 @@ export function main({ api, gh = realGh, env = process.env, repo: repoOption, li 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}`) + const baseline = baselineOption ?? (arg('--baseline') !== undefined || env.BASELINE === '1') + if (baseline && (sinceOption ?? arg('--since') ?? env.SINCE)) throw new Error('--baseline and --since are exclusive') + const since = baseline ? null : resolveSince(fetchJson, base, sinceOption ?? arg('--since') ?? env.SINCE) + const commits = fetchJson(`${base}/commits?sha=main&per_page=${limit}${since ? `&since=${encodeURIComponent(since)}` : ''}`) if (!Array.isArray(commits)) throw new Error('the commits API did not return a list') const findings = audit({ commits, + since, + baseline: baseline === true, 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 } + const fresh = (f) => !f.earlier + const bypassed = findings.filter((f) => f.pr && fresh(f)).length + const direct = findings.filter((f) => !f.pr && fresh(f)).length + const earlier = findings.filter((f) => f.earlier).length + console.error( + `audit-merges: ${commits.length} commits on main, ${bypassed} bypassed the queue, ${direct} direct, ${earlier} earlier than the watermark${baseline ? ' (baseline)' : since ? ` (${since})` : ''}`, + ) + return { findings, bypassed, direct, earlier, since, baseline: baseline === true, commits: commits.length, ok: exitCodeFor(findings) === 0 } } if (process.argv[1] === fileURLToPath(import.meta.url)) { diff --git a/.github/scripts/audit-merges.test.mjs b/.github/scripts/audit-merges.test.mjs index 90bfd2853..c5bfb8a9d 100644 --- a/.github/scripts/audit-merges.test.mjs +++ b/.github/scripts/audit-merges.test.mjs @@ -128,3 +128,142 @@ test('audit() keeps commit order and skips a commit with no sha', () => { }) assert.deepEqual(findings.map((f) => f.pr), [680]) }) + +// #697: the watermark. This history holds bypasses from before `merge-gate` became a required check, +// so an unfiltered walk is red forever and a check that is always red is a check nobody reads. +const stamp = (fixture, date) => ({ sha: fixture.sha, commit: { committer: { date } } }) + +// A minimal API for the watermark cases: every commit carries its date, the list path may or may +// not carry `since=`, and one commit can be asked for by sha to resolve a watermark. +const watermarkApi = (entries) => { + const bySha = new Map(entries.map((entry) => [entry.fixture.sha, entry])) + const paths = [] + const api = (path) => { + paths.push(path) + if (/^repos\/o\/r\/commits\?sha=main&per_page=50(&since=[^&]+)?$/.test(path)) return entries.map((entry) => stamp(entry.fixture, entry.date)) + const commit = /^repos\/o\/r\/commits\/([0-9a-f]+)$/.exec(path) + if (commit) return { commit: { committer: { date: bySha.get(commit[1])?.date } } } + const pulls = /^repos\/o\/r\/commits\/([0-9a-f]+)\/pulls\?per_page=100$/.exec(path) + if (pulls) return bySha.get(pulls[1]).fixture.pulls + const runs = /^repos\/o\/r\/actions\/runs\?event=merge_group&head_sha=([0-9a-f]+)&per_page=100$/.exec(path) + if (runs) return { workflow_runs: bySha.get(runs[1]).fixture.runs } + throw new Error(`unexpected path ${path}`) + } + return { api, paths } +} + +const runMain = (args) => { + const logs = [] + const errors = [] + const [log, error] = [console.log, console.error] + console.log = (line) => logs.push(line) + console.error = (line) => errors.push(line) + try { + return { result: main(args), logs, errors } + } finally { + console.log = log + console.error = error + } +} + +test('a finding at or before the watermark is printed as earlier and leaves the exit code at 0', () => { + const bypassed = fixture('bypassed') + const direct = fixture('direct') + const since = '2026-10-21T12:00:00Z' + const { api } = watermarkApi([ + { fixture: bypassed, date: '2026-10-21T00:00:00Z' }, + { fixture: direct, date: since }, // the boundary itself counts as earlier + ]) + const { result, logs, errors } = runMain({ api, repo: 'o/r', since }) + assert.deepEqual(logs, [ + `earlier bypass: #680 ${bypassed.sha.slice(0, 12)} (merge_group: failure)`, + `earlier direct commit: ${direct.sha}`, + ]) + assert.equal(exitCodeFor(result.findings), 0) + assert.equal(result.earlier, 2) + assert.equal(result.bypassed, 0) + assert.equal(result.direct, 0) + assert.match(errors.join('\n'), /2 commits on main, 0 bypassed the queue, 0 direct, 2 earlier than the watermark/) +}) + +test('only a finding newer than the watermark fails, and it alone counts as bypassed', () => { + const bypassed = fixture('bypassed') + const absent = fixture('absent') + const since = '2026-10-21T12:00:00Z' + const { api } = watermarkApi([ + { fixture: bypassed, date: '2026-10-21T00:00:00Z' }, + { fixture: absent, date: '2026-10-22T06:00:00Z' }, + ]) + const { result, logs } = runMain({ api, repo: 'o/r', since }) + assert.deepEqual(logs, [ + `earlier bypass: #680 ${bypassed.sha.slice(0, 12)} (merge_group: failure)`, + `bypassed queue: #622 ${absent.sha.slice(0, 12)} (merge_group: absent)`, + ]) + assert.equal(exitCodeFor(result.findings), 1) + assert.equal(result.bypassed, 1) + assert.equal(result.earlier, 1) +}) + +test('a sha watermark is resolved to its committer date and the walk starts there', () => { + const queued = fixture('queued') + const bypassed = fixture('bypassed') + const date = '2026-10-21T12:00:00Z' + const { api, paths } = watermarkApi([ + { fixture: queued, date }, + { fixture: bypassed, date: '2026-10-22T06:00:00Z' }, + ]) + const { result } = runMain({ api, repo: 'o/r', since: queued.sha }) + assert.equal(result.since, '2026-10-21T12:00:00.000Z') + assert.ok(paths.includes(`repos/o/r/commits/${queued.sha}`), 'the sha is resolved through the commits API') + assert.ok( + paths.includes('repos/o/r/commits?sha=main&per_page=50&since=2026-10-21T12%3A00%3A00.000Z'), + 'the walk asks for commits from the watermark on', + ) +}) + +test('an unreadable watermark is an error, not a silent unfiltered walk', () => { + const queued = fixture('queued') + const { api } = watermarkApi([{ fixture: queued, date: '2026-10-21T12:00:00Z' }]) + assert.throws(() => main({ api, repo: 'o/r', since: 'not-a-date' }), /--since is not a sha or an ISO date/) + assert.throws(() => audit({ commits: [], since: 'nope' }), /--since is not an ISO date/) +}) + +test('the patrol workflow is read-only and takes its watermark from its own previous run', () => { + const text = readFileSync(new URL('../workflows/audit-merges.yml', import.meta.url), 'utf8') + const permissions = text.slice(text.indexOf('\npermissions:'), text.indexOf('\nconcurrency:')) + assert.match(permissions, /^ {2}contents: read$/m) + assert.match(permissions, /^ {2}actions: read$/m) + assert.match(permissions, /^ {2}pull-requests: read$/m) + assert.doesNotMatch(permissions, /write/, 'the patrol must never be able to write') + assert.match(text, /^ {2}schedule:$/m, 'the patrol runs on a schedule') + assert.match(text, /^ {2}workflow_dispatch:$/m, 'and by hand') + assert.match(text, /status=success&per_page=1/) + assert.match(text, /workflow_runs\[0\]\.head_sha/) + assert.match(text, /--since "\$\{\{ steps\.watermark\.outputs\.since \}\}"/) + assert.match(text, /--baseline/, "a first run baselines instead of failing on the history") + assert.doesNotMatch(text, /pull_request_target|gh pr merge|git push|contents: write/) +}) + +test('a baseline walk reports the whole window as earlier and exits 0', () => { + const bypassed = fixture('bypassed') + const absent = fixture('absent') + const { api, paths } = watermarkApi([ + { fixture: bypassed, date: '2026-10-21T00:00:00Z' }, + { fixture: absent, date: '2026-10-22T06:00:00Z' }, + ]) + const { result, logs, errors } = runMain({ api, repo: 'o/r', baseline: true }) + assert.deepEqual(logs, [ + `earlier bypass: #680 ${bypassed.sha.slice(0, 12)} (merge_group: failure)`, + `earlier bypass: #622 ${absent.sha.slice(0, 12)} (merge_group: absent)`, + ]) + assert.equal(exitCodeFor(result.findings), 0) + assert.equal(result.earlier, 2) + assert.ok(!paths.some((p) => p.includes('since=')), 'a baseline walk does not ask for commits from a date') + assert.match(errors.join('\n'), /2 earlier than the watermark \(baseline\)/) +}) + +test('--baseline and --since together is an error, not one silently winning', () => { + const queued = fixture('queued') + const { api } = watermarkApi([{ fixture: queued, date: '2026-10-21T12:00:00Z' }]) + assert.throws(() => main({ api, repo: 'o/r', baseline: true, since: '2026-10-21T12:00:00Z' }), /--baseline and --since are exclusive/) +}) diff --git a/.github/workflows/audit-merges.yml b/.github/workflows/audit-merges.yml new file mode 100644 index 000000000..76052ce0f --- /dev/null +++ b/.github/workflows/audit-merges.yml @@ -0,0 +1,63 @@ +# The read-only patrol for every merge on main (#689, #697). `merge-gate` refuses a queue entry it +# does not approve, and the merge queue is the only route into `main` — this job checks the second +# half: `audit-merges.mjs` walks the recent commits and reports the ones that landed without a +# successful `Merge gate` run on the commit that landed. +# +# Red means "new". History holds bypasses from before `merge-gate` became a required check, so the +# walk starts at the `head_sha` of this workflow's own previous successful run: everything at or +# before it prints as `earlier bypass` and never fails. A first run has no previous run, so it +# baselines on the current time — the history at that moment is the baseline, and the first green is +# the watermark. Nothing is committed for this; the state is the run history. +# +# Read-only by construction: `contents: read`, `actions: read`, `pull-requests: read`, and the script +# only reads the API. It never comments, labels, enqueues or merges. + +name: Audit merges + +on: + schedule: + - cron: "41 1 * * *" # daily 09:41 Asia/Shanghai + workflow_dispatch: + +permissions: + contents: read + actions: read + pull-requests: read + +concurrency: + group: audit-merges + cancel-in-progress: false + +jobs: + audit: + name: audit-merges + runs-on: ubuntu-latest + timeout-minutes: 10 + env: + GH_REPO: ${{ github.repository }} + GH_TOKEN: ${{ github.token }} + steps: + - name: Check out main + uses: actions/checkout@v7 + with: + ref: main + fetch-depth: 1 + persist-credentials: false + + - name: Watermark: the head of the previous successful run + id: watermark + run: | + previous="$(gh api "repos/$GITHUB_REPOSITORY/actions/workflows/audit-merges.yml/runs?status=success&per_page=1" --jq '.workflow_runs[0].head_sha // empty')" + if [ -z "$previous" ]; then + echo "no previous successful run: baselining the history this run finds" + else + echo "since=$previous" >> "$GITHUB_OUTPUT" + fi + + - name: Audit merges against the watermark + run: | + if [ -n "${{ steps.watermark.outputs.since }}" ]; then + node .github/scripts/audit-merges.mjs --limit 50 --since "${{ steps.watermark.outputs.since }}" + else + node .github/scripts/audit-merges.mjs --limit 50 --baseline + fi diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 1a3857538..365d39a05 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -190,6 +190,13 @@ agent reads, and the record a later enforcement step is checked against. **The gates themselves** — `hold`, the newest-conclusion rule, and the `merge-gate` job that enforces both on a queue entry — are in [`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates). This section is only the plumbing. +**The patrol.** [`audit-merges.yml`](../.github/workflows/audit-merges.yml) runs +[`audit-merges.mjs`](../.github/scripts/audit-merges.mjs) daily and checks the other half of the gate: that every commit +on `main` came through the queue with a successful `Merge gate` run on the commit that landed. It is read-only +(`contents: read`, `actions: read`, `pull-requests: read`) and takes its watermark from the `head_sha` of its own +previous successful run, so a red run means a bypass **newer than that watermark** — findings at or before it print as +`earlier bypass` and never fail it (#697). A first run has no previous run and baselines the history it finds. + **How to merge.** A direct `gh pr merge --squash` is refused with `! The merge strategy for main is set by the merge queue`; that is not a failure. Enqueue instead: `gh pr merge --auto --squash [--match-head-commit ]` (gh has no `--merge-queue` flag). gh may still print the strategy line, or `! Pull request # is already queued to merge`; the PR is