diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index c1a18949..94bb70a2 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -39,6 +39,9 @@ Always include: ```text Verdict: PASS | FAIL | INC at Written-by: Reviewer +Signed-by: Reviewer +Nonce: <16 hex characters, one per report> +Signature: Posted-by: Ran: @@ -61,6 +64,21 @@ first line and treat any other shape as no conclusion. The sha is the whole 40 c `role` is the `Written-by` name and is not an `Owner:` of the pull request. The gate parses the line; it does not check a signature. +The three signature lines are what turns the attribution into proof (#1110): the gate verifies +`Signature:` against your workstation's public key in `.github/agent/signers.json`, over +`FBV1|/#@@@@`. `Signed-by:` must name the +same Bot as `Written-by:`, the nonce must be fresh for this report, and the head must be the **full** sha +you judged — a short sha on the `Verdict:` line is fine, but it is not what you sign, so copy the 40 +characters. A `Verdict:` that does not verify is not a conclusion at all: it cannot pass and it cannot hold a +merge, and the gate summary lists it as `unverified:`. The signature and the attestation are separate +refusals: write both when the posting login is the pull request author. Sign with your own key, which stays +in your secret card: + +```sh +node .github/scripts/sign-verdict.mjs --repo ThinkFlowLab/FlyBot --pr --verdict PASS \ + --workstation Reviewer --head +``` + What the gates read as evidence (`review-gate.mjs` `parseReport`; #660–#663 refused at the queue for the first point): diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index db65a92f..8802243d 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -28,6 +28,7 @@ import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { isMechanicalRelease, judge } from './merge-gate.mjs' import { loadPolicy } from './review-gate.mjs' +import { loadSigners } from './verdict-signature.mjs' import { guard, recordOnlyAt, short } from './enqueue-rules.mjs' // The rule bodies moved to enqueue-rules.mjs (#960); re-exported here because this is the module the @@ -82,7 +83,9 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p // believes it has, and `--sha` lets a caller insist on the head it read a moment ago. // `judge` asks `adminsFor` when this tier needs a PASS. const policy = loadPolicy(policyPath) - const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue', body: pull.body }) + // #1110: the same keys `merge-gate` reads, so the check and the printed command cannot disagree. + const signers = loadSigners({ policy, path: arg('--signers') || env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) + const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, pr, signers, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue', body: pull.body }) const mechanicalRelease = isMechanicalRelease(files.map((f) => f.filename)) // The branch this repository owns, not a contributor's: `head.repo` is null when the fork was deleted, // which is not ours either. Both sides are lower-cased because the repository name in the API paths is diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 1c5b450e..5ee1c93f 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -25,6 +25,7 @@ import { appendFileSync } from 'node:fs' import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { adminsFor, decide, loadPolicy } from './review-gate.mjs' +import { loadSigners } from './verdict-signature.mjs' import { createGate, loadRules } from './risk-gate.mjs' import { guard } from './enqueue-rules.mjs' @@ -100,7 +101,7 @@ const short = (s) => String(s ?? '').slice(0, SHORT) // expectedHead: optional, a head the caller insists on (`--sha` on a hand run); a different live head // refuses. The queue's base sha is never a head: a merge_group entry passes none here (see // prFromHeadRef). reports: its comments and review bodies, oldest first (review-gate main()). -export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo, body, owners } = {}) { +export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo, body, owners, pr, signers } = {}) { const reasons = [] const target = String(head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -120,7 +121,7 @@ export function judge({ labels, files, head, author, reports, policy, expectedHe // A tier that needs no PASS also needs no permission lookup. const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } - : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }) }) + : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers }) if (!verdict.ok) reasons.push(verdict.reason) // A report says what the queue entry would refuse; only the queue entry refuses. Fail a // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a @@ -150,6 +151,11 @@ export function render(result, { pr, labels } = {}) { '', `Pull request #${pr}, head \`${result.head}\`, labels: ${(labels ?? []).join(', ') || '(none)'}.`, ] + // #1110: a conclusion whose signature does not verify is named, never silently dropped. + const unverified = result.verdict?.unverified ?? [] + if (unverified.length) { + out.push('', 'Unverified — not read as conclusions:', ...unverified.slice(-5).map((e) => `- unverified: ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${short(e.tip ?? 'no tip')} — ${String(e.why ?? '').replace(/^[^:]+: unverified — /, '')}`)) + } const seen = (result.verdict?.judged ?? []).filter((e) => e.counts || e.verdict) if (seen.length) out.push('', 'Conclusive texts read (newest last):', ...seen.slice(-5).map((e) => `- ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${short(e.tip ?? 'no tip')}${e.counts ? '' : ' (not counted)'}`)) out.push('', '_Rules: [`AGENTS.md` → Merge gates](../../AGENTS.md#merge-gates). This check refuses the entry; it never labels, comments, approves or merges._') @@ -217,7 +223,10 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO // base copy of risk-gate.mjs (see exemptFromPass). const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) const policy = loadPolicy(policyPath) - const result = judge({ labels, files, head, author, reports, policy, gh, repo, expectedHead: arg('--sha'), mode, body: pull.body }) + // #1110: one load per run, from the same checkout the policy came from (the workflow checks out the base + // branch, so a pull request cannot edit the keys it is judged by). + const signers = loadSigners({ policy, path: arg('--signers') || env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) + const result = judge({ labels, files, head, author, reports, policy, gh, repo, pr, signers, expectedHead: arg('--sha'), mode, body: pull.body }) // #960 (B): `enqueue-guard` is not a required context today, so `merge-gate` is the only gate on a // queue entry. `judge` passes the pull request body into `decide` so an author-login PASS is checked // against `Owner:` (#644). It does not apply the body rules itself: the fallback runs the guard's own diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index aae3154d..410d2ecf 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -36,6 +36,7 @@ import { readFileSync, appendFileSync } from 'node:fs' import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' +import { SIGNERS_PATH, loadSigners, verifyReport } from './verdict-signature.mjs' const SHORT = 12 // #676: a conclusion is read only from someone GitHub ties to this repository. The fail-closed rule @@ -253,10 +254,31 @@ const where = (r) => `${r.kind === 'review' ? 'review' : 'comment'} by ${r.login const isBotAccount = (r) => String(r.type ?? '').toLowerCase() === 'bot' || /\[bot\]$/i.test(login(r)) const botKind = (r) => (String(r.type ?? '').toLowerCase() === 'bot' ? 'account type Bot' : 'login ends with [bot]') +// #1110: what the policy asks of a signature, and the one refusal a broken configuration produces. The +// roster lives in the policy, the public keys live in `signers.json`, and both must be present together: +// half of that pair is not a reason to read unsigned reports, it is a reason to refuse every pull request +// until it is repaired. +function signingState({ policy, signers, pr }) { + const roster = Array.isArray(policy?.signers) ? policy.signers.map((name) => String(name).trim()).filter(Boolean) : [] + if (!roster.length) return { enforced: false, error: null } + if (!signers || signers.error === undefined) { + return { enforced: false, error: `.github/agent/review-policy.json lists ${roster.length} signing workstation(s) (${roster.join(', ')}), but ${SIGNERS_PATH} was not loaded: the gate refuses rather than reading unsigned reports` } + } + if (signers.error) return { enforced: false, error: signers.error } + if (!String(pr ?? '').trim()) { + return { enforced: false, error: 'a signed verdict names the pull request number it belongs to, and this run asked about no pull request' } + } + return { enforced: true, required: roster, keys: signers.keys, error: null } +} + // reports: [{ login, body, at, kind, id }] from the comments and review bodies of one PR. // `body` is the pull request body (live entry points pass it). `owners` is that list already parsed; // either one makes the Owner check possible. Neither one refuses an author-login conclusion. -export function decide({ head, author, reports, policy, admins, body, owners } = {}) { +// `signers`: the loaded `.github/agent/signers.json` (`loadSigners`). When the policy's `signers` roster +// is non-empty every conclusion must verify against it (#1110); an unverified one is not a conclusion and +// never joins the newest-wins scan, so it can neither pass nor veto. `repo` and `pr` are signed into the +// report, so a signature cannot move to another pull request or another tip. +export function decide({ head, author, reports, policy, admins, body, owners, repo, pr, signers } = {}) { const headSha = String(head ?? '').toLowerCase() const authorLogin = String(author ?? '').toLowerCase() const ownersKnown = owners !== undefined || body !== undefined @@ -268,6 +290,12 @@ export function decide({ head, author, reports, policy, admins, body, owners } = const reasons = [] // reports that do not name this tip at all const judged = [] const atTip = [] // conclusions that name this tip, valid or not, in the order the API returned them + const unverified = [] // conclusions this tip carries whose signature does not verify: not read at all + // #1110: the roster in `review-policy.json` is the switch. A roster with no loaded signers file, or a + // file that cannot be read or is missing a workstation's key, is a configuration error — the gate + // refuses rather than falling back to unsigned reports, which is the hole this closes. + const signing = signingState({ policy, signers, pr }) + if (signing.error) return { ok: false, verdict: null, by: null, tip: null, reason: signing.error, judged, unverified } for (const r of reports ?? []) { const parsed = parseReport(r.body) @@ -293,6 +321,19 @@ export function decide({ head, author, reports, policy, admins, body, owners } = reasons.push(entry.why) continue } + // #1110, before anything below can read it: a conclusion whose signature does not verify is not a + // conclusion. It stays out of `atTip`, so it neither passes nor refuses an older PASS, and the summary + // lists it as unverified. Reading it here — before the association, writer and reviewers checks — is + // what makes the rule one cut rather than one more refusal stacked on the old path. + if (signing.enforced) { + const signed = verifyReport({ repo, pr, head: headSha, verdict: parsed.verdict, writer: parsed.writer, body: r.body, signers: signing }) + if (!signed.ok) { + entry.why = `${where(r)}: unverified — ${signed.reason}` + unverified.push(entry) + continue + } + entry.workstation = signed.workstation + } const association = String(r.association ?? '').toUpperCase() // #928 (owner decision 2026-10-24): a login the policy lists in `selfReview` may be a CONTRIBUTOR // and still count, but only on its own pull request. That is one exception for one person, not a @@ -330,10 +371,10 @@ export function decide({ head, author, reports, policy, admins, body, owners } = entry.counts = true } - if (!reviewers.size) return { ok: false, verdict: null, by: null, tip: null, reason: 'no reviewer is listed in .github/agent/review-policy.json', judged } + if (!reviewers.size) return { ok: false, verdict: null, by: null, tip: null, reason: 'no reviewer is listed in .github/agent/review-policy.json', judged, unverified } if (!atTip.length) { const reason = reasons.length ? reasons.join('; ') : `no Verifier report names the tip ${headSha.slice(0, SHORT)}` - return { ok: false, verdict: null, by: null, tip: null, reason, judged } + return { ok: false, verdict: null, by: null, tip: null, reason, judged, unverified } } // The newest word at this tip decides, and that includes a conclusion the checks refused: an invalid @@ -343,15 +384,15 @@ export function decide({ head, author, reports, policy, admins, body, owners } = .map((e, i) => ({ e, i, t: Date.parse(e.at ?? 0) || 0 })) .sort((a, b) => a.t - b.t || a.i - b.i) .at(-1).e - if (!last.counts) return { ok: false, verdict: last.verdict, by: null, tip: last.tip, id: last.id, kind: last.kind, reason: last.why, judged } + if (!last.counts) return { ok: false, verdict: last.verdict, by: null, tip: last.tip, id: last.id, kind: last.kind, reason: last.why, judged, unverified } const { parsed } = last if (last.verdict !== 'PASS') { - return { ok: false, verdict: last.verdict, by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login} says ${last.verdict ?? 'nothing'} at this tip`, judged } + return { ok: false, verdict: last.verdict, by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login} says ${last.verdict ?? 'nothing'} at this tip`, judged, unverified } } if (!parsed.ran.length) { - return { ok: false, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS lists no command it ran`, judged } + return { ok: false, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS lists no command it ran`, judged, unverified } } - return { ok: true, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS names this tip`, judged } + return { ok: true, verdict: 'PASS', by: last.login, tip: last.tip, id: last.id, kind: last.kind, reason: `${last.login}'s PASS names this tip`, judged, unverified } } export function render(result, { pr, head, author } = {}) { @@ -364,6 +405,13 @@ export function render(result, { pr, head, author } = {}) { '', `Pull request #${pr}, author ${author}, head \`${String(head).slice(0, SHORT)}\`.`, ] + // #1110: a conclusion this tip carries and the gate could not verify is named here, so a reader never + // wonders why a PASS that is visibly on the pull request is not the one being reported. + const unverifiedReports = result.unverified ?? [] + if (unverifiedReports.length) { + out.push('', 'Unverified — not read as conclusions:') + for (const e of unverifiedReports.slice(0, 10)) out.push(`- unverified: ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${(e.tip ?? 'no tip').slice(0, SHORT)} — ${String(e.why ?? '').replace(/^[^:]+: unverified — /, '')}`) + } const seen = result.judged.filter((e) => e.counts || e.verdict) if (seen.length) out.push('', 'Verifier reports read:', ...seen.slice(0, 10).map((e) => `- ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${(e.tip ?? 'no tip').slice(0, SHORT)}, written-by ${e.writer ?? 'nothing'}${e.counts ? '' : ' (not counted)'}`)) out.push('', '_Report only (Phase 1): this does not block, label, approve or merge anything. Rules: `.github/agent/review-policy.json`; see [`docs/autonomous-dev.md`](../../docs/autonomous-dev.md)._') @@ -402,7 +450,11 @@ function main() { ] const policy = loadPolicy(policyPath) - const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }) }) + // #1110: the keys come from the same checkout the policy does, so a pull request that edits either file + // changes only what its own run reads. `loadSigners` is fail-closed on its own; `decide` refuses when the + // policy lists a roster and this is missing. + const signers = loadSigners({ policy, path: arg('--signers') || process.env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) + const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers }) console.log(`review-gate (report only): PR #${pr} tip ${String(head).slice(0, SHORT)} — ${result.ok ? `PASS by ${result.by}` : result.reason}`) const text = render(result, { pr, head, author }) if (process.env.GITHUB_STEP_SUMMARY) appendFileSync(process.env.GITHUB_STEP_SUMMARY, text + '\n') diff --git a/.github/scripts/sign-verdict.mjs b/.github/scripts/sign-verdict.mjs new file mode 100644 index 00000000..22829fc7 --- /dev/null +++ b/.github/scripts/sign-verdict.mjs @@ -0,0 +1,77 @@ +// The three lines a reviewing workstation adds to its conclusion (#1110). +// +// DSH_SECRET_VERDICT_KEY_REVIEWER="$(cat reviewer-key.pem)" \ +// node .github/scripts/sign-verdict.mjs --repo ThinkFlowLab/FlyBot --pr 1132 --verdict PASS \ +// --workstation Reviewer --head +// +// It prints the canonical string it signed and the three lines to paste under the conclusion: +// +// Signed-by: Reviewer +// Nonce: <16 hex characters, one per report> +// Signature: +// +// The key comes from the workstation's own secret — `DSH_SECRET_VERDICT_KEY_` by default, +// or `--key-file ` for a hand run — and is never printed, echoed or written. The head must be the +// full 40 characters: a short sha is a prefix of many heads, and the signature has to name exactly one +// tip (#1112). `--repo` and `--pr` default to the workflow's `GH_REPO`/`PR`, so a Bot signs the pull +// request it is judging unless it says otherwise. +import { createPrivateKey } from 'node:crypto' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { randomBytes } from 'node:crypto' +import { canonical, signatureLines } from './verdict-signature.mjs' + +function arg(name) { + const i = process.argv.indexOf(name) + return i > 0 ? process.argv[i + 1] : undefined +} + +export function keyEnv(workstation) { + return `DSH_SECRET_VERDICT_KEY_${String(workstation ?? '').toUpperCase().replace(/[^A-Z0-9]+/g, '_')}` +} + +// The PEM (or the DER PKCS8, base64url) of one workstation's private key, from its own secret or a file. +export function readKey({ workstation, keyFile, env = process.env } = {}) { + const text = keyFile ? readFileSync(keyFile, 'utf8') : env[keyEnv(workstation)] + if (!text?.trim()) { + throw new Error(`no private key for ${workstation}: set ${keyEnv(workstation)} in the workstation's own secret card, or pass --key-file`) + } + const value = text.trim() + try { + return createPrivateKey({ key: /-----BEGIN/.test(value) ? value : Buffer.from(value, 'base64url'), format: /-----BEGIN/.test(value) ? undefined : 'der', type: /-----BEGIN/.test(value) ? undefined : 'pkcs8' }) + } catch (error) { + throw new Error(`that is not an Ed25519 private key (${error.message}); the key stays where it is, this script never prints it`) + } +} + +export function sign({ repo, pr, verdict, head, workstation, key, nonce = randomBytes(8).toString('hex') } = {}) { + const data = canonical({ repo, pr, verdict, sha: head, workstation, nonce }) + return { data, lines: signatureLines({ repo, pr, verdict, head, workstation, nonce, privateKey: key }) } +} + +function main() { + const workstation = arg('--workstation') || arg('--signed-by') + if (!workstation) throw new Error('pass --workstation ') + const repo = arg('--repo') || process.env.GH_REPO + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + const pr = arg('--pr') || process.env.PR + if (!pr) throw new Error('set PR or pass --pr N') + const verdict = arg('--verdict') + if (!verdict) throw new Error('pass --verdict PASS|INC') + const head = arg('--head') || arg('--sha') + if (!head) throw new Error('pass --head ') + const key = readKey({ workstation, keyFile: arg('--key-file') }) + const signed = sign({ repo, pr, verdict, head, workstation, key }) + console.log(`sign-verdict: ${workstation} signs ${signed.data}`) + console.log('') + console.log(signed.lines) +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + try { + main() + } catch (error) { + console.error(`sign-verdict: ${error.message}`) + process.exit(1) + } +} diff --git a/.github/scripts/verdict-signature.mjs b/.github/scripts/verdict-signature.mjs new file mode 100644 index 00000000..7980e3c9 --- /dev/null +++ b/.github/scripts/verdict-signature.mjs @@ -0,0 +1,148 @@ +// A `Verdict:` counts only when the workstation that claims to have written it signed it (#1110). +// +// Every Bot writes through the one shared `Birfy` account, so `Written-by: Reviewer` is a claim and +// nothing more: a Bot, an agent, or a person on that account can write a PASS in a Reviewer's name and +// the gate reads it as a conclusion (`#1094`, `#1095`, `#1108`, `#1115` on main). This closes that door +// without asking the owner for a second GitHub identity per workstation: each reviewing workstation +// holds one Ed25519 private key, its public key sits in `.github/agent/signers.json` in this repository, +// and the gate verifies the signature offline — an attacker with the repository, the account and every +// workflow secret still cannot sign for a workstation it has no private key for. +// +// What a signed conclusion carries, under its `Verdict:` and `Written-by:` lines: +// +// Signed-by: Reviewer +// Nonce: 3f8c1a9e5b2d4706 +// Signature: +// +// The signed string is `canonical()` below: +// +// FBV1|/#@@@@ +// +// Every field in it closes one replay: the repository and pull request number stop a report moving to +// another pull request, exactly 40 characters of sha stop it moving to another tip (a 7-character sha +// is a prefix of many heads, #1112), the verdict stops an INC being replayed as a PASS, the workstation +// stops one workstation's signature being read as another's, and the nonce stops the same signed text +// being reused as a second conclusion. A short sha in the `Verdict:` line is *not* signed: the gate +// fills in the head it is judging, so a signature can never be lifted onto a tip its author did not see. +// +// Fail closed, one cut (owner decision 2026-10-25, #1110): when `.github/agent/review-policy.json` lists +// signing workstations, a `Verdict:` whose signature does not verify is not a conclusion at all — it +// does not pass, it does not vote in the newest-wins scan, and it does not refuse an older PASS. The +// summary lists it as unverified. There is no unsigned fallback: the honest cost is that an unsigned INC +// no longer holds a merge either, which is why the keys land before the gate does (#1111). +import { readFileSync } from 'node:fs' +import { createPublicKey, sign, verify } from 'node:crypto' + +export const PREFIX = 'FBV1' +export const SIGNERS_PATH = '.github/agent/signers.json' +export const POLICY_PATH = '.github/agent/review-policy.json' +// The signature lines sit directly under the conclusion, so they are read from the header only: a body +// that quotes an older signed report further down must not lend that signature to the text above it. +const HEADER_LINES = 8 +const FULL_SHA = /^[0-9a-f]{40}$/ + +// The one string a workstation signs. `sha` must be the full head: a short sha is exactly the value a +// replay can move between tips, so a signature over one is refused rather than silently widened. +export function canonical({ repo, pr, verdict, sha, workstation, nonce }) { + const full = String(sha ?? '').toLowerCase() + if (!FULL_SHA.test(full)) throw new Error(`the signed head must be the full 40-character sha, not "${sha}"`) + const number = String(pr ?? '').trim() + if (!/^\d+$/.test(number)) throw new Error(`the signed report must name a pull request number, not "${pr}"`) + const name = String(workstation ?? '').trim() + if (!name) throw new Error('the signed report must name the workstation that signed it') + const once = String(nonce ?? '').trim() + if (!once) throw new Error('the signed report must carry a nonce') + return `${PREFIX}|${String(repo ?? '').toLowerCase()}#${number}@${String(verdict ?? '').toUpperCase()}@${full}@${name}@${once}` +} + +// `Signed-by:`, `Nonce:` and `Signature:` as the report wrote them. Nothing here decides anything: a +// missing field is reported as missing, and `verifyReport` refuses it with the reason that names it. +export function parseSignature(body) { + const out = { signedBy: null, nonce: null, signature: null } + let seen = 0 + for (const raw of String(body ?? '').split('\n')) { + const line = raw.replace(/[*`]/g, '').trim() + if (!line) continue + if (++seen > HEADER_LINES) break + let match + if (!out.signedBy && (match = /^Signed-by\s*:\s*(.+)$/i.exec(line))) { + // The first clause is the name, the way `Written-by:` reads: `Reviewer. Key B.` names Reviewer. + out.signedBy = match[1].split(/[.,(;]/)[0].trim() || null + } + if (!out.nonce && (match = /^Nonce\s*:\s*(\S+)\s*$/i.exec(line))) out.nonce = match[1] + if (!out.signature && (match = /^Signature\s*:\s*(\S+)\s*$/i.exec(line))) out.signature = match[1] + } + return out +} + +// The signers file, loaded against the policy's roster. `required` is every workstation the policy says +// may sign; `keys` maps a workstation name to its parsed public key. A roster without a usable key for +// one of them is a configuration error, not a reason to skip that workstation: the file is the gate's +// trust anchor, and a gate that cannot load it refuses every pull request. +export function loadSigners({ policy, path = SIGNERS_PATH, read = (p) => readFileSync(p, 'utf8') } = {}) { + const required = Array.isArray(policy?.signers) ? policy.signers.map((name) => String(name).trim()).filter(Boolean) : [] + if (!required.length) return { required: [], keys: new Map(), error: null } + let table + try { + table = JSON.parse(read(path)) + } catch (error) { + const why = error?.code === 'ENOENT' ? 'is missing' : `cannot be read (${error.message})` + return { + required, + keys: new Map(), + error: `${path} ${why} while ${POLICY_PATH} lists ${required.length} signing workstation(s) (${required.join(', ')}): a verdict gate that cannot load its keys refuses every pull request`, + } + } + const keys = new Map() + const problems = [] + for (const name of required) { + const value = table?.signers?.[name] + if (!value) { + problems.push(`${name} has no public key`) + continue + } + try { + keys.set(name, createPublicKey({ key: Buffer.from(String(value), 'base64url'), format: 'der', type: 'spki' })) + } catch (error) { + problems.push(`${name}'s public key is not a base64url DER SPKI (${error.message})`) + } + } + if (problems.length) { + return { required, keys, error: `${path} is not a usable signers file: ${problems.join('; ')}. Mint one Ed25519 key per workstation (#1111) and store the base64url of "openssl pkey -pubout -outform DER" there` } + } + return { required, keys, error: null } +} + +// One report's signature, checked against the head being judged. `head` — never the short sha in the +// report's own text — is what the signature has to name, so a signed report verifies at exactly one tip. +export function verifyReport({ repo, pr, head, verdict, writer, body, signers } = {}) { + const signature = parseSignature(body) + if (!signature.signedBy) return { ok: false, reason: 'no Signed-by line naming the workstation that signed it' } + if (!signature.signature) return { ok: false, reason: `Signed-by names ${signature.signedBy}, but the report carries no Signature line` } + if (!signature.nonce) return { ok: false, reason: `Signed-by names ${signature.signedBy}, but the report carries no Nonce line, so the same signature could stand in for a second conclusion` } + if (writer && signature.signedBy.toLowerCase() !== String(writer).trim().toLowerCase()) { + return { ok: false, reason: `Signed-by names ${signature.signedBy}, but Written-by names ${writer}` } + } + const key = signers?.keys?.get(signature.signedBy) + if (!key) { + const listed = signers?.required?.length ? `; the signing workstations are ${signers.required.join(', ')}` : '' + return { ok: false, reason: `Signed-by names "${signature.signedBy}", which has no public key in ${SIGNERS_PATH}${listed}` } + } + const data = canonical({ repo, pr, verdict, sha: head, workstation: signature.signedBy, nonce: signature.nonce }) + let ok = false + try { + ok = verify(null, Buffer.from(data, 'utf8'), key, Buffer.from(signature.signature, 'base64url')) + } catch { + ok = false + } + if (!ok) return { ok: false, reason: `the Signature does not verify for ${signature.signedBy} over \`${data}\`` } + return { ok: true, workstation: signature.signedBy, signed: data } +} + +// The three lines a workstation adds to its report. Kept here (one implementation) so the signer script, +// the tests and any other writer cannot disagree about the string or the line names. +export function signatureLines({ repo, pr, verdict, head, workstation, nonce, privateKey }) { + const data = canonical({ repo, pr, verdict, sha: head, workstation, nonce }) + const signature = sign(null, Buffer.from(data, 'utf8'), privateKey).toString('base64url') + return [`Signed-by: ${workstation}`, `Nonce: ${nonce}`, `Signature: ${signature}`].join('\n') +} diff --git a/.github/scripts/verdict-signature.test.mjs b/.github/scripts/verdict-signature.test.mjs new file mode 100644 index 00000000..d40fca4f --- /dev/null +++ b/.github/scripts/verdict-signature.test.mjs @@ -0,0 +1,247 @@ +// #1110: a conclusion counts only when the workstation that claims it signed it. +// +// The four impersonation bodies at the bottom are verbatim excerpts (the conclusion lines) of comments +// on main: #1094 comment 6392483805, #1095 comment 6392479580, #1108 comment 6393564871, #1115 comment +// 6393959918. Each one carries `Written-by: Reviewer` from the shared account, none carries a signature, +// and every one of them was read as a conclusion by the gate before this change. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { generateKeyPairSync } from 'node:crypto' +import { fileURLToPath } from 'node:url' +import { canonical, loadSigners, parseSignature } from './verdict-signature.mjs' +import { decide, loadPolicy, render } from './review-gate.mjs' +import { keyEnv, readKey, sign as signVerdict } from './sign-verdict.mjs' + +const HEAD = '2a09e5fe794c0d6840a263bdeef25dc0775aab7b' +const OLD = 'd797cbe4d3e7c3653f7ded379eefa85209342703' +const REPO = 'ThinkFlowLab/FlyBot' +const PR = 1132 + +function mint() { + const { publicKey, privateKey } = generateKeyPairSync('ed25519') + return { privateKey, der: publicKey.export({ format: 'der', type: 'spki' }).toString('base64url') } +} +const REVIEWER = mint() +const REVIEWER2 = mint() + +// A signers file that agrees with the policy, read without touching the filesystem. +const signersFile = (signers) => () => JSON.stringify({ version: 1, signers }) +const loaded = (table = { Reviewer: REVIEWER.der, Reviewer2: REVIEWER2.der }, policy = { signers: ['Reviewer', 'Reviewer2'] }) => + loadSigners({ policy, path: 'signers.json', read: signersFile(table) }) + +const policy = { reviewers: ['Birfy'], selfReview: ['Birfy'], writers: ['Reviewer', 'Reviewer2', 'Birfy'], signers: ['Reviewer', 'Reviewer2'] } + +const lines = ({ tip = HEAD, verdict = 'PASS', ran = [' - npm run verify → pass'], writer = 'Reviewer' } = {}) => + [`Verdict: ${verdict} at ${tip}`, `Written-by: ${writer}`, 'Ran:', ...ran, 'Not run: - smoke — host code unchanged'] + +// A report signed the way `sign-verdict.mjs` signs one: the canonical string names the full head, the +// workstation and the nonce; the `Verdict:` line may carry a short sha, which is not signed. +function signed({ tip = HEAD, verdict = 'PASS', workstation = 'Reviewer', key = REVIEWER.privateKey, nonce = 'n1', head = HEAD, ran, pr = PR, repo = REPO, attest = true } = {}) { + const signature = signVerdict({ repo, pr, verdict, head, workstation, key, nonce }).lines + // The header is read in order, and both #1110's signature lines and #1148's attestation have to fit in + // it: Verdict, Written-by, Attestation, then the three signature lines, then the evidence. Every pull + // request here is opened by the shared account, so the author's own login needs both; the attestation + // case below drops one to prove the two refusals stayed separate. + return [ + `Verdict: ${verdict} at ${tip}`, + 'Written-by: Reviewer', + ...(attest ? [`Attestation: ${head} role=${workstation}`] : []), + ...signature.split('\n'), + 'Ran:', + ...(ran ?? [' - npm run verify → pass']), + 'Not run: - smoke — host code unchanged', + ].join('\n') +} + +const comment = (body, at = '2026-10-25T08:00:00Z', login = 'Birfy') => ({ login, type: 'User', body, at, kind: 'comment', association: 'COLLABORATOR' }) +const judge = (reports, { signers = loaded(), policy: p = policy, head = HEAD, pr = PR } = {}) => decide({ head, author: 'Birfy', reports, policy: p, repo: REPO, pr, signers, owners: ['Dev3'] }) + +test('a signed PASS by a listed reviewer counts, and the signature covers the full head', () => { + const r = judge([comment(signed())]) + assert.equal(r.ok, true) + assert.equal(r.by, 'Birfy') + assert.deepEqual(r.unverified, []) +}) + +test('#1148 + #1110: a correctly signed report from the author login still needs the attestation', () => { + // The signature verifies, so the report is not unverified: the refusal is the attestation one, which is + // the proof that the two rules were kept apart instead of one replacing the other. + const r = judge([comment(signed({ attest: false }))]) + assert.equal(r.ok, false) + assert.match(r.reason, /not an attestation/) + assert.deepEqual(r.unverified, []) +}) + +test('a report with no Signed-by line is not a conclusion: it neither passes nor joins the newest-wins scan', () => { + const r = judge([comment(lines().join('\n'))]) + assert.equal(r.ok, false) + assert.match(r.reason, /no Verifier report names the tip/) + assert.equal(r.unverified.length, 1) + assert.match(r.unverified[0].why, /no Signed-by line/) +}) + +test('a PASS with a Signed-by line but no Signature line is unverified', () => { + const body = [...lines(), 'Signed-by: Reviewer', 'Nonce: n1'].join('\n') + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /no Signature line/) +}) + +test('a signature made with another workstation\'s private key does not count', () => { + // Claims Reviewer, signed with Reviewer2's key: the workstation is inside the signed string, so the + // Reviewer public key cannot verify it. + const r = judge([comment(signed({ workstation: 'Reviewer', key: REVIEWER2.privateKey }))]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /does not verify for Reviewer/) +}) + +test('Signed-by must name the same workstation as Written-by', () => { + const body = [...lines(), ...signVerdict({ repo: REPO, pr: PR, verdict: 'PASS', head: HEAD, workstation: 'Reviewer2', key: REVIEWER2.privateKey, nonce: 'n2' }).lines.split('\n')].join('\n') + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /Signed-by names Reviewer2, but Written-by names Reviewer/) +}) + +test('Signed-by naming a workstation with no public key is unverified, and says who may sign', () => { + const body = [...lines({ writer: 'Reviewer3' }), 'Signed-by: Reviewer3', 'Nonce: n3', 'Signature: AAAA'].join('\n') + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /has no public key in \.github\/agent\/signers\.json/) + assert.match(r.unverified[0].why, /Reviewer, Reviewer2/) +}) + +test('replay onto another tip: a signature over the previous head does not verify at the new one', () => { + const body = [...lines({ tip: HEAD }), ...signVerdict({ repo: REPO, pr: PR, verdict: 'PASS', head: OLD, workstation: 'Reviewer', key: REVIEWER.privateKey, nonce: 'n4' }).lines.split('\n')].join('\n') + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /does not verify/) +}) + +test('replay with another nonce: editing the Nonce line breaks the signature', () => { + const body = signed({ nonce: 'n5' }).replace('Nonce: n5', 'Nonce: n6') + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /does not verify/) +}) + +test('an INC is governed the same way: unsigned, it no longer holds an earlier signed PASS', () => { + const r = judge([ + comment(signed({ nonce: 'p1', at: 1 }), '2026-10-25T08:00:00Z'), + comment(lines({ verdict: 'INC' }).join('\n'), '2026-10-25T08:05:00Z'), + ]) + assert.equal(r.ok, true, 'the unsigned INC is not a conclusion, so it cannot veto') + assert.equal(r.unverified.length, 1) +}) + +test('a signed INC still refuses, and it is the newest word that decides', () => { + const r = judge([ + comment(signed({ nonce: 'p2' }), '2026-10-25T08:00:00Z'), + comment(signed({ verdict: 'INC', nonce: 'p3' }), '2026-10-25T08:05:00Z'), + ]) + assert.equal(r.ok, false) + assert.equal(r.verdict, 'INC') + assert.match(r.reason, /says INC at this tip/) +}) + +test('configuration error: the policy lists workstations and the signers file is missing', () => { + const signers = loadSigners({ policy, path: '.github/agent/signers.json', read: () => { const e = new Error('no such file'); e.code = 'ENOENT'; throw e } }) + const r = judge([comment(signed())], { signers }) + assert.equal(r.ok, false) + assert.match(r.reason, /is missing while \.github\/agent\/review-policy\.json lists 2 signing workstation/) +}) + +test('configuration error: a listed workstation has no public key', () => { + const signers = loadSigners({ policy, path: 'signers.json', read: signersFile({ Reviewer: REVIEWER.der }) }) + assert.match(signers.error, /Reviewer2 has no public key/) + const r = judge([comment(signed())], { signers }) + assert.equal(r.ok, false) + assert.match(r.reason, /not a usable signers file/) +}) + +test('configuration error: a policy with a roster is never read without its keys', () => { + const r = decide({ head: HEAD, author: 'Birfy', reports: [comment(signed())], policy, repo: REPO, pr: PR }) + assert.equal(r.ok, false) + assert.match(r.reason, /was not loaded/) +}) + +test('a signature is not a permission: a valid signature from a login outside reviewers still refuses', () => { + const r = judge([comment(signed({ nonce: 'n7' }), '2026-10-25T08:00:00Z', 'someone-else')]) + assert.equal(r.ok, false) + assert.match(r.reason, /not a reviewer listed/) +}) + +test('a bot account writes no conclusion even with a valid signature (#943 survives #1110)', () => { + const body = signed({ nonce: 'n8' }) + const r = judge([{ login: 'Birfy', type: 'Bot', body, at: '2026-10-25T08:00:00Z', kind: 'comment', association: 'COLLABORATOR' }]) + assert.equal(r.ok, false) + assert.match(r.reason, /written by a bot account/) +}) + +test('the signature lines are read from the header only: a quoted signed block below does not sign the text above', () => { + const quoted = signed({ nonce: 'n9' }).split('\n').slice(-3) + const body = [...lines(), 'Signed-by: nobody', '', 'Earlier report quoted for context:', '', 'Prose.', '', 'Numbers.', '', ...quoted].join('\n') + const signature = parseSignature(body) + assert.equal(signature.signedBy, 'nobody') + assert.equal(signature.signature, null) + const r = judge([comment(body)]) + assert.equal(r.ok, false) + assert.match(r.unverified[0].why, /no Signature line/) +}) + +test('the summary marks an unverified report as unverified', () => { + const r = judge([comment(lines().join('\n'))]) + const text = render(r, { pr: PR, head: HEAD, author: 'Birfy' }) + assert.match(text, /Unverified — not read as conclusions:/) + assert.match(text, /- unverified: Birfy \(comment\): PASS at 2a09e5fe/) +}) + +test('the canonical string names every field that stops a replay', () => { + // The repository is lower-cased into the signed string, so a report cannot move by spelling. + assert.equal(canonical({ repo: 'ThinkFlowLab/FlyBot', pr: 1132, verdict: 'pass', sha: HEAD, workstation: 'Reviewer', nonce: 'n' }), + `FBV1|thinkflowlab/flybot#1132@PASS@${HEAD}@Reviewer@n`) + assert.throws(() => canonical({ repo: REPO, pr: PR, verdict: 'PASS', sha: HEAD.slice(0, 8), workstation: 'Reviewer', nonce: 'n' }), /full 40-character sha/) + assert.throws(() => canonical({ repo: REPO, pr: PR, verdict: 'PASS', sha: HEAD, workstation: 'Reviewer', nonce: '' }), /nonce/) +}) + +test('the signer round-trips: what sign-verdict.mjs prints is what the gate verifies', () => { + const printed = signVerdict({ repo: REPO, pr: PR, verdict: 'PASS', head: HEAD, workstation: 'Reviewer', key: REVIEWER.privateKey, nonce: 'abc123' }) + assert.equal(printed.data, `FBV1|thinkflowlab/flybot#1132@PASS@${HEAD}@Reviewer@abc123`) + // Where the workstation writes them: under the conclusion, inside the header, with the attestation the + // shared author login needs as well (#1148). + const body = [`Verdict: PASS at ${HEAD}`, 'Written-by: Reviewer', `Attestation: ${HEAD} role=Reviewer`, ...printed.lines.split('\n'), 'Ran:', ' - npm run verify → pass', 'Not run: - smoke — host code unchanged'].join('\n') + const r = judge([comment(body)]) + assert.equal(r.ok, true) +}) + +test('the signer refuses a workstation with no key and never echoes a key', () => { + assert.equal(keyEnv('Reviewer2'), 'DSH_SECRET_VERDICT_KEY_REVIEWER2') + assert.throws(() => readKey({ workstation: 'Reviewer', env: {} }), /set DSH_SECRET_VERDICT_KEY_REVIEWER in the workstation's own secret card/) +}) + +// The shipped pair: once `review-policy.json` lists signing workstations, the file in the repository must +// carry a usable key for every one of them, or every pull request is refused. This test is the reason a +// forgotten key is caught here rather than by a wedged queue. +test('the repository\'s own policy and signers file agree', () => { + const shipped = loadPolicy(fileURLToPath(new URL('../agent/review-policy.json', import.meta.url))) + const signers = loadSigners({ policy: shipped, path: fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) + assert.equal(signers.error, null) + assert.deepEqual([...signers.keys.keys()], shipped.signers ?? []) +}) + +// The four instances from #1110, verbatim conclusion lines. Signed by nobody, so unverified — this is the +// shape the gate used to read as a PASS from the Reviewer. +const INSTANCES = [ + { pr: 1094, head: '2868d5761613f1304bfb4826e082f37fb357607f', body: 'Verdict: PASS at 2868d5761613f1304bfb4826e082f37fb357607f\nWritten-by: Reviewer\n\nFixes #1088. Rebased onto main with #1078.\n\nRan:\n- `TZ=UTC npm run verify` → pass at tip\n- CI tests-ok → SUCCESS' }, + { pr: 1095, head: '370019d466de29ebbcbcdb644c82b74e88266e06', body: 'Verdict: PASS at 370019d466de29ebbcbcdb644c82b74e88266e06\nWritten-by: Reviewer\n\nFixes #1087. Rebased tip: friend DM shows Waiting for their approval.\n\nRan:\n- `TZ=UTC npm run verify` → pass at tip\n- CI tests-ok → SUCCESS' }, + { pr: 1108, head: '7799db990423ab2784253e3ae643c5c3d92cba9a', body: 'Verdict: PASS at 7799db990423ab2784253e3ae643c5c3d92cba9a\nWritten-by: Reviewer\n\nFixes #1102. Rebased onto main 506e656 after #1105.\n\nRan:\n- `TZ=UTC npm run verify` → pass (core 1445, suites fail 0)\n- Fail-without-change: focused tests failed 4/10 without the change, 10/10 with it' }, + { pr: 1115, head: '83e41117e7139f5bcf585131ec03847005ffe31a', body: 'Verdict: PASS at 83e41117e7139f5bcf585131ec03847005ffe31a\nWritten-by: Reviewer\n\nRan:\n - git fetch origin main && git rev-parse HEAD && git diff origin/main...HEAD --stat → tip 83e41117e7139f5bcf585131ec03847005ffe31a\n - GH_REPO=ThinkFlowLab/FlyBot node .github/scripts/risk-gate.mjs --pr 1115 → risk-tier: T3' }, +] + +for (const instance of INSTANCES) { + test(`#${instance.pr}'s impersonated PASS reads as unverified, never as a conclusion`, () => { + const r = decide({ head: instance.head, author: 'Birfy', reports: [comment(instance.body)], policy, repo: REPO, pr: instance.pr, signers: loaded() }) + assert.equal(r.ok, false) + assert.equal(r.unverified.length, 1) + assert.match(r.unverified[0].why, /no Signed-by line/) + }) +} diff --git a/AGENTS.md b/AGENTS.md index b6dac434..c90522c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,7 +149,7 @@ The gate refuses what it can check, and fails closed. [`review-gate.mjs`](.githu The gate reads a conclusion only from someone tied to this repository (#676): `author_association` `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`), the pull request's own author when that login is in `selfReview` (#928), or a login the permission API reports as admin. The permission API is for an admin GitHub did not tie to the repository — a `CONTRIBUTOR` stamp on someone else's pull request — not for the own-author carve-out. That own-pull-request PASS is already `selfOnly` (#926, #928); the lookup does not recover it. A `CONTRIBUTOR` who is none of those is not a conclusion, so a passer-by's `Verdict:` can neither pass nor hold a merge. A missing association is not assumed, and a failed permission lookup is not admin. The lookup uses the token the job already has, and only for a Tip+Verdict conclusion by a login in `reviewers` that is not already `OWNER`, `MEMBER` or `COLLABORATOR`. A `selfReview` login is skipped only on that author's own report, because the shipped policy lists every reviewer in `selfReview` and skipping the whole list would never call. Admin does not skip the reviewers list or the own-author rule. The two qualifications do not replace each other: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else’s, while admin is read only through the permission API and only for a login already in `reviewers`. -What this buys is presence and a name from a closed list, not truth: with one shared identity the line is a self-report that can be left out (refused) and can name the wrong Bot (undetectable here). The write layer knowing the writer is the DSH-side half, #633. +What this buys is presence and a name from a closed list, not truth: with one shared identity the line is a self-report that can be left out (refused) and can name the wrong Bot. **Since #1110 the signature is what closes that gap:** a conclusion also carries `Signed-by:` / `Nonce:` / `Signature:`, one Ed25519 key per reviewing workstation, the public keys in [`.github/agent/signers.json`](.github/agent/signers.json), verified offline by the gate over `FBV1|#@@@@`. A `Verdict:` whose signature does not verify is **not a conclusion** — it cannot pass, it does not vote in the newest-wins scan, and it does not hold a merge; the summary lists it as `unverified:`. There is no unsigned fallback, and a policy roster without a usable key for every workstation is a configuration error that refuses everything. A workstation signs with its own key, which never leaves its secret card: `node .github/scripts/sign-verdict.mjs --pr --verdict PASS --workstation --head `; the rules are in [docs/autonomous-dev.md → Signed conclusions](docs/autonomous-dev.md#signed-conclusions-1110). The write layer knowing the writer is the DSH-side half, #633. ### Gates @@ -181,7 +181,7 @@ Four rules decide what a queue entry must pass. The mechanical judge runs where 1. **`hold` blocks mechanically.** A pull request labelled `hold` fails `merge-gate` on its queue entry. The label is only a marker; the job is the gate, and it is a required check of the `main merge queue` ruleset (#639, set 2026-10-22), so the refusal holds the entry. 2. **Labelling a pull request `hold` means adding a record.** Add the label **and** post a new comment saying **Record only — do not merge / 仅记录、禁止入队**. The pair is what holds it. Add that comment; never edit or delete an earlier PASS report to change what it said — the history is the record, and rewriting it is the thing that reads as an incident afterwards. 3. **An authorisation is not a queue permit.** The owner or the Chief saying "let it through", "claim it" or "handle it along with the rest" licenses no specific pull request for the queue. Entering the queue is an explicit action on that one pull request, after its own gate passed, while it carries no `hold`. #566 is the case this exists for: "handle it along with the rest" was read as a permit, the tip was PASSed at 09:48:18Z, enqueued at 09:48:28Z and merged at 09:50:40Z as `abaf938b`. -4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged — the pull request's own head, on a queue entry too (the `` in `gh-readonly-queue/main/pr--` is the entry's base, not its head; #654) — and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too, and so does a report with no `Written-by:` line or one that names a writer the policy does not list (#627). Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written-by: Reviewer`, and a relay adds `Posted-by: Chief (proxy…)`; the older prose `Written by: …` reads the same way) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. +4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged — the pull request's own head, on a queue entry too (the `` in `gh-readonly-queue/main/pr--` is the entry's base, not its head; #654) — and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too, and so does a report with no `Written-by:` line or one that names a writer the policy does not list (#627). A `Verdict:` whose signature does not verify is not a conclusion at all (#1110): it is skipped before that scan rather than counted as a refusal, so an unsigned INC no longer holds a merge — the reason the keys land before the gate does. Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written-by: Reviewer`, and a relay adds `Posted-by: Chief (proxy…)`; the older prose `Written by: …` reads the same way) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. A review's `state` is not read at all: approvals are not a gate any more (user decision 2026-10-22), and the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC — the INC on #610 was a comment rather than a review. diff --git a/changelog.d/1110-signed-verdicts.changed.md b/changelog.d/1110-signed-verdicts.changed.md new file mode 100644 index 00000000..2104a7b3 --- /dev/null +++ b/changelog.d/1110-signed-verdicts.changed.md @@ -0,0 +1 @@ +A `Verdict:` now counts only when the workstation that claims to have written it signed it: each reviewing workstation holds one Ed25519 private key, its public key sits in `.github/agent/signers.json`, and the gates verify `Signed-by:` / `Nonce:` / `Signature:` offline over the pull request, the full tip SHA, the verdict and the workstation — so a PASS written in a Reviewer's name on the shared account is no longer a conclusion, it is listed as unverified ([#1110](https://github.com/ThinkFlowLab/FlyBot/issues/1110)). diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index ef26b8f8..1b1ece6f 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -162,17 +162,69 @@ What counts as a reviewed tip, all of it: list: every Bot writes as the shared `Birfy` account, so a list containing it would void the whole pipeline. `cursor[bot]`'s report at #940's tip is the case (`Correction from Reviewer`, [comment `6381646389`](https://github.com/ThinkFlowLab/FlyBot/pull/940#issuecomment-6381646389)). +7. **The report is signed by the workstation it names** (#1110): `Signed-by:` + `Nonce:` + `Signature:`, + verified against `.github/agent/signers.json` for the full head being judged. A `Verdict:` that does not + verify is not a conclusion and stays out of the newest-wins scan — see [Signed conclusions](#signed-conclusions-1110) + below for the canonical string, the configuration errors and the cost. + +#### Signed conclusions (#1110) + +One shared GitHub identity makes `Written-by:` a claim, so since #1110 a conclusion also has to be +**signed by the workstation it names**. Each reviewing workstation holds one +Ed25519 private key in its own secret (`DSH_SECRET_VERDICT_KEY_`); the public keys are in +[`.github/agent/signers.json`](../.github/agent/signers.json), and the roster of workstations that must +have one is the `signers` list in [`review-policy.json`](../.github/agent/review-policy.json). A report +adds three lines under its attribution: + +```text +Signed-by: Reviewer +Nonce: 3f8c1a9e5b2d4706 +Signature: +``` + +The signed string is `FBV1|/#@@@@` +([`verdict-signature.mjs`](../.github/scripts/verdict-signature.mjs) `canonical`). Every field closes one +replay: the repository and pull request number stop a report moving to another pull request, exactly 40 +characters of sha stop it moving to another tip (a 7-character sha is a prefix of many heads, #1112), the +verdict stops an INC being replayed as a PASS, the workstation stops one workstation's signature reading as +another's, and the nonce stops the same signed text standing as a second conclusion. A short sha on the +`Verdict:` line is **not** signed: the gate fills in the head it is judging, so a signature can never be +lifted onto a tip its author did not see. + +It is fail-closed and one cut. When the policy lists signing workstations, a `Verdict:` whose signature +does not verify **is not a conclusion at all**: it does not pass, it does not join the newest-wins scan, and +it does not refuse an older PASS — the summary lists it as `unverified:`. There is no unsigned fallback, so +the honest cost is that an unsigned INC no longer holds a merge either; the keys therefore land before the +gate does ([#1111](https://github.com/ThinkFlowLab/FlyBot/issues/1111)). A roster with a missing or unreadable +signers file, or a workstation whose key is missing or does not parse, is a **configuration error** and +refuses every pull request: half of that pair is never read as "unsigned is fine". The signature is not a +permission — the association, `writers`, `reviewers` and bot-account checks run exactly as before, and the +conclusion never reached the reviewed-tip job at all: the signed lines sit in the header, where the author +attestation (#644) also lives, so a body can carry both and the two refusals stay separate. + +A workstation signs with its own key, which never leaves its secret card: + +```sh +node .github/scripts/sign-verdict.mjs --repo ThinkFlowLab/FlyBot --pr 1132 --verdict PASS \ + --workstation Reviewer --head +``` + **Attribution (#627).** Every state-changing write says who wrote it: `Written-by: ` is its first line, and a conclusion puts it on the second, under the fixed verdict line. `decide` refuses a report with no such line and, separately, one whose name is not in the policy's `writers`; Triage reports a write without one instead of editing it. The rule itself lives in AGENTS.md — one shared GitHub identity -makes the line a self-report, so the gate guarantees presence, not truth (#633). +makes the line a self-report, so the gate guarantees presence, not truth (#633). Since #1110 the signature +is what closes that gap: the line still claims, the key proves. | File | Purpose | | --- | --- | -| `.github/agent/review-policy.json` | the logins whose PASS counts, `selfReview`, the authors who may PASS their own PR, and `writers`, the Bot names a `Written-by:` line may carry (#627) — a roster, not a permission: `reviewers`/`selfReview` are the permission lists (#962); editing it is T3 | +| `.github/agent/review-policy.json` | the logins whose PASS counts, `selfReview`, the authors who may PASS their own PR, and `writers`, the Bot names a `Written-by:` line may carry (#627) — a roster, not a permission: `reviewers`/`selfReview` are the permission lists (#962); editing it is T3. `signers` lists the workstations that must have a public key (#1110) | +| `.github/agent/signers.json` | one Ed25519 public key per signing workstation, base64url DER SPKI from `openssl pkey -pubout -outform DER` (#1110); private keys never enter the repository | | `.github/scripts/review-gate.mjs` | the rule above, for one pull request at a time | +| `.github/scripts/verdict-signature.mjs` | the canonical string, the signature lines, `loadSigners` and `verifyReport` — one implementation for the gates, the signer and the tests (#1110) | +| `.github/scripts/sign-verdict.mjs` | what a workstation runs to sign its own conclusion; reads its own secret, never prints a key (#1110) | | `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author, and only with an `Attestation:` line when that login is the author, #644), unlisted login, FAIL after PASS, missing `Ran:` (a missing `Not run:` no longer refuses), a missing `Written-by:` line and one naming an unlisted writer (#627), a `selfReview` login counting only on its own pull request (#928), a `selfReview` admin on someone else's pull request (permission API, `CONTRIBUTOR` stamp), a `CONTRIBUTOR` who is neither still refused, and the workflow's shape | +| `.github/scripts/verdict-signature.test.mjs` | a real signature counting, a missing `Signed-by:`/`Signature:`/`Nonce:`, another workstation's private key, a replay onto another tip and one with another nonce, a signed and an unsigned INC, the configuration errors, a bot account with a valid signature (#943 survives), the author attestation and the signature staying separate refusals (#1148 + #1110), and the four impersonated PASSes from #1094/#1095/#1108/#1115 read as unverified (#1110) | | `.github/workflows/review-gate.yml` | reports it in the job summary; `continue-on-error`, never required | Run it by hand (read-only API calls with your `gh` login):