From ef96b5002922ce7e091335428c04b443bec4f53f Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Fri, 30 Oct 2026 23:34:33 +0800 Subject: [PATCH 1/3] Guard the review step against a second conclusion at one tip A review node entered twice at an unchanged tip posted a second `Verdict:` (#1772 took two PASS comments at 5ea9acbb). `review-once.mjs` reuses `review-gate.mjs`'s `parseReport` to spot a conclusion that already names the whole tip, then posts nothing and reports `reused conclusion at `. `merge-gate` newest-wins is unchanged. Docs-not-needed: scripts and their tests; no subsystem page owns .github/scripts. --- .github/scripts/review-once.mjs | 119 +++++++++++++++++++++++++++ .github/scripts/review-once.test.mjs | 80 ++++++++++++++++++ 2 files changed, 199 insertions(+) create mode 100644 .github/scripts/review-once.mjs create mode 100644 .github/scripts/review-once.test.mjs diff --git a/.github/scripts/review-once.mjs b/.github/scripts/review-once.mjs new file mode 100644 index 000000000..f9ce08fec --- /dev/null +++ b/.github/scripts/review-once.mjs @@ -0,0 +1,119 @@ +// 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 comments with the token in `$DSH_SECRET_GITHUB_TOKEN` (never printed). +// When a comment already opens with a `Verdict:` naming this whole tip, it prints one line +// `reused conclusion at ` and posts nothing; otherwise it posts the body and says so. +// +// 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) +} + +// The conclusion this history already 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). +export function alreadyConcluded({ comments = [], tip } = {}) { + const wanted = String(tip ?? '').toLowerCase() + if (!FULL_SHA.test(wanted)) return null + for (const comment of comments ?? []) { + if (isSeparateAccount(comment?.user)) continue + const report = parseReport(comment?.body) + if (!report.verdict && !report.unknownVerdict) continue + if (String(report.tip ?? '').toLowerCase() !== wanted) continue + return { comment, report } + } + return null +} + +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 conclusion at `tip` is already there. 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', `/repos/${REPO}/issues/${pr}/comments?per_page=100`) + if (listed.status !== 200) throw new Error(`GET the comments on #${pr} answered ${listed.status}`) + const comments = Array.isArray(listed.json) ? listed.json : [] + + const hit = alreadyConcluded({ comments, tip: sha }) + if (hit) { + log(`reused conclusion at ${sha}`) + return { posted: false, reused: true, tip: sha, comment: hit.comment?.id ?? 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, 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..f93234421 --- /dev/null +++ b/.github/scripts/review-once.test.mjs @@ -0,0 +1,80 @@ +// 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) +}) From ae1ba6889d1edad692af0ee71d9deaecd18fc960 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Fri, 30 Oct 2026 23:42:34 +0800 Subject: [PATCH 2/3] Judge the newest conclusion at the tip, not the first The first version reused on any conclusion naming the tip, so it also swallowed the two bodies AGENTS.md still allows there: a replacement PASS for a report merge-gate rejected for having no `Ran:` line, and a PASS posted after `Correction from : the PASS at is withdrawn`. A later REQUEST_CHANGES was dropped too, leaving the old PASS standing. It now reads the comments newest-first and compares stamps itself, reuses only when the newest conclusion at the tip is a PASS and the body is a PASS with no `Replaces:` line, and posts every other body so newest-wins still sees it. Four cases cover that, red on the previous blob. Docs-not-needed: scripts and their tests; no subsystem page owns .github/scripts. --- .github/scripts/review-once.mjs | 76 ++++++++++++++++++++++------ .github/scripts/review-once.test.mjs | 50 ++++++++++++++++++ 2 files changed, 111 insertions(+), 15 deletions(-) diff --git a/.github/scripts/review-once.mjs b/.github/scripts/review-once.mjs index f9ce08fec..025b8be70 100644 --- a/.github/scripts/review-once.mjs +++ b/.github/scripts/review-once.mjs @@ -8,8 +8,13 @@ // pbpaste | node .github/scripts/review-once.mjs --pr 12 --tip <40-char sha> // // It reads the pull request's comments with the token in `$DSH_SECRET_GITHUB_TOKEN` (never printed). -// When a comment already opens with a `Verdict:` naming this whole tip, it prints one line -// `reused conclusion at ` and posts nothing; otherwise it posts the body and says so. +// When the newest conclusion at this whole tip is a PASS and the body being posted is a PASS too, 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 is not a PASS always posts: a later INC / REQUEST_CHANGES +// supersedes a PASS under newest-wins, and dropping it would let the old PASS stand. // // 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 @@ -31,19 +36,60 @@ const isSeparateAccount = (user) => { return user.type !== 'User' || /\[bot\]$/i.test(login) } -// The conclusion this history already carries at `tip`, or `null`. Only the full 40-character tip +// 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 - for (const comment of comments ?? []) { + 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) continue + if (!report.verdict && !report.unknownVerdict && !report.withdraw) continue if (String(report.tip ?? '').toLowerCase() !== wanted) continue - return { comment, report } + const at = stampOf(comment, index) + if (at >= newestAt) { + newestAt = at + newest = { comment, report } + } } - return null + 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 is not another PASS is a later word that must reach the pull request. +const REPLACES = /^\s*Replaces\s*:/im +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 ?? '')) + 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 (newest.report.verdict !== 'PASS') return { post: true, reason: 'the newest conclusion at this tip is not a PASS' } + if (!isPass) return { post: true, reason: 'the body being posted is not a PASS and supersedes the PASS here' } + if (isReplacement) return { post: true, reason: 'a replacement PASS for a verdict merge-gate rejected' } + return { post: false, reason: 'a PASS already names this tip' } } async function realRequest({ method, url, body, token }) { @@ -68,9 +114,9 @@ async function realRequest({ method, url, body, token }) { return { status: response.status, json } } -// Post `body` on `pr` unless a conclusion at `tip` is already there. 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. +// 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)}`) @@ -78,20 +124,20 @@ export async function postOnce({ pr, tip, body, token = process.env.DSH_SECRET_G 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', `/repos/${REPO}/issues/${pr}/comments?per_page=100`) + 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 hit = alreadyConcluded({ comments, tip: sha }) - if (hit) { + const decision = postDecision({ comments, tip: sha, body }) + if (!decision.post) { log(`reused conclusion at ${sha}`) - return { posted: false, reused: true, tip: sha, comment: hit.comment?.id ?? null } + 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, comment: created.json?.id ?? null } + 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 diff --git a/.github/scripts/review-once.test.mjs b/.github/scripts/review-once.test.mjs index f93234421..9b291bc26 100644 --- a/.github/scripts/review-once.test.mjs +++ b/.github/scripts/review-once.test.mjs @@ -78,3 +78,53 @@ test('#1798: an API failure is loud, never a silent non-post', async () => { 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) +}) From cd28cb7bb26ac352c01941ec0038f4de6254687a Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 31 Oct 2026 00:08:51 +0800 Subject: [PATCH 3/3] Reuse the same verdict already at the tip, not just the same PASS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second review found the mirror of the first bug: `postDecision` posted every non-PASS body, so a review node entered twice at an unchanged tip posted two `Verdict: REQUEST_CHANGES` at that tip — the incident #1798 exists to stop, and the issue's acceptance ("exactly one Verdict: comment at that tip") missed for refusals. It now reuses whenever the body's verdict equals the newest conclusion at the whole tip, so only a flipped verdict, a withdrawal and a `Replaces:` line still post; the header no longer claims parity with the gates' review-body channel, which this list does not read. Docs-not-needed: scripts and their tests; no subsystem page owns .github/scripts. --- .github/scripts/review-once.mjs | 36 ++++++++++++++++++---------- .github/scripts/review-once.test.mjs | 16 +++++++++++++ 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/.github/scripts/review-once.mjs b/.github/scripts/review-once.mjs index 025b8be70..d373cbbf8 100644 --- a/.github/scripts/review-once.mjs +++ b/.github/scripts/review-once.mjs @@ -7,14 +7,19 @@ // 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 comments with the token in `$DSH_SECRET_GITHUB_TOKEN` (never printed). -// When the newest conclusion at this whole tip is a PASS and the body being posted is a PASS too, 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 is not a PASS always posts: a later INC / REQUEST_CHANGES -// supersedes a PASS under newest-wins, and dropping it would let the old PASS stand. +// 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 @@ -77,19 +82,24 @@ export function alreadyConcluded({ comments = [], tip } = {}) { // 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 is not another PASS is a later word that must reach the pull request. +// (#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 (newest.report.verdict !== 'PASS') return { post: true, reason: 'the newest conclusion at this tip is not a PASS' } - if (!isPass) return { post: true, reason: 'the body being posted is not a PASS and supersedes the PASS here' } - if (isReplacement) return { post: true, reason: 'a replacement PASS for a verdict merge-gate rejected' } - return { post: false, reason: 'a PASS already names this tip' } + 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 }) { diff --git a/.github/scripts/review-once.test.mjs b/.github/scripts/review-once.test.mjs index 9b291bc26..2a4ee24e2 100644 --- a/.github/scripts/review-once.test.mjs +++ b/.github/scripts/review-once.test.mjs @@ -128,3 +128,19 @@ test('#1851: the newest conclusion at the tip decides, not the first in the list 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) +})