Add default-branch-protection ruleset with FVM path-scoped review policy#8613
Add default-branch-protection ruleset with FVM path-scoped review policy#8613Kay-Zee wants to merge 1 commit into
Conversation
Dependency ReviewThe following issues were found:
License Issues.github/workflows/verify-rulesets.yml
OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughThe PR adds a declarative default-branch protection ruleset, an administrator-run script for creating or updating GitHub rulesets, documentation for the operating model, and a read-only workflow that detects drift between JSON specifications and live rulesets. ChangesRuleset management
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/rulesets/README.md (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify the language for fenced code blocks.
Adding a language identifier (such as
shorbash) to fenced code blocks improves markdown readability by enabling proper syntax highlighting.
.github/rulesets/README.md#L23-L25: Addshto the opening backticks..github/rulesets/README.md#L53-L55: Addshto the opening backticks..github/rulesets/README.md#L134-L136: Addshto the opening backticks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/rulesets/README.md around lines 23 - 25, Specify the shell language on the opening fences for all three command examples in .github/rulesets/README.md: lines 23-25, 53-55, and 134-136. Add the sh identifier while preserving each example’s existing command and content.Source: Linters/SAST tools
.github/workflows/verify-rulesets.yml (1)
32-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrevent credential persistence in the checkout action.
Setting
persist-credentials: falseensures theGITHUB_TOKENisn't left in the local git config, reducing the risk of accidental exposure in subsequent steps.🛡️ Proposed fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/verify-rulesets.yml at line 32, Update the actions/checkout@v4 step in the workflow to set persist-credentials to false, ensuring the checkout action does not retain the GITHUB_TOKEN in local Git configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/rulesets/apply.sh:
- Around line 46-47: Paginate both rulesets index requests by adding the gh api
pagination option to the calls in .github/rulesets/apply.sh lines 46-47 and
.github/workflows/verify-rulesets.yml lines 37-38; no other logic changes are
needed.
In @.github/workflows/verify-rulesets.yml:
- Around line 24-27: Remove the invalid administration entry from the workflow’s
permissions block, leaving contents: read as the required permission for the
ruleset verification job.
---
Nitpick comments:
In @.github/rulesets/README.md:
- Around line 23-25: Specify the shell language on the opening fences for all
three command examples in .github/rulesets/README.md: lines 23-25, 53-55, and
134-136. Add the sh identifier while preserving each example’s existing command
and content.
In @.github/workflows/verify-rulesets.yml:
- Line 32: Update the actions/checkout@v4 step in the workflow to set
persist-credentials to false, ensuring the checkout action does not retain the
GITHUB_TOKEN in local Git configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d44e5258-e129-4869-833b-2f2446a29f02
📒 Files selected for processing (4)
.github/rulesets/README.md.github/rulesets/apply.sh.github/rulesets/default-branch-protection.json.github/workflows/verify-rulesets.yml
| id=$(gh api "/repos/$repo/rulesets" \ | ||
| | jq -r --arg n "$name" '.[] | select(.name == $n) | .id') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Paginate the gh api call 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 --paginate automatically 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/rulesets/apply.sh around lines 46 - 47, Paginate both rulesets index
requests by adding the gh api pagination option to the calls in
.github/rulesets/apply.sh lines 46-47 and .github/workflows/verify-rulesets.yml
lines 37-38; no other logic changes are needed.
| permissions: | ||
| administration: read | ||
| contents: read | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the invalid administration permission scope.
The administration scope is not a valid permission for GITHUB_TOKEN in GitHub Actions. Its presence will cause a workflow syntax error, preventing the job from running entirely. The contents: read scope is generally sufficient to read rulesets.
🔨 Proposed fix
permissions:
- administration: read
contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| administration: read | |
| contents: read | |
| permissions: | |
| contents: read |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 25-25: unknown permission scope "administration". all available permission scopes are "actions", "artifact-metadata", "attestations", "checks", "contents", "deployments", "discussions", "id-token", "issues", "models", "packages", "pages", "pull-requests", "repository-projects", "security-events", "statuses"
(permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/verify-rulesets.yml around lines 24 - 27, Remove the
invalid administration entry from the workflow’s permissions block, leaving
contents: read as the required permission for the ruleset verification job.
Source: Linters/SAST tools
| "*", | ||
| "!fvm/**" | ||
| ], | ||
| "approvals_needed": 2 |
There was a problem hiding this comment.
Shouldn't this be 3 if we want 2 reviewers + AI, before we switch to 1 reviewer + AI
3 participants
Codifies the master-branch protection policy as a ruleset spec in the repo, adds a small CLI tool for admins to apply changes, and a workflow that shouts if the live ruleset ever drifts from what's in git.
Motivation is FVM review capacity — with a small pool of reviewers and a growing stack of FVM PRs, we'd like an AI reviewer to be able to count as one of the two required approvals on FVM-only changes, without letting bot approvals
substitute for humans anywhere else. GitHub rulesets have a
required_reviewersfeature that expresses exactly this: for the pattern["*", "!fvm/**"], the team@onflow/flow-core-protocol(what CODEOWNERS already says owns the repo)must supply 2 approvals; on FVM-only PRs there's no team requirement, so any two approvers satisfy the global count. Assumption baked in: exactly one AI reviewer account exists on the repo — if we ever add a second, we'll want a follow-up
entry to keep "2 bots, 0 humans" impossible.
The rest of the ruleset is a 1:1 mirror of what's currently on
mastervia legacy branch protection — same 48 required checks (strict mode), same code-owner review requirement, same "don't dismiss stale reviews", same admin-can-bypasssemantics via
RepositoryRoleid 5. Nothing about baseline enforcement is changing; the ruleset is just the modern container for it, with room for the path-scoped reviewer rule that classic branch protection doesn't support.The read-only pattern is deliberate: the workflow only ever calls the API with
administration: read, so there's no stored PAT and no write-capable service account. Policy changes flow through PRs (this file being reviewed like anyother), and an admin applies them locally with
apply.shusing their own credentials. If someone hand-edits the ruleset in the UI, the daily verify run and the next PR touching this directory both fail loudly, naming the drifted field.Trade-off vs full IaC: no auto-apply, admin has to run one command. Fine for something that changes a handful of times a year.
Rollout after merge: an admin runs
apply.sh .github/rulesets/default-branch-protection.jsonto create the live ruleset, confirmsverify-rulesetspasses, then removes the legacy branch protection onmaster(which the ruleset nowsupersedes). The
verify-rulesetscheck itself isn't in the required-checks list yet — worth adding as a follow-up once we've observed the exact check-run name GitHub records for it.Companion work — the actual AI reviewer install/config, and mirroring this to
flow-go-internal— will land separately.Summary by CodeRabbit
New Features
Documentation