Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions .github/workflows/core-maintainer.yml
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:

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Collaborator

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

/.github/ @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/MAINTAINERS.md @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/tasks/scripts/*maintainer*.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr

Copy link
Copy Markdown
Collaborator

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
Loading

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
107 changes: 107 additions & 0 deletions .github/workflows/maintainers-change-alert.yml
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"
1 change: 1 addition & 0 deletions .github/zizmor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ rules:
# head code. Keep each suppression scoped to its reviewed trigger block.
- dco.yml:3
- e2e-label-help.yml:13
- maintainers-change-alert.yml:6
- release-canary.yml:3
- required-ci-gates.yml:3
- vouch-check.yml:3
4 changes: 4 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ Do not start substantial issue-backed work until a maintainer has accepted the i

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.

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.

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.

## Agent Skills

OpenShell keeps skills for using the product separate from skills for developing the repository.
Expand Down
102 changes: 102 additions & 0 deletions tasks/scripts/alert_maintainer_change.py
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:
Comment thread
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:
Comment thread
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())
87 changes: 87 additions & 0 deletions tasks/scripts/alert_maintainer_change_test.py
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
Loading
Loading