From 047a916d102d6462c64ce9cf786f74a14e3331da Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 25 Oct 2026 10:03:23 +0000 Subject: [PATCH 1/2] Require a tip attestation for an author-login review A Verdict comment from the pull request author's GitHub login is not an independent review. decide() accepts that login only when the header carries Attestation: role=, the role matches Written-by, and the role is not an Owner. Related #644 Co-authored-by: Hongsheng Liu --- .claude/skills/review-pr/SKILL.md | 9 +- .github/scripts/enqueue-guard.mjs | 6 +- .github/scripts/enqueue-guard.test.mjs | 37 ++++-- .github/scripts/merge-gate.mjs | 6 +- .github/scripts/merge-gate.test.mjs | 24 +++- .github/scripts/review-gate.mjs | 83 ++++++++++++- .github/scripts/review-gate.test.mjs | 138 +++++++++++++++++++-- AGENTS.md | 3 +- changelog.d/644-tip-attestation.changed.md | 1 + docs/autonomous-dev.md | 9 +- docs/code-standards.md | 1 + 11 files changed, 277 insertions(+), 40 deletions(-) create mode 100644 changelog.d/644-tip-attestation.changed.md diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 9e4d5465e..18ee6693a 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -37,9 +37,10 @@ Owned paths and what each subsystem owns live in [docs/subsystems/](../../../doc Always include: ```text -Verdict: PASS | FAIL | INC at +Verdict: PASS | FAIL | INC at Written-by: Reviewer Posted-by: +Attestation: role=Reviewer Ran: - → pass|fail @@ -49,10 +50,12 @@ Not run: ``` The first line is the whole conclusion (#637): nothing may come before it, because the gates read the -first line and treat any other shape as no conclusion. The `Written-by:` line is the attribution every write carries (#627): it names a Bot from the policy's +first line and treat any other shape as no conclusion. The sha is the whole 40 characters (#1112). The `Written-by:` line is the attribution every write carries (#627): it names a Bot from the policy's `writers`, the gates refuse a report without it, and it is a self-report, not proof — see [AGENTS.md](../../../AGENTS.md#every-state-changing-write-says-who-wrote-it-627), which is the single source. +`Attestation:` is required when the GitHub login posting the conclusion is the pull request author (#644, first slice). It stays in the header, before the blank line above `Ran:`. `role` is the `Written-by` name and is not an `Owner:` of the pull request. A conclusion posted by a different GitHub login omits the line. The gate parses the line; it does not check a signature. + What the gates read as evidence (`review-gate.mjs` `parseReport`; #660–#663 refused at the queue for the first point): @@ -127,7 +130,7 @@ Do not fill contributor checkboxes or flip draft status unless asked. Post the report as a **pull request comment** (or the review body) only when a human asks for it, and only from the reviewer's own login: `.github/scripts/review-gate.mjs` reads the newest Verifier block that names the head, from a login in `.github/agent/review-policy.json`. The pull request's own -author counts only when listed in the policy's `selfReview` (the owner's Bot/agent, `Birfy`, may PASS its own PR). Keep the block intact — the `Verdict: … at ` first line, the attribution line, `Ran:`, and `Not run:` when you skipped something — because a +author counts only when listed in the policy's `selfReview` (the owner's Bot/agent, `Birfy`, may PASS its own PR) and, when that login is the author, the header also carries `Attestation: role=` (#644). Keep the block intact — the `Verdict: … at ` first line, the attribution line, the attestation line when the login is the author, `Ran:`, and `Not run:` when you skipped something — because a missing `Ran:` command is reported as incomplete evidence (`Not run:` is not checked, owner decision 2026-10-22 09:57 Asia/Shanghai), and a PASS at an older sha stops counting the moment the tip moves. Posting it is not merge authority: see AGENTS.md step 6. T0/T1 need no PASS; T2/T3 need a PASS at the tip, and the agent that gated the PR then enqueues it itself (owner decision 2026-10-21). diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index eeae7f9c9..db65a92f0 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -11,7 +11,9 @@ // // So this is the step immediately before that call. It refuses — exit 1, the reason printed — when // the pull request carries `hold`, when the newest conclusion at its head is not a PASS, or when that -// conclusion is itself marked "record only — do not merge". It also refuses a pull request that names +// conclusion is itself marked "record only — do not merge". A PASS whose GitHub login is the pull +// request author counts only with an `Attestation:` line (#644); `judge` reads that from `decide`. +// It also refuses a pull request that names // no issue in its body or carries no `agent-pr` label (#718; a mechanical release PR and a fork are // exempt), so // "one issue, one pull request" is checked rather than remembered. It reuses `judge({ mode: 'queue' })`, so @@ -80,7 +82,7 @@ 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' }) + const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, 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/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 79294edc5..d383e01eb 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -28,8 +28,16 @@ const NO_SHARED_LOGIN = policyFile({ reviewers: ['reviewer-agent'] }) // The fixed format (#637): the first line is the conclusion, the evidence below it. `Ran:` is what // makes a PASS evidence rather than a claim (review-gate's rule, reused here). -const report = ({ tip = HEAD, verdict = 'PASS', extra = [] } = {}) => - [`Verdict: ${verdict} at ${tip}`, 'Written-by: reviewer-agent', '', 'Ran: npm run verify → pass', 'Not run: smoke', ...extra].join('\n') +const report = ({ tip = HEAD, verdict = 'PASS', extra = [], attest = null } = {}) => + [ + `Verdict: ${verdict} at ${tip}`, + 'Written-by: reviewer-agent', + ...(attest ? [`Attestation: ${tip} role=${attest}`] : []), + '', + 'Ran: npm run verify → pass', + 'Not run: smoke', + ...extra, + ].join('\n') // #676: only a comment GitHub ties to this repository can be a conclusion, so the fixture carries the // association the API sends for the shared `Birfy` account. @@ -171,12 +179,12 @@ test('#602: hold refuses a T0/T1 pull request too', () => { assert.equal(r.held, true) }) -test('#602: the self-gate case (author Birfy, PASS by Birfy) is allowed', () => { +test('#602: the self-gate case (author Birfy, PASS by Birfy) is allowed when the comment attests a role that is not the Owner', () => { const r = main({ gh: fakeGh({ files: [HOST_FILE], author: 'Birfy', - comments: [{ user: { login: 'Birfy' }, body: report(), created_at: '2026-10-21T10:31:00Z', author_association: 'OWNER' }], + comments: [{ user: { login: 'Birfy' }, body: report({ attest: 'reviewer-agent' }), created_at: '2026-10-21T10:31:00Z', author_association: 'OWNER' }], }), env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, repo: 'ThinkFlowLab/FlyBot', @@ -185,6 +193,21 @@ test('#602: the self-gate case (author Birfy, PASS by Birfy) is allowed', () => assert.equal(r.ok, true, r.reasons.join('; ')) }) +test('#644: the self-gate case with only the author\'s Verdict comment is refused', () => { + const r = main({ + gh: fakeGh({ + files: [HOST_FILE], + author: 'Birfy', + comments: [{ user: { login: 'Birfy' }, body: report(), created_at: '2026-10-21T10:31:00Z', author_association: 'OWNER' }], + }), + env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, + repo: 'ThinkFlowLab/FlyBot', + pr: 566, + }) + assert.equal(r.ok, false) + assert.match(r.reasons.join('; '), /not an attestation/) +}) + // #718: a pull request with no issue behind it, or with no `agent-pr` label, is refused before the // queue: #711/#712/#713 arrived with neither and the guard said nothing, because it only read `hold`, // the conclusions and the diff. The release PR is the one exception — it has neither by design. @@ -274,8 +297,8 @@ test('#718: a fork stays exempt under any --repo spelling', () => { // The guard is only in the path if the instructions that tell an agent to enqueue name it; otherwise // the next agent runs `gh pr merge` from memory and #602 is back. -const contributor = (login, at = '2026-10-24T03:42:31Z') => - ({ user: { login }, body: report(), submitted_at: at, author_association: 'CONTRIBUTOR' }) +const contributor = (login, at = '2026-10-24T03:42:31Z', body = report()) => + ({ user: { login }, body, submitted_at: at, author_association: 'CONTRIBUTOR' }) test('#925: a MEMBER PASS is accepted and does not ask the permission API', () => { const gh = fakeGh({ files: [HOST_FILE], comments: [comment()] }) @@ -302,7 +325,7 @@ test('#925: a selfReview login stamped CONTRIBUTOR is accepted on its own pull r const gh = fakeGh({ files: [HOST_FILE], author: 'Birfy', - reviews: [contributor('Birfy')], + reviews: [contributor('Birfy', '2026-10-24T03:42:31Z', report({ attest: 'reviewer-agent' }))], permission: 'throw', }) const r = main({ gh, env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, repo: 'ThinkFlowLab/FlyBot', pr: 566 }) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 35416f134..bfff907ae 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -100,7 +100,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 } = {}) { +export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo, body, owners } = {}) { const reasons = [] const target = String(head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -120,7 +120,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, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }) }) + : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }) }) 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 @@ -217,7 +217,7 @@ 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 }) + const result = judge({ labels, files, head, author, reports, policy, gh, repo, 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 — and `judge` reads neither the body nor the labels. The fallback runs the guard's own // `guard()` over the same pull request, so a body with no `Owner:`/`Reviewer:`, no `Fixes #`, or no diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 2d9f5eefc..a62cda78c 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -384,13 +384,13 @@ test('#654: the workflow hands the script the merge_group base and entry shas', const REAL = JSON.parse(readFileSync(new URL('./fixtures/review-reports-660-663.json', import.meta.url), 'utf8')).reports const REAL_POLICY = fileURLToPath(new URL('../agent/review-policy.json', import.meta.url)) -const queueReal = (fx, body = fx.comment.body) => main({ +const queueReal = (fx, body = fx.comment.body, pullBody) => main({ gh: (...args) => { const path = args[args.length - 1] if (path.includes('/files')) return fx.files.map((filename) => JSON.stringify({ filename })).join('\n') if (path.includes('/comments')) return JSON.stringify({ user: { login: fx.comment.login }, body, created_at: fx.comment.created_at, author_association: 'OWNER' }) if (path.includes('/reviews')) return '' - if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: fx.head, repo: { full_name: 'ThinkFlowLab/FlyBot' } }, user: { login: fx.author }, labels: [{ name: 'agent-pr' }], body: ['Owner: Reviewer', 'Reviewer: Birfy', 'Fixes #' + fx.pr, ''].join('\n') }) + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: fx.head, repo: { full_name: 'ThinkFlowLab/FlyBot' } }, user: { login: fx.author }, labels: [{ name: 'agent-pr' }], body: pullBody ?? ['Owner: Reviewer', 'Reviewer: Birfy', 'Fixes #' + fx.pr, ''].join('\n') }) throw new Error('fake gh: unexpected ' + path) }, env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: REAL_POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', HEAD_REF: `refs/heads/gh-readonly-queue/main/pr-${fx.pr}-c74e1207${'0'.repeat(32)}`, MODE: 'queue' }, @@ -416,14 +416,28 @@ test('#660: the real PASS, which names what it did not run, passes the queue ent // Owner decision 2026-10-22 09:57 Asia/Shanghai (#667): "Not run:" is no longer checked, so the three // reports without one pass as they were written. -test('#660-#663: all four real PASS reports pass the queue entry end to end, as written', () => { +test('#660-#663: a PASS from another GitHub login still passes; the author\'s own comment does not (#644)', () => { for (const fx of REAL) { const result = queueReal(fx) - assert.equal(result.ok, true, `#${fx.pr}: ${result.reasons.join('; ')}`) - assert.equal(result.verdict.by, 'Birfy') assert.equal(result.head, fx.head) + if (fx.author === fx.comment.login) { + assert.equal(result.ok, false, `#${fx.pr} is the author's own comment and carries no attestation`) + assert.match(result.reasons.join('; '), /not an attestation/) + } else { + assert.equal(result.ok, true, `#${fx.pr}: ${result.reasons.join('; ')}`) + assert.equal(result.verdict.by, 'Birfy') + } } assert.equal(REAL.filter((fx) => !/^Not run\b/m.test(fx.comment.body)).length, 3, '#661-#663 carry no Not run line') + assert.equal(REAL.filter((fx) => fx.author === fx.comment.login).map((fx) => fx.pr).join(','), '663') +}) + +test('#644: #663 passes once the author comment attests a role that is not the Owner', () => { + const fx = REAL.find((f) => f.pr === 663) + const body = fx.comment.body.replace('Written-by: Reviewer\n', `Written-by: Reviewer\nAttestation: ${fx.head} role=Reviewer\n`) + const result = queueReal(fx, body, `Owner: Dev\nReviewer: Birfy\nFixes #${fx.pr}\n`) + assert.equal(result.ok, true, result.reasons.join('; ')) + assert.equal(result.verdict.by, 'Birfy') }) test('#660-#663: the real reports still refuse at the queue on a wrong tip, an INC after them, or hold', () => { diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 3fa1fa4bf..aae3154d5 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -11,6 +11,11 @@ // - it is not the pull request author's own report, unless the author is listed in the policy's // `selfReview` (owner decision 2026-10-19: the owner's Bot/agent, acting as Birfy, may PASS // its own pull requests; any other author still needs someone else); +// - when that login is the pull request author, `selfReview` is not the review (#644, option B, +// first slice). The header must also carry `Attestation: <40-character sha> role=`: the sha +// is the whole tip, the role is the `Written-by` name, and that role is not an Owner of the pull +// request. A comment with no such line is the author's own identity, and it does not count. A +// conclusion from a different GitHub login is unchanged. The line is parsed, not signed; // - the login is tied to this repository (#676, #925): `author_association` is OWNER, MEMBER or // COLLABORATOR, the login is this pull request's own author listed in `selfReview`, or // `adminsFor` found repo admin. A CONTRIBUTOR who is none of those is not a conclusion; @@ -24,8 +29,8 @@ // - `Not run:` is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai, #667): // `hasNotRun` is still parsed for the summary, and a PASS without it is not refused. // The newest qualifying report that names this tip decides, so a later FAIL at the same tip -// vetoes an earlier PASS. A tip that moves past the report is not reviewed: the old tip is -// not a prefix of the new head and stops counting. +// vetoes an earlier PASS. A tip that moves past the report is not reviewed: the report names +// the whole 40-character sha (#1112), so an older head stops counting. // // The pull request is read only through the GitHub API; no PR code is checked out or run. import { readFileSync, appendFileSync } from 'node:fs' @@ -90,7 +95,7 @@ export function parseReport(body) { // markdown emphasis, line for line, for everything else. const raw = String(body ?? '').split('\n') const lines = raw.map((line) => line.replace(/[*`]/g, '')) - const out = { tip: null, verdict: null, ran: [], hasNotRun: false, writer: null } + const out = { tip: null, verdict: null, ran: [], hasNotRun: false, writer: null, attestation: null } // The fixed format (#637): the first non-empty line is the whole conclusion, // `Verdict: PASS at ` (or INC / REQUEST_CHANGES). The attribution line and the evidence go // below it, so a verdict Chief posts for a Reviewer with no write channel parses the same way. @@ -136,9 +141,66 @@ export function parseReport(body) { if (/^Not run\b[^:\n]*:/i.test(line)) out.hasNotRun = true } out.writer = parseWriter(lines) + // #644: the attestation is part of the conclusion header, the lines before the first blank. A copy + // quoted under `Ran:` sits after that blank and must not lend this report a marker it did not open with. + out.attestation = attestationInHeader(lines) return out } +// The header is the non-empty lines before the first blank, and never more than this many: a body with +// no blank line must not pick up an `Attestation:` buried in a quote. #1112: the sha is the whole tip, +// so 7–40 hex is parsed and a short one is refused later as "not the full tip", the same way a short +// `Verdict:` is. +const ATTEST_CAP = 8 +const ATTEST_LINE = /^Attestation:\s*([0-9a-f]{7,40})\s+role=([A-Za-z][A-Za-z0-9-]*)\s*$/i +function attestationInHeader(lines) { + let seen = 0 + for (const line of lines) { + const text = line.trim() + if (!text) { + if (seen > 0) return null + continue + } + if (++seen > ATTEST_CAP) return null + if (!/^Attestation\s*:/i.test(text)) continue + const m = ATTEST_LINE.exec(text) + if (!m) return { malformed: true } + return { sha: m[1].toLowerCase(), role: m[2] } + } + return null +} + +// The same Owner line `enqueue-rules.mjs` reads, copied here because that module imports this one. +// First 20 lines, comma-separated, a leading `@` stripped. An absent line is an empty list, which is +// not the same as a caller that passed no body at all. +const OWNER_HEADER = 20 +function ownerNames(body) { + const lines = String(body ?? '').split(/\r?\n/).slice(0, OWNER_HEADER) + const owner = lines.find((line) => /^\s*Owner:/i.test(line)) + if (!owner) return [] + return [...new Set(owner.replace(/^\s*Owner:/i, '').split(',').map((value) => value.trim().replace(/^@/, '').toLowerCase()).filter(Boolean))] +} + +// #644: why an author-login conclusion does not count, or null when the attestation is the one this +// tip can trust. `ownersKnown` is false when the caller passed neither a body nor an owner list — +// fail closed, because "no owners" would otherwise let the author name their own role. +function attestationRefusal({ parsed, headSha, owners, ownersKnown }) { + const att = parsed.attestation + if (!att) return 'the pull request author\'s own GitHub identity is not an attestation (no Attestation line binding this tip and a reviewer role)' + if (att.malformed) return 'Attestation line is not `Attestation: role=`' + if (att.sha !== headSha) { + return att.sha.length < 40 + ? `Attestation names ${att.sha.slice(0, SHORT)} (${att.sha.length} characters), not the full tip ${headSha} — an attestation names the whole 40-character sha` + : `Attestation names ${att.sha.slice(0, SHORT)}, not the tip ${headSha.slice(0, SHORT)}` + } + if (att.role.toLowerCase() !== String(parsed.writer ?? '').toLowerCase()) { + return `Attestation role "${att.role}" is not the Written-by name "${parsed.writer}"` + } + if (!ownersKnown) return 'no Owner list was supplied, so an author-login conclusion cannot be attested' + if (owners.has(att.role.toLowerCase())) return `Attestation role "${att.role}" is the Owner of this pull request` + return null +} + // #627: the attribution line, `Written-by: ` (the older prose `Written by: Reviewer. Posted by: // …` reads the same way). Only the header is read — the first few non-empty lines — so a report that // arrives without one does not borrow a writer from a block it quotes further down. @@ -192,9 +254,13 @@ const isBotAccount = (r) => String(r.type ?? '').toLowerCase() === 'bot' || /\[b const botKind = (r) => (String(r.type ?? '').toLowerCase() === 'bot' ? 'account type Bot' : 'login ends with [bot]') // reports: [{ login, body, at, kind, id }] from the comments and review bodies of one PR. -export function decide({ head, author, reports, policy, admins } = {}) { +// `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 } = {}) { const headSha = String(head ?? '').toLowerCase() const authorLogin = String(author ?? '').toLowerCase() + const ownersKnown = owners !== undefined || body !== undefined + const ownerSet = new Set((owners !== undefined ? owners : ownersKnown ? ownerNames(body) : []).map((name) => String(name).toLowerCase())) const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase())) const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) const writers = new Set((policy?.writers ?? []).map((w) => String(w).toLowerCase())) @@ -254,6 +320,13 @@ export function decide({ head, author, reports, policy, admins } = {}) { if (!writers.has(parsed.writer.toLowerCase())) { entry.why = `${where(r)}: Written-by names "${parsed.writer}", who is not in the writers list of .github/agent/review-policy.json`; continue } if (login(r) === authorLogin && !selfReview.has(authorLogin)) { entry.why = `${where(r)}: the pull request's own author is not a reviewer (not in selfReview)`; continue } if (!reviewers.has(login(r))) { entry.why = `${where(r)}: not a reviewer listed in .github/agent/review-policy.json`; continue } + // #644: `selfReview` lets the shared account PASS its own pull request. That comment is still the + // author's GitHub identity. It counts only with an attestation the header binds to this tip and to + // a reviewer role that is not the Owner. A different login never reaches this check. + if (login(r) === authorLogin) { + const why = attestationRefusal({ parsed, headSha, owners: ownerSet, ownersKnown }) + if (why) { entry.why = `${where(r)}: ${why}`; continue } + } entry.counts = true } @@ -329,7 +402,7 @@ function main() { ] const policy = loadPolicy(policyPath) - const result = decide({ head, author, reports, policy, admins: adminsFor({ gh, repo, reports, policy, author }) }) + const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }) }) 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/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index f37665922..da82c3225 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -8,10 +8,11 @@ const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' const policy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], writers: ['Reviewer', 'Chief'] } -const report = ({ tip = HEAD, verdict = 'PASS', ran = [' - npm run verify → pass'], notRun = true, writer = 'Reviewer' } = {}) => +const report = ({ tip = HEAD, verdict = 'PASS', ran = [' - npm run verify → pass'], notRun = true, writer = 'Reviewer', attest = false } = {}) => [ `Tip SHA: ${tip}`, `Written-by: ${writer}`, + ...(attest ? [`Attestation: ${tip} role=${writer}`] : []), 'Ran:', ...ran, ...(notRun ? ['Not run:', ' - smoke — host code unchanged'] : []), @@ -48,22 +49,25 @@ test('the pull request author does not review their own tip unless listed in sel test('self-review: a listed author may PASS their own pull request at the tip (owner decision 2026-10-19)', () => { const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] } - const r = decide({ head: HEAD, author: 'Birfy', reports: [comment('Birfy')], policy: selfPolicy }) - assert.equal(r.ok, true) + const bare = decide({ head: HEAD, author: 'Birfy', reports: [comment('Birfy')], policy: selfPolicy, owners: ['Dev'] }) + assert.equal(bare.ok, false, 'a self-PASS with no Attestation line is the author\'s own comment (#644)') + assert.match(bare.reason, /not an attestation/) + const r = decide({ head: HEAD, author: 'Birfy', reports: [comment('Birfy', report({ attest: true }))], policy: selfPolicy, owners: ['Dev'] }) + assert.equal(r.ok, true, r.reason) assert.equal(r.by, 'Birfy') - assert.equal(decide({ head: HEAD, author: 'birfy', reports: [comment('Birfy')], policy: selfPolicy }).ok, true, 'logins compare case-insensitively') + assert.equal(decide({ head: HEAD, author: 'birfy', reports: [comment('Birfy', report({ attest: true }))], policy: selfPolicy, owners: ['Dev'] }).ok, true, 'logins compare case-insensitively') }) test('self-review keeps every other rule: tip, evidence, listed reviewer, newest wins', () => { const selfPolicy = { reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] } const own = (body, at) => comment('Birfy', body, at) - const run = (reports, policy = selfPolicy) => decide({ head: HEAD, author: 'Birfy', reports, policy }) - assert.equal(run([own(report({ tip: OLD }))]).ok, false, 'a new commit voids the self-PASS') - assert.equal(run([own(report({ ran: [] }))]).ok, false, 'no Ran: command, no evidence') - assert.equal(run([own(report({ notRun: false }))]).ok, true, 'no Not run: line is no longer checked (owner decision 2026-10-22)') - assert.equal(run([own(report({ verdict: 'FAIL' }))]).verdict, 'FAIL') - assert.equal(run([own(report(), '2026-10-18T01:00:00Z'), own(report({ verdict: 'FAIL' }), '2026-10-18T02:00:00Z')]).ok, false, 'a later FAIL vetoes a self-PASS') - assert.equal(run([own(report())], { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] }).ok, false, 'selfReview does not make an unlisted login a reviewer') + const run = (reports, policy = selfPolicy) => decide({ head: HEAD, author: 'Birfy', reports, policy, owners: ['Dev'] }) + assert.equal(run([own(report({ tip: OLD, attest: true }))]).ok, false, 'a new commit voids the self-PASS') + assert.equal(run([own(report({ ran: [], attest: true }))]).ok, false, 'no Ran: command, no evidence') + assert.equal(run([own(report({ notRun: false, attest: true }))]).ok, true, 'no Not run: line is no longer checked (owner decision 2026-10-22)') + assert.equal(run([own(report({ verdict: 'FAIL', attest: true }))]).verdict, 'FAIL') + assert.equal(run([own(report({ attest: true }), '2026-10-18T01:00:00Z'), own(report({ verdict: 'FAIL', attest: true }), '2026-10-18T02:00:00Z')]).ok, false, 'a later FAIL vetoes a self-PASS') + assert.equal(run([own(report({ attest: true }))], { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer', 'Birfy'] }).ok, false, 'selfReview does not make an unlisted login a reviewer') assert.equal(run([comment('reviewer-agent')]).ok, true, 'another listed reviewer still counts on that pull request') }) @@ -502,9 +506,9 @@ test('#676: a conclusion from outside the repository counts for nothing', () => // and #676 still refuses everyone nobody listed. test('#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their own pull request', () => { const selfPolicy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer', 'hsliuustc0106'] } - const own = (login = 'hsliuustc0106') => comment(login, report({ writer: 'hsliuustc0106' }), '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR') + const own = (login = 'hsliuustc0106', body = report({ writer: 'hsliuustc0106', attest: true })) => comment(login, body, '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR') - const mine = decide({ head: HEAD, author: 'hsliuustc0106', reports: [own()], policy: selfPolicy }) + const mine = decide({ head: HEAD, author: 'hsliuustc0106', reports: [own()], policy: selfPolicy, owners: ['Dev'] }) assert.equal(mine.ok, true, 'listed in selfReview on its own pull request: the PASS counts') assert.equal(mine.by, 'hsliuustc0106') assert.equal(mine.judged[0].counts, true) @@ -704,3 +708,111 @@ test('#943: at one tip the bot report does not count and the shared account\'s r assert.equal(entry.counts, false) assert.match(String(entry.why), /bot account/) }) + +// #644 option B, first slice. Every Bot writes as the pull request author, so a Verdict comment +// from that login is the author talking. It counts only when the header binds this tip to a reviewer +// role that is not the Owner. A different GitHub login still needs no attestation line. +const attestPolicy = { reviewers: ['Birfy', 'reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer', 'Dev'] } +const fixedPass = ({ attest = null, writer = 'Reviewer', tip = HEAD, ran = 'Ran: npm test → pass' } = {}) => [ + `Verdict: PASS at ${tip}`, + `Written-by: ${writer}`, + 'Posted-by: Chief', + ...(attest ? [`Attestation: ${attest}`] : []), + '', + ran, +].join('\n') +const authorPass = (body, owners = ['Dev']) => decide({ + head: HEAD, + author: 'Birfy', + reports: [comment('Birfy', body)], + policy: attestPolicy, + owners, +}) + +test('#644: the author\'s own Verdict comment is not an attestation', () => { + const bare = authorPass(fixedPass()) + assert.equal(bare.ok, false) + assert.match(bare.reason, /not an attestation/) + assert.equal(bare.judged[0].counts, false) + + const prose = authorPass('Looks good, I PASS this tip. Attestation: ' + HEAD + ' role=Reviewer') + assert.equal(prose.ok, false) + assert.match(prose.reason, /no Verifier report names the tip/) +}) + +test('#644: a header attestation at the full tip, for a role that is not the Owner, counts', () => { + const ok = authorPass(fixedPass({ attest: `${HEAD} role=Reviewer` })) + assert.equal(ok.ok, true, ok.reason) + assert.equal(ok.by, 'Birfy') + // The same shape read from the pull request body, which is what the live entry points pass. + const fromBody = decide({ + head: HEAD, + author: 'Birfy', + reports: [comment('Birfy', fixedPass({ attest: `${HEAD} role=Reviewer` }))], + policy: attestPolicy, + body: 'Owner: Dev\nReviewer: Reviewer\nFixes #644\n', + }) + assert.equal(fromBody.ok, true, fromBody.reason) +}) + +test('#644: a short sha, a quoted line, a mismatched role, the Owner role, or no Owner list refuses', () => { + const short = authorPass(fixedPass({ attest: `${HEAD.slice(0, 7)} role=Reviewer` })) + assert.equal(short.ok, false) + assert.match(short.reason, /7 characters/) + + const otherTip = 'b'.repeat(40) + const wrong = authorPass(fixedPass({ attest: `${otherTip} role=Reviewer` })) + assert.equal(wrong.ok, false) + assert.match(wrong.reason, /not the tip/) + + const quoted = authorPass([ + `Verdict: PASS at ${HEAD}`, + 'Written-by: Reviewer', + '', + 'The draft below is not this conclusion:', + `Attestation: ${HEAD} role=Reviewer`, + 'Ran: npm test → pass', + ].join('\n')) + assert.equal(quoted.ok, false) + assert.match(quoted.reason, /not an attestation/) + + const proseLine = authorPass(fixedPass({ attest: 'please review role=Reviewer' })) + assert.equal(proseLine.ok, false) + assert.match(proseLine.reason, /Attestation: role=/) + + const mismatch = authorPass(fixedPass({ attest: `${HEAD} role=Dev` })) + assert.equal(mismatch.ok, false) + assert.match(mismatch.reason, /not the Written-by name/) + + const owner = authorPass(fixedPass({ attest: `${HEAD} role=Reviewer` }), ['Reviewer']) + assert.equal(owner.ok, false) + assert.match(owner.reason, /is the Owner of this pull request/) + const fromAt = decide({ + head: HEAD, + author: 'Birfy', + reports: [comment('Birfy', fixedPass({ attest: `${HEAD} role=Reviewer` }))], + policy: attestPolicy, + body: 'Owner: @Reviewer, Dev\nReviewer: Birfy\n', + }) + assert.equal(fromAt.ok, false) + assert.match(fromAt.reason, /is the Owner/) + + const noOwners = decide({ + head: HEAD, + author: 'Birfy', + reports: [comment('Birfy', fixedPass({ attest: `${HEAD} role=Reviewer` }))], + policy: attestPolicy, + }) + assert.equal(noOwners.ok, false) + assert.match(noOwners.reason, /no Owner list/) +}) + +test('#644: a conclusion from a different GitHub login needs no attestation line', () => { + const other = decide({ + head: HEAD, + author: 'opener', + reports: [comment('reviewer-agent', fixedPass())], + policy: attestPolicy, + }) + assert.equal(other.ok, true, other.reason) +}) diff --git a/AGENTS.md b/AGENTS.md index a8eea7555..b6dac434b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,7 +154,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh ### Gates - **T0 / T1:** merge through the merge queue once `tests-ok` is green at the tip, the PR is mergeable and design-conformant. No PASS is required. -- **T2 / T3:** a PASS at the head commit from a reviewer in [`.github/agent/review-policy.json`](.github/agent/review-policy.json), with green CI, and the agent that gated it then enqueues it itself ([self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized)). **T2 and T3 need no maintainer approve** (user decision 2026-10-22): no human `APPROVED` review is read or required, and `merge-gate` re-judges the head on the queue entry. **Self-PASS across Bots counts:** a PR opened by the owner's Bot/agent (`Birfy`) may be PASSed by the owner's own reviewer Bot/agent (the policy's `selfReview` list) — the shared account is why that list exists, not a licence for an implementer to review its own pull request. Neither the PASS nor green CI replaces the other. +- **T2 / T3:** a PASS at the head commit from a reviewer in [`.github/agent/review-policy.json`](.github/agent/review-policy.json), with green CI, and the agent that gated it then enqueues it itself ([self-gate](#owner-decision-the-bot-gates-and-merges-its-own-prs-2026-10-21-owner-authorized)). **T2 and T3 need no maintainer approve** (user decision 2026-10-22): no human `APPROVED` review is read or required, and `merge-gate` re-judges the head on the queue entry. **Self-PASS across Bots counts:** a PR opened by the owner's Bot/agent (`Birfy`) may be PASSed by the owner's own reviewer Bot/agent (the policy's `selfReview` list) — the shared account is why that list exists, not a licence for an implementer to review its own pull request. Neither the PASS nor green CI replaces the other. When that PASS is posted by the pull request author's own GitHub login, it also carries the attestation in [Merge gates](#merge-gates) (#644). - **A PASS must list what it ran.** The `Ran:` line (or a `Ran …:` heading over `$ ` commands) carrying at least one real command is part of the gate, not a nicety: `merge-gate` refuses a PASS that lists none (#681, #574). When a PASS at a tip cannot be read for that reason, the reviewer may post **one replacement PASS at the same tip**: its body keeps the original conclusion, adds the `Ran:` lines of what was actually run, and carries `Replaces: the PASS at merge-gate rejected for having no Ran: line`. That is a replacement, not a second conclusion — apart from it, one tip still carries one. - Any new commit voids a PASS. CI green, the merge queue and the no-bypass rule apply to every tier. - Agents never bypass a gate and never admin-merge (the routines run as the maintainer account `Birfy`, but use it only for the actions above, never for admin merge, bypass or changing protection rules). Run `verify` with `TZ=UTC`. @@ -165,6 +165,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh - **T2 and T3 need no maintainer approve** (user decision 2026-10-22 07:15Z). A human `APPROVED` review is not one of the gates and nothing reads one; the tier decides how closely a change is reviewed, not who signs it off. - **No implementer writes the Verdict.** On the API side self-review cannot be told apart mechanically — every pull request is opened by the shared `Birfy` account, and `selfReview: ["Birfy"]` in [`review-policy.json`](.github/agent/review-policy.json) makes a PASS always look like Birfy judging Birfy — so what the team holds is the role split: the implementer Bot does not write the Verdict, the Bot that writes the Verdict neither changes the tip nor re-judges its own verdict, and `selfReview` stays as it is because clearing it would leave every pull request without a conclusion. +- **An author-login comment is not a review (#644, option B, first slice).** When the conclusion's GitHub login is the pull request author, `selfReview` is not enough. The header — the lines before the first blank, and within the first eight non-empty lines — carries `Attestation: <40-character sha> role=`. The sha is the whole tip (#1112). `role` is the `Written-by` name. That role is not an `Owner:` of the pull request. A free-form comment, a `Verdict:` with no attestation line, a line after the blank, a short sha, a role that is not the `Written-by` name, or a role that is the Owner refuses. A conclusion from a different GitHub login is unchanged. `decide` in [`review-gate.mjs`](.github/scripts/review-gate.mjs) is the check, and the reviewed-tip job, `enqueue-guard` and `merge-gate` all call it. The line is parsed, not signed; a signature is the later slice of #644. T0/T1 still need no PASS. - **`hold` is human-only.** Only a person sets the label and only a person lifts it; automation refuses a held pull request and never removes the label itself. That is the one human input left on the merge path. - **The label awaiting-approval is retired.** A PASS at the tip goes straight to the queue through the enqueue guard (#687); no pull request waits for an approval click and the label is not set again. - **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #` / `Closes #`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. It refuses a **live, non-draft** request whose body carries no `Owner:` line or no `Reviewer:` line in its first 20 lines, or a `Reviewer:` that is neither a `reviewers` login nor a `writers` Bot name, or that names nobody who can record a PASS — a Bot name counts only through the one identity every Bot writes as, `Birfy`, so it names nobody while that account is outside `reviewers` (#947); a `Reviewer:` that names the same Bot as the `Owner:` is refused too, compared name by name after commas, a leading `@` and case are normalised, because one Bot owning and reviewing the same pull request is the shape #940 had with no lines at all (#951); both lines also need a value the policy knows — a `Reviewer:` outside both vocabularies and an `Owner:` outside `writers` are refused, and a line with no value is the missing line, so an empty `Owner:` cannot clear the comparison (#951); the refusal names the missing line, or the word and the list it missed (#943, #947, #951; #940 merged in about seven minutes with the largest diff of the day and neither line). A **merged** pull request is never re-judged: this check also runs on every comment, and history is not a request. Two exemptions: a mechanical release PR, which carries neither by design, and a **fork** (`pull.head.repo.full_name` is not this repository), whose head we cannot push to and where the contributor keeps their own workflow. diff --git a/changelog.d/644-tip-attestation.changed.md b/changelog.d/644-tip-attestation.changed.md new file mode 100644 index 000000000..079d14a2c --- /dev/null +++ b/changelog.d/644-tip-attestation.changed.md @@ -0,0 +1 @@ +The merge gate now refuses a tip whose only review is a comment from the pull request author's own GitHub account. A PASS from that login counts when the comment header carries `Attestation: role=` for this tip, the role matches `Written-by`, and the role is not an Owner of the pull request. A review posted by a different GitHub account is unchanged ([#644](https://github.com/ThinkFlowLab/FlyBot/issues/644)). diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 93361a6db..ef26b8f84 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -144,6 +144,13 @@ What counts as a reviewed tip, all of it: Bot/agent, so no independent reviewer is required. That is an exception about the shared *account*, not about the Bot: an implementer never reviews its own pull request ([`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates)). Any other author still needs someone else. A PASS does not replace green CI, and a new commit voids it. + **When the login is that author, the comment also carries an attestation (#644, option B, first slice).** + The header — the lines before the first blank, within the first eight non-empty lines — includes + `Attestation: <40-character sha> role=`. The sha is the whole tip. `role` is the `Written-by` name and is + not an `Owner:` of the pull request. A `Verdict:` with no such line, a line after the blank, a short sha, or a + role that is the Owner is the author's own GitHub identity and does not count. A conclusion from a different + GitHub login does not carry the line. `decide` parses it; a signature is the later slice of #644. The + reviewed-tip job, `enqueue-guard` and `merge-gate` call that one function. 3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. The comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`, or the report must be that pull request's own author listed in `selfReview` (#928), or the login must be shown as a repo admin by the permission API. Org repositories do not report `OWNER`. A `CONTRIBUTOR` who is none of those counts for nothing, so they can neither pass a tip nor hold one. An association the API did not send is not assumed, and a failed permission lookup is not admin. The permission API is for an admin GitHub did not tie to the repository: a `CONTRIBUTOR` stamp on someone else's pull request, asked only for a Tip+Verdict conclusion by a login in `reviewers`. A `selfReview` login is skipped only on that author's own report, which is the #928 carve-out and needs no lookup — skipping every `selfReview` login would never call, because the shipped policy lists every reviewer there. That lookup does not recover the own-pull-request PASS; #926 and #928 already do. 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`. Every other check is unchanged. 4. The sha on that first line **is the head being judged**. A later commit moves the tip and the old PASS stops counting; that report is then listed as "not counted", with the sha it named. @@ -165,7 +172,7 @@ makes the line a self-report, so the gate guarantees presence, not truth (#633). | --- | --- | | `.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/scripts/review-gate.mjs` | the rule above, for one pull request at a time | -| `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author), 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/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/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): diff --git a/docs/code-standards.md b/docs/code-standards.md index e32444d7a..c0814ae21 100644 --- a/docs/code-standards.md +++ b/docs/code-standards.md @@ -49,6 +49,7 @@ A change that keeps a membership, roster or presence list in sync has the same f ## Merge only a reviewed tip - By tier: **T0/T1** merge on green required checks; **T2/T3** need a **PASS at the same tip SHA** from the Reviewer ([`review-pr`](../.claude/skills/review-pr/SKILL.md)), and neither the PASS nor green CI replaces the other. The authoritative wording, the enqueue guard and the queue are in [AGENTS.md → Merge gates](../AGENTS.md#merge-gates); this page keeps only what a change must satisfy. +- **Author-login review is an attestation (#644, first slice).** When the conclusion's GitHub login is the pull request author, a `Verdict:` comment does not count. The header carries `Attestation: role=` before the blank line: the sha is the whole tip, the role is the `Written-by` name, and that role is not an `Owner:` of the pull request. A conclusion from a different GitHub login is unchanged. The line is what `decide` parses; a signature is a later slice of #644. - **One conclusion per tip.** A second `Verdict:` at the same SHA is an incident; a new commit voids the PASS; a correction reads `Correction from : …` and is not a verdict. - **Judge the merge tree.** Green on the pull request page is the head alone; the queue rebuilds the merge with the current `main`, and that run decides (#759). - Required checks ([CONTRIBUTING → Test a change](../CONTRIBUTING.md#test-a-change)) are green, or honestly listed as not run with a maintainer waiver: `npm test`, the client `--check`, `npm run lint`, plus the smoke bench when `index.js` or `src/host/` changes. From d216f1296a1b1c3561088ae443e26ccf78eb3194 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 25 Oct 2026 10:06:41 +0000 Subject: [PATCH 2/2] Name #644 as related, not fixed This tip only parses the attestation line. A Fixes keyword would close the issue before the signed-marker slice exists. judge passes the pull request body into decide for that check, and the review-pr example adds Attestation only for a same-login PASS. Co-authored-by: Hongsheng Liu --- .claude/skills/review-pr/SKILL.md | 9 +++++++-- .github/scripts/merge-gate.mjs | 3 ++- .github/scripts/review-gate.test.mjs | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 18ee6693a..c1a189492 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -40,7 +40,6 @@ Always include: Verdict: PASS | FAIL | INC at Written-by: Reviewer Posted-by: -Attestation: role=Reviewer Ran: - → pass|fail @@ -49,12 +48,18 @@ Not run: - — reason ``` +When the GitHub login posting the conclusion is the pull request author, add this line in that header, before the blank above `Ran:` (#644). A conclusion from a different GitHub login omits it: + +```text +Attestation: role=Reviewer +``` + The first line is the whole conclusion (#637): nothing may come before it, because the gates read the first line and treat any other shape as no conclusion. The sha is the whole 40 characters (#1112). The `Written-by:` line is the attribution every write carries (#627): it names a Bot from the policy's `writers`, the gates refuse a report without it, and it is a self-report, not proof — see [AGENTS.md](../../../AGENTS.md#every-state-changing-write-says-who-wrote-it-627), which is the single source. -`Attestation:` is required when the GitHub login posting the conclusion is the pull request author (#644, first slice). It stays in the header, before the blank line above `Ran:`. `role` is the `Written-by` name and is not an `Owner:` of the pull request. A conclusion posted by a different GitHub login omits the line. The gate parses the line; it does not check a signature. +`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. 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/merge-gate.mjs b/.github/scripts/merge-gate.mjs index bfff907ae..1c5b450ec 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -219,7 +219,8 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO const policy = loadPolicy(policyPath) const result = judge({ labels, files, head, author, reports, policy, gh, repo, 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 — and `judge` reads neither the body nor the labels. The fallback runs the guard's own + // 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 // `guard()` over the same pull request, so a body with no `Owner:`/`Reviewer:`, no `Fixes #`, or no // `agent-pr` is refused here too, instead of merging under a green check. It answers only for the // queue: the `pull_request` run is a report and never fails (#639). diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index da82c3225..f86758e03 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -750,7 +750,7 @@ test('#644: a header attestation at the full tip, for a role that is not the Own author: 'Birfy', reports: [comment('Birfy', fixedPass({ attest: `${HEAD} role=Reviewer` }))], policy: attestPolicy, - body: 'Owner: Dev\nReviewer: Reviewer\nFixes #644\n', + body: 'Owner: Dev\nReviewer: Reviewer\nRelated #644\n', }) assert.equal(fromBody.ok, true, fromBody.reason) })