mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-02 07:34:45 +08:00
feat(ci): add the maintainer approval decision tools
Adds the two stdlib-only helpers that back the maintainer approval gate, ahead of the workflows that call them. check_maintainer_approval.py parses the linked handles out of MAINTAINERS.md, folds a review list to each reviewer's latest decisive position, and exits non-zero unless a maintainer's latest position is an approval. It fails closed when the list yields no handles. alert_maintainer_change.py renders the approver-set delta between two versions of MAINTAINERS.md, and prints nothing when the set is unchanged. Landing these first lets the gate workflow, which reads both the list and the decision logic from main rather than from the pull request ref, actually execute on the pull request that introduces it. Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
This commit is contained in:
@@ -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:
|
||||
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:
|
||||
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())
|
||||
@@ -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
|
||||
@@ -0,0 +1,91 @@
|
||||
#!/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
|
||||
|
||||
"""Exit non-zero unless a maintainer listed in MAINTAINERS.md has approved.
|
||||
|
||||
Runs as bare `python3` on the Actions runner, so it must stay stdlib-only.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import json
|
||||
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/alert_maintainer_change.py, which reports the deltas this
|
||||
# gate enforces.
|
||||
MAINTAINER_RE = re.compile(
|
||||
r"\[@([A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)\]\(https://github\.com/"
|
||||
)
|
||||
|
||||
# States that express a standing position. COMMENTED and PENDING leave a
|
||||
# reviewer's earlier approval intact, which is how GitHub itself treats them.
|
||||
DECISIVE_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"})
|
||||
|
||||
|
||||
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 parse_reviews_to_approvers(reviews: list[dict]) -> set[str]:
|
||||
"""Return the lowercased logins whose current review state is an approval.
|
||||
|
||||
A reviewer's latest decisive review supersedes their earlier ones, ordered
|
||||
by review id rather than by the order the caller assembled the pages in.
|
||||
"""
|
||||
positions: dict[str, str] = {}
|
||||
for entry in sorted(reviews, key=lambda r: r.get("id") or 0):
|
||||
state = str(entry.get("state") or "").upper()
|
||||
if state not in DECISIVE_STATES:
|
||||
continue
|
||||
login = str((entry.get("user") or {}).get("login") or "").lower()
|
||||
if login:
|
||||
positions[login] = state
|
||||
return {login for login, state in positions.items() if state == "APPROVED"}
|
||||
|
||||
|
||||
def main(argv: list[str] | None = None) -> int:
|
||||
parser = argparse.ArgumentParser(description=__doc__)
|
||||
parser.add_argument(
|
||||
"--maintainers",
|
||||
required=True,
|
||||
type=Path,
|
||||
help="MAINTAINERS.md read from the default branch",
|
||||
)
|
||||
parser.add_argument(
|
||||
"--reviews",
|
||||
required=True,
|
||||
type=Path,
|
||||
help="JSON array returned by the list-reviews API",
|
||||
)
|
||||
args = parser.parse_args(argv)
|
||||
|
||||
maintainers = parse_maintainers(args.maintainers.read_text(encoding="utf-8"))
|
||||
if not maintainers:
|
||||
# Fail closed: an unparseable list must never satisfy the gate.
|
||||
print("Could not parse any maintainers from MAINTAINERS.md")
|
||||
return 1
|
||||
|
||||
reviews = json.loads(args.reviews.read_text(encoding="utf-8"))
|
||||
approvers = sorted(parse_reviews_to_approvers(reviews) & maintainers)
|
||||
if not approvers:
|
||||
print("Needs approval from a maintainer listed in MAINTAINERS.md")
|
||||
return 1
|
||||
|
||||
print("Approved by " + ", ".join(f"@{login}" for login in approvers))
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -0,0 +1,94 @@
|
||||
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
|
||||
# SPDX-License-Identifier: Apache-2.0
|
||||
|
||||
"""Tests for tasks/scripts/check_maintainer_approval.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 json
|
||||
|
||||
import check_maintainer_approval as gate
|
||||
|
||||
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 review(login: str, state: str) -> dict:
|
||||
return {"user": {"login": login}, "state": state}
|
||||
|
||||
|
||||
def run(tmp_path, table: str, reviews: list[dict]) -> int:
|
||||
maintainers = tmp_path / "MAINTAINERS.md"
|
||||
maintainers.write_text(table, encoding="utf-8")
|
||||
reviews_file = tmp_path / "reviews.json"
|
||||
reviews_file.write_text(json.dumps(reviews), encoding="utf-8")
|
||||
return gate.main(
|
||||
["--maintainers", str(maintainers), "--reviews", str(reviews_file)]
|
||||
)
|
||||
|
||||
|
||||
def test_parse_maintainers_extracts_linked_logins() -> None:
|
||||
assert gate.parse_maintainers(TABLE) == {"derekwaynecarr", "elezar", "pimlock"}
|
||||
|
||||
|
||||
def test_parse_maintainers_ignores_unlinked_mentions() -> None:
|
||||
# A prose mention must not silently grant approval rights.
|
||||
prose = TABLE + "\nThanks to [@drive-by](mailto:nobody@example.com) too.\n"
|
||||
assert "drive-by" not in gate.parse_maintainers(prose)
|
||||
|
||||
|
||||
def test_comment_after_approval_does_not_revoke_it() -> None:
|
||||
reviews = [review("elezar", "APPROVED"), review("elezar", "COMMENTED")]
|
||||
assert gate.parse_reviews_to_approvers(reviews) == {"elezar"}
|
||||
|
||||
|
||||
def test_dismissed_review_revokes_approval() -> None:
|
||||
reviews = [review("elezar", "APPROVED"), review("elezar", "DISMISSED")]
|
||||
assert gate.parse_reviews_to_approvers(reviews) == set()
|
||||
|
||||
|
||||
def test_changes_requested_after_approval_revokes_it() -> None:
|
||||
reviews = [review("elezar", "APPROVED"), review("elezar", "CHANGES_REQUESTED")]
|
||||
assert gate.parse_reviews_to_approvers(reviews) == set()
|
||||
|
||||
|
||||
def test_out_of_order_reviews_still_respect_the_latest_position() -> None:
|
||||
# Ordering comes from the review id, not the order the caller happened
|
||||
# to assemble the pages in.
|
||||
reviews = [
|
||||
{"id": 2, "user": {"login": "elezar"}, "state": "DISMISSED"},
|
||||
{"id": 1, "user": {"login": "elezar"}, "state": "APPROVED"},
|
||||
]
|
||||
assert gate.parse_reviews_to_approvers(reviews) == set()
|
||||
|
||||
|
||||
def test_exits_zero_when_a_maintainer_approved(tmp_path) -> None:
|
||||
assert run(tmp_path, TABLE, [review("pimlock", "APPROVED")]) == 0
|
||||
|
||||
|
||||
def test_matches_logins_case_insensitively(tmp_path) -> None:
|
||||
assert run(tmp_path, TABLE, [review("PiMlOcK", "APPROVED")]) == 0
|
||||
|
||||
|
||||
def test_exits_nonzero_with_no_reviews(tmp_path) -> None:
|
||||
# The exit code is the check result, so this is the gate's actual contract.
|
||||
assert run(tmp_path, TABLE, []) != 0
|
||||
|
||||
|
||||
def test_exits_nonzero_on_non_maintainer_approval(tmp_path) -> None:
|
||||
assert run(tmp_path, TABLE, [review("outsider", "APPROVED")]) != 0
|
||||
|
||||
|
||||
def test_fails_closed_on_unparseable_list(tmp_path) -> None:
|
||||
assert run(tmp_path, "# Maintainers\n", [review("pimlock", "APPROVED")]) != 0
|
||||
@@ -21,6 +21,7 @@ depends = [
|
||||
"test:codex-security-release-range",
|
||||
"test:docs-website",
|
||||
"test:docs-nav",
|
||||
"test:maintainer-approval",
|
||||
]
|
||||
|
||||
["test:docs-website"]
|
||||
@@ -79,6 +80,11 @@ description = "Test Codex Security release-range resolution"
|
||||
run = "uv run --no-project --with pytest pytest -o \"python_files=*_test.py\" tasks/scripts/codex_security_range_test.py"
|
||||
hide = true
|
||||
|
||||
["test:maintainer-approval"]
|
||||
description = "Test the maintainer approval gate and change-alert tools"
|
||||
run = "uv run --no-project --with pytest pytest -o \"python_files=*_test.py\" tasks/scripts/check_maintainer_approval_test.py tasks/scripts/alert_maintainer_change_test.py"
|
||||
hide = true
|
||||
|
||||
[e2e]
|
||||
description = "Run all end-to-end tests (Rust + Python + MCP)"
|
||||
depends = ["e2e:rust", "e2e:python", "e2e:mcp"]
|
||||
|
||||
Reference in New Issue
Block a user