Skip to content
4 changes: 3 additions & 1 deletion .github/scripts/enqueue-guard.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,9 @@ export function decidePull({ gh = realGh, env = process.env, repo: repoOption, p

// The live head is judged, never a cached one: a push that moved the tip voids the PASS the caller
// believes it has, and `--sha` lets a caller insist on the head it read a moment ago.
const result = judge({ labels, files, head, author: pull.user?.login, reports, policy: loadPolicy(policyPath), expectedHead: expectedHead ?? arg('--sha'), mode: 'queue' })
// `judge` asks `adminsFor` when this tier needs a PASS.
const policy = loadPolicy(policyPath)
const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, gh, repo, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue' })
const mechanicalRelease = isMechanicalRelease(files.map((f) => f.filename))
// The branch this repository owns, not a contributor's: `head.repo` is null when the fork was deleted,
// which is not ours either. Both sides are lower-cased because the repository name in the API paths is
Expand Down
65 changes: 64 additions & 1 deletion .github/scripts/enqueue-guard.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@ 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' } = {}) => {
const fakeGh = ({ body = 'Fixes #602\n', comments = [], reviews = [], labels = ['agent-pr'], files = [], head = HEAD, author = 'opener', headRepo = 'ThinkFlowLab/FlyBot', permission = undefined } = {}) => {
const calls = []
const gh = (...args) => {
const path = args[args.length - 1]
calls.push(path)
if (path.includes('/collaborators/') && path.includes('/permission')) {
if (permission == null || permission === 'throw') throw new Error('fake gh: permission ' + path)
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')
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')
Expand Down Expand Up @@ -258,9 +262,68 @@ test('#718: a fork stays exempt under any --repo spelling', () => {

// The guard is only in the path if the instructions that tell an agent to enqueue name it; otherwise
// the next agent runs `gh pr merge` from memory and #602 is back.
const contributor = (login, at = '2026-10-24T03:42:31Z') =>
({ user: { login }, body: report(), submitted_at: at, author_association: 'CONTRIBUTOR' })

test('#925: a MEMBER PASS is accepted and does not ask the permission API', () => {
const gh = fakeGh({ 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(gh.calls.some((path) => path.includes('/permission')), false)
})

test('#925: a CONTRIBUTOR who is not in selfReview and is not a repo admin is refused', () => {
for (const permission of ['write', 'throw']) {
const r = run({ reviews: [contributor('reviewer-agent')], permission })
assert.equal(r.ok, false, permission)
assert.match(r.reasons.join('; '), /author association CONTRIBUTOR/)
}
})

test('#925: a CONTRIBUTOR who has admin on the repo is accepted', () => {
const r = run({ reviews: [contributor('reviewer-agent')], permission: 'admin' })
assert.equal(r.ok, true, r.reasons.join('; '))
assert.equal(r.verdict.by, 'reviewer-agent')
})

test('#925: a selfReview login stamped CONTRIBUTOR is accepted on its own pull request without an admin lookup', () => {
const gh = fakeGh({
files: [HOST_FILE],
author: 'Birfy',
reviews: [contributor('Birfy')],
permission: 'throw',
})
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.verdict.by, 'Birfy')
assert.equal(gh.calls.some((path) => path.includes('/permission')), false)
})

test('#925: a selfReview login judging someone else\'s pull request is still refused (#931)', () => {
const r = run({ author: 'opener', reviews: [contributor('Birfy')], permission: 'throw' })
assert.equal(r.ok, false)
assert.match(r.reasons.join('; '), /author association CONTRIBUTOR/)
})

test('#925: a tier that needs no PASS does not ask the permission API', () => {
const gh = fakeGh({ files: [TEST_FILE], reviews: [contributor('reviewer-agent')], permission: 'throw' })
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.exempt, true)
assert.equal(gh.calls.some((path) => path.includes('/permission')), false)
})

test('#602: the instructions that enqueue point at the guard', () => {
const agents = readFileSync(new URL('../../AGENTS.md', import.meta.url), 'utf8')
const fixIssue = readFileSync(new URL('../../.claude/skills/fix-issue/SKILL.md', import.meta.url), 'utf8')
const plumbing = readFileSync(new URL('../../docs/autonomous-dev.md', import.meta.url), 'utf8')
for (const text of [agents, fixIssue, plumbing]) assert.match(text, /enqueue-guard\.mjs/)
})

test('#925: a selfReview login with admin on someone else\'s pull request is accepted, and the lookup is asked once', () => {
const gh = fakeGh({ files: [HOST_FILE], author: 'opener', reviews: [contributor('Birfy')], permission: 'admin' })
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.verdict.by, 'Birfy')
assert.ok(gh.calls.includes('repos/ThinkFlowLab/FlyBot/collaborators/Birfy/permission'))
})
12 changes: 8 additions & 4 deletions .github/scripts/merge-gate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import { appendFileSync } from 'node:fs'
import { execFileSync } from 'node:child_process'
import { fileURLToPath } from 'node:url'
import { decide, loadPolicy } from './review-gate.mjs'
import { adminsFor, decide, loadPolicy } from './review-gate.mjs'
import { createGate, loadRules } from './risk-gate.mjs'

const SHORT = 12
Expand Down Expand Up @@ -99,7 +99,7 @@ const short = (s) => String(s ?? '').slice(0, SHORT)
// expectedHead: optional, a head the caller insists on (`--sha` on a hand run); a different live head
// refuses. The queue's base sha is never a head: a merge_group entry passes none here (see
// prFromHeadRef). reports: its comments and review bodies, oldest first (review-gate main()).
export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue' }) {
export function judge({ labels, files, head, author, reports, policy, expectedHead, mode = 'queue', admins, gh, repo } = {}) {
const reasons = []
const target = String(head ?? '').toLowerCase()
if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] }
Expand All @@ -116,7 +116,10 @@ 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 })
const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } : decide({ head: target, author, reports, policy })
// 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, admins: admins ?? adminsFor({ gh, repo, reports, policy, author }) })
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
Expand Down Expand Up @@ -189,7 +192,8 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO
// The whole file record, not just the name: the T0/T1 exemption is computed from the diff by the
// base copy of risk-gate.mjs (see exemptFromPass).
const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`)
const result = judge({ labels, files, head, author, reports, policy: loadPolicy(policyPath), expectedHead: arg('--sha'), mode })
const policy = loadPolicy(policyPath)
const result = judge({ labels, files, head, author, reports, policy, gh, repo, expectedHead: arg('--sha'), mode })
// The entry's own commit and its base are logged, never judged: neither is a head a Verifier names.
const base = env.BASE_SHA || fromRef?.base
const entry = env.ENTRY_SHA || (fromRef ? env.GITHUB_SHA : '')
Expand Down
56 changes: 56 additions & 0 deletions .github/scripts/merge-gate.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,33 @@ test('#637: a verdict Chief posts for the Reviewer parses like the Reviewer wrot
assert.equal(result.verdict.by, 'reviewer-agent')
})

test('#925: a CONTRIBUTOR review by a repo admin passes the queue entry, and a failed lookup does not', () => {
const body = firstLine('PASS')
const pull = JSON.stringify({ head: { sha: HEAD }, user: { login: 'opener' }, labels: [] })
const review = JSON.stringify({ user: { login: 'reviewer-agent' }, body, submitted_at: '2026-10-24T03:42:31Z', author_association: 'CONTRIBUTOR' })
const gh = (permission) => (...args) => {
const path = args[args.length - 1]
if (path.includes('/permission')) {
if (permission !== 'admin') throw new Error('404')
return JSON.stringify({ permission: 'admin', role_name: 'admin' })
}
if (path.includes('/files')) return JSON.stringify({ filename: 'core/src/host/turns.js' })
if (path.includes('/comments')) return ''
if (path.includes('/reviews')) return review
if (/\/pulls\/\d+$/.test(path)) return pull
throw new Error('fake gh: unexpected ' + path)
}
const policyPath = join(mkdtempSync(join(tmpdir(), 'merge-gate-policy-')), 'review-policy.json')
writeFileSync(policyPath, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] }))
const env = { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policyPath, GH_REPO: 'ThinkFlowLab/FlyBot', PR: '925' }
const allowed = main({ gh: gh('admin'), env, repo: 'ThinkFlowLab/FlyBot', pr: 925 })
assert.equal(allowed.ok, true, allowed.reasons.join('; '))
assert.equal(allowed.verdict.by, 'reviewer-agent')
const refused = main({ gh: gh('none'), env, repo: 'ThinkFlowLab/FlyBot', pr: 925 })
assert.equal(refused.ok, false)
assert.match(refused.reasons.join('; '), /author association CONTRIBUTOR/)
})

test('#637: a body that does not start with the verdict is no conclusion, so the entry is refused', () => {
const prefixed = ['Verdict (written by Reviewer, posted by Chief)', '', firstLine('PASS')].join('\n')
const result = runGate({ comments: [apiComment(prefixed, '2026-10-21T12:49:00Z')] })
Expand Down Expand Up @@ -412,3 +439,32 @@ test('#660-#663: the fenced Ran block is evidence only with a $ command, and a T
assert.equal(r.ok, false)
assert.match(r.reasons.join('; '), /lists no command it ran/)
})

test('#925: admin does not skip the own-author rule, and selfReview does not carry to someone else\'s pull request', () => {
const own = judge({ labels: [], head: HEAD, author: 'reviewer-agent', reports: [say(report(), '2026-10-24T03:42:31Z', 'comment')], policy, admins: ['reviewer-agent'] })
assert.equal(own.ok, false, 'admin does not skip the own-author rule')
assert.match(own.reasons.join('; '), /own author is not a reviewer/)

const listed = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer'] }
const other = judge({
labels: [],
head: HEAD,
author: 'opener',
reports: [{ login: 'hsliuustc0106', body: report(), at: '2026-10-24T03:42:31Z', kind: 'comment', association: 'CONTRIBUTOR' }],
policy: listed,
admins: [],
})
assert.equal(other.ok, false, 'a selfReview login judging someone else\'s pull request is not exempt (#931)')
assert.match(other.reasons.join('; '), /author association CONTRIBUTOR/)
})

test('#925: a selfReview login with admin on someone else\'s pull request lets the entry through', () => {
const listed = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer'] }
const other = { login: 'hsliuustc0106', body: report(), at: '2026-10-24T03:42:31Z', kind: 'comment', association: 'CONTRIBUTOR' }
const refused = judge({ labels: [], head: HEAD, author: 'opener', reports: [other], policy: listed, admins: [] })
assert.equal(refused.ok, false)
assert.match(refused.reasons.join('; '), /author association CONTRIBUTOR/)
const allowed = judge({ labels: [], head: HEAD, author: 'opener', reports: [other], policy: listed, admins: ['hsliuustc0106'] })
assert.equal(allowed.ok, true, allowed.reasons.join('; '))
assert.equal(allowed.verdict.by, 'hsliuustc0106')
})
59 changes: 56 additions & 3 deletions .github/scripts/review-gate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
// - it is not the pull request author's own report, unless the author is listed in the policy's
// `selfReview` (owner decision 2026-10-19: the owner's Bot/agent, acting as Birfy, may PASS
// its own pull requests; any other author still needs someone else);
// - the login is tied to this repository (#676, #925): `author_association` is OWNER, MEMBER or
// COLLABORATOR, the login is this pull request's own author listed in `selfReview`, or
// `adminsFor` found repo admin. A CONTRIBUTOR who is none of those is not a conclusion;
// - it carries the Verifier block review-pr already writes, with a `Tip SHA:` that is the
// head being judged and a `Verdict:`;
// - it names its writer in the header: `Written-by: <Bot>` (the older prose `Written by: …` reads
Expand All @@ -35,6 +38,51 @@ const SHORT = 12
// could hold a merge. A comment from anyone else is not a conclusion: it can neither pass nor block.
const ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR'])

// A conclusion GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR), from a login
// in `reviewers`, asked once via the collaborator permission API. Anyone else can never pass the
// reviewers check, so they are not looked up. A `selfReview` login is already qualified on its own
// pull request (#928), so only that shape is skipped; on anyone else's pull request the permission
// API is the one thing that can qualify it. Skipping every `selfReview` login left this rule dead —
// today every reviewer is also in `selfReview`. A comment with no Tip SHA and Verdict is not a
// conclusion. Metadata read on the token
// the job already has. Fail closed: no repo, no `gh`, a 403/404, or a body that is not
// the permission object is not admin. `permission: admin` or `permissions.admin` counts.
export function adminsFor({ gh, repo, reports, policy, author } = {}) {
if (!repo || typeof gh !== 'function') return []
const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase()))
const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase()))
const authorLogin = String(author ?? '').toLowerCase()
const admins = []
const seen = new Set()
for (const r of reports ?? []) {
if (ASSOCIATIONS.has(String(r.association ?? '').toUpperCase())) continue
const name = String(r.login ?? '').trim()
const key = name.toLowerCase()
if (!name || !reviewers.has(key)) continue
if (selfReview.has(key) && key === authorLogin) continue
const parsed = parseReport(r.body)
if (!parsed.tip || !parsed.verdict) continue
if (seen.has(key)) continue
seen.add(key)
let text
try {
text = gh('api', `repos/${repo}/collaborators/${encodeURIComponent(name)}/permission`)
} catch {
// Fail closed. A 403 or 404 is not admin.
continue
}
let data
try {
data = JSON.parse(text)
} catch {
// A body that is not the permission object is not admin.
continue
}
if (String(data?.permission ?? '').toLowerCase() === 'admin' || data?.permissions?.admin === true) admins.push(name)
}
return admins
}

// The last Verifier block in one body: a comment may hold an earlier review that a later
// one replaced, and only the last block is the verdict the reviewer stands behind.
export function parseReport(body) {
Expand Down Expand Up @@ -137,12 +185,13 @@ const login = (r) => String(r.login ?? '').toLowerCase()
const where = (r) => `${r.kind === 'review' ? 'review' : 'comment'} by ${r.login}`

// reports: [{ login, body, at, kind, id }] from the comments and review bodies of one PR.
export function decide({ head, author, reports, policy }) {
export function decide({ head, author, reports, policy, admins } = {}) {
const headSha = String(head ?? '').toLowerCase()
const authorLogin = String(author ?? '').toLowerCase()
const reviewers = new Set((policy?.reviewers ?? []).map((l) => String(l).toLowerCase()))
const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase()))
const writers = new Set((policy?.writers ?? []).map((w) => String(w).toLowerCase()))
const adminSet = new Set((admins ?? []).map((l) => String(l).toLowerCase()))
const reasons = [] // reports that do not name this tip at all
const judged = []
const atTip = [] // conclusions that name this tip, valid or not, in the order the API returned them
Expand All @@ -163,8 +212,11 @@ export function decide({ head, author, reports, policy }) {
// and still count, but only on its own pull request. That is one exception for one person, not a
// second reviewers list: the same login judging someone else's pull request keeps every rule
// below, and #676 still refuses a login nobody listed.
// #936 (#925): this gate also accepts a repository admin, the one fact the association stamp gets
// wrong for this account. `admins` is what `adminsFor` resolved; it skips no check below.
const selfOnly = selfReview.has(login(r)) && login(r) === authorLogin
if (!ASSOCIATIONS.has(association) && !selfOnly) {
const isAdmin = adminSet.has(login(r))
if (!ASSOCIATIONS.has(association) && !selfOnly && !isAdmin) {
reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, so it is not a conclusion`)
continue
}
Expand Down Expand Up @@ -256,7 +308,8 @@ function main() {
.map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, association: v.author_association, url: v.html_url })),
]

const result = decide({ head, author, reports, policy: loadPolicy(policyPath) })
const policy = loadPolicy(policyPath)
const result = decide({ head, author, reports, policy, admins: adminsFor({ gh, repo, reports, policy, author }) })
console.log(`review-gate (report only): PR #${pr} tip ${String(head).slice(0, SHORT)} — ${result.ok ? `PASS by ${result.by}` : result.reason}`)
const text = render(result, { pr, head, author })
if (process.env.GITHUB_STEP_SUMMARY) appendFileSync(process.env.GITHUB_STEP_SUMMARY, text + '\n')
Expand Down
Loading
Loading