feat(ci): gate merges on maintainer approval via a required status check by purp · Pull Request #3176 · NVIDIA/OpenShell

purp · 2026-09-04T02:28:42Z

Summary

Adds a OpenShell / Core Approval commit status that passes only when someone listed in MAINTAINERS.md has approved the pull request. Once registered on ruleset 13332227 it becomes the approval gate, which is what makes it safe to disable the CODEOWNERS wildcard in #3168.

The motivating problem is that every native GitHub mechanism enforcing who must approve also notifies them:

Mechanism	Enforces	Notifies	Supports outside collaborators
`CODEOWNERS`	yes	yes	yes
Ruleset required reviewers	yes	yes	no (teams only)
Required status check	yes	no	yes (the list is a file)

A required status check is the only primitive that gates a merge silently. That splits enforcement from notification: this PR is the enforcement half. Targeted notification (area labels, per-area rosters) follows separately.

A file-based approver list also solves the outside-collaborator problem — three of thirteen maintainers cannot join an NVIDIA org team, but can be listed in a file.

Related Issue

No issue required. This does not touch OpenShell platform code — it is maintenance of the project's own review machinery, and is non-breaking as merged. The gate only starts enforcing when the status context is added to ruleset 13332227, which is a separate manual step (see Rollout).

Changes

tasks/scripts/core_approval.py — stdlib-only helper. decide parses MAINTAINERS.md, folds the review list to each reviewer's latest decisive position, and prints state<TAB>description. diff renders the approver-set delta between two versions of the file.
tasks/scripts/core_approval_test.py — 17 tests. tasks/test.toml gains test:core-approval.
.github/workflows/core-approval.yml — publishes the status on pull_request_target, pull_request_review, merge_group, and workflow_dispatch.
.github/workflows/maintainers-change-alert.yml — comments the added/removed handles on PRs touching MAINTAINERS.md, so reviewers see the delta instead of diffing a markdown table.
.github/zizmor.yml — two dangerous-triggers suppressions for the new workflows.

Security invariants

Never check out or execute PR head code. pull_request_target runs with a write-capable token; executing contributor code under it is the standard escalation. The checkout is pinned to ref: main with a sparse checkout of only the helper script and persist-credentials: false.
MAINTAINERS.md is read from main, never the PR ref. Otherwise a contributor adds themselves in their own PR and self-approves.
Fail closed. An unparseable or empty list fails. No error path can publish success.

No ${{ }} interpolation appears inside any run: block; every context value routes through env:.

Why this trigger set

pull_request does not re-fire when someone approves, so the check would go stale red forever. pull_request_review fires but its jobs are not auto-surfaced as PR checks, so the status is POSTed explicitly. Fork PRs get a read-only token under pull_request and cannot POST at all; pull_request_target and pull_request_review both run in base-repo context.

merge_group publishes success unconditionally. A merge group only forms after the PR already satisfied this gate, and approvals cannot change while an entry sits in the queue. Without this, a required check that never reports on the merge-group ref stalls every entry for the full check_response_timeout_minutes (60).

Testing

mise run pre-commit passes
Unit tests added — 17 tests covering the valid table, a reformatted table, a zero-handle table (fails closed), self-approval, non-maintainer approval, dismissal and change-request semantics, out-of-order review IDs, and description truncation at 140 characters
E2E tests added/updated — not applicable

There is no pre-merge dry run, by construction. pull_request_target runs the base branch's workflow definition, so this workflow cannot be exercised from its own PR. if: github.repository_owner == 'NVIDIA' also keeps it from running on forks. Behavior is covered by unit tests here and verified live after merge, before the check is made required.

Reviewer notes

No $GITHUB_ENV writes. An earlier revision passed the head SHA between steps via $GITHUB_ENV, which trips zizmor's github-env rule under pull_request_target and fails the code-scanning check. The workflow now uses step outputs (steps.publish.outputs.posted / head_sha), which carry the same values without granting later steps an attacker-shaped environment. Note that the Zizmor High report job is informational, but the SARIF it uploads feeds a separate Advanced Security zizmor check that does fail on new alerts.
Residual gap: on workflow_dispatch, no SHA is knowable until the PR lookup runs, so a failure strictly before that point publishes nothing and logs a warning. A failure after the lookup is covered by the head_sha step output. Every other event resolves the SHA from the event payload.
Stale-dismissal race: the ruleset sets dismiss_stale_reviews_on_push: true. A push and a dismissal can interleave, but the check re-runs on synchronize, so it converges.
Admin bypass is unchanged. Admins can bypass this check exactly as they can bypass today's CODEOWNERS gate. Reducing the admin surface is deliberately out of scope.
johnnygreco holds admin via openshell-codeowners but is absent from MAINTAINERS.md. Deferred; revisit when the team list is reconciled.

