Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions .github/scripts/test-workflow.test.mjs
Original file line number Diff line number Diff line change
@@ -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}`)
Expand All @@ -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', () => {
Expand Down
12 changes: 11 additions & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,20 @@ jobs:
# policy/gates.json (#2054): a pull request that edits an acceptance gate carries
# `Policy-sign: <comment id or url>` 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'
Expand Down
1 change: 1 addition & 0 deletions changelog.d/2099-base-policy-sign.fixed.md
Original file line number Diff line number Diff line change
@@ -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)).
11 changes: 7 additions & 4 deletions docs/governance.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,13 @@ Policy-sign: <comment id or url>

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
Expand Down
Loading