diff --git a/.github/scripts/merge-gate.mjs b/.github/scripts/merge-gate.mjs index 39cb6788e..d8c430f11 100644 --- a/.github/scripts/merge-gate.mjs +++ b/.github/scripts/merge-gate.mjs @@ -17,10 +17,17 @@ // A `pull_request`-time result is never reused: on #610 the tip report ran at 10:31:08Z and the INC // landed at 11:24:37Z, 54 minutes later, so the verdict is re-read here, live, for this head. // -// A review's `state` is deliberately not read. The account cannot approve its own pull requests, so -// its reviews stay `COMMENTED` even when it says INC, and the INC on #610 was a comment, not a review. -// A body that carries only a `## Review —` heading and no `Verdict:` line is "no conclusion", which -// refuses: the refusal direction never depends on reading more of the text than `decide` does. +// A review's `state` is deliberately not read *as a verdict*. The account cannot approve its own pull +// requests, so its reviews stay `COMMENTED` even when it says INC, and the INC on #610 was a comment, +// not a review. A body that carries only a `## Review —` heading and no `Verdict:` line is "no +// conclusion", which refuses: the refusal direction never depends on reading more of the text than +// `decide` does. +// +// `state` is read for the other half of a T2/T3 entry — the maintainer's approve (#691). Until it was +// read, that half was enforced by nobody: #675, #676, #682, #684, #685 and #686 merged on 2026-10-22 +// on a green `merge_group` run with zero reviews of any kind. An approval is a review, and the API +// says everything the rule needs: `state`, `commit_id`, the author and its `user.type`, plus the +// account's repository `permission`, which the caller resolves and never guesses from an association. import { appendFileSync } from 'node:fs' import { execFileSync } from 'node:child_process' import { fileURLToPath } from 'node:url' @@ -66,6 +73,11 @@ const fileNames = (files) => fileList(files).map((f) => f.filename) // labels: every Bot shares one account and a label carries no signature (#627), so whoever can label // could turn a T3 into "no review needed" — which is the whole gate. const NO_PASS_TIERS = new Set(['T0', 'T1']) +// #691: the tiers whose entry also needs one approved review. The exempt path (#640) covers T0/T1 and +// the mechanical release PR, which carries a standing approval of its own, so neither is listed here. +const APPROVAL_TIERS = new Set(['T2', 'T3']) +const APPROVED = 'APPROVED' +const APPROVER_PERMISSIONS = new Set(['admin', 'write', 'maintain']) const RELEASE_FILES = [ /^CHANGELOG\.md$/, /^changelog\.d\//, @@ -88,11 +100,39 @@ export const exemptFromPass = ({ tier, files }) => { } const short = (s) => String(s ?? '').slice(0, SHORT) +// The one approval that counts for an entry, or null (#691). Every clause refuses rather than passes: +// - only a review whose `state` is `APPROVED`; a dismissed approval comes back `DISMISSED`; +// - its `commit_id` must name the head being merged — an approval at an older tip is not this head, +// the same rule a PASS gets; +// - a `Bot` account approves nothing: the rule exists for a person, and the API says which is which; +// - the account needs repository write access. `permission` is resolved by the caller from +// `GET /repos/{o}/{r}/collaborators/{login}/permission`, never inferred from an association: +// `CONTRIBUTOR` on a fork review is not write access; +// - never the pull request's own author. `Birfy` authors every agent pull request, and GitHub +// already refuses a self-approval; this keeps the queue's answer identical to GitHub's. +export const approvalAt = (approvals, { head, author } = {}) => { + const target = String(head ?? '').toLowerCase() + if (!target) return null + const own = String(author ?? '').toLowerCase() + for (const a of approvals ?? []) { + if (String(a?.state ?? '').toUpperCase() !== APPROVED) continue + const commit = String(a?.commit_id ?? '').toLowerCase() + if (!commit || !commit.startsWith(target)) continue + if (String(a?.type ?? '').toLowerCase() === 'bot') continue + if (!APPROVER_PERMISSIONS.has(String(a?.permission ?? '').toLowerCase())) continue + if (!a?.login || String(a.login).toLowerCase() === own) continue + return a + } + return null +} + // labels: the pull request's labels. head: its current head sha, the one judged in both modes. // 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' }) { +// approvals: its reviews, as the API returns them (`{ login, state, commit_id, type, permission }`, +// #691) — injected like the labels, so the rule stays pure and a test drives it without the network. +export function judge({ labels, files, head, author, reports, approvals = [], policy, expectedHead, mode = 'queue' }) { const reasons = [] const target = String(head ?? '').toLowerCase() if (!target) return { ok: false, head: '', verdict: null, reasons: ['no head sha to judge'] } @@ -111,11 +151,17 @@ export function judge({ labels, files, head, author, reports, policy, expectedHe const exempt = exemptFromPass({ tier, files }) const verdict = exempt ? { ok: true, verdict: null, by: null, exempt: true } : decide({ head: target, author, reports, policy }) if (!verdict.ok) reasons.push(verdict.reason) + // #691: the human half of a T2/T3 entry, which nothing enforced before this: six pull requests + // merged on 2026-10-22 with zero reviews. The exemption is shared with the PASS on purpose — a T0/T1 + // routine fix and the standing-approval release PR must not start waiting for a click. + const needsApproval = !exempt && APPROVAL_TIERS.has(tier) + const approval = needsApproval ? approvalAt(approvals, { head: target, author }) : null + if (needsApproval && !approval) reasons.push(`${tier} entry has no approved review`) // 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 // comment starts no run of its own (#639). const report = mode === 'report' - return { ok: report ? true : reasons.length === 0, head: target, held, verdict, exempt, tier, reasons, mode } + return { ok: report ? true : reasons.length === 0, head: target, held, verdict, exempt, tier, needsApproval, approval, reasons, mode } } export function render(result, { pr, labels } = {}) { @@ -134,11 +180,16 @@ export function render(result, { pr, labels } = {}) { : result.ok && result.exempt ? `Queue entry for #${pr} may merge without a PASS: ${result.reasons.length ? 'no \`hold\`' : `${result.tier ? 'a ' + result.tier : 'a T0/T1'} tier or a mechanical release PR (AGENTS.md 6), computed from the changed files`}.` : result.ok - ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}.` + ? `Queue entry for #${pr} may merge: no \`hold\`, ${result.verdict?.by}'s PASS names ${short(result.head)}${result.approval ? `, approved by ${result.approval.login}` : ''}.` : `Queue entry for #${pr} must not merge: ${result.reasons.join('; ')}.`, '', `Pull request #${pr}, head \`${result.head}\`, labels: ${(labels ?? []).join(', ') || '(none)'}.`, ] + if (result.needsApproval) { + out.push('', result.approval + ? `Approval at this head: ${result.approval.login} (\`${result.approval.permission}\`) approved \`${short(result.approval.commit_id)}\`.` + : `No approved review at this head: a ${result.tier} entry needs one, from an account that is not the author, is not a Bot, and has write access.`) + } const seen = (result.verdict?.judged ?? []).filter((e) => e.counts || e.verdict) if (seen.length) out.push('', 'Conclusive texts read (newest last):', ...seen.slice(-5).map((e) => `- ${e.login} (${e.kind}): ${e.verdict ?? 'no verdict'} at ${short(e.tip ?? 'no tip')}${e.counts ? '' : ' (not counted)'}`)) out.push('', '_Rules: [`AGENTS.md` → Merge gates](../../AGENTS.md#merge-gates). This check refuses the entry; it never labels, comments, approves or merges._') @@ -169,12 +220,38 @@ export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prO const head = pull.head?.sha const author = pull.user?.login const labels = (pull.labels ?? []).map((l) => l.name) + // One read serves both halves: a review body may carry the PASS/INC (`decide`), and a review's + // `state` is the approval (#691). Filtering the bodies out of the reports, as before, keeps + // `decide` judging exactly what it judged until now. + const reviews = list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) const reports = [ ...list(`repos/${repo}/issues/${pr}/comments?per_page=100`).map((c) => ({ kind: 'comment', login: c.user?.login, body: c.body, at: c.created_at, association: c.author_association })), - ...list(`repos/${repo}/pulls/${pr}/reviews?per_page=100`) + ...reviews .filter((v) => (v.body ?? '').trim()) .map((v) => ({ kind: 'review', login: v.user?.login, body: v.body, at: v.submitted_at, association: v.author_association })), ] + // Only an `APPROVED` review can be an approval, so only those cost a permission lookup; the + // endpoint answers 404 for an account with no access to the repository, which is a "no write + // access" answer and not a reason to fail the whole judgement. One lookup per login. + const permissions = new Map() + const permissionOf = (login) => { + if (!login) return null + if (!permissions.has(login)) { + let permission = null + try { + permission = JSON.parse(gh('api', `repos/${repo}/collaborators/${login}/permission`))?.permission ?? null + } catch { permission = null } + permissions.set(login, permission) + } + return permissions.get(login) + } + const approvals = reviews.map((v) => ({ + login: v.user?.login, + state: v.state, + commit_id: v.commit_id, + type: v.user?.type, + permission: String(v.state ?? '').toUpperCase() === APPROVED ? permissionOf(v.user?.login) : null, + })) // The workflow passes both events through this same entry point: `merge_group` judges, `pull_request` // reports (#639). MODE (or `--report`) is there for a hand run and for the tests. @@ -182,12 +259,15 @@ 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 result = judge({ labels, files, head, author, reports, approvals, policy: loadPolicy(policyPath), 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 : '') console.log(`merge-gate: PR #${pr} head ${short(head)}${base ? `, queue base ${short(base)}` : ''}${entry ? `, entry ${short(entry)}` : ''}, labels [${labels.join(', ')}], mode ${mode}`) console.log(`merge-gate: newest conclusion at ${short(result.head)} — ${result.verdict?.verdict ?? 'none'}${result.verdict?.by ? ` by ${result.verdict.by}` : ''}`) + if (result.needsApproval) { + console.log(`merge-gate: approved review at ${short(result.head)} — ${result.approval ? `${result.approval.login} (${result.approval.permission})` : 'none'}`) + } console.log(mode === 'report' ? `merge-gate: report — ${result.reasons.length ? `the queue entry would refuse: ${result.reasons.join('; ')}` : 'the queue entry would pass'}` : `merge-gate: ${result.ok ? 'PASS' : `REFUSE — ${result.reasons.join('; ')}`}`) diff --git a/.github/scripts/merge-gate.test.mjs b/.github/scripts/merge-gate.test.mjs index f3198b581..763d3689b 100644 --- a/.github/scripts/merge-gate.test.mjs +++ b/.github/scripts/merge-gate.test.mjs @@ -130,12 +130,20 @@ import { mkdtempSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -const fakeGh = ({ comments, reviews = [], labels = [], files = [], head = HEAD }) => (...args) => { +const fakeGh = ({ comments, reviews = [], approvals = [], labels = [], files = [], head = HEAD }) => (...args) => { const path = args[args.length - 1] + // #691: the permission of an approver is resolved through the API, never read off the review, and + // an account with no access answers 404 — which is `execFileSync` throwing here. + const who = /\/collaborators\/([^/]+)\/permission$/.exec(path) + if (who) { + const known = [...approvals].flat().find((entry) => entry.user?.login === who[1] && entry.permission) + if (!known) throw new Error(`fake gh: 404 for the permission of ${who[1]}`) + return JSON.stringify({ permission: known.permission }) + } const pull = { head: { sha: head }, user: { login: 'opener' }, labels: labels.map((name) => ({ name })) } if (path.includes('/files')) return (files ?? []).map((filename) => JSON.stringify({ filename })).join('\n') if (path.includes('/comments')) return [comments].flat().map((entry) => JSON.stringify(entry)).join('\n') - if (path.includes('/reviews')) return [reviews].flat().map((entry) => JSON.stringify(entry)).join('\n') + if (path.includes('/reviews')) return [...[...reviews].flat(), ...[...approvals].flat()].map((entry) => JSON.stringify(entry)).join('\n') if (/\/pulls\/\d+$/.test(path)) return JSON.stringify(pull) throw new Error('fake gh: unexpected ' + path) } @@ -154,6 +162,22 @@ const runGate = (fixture) => { const apiComment = (body, at) => ({ user: { login: 'reviewer-agent' }, body, created_at: at, author_association: 'MEMBER' }) const apiReview = (body, at) => ({ user: { login: 'reviewer-agent' }, body, submitted_at: at, author_association: 'MEMBER' }) +// #691: the approve a queue entry needs. `permission` is not part of the API's review object — it +// comes from `GET /repos/{o}/{r}/collaborators/{login}/permission`, which the fake resolves by login, +// so a test that forgets the endpoint fails the way the real run would. +const apiApproval = ({ head = HEAD, login = 'hsliuustc0106', permission = 'admin', state = 'APPROVED', type = 'User', commit = head } = {}) => ({ + user: { login, type }, + state, + commit_id: commit, + submitted_at: '2026-10-22T06:00:00Z', + body: '', + permission, +}) +// The shape `judge()` takes: the workflow flattens the API review into these five fields. +const flatApproval = (over = {}) => { + const a = apiApproval(over) + return { login: a.user.login, state: a.state, commit_id: a.commit_id, type: a.user.type, permission: a.permission } +} const firstLine = (verdict, tip = HEAD) => [ `Verdict: ${verdict} at ${tip}`, @@ -276,7 +300,7 @@ const runQueue = (fixture) => { writeFileSync(policyFile, JSON.stringify({ reviewers: ['reviewer-agent'], selfReview: [], writers: ['Reviewer'] })) // Exactly what merge-gate.yml hands the script on a merge_group run, and no PR number. return main({ - gh: fakeGh({ head: RUN_HEAD, files: RUN_FILES, ...fixture }), + gh: fakeGh({ head: RUN_HEAD, files: RUN_FILES, approvals: [apiApproval({ head: RUN_HEAD })], ...fixture }), env: { GITHUB_STEP_SUMMARY: '', REVIEW_POLICY: policyFile, @@ -348,7 +372,8 @@ const queueReal = (fx, body = fx.comment.body) => main({ const path = args[args.length - 1] if (path.includes('/files')) return fx.files.map((filename) => JSON.stringify({ filename })).join('\n') if (path.includes('/comments')) return JSON.stringify({ user: { login: fx.comment.login }, body, created_at: fx.comment.created_at, author_association: 'OWNER' }) - if (path.includes('/reviews')) return '' + if (/\/collaborators\/[^/]+\/permission$/.test(path)) return JSON.stringify({ permission: 'admin' }) + if (path.includes('/reviews')) return JSON.stringify({ user: { login: 'hsliuustc0106', type: 'User' }, state: 'APPROVED', commit_id: fx.head, submitted_at: '2026-10-22T06:00:00Z', body: '' }) if (/\/pulls\/\d+$/.test(path)) return JSON.stringify({ head: { sha: fx.head }, user: { login: fx.author }, labels: [] }) throw new Error('fake gh: unexpected ' + path) }, @@ -401,3 +426,70 @@ 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/) }) + +// ---- #691: the human half of a T2/T3 entry. Six pull requests (#675, #676, #682, #684, #685, #686) +// merged on 2026-10-22 on a green `merge_group` run with zero reviews of any kind, so before this rule +// "T2/T3 need a maintainer approve at the tip plus a PASS" was enforced by nobody. +const passed = (tip = RUN_HEAD) => apiComment(firstLine('PASS', tip), '2026-10-22T00:30:00Z') +// #654's own diff is T2 (core/src/host/**); a change to a T3 host file is what names the T3 refusal. +const T3_FILES = ['core/src/host/channel.js', 'changelog.d/691-approval.fixed.md'] + +test('#691: a T3 entry with a PASS and no approved review refuses', () => { + const r = runQueue({ files: T3_FILES, comments: [passed()], approvals: [] }) + assert.equal(r.ok, false) + assert.equal(r.tier, 'T3') + assert.equal(r.needsApproval, true) + assert.equal(r.approval, null) + assert.match(r.reasons.join('; '), /T3 entry has no approved review/) + assert.equal(r.verdict.verdict, 'PASS', 'the PASS half is unchanged: this refusal is the new one') +}) + +test('#691: one approved review at the head, from a non-author account with write access, lets it through', () => { + const r = runQueue({ comments: [passed()], approvals: [apiApproval({ head: RUN_HEAD })] }) + assert.equal(r.ok, true, r.reasons.join('; ')) + assert.equal(r.approval.login, 'hsliuustc0106') + assert.equal(r.approval.commit_id, RUN_HEAD) +}) + +test('#691: a T0 file set still needs no approval, and the report run still never fails', () => { + const t0 = judgeAt({ files: ['docs/subsystems/core-host.md'], reports: [], approvals: [] }) + assert.equal(t0.ok, true, 'the exempt path is unchanged (#640)') + assert.equal(t0.needsApproval, false) + assert.equal(t0.approval, null) + assert.equal(t0.tier, 'T0') + // The release PR is exempt for the same reason: it carries a standing approval. + const release = ['CHANGELOG.md', 'changelog.d/691-x.fixed.md', 'core/package.json'] + assert.equal(judgeAt({ files: release, reports: [], approvals: [] }).needsApproval, false) + // A `pull_request` run reports the refusal and still passes, as it does for every other refusal. + const report = judgeAt({ files: ['core/src/host/turns.js'], reports: [say()], approvals: [], mode: 'report' }) + assert.equal(report.ok, true, 'the report never fails') + assert.equal(report.needsApproval, true, 'it still says an approval is owed') + assert.equal(report.approval, null) +}) + +test('#691: an approval that does not count — older tip, the author, a Bot, read access, dismissed', () => { + const atHead = { files: T3_FILES, comments: [passed()] } + const withApproval = (a) => runQueue({ ...atHead, approvals: [a] }) + const refuses = (r, why) => { + assert.equal(r.ok, false, why) + assert.match(r.reasons.join('; '), /T3 entry has no approved review/, why) + } + refuses(withApproval(apiApproval({ head: RUN_HEAD, commit: 'f'.repeat(40) })), 'an approval at an older tip is not this head') + refuses(withApproval(apiApproval({ head: RUN_HEAD, login: 'opener' })), 'the author approves nothing') + refuses(withApproval(apiApproval({ head: RUN_HEAD, type: 'Bot' })), 'a Bot is not the human the rule exists for') + refuses(withApproval(apiApproval({ head: RUN_HEAD, permission: 'read' })), 'read access is not write access') + refuses(withApproval(apiApproval({ head: RUN_HEAD, permission: null })), 'no permission resolved is no write access') + refuses(withApproval(apiApproval({ head: RUN_HEAD, state: 'DISMISSED' })), 'a dismissed approval does not count') + refuses(withApproval(apiApproval({ head: RUN_HEAD, state: 'COMMENTED' })), 'a comment is not an approval') + assert.equal(withApproval(apiApproval({ head: RUN_HEAD, permission: 'write' })).ok, true, 'write is enough') + assert.equal(withApproval(apiApproval({ head: RUN_HEAD, permission: 'maintain' })).ok, true, 'maintain is enough') +}) + +test('#691: a T2 entry needs the approval too, and only the queue entry refuses', () => { + const t2 = { files: ['core/src/host/turns.js'], reports: [say()], approvals: [] } + const report = judgeAt({ ...t2, mode: 'report' }) + assert.equal(report.ok, true) + assert.match(report.reasons.join('; '), /T2 entry has no approved review/) + assert.equal(judgeAt({ ...t2, mode: 'queue' }).ok, false) + assert.equal(judgeAt({ ...t2, mode: 'queue', approvals: [flatApproval({ head: HEAD })] }).ok, true, 'judge() takes the shape the workflow hands it') +}) diff --git a/AGENTS.md b/AGENTS.md index 1cf433a90..93c658364 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -137,14 +137,16 @@ What this buys is presence and a name from a closed list, not truth: with one sh ### Merge gates -Four rules decide whether a pull request may enter the queue. The mechanical one runs where the merge actually happens: [`merge-gate.yml`](.github/workflows/merge-gate.yml) fails the `merge-gate` job on a `merge_group` entry, and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs) does the judging. +Five rules decide whether a pull request may enter the queue. The mechanical one runs where the merge actually happens: [`merge-gate.yml`](.github/workflows/merge-gate.yml) fails the `merge-gate` job on a `merge_group` entry, and [`merge-gate.mjs`](.github/scripts/merge-gate.mjs) does the judging. 1. **`hold` blocks mechanically.** A pull request labelled `hold` fails `merge-gate` on its queue entry. The label is only a marker; the job is the gate. It blocks the queue once `merge-gate` is listed among the required checks of the `main merge queue` ruleset — a ruleset change for the maintainer, never an agent; until then the job reports without blocking. 2. **Labelling a pull request `hold` means adding a record.** Add the label **and** post a new comment saying **Record only — do not merge / 仅记录、禁止入队**. The pair is what holds it. Add that comment; never edit or delete an earlier PASS report to change what it said — the history is the record, and rewriting it is the thing that reads as an incident afterwards. 3. **An authorisation is not a queue permit.** The owner or the Chief saying "let it through", "claim it" or "handle it along with the rest" licenses no specific pull request for the queue. Entering the queue is an explicit action on that one pull request, after its own gate passed, while it carries no `hold`. #566 is the case this exists for: "handle it along with the rest" was read as a permit, the tip was PASSed at 09:48:18Z, enqueued at 09:48:28Z and merged at 09:50:40Z as `abaf938b`. 4. **The newest conclusion at the head being merged decides.** Read the pull request's comments and review bodies, live, for the exact head SHA being merged — the pull request's own head, on a queue entry too (the `` in `gh-readonly-queue/main/pr--` is the entry's base, not its head; #654) — and take the newest text that carries `Verdict:` or a `## Review —` heading: INC or REQUEST_CHANGES refuses, PASS allows, no conclusion refuses too, and so does a report with no `Written-by:` line or one that names a writer the policy does not list (#627). Since #637 the format is fixed: the **first line** of the conclusion is exactly `Verdict: PASS at ` / `Verdict: INC at ` / `Verdict: REQUEST_CHANGES at `, with the attribution on the second line (`Written-by: Reviewer`, and a relay adds `Posted-by: Chief (proxy…)`; the older prose `Written by: …` reads the same way) and the evidence below it — a body that carries a verdict anywhere else is no conclusion, and no conclusion refuses. A result from `pull_request` time never stands in — on #610 the tip report ran at 10:31:08Z and the INC landed at 11:24:37Z, 54 minutes later. The gating agent checks this before enqueueing; `merge-gate.yml` checks it again at `merge_group`, and the stricter answer wins. That job runs on two events: at `merge_group` it judges and a refusal fails it, and at `pull_request` it only reports what the queue entry would refuse — a required check that never reports on the pull request head leaves every pull request waiting for a status that never comes and unable to enter the queue at all (#639). The required status check is the job name `merge-gate`; the workflow name `Merge gate` is not a context any run produces. -A review's `state` is not what is read: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. +5. **A T2/T3 entry also needs one approved review at its head (#691).** `merge-gate` refuses a T2/T3 queue entry that carries no `APPROVED` review naming the head being merged, with ` entry has no approved review`. An approval counts only when every clause holds: its `commit_id` is the judged head (an approval at an older tip is not this head, the same rule a PASS gets); the account is not a `Bot`; its repository permission is `admin`, `write` or `maintain`, resolved from the collaborators API rather than inferred from an association; and it is not the pull request's author. A `DISMISSED` approval does not count, and T0/T1 plus the standing-approval release PR skip it together with the PASS. Until #691 this half was enforced by nobody: six pull requests merged on 2026-10-22 on a green `merge_group` run with zero reviews of any kind. The consequence is deliberate — every agent pull request is authored by `Birfy` and GitHub refuses a self-approval, so the only possible approver is the maintainer's own account, and a T2/T3 `merge-gate` refusal that names this rule means **waiting for the human**, not a regression. The owner's 2026-10-22 rule keeps both halves, a PASS at the tip and a maintainer approve; this is the mechanical half. + +A review's `state` is not read *as a verdict*: the account cannot approve its own pull requests, so its reviews stay `COMMENTED` even for an INC, and the INC on #610 was a comment rather than a review. It is read for the approve above, which is a different question. **The T0/T1 exemption is computed, never labelled.** `merge-gate.mjs` runs the base copy of [`risk-gate.mjs`](.github/scripts/risk-gate.mjs) over the changed files and `.github/agent/risk-rules.json` to get the tier; a `risk:T0`/`risk:T1` label does nothing. Every Bot shares one account and a label carries no signature (#627), so a label-based exemption would let whoever can label turn a T3 into "no review needed" — the gate itself. A mechanical release PR is recognised by its files as before, and `hold` still refuses whatever the tier. diff --git a/docs/autonomous-dev.md b/docs/autonomous-dev.md index 1a3857538..07ad7f937 100644 --- a/docs/autonomous-dev.md +++ b/docs/autonomous-dev.md @@ -187,7 +187,7 @@ agent reads, and the record a later enforcement step is checked against. `main` requires a **merge queue** (repository ruleset `main merge queue`, active since 2026-10-17). `test.yml` has a `merge_group:` trigger, so every queue entry runs `Test`. -**The gates themselves** — `hold`, the newest-conclusion rule, and the `merge-gate` job that enforces both on a queue +**The gates themselves** — `hold`, the newest-conclusion rule, the T2/T3 approve, and the `merge-gate` job that enforces them on a queue entry — are in [`AGENTS.md` → Merge gates](../AGENTS.md#merge-gates). This section is only the plumbing. **How to merge.** A direct `gh pr merge --squash` is refused with `! The merge strategy for main is set by the merge