Skip to content

Read the Owner/Reviewer lines before the queue, and refuse a bot account's Verdict #944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Birfy merged 1 commit into from
Oct 24, 2026
Merged

Read the Owner/Reviewer lines before the queue, and refuse a bot account's Verdict #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 40 additions & 4 deletions .github/scripts/enqueue-guard.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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"')
Expand All @@ -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 #<n>" or "Fixes owner/repo#<n>"')
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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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`)

Expand All @@ -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 }
}

Expand Down
106 changes: 100 additions & 6 deletions .github/scripts/enqueue-guard.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] }))
Expand All @@ -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]
Expand All @@ -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)
}
Expand Down Expand Up @@ -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}`)
Expand All @@ -200,21 +203,21 @@ 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 #<n> 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('; '))
})

// 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('; '))
})
Expand Down Expand Up @@ -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: '<!-- CURSOR_AGENT_PR_BODY_BEGIN -->\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('; '))
})
7 changes: 5 additions & 2 deletions .github/scripts/gate-check.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] }))
Expand All @@ -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) => {
Expand Down Expand Up @@ -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')
Expand Down
Loading
Loading