Skip to content

fix(governance): restrict direct pushes to trade/maintainers team#86

Merged
iap merged 2 commits into
devfrom
fix/governance-push-restrictions
May 10, 2026
Merged

fix(governance): restrict direct pushes to trade/maintainers team#86
iap merged 2 commits into
devfrom
fix/governance-push-restrictions

Conversation

@iap
Copy link
Copy Markdown
Contributor

@iap iap commented May 10, 2026

Restricts direct pushes on dev, canary, and main to the trade/maintainers team. Any future collaborator with write access must go through a PR with CI gates.

Changes:

  • apply-governance.sh: hardcode maintainers team push restriction on all branches, remove unused csv_to_json_array and build_restrictions_json functions
  • verify-governance.sh: add push restriction team slug check

Governance applied and verified passing locally.

Scope: governance

Verification: bash scripts/github/verify-governance.sh passes with all PASS.

Risk: Low. No effect on current sole maintainer. Closes direct-push hole for future collaborators.

Summary by CodeRabbit

  • Chores
    • Enforced standardized branch protection on main, canary, and dev to restrict direct pushes to the maintainers team only, replacing the prior per-branch allowlist approach.
    • Strengthened governance checks to verify that protected branches consistently include and enforce maintainer-only push restrictions.

Review Change Stack

All three branches now require pushes to come from the trade/maintainers
team. Removes the unused csv_to_json_array and build_restrictions_json
functions. verify-governance.sh now checks push restriction team slug.
@iap iap requested a review from a team as a code owner May 10, 2026 21:43
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 10, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9343914b-14cc-47d6-b9fd-e2c6e573ddf3

📥 Commits

Reviewing files that changed from the base of the PR and between a1b8944 and b78cf7e.

📒 Files selected for processing (1)
  • contracts/foundry.toml

Walkthrough

Governance scripts now hardcode a fixed MAINTAINERS_RESTRICTIONS_JSON restricting direct pushes to the trade/maintainers team for main, canary, and dev; CSV-to-JSON helper logic was removed; verification now checks branch protections include the maintainers team. A trailing blank line was added to contracts/foundry.toml.

Changes

Governance Configuration Hardening

Layer / File(s) Summary
Specification
scripts/github/apply-governance.sh
Environment variable docs removed per-branch *_PUSH_ALLOW_* mentions; MAINTAINERS_RESTRICTIONS_JSON constant added.
Helper Function Removal
scripts/github/apply-governance.sh
Deleted csv_to_json_array and build_restrictions_json helper functions used for dynamic restriction construction.
Branch Protection Application
scripts/github/apply-governance.sh
main, canary, and dev branch protection calls updated to pass the fixed MAINTAINERS_RESTRICTIONS_JSON for restrictions.
Verification Logic
scripts/github/verify-governance.sh
check_branch now extracts .restrictions.teams[].slug and enforces the maintainers team slug is present.
Nonfunctional
contracts/foundry.toml
Added an extra blank line (no functional change).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • trade/mark#45: Modifies the same governance scripts (apply-governance.sh and verify-governance.sh).
  • trade/mark#33: Also alters branch-protection verification logic in verify-governance.sh.
  • trade/mark#19: Prior governance tooling changes touching apply-governance.sh and verify-governance.sh.

Suggested labels

codex

Poem

🐰 I hop through scripts with glee,
I tuck the allowlists in a tree,
One maintainer team now stands,
Guarding three branches with steady hands,
A tidy change — cheers from me!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description covers key sections (Summary, Scope, Verification, Risk, Governance) but missing comprehensive details for several template sections. Complete the checklist items in all sections, particularly mark the governance items and verify all required checks (forge build/test, slither scan, CODEOWNER review, runbook updates) are actually addressed.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: restricting direct pushes to the trade/maintainers team across multiple branches.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/governance-push-restrictions

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@scripts/github/verify-governance.sh`:
- Around line 116-121: The current check only inspects the first team slug via
push_team and can miss extra allowed principals; update the verification to (1)
collect all team slugs (e.g. via jq '.restrictions.teams[]?.slug') and assert
that the only team present is "maintainers" (no other team slugs), and (2)
ensure that .restrictions.users and .restrictions.apps are empty; replace the
single push_team/head check with jq-based checks that test membership and counts
(or compare arrays) and return failure if teams contain anything besides
"maintainers" or if users/apps arrays are non-empty.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2911f81e-12d5-497e-beae-529ccc410d8f

📥 Commits

Reviewing files that changed from the base of the PR and between 7d7569c and a1b8944.

📒 Files selected for processing (2)
  • scripts/github/apply-governance.sh
  • scripts/github/verify-governance.sh

Comment thread scripts/github/verify-governance.sh
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1b8944316

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/github/verify-governance.sh
@iap iap merged commit cb479f3 into dev May 10, 2026
21 checks passed
@iap iap deleted the fix/governance-push-restrictions branch May 10, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant