From 7c22347a300e04fa493a4f014f71d7901afb84b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 18:39:47 +0000 Subject: [PATCH] fix(ci): the ADR merge gate requires an APPROVED review, from anyone The gate matched the approving account against a hard-coded `MAINTAINER_APPROVERS` list. That proxy became unsatisfiable once cloud sessions began authoring PRs under the maintainer's own account: GitHub forbids self-approval, so a required context was permanently red on exactly the PRs the human was driving, while an AI seat holding the same credential could still satisfy it. Per the maintainer ruling of 2026-08-12, the account list is retired rather than widened: the gate now passes when the PR's latest state-setting review is APPROVED, whoever submitted it. The revocation semantics are unchanged -- a later CHANGES_REQUESTED or DISMISSED still turns it red, and COMMENTED/PENDING still set nothing. - `MAINTAINER_APPROVERS` and `isMaintainer` are removed; `latestMaintainerReviewState` becomes `latestReviewState`, folding over every reviewer; `approvalsFromNonMaintainers` becomes `approverLogins`, a pure diagnostic that no longer asserts a distinction the gate does not draw. - Both file headers are rewritten rather than patched: they now state what the gate guarantees (someone approved, currently) and what it no longer does (that the approver is the maintainer, that a human merged). - The job name `ADR maintainer approval` is deliberately unchanged -- it is the required-context string in the `main` ruleset. --- .github/workflows/adr-merge-approval.yml | 51 ++-- scripts/check-adr-merge-approval.mjs | 293 +++++++++++++---------- scripts/check-required-contexts.mjs | 5 +- 3 files changed, 209 insertions(+), 140 deletions(-) diff --git a/.github/workflows/adr-merge-approval.yml b/.github/workflows/adr-merge-approval.yml index 73888789e8..882965ff66 100644 --- a/.github/workflows/adr-merge-approval.yml +++ b/.github/workflows/adr-merge-approval.yml @@ -1,17 +1,28 @@ name: ADR Merge Approval -# Machine enforcement of the #6741 ruling (maintainer, verbatim): -# 「adr 只能由维护者自己确认,人工合并,ai 不得擅自合并。」 +# Machine enforcement of the 2026-08-12 ruling (maintainer, verbatim): +# 「门禁改成只要求「APPROVED review 存在」」/「不要指定具体的人」 # -# A PR whose diff touches docs/adr/** must carry an APPROVED review from the -# maintainer's OWN account before it is mergeable; approvals from the shared -# bot/agent identities deliberately do not count. Prose enforcement was -# measured insufficient the day the ruling landed — two different AI-operated -# seats merged docs/adr/** PRs within the following hour (#6671, #6732; the -# full record and both replays live in scripts/check-adr-merge-approval.mjs -# and its --self-test). Drafting ADR PRs stays open to every seat; only the -# merge is reserved, and the maintainer's own approval + merge is the intended -# zero-extra-friction green path. +# A PR whose diff touches docs/adr/** must carry an APPROVED review before it +# is mergeable. The gate does NOT check WHO approved: any account with review +# rights on this repo — including an AI seat — satisfies it, which is the +# accepted cost of the ruling and is stated in full in the two-clause table at +# the head of scripts/check-adr-merge-approval.mjs. The approval must be +# current: a later CHANGES_REQUESTED or DISMISSED revokes it. +# +# This supersedes the account-identity rule this workflow used to describe +# (#6741 「adr 只能由维护者自己确认,人工合并,ai 不得擅自合并。」, enforced by +# matching the maintainer's numeric account id). That proxy became +# unsatisfiable once cloud sessions began authoring PRs under the maintainer's +# own account, since GitHub forbids self-approval (#8161). #6741's two halves +# survive as convention, not as anything this workflow can measure. +# +# Prose enforcement was measured insufficient the day #6741 landed — two +# different AI-operated seats merged docs/adr/** PRs within the following hour +# (#6671, #6732; the full record and both replays live in +# scripts/check-adr-merge-approval.mjs and its --self-test). Both had ZERO +# reviews of any kind, so both stay red under the widened rule too. Drafting +# ADR PRs stays open to every seat; only the merge is gated. # # Deliberately NO `paths` filter, on either trigger — the same choice # changeset-presence.yml made in objectui (#3769) and for the same reason @@ -30,9 +41,11 @@ on: pull_request: branches: [main] # An approval does not fire `pull_request`, so without this trigger the - # failed check would sit red after the maintainer approves until someone - # re-ran it by hand. Subscribing to reviews makes the maintainer's approval - # itself re-run the gate — the zero-friction green path the card requires. + # failed check would sit red after the approval lands until someone re-ran + # it by hand. Subscribing to reviews makes the approval itself re-run the + # gate — the zero-friction green path the card requires. `pull-requests: + # read` below covers listing reviews from ANY account, so the widened rule + # needs no extra permission or token scope. # (On non-ADR PRs a review re-runs the cheap clean path; harmless.) pull_request_review: types: [submitted, edited, dismissed] @@ -55,6 +68,14 @@ permissions: jobs: adr-merge-approval: + # ⛔ Do NOT rename this job. Its name IS the required status-context string + # in the `main` ruleset (#7022), and it is registered under that exact + # spelling in scripts/check-required-contexts.mjs — renaming it here alone + # leaves the ruleset waiting for a context that never reports, which hangs + # the merge queue until the 60-minute timeout. The word "maintainer" now + # over-claims (see this file's header: any approver counts); correcting it + # is a settings action nobody in CI can perform, so it is tracked as + # follow-up work rather than done here. name: ADR maintainer approval runs-on: ubuntu-latest timeout-minutes: 5 @@ -77,7 +98,7 @@ jobs: # install, no build. The self-test runs first (repo convention), then # the gate. GITHUB_TOKEN is only read on the gated path (a docs/adr/** # diff needs the PR's review list); the clean path does zero lookups. - - name: Require the maintainer's own approval on docs/adr/** diffs + - name: Require an APPROVED review on docs/adr/** diffs env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: node scripts/check-adr-merge-approval.mjs --self-test && node scripts/check-adr-merge-approval.mjs diff --git a/scripts/check-adr-merge-approval.mjs b/scripts/check-adr-merge-approval.mjs index a08d5801b3..1f18eb2380 100644 --- a/scripts/check-adr-merge-approval.mjs +++ b/scripts/check-adr-merge-approval.mjs @@ -2,53 +2,75 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. // // check-adr-merge-approval -- a PR whose diff touches docs/adr/** must not be -// mergeable without an APPROVED review from the maintainer's own account. +// mergeable without an APPROVED review on it. Any account's approval counts. // // node scripts/check-adr-merge-approval.mjs # gate mode (CI and local) // node scripts/check-adr-merge-approval.mjs --pr 6671 # replay a PR via the live API // node scripts/check-adr-merge-approval.mjs --files-json f.json --reviews-json r.json // node scripts/check-adr-merge-approval.mjs --self-test # the checker itself // -// ## The ruling this enforces (#6741, maintainer, verbatim) +// ## The ruling this enforces (maintainer, 2026-08-12, verbatim) // -// 「adr 只能由维护者自己确认,人工合并,ai 不得擅自合并。」 +// 「门禁改成只要求「APPROVED review 存在」」 +// 「不要指定具体的人」 // -// ADR merges are reserved to the maintainer in person. Drafting and pushing -// ADR PRs stays open to every seat -- this gate reserves the MERGE, nothing -// else, and the maintainer's own approval + merge is the intended -// zero-extra-friction green path. +// The pass condition is the PRESENCE of an approving review. No account list, +// no identity judgement -- and, per the second sentence, no configurable one +// either: there is no list any more, not a list that moved somewhere else. +// +// ## What this gate does and does not guarantee -- read this before trusting it +// +// GUARANTEED (machine-enforced here) | NOT guaranteed (convention only) +// -------------------------------------+---------------------------------------- +// a docs/adr/** diff cannot reach a | that the approver is the maintainer. +// mergeable state with no approving | Any account with review rights on this +// review on the PR | repo -- INCLUDING an AI seat -- satisfies +// | this gate. That is the accepted cost of +// | the 2026-08-12 ruling, stated out loud. +// -------------------------------------+---------------------------------------- +// the approval is CURRENT, not | that a human performed the merge. This +// historical: a later CHANGES_REQUESTED| gate reads reviews only; it does not +// or DISMISSED revokes it and the | look at who merges, and does not block +// gate goes red again | auto-merge being ARMED (#8012) +// +// The 2026-08-12 ruling SUPERSEDES the account-identity proxy this file used +// to implement. That proxy came from #6741 (maintainer, verbatim) +// 「adr 只能由维护者自己确认,人工合并,ai 不得擅自合并。」 and was sound only +// while maintainer-account != author-account. It stopped being sound when +// cloud dev sessions began authoring under the maintainer's own account: +// GitHub forbids self-approval, so the gate became permanently red on exactly +// the PRs the human was driving, while an AI seat holding the same credential +// could still satisfy it (#8161 row 1, #8012 row 2). #6741's two halves are +// not deleted -- they are now CONVENTION, not machine-enforced, and this +// header says so rather than letting the code and the comment drift apart +// again. // // ## Why a machine gate rather than prose (#6785, measured) // -// The ruling was filed at 13:52Z on 2026-08-08. Within the following hour, two -// DIFFERENT AI-operated seats merged docs/adr/** PRs anyway: #6671 at 14:23Z -// (`os-zhuang`) and #6732 at 14:38Z (`os-project-manager` -- while the PR was -// in DRAFT state, so parking a PR as draft is not a barrier either). Neither -// merge was the maintainer's; both had ZERO reviews of any kind. A ruling -// written into an issue does not reach sessions that never read that issue. -// This repo's own doctrine -- declared = enforced -- applies to governance -// exactly as it applies to metadata. Both violations are replayed as -// fixtures in `--self-test`, pinned RED forever. +// The #6741 ruling was filed at 13:52Z on 2026-08-08. Within the following +// hour, two DIFFERENT AI-operated seats merged docs/adr/** PRs anyway: #6671 +// at 14:23Z (`os-zhuang`) and #6732 at 14:38Z (`os-project-manager` -- while +// the PR was in DRAFT state, so parking a PR as draft is not a barrier +// either). Neither merge was the maintainer's; both had ZERO reviews of any +// kind. A ruling written into an issue does not reach sessions that never read +// that issue. This repo's own doctrine -- declared = enforced -- applies to +// governance exactly as it applies to metadata. Both violations are replayed +// as fixtures in `--self-test`, pinned RED forever: they had no reviews at +// all, so they stay red under the widened rule too. // // ## The decision rule // // diff does not touch docs/adr/** -> PASS, with ZERO API lookups -// diff touches docs/adr/** -> PASS only if the PR carries an -// APPROVED review from the maintainer's -// own account (bot/agent approvals do -// not count) +// diff touches docs/adr/** -> PASS only if the PR's latest +// state-setting review is APPROVED // -// ## Why the approver set is a hard-coded constant -// -// `MAINTAINER_APPROVERS` below is deliberately NOT configurable via repo/org -// Actions variables, workflow inputs, or environment: every one of those -// surfaces is writable by the shared AI identities (`os-*`, `claude`, -// `yinlianghui`, ...), and those identities granting themselves approval -// rights is precisely the failure mode this gate exists to close (#6785). -// Matching is by numeric account ID, not login: a login can be released and -// re-registered by someone else; the ID cannot. Changing the accepted set is -// a governance change -- a reviewed PR to this file, which .github/CODEOWNERS -// routes to the maintainer. +// "Latest state-setting" rather than "any APPROVED review has ever existed": +// an unrevokable approval would be a one-way door -- approve once, force-push +// anything. APPROVED / CHANGES_REQUESTED / DISMISSED set the standing; +// COMMENTED and PENDING set nothing. With the account filter gone the fold +// runs over ALL reviewers, which is the strict direction on both edges: a +// CHANGES_REQUESTED from a SECOND reviewer revokes a first reviewer's +// approval (red), and it takes a fresh approval -- from anyone -- to clear it. // // ## Never a filtered trigger, never a silent skip // @@ -89,17 +111,6 @@ const scriptDir = dirname(fileURLToPath(import.meta.url)); /** The governed surface. A path prefix, matched against repo-relative paths. */ export const ADR_PATH_PREFIX = 'docs/adr/'; -/** - * The accounts whose APPROVED review satisfies this gate. See the header for - * why this is a hard-coded constant and why matching is ID-first. - * - * `hotlong` = 50353452 is verified, not assumed: 2,153 commits on `main` are - * authored as `50353452+hotlong@users.noreply.github.com`, and GitHub's - * noreply address form is `{id}+{login}@users.noreply.github.com`, which ties - * the login to the ID in the repo's own history. - */ -export const MAINTAINER_APPROVERS = [{ login: 'hotlong', id: 50353452 }]; - /** Review states that SET the reviewer's standing; COMMENTED/PENDING do not. */ const STATE_SETTING = new Set(['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED']); @@ -113,53 +124,48 @@ export function adrFilesIn(paths) { } /** - * Does this review's author count as the maintainer? - * - * ID-first: when the payload carries a numeric id (the real API always does), - * the id alone decides -- a review from an account merely NAMED like the - * maintainer, with a different id, does not count. The login fallback exists - * only for hand-built fixtures that omit ids. - */ -export function isMaintainer(user, approvers = MAINTAINER_APPROVERS) { - if (!user) return false; - return approvers.some((a) => (user.id != null ? user.id === a.id : user.login === a.login)); -} - -/** - * The maintainer's CURRENT review standing, from the full review list. + * The PR's CURRENT review standing, from the full review list -- no account + * filter, per the 2026-08-12 ruling 「不要指定具体的人」. * * Reviews are walked in submission order (the API returns them ascending; * `submitted_at` is used as the tiebreak-stable sort key when present). Only * APPROVED / CHANGES_REQUESTED / DISMISSED change the standing -- a later * COMMENTED does not revoke an approval, a later CHANGES_REQUESTED or a - * dismissal does. + * dismissal does, whoever submitted it. * - * @returns {string|null} the latest state-setting state, or null when the - * maintainer has never reviewed + * @returns {string|null} the latest state-setting state, or null when nobody + * has submitted a state-setting review */ -export function latestMaintainerReviewState(reviews, approvers = MAINTAINER_APPROVERS) { - const mine = reviews - .filter((r) => isMaintainer(r?.user, approvers)) +export function latestReviewState(reviews) { + const ordered = reviews .map((r, i) => ({ r, i })) .sort((a, b) => { - const ta = a.r.submitted_at ? Date.parse(a.r.submitted_at) : 0; - const tb = b.r.submitted_at ? Date.parse(b.r.submitted_at) : 0; + const ta = a.r?.submitted_at ? Date.parse(a.r.submitted_at) : 0; + const tb = b.r?.submitted_at ? Date.parse(b.r.submitted_at) : 0; return ta - tb || a.i - b.i; }); let state = null; - for (const { r } of mine) { - const s = String(r.state ?? '').toUpperCase(); + for (const { r } of ordered) { + const s = String(r?.state ?? '').toUpperCase(); if (STATE_SETTING.has(s)) state = s; } return state; } -/** Logins whose APPROVED reviews exist but deliberately do not count. */ -export function approvalsFromNonMaintainers(reviews, approvers = MAINTAINER_APPROVERS) { +/** + * Every login that has submitted an APPROVED review, for the verdict message. + * + * Purely diagnostic, and deliberately NOT a judgement: this replaces the old + * `approvalsFromNonMaintainers`, whose name asserted a maintainer/non- + * maintainer distinction the gate no longer draws. It earns its place on the + * RED path, where "someone approved, yet the standing is CHANGES_REQUESTED" + * is the confusing case a reader needs named. + */ +export function approverLogins(reviews) { return [ ...new Set( reviews - .filter((r) => String(r?.state ?? '').toUpperCase() === 'APPROVED' && !isMaintainer(r?.user, approvers)) + .filter((r) => String(r?.state ?? '').toUpperCase() === 'APPROVED') .map((r) => r?.user?.login ?? '(unknown)'), ), ]; @@ -175,9 +181,9 @@ export function approvalsFromNonMaintainers(reviews, approvers = MAINTAINER_APPR * @param {string[]} input.changedPaths repo-relative changed paths * @param {() => Promise} input.getReviews lazy review-list fetch * @returns {Promise<{ok: boolean, kind: string, adrFiles: string[], checked: number, - * state?: string|null, strangerApprovals?: string[]}>} + * state?: string|null, approvals?: string[]}>} */ -export async function decide({ changedPaths, getReviews, approvers = MAINTAINER_APPROVERS }) { +export async function decide({ changedPaths, getReviews }) { const adrFiles = adrFilesIn(changedPaths); const checked = changedPaths.length; if (adrFiles.length === 0) return { ok: true, kind: 'no-adr-diff', adrFiles, checked }; @@ -186,16 +192,10 @@ export async function decide({ changedPaths, getReviews, approvers = MAINTAINER_ if (!Array.isArray(reviews)) { throw new Error(`the review list is ${typeof reviews}, not an array -- refusing to guess (see header: missing input fails loud)`); } - const state = latestMaintainerReviewState(reviews, approvers); - if (state === 'APPROVED') return { ok: true, kind: 'maintainer-approved', adrFiles, checked, state }; - return { - ok: false, - kind: 'missing-maintainer-approval', - adrFiles, - checked, - state, - strangerApprovals: approvalsFromNonMaintainers(reviews, approvers), - }; + const state = latestReviewState(reviews); + const approvals = approverLogins(reviews); + if (state === 'APPROVED') return { ok: true, kind: 'approved', adrFiles, checked, state, approvals }; + return { ok: false, kind: 'missing-approval', adrFiles, checked, state, approvals }; } /** @@ -386,7 +386,7 @@ const fetchAssociatedPrs = ({ apiUrl, repo, token }, sha) => // -- reporting ---------------------------------------------------------------- -const RULING = '「adr 只能由维护者自己确认,人工合并,ai 不得擅自合并。」 (#6741, maintainer, verbatim)'; +const RULING = '「门禁改成只要求「APPROVED review 存在」」/「不要指定具体的人」 (maintainer, 2026-08-12, verbatim; #8161)'; function reportVerdict(verdict, { source }) { if (verdict.ok && verdict.kind === 'no-adr-diff') { @@ -397,32 +397,32 @@ function reportVerdict(verdict, { source }) { return 0; } if (verdict.ok) { + const who = (verdict.approvals ?? []).map((s) => `'${s}'`).join(', '); console.log( - `✅ ${verdict.adrFiles.length} file(s) under ${ADR_PATH_PREFIX} and an APPROVED review from the ` + - `maintainer's own account is present (${source}).\n` + + `✅ ${verdict.adrFiles.length} file(s) under ${ADR_PATH_PREFIX} and the PR's current review standing is ` + + `APPROVED${who ? ` (approved by ${who})` : ''} (${source}).\n` + verdict.adrFiles.map((f) => ` • ${f}`).join('\n'), ); return 0; } - const strangers = verdict.strangerApprovals ?? []; + const approvals = verdict.approvals ?? []; console.error( - `\n❌ This change touches ${ADR_PATH_PREFIX} and carries no APPROVED review from the maintainer's own account.\n\n` + + `\n❌ This change touches ${ADR_PATH_PREFIX} and the PR's current review standing is not APPROVED.\n\n` + verdict.adrFiles.map((f) => ` • ${f}`).join('\n') + '\n\n The ruling being enforced: ' + RULING + - '\n ADR merges are reserved to the maintainer in person. Drafting this PR was fine and stays fine --\n' + - ' only the MERGE is reserved.\n' + + '\n Drafting and pushing this PR was fine and stays fine -- only the MERGE is gated.\n' + (verdict.state - ? `\n The maintainer's current review standing on this PR is ${verdict.state}, not APPROVED.\n` + ? `\n The latest state-setting review on this PR is ${verdict.state}, not APPROVED.\n` + : '\n No state-setting review (APPROVED / CHANGES_REQUESTED / DISMISSED) has been submitted at all.\n') + + (approvals.length > 0 && verdict.state !== 'APPROVED' + ? `\n APPROVED review(s) from ${approvals.map((s) => `'${s}'`).join(', ')} exist but no longer stand:\n` + + ` a later ${verdict.state} superseded them. An approval is revocable by design -- see the header.\n` : '') + - (strangers.length > 0 - ? `\n APPROVED review(s) from ${strangers.map((s) => `'${s}'`).join(', ')} exist and deliberately do NOT\n` + - ' count: shared bot/agent identities merging ADRs is the exact failure this gate was built to stop\n' + - ' (#6785 -- two AI-seat merges within an hour of the ruling).\n' - : '') + - `\n Green path: the maintainer (${MAINTAINER_APPROVERS.map((a) => '@' + a.login).join(', ')}) reviews and\n` + - ' approves; the approval re-runs this check via the pull_request_review trigger, and it goes green\n' + - ' with no further action. See the header of scripts/check-adr-merge-approval.mjs.', + '\n Green path: anyone with review rights on this repo approves the PR; that approval re-runs this\n' + + ' check via the pull_request_review trigger and it goes green with no further action. This gate does\n' + + ' NOT check who approved: see the two-clause table in scripts/check-adr-merge-approval.mjs, which\n' + + " states what it guarantees and what it leaves to convention (#6741's 「维护者自己确认」/「人工合并」).", ); return 1; } @@ -549,7 +549,7 @@ if (invokedDirectly && !process.argv.includes('--self-test')) { // -- self-test ---------------------------------------------------------------- // -// Assertions over the REAL functions (`decide`, `latestMaintainerReviewState`, +// Assertions over the REAL functions (`decide`, `latestReviewState`, // `resolvePullNumber`, ...), never imitations. Every red-path fixture's // expected direction is stated in its comment BEFORE the assertion runs. @@ -582,8 +582,11 @@ async function selfTest() { if (!cond) failures.push(`${name}: ${detail}`); }; - // Fixture identities. The maintainer entry mirrors MAINTAINER_APPROVERS; the - // others are the real shared AI-seat accounts this gate must refuse. + // Fixture identities: the maintainer's real account and the real shared + // AI-seat accounts. Under the 2026-08-12 ruling the gate draws NO + // distinction between them -- several assertions below exist precisely to + // pin that, and they are the ones that inverted when the account filter was + // removed. const HOTLONG = { login: 'hotlong', id: 50353452 }; const OS_ZHUANG = { login: 'os-zhuang', id: 277994282 }; const OS_PM = { login: 'os-project-manager', id: 314343378 }; @@ -616,7 +619,8 @@ async function selfTest() { // ── ADR diff, no reviews at all → RED (predicted: RED) ────────────────── { const v = await decide({ changedPaths: ['docs/adr/0001-x.md'], getReviews: async () => [] }); - assert('adr-diff-without-reviews-is-red', !v.ok && v.kind === 'missing-maintainer-approval', JSON.stringify(v)); + assert('adr-diff-without-reviews-is-red', !v.ok && v.kind === 'missing-approval', JSON.stringify(v)); + assert('no-reviews-leaves-the-standing-null', v.state === null, `expected a null standing, got ${JSON.stringify(v.state)}`); } // ── ADR diff + maintainer APPROVED → GREEN (predicted: GREEN) ──────────── @@ -625,42 +629,61 @@ async function selfTest() { changedPaths: ['docs/adr/0001-x.md'], getReviews: async () => [review(HOTLONG, 'APPROVED', '2026-08-08T15:00:00Z')], }); - assert('maintainer-approval-is-green', v.ok && v.kind === 'maintainer-approved', JSON.stringify(v)); + assert('maintainer-approval-is-green', v.ok && v.kind === 'approved', JSON.stringify(v)); } - // ── bot/agent approvals must NOT satisfy the gate (predicted: RED) ─────── + // ── THE WIDENED RULE, pinned in the direction that used to be RED ─────── + // Before 2026-08-12 each of these was refused because the approver was not + // account 50353452. The ruling 「不要指定具体的人」 inverts them: + // predicted GREEN, one approving account at a time so a single fixture + // cannot pass on some other account's behalf. + { + for (const seat of [OS_ZHUANG, OS_PM, YINLIANGHUI, { login: 'claude[bot]', id: 242468646 }]) { + const v = await decide({ + changedPaths: ['docs/adr/0001-x.md'], + getReviews: async () => [review(seat, 'APPROVED', '2026-08-08T15:00:00Z')], + }); + assert(`approval-from-${seat.login}-is-green`, v.ok && v.kind === 'approved', JSON.stringify(v)); + } + // An account this repo has never seen, id and login alike: there is no + // list left to be on, so the id cannot matter (predicted: GREEN). + const v = await decide({ + changedPaths: ['docs/adr/0001-x.md'], + getReviews: async () => [review({ login: 'nobody-has-ever-heard-of-this-one', id: 1 }, 'APPROVED', '2026-08-08T15:00:00Z')], + }); + assert('approval-from-an-unknown-account-is-green', v.ok, JSON.stringify(v)); + assert('the-verdict-names-who-approved', (v.approvals ?? []).includes('nobody-has-ever-heard-of-this-one'), JSON.stringify(v.approvals)); + } + + // ── revocation survives the widening (predicted: RED) ──────────────────── + // The other half of the ruling: the pass condition is the CURRENT + // standing, not "an APPROVED review has ever existed". An approval + // followed by a CHANGES_REQUESTED must go back to red, and the verdict + // must still name the superseded approval so the red is explicable. { const v = await decide({ changedPaths: ['docs/adr/0001-x.md'], getReviews: async () => [ review(OS_ZHUANG, 'APPROVED', '2026-08-08T15:00:00Z'), - review(OS_PM, 'APPROVED', '2026-08-08T15:01:00Z'), - review(YINLIANGHUI, 'APPROVED', '2026-08-08T15:02:00Z'), - review({ login: 'claude[bot]', id: 242468646 }, 'APPROVED', '2026-08-08T15:03:00Z'), + review(OS_ZHUANG, 'CHANGES_REQUESTED', '2026-08-08T15:05:00Z'), ], }); - assert('bot-approvals-do-not-count', !v.ok, JSON.stringify(v)); - assert( - 'bot-approvals-are-named-in-the-verdict', - (v.strangerApprovals ?? []).includes('os-zhuang') && (v.strangerApprovals ?? []).includes('yinlianghui'), - `the verdict must name the approvals that deliberately do not count, got ${JSON.stringify(v.strangerApprovals)}`, - ); + assert('approval-then-changes-requested-is-red', !v.ok && v.state === 'CHANGES_REQUESTED', JSON.stringify(v)); + assert('a-superseded-approval-is-still-named', (v.approvals ?? []).includes('os-zhuang'), JSON.stringify(v.approvals)); } - // ── an account NAMED like the maintainer with a different id → RED ────── - // Login-squat protection: the id decides when present (predicted: RED). + // ── COMMENTED alone sets no standing (predicted: RED) ──────────────────── { const v = await decide({ changedPaths: ['docs/adr/0001-x.md'], - getReviews: async () => [review({ login: 'hotlong', id: 1 }, 'APPROVED', '2026-08-08T15:00:00Z')], + getReviews: async () => [review(HOTLONG, 'COMMENTED', '2026-08-08T15:00:00Z')], }); - assert('login-alone-with-wrong-id-does-not-count', !v.ok, JSON.stringify(v)); + assert('commented-alone-is-red', !v.ok && v.state === null, JSON.stringify(v)); } - // ── review-state sequencing over the maintainer's own reviews ──────────── + // ── review-state sequencing, one reviewer ──────────────────────────────── { - const seq = (states) => - latestMaintainerReviewState(states.map((s, i) => review(HOTLONG, s, `2026-08-08T15:0${i}:00Z`))); + const seq = (states) => latestReviewState(states.map((s, i) => review(HOTLONG, s, `2026-08-08T15:0${i}:00Z`))); // approval then CHANGES_REQUESTED → not approved (predicted: RED path) assert('later-changes-requested-revokes', seq(['APPROVED', 'CHANGES_REQUESTED']) === 'CHANGES_REQUESTED', seq(['APPROVED', 'CHANGES_REQUESTED'])); // CHANGES_REQUESTED then approval → approved (predicted: GREEN path) @@ -671,6 +694,26 @@ async function selfTest() { assert('comment-does-not-revoke', seq(['APPROVED', 'COMMENTED']) === 'APPROVED', seq(['APPROVED', 'COMMENTED'])); } + // ── review-state sequencing ACROSS reviewers ───────────────────────────── + // New surface: with the account filter gone the fold runs over everyone, + // so these two cases exist for the first time. Directions predicted from + // the header's "strict on both edges" rule, not from running it. + { + const across = (pairs) => latestReviewState(pairs.map(([u, s], i) => review(u, s, `2026-08-08T15:0${i}:00Z`))); + // one seat approves, a SECOND asks for changes → revoked (predicted: RED) + assert( + 'a-second-reviewers-changes-request-revokes', + across([[OS_ZHUANG, 'APPROVED'], [HOTLONG, 'CHANGES_REQUESTED']]) === 'CHANGES_REQUESTED', + across([[OS_ZHUANG, 'APPROVED'], [HOTLONG, 'CHANGES_REQUESTED']]), + ); + // changes requested, then a DIFFERENT account approves (predicted: GREEN) + assert( + 'a-second-reviewers-approval-clears-it', + across([[HOTLONG, 'CHANGES_REQUESTED'], [OS_PM, 'APPROVED']]) === 'APPROVED', + across([[HOTLONG, 'CHANGES_REQUESTED'], [OS_PM, 'APPROVED']]), + ); + } + // ── PR resolution ──────────────────────────────────────────────────────── { const cases = [ @@ -708,15 +751,17 @@ async function selfTest() { const v = await decide({ changedPaths: files, getReviews: async () => reviews }); assert(`historical-pr-${pr}-is-red-under-this-gate`, !v.ok, `PR #${pr} merged with no maintainer approval must replay RED, got ${JSON.stringify(v)}`); } - // The same two, had the maintainer approved → GREEN (predicted: GREEN): - // pins that the gate's red on the real history is ABOUT the missing - // approval, not about ADR diffs being unmergeable per se. + // The same two, had ANY account approved → GREEN (predicted: GREEN): pins + // that the gate's red on the real history is ABOUT the missing approval, + // not about ADR diffs being unmergeable per se. The approver here is the + // AI seat that merged #6671 — under the pre-2026-08-12 rule this pair was + // red, and it is the widened rule replayed against real captured payloads. for (const { pr, files } of HISTORICAL_VIOLATIONS) { const v = await decide({ changedPaths: files, - getReviews: async () => [review(HOTLONG, 'APPROVED', '2026-08-08T15:00:00Z')], + getReviews: async () => [review(OS_ZHUANG, 'APPROVED', '2026-08-08T15:00:00Z')], }); - assert(`historical-pr-${pr}-with-maintainer-approval-is-green`, v.ok, JSON.stringify(v)); + assert(`historical-pr-${pr}-with-any-approval-is-green`, v.ok, JSON.stringify(v)); } } catch (error) { failures.push(`unexpected error: ${error?.stack ?? error}`); diff --git a/scripts/check-required-contexts.mjs b/scripts/check-required-contexts.mjs index cf206bf258..98017b2ad8 100644 --- a/scripts/check-required-contexts.mjs +++ b/scripts/check-required-contexts.mjs @@ -193,7 +193,10 @@ export const REQUIRED_CONTEXTS = [ job: 'adr-merge-approval', context: 'ADR maintainer approval', authorized: '#7022 maintainer settings action, confirmed to the devx PM seat 2026-08-10 ~02:3xZ (screenshot of the `main` ruleset)', - carries: 'the #6741 ruling that only the maintainer own-account approval may land a docs/adr/** merge (#6942/#6962 landed unapproved while this context sat outside the required set)', + // The context STRING is load-bearing (it is what the ruleset requires) and + // must not change; the word "maintainer" in it is now historical — see + // #8161. What the check actually enforces is stated below. + carries: 'the rule that a docs/adr/** diff may not merge without an APPROVED review on the PR — any approver, per the maintainer ruling of 2026-08-12 (#8161), which superseded the #6741 own-account proxy (#6942/#6962 landed unapproved while this context sat outside the required set)', }, ];