diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index e6ca324b..a2390ed1 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -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 diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 521f1f4c..fabad4c6 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -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') @@ -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')) +}) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 7c9a7c4d..14855e07 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -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 @@ -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'] } @@ -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 @@ -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 : '') diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 92c0ace1..116dd072 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -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')] }) @@ -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') +}) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index edd97b35..6bfa8961 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -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: ` (the older prose `Written by: …` reads @@ -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) { @@ -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 @@ -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 } @@ -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') diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index b7014018..2737784b 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -2,7 +2,7 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { readFileSync } from 'node:fs' -import { decide, loadPolicy, parseReport, render } from './review-gate.mjs' +import { adminsFor, decide, loadPolicy, parseReport, render } from './review-gate.mjs' const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' @@ -516,3 +516,153 @@ test('#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their ow assert.equal(elsewhere.ok, false, 'a listed self-reviewer judging someone else\'s pull request is not exempt') assert.equal(elsewhere.judged[0].counts, false) }) + +// #936 / #925: a login holding admin on this repository is stamped CONTRIBUTOR on its reviews, so the +// association gate cannot be the only word. The permission API is asked only for a Tip+Verdict +// conclusion by a login already in `reviewers`; it never widens the reviewers list, never skips the +// own-author rule, and a failed lookup is not admin. +test('#925: repo admin qualifies a CONTRIBUTOR, and does not skip the reviewers list or the own-author rule', () => { + const adminPolicy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: [], writers: ['Reviewer'] } + const pass = comment('hsliuustc0106', report(), '2026-10-18T01:00:00Z', 'review', 'CONTRIBUTOR') + + const accepted = decide({ head: HEAD, author: 'opener', reports: [pass], policy: adminPolicy, admins: ['HSLiuustc0106'] }) + assert.equal(accepted.ok, true, accepted.reason) + assert.equal(accepted.by, 'hsliuustc0106') + + const notAdmin = decide({ head: HEAD, author: 'opener', reports: [pass], policy: adminPolicy }) + assert.equal(notAdmin.ok, false) + assert.match(notAdmin.reason, /author association CONTRIBUTOR/) + + const outsider = decide({ + head: HEAD, + author: 'opener', + reports: [comment('passer-by', report(), '2026-10-18T01:00:00Z', 'review', 'CONTRIBUTOR')], + policy: adminPolicy, + admins: ['passer-by'], + }) + assert.equal(outsider.ok, false, 'admin does not skip the reviewers list') + assert.match(outsider.reason, /not a reviewer/) + + const selfPass = decide({ head: HEAD, author: 'hsliuustc0106', reports: [pass], policy: adminPolicy, admins: ['hsliuustc0106'] }) + assert.equal(selfPass.ok, false, 'admin does not skip the own-author selfReview rule') + assert.match(selfPass.reason, /own author is not a reviewer/) +}) + +test('#925: a Tip+Verdict from a login outside reviewers does not call the permission API', () => { + const calls = [] + const gh = (_cmd, path) => { + calls.push(path) + return JSON.stringify({ permission: 'admin' }) + } + const p = { reviewers: ['Reviewer-Agent'], selfReview: [], writers: ['Reviewer'] } + const body = `Verdict: PASS at ${HEAD}` + const admins = adminsFor({ + gh, + repo: 'ThinkFlowLab/FlyBot', + policy: p, + reports: [ + { login: 'passer-by', association: 'CONTRIBUTOR', body }, + { login: 'reviewer-agent', association: 'CONTRIBUTOR', body }, + ], + }) + assert.deepEqual(admins, ['reviewer-agent']) + assert.deepEqual(calls, ['repos/ThinkFlowLab/FlyBot/collaborators/reviewer-agent/permission']) +}) + +test('#925: the permission API is asked only for a listed reviewer conclusion the association and selfReview do not already qualify', () => { + const p = { reviewers: ['reviewer-agent', 'Birfy', 'hsliuustc0106', 'write-user', 'custom', 'bad-body', 'gone'], selfReview: ['Birfy'], writers: ['Reviewer'] } + const verdict = `Verdict: PASS at ${HEAD}` + const reports = [ + { login: 'reviewer-agent', association: 'MEMBER', body: verdict }, + { login: 'Birfy', association: 'CONTRIBUTOR', body: verdict }, + { login: 'chatter', association: 'CONTRIBUTOR', body: 'LGTM, ship it' }, + { login: 'tiponly', association: 'CONTRIBUTOR', body: `Tip SHA: ${HEAD}` }, + { login: 'hsliuustc0106', association: 'CONTRIBUTOR', body: verdict }, + { login: 'hsliuustc0106', association: 'NONE', body: verdict }, + { login: 'writer', association: 'CONTRIBUTOR', body: verdict }, + { login: 'broken', association: 'CONTRIBUTOR', body: verdict }, + { login: 'missing', association: 'CONTRIBUTOR', body: verdict }, + { login: 'write-user', association: 'CONTRIBUTOR', body: verdict }, + { login: 'custom', association: 'CONTRIBUTOR', body: verdict }, + { login: 'bad-body', association: 'CONTRIBUTOR', body: verdict }, + { login: 'gone', association: 'CONTRIBUTOR', body: verdict }, + { login: '', association: 'NONE', body: verdict }, + ] + const calls = [] + const gh = (_cmd, path) => { + calls.push(path) + const login = decodeURIComponent(path.split('/collaborators/')[1].split('/')[0]) + if (login === 'hsliuustc0106') return JSON.stringify({ permission: 'admin', role_name: 'admin' }) + if (login === 'write-user') return JSON.stringify({ permission: 'write', role_name: 'write', permissions: { admin: false } }) + if (login === 'custom') return JSON.stringify({ permission: 'write', permissions: { admin: true } }) + if (login === 'bad-body') return 'not-json' + throw new Error('HTTP 404') + } + // `author` decides which shape is skipped: the own-author `selfReview` carve-out (#928) needs no + // lookup, everyone else is asked. `Birfy` is in `selfReview` and is not this author, so it is asked + // and its 404 means "not admin". + assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy: p, author: 'opener' }), ['hsliuustc0106', 'custom']) + assert.deepEqual(calls, [ + 'repos/ThinkFlowLab/FlyBot/collaborators/Birfy/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/write-user/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/custom/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/bad-body/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/gone/permission', + ]) + for (const outsider of ['writer', 'broken', 'missing']) { + assert.equal(calls.some((path) => path.includes(`/collaborators/${outsider}/`)), false, outsider) + } + // Their own pull request: the `selfReview` carve-out already qualifies the report, so no lookup. + const ownCalls = [] + const own = adminsFor({ + gh: (_cmd, path) => { ownCalls.push(path); return JSON.stringify({ permission: 'admin' }) }, + repo: 'ThinkFlowLab/FlyBot', + reports: [{ login: 'Birfy', association: 'CONTRIBUTOR', body: verdict }], + policy: p, + author: 'Birfy', + }) + assert.deepEqual(own, []) + assert.deepEqual(ownCalls, []) + assert.deepEqual(adminsFor({ gh, reports, policy: p, author: 'opener' }), []) + assert.equal(calls.length, 6, 'no repo means no lookup') +}) + +test('#925: OWNER, MEMBER and COLLABORATOR still count, and a bare CONTRIBUTOR does not', () => { + for (const association of ['OWNER', 'MEMBER', 'COLLABORATOR']) { + const r = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z', 'review', association)], policy }) + assert.equal(r.ok, true, association) + } + const bare = decide({ head: HEAD, author: 'opener', reports: [comment('reviewer-agent', report(), '2026-10-18T01:00:00Z', 'review', 'CONTRIBUTOR')], policy }) + assert.equal(bare.ok, false) + assert.match(bare.reason, /author association CONTRIBUTOR/) +}) + +// The pair to #931's negative case. Same policy shape, same login in `selfReview`, same +// `CONTRIBUTOR` report on **someone else's** pull request — the only difference is the admin +// evidence `adminsFor` collected, and the comment below says so at both ends. That case stays +// `false` (a non-admin `selfReview` login has no right on another author's pull request); this one +// is `true` because repository admin is a separate qualification, not a widening of `selfReview`. +test('#928 pair: the same report counts once the API reports admin, and the lookup is asked once', () => { + const selfPolicy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer', 'hsliuustc0106'] } + const reports = [comment('hsliuustc0106', report({ writer: 'hsliuustc0106' }), '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR')] + const calls = [] + const gh = (_cmd, path) => { calls.push(path); return JSON.stringify({ permission: 'admin', role_name: 'admin' }) } + const admins = adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy: selfPolicy, author: 'opener' }) + assert.deepEqual(admins, ['hsliuustc0106'], 'a selfReview login on someone else\'s pull request is looked up') + assert.deepEqual(calls, ['repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission'], 'asked exactly once') + + const r = decide({ head: HEAD, author: 'opener', reports, policy: selfPolicy, admins }) + assert.equal(r.ok, true, r.reason) + assert.equal(r.by, 'hsliuustc0106') + + // Identical fixture, no admin evidence: this is exactly #931's negative case (asserted there, not + // restated here, so that case stays the single red when the carve-out is widened). + assert.deepEqual(adminsFor({ gh: (_cmd, path) => { calls.push(path); return JSON.stringify({ permission: 'write' }) }, repo: 'ThinkFlowLab/FlyBot', reports, policy: selfPolicy, author: 'opener' }), []) + + // Their own pull request: the `selfReview` carve-out already qualifies the report, so no lookup. + const ownCalls = [] + const own = adminsFor({ gh: (_cmd, path) => { ownCalls.push(path); return JSON.stringify({ permission: 'admin' }) }, repo: 'ThinkFlowLab/FlyBot', reports, policy: selfPolicy, author: 'hsliuustc0106' }) + assert.deepEqual(own, []) + assert.deepEqual(ownCalls, []) +}) diff --git a/AGENTS.md b/AGENTS.md index 12bceb1b..01e121bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -147,7 +147,7 @@ The owner kept one shared GitHub identity for every Bot (2026-10-21), so attribu The gate refuses what it can check, and fails closed. [`review-gate.mjs`](.github/scripts/review-gate.mjs) — and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs), which imports its `decide`, so a queue entry is judged by the same rule — refuses a report that carries **no** `Written-by:` line, and refuses one whose name is **not** in the policy's `writers` list. Two separate refusals, so the log says which defect fired. Triage names a write that carries no such line in one comment and never repairs it by editing someone else's text. -The gate reads a conclusion only from someone GitHub ties to this repository: the comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`, or it is not a conclusion and counts for nothing — otherwise any passer-by could hold a merge with one `Verdict: INC` line, because the newest conclusion decides (#676). An association the API did not send is not assumed. One exception (owner decision 2026-10-24, #928): a login the policy lists in `selfReview` may be a `CONTRIBUTOR` and still count, but **only on its own pull request** — the same login on someone else's pull request is refused like any other outsider, and every other check (tip, `Written-by:`, `Ran:`, reviewer list, newest word) is unchanged. +The gate reads a conclusion only from someone tied to this repository (#676): `author_association` `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`), the pull request's own author when that login is in `selfReview` (#928), or a login the permission API reports as admin. The permission API is for an admin GitHub did not tie to the repository — a `CONTRIBUTOR` stamp on someone else's pull request — not for the own-author carve-out. That own-pull-request PASS is already `selfOnly` (#926, #928); the lookup does not recover it. A `CONTRIBUTOR` who is none of those is not a conclusion, so a passer-by's `Verdict:` can neither pass nor hold a merge. A missing association is not assumed, and a failed permission lookup is not admin. The lookup uses the token the job already has, and only for a Tip+Verdict conclusion by a login in `reviewers` that is not already `OWNER`, `MEMBER` or `COLLABORATOR`. A `selfReview` login is skipped only on that author's own report, because the shipped policy lists every reviewer in `selfReview` and skipping the whole list would never call. Admin does not skip the reviewers list or the own-author rule. The two qualifications do not replace each other: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else’s, while admin is read only through the permission API and only for a login already in `reviewers`. What this buys is presence and a name from a closed list, not truth: with one shared identity the line is a self-report that can be left out (refused) and can name the wrong Bot (undetectable here). The write layer knowing the writer is the DSH-side half, #633. diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 3deff63d..4e7675bb 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -144,7 +144,7 @@ What counts as a reviewed tip, all of it: Bot/agent, so no independent reviewer is required. That is an exception about the shared *account*, not about the Bot: an implementer never reviews its own pull request ([`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates)). Any other author still needs someone else. A PASS does not replace green CI, and a new commit voids it. -3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. The comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`: a conclusion from anyone else counts for nothing, so it can neither pass a tip nor hold one (#676). One exception (owner decision 2026-10-24, #928): a login the policy lists in `selfReview` may be a `CONTRIBUTOR` and still count, but **only on its own pull request** — the same login judging someone else's pull request is refused like any other outsider, and every other check is unchanged. +3. The conclusion format is fixed (#637): the **first line** is `Verdict: PASS at ` (or `INC` / `REQUEST_CHANGES`), the second is the attribution (`Written-by: Reviewer`; a relay adds `Posted-by: Chief (proxy…)` on the third line, and the older prose `Written by: …` reads the same way), then the evidence — `Ran:` (or a `Ran …:` heading over a fenced block of `$ ` commands, #660–#663) with at least one command; `Not run:` naming any gap is recommended but not checked (owner decision 2026-10-22 09:57 Asia/Shanghai). A body whose first line is not the verdict is no conclusion. The comment's `author_association` must be `OWNER`, `MEMBER` or `COLLABORATOR`, or the report must be that pull request's own author listed in `selfReview` (#928), or the login must be shown as a repo admin by the permission API. Org repositories do not report `OWNER`. A `CONTRIBUTOR` who is none of those counts for nothing, so they can neither pass a tip nor hold one. An association the API did not send is not assumed, and a failed permission lookup is not admin. The permission API is for an admin GitHub did not tie to the repository: a `CONTRIBUTOR` stamp on someone else's pull request, asked only for a Tip+Verdict conclusion by a login in `reviewers`. A `selfReview` login is skipped only on that author's own report, which is the #928 carve-out and needs no lookup — skipping every `selfReview` login would never call, because the shipped policy lists every reviewer there. That lookup does not recover the own-pull-request PASS; #926 and #928 already do. The two qualifications do not replace each other: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else’s, while admin is read only through the permission API and only for a login already in `reviewers`. Every other check is unchanged. 4. The sha on that first line **is the head being judged**. A later commit moves the tip and the old PASS stops 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 @@ -160,7 +160,7 @@ makes the line a self-report, so the gate guarantees presence, not truth (#633). | --- | --- | | `.github/agent/review-policy.json` | the logins whose PASS counts, `selfReview`, the authors who may PASS their own PR, and `writers`, the Bot names a `Written-by:` line may carry (#627); editing it is T3 | | `.github/scripts/review-gate.mjs` | the rule above, for one pull request at a time | -| `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author), unlisted login, FAIL after PASS, missing `Ran:` (a missing `Not run:` no longer refuses), a missing `Written-by:` line and one naming an unlisted writer (#627), a `selfReview` login with a `CONTRIBUTOR` association counting only on its own pull request (#928), and the workflow's shape | +| `.github/scripts/review-gate.test.mjs` | stale tip, self-review (allowed only for a `selfReview` author), unlisted login, FAIL after PASS, missing `Ran:` (a missing `Not run:` no longer refuses), a missing `Written-by:` line and one naming an unlisted writer (#627), a `selfReview` login counting only on its own pull request (#928), a `selfReview` admin on someone else's pull request (permission API, `CONTRIBUTOR` stamp), a `CONTRIBUTOR` who is neither still refused, and the workflow's shape | | `.github/workflows/review-gate.yml` | reports it in the job summary; `continue-on-error`, never required | Run it by hand (read-only API calls with your `gh` login):