Rollout

Order matters — reversing steps 1–3 and 4 blocks every open PR.

Merge this PR.
Backfill open PRs via workflow_dispatch (pr_number is required; there is no sweep mode). PRs opened before the workflow existed otherwise sit at "Expected" forever once the check is required.
Verify live: a fork PR posts a status; a non-maintainer approval stays red; a maintainer approval turns it green with no new push; a push returns it red; a PR adding a handle to MAINTAINERS.md cannot self-approve.
Add context OpenShell / Core Approval to ruleset 13332227.
Merge chore(codeowners): disable wildcard ownership rule #3168.

Checklist

Follows Conventional Commits
Commits are signed off (DCO)
Architecture docs updated (if applicable) — not applicable, no platform code changed

copy-pr-bot · 2026-09-04T02:28:45Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

A checkout or API failure on a merge_group run left STATUS_SHA empty, so the guard skipped and the required check sat at Expected until the queue timed out. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

Writing to $GITHUB_ENV under pull_request_target trips zizmor's github-env rule, which fails the code-scanning check. Step outputs carry the same values without granting later steps an attacker-shaped environment, and the resolved head SHA now also covers a workflow_dispatch run that fails after the pull request lookup. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

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>

purp

Refactor per comments.

purp · 2026-09-08T23:35:32Z

+
+"""Decide whether a pull request carries an approval from a listed maintainer.
+
+The logic here is pure so it can be unit tested. The calling workflow does the


Lose the comment about purity of logic. Comments say what we do and why, concisely, and only when reading the code isn't explicit enough.

purp · 2026-09-08T23:46:16Z

The maintainers change alert should be a separate, simple tool named alert_maintainer_change.py

This should be named check_maintainer_approval.py

Neither needs a verb argument like detect or diff

purp · 2026-09-09T00:28:49Z

+| Name | GitHub ID | Company/Organization |
+| --- | --- | --- |
+| Derek Carr | [@derekwaynecarr](https://github.com/derekwaynecarr) | Red Hat |
+| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA |


Let's recognize someone who's contributed more to the project. Use Piotr and Evan here.

purp · 2026-09-09T00:31:44Z

+    assert "drive-by" not in ca.parse_maintainers(prose)
+
+
+def test_parse_maintainers_returns_empty_when_table_is_reformatted() -> None:


This is the same as an unparseable list. Drop this test.

purp · 2026-09-09T00:54:27Z

+    )
+
+
+def decide(markdown: str, reviews: list[dict]) -> tuple[bool, str]:


This should just move to main now that the tool has only one purpose.

purp · 2026-09-09T01:02:41Z

+    return {match.group(1).lower() for match in MAINTAINER_RE.finditer(markdown)}
+
+
+def latest_positions(reviews: list[dict]) -> dict[str, str]:


Rework to be parse_reviews_to_approvers. Read reviews.json, resolve each reviewer's current state, then filter to only those that are currently "APPROVED". Returns the list of logins who currently have approved reviews.

purp · 2026-09-09T01:04:45Z

+    return positions
+
+
+def approving_maintainers(reviews: list[dict], maintainers: set[str]) -> list[str]:


Drop this. Not needed if parse_reviews_to_approvers returns the list of approving logins

Each CI tool now does one thing: check_maintainer_approval.py decides the gate, alert_maintainer_change.py renders the approver-set delta. Neither takes a verb argument. The login pattern is duplicated so each tool stands alone; a test asserts the two patterns stay identical. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

purp

Please address all comments.

purp · 2026-09-09T05:03:39Z


 jobs:
  describe-change:
    name: Comment on the approver set change


"... if MAINTAINERS.md has changed."

purp · 2026-09-09T05:04:07Z

 jobs:
  describe-change:
    name: Comment on the approver set change
    if: github.repository_owner == 'NVIDIA'


Why is this conditional needed?

purp · 2026-09-09T05:05:29Z

-          # tasks/scripts/core_approval.py, which emits it as the first line of
-          # the body. If they drift, the lookup below silently stops matching
-          # and every push stacks another comment.
+          # tasks/scripts/alert_maintainer_change.py, which emits it as the


Can we just set it as an environment variable that we use here and the alert script gets for free? Less maintenance and coupling is a goodness.

purp · 2026-09-09T05:07:52Z

+# 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."

purp · 2026-09-09T05:10:40Z

+
+"""Print a review comment describing how a pull request changes the approver set.
+
+The calling workflow does the I/O: it extracts MAINTAINERS.md at the base and


