Skip to content

Commit f92a2df

Browse files
committed
feat(ci): gate merges on maintainer approval via a silent status check
Add a required check that passes only when someone listed in MAINTAINERS.md has an approving review on the pull request. Unlike CODEOWNERS and ruleset required reviewers, a status check enforces who must approve without notifying anyone, and its approver list is a file, so outside collaborators can be listed. The job reports through its own exit code rather than a posted commit status, so it needs no write token and no head SHA. It runs on pull_request_review, and on merge_group because the queue waits for required contexts to report on the merge group ref regardless of the pull request result. MAINTAINERS.md and the decision helper are read from main, never the pull request ref, so a contributor cannot add themselves and self-approve. The helper fails closed when no handles parse. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
1 parent 853e529 commit f92a2df

7 files changed

Lines changed: 135 additions & 224 deletions

File tree

.github/workflows/core-approval.yml

Lines changed: 0 additions & 151 deletions
This file was deleted.
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
# SPDX-License-Identifier: Apache-2.0
3+
4+
name: Maintainer Approval
5+
6+
on:
7+
merge_group:
8+
types: [checks_requested]
9+
pull_request_review:
10+
types: [submitted, dismissed]
11+
12+
permissions:
13+
contents: read
14+
pull-requests: read
15+
16+
concurrency:
17+
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
18+
cancel-in-progress: true
19+
20+
jobs:
21+
maintainer-approval:
22+
# This name is the required status check context. Changing it silently
23+
# breaks the ruleset entry that gates merges on this job.
24+
name: OpenShell / Maintainer Approval
25+
if: github.repository_owner == 'NVIDIA'
26+
runs-on: ubuntu-latest
27+
timeout-minutes: 5
28+
steps:
29+
# Check out the default branch, never the pull request head. Both the
30+
# maintainer list and the decision helper must come from main: reading
31+
# either from the pull request ref would let a contributor add themselves
32+
# to the list, or rewrite the decision logic, and self-approve.
33+
- name: Check out the maintainer list and helper
34+
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
35+
with:
36+
ref: main
37+
sparse-checkout: |
38+
MAINTAINERS.md
39+
tasks/scripts/core_approval.py
40+
sparse-checkout-cone-mode: false
41+
persist-credentials: false
42+
43+
- name: Require an approving review from a maintainer
44+
env:
45+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
46+
GH_REPO: ${{ github.repository }}
47+
PR_NUMBER: ${{ github.event.pull_request.number }}
48+
# merge_group carries no pull request in its payload, but the queue
49+
# branch names the entry: gh-readonly-queue/main/pr-3027-<base sha>.
50+
MERGE_GROUP_REF: ${{ github.ref_name }}
51+
shell: bash
52+
run: |
53+
set -euo pipefail
54+
55+
if [ -z "${PR_NUMBER:-}" ]; then
56+
if [[ "$MERGE_GROUP_REF" =~ /pr-([0-9]+)-[0-9a-f]+$ ]]; then
57+
PR_NUMBER="${BASH_REMATCH[1]}"
58+
else
59+
# Fail closed: an unrecognised ref must never satisfy the gate.
60+
echo "::error::No pull request resolved from '$MERGE_GROUP_REF'."
61+
exit 1
62+
fi
63+
fi
64+
65+
gh api --paginate "repos/$GH_REPO/pulls/$PR_NUMBER/reviews" --jq '.[]' \
66+
| jq -s '.' > reviews.json
67+
68+
# Exits non-zero when no maintainer's latest decisive review is an
69+
# approval, and when MAINTAINERS.md yields no handles. That exit code
70+
# is the check result; nothing is posted anywhere.
71+
python3 tasks/scripts/core_approval.py decide \
72+
--maintainers MAINTAINERS.md \
73+
--reviews reviews.json

.github/workflows/maintainers-change-alert.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ jobs:
8686
# the body. If they drift, the lookup below silently stops matching
8787
# and every push stacks another comment.
8888
COMMENT_ID=$(gh api --paginate "repos/$GH_REPO/issues/$PR_NUMBER/comments" \
89-
--jq '.[] | select(.body | startswith("<!-- core-approval-maintainer-delta -->")) | .id' \
89+
--jq '.[] | select(.body | startswith("<!-- maintainer-approval-delta -->")) | .id' \
9090
| head -n 1)
9191
9292
if [ -n "$COMMENT_ID" ]; then

.github/zizmor.yml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ rules:
66
ignore:
77
# These base-branch workflows never check out or execute pull request
88
# head code. Keep each suppression scoped to its reviewed trigger block.
9-
- core-approval.yml:6
109
- dco.yml:3
1110
- e2e-label-help.yml:13
1211
- maintainers-change-alert.yml:6

CONTRIBUTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ Do not start substantial issue-backed work until a maintainer has accepted the i
6868

6969
Use agents and the repository skills as needed to understand the affected code, evaluate tradeoffs, implement the smallest coherent change, and verify it. The pull request should explain what changed and how it was tested; it should not substitute an agent transcript for the contributor's understanding.
7070

71-
Every pull request must be approved by someone listed in [MAINTAINERS.md](MAINTAINERS.md) before it can merge. This is enforced by the `OpenShell / Core Approval` status check, which turns green once one of those reviewers approves. Reviews from other contributors are welcome and count toward the general approval requirement, but they do not satisfy this check.
71+
Every pull request must be approved by someone listed in [MAINTAINERS.md](MAINTAINERS.md) before it can merge. This is enforced by the `OpenShell / Maintainer Approval` status check, which turns green once one of those reviewers approves. Reviews from other contributors are welcome and count toward the general approval requirement, but they do not satisfy this check.
7272

