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)', }, ];