diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index a2390ed1..fbe22462 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -50,9 +50,37 @@ const RECORD_ONLY = /^[\s>*+-]*(?:record only\s*[—–-]+\s*do not (?:merge|que // The rules only hold where the branch is ours: a fork is exempt, because we cannot push to its head // and the owner keeps their own workflow there (#711/#713). const ISSUE_REF = /^[\s>#-]*(?:fixes|closes)\s*(?:[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+)?#\d+/im +// #943: the body says who owns the pull request and who reviews it. `Owner:`/`Reviewer:` were a +// sentence in AGENTS.md that nothing read: #940 went from opened to merged in about seven minutes with +// the largest diff of the day, and its body carried neither line. +// +// The first 20 lines, not the first two: an agent's body opens with its own HTML comment (the open #941 +// carries `Owner:` on line 4), and a check that only read the top would refuse every one of them. +const HEADER_LINES = 20 +const OWNER_LINE = /^\s*Owner:/i +const REVIEWER_LINE = /^\s*Reviewer:/i const short = (s) => String(s ?? '').slice(0, SHORT) const labelNames = (labels) => (labels ?? []).map((l) => String(typeof l === 'string' ? l : l?.name ?? '').toLowerCase()) +// The refusal names the line, not just the defect, so the fix is "add this line" rather than "something +// about the body". A `Reviewer:` line with no value names nobody, which is the same defect as a missing +// line: the queue is meant to know who reviews, not to see the word. A value is one login, with or +// without a leading `@`, compared case-insensitively against the policy's `reviewers`. +function headerReasons(body, policy) { + const lines = String(body ?? '').split(/\r?\n/).slice(0, HEADER_LINES) + const reasons = [] + if (!lines.some((line) => OWNER_LINE.test(line))) reasons.push('missing line: Owner:') + const reviewer = lines.find((line) => REVIEWER_LINE.test(line)) + if (!reviewer) reasons.push('missing line: Reviewer:') + else { + const names = reviewer.replace(REVIEWER_LINE, '').split(',').map((value) => value.trim().replace(/^@/, '').toLowerCase()).filter(Boolean) + if (!names.length) reasons.push('missing line: Reviewer:') + const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase())) + for (const name of names) if (!reviewers.has(name)) reasons.push(`Reviewer names "${name}", who is not in reviewers`) + } + return reasons +} + // The newest conclusion that names this head, if it carries the record-only marker. Only a body that // parses as a conclusion counts, so ordinary prose quoting the phrase is no marker; only the newest // one counts, so a fresh PASS at the same head supersedes an older parked report. @@ -69,7 +97,7 @@ export function recordOnlyAt(reports, head) { } // The decision, without the network: `result` is merge-gate's `judge` output for this head. -export function guard({ pr, head, labels, body, mechanicalRelease, fork, result, reports }) { +export function guard({ pr, head, labels, body, mechanicalRelease, fork, draft, merged, result, reports, policy }) { const quiet = recordOnlyAt(reports, result.head ?? head) const reasons = [...result.reasons] if (quiet) reasons.push('the newest conclusion at this head says "record only — do not merge"') @@ -81,6 +109,11 @@ export function guard({ pr, head, labels, body, mechanicalRelease, fork, result, if (!mechanicalRelease && !fork) { if (!ISSUE_REF.test(String(body ?? ''))) reasons.push('no issue reference in the pull request body: add "Fixes #" or "Fixes owner/repo#"') if (!labelNames(labels).includes('agent-pr')) reasons.push('missing label: agent-pr') + // #943: only a request to enter the queue. A draft is not entering it, and a merged pull request is + // never re-judged: this check runs again on every comment (enqueue-guard.yml), so without the second + // half a sentence on an old merged pull request would turn its check red over a body nobody can edit + // any more (#926, #925, #908 and others merged without these lines). + if (!draft && !merged) reasons.push(...headerReasons(body, policy)) } const ok = result.ok && !quiet && reasons.length === 0 return { @@ -90,6 +123,8 @@ export function guard({ pr, head, labels, body, mechanicalRelease, fork, result, recordOnly: Boolean(quiet), mechanicalRelease: Boolean(mechanicalRelease), fork: Boolean(fork), + draft: Boolean(draft), + merged: Boolean(merged), labels: labels ?? [], // One loud sentence: a refusal says what to do ("do not enqueue") and why, so it cannot be read // as a warning that some other step may override. @@ -126,11 +161,12 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p const labels = (pull.labels ?? []).map((l) => l.name) // #676: the association travels with the report, the way review-gate.mjs reads the same two APIs. Without // it every conclusion is refused as "not tied to this repository", so a PASS could never clear the guard. + // #943: the account type travels too, for the same reason — a bot account writes no conclusion. const reports = [ - ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, body: c.body, at: c.created_at, association: c.author_association })), + ...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 })), ...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) .filter((v) => (v.body ?? '').trim()) - .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, association: v.author_association })), + .map((v) => ({ kind: 'review', login: v.user?.login, type: v.user?.type, body: v.body, at: v.submitted_at, association: v.author_association })), ] const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) @@ -144,7 +180,7 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p // which is not ours either. Both sides are lower-cased because the repository name in the API paths is // case-insensitive: `--repo thinkflowlab/flybot` used to read our own branch as a fork and print ALLOW. const fork = String(pull.head?.repo?.full_name ?? '').toLowerCase() !== String(repo).toLowerCase() - const decision = guard({ pr, head, labels, body: pull.body, mechanicalRelease, fork, result, reports }) + const decision = guard({ pr, head, labels, body: pull.body, mechanicalRelease, fork, draft: pull.draft, merged: pull.merged, result, reports, policy }) return { decision, pull, repo, pr } } diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index fabad4c6..78d8fcab 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -12,6 +12,9 @@ const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' const HOST_FILE = 'core/src/host/turns.js' // T2: needs a PASS const TEST_FILE = 'core/test/enqueue-guard.test.js' // T1: needs none +// #943: a pull request that may enter the queue says who owns it and who reviews it, in its body. +const HEADERS = 'Owner: Dev\nReviewer: reviewer-agent\n' + const policyFile = () => { const path = join(mkdtempSync(join(tmpdir(), 'enqueue-guard-policy-')), 'review-policy.json') writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Birfy'] })) @@ -29,7 +32,7 @@ const report = ({ tip = HEAD, verdict = 'PASS', extra = [] } = {}) => const comment = (body = report(), at = '2026-10-21T10:31:00Z') => ({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' }) -const fakeGh = ({ body = 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined } = {}) => { +const fakeGh = ({ body = HEADERS + 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined, draft = false, merged = false } = {}) => { const calls = [] const gh = (...args) => { const path = args[args.length - 1] @@ -41,7 +44,7 @@ const fakeGh = ({ body = 'Fixes #602\n', comments = [], reviews = [], labels = [ if (path.includes('/files')) return files.map((f) => JSON.stringify(typeof f === 'string' ? { filename: f } : f)).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)) return JSON.stringify({ head: { sha: head, repo: { full_name: headRepo } }, user: { login: author }, body, labels: labels.map((name) => ({ name })) }) + if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: head, repo: { full_name: headRepo } }, user: { login: author }, body, labels: labels.map((name) => ({ name })), draft, merged }) // Anything else — `gh pr merge` above all — is the guard doing more than checking. throw new Error('fake gh: unexpected ' + path) } @@ -190,7 +193,7 @@ test('#718: a pull request without the agent-pr label is not enqueued', () => { }) test('#718: a Closes reference and the agent-pr label let it through', () => { - const gh = fakeGh({ body: 'Closes #714\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) + const gh = fakeGh({ body: HEADERS + 'Closes #714\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) 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.command, `gh pr merge 566 --auto --squash --match-head-commit ${HEAD}`) @@ -200,13 +203,13 @@ test('#718: a Closes reference and the agent-pr label let it through', () => { // `Fixes ThinkFlowLab/FlyBot#801`. The bare spelling there points at a number the fork does not have, so // the guard has to accept the qualified one or every fork pull request is refused at the last step. test('a cross-repository Fixes reference is an issue reference', () => { - const gh = fakeGh({ body: 'Fixes ThinkFlowLab/FlyBot#801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) + const gh = fakeGh({ body: HEADERS + 'Fixes ThinkFlowLab/FlyBot#801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) 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('; ')) }) test('the bare Fixes # spelling still counts next to the cross-repository one', () => { - const gh = fakeGh({ body: 'Fixes #801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) + const gh = fakeGh({ body: HEADERS + 'Fixes #801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) 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('; ')) }) @@ -214,7 +217,7 @@ test('the bare Fixes # spelling still counts next to the cross-repository one // The space after the keyword was optional before the cross-repository form existed and stays optional: // widening the pattern must not quietly drop a spelling that used to count. test('a Fixes reference with no space before the hash still counts', () => { - const gh = fakeGh({ body: 'Fixes#801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) + const gh = fakeGh({ body: HEADERS + 'Fixes#801\n\nthe detail', labels: ['agent-pr'], files: [HOST_FILE], comments: [comment()] }) 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('; ')) }) @@ -327,3 +330,94 @@ test('#925: a selfReview login with admin on someone else\'s pull request is acc assert.equal(r.verdict.by, 'Birfy') assert.ok(gh.calls.includes('repos/ThinkFlowLab/FlyBot/collaborators/Birfy/permission')) }) + +// #943, part 1: the two lines the pipeline has been described with everywhere, read before the queue. +// #940 went from opened to merged in about seven minutes with the largest diff of the day and its body +// carried neither line, so the convention was a sentence nothing checked. Both lines present is not +// enough on its own — the Reviewer named has to be someone who may write a conclusion at all. +test('#943: a body with no Owner: line is refused, and the refusal names that line', () => { + const r = run({ body: 'Reviewer: reviewer-agent\nFixes #943\n', comments: [comment()] }) + assert.equal(r.ok, false) + assert.ok(r.reasons.includes('missing line: Owner:'), r.reasons.join('; ')) + assert.equal(r.reasons.some((reason) => /missing line: Reviewer:/.test(reason)), false, 'only the line that is missing is named') +}) + +test('#943: a body with no Reviewer: line is refused, and the refusal names that line', () => { + const r = run({ body: 'Owner: Dev\nFixes #943\n', comments: [comment()] }) + assert.equal(r.ok, false) + assert.ok(r.reasons.includes('missing line: Reviewer:'), r.reasons.join('; ')) + assert.equal(r.reasons.some((reason) => /missing line: Owner:/.test(reason)), false, 'only the line that is missing is named') +}) + +test('#943: a Reviewer: naming a login outside `reviewers` is refused, and the refusal names the login', () => { + const r = run({ body: 'Owner: Dev\nReviewer: someone-else\nFixes #943\n', comments: [comment()] }) + assert.equal(r.ok, false) + assert.ok(r.reasons.includes('Reviewer names "someone-else", who is not in reviewers'), r.reasons.join('; ')) +}) + +test('#943: an @ and any case are fine on a login `reviewers` lists', () => { + const r = run({ body: 'Owner: Dev\nReviewer: @Reviewer-Agent, BIRFY\nFixes #943\n', comments: [comment()] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) + +// #941's shape: the body starts with the agent's own HTML comment, so the lines sit on the 4th and 5th +// line, not on the first two. Reading only the first two would refuse every agent pull request. +test('#943: the lines are found in the first 20 lines, not only the first two', () => { + const r = run({ body: '\n\n' + HEADERS + 'Fixes #943\n', comments: [comment()] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) + +test('#943: lines after the first 20 do not count', () => { + const far = 'Fixes #943\n' + 'filler\n'.repeat(19) + HEADERS + const r = run({ body: far, comments: [comment()] }) + assert.equal(r.ok, false) + assert.ok(r.reasons.includes('missing line: Owner:'), r.reasons.join('; ')) + assert.ok(r.reasons.includes('missing line: Reviewer:'), r.reasons.join('; ')) +}) + +// A draft is not entering the queue, and history is never re-read: the check runs on the enqueue request +// for a live head only, so #940's merged record stays exactly as it is. +test('#943: a draft is not refused by the body check', () => { + const r = run({ body: 'Fixes #943\n', draft: true, comments: [comment()] }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) + +// #943, part 2: a GitHub bot account writes no conclusion. The shared account every Bot writes as +// (`Birfy`) is a `User` and keeps counting, which is why this is a rule about account type and not a +// login list — a list containing `Birfy` would void the whole pipeline. +test('#943: a Verdict: written by a bot account is not a conclusion here either', () => { + const bot = { user: { login: 'cursor[bot]', type: 'Bot' }, body: report(), created_at: '2026-10-24T07:40:42Z', author_association: 'NONE' } + const shared = { user: { login: 'Birfy', type: 'User' }, body: report().replace('reviewer-agent', 'Birfy'), created_at: '2026-10-24T07:45:10Z', author_association: 'MEMBER' } + + const alone = run({ comments: [bot] }) + assert.equal(alone.ok, false) + assert.match(alone.reasons.join('; '), /cursor\[bot\].*bot account/) + assert.equal(alone.verdict.judged.find((e) => e.login === 'cursor[bot]').counts, false) + + const both = run({ comments: [bot, shared] }) + assert.equal(both.ok, true, both.reasons.join('; ')) + assert.equal(both.verdict.by, 'Birfy') +}) + +test('#943: a Reviewer: line with no login names nobody, so it is the missing line', () => { + const r = run({ body: 'Owner: Dev\nReviewer:\nFixes #943\n', comments: [comment()] }) + assert.equal(r.ok, false) + assert.ok(r.reasons.includes('missing line: Reviewer:'), r.reasons.join('; ')) +}) + +// #943 INC at 87b49961: this check runs again on every comment (enqueue-guard.yml, `issue_comment`), +// so "a merged pull request is never re-judged" was only true of the code that never read `merged`. +// A sentence on #926/#925/#908 — merged without these lines, their bodies uneditable now — turned +// their check red. A merged pull request is history; only a live request is judged. +test('#943 INC: a merged pull request is not re-judged when a comment arrives', () => { + const body = 'Fixes #602\n\nthe detail' // no Owner:/Reviewer:, the shape #926/#925/#908 merged with + const merged = run({ body, merged: true, comments: [comment()] }) + assert.equal(merged.ok, true, merged.reasons.join('; ')) + assert.equal(merged.reasons.some((reason) => /missing line:/.test(reason)), false) + // The negative control: the same body on an open, non-draft pull request is still refused, so the + // exemption is `merged` and not a missing fixture field. + const live = run({ body, comments: [comment()] }) + assert.equal(live.ok, false) + assert.ok(live.reasons.includes('missing line: Owner:'), live.reasons.join('; ')) + assert.ok(live.reasons.includes('missing line: Reviewer:'), live.reasons.join('; ')) +}) diff --git a/.github/scripts/gate-check.test.mjs b/.github/scripts/gate-check.test.mjs index 00a41fbf..0637cd5c 100644 --- a/.github/scripts/gate-check.test.mjs +++ b/.github/scripts/gate-check.test.mjs @@ -13,6 +13,9 @@ const HOST_FILE = 'core/src/host/turns.js' // T2: needs a PASS const TEST_FILE = 'core/test/gate-check.test.js' // T1: needs none const REPO = 'Birfy/FlyBot' +// #943: a live, non-draft pull request carries the two lines that say who owns and reviews it. +const HEADERS = 'Owner: Dev\nReviewer: reviewer-agent\n' + const policyFile = () => { const path = join(mkdtempSync(join(tmpdir(), 'gate-check-policy-')), 'review-policy.json') writeFileSync(path, JSON.stringify({ reviewers: ['reviewer-agent', 'Birfy'], selfReview: ['Birfy'], writers: ['reviewer-agent', 'Reviewer', 'Birfy'] })) @@ -27,7 +30,7 @@ const comment = (body = report(), at = '2026-10-23T07:00:00Z') => // A repository with one pull request and the check runs written on it so far. `gh` answers both call // shapes the scripts use: variadic reads (`gh('api', …, path)`) and writes (`gh([args], json)`). -const world = ({ head = HEAD, labels = ['agent-pr'], body = 'Fixes #812\n', comments = [], files = [HOST_FILE], author = 'opener', headRepo = REPO } = {}) => { +const world = ({ head = HEAD, labels = ['agent-pr'], body = HEADERS + 'Fixes #812\n', comments = [], files = [HOST_FILE], author = 'opener', headRepo = REPO } = {}) => { const state = { head, labels, body, comments, files, author, headRepo, runs: [], writes: [], nextId: 100 } const pull = () => ({ head: { sha: state.head, repo: { full_name: state.headRepo } }, user: { login: state.author }, body: state.body, labels: state.labels.map((name) => ({ name })) }) const handle = (args, input) => { @@ -183,7 +186,7 @@ test('#812: a comment arriving after a refusal PATCHes the same run green', () = }) test('#812: a T0/T1 head is green without a PASS, and the summary says why', () => { - const state = world({ body: 'Fixes #812\n', files: [TEST_FILE], comments: [] }) + const state = world({ body: HEADERS + 'Fixes #812\n', files: [TEST_FILE], comments: [] }) const r = run(state) assert.equal(r.applied.conclusion, 'success') assert.equal(state.runs[0].conclusion, 'success') diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 116dd072..fa8d6515 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -468,3 +468,16 @@ test('#925: a selfReview login with admin on someone else\'s pull request lets t assert.equal(allowed.ok, true, allowed.reasons.join('; ')) assert.equal(allowed.verdict.by, 'hsliuustc0106') }) + +// #943, part 2: the queue entry reads the same `decide`, so a bot account's `Verdict:` refuses the entry +// and a PASS from the shared account at the same tip lets it through. +test('#943: a bot account\'s Verdict: does not let a queue entry through', () => { + const bot = { login: 'cursor[bot]', body: report(), at: '2026-10-24T07:40:42Z', kind: 'comment', association: 'NONE', type: 'Bot' } + const refused = judgeAt({ reports: [bot] }) + assert.equal(refused.ok, false) + assert.match(refused.reasons.join('; '), /bot account/) + + const shared = { login: 'Birfy', body: report(), at: '2026-10-24T07:45:10Z', kind: 'comment', association: 'MEMBER', type: 'User' } + const r = judge({ labels: [], head: HEAD, author: 'opener', reports: [bot, shared], policy: { ...policy, reviewers: ['Birfy'], selfReview: ['Birfy'] }, expectedHead: HEAD }) + assert.equal(r.ok, true, r.reasons.join('; ')) +}) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 6bfa8961..455ec338 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -183,6 +183,13 @@ function ranCommands(raw, lines, at, inline) { const login = (r) => String(r.login ?? '').toLowerCase() const where = (r) => `${r.kind === 'review' ? 'review' : 'comment'} by ${r.login}` +// #943: a GitHub bot account writes no conclusion, whatever its body says. `reviewers` cannot express +// this — every Bot writes as the shared `Birfy` account, which is a `User`, so a login list that refused +// bot accounts would void the whole pipeline — so the test is the account type, plus the login shape the +// API has always used for them (`cursor[bot]`). At #940's tip such a `Verdict:` sat beside the +// Reviewer's; what voids it is the real Reviewer's `Correction from Reviewer:`, not a second read here. +const isBotAccount = (r) => String(r.type ?? '').toLowerCase() === 'bot' || /\[bot\]$/i.test(login(r)) +const botKind = (r) => (String(r.type ?? '').toLowerCase() === 'bot' ? 'account type Bot' : 'login ends with [bot]') // reports: [{ login, body, at, kind, id }] from the comments and review bodies of one PR. export function decide({ head, author, reports, policy, admins } = {}) { @@ -207,6 +214,13 @@ export function decide({ head, author, reports, policy, admins } = {}) { continue } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion + // #943: refused before the association test, with its own reason: a bot account is not a + // conclusion for a reason unlisted logins cannot explain, and the log has to say which defect fired. + if (isBotAccount(r)) { + entry.why = `${where(r)}: written by a bot account (${botKind(r)}), so it is not a conclusion` + reasons.push(entry.why) + continue + } const association = String(r.association ?? '').toUpperCase() // #928 (owner decision 2026-10-24): a login the policy lists in `selfReview` may be a CONTRIBUTOR // and still count, but only on its own pull request. That is one exception for one person, not a @@ -302,10 +316,10 @@ function main() { 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, body: c.body, at: c.created_at, association: c.author_association, url: c.html_url })), + ...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, body: v.body, at: v.submitted_at, association: v.author_association, url: v.html_url })), + .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 policy = loadPolicy(policyPath) diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index 2737784b..fca836ed 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -666,3 +666,39 @@ test('#928 pair: the same report counts once the API reports admin, and the look assert.deepEqual(own, []) assert.deepEqual(ownCalls, []) }) + +// #943, part 2: the gate does not read the writer's account type today, so a bot account's `Verdict:` is +// a conclusion — `cursor[bot]` wrote one at #940's tip beside the Reviewer's. Membership of `reviewers` +// cannot express the rule: `Birfy` is the one account every Bot writes as, so a login list that refused +// bot accounts would refuse every conclusion. The rule is the account type, not the name. +const botReport = (login, { tip = HEAD, verdict = 'PASS', at = '2026-10-24T07:40:42Z', kind = 'comment', association = 'NONE', type = 'Bot' } = {}) => + ({ login, body: report({ tip, verdict }), at, kind, association, type }) + +test('#943: a GitHub bot account writes no conclusion, and the reason names the account', () => { + const r = decide({ head: HEAD, author: 'opener', reports: [botReport('cursor[bot]')], policy }) + assert.equal(r.ok, false) + assert.equal(r.by, null) + const entry = r.judged.find((e) => e.login === 'cursor[bot]') + assert.equal(entry.counts, false) + assert.match(String(entry.why ?? r.reason), /bot account/) + assert.match(String(r.reason), /cursor\[bot\]/) +}) + +test('#943: the login ending in [bot] is the same rule when the API reports no account type', () => { + for (const type of ['User', undefined]) { + const r = decide({ head: HEAD, author: 'opener', reports: [botReport('dependabot[bot]', { type })], policy: { ...policy, reviewers: ['dependabot[bot]'] } }) + assert.equal(r.ok, false, String(type)) + assert.match(String(r.judged.find((e) => e.login === 'dependabot[bot]').why), /bot account/) + } +}) + +test('#943: at one tip the bot report does not count and the shared account\'s report does (#940)', () => { + const cursor = botReport('cursor[bot]') + const birfy = { login: 'Birfy', body: report({ writer: 'Reviewer' }), at: '2026-10-24T07:45:10Z', kind: 'comment', association: 'MEMBER', type: 'User' } + const r = decide({ head: HEAD, author: 'opener', reports: [cursor, birfy], policy: { ...policy, reviewers: ['Birfy'], selfReview: ['Birfy'] } }) + assert.equal(r.ok, true, r.reason) + assert.equal(r.by, 'Birfy') + const entry = r.judged.find((e) => e.login === 'cursor[bot]') + assert.equal(entry.counts, false) + assert.match(String(entry.why), /bot account/) +}) diff --git a/AGENTS.md b/AGENTS.md index 01e121bf..4b2a1abb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -167,7 +167,7 @@ What this buys is presence and a name from a closed list, not truth: with one sh - **No implementer writes the Verdict.** On the API side self-review cannot be told apart mechanically — every pull request is opened by the shared `Birfy` account, and `selfReview: ["Birfy"]` in [`review-policy.json`](.github/agent/review-policy.json) makes a PASS always look like Birfy judging Birfy — so what the team holds is the role split: the implementer Bot does not write the Verdict, the Bot that writes the Verdict neither changes the tip nor re-judges its own verdict, and `selfReview` stays as it is because clearing it would leave every pull request without a conclusion. - **`hold` is human-only.** Only a person sets the label and only a person lifts it; automation refuses a held pull request and never removes the label itself. That is the one human input left on the merge path. - **The label awaiting-approval is retired.** A PASS at the tip goes straight to the queue through the enqueue guard (#687); no pull request waits for an approval click and the label is not set again. -- **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #` / `Closes #`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. Two exemptions: a mechanical release PR, which carries neither by design, and a **fork** (`pull.head.repo.full_name` is not this repository), whose head we cannot push to and where the contributor keeps their own workflow. +- **A pull request enters the queue with its issue and its `agent-pr` label.** On a branch this repository owns, the enqueue guard also refuses one whose body names no issue (`Fixes #` / `Closes #`) and one without the `agent-pr` label (#718), so "one issue, one pull request" is checked rather than remembered. It refuses a **live, non-draft** request whose body carries no `Owner:` line or no `Reviewer:` line in its first 20 lines, or names a `Reviewer:` who is not in `reviewers` — the refusal names the missing line or the login (#943; #940 merged in about seven minutes with the largest diff of the day and neither line). A **merged** pull request is never re-judged: this check also runs on every comment, and history is not a request. Two exemptions: a mechanical release PR, which carries neither by design, and a **fork** (`pull.head.repo.full_name` is not this repository), whose head we cannot push to and where the contributor keeps their own workflow. **On the code fork `Birfy/FlyBot`, where this team's pull requests actually merge, the gate is a ruleset, not a queue** (#812). Once the ruleset from `--apply` is on that repository, its `main` requires two checks, `tests-ok` and `enqueue-guard`, with `bypass_actors: []`, so `PUT /pulls//merge` is refused — 405/422 — while the head carries a `hold`, no PASS naming that exact tip, a PASS that lists no `Ran:`, no `agent-pr`, or a body naming no issue. The required check is written by [`enqueue-guard.yml`](.github/workflows/enqueue-guard.yml), from code taken from `main` and never from the pull request, on every event that can change the answer for a fixed head: a push, a label added or removed, a comment, a review body, or a manual run. It calls [`enqueue-guard.mjs`](.github/scripts/enqueue-guard.mjs)'s own decision — the same function that prints the command — so the check and the command cannot disagree; a new commit is a new head and a new check run, and a green check never carries over. The ruleset is [`merge-gate-ruleset.json`](.github/agent/merge-gate-ruleset.json): `node .github/scripts/gate-ruleset.mjs --check` reads the live state and writes nothing, while `--apply` and `--rollback` change repository protection and are the owner's call, not a Bot's. The order is not optional: the check must already report on the open pull requests **before** the ruleset goes on, or every one of them reads "Expected — waiting for status" and cannot merge. @@ -180,7 +180,7 @@ Four rules decide what a queue entry must pass. The mechanical judge runs where A review's `state` is not read at all: approvals are not a gate any more (user decision 2026-10-22), and the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC — the INC on #610 was a comment rather than a review. -**One conclusion per tip.** A tip carries at most one conclusion: one `Verdict:` block naming one SHA. Two conclusions at the same tip is an incident, not a tie to be broken — nobody re-verdicts to win, the Reviewer that wrote the later one retracts it in a comment, and the pull request waits. A correction reads `Correction from : …` and carries no `Verdict:` line, so it never competes with the standing conclusion; the replacement PASS for a missing `Ran:` ([above](#gates)) is the only other body allowed at one tip. +**One conclusion per tip.** A tip carries at most one conclusion: one `Verdict:` block naming one SHA. Two conclusions at the same tip is an incident, not a tie to be broken — nobody re-verdicts to win, the Reviewer that wrote the later one retracts it in a comment, and the pull request waits. A correction reads `Correction from : …` and carries no `Verdict:` line, so it never competes with the standing conclusion; the replacement PASS for a missing `Ran:` ([above](#gates)) is the only other body allowed at one tip. **A GitHub bot account's `Verdict:` is never a conclusion** — `user.type` is `Bot`, or the login ends `[bot]` — whatever its body says (#943, `cursor[bot]` at #940's tip); the rule is the account type and not a login list, because every Bot writes as the shared `Birfy` account and a list containing it would void the whole pipeline, so the real Reviewer voids such a report at that tip with `Correction from Reviewer:`. **Judged on the merge tree, never on the pull request page.** The page answers "does this commit pass alone"; the queue answers "does this commit pass merged onto the current `main`", and the second one is the merge decision. #759 was green on every check, was kicked out of the queue twice, and went red on `main` at `core/test/follow-through.test.js:264`. A red queue entry that names files you did not touch is the stale base of [step 2](#pipeline-contract), not bad luck: fix the base, do not re-enqueue blindly. diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 4e7675bb..359cd047 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -149,6 +149,11 @@ What counts as a reviewed tip, all of it: counting; that report is then listed as "not counted", with the sha it named. 5. The **newest** report that names this tip decides, so a later FAIL at the same tip vetoes an earlier PASS, and a later PASS at the same tip replaces a FAIL. +6. **A GitHub bot account writes no conclusion** (#943): `user.type` is `Bot`, or the login ends `[bot]`, and the + report joins `judged` with `counts: false` and a reason naming the account, whatever its body says — the real + Reviewer voids it at that tip with `Correction from Reviewer:`. The rule is the account type and not a login + list: every Bot writes as the shared `Birfy` account, so a list containing it would void the whole pipeline. + `cursor[bot]`'s report at #940's tip is the case (`Correction from Reviewer`, [comment `6381646389`](https://github.com/ThinkFlowLab/FlyBot/pull/940#issuecomment-6381646389)). **Attribution (#627).** Every state-changing write says who wrote it: `Written-by: ` is its first line, and a conclusion puts it on the second, under the fixed verdict line. `decide` refuses a report @@ -210,7 +215,9 @@ entry — are in [`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates). This s **How to merge.** A direct `gh pr merge --squash` is refused with `! The merge strategy for main is set by the merge queue`; that is not a failure. Run `node .github/scripts/enqueue-guard.mjs --pr ` first — it re-reads the live head and exits 1 on a `hold`, on a newest conclusion that is not a PASS, and on a report marked "record only — do not merge", so a -PASS is never a queue permit on its own (#602); on success it prints the command and never runs it. Enqueue with +PASS is never a queue permit on its own (#602); on a live, non-draft pull request of ours it also refuses a body with no +`Owner:`/`Reviewer:` line in its first 20 lines (or a `Reviewer:` outside `reviewers`), naming the missing line or the +login (#943) — a merged pull request is not re-judged, because this check also runs on every comment; on success it prints the command and never runs it. Enqueue with `gh pr merge --auto --squash --match-head-commit ` (gh has no `--merge-queue` flag). gh may still print the strategy line, or `! Pull request # is already queued to merge`; the PR is then in the queue (`gh api graphql` → `pullRequest.isInMergeQueue`). The queue, not the agent, performs the squash merge