Require a second person's signature to change an acceptance gate

.github/scripts/policy-sign.mjs

@@ -0,0 +1,155 @@
+// A pull request that edits an acceptance gate needs another person's signature (#2054).
+//
+// The gates that judge this repository — `merge-gate.mjs`, `review-gate.mjs`, `pass-label.mjs`,
+// `risk-gate.mjs` and `scripts/self-test-catalog.mjs` — live in the same repository they judge, so a
+// pull request can change what "pass" means and every gate downstream will read the new rule. This
+// is the smallest structural step against that: `policy/gates.json` names the files, and a pull
+// request touching one of them must carry `Policy-sign: <comment id or url>` in its body, naming a
+// comment written by someone other than the pull request's author. A person cannot sign their own
+// gate change; anyone else with comment access can. The comment must also name the tip it signs: a
+// signature that outlived a push would let the author sign one change set and merge another.
+//
+//   node .github/scripts/policy-sign.mjs                 # in CI: reads the event and the pull request
+//   node .github/scripts/policy-sign.mjs --pr 2054       # by hand
+//
+// The rule is `judge` below and is pure: the CLI reads the pull request and its comments with `gh`
+// and hands them over, so the tests never touch the network. Failing closed is deliberate — a check
+// that could not run is not a licence, the same rule the host's own reads follow.
+import { readFileSync } from 'node:fs'
+import { fileURLToPath } from 'node:url'
+
+export const POLICY_PATH = fileURLToPath(new URL('../../policy/gates.json', import.meta.url))
+
+export function loadPolicy(path = POLICY_PATH) {
+  const policy = JSON.parse(readFileSync(path, 'utf8'))
+  const files = policy?.gateFiles
+  if (!Array.isArray(files) || files.length === 0) throw new Error(`${path}: gateFiles must be a non-empty list`)
+  return policy
+}
+
+// The changed files that are gate files. An entry ending in `/` is a directory: anything under it
+// counts, so a new file in `policy/` needs no second list to be covered.
+export function gateFiles(changed = [], policy = loadPolicy()) {
+  const entries = policy.gateFiles.map(String)
+  return changed
+    .map(String)
+    .filter(file => entries.some(entry => entry.endsWith('/') ? file.startsWith(entry) : file === entry))
+}
+
+// The id a `Policy-sign:` line names. An id is taken as it is; a url is read as its trailing comment
+// number, so the line can quote the comment a person actually posted. A value that is neither is
+// returned unchanged and then refused as "not on this pull request" — one refusal, not two.
+export function parseSignature(body = '') {
+  const match = /^Policy-sign:\s*(\S+)\s*$/m.exec(String(body ?? ''))
+  if (match === null) return undefined
+  const value = match[1]
+  if (/^\d+$/.test(value)) return value
+  const url = /(?:#issuecomment-|\/comments\/)(\d+)\s*$/.exec(value)
+  return url === null ? value : url[1]
+}
+
+export function judge({ files = [], body = '', author = '', comments = [], head = '', policy = loadPolicy() }) {
+  const touched = gateFiles(files, policy)
+  if (touched.length === 0) return { ok: true, reason: 'no gate file changed, so no signature is needed' }
+
+  const named = touched.length === 1 ? touched[0] : `${touched.length} gate files (${touched.join(', ')})`
+  const token = parseSignature(body)
+  if (token === undefined) {
+    return { ok: false, reason: `a gate file changed (${named}), and the body carries no \`Policy-sign:\` line naming a comment` }
+  }
+  const comment = comments.find(entry => String(entry?.id ?? '') === String(token))
+  if (comment === undefined) {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, which is not on this pull request` }
+  }
+  const login = String(comment.user?.login ?? '')
+  if (login !== '' && login === String(author)) {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, written by the pull request's own author — a gate change needs another person` }
+  }
+  if (comment.user?.type !== 'User') {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, written by a bot account (${login === '' ? 'unknown login' : login})` }
+  }
+  // The comment has to name the tip it signs. A comment reviewed one change set; without this, the
+  // author can sign a harmless tip and then push the change the signature was supposed to cover.
+  const tip = String(head ?? '').trim().toLowerCase()
+  if (tip === '') {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, but this run carries no head sha to bind it to — the check fails closed` }
+  }
+  const tokens = String(comment.body ?? '').toLowerCase().match(/[0-9a-f]{12,40}/g) ?? []
+  if (!tokens.some(value => tip.startsWith(value))) {
+    return { ok: false, reason: `\`Policy-sign:\` names comment ${token}, which does not name this tip ${String(head).trim()} — a signature binds the tip it was written for, so a new tip needs a new signature` }
+  }
+  return { ok: true, reason: `signed by ${login} in comment ${token}`, signer: login }
+}
+
+const API = 'https://api.github.com'
+
+// The pull request this run is about: its own event, the queue branch for a merge-group entry, or
+// `--pr` by hand. Anything else (a push to main, a manual run) carries no pull request to judge.
+export function pullRequestNumber(argv = process.argv.slice(2), env = process.env) {
+  const at = argv.indexOf('--pr')
+  if (at !== -1 && argv[at + 1] !== undefined) return Number(argv[at + 1])
+  let event = {}
+  try { event = JSON.parse(readFileSync(env.GITHUB_EVENT_PATH, 'utf8')) } catch { return undefined }
+  if (event.pull_request?.number !== undefined) return event.pull_request.number
+  const ref = event.merge_group?.head_ref ?? ''
+  const match = /^gh-readonly-queue\/[^/]+\/pr-(\d+)-/.exec(ref)
+  return match === null ? undefined : Number(match[1])
+}
+
+// GitHub's own REST API, with the job's token: the workflow already has `pull-requests: read`, and
+// the runner is not assumed to carry `gh` (the shared box the Bots work on does not).
+//
+// A single-object answer (the pull request itself) is returned as it is: only a list is paged, and
+// only a list may be flattened. Flattening the object wrapped it in a one-element array, so every
+// read of `pull.body`, `pull.user` and `pull.head` came back empty and every gate change was
+// refused with "carries no `Policy-sign:` line" (#2060).
+async function api(path, token) {
+  const pages = []
+  for (let page = 1; ; page += 1) {
+    const response = await fetch(`${API}${path}${path.includes('?') ? '&' : '?'}per_page=100&page=${page}`, {
+      headers: { Authorization: `Bearer ${token}`, Accept: 'application/vnd.github+json', 'X-GitHub-Api-Version': '2022-11-28' },
+    })
+    if (!response.ok) throw new Error(`${response.status} ${response.statusText} on ${path}`)
+    const body = await response.json()
+    if (!Array.isArray(body)) return body
+    pages.push(body)
+    if (body.length < 100) return pages.flat()
+  }
+}
+
+export async function readPull(repo, number, token) {
+  const pull = await api(`/repos/${repo}/pulls/${number}`, token)
+  const files = (await api(`/repos/${repo}/pulls/${number}/files`, token)).map(file => file.filename)
+  const comments = await api(`/repos/${repo}/issues/${number}/comments`, token)
+  return { files, body: pull.body ?? '', author: pull.user?.login ?? '', head: pull.head?.sha ?? '', comments }
+}
+
+export async function main(argv = process.argv.slice(2), env = process.env) {
+  const repo = env.GITHUB_REPOSITORY ?? (() => {
+    const at = argv.indexOf('--repo')
+    return at === -1 ? '' : argv[at + 1]
+  })()
+  const number = pullRequestNumber(argv, env)
+  if (number === undefined) {
+    process.stdout.write('policy-sign: this run carries no pull request, so there is nothing to sign\n')
+    return 0
+  }
+  let pull
+  try {
+    pull = await readPull(repo, number, env.GH_TOKEN ?? env.GITHUB_TOKEN ?? '')
+  } catch (error) {
+    const why = String(error?.message ?? error).trim().split('\n')[0]
+    process.stdout.write(`::error::policy-sign cannot read pull request #${number}: ${why}\n`)
+    return 1
+  }
+  const verdict = judge({ files: pull.files, body: pull.body, author: pull.author, head: pull.head, comments: pull.comments })
+  if (verdict.ok) {
+    process.stdout.write(`policy-sign ok (#${number}): ${verdict.reason}\n`)
+    return 0
+  }
+  process.stdout.write(`policy-sign refused (#${number}): ${verdict.reason}\n`)
+  process.stdout.write(`::error::${verdict.reason}\n`)
+  return 1
+}
+
+if (process.argv[1] === fileURLToPath(import.meta.url)) process.exit(await main())

