feat(ci): gate merges on maintainer approval via a required status check - #3176
Draft
purp wants to merge 9 commits into
Draft
feat(ci): gate merges on maintainer approval via a required status check#3176purp wants to merge 9 commits into
purp wants to merge 9 commits into
Conversation
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
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>
|
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. |
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>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds a
OpenShell / Core Approvalcommit status that passes only when someone listed inMAINTAINERS.mdhas approved the pull request. Once registered on ruleset13332227it becomes the approval gate, which is what makes it safe to disable theCODEOWNERSwildcard in #3168.The motivating problem is that every native GitHub mechanism enforcing who must approve also notifies them:
CODEOWNERSA 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.decideparsesMAINTAINERS.md, folds the review list to each reviewer's latest decisive position, and printsstate<TAB>description.diffrenders the approver-set delta between two versions of the file.tasks/scripts/core_approval_test.py— 17 tests.tasks/test.tomlgainstest:core-approval..github/workflows/core-approval.yml— publishes the status onpull_request_target,pull_request_review,merge_group, andworkflow_dispatch..github/workflows/maintainers-change-alert.yml— comments the added/removed handles on PRs touchingMAINTAINERS.md, so reviewers see the delta instead of diffing a markdown table..github/zizmor.yml— twodangerous-triggerssuppressions for the new workflows.Security invariants
pull_request_targetruns with a write-capable token; executing contributor code under it is the standard escalation. The checkout is pinned toref: mainwith a sparse checkout of only the helper script andpersist-credentials: false.MAINTAINERS.mdis read frommain, never the PR ref. Otherwise a contributor adds themselves in their own PR and self-approves.success.No
${{ }}interpolation appears inside anyrun:block; every context value routes throughenv:.Why this trigger set
pull_requestdoes not re-fire when someone approves, so the check would go stale red forever.pull_request_reviewfires but its jobs are not auto-surfaced as PR checks, so the status is POSTed explicitly. Fork PRs get a read-only token underpull_requestand cannot POST at all;pull_request_targetandpull_request_reviewboth run in base-repo context.merge_grouppublishes 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 fullcheck_response_timeout_minutes(60).Testing
mise run pre-commitpassesThere is no pre-merge dry run, by construction.
pull_request_targetruns 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
$GITHUB_ENVwrites. An earlier revision passed the head SHA between steps via$GITHUB_ENV, which trips zizmor'sgithub-envrule underpull_request_targetand 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 theZizmor High reportjob is informational, but the SARIF it uploads feeds a separate Advanced Securityzizmorcheck that does fail on new alerts.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 thehead_shastep output. Every other event resolves the SHA from the event payload.dismiss_stale_reviews_on_push: true. A push and a dismissal can interleave, but the check re-runs onsynchronize, so it converges.CODEOWNERSgate. Reducing the admin surface is deliberately out of scope.johnnygrecoholds admin viaopenshell-codeownersbut is absent fromMAINTAINERS.md. Deferred; revisit when the team list is reconciled.Rollout
Order matters — reversing steps 1–3 and 4 blocks every open PR.
workflow_dispatch(pr_numberis required; there is no sweep mode). PRs opened before the workflow existed otherwise sit at "Expected" forever once the check is required.MAINTAINERS.mdcannot self-approve.OpenShell / Core Approvalto ruleset13332227.Checklist