From 7a6bc14470dd452f24e91d0e38bd5b4695f0e145 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:02:07 +1000 Subject: [PATCH] Restore branch protection and assert it on every PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both branch rulesets were `enforcement: disabled`. `main` had no required status checks at all — PR #190 (212 files, +60766/-37428) merged green 29 minutes after its own CI failures were filed as issues #202, #203 and #204. Two structural traps kept the gates off, and both are now checked: Phantom context. Ruleset 7726557 required a check named "CI". No job reports that name, so enabling it would hang every PR pending forever. The reachable fix under pressure is to switch the ruleset off, which is what happened. Its required_status_checks rule is dropped; it now carries the PR requirement (squash-only, thread resolution) and ruleset 6154907 owns the checks. Path-filtered required job. ci-windows.yml skipped via `on: paths-ignore:`, which cannot be a required check: a filtered-out run never reports, and a required check that never reports blocks the merge forever. Converted to job-level `if:` skipping, which reports "skipped" and counts as passing, so windows-core can now be required. Both rulesets are active, with no bypass actors, and six required checks: Detect changed areas, Test/Format/Build & Validate, Rust Compiler, WebAssembly target, Website E2E, Windows Core. The wasm corpus differential was advisory by design — that is how a wasm-only miscompile (#202) could land unopposed. scripts/verify-branch-protection.mjs pins that list and runs in the `changes` job, the one required job that never skips. It fails on a disabled ruleset, a bypass actor, drift in either direction between the list and the ruleset, a context matching no job name, and a required job its workflow path-filters. Verified against the real broken state: it reports every one. Stale comments claiming these jobs are unrequired are corrected, and CLAUDE.md gains the rule this violated — a gate you can turn off is not a gate. --- .github/workflows/ci-windows.yml | 36 ++++-- .github/workflows/ci.yml | 41 +++--- CLAUDE.md | 12 ++ scripts/verify-branch-protection.mjs | 181 +++++++++++++++++++++++++++ 4 files changed, 246 insertions(+), 24 deletions(-) create mode 100644 scripts/verify-branch-protection.mjs diff --git a/.github/workflows/ci-windows.yml b/.github/workflows/ci-windows.yml index 98be82ac..bc54cddd 100644 --- a/.github/workflows/ci-windows.yml +++ b/.github/workflows/ci-windows.yml @@ -5,12 +5,6 @@ name: CI (Windows) on: pull_request: branches: [main] - # Website-only PRs don't touch the compiler — skip the Windows build entirely. - # windows-core is NOT a required status check, so skipping the whole workflow - # via path filtering here is safe and won't leave a pending check blocking the - # merge. (ci.yml's required `ci` job uses job-level `if` skipping instead.) - paths-ignore: - - 'website/**' workflow_dispatch: concurrency: @@ -18,17 +12,41 @@ concurrency: cancel-in-progress: true jobs: + # Website-only PRs don't touch the compiler, so `windows-core` skips — but it + # must skip at the JOB level, never via an `on: paths-ignore:` filter. A + # required check belonging to a workflow that path-filtering skipped stays + # pending forever and blocks the merge; a job skipped by `if:` reports as + # PASSING. `windows-core` IS a required status check on `main` (ruleset + # 6154907), which is why the filter that used to live here is now this job. + changes: + name: Detect changed areas (Windows) + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + code: ${{ steps.filter.outputs.code }} + steps: + - uses: actions/checkout@v4 + - uses: dorny/paths-filter@v3 + id: filter + with: + filters: | + code: + - '!website/**' + windows-core: + # IMPORTANT: this name is a required status check on `main` (ruleset 6154907). + # Renaming it will break branch protection — coordinate with the ruleset update. name: Windows Core Build & Smoke Test + needs: changes + if: ${{ needs.changes.outputs.code == 'true' }} runs-on: windows-latest timeout-minutes: 25 steps: - uses: actions/checkout@v4 # No duplication gate here. Deslop measures source text, so its verdict is - # platform-independent — and this `windows-core` job is NOT a required - # status check, so gating from here could never block a merge. The gate - # lives in ci.yml's required `ci` job instead. + # platform-independent, so running it on a second OS would only duplicate + # ci.yml's required `ci` job. The gate lives there. - name: Install Rust (stable) uses: dtolnay/rust-toolchain@stable diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8561df6f..2e32f6be 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,8 +17,9 @@ jobs: # runtime, VS Code extension or web-compiler toolchain. Detect whether anything # OUTSIDE website/ changed so the heavy jobs below can skip. # - # Required-check note: the `ci` job ("Test, Format, Build & Validate") is the - # required status check on `main` (ruleset 6154907). Skipping a job via an `if:` + # Required-check note: ruleset 6154907 requires FIVE checks on `main` — this + # `changes` job, `ci`, `rust`, `wasm` and `website` — plus ci-windows.yml's + # `windows-core`. Skipping a job via an `if:` # condition reports its check as "skipped", which branch protection treats as # PASSING — so a website-only PR still merges. Do NOT convert this to an # `on: paths:` filter: a workflow skipped by path filtering leaves the required @@ -41,8 +42,18 @@ jobs: code: - '!website/**' + # Branch protection lives in GitHub's settings, not in this tree, so it + # drifts silently — both rulesets were once found `enforcement: disabled` + # with a 212-file PR already merged past them, and nothing in the repo + # could have noticed. This step is the assertion that would have. It runs + # HERE because `changes` is the one required job that never skips. + - name: Branch protection intact + env: + GH_TOKEN: ${{ github.token }} + run: node scripts/verify-branch-protection.mjs + ci: - # IMPORTANT: this name is the required status check on `main` (ruleset 6154907). + # IMPORTANT: this name is a required status check on `main` (ruleset 6154907). # Renaming it will break branch protection — coordinate with the ruleset update. name: Test, Format, Build & Validate needs: changes @@ -137,11 +148,13 @@ jobs: run: npm run test:shipwright working-directory: ./vscode-extension - # Official duplication gate. This MUST live in the `ci` job: `ci` is the - # required status check on `main` (ruleset 6154907), so a gate anywhere - # else cannot block a merge. It previously sat in ci-windows.yml, whose - # `windows-core` job is explicitly NOT required — which silently turned - # the duplication ceiling into a suggestion. The committed .deslop.toml is + # Official duplication gate. This lives in the `ci` job because deslop + # measures source text — one platform's verdict is every platform's, so a + # second copy in ci-windows.yml would only duplicate this one. (It did sit + # there once, back when `windows-core` was not a required check, which + # silently turned the duplication ceiling into a suggestion. Every job + # named in ruleset 6154907 can block a merge now; this one is `ci`'s.) + # The committed .deslop.toml is # the threshold source (deslop exits 3 when over); reports stay ephemeral # rather than consuming Actions artifact storage. `version` is explicit # because the action is pinned to the immutable commit for v0.27.0 rather @@ -411,13 +424,11 @@ jobs: # A backend that miscompiles arithmetic, strings, maps or pattern # matching outside hello/studio has nowhere else to be caught. # - # CAVEAT: this `wasm` job is NOT a required status check (only `ci` is, - # per ruleset 6154907), so a red result here does not block a merge — - # the same arrangement the deleted harness had. The sysroot download and - # wasm runtime build this would need in `ci` are now there anyway (the - # coverage gate depends on them), so moving this corpus differential - # into `ci` costs only its runtime — still a deliberate call, not a - # silent one. + # This `wasm` job IS a required status check on `main` (ruleset + # 6154907), so a red result here blocks the merge. It was advisory once, + # which meant a wasm-only miscompile could land unopposed; issue #202 + # was found that way. Do not demote it back without demoting the ruleset + # entry with it — an advisory gate reads as a gate and is not one. run: OSPREY_TARGET=wasm32 zsh crates/run_test_corpus.sh # ─── Website end-to-end (Playwright) ──────────────────────────────────────── diff --git a/CLAUDE.md b/CLAUDE.md index 7febe196..f62b02f0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,6 +9,8 @@ Guidance for agents working in this repository. Nearly-complete features get finished. Completely broken features get removed and reported, not papered over. Before adding anything new, ask whether the effort would better harden something that exists. +That doesn't mean taking shortcuts to finish features. That means doing the HARD WORK to COMPLETE FEATURES IN THE HIGHEST POSSIBLE quality level + ## Tests outrank code **A failing test that pins a compiler bug is worth more than a speculative fix.** A red test survives refactors and turns a suspicion into an enforceable contract. Code you *believe* is correct is a liability until an assertion proves it. @@ -20,6 +22,16 @@ Nearly-complete features get finished. Completely broken features get removed an - Coverage thresholds live in `coverage-thresholds.json` and only go up. - Expand existing examples/tests instead of adding files. Keep examples concise, mixing many language constructs per file. +## 🚨 The pipeline is not negotiable + +**A gate you can turn off is not a gate.** On 2026-08-12 both branch rulesets were found `enforcement: disabled` — a 212-file PR merged past them 29 minutes after its own CI failures were filed as issues #202/#203/#204. Nothing in the tree could have noticed, because branch protection lives in GitHub's settings. + +- **Never disable, weaken, bypass or narrow a CI gate to get a merge.** Not the rulesets, not a required-check list, not a job's `if:`, not a test's timeout. If a check is red, the code is wrong — fix the code. +- **"Advisory" is deleted.** Marking a job "not a required status check" removes it. If it is worth running it is worth blocking on; if it is not worth blocking on, delete the job and say so. +- **Never merge with a known failure.** An open issue describing a red check on the branch is a blocker, not a footnote. File-and-merge is the exact failure this section exists to prevent. +- The required checks are pinned in [`scripts/verify-branch-protection.mjs`](scripts/verify-branch-protection.mjs), asserted by the `changes` job on every PR. Changing the gate means changing that list and the ruleset together — the check fails until they agree. +- Adding a required check? It must skip via a **job-level `if:`**, never `on: paths:`/`paths-ignore:`. A path-filtered job never reports, and a required check that never reports blocks every merge forever. + ## 🚨 Broken Code Process Upon encountering code that fails silently: diff --git a/scripts/verify-branch-protection.mjs b/scripts/verify-branch-protection.mjs new file mode 100644 index 00000000..a997f729 --- /dev/null +++ b/scripts/verify-branch-protection.mjs @@ -0,0 +1,181 @@ +#!/usr/bin/env node +// Asserts that `main` is actually gated — that the branch rulesets are ENABLED, +// that they require the checks listed here, and that each required check is one +// a workflow can really report. +// +// This exists because both rulesets were found `enforcement: "disabled"` with a +// 212-file PR already merged past them. Nothing in the repo could have noticed: +// branch protection lives in GitHub's settings, not in the tree, so it drifts +// silently and by definition no test covers it. This is that test. +// +// Two traps are checked structurally, because both are why the gates were off: +// +// Phantom context — ruleset 7726557 required a check named "CI". No job +// reports that name (jobs report their `name:`, and the workflow's own name +// is not a check run), so every PR would sit pending forever. A required +// check that can never report is indistinguishable from a broken repo, and +// the fix reached for under pressure is to switch the ruleset off. +// +// Path-filtered required job — a job skipped by a workflow-level `on: paths:` +// / `paths-ignore:` filter never reports at all, and a required check that +// never reports blocks the merge forever. A job skipped by a job-level `if:` +// reports "skipped", which counts as PASSING. Required jobs must therefore +// skip via `if:`, never via path filtering. +// +// Run: node scripts/verify-branch-protection.mjs [--repo owner/name] + +import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' + +const DEFAULT_REPO = 'Nimblesite/osprey' +const WORKFLOW_DIR = '.github/workflows' + +// The gate list. Adding a required check means adding it here AND to the +// ruleset; this script fails until the two agree, in either direction. +const EXPECTED_CONTEXTS = [ + 'Detect changed areas', + 'Test, Format, Build & Validate', + 'Rust Compiler (fmt, clippy, test, corpus)', + 'WebAssembly target (wasm32-wasip1)', + 'Website E2E (Playwright)', + 'Windows Core Build & Smoke Test', +] + +const argRepo = (argv) => { + const i = argv.indexOf('--repo') + return i >= 0 && argv[i + 1] ? argv[i + 1] : DEFAULT_REPO +} + +const api = async (path, token) => { + const headers = { accept: 'application/vnd.github+json', 'user-agent': 'osprey-ci' } + if (token) headers.authorization = `Bearer ${token}` + const res = await fetch(`https://api.github.com${path}`, { headers }) + if (!res.ok) throw new Error(`GET ${path} -> ${res.status} ${res.statusText}`) + return res.json() +} + +// --- workflow scanning (no YAML dependency; structure-specific by design) ---- + +const indentOf = (line) => line.length - line.trimStart().length + +// Every `name:` at job level (4 spaces) inside the `jobs:` block, paired with +// whether that job carries a job-level `if:`. Those names ARE the check-run +// contexts GitHub reports. +const scanJobs = (text) => { + const lines = text.split('\n') + const start = lines.findIndex((l) => /^jobs:\s*$/.test(l)) + if (start < 0) return [] + const jobs = [] + let current = null + for (const line of lines.slice(start + 1)) { + if (line.trim() === '' || line.trimStart().startsWith('#')) continue + if (indentOf(line) === 0) break + if (indentOf(line) === 2 && /^\s{2}[\w-]+:\s*$/.test(line)) { + current = { key: line.trim().replace(':', ''), name: null, hasIf: false } + jobs.push(current) + continue + } + if (!current || indentOf(line) !== 4) continue + const name = line.match(/^\s{4}name:\s*(.+?)\s*$/) + if (name) current.name = name[1].replace(/^['"]|['"]$/g, '') + if (/^\s{4}if:/.test(line)) current.hasIf = true + } + return jobs +} + +// A `paths:` / `paths-ignore:` key inside the `on:` block — the filter that +// leaves a required check pending forever. +const hasPathFilter = (text) => { + const lines = text.split('\n') + const start = lines.findIndex((l) => /^on:\s*$/.test(l)) + if (start < 0) return false + for (const line of lines.slice(start + 1)) { + if (indentOf(line) === 0 && line.trim() !== '') break + if (/^\s+paths(-ignore)?:/.test(line)) return true + } + return false +} + +const readWorkflows = () => + readdirSync(WORKFLOW_DIR) + .filter((f) => f.endsWith('.yml') || f.endsWith('.yaml')) + .map((f) => { + const text = readFileSync(join(WORKFLOW_DIR, f), 'utf8') + return { file: f, jobs: scanJobs(text), pathFiltered: hasPathFilter(text) } + }) + +// --- assertions ------------------------------------------------------------- + +const checkRulesets = (rulesets, detail) => { + const failures = [] + const onDefault = rulesets.filter((r) => + (detail[r.id]?.conditions?.ref_name?.include ?? []).includes('~DEFAULT_BRANCH'), + ) + if (onDefault.length === 0) failures.push('no ruleset targets the default branch — `main` is ungated') + + for (const r of onDefault) { + const full = detail[r.id] + if (full.enforcement !== 'active') { + failures.push(`ruleset ${r.id} "${r.name}" is enforcement="${full.enforcement}" — it gates nothing`) + } + if ((full.bypass_actors ?? []).length > 0) { + const who = full.bypass_actors.map((a) => a.actor_type).join(', ') + failures.push(`ruleset ${r.id} "${r.name}" has bypass actors (${who}) — the gate is optional for them`) + } + } + return { failures, onDefault } +} + +const requiredContexts = (onDefault, detail) => + onDefault.flatMap((r) => + (detail[r.id].rules ?? []) + .filter((rule) => rule.type === 'required_status_checks') + .flatMap((rule) => rule.parameters.required_status_checks.map((c) => c.context)), + ) + +const checkContexts = (actual, workflows) => { + const failures = [] + const missing = EXPECTED_CONTEXTS.filter((c) => !actual.includes(c)) + const extra = actual.filter((c) => !EXPECTED_CONTEXTS.includes(c)) + if (missing.length) failures.push(`ruleset does not require: ${missing.join(', ')}`) + if (extra.length) failures.push(`ruleset requires checks absent from EXPECTED_CONTEXTS: ${extra.join(', ')}`) + + const allJobs = workflows.flatMap((w) => w.jobs.map((j) => ({ ...j, file: w.file, pathFiltered: w.pathFiltered }))) + for (const context of actual) { + const job = allJobs.find((j) => j.name === context) + if (!job) { + failures.push(`required check "${context}" matches no job name — it can never report, so every PR hangs pending`) + continue + } + if (job.pathFiltered) { + failures.push( + `required check "${context}" lives in ${job.file}, which path-filters at the \`on:\` level — ` + + 'a filtered-out run never reports and blocks the merge forever; skip with a job-level `if:` instead', + ) + } + } + return failures +} + +// --- main ------------------------------------------------------------------- + +const repo = argRepo(process.argv) +const token = process.env.GH_TOKEN || process.env.GITHUB_TOKEN +const summary = await api(`/repos/${repo}/rulesets`, token) +const detail = Object.fromEntries( + await Promise.all(summary.map(async (r) => [r.id, await api(`/repos/${repo}/rulesets/${r.id}`, token)])), +) + +const { failures: rulesetFailures, onDefault } = checkRulesets(summary, detail) +const contexts = requiredContexts(onDefault, detail) +const failures = [...rulesetFailures, ...checkContexts(contexts, readWorkflows())] + +if (failures.length > 0) { + console.error(`Branch protection on ${repo} is not intact:\n`) + for (const f of failures) console.error(` ✗ ${f}`) + console.error('\nFix the ruleset at https://github.com/' + repo + '/settings/rules — not this script.') + process.exit(1) +} + +console.log(`Branch protection intact on ${repo}: ${onDefault.length} active ruleset(s), ${contexts.length} required checks.`) +for (const c of contexts) console.log(` ✓ ${c}`)