From 87993dd3b90f7c311285251981312c86ce8eb8da Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 21 Sep 2026 07:13:32 +0000 Subject: [PATCH] fix(ci): reconcile production-environment-protection-guard to the ratified 2026-09-14 shape (BLO-34896) Board approval 60e271b7 (approved 2026-09-14T19:57:20Z) ruled that the current paperclip-production shape -- required_reviewers [kkroo] with prevent_self_review=false -- IS the intended shape, superseding b75f8156. The guard still asserted b75f8156's shape and had been failing 8/8 scheduled runs, minting a critical alert and an escalation chain roughly every 12h. Reconcile the guard, not the environment: - RATIFIED_REVIEWERS is now ['kkroo']. - prevent_self_review is reported under `observed` but no longer asserted. It was re-ratified as false, so asserting it made the guard permanently red about a deliberate board decision. - Removing that disjunct also fixes a live masking defect: because `||` short-circuits, prevent_self_review=false sent every run down the required_reviewers_rule branch and made the membership comparison in the `else` UNREACHABLE. The 2026-08-30 narrowing was therefore never actually reported as a membership change. Detection of the dangerous state is unchanged and mutation-tested: an absent rule or an empty reviewer list still yields required_reviewers_rule at severity=critical. The authoritative record is now traceable in-repo so a fourth agent run does not re-derive which approval supersedes which. --- ...heck-production-environment-protection.mjs | 61 +++++++++++-- .../post-environment-protection-alert.mjs | 2 +- ...production-environment-protection.test.mjs | 91 ++++++++++++++++--- ...roduction-environment-protection-guard.yml | 11 ++- 4 files changed, 135 insertions(+), 30 deletions(-) diff --git a/.github/scripts/check-production-environment-protection.mjs b/.github/scripts/check-production-environment-protection.mjs index 626bc3243a60..7adc35820a5a 100644 --- a/.github/scripts/check-production-environment-protection.mjs +++ b/.github/scripts/check-production-environment-protection.mjs @@ -4,12 +4,31 @@ * * Reads the live GitHub environment that gates `helm upgrade` (default: * paperclip-production) and asserts the controls the board ratified on - * approval b75f8156: - * 1. a `required_reviewers` protection rule with prevent_self_review === true - * whose reviewer set is exactly the ratified set + * approval 60e271b7 (2026-09-14), which SUPERSEDES approval b75f8156 + * (2026-08-03) as the authoritative record of the intended shape: + * 1. a `required_reviewers` protection rule whose reviewer list is non-empty + * and whose membership is exactly the ratified set * 2. can_admins_bypass === false * 3. deployment_branch_policy.protected_branches === true * + * WHICH RECORD IS AUTHORITATIVE (BLO-34896 / BLO-34527) — do not re-derive this. + * b75f8156 ratified two reviewers plus prevent_self_review. On 2026-08-30 the + * environment was narrowed to `[kkroo]` with prevent_self_review=false. Card + * 60e271b7 asked the board which of the two shapes was intended and was + * APPROVED on 2026-09-14T19:57:20Z, ruling (A): the narrowed shape IS the + * intended shape, reconcile the guard and not the environment. The ruling is + * recorded on BLO-34527. Three separate agent runs have now re-litigated this + * question; the answer lives here so a fourth does not have to. + * + * WHY prevent_self_review IS NO LONGER ASSERTED. It was re-ratified as false by + * 60e271b7, so asserting it would make the guard permanently red about a + * deliberate board decision — which erodes the same slack-relay channel this + * check depends on. It is still reported under `observed` so the single-approver + * posture stays visible in every alert and run log. The residual risk (one + * person can both dispatch and approve a production deploy, and their + * unavailability is a total deploy outage) is recorded on BLO-22329, not here: + * a detector should assert the ratified shape, not re-argue it. + * * Why the reviewer set is compared by membership and not merely for * non-emptiness (BLO-22329): the 2026-08-08 drift *added* `kkroo` — a repo * admin — as a third reviewer and flipped `can_admins_bypass` to true, which @@ -28,10 +47,12 @@ import { writeFileSync } from 'node:fs'; import { ghFetch } from './get-bot-token.mjs'; /** - * The reviewer set ratified on approval b75f8156. Changing it is deliberately a - * code change: the PR is the audit trail that the two silent edits lacked. + * The reviewer set ratified on approval 60e271b7 (2026-09-14), superseding the + * ['eyad-hussein', 'MohamedElmdary'] set of b75f8156. Changing it is + * deliberately a code change: the PR is the audit trail that the two silent + * edits lacked. */ -export const RATIFIED_REVIEWERS = ['eyad-hussein', 'MohamedElmdary']; +export const RATIFIED_REVIEWERS = ['kkroo']; /** A reviewer entry is either a User (login) or a Team (slug). */ function reviewerName(entry) { @@ -71,10 +92,28 @@ export function evaluateEnvironmentProtection(env, options = {}) { ? rule.reviewers.map(reviewerName).filter(Boolean) : []; - if (rule == null || reviewers.length === 0 || rule.prevent_self_review !== true) { + // THE DANGEROUS STATE: no rule at all, or a rule with nobody on it. Either + // way there is no effective gate on a production deploy, which is the one + // thing this check exists to shout about. Never weaken this clause to make a + // run go green (BLO-34896 AC2). + // + // `prevent_self_review` is deliberately NOT a disjunct here. It used to be, + // and because `||` short-circuits, the live prevent_self_review=false state + // sent every run down this branch and the membership comparison in the `else` + // below became UNREACHABLE — so the 2026-08-30 narrowing to [kkroo] was never + // actually reported as a membership change, only as a self-review complaint. + // A compound clause that skips a sibling check is how a tolerated drift masks + // an untolerated one; keep these conditions about "is there a gate at all". + // + // `rule == null` is SUBSUMED by `reviewers.length === 0` (an absent rule makes + // `reviewers` derive to []), so it survives mutation testing — it is kept for + // legibility, not coverage. Do not read the absent-rule test below as a guard + // on this term specifically. + if (rule == null || reviewers.length === 0) { violation( VIOLATION_KINDS.REQUIRED_REVIEWERS_RULE, - 'required_reviewers: missing, or reviewers is empty, or prevent_self_review is not true', + 'required_reviewers: rule is missing, or its reviewer list is empty — ' + + 'there is no effective approval gate on production deploys', ); } else { // Compare membership case-insensitively; GitHub logins are case-preserving @@ -192,8 +231,10 @@ async function main() { if (compliant) { console.log( `PASS: ${repo} environment '${environmentName}' matches the ratified protection shape ` + - `(required_reviewers ${JSON.stringify(observed.reviewers)} + prevent_self_review, ` + - 'can_admins_bypass=false, deployment_branch_policy.protected_branches=true).', + `(required_reviewers ${JSON.stringify(observed.reviewers)}, ` + + `can_admins_bypass=false, deployment_branch_policy.protected_branches=true). ` + + `Observed prevent_self_review=${JSON.stringify(observed.prevent_self_review)} ` + + '(re-ratified as permitted by approval 60e271b7; reported, not asserted).', ); writeSummary({ status: 'compliant', repo, environment: environmentName, observed }); process.exitCode = 0; diff --git a/.github/scripts/post-environment-protection-alert.mjs b/.github/scripts/post-environment-protection-alert.mjs index b83761707dee..9ef94080d2b0 100644 --- a/.github/scripts/post-environment-protection-alert.mjs +++ b/.github/scripts/post-environment-protection-alert.mjs @@ -52,7 +52,7 @@ export function buildAlert({ exitCode, summary, runUrl, repo, environment, now } ? `The ${environment} environment could NOT be read, so its protection state is unknown. ` + `This is never a pass. Reason: ${summary?.reason ?? 'unknown'}` : `The ${environment} environment no longer matches the board-ratified protection shape ` + - `(approval b75f8156). Violations:\n` + + `(approval 60e271b7, which supersedes b75f8156). Violations:\n` + (summary?.violations ?? ['(summary unavailable — see run log)']) .map((v) => ` - ${v}`) .join('\n'); diff --git a/.github/scripts/tests/check-production-environment-protection.test.mjs b/.github/scripts/tests/check-production-environment-protection.test.mjs index 69c1fbe9f8bd..adbcbc231c07 100644 --- a/.github/scripts/tests/check-production-environment-protection.test.mjs +++ b/.github/scripts/tests/check-production-environment-protection.test.mjs @@ -10,15 +10,24 @@ const user = (login) => ({ type: 'User', reviewer: { login } }); // ── evaluateEnvironmentProtection ──────────────────────────────────────────── +// The EXACT live shape of paperclip-production, re-read 2026-09-21T07:0xZ: +// {"can_admins_bypass":false,"updated_at":"2026-08-30T07:13:06Z", +// "rules":[{"type":"branch_policy"}, +// {"type":"required_reviewers","prevent_self_review":false, +// "reviewers":["kkroo"]}]} +// Ratified as intended by board approval 60e271b7 (2026-09-14), superseding +// b75f8156. This fixture IS the acceptance criterion for BLO-34896: the guard +// must be green on it *without* the environment moving. Note prevent_self_review +// is false here on purpose — see the "reported, not asserted" test below. const COMPLIANT_ENV = { can_admins_bypass: false, - updated_at: '2026-08-06T05:55:07Z', + updated_at: '2026-08-30T07:13:06Z', protection_rules: [ { id: 61677470, type: 'branch_policy' }, { id: 61904232, type: 'required_reviewers', - prevent_self_review: true, + prevent_self_review: false, reviewers: RATIFIED_REVIEWERS.map(user), }, ], @@ -32,6 +41,13 @@ test('evaluateEnvironmentProtection: passes the board-ratified shape', () => { assert.deepEqual(result.observed.reviewers, RATIFIED_REVIEWERS); }); +test('evaluateEnvironmentProtection: the ratified reviewer set is the 60e271b7 set, not the superseded b75f8156 one', () => { + // Pins the record that BLO-34896 exists to stop re-deriving. If someone + // restores the two-reviewer set without a new board ruling, this fails and + // the PR diff is the place that conversation happens. + assert.deepEqual(RATIFIED_REVIEWERS, ['kkroo']); +}); + test('evaluateEnvironmentProtection: flags the 2026-08-04 lapse shape (required_reviewers gone, admin bypass true)', () => { // Exact shape from GET /repos/Blockcast/paperclip/environments/paperclip-production // as recorded on board approval 06ff894e (updated_at 2026-08-04T09:21:50Z). @@ -48,7 +64,7 @@ test('evaluateEnvironmentProtection: flags the 2026-08-04 lapse shape (required_ assert.match(result.violations[1], /can_admins_bypass/); }); -test('evaluateEnvironmentProtection: flags the 2026-08-08 WIDENING shape (extra admin reviewer + admin bypass)', () => { +test('evaluateEnvironmentProtection: flags the 2026-08-08 WIDENING shape (extra admin reviewers + admin bypass)', () => { // Exact live shape re-probed 2026-08-14T08:46Z: the 08-08 "temporary" override // that was never restored. A non-emptiness check passes this; membership // comparison is what catches it. Regression guard for BLO-22329. @@ -70,7 +86,7 @@ test('evaluateEnvironmentProtection: flags the 2026-08-08 WIDENING shape (extra const result = evaluateEnvironmentProtection(widenedEnv); assert.equal(result.compliant, false); assert.equal(result.violations.length, 2); - assert.match(result.violations[0], /required_reviewers membership.*kkroo/); + assert.match(result.violations[0], /required_reviewers membership.*eyad-hussein.*MohamedElmdary/); assert.match(result.violations[1], /can_admins_bypass/); }); @@ -89,7 +105,7 @@ test('evaluateEnvironmentProtection: flags a removed ratified reviewer', () => { }; const result = evaluateEnvironmentProtection(env); assert.equal(result.compliant, false); - assert.match(result.violations[0], /required_reviewers membership.*missing.*MohamedElmdary/); + assert.match(result.violations[0], /required_reviewers membership.*missing.*kkroo/); }); test('evaluateEnvironmentProtection: reviewer membership is case-insensitive', () => { @@ -101,7 +117,7 @@ test('evaluateEnvironmentProtection: reviewer membership is case-insensitive', ( id: 2, type: 'required_reviewers', prevent_self_review: true, - reviewers: [user('Eyad-Hussein'), user('mohamedelmdary')], + reviewers: [user('KKroo')], }, ], }; @@ -134,6 +150,11 @@ test('evaluateEnvironmentProtection: resolves Team reviewers by slug', () => { assert.deepEqual(result.observed.reviewers, ['release-approvers']); }); +// ── the dangerous state: no effective gate (BLO-34896 AC2) ─────────────────── +// These two are the negative control for the reconciliation. The guard was made +// green against the live prevent_self_review=false shape; it must NOT have gone +// green by weakening its detection of "there is no approval gate at all". + test('evaluateEnvironmentProtection: flags empty reviewers as non-compliant even if the rule exists', () => { const env = { ...COMPLIANT_ENV, @@ -145,24 +166,65 @@ test('evaluateEnvironmentProtection: flags empty reviewers as non-compliant even const result = evaluateEnvironmentProtection(env); assert.equal(result.compliant, false); assert.match(result.violations[0], /required_reviewers/); + assert.deepEqual(result.violationKinds, ['required_reviewers_rule']); }); -test('evaluateEnvironmentProtection: flags prevent_self_review !== true even with reviewers present', () => { +test('evaluateEnvironmentProtection: flags an absent required_reviewers rule', () => { const env = { + ...COMPLIANT_ENV, + protection_rules: [{ id: 1, type: 'branch_policy' }], + }; + const result = evaluateEnvironmentProtection(env); + assert.equal(result.compliant, false); + assert.deepEqual(result.violationKinds, ['required_reviewers_rule']); +}); + +test('evaluateEnvironmentProtection: prevent_self_review is REPORTED but not asserted', () => { + // Re-ratified as permitted-false by approval 60e271b7 (2026-09-14), so it must + // not fail the run — that is the whole point of BLO-34896. It must still show + // up in `observed`, which is what carries the single-approver posture into + // every alert and run log. + const result = evaluateEnvironmentProtection(COMPLIANT_ENV); + assert.equal(result.compliant, true); + assert.equal(result.observed.prevent_self_review, false); + + // ...and flipping it the other way is a strengthening, not a violation. + const stricter = evaluateEnvironmentProtection({ ...COMPLIANT_ENV, protection_rules: [ { id: 1, type: 'branch_policy' }, { id: 2, type: 'required_reviewers', - prevent_self_review: false, + prevent_self_review: true, reviewers: RATIFIED_REVIEWERS.map(user), }, ], + }); + assert.equal(stricter.compliant, true); +}); + +test('evaluateEnvironmentProtection: prevent_self_review=false does NOT mask the membership check', () => { + // The defect this reconciliation fixed. `prevent_self_review !== true` used to + // be a disjunct of the required_reviewers_rule clause, and because `||` + // short-circuits, the live false value sent every run down that branch and the + // membership comparison in the `else` was unreachable. A tolerated drift was + // hiding an untolerated one. Mutation guard: re-add that disjunct and this + // fails, because the result collapses to required_reviewers_rule. + const env = { + ...COMPLIANT_ENV, + protection_rules: [ + { id: 1, type: 'branch_policy' }, + { + id: 2, + type: 'required_reviewers', + prevent_self_review: false, + reviewers: [user('somebody-unratified')], + }, + ], }; const result = evaluateEnvironmentProtection(env); - assert.equal(result.compliant, false); - assert.match(result.violations[0], /required_reviewers/); + assert.deepEqual(result.violationKinds, ['required_reviewers_membership']); }); test('evaluateEnvironmentProtection: flags missing deployment_branch_policy.protected_branches', () => { @@ -199,7 +261,8 @@ test('evaluateEnvironmentProtection: violationKinds stays in lockstep with viola }); test('evaluateEnvironmentProtection: a membership widening is kind-tagged distinctly from a bypass flip', () => { - // The live 2026-09-01 shape: kkroo added, but can_admins_bypass still false. + // A membership widening with can_admins_bypass still false: an extra reviewer + // beyond the ratified set, which is exactly the 2026-08-08 incident shape. const widened = evaluateEnvironmentProtection({ ...COMPLIANT_ENV, protection_rules: [ @@ -208,7 +271,7 @@ test('evaluateEnvironmentProtection: a membership widening is kind-tagged distin id: 61904232, type: 'required_reviewers', prevent_self_review: true, - reviewers: [...RATIFIED_REVIEWERS, 'kkroo'].map(user), + reviewers: [...RATIFIED_REVIEWERS, 'eyad-hussein'].map(user), }, ], }); @@ -224,7 +287,7 @@ test('evaluateEnvironmentProtection: a membership widening is kind-tagged distin id: 61904232, type: 'required_reviewers', prevent_self_review: true, - reviewers: [...RATIFIED_REVIEWERS, 'kkroo'].map(user), + reviewers: [...RATIFIED_REVIEWERS, 'eyad-hussein'].map(user), }, ], }); @@ -245,7 +308,7 @@ test('evaluateEnvironmentProtection: violation kinds carry no observed values', id: 61904232, type: 'required_reviewers', prevent_self_review: true, - reviewers: [...RATIFIED_REVIEWERS, 'kkroo'].map(user), + reviewers: [...RATIFIED_REVIEWERS, 'eyad-hussein'].map(user), }, ], }); diff --git a/.github/workflows/production-environment-protection-guard.yml b/.github/workflows/production-environment-protection-guard.yml index fbd49aea64bf..b23c289c0c3e 100644 --- a/.github/workflows/production-environment-protection-guard.yml +++ b/.github/workflows/production-environment-protection-guard.yml @@ -1,11 +1,12 @@ name: Production Environment Protection Guard # Re-reads the live `paperclip-production` GitHub environment and asserts the -# board-ratified protection shape (approval b75f8156). Exists because a closed -# approval card was being treated as evidence the control existed: the -# protection has now silently drifted three times (2026-07-31 never applied, -# 2026-08-04 lapsed, 2026-08-08 widened under an incident override that was -# never restored) and each time nobody noticed for 2-6 days. See BLO-22329. +# board-ratified protection shape (approval 60e271b7, 2026-09-14, which +# SUPERSEDES b75f8156). Exists because a closed approval card was being treated +# as evidence the control existed: the protection has now silently drifted three +# times (2026-07-31 never applied, 2026-08-04 lapsed, 2026-08-08 widened under +# an incident override that was never restored) and each time nobody noticed for +# 2-6 days. See BLO-22329, and BLO-34896 for which record is authoritative. # # A red workflow run is deliberately NOT the alerting path — it only emails the # last committer, which is the exact failure mode being fixed. Drift is pushed