diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index bc16940a9..b367e83a4 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -69,6 +69,12 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p ...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 })), + // #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. + ...list(`repos/${repo}/pulls/${pr}/comments?per_page=100`) + .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 })), ] const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) // #1112: the head is read again after the comments and the diff. A push in the middle of this run diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index dd03f77f1..05fae621e 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -76,7 +76,7 @@ const withCheckpoint = (text, files) => { return `${text}\n${checkpoint({ issue, claim, files: names.length ? names : [HOST_FILE] })}` } -const fakeGh = ({ body = null, raw = false, comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, heads = null, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined, draft = false, merged = false, branch = 'dev/fixture' } = {}) => { +const fakeGh = ({ body = null, raw = false, comments = [], reviews = [], reviewComments = [], labels = ['agent-pr'], files = [], head = HEAD, heads = null, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined, draft = false, merged = false, branch = 'dev/fixture' } = {}) => { // `raw` is for the tests about the checkpoint itself: they must be able to hand over a body without one. const given = body ?? `${HEADERS}Fixes #602\n` const text = raw ? given : withCheckpoint(given, files) @@ -90,6 +90,9 @@ const fakeGh = ({ body = null, raw = false, comments = [], reviews = [], labels return JSON.stringify({ permission, role_name: permission, user: { login: 'reviewed' } }) } if (path.includes('/files')) return files.map((f) => JSON.stringify(typeof f === 'string' ? { filename: f } : f)).join('\n') + // The line-anchored comments are their own endpoint; without this branch the fixture would answer + // them with the issue comments and a test could not tell the two faces apart. + if (/\/pulls\/\d+\/comments/.test(path)) return reviewComments.map((c) => JSON.stringify(c)).join('\n') if (path.includes('/comments')) return comments.map((c) => JSON.stringify(c)).join('\n') if (path.includes('/reviews')) return reviews.map((r) => JSON.stringify(r)).join('\n') if (/\/pulls\/\d+$/.test(path)) { @@ -1048,3 +1051,69 @@ test('#602 (2026-11-01): a T0/T1 pull request with no PASS is stopped by a revie assert.match(r.reasons.join('; '), /asks not to merge this pull request/) assert.match(r.reasons.join('; '), /6561490254/) }) + +// 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 +// the same reader — and it is the one face that is a request and never a conclusion (#2091). +const inlineNote = ({ id = 6561490255, login = 'hugo', type = 'User', body = '@Birfy — **please do not merge this tip.** This line is the one that reads the wrong list.', at = '2026-11-01T17:38:11Z', association = 'CONTRIBUTOR', path = 'core/src/client/kanban.js', line = 422, replyTo = null } = {}) => + ({ id, user: { login, type }, body, created_at: at, author_association: association, path, line, ...(replyTo == null ? {} : { in_reply_to_id: replyTo }) }) + +test('#602 (2026-11-01): an inline review comment asking not to merge this tip refuses too, and names it', () => { + const gh = fakeGh({ files: [HOST_FILE], comments: [comment(report(), '2026-11-01T17:35:30Z')], reviewComments: [inlineNote()] }) + const r = main({ gh, env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, repo: 'ThinkFlowLab/FlyBot', pr: 566 }) + assert.equal(r.ok, false, 'a request written on one line of the diff is the same request') + const text = r.reasons.join('; ') + assert.match(text, /asks not to merge this pull request/) + assert.match(text, /6561490255/, 'the refusal names the comment it read') + // The two faces are read from their own endpoints: a fixture that answers both from one array would + // let an issue comment pose as the inline one and make this case green for the wrong reason. + assert.ok(gh.calls.some((path) => /\/issues\/566\/comments/.test(path)), 'the issue comments are read') + assert.ok(gh.calls.some((path) => /\/pulls\/566\/comments/.test(path)), 'the line-anchored comments are read as their own face') +}) + +test('#602 (2026-11-01): an inline comment that only talks about the code is not a stop', () => { + const talk = inlineNote({ id: 6561490256, body: 'This branch reads `own` here — is that the list the room board draws?' }) + const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviewComments: [talk] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) + +test('#602 (2026-11-01): a conclusion written on one line of the diff is a request reader only, never a verdict', () => { + // A MEMBER can write a Verifier block in a line-anchored comment; it is still not one of the two faces a + // conclusion is written in. It neither passes nor stops: the issue comment's PASS stays the one read. + const gh = fakeGh({ + files: [HOST_FILE], + comments: [comment(report(), '2026-11-01T17:35:30Z', { id: 9001 })], + reviewComments: [inlineNote({ id: 9002, login: 'reviewer-agent', association: 'MEMBER', body: report({ extra: ['**please do not merge this tip.**'] }) })], + }) + const r = main({ gh, env: { REVIEW_POLICY: POLICY, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '566' }, repo: 'ThinkFlowLab/FlyBot', pr: 566 }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.stopRequest, null, 'a body carrying a conclusion is not a stop') + assert.equal(r.verdict.id, 9001, 'the conclusion is the issue comment, not the line-anchored one') + assert.equal(r.verdict.by, 'reviewer-agent') +}) + +test('#602 (2026-11-01): a "record only" marker on one line of the diff does not hold the entry', () => { + // The marker is read from a body that parses as a conclusion (`recordOnlyAt`): a line-anchored comment + // is not one of the faces a conclusion is written in, so a Verifier block parked there cannot park the + // pull request either. + const parked = inlineNote({ id: 6561490257, login: 'reviewer-agent', association: 'MEMBER', body: report({ extra: ['Record only — do not merge'] }) }) + const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviewComments: [parked] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) + +test('#602 (2026-11-01): a reply on an inline thread is the same request, and it is read once', () => { + const reply = inlineNote({ id: 6561490258, body: '@Birfy **please do not merge this tip** — the point above still stands.', replyTo: 6561490255 }) + const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z')], reviewComments: [reply] }) + assert.equal(r.ok, false, r.reasons.join('; ')) + assert.equal((r.reasons.join('; ').match(/6561490258/g) ?? []).length, 1, 'a reply is read once, not once per face') +}) + +test('#602 (2026-11-01): an inline stop written before the PASS at this tip is not this tip\'s hold', () => { + // The window is the one `stopRequestAt` already reads: a request older than the conclusion this + // decision used has been answered by that PASS. The third face inherits it rather than opening a wider + // window of its own. + const early = inlineNote({ id: 6561490259, at: '2026-11-01T17:00:00Z' }) + const r = run({ comments: [comment(report(), '2026-11-01T17:35:30Z', { id: 9103 })], reviewComments: [early] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) diff --git a/.github/scripts/enqueue-rules.mjs b/.github/scripts/enqueue-rules.mjs index e886be076..d386dea94 100644 --- a/.github/scripts/enqueue-rules.mjs +++ b/.github/scripts/enqueue-rules.mjs @@ -14,6 +14,13 @@ const SHORT = 12 // first real one wrote `- \`RECORD_ONLY\` matches … a record written only in Chinese would not keep the // pull request out` — must not freeze itself (#687). A bullet or a quote line is allowed, but nothing // may follow the marker on it except its other language half. +// #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' + 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. // #718: a pull request with no issue behind it, or with no `agent-pr` label, is not the routine's work at @@ -221,6 +228,7 @@ export function recordOnlyAt(reports, head) { const target = String(head ?? '').toLowerCase() if (!target) return null const at = (reports ?? []) + .filter(isConclusionSource) .map((r, i) => ({ r, i, parsed: parseReport(r.body) })) .filter(({ parsed }) => parsed.verdict && parsed.tip && target === parsed.tip) .map((x) => ({ ...x, t: Date.parse(x.r.at ?? 0) || 0 })) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index cba77e064..15e417272 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -27,7 +27,7 @@ import { fileURLToPath } from 'node:url' import { adminsFor, decide, loadPolicy } from './review-gate.mjs' import { loadSigners } from './verdict-signature.mjs' import { createGate, loadRules } from './risk-gate.mjs' -import { guard } from './enqueue-rules.mjs' +import { guard, isConclusionSource } from './enqueue-rules.mjs' const SHORT = 12 const HOLD = 'hold' @@ -118,10 +118,14 @@ export function judge({ labels, files, head, author, reports, policy, expectedHe // (#640, INC point 2): a refusal here would stop every routine fix and every release. const tier = fileList(files).length ? computedTier(files) : null const exempt = exemptFromPass({ tier, files }) + // #2091: a conclusion is written in one of the two faces the API has for it — an issue comment or a + // review body. A line-anchored review comment reaches `guard()` as a request only, so it is dropped + // here, before `decide` and `adminsFor`, and cannot become a verdict or a permission lookup. + const conclusionReports = (reports ?? []).filter(isConclusionSource) // A tier that needs no PASS also needs no permission lookup. const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } - : decide({ head: target, author, reports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }), repo, pr, signers, gh, changedPaths: fileNames(files), sameChangeSet }) + : decide({ head: target, author, reports: conclusionReports, policy, body, owners, admins: admins ?? adminsFor({ gh, repo, reports: conclusionReports, policy, author }), repo, pr, signers, gh, changedPaths: fileNames(files), sameChangeSet }) if (!verdict.ok) reasons.push(verdict.reason) // A report says what the queue entry would refuse; only the queue entry refuses. Fail a // `pull_request` run and it keeps failing after the INC it names is answered by a PASS, because a @@ -219,6 +223,12 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO ...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 })), + // #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. + ...list(`repos/${repo}/pulls/${pr}/comments?per_page=100`) + .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 })), ] // The whole file record, not just the name: the T0/T1 exemption is computed from the diff by the diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 4873fecf3..647dd75bb 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -162,28 +162,40 @@ const checkpointFor = (text, files = [], pr = 637) => { return ['', '\u0060\u0060\u0060json', JSON.stringify(fields, null, 2), '\u0060\u0060\u0060', ''].join('\n') } const bodyWithCheckpoint = (text, files = [], pr = 637) => (//.test(text) ? text : `${text}\n${checkpointFor(text, files, pr)}`) -const fakeGh = ({ comments, reviews = [], labels = ['agent-pr'], files = [], head = HEAD, body = null, pr = 637, headRepo = 'ThinkFlowLab/FlyBot', draft = false, merged = false }) => (...args) => { - const path = args[args.length - 1] - const text = bodyWithCheckpoint(body ?? `${HEADERS}Fixes #${pr}\n`, files, pr) - const pull = { head: { sha: head, repo: { full_name: headRepo } }, user: { login: 'opener' }, labels: labels.map((name) => ({ name })), body: text, draft, merged } - if (path.includes('/files')) return (files ?? []).map((filename) => JSON.stringify({ filename })).join('\n') - if (path.includes('/comments')) return [comments].flat().map((entry) => JSON.stringify(entry)).join('\n') - if (path.includes('/reviews')) return [reviews].flat().map((entry) => JSON.stringify(entry)).join('\n') - if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) - throw new Error('fake gh: unexpected ' + path) +const fakeGh = ({ comments, reviews = [], reviewComments = [], labels = ['agent-pr'], files = [], head = HEAD, body = null, pr = 637, headRepo = 'ThinkFlowLab/FlyBot', draft = false, merged = false }) => { + const calls = [] + const gh = (...args) => { + const path = args[args.length - 1] + calls.push(path) + const text = bodyWithCheckpoint(body ?? `${HEADERS}Fixes #${pr}\n`, files, pr) + const pull = { head: { sha: head, repo: { full_name: headRepo } }, user: { login: 'opener' }, labels: labels.map((name) => ({ name })), body: text, draft, merged } + if (path.includes('/files')) return (files ?? []).map((filename) => JSON.stringify({ filename })).join('\n') + // The line-anchored comments are their own endpoint: one array for both would let an issue comment + // answer as an inline one and make a test green for the wrong reason. + if (/\/pulls\/\d+\/comments/.test(path)) return [reviewComments].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (path.includes('/comments')) return [comments].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (path.includes('/reviews')) return [reviews].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) + throw new Error('fake gh: unexpected ' + path) + } + gh.calls = calls + return gh } -const runGate = (fixture) => { +const runGateVia = (fixture) => { const policy = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json') writeFileSync(policy, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) - return main({ - gh: fakeGh(fixture), + const gh = fakeGh(fixture) + const result = main({ + gh, env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policy }, repo: 'ThinkFlowLab/FlyBot', pr: 637, headRef: ENTRY, }) + return { result, gh } } +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 @@ -262,6 +274,36 @@ test('#602 (2026-11-01): a comment asking not to merge refuses the queue entry, assert.match(author.reasons.join('; '), /comment 2 by opener/) }) +// The third face: a comment written on one line of the diff (`pulls/{pr}/comments`). It reaches the stop +// reader like the other two, and only that reader — a Verifier block written there is not a conclusion +// (#2091). +const apiInlineComment = ({ id = 6561490257, login = 'hugo', type = 'User', body = '@Birfy — **please do not merge this tip.** This line reads the wrong list.', at = '2026-11-01T17:38:11Z', association = 'CONTRIBUTOR', path = 'core/src/client/kanban.js', line = 422 } = {}) => + ({ id, user: { login, type }, body, created_at: at, author_association: association, path, line }) + +test('#602 (2026-11-01): an inline review comment asking not to merge refuses the queue entry, through main()', () => { + const { result, gh } = runGateVia({ comments: [apiComment(firstLine('PASS'), '2026-11-01T17:35:30Z')], reviewComments: [apiInlineComment()] }) + assert.equal(result.ok, false, 'a request written on one line of the diff is the same request') + assert.match(result.reasons.join('; '), /asks not to merge this pull request: comment 6561490257/) + // The two faces come from their own endpoints, so this cannot be green because an issue comment answered + // for the inline one. + assert.ok(gh.calls.some((path) => /\/issues\/637\/comments/.test(path)), 'the issue comments are read') + assert.ok(gh.calls.some((path) => /\/pulls\/637\/comments/.test(path)), 'the line-anchored comments are read as their own face') +}) + +test('#602 (2026-11-01): a conclusion written on one line of the diff is a request reader only, never a verdict', () => { + const { result } = runGateVia({ + comments: [apiComment(firstLine('PASS'), '2026-11-01T17:35:30Z', 9101)], + reviewComments: [apiInlineComment({ id: 9102, login: 'reviewer-agent', association: 'MEMBER', body: firstLine('PASS') })], + }) + assert.equal(result.ok, true, result.reasons.join('; ')) + assert.equal(result.verdict.id, 9101, 'the conclusion is the issue comment, not the line-anchored one') +}) + +test('#602 (2026-11-01): an inline stop older than the PASS at this tip does not refuse the entry', () => { + const result = runGate({ comments: [apiComment(firstLine('PASS'), '2026-11-01T17:35:30Z')], reviewComments: [apiInlineComment({ at: '2026-11-01T17:00:00Z' })] }) + assert.equal(result.ok, true, result.reasons.join('; ')) +}) + test('#602 (2026-11-01): a stop older than the PASS at this tip does not refuse the entry', () => { const result = runGate({ comments: [stopComment({ at: '2026-11-01T17:00:00Z' }), apiComment(firstLine('PASS'), '2026-11-01T17:35:30Z')] }) assert.equal(result.ok, true, result.reasons.join('; '))