From 014b08ef23d71282b6fd150f21aa9c21c52f1670 Mon Sep 17 00:00:00 2001 From: FlyBot Dev-4 Date: Mon, 2 Nov 2026 07:02:12 +0000 Subject: [PATCH] fix(ci): judge the gate signature with the base copy of the checker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test.yml` checked the repository out with no `ref`, so on `pull_request` it carried the merge commit — the pull request's own tree — and the `Gate change signature` step ran the copy of `policy-sign.mjs` that the pull request may have edited. Weakening or deleting `judge()`, or dropping the step, therefore left every check green while the gate the signature protects had been removed. The step now runs `base/.github/scripts/policy-sign.mjs`, from a checkout of `github.event.merge_group.base_sha || github.event.pull_request.base.sha` into `path: base`, the shape `merge-gate.yml` already uses. Both halves of that expression are required: a queue run carries no `pull_request`, and a fallback to the triggering sha would run the pull request's copy again on the one run that decides the merge. The base copy loads `policy/gates.json` relative to itself, so it reads the base policy; the pull request's files, body and comments still come from the API, so no other input moves. `test-workflow.test.mjs` narrows its "nothing keys on the pull_request event" assertion from the whole file to the positions that can skip a job (a job-level `if:`, and any condition on the event), and adds the reverse assertion that the event is read on exactly one line: the `ref:` of the base checkout. Two new tests pin the step to the base copy, one of them by running the step's own `run:` command over a fixture whose head copy says ok and whose base copy refuses. Written-by: Dev-4 --- .github/scripts/test-workflow.test.mjs | 52 +++++++++++++++++++++- .github/workflows/test.yml | 12 ++++- changelog.d/2099-base-policy-sign.fixed.md | 1 + docs/governance.md | 11 +++-- 4 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 changelog.d/2099-base-policy-sign.fixed.md diff --git a/.github/scripts/test-workflow.test.mjs b/.github/scripts/test-workflow.test.mjs index 76f6de218..b4c62cd79 100644 --- a/.github/scripts/test-workflow.test.mjs +++ b/.github/scripts/test-workflow.test.mjs @@ -1,10 +1,20 @@ // test.yml: the shape that a merge queue and the required `tests-ok` check rely on (text checks; no YAML parser needed). import { test } from 'node:test' import assert from 'node:assert/strict' -import { readFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { spawnSync } from 'node:child_process' const text = readFileSync(new URL('../workflows/test.yml', import.meta.url), 'utf8') const triggers = text.slice(text.indexOf('\non:\n'), text.indexOf('\nconcurrency:')) +// One step of the `test` job, by the name it reports (#2054, #2099). +const step = name => { + const at = text.indexOf(` - name: ${name}\n`) + assert.ok(at >= 0, `no step ${name}`) + const next = text.slice(at + 1).search(/\n - /) + return text.slice(at, next < 0 ? undefined : at + 1 + next) +} const job = name => { const start = text.indexOf(`\n ${name}:\n`) assert.ok(start >= 0, `no job ${name}`) @@ -24,9 +34,47 @@ test('triggers: pull_request, merge_group (no filters), push only on main, dispa }) test('no job is skipped on merge_group: nothing keys on the pull_request event', () => { - assert.doesNotMatch(text, /github\.event\.pull_request/) + // #2099 narrowed this: the base checkout reads `github.event.pull_request.base.sha` as a `with:` + // input, which skips nothing. What the rule forbids is a position that can skip a job — a + // job-level `if:`, or any condition keyed on the event — so the reverse assertion below pins the + // event to the one line it may be read on: the `ref:` of the base checkout. assert.doesNotMatch(text, /event_name\s*[!=]=\s*'pull_request'/) + assert.doesNotMatch(text, /^\s+if:.*github\.event\.pull_request/m, 'a condition on the pull_request event would skip a queue entry') for (const name of ['test', 'plugins', 'package']) assert.doesNotMatch(job(name), /^\s+if:/m, `${name} must run for every event`) + const reads = text.split('\n').filter(line => line.includes('github.event.pull_request')) + assert.equal(reads.length, 1, 'only the base checkout reads the pull_request event') + assert.match(reads[0], /^\s+ref: \$\{\{ github\.event\.merge_group\.base_sha \|\| github\.event\.pull_request\.base\.sha \}\}$/) +}) + +// #2099: `test.yml` ran the copy of the checker that the pull request itself carries, so a pull +// request could weaken `judge()` — or delete the step — and pass the check it had just edited. The +// signature is now judged by the copy in the base commit, the shape `merge-gate.yml` already uses. +test('#2099: the gate signature runs the checker from the base commit', () => { + const signature = step('Gate change signature') + assert.match(signature, /^\s+run: node base\/\.github\/scripts\/policy-sign\.mjs$/m, "the step runs the base copy, not the pull request's own") + + const at = text.indexOf(' path: base\n') + assert.ok(at >= 0, 'test.yml checks the base commit out into `base/`') + const start = text.lastIndexOf('\n - ', at) + 1 + const base = text.slice(start, text.indexOf('\n - ', at)) + assert.match(base, /uses: actions\/checkout@3d3c42e5/, 'the same pinned checkout the rest of the file uses') + assert.match(base, /^ {8}with:\n {10}ref: \$\{\{ github\.event\.merge_group\.base_sha \|\| github\.event\.pull_request\.base\.sha \}\}$/m, 'the read is a `with:` input — an input cannot skip a job — and a queue run carries no pull_request, so merge_group.base_sha is required') + assert.match(base, /persist-credentials: false/) +}) + +test('#2099: a pull request that weakens its own copy of the checker is still refused', () => { + const command = /^\s+run: (.+)$/m.exec(step('Gate change signature'))?.[1] ?? '' + const [program, script] = command.split(/\s+/) + assert.equal(program, 'node', 'the signature step runs node') + const dir = mkdtempSync(join(tmpdir(), 'policy-sign-')) + // The pull request's own copy says "ok, carry on"; the base copy refuses. Which copy the step + // runs is the whole difference, so the fixture makes the two answer differently. + for (const [path, body] of [['.github/scripts/policy-sign.mjs', 'process.exit(0)\n'], ['base/.github/scripts/policy-sign.mjs', 'process.exit(1)\n']]) { + mkdirSync(dirname(join(dir, path)), { recursive: true }) + writeFileSync(join(dir, path), body) + } + const run = spawnSync(program, [script], { cwd: dir, encoding: 'utf8' }) + assert.notEqual(run.status, 0, `the step ran \`${script}\`: a pull request that weakened its own copy must still be refused`) }) test('tests-ok: always runs and is green only when test, plugins and package are', () => { diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ab7018ffc..73b05acb8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -44,10 +44,20 @@ jobs: # policy/gates.json (#2054): a pull request that edits an acceptance gate carries # `Policy-sign: ` in its body, written by someone other than its author. # It runs before the suite: a gate change nobody else signed is not worth 20 minutes of tests. + # #2099: the checker comes from the base commit, not from the pull request's own tree, so a + # pull request that weakens or deletes `policy-sign.mjs` is still judged by the copy it started + # from. `merge-gate.yml` checks base out the same way. The `merge_group` half + # is required, not decoration: a queue run carries no `pull_request`, and an expression with + # only that half falls back to the sha that triggered the run, which is the pull request again. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.merge_group.base_sha || github.event.pull_request.base.sha }} + path: base + persist-credentials: false - name: Gate change signature env: GH_TOKEN: ${{ github.token }} - run: node .github/scripts/policy-sign.mjs + run: node base/.github/scripts/policy-sign.mjs - uses: ./.github/actions/flybot-tests with: node: '24' diff --git a/changelog.d/2099-base-policy-sign.fixed.md b/changelog.d/2099-base-policy-sign.fixed.md new file mode 100644 index 000000000..9068b203e --- /dev/null +++ b/changelog.d/2099-base-policy-sign.fixed.md @@ -0,0 +1 @@ +The gate-change signature is now judged by the copy of the checker in the pull request's base commit instead of the copy in its own tree, so a pull request can no longer weaken or delete `policy-sign.mjs` and pass the check it just edited — the `Gate change signature` step still runs before the suite, and a signature is still a comment by someone other than the author that names the head ([#2099](https://github.com/ThinkFlowLab/FlyBot/issues/2099)). diff --git a/docs/governance.md b/docs/governance.md index 5c3b0bc24..86b7ab6ea 100644 --- a/docs/governance.md +++ b/docs/governance.md @@ -153,10 +153,13 @@ Policy-sign: The comment it names must be on that pull request, must have been written by someone other than the pull request's author, and must name the tip it signs — the head sha, in full or abbreviated to at -least twelve characters. A GitHub bot account cannot sign. `Test`'s `Gate change signature` step runs -before the suite, so a missing line, a comment that is not on the pull request, a self-signature, or a -comment written for another tip fails the pull request. Anyone else with comment access can sign, and -editing the body is the whole ceremony: no new workflow, no threshold changes, no merge path. +least twelve characters. A GitHub bot account cannot sign. `Test`'s `Gate change signature` step runs, +before the suite, the copy of the checker checked out from the pull request's **base** commit — not the +copy in the pull request's own tree — so a change that edits, weakens or deletes the checker is still +judged by the one it started from. A missing line, a comment that is not on the pull request, a +self-signature, or a comment written for another tip fails the pull request. +Anyone else with comment access can sign, and editing the body is the whole ceremony: no new workflow, +no threshold changes, no merge path. A signature signs one head, not one pull request: every new tip needs a new comment and a new line. That is the price of binding, and it is the point — a signature that survived a push would let an