Unnecessary comment. We can read this in the code. Comments say what we do and why concisely and only when the code doesn't speak for itself.

purp · 2026-09-09T05:12:48Z

+    added, removed = sorted(new - old), sorted(old - new)
+
+    lines = [COMMENT_MARKER, "## Maintainer list change", ""]
+    if not added and not removed:


In this case we should not comment. It makes no value and unneeded noise is distracting.

purp · 2026-09-09T05:14:57Z

+            "satisfy `OpenShell / Maintainer Approval`."
+        )
+
+    if not new:


This condition should cause the check to fail.

purp · 2026-09-09T05:27:27Z

+def test_names_added_and_removed_logins() -> None:
+    after = TABLE.replace(
+        "| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA |\n",
+        "| New Person | [@newbie](https://github.com/newbie) | NVIDIA |\n",


Pick a real contributor like Mrunal Patel

purp · 2026-09-09T05:28:19Z

+    assert "does not change" in body
+
+
+def test_warns_when_the_result_parses_empty() -> None:


Check should fail in this condition

purp · 2026-09-09T05:30:02Z

+
+"""Exit non-zero unless a maintainer listed in MAINTAINERS.md has approved.
+
+The calling workflow does the I/O: it checks out MAINTAINERS.md from the


Again, not needed. Comments say what we do and why concisely and only when the code doesn't speak for itself.

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

Comment thread .github/workflows/core-approval.yml Fixed

Comment thread .github/workflows/core-approval.yml Fixed

purp added 10 commits September 8, 2026 16:31

feat(ci): add maintainer approval decision helper

af1a51d

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

fix(ci): order maintainer reviews by review id

2fb533c

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

feat(ci): publish the core approval status check

635815f

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

fix(ci): publish a red core approval status on abort

587a57c

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

feat(ci): comment the approver set delta on MAINTAINERS.md changes

5c67728

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

fix(ci): fail loudly when the MAINTAINERS.md fetch errors

b351c74

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

fix(ci): let the approval guard fire on early failures

467a813

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

fix(ci): seed the merge group SHA for the approval guard

dad00ec

A checkout or API failure on a merge_group run left STATUS_SHA empty, so the guard skipped and the required check sat at Expected until the queue timed out. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

purp force-pushed the feat/core-approval-gate/purp branch from b6102b0 to f92a2df Compare September 8, 2026 23:32

purp commented Sep 9, 2026

View reviewed changes


		"""Decide whether a pull request carries an approval from a listed maintainer.

		The logic here is pure so it can be unit tested. The calling workflow does the

		assert "drive-by" not in ca.parse_maintainers(prose)


		def test_parse_maintainers_returns_empty_when_table_is_reformatted() -> None:

		)


		def decide(markdown: str, reviews: list[dict]) -> tuple[bool, str]:

		return {match.group(1).lower() for match in MAINTAINER_RE.finditer(markdown)}


		def latest_positions(reviews: list[dict]) -> dict[str, str]:

		return positions


		def approving_maintainers(reviews: list[dict], maintainers: set[str]) -> list[str]:


		"""Print a review comment describing how a pull request changes the approver set.

		The calling workflow does the I/O: it extracts MAINTAINERS.md at the base and

		assert "does not change" in body


		def test_warns_when_the_result_parses_empty() -> None:


		"""Exit non-zero unless a maintainer listed in MAINTAINERS.md has approved.

		The calling workflow does the I/O: it checks out MAINTAINERS.md from the

Uh oh!

Conversation

purp commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Security invariants

Why this trigger set

Testing

Reviewer notes

Rollout

Checklist

Uh oh!

copy-pr-bot Bot commented Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

purp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

purp commented Sep 4, 2026 •

edited

Loading