From 1c68f54e0a848d2c908e47768608d493d067dfb6 Mon Sep 17 00:00:00 2001 From: Kan Zhang Date: Mon, 20 Jul 2026 15:31:25 -0700 Subject: [PATCH] Add default-branch-protection ruleset with FVM path-scoped review policy --- .github/rulesets/README.md | 142 ++++++++++ .github/rulesets/apply.sh | 57 ++++ .../rulesets/default-branch-protection.json | 248 ++++++++++++++++++ .github/workflows/verify-rulesets.yml | 94 +++++++ 4 files changed, 541 insertions(+) create mode 100644 .github/rulesets/README.md create mode 100755 .github/rulesets/apply.sh create mode 100644 .github/rulesets/default-branch-protection.json create mode 100644 .github/workflows/verify-rulesets.yml diff --git a/.github/rulesets/README.md b/.github/rulesets/README.md new file mode 100644 index 00000000000..080643a2f17 --- /dev/null +++ b/.github/rulesets/README.md @@ -0,0 +1,142 @@ +# Rulesets + +Source of truth for branch protection policy on this repository. Each JSON +file here is the desired shape of a named GitHub ruleset. The +[`verify-rulesets`](../workflows/verify-rulesets.yml) workflow compares the +live rulesets against these specs on every push, on every PR touching this +directory, and daily on a schedule. + +The workflow is **read-only** — it never modifies GitHub state. When a spec +and live differ, a repo admin must reconcile via +`Settings -> Rules -> Rulesets` in the UI. This gives us: + +- A PR-reviewable audit trail for every policy change. +- Zero write tokens sitting in the repo. +- An automated alarm when someone hand-edits the UI without a matching PR. + +## Applying changes + +Ruleset changes are applied by a repo admin running [`apply.sh`](apply.sh) +from their own machine, using their own `gh` credentials — no service +account, no long-lived tokens, no CI job with write access. + +``` +.github/rulesets/apply.sh .github/rulesets/default-branch-protection.json +``` + +The script creates the ruleset if it doesn't exist yet, or updates it in +place if it does. It requires the caller to have Administration: write on +the repo. Nothing in this repo grants that permission — the caller uses +their own admin standing. + +## Change flow + +For any change to a ruleset: + +1. Open a PR editing the relevant JSON file. Reviewers evaluate the + *policy change* here — this is the substantive review. +2. Before merging, a repo admin runs `apply.sh` locally against the + proposed spec (checkout the branch, then run the command above). This + applies the change to the live ruleset. +3. The `verify-rulesets` check turns green (may need a re-run via + `workflow_dispatch`), unblocking the merge. +4. Merge the PR. The next daily run confirms the state. + +If the check fails on `master` or on a schedule run, the message names +which ruleset drifted. Reconcile by re-running `apply.sh` against the +spec on `master`, or by making the intentional change via a PR. + +## Bootstrapping a new ruleset + +For a spec that does not yet exist live, the same command creates it: + +``` +.github/rulesets/apply.sh .github/rulesets/.json +``` + +After creation, run the `verify-rulesets` workflow once via +`workflow_dispatch` to confirm live matches spec. + +## Policies + +### `default-branch-protection.json` + +Mirrors the legacy branch protection currently on `master`, and adds a +path-scoped required-reviewer entry for the FVM policy. Intended to +replace the legacy branch protection entirely once bootstrapped. + +The ruleset carries four rules: + +- **`deletion`** — the branch cannot be deleted. +- **`non_fast_forward`** — force pushes are blocked. +- **`pull_request`** — 2 approving reviews, code-owner review required, + stale reviews are NOT dismissed on push (matches legacy). Adds the FVM + path-scoped required reviewer (see below). +- **`required_status_checks`** — the 48 checks currently required by legacy + branch protection, with strict mode (branches must be up to date). + +`bypass_actors` grants `RepositoryRole` id `5` (Admin) always-bypass, +matching the legacy setting `enforce_admins.enabled: false` — any repo +admin can push past the ruleset. This is intentionally the least +restrictive option and mirrors current behavior 1:1. + +If a stricter policy is desired later, options include: switching to +`actor_type: "OrganizationAdmin"` (org owners only, no repo admins), +adding `bypass_mode: "pull_request"` (bypass allowed only on PRs, not +direct pushes), or removing `bypass_actors` entirely (no bypass). + +Existing tag ruleset on this repo uses a team-based bypass +(`actor_type: "Team"`, `flow-engineering`). We deliberately do not match +that here — the tag ruleset and this branch ruleset serve different +purposes and can carry different bypass policies. + +#### FVM path-scoped review policy + +The `pull_request` rule includes a `required_reviewers` entry: + +```json +{ + "reviewer_id": 11293013, + "file_patterns": ["*", "!fvm/**"], + "approvals_needed": 2 +} +``` + +`reviewer_id: 11293013` is `@onflow/flow-core-protocol`, the team CODEOWNERS +already designates as the owner of the whole repo. Combined with the global +`required_approving_review_count: 2`, the effect is: + +| PR touches | Approvals that satisfy the policy | +| -------------------- | ----------------------------------------------------------------------------------------------------- | +| non-FVM files | 2 humans in `@onflow/flow-core-protocol`. Bot approvals do not satisfy the team requirement. | +| FVM files only | Any 2 approvers with write access (satisfies the global count; no team requirement on FVM paths). | +| Mixed FVM + non-FVM | Must satisfy both: 2 humans in `@onflow/flow-core-protocol` for the non-FVM portion. | + +The point is to let an approving AI reviewer count as one of the two on +strictly-FVM PRs, without allowing bot approvals to substitute for humans +anywhere else. + +**Assumption:** exactly one AI reviewer account exists on the repo. If a +second is introduced, add a second required-reviewer entry with +`file_patterns: ["fvm/**"]` and `approvals_needed: 1` pointing at a +humans-only team, so a "2 bots, 0 humans" merge on FVM stays impossible. + +If FVM eventually gets a dedicated reviewer team (e.g. +`@onflow/flow-cadence-execution`, which exists on the org and is described +as covering the Cadence execution stack), a second required-reviewer entry +can carry that too. + +### Team IDs + +`reviewer_id` in the JSON is a numeric GitHub team ID, not a slug. To +resolve or verify one: + +``` +gh api /orgs/onflow/teams/ --jq .id +``` + +Currently used: + +| team slug | numeric id | used in | +| ---------------------- | ---------- | ------------------------------------ | +| `flow-core-protocol` | 11293013 | `default-branch-protection.json` (required reviewer) | diff --git a/.github/rulesets/apply.sh b/.github/rulesets/apply.sh new file mode 100755 index 00000000000..85efde7c6df --- /dev/null +++ b/.github/rulesets/apply.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# +# Apply a ruleset JSON spec to the repository via the GitHub REST API. +# +# Uses the currently-authenticated `gh` credentials. Requires the caller +# to have Administration: write on the target repo — this is a manual +# admin action, not something CI runs. +# +# usage: +# .github/rulesets/apply.sh +# +# Creates the ruleset if it does not yet exist (by name), updates it in +# place otherwise. Idempotent: re-running when the live and spec already +# match is a no-op from the caller's perspective. + +set -euo pipefail + +if [ $# -ne 1 ]; then + echo "usage: $0 " >&2 + exit 2 +fi + +spec="$1" +if [ ! -f "$spec" ]; then + echo "error: spec not found: $spec" >&2 + exit 2 +fi + +if ! command -v gh >/dev/null; then + echo "error: gh CLI is required" >&2 + exit 2 +fi +if ! command -v jq >/dev/null; then + echo "error: jq is required" >&2 + exit 2 +fi + +repo=$(gh repo view --json nameWithOwner --jq .nameWithOwner) +name=$(jq -r .name "$spec") + +if [ -z "$name" ] || [ "$name" = "null" ]; then + echo "error: spec is missing top-level 'name' field: $spec" >&2 + exit 2 +fi + +id=$(gh api "/repos/$repo/rulesets" \ + | jq -r --arg n "$name" '.[] | select(.name == $n) | .id') + +if [ -z "$id" ] || [ "$id" = "null" ]; then + echo "creating ruleset '$name' on $repo..." + gh api -X POST "/repos/$repo/rulesets" --input "$spec" >/dev/null + echo "created." +else + echo "updating ruleset '$name' (id $id) on $repo..." + gh api -X PUT "/repos/$repo/rulesets/$id" --input "$spec" >/dev/null + echo "updated." +fi diff --git a/.github/rulesets/default-branch-protection.json b/.github/rulesets/default-branch-protection.json new file mode 100644 index 00000000000..edc4a4c37ed --- /dev/null +++ b/.github/rulesets/default-branch-protection.json @@ -0,0 +1,248 @@ +{ + "name": "default-branch-protection", + "target": "branch", + "enforcement": "active", + "bypass_actors": [ + { + "actor_id": 5, + "actor_type": "RepositoryRole", + "bypass_mode": "always" + } + ], + "conditions": { + "ref_name": { + "include": [ + "~DEFAULT_BRANCH" + ], + "exclude": [] + } + }, + "rules": [ + { + "type": "deletion" + }, + { + "type": "non_fast_forward" + }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 2, + "dismiss_stale_reviews_on_push": false, + "require_code_owner_review": true, + "require_last_push_approval": false, + "required_review_thread_resolution": false, + "required_reviewers": [ + { + "reviewer_id": 11293013, + "file_patterns": [ + "*", + "!fvm/**" + ], + "approvals_needed": 2 + } + ] + } + }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": true, + "required_status_checks": [ + { + "context": "Lint (./)", + "integration_id": 15368 + }, + { + "context": "Lint (./integration/)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (cmd)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (consensus)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (admin)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (fvm)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (others)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (ledger)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (module)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (utils)", + "integration_id": 15368 + }, + { + "context": "Tidy", + "integration_id": 15368 + }, + { + "context": "Lint (./insecure/)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/p2p)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/access)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/collection)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/common)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/consensus)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/execution/computation)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/execution/ingestion)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/execution)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (engine/verification)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (module/dkg)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/test/cohort1)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/test/cohort2)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/alsp)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/p2p/connection)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network/p2p/scoring)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (network)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (state)", + "integration_id": 15368 + }, + { + "context": "Unit Tests (storage)", + "integration_id": 15368 + }, + { + "context": "Integration Tests Others (integration)", + "integration_id": 15368 + }, + { + "context": "Integration Tests (MVP Integration Tests, make -C integration mvp-tests, blacksmith-4vcpu-ubuntu-...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Network Integration Tests, make -C integration network-tests, blacksmith-4vcpu...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Access Cohort1 Integration Tests, make -C integration access-cohort1-tests, bl...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Collection Integration Tests, make -C integration collection-tests, blacksmith...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Consensus Integration Tests, make -C integration consensus-tests, blacksmith-4...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Execution Integration Tests, make -C integration execution-tests, blacksmith-4...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Ghost Integration Tests, make -C integration ghost-tests, blacksmith-4vcpu-ubu...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Upgrade Integration Tests, make -C integration upgrades-tests, blacksmith-4vcp...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Access Cohort2 Integration Tests, make -C integration access-cohort2-tests, bl...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Access Cohort3 Integration Tests, make -C integration access-cohort3-tests, bl...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Access Cohort4 Integration Tests, make -C integration access-cohort4-tests, bl...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (BFT (Gossipsub) Integration Tests, make -C integration bft-gossipsub-tests, bl...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (BFT (Protocol) Integration Tests, make -C integration bft-protocol-tests, blac...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Epoch Cohort1 Integration Tests, make -C integration epochs-cohort1-tests, bla...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Epoch Cohort2 Integration Tests, make -C integration epochs-cohort2-tests, bla...", + "integration_id": 15368 + }, + { + "context": "Integration Tests (Verification Integration Tests, make -C integration verification-tests, blacks...", + "integration_id": 15368 + } + ] + } + } + ] +} diff --git a/.github/workflows/verify-rulesets.yml b/.github/workflows/verify-rulesets.yml new file mode 100644 index 00000000000..0ed30d14c6f --- /dev/null +++ b/.github/workflows/verify-rulesets.yml @@ -0,0 +1,94 @@ +name: verify-rulesets + +# Verifies that every JSON spec under .github/rulesets/ matches the live +# ruleset with the same name on this repository. Read-only: the workflow +# never writes to GitHub. On drift, it fails loudly so a repo admin can +# reconcile via the UI (Settings -> Rules -> Rulesets). +# +# See .github/rulesets/README.md for the reconciliation flow. + +on: + pull_request: + paths: + - '.github/rulesets/**' + - '.github/workflows/verify-rulesets.yml' + push: + branches: [master] + paths: + - '.github/rulesets/**' + - '.github/workflows/verify-rulesets.yml' + schedule: + - cron: '0 8 * * *' # daily at 08:00 UTC, catches out-of-band UI edits + workflow_dispatch: + +permissions: + administration: read + contents: read + +jobs: + verify: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Fetch live rulesets index + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh api "/repos/${{ github.repository }}/rulesets" > live_index.json + + - name: Compare each spec against live + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + + # Fields that live rulesets carry but the spec does not. We strip + # both sides equally before diffing so unrelated metadata cannot + # produce false positives. bypass_actors is intentionally NOT + # stripped: it is part of the policy and we want drift on it. + STRIP='del( + .id, + .node_id, + .created_at, + .updated_at, + .source, + .source_type, + ._links, + .current_user_can_bypass + )' + + shopt -s nullglob + specs=(.github/rulesets/*.json) + if [ ${#specs[@]} -eq 0 ]; then + echo "no ruleset specs found under .github/rulesets/" + exit 0 + fi + + fail=0 + for spec in "${specs[@]}"; do + name=$(jq -r .name "$spec") + id=$(jq -r --arg n "$name" \ + '.[] | select(.name == $n) | .id' live_index.json) + + if [ -z "$id" ] || [ "$id" = "null" ]; then + echo "::error file=$spec::ruleset '$name' does not exist on the live repo" + echo " create it via Settings -> Rules -> Rulesets, then re-run this workflow." + fail=1 + continue + fi + + gh api "/repos/${{ github.repository }}/rulesets/$id" > "live_${name}.json" + + jq "$STRIP" "live_${name}.json" | jq -S . > "live_${name}.norm.json" + jq "$STRIP" "$spec" | jq -S . > "spec_${name}.norm.json" + + if ! diff -u "spec_${name}.norm.json" "live_${name}.norm.json"; then + echo "::error file=$spec::ruleset '$name' has drifted from the spec" + echo " reconcile the live ruleset via Settings -> Rules -> Rulesets" + echo " to match this spec, then re-run this workflow." + fail=1 + fi + done + + exit $fail