MetaMask / MetaMask/metamask-extension

[P1, hard] Adopt `codeowners-plus` GitHub Action for better CO reviews - smart dismissals, enforcement of complete reviews

Open
#39,740 0 comments 0 reactions 1 assignee Claimed by @HowardBraham View on GitHub
for-migration INVALID-ISSUE-TEMPLATE team-extension-platform type-process
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

## Problem

PR authors can request codeowner (CO) reviews without having any "complete review". If multiple codeowners approve only their related changes, the PR can get merged without a proper complete review of the entire changeset.

### Example Scenarios

1. A PR touches files owned by Team A and Team B
2. Team A's codeowner approves only the files they own
3. Team B's codeowner approves only the files they own
4. PR merges without anyone reviewing the complete PR holistically

### Edge Case

There's a legitimate scenario where a team reviews a complete PR that is also CO-owned by that same team. Any enforcement mechanism needs to account for this.

## Proposed Solution

Adopt [codeowners-plus](https://github.com/multimediallc/codeowners-plus) GitHub Action, which provides several features that address this gap:

| Feature | Description | How It Helps |
|---------|-------------|---------------|
| **AND codeowner rules** | Require approval from multiple specified owners | Ensures comprehensive review coverage |
| **`unskippable_reviewers`** | Specify reviewers that cannot be skipped via `max_reviews` setting | Guarantees certain reviewers always review |
| **`require_both_branch_reviewers`** | Requires approval from codeowners defined in BOTH base branch AND PR branch | Prevents bypassing via branch-specific rules |
| **Smart dismissal** | Only dismiss approval when owned files change | Reduces re-review churn for unrelated changes |

## Context

- GitHub has an upstream feature request for native AND rules but no timeline: [community/discussions/166110](https://github.com/orgs/community/discussions/166110)
- Previous approach ([code-captains](https://github.com/upshift-dev/code-captains)) appears abandoned
- As the codebase and teams grow, this becomes increasingly important

## Tasks

- [ ] Evaluate `codeowners-plus` action in a test repository
- [ ] Define AND rules for critical paths (e.g., security-sensitive code)
- [ ] Determine `unskippable_reviewers` list for complete review enforcement
- [ ] Draft updated CODEOWNERS configuration
- [ ] Test edge case: team reviewing complete PR that they also CO-own
- [ ] Roll out to metamask-extension repository
- [ ] Document new review workflow for contributors

## References

- [codeowners-plus](https://github.com/multimediallc/codeowners-plus) - 33 stars, actively maintained (Go)
- [GitHub AND rules discussion](https://github.com/orgs/community/discussions/166110)
- [code-captains](https://github.com/upshift-dev/code-captains) - appears abandoned (TypeScript)

## Open Questions

1. How do we handle the case where a team legitimately reviews a complete PR they also CO-own?
2. Should we phase this in gradually (starting with critical paths) or adopt comprehensively?
3. What's the impact on PR velocity and reviewer load?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.