From 763e15078270c128dd5302f76a932c2eecf91e15 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Wed, 11 Nov 2026 19:14:36 +0800 Subject: [PATCH] fix(gates): give review-gate the same report rows the guard and the queue read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.github/scripts/review-gate.mjs` built its `reports` rows in a third copy that had drifted from `enqueue-guard.mjs` and `merge-gate.mjs`: the review row carried no `id` (the handle a decision names), no `state` (`stopRequestAt` reads CHANGES_REQUESTED from it — the whole of a refusal a person writes without typing), a review whose body was empty was dropped, and a comment written on one line of the diff never reached the report at all. So this report could say PASS while the queue gate refused the same tip for a standing request. `reportsFrom` now builds all three faces once, and the conclusion readers skip `kind: 'review-comment'` through the one predicate `isConclusionSource`, which moves to `review-gate.mjs` (the module that owns what a conclusion is) and is re-exported by `enqueue-rules.mjs`, so both readers keep one definition and no import cycle is introduced. The three row builders are deliberately left in three places; merging them is a separate change. --- .github/scripts/enqueue-rules.mjs | 9 ++-- .github/scripts/review-gate.mjs | 47 ++++++++++++++++--- .github/scripts/review-gate.test.mjs | 68 +++++++++++++++++++++++++++- 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index ad4cc6435..869a93faf 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -3,7 +3,7 @@ // read the same text: the guard that prints the command, and `merge-gate.mjs`, whose required check is // the only gate on a `merge_group` entry today (#960). A second copy of these rules would be two // answers to one question, which is #812's kill condition. -import { parseReport } from './review-gate.mjs' +import { parseReport, isConclusionSource } from './review-gate.mjs' const SHORT = 12 // AGENTS.md → Merge gates rule 2: the `hold` label is paired with a comment saying @@ -17,9 +17,10 @@ const SHORT = 12 // #2091: a comment written on one line of the diff (`pulls/{pr}/comments`, `kind: 'review-comment'`) is a // request and never a conclusion. `stopRequestAt` reads all three faces, because a person's "do not // merge" is the same request in whichever box it is typed; the conclusion readers take the two faces a -// Verifier block is written in, so adding the third input cannot add a way to record a PASS. One -// predicate, used by every reader that parses a body for a conclusion. -export const isConclusionSource = (r) => r?.kind !== 'review-comment' +// Verifier block is written in, so adding the third input cannot add a way to record a PASS. The +// predicate itself is `isConclusionSource` in `review-gate.mjs`, re-exported here so the guard and +// `merge-gate.mjs` keep one import. +export { isConclusionSource } const RECORD_ONLY = /^[\s>*+-]*(?:record only\s*[—–-]+\s*do not (?:merge|queue)(?:\s*[/／]\s*仅记录[、,，]?\s*禁止入队)?|仅记录[、,，]?\s*禁止入队)\s*[.。]?\s*$/im // AGENTS.md → Claim: a pull request says `Fixes #N` in its body, and one issue carries one pull request. diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 8814e7117..ba5563f26 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -125,6 +125,14 @@ function parseCorrection(line) { return { by, tip: tip[1].toLowerCase() } } +// #2091: a comment written on one line of the diff (`pulls/{pr}/comments`, `kind: 'review-comment'`) is a +// request and never a conclusion. `stopRequestAt` reads all three faces, because a person's "do not merge" +// is the same request in whichever box it is typed; a conclusion is written in one of the other two, so +// adding the third input cannot add a way to record a PASS. One predicate, asked by every reader that +// parses a body for a conclusion. It lives here because this module owns what a conclusion is; +// `enqueue-rules.mjs` re-exports it, so its callers keep one import. +export const isConclusionSource = (r) => r?.kind !== 'review-comment' + // The last Verifier block in one body: a comment may hold an earlier review that a later // one replaced, and only the last block is the verdict the reviewer stands behind. export function parseReport(body) { @@ -595,6 +603,29 @@ function arg(name) { return i > 0 ? process.argv[i + 1] : undefined } +// #2092/#2120/#2870: the rows every gate reader parses, in one place. This file's copy had drifted from +// `enqueue-guard.mjs:67` and `merge-gate.mjs:224`: it carried no `id` (the handle a decision prints, and +// the one `conclusionAt` and `stopRequestAt` answer with), no review `state` (the whole of a +// CHANGES_REQUESTED review with no body, which `stopRequestAt` reads as "asked for changes"), dropped a +// review whose body is empty (#2119: "Request changes" is the one refusal a person writes without +// typing), and never read the comments written on one line of the diff (#2091). +export function reportsFrom({ comments = [], reviews = [], reviewComments = [] } = {}) { + return [ + ...comments.map((c) => ({ + id: c.id, kind: 'comment', login: c.user?.login, type: c.user?.type, body: c.body, + at: c.created_at, association: c.author_association, url: c.html_url, + })), + ...reviews.map((v) => ({ + id: v.id, kind: 'review', state: v.state, login: v.user?.login, type: v.user?.type, body: v.body, + at: v.submitted_at, association: v.author_association, url: v.html_url, + })), + ...reviewComments.filter((c) => (c.body ?? '').trim()).map((c) => ({ + id: c.id, kind: 'review-comment', login: c.user?.login, type: c.user?.type, body: c.body, + at: c.created_at, association: c.author_association, url: c.html_url, + })), + ] +} + function main() { const policyPath = arg('--policy') || process.env.REVIEW_POLICY || fileURLToPath(new URL('../agent/review-policy.json', import.meta.url)) const repo = arg('--repo') || process.env.GH_REPO @@ -607,12 +638,11 @@ function main() { const author = pull.user?.login if (!head) throw new Error(`pull request ${pr} has no head sha`) - const reports = [ - ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, type: c.user?.type, body: c.body, at: c.created_at, association: c.author_association, url: c.html_url })), - ...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) - .filter((v) => (v.body ?? '').trim()) - .map((v) => ({ kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association, url: v.html_url })), - ] + const reports = reportsFrom({ + comments: list(`repos/${repo}/issues/${pr}/comments?per_page=100`), + reviews: list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`), + reviewComments: list(`repos/${repo}/pulls/${pr}/comments?per_page=100`), + }) const policy = loadPolicy(policyPath) // #1317: the changed-file list, read here so a tip that moved only because `main` moved can be told @@ -623,7 +653,10 @@ function main() { // changes only what its own run reads. `loadSigners` is fail-closed on its own; `decide` refuses when the // policy lists a roster and this is missing. const signers = loadSigners({ policy, path: arg('--signers') || process.env.VERDICT_SIGNERS || fileURLToPath(new URL('../agent/signers.json', import.meta.url)) }) - const result = decide({ head, author, reports, policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers, gh, changedPaths }) + // Only the two faces a Verifier block is written in are conclusions (#2091): the same filter + // `merge-gate.mjs:127` applies, so a comment on one line of the diff cannot pass here and be refused + // there. `adminsFor` still reads every row — a login is a login wherever it wrote. + const result = decide({ head, author, reports: reports.filter(isConclusionSource), policy, body: pull.body, admins: adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers, gh, changedPaths }) 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 0dc441e2e..c060952fb 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -2,7 +2,8 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { readFileSync } from 'node:fs' -import { adminsFor, decide, loadPolicy, parseReport, render } from './review-gate.mjs' +import { adminsFor, decide, isConclusionSource, loadPolicy, parseReport, render, reportsFrom } from './review-gate.mjs' +import { stopRequestAt } from './enqueue-rules.mjs' const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' @@ -1008,3 +1009,68 @@ test('#1335: a correction listed before the PASS it withdraws still refuses — assert.equal(r.ok, false, 'the PASS it withdraws is listed after it, and the withdrawal still refuses') assert.match(r.reason, new RegExp(`the PASS at ${HEAD} was withdrawn by Reviewer`)) }) + +// #2870: this file's rows had drifted from `enqueue-guard.mjs:67` and `merge-gate.mjs:224`. A review that +// asks for changes with no body is the whole of what a person types there ("Request changes" is a button), +// and the queue gate answers it from the review's own `state`; without `state` (and without the row at all, +// which an empty-body filter dropped) the report here could say PASS while the guard refused the same pull +// request. These fixtures are GitHub API objects, the shape `reportsFrom` reads. +const ghReview = (over = {}) => ({ + id: 9001, + state: 'CHANGES_REQUESTED', + body: '', + submitted_at: '2026-11-11T10:00:00Z', + user: { login: 'reviewer-agent', type: 'User' }, + author_association: 'MEMBER', + html_url: 'https://github.com/ThinkFlowLab/FlyBot/pull/6#pullrequestreview-9001', + ...over, +}) +const ghComment = (over = {}) => ({ + id: 7001, + body: 'do not merge this tip', + created_at: '2026-11-11T10:00:00Z', + user: { login: 'reviewer-agent', type: 'User' }, + author_association: 'MEMBER', + html_url: 'https://github.com/ThinkFlowLab/FlyBot/pull/6#issuecomment-7001', + ...over, +}) + +test('#2870: a review with no body still arrives as a row, with its id and state', () => { + const rows = reportsFrom({ reviews: [ghReview()] }) + assert.equal(rows.length, 1, 'an empty body must not drop the row (#2119)') + assert.equal(rows[0].id, 9001, 'the id is the handle a decision names') + assert.equal(rows[0].kind, 'review') + assert.equal(rows[0].state, 'CHANGES_REQUESTED', 'the state is what a request written with no sentence says') + assert.equal(rows[0].body, '') +}) + +test('#2870: the same rows answer the queue gate\'s stop request for a review that asked for changes', () => { + const rows = reportsFrom({ reviews: [ghReview()] }) + const stop = stopRequestAt(rows, { author: 'opencode-agent' }) + assert.equal(stop?.phrase, 'asked for changes') + assert.equal(stop?.because, 'state', 'it is read from the state, not from a body that is empty') + assert.equal(stop?.id, 9001) +}) + +test('#2870: an APPROVED review with no body keeps its row and changes no conclusion', () => { + const rows = reportsFrom({ + reviews: [ghReview({ id: 9002, state: 'APPROVED' })], + comments: [ghComment({ id: 7002, body: report() })], + }) + assert.equal(rows.length, 2, 'both rows are kept') + assert.equal(rows.filter(isConclusionSource).length, 2, 'a review is one of the two conclusion faces') + const r = decide({ head: HEAD, author: 'opencode-agent', reports: rows.filter(isConclusionSource), policy }) + assert.equal(r.ok, true, r.reason) + assert.equal(r.by, 'reviewer-agent', 'the PASS in the comment still decides') + assert.equal(stopRequestAt(rows, { author: 'opencode-agent' }), null, 'an approval is not a request') +}) + +test('#2870: a comment on one line of the diff is a request, never a conclusion', () => { + const rows = reportsFrom({ reviewComments: [ghComment({ body: report() })] }) + assert.equal(rows.length, 1) + assert.equal(rows[0].kind, 'review-comment') + assert.equal(rows.filter(isConclusionSource).length, 0, 'its Verifier block is not a conclusion (#2091)') + assert.equal(decide({ head: HEAD, author: 'opencode-agent', reports: rows.filter(isConclusionSource), policy }).ok, false, 'no conclusion names the tip') + const ask = reportsFrom({ reviewComments: [ghComment({ body: 'one more thing — do not merge this tip\n' })] }) + assert.equal(stopRequestAt(ask, { author: 'opencode-agent' })?.phrase, 'do not merge') +})