-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(ci): gate merges on maintainer approval via a required status check #3176
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
purp
wants to merge
12
commits into
NVIDIA:main
Choose a base branch
from
purp:feat/core-approval-gate/purp
base: main
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.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
af1a51d
feat(ci): add maintainer approval decision helper
purp 2fb533c
fix(ci): order maintainer reviews by review id
purp 635815f
feat(ci): publish the core approval status check
purp 587a57c
fix(ci): publish a red core approval status on abort
purp 5c67728
feat(ci): comment the approver set delta on MAINTAINERS.md changes
purp b351c74
fix(ci): fail loudly when the MAINTAINERS.md fetch errors
purp 467a813
fix(ci): let the approval guard fire on early failures
purp dad00ec
fix(ci): seed the merge group SHA for the approval guard
purp 853e529
fix(ci): pass the approval status SHA as a step output
purp f92a2df
feat(ci): gate merges on maintainer approval via a silent status check
purp 11c7836
refactor(ci): split the maintainer approval helper into two tools
purp 3ea8340
refactor(ci): stay silent when the approver set is unchanged
purp 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
Some comments aren't visible on the classic Files Changed page.
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,73 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: Maintainer Approval | ||
|
|
||
| on: | ||
| merge_group: | ||
| types: [checks_requested] | ||
| pull_request_review: | ||
| types: [submitted, dismissed] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| maintainer-approval: | ||
| # This name is the required status check context. Changing it silently | ||
| # breaks the ruleset entry that gates merges on this job. | ||
| name: OpenShell / Maintainer Approval | ||
| if: github.repository_owner == 'NVIDIA' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| # Check out the default branch, never the pull request head. Both the | ||
| # maintainer list and the decision helper must come from main: reading | ||
| # either from the pull request ref would let a contributor add themselves | ||
| # to the list, or rewrite the decision logic, and self-approve. | ||
| - name: Check out the maintainer list and helper | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| ref: main | ||
| sparse-checkout: | | ||
| MAINTAINERS.md | ||
| tasks/scripts/check_maintainer_approval.py | ||
| sparse-checkout-cone-mode: false | ||
| persist-credentials: false | ||
|
|
||
| - name: Require an approving review from a maintainer | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GH_REPO: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| # merge_group carries no pull request in its payload, but the queue | ||
| # branch names the entry: gh-readonly-queue/main/pr-3027-<base sha>. | ||
| MERGE_GROUP_REF: ${{ github.ref_name }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if [ -z "${PR_NUMBER:-}" ]; then | ||
| if [[ "$MERGE_GROUP_REF" =~ /pr-([0-9]+)-[0-9a-f]+$ ]]; then | ||
| PR_NUMBER="${BASH_REMATCH[1]}" | ||
| else | ||
| # Fail closed: an unrecognised ref must never satisfy the gate. | ||
| echo "::error::No pull request resolved from '$MERGE_GROUP_REF'." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| gh api --paginate "repos/$GH_REPO/pulls/$PR_NUMBER/reviews" --jq '.[]' \ | ||
| | jq -s '.' > reviews.json | ||
|
|
||
| # Exits non-zero when no maintainer's latest decisive review is an | ||
| # approval, and when MAINTAINERS.md yields no handles. That exit code | ||
| # is the check result; nothing is posted anywhere. | ||
| python3 tasks/scripts/check_maintainer_approval.py \ | ||
| --maintainers MAINTAINERS.md \ | ||
| --reviews reviews.json | ||
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,107 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: Maintainers Change Alert | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize] | ||
| paths: | ||
| - MAINTAINERS.md | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| describe-change: | ||
| name: Comment on the approver set change if MAINTAINERS.md has changed | ||
| if: github.repository_owner == 'NVIDIA' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| # Default branch only. The helper must be the reviewed version, not | ||
| # whatever the pull request happens to contain. | ||
| - name: Check out the change-alert helper | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| ref: main | ||
| sparse-checkout: tasks/scripts/alert_maintainer_change.py | ||
| sparse-checkout-cone-mode: false | ||
| persist-credentials: false | ||
|
|
||
| - name: Post the maintainer delta | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GH_REPO: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| # Single source of truth for the comment marker: the script emits | ||
| # it as the first line of the body, the lookup below matches on it. | ||
| COMMENT_MARKER: "<!-- maintainer-approval-delta -->" | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Fetching file contents is reading data, not executing it. The head | ||
| # revision is never checked out or run. | ||
| # | ||
| # A 404 means the file genuinely does not exist at that revision — a | ||
| # pull request that adds or deletes MAINTAINERS.md — and yields an | ||
| # empty side of the comparison. Every other failure is fatal: an empty | ||
| # file from a rate limit or a 5xx would render as "every maintainer | ||
| # was just added" or "nothing changed", both of which mislead the | ||
| # reviewer about who can merge code. | ||
| fetch_maintainers() { | ||
| local ref="$1" out="$2" err | ||
| err="$(mktemp)" | ||
| if gh api -H "Accept: application/vnd.github.raw" \ | ||
| "repos/$GH_REPO/contents/MAINTAINERS.md?ref=$ref" > "$out" 2>"$err"; then | ||
| rm -f "$err" | ||
| return 0 | ||
| fi | ||
| if grep -q 'HTTP 404' "$err"; then | ||
| rm -f "$err" | ||
| : > "$out" | ||
| return 0 | ||
| fi | ||
| echo "::error::Could not fetch MAINTAINERS.md at $ref" | ||
| cat "$err" >&2 | ||
| rm -f "$err" | ||
| return 1 | ||
| } | ||
|
|
||
| fetch_maintainers "$BASE_SHA" before.md | ||
| fetch_maintainers "$HEAD_SHA" after.md | ||
|
|
||
| # An unparseable result exits non-zero, but the comment explaining | ||
| # why still has to be posted before this job fails. | ||
| status=0 | ||
| python3 tasks/scripts/alert_maintainer_change.py \ | ||
| --before before.md --after after.md > body.md || status=$? | ||
|
|
||
| # No output means the approver set did not change. A comment saying | ||
| # so is noise, so post nothing. | ||
| if [ -s body.md ]; then | ||
| cat body.md >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| # Update the existing comment rather than stacking one per push. | ||
| COMMENT_ID=$(gh api --paginate "repos/$GH_REPO/issues/$PR_NUMBER/comments" \ | ||
| --jq '.[] | select(.body | startswith($ENV.COMMENT_MARKER)) | .id' \ | ||
| | head -n 1) | ||
|
|
||
| if [ -n "$COMMENT_ID" ]; then | ||
| gh api --method PATCH "repos/$GH_REPO/issues/comments/$COMMENT_ID" \ | ||
| -F "body=@body.md" >/dev/null | ||
| else | ||
| gh api --method POST "repos/$GH_REPO/issues/$PR_NUMBER/comments" \ | ||
| -F "body=@body.md" >/dev/null | ||
| fi | ||
| fi | ||
|
|
||
| exit "$status" |
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
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
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,102 @@ | ||
| #!/usr/bin/env python3 | ||
| # /// script | ||
| # requires-python = ">=3.11" | ||
| # dependencies = [] | ||
| # /// | ||
|
|
||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Print a review comment describing how a pull request changes the approver set | ||
| to make sure that reviewers notice the change. | ||
|
|
||
| Prints nothing when the approver set is unchanged, and exits non-zero when the | ||
| updated file yields no maintainers at all. | ||
|
|
||
| Runs as bare `python3` on the Actions runner, so it must stay stdlib-only. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import os | ||
| import re | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| # Only a login that appears as a link to a GitHub profile counts. A bare | ||
| # "[@someone]" in prose must never widen the approver set. Keep this in step | ||
| # with tasks/scripts/check_maintainer_approval.py, the gate this reports on. | ||
| MAINTAINER_RE = re.compile( | ||
| r"\[@([A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)\]\(https://github\.com/" | ||
| ) | ||
|
|
||
|
|
||
| def parse_maintainers(markdown: str) -> set[str]: | ||
| """Return the lowercased GitHub logins listed in a MAINTAINERS.md table.""" | ||
| return {match.group(1).lower() for match in MAINTAINER_RE.finditer(markdown)} | ||
|
|
||
|
|
||
| def format_delta(marker: str, before: str, after: str) -> str: | ||
| """Render a review comment, or an empty string when nothing changed.""" | ||
| old, new = parse_maintainers(before), parse_maintainers(after) | ||
| added, removed = sorted(new - old), sorted(old - new) | ||
| if not added and not removed: | ||
|
purp marked this conversation as resolved.
|
||
| return "" | ||
|
|
||
| lines = [marker, "## Maintainer list change", ""] | ||
| if added: | ||
| lines += ["**Gains approval rights:**", ""] | ||
| lines += [f"- @{login}" for login in added] | ||
| lines.append("") | ||
| if removed: | ||
| lines += ["**Loses approval rights:**", ""] | ||
| lines += [f"- @{login}" for login in removed] | ||
| lines.append("") | ||
| lines.append( | ||
| "Confirm every change is intended. Anyone listed here can single-handedly " | ||
| "satisfy `OpenShell / Maintainer Approval`." | ||
| ) | ||
|
|
||
| if not new: | ||
|
purp marked this conversation as resolved.
|
||
| lines += [ | ||
| "", | ||
| "> [!WARNING]", | ||
| "> No logins parse from the updated file. Merging this would make the " | ||
| "approval gate fail closed on every pull request.", | ||
| ] | ||
| return "\n".join(lines) | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument( | ||
| "--before", required=True, type=Path, help="MAINTAINERS.md at the base commit" | ||
| ) | ||
| parser.add_argument( | ||
| "--after", required=True, type=Path, help="MAINTAINERS.md at the head commit" | ||
| ) | ||
| args = parser.parse_args(argv) | ||
|
|
||
| marker = os.environ.get("COMMENT_MARKER") | ||
| if not marker: | ||
| print("COMMENT_MARKER is not set", file=sys.stderr) | ||
| return 1 | ||
|
|
||
| after = args.after.read_text(encoding="utf-8") | ||
| body = format_delta(marker, args.before.read_text(encoding="utf-8"), after) | ||
| if body: | ||
| print(body) | ||
|
|
||
| if not parse_maintainers(after): | ||
| print( | ||
| "No logins parse from the updated MAINTAINERS.md; the approval gate " | ||
| "would fail closed on every pull request.", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) | ||
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,87 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Tests for tasks/scripts/alert_maintainer_change.py. | ||
|
|
||
| Run via `mise run test:maintainer-approval`, which provides pytest through | ||
| `uv run --with pytest`. pytest puts this file's directory on sys.path, so the | ||
| sibling script imports directly. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import alert_maintainer_change as alert | ||
|
|
||
| MARKER = "<!-- maintainer-approval-delta -->" | ||
|
|
||
| TABLE = """# Maintainers | ||
|
|
||
| | Name | GitHub ID | Company/Organization | | ||
| | --- | --- | --- | | ||
| | Derek Carr | [@derekwaynecarr](https://github.com/derekwaynecarr) | Red Hat | | ||
| | Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | | ||
| | Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | | ||
| """ | ||
|
|
||
|
|
||
| def run(tmp_path, monkeypatch, before: str, after: str, marker: str = MARKER): | ||
| """Return the tool's exit code and the comment body it printed.""" | ||
| monkeypatch.setenv("COMMENT_MARKER", marker) | ||
| before_file = tmp_path / "before.md" | ||
| before_file.write_text(before, encoding="utf-8") | ||
| after_file = tmp_path / "after.md" | ||
| after_file.write_text(after, encoding="utf-8") | ||
| return alert.main(["--before", str(before_file), "--after", str(after_file)]) | ||
|
|
||
|
|
||
| def test_parse_maintainers_extracts_linked_logins() -> None: | ||
| assert alert.parse_maintainers(TABLE) == {"derekwaynecarr", "elezar", "pimlock"} | ||
|
|
||
|
|
||
| def test_names_added_and_removed_logins() -> None: | ||
| after = TABLE.replace( | ||
| "| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA |\n", | ||
| "| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat |\n", | ||
| ) | ||
| body = alert.format_delta(MARKER, TABLE, after) | ||
| assert "@mrunalp" in body | ||
| assert "@pimlock" in body | ||
|
|
||
|
|
||
| def test_says_nothing_when_only_prose_moves() -> None: | ||
| # An unchanged approver set is not worth a comment. | ||
| assert alert.format_delta(MARKER, TABLE, TABLE + "\nSee CONTRIBUTING.md.\n") == "" | ||
|
|
||
|
|
||
| def test_prints_nothing_when_the_approver_set_is_unchanged( | ||
| tmp_path, monkeypatch, capsys | ||
| ) -> None: | ||
| assert run(tmp_path, monkeypatch, TABLE, TABLE) == 0 | ||
| assert capsys.readouterr().out == "" | ||
|
|
||
|
|
||
| def test_fails_when_the_result_parses_empty(tmp_path, monkeypatch, capsys) -> None: | ||
| # Merging this would make the approval gate fail closed on every PR. | ||
| assert run(tmp_path, monkeypatch, TABLE, "# Maintainers\n\n- pimlock\n") != 0 | ||
| assert "WARNING" in capsys.readouterr().out | ||
|
|
||
|
|
||
| def test_fails_when_the_marker_is_unset(tmp_path, monkeypatch) -> None: | ||
| assert run(tmp_path, monkeypatch, TABLE, TABLE, marker="") != 0 | ||
|
|
||
|
|
||
| def test_body_starts_with_the_marker_the_workflow_supplies( | ||
| tmp_path, monkeypatch, capsys | ||
| ) -> None: | ||
| # The workflow finds its earlier comment with this prefix. | ||
| after = TABLE + "| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA |\n" | ||
| assert run(tmp_path, monkeypatch, TABLE, after) == 0 | ||
| assert capsys.readouterr().out.startswith(MARKER) | ||
|
|
||
|
|
||
| def test_login_pattern_matches_the_gate() -> None: | ||
| # Each tool parses MAINTAINERS.md on its own. If the patterns drift, this | ||
| # alert reports a delta that differs from what the gate enforces. | ||
| import check_maintainer_approval as gate | ||
|
|
||
| assert alert.MAINTAINER_RE.pattern == gate.MAINTAINER_RE.pattern |
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.
With this trigger, it's possible to overwrite the contents of the job in the PR, right?
E.g. if I change it to
run: true, it would run that and report the check as successful, so I think it would be possible to create a PR, override this check, approve by anybody and merge?The merge action is limited to anyone with write permissions though, so maybe this is fine.
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.
Actually, it looks like there maybe be a way to close this gap - we could add all the workflows to require codeowners approval, this way these files would be enforced by github (and consequently would ping everyone, but probably this is fine, as these changes are not as often?).
Something like
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.
Here's a visualization on how it would fit in the process.
stateDiagram-v2 [*] --> AwaitingCheck: PR opened AwaitingCheck --> Evaluating: Review submitted or dismissed Evaluating --> Blocked: No current maintainer approval Evaluating --> Blocked: Lookup or parsing fails Evaluating --> Approved: Current maintainer approval exists Blocked --> Evaluating: Review submitted or dismissed Approved --> Evaluating: Review submitted or dismissed Approved --> AwaitingCheck: New commit pushed Blocked --> AwaitingCheck: New commit pushed Approved --> AwaitingOtherRequirements AwaitingOtherRequirements --> Evaluating: Review submitted or dismissed AwaitingOtherRequirements --> AwaitingCheck: New commit pushed AwaitingOtherRequirements --> MergeQueue: All requirements pass and authorized user queues PR MergeQueue --> QueueEvaluation: merge_group event QueueEvaluation --> QueueBlocked: Approval check or other required checks fail QueueEvaluation --> Merged: Approval check and all other requirements pass Merged --> [*] note right of Evaluating Read maintainer list and helper from main. Fetch current reviews from GitHub. end note note right of AwaitingOtherRequirements Proposed scoped CODEOWNERS protection: changes to enforcement files require native codeowner approval. Ordinary PRs do not request codeowners. end note note right of AwaitingCheck This PR does not trigger the check on opening or pushing alone. A review event starts evaluation. end note