Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/scripts/enqueue-guard.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,10 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p
const reports = [
...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).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 })),
...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`)
.filter((v) => (v.body ?? '').trim())
.map((v) => ({ id: v.id, kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association })),
// #2119: a review's state is part of the record, and an empty body must not drop the row — "Request
// changes" is the one refusal a person writes without typing. The text readers see `''` and find no
// sentence, so the row adds a request and never a conclusion.
.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 })),
// #2091: a comment written on one line of the diff is a third place a person can write, and the stop
// request is read from all three. `kind: 'review-comment'` keeps it out of the conclusion readers
// (`isConclusionSource`), which is the half that does not change: it is a request, never a verdict.
Expand Down
88 changes: 88 additions & 0 deletions .github/scripts/enqueue-guard.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,94 @@ test('#602 (2026-11-01): a T0/T1 pull request with no PASS is stopped by a revie
assert.match(r.reasons.join('; '), /6561490254/)
})

// ---- #2119: "Request changes" is the one refusal a person can write without typing anything, and it
// carried no text for the stop reader to match: the guard dropped every review with an empty body on the
// way in, so the review state never reached `stopRequestAt` and the answer was ALLOW. The state is a
// fourth face — the review's own verdict — and it is read like the other three. The fixture is the one
// above (`reviewNote`), which already carries `state`; only the state and the empty body are new.
test('#2119: a review requesting changes stops the enqueue with no body, and names the review', () => {
const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [reviewNote({ id: 6561490260, state: 'CHANGES_REQUESTED', body: '' })] })
assert.equal(r.ok, false, 'an empty-body "Request changes" is a stop, not a review with nothing to say')
assert.match(r.reasons.join('; '), /review 6561490260 by hugo asked for changes/)
assert.equal(r.stopRequest?.id, 6561490260, 'the refusal names the review a reader has to answer')
})

test('#2119: the same review with the COMMENTED state and no body leaves the decision alone', () => {
const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [reviewNote({ id: 6561490266, state: 'COMMENTED', body: '' })] })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.stopRequest, null, 'a comment-only review asks for nothing')
})

test('#2119: a later APPROVED review by the same login clears the request, and the record says which review cleared it', () => {
const refused = reviewNote({ id: 6561490260, state: 'CHANGES_REQUESTED', body: '' })
const cleared = reviewNote({ id: 6561490261, state: 'APPROVED', body: '', at: '2026-11-01T17:45:00Z' })
const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [refused, cleared] })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.stopRequest, null, 'review 6561490261 (APPROVED, 17:45:00Z) cleared the request review 6561490260 made')
// And the same pair the other way round stays a request: an approval is only an answer to a request it
// follows, never a licence to ignore one written after it.
const later = reviewNote({ id: 6561490262, state: 'CHANGES_REQUESTED', body: '', at: '2026-11-01T17:50:00Z' })
const after = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [cleared, later] })
assert.equal(after.ok, false)
assert.match(after.reasons.join('; '), /review 6561490262 by hugo asked for changes/)
})

test("#2119: another login's approval does not answer the request, and a comment that lets the tip merge does", () => {
const refused = reviewNote({ id: 6561490260, state: 'CHANGES_REQUESTED', body: '' })
// A different person approving is not the reviewer changing their mind: the request still stands.
const outsider = reviewNote({ id: 6561490270, login: 'someone-else', state: 'APPROVED', body: '', at: '2026-11-01T17:45:00Z' })
const still = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [refused, outsider] })
assert.equal(still.ok, false, 'only the login that asked can take it back with a review')
assert.match(still.reasons.join('; '), /review 6561490260 by hugo asked for changes/)
// The third way out the issue names: a newer comment that lets this tip merge. It is the same
// `LIFT_REQUEST` path a written stop already answers to, so a state stop is not wedged waiting for a
// second trip through the review UI.
const lifted = note({ id: 6561490271, body: 'Covered now — merge it.', at: '2026-11-01T17:50:00Z' })
const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z'), lifted], reviews: [refused] })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.stopRequest, null, 'comment 6561490271 (17:50:00Z) cleared the request review 6561490260 made')
})

test('#2119: a dismissal clears the request it dismissed, and a request older than the PASS does not apply', () => {
const refused = reviewNote({ id: 6561490260, state: 'CHANGES_REQUESTED', body: '' })
const dismissed = reviewNote({ id: 6561490263, state: 'DISMISSED', body: '', at: '2026-11-01T17:45:00Z' })
const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviews: [refused, dismissed] })
assert.equal(r.ok, true, r.reasons.join('; '))
const early = reviewNote({ id: 6561490264, state: 'CHANGES_REQUESTED', body: '', at: '2026-11-01T17:00:00Z' })
// The window opens at the conclusion this decision used, so the PASS needs its id — the handle
// `conclusionAt` reads the time from (#1112).
const after = run({ comments: [comment(report(), '2026-11-01T17:35:30Z', { id: 6561490265 })], reviews: [early] })
assert.equal(after.ok, true, 'the PASS at 17:35:30Z answered the request written at 17:00:00Z')
})

test('#2119: a review request and its approval do not answer a request written before them', () => {
// The two kinds of request are read in one pass over the same list, and they are not the same request:
// the review UI answers a review, a comment answers a comment. A `CHANGES_REQUESTED` row that lands
// after a written hold is the later request; the same login's later `APPROVED` answers that review and
// leaves the hold standing, because the hold was never written as a review.
const pass = comment(report(), '2026-11-01T17:35:30Z')
const hold = note({ id: 6561490280, body: '**hold** — the second file is still wrong', at: '2026-11-01T17:38:00Z' })
const asked = reviewNote({ id: 6561490281, state: 'CHANGES_REQUESTED', body: '', at: '2026-11-01T17:38:11Z' })
const approved = reviewNote({ id: 6561490282, state: 'APPROVED', body: '', at: '2026-11-01T17:45:00Z' })
const both = run({ comments: [pass, hold], reviews: [asked] })
assert.equal(both.ok, false, 'the review is a request too, and it is the later of the two')
assert.match(both.reasons.join('; '), /review 6561490281 by hugo asked for changes/, 'the later request is the one named')
const cleared = run({ comments: [pass, hold], reviews: [asked, approved] })
assert.equal(cleared.ok, false, 'approving the review answers the review, not the hold written before it')
assert.match(cleared.reasons.join('; '), /comment 6561490280 by hugo/, 'the hold is still the request that stands')
assert.equal(cleared.stopRequest?.id, 6561490280)
// A third person's hold is answered the same way: the review approval never touched it.
const other = note({ id: 6561490283, login: 'someone-else', body: '**hold** — please do not merge this tip', at: '2026-11-01T17:38:00Z' })
const third = run({ comments: [pass, other], reviews: [asked, approved] })
assert.equal(third.ok, false, "another login's review approval does not answer a hold it never wrote")
assert.equal(third.stopRequest?.id, 6561490283)
// A dismissal is the same answer as an approval: it dismisses the review, not the comment.
const dismissed = reviewNote({ id: 6561490284, state: 'DISMISSED', body: '', at: '2026-11-01T17:45:00Z' })
const gone = run({ comments: [pass, hold], reviews: [asked, dismissed] })
assert.equal(gone.ok, false, 'the dismissal does not lift a request that was not a review')
assert.equal(gone.stopRequest?.id, 6561490280)
})

// The third face the API offers: a comment written on one line of the diff (`pulls/{pr}/comments`). It
// carries the same `body` and `user` as the other two, and `created_at` where a review carries
// `submitted_at`. A person reaching for the line that is wrong writes the request there, so it reaches
Expand Down
49 changes: 43 additions & 6 deletions .github/scripts/enqueue-rules.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -276,17 +276,51 @@ export function stopRequestAt(reports, { author, since } = {}) {
.filter(({ r }) => !parseReport(r?.body).verdict || responseOnly(r?.body))
.filter(({ t }) => after == null || t > after)
.sort((a, b) => a.t - b.t || a.i - b.i)
let stop = null
for (const { r } of rows) {
// Two kinds of request, kept apart because they are answered by different things: a body that asks not
// to merge (`written`) is answered in writing, while a review that asked for changes (`stateStop`) is
// answered by a review. The review UI does not answer a comment, so one may not erase the other.
let written = null
let stateStop = null
for (const { r, i, t } of rows) {
const text = withoutQuotes(r.body)
const state = String(r?.state ?? '').toUpperCase()
// #2119: "Request changes" is a request with no sentence in it, so the review's own state is read
// before its text — with a body or without one, the review's id is the handle and its state is what
// it asked for. It is read only here: a review body is still a conclusion when it carries a
// `Verdict:` (the filter above), and the state never makes one. It replaces only an earlier state
// request: a request written as a comment is still standing (#2119, review 2026-11-02).
if (state === 'CHANGES_REQUESTED') {
stateStop = { id: r.id, login: r.login, at: r.at ?? null, phrase: 'asked for changes', kind: 'review', because: 'state', t, i }
continue
}
// The answers a state request takes: the same login's later approval, or the dismissal of the
// review. Only a state request is cleared this way — a written request is still answered in
// writing or by a corrected tip (#602, 2026-11-01) — and only a later row, because `rows` is
// sorted by the time GitHub recorded.
if (stateStop && (state === 'APPROVED' || state === 'DISMISSED')
&& String(r?.login ?? '').toLowerCase() === String(stateStop.login ?? '').toLowerCase()) {
stateStop = null
continue
}
const phrase = STOP_REQUEST.exec(text)?.[0] ?? HOLD_LINE.exec(text)?.[0]
if (phrase) {
stop = { id: r.id, login: r.login, at: r.at ?? null, phrase: phrase.trim() }
written = { id: r.id, login: r.login, at: r.at ?? null, phrase: phrase.trim(), t, i }
continue
}
if (stop && unconditionalLift(text)) stop = null
// The lift is a person saying this tip may merge, without saying which kind of request they wrote
// before: it answers both. `unconditionalLift` is main's reading of that sentence (#2246): a lift
// that names a condition, or that a preceding clause withholds, answers nothing.
if ((written || stateStop) && unconditionalLift(text)) {
written = null
stateStop = null
}
}
return stop
// Both may stand at once, and the later one is the request a reader has to answer next. `rows` is
// sorted, so `t` (then the source index, for two rows in the same second) says which is later.
if (!written) return stateStop
if (!stateStop) return written
const later = stateStop.t > written.t || (stateStop.t === written.t && stateStop.i > written.i)
return later ? stateStop : written
}

// Where the window opens: the timestamp of the conclusion this decision used (#1112 gives it an id, the
Expand Down Expand Up @@ -451,7 +485,10 @@ export function guard({ pr, head, labels, body, files, mechanicalRelease, fork,
// #602 (2026-11-01): the stop request in the comments, read after the conclusion this decision used. A
// merged pull request is history, not a request — this check runs again on every comment (#926, #943).
const stop = merged ? null : stopRequestAt(reports, { author, since: conclusionAt(reports, result.verdict) })
if (stop) reasons.push(`a comment asks not to merge this pull request: comment ${stop.id ?? 'without an id'} by ${stop.login} at ${stop.at ?? 'no time'} says "${stop.phrase}" — clear it on the record with a newer comment that lets this tip merge, or push a corrected tip that earns its own PASS`)
// #2119: a review that asked for changes carries no sentence to quote, so the refusal names the
// review and what it asked for, and says how to answer a state rather than a text.
if (stop?.because === 'state') reasons.push(`review ${stop.id ?? 'without an id'} by ${stop.login} asked for changes${stop.at ? ` at ${stop.at}` : ''} — answer it with a later approving review or a dismissal, or push a corrected tip that earns its own PASS`)
else if (stop) reasons.push(`a comment asks not to merge this pull request: comment ${stop.id ?? 'without an id'} by ${stop.login} at ${stop.at ?? 'no time'} says "${stop.phrase}" — clear it on the record with a newer comment that lets this tip merge, or push a corrected tip that earns its own PASS`)
// Two exemptions, both about what the pull request is rather than how risky it is. A mechanical release
// PR carries neither an issue nor the label by design — it is the routine's own daily output under the
// standing approval (AGENTS.md 6). A fork is not ours: its head cannot be pushed by us, and the owner runs
Expand Down
17 changes: 11 additions & 6 deletions .github/scripts/merge-gate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@
// A `pull_request`-time result is never reused: on #610 the tip report ran at 10:31:08Z and the INC
// landed at 11:24:37Z, 54 minutes later, so the verdict is re-read here, live, for this head.
//
// A review's `state` is deliberately not read. The account cannot approve its own pull requests, so
// its reviews stay `COMMENTED` even when it says INC, and the INC on #610 was a comment, not a review.
// A body that carries only a `## Review —` heading and no `Verdict:` line is "no conclusion", which
// refuses: the refusal direction never depends on reading more of the text than `decide` does.
// A review's `state` is read, but only as a stop request (#2119): "Request changes" with no body is the
// one refusal a person writes without typing, and it is a stop whichever box it was typed in. The state
// is still not a conclusion. The account cannot approve its own pull requests, so its reviews stay
// `COMMENTED` even when it says INC, and the INC on #610 was a comment, not a review — a `COMMENTED`
// review with no body is not a stop either. A body that carries only a `## Review —` heading and no
// `Verdict:` line is "no conclusion", which refuses: the refusal direction never depends on reading
// more of the text than `decide` does.
import { appendFileSync } from 'node:fs'
import { execFileSync } from 'node:child_process'
import { fileURLToPath } from 'node:url'
Expand Down Expand Up @@ -221,8 +224,10 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO
const reports = [
...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).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 })),
...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`)
.filter((v) => (v.body ?? '').trim())
.map((v) => ({ id: v.id, kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association })),
// #2119: a review's state is part of the record, and an empty body must not drop the row — "Request
// changes" is the one refusal a person writes without typing. The text readers see `''` and find no
// sentence, so the row adds a request and never a conclusion.
.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 })),
// #2091: a comment written on one line of the diff is a third place a person can write, and the stop
// request is read from all three. `kind: 'review-comment'` keeps it out of the conclusion readers
// (`isConclusionSource`), which is the half that does not change: it is a request, never a verdict.
Expand Down
16 changes: 15 additions & 1 deletion .github/scripts/merge-gate.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ const runGate = (fixture) => runGateVia(fixture).result
// The comment id is the handle a refusal names (#1112), and #602 (2026-11-01) names the stop it reads.
let commentId = 5000
const apiComment = (body, at, id = commentId++) => ({ id, user: { login: 'reviewer-agent', type: 'User' }, body, created_at: at, author_association: 'MEMBER' })
const apiReview = (body, at) => ({ user: { login: 'reviewer-agent', type: 'User' }, body, submitted_at: at, author_association: 'MEMBER' })
const apiReview = (body, at, { id, login = 'reviewer-agent', type = 'User', state, association = 'MEMBER' } = {}) =>
({ ...(id == null ? {} : { id }), user: { login, type }, body, submitted_at: at, author_association: association, ...(state == null ? {} : { state }) })

const firstLine = (verdict, tip = HEAD) => [
`Verdict: ${verdict} at ${tip}`,
Expand Down Expand Up @@ -309,6 +310,19 @@ test('#602 (2026-11-01): a stop older than the PASS at this tip does not refuse
assert.equal(result.ok, true, result.reasons.join('; '))
})

// ---- #2119: the same empty-body "Request changes", read at the last moment before the merge. `merge-gate`
// builds its own `reports` from the same three endpoints, so it dropped the review state the same way the
// guard did and answered ok for a pull request a person had rejected without typing. One case is enough
// here: the clearing rules are `stopRequestAt`'s and are pinned in `enqueue-guard.test.mjs`.
test('#2119: a review requesting changes with no body refuses the queue entry', () => {
const result = runGate({
comments: [apiComment(firstLine('PASS'), '2026-11-01T17:35:30Z')],
reviews: [apiReview('', '2026-11-01T17:38:11Z', { id: 6561490260, login: 'hugo', association: 'CONTRIBUTOR', state: 'CHANGES_REQUESTED' })],
})
assert.equal(result.ok, false, 'a queue entry is stopped by "Request changes" with no text')
assert.match(result.reasons.join('; '), /review 6561490260 by hugo asked for changes/)
})

test('#637: hold still refuses through main()', () => {
const result = runGate({ labels: ['hold'], comments: [apiComment(firstLine('PASS'), '2026-10-21T12:40:00Z')] })
assert.equal(result.ok, false)
Expand Down
1 change: 1 addition & 0 deletions changelog.d/2119-review-state-stop.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
A review asking for changes stopped nothing: the state was dropped on the way in and an empty body matched no stop, so both the enqueue guard and the queue entry answered ok. The review's state is now read — `CHANGES_REQUESTED` refuses with or without a body, names the review, and a later approval or a dismissal clears it. The written request and the review's request are read apart, so approving the review answers the review and leaves a hold written in a comment standing.
Loading