From fc9a88be9d64be68626591ca5da475a1506db65c Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Mon, 2 Nov 2026 08:29:31 +0800 Subject: [PATCH] fix(gate): read a stop request written in a review body (#2084) `stopRequestAt()` dropped every review with a `kind === 'comment'` filter before it looked at the bodies, so "please do not merge this tip" typed into a review reached the guard and was ignored. The callers already build `reports` from both issue comments and review bodies (`enqueue-guard.mjs:69-71`, `merge-gate.mjs:219-221`); the verdict filter right below it is what keeps a conclusion from being read as its own stop. Remove the kind filter and say in the comment that a comment and a review body are read alike. A body carrying a conclusion is still a conclusion. Fixes #2084 --- .github/scripts/enqueue-guard.test.mjs | 34 ++++++++++++++++++++++++++ .github/scripts/enqueue-rules.mjs | 17 +++++++------ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 839efa00e..dd03f77f1 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -1014,3 +1014,37 @@ test('#602 (2026-11-01): a comment asking not to merge refuses a merged pull req const r = run({ merged: true, comments: [comment(report(), '2026-11-01T17:35:30Z'), note({ id: 8 })] }) assert.equal(r.ok, true, r.reasons.join('; ')) }) + +// A review body, the shape `reports` builds: the caller keeps the review's body and sends `submitted_at` +// as `at` (`enqueue-guard.mjs:69-71`, `merge-gate.mjs:219-221`), so a request written as a review is the +// same request in the same list. +const reviewNote = ({ id = 6561490251, login = 'hugo', type = 'User', body = '@Birfy — **please do not merge this tip.** Your PASS at ' + HEAD + ' is on the record and correct as to what it checked; the tip does not cover the readers it describes.', at = '2026-11-01T17:38:11Z', association = 'CONTRIBUTOR', state = 'COMMENTED' } = {}) => + ({ id, user: { login, type }, body, submitted_at: at, author_association: association, state }) + +test('#602 (2026-11-01): a review asking not to merge this tip refuses too, and names the review', () => { + const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [reviewNote()] }) + assert.equal(r.ok, false, 'a request written as a review is the same request') + const text = r.reasons.join('; ') + assert.match(text, /asks not to merge this pull request/) + assert.match(text, /6561490251/, 'the refusal names the review it read') + assert.match(text, /please do not merge/) + assert.equal(r.command, `gh pr merge 566 --auto --squash --match-head-commit ${HEAD}`, 'the command is printed for a reader, never run') +}) + +test('#602 (2026-11-01): a review carrying a conclusion is a conclusion, not a stop', () => { + // The same sentence that stops a comment must not make a conclusion stop itself: the verdict is what + // `decide` reads, and `recordOnlyAt` already owns the marker a parked PASS writes. + const pass = reviewNote({ id: 6561490252, login: 'reviewer-agent', association: 'MEMBER', body: report({ extra: ['**please do not merge this tip.**'] }) }) + const passing = run({ reviews: [pass] }) + assert.equal(passing.ok, true, `a PASS review is a conclusion, not a request: ${passing.reasons.join('; ')}`) + const inc = reviewNote({ id: 6561490253, login: 'reviewer-agent', association: 'MEMBER', body: report({ verdict: 'INC', extra: ['**please do not merge this tip.**'] }) }) + assert.equal(run({ reviews: [inc] }).stopRequest, null, 'an INC review is judged by decide, not read as a stop') +}) + +test('#602 (2026-11-01): a T0/T1 pull request with no PASS is stopped by a review too', () => { + const held = reviewNote({ id: 6561490254, body: '**hold** — the second file is still wrong' }) + const r = run({ files: [TEST_FILE], reviews: [held] }) + assert.equal(r.ok, false) + assert.match(r.reasons.join('; '), /asks not to merge this pull request/) + assert.match(r.reasons.join('; '), /6561490254/) +}) diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index d1e9a7853..e886be076 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -131,10 +131,11 @@ function headerReasons(body, policy) { // merge this tip" at 17:38:11, merged at 17:39:54 — because the guard read the labels, the diff and the // conclusions, and never the comments. // -// What counts: a comment by a person (`type: "User"`) or by the pull request's own author — in #2067 the -// author was the only one objecting, so a "only a non-author can stop" rule would have merged it too — -// written after the conclusion this decision used. A request older than the PASS at this tip is about the -// state that PASS has since read. With no conclusion to anchor on (a T0/T1 change) the whole list is read. +// What counts: a comment or a review body (#2073) by a person (`type: "User"`) or by the pull request's +// own author, written after the conclusion this decision used — in #2067 the author was the only one +// objecting, so a "only a non-author can stop" rule would have merged it too. A request older than the +// PASS at this tip is about the state that PASS has since read. With no conclusion to anchor on (a T0/T1 +// change) the whole list is read. // // The phrases are the ones people write, not a marker: #2067's request is a bold sentence, and its addendum // only says "the hold request above". Quoted text and code blocks are mentions, not requests — the first @@ -182,10 +183,12 @@ export function stopRequestAt(reports, { author, since } = {}) { const after = Number.isFinite(since) ? since : null const rows = (reports ?? []) .map((r, i) => ({ r, i, t: Date.parse(r?.at ?? '') || 0 })) - .filter(({ r }) => (r?.kind ?? 'comment') === 'comment') .filter(({ r }) => String(r?.type ?? '') === 'User' || (login !== '' && String(r?.login ?? '').toLowerCase() === login)) - // A conclusion is a review, not a request: "record only — do not merge" in a PASS is the marker - // `recordOnlyAt` already reads, and reading it here too would make every parked PASS its own stop. + // A comment and a review body are read alike (#2073): the request a person writes is the same one + // whichever box it was typed in, and the callers already pass both (`enqueue-guard.mjs:69-71`, + // `merge-gate.mjs:219-221`). A body carrying a conclusion is the conclusion, not a request: + // "record only — do not merge" in a PASS is the marker `recordOnlyAt` already reads, and reading it + // here too would make every parked PASS its own stop. .filter(({ r }) => !parseReport(r?.body).verdict) .filter(({ t }) => after == null || t > after) .sort((a, b) => a.t - b.t || a.i - b.i)