-
Notifications
You must be signed in to change notification settings - Fork 214
Add default-branch-protection ruleset with FVM path-scoped review policy #8613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Kay-Zee
wants to merge
1
commit into
master
Choose a base branch
from
kan/fvm-review-policy-ruleset
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+541
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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/<name>.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/<team-slug> --jq .id | ||
| ``` | ||
|
|
||
| Currently used: | ||
|
|
||
| | team slug | numeric id | used in | | ||
| | ---------------------- | ---------- | ------------------------------------ | | ||
| | `flow-core-protocol` | 11293013 | `default-branch-protection.json` (required reviewer) | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <path/to/spec.json> | ||
| # | ||
| # 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 <path/to/spec.json>" >&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 | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Paginate the
gh apicall to ensure all rulesets are retrieved.Both files list repository rulesets without pagination. By default, the GitHub API paginates results (30 items per page). If the repository accumulates many rulesets, a single call may not return the complete list, causing drift detection or apply logic to silently miss existing rulesets. Adding
--paginateautomatically fetches and merges all pages..github/rulesets/apply.sh#L46-L47: Add--paginateto thegh apicall fetching the rulesets index..github/workflows/verify-rulesets.yml#L37-L38: Add--paginateto thegh apicall fetching the rulesets index.📍 Affects 2 files
.github/rulesets/apply.sh#L46-L47(this comment).github/workflows/verify-rulesets.yml#L37-L38🤖 Prompt for AI Agents