diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 2b1e795f29..e9899931bd 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -31,6 +31,11 @@ /tasks/scripts/check_maintainer_approval_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr /tasks/scripts/alert_maintainer_change_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +# Reviewer routing registry and helper +/REVIEWERS.md @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/tasks/scripts/check_review_readiness.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/tasks/scripts/check_review_readiness_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr + # PR issue acceptance checks /tasks/scripts/check_issue_acceptance.cjs @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr /tasks/scripts/check_issue_acceptance.test.cjs @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr diff --git a/.github/workflows/review-readiness.yml b/.github/workflows/review-readiness.yml new file mode 100644 index 0000000000..876d3e490e --- /dev/null +++ b/.github/workflows/review-readiness.yml @@ -0,0 +1,48 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +name: Review Readiness + +on: + pull_request_target: + types: [opened, ready_for_review, labeled, unlabeled] + +permissions: + contents: read + pull-requests: write + +concurrency: + group: review-readiness-${{ github.event.pull_request.number }} + cancel-in-progress: false + +jobs: + reviewers: + name: OpenShell / Review Readiness + if: >- + !github.event.pull_request.draft && + (github.event.action == 'opened' || + github.event.action == 'ready_for_review' || + startsWith(github.event.label.name, 'area:') || + startsWith(github.event.label.name, 'topic:')) + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + # Use the merged roster and helper, never code from the PR head. + - name: Check out the reviewer roster and helper + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.sha }} + sparse-checkout: | + REVIEWERS.md + tasks/scripts/check_review_readiness.py + sparse-checkout-cone-mode: false + persist-credentials: false + + - name: Reconcile reviewer requests + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + python3 tasks/scripts/check_review_readiness.py \ + --repo "$GH_REPO" --pr "$PR_NUMBER" --apply diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4050bc3f4c..4053caa7c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -70,9 +70,11 @@ Use agents and the repository skills as needed to understand the affected code, 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. -Pull requests that touch the enforcement machinery itself — anything under `.github/workflows/`, `.github/actions/`, `MAINTAINERS.md`, `.github/CODEOWNERS`, or the enforcement scripts in `tasks/scripts/` — additionally require a code owner's approval, listed in [.github/CODEOWNERS](.github/CODEOWNERS). The status check reads its inputs from `main`, but a workflow triggered by a review runs the pull request's own copy of the workflow file, so GitHub's native code owner requirement is what keeps a change from disabling the gate that would have blocked it. +Pull requests that touch the enforcement machinery or reviewer routing — anything under `.github/workflows/`, `.github/actions/`, `MAINTAINERS.md`, `REVIEWERS.md`, `.github/CODEOWNERS`, or the enforcement and reviewer-routing scripts in `tasks/scripts/` — additionally require a code owner's approval, listed in [.github/CODEOWNERS](.github/CODEOWNERS). The status check reads its inputs from `main`, but a workflow triggered by a review runs the pull request's own copy of the workflow file, so GitHub's native code owner requirement is what keeps a change from disabling the gate that would have blocked it. -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. +Use exactly one `area:` label and any relevant `topic:` labels on your pull request. The [reviewer routing workflow](.github/workflows/review-readiness.yml) requests all matching members of [REVIEWERS.md](REVIEWERS.md) with repository write access, excluding the author, when a PR opens ready for review, leaves draft status, or gains or loses an area/topic label. Manual reviewer choices and people who have commented or reviewed are preserved. Propose roster membership changes through a reviewed PR. + +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 diff --git a/REVIEWERS.md b/REVIEWERS.md new file mode 100644 index 0000000000..e56885efef --- /dev/null +++ b/REVIEWERS.md @@ -0,0 +1,245 @@ +# Review areas and topics + +## Areas + +Areas are OpenShell-specific components or system boundaries. They represent the division of labor across the project. Keep the number of areas small enough to support named leads and reporting during community calls. + +### area:build + +CI/CD workflows, release pipelines, build targets, packaging, and artifact publication. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | + +### area:drivers + +Infrastructure orchestration, including compute scheduling, secret management, runtime integrations (GPU, Docker, Podman, virtual machines, Kata Containers, and Kubernetes), deployment topologies, and pause/resume lifecycle operations. Includes host OS and architecture support (including amd64 and arm64), backend-specific isolation (including Windows AppContainer and isolation sessions), and compatibility. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | + +### area:runtime + +Sandbox execution, isolation, process supervision, policy enforcement, network proxying, middleware, volumes, and (agent) memory. Includes shared isolation primitives such as Linux namespaces, seccomp and Landlock, and process/executable identity. + +Provider discovery and configuration, profiles, sandbox attachments, provider-contributed policy, credential resolution and delivery, refresh and rotation, and provider lifecycle management. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Shiju | [@shiju-nv](https://github.com/shiju-nv) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | + +### area:enterprise + +Authentication, multitenancy, gateway interceptors, FIPS compliance, and other enterprise requirements. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Seth Jennings | [@sjenning](https://github.com/sjenning) | Red Hat | +| Derek Carr | [@derekwaynecarr](https://github.com/derekwaynecarr) | Red Hat | + +### area:platform + +SDKs, CLIs, API machinery, gateway infrastructure, persistence (eg postgres, sqlite support), configuration (gateway.toml), and gateway-side sandbox and session management. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | + +## Topics + +Topics connect contributors with work matching their expertise or interests. They describe technical concerns across areas rather than separate component ownership. + +### topic:api + +Public API contracts, gRPC and protobuf definitions, SDK interfaces, versioning, compatibility, API ergonomics, design patterns, component and communication standards. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat | + +### topic:architecture + +Cross-cutting system design, component boundaries, data and control flow, architectural decisions, and RFC-level concerns. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat | + +### topic:authnz + +Authentication, authorization, credential protection, policy integrity/signed policiesIncludes identity verification, login and session handling, OIDC integration, bearer-token validation, access permissions, and authorization across tenant and workspace boundaries. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Seth Jennings | [@sjenning](https://github.com/sjenning) | Red Hat | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | + +### topic:compliance + +FIPS, regulatory alignment, auditability, and standardization requirements. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | + +### topic:documentation + +Architecture documentation, README maintenance, project website updates, and automated documentation generation. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | + +### topic:dx + +Ensure OpenShell is easy to contribute to for humans and agents. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | + +### topic:gpu + +GPU scheduling, Kubernetes Dynamic Resource Allocation (DRA), Container Device Interface (CDI) configuration, Multi-Instance GPU (MIG) support, and hardware-accelerated runtime integration. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | +| Adrien Langou | [@alangou](https://github.com/alangou) | NVIDIA | + +### topic:kubernetes + +Kubernetes operator development, custom resource definitions (CRDs), deployment controllers, and native integration with Kubernetes APIs. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | + +### topic:mcp + +Model Context Protocol (MCP) support, server and client implementations, and standard integration. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | +| Shiju | [@shiju-nv](https://github.com/shiju-nv) | NVIDIA | + +### topic:networking + +General network transport, connectivity, and proxy behavior not covered by the more specialized L7 category. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | +| Adrien Langou | [@alangou](https://github.com/alangou) | NVIDIA | +| Shiju | [@shiju-nv](https://github.com/shiju-nv) | NVIDIA | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | + +### topic:observability + +Logging, metrics, tracing, telemetry, dashboards, event correlation, and the collection or export of operational data from gateways and sandboxes. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | + +### topic:operability + +Deployment and configuration of OpenShell installations, upgrades, backup and restore, disaster recovery, operational diagnostics, and maintenance of running installations. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | + +### topic:performance + +Startup time, latency, throughput, resource overhead, scalability, and benchmarking. Includes rate limiting. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | + +### topic:policy + +Policy authoring, validation, proving, distribution, enforcement, and policy ergonomics across runtimes and APIs. Includes policy hardening. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Adrien Langou | [@alangou](https://github.com/alangou) | NVIDIA | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Shiju | [@shiju-nv](https://github.com/shiju-nv) | NVIDIA | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | + +### topic:reliability + +High availability, failure recovery, retries, graceful degradation, fault tolerance, and operational resilience. Includes correctness across restarts. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | +| Divesh Chowdary | [@FrostGod](https://github.com/FrostGod) | NVIDIA | + +### topic:sdk + +Software Development Kit (SDK) creation, maintenance, language bindings, and distribution. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | +| Max Dubrinsky | [@maxdubrinsky](https://github.com/maxdubrinsky) | Gretel | + +### topic:security + +Vulnerability management, static/dynamic analysis, FSR (Functional Security Requirement) implementation, and security auditing. Includes bypass detection, abuse prevention, and vulnerability remediation. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Adrien Langou | [@alangou](https://github.com/alangou) | NVIDIA | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | +| Jim Meyer | [@purp](https://github.com/purp) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | + +### topic:testing + +Unit, integration, regression, conformance, and end-to-end testing, including test infrastructure, coverage gaps, flaky tests, and environment-specific test failures. + +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | +| Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py new file mode 100644 index 0000000000..5d2c9d2ee0 --- /dev/null +++ b/tasks/scripts/check_review_readiness.py @@ -0,0 +1,263 @@ +#!/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 + +"""Reconcile PR reviewers from the merged REVIEWERS.md and current PR metadata. + +Preview with --repo OWNER/REPO --pr NUMBER; add --apply to update requests. +Request all matching members with write access, excluding the author. Preserve +manual choices, unknown request provenance, and anyone who commented or reviewed. +The parser and planning helpers are importable. No state is stored between runs. +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from dataclasses import asdict, dataclass +from pathlib import Path + +HEADER = ["Name", "GitHub ID", "Company/Organization"] +LABEL = re.compile(r"(?:area|topic):[a-z0-9]+(?:-[a-z0-9]+)*") +PROFILE = re.compile(r"\[@([a-z0-9-]+)\]\(https://github\.com/([a-z0-9-]+)/?\)", re.I) + + +@dataclass(frozen=True) +class ReviewPlan: + add: tuple[str, ...] + remove: tuple[str, ...] + + +def parse_reviewers(markdown: str) -> dict[str, set[str]]: + """Read category headings and three-column tables, validating identities. + + This reads the roster's plain Markdown format, not arbitrary Markdown. + Empty tables are valid; duplicate categories or members are errors. + """ + tables = {} + label = None + for raw in markdown.splitlines(): + line = raw.strip() + if line.startswith("#"): + label = line.removeprefix("### ") + if not LABEL.fullmatch(label): + label = None + continue + if label in tables: + raise ValueError(f"duplicate category {label}") + tables[label] = [] + elif label and line.startswith("|"): + if not line.endswith("|"): + raise ValueError(f"{label}: table rows need enclosing pipes") + tables[label].append([cell.strip() for cell in line[1:-1].split("|")]) + if not tables: + raise ValueError("no area/topic membership tables found") + categories = {} + for label, rows in tables.items(): + if ( + len(rows) < 2 + or rows[0] != HEADER + or len(rows[1]) != 3 + or not all(re.fullmatch(r":?-{3,}:?", cell) for cell in rows[1]) + ): + raise ValueError(f"{label}: expected a three-column membership table") + members = set() + for row in rows[2:]: + match = PROFILE.fullmatch(row[1]) if len(row) == 3 else None + if not match or match[1].lower() != match[2].lower(): + raise ValueError(f"{label}: GitHub ID must link to the same profile") + login = match[1].lower() + if login in members: + raise ValueError(f"{label}: duplicate member @{login}") + members.add(login) + categories[label] = members + return categories + + +def select_reviewers(categories: dict[str, set[str]], labels: set[str]) -> set[str]: + """Return matching handles for one primary area and any number of topics.""" + relevant = {label for label in labels if label.startswith(("area:", "topic:"))} + if sum(label.startswith("area:") for label in relevant) != 1: + raise ValueError("a ready PR must have exactly one area label") + unknown = relevant - categories.keys() + if unknown: + raise ValueError("unknown area/topic labels: " + ", ".join(sorted(unknown))) + return set().union(*(categories[label] for label in relevant)) + + +def engaged_reviewers(*feeds: list[dict]) -> set[str]: + """Include every review state, general comment, and inline comment.""" + return { + entry["user"]["login"].lower() + for feed in feeds + for entry in feed + if (entry.get("user") or {}).get("login") + } + + +def manual_reviewers(events: list[dict], requested: set[str]) -> set[str]: + """Reconstruct human additions/removals; preserve unknown provenance too. + + Bot actors identify automated changes. A human request stays protected + through repeated requests until removal. Human removals are also protected + from immediate re-requesting. Team events do not affect individual requests. + """ + active, manual = set(), set() + changes = [ + event + for event in events + if event.get("event") in {"review_requested", "review_request_removed"} + ] + for event in sorted(changes, key=lambda entry: entry["id"]): + login = (event.get("requested_reviewer") or {}).get("login") + if not login: + continue + login = login.lower() + automated = (event.get("actor") or {}).get("type") == "Bot" + if event["event"] == "review_requested": + if login not in active: + manual.discard(login) + active.add(login) + if not automated: + manual.add(login) + else: + active.discard(login) + if automated: + manual.discard(login) + else: + manual.add(login) + return manual | ({login.lower() for login in requested} - active) + + +def plan_review_requests( + selected: set[str], + *, + requested: set[str], + engaged: set[str], + manual: set[str], +) -> ReviewPlan: + """Compute changes without IO, preserving engaged and manual reviewer state.""" + selected, requested, engaged, manual = ( + {login.lower() for login in group} + for group in (selected, requested, engaged, manual) + ) + protected = engaged | manual + return ReviewPlan( + tuple(sorted(selected - requested - protected)), + tuple(sorted(requested - selected - protected)), + ) + + +def _api(path: str, *, method: str = "GET", data=None, pages: bool = False): + command = ["gh", "api", path, "--method", method] + if pages: + command.extend(["--paginate", "--slurp"]) + if data is not None: + command.extend(["--input", "-"]) + result = subprocess.run( + command, + input=json.dumps(data) if data is not None else None, + capture_output=True, + text=True, + check=True, + ) + value = json.loads(result.stdout) if result.stdout.strip() else None + return [item for page in value for item in page] if pages else value + + +def _load_reviewers(repository: str, number: int, pr: dict) -> dict[str, set[str]]: + root = f"repos/{repository}" + pull = f"{root}/pulls/{number}" + reviews = _api(f"{pull}/reviews?per_page=100", pages=True) + comments = _api(f"{root}/issues/{number}/comments?per_page=100", pages=True) + inline = _api(f"{pull}/comments?per_page=100", pages=True) + timeline = _api(f"{root}/issues/{number}/timeline?per_page=100", pages=True) + requested = {user["login"].lower() for user in pr["requested_reviewers"]} + return { + "requested": requested, + "engaged": engaged_reviewers(reviews, comments, inline), + "manual": manual_reviewers(timeline, requested), + } + + +def _eligible_reviewers(repository: str, logins: set[str]) -> set[str]: + eligible = set() + for login in sorted(logins): + value = _api(f"repos/{repository}/collaborators/{login}/permission") + if value.get("permission") in {"write", "maintain", "admin"}: + eligible.add(login) + return eligible + + +def _routing_key(pr: dict): + return pr["state"], pr["draft"], { + label["name"] + for label in pr["labels"] + if label["name"].startswith(("area:", "topic:")) + } + + +def check_review_readiness( + categories: dict[str, set[str]], + repository: str, + number: int, + *, + apply=False, +) -> ReviewPlan | None: + """Plan or apply routing. Callers must serialize runs for the same PR.""" + pull = f"repos/{repository}/pulls/{number}" + pr = _api(pull) + if pr["state"] != "open" or pr["draft"]: + return None + selected = select_reviewers(categories, {label["name"] for label in pr["labels"]}) + selected = _eligible_reviewers(repository, selected - {pr["user"]["login"].lower()}) + plan = plan_review_requests(selected, **_load_reviewers(repository, number, pr)) + if apply: + endpoint = f"{pull}/requested_reviewers" + if plan.add: + _api(endpoint, method="POST", data={"reviewers": list(plan.add)}) + if plan.remove: + # Re-read human choices and engagement immediately before removals. + latest = _api(pull) + if _routing_key(latest) != _routing_key(pr): + raise ValueError("PR routing changed; rerun reconciliation") + refreshed = plan_review_requests( + selected, **_load_reviewers(repository, number, latest) + ) + plan = ReviewPlan(plan.add, refreshed.remove) + if plan.remove: + _api(endpoint, method="DELETE", data={"reviewers": list(plan.remove)}) + return plan + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--reviewers", type=Path, default=Path("REVIEWERS.md")) + parser.add_argument("--repo", required=True, help="target OWNER/REPO") + parser.add_argument("--pr", required=True, type=int, help="pull request number") + parser.add_argument( + "--apply", action="store_true", help="apply requests; otherwise preview JSON" + ) + args = parser.parse_args(argv) + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", args.repo) or args.pr <= 0: + parser.error("--repo must be OWNER/REPO and --pr must be positive") + try: + categories = parse_reviewers(args.reviewers.read_text(encoding="utf-8")) + plan = check_review_readiness(categories, args.repo, args.pr, apply=args.apply) + except (OSError, UnicodeError, ValueError, subprocess.CalledProcessError) as exc: + print(f"Review readiness: {exc}", file=sys.stderr) + return 1 + print(json.dumps(asdict(plan) if plan is not None else {"skipped": "closed or draft PR"})) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py new file mode 100644 index 0000000000..e6e942d470 --- /dev/null +++ b/tasks/scripts/check_review_readiness_test.py @@ -0,0 +1,423 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Offline tests for roster parsing and PR reviewer reconciliation.""" + +from __future__ import annotations + +import contextlib +import io +import json +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import check_review_readiness as routing + +TABLE = "| Name | GitHub ID | Company/Organization |\n| --- | --- | --- |\n" +ALICE = "| Alice | [@Alice](https://github.com/alice) | Example |\n" +DOCUMENT = "# Reviewers\n\n## Areas\n\n### area:runtime\n\nScope.\n\n" + TABLE + ALICE +CATEGORIES = { + "area:runtime": {"alice", "bob"}, + "area:build": {"dave"}, + "topic:policy": {"bob", "carol"}, +} +REPO = "owner/repo" +PULL = "repos/owner/repo/pulls/42" + + +def pr(*labels, requested=(), author="author", draft=False, state="open"): + return { + "state": state, + "draft": draft, + "user": {"login": author}, + "labels": [{"name": label} for label in labels], + "requested_reviewers": [{"login": login} for login in requested], + } + + +def reviewers(*, requested=(), engaged=(), manual=()): + return {"requested": set(requested), "engaged": set(engaged), "manual": set(manual)} + + +def event(number, login, *, human=False, removed=False): + return { + "id": number, + "event": "review_request_removed" if removed else "review_requested", + "actor": {"type": "User" if human else "Bot"}, + "requested_reviewer": {"login": login}, + } + + +class RegistryTests(unittest.TestCase): + def test_reads_only_membership_rows_and_allows_empty_categories(self): + text = DOCUMENT.replace( + "Scope.", "Thanks [@outside](https://github.com/outside)." + ) + text += "\n## Topics\n\n### topic:policy\n\n" + TABLE + self.assertEqual( + routing.parse_reviewers(text), + {"area:runtime": {"alice"}, "topic:policy": set()}, + ) + + def test_same_person_can_belong_to_multiple_categories(self): + text = DOCUMENT + "\n### area:build\n\n" + TABLE + ALICE + self.assertEqual(routing.parse_reviewers(text)["area:build"], {"alice"}) + + def test_rejects_duplicate_category_or_member(self): + for text in (DOCUMENT + ALICE, DOCUMENT + "\n### area:runtime\n" + TABLE): + with ( + self.subTest(text=text), + self.assertRaisesRegex(ValueError, "duplicate"), + ): + routing.parse_reviewers(text) + + def test_rejects_missing_or_malformed_membership_table(self): + cases = [ + "# Reviewers", + DOCUMENT.replace(TABLE, ""), + DOCUMENT.replace("| GitHub ID |", "| Role |"), + DOCUMENT.replace("| --- | --- | --- |", "| --- | --- |"), + DOCUMENT.replace(ALICE, ALICE.rstrip().removesuffix("|")), + ] + for text in cases: + with self.subTest(text=text), self.assertRaises(ValueError): + routing.parse_reviewers(text) + + def test_rejects_mismatched_or_ambiguous_profile_links(self): + for profile in ( + "[@alice](https://github.com/other)", + "@alice", + "[@alice](https://github.com.evil/alice)", + "[@alice](https://github.com/alice/repo)", + ): + text = DOCUMENT.replace("[@Alice](https://github.com/alice)", profile) + with self.subTest(profile=profile), self.assertRaises(ValueError): + routing.parse_reviewers(text) + + +class PlanningTests(unittest.TestCase): + def test_selects_all_matching_members_deduplicated(self): + self.assertEqual( + routing.select_reviewers( + CATEGORIES, {"area:runtime", "topic:policy", "kind:bug"} + ), + {"alice", "bob", "carol"}, + ) + + def test_requires_one_known_area_and_known_topics(self): + for labels in ( + set(), + {"topic:policy"}, + {"area:build", "area:runtime"}, + {"area:unknown"}, + {"area:runtime", "topic:unknown"}, + ): + with self.subTest(labels=labels), self.assertRaises(ValueError): + routing.select_reviewers(CATEGORIES, labels) + + def test_preserves_manual_and_engaged_reviewers_during_label_changes(self): + plan = routing.plan_review_requests( + {"dave"}, + **reviewers( + requested=("alice", "bob", "manual"), + engaged=("BOB",), + manual=("MANUAL",), + ), + ) + self.assertEqual(plan, routing.ReviewPlan(("dave",), ("alice",))) + + def test_does_not_re_request_engaged_or_manually_removed_reviewers(self): + plan = routing.plan_review_requests( + {"alice", "bob", "carol"}, + **reviewers(requested=("carol",), engaged=("ALICE",), manual=("bob",)), + ) + self.assertEqual(plan, routing.ReviewPlan((), ())) + + def test_repeated_routing_is_idempotent(self): + self.assertEqual( + routing.plan_review_requests( + {"alice"}, **reviewers(requested=("alice",)) + ), + routing.ReviewPlan((), ()), + ) + + def test_all_review_states_and_comment_types_count_as_engaged(self): + states = ("APPROVED", "CHANGES_REQUESTED", "DISMISSED", "COMMENTED", "PENDING") + reviews = [{"user": {"login": state}, "state": state} for state in states] + result = routing.engaged_reviewers( + reviews, + [{"user": {"login": "General"}}, {"user": None}], + [{"user": {"login": "Inline"}}], + ) + self.assertEqual(result, {state.lower() for state in states} | {"general", "inline"}) + + +class TimelineTests(unittest.TestCase): + def test_distinguishes_human_and_bot_requests(self): + events = [event(1, "Alice", human=True), event(2, "Bob")] + self.assertEqual( + routing.manual_reviewers(events, {"alice", "bob"}), {"alice"} + ) + + def test_unknown_actor_and_missing_history_are_preserved(self): + unknown = event(1, "alice") + unknown["actor"] = None + self.assertEqual( + routing.manual_reviewers([unknown], {"alice", "bob"}), {"alice", "bob"} + ) + + def test_bot_re_request_does_not_erase_existing_human_request(self): + events = [event(1, "alice", human=True), event(2, "alice")] + self.assertEqual(routing.manual_reviewers(events, {"alice"}), {"alice"}) + + def test_orders_events_and_preserves_manual_additions_and_removals(self): + events = [ + event(4, "bob", human=True, removed=True), + event(3, "alice", human=True), + event(2, "alice", removed=True), + event(1, "alice"), + {"id": 5, "event": "review_requested", "requested_team": {"slug": "team"}}, + ] + self.assertEqual(routing.manual_reviewers(events, {"alice"}), {"alice", "bob"}) + + def test_new_automatic_request_after_removal_is_not_protected(self): + events = [ + event(1, "alice", human=True), + event(2, "alice", removed=True), + event(3, "alice"), + ] + self.assertEqual(routing.manual_reviewers(events, {"alice"}), set()) + + +class GitHubTests(unittest.TestCase): + def test_api_reads_all_pages(self): + with mock.patch.object(routing.subprocess, "run") as run: + run.return_value.stdout = json.dumps( + [[{"user": {"login": "first"}}], [{"user": {"login": "last"}}]] + ) + result = routing._api("endpoint", pages=True) + self.assertEqual(routing.engaged_reviewers(result), {"first", "last"}) + self.assertIn("--paginate", run.call_args.args[0]) + self.assertIn("--slurp", run.call_args.args[0]) + + def test_loads_all_engagement_and_timeline_feeds(self): + responses = [ + [{"user": {"login": "Reviewer"}}], + [{"user": {"login": "Commenter"}}], + [{"user": {"login": "Inline"}}], + [event(1, "Manual", human=True)], + ] + with mock.patch.object(routing, "_api", side_effect=responses) as api: + result = routing._load_reviewers(REPO, 42, pr(requested=("Manual",))) + self.assertEqual( + result, + reviewers( + requested=("manual",), + engaged=("reviewer", "commenter", "inline"), + manual=("manual",), + ), + ) + self.assertTrue(all(call.kwargs["pages"] for call in api.call_args_list)) + self.assertIn("/timeline?per_page=100", api.call_args.args[0]) + + def test_only_current_write_maintain_admin_permissions_are_eligible(self): + responses = [ + {"permission": role} + for role in ("admin", "maintain", "read", "triage", "write") + ] + with mock.patch.object(routing, "_api", side_effect=responses): + self.assertEqual( + routing._eligible_reviewers(REPO, {"a", "b", "c", "d", "e"}), + {"a", "b", "e"}, + ) + + +class ReconciliationTests(unittest.TestCase): + def run_routing(self, categories, *, snapshots, responses, apply=True): + with ( + mock.patch.object(routing, "_api", side_effect=responses) as api, + mock.patch.object(routing, "_load_reviewers", side_effect=snapshots) as load, + mock.patch.object( + routing, "_eligible_reviewers", side_effect=lambda _repo, logins: logins + ) as eligible, + ): + plan = routing.check_review_readiness(categories, REPO, 42, apply=apply) + return plan, api, load, eligible + + def test_initial_assignment_excludes_author_without_extra_refresh(self): + plan, api, load, eligible = self.run_routing( + CATEGORIES, + snapshots=[reviewers()], + responses=[pr("area:runtime", "topic:policy", author="ALICE"), None], + ) + self.assertEqual(plan, routing.ReviewPlan(("bob", "carol"), ())) + eligible.assert_called_once_with(REPO, {"bob", "carol"}) + load.assert_called_once() + self.assertEqual(api.call_count, 2) + self.assertEqual( + api.call_args.kwargs, + {"method": "POST", "data": {"reviewers": ["bob", "carol"]}}, + ) + + def test_empty_area_does_not_block_topic_reviewers(self): + categories = {"area:runtime": set(), "topic:policy": {"carol"}} + plan, _, _, _ = self.run_routing( + categories, + snapshots=[reviewers()], + responses=[pr("area:runtime", "topic:policy")], + apply=False, + ) + self.assertEqual(plan.add, ("carol",)) + + def test_draft_and_closed_prs_skip_all_routing(self): + for value in (pr(draft=True), pr(state="closed")): + with self.subTest(value=value): + plan, api, load, eligible = self.run_routing( + CATEGORIES, snapshots=[], responses=[value] + ) + self.assertIsNone(plan) + api.assert_called_once_with(PULL) + load.assert_not_called() + eligible.assert_not_called() + + def test_preview_makes_no_changes(self): + plan, api, _, _ = self.run_routing( + CATEGORIES, + snapshots=[reviewers()], + responses=[pr("area:build")], + apply=False, + ) + self.assertEqual(plan.add, ("dave",)) + api.assert_called_once_with(PULL) + + def test_refresh_before_removal_protects_new_manual_choices_and_engagement(self): + before = pr("area:build", requested=("alice", "bob", "carol")) + after = pr("area:build", requested=("alice", "bob", "carol", "dave")) + snapshots = [ + reviewers(requested=("alice", "bob", "carol")), + reviewers( + requested=("alice", "bob", "carol", "dave"), + engaged=("bob",), + manual=("carol",), + ), + ] + plan, api, _, _ = self.run_routing( + CATEGORIES, snapshots=snapshots, responses=[before, None, after, None] + ) + self.assertEqual(plan, routing.ReviewPlan(("dave",), ("alice",))) + self.assertEqual( + api.call_args, + mock.call( + f"{PULL}/requested_reviewers", + method="DELETE", + data={"reviewers": ["alice"]}, + ), + ) + + def test_changed_routing_or_draft_status_stops_removals(self): + before = pr("area:build", requested=("alice", "dave")) + for after in ( + pr("area:runtime"), + pr("area:build", draft=True), + pr("area:build", state="closed"), + ): + with ( + self.subTest(after=after), + mock.patch.object(routing, "_api", side_effect=[before, after]) as api, + mock.patch.object( + routing, "_load_reviewers", + return_value=reviewers(requested=("alice", "dave")), + ), + mock.patch.object(routing, "_eligible_reviewers", return_value={"dave"}), + ): + with self.assertRaisesRegex(ValueError, "routing changed"): + routing.check_review_readiness(CATEGORIES, REPO, 42, apply=True) + self.assertTrue(all( + call.kwargs.get("method", "GET") == "GET" + for call in api.call_args_list + )) + + def test_new_commit_and_unrelated_label_do_not_interrupt_routing(self): + before = pr("area:build", requested=("alice", "dave")) + after = pr("area:build", "kind:bug", requested=("alice", "dave")) + before["head"], after["head"] = {"sha": "old"}, {"sha": "new"} + snapshots = [reviewers(requested=("alice", "dave"))] * 2 + plan, _, _, _ = self.run_routing( + CATEGORIES, snapshots=snapshots, responses=[before, after, None] + ) + self.assertEqual(plan.remove, ("alice",)) + + def test_retry_after_partial_failure_uses_updated_github_metadata(self): + before = pr("area:build", requested=("alice",)) + after = pr("area:build", requested=("alice", "dave")) + snapshots = [ + reviewers(requested=("alice",)), reviewers(requested=("alice", "dave")) + ] + with self.assertRaises(subprocess.CalledProcessError): + self.run_routing( + CATEGORIES, + snapshots=snapshots, + responses=[before, None, after, subprocess.CalledProcessError(1, "gh")], + ) + plan, api, _, _ = self.run_routing( + CATEGORIES, snapshots=[snapshots[1]] * 2, responses=[after, after, None] + ) + self.assertEqual(plan, routing.ReviewPlan((), ("alice",))) + self.assertFalse(any( + call.kwargs.get("method") == "POST" for call in api.call_args_list + )) + + +class CliTests(unittest.TestCase): + def test_emits_plan_and_passes_apply_option(self): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "REVIEWERS.md" + path.write_text(DOCUMENT, encoding="utf-8") + output = io.StringIO() + with ( + mock.patch.object( + routing, "check_review_readiness", + return_value=routing.ReviewPlan(("alice",), ()), + ) as check, + contextlib.redirect_stdout(output), + ): + self.assertEqual( + routing.main([ + "--reviewers", str(path), "--repo", REPO, + "--pr", "42", "--apply", + ]), + 0, + ) + self.assertEqual( + json.loads(output.getvalue()), {"add": ["alice"], "remove": []} + ) + check.assert_called_once_with( + {"area:runtime": {"alice"}}, REPO, 42, apply=True + ) + + def test_invalid_registry_fails_before_contacting_github(self): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "REVIEWERS.md" + path.write_text("not a registry", encoding="utf-8") + output, errors = io.StringIO(), io.StringIO() + with ( + mock.patch.object(routing, "_api") as api, + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(errors), + ): + self.assertEqual( + routing.main([ + "--reviewers", str(path), "--repo", REPO, "--pr", "42", + ]), + 1, + ) + api.assert_not_called() + self.assertEqual(output.getvalue(), "") + self.assertIn("no area/topic", errors.getvalue()) + + +if __name__ == "__main__": + unittest.main() diff --git a/tasks/test.toml b/tasks/test.toml index 95e049d9ab..094319f113 100644 --- a/tasks/test.toml +++ b/tasks/test.toml @@ -23,6 +23,7 @@ depends = [ "test:docs-website", "test:docs-nav", "test:maintainer-approval", + "test:review-readiness", ] ["test:docs-website"] @@ -91,6 +92,11 @@ 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 +["test:review-readiness"] +description = "Test PR review readiness and reviewer reconciliation" +run = "uv run --no-project python -m unittest discover -s tasks/scripts -p check_review_readiness_test.py" +hide = true + [e2e] description = "Run all end-to-end tests (Rust + Python + MCP)" depends = ["e2e:rust", "e2e:python", "e2e:mcp"]