From 91c28ee75ecb69eb2e31bb750e5c5835c718a342 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 24 Oct 2026 06:04:28 +0000 Subject: [PATCH 1/8] fix(gate): count a qualified actor's PASS when GitHub reports CONTRIBUTOR enqueue-guard dropped the Verifier PASS on #925 because the review's author_association was CONTRIBUTOR, even though that login has admin on the repository. Org repositories do not report OWNER. Keep OWNER, MEMBER and COLLABORATOR, and also count a selfReview login or a login the permission API reports as admin. A CONTRIBUTOR who is neither stays out. Admin does not skip the reviewers list or the own-author rule. The lookup uses the token the job already has. Co-authored-by: Hongsheng Liu --- .github/scripts/enqueue-guard.mjs | 8 ++- .github/scripts/enqueue-guard.test.mjs | 46 ++++++++++++- .github/scripts/merge-gate.mjs | 10 +-- .github/scripts/merge-gate.test.mjs | 27 ++++++++ .github/scripts/review-gate.mjs | 81 ++++++++++++++++++++--- .github/scripts/review-gate.test.mjs | 92 +++++++++++++++++++++++--- AGENTS.md | 2 +- docs/autonomous-dev.md | 4 +- 8 files changed, 242 insertions(+), 28 deletions(-) diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index e6ca324b..3c7891c2 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -25,7 +25,7 @@ import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { isMechanicalRelease, judge } from './merge-gate.mjs' -import { loadPolicy, parseReport } from './review-gate.mjs' +import { loadPolicy, loginsNeedingAdmin, parseReport, repoAdminLogins } from './review-gate.mjs' const SHORT = 12 // AGENTS.md → Merge gates rule 2: the `hold` label is paired with a comment saying @@ -136,7 +136,11 @@ 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' }) + // #925: GitHub reports CONTRIBUTOR for a repo admin. Ask the permission API (the token this job + // already has) for every login the association and selfReview do not already qualify. + const policy = loadPolicy(policyPath) + const admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) + const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, admins, 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..d6c10491 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,6 +262,46 @@ 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/) + assert.match(r.reasons.join('; '), /not in selfReview/) + } +}) + +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 without an admin lookup', () => { + const gh = fakeGh({ + files: [HOST_FILE], + author: 'opener', + 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) + const own = run({ author: 'Birfy', reviews: [contributor('Birfy')], permission: 'throw' }) + assert.equal(own.ok, true, own.reasons.join('; ')) +}) + 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') diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 7c9a7c4d..f199cfa2 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 { decide, loadPolicy, loginsNeedingAdmin, repoAdminLogins } 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 } = {}) { const reasons = [] const target = String(head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -116,7 +116,7 @@ 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 }) + const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } : decide({ head: target, author, reports, policy, admins }) 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 +189,9 @@ 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 admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) + const result = judge({ labels, files, head, author, reports, policy, admins, 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..e60e57c2 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')] }) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index edd97b35..ba0b044c 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -11,6 +11,10 @@ // - 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): `author_association` is OWNER, MEMBER or +// COLLABORATOR, or the login is in `selfReview`, or it has admin on the repo. Org repositories +// do not report OWNER. GitHub stamps a repo admin as CONTRIBUTOR (#925); that stamp is not a +// refusal for one of those logins. A CONTRIBUTOR who is none of them 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 +39,57 @@ 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']) +// Logins whose association is not already OWNER, MEMBER or COLLABORATOR, and which `selfReview` +// does not already qualify. `decide` still applies every other check; this only picks who the +// permission API has to be asked about. A listed self-reviewer needs no lookup. +export function loginsNeedingAdmin(reports, policy) { + const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) + const out = [] + 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 || seen.has(key) || selfReview.has(key)) continue + seen.add(key) + out.push(name) + } + return out +} + +// `GET /repos/{owner}/{repo}/collaborators/{username}/permission` — metadata read, which the +// job's token already has. No new secret. Fail closed: a 403, a 404 or a body that is not the +// permission object is not admin (#676). `permission: admin` is the legacy role; `permissions.admin` +// covers a custom role that still grants admin. +export function repoAdminLogins({ gh, repo, logins } = {}) { + if (!repo || typeof gh !== 'function') return [] + const admins = [] + const seen = new Set() + for (const raw of logins ?? []) { + const name = String(raw ?? '').trim() + const key = name.toLowerCase() + if (!name || 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, and this lookup must not become a grant. + continue + } + let data + try { + data = JSON.parse(text) + } catch { + // A body that is not the permission object is not admin. + continue + } + const level = String(data?.permission ?? '').toLowerCase() + if (level === '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 +192,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 @@ -159,13 +215,18 @@ export function decide({ head, author, reports, policy }) { } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion 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 - // second reviewers list: the same login judging someone else's pull request keeps every rule - // below, and #676 still refuses a login nobody listed. - const selfOnly = selfReview.has(login(r)) && login(r) === authorLogin - if (!ASSOCIATIONS.has(association) && !selfOnly) { - reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, so it is not a conclusion`) + // #676, widened after #925. Org repositories never report OWNER; MEMBER (and COLLABORATOR, + // where GitHub sends it) is the association bar. GitHub stamps a repo admin's review + // CONTRIBUTOR — on #925 that discarded hsliuustc0106's PASS, and listing the login in + // selfReview (#926) did not clear the stamp while this check still required the association. + // A login in selfReview, or one the caller resolved as repo admin (`admins`, or `admin: true` + // on the report), is tied to this repository anyway. A CONTRIBUTOR who is neither is not a + // conclusion. A failed permission lookup is not admin. The own-author and reviewers checks + // below are unchanged: admin is not a self-PASS and not a second reviewers list. + const listedSelf = selfReview.has(login(r)) + const isAdmin = r.admin === true || adminSet.has(login(r)) + if (!ASSOCIATIONS.has(association) && !listedSelf && !isAdmin) { + reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, and ${r.login} is not in selfReview and was not shown as a repo admin, so it is not a conclusion`) continue } // #676: a conclusion at this tip belongs to the newest-word question even when it fails a check @@ -256,7 +317,9 @@ 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 admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) + const result = decide({ head, author, reports, policy, admins }) 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..7d4272f0 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 { decide, loadPolicy, loginsNeedingAdmin, parseReport, render, repoAdminLogins } from './review-gate.mjs' const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' @@ -494,11 +494,23 @@ test('#676: a conclusion from outside the repository counts for nothing', () => assert.match(missing.reason, /author association missing/) }) -// #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. One exception for one person, never a second -// reviewers list: the same login on someone else's pull request stays outside the association rule, -// and #676 still refuses everyone nobody listed. -test('#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their own pull request', () => { +// #925: GitHub stamps an org admin's review CONTRIBUTOR. OWNER/MEMBER/COLLABORATOR still count. +// A login in selfReview counts on any pull request (the #928 own-pull-request limit dropped that +// stamp while the reviewer was not the author). A repo admin counts the same way. A CONTRIBUTOR +// who is neither is still not a conclusion (#676). Admin does not skip the reviewers list or the +// own-author rule. +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/) + assert.match(bare.reason, /not in selfReview/) +}) + +test('#925: a selfReview login stamped CONTRIBUTOR counts on their own pull request and on someone else\'s', () => { const selfPolicy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer', 'hsliuustc0106'] } const own = (login = 'hsliuustc0106') => comment(login, report({ writer: 'hsliuustc0106' }), '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR') @@ -507,12 +519,74 @@ test('#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their ow assert.equal(mine.by, 'hsliuustc0106') assert.equal(mine.judged[0].counts, true) + const elsewhere = decide({ head: HEAD, author: 'opener', reports: [own()], policy: selfPolicy }) + assert.equal(elsewhere.ok, true, 'selfReview qualifies the association on any pull request in this repo') + assert.equal(elsewhere.by, 'hsliuustc0106') + const unlisted = decide({ head: HEAD, author: 'opener', reports: [own('passer-by')], policy: selfPolicy }) assert.equal(unlisted.ok, false, 'a CONTRIBUTOR nobody listed is still not a conclusion') assert.match(unlisted.reason, /author association CONTRIBUTOR/) assert.equal(unlisted.judged[0].counts, false) +}) - const elsewhere = decide({ head: HEAD, author: 'opener', reports: [own()], policy: selfPolicy }) - 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) +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 flagged = decide({ head: HEAD, author: 'opener', reports: [{ ...pass, admin: true }], policy: adminPolicy }) + assert.equal(flagged.ok, true, 'admin: true on the report is the same grant') + + 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: the permission API is asked only for logins the association and selfReview do not already qualify', () => { + const policy = { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer'] } + const reports = [ + { login: 'reviewer-agent', association: 'MEMBER' }, + { login: 'Birfy', association: 'CONTRIBUTOR' }, + { login: 'hsliuustc0106', association: 'CONTRIBUTOR' }, + { login: 'hsliuustc0106', association: 'NONE' }, + { login: '', association: 'NONE' }, + ] + assert.deepEqual(loginsNeedingAdmin(reports, policy), ['hsliuustc0106']) + + 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', permissions: { admin: true } }) + if (login === 'writer') return JSON.stringify({ permission: 'write', role_name: 'write', permissions: { admin: false } }) + if (login === 'custom') return JSON.stringify({ permission: 'write', permissions: { admin: true } }) + if (login === 'broken') return 'not-json' + throw new Error('HTTP 404') + } + assert.deepEqual(repoAdminLogins({ gh, repo: 'ThinkFlowLab/FlyBot', logins: ['hsliuustc0106', 'hsliuustc0106', 'writer', 'custom', 'broken', 'missing', ''] }), ['hsliuustc0106', 'custom']) + assert.deepEqual(calls, [ + 'repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/writer/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/custom/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/broken/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/missing/permission', + ]) + assert.deepEqual(repoAdminLogins({ gh, logins: ['hsliuustc0106'] }), []) }) diff --git a/AGENTS.md b/AGENTS.md index 12bceb1b..e5fbc1b1 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` of `OWNER`, `MEMBER` or `COLLABORATOR` counts — org repositories do not report `OWNER`; `MEMBER` is the org bar, and `COLLABORATOR` still counts where GitHub sends it. A login listed in `selfReview`, or a login the repository permission API reports as admin, also counts when GitHub stamps the review `CONTRIBUTOR` (#925: an org admin's PASS was discarded, and adding the login to `selfReview` in #926 did not by itself clear that stamp). A `CONTRIBUTOR` who is neither 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. An association the API did not send is not assumed, and a permission lookup that fails is not admin. The lookup uses the token the job already has. Every other check (tip, `Written-by:`, `Ran:`, reviewer list, the own-author rule, newest word) is unchanged: admin does not PASS your own pull request unless `selfReview` lists you, and it does not skip the reviewers list. 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..85476ade 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 login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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. 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 or a repo admin whose association is `CONTRIBUTOR` (#925), 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): From a7776a17bd0a1f875e39d116d6e5e8e39317d669 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 24 Oct 2026 06:22:15 +0000 Subject: [PATCH 2/8] refactor(gate): one adminsFor lookup for a qualified PASS enqueue-guard, merge-gate and review-gate each repeated the same permission lookup. One helper asks the API only for logins that OWNER/MEMBER/COLLABORATOR and selfReview do not already qualify, and judge skips that call when the tier needs no PASS. Accept and refuse rules are unchanged. Co-authored-by: Hongsheng Liu --- .github/scripts/enqueue-guard.mjs | 8 ++-- .github/scripts/enqueue-guard.test.mjs | 10 ++++- .github/scripts/merge-gate.mjs | 12 +++--- .github/scripts/review-gate.mjs | 56 +++++++------------------- .github/scripts/review-gate.test.mjs | 18 ++++----- AGENTS.md | 2 +- 6 files changed, 43 insertions(+), 63 deletions(-) diff --git a/.github/scripts/enqueue-guard.mjs b/.github/scripts/enqueue-guard.mjs index 3c7891c2..a2390ed1 100644 --- a/.github/scripts/enqueue-guard.mjs +++ b/.github/scripts/enqueue-guard.mjs @@ -25,7 +25,7 @@ import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' import { isMechanicalRelease, judge } from './merge-gate.mjs' -import { loadPolicy, loginsNeedingAdmin, parseReport, repoAdminLogins } from './review-gate.mjs' +import { loadPolicy, parseReport } from './review-gate.mjs' const SHORT = 12 // AGENTS.md → Merge gates rule 2: the `hold` label is paired with a comment saying @@ -136,11 +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. - // #925: GitHub reports CONTRIBUTOR for a repo admin. Ask the permission API (the token this job - // already has) for every login the association and selfReview do not already qualify. + // `judge` asks `adminsFor` when this tier needs a PASS. const policy = loadPolicy(policyPath) - const admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) - const result = judge({ labels, files, head, author: pull.user?.login, reports, policy, admins, expectedHead: expectedHead ?? arg('--sha'), mode: 'queue' }) + 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 d6c10491..3b1575e9 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -298,8 +298,14 @@ test('#925: a selfReview login stamped CONTRIBUTOR is accepted without an admin assert.equal(r.ok, true, r.reasons.join('; ')) assert.equal(r.verdict.by, 'Birfy') assert.equal(gh.calls.some((path) => path.includes('/permission')), false) - const own = run({ author: 'Birfy', reviews: [contributor('Birfy')], permission: 'throw' }) - assert.equal(own.ok, true, own.reasons.join('; ')) +}) + +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', () => { diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index f199cfa2..3dfc267d 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, loginsNeedingAdmin, repoAdminLogins } 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', admins } = {}) { +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, admins }) + // 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 }) }) 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 @@ -190,8 +193,7 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO // base copy of risk-gate.mjs (see exemptFromPass). const files = list(`repos/${repo}/pulls/${pr}/files?per_page=100`) const policy = loadPolicy(policyPath) - const admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) - const result = judge({ labels, files, head, author, reports, policy, admins, expectedHead: arg('--sha'), mode }) + 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/review-gate.mjs b/.github/scripts/review-gate.mjs index ba0b044c..8195e47e 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -11,10 +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): `author_association` is OWNER, MEMBER or -// COLLABORATOR, or the login is in `selfReview`, or it has admin on the repo. Org repositories -// do not report OWNER. GitHub stamps a repo admin as CONTRIBUTOR (#925); that stamp is not a -// refusal for one of those logins. A CONTRIBUTOR who is none of them is not a conclusion; +// - the login is tied to this repository (#676, #925): `author_association` is OWNER, MEMBER or +// COLLABORATOR, or the login is 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 @@ -39,12 +38,14 @@ 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']) -// Logins whose association is not already OWNER, MEMBER or COLLABORATOR, and which `selfReview` -// does not already qualify. `decide` still applies every other check; this only picks who the -// permission API has to be asked about. A listed self-reviewer needs no lookup. -export function loginsNeedingAdmin(reports, policy) { +// Logins GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR, or `selfReview`), +// asked once via `GET /repos/{owner}/{repo}/collaborators/{username}/permission`. 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 } = {}) { + if (!repo || typeof gh !== 'function') return [] const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) - const out = [] + const admins = [] const seen = new Set() for (const r of reports ?? []) { if (ASSOCIATIONS.has(String(r.association ?? '').toUpperCase())) continue @@ -52,29 +53,11 @@ export function loginsNeedingAdmin(reports, policy) { const key = name.toLowerCase() if (!name || seen.has(key) || selfReview.has(key)) continue seen.add(key) - out.push(name) - } - return out -} - -// `GET /repos/{owner}/{repo}/collaborators/{username}/permission` — metadata read, which the -// job's token already has. No new secret. Fail closed: a 403, a 404 or a body that is not the -// permission object is not admin (#676). `permission: admin` is the legacy role; `permissions.admin` -// covers a custom role that still grants admin. -export function repoAdminLogins({ gh, repo, logins } = {}) { - if (!repo || typeof gh !== 'function') return [] - const admins = [] - const seen = new Set() - for (const raw of logins ?? []) { - const name = String(raw ?? '').trim() - const key = name.toLowerCase() - if (!name || 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, and this lookup must not become a grant. + // Fail closed. A 403 or 404 is not admin. continue } let data @@ -84,8 +67,7 @@ export function repoAdminLogins({ gh, repo, logins } = {}) { // A body that is not the permission object is not admin. continue } - const level = String(data?.permission ?? '').toLowerCase() - if (level === 'admin' || data?.permissions?.admin === true) admins.push(name) + if (String(data?.permission ?? '').toLowerCase() === 'admin' || data?.permissions?.admin === true) admins.push(name) } return admins } @@ -215,16 +197,9 @@ export function decide({ head, author, reports, policy, admins } = {}) { } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion const association = String(r.association ?? '').toUpperCase() - // #676, widened after #925. Org repositories never report OWNER; MEMBER (and COLLABORATOR, - // where GitHub sends it) is the association bar. GitHub stamps a repo admin's review - // CONTRIBUTOR — on #925 that discarded hsliuustc0106's PASS, and listing the login in - // selfReview (#926) did not clear the stamp while this check still required the association. - // A login in selfReview, or one the caller resolved as repo admin (`admins`, or `admin: true` - // on the report), is tied to this repository anyway. A CONTRIBUTOR who is neither is not a - // conclusion. A failed permission lookup is not admin. The own-author and reviewers checks - // below are unchanged: admin is not a self-PASS and not a second reviewers list. + // #676 / #925. `admins` is whatever `adminsFor` resolved; it does not skip the checks below. const listedSelf = selfReview.has(login(r)) - const isAdmin = r.admin === true || adminSet.has(login(r)) + const isAdmin = adminSet.has(login(r)) if (!ASSOCIATIONS.has(association) && !listedSelf && !isAdmin) { reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, and ${r.login} is not in selfReview and was not shown as a repo admin, so it is not a conclusion`) continue @@ -318,8 +293,7 @@ function main() { ] const policy = loadPolicy(policyPath) - const admins = repoAdminLogins({ gh, repo, logins: loginsNeedingAdmin(reports, policy) }) - const result = decide({ head, author, reports, policy, admins }) + const result = decide({ head, author, reports, policy, admins: adminsFor({ gh, repo, reports, policy }) }) 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 7d4272f0..fac063e9 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, loginsNeedingAdmin, parseReport, render, repoAdminLogins } from './review-gate.mjs' +import { adminsFor, decide, loadPolicy, parseReport, render } from './review-gate.mjs' const HEAD = 'a4d506a1b2c3d4e5f60718293a4b5c6d7e8f9012' const OLD = '175328d0c1b2a3d4e5f60718293a4b5c6d7e8f90' @@ -537,9 +537,6 @@ test('#925: repo admin qualifies a CONTRIBUTOR, and does not skip the reviewers assert.equal(accepted.ok, true, accepted.reason) assert.equal(accepted.by, 'hsliuustc0106') - const flagged = decide({ head: HEAD, author: 'opener', reports: [{ ...pass, admin: true }], policy: adminPolicy }) - assert.equal(flagged.ok, true, 'admin: true on the report is the same grant') - const notAdmin = decide({ head: HEAD, author: 'opener', reports: [pass], policy: adminPolicy }) assert.equal(notAdmin.ok, false) assert.match(notAdmin.reason, /author association CONTRIBUTOR/) @@ -566,21 +563,23 @@ test('#925: the permission API is asked only for logins the association and self { login: 'Birfy', association: 'CONTRIBUTOR' }, { login: 'hsliuustc0106', association: 'CONTRIBUTOR' }, { login: 'hsliuustc0106', association: 'NONE' }, + { login: 'writer', association: 'CONTRIBUTOR' }, + { login: 'custom', association: 'CONTRIBUTOR' }, + { login: 'broken', association: 'CONTRIBUTOR' }, + { login: 'missing', association: 'CONTRIBUTOR' }, { login: '', association: 'NONE' }, ] - assert.deepEqual(loginsNeedingAdmin(reports, policy), ['hsliuustc0106']) - 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', permissions: { admin: true } }) + if (login === 'hsliuustc0106') return JSON.stringify({ permission: 'admin', role_name: 'admin' }) if (login === 'writer') return JSON.stringify({ permission: 'write', role_name: 'write', permissions: { admin: false } }) if (login === 'custom') return JSON.stringify({ permission: 'write', permissions: { admin: true } }) if (login === 'broken') return 'not-json' throw new Error('HTTP 404') } - assert.deepEqual(repoAdminLogins({ gh, repo: 'ThinkFlowLab/FlyBot', logins: ['hsliuustc0106', 'hsliuustc0106', 'writer', 'custom', 'broken', 'missing', ''] }), ['hsliuustc0106', 'custom']) + assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy }), ['hsliuustc0106', 'custom']) assert.deepEqual(calls, [ 'repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission', 'repos/ThinkFlowLab/FlyBot/collaborators/writer/permission', @@ -588,5 +587,6 @@ test('#925: the permission API is asked only for logins the association and self 'repos/ThinkFlowLab/FlyBot/collaborators/broken/permission', 'repos/ThinkFlowLab/FlyBot/collaborators/missing/permission', ]) - assert.deepEqual(repoAdminLogins({ gh, logins: ['hsliuustc0106'] }), []) + assert.deepEqual(adminsFor({ gh, reports, policy }), []) + assert.equal(calls.length, 5, 'no repo means no lookup') }) diff --git a/AGENTS.md b/AGENTS.md index e5fbc1b1..e01e1e03 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 tied to this repository (#676). `author_association` of `OWNER`, `MEMBER` or `COLLABORATOR` counts — org repositories do not report `OWNER`; `MEMBER` is the org bar, and `COLLABORATOR` still counts where GitHub sends it. A login listed in `selfReview`, or a login the repository permission API reports as admin, also counts when GitHub stamps the review `CONTRIBUTOR` (#925: an org admin's PASS was discarded, and adding the login to `selfReview` in #926 did not by itself clear that stamp). A `CONTRIBUTOR` who is neither 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. An association the API did not send is not assumed, and a permission lookup that fails is not admin. The lookup uses the token the job already has. Every other check (tip, `Written-by:`, `Ran:`, reviewer list, the own-author rule, newest word) is unchanged: admin does not PASS your own pull request unless `selfReview` lists you, and it does not skip the reviewers list. +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`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 logins the association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. 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. From e850e158be2e1116394406eac78e641260b52078 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 24 Oct 2026 06:23:51 +0000 Subject: [PATCH 3/8] fix(gate): look up admin only for a tip verdict A comment with no Tip SHA and Verdict is not a conclusion, so the permission API is not asked about its author. Accept and refuse rules are unchanged. Co-authored-by: Hongsheng Liu --- .github/scripts/review-gate.mjs | 14 +++++++++----- .github/scripts/review-gate.test.mjs | 23 +++++++++++++---------- AGENTS.md | 2 +- docs/autonomous-dev.md | 2 +- 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 8195e47e..5ef22ca8 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -38,10 +38,11 @@ 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']) -// Logins GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR, or `selfReview`), -// asked once via `GET /repos/{owner}/{repo}/collaborators/{username}/permission`. 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. +// A conclusion GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR, or +// `selfReview`), asked once via the collaborator permission API. A comment with no Tip SHA +// and Verdict is not a conclusion, so it is not looked up. 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 } = {}) { if (!repo || typeof gh !== 'function') return [] const selfReview = new Set((policy?.selfReview ?? []).map((l) => String(l).toLowerCase())) @@ -51,7 +52,10 @@ export function adminsFor({ gh, repo, reports, policy } = {}) { if (ASSOCIATIONS.has(String(r.association ?? '').toUpperCase())) continue const name = String(r.login ?? '').trim() const key = name.toLowerCase() - if (!name || seen.has(key) || selfReview.has(key)) continue + if (!name || selfReview.has(key)) continue + const parsed = parseReport(r.body) + if (!parsed.tip || !parsed.verdict) continue + if (seen.has(key)) continue seen.add(key) let text try { diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index fac063e9..d679f77e 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -556,18 +556,21 @@ test('#925: repo admin qualifies a CONTRIBUTOR, and does not skip the reviewers assert.match(selfPass.reason, /own author is not a reviewer/) }) -test('#925: the permission API is asked only for logins the association and selfReview do not already qualify', () => { +test('#925: the permission API is asked only for a conclusion the association and selfReview do not already qualify', () => { const policy = { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['Reviewer'] } + const verdict = `Verdict: PASS at ${HEAD}` const reports = [ - { login: 'reviewer-agent', association: 'MEMBER' }, - { login: 'Birfy', association: 'CONTRIBUTOR' }, - { login: 'hsliuustc0106', association: 'CONTRIBUTOR' }, - { login: 'hsliuustc0106', association: 'NONE' }, - { login: 'writer', association: 'CONTRIBUTOR' }, - { login: 'custom', association: 'CONTRIBUTOR' }, - { login: 'broken', association: 'CONTRIBUTOR' }, - { login: 'missing', association: 'CONTRIBUTOR' }, - { login: '', association: 'NONE' }, + { 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: 'custom', association: 'CONTRIBUTOR', body: verdict }, + { login: 'broken', association: 'CONTRIBUTOR', body: verdict }, + { login: 'missing', association: 'CONTRIBUTOR', body: verdict }, + { login: '', association: 'NONE', body: verdict }, ] const calls = [] const gh = (_cmd, path) => { diff --git a/AGENTS.md b/AGENTS.md index e01e1e03..bc90a446 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 tied to this repository (#676): `author_association` `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 logins the association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. +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`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. 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 85476ade..70bcf49b 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`, or the login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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. 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 login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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 asked only for a conclusion (a Tip SHA and a Verdict) the association and `selfReview` do not already qualify. 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 From 861b845834b3c0f3e854f763e15f1cfe6b184f6e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 24 Oct 2026 06:26:55 +0000 Subject: [PATCH 4/8] fix(gate): skip the admin lookup for a login outside reviewers A conclusion whose login is not in policy.reviewers can never pass that check, so adminsFor does not call the permission API for it. Co-authored-by: Hongsheng Liu --- .github/scripts/review-gate.mjs | 12 ++++---- .github/scripts/review-gate.test.mjs | 43 ++++++++++++++++++++++------ AGENTS.md | 2 +- docs/autonomous-dev.md | 2 +- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 5ef22ca8..3d055a69 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -39,20 +39,22 @@ const SHORT = 12 const ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']) // A conclusion GitHub did not already tie to the repo (OWNER/MEMBER/COLLABORATOR, or -// `selfReview`), asked once via the collaborator permission API. A comment with no Tip SHA -// and Verdict is not a conclusion, so it is not looked up. 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. +// `selfReview`), 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 +// 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 } = {}) { 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 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 || selfReview.has(key)) continue + if (!name || selfReview.has(key) || !reviewers.has(key)) continue const parsed = parseReport(r.body) if (!parsed.tip || !parsed.verdict) continue if (seen.has(key)) continue diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index d679f77e..f5f302d8 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -556,8 +556,29 @@ test('#925: repo admin qualifies a CONTRIBUTOR, and does not skip the reviewers assert.match(selfPass.reason, /own author is not a reviewer/) }) -test('#925: the permission API is asked only for a conclusion the association and selfReview do not already qualify', () => { - const policy = { reviewers: ['reviewer-agent'], selfReview: ['Birfy'], writers: ['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 policy = { reviewers: ['Reviewer-Agent'], selfReview: [], writers: ['Reviewer'] } + const body = `Verdict: PASS at ${HEAD}` + const admins = adminsFor({ + gh, + repo: 'ThinkFlowLab/FlyBot', + policy, + 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 policy = { reviewers: ['reviewer-agent', '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 }, @@ -567,9 +588,12 @@ test('#925: the permission API is asked only for a conclusion the association an { login: 'hsliuustc0106', association: 'CONTRIBUTOR', body: verdict }, { login: 'hsliuustc0106', association: 'NONE', body: verdict }, { login: 'writer', association: 'CONTRIBUTOR', body: verdict }, - { login: 'custom', 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 = [] @@ -577,19 +601,22 @@ test('#925: the permission API is asked only for a conclusion the association an 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 === 'writer') return JSON.stringify({ permission: 'write', role_name: 'write', permissions: { admin: false } }) + 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 === 'broken') return 'not-json' + if (login === 'bad-body') return 'not-json' throw new Error('HTTP 404') } assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy }), ['hsliuustc0106', 'custom']) assert.deepEqual(calls, [ 'repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission', - 'repos/ThinkFlowLab/FlyBot/collaborators/writer/permission', + 'repos/ThinkFlowLab/FlyBot/collaborators/write-user/permission', 'repos/ThinkFlowLab/FlyBot/collaborators/custom/permission', - 'repos/ThinkFlowLab/FlyBot/collaborators/broken/permission', - 'repos/ThinkFlowLab/FlyBot/collaborators/missing/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) + } assert.deepEqual(adminsFor({ gh, reports, policy }), []) assert.equal(calls.length, 5, 'no repo means no lookup') }) diff --git a/AGENTS.md b/AGENTS.md index bc90a446..92107375 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 tied to this repository (#676): `author_association` `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. +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`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. 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 70bcf49b..4bb585c5 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`, or the login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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 asked only for a conclusion (a Tip SHA and a Verdict) the association and `selfReview` do not already qualify. 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 login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association and `selfReview` do not already qualify. 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 From 031d1e8e8dd2c2507b0e76c31bdcfcce1565d5eb Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 24 Oct 2026 14:58:30 +0800 Subject: [PATCH 5/8] fix(gate): keep the selfReview carve-out own-author only, accept a repo admin The branch widened selfReview to qualify any pull request, which deletes the #931 negative case and turns the list into a second reviewers list. Restore selfOnly (selfReview.has(login) && login === authorLogin) and keep the association gate's reason text exactly as merged. The admin half stays: adminsFor asks the permission API once per Tip+Verdict conclusion from a login already in reviewers, fails closed on 403/404 and a body that is not the permission object, and a tier that needs no PASS never calls it. Fixes #936 --- .github/scripts/enqueue-guard.test.mjs | 11 +++-- .github/scripts/merge-gate.test.mjs | 18 ++++++++ .github/scripts/review-gate.mjs | 17 +++++--- .github/scripts/review-gate.test.mjs | 58 ++++++++++++++------------ AGENTS.md | 2 +- docs/autonomous-dev.md | 4 +- 6 files changed, 72 insertions(+), 38 deletions(-) diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 3b1575e9..6721def4 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -277,7 +277,6 @@ test('#925: a CONTRIBUTOR who is not in selfReview and is not a repo admin is re const r = run({ reviews: [contributor('reviewer-agent')], permission }) assert.equal(r.ok, false, permission) assert.match(r.reasons.join('; '), /author association CONTRIBUTOR/) - assert.match(r.reasons.join('; '), /not in selfReview/) } }) @@ -287,10 +286,10 @@ test('#925: a CONTRIBUTOR who has admin on the repo is accepted', () => { assert.equal(r.verdict.by, 'reviewer-agent') }) -test('#925: a selfReview login stamped CONTRIBUTOR is accepted without an admin lookup', () => { +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: 'opener', + author: 'Birfy', reviews: [contributor('Birfy')], permission: 'throw', }) @@ -300,6 +299,12 @@ test('#925: a selfReview login stamped CONTRIBUTOR is accepted without an admin 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 }) diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index e60e57c2..1e30ae70 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -439,3 +439,21 @@ 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/) +}) diff --git a/.github/scripts/review-gate.mjs b/.github/scripts/review-gate.mjs index 3d055a69..55a3d6df 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -12,8 +12,8 @@ // `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, or the login is in `selfReview`, or `adminsFor` found repo admin. A CONTRIBUTOR -// who is none of those is not a conclusion; +// 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 @@ -203,11 +203,16 @@ export function decide({ head, author, reports, policy, admins } = {}) { } if (!parsed.verdict) continue // a `Tip SHA` block with no verdict is not a conclusion const association = String(r.association ?? '').toUpperCase() - // #676 / #925. `admins` is whatever `adminsFor` resolved; it does not skip the checks below. - const listedSelf = selfReview.has(login(r)) + // #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 + // 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 const isAdmin = adminSet.has(login(r)) - if (!ASSOCIATIONS.has(association) && !listedSelf && !isAdmin) { - reasons.push(`${where(r)}: author association ${association || 'missing'} is not OWNER, MEMBER or COLLABORATOR, and ${r.login} is not in selfReview and was not shown as a repo admin, so it is not a conclusion`) + 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 } // #676: a conclusion at this tip belongs to the newest-word question even when it fails a check diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index f5f302d8..6986b19e 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -494,23 +494,11 @@ test('#676: a conclusion from outside the repository counts for nothing', () => assert.match(missing.reason, /author association missing/) }) -// #925: GitHub stamps an org admin's review CONTRIBUTOR. OWNER/MEMBER/COLLABORATOR still count. -// A login in selfReview counts on any pull request (the #928 own-pull-request limit dropped that -// stamp while the reviewer was not the author). A repo admin counts the same way. A CONTRIBUTOR -// who is neither is still not a conclusion (#676). Admin does not skip the reviewers list or the -// own-author rule. -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/) - assert.match(bare.reason, /not in selfReview/) -}) - -test('#925: a selfReview login stamped CONTRIBUTOR counts on their own pull request and on someone else\'s', () => { +// #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. One exception for one person, never a second +// reviewers list: the same login on someone else's pull request stays outside the association rule, +// and #676 still refuses everyone nobody listed. +test('#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their own pull request', () => { const selfPolicy = { reviewers: ['reviewer-agent', 'hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer', 'hsliuustc0106'] } const own = (login = 'hsliuustc0106') => comment(login, report({ writer: 'hsliuustc0106' }), '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR') @@ -519,16 +507,20 @@ test('#925: a selfReview login stamped CONTRIBUTOR counts on their own pull requ assert.equal(mine.by, 'hsliuustc0106') assert.equal(mine.judged[0].counts, true) - const elsewhere = decide({ head: HEAD, author: 'opener', reports: [own()], policy: selfPolicy }) - assert.equal(elsewhere.ok, true, 'selfReview qualifies the association on any pull request in this repo') - assert.equal(elsewhere.by, 'hsliuustc0106') - const unlisted = decide({ head: HEAD, author: 'opener', reports: [own('passer-by')], policy: selfPolicy }) assert.equal(unlisted.ok, false, 'a CONTRIBUTOR nobody listed is still not a conclusion') assert.match(unlisted.reason, /author association CONTRIBUTOR/) assert.equal(unlisted.judged[0].counts, false) + + const elsewhere = decide({ head: HEAD, author: 'opener', reports: [own()], policy: selfPolicy }) + 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') @@ -562,12 +554,12 @@ test('#925: a Tip+Verdict from a login outside reviewers does not call the permi calls.push(path) return JSON.stringify({ permission: 'admin' }) } - const policy = { reviewers: ['Reviewer-Agent'], selfReview: [], writers: ['Reviewer'] } + const p = { reviewers: ['Reviewer-Agent'], selfReview: [], writers: ['Reviewer'] } const body = `Verdict: PASS at ${HEAD}` const admins = adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', - policy, + policy: p, reports: [ { login: 'passer-by', association: 'CONTRIBUTOR', body }, { login: 'reviewer-agent', association: 'CONTRIBUTOR', body }, @@ -578,7 +570,7 @@ test('#925: a Tip+Verdict from a login outside reviewers does not call the permi }) test('#925: the permission API is asked only for a listed reviewer conclusion the association and selfReview do not already qualify', () => { - const policy = { reviewers: ['reviewer-agent', 'hsliuustc0106', 'write-user', 'custom', 'bad-body', 'gone'], selfReview: ['Birfy'], writers: ['Reviewer'] } + const p = { reviewers: ['reviewer-agent', '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 }, @@ -606,7 +598,7 @@ test('#925: the permission API is asked only for a listed reviewer conclusion th if (login === 'bad-body') return 'not-json' throw new Error('HTTP 404') } - assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy }), ['hsliuustc0106', 'custom']) + assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy: p }), ['hsliuustc0106', 'custom']) assert.deepEqual(calls, [ 'repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission', 'repos/ThinkFlowLab/FlyBot/collaborators/write-user/permission', @@ -617,6 +609,20 @@ test('#925: the permission API is asked only for a listed reviewer conclusion th for (const outsider of ['writer', 'broken', 'missing']) { assert.equal(calls.some((path) => path.includes(`/collaborators/${outsider}/`)), false, outsider) } - assert.deepEqual(adminsFor({ gh, reports, policy }), []) + // `Birfy` is the selfReview login: its own pull request is covered by that carve-out, so it is + // never looked up as an admin — which is also why the same login on someone else's pull request + // stays refused (#931). + assert.equal(calls.some((path) => path.includes('/collaborators/Birfy/')), false, 'a selfReview login is not looked up') + assert.deepEqual(adminsFor({ gh, reports, policy: p }), []) assert.equal(calls.length, 5, '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/) +}) diff --git a/AGENTS.md b/AGENTS.md index 92107375..22ed7faf 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 tied to this repository (#676): `author_association` `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`), a login in `selfReview`, or a login the permission API reports as admin (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. +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 (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. 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 4bb585c5..f45e306a 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`, or the login must be listed in `selfReview` or shown as a repo admin by the permission API (#676, #925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association and `selfReview` do not already qualify. 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 (#925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association and `selfReview` do not already qualify. 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 or a repo admin whose association is `CONTRIBUTOR` (#925), a `CONTRIBUTOR` who is neither still refused, 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 repo admin whose association is `CONTRIBUTOR` (#925), 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): From f8d7ddc75223dab7aa1f431daeeb15cc4bc6f285 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 24 Oct 2026 15:09:04 +0800 Subject: [PATCH 6/8] fix(gate): ask the permission API for a reviewer who is not the pull request's author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit adminsFor skipped every login in selfReview, and on this repository every reviewer is also in selfReview, so the admin rule had no candidate and never fired: the gate decided with isAdmin always false. Skip only the shape the #928 carve-out already covers — the pull request's own author — and let every other reviewer be asked. Fixes #936 --- .github/scripts/enqueue-guard.test.mjs | 8 ++++ .github/scripts/merge-gate.mjs | 2 +- .github/scripts/merge-gate.test.mjs | 11 ++++++ .github/scripts/review-gate.mjs | 19 ++++++---- .github/scripts/review-gate.test.mjs | 51 ++++++++++++++++++++++---- AGENTS.md | 2 +- docs/autonomous-dev.md | 2 +- 7 files changed, 77 insertions(+), 18 deletions(-) diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 6721def4..8868ba33 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -319,3 +319,11 @@ test('#602: the instructions that enqueue point at the guard', () => { 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.equal(gh.calls.filter((path) => path.includes('/permission')).length, 1) +}) diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 3dfc267d..14855e07 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -119,7 +119,7 @@ export function judge({ labels, files, head, author, reports, policy, expectedHe // 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 }) }) + : 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 diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index 1e30ae70..116dd072 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -457,3 +457,14 @@ test('#925: admin does not skip the own-author rule, and selfReview does not car 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 55a3d6df..6bfa8961 100644 --- a/.github/scripts/review-gate.mjs +++ b/.github/scripts/review-gate.mjs @@ -38,23 +38,28 @@ 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, or -// `selfReview`), 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 -// comment with no Tip SHA and Verdict is not a conclusion. Metadata read on the token +// 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 } = {}) { +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 || selfReview.has(key) || !reviewers.has(key)) continue + 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 @@ -304,7 +309,7 @@ function main() { ] const policy = loadPolicy(policyPath) - const result = decide({ head, author, reports, policy, admins: adminsFor({ gh, repo, reports, policy }) }) + 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 6986b19e..befcccf2 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -570,7 +570,7 @@ test('#925: a Tip+Verdict from a login outside reviewers does not call the permi }) 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', 'hsliuustc0106', 'write-user', 'custom', 'bad-body', 'gone'], selfReview: ['Birfy'], writers: ['Reviewer'] } + 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 }, @@ -598,8 +598,12 @@ test('#925: the permission API is asked only for a listed reviewer conclusion th if (login === 'bad-body') return 'not-json' throw new Error('HTTP 404') } - assert.deepEqual(adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy: p }), ['hsliuustc0106', 'custom']) + // `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', @@ -609,12 +613,19 @@ test('#925: the permission API is asked only for a listed reviewer conclusion th for (const outsider of ['writer', 'broken', 'missing']) { assert.equal(calls.some((path) => path.includes(`/collaborators/${outsider}/`)), false, outsider) } - // `Birfy` is the selfReview login: its own pull request is covered by that carve-out, so it is - // never looked up as an admin — which is also why the same login on someone else's pull request - // stays refused (#931). - assert.equal(calls.some((path) => path.includes('/collaborators/Birfy/')), false, 'a selfReview login is not looked up') - assert.deepEqual(adminsFor({ gh, reports, policy: p }), []) - assert.equal(calls.length, 5, 'no repo means no lookup') + // 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', () => { @@ -626,3 +637,27 @@ test('#925: OWNER, MEMBER and COLLABORATOR still count, and a bare CONTRIBUTOR d assert.equal(bare.ok, false) assert.match(bare.reason, /author association CONTRIBUTOR/) }) + +// #925: the shape the admin rule exists for — a login in `selfReview` who is NOT this pull request's +// author. `selfOnly` cannot cover it, so the permission API decides, and it must actually be asked. +test('#925: a selfReview login judging someone else\'s pull request counts when the API reports admin', () => { + const p = { reviewers: ['hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer'] } + const body = report() + const reports = [{ login: 'hsliuustc0106', association: 'CONTRIBUTOR', body }] + 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: p, author: 'opener' }) + assert.deepEqual(admins, ['hsliuustc0106']) + assert.equal(calls.length, 1, 'the permission API is asked once') + + const r = decide({ head: HEAD, author: 'opener', reports: [comment('hsliuustc0106', body, '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR')], policy: p, admins }) + assert.equal(r.ok, true, r.reason) + assert.equal(r.by, 'hsliuustc0106') + + // On its own pull request the carve-out already qualifies it, so the lookup is skipped — that is + // the shape #931 pins, and this test must not restate it (the #931 case stays the only red there). + const ownCalls = [] + const own = adminsFor({ gh: (_cmd, path) => { ownCalls.push(path); return JSON.stringify({ permission: 'admin' }) }, repo: 'ThinkFlowLab/FlyBot', reports, policy: p, author: 'hsliuustc0106' }) + assert.deepEqual(own, []) + assert.deepEqual(ownCalls, []) +}) diff --git a/AGENTS.md b/AGENTS.md index 22ed7faf..12b549b9 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 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 (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` whose association and `selfReview` do not already qualify. Admin does not skip the reviewers list or the own-author rule. +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 (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association does not already qualify, and that is not this pull request's own author listed in `selfReview`. Admin does not skip the reviewers list or the own-author rule. 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 f45e306a..9715d27e 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`, 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 (#925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association and `selfReview` do not already qualify. 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 (#925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association does not already qualify, and that is not this pull request's own author listed in `selfReview`. 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 From 381b94c1ed4524931eb43bd3820655921f0e54b3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 24 Oct 2026 07:12:38 +0000 Subject: [PATCH 7/8] docs(gate): the permission API is not the own-author carve-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md and autonomous-dev.md cited the collaborator permission API as the #925 recovery. That own-pull-request PASS is already selfOnly (#926, #928). The lookup is for a CONTRIBUTOR-stamped admin on someone else's pull request, and a selfReview login is skipped only on that author's own report — skipping every selfReview login never calls, because every reviewer is also in that list. The someone-else's-PR tests now require the collaborators//permission call. Fixes #936 Co-authored-by: Hongsheng Liu --- .github/scripts/enqueue-guard.test.mjs | 2 +- .github/scripts/review-gate.test.mjs | 7 ++++--- AGENTS.md | 2 +- docs/autonomous-dev.md | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/scripts/enqueue-guard.test.mjs b/.github/scripts/enqueue-guard.test.mjs index 8868ba33..fabad4c6 100644 --- a/.github/scripts/enqueue-guard.test.mjs +++ b/.github/scripts/enqueue-guard.test.mjs @@ -325,5 +325,5 @@ test('#925: a selfReview login with admin on someone else\'s pull request is acc 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.filter((path) => path.includes('/permission')).length, 1) + assert.ok(gh.calls.includes('repos/ThinkFlowLab/FlyBot/collaborators/Birfy/permission')) }) diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index befcccf2..78ea0ba6 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -638,8 +638,9 @@ test('#925: OWNER, MEMBER and COLLABORATOR still count, and a bare CONTRIBUTOR d assert.match(bare.reason, /author association CONTRIBUTOR/) }) -// #925: the shape the admin rule exists for — a login in `selfReview` who is NOT this pull request's -// author. `selfOnly` cannot cover it, so the permission API decides, and it must actually be asked. +// A selfReview login who is not this pull request's author is not covered by selfOnly (#928). +// The permission API is what can qualify that CONTRIBUTOR stamp, and it must actually be asked. +// The own-pull-request PASS is selfOnly (#926, #928); this lookup does not recover it. test('#925: a selfReview login judging someone else\'s pull request counts when the API reports admin', () => { const p = { reviewers: ['hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer'] } const body = report() @@ -648,7 +649,7 @@ test('#925: a selfReview login judging someone else\'s pull request counts when const gh = (_cmd, path) => { calls.push(path); return JSON.stringify({ permission: 'admin', role_name: 'admin' }) } const admins = adminsFor({ gh, repo: 'ThinkFlowLab/FlyBot', reports, policy: p, author: 'opener' }) assert.deepEqual(admins, ['hsliuustc0106']) - assert.equal(calls.length, 1, 'the permission API is asked once') + assert.deepEqual(calls, ['repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission']) const r = decide({ head: HEAD, author: 'opener', reports: [comment('hsliuustc0106', body, '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR')], policy: p, admins }) assert.equal(r.ok, true, r.reason) diff --git a/AGENTS.md b/AGENTS.md index 12b549b9..2d290161 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 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 (#925, when GitHub stamps the review `CONTRIBUTOR`). 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 conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association does not already qualify, and that is not this pull request's own author listed in `selfReview`. Admin does not skip the reviewers list or the own-author rule. +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. 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 9715d27e..163b5256 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`, 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 (#925). 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 asked only for a conclusion (a Tip SHA and a Verdict) by a login in `reviewers` that the association does not already qualify, and that is not this pull request's own author listed in `selfReview`. 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. 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 counting only on its own pull request (#928), a repo admin whose association is `CONTRIBUTOR` (#925), a `CONTRIBUTOR` who is neither still refused, 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): From 2288844f3d191d9e884a190897054d1fe30392a7 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 24 Oct 2026 15:16:27 +0800 Subject: [PATCH 8/8] docs(gate): say that selfReview and admin do not replace each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The own-author carve-out answers one question only — may this author review their own pull request — and gives that login no right on anyone else's. Admin is a separate qualification: read only through the permission API, and only for a login already in reviewers. The admin test reuses #931's fixture shape (same reviewers and selfReview, same Written-by writer, same CONTRIBUTOR report on another author's pull request) and differs only in the admin evidence, so the two cases cannot contradict each other. Fixes #936 --- .github/scripts/review-gate.test.mjs | 32 ++++++++++++++++------------ AGENTS.md | 2 +- docs/autonomous-dev.md | 2 +- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/.github/scripts/review-gate.test.mjs b/.github/scripts/review-gate.test.mjs index 78ea0ba6..2737784b 100644 --- a/.github/scripts/review-gate.test.mjs +++ b/.github/scripts/review-gate.test.mjs @@ -638,27 +638,31 @@ test('#925: OWNER, MEMBER and COLLABORATOR still count, and a bare CONTRIBUTOR d assert.match(bare.reason, /author association CONTRIBUTOR/) }) -// A selfReview login who is not this pull request's author is not covered by selfOnly (#928). -// The permission API is what can qualify that CONTRIBUTOR stamp, and it must actually be asked. -// The own-pull-request PASS is selfOnly (#926, #928); this lookup does not recover it. -test('#925: a selfReview login judging someone else\'s pull request counts when the API reports admin', () => { - const p = { reviewers: ['hsliuustc0106'], selfReview: ['hsliuustc0106'], writers: ['Reviewer'] } - const body = report() - const reports = [{ login: 'hsliuustc0106', association: 'CONTRIBUTOR', body }] +// 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: p, author: 'opener' }) - assert.deepEqual(admins, ['hsliuustc0106']) - assert.deepEqual(calls, ['repos/ThinkFlowLab/FlyBot/collaborators/hsliuustc0106/permission']) + 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: [comment('hsliuustc0106', body, '2026-10-18T01:00:00Z', 'comment', 'CONTRIBUTOR')], policy: p, admins }) + const r = decide({ head: HEAD, author: 'opener', reports, policy: selfPolicy, admins }) assert.equal(r.ok, true, r.reason) assert.equal(r.by, 'hsliuustc0106') - // On its own pull request the carve-out already qualifies it, so the lookup is skipped — that is - // the shape #931 pins, and this test must not restate it (the #931 case stays the only red there). + // 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: p, author: 'hsliuustc0106' }) + 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 2d290161..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 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 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 163b5256..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`, 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. 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