diff --git a/.github/scripts/review-once.mjs b/.github/scripts/review-once.mjs new file mode 100644 index 000000000..d373cbbf8 --- /dev/null +++ b/.github/scripts/review-once.mjs @@ -0,0 +1,175 @@ +// One conclusion per tip (#1798): a review node that is entered twice at an unchanged tip must not +// write a second `Verdict:`. AGENTS.md says a tip carries at most one conclusion; until this the rule +// was a convention the gates only encoded as newest-wins, so #1772 took two `Verdict: PASS` comments at +// `5ea9acbb…` and merged on the second one. The gate is not relaxed here — the fix is to not write the +// second conclusion. +// +// node .github/scripts/review-once.mjs --pr 12 --tip <40-char sha> --body-file verdict.md +// pbpaste | node .github/scripts/review-once.mjs --pr 12 --tip <40-char sha> +// +// It reads the pull request's issue comments with the token in `$DSH_SECRET_GITHUB_TOKEN` (never +// printed). When the newest conclusion at this whole tip carries the same verdict as the body being +// posted, it prints one line `reused conclusion at ` and posts nothing; otherwise it posts the +// body and says so. The exceptions are the two bodies AGENTS.md still allows at a tip: a replacement +// PASS (`Replaces: the PASS at merge-gate rejected for having no Ran: line`) always posts, and a +// PASS posted after `Correction from : the PASS at is withdrawn` posts because a withdrawal +// is what voided the old one. A body that carries a *different* verdict always posts: a later INC / +// REQUEST_CHANGES supersedes a PASS under newest-wins, and a later PASS supersedes a withdrawn or +// refused one, so dropping either would leave the older word standing. +// +// The parity claimed here is with `parseReport` — the parser `review-gate` and `merge-gate` read — not +// with every channel those gates read: a `Verdict:` submitted as a review body is not in this list, +// so it cannot stop a duplicate comment here. +// +// The parse is `parseReport` from `review-gate.mjs` — the parser `review-gate` and `merge-gate` read, +// so a comment this refuses to duplicate is exactly the comment the gates already count. A verdict +// naming a *different* tip is not this tip's conclusion and does not block the post: a new commit is +// a new conclusion. +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { parseReport } from './review-gate.mjs' + +const API = 'https://api.github.com' +const REPO = 'ThinkFlowLab/FlyBot' +export const FULL_SHA = /^[0-9a-f]{40}$/ + +// A `Verdict:` GitHub did not write as a user is never a conclusion (#943, #1344), so it must not +// silence a real one. Anything that is not clearly a user account is skipped and the post goes ahead. +const isSeparateAccount = (user) => { + if (!user || typeof user !== 'object') return true + const login = String(user.login ?? '') + return user.type !== 'User' || /\[bot\]$/i.test(login) +} + +// GitHub orders comments oldest-first by default; this asks for the newest first (the one exception +// the reviewer asked for) so the conclusion that decides is on the first page even on a long pull +// request. `alreadyConcluded` does not trust that order: it compares stamps itself. +const COMMENTS = (pr) => `/repos/${REPO}/issues/${pr}/comments?per_page=100&sort=created&direction=desc` + +// The comment's own position in the history: its creation time, else its id (both monotonic on a +// repository), else the array index the caller handed in — a test fixture in posted order. +function stampOf(comment, index) { + const at = Date.parse(String(comment?.created_at ?? '')) + if (Number.isFinite(at)) return at + if (Number.isFinite(Number(comment?.id))) return Number(comment.id) + return index +} + +// The newest conclusion this history carries at `tip`, or `null`. Only the full 40-character tip +// counts: a `Verdict: PASS at 5ea9acbb` is a prefix that names every head starting that way (#1112). +// "Newest" is what `merge-gate` reads too (AGENTS.md, newest-wins), so the one this returns is the +// one that decides the tip, and a PASS followed by a withdrawal is not a PASS any more. +export function alreadyConcluded({ comments = [], tip } = {}) { + const wanted = String(tip ?? '').toLowerCase() + if (!FULL_SHA.test(wanted)) return null + let newest = null + let newestAt = -Infinity + const list = Array.isArray(comments) ? comments : [] + for (let index = 0; index < list.length; index++) { + const comment = list[index] + if (isSeparateAccount(comment?.user)) continue + const report = parseReport(comment?.body) + if (!report.verdict && !report.unknownVerdict && !report.withdraw) continue + if (String(report.tip ?? '').toLowerCase() !== wanted) continue + const at = stampOf(comment, index) + if (at >= newestAt) { + newestAt = at + newest = { comment, report } + } + } + return newest +} + +// The two shapes AGENTS.md allows at a tip whose conclusion already exists: a replacement PASS for a +// report `merge-gate` rejected for having no `Ran:` (#681), and a PASS that supersedes a withdrawn one +// (#1335). Everything else that carries a different verdict than the newest one is a later word that +// must reach the pull request; the same verdict at the same tip is the second conclusion #1798 exists +// to stop, whatever it says below the header (#1851). +const REPLACES = /^\s*Replaces\s*:/im +const verdictKind = (report) => report?.verdict ?? report?.unknownVerdict ?? null +export function postDecision({ comments, tip, body } = {}) { + const newest = alreadyConcluded({ comments, tip }) + const report = parseReport(body) + const isPass = report.verdict === 'PASS' + const isReplacement = REPLACES.test(String(body ?? '')) + const kind = verdictKind(report) + if (!newest) return { post: true, reason: 'no conclusion at this tip' } + if (newest.report.withdraw) return { post: true, reason: 'the newest conclusion at this tip withdrew its PASS' } + if (isPass && isReplacement) return { post: true, reason: 'a replacement PASS for a verdict merge-gate rejected' } + if (kind && kind === verdictKind(newest.report)) { + return { post: false, reason: newest.report.verdict === 'PASS' ? 'a PASS already names this tip' : `the same ${kind} already names this tip` } + } + return { post: true, reason: 'the body carries a different verdict than the newest conclusion here' } +} + +async function realRequest({ method, url, body, token }) { + if (!token) throw new Error('DSH_SECRET_GITHUB_TOKEN is not set') + const response = await fetch(url, { + method, + headers: { + Authorization: `Bearer ${token}`, + Accept: 'application/vnd.github+json', + 'User-Agent': 'flybot-review-once', + ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), + }, + body: body === undefined ? undefined : JSON.stringify(body), + }) + const text = await response.text() + let json = null + try { + json = text ? JSON.parse(text) : null + } catch { + json = null + } + return { status: response.status, json } +} + +// Post `body` on `pr` unless a PASS already names `tip` and this body adds no second conclusion. +// Returns what it did; the caller turns that into an exit code and a summary line. Every failure +// throws, so the node goes red rather than a review passing silently because the POST failed. +export async function postOnce({ pr, tip, body, token = process.env.DSH_SECRET_GITHUB_TOKEN, request = realRequest, log = console.log } = {}) { + const sha = String(tip ?? '').toLowerCase() + if (!FULL_SHA.test(sha)) throw new Error(`--tip must be the whole 40-character sha, got ${JSON.stringify(tip ?? null)}`) + if (!Number.isInteger(Number(pr)) || Number(pr) <= 0) throw new Error(`--pr must be a pull request number, got ${JSON.stringify(pr ?? null)}`) + if (typeof body !== 'string' || body.trim() === '') throw new Error('the conclusion body is empty') + + const call = (method, path, payload) => request({ method, url: `${API}${path}`, body: payload, token }) + const listed = await call('GET', COMMENTS(pr)) + if (listed.status !== 200) throw new Error(`GET the comments on #${pr} answered ${listed.status}`) + const comments = Array.isArray(listed.json) ? listed.json : [] + + const decision = postDecision({ comments, tip: sha, body }) + if (!decision.post) { + log(`reused conclusion at ${sha}`) + return { posted: false, reused: true, tip: sha, reason: decision.reason, comment: null } + } + + const created = await call('POST', `/repos/${REPO}/issues/${pr}/comments`, { body }) + if (created.status !== 201) throw new Error(`POST the conclusion on #${pr} answered ${created.status}`) + log(`posted conclusion at ${sha}`) + return { posted: true, reused: false, tip: sha, reason: decision.reason, comment: created.json?.id ?? null } +} + +// The process exit code: 0 for a post and 0 for a reuse. A reuse is the good outcome here, not a +// refusal — it is the second entry at an unchanged tip doing the right thing. +export async function main({ argv = process.argv.slice(2), body, request, token, log = console.log } = {}) { + const option = (name) => { + const at = argv.indexOf(`--${name}`) + return at < 0 ? null : (argv[at + 1] ?? null) + } + const pr = Number(option('pr')) + const tip = option('tip') + const file = option('body-file') + const text = body ?? (file ? readFileSync(file, 'utf8') : readFileSync(0, 'utf8')) + await postOnce({ pr, tip, body: text, request, token, log }) + return 0 +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main() + .then((code) => process.exit(code)) + .catch((error) => { + console.error(`review-once: ${error.message}`) + process.exit(1) + }) +} diff --git a/.github/scripts/review-once.test.mjs b/.github/scripts/review-once.test.mjs new file mode 100644 index 000000000..2a4ee24e2 --- /dev/null +++ b/.github/scripts/review-once.test.mjs @@ -0,0 +1,146 @@ +// One conclusion per tip (#1798). Three cases, and the middle one is the defect: a review node +// entered twice at an unchanged tip must reuse the conclusion already there instead of writing a +// second `Verdict:` — #1772 took two `Verdict: PASS` comments at `5ea9acbb…`. The other two protect +// the normal path, so the guard cannot pass by never posting. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { alreadyConcluded, postOnce, main } from './review-once.mjs' + +const tip = '5ea9acbb023a3e119c353437cfb87d021cf5765e' +const other = '6a6f22bbb6e89beb19e475fc8fc7eb528bb9ad84' + +const conclusion = (sha = tip) => `Verdict: PASS at ${sha} +Written-by: Reviewer + +## What I ran +- \`node --test .github/scripts/review-once.test.mjs\` → 3/3 + +## Risk +T1.` + +const comment = (body) => ({ id: 6515589279, user: { login: 'Birfy', type: 'User' }, body }) + +// A stand-in for the GitHub API: it answers the comment list and records every write. +function api(comments = [], { failGet = false } = {}) { + const calls = [] + const request = async ({ method, url, body }) => { + calls.push({ method, url, body }) + if (method === 'GET') return { status: failGet ? 500 : 200, json: comments } + return { status: 201, json: { id: 1234 } } + } + return { calls, request, posts: () => calls.filter((c) => c.method === 'POST') } +} + +test('#1798: no conclusion at the tip posts exactly once', async () => { + const seen = api([{ id: 1, user: { login: 'Birfy', type: 'User' }, body: 'LGTM, running the gate now.' }]) + const lines = [] + const result = await postOnce({ pr: 1772, tip, body: conclusion(), request: seen.request, token: 'stub', log: (line) => lines.push(line) }) + assert.equal(result.posted, true) + assert.equal(result.reused, false) + assert.equal(seen.posts().length, 1) + assert.equal(seen.posts()[0].body.body, conclusion()) + assert.deepEqual(lines, [`posted conclusion at ${tip}`]) +}) + +test('#1798: a conclusion already naming this tip posts nothing and reports one reused line', async () => { + const seen = api([comment(conclusion()), { id: 2, user: { login: 'Birfy', type: 'User' }, body: 'unrelated' }]) + const lines = [] + const code = await main({ argv: ['--pr', '1772', '--tip', tip], body: conclusion(), request: seen.request, token: 'stub', log: (line) => lines.push(line) }) + assert.equal(code, 0, 'a reuse is the good outcome, so the node exits 0') + assert.equal(seen.posts().length, 0, 'no second conclusion at an unchanged tip') + assert.deepEqual(lines, [`reused conclusion at ${tip}`], 'exactly one line says the conclusion was reused') +}) + +test('#1798: a conclusion naming another tip does not block the post', async () => { + const seen = api([comment(conclusion(other))]) + const lines = [] + const result = await postOnce({ pr: 1772, tip, body: conclusion(), request: seen.request, token: 'stub', log: (line) => lines.push(line) }) + assert.equal(result.posted, true) + assert.equal(seen.posts().length, 1, 'a new commit is a new conclusion') + assert.equal(seen.posts()[0].body.body, conclusion()) +}) + +test('#1798: a prefix of the tip is not the tip', () => { + assert.equal(alreadyConcluded({ comments: [comment(conclusion(tip.slice(0, 12)))], tip }), null, 'a 12-character prefix names every head that starts that way (#1112)') + assert.ok(alreadyConcluded({ comments: [comment(conclusion())], tip })) + assert.equal(alreadyConcluded({ comments: [comment(conclusion())], tip: other }), null) +}) + +test('#1798: a bot account or an untyped account is not a conclusion', () => { + const bot = { id: 3, user: { login: 'cursor[bot]', type: 'Bot' }, body: conclusion() } + const untyped = { id: 4, user: {}, body: conclusion() } + assert.equal(alreadyConcluded({ comments: [bot], tip }), null, 'a bot account is never a conclusion (#943, #1344)') + assert.equal(alreadyConcluded({ comments: [untyped], tip }), null) +}) + +test('#1798: an API failure is loud, never a silent non-post', async () => { + const seen = api([], { failGet: true }) + await assert.rejects(postOnce({ pr: 1772, tip, body: conclusion(), request: seen.request, token: 'stub', log: () => {} }), /answered 500/) + assert.equal(seen.posts().length, 0) +}) + +// The reviewer's three probes (#1851): the guard must not swallow the bodies AGENTS.md still allows at +// one tip. Each asserts a real POST, because "reused" here would kill the only remedy the gate has. + +const replacement = `${conclusion()} + +Replaces: the PASS at ${tip} merge-gate rejected for having no Ran: line` + +const withdrawal = (sha = tip) => `Correction from Reviewer: the PASS at ${sha} is withdrawn +Written-by: Reviewer` + +const request = (sha = tip) => `Verdict: REQUEST_CHANGES at ${sha} +Written-by: Reviewer +Attestation: ${sha} role=Reviewer + +Ran: \`node --test .github/scripts/review-once.test.mjs\` +Blocked: the replacement PASS would be swallowed.` + +test('#1851: a replacement PASS at the same tip is posted, not reused', async () => { + const seen = api([comment(conclusion())]) + const lines = [] + const result = await postOnce({ pr: 1581, tip, body: replacement, request: seen.request, token: 'stub', log: (line) => lines.push(line) }) + assert.equal(result.posted, true, 'a PASS merge-gate rejected for having no Ran: line must be replaceable at its tip') + assert.equal(seen.posts().length, 1) + assert.deepEqual(lines, [`posted conclusion at ${tip}`]) +}) + +test('#1851: a PASS after a withdrawal at the same tip is posted', async () => { + const seen = api([comment(conclusion()), comment(withdrawal()), { id: 9, user: { login: 'Birfy', type: 'User' }, body: 'unrelated' }]) + const result = await postOnce({ pr: 1335, tip, body: conclusion(), request: seen.request, token: 'stub', log: () => {} }) + assert.equal(result.posted, true, 'the withdrawal voided the old PASS, so a new one is not a second conclusion (#1335)') + assert.equal(seen.posts().length, 1) +}) + +test('#1851: a non-PASS body at a tip that already carries a PASS is posted', async () => { + const seen = api([comment(conclusion())]) + const result = await postOnce({ pr: 610, tip, body: request(), request: seen.request, token: 'stub', log: () => {} }) + assert.equal(result.posted, true, 'a later INC / REQUEST_CHANGES supersedes the PASS under newest-wins; dropping it leaves the veto unwritten') + assert.equal(seen.posts().length, 1) +}) + +test('#1851: the newest conclusion at the tip decides, not the first in the list', async () => { + const older = { id: 10, created_at: '2026-10-30T05:17:27Z', user: { login: 'Birfy', type: 'User' }, body: conclusion() } + const newer = { id: 11, created_at: '2026-10-30T05:21:24Z', user: { login: 'Birfy', type: 'User' }, body: withdrawal() } + assert.equal(alreadyConcluded({ comments: [newer, older], tip })?.comment?.id, 11, 'a withdrawal newer than the PASS is the newest conclusion') + const seen = api([newer, older]) + const result = await postOnce({ pr: 1335, tip, body: conclusion(), request: seen.request, token: 'stub', log: () => {} }) + assert.equal(result.posted, true) + assert.equal(seen.posts().length, 1) +}) + +test('#1851: the same refusal already at the tip is reused, not posted a second time', async () => { + const seen = api([comment(request())]) + const lines = [] + const code = await main({ argv: ['--pr', '610', '--tip', tip], body: request(), request: seen.request, token: 'stub', log: (line) => lines.push(line) }) + assert.equal(code, 0, 'a reuse is the good outcome, so the node exits 0') + assert.equal(seen.posts().length, 0, 'a second Verdict: at an unchanged tip is the incident #1798 exists to stop') + assert.deepEqual(lines, [`reused conclusion at ${tip}`], 'exactly one line says the conclusion was reused') +}) + +test('#1851: a flipped verdict at the tip still posts', async () => { + const seen = api([comment(request())]) + const result = await postOnce({ pr: 610, tip, body: conclusion(), request: seen.request, token: 'stub', log: () => {} }) + assert.equal(result.posted, true, 'a PASS over a refusal is a different verdict, not a duplicate') + assert.equal(seen.posts().length, 1) +})