.github/scripts/policy-sign.test.mjs

@@ -0,0 +1,215 @@
+// policy/gates.json's signature rule (#2054): a pull request that edits an acceptance gate must
+// carry `Policy-sign: <comment id or url>` in its body, and that comment must be written by someone
+// other than the author and must name the tip it signs. These tests pin the cases the issue names —
+// no line, own signature, another person's signature, an unrelated comment — plus the shapes a
+// signature can be wrong in: a comment that is not on the pull request, a bot account, and a comment
+// written for an older tip (a signature that does not move with the head signs nothing).
+import { test } from 'node:test'
+import assert from 'node:assert/strict'
+import { judge, loadPolicy, main, parseSignature, readPull } from './policy-sign.mjs'
+
+const policy = loadPolicy()
+
+const GATE_FILE = '.github/scripts/merge-gate.mjs'
+const PR = { author: 'HugoXK' }
+const HEAD = '9da401de2bdcd80ba851377ad1014d3bc5e5a90e'
+const OTHER_TIP = '2b42c2ec9951c1279b0d8634f4672f887f82cb10'
+
+test('#2054: a pull request that touches no gate file needs no signature', () => {
+  const verdict = judge({ files: ['core/src/host/project-state.js'], body: 'Fixes #1', ...PR, head: HEAD, comments: [], policy })
+  assert.equal(verdict.ok, true)
+  assert.match(verdict.reason, /no gate file/)
+})
+
+test('#2054: a gate change with no `Policy-sign:` line is refused, and names the line', () => {
+  const verdict = judge({ files: ['core/src/x.js', GATE_FILE], body: 'Fixes #1\n\nTightens the gate.', ...PR, head: HEAD, comments: [], policy })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /`Policy-sign:`/)
+  assert.match(verdict.reason, /merge-gate\.mjs/, 'the refusal names the file that made the signature required')
+})
+
+test('#2054: a signature written by the pull request author is refused', () => {
+  const verdict = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: 42',
+    ...PR,
+    head: HEAD,
+    comments: [{ id: 42, body: `ok to change the gate at ${HEAD}`, user: { login: 'HugoXK', type: 'User' } }],
+    policy,
+  })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /own author/)
+})
+
+test('#2054: another person\'s signature is accepted, by id or by url', () => {
+  const comments = [{ id: 42, body: `ok to change the gate at ${HEAD}`, user: { login: 'Birfy', type: 'User' } }]
+  const byId = judge({ files: [GATE_FILE], body: 'Fixes #1\n\nPolicy-sign: 42', ...PR, head: HEAD, comments, policy })
+  assert.deepEqual(byId, { ok: true, reason: 'signed by Birfy in comment 42', signer: 'Birfy' })
+
+  const byUrl = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: https://github.com/ThinkFlowLab/FlyBot/pull/2054#issuecomment-42',
+    ...PR,
+    head: HEAD,
+    comments,
+    policy,
+  })
+  assert.equal(byUrl.ok, true, 'a url is read as its trailing comment id')
+  assert.equal(byUrl.signer, 'Birfy')
+})
+
+test('#2054: a signature naming this tip by its short prefix is accepted', () => {
+  const verdict = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: 42',
+    ...PR,
+    head: HEAD,
+    comments: [{ id: 42, body: 'ok to change the gate at 9da401de2bdc', user: { login: 'Birfy', type: 'User' } }],
+    policy,
+  })
+  assert.equal(verdict.ok, true, 'a comment quoting the tip abbreviated is still about this tip')
+})
+
+test('#2054: a signature written for another tip is refused', () => {
+  const verdict = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: 42',
+    ...PR,
+    head: HEAD,
+    comments: [{ id: 42, body: `ok to change the gate at ${OTHER_TIP}`, user: { login: 'Birfy', type: 'User' } }],
+    policy,
+  })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /does not name this tip/)
+  assert.match(verdict.reason, /a new tip needs a new signature/)
+})
+
+test('#2054: a signature that names no tip at all is refused', () => {
+  const verdict = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: 42',
+    ...PR,
+    head: HEAD,
+    comments: [{ id: 42, body: 'ok to change the gate', user: { login: 'Birfy', type: 'User' } }],
+    policy,
+  })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /does not name this tip/)
+})
+
+test('#2054: a signature naming a comment that is not on the pull request is refused', () => {
+  const verdict = judge({ files: [GATE_FILE], body: 'Fixes #1\n\nPolicy-sign: 99', ...PR, head: HEAD, comments: [{ id: 42, user: { login: 'Birfy', type: 'User' } }], policy })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /not on this pull request/)
+})
+
+test('#2054: a signature written by a bot account is refused', () => {
+  const verdict = judge({
+    files: [GATE_FILE],
+    body: 'Fixes #1\n\nPolicy-sign: 42',
+    ...PR,
+    head: HEAD,
+    comments: [{ id: 42, body: `ok at ${HEAD}`, user: { login: 'some-helper[bot]', type: 'Bot' } }],
+    policy,
+  })
+  assert.equal(verdict.ok, false)
+  assert.match(verdict.reason, /bot account/)
+})
+
+test('#2054: the line is read from the body only, on its own line', () => {
+  assert.equal(parseSignature('Fixes #1\n\nPolicy-sign: 42\n'), '42')
+  assert.equal(parseSignature('Fixes #1\n\n> Policy-sign: 42'), undefined, 'a quoted line is prose, not the signature')
+  assert.equal(parseSignature('Fixes #1\n\nWe need a Policy-sign: 42 here'), undefined, 'the line must start with the field')
+})
+
+// The read path had no test, and it was wrong: `api()` flattened every answer, so the pull request
+// — one object, not a list — arrived as `[{...}]` and `body`, `author` and `head` all read empty.
+// Every gate change was then refused with "carries no `Policy-sign:` line", so a signed gate change
+// could never pass (#2060). These cases replace `fetch` and run the code the workflow runs.
+function stubFetch(routes) {
+  const real = globalThis.fetch
+  const calls = []
+  const keys = Object.keys(routes).sort((a, b) => b.length - a.length)
+  globalThis.fetch = async input => {
+    const url = new URL(String(input))
+    const key = keys.find(suffix => url.pathname.endsWith(suffix))
+    if (key === undefined) throw new Error(`unexpected fetch ${url.pathname}`)
+    const page = Number(url.searchParams.get('page') ?? 1)
+    const answer = typeof routes[key] === 'function' ? routes[key](page) : routes[key]
+    calls.push({ path: url.pathname, page })
+    return { ok: true, status: 200, statusText: 'OK', json: async () => structuredClone(answer) }
+  }
+  return { calls, restore: () => { globalThis.fetch = real } }
+}
+
+function pullAnswer(body) {
+  return { body, user: { login: 'HugoXK' }, head: { sha: HEAD } }
+}
+
+const SIGNED_PULL = {
+  files: [{ filename: 'core/src/x.js' }, { filename: GATE_FILE }],
+  comments: [{ id: 42, body: `ok to change the gate at ${HEAD}`, user: { login: 'Birfy', type: 'User' } }],
+}
+
+async function runMain(routes) {
+  const stub = stubFetch(routes)
+  const written = []
+  const write = process.stdout.write
+  process.stdout.write = chunk => { written.push(String(chunk)); return true }
+  try {
+    const code = await main(['--pr', '2060'], { GITHUB_REPOSITORY: 'ThinkFlowLab/FlyBot', GH_TOKEN: 'token' })
+    return { code, out: written.join(''), calls: stub.calls }
+  } finally {
+    process.stdout.write = write
+    stub.restore()
+  }
+}
+
+test('#2060: readPull keeps the pull request it read instead of wrapping it in a list', async t => {
+  const stub = stubFetch({
+    '/pulls/2060/files': SIGNED_PULL.files,
+    '/issues/2060/comments': SIGNED_PULL.comments,
+    '/pulls/2060': pullAnswer('Fixes #1\n\nPolicy-sign: 42'),
+  })
+  t.after(stub.restore)
+  const pull = await readPull('ThinkFlowLab/FlyBot', 2060, 'token')
+  assert.deepEqual(pull.files, ['core/src/x.js', GATE_FILE])
+  assert.equal(pull.body, 'Fixes #1\n\nPolicy-sign: 42', 'the body is on the object; a one-element array has none')
+  assert.equal(pull.author, 'HugoXK')
+  assert.equal(pull.head, HEAD)
+  assert.equal(pull.comments.length, 1)
+})
+
+test('#2060: a signed gate change is accepted end to end through main()', async t => {
+  const run = await runMain({
+    '/pulls/2060/files': SIGNED_PULL.files,
+    '/issues/2060/comments': SIGNED_PULL.comments,
+    '/pulls/2060': pullAnswer('Fixes #1\n\nPolicy-sign: 42'),
+  })
+  assert.equal(run.code, 0, run.out)
+  assert.match(run.out, /policy-sign ok \(#2060\): signed by Birfy in comment 42/)
+})
+
+test('#2060: an unsigned gate change is still refused end to end through main()', async t => {
+  const run = await runMain({
+    '/pulls/2060/files': SIGNED_PULL.files,
+    '/issues/2060/comments': SIGNED_PULL.comments,
+    '/pulls/2060': pullAnswer('Fixes #1\n\nTightens the gate.'),
+  })
+  assert.equal(run.code, 1)
+  assert.match(run.out, /carries no `Policy-sign:` line/)
+})
+
+test('#2060: a list answer is still paged and flattened', async t => {
+  const page1 = Array.from({ length: 100 }, (unused, index) => ({ filename: `p${index}.js` }))
+  const stub = stubFetch({
+    '/pulls/2060/files': page => (page === 1 ? page1 : [{ filename: GATE_FILE }]),
+    '/issues/2060/comments': [],
+    '/pulls/2060': pullAnswer('Fixes #1'),
+  })
+  t.after(stub.restore)
+  const pull = await readPull('ThinkFlowLab/FlyBot', 2060, 'token')
+  assert.equal(pull.files.length, 101)
+  assert.equal(stub.calls.filter(call => call.path.endsWith('/files')).length, 2, 'a full page asks for the next one')
+  assert.equal(pull.body, 'Fixes #1')
+})

.github/workflows/test.yml

@@ -31,10 +31,23 @@ jobs:
     name: ubuntu-latest · Node 24 · DSH 0.2.0-rc.2
     runs-on: ubuntu-latest
     timeout-minutes: 25
+    # `pull-requests: read` is for the gate-signature step (#2054): it reads the pull request's
+    # changed files, body and comments. The workflow-level `contents: read` alone would make that
+    # call a 403.
+    permissions:
+      contents: read
+      pull-requests: read
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1  # v7.0.1
         with:
           persist-credentials: false
+      # 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.
+      - name: Gate change signature
+        env:
+          GH_TOKEN: ${{ github.token }}
+        run: node .github/scripts/policy-sign.mjs
       - uses: ./.github/actions/flybot-tests
         with:
           node: '24'

changelog.d/2054-gate-signature.added.md

@@ -0,0 +1 @@
+A pull request that edits an acceptance gate (`policy/gates.json`) must carry `Policy-sign:` naming a comment by someone other than its author that also names the tip it signs, so a signature cannot be carried over to a later push, and `Test` fails the pull request otherwise — a pull request can no longer change what "pass" means on its own ([#2054](https://github.com/ThinkFlowLab/FlyBot/issues/2054)).