Guard the review step against a second conclusion at one tip

.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 <tip>` 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 <sha> merge-gate rejected for having no Ran: line`) always posts, and a
+// PASS posted after `Correction from <Bot>: the PASS at <sha> 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)
+    })
+}

.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)
+})