diff --git a/.github/scripts/policy-sign.mjs b/.github/scripts/policy-sign.mjs new file mode 100644 index 000000000..63d349d29 --- /dev/null +++ b/.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: ` 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()) diff --git a/.github/scripts/policy-sign.test.mjs b/.github/scripts/policy-sign.test.mjs new file mode 100644 index 000000000..e095fe1b8 --- /dev/null +++ b/.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: ` 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') +}) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e4c3d7e41..ab7018ffc 100644 --- a/.github/workflows/test.yml +++ b/.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: ` 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' diff --git a/changelog.d/2054-gate-signature.added.md b/changelog.d/2054-gate-signature.added.md new file mode 100644 index 000000000..4d726aadb --- /dev/null +++ b/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)). diff --git a/docs/governance.md b/docs/governance.md index 1387644f7..5c3b0bc24 100644 --- a/docs/governance.md +++ b/docs/governance.md @@ -139,3 +139,32 @@ produces follow-up tasks carrying that file as evidence. `blocked` states the bl sentence and writes nothing. A Project Assistant may take a position but never closes the meeting or writes the decision; the Chief or the owner does. The meeting record itself stays in the existing store, and the file it archives is the durable part. + +## 9. Changing a gate needs a second person + +The acceptance gates — `merge-gate`, `review-gate`, `pass-label`, `risk-gate`, +`scripts/self-test-catalog.mjs`, and the checker and workflow step that enforce this rule — are +listed in [`policy/gates.json`](../policy/gates.json). A pull request that edits one of them, or +anything under `policy/`, must carry one line in its body: + +``` +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. + +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 +author sign a reviewable change set and then push another one under it. + +The cost is accepted and is real: a gate change — including our own Bots' pull requests — waits for a +second person to comment before it can merge, so the Lead of the round that owns the pull request asks +for the signature and adds the line. A line written into the body after the check went green starts no +new run; the step reads the body on the next push and on the queue entry. The step runs in the `test` +job of every pull request and fails closed when it cannot read the pull request at all, so a GitHub API +outage reddens `tests-ok` for every pull request, not only the ones that change a gate. diff --git a/policy/gates.json b/policy/gates.json new file mode 100644 index 000000000..21a301467 --- /dev/null +++ b/policy/gates.json @@ -0,0 +1,15 @@ +{ + "$comment": "The acceptance gates judge this repository and live in the same repository they judge (#2054). A pull request that changes one of these files, or anything under policy/, must carry `Policy-sign: ` in its body, and that comment must have been written by someone other than the pull request's author. The checker itself and the workflow that runs it are listed too: a rule that does not cover its own switch is advice, not a gate. An entry ending in `/` is a directory, so a new file under policy/ is covered without a second edit.", + "line": "Policy-sign: ", + "signer": "someone other than the pull request's author; a GitHub bot account cannot sign", + "gateFiles": [ + ".github/scripts/merge-gate.mjs", + ".github/scripts/review-gate.mjs", + ".github/scripts/pass-label.mjs", + ".github/scripts/risk-gate.mjs", + "scripts/self-test-catalog.mjs", + ".github/scripts/policy-sign.mjs", + ".github/workflows/test.yml", + "policy/" + ] +}