7373
Maintainers are not requested automatically. If your pull request has been idle, ask for a reviewer in the pull request or in the CNCF Slack channel rather than waiting.
7474

tasks/scripts/core_approval.py

Lines changed: 23 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@
1010
"""Decide whether a pull request carries an approval from a listed maintainer.
1111
1212
The logic here is pure so it can be unit tested. The calling workflow does the
13-
I/O: it fetches MAINTAINERS.md pinned to the default branch, lists the pull
14-
request's reviews, and passes both in as files.
13+
I/O: it checks out MAINTAINERS.md from the default branch, lists the pull
14+
request's reviews, and passes both in as files. `decide` exits non-zero when
15+
the gate is not satisfied, and that exit code is the check result.
1516
1617
Runs as bare `python3` on the Actions runner, so it must stay stdlib-only.
1718
"""
@@ -34,10 +35,7 @@
3435
# reviewer's earlier approval intact, which is how GitHub itself treats them.
3536
DECISIVE_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"})
3637

37-
# GitHub truncates commit status descriptions past this length.
38-
DESCRIPTION_LIMIT = 140
39-
40-
COMMENT_MARKER = "<!-- core-approval-maintainer-delta -->"
38+
COMMENT_MARKER = "<!-- maintainer-approval-delta -->"
4139

4240

4341
def parse_maintainers(markdown: str) -> set[str]:
@@ -62,34 +60,27 @@ def latest_positions(reviews: list[dict]) -> dict[str, str]:
6260
return positions
6361

6462

65-
def approving_maintainers(
66-
reviews: list[dict], maintainers: set[str], author: str
67-
) -> list[str]:
63+
def approving_maintainers(reviews: list[dict], maintainers: set[str]) -> list[str]:
6864
"""Return the listed maintainers whose standing position is an approval."""
69-
author = author.lower()
7065
return sorted(
7166
login
7267
for login, state in latest_positions(reviews).items()
73-
if state == "APPROVED" and login in maintainers and login != author
68+
if state == "APPROVED" and login in maintainers
7469
)
7570

7671

77-
def decide(markdown: str, reviews: list[dict], author: str) -> tuple[str, str]:
78-
"""Return the (state, description) to publish as a commit status."""
72+
def decide(markdown: str, reviews: list[dict]) -> tuple[bool, str]:
73+
"""Return whether the gate is satisfied, and a line explaining why."""
7974
maintainers = parse_maintainers(markdown)
8075
if not maintainers:
8176
# Fail closed. An unparseable or empty list must never satisfy the gate.
82-
return "failure", "Could not parse any maintainers from MAINTAINERS.md"
77+
return False, "Could not parse any maintainers from MAINTAINERS.md"
8378

84-
approvers = approving_maintainers(reviews, maintainers, author)
79+
approvers = approving_maintainers(reviews, maintainers)
8580
if not approvers:
86-
return "failure", "Needs approval from a maintainer listed in MAINTAINERS.md"
81+
return False, "Needs approval from a maintainer listed in MAINTAINERS.md"
8782

88-
shown = ", ".join(f"@{login}" for login in approvers[:3])
89-
remainder = len(approvers) - 3
90-
if remainder > 0:
91-
shown = f"{shown} and {remainder} more"
92-
return "success", f"Approved by {shown}"[:DESCRIPTION_LIMIT]
83+
return True, "Approved by " + ", ".join(f"@{login}" for login in approvers)
9384

9485

9586
def format_delta(before: str, after: str) -> str:
@@ -114,7 +105,7 @@ def format_delta(before: str, after: str) -> str:
114105
lines.append("")
115106
lines.append(
116107
"Confirm every change is intended. Anyone listed here can single-handedly "
117-
"satisfy `OpenShell / Core Approval`."
108+
"satisfy `OpenShell / Maintainer Approval`."
118109
)
119110

120111
if not new:
@@ -132,7 +123,7 @@ def build_parser() -> argparse.ArgumentParser:
132123
subcommands = parser.add_subparsers(dest="command", required=True)
133124

134125
decide_cmd = subcommands.add_parser(
135-
"decide", help="print the commit status to publish, as 'state<TAB>description'"
126+
"decide", help="exit 0 when a listed maintainer has approved, 1 otherwise"
136127
)
137128
decide_cmd.add_argument(
138129
"--maintainers",
@@ -146,9 +137,6 @@ def build_parser() -> argparse.ArgumentParser:
146137
type=Path,
147138
help="JSON array returned by the list-reviews API",
148139
)
149-
decide_cmd.add_argument(
150-
"--author", default="", help="pull request author, excluded from approvers"
151-
)
152140

153141
diff_cmd = subcommands.add_parser(
154142
"diff", help="print a review comment describing the approver set change"
@@ -167,17 +155,16 @@ def main(argv: list[str] | None = None) -> int:
167155

168156
if args.command == "decide":
169157
reviews = json.loads(args.reviews.read_text(encoding="utf-8"))
170-
state, description = decide(
171-
args.maintainers.read_text(encoding="utf-8"), reviews, args.author
172-
)
173-
print(f"{state}\t{description}")
174-
else:
175-
print(
176-
format_delta(
177-
args.before.read_text(encoding="utf-8"),
178-
args.after.read_text(encoding="utf-8"),
179-
)
158+
approved, reason = decide(args.maintainers.read_text(encoding="utf-8"), reviews)
159+
print(reason)
160+
return 0 if approved else 1
161+
162+
print(
163+
format_delta(
164+
args.before.read_text(encoding="utf-8"),
165+
args.after.read_text(encoding="utf-8"),
180166
)
167+
)
181168
return 0
182169

183170

0 commit comments

Comments
 (0)