diff --git a/.config/lefthook.yml b/.config/lefthook.yml index cc4a4fd..9423a72 100644 --- a/.config/lefthook.yml +++ b/.config/lefthook.yml @@ -22,6 +22,12 @@ pre-commit: glob: '.config/**' run: task check:config + # Both halves of the review gate, plus the workflows whose job names its + # required checks name -- renaming a job is the quiet way to break it. + - name: rulesets + glob: '.github/{rulesets/**,CODEOWNERS,workflows/*.yml}' + run: task check:rulesets + - name: markdown glob: '**/*.md' run: task check:md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b37fe72..8a50b39 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,24 +1,50 @@ -# See https://docs.github.com/en/repositories/managing-your-repositories-settings-and-features/customizing-your-repository/about-code-owners +# CODEOWNERS — the ownership half of the selective code-owner review gate. # -# Pattern matching is gitignore-style and the last matching entry wins. The -# `*` catch-all covers everything; the entries below restate ownership for the -# areas where a change alters a public contract, so the intent survives a -# future split of ownership. +# Docs: https://docs.github.com/en/repositories/managing-your-repositories-settings-and-features/customizing-your-repository/about-code-owners +# Why it is selective: docs/adr/0015-selective-code-owner-review.md +# The other half: .github/rulesets/main-branch.json +# +# How review works on this repository: +# +# A pull request that touches NO path listed below merges on green CI alone. +# The required checks are the gate; no human approval is requested or needed. +# A pull request that touches a path listed below additionally requires an +# approving review from that path's owner, with every review thread resolved. +# +# The ruleset sets `required_approving_review_count: 0`, which is the sanctioned +# "no blanket reviewers" value, and `require_code_owner_review: true`, which is +# evaluated per changed file. Listing a path here is therefore a commitment to +# review every change to it — not a label. +# +# Invariants. Each one silently breaks the gate rather than failing loudly, so +# `task check:rulesets` enforces all of them: +# +# - NEVER add a `*` catch-all. It makes every pull request code-owned and +# re-imposes the blanket gate this file exists to retire. +# - LAST matching pattern wins. Order broad to specific. +# - ALL owners for a pattern go on ONE line. A second line for the same +# pattern silently replaces the first rather than adding to it. +# - Owners need EXPLICIT write access to this repository. An owner without it +# is ignored with no error, and the path quietly becomes unowned. +# - `!` negation and `[ ]` ranges do not work here. A path listed with no +# owner is the supported way to carve out an explicitly unowned exception. +# - An owner's own pull requests are exempt: GitHub cannot request a review +# from the author, so authorship waives the requirement for the patterns +# that author owns. The `codeowners-notice` workflow posts a comment on +# those pull requests so a self-owned edit is at least visible. # # NOTE: musher-dev/platform's CODEOWNERS names `@merrelljustin`, which is not a # real GitHub account (the API returns 404). Do not copy that handle here — a # CODEOWNERS entry naming a nonexistent user silently disables the review gate # it appears to configure. +# +# What is deliberately NOT owned: `specifications/` and `conformance/` carry the +# normative contract, and GOVERNANCE.md still requires maintainer approval for a +# breaking change to either. That obligation is now social rather than +# mechanical — ADR 0015 records the trade and why it was accepted. -* @justinmerrell - -# The normative surface. A change here obligates the CLI, the API, and every -# SDK, and a compatibility break requires a new major directory. -/specifications/ @justinmerrell -/conformance/ @justinmerrell - -# Architecture decisions are immutable once accepted; supersede, never rewrite. -/docs/adr/ @justinmerrell - -# Release, publication, and branch-protection configuration. -/.github/ @justinmerrell +# The gate cannot be changed unreviewed. GitHub's own recommendation: protect +# CODEOWNERS with CODEOWNERS, and protect the ruleset the same way — between +# them these two paths are the whole control. +/.github/CODEOWNERS @justinmerrell +/.github/rulesets/ @justinmerrell diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index b4fa41f..b1eddc7 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -88,6 +88,19 @@ case for it. `check:conformance` fails otherwise, and the only way out is an entry in the runner's `UNCOVERED` list saying why the code cannot be exercised. +### How your pull request merges + +There is no blanket review requirement. A pull request that touches no path +listed in [`.github/CODEOWNERS`](CODEOWNERS) — which today is only the review +gate's own two configuration files — merges once the required checks are green: +`Lint`, `Schema`, `Site Build`, and `Signed off`. Nobody has to approve it. + +That is a deliberate trade, not an oversight. It puts the weight on the checks +below, which is where it belongs for a repository whose contract is machine +verifiable. [ADR 0015](../docs/adr/0015-selective-code-owner-review.md) explains +it, and GOVERNANCE.md still asks for a maintainer's eyes on a change of +consequence even where nothing blocks the merge. + `task check` runs, in order: | Step | What it enforces | @@ -95,6 +108,7 @@ runner's `UNCOVERED` list saying why the code cannot be exercised. | `check:format` | Biome formatting and lint of `tools/` | | `check:types` | TypeScript typecheck of `tools/` | | `check:config` | The `.config/` layout: every file indexed, reachable, and a declaration (CFG-01..CFG-08) | +| `check:rulesets` | The two halves of the review gate agree, and no required status check can hang a pull request (RUL-01..RUL-09) | | `check:schema` | Every `src/` module is valid JSON Schema 2020-12; `$id`s are unique and canonical; no remote `$ref` | | `check:drift` | The committed `dist/` bundle matches a fresh compile of `src/` | | `check:examples` | Every file in `examples/` validates against its family's bundle | diff --git a/.github/rulesets/RULESETS.md b/.github/rulesets/RULESETS.md index dd3214c..c0cc6b6 100644 --- a/.github/rulesets/RULESETS.md +++ b/.github/rulesets/RULESETS.md @@ -33,18 +33,104 @@ gh api -X PUT repos/musher-dev/spec/rulesets/ \ Protects the default branch: -- **Pull request required**, one approving review, **code-owner review - required**. Every change here alters a public contract; CODEOWNERS review is - the compatibility gate described in GOVERNANCE.md. -- **Stale reviews dismissed on push**, so an approval cannot survive a rewrite. +- **Pull request required**, with **no blanket approval** and **code-owner + review required** — see the next section, which is the whole point of the + file. - **Linear history**, squash-merge only. The specification's history should read as a sequence of deliberate changes. -- **Required status checks**: `Lint`, `Schema`, `Site Build`, `Signed off`. - `Schema` is the one that matters most — it carries the bundle drift gate. - `Site Build` carries the publication-ledger gates. `Signed off` is the DCO - check CONTRIBUTING.md requires. +- **Required status checks**: `Lint`, `Schema`, `Site Build`, `Signed off`, + each pinned to `integration_id: 15368` so only the GitHub Actions app can + satisfy them. `Schema` is the one that matters most — it carries the bundle + drift gate. `Site Build` carries the publication-ledger gates. `Signed off` + is the DCO check CONTRIBUTING.md requires. - **Deletion and force-push blocked.** +### The selective code-owner review gate + +Two parameters are paired deliberately, and neither means anything alone: + +```json +"required_approving_review_count": 0, +"require_code_owner_review": true +``` + +A pull request touching no path in `.github/CODEOWNERS` merges on green CI; one +touching an owned path also needs that owner's approval, with every review +thread resolved. `0` is GitHub's sanctioned "no blanket reviewers" value, and +the code-owner requirement is evaluated per changed file. +[ADR 0015](../../docs/adr/0015-selective-code-owner-review.md) says why. + +Six invariants keep the mechanism working. Each one breaks it *silently* — the +first four are checked by `task check:rulesets` (RUL-05..RUL-09); the last two +are only visible against live GitHub state: + +1. **No `*` catch-all in `.github/CODEOWNERS`.** It makes every pull request + code-owned, which is the blanket gate again wearing a different hat. +2. **`require_last_push_approval` stays `false`.** With zero required approvals + it produces a self-contradictory, unmergeable state. +3. **Every required context must name a job some workflow publishes**, and that + workflow must not filter on `paths:`. Either mistake yields a context that + never reports and a pull request that hangs forever. This is why the + validator runs inside the existing `Lint` job rather than as a workflow of + its own. +4. **Every root-anchored CODEOWNERS pattern must resolve to a real path.** + CODEOWNERS fails open: a stale entry reads as ownership and grants none. So + does an owner without explicit write access, or an invisible team. +5. **No classic branch protection rule may coexist on `main`.** Classic rules + and rulesets aggregate most-restrictive, so a leftover rule requiring one + approval silently restores blanket review. `gh api + repos/musher-dev/spec/branches/main/protection` must return `404`. +6. **No org-level ruleset may impose an approval count on this repository.** + Same aggregation. `spec` is deliberately absent from the org `pr-workflow` + ruleset's include list; do not add it. + +**An owner's own pull requests are exempt.** GitHub cannot request a review from +the author, so authorship waives the requirement for the patterns that author +owns. The gate protects owned paths from *other* contributors, not from their +owner. `.github/workflows/codeowners-notice.yml` posts a sticky comment on +self-owned edits so the waiver is at least visible. + +### Changing the review gate + +The aggregation traps in invariants 5 and 6 are undetectable offline, so a +change to the `pull_request` rule runs this sequence rather than just an apply: + +1. Reconcile any drift first (see below); do not layer a change on top of one. +2. `gh api repos/musher-dev/spec/branches/main/protection` — a `404` is the + desired answer. +3. `gh api repos/musher-dev/spec/rules/branches/main` lists every rule that + actually applies, whatever its source. Confirm no org-sourced `pull_request` + rule carries a nonzero `required_approving_review_count`. +4. Apply via the `PUT` recipe above. +5. Verify **both directions** with two throwaway pull requests: one touching no + owned path must show zero approvals required and merge on green CI; one + touching `.github/rulesets/` must block awaiting a code owner. Open the + second from a non-owner account, since an owner's own pull request is waived + by design. + +### Detecting drift + +Drift is the live ruleset diverging from these files because someone edited it +in the UI. `task check:rulesets` cannot see it: the default `GITHUB_TOKEN` lacks +`administration: read`, so a workflow-based detector would need a long-lived +secret or would fail open. It is an operator check — run it during a security +review, or whenever review behaviour surprises you: + +```sh +for pair in "20585885:main-branch.json" "20585889:release-tags.json"; do + id="${pair%%:*}"; file=".github/rulesets/${pair##*:}" + diff -u <(jq -S '{name,target,enforcement,bypass_actors,conditions,rules}' "$file") <(gh api "repos/musher-dev/spec/rulesets/$id" --jq '{name,target,enforcement,bypass_actors,conditions,rules}' | jq -S .) && echo "in step: $file" || echo "DRIFT: $file" +done +``` + +The live response also carries `dismissal_restriction`, which GitHub supplies +and `PUT` does not require; it is the one expected difference. + +If they diverge, decide which side wins. **File wins** — reapply with the `PUT` +recipe. **Live wins** — re-export into the file and open a pull request +explaining the change. Do not leave it unresolved: a ruleset nobody can predict +from the repository is a ruleset nobody reviews. + ## `release-tags.json` Makes releases immutable: @@ -67,7 +153,16 @@ become a published version. See ## Bypass `main-branch` allows `OrganizationAdmin` bypass, matching the org-level -convention. That is an escape hatch for incident response, not a workflow. +convention. That is an escape hatch for incident response, not a workflow — and +until 2026-08-25 it was the workflow, spent on eight consecutive merges because +the blanket approval requirement could not be satisfied by the only maintainer. +The selective gate above exists so that a bypass in the audit log is a signal +again. + +**`release-tags` declared `bypass_actors: []` and the live ruleset granted +`OrganizationAdmin` bypass anyway** — the file had never been re-applied. It was +reconciled in the file's favour on 2026-08-25, which is what the paragraph below +has always claimed. **`release-tags` allows no bypass at all.** Using a bypass there means mutating a published artifact, which is the one thing this repository promises never diff --git a/.github/rulesets/main-branch.json b/.github/rulesets/main-branch.json index 43d6abb..948b59f 100644 --- a/.github/rulesets/main-branch.json +++ b/.github/rulesets/main-branch.json @@ -10,7 +10,7 @@ }, "bypass_actors": [ { - "actor_id": 1, + "actor_id": null, "actor_type": "OrganizationAdmin", "bypass_mode": "always" } @@ -22,11 +22,13 @@ { "type": "pull_request", "parameters": { - "required_approving_review_count": 1, - "dismiss_stale_reviews_on_push": true, + "required_approving_review_count": 0, + "dismiss_stale_reviews_on_push": false, + "required_reviewers": [], "require_code_owner_review": true, "require_last_push_approval": false, "required_review_thread_resolution": true, + "require_extra_approval_for_unattributed_changes": true, "allowed_merge_methods": ["squash"] } }, @@ -36,10 +38,10 @@ "strict_required_status_checks_policy": true, "do_not_enforce_on_create": false, "required_status_checks": [ - { "context": "Lint" }, - { "context": "Schema" }, - { "context": "Site Build" }, - { "context": "Signed off" } + { "context": "Lint", "integration_id": 15368 }, + { "context": "Schema", "integration_id": 15368 }, + { "context": "Site Build", "integration_id": 15368 }, + { "context": "Signed off", "integration_id": 15368 } ] } } diff --git a/.github/workflows/codeowners-notice.yml b/.github/workflows/codeowners-notice.yml new file mode 100644 index 0000000..0e786e0 --- /dev/null +++ b/.github/workflows/codeowners-notice.yml @@ -0,0 +1,185 @@ +name: CODEOWNERS Notice + +# The compensating control for the one hole in the selective review gate. +# +# GitHub cannot request a review from a pull request's author, so authorship +# waives the code-owner requirement for the patterns that author owns. On this +# repository both owned paths have a single owner, which means the person most +# able to change the gate is exactly the person the gate never stops. Nothing +# would be reviewed and nobody would be told. +# +# So this workflow tells them. It posts, updates, and removes one sticky comment +# listing the paths in the pull request that its own author owns. +# +# It is a NOTIFIER, not a gate: it validates nothing, blocks nothing, and must +# never be added to the required checks in .github/rulesets/main-branch.json +# (`task check:rulesets` RUL-09 would accept it; the ruleset should not). A +# failure of the notifier itself still fails the workflow so it cannot rot +# unnoticed. +# +# Ownership matching approximates CODEOWNERS with `git check-ignore -v`, whose +# last-match-wins precedence is identical. The two places the approximation +# would diverge -- `!` negation and `[ ]` ranges -- are rejected by RUL-06, so +# it is exact for any file that passes CI. +# +# See docs/adr/0015-selective-code-owner-review.md. + +on: + pull_request: + types: [opened, synchronize, ready_for_review] + +permissions: + contents: read + pull-requests: write + +concurrency: + group: codeowners-notice-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + self-owned-notice: + name: Self-owned paths notice + # Drafts are skipped; `ready_for_review` re-fires this when the pull request + # leaves draft. + if: github.event.pull_request.draft == false && !endsWith(github.event.pull_request.user.login, '[bot]') + runs-on: ubuntu-latest + timeout-minutes: 3 + steps: + # CODEOWNERS is read from the base branch -- the same file GitHub itself + # uses to compute this pull request's review requirements. Reading the + # head would let a pull request rewrite the rules it is judged by. + - name: Check out the base branch CODEOWNERS + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + ref: ${{ github.event.pull_request.base.sha }} + sparse-checkout: .github/CODEOWNERS + sparse-checkout-cone-mode: false + + - name: Detect author-owned changed paths + id: detect + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + run: | + set -euo pipefail + + codeowners=.github/CODEOWNERS + if [[ ! -f "$codeowners" ]]; then + echo "no CODEOWNERS on the base branch; nothing to notice" + echo "self_owned=" >> "$GITHUB_OUTPUT" + exit 0 + fi + + workdir=$(mktemp -d) + + # Translate CODEOWNERS into a parallel pair: a gitignore-style pattern + # file with blank lines preserved so line numbers align, and a map + # from line number to that line's owners. + declare -A owners_by_line + lineno=0 + while IFS= read -r raw; do + lineno=$((lineno + 1)) + line="${raw%%#*}" + line="$(echo "$line" | xargs || true)" + if [[ -z "$line" ]]; then + echo "" >> "$workdir/patterns" + continue + fi + read -r pattern owners <<< "$line" + echo "$pattern" >> "$workdir/patterns" + owners_by_line[$lineno]="$owners" + done < "$codeowners" + + gh api --paginate "/repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/files" \ + --jq '.[].filename' > "$workdir/changed" || true + if [[ ! -s "$workdir/changed" ]]; then + echo "self_owned=" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # An empty scratch repository isolates matching from this repository's + # own .gitignore. `check-ignore -v` reports the LAST matching pattern + # per path, which is exactly CODEOWNERS precedence. + git init -q "$workdir/scratch" + git -C "$workdir/scratch" -c "core.excludesFile=$workdir/patterns" \ + check-ignore -v --no-index --stdin \ + < "$workdir/changed" > "$workdir/matches" || true + + author_lc="${PR_AUTHOR,,}" + : > "$workdir/self_owned" + while IFS= read -r match; do + src_line="${match%%$'\t'*}" + file="${match#*$'\t'}" + matched_lineno="$(cut -d: -f2 <<< "$src_line")" + owners="${owners_by_line[$matched_lineno]:-}" + # A pattern with no owner is the documented explicitly-unowned carve-out. + [[ -z "$owners" ]] && continue + owners_lc="${owners,,}" + read -ra owner_list <<< "$owners_lc" + is_author_owner=false + for owner in "${owner_list[@]}"; do + [[ "$owner" == "@${author_lc}" ]] && is_author_owner=true + done + [[ "$is_author_owner" == false ]] && continue + if [[ "${#owner_list[@]}" -eq 1 ]]; then + echo "- \`${file}\` — you are the **sole owner**; no review will be requested" >> "$workdir/self_owned" + else + echo "- \`${file}\` — co-owned (\`${owners}\`); the other owners were requested" >> "$workdir/self_owned" + fi + done < "$workdir/matches" + + { + echo "self_owned<> "$GITHUB_OUTPUT" + + - name: Upsert or remove the sticky comment + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + SELF_OWNED: ${{ steps.detect.outputs.self_owned }} + run: | + set -euo pipefail + + marker='' + all_ids=$(gh api --paginate \ + "/repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --jq ".[] | select(.body | startswith(\"$marker\")) | .id") + existing_id="${all_ids%%$'\n'*}" + + if [[ -z "$SELF_OWNED" ]]; then + if [[ -n "$existing_id" ]]; then + gh api --method DELETE \ + "/repos/${GITHUB_REPOSITORY}/issues/comments/${existing_id}" + echo "removed stale notice (no self-owned paths remain)" + else + echo "no self-owned paths; nothing to do" + fi + exit 0 + fi + + body="$(printf '%s\n' \ + "$marker" \ + '### You own some of the paths this pull request changes' \ + '' \ + "@${PR_AUTHOR}, this pull request touches paths you are a code owner for. Your authorship **waives the code-owner review requirement** — GitHub cannot request a review from a pull request's author — so nobody is automatically notified about these changes:" \ + '' \ + "$SELF_OWNED" \ + '' \ + 'Double-check they are intentional. If you want a second pair of eyes, request a review manually. (Automated notice from the codeowners-notice workflow — ADR 0015.)')" + + if [[ -n "$existing_id" ]]; then + gh api --method PATCH \ + "/repos/${GITHUB_REPOSITORY}/issues/comments/${existing_id}" \ + -f body="$body" > /dev/null + echo "updated existing notice" + else + gh api --method POST \ + "/repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + -f body="$body" > /dev/null + echo "posted notice" + fi diff --git a/CLAUDE.md b/CLAUDE.md index cd5819e..8236db9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,6 +38,13 @@ implementation of what is defined here. add a config to the repo root when the tool accepts a config flag, and never rely on default discovery — every caller names its config explicitly. `task check:config` enforces this (CFG-01..CFG-08). See docs/adr/0011. +9. **Review is a code-owner gate, not a blanket approval.** A pull request + touching no path in `.github/CODEOWNERS` merges on green CI; one touching an + owned path needs that owner. The two halves — `required_approving_review_count: + 0` plus `require_code_owner_review: true`, and a CODEOWNERS with no `*` + catch-all — are useless separately and both break silently, so never change + one without the other. `task check:rulesets` enforces this (RUL-01..RUL-09). + See docs/adr/0015. ## Layout diff --git a/GOVERNANCE.md b/GOVERNANCE.md index ce48ab3..5d5b4b3 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -34,7 +34,18 @@ beyond the [DCO](.github/CONTRIBUTING.md#sign-your-work) is required. ## Decision process Ordinary changes — a new optional field, a corrected description, an additional -conformance fixture — need one maintainer approval and green CI. +conformance fixture — need green CI. Green CI is the whole of the mechanical +gate: review is required only on the paths listed in +[`.github/CODEOWNERS`](.github/CODEOWNERS), which are the review gate's own two +configuration files and nothing else. + +Maintainer approval is still expected on a change of consequence, and this +document still requires it where it says so below. What changed is that it is +requested rather than enforced. A blanket approval requirement was tried and +measured: with one maintainer it blocked every pull request that maintainer opened, +and was merged past with an administrator bypass every time, which enforced nothing and +taught the bypass. [ADR 0015](docs/adr/0015-selective-code-owner-review.md) +records the reasoning and what it costs. Structural changes need an accepted ADR first. That covers: @@ -55,6 +66,13 @@ Any change that would cause a previously valid document to fail validation is a 2. A new `v` major directory — the previous major keeps working unchanged 3. A migration note in the new major's `spec.md` +Requirement 1 is an obligation on the maintainer, not a gate the repository +enforces: `specifications/` and `conformance/` are deliberately unowned, so a +breaking change is not blocked awaiting a review. What *is* enforced on every +pull request is the machinery that detects the breakage — `check:drift`, +`check:compat`, `check:published`, and the conformance suite, all required. See +[ADR 0015 §4](docs/adr/0015-selective-code-owner-review.md). + Adding a required field, narrowing an enum, tightening a pattern, and removing a field are all breaking. Adding an optional field is not. @@ -93,8 +111,10 @@ and the proposing pull request must say so in three parts: existing one in two makes both less useful, because a listing that could sit in either now sits in whichever its author picked. -Approval is one maintainer, as for any ordinary change. What is not ordinary is -that a reviewer is expected to reject a well-formed term on editorial grounds — +Approval is one maintainer. `specifications/` is unowned, so nothing blocks the +merge awaiting it — a category addition is one of the changes where the +obligation in Decision process is real and the gate is not. What is not ordinary +is that a reviewer is expected to reject a well-formed term on editorial grounds — a taxonomy is judged by what it excludes, and twenty categories is a taxonomy while sixty is a list with none. @@ -187,8 +207,11 @@ One more is worth naming, for a different reason. tree to Cloudflare Pages, which makes it the only dependency here that is handed a credential. It is pinned to an exact version in `tools/package.json` and the lockfile rather than fetched at deploy time, so the code that receives the token -changes only through a reviewed diff under CODEOWNERS. Nothing else in `tools/` -holds a secret, and nothing published derives from wrangler either. +changes only through a diff someone opened deliberately — never through a +resolution that moved on its own. `tools/` is not a CODEOWNERS path, so that +diff is not gated on a review; the exact pin plus the lockfile is what carries +the guarantee. Nothing else in `tools/` holds a secret, and nothing published +derives from wrangler either. ## Security diff --git a/Taskfile.yml b/Taskfile.yml index a5c6859..dc78958 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -59,6 +59,7 @@ tasks: cmds: - task: check:format - task: check:config + - task: check:rulesets - task: check:types - task: check:schema - task: check:drift diff --git a/docs/adr/0015-selective-code-owner-review.md b/docs/adr/0015-selective-code-owner-review.md new file mode 100644 index 0000000..298db6c --- /dev/null +++ b/docs/adr/0015-selective-code-owner-review.md @@ -0,0 +1,163 @@ +# ADR 0015: Review is a code-owner gate, not a blanket approval + +- **Status:** Accepted +- **Date:** 2026-08-25 +- **Refines:** [ADR 0001](0001-canonical-repository-architecture.md) + +## Context + +Until this decision the `main-branch` ruleset required one approving review on +every pull request, and `.github/CODEOWNERS` opened with a `*` catch-all naming +the repository's single maintainer. Together those two settings said: nothing +merges here without `@justinmerrell`'s approval. + +That is not what happened. `@justinmerrell` is the only owner, GitHub cannot +request a review from a pull request's author, and the ruleset grants +`OrganizationAdmin` an always-bypass. So every pull request they opened arrived at +the merge box blocked, and every one was merged by spending the bypass. Eight +consecutive merges to `main` — #42, #45, #47, #48, #52, #53, #55, #57 — carry +`reviewDecision: REVIEW_REQUIRED`. The gate was not satisfied on any of them. It +was stepped over on all of them. + +A control exercised only by bypassing it is worse than no control, for the +reason `.github/rulesets/RULESETS.md` already gives about tag creation: it +describes a protection a reader will then not think to add. It also trains the +one person who can disable the ruleset entirely to treat the bypass as a normal +step, which is precisely the habit that makes an admin account worth stealing. + +Meanwhile the requirement's real cost fell on everyone else. A contributor +fixing a typo in the dev container waited on the same approval as someone +narrowing an enum in a published schema, and the maintainer's attention was +spent equally on both. Review that is demanded everywhere is review that is +skimmed everywhere. + +`musher-dev/platform` reached the same conclusion and solved it, and it is worth +adopting the same mechanism rather than a third variant: a contributor moving +between the two repositories should not have to relearn when review is required. + +## Decision + +### 1. Review is required per changed file, not per pull request + +`.github/rulesets/main-branch.json` pairs two parameters: + +```json +"required_approving_review_count": 0, +"require_code_owner_review": true +``` + +A count of `0` is GitHub's sanctioned "no blanket reviewers" value; the +code-owner requirement is evaluated against each file the pull request touches. +Together they mean a pull request that touches no path in `.github/CODEOWNERS` +merges on green CI alone, and a pull request that touches an owned path +additionally needs that path's owner to approve, with every review thread +resolved. + +Review stops being a toll on all changes and becomes a guarantee about specific +ones. + +### 2. The owned set is the gate itself, and nothing else + +``` +/.github/CODEOWNERS @justinmerrell +/.github/rulesets/ @justinmerrell +``` + +These two paths *are* the control. Leaving them unowned would let a single pull +request quietly widen or remove the gate, so they are protected the way GitHub +recommends protecting CODEOWNERS: with CODEOWNERS. + +Everything else — including `specifications/` and `conformance/`, which carry +the normative contract — is unowned and merges on green CI. That is the part of +this decision that deserves to be uncomfortable, and §4 addresses it. + +### 3. Both halves are mechanically kept in step + +Either half is silently useless without the other, and both failure modes look +like tightening: add a `*` catch-all and every pull request is owned again; set +the count back to `1` and every pull request is blocked again. Neither change +produces an error. + +`tools/src/rulesets.ts`, run by `task check:rulesets` inside the existing `Lint` +job, enforces RUL-01..RUL-09 — the ruleset shape, the `0` + `require_code_owner_review` +pairing, `require_last_push_approval: false`, the absence of a catch-all, owner +syntax, and that every root-anchored CODEOWNERS pattern resolves to a real path. + +RUL-09 goes beyond platform's version, which documents the rule and enforces +nothing: every required status check must name a job that some workflow actually +publishes, and that workflow must not filter on `paths:`. Both mistakes produce +the same outcome — a required context that never reports, and a pull request +that hangs forever. It is also why this check runs inside `Lint` rather than as +its own workflow: a path-filtered validator would be the bug it exists to catch. + +### 4. Compatibility review survives as an obligation, not a gate + +GOVERNANCE.md still requires maintainer approval for a breaking change to a +published contract. Under §2 nothing mechanically enforces that on +`specifications/`. + +This is stated plainly rather than papered over. The trade is deliberate: the +mechanical gate on those paths was never actually satisfied, so removing it +gives up an enforcement that existed only on paper, while the checks that *did* +run — `check:drift`, `check:compat`, `check:published`, the conformance suite — +are unaffected and remain required. Those catch the specific failure a +compatibility review is looking for far more reliably than an approval click +does. + +### 5. A self-owned edit is announced + +GitHub waives the code-owner requirement for a pull request's own author. With a +single owner on both governance paths, that means the person able to change the +gate is the person it never stops. + +`.github/workflows/codeowners-notice.yml` posts and maintains one sticky comment +listing the paths in a pull request that its own author owns. It is a notifier, +not a gate: it blocks nothing and is deliberately absent from the required +checks. It converts a silent waiver into a visible one. + +## Alternatives considered + +**Keep the blanket approval and stop bypassing it.** This is the honest version +of the status quo, and it does not work with one maintainer: nobody can +approve their own pull request, so the repository would be unable to accept its +own maintainer's changes at all. + +**Own the normative surface too** (`/specifications/`, `/conformance/`, +`/docs/adr/`). Tempting, and it would keep GOVERNANCE.md's compatibility-review +promise mechanical. Rejected because with a single owner it reproduces the +original failure exactly: that maintainer's own contract changes would still be +waived by authorship, and everyone else's would still be blocked on one person. It buys a +gate that binds only the contributors it was not written for. When there is a +second maintainer this is the first thing to revisit. + +**Require two approvals with a bot bypass for automation.** More machinery, a +dedicated GitHub App to provision, and still no answer to the one-maintainer +problem. + +## Consequences + +- A pull request touching only `tools/`, `.devcontainer/`, `.config/`, prose, or + the specifications merges on green CI. The required checks — `Lint`, `Schema`, + `Site Build`, `Signed off` — become the real gate, and their coverage now + matters more than it did. +- Changing the gate still requires review, from the owner of the two paths that + define it. +- The `OrganizationAdmin` bypass stops being part of the daily workflow and goes + back to being an incident-response escape hatch. A bypass in the audit log is + once again a signal. +- GOVERNANCE.md's approval requirements are now social where they were + advertised as mechanical, and say so. +- A second maintainer changes the calculus: at that point owning + `specifications/` and `conformance/` costs nothing and buys back the + compatibility gate. Revisit then, not before. + +## Follow-ups + +- Live ruleset state is still applied by hand, per `.github/rulesets/RULESETS.md`. + The default `GITHUB_TOKEN` cannot read rulesets, so drift detection needs an + operator with a PAT; `task check:rulesets` validates the committed files but + cannot see GitHub. +- `Conventional PR title` (from `lint-pr.yml`) is not a required check. Platform + requires its equivalent. It is a candidate for the required set once its + `pull_request_target` trigger has been reviewed for the token exposure that + trigger implies. diff --git a/taskfiles/check.Taskfile.yml b/taskfiles/check.Taskfile.yml index 45a3d85..f7f962d 100644 --- a/taskfiles/check.Taskfile.yml +++ b/taskfiles/check.Taskfile.yml @@ -33,6 +33,12 @@ tasks: cmds: - bun run src/config.ts + rulesets: + desc: Verify the review gate holds together (RUL-01..RUL-09) + dir: '{{.ROOT_DIR}}/{{.TOOLS_DIR}}' + cmds: + - bun run src/rulesets.ts + # --- The normative gates --------------------------------------------------- schema: diff --git a/taskfiles/ci.Taskfile.yml b/taskfiles/ci.Taskfile.yml index 8891226..5054c16 100644 --- a/taskfiles/ci.Taskfile.yml +++ b/taskfiles/ci.Taskfile.yml @@ -18,6 +18,7 @@ tasks: cmds: - task: :check:format - task: :check:config + - task: :check:rulesets - task: :check:commits - task: :check:links - task: :check:md diff --git a/tools/src/rulesets.ts b/tools/src/rulesets.ts new file mode 100644 index 0000000..a2ac57e --- /dev/null +++ b/tools/src/rulesets.ts @@ -0,0 +1,313 @@ +/** + * Keep the two halves of the selective code-owner review gate in step. + * + * The gate is one mechanism split across two files. `.github/rulesets/ + * main-branch.json` pairs `required_approving_review_count: 0` with + * `require_code_owner_review: true`, so review is demanded per changed file + * rather than per pull request; `.github/CODEOWNERS` decides which files those + * are. Break either half and the other becomes meaningless — but neither fails + * loudly. A `*` catch-all restores the blanket gate. A count of `1` restores it + * from the other side. Both changes look like tightening and read as harmless. + * + * The ruleset files are also `PUT` back to GitHub verbatim, which makes them + * the rare config where a shape error is discovered in production: a leaked + * server-side field makes the apply fail, and a mistyped required-check context + * makes every pull request hang forever waiting for a check that will never + * report. RUL-03 and RUL-09 are those two failure modes. + * + * RUL-09 has no counterpart in `musher-dev/platform`, which documents the rule + * in prose and enforces nothing. It is the reason this runs inside the existing + * `Lint` job rather than as its own workflow: a required check that lives in a + * `paths:`-filtered workflow is the very hang it exists to prevent. + * + * See docs/adr/0015-selective-code-owner-review.md and + * .github/rulesets/RULESETS.md. + * + * NON-NORMATIVE, like everything under tools/. + */ +import { existsSync, readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { parse } from 'yaml' +import { Failures, REPO_ROOT } from './spec.ts' + +const RULESETS_DIR = join('.github', 'rulesets') +const CODEOWNERS = join('.github', 'CODEOWNERS') +const WORKFLOWS_DIR = join('.github', 'workflows') + +/** The ruleset carrying the review gate. Named because RUL-07/08 only apply to it. */ +const MAIN_RULESET = 'main-branch.json' + +const VALID_TARGETS = ['branch', 'tag', 'push', 'repository'] +const VALID_ENFORCEMENT = ['active', 'disabled', 'evaluate'] + +/** + * Fields `GET /rulesets/{id}` adds and `PUT` rejects. Committing one is how a + * re-export of live state turns into an apply that fails at the API. + */ +const SERVER_SIDE_FIELDS = [ + 'id', + 'node_id', + 'source', + 'source_type', + 'current_user_can_bypass', + '_links', + 'created_at', + 'updated_at', +] + +/** `@user` or `@org/team`. GitHub also allows a bare email; we do not. */ +const OWNER_PATTERN = /^@[A-Za-z0-9][A-Za-z0-9-]*(\/[A-Za-z0-9._-]+)?$/ + +interface CodeownersEntry { + readonly line: number + readonly pattern: string + readonly owners: string[] +} + +/** Strip comments and blanks; return one entry per meaningful line. */ +function parseCodeowners(text: string): CodeownersEntry[] { + const entries: CodeownersEntry[] = [] + const lines = text.split('\n') + for (let i = 0; i < lines.length; i++) { + const stripped = (lines[i] ?? '').replace(/#.*$/, '').trim() + if (stripped === '') continue + const [pattern, ...owners] = stripped.split(/\s+/) + if (pattern === undefined) continue + entries.push({ line: i + 1, pattern, owners }) + } + return entries +} + +/** + * Every job name a workflow publishes as a check context, and whether that + * workflow filters by path. + * + * A job with no `name:` reports under its key, which is what GitHub does. Only + * `paths`/`paths-ignore` matter: a `branches:` filter still reports on every + * pull request targeting that branch. + */ +interface WorkflowContexts { + readonly contexts: Map + readonly pathFiltered: Set +} + +function workflowContexts(repoRoot: string): WorkflowContexts { + const contexts = new Map() + const pathFiltered = new Set() + const dir = join(repoRoot, WORKFLOWS_DIR) + if (!existsSync(dir)) return { contexts, pathFiltered } + + for (const file of readdirSync(dir).sort()) { + if (!file.endsWith('.yml') && !file.endsWith('.yaml')) continue + let doc: unknown + try { + doc = parse(readFileSync(join(dir, file), 'utf8')) + } catch { + continue // check:workflow owns YAML validity; do not double-report. + } + if (typeof doc !== 'object' || doc === null) continue + const workflow = doc as Record + + // YAML 1.2 keeps `on` a string, but a 1.1-minded editor may yield `true`. + const triggers = (workflow.on ?? workflow.true) as Record | undefined + const filtered = + typeof triggers === 'object' && + triggers !== null && + Object.values(triggers).some( + (event) => + typeof event === 'object' && + event !== null && + ('paths' in event || 'paths-ignore' in event), + ) + + const jobs = workflow.jobs as Record | undefined + if (typeof jobs !== 'object' || jobs === null) continue + for (const [id, job] of Object.entries(jobs)) { + const name = + typeof job === 'object' && + job !== null && + typeof (job as { name?: unknown }).name === 'string' + ? (job as { name: string }).name + : id + contexts.set(name, file) + if (filtered) pathFiltered.add(name) + } + } + return { contexts, pathFiltered } +} + +export function rulesetViolations(repoRoot: string = REPO_ROOT): string[] { + const problems: string[] = [] + const dir = join(repoRoot, RULESETS_DIR) + + const files = existsSync(dir) + ? readdirSync(dir) + .filter((f) => f.endsWith('.json')) + .sort() + : [] + + if (files.length === 0) { + problems.push( + `RUL-01: no ruleset files under ${RULESETS_DIR}/. Branch and tag protection ` + + 'would then live only in the GitHub UI, where it cannot be reviewed or restored.', + ) + } + + const { contexts, pathFiltered } = workflowContexts(repoRoot) + + for (const file of files) { + const rel = `${RULESETS_DIR}/${file}` + let ruleset: Record + try { + ruleset = JSON.parse(readFileSync(join(dir, file), 'utf8')) + } catch (error) { + problems.push(`RUL-01: ${rel} is not valid JSON — ${(error as Error).message}`) + continue + } + + for (const key of ['name', 'target', 'enforcement', 'rules']) { + if (!(key in ruleset)) problems.push(`RUL-01: ${rel} is missing the required key \`${key}\`.`) + } + + const target = ruleset.target + if (typeof target === 'string' && !VALID_TARGETS.includes(target)) { + problems.push( + `RUL-02: ${rel} has target \`${target}\`; expected ${VALID_TARGETS.join(' | ')}.`, + ) + } + + const enforcement = ruleset.enforcement + if (typeof enforcement === 'string' && !VALID_ENFORCEMENT.includes(enforcement)) { + problems.push( + `RUL-02: ${rel} has enforcement \`${enforcement}\`; expected ${VALID_ENFORCEMENT.join(' | ')}.`, + ) + } + + for (const field of SERVER_SIDE_FIELDS) { + if (field in ruleset) { + problems.push( + `RUL-03: ${rel} carries the server-side field \`${field}\`. GET adds it and PUT ` + + 'rejects it, so applying this file would fail. Strip it before committing.', + ) + } + } + + const rules = ruleset.rules + if (!Array.isArray(rules) || rules.length === 0) { + problems.push(`RUL-04: ${rel} has an empty \`rules\` array, which protects nothing.`) + continue + } + + const typed = rules.filter( + (rule): rule is Record => typeof rule === 'object' && rule !== null, + ) + + if (file === MAIN_RULESET) { + const pull = typed.find((rule) => rule.type === 'pull_request') + if (pull === undefined) { + problems.push(`RUL-07: ${rel} has no \`pull_request\` rule — the review gate is gone.`) + } else { + const parameters = (pull.parameters ?? {}) as Record + if (parameters.require_code_owner_review !== true) { + problems.push( + `RUL-07: ${rel} must set \`require_code_owner_review: true\`. It is the half of ` + + 'the gate that makes CODEOWNERS mean anything.', + ) + } + if (parameters.required_approving_review_count !== 0) { + problems.push( + `RUL-07: ${rel} must set \`required_approving_review_count: 0\`. A nonzero count ` + + 're-imposes blanket review on pull requests that own no code.', + ) + } + if (parameters.require_last_push_approval !== false) { + problems.push( + `RUL-08: ${rel} must keep \`require_last_push_approval: false\`. Combined with ` + + 'zero required approvals it produces a state no pull request can ever satisfy.', + ) + } + } + } + + for (const rule of typed) { + if (rule.type !== 'required_status_checks') continue + const parameters = (rule.parameters ?? {}) as Record + const checks = parameters.required_status_checks + if (!Array.isArray(checks)) continue + for (const check of checks) { + const context = (check as { context?: unknown }).context + if (typeof context !== 'string') continue + const source = contexts.get(context) + if (source === undefined) { + problems.push( + `RUL-09: ${rel} requires the check \`${context}\`, which no job in ` + + `${WORKFLOWS_DIR}/ publishes. A required context that never reports leaves every ` + + 'pull request permanently unmergeable.', + ) + } else if (pathFiltered.has(context)) { + problems.push( + `RUL-09: ${rel} requires \`${context}\`, published by ${WORKFLOWS_DIR}/${source}, ` + + 'which filters on `paths:`. It will not report on a pull request the filter ' + + 'misses, and that pull request hangs forever.', + ) + } + } + } + } + + problems.push(...codeownersViolations(repoRoot)) + return problems +} + +function codeownersViolations(repoRoot: string): string[] { + const problems: string[] = [] + const path = join(repoRoot, CODEOWNERS) + + if (!existsSync(path)) { + problems.push( + `RUL-05: ${CODEOWNERS} is missing. With \`require_code_owner_review: true\` and no ` + + 'ownership registry, the gate protects nothing at all.', + ) + return problems + } + + for (const entry of parseCodeowners(readFileSync(path, 'utf8'))) { + const at = `${CODEOWNERS}:${entry.line}` + + if (entry.pattern === '*') { + problems.push( + `RUL-05: ${at} is a \`*\` catch-all. It makes every pull request code-owned and ` + + 'restores the blanket review gate ADR 0015 retired.', + ) + continue + } + + for (const owner of entry.owners) { + if (!OWNER_PATTERN.test(owner)) { + problems.push( + `RUL-06: ${at} has the malformed owner \`${owner}\`; expected @user or @org/team.`, + ) + } + } + + // A root-anchored literal pattern that matches nothing owns nothing, and + // CODEOWNERS reports no error for it — the path silently becomes unowned. + const literal = entry.pattern.startsWith('/') && !/[*?[\]]/.test(entry.pattern) + if (literal && !existsSync(join(repoRoot, entry.pattern.slice(1)))) { + problems.push( + `RUL-06: ${at} points at \`${entry.pattern}\`, which does not exist. CODEOWNERS fails ` + + 'open, so this entry reads as ownership while granting none.', + ) + } + } + + return problems +} + +function main(): void { + const failures = new Failures() + for (const problem of rulesetViolations()) failures.add(problem) + failures.report('Ruleset and CODEOWNERS halves of the review gate agree (RUL-01..RUL-09).') +} + +if (import.meta.main) main()