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}`)