From bb4057d1ce3072057f1263f7458e09601cbb4522 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Thu, 22 Oct 2026 17:04:14 +0000 Subject: [PATCH 01/18] docs(review): propose area and topic reviewer registry Signed-off-by: Jim Meyer --- REVIEWERS.md | 274 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 274 insertions(+) create mode 100644 REVIEWERS.md diff --git a/REVIEWERS.md b/REVIEWERS.md new file mode 100644 index 0000000000..21dfbb1a5a --- /dev/null +++ b/REVIEWERS.md @@ -0,0 +1,274 @@ +# Review areas and topics + +Draft for community review. All names below are suggestions, not confirmed volunteers. No automatic requests, notifications, or new approval requirements are enabled by this draft. + +Related work: [review routing and membership workflow (#4839)](https://github.com/NVIDIA/OpenShell/issues/4839). + +## Purpose + +Help authors and tools invite the right people to review OpenShell changes. Areas identify components and system boundaries with coordinating ownership. Topics identify expertise that can apply across several areas. An issue or PR can involve several topics. + +These definitions follow the group's reviewed areas/topics proposal. This file is the proposed repository registry; the initial roster and format remain open for review. + +## Join, change, or leave a roster + +Open a PR editing the relevant membership tables below. Include your GitHub handle, role, status, and review scope. Explain the change in the PR description. A reviewed and merged PR is the sign-up mechanism for every role. + +For the initial proposal, comment on the forthcoming registry PR to suggest additions, removals, or scope corrections. A proposed person's agreement is needed before their entry becomes confirmed. Contributions linked below are evidence for discussion, not proof of willingness or complete subject-matter coverage. + +Subscriber entries require the individual's opt-in. No subscribers have been enrolled in this draft. + +## Roles and coverage + +| Role | Participation | Repository privilege | +| --- | --- | --- | +| maintainer | Available for review and approval in the recorded scope. | Maintain or admin. | +| reviewer | Available for selected review requests in the recorded scope. | Write. | +| subscriber | Receives relevant-work notifications and can contribute feedback without a review obligation. | Read, including public access. | + +Roles follow repository privileges. Aligning today's privileges and the CNCF organization's permissions is outside this proposal. Suggested maintainer entries are drawn from [MAINTAINERS.md](MAINTAINERS.md); this registry does not grant repository access or change the existing maintainer approval check. + +Target at least two confirmed maintainers covering every area and topic. Suggestions do not count as confirmed coverage. Scope limitations matter: two names do not establish two-person coverage of every specialty. All categories currently need confirmation; compliance and MCP also need additional maintainer nominations. + +## Proposed routing behavior + +Every ready-for-review PR has exactly one primary area for coordination and zero or more relevant topics. There is no hard topic limit; apply topics that merit specialist review. Other materially affected areas still contribute reviewers even when they are not the primary area. + +Select a non-author maintainer from the primary area and relevant reviewers for the topics and secondary-area impact. Reuse people who cover several categories, distribute requests among eligible volunteers, and allow human reassignment. Notify opted-in subscribers separately and deduplicate notifications across categories. + +Routing must use confirmed membership from the trusted merged registry. A PR changing this file cannot use its proposed membership to authorize its own review. Selection and notifications are follow-up work in #4839; they do not make every roster member a required approver. + +## Registry format + +Each category has a level-three heading containing its exact label, a scope description, and a membership table with these fixed columns: + +| Column | Meaning | +| --- | --- | +| GitHub | One GitHub login, written as an @mention. | +| Role | `maintainer`, `reviewer`, or `subscriber`. | +| Status | `suggested` or `confirmed`. Only confirmed entries may receive automated requests or notifications. | +| Scope | The person's review scope or notification interests. | +| Basis | Optional contribution evidence for a suggestion, or `Self-nominated` for a volunteer. | + +Use one row per person in a category. A maintainer or reviewer can cover several categories, but should not also have a subscriber row in the same category. Remove an entry through a reviewed PR to withdraw, or change it to subscriber when the person wants notifications without review requests. + +## 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. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @matthewgrossman | maintainer | suggested | Release pipelines and packaging | [#4754](https://github.com/NVIDIA/OpenShell/pull/4754) | +| @SDAChess | maintainer | suggested | Build tooling and compatibility checks | [#4611](https://github.com/NVIDIA/OpenShell/pull/4611) | + +### area:drivers + +Infrastructure orchestration, including compute scheduling, secret management, runtime integrations (GPU, Docker, 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. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @elezar | maintainer | suggested | Compute capabilities and backend integration | [#3010](https://github.com/NVIDIA/OpenShell/pull/3010) | +| @drew | maintainer | suggested | Compute-driver and supervisor integration | [#4656](https://github.com/NVIDIA/OpenShell/pull/4656) | +| @mrunalp | maintainer | suggested | Kubernetes compute driver | [#4527](https://github.com/NVIDIA/OpenShell/pull/4527) | + +### 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. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @drew | maintainer | suggested | Isolation, workload execution, and supervisor lifecycle | [#4106](https://github.com/NVIDIA/OpenShell/pull/4106) | +| @TaylorMutch | maintainer | suggested | Sandbox and supervisor integration | [#4823](https://github.com/NVIDIA/OpenShell/pull/4823) | +| @shiju-nv | reviewer | suggested | Provider credentials and protocol inspection | [#4152](https://github.com/NVIDIA/OpenShell/pull/4152) | + +### area:enterprise + +Authentication, multitenancy, gateway interceptors, FIPS compliance, and other enterprise requirements. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @sjenning | maintainer | suggested | Extension contracts and provider authorization | [#4357](https://github.com/NVIDIA/OpenShell/pull/4357) | +| @derekwaynecarr | maintainer | suggested | Service authorization | [#3796](https://github.com/NVIDIA/OpenShell/pull/3796) | + +### area:platform + +SDKs, CLIs, API machinery, gateway infrastructure, persistence (eg postgres, sqlite support), configuration (gateway.toml), and gateway-side sandbox and session management. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @pimlock | maintainer | suggested | Gateway configuration and supervisor sessions | [#4320](https://github.com/NVIDIA/OpenShell/pull/4320) | +| @TaylorMutch | maintainer | suggested | CLI and Rust SDK integration | [#4817](https://github.com/NVIDIA/OpenShell/pull/4817) | + +## 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. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @drew | maintainer | suggested | Public API entities and references | [#3272](https://github.com/NVIDIA/OpenShell/pull/3272) | +| @mrunalp | maintainer | suggested | Workspace selectors and mutation contracts | [#3321](https://github.com/NVIDIA/OpenShell/pull/3321) | + +### topic:architecture + +Cross-cutting system design, component boundaries, data and control flow, architectural decisions, and RFC-level concerns. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @drew | maintainer | suggested | Runtime and compute-driver boundaries | [#4106](https://github.com/NVIDIA/OpenShell/pull/4106) | +| @mrunalp | maintainer | suggested | Gateway mutation architecture | [#3321](https://github.com/NVIDIA/OpenShell/pull/3321) | + +### 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. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @sjenning | maintainer | suggested | Provider authorization and extension negotiation | [#4357](https://github.com/NVIDIA/OpenShell/pull/4357) | +| @krishicks | maintainer | suggested | Gateway and sandbox authentication | [#4762](https://github.com/NVIDIA/OpenShell/pull/4762) | + +### topic:compliance + +FIPS, regulatory alignment, auditability, and standardization requirements. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @johntmyers | maintainer | suggested | Repository license and compliance text; FIPS coverage needs nomination | [#3467](https://github.com/NVIDIA/OpenShell/pull/3467) | + +### topic:documentation + +Architecture documentation, README maintenance, project website updates, and automated documentation generation. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @krishicks | maintainer | suggested | Observability documentation | [#4560](https://github.com/NVIDIA/OpenShell/pull/4560) | +| @drew | maintainer | suggested | Deployment and contributor-facing documentation | [#4622](https://github.com/NVIDIA/OpenShell/pull/4622) | + +### topic:dx + +Ensure OpenShell is easy to contribute to for humans and agents. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @elezar | maintainer | suggested | Contributor triage and workflow tooling | [#4534](https://github.com/NVIDIA/OpenShell/pull/4534) | +| @johntmyers | maintainer | suggested | Agent review and contributor workflows | [#4309](https://github.com/NVIDIA/OpenShell/pull/4309) | + +### topic:gpu + +GPU scheduling, Kubernetes Dynamic Resource Allocation (DRA), Container Device Interface (CDI) configuration, Multi-Instance GPU (MIG) support, and hardware-accelerated runtime integration. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @elezar | maintainer | suggested | Device capabilities and compute integration | [#3010](https://github.com/NVIDIA/OpenShell/pull/3010) | +| @alangou | maintainer | suggested | CDI device identity and validation | [#4306](https://github.com/NVIDIA/OpenShell/pull/4306) | + +### topic:kubernetes + +Kubernetes operator development, custom resource definitions (CRDs), deployment controllers, and native integration with Kubernetes APIs. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @mrunalp | maintainer | suggested | Kubernetes compute-driver behavior | [#4527](https://github.com/NVIDIA/OpenShell/pull/4527) | +| @drew | maintainer | suggested | Shared supervisor deployment | [#4695](https://github.com/NVIDIA/OpenShell/pull/4695) | + +### topic:mcp + +Model Context Protocol (MCP) support, server and client implementations, and standard integration. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @johntmyers | maintainer | suggested | Network enforcement integration; MCP-specific coverage needs confirmation | [#2723](https://github.com/NVIDIA/OpenShell/pull/2723) | +| @shiju-nv | reviewer | suggested | MCP protocol and revision enforcement | [#3850](https://github.com/NVIDIA/OpenShell/pull/3850) | + +### topic:networking + +General network transport, connectivity, and proxy behavior not covered by the more specialized L7 category. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @johntmyers | maintainer | suggested | Proxy egress and transparent TCP | [#2723](https://github.com/NVIDIA/OpenShell/pull/2723) | +| @alangou | maintainer | suggested | DNS and hostname policy enforcement | [#4602](https://github.com/NVIDIA/OpenShell/pull/4602) | +| @shiju-nv | reviewer | suggested | HTTP and application-protocol inspection | [#3753](https://github.com/NVIDIA/OpenShell/pull/3753) | + +### topic:observability + +Logging, metrics, tracing, telemetry, dashboards, event correlation, and the collection or export of operational data from gateways and sandboxes. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @krishicks | maintainer | suggested | OCSF and OpenTelemetry | [#4545](https://github.com/NVIDIA/OpenShell/pull/4545) | +| @matthewgrossman | maintainer | suggested | Tracing correctness and log handling | [#4212](https://github.com/NVIDIA/OpenShell/pull/4212) | + +### topic:operability + +Deployment and configuration of OpenShell installations, upgrades, backup and restore, disaster recovery, operational diagnostics, and maintenance of running installations. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @pimlock | maintainer | suggested | Gateway configuration and Kubernetes reconciliation | [#4676](https://github.com/NVIDIA/OpenShell/pull/4676) | +| @TaylorMutch | maintainer | suggested | HA gateway operation | [#1868](https://github.com/NVIDIA/OpenShell/pull/1868) | + +### topic:performance + +Startup time, latency, throughput, resource overhead, scalability, and benchmarking. Includes rate limiting. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @matthewgrossman | maintainer | suggested | Gateway and supervisor overhead | [#4619](https://github.com/NVIDIA/OpenShell/pull/4619) | +| @drew | maintainer | suggested | Sandbox process lifecycle overhead | [#3757](https://github.com/NVIDIA/OpenShell/pull/3757) | + +### topic:policy + +Policy authoring, validation, proving, distribution, enforcement, and policy ergonomics across runtimes and APIs. Includes policy hardening. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @alangou | maintainer | suggested | Policy matching and proof correctness | [#4326](https://github.com/NVIDIA/OpenShell/pull/4326) | +| @pimlock | maintainer | suggested | Policy authoring and rule proposals | [#3707](https://github.com/NVIDIA/OpenShell/pull/3707) | +| @shiju-nv | reviewer | suggested | Provider and application-protocol policy | [#3850](https://github.com/NVIDIA/OpenShell/pull/3850) | + +### topic:reliability + +High availability, failure recovery, retries, graceful degradation, fault tolerance, and operational resilience. Includes correctness across restarts. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @TaylorMutch | maintainer | suggested | HA gateway behavior | [#1868](https://github.com/NVIDIA/OpenShell/pull/1868) | +| @pimlock | maintainer | suggested | Gateway reconciliation and lifecycle | [#4676](https://github.com/NVIDIA/OpenShell/pull/4676) | + +### topic:sdk + +Software Development Kit (SDK) creation, maintenance, language bindings, and distribution. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @TaylorMutch | maintainer | suggested | Rust SDK and CLI integration | [#4817](https://github.com/NVIDIA/OpenShell/pull/4817) | +| @SDAChess | maintainer | suggested | TypeScript SDK | [#4495](https://github.com/NVIDIA/OpenShell/pull/4495) | +| @maxdubrinsky | reviewer | suggested | Python SDK and credential-driver bindings | [#4760](https://github.com/NVIDIA/OpenShell/pull/4760) | + +### topic:security + +Vulnerability management, static/dynamic analysis, FSR (Functional Security Requirement) implementation, and security auditing. Includes bypass detection, abuse prevention, and vulnerability remediation. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @alangou | maintainer | suggested | Security scanning and policy validation | [#4203](https://github.com/NVIDIA/OpenShell/pull/4203) | +| @johntmyers | maintainer | suggested | Security regression review | [#4338](https://github.com/NVIDIA/OpenShell/pull/4338) | + +### topic:testing + +Unit, integration, regression, conformance, and end-to-end testing, including test infrastructure, coverage gaps, flaky tests, and environment-specific test failures. + +| GitHub | Role | Status | Scope | Basis | +| --- | --- | --- | --- | --- | +| @matthewgrossman | maintainer | suggested | Conformance and tracing tests | [#4521](https://github.com/NVIDIA/OpenShell/pull/4521) | +| @SDAChess | maintainer | suggested | Test infrastructure and compatibility checks | [#4488](https://github.com/NVIDIA/OpenShell/pull/4488) | From b45320ab90aa4322c5766dc6ec35152f74d7895b Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Thu, 22 Oct 2026 20:55:13 +0000 Subject: [PATCH 02/18] docs(review): keep registry focused on areas and rosters Signed-off-by: Jim Meyer --- REVIEWERS.md | 52 ---------------------------------------------------- 1 file changed, 52 deletions(-) diff --git a/REVIEWERS.md b/REVIEWERS.md index 21dfbb1a5a..6ae2711396 100644 --- a/REVIEWERS.md +++ b/REVIEWERS.md @@ -1,57 +1,5 @@ # Review areas and topics -Draft for community review. All names below are suggestions, not confirmed volunteers. No automatic requests, notifications, or new approval requirements are enabled by this draft. - -Related work: [review routing and membership workflow (#4839)](https://github.com/NVIDIA/OpenShell/issues/4839). - -## Purpose - -Help authors and tools invite the right people to review OpenShell changes. Areas identify components and system boundaries with coordinating ownership. Topics identify expertise that can apply across several areas. An issue or PR can involve several topics. - -These definitions follow the group's reviewed areas/topics proposal. This file is the proposed repository registry; the initial roster and format remain open for review. - -## Join, change, or leave a roster - -Open a PR editing the relevant membership tables below. Include your GitHub handle, role, status, and review scope. Explain the change in the PR description. A reviewed and merged PR is the sign-up mechanism for every role. - -For the initial proposal, comment on the forthcoming registry PR to suggest additions, removals, or scope corrections. A proposed person's agreement is needed before their entry becomes confirmed. Contributions linked below are evidence for discussion, not proof of willingness or complete subject-matter coverage. - -Subscriber entries require the individual's opt-in. No subscribers have been enrolled in this draft. - -## Roles and coverage - -| Role | Participation | Repository privilege | -| --- | --- | --- | -| maintainer | Available for review and approval in the recorded scope. | Maintain or admin. | -| reviewer | Available for selected review requests in the recorded scope. | Write. | -| subscriber | Receives relevant-work notifications and can contribute feedback without a review obligation. | Read, including public access. | - -Roles follow repository privileges. Aligning today's privileges and the CNCF organization's permissions is outside this proposal. Suggested maintainer entries are drawn from [MAINTAINERS.md](MAINTAINERS.md); this registry does not grant repository access or change the existing maintainer approval check. - -Target at least two confirmed maintainers covering every area and topic. Suggestions do not count as confirmed coverage. Scope limitations matter: two names do not establish two-person coverage of every specialty. All categories currently need confirmation; compliance and MCP also need additional maintainer nominations. - -## Proposed routing behavior - -Every ready-for-review PR has exactly one primary area for coordination and zero or more relevant topics. There is no hard topic limit; apply topics that merit specialist review. Other materially affected areas still contribute reviewers even when they are not the primary area. - -Select a non-author maintainer from the primary area and relevant reviewers for the topics and secondary-area impact. Reuse people who cover several categories, distribute requests among eligible volunteers, and allow human reassignment. Notify opted-in subscribers separately and deduplicate notifications across categories. - -Routing must use confirmed membership from the trusted merged registry. A PR changing this file cannot use its proposed membership to authorize its own review. Selection and notifications are follow-up work in #4839; they do not make every roster member a required approver. - -## Registry format - -Each category has a level-three heading containing its exact label, a scope description, and a membership table with these fixed columns: - -| Column | Meaning | -| --- | --- | -| GitHub | One GitHub login, written as an @mention. | -| Role | `maintainer`, `reviewer`, or `subscriber`. | -| Status | `suggested` or `confirmed`. Only confirmed entries may receive automated requests or notifications. | -| Scope | The person's review scope or notification interests. | -| Basis | Optional contribution evidence for a suggestion, or `Self-nominated` for a volunteer. | - -Use one row per person in a category. A maintainer or reviewer can cover several categories, but should not also have a subscriber row in the same category. Remove an entry through a reviewed PR to withdraw, or change it to subscriber when the person wants notifications without review requests. - ## 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. From 0232b6bcc3235f2684bfb3b72b6c99b48429a617 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Thu, 22 Oct 2026 21:24:51 +0000 Subject: [PATCH 03/18] docs(review): match roster tables to maintainer format Signed-off-by: Jim Meyer --- REVIEWERS.md | 192 +++++++++++++++++++++++++-------------------------- 1 file changed, 96 insertions(+), 96 deletions(-) diff --git a/REVIEWERS.md b/REVIEWERS.md index 6ae2711396..f56f06dfd9 100644 --- a/REVIEWERS.md +++ b/REVIEWERS.md @@ -8,20 +8,20 @@ Areas are OpenShell-specific components or system boundaries. They represent the CI/CD workflows, release pipelines, build targets, packaging, and artifact publication. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @matthewgrossman | maintainer | suggested | Release pipelines and packaging | [#4754](https://github.com/NVIDIA/OpenShell/pull/4754) | -| @SDAChess | maintainer | suggested | Build tooling and compatibility checks | [#4611](https://github.com/NVIDIA/OpenShell/pull/4611) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | ### area:drivers Infrastructure orchestration, including compute scheduling, secret management, runtime integrations (GPU, Docker, 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @elezar | maintainer | suggested | Compute capabilities and backend integration | [#3010](https://github.com/NVIDIA/OpenShell/pull/3010) | -| @drew | maintainer | suggested | Compute-driver and supervisor integration | [#4656](https://github.com/NVIDIA/OpenShell/pull/4656) | -| @mrunalp | maintainer | suggested | Kubernetes compute driver | [#4527](https://github.com/NVIDIA/OpenShell/pull/4527) | +| 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 | ### area:runtime @@ -29,29 +29,29 @@ Sandbox execution, isolation, process supervision, policy enforcement, network p Provider discovery and configuration, profiles, sandbox attachments, provider-contributed policy, credential resolution and delivery, refresh and rotation, and provider lifecycle management. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @drew | maintainer | suggested | Isolation, workload execution, and supervisor lifecycle | [#4106](https://github.com/NVIDIA/OpenShell/pull/4106) | -| @TaylorMutch | maintainer | suggested | Sandbox and supervisor integration | [#4823](https://github.com/NVIDIA/OpenShell/pull/4823) | -| @shiju-nv | reviewer | suggested | Provider credentials and protocol inspection | [#4152](https://github.com/NVIDIA/OpenShell/pull/4152) | +| 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 | ### area:enterprise Authentication, multitenancy, gateway interceptors, FIPS compliance, and other enterprise requirements. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @sjenning | maintainer | suggested | Extension contracts and provider authorization | [#4357](https://github.com/NVIDIA/OpenShell/pull/4357) | -| @derekwaynecarr | maintainer | suggested | Service authorization | [#3796](https://github.com/NVIDIA/OpenShell/pull/3796) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @pimlock | maintainer | suggested | Gateway configuration and supervisor sessions | [#4320](https://github.com/NVIDIA/OpenShell/pull/4320) | -| @TaylorMutch | maintainer | suggested | CLI and Rust SDK integration | [#4817](https://github.com/NVIDIA/OpenShell/pull/4817) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | ## Topics @@ -61,162 +61,162 @@ Topics connect contributors with work matching their expertise or interests. The Public API contracts, gRPC and protobuf definitions, SDK interfaces, versioning, compatibility, API ergonomics, design patterns, component and communication standards. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @drew | maintainer | suggested | Public API entities and references | [#3272](https://github.com/NVIDIA/OpenShell/pull/3272) | -| @mrunalp | maintainer | suggested | Workspace selectors and mutation contracts | [#3321](https://github.com/NVIDIA/OpenShell/pull/3321) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @drew | maintainer | suggested | Runtime and compute-driver boundaries | [#4106](https://github.com/NVIDIA/OpenShell/pull/4106) | -| @mrunalp | maintainer | suggested | Gateway mutation architecture | [#3321](https://github.com/NVIDIA/OpenShell/pull/3321) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @sjenning | maintainer | suggested | Provider authorization and extension negotiation | [#4357](https://github.com/NVIDIA/OpenShell/pull/4357) | -| @krishicks | maintainer | suggested | Gateway and sandbox authentication | [#4762](https://github.com/NVIDIA/OpenShell/pull/4762) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @johntmyers | maintainer | suggested | Repository license and compliance text; FIPS coverage needs nomination | [#3467](https://github.com/NVIDIA/OpenShell/pull/3467) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | ### topic:documentation Architecture documentation, README maintenance, project website updates, and automated documentation generation. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @krishicks | maintainer | suggested | Observability documentation | [#4560](https://github.com/NVIDIA/OpenShell/pull/4560) | -| @drew | maintainer | suggested | Deployment and contributor-facing documentation | [#4622](https://github.com/NVIDIA/OpenShell/pull/4622) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | ### topic:dx Ensure OpenShell is easy to contribute to for humans and agents. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @elezar | maintainer | suggested | Contributor triage and workflow tooling | [#4534](https://github.com/NVIDIA/OpenShell/pull/4534) | -| @johntmyers | maintainer | suggested | Agent review and contributor workflows | [#4309](https://github.com/NVIDIA/OpenShell/pull/4309) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @elezar | maintainer | suggested | Device capabilities and compute integration | [#3010](https://github.com/NVIDIA/OpenShell/pull/3010) | -| @alangou | maintainer | suggested | CDI device identity and validation | [#4306](https://github.com/NVIDIA/OpenShell/pull/4306) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @mrunalp | maintainer | suggested | Kubernetes compute-driver behavior | [#4527](https://github.com/NVIDIA/OpenShell/pull/4527) | -| @drew | maintainer | suggested | Shared supervisor deployment | [#4695](https://github.com/NVIDIA/OpenShell/pull/4695) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Mrunal Patel | [@mrunalp](https://github.com/mrunalp) | Red Hat | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | ### topic:mcp Model Context Protocol (MCP) support, server and client implementations, and standard integration. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @johntmyers | maintainer | suggested | Network enforcement integration; MCP-specific coverage needs confirmation | [#2723](https://github.com/NVIDIA/OpenShell/pull/2723) | -| @shiju-nv | reviewer | suggested | MCP protocol and revision enforcement | [#3850](https://github.com/NVIDIA/OpenShell/pull/3850) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @johntmyers | maintainer | suggested | Proxy egress and transparent TCP | [#2723](https://github.com/NVIDIA/OpenShell/pull/2723) | -| @alangou | maintainer | suggested | DNS and hostname policy enforcement | [#4602](https://github.com/NVIDIA/OpenShell/pull/4602) | -| @shiju-nv | reviewer | suggested | HTTP and application-protocol inspection | [#3753](https://github.com/NVIDIA/OpenShell/pull/3753) | +| 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 | ### topic:observability Logging, metrics, tracing, telemetry, dashboards, event correlation, and the collection or export of operational data from gateways and sandboxes. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @krishicks | maintainer | suggested | OCSF and OpenTelemetry | [#4545](https://github.com/NVIDIA/OpenShell/pull/4545) | -| @matthewgrossman | maintainer | suggested | Tracing correctness and log handling | [#4212](https://github.com/NVIDIA/OpenShell/pull/4212) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Kris Hicks | [@krishicks](https://github.com/krishicks) | NVIDIA | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | ### topic:operability Deployment and configuration of OpenShell installations, upgrades, backup and restore, disaster recovery, operational diagnostics, and maintenance of running installations. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @pimlock | maintainer | suggested | Gateway configuration and Kubernetes reconciliation | [#4676](https://github.com/NVIDIA/OpenShell/pull/4676) | -| @TaylorMutch | maintainer | suggested | HA gateway operation | [#1868](https://github.com/NVIDIA/OpenShell/pull/1868) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | ### topic:performance Startup time, latency, throughput, resource overhead, scalability, and benchmarking. Includes rate limiting. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @matthewgrossman | maintainer | suggested | Gateway and supervisor overhead | [#4619](https://github.com/NVIDIA/OpenShell/pull/4619) | -| @drew | maintainer | suggested | Sandbox process lifecycle overhead | [#3757](https://github.com/NVIDIA/OpenShell/pull/3757) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Drew Newberry | [@drew](https://github.com/drew) | NVIDIA | ### topic:policy Policy authoring, validation, proving, distribution, enforcement, and policy ergonomics across runtimes and APIs. Includes policy hardening. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @alangou | maintainer | suggested | Policy matching and proof correctness | [#4326](https://github.com/NVIDIA/OpenShell/pull/4326) | -| @pimlock | maintainer | suggested | Policy authoring and rule proposals | [#3707](https://github.com/NVIDIA/OpenShell/pull/3707) | -| @shiju-nv | reviewer | suggested | Provider and application-protocol policy | [#3850](https://github.com/NVIDIA/OpenShell/pull/3850) | +| 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 | ### topic:reliability High availability, failure recovery, retries, graceful degradation, fault tolerance, and operational resilience. Includes correctness across restarts. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @TaylorMutch | maintainer | suggested | HA gateway behavior | [#1868](https://github.com/NVIDIA/OpenShell/pull/1868) | -| @pimlock | maintainer | suggested | Gateway reconciliation and lifecycle | [#4676](https://github.com/NVIDIA/OpenShell/pull/4676) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Taylor Mutch | [@TaylorMutch](https://github.com/TaylorMutch) | NVIDIA | +| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | ### topic:sdk Software Development Kit (SDK) creation, maintenance, language bindings, and distribution. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @TaylorMutch | maintainer | suggested | Rust SDK and CLI integration | [#4817](https://github.com/NVIDIA/OpenShell/pull/4817) | -| @SDAChess | maintainer | suggested | TypeScript SDK | [#4495](https://github.com/NVIDIA/OpenShell/pull/4495) | -| @maxdubrinsky | reviewer | suggested | Python SDK and credential-driver bindings | [#4760](https://github.com/NVIDIA/OpenShell/pull/4760) | +| 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. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @alangou | maintainer | suggested | Security scanning and policy validation | [#4203](https://github.com/NVIDIA/OpenShell/pull/4203) | -| @johntmyers | maintainer | suggested | Security regression review | [#4338](https://github.com/NVIDIA/OpenShell/pull/4338) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Adrien Langou | [@alangou](https://github.com/alangou) | NVIDIA | +| John T. Myers | [@johntmyers](https://github.com/johntmyers) | NVIDIA | ### topic:testing Unit, integration, regression, conformance, and end-to-end testing, including test infrastructure, coverage gaps, flaky tests, and environment-specific test failures. -| GitHub | Role | Status | Scope | Basis | -| --- | --- | --- | --- | --- | -| @matthewgrossman | maintainer | suggested | Conformance and tracing tests | [#4521](https://github.com/NVIDIA/OpenShell/pull/4521) | -| @SDAChess | maintainer | suggested | Test infrastructure and compatibility checks | [#4488](https://github.com/NVIDIA/OpenShell/pull/4488) | +| Name | GitHub ID | Company/Organization | +| --- | --- | --- | +| Matthew Grossman | [@matthewgrossman](https://github.com/matthewgrossman) | NVIDIA | +| Simon Scatton | [@SDAChess](https://github.com/SDAChess) | NVIDIA | From bed467b608a5e4d7778cff792b67128578c370ff Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Thu, 22 Oct 2026 21:55:08 +0000 Subject: [PATCH 04/18] feat(review): add offline reviewer registry parser Signed-off-by: Jim Meyer --- tasks/scripts/parse_reviewers.md | 83 +++++++++ tasks/scripts/parse_reviewers.py | 243 ++++++++++++++++++++++++++ tasks/scripts/parse_reviewers_test.py | 165 +++++++++++++++++ tasks/test.toml | 6 + 4 files changed, 497 insertions(+) create mode 100644 tasks/scripts/parse_reviewers.md create mode 100644 tasks/scripts/parse_reviewers.py create mode 100644 tasks/scripts/parse_reviewers_test.py diff --git a/tasks/scripts/parse_reviewers.md b/tasks/scripts/parse_reviewers.md new file mode 100644 index 0000000000..5bbc9db70d --- /dev/null +++ b/tasks/scripts/parse_reviewers.md @@ -0,0 +1,83 @@ +# Reviewer registry parser + +`parse_reviewers.py` provides an offline Python API and a JSON CLI for +[`REVIEWERS.md`](../../REVIEWERS.md). It uses only the Python standard library. +It reads membership, category labels, and scope descriptions. Repository roles, +permission checks, reviewer selection, notifications, and approval requirements +belong to consumers of this data. + +Validate the registry: + +```shell +uv run --no-project tasks/scripts/parse_reviewers.py --reviewers REVIEWERS.md --check +``` + +Export JSON: + +```shell +uv run --no-project tasks/scripts/parse_reviewers.py --reviewers REVIEWERS.md +``` + +Run the focused tests: + +```shell +mise run test:reviewers +``` + +## Supported format + +- Categories use level-three headings such as `### area:runtime` under `## Areas` + or `### topic:policy` under `## Topics`. Labels use lowercase letters, digits, + and hyphen-separated words. Category labels are unique. +- Each category has a nonempty scope description followed by one contiguous + table with exactly `Name`, `GitHub ID`, and `Company/Organization` columns. +- Tables use leading and trailing pipes. Separator alignment markers and escaped + pipes in values are supported. Names and organizations must be nonempty. +- The GitHub ID is a link such as `[@Alice](https://github.com/alice)` whose + displayed handle matches the profile path, case-insensitively. A trailing slash + on the profile URL is accepted. Stored logins are lowercase. +- A person may belong to several categories. Duplicate handles within one + category are errors, including duplicates with different capitalization. +- An explicitly empty membership table is valid. Coverage requirements are + separate from syntax validation and need repository permission information. +- Profile links in prose, code examples, or HTML comments do not add members. + The parser does not support arbitrary Markdown table extensions, reference + links, or HTML tables. + +## Python API and JSON contract + +`parse_reviewers(markdown)` returns a tuple of immutable `Category` records. +Each record contains `label`, `description`, and a tuple of `Member` records. +Members contain `name`, `login`, and `organization`. Category/member order follows +the document; descriptions retain Markdown text and paragraph breaks. + +The CLI emits this versioned structure: + +```json +{ + "schema_version": 1, + "categories": [ + { + "label": "area:runtime", + "description": "Workload execution.", + "members": [ + {"name": "Alice", "login": "alice", "organization": "Example"} + ] + } + ] +} +``` + +Consumers can look up categories, combine/deduplicate their member sets, reverse +the mapping to find a person's memberships, or compare two parsed versions to +report membership changes. Empty categories produce an empty `members` array. + +Malformed registries raise `RegistryError` in Python. The CLI reports the input +path and diagnostic on stderr, exits nonzero, and emits no partial JSON. Structural +errors include a source line number. `--check` prints a validation summary instead +of JSON. Missing or unreadable input also exits nonzero. + +For operational routing or approval decisions, read both the helper and registry +from a trusted merged ref, as the existing maintainer-approval check does. Parsing +a proposed PR version is useful for reporting changes; it does not authorize +that proposed membership to review the same PR. diff --git a/tasks/scripts/parse_reviewers.py b/tasks/scripts/parse_reviewers.py new file mode 100644 index 0000000000..36c8afce9b --- /dev/null +++ b/tasks/scripts/parse_reviewers.py @@ -0,0 +1,243 @@ +#!/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 + +"""Validate REVIEWERS.md and emit its categories and members as JSON. + +This dependency-free parser reads the documented registry format, not arbitrary +Markdown. It performs no GitHub requests and assigns no roles or review rights. +""" + +from __future__ import annotations + +import argparse +import json +import re +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](?:[a-z0-9-]{0,37}[a-z0-9])?)\]" + r"\(https://github\.com/([a-z0-9](?:[a-z0-9-]{0,37}[a-z0-9])?)/?\)", + re.IGNORECASE, +) + + +class RegistryError(ValueError): + """The source does not satisfy the registry format.""" + + +@dataclass(frozen=True) +class Member: + name: str + login: str + organization: str + + +@dataclass(frozen=True) +class Category: + label: str + description: str + members: tuple[Member, ...] + + +def _error(line: int, message: str) -> RegistryError: + return RegistryError(f"line {line}: {message}") + + +def _visible_lines(markdown: str): + """Ignore comments and code examples without treating their links as members.""" + fence = None + comment = False + last_line = 1 + for number, raw in enumerate(markdown.splitlines(), 1): + last_line = number + if fence: + closing = r" {0,3}" + re.escape(fence[0]) + "{" + str(len(fence)) + r",}\s*" + if re.fullmatch(closing, raw): + fence = None + continue + visible = "" + remaining = raw + while remaining: + if comment: + _, closing, remaining = remaining.partition("-->") + if not closing: + break + comment = False + else: + before, opening, remaining = remaining.partition("\n" + "```markdown\n### topic:fake\n" + HEADER + ALICE + "```\n" + "~~~\n" + HEADER + ALICE + "~~~\n" + ) + text = DOCUMENT.replace("Workload execution.", examples + "Workload execution.") + result = registry.parse_reviewers(text) + self.assertEqual(len(result), 1) + self.assertEqual([m.login for m in result[0].members], ["alice"]) + + def test_comments_between_members_do_not_end_the_table(self): + other = "| Other | [@other](https://github.com/other) | Example |\n" + text = DOCUMENT + "\n" + other + self.assertEqual( + [m.login for m in registry.parse_reviewers(text)[0].members], + ["alice", "other"], + ) + + def test_profile_links_outside_category_tables_do_not_count(self): + text = "[@outside](https://github.com/outside)\n\n" + DOCUMENT + self.assertEqual([m.login for m in registry.parse_reviewers(text)[0].members], ["alice"]) + + def test_escaped_pipes_and_aligned_separator(self): + text = DOCUMENT.replace("| --- | --- | --- |", "| :--- | :---: | ---: |") + text = text.replace("| Example |", r"| Example\|Labs |") + self.assertEqual(registry.parse_reviewers(text)[0].members[0].organization, "Example|Labs") + + def test_profile_case_and_trailing_slash_are_accepted(self): + text = DOCUMENT.replace("https://github.com/alice", "https://github.com/ALICE/") + self.assertEqual(registry.parse_reviewers(text)[0].members[0].login, "alice") + + def test_rejects_rows_with_ambiguous_or_invalid_identity(self): + invalid = [ + ALICE.replace("/alice)", "/someone-else)"), + ALICE.replace("https://github.com/alice", "https://github.com/alice/repo"), + ALICE.replace("https://github.com/alice", "https://github.com.evil/alice"), + ALICE.replace("https://github.com/alice", "http://github.com/alice"), + ALICE.replace("[@Alice](https://github.com/alice)", "@Alice"), + ALICE.replace("[@Alice](https://github.com/alice)", "[@Alice](https://github.com/alice) extra"), + ALICE.replace("| Alice |", "| |"), + ALICE.replace("| Example |", "| |"), + ] + for row in invalid: + with self.subTest(row=row), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(DOCUMENT.replace(ALICE, row)) + + def test_rejects_duplicate_members_case_insensitively(self): + with self.assertRaisesRegex(registry.RegistryError, "duplicate member @alice"): + registry.parse_reviewers(DOCUMENT + ALICE.replace("@Alice", "@alice")) + + def test_rejects_bad_table_structure(self): + invalid = [ + DOCUMENT.replace(HEADER, ""), + DOCUMENT.replace("| --- | --- | --- |\n", ""), + DOCUMENT.replace("| GitHub ID |", "| Role |"), + DOCUMENT.replace("| --- | --- | --- |", "| --- | --- |"), + DOCUMENT.replace(ALICE, ALICE.rstrip().rstrip("|") + "\n"), + DOCUMENT.replace(ALICE, ALICE.removeprefix("|")), + DOCUMENT.replace(ALICE, ALICE.rstrip() + " Extra |\n"), + DOCUMENT + "\n" + HEADER + ALICE, + ] + for text in invalid: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_duplicate_or_misplaced_categories(self): + invalid = [ + DOCUMENT + "\n### area:runtime\n\nOther scope.\n\n" + HEADER, + DOCUMENT.replace("area:runtime", "topic:runtime"), + DOCUMENT.replace("area:runtime", "area:Runtime"), + DOCUMENT.replace("area:runtime", "area:runtime_"), + DOCUMENT.replace("## Areas\n", ""), + DOCUMENT + "\n## Areas\n", + ] + for text in invalid: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_missing_scope_or_no_registry(self): + for text in [DOCUMENT.replace("Workload execution.", ""), "# Reviewers\n", ""]: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_unterminated_comments_and_examples(self): + for suffix in ["\n") + if not closing: + break + comment = False + else: + before, opening, remaining = remaining.partition("\n" + "```markdown\n### topic:fake\n" + HEADER + ALICE + "```\n" + "~~~\n" + HEADER + ALICE + "~~~\n" + ) + text = DOCUMENT.replace("Workload execution.", examples + "Workload execution.") + result = registry.parse_reviewers(text) + self.assertEqual(len(result), 1) + self.assertEqual([m.login for m in result[0].members], ["alice"]) + + def test_comments_between_members_do_not_end_the_table(self): + other = "| Other | [@other](https://github.com/other) | Example |\n" + text = DOCUMENT + "\n" + other + self.assertEqual( + [m.login for m in registry.parse_reviewers(text)[0].members], + ["alice", "other"], + ) + + def test_profile_links_outside_category_tables_do_not_count(self): + text = "[@outside](https://github.com/outside)\n\n" + DOCUMENT + self.assertEqual( + [m.login for m in registry.parse_reviewers(text)[0].members], ["alice"] + ) + + def test_escaped_pipes_and_aligned_separator(self): + text = DOCUMENT.replace("| --- | --- | --- |", "| :--- | :---: | ---: |") + text = text.replace("| Example |", r"| Example\|Labs |") + self.assertEqual( + registry.parse_reviewers(text)[0].members[0].organization, "Example|Labs" + ) + + def test_profile_case_and_trailing_slash_are_accepted(self): + text = DOCUMENT.replace("https://github.com/alice", "https://github.com/ALICE/") + self.assertEqual(registry.parse_reviewers(text)[0].members[0].login, "alice") + + def test_rejects_rows_with_ambiguous_or_invalid_identity(self): + invalid = [ + ALICE.replace("/alice)", "/someone-else)"), + ALICE.replace("https://github.com/alice", "https://github.com/alice/repo"), + ALICE.replace("https://github.com/alice", "https://github.com.evil/alice"), + ALICE.replace("https://github.com/alice", "http://github.com/alice"), + ALICE.replace("[@Alice](https://github.com/alice)", "@Alice"), + ALICE.replace( + "[@Alice](https://github.com/alice)", + "[@Alice](https://github.com/alice) extra", + ), + ALICE.replace("| Alice |", "| |"), + ALICE.replace("| Example |", "| |"), + ] + for row in invalid: + with self.subTest(row=row), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(DOCUMENT.replace(ALICE, row)) + + def test_rejects_duplicate_members_case_insensitively(self): + with self.assertRaisesRegex(registry.RegistryError, "duplicate member @alice"): + registry.parse_reviewers(DOCUMENT + ALICE.replace("@Alice", "@alice")) + + def test_rejects_bad_table_structure(self): + invalid = [ + DOCUMENT.replace(HEADER, ""), + DOCUMENT.replace("| --- | --- | --- |\n", ""), + DOCUMENT.replace("| GitHub ID |", "| Role |"), + DOCUMENT.replace("| --- | --- | --- |", "| --- | --- |"), + DOCUMENT.replace(ALICE, ALICE.rstrip().rstrip("|") + "\n"), + DOCUMENT.replace(ALICE, ALICE.removeprefix("|")), + DOCUMENT.replace(ALICE, ALICE.rstrip() + " Extra |\n"), + DOCUMENT + "\n" + HEADER + ALICE, + ] + for text in invalid: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_duplicate_or_misplaced_categories(self): + invalid = [ + DOCUMENT + "\n### area:runtime\n\nOther scope.\n\n" + HEADER, + DOCUMENT.replace("area:runtime", "topic:runtime"), + DOCUMENT.replace("area:runtime", "area:Runtime"), + DOCUMENT.replace("area:runtime", "area:runtime_"), + DOCUMENT.replace("## Areas\n", ""), + DOCUMENT + "\n## Areas\n", + ] + for text in invalid: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_missing_scope_or_no_registry(self): + for text in [DOCUMENT.replace("Workload execution.", ""), "# Reviewers\n", ""]: + with self.subTest(text=text), self.assertRaises(registry.RegistryError): + registry.parse_reviewers(text) + + def test_rejects_unterminated_comments_and_examples(self): + for suffix in ["\n") - if not closing: - break - comment = False - else: - before, opening, remaining = remaining.partition("\n" - "```markdown\n### topic:fake\n" + HEADER + ALICE + "```\n" - "~~~\n" + HEADER + ALICE + "~~~\n" - ) - text = DOCUMENT.replace("Workload execution.", examples + "Workload execution.") - result = registry.parse_reviewers(text) - self.assertEqual(len(result), 1) - self.assertEqual([m.login for m in result[0].members], ["alice"]) - - def test_comments_between_members_do_not_end_the_table(self): - other = "| Other | [@other](https://github.com/other) | Example |\n" - text = DOCUMENT + "\n" + other - self.assertEqual( - [m.login for m in registry.parse_reviewers(text)[0].members], - ["alice", "other"], - ) - - def test_profile_links_outside_category_tables_do_not_count(self): - text = "[@outside](https://github.com/outside)\n\n" + DOCUMENT - self.assertEqual([m.login for m in registry.parse_reviewers(text)[0].members], ["alice"]) - - def test_escaped_pipes_and_aligned_separator(self): - text = DOCUMENT.replace("| --- | --- | --- |", "| :--- | :---: | ---: |") - text = text.replace("| Example |", r"| Example\|Labs |") - self.assertEqual(registry.parse_reviewers(text)[0].members[0].organization, "Example|Labs") - - def test_profile_case_and_trailing_slash_are_accepted(self): - text = DOCUMENT.replace("https://github.com/alice", "https://github.com/ALICE/") - self.assertEqual(registry.parse_reviewers(text)[0].members[0].login, "alice") - - def test_rejects_rows_with_ambiguous_or_invalid_identity(self): - invalid = [ - ALICE.replace("/alice)", "/someone-else)"), - ALICE.replace("https://github.com/alice", "https://github.com/alice/repo"), - ALICE.replace("https://github.com/alice", "https://github.com.evil/alice"), - ALICE.replace("https://github.com/alice", "http://github.com/alice"), - ALICE.replace("[@Alice](https://github.com/alice)", "@Alice"), - ALICE.replace("[@Alice](https://github.com/alice)", "[@Alice](https://github.com/alice) extra"), - ALICE.replace("| Alice |", "| |"), - ALICE.replace("| Example |", "| |"), - ] - for row in invalid: - with self.subTest(row=row), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(DOCUMENT.replace(ALICE, row)) - - def test_rejects_duplicate_members_case_insensitively(self): - with self.assertRaisesRegex(registry.RegistryError, "duplicate member @alice"): - registry.parse_reviewers(DOCUMENT + ALICE.replace("@Alice", "@alice")) - - def test_rejects_bad_table_structure(self): - invalid = [ - DOCUMENT.replace(HEADER, ""), - DOCUMENT.replace("| --- | --- | --- |\n", ""), - DOCUMENT.replace("| GitHub ID |", "| Role |"), - DOCUMENT.replace("| --- | --- | --- |", "| --- | --- |"), - DOCUMENT.replace(ALICE, ALICE.rstrip().rstrip("|") + "\n"), - DOCUMENT.replace(ALICE, ALICE.removeprefix("|")), - DOCUMENT.replace(ALICE, ALICE.rstrip() + " Extra |\n"), - DOCUMENT + "\n" + HEADER + ALICE, - ] - for text in invalid: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_duplicate_or_misplaced_categories(self): - invalid = [ - DOCUMENT + "\n### area:runtime\n\nOther scope.\n\n" + HEADER, - DOCUMENT.replace("area:runtime", "topic:runtime"), - DOCUMENT.replace("area:runtime", "area:Runtime"), - DOCUMENT.replace("area:runtime", "area:runtime_"), - DOCUMENT.replace("## Areas\n", ""), - DOCUMENT + "\n## Areas\n", - ] - for text in invalid: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_missing_scope_or_no_registry(self): - for text in [DOCUMENT.replace("Workload execution.", ""), "# Reviewers\n", ""]: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_unterminated_comments_and_examples(self): - for suffix in ["\n") - if not closing: - break - comment = False - else: - before, opening, remaining = remaining.partition("\n" - "```markdown\n### topic:fake\n" + HEADER + ALICE + "```\n" - "~~~\n" + HEADER + ALICE + "~~~\n" +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 = DOCUMENT.replace("Workload execution.", examples + "Workload execution.") - result = registry.parse_reviewers(text) - self.assertEqual(len(result), 1) - self.assertEqual([m.login for m in result[0].members], ["alice"]) - - def test_comments_between_members_do_not_end_the_table(self): - other = "| Other | [@other](https://github.com/other) | Example |\n" - text = DOCUMENT + "\n" + other + text += "\n## Topics\n\n### topic:policy\n\n" + TABLE self.assertEqual( - [m.login for m in registry.parse_reviewers(text)[0].members], - ["alice", "other"], + routing.parse_reviewers(text), + {"area:runtime": {"alice"}, "topic:policy": set()}, ) - def test_profile_links_outside_category_tables_do_not_count(self): - text = "[@outside](https://github.com/outside)\n\n" + DOCUMENT - self.assertEqual( - [m.login for m in registry.parse_reviewers(text)[0].members], ["alice"] - ) + 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_escaped_pipes_and_aligned_separator(self): - text = DOCUMENT.replace("| --- | --- | --- |", "| :--- | :---: | ---: |") - text = text.replace("| Example |", r"| Example\|Labs |") - self.assertEqual( - registry.parse_reviewers(text)[0].members[0].organization, "Example|Labs" - ) + 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_profile_case_and_trailing_slash_are_accepted(self): - text = DOCUMENT.replace("https://github.com/alice", "https://github.com/ALICE/") - self.assertEqual(registry.parse_reviewers(text)[0].members[0].login, "alice") - - def test_rejects_rows_with_ambiguous_or_invalid_identity(self): - invalid = [ - ALICE.replace("/alice)", "/someone-else)"), - ALICE.replace("https://github.com/alice", "https://github.com/alice/repo"), - ALICE.replace("https://github.com/alice", "https://github.com.evil/alice"), - ALICE.replace("https://github.com/alice", "http://github.com/alice"), - ALICE.replace("[@Alice](https://github.com/alice)", "@Alice"), - ALICE.replace( - "[@Alice](https://github.com/alice)", - "[@Alice](https://github.com/alice) extra", - ), - ALICE.replace("| Alice |", "| |"), - ALICE.replace("| Example |", "| |"), - ] - for row in invalid: - with self.subTest(row=row), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(DOCUMENT.replace(ALICE, row)) - - def test_rejects_duplicate_members_case_insensitively(self): - with self.assertRaisesRegex(registry.RegistryError, "duplicate member @alice"): - registry.parse_reviewers(DOCUMENT + ALICE.replace("@Alice", "@alice")) - - def test_rejects_bad_table_structure(self): - invalid = [ - DOCUMENT.replace(HEADER, ""), - DOCUMENT.replace("| --- | --- | --- |\n", ""), + 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().rstrip("|") + "\n"), - DOCUMENT.replace(ALICE, ALICE.removeprefix("|")), - DOCUMENT.replace(ALICE, ALICE.rstrip() + " Extra |\n"), - DOCUMENT + "\n" + HEADER + ALICE, + DOCUMENT.replace(ALICE, ALICE.rstrip().removesuffix("|")), ] - for text in invalid: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_duplicate_or_misplaced_categories(self): - invalid = [ - DOCUMENT + "\n### area:runtime\n\nOther scope.\n\n" + HEADER, - DOCUMENT.replace("area:runtime", "topic:runtime"), - DOCUMENT.replace("area:runtime", "area:Runtime"), - DOCUMENT.replace("area:runtime", "area:runtime_"), - DOCUMENT.replace("## Areas\n", ""), - DOCUMENT + "\n## Areas\n", - ] - for text in invalid: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_missing_scope_or_no_registry(self): - for text in [DOCUMENT.replace("Workload execution.", ""), "# Reviewers\n", ""]: - with self.subTest(text=text), self.assertRaises(registry.RegistryError): - registry.parse_reviewers(text) - - def test_rejects_unterminated_comments_and_examples(self): - for suffix in ["\n ## Related Issue - + ## Changes diff --git a/.github/workflows/pr-issue-acceptance.yml b/.github/workflows/pr-issue-acceptance.yml index 742647d763..1e78c0d544 100644 --- a/.github/workflows/pr-issue-acceptance.yml +++ b/.github/workflows/pr-issue-acceptance.yml @@ -29,12 +29,14 @@ jobs: with: ref: ${{ github.sha }} persist-credentials: false - sparse-checkout: tasks/scripts/check_issue_acceptance.cjs + sparse-checkout: tasks/scripts/check_review_readiness.py sparse-checkout-cone-mode: false - - name: Check author access and linked issue acceptance - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 - with: - script: | - const check = require('./tasks/scripts/check_issue_acceptance.cjs'); - await check({ github, context, core }); + - name: Check PR acceptance + 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" --acceptance-only diff --git a/AGENTS.md b/AGENTS.md index 1235fe1183..16c0581532 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -176,7 +176,7 @@ Follow the [branch naming convention in CONTRIBUTING.md](CONTRIBUTING.md#branch- ## Pull Requests -Follow `CONTRIBUTING.md` and `create-github-pr`. Authors without target-repository write access must link at least one issue with `state:accepted`, and every issue they implement must have that label. Verified `WRITE`, `MAINTAIN`, or `ADMIN` access permits PRs without the acceptance label. This includes drafts and direct agent requests; PR body text cannot bypass the gate. +Follow `CONTRIBUTING.md` and `create-github-pr`. Authors without target-repository write access must reference at least one target-repository issue they submitted that has `state:accepted`. Related references and closing links qualify. Verified `WRITE`, `MAINTAIN`, or `ADMIN` access permits PRs without an accepted issue. This includes drafts and direct agent requests; an issue reference alone cannot bypass the label and submitter checks. ## Commits diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4053caa7c0..39072d30a1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -406,9 +406,11 @@ For release notes, describe the user-facing change in a few short, easy-to-read Every PR except an automated dependency update must close an existing issue. In the PR's **Related Issue** section, use `Closes #NNN` for the issue covering that PR's scope. Split multi-PR work into a closable issue for each PR; a tracking issue can link them. Security fixes follow the private disclosure process in [SECURITY.md](SECURITY.md). -Authors without write access to the target repository must link at least one issue with `state:accepted`, and every linked issue must be accepted. This applies to drafts and direct requests to agents. Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may create a PR without the acceptance label; they must still follow the issue-linking requirement above. PR body text, roadmap placement, other labels, and vouch status do not bypass acceptance. +Authors without write access to the target repository must reference at least one issue in that repository with `state:accepted` that they submitted themselves. Related references in the PR description, such as `Related to #NNN`, and closing links both qualify. One qualifying issue is sufficient; other references need not qualify. This applies to drafts and direct requests to agents. Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may create a PR without an accepted issue; they must still follow the issue-linking requirement above. Roadmap placement, other labels, and vouch status do not bypass acceptance. -The `PR Issue Acceptance` workflow reports the native `OpenShell / PR Issue Acceptance` job result for the triggering PR when it is opened, edited, reopened, or updated with new commits. It does not publish a shared commit status; lookup and execution errors fail the job. Link accepted issues in this repository using `Fixes #NNN` or `Closes #NNN`; GitHub recognizes closing references when a PR targets the default branch. Rerun the workflow after issue labels change. Making the job required for merging and configuring any required Actions event policy are separate repository settings; merge-queue support is not included. +Both acceptance checks use `tasks/scripts/check_review_readiness.py`. The `PR Issue Acceptance` workflow reports the native `OpenShell / PR Issue Acceptance` job result when a PR is opened, edited, reopened, or updated with new commits, including drafts. It only reports acceptance and does not change the PR. `Review Readiness` checks the same policy when a ready PR is opened or leaves draft; it moves rejected work to draft before requesting reviewers. Area/topic label changes only reconcile reviewers. + +The checks do not publish a shared commit status; lookup and execution errors fail the job. Description references support `#NNN`, `OWNER/REPO#NNN`, and GitHub issue URLs. Rerun `PR Issue Acceptance` after issue labels change. Making the job required for merging and configuring any required Actions event policy are separate repository settings; merge-queue support is not included. Automated dependency update PRs, including Dependabot PRs, are exempt from the separate issue and closing-reference requirement. Human-authored dependency updates follow the normal issue requirement. The exception does not bypass `PR Issue Acceptance` for authors without repository write access or change applicable verification, review, or private vulnerability disclosure requirements. diff --git a/tasks/scripts/check_issue_acceptance.cjs b/tasks/scripts/check_issue_acceptance.cjs deleted file mode 100644 index 8b81ef82fc..0000000000 --- a/tasks/scripts/check_issue_acceptance.cjs +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -module.exports = async function checkIssueAcceptance({ github, context, core }) { - const { owner, repo } = context.repo; - const { data: pr } = await github.rest.pulls.get({ - owner, repo, pull_number: context.payload.pull_request.number, - }); - const author = pr.user.login; - - async function evaluate() { - try { - const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner, repo, username: author, - }); - // The REST API maps maintain to write and triage to read. - if (['write', 'maintain', 'admin'].includes(data.permission)) { - core.info(`${author} has repository write access; acceptance gate passed.`); - return; - } - } catch (error) { - if (error.status !== 404) throw error; - } - - const issues = []; - let cursor = null; - do { - const result = await github.graphql(` - query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $number) { - closingIssuesReferences(first: 100, after: $cursor) { - nodes { number url repository { name owner { login } } } - pageInfo { hasNextPage endCursor } - } - } - } - } - `, { owner, repo, number: pr.number, cursor }); - const linked = result.repository.pullRequest.closingIssuesReferences; - issues.push(...linked.nodes); - cursor = linked.pageInfo.hasNextPage ? linked.pageInfo.endCursor : null; - } while (cursor); - - if (issues.length === 0) { - return 'Community PRs must link an issue with state:accepted using ' - + 'Fixes #NNN or Closes #NNN.'; - } - - const unaccepted = []; - for (const issue of issues) { - if (`${issue.repository.owner.login}/${issue.repository.name}`.toLowerCase() - !== `${owner}/${repo}`.toLowerCase()) { - return `Acceptance must be recorded in ${owner}/${repo}, not ${issue.url}.`; - } - const labels = await github.paginate(github.rest.issues.listLabelsOnIssue, { - owner: issue.repository.owner.login, - repo: issue.repository.name, - issue_number: issue.number, - per_page: 100, - }); - if (!labels.some(label => label.name === 'state:accepted')) { - unaccepted.push(issue.url); - } - } - - if (unaccepted.length) { - return 'Community PRs require state:accepted on every linked issue. ' - + `Ask a maintainer to accept: ${unaccepted.join(', ')}. ` - + 'After acceptance, rerun this workflow.'; - } - core.info(`All ${issues.length} linked issue(s) have state:accepted.`); - } - - const failure = await evaluate(); - if (failure) core.setFailed(failure); -}; diff --git a/tasks/scripts/check_issue_acceptance.test.cjs b/tasks/scripts/check_issue_acceptance.test.cjs deleted file mode 100644 index 443c8edaf9..0000000000 --- a/tasks/scripts/check_issue_acceptance.test.cjs +++ /dev/null @@ -1,131 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -const assert = require('node:assert/strict'); -const test = require('node:test'); -const checkIssueAcceptance = require('./check_issue_acceptance.cjs'); - -async function check({ - permission = 'read', issues = [], body = '', permissionError, - number = 7, headSha = 'pr-head', pullError, issueError, labelError, -} = {}) { - const failures = []; - const github = { - rest: { - pulls: { - get: async ({ pull_number }) => { - assert.equal(pull_number, number); - if (pullError) throw new Error('PR lookup failed'); - return { data: { - number, user: { login: 'contributor' }, head: { sha: headSha }, body, - } }; - }, - }, - repos: { - getCollaboratorPermissionLevel: async ({ username }) => { - assert.equal(username, 'contributor'); - if (permissionError) throw Object.assign(new Error('lookup failed'), { status: permissionError }); - return { data: { permission } }; - }, - createCommitStatus() { assert.fail('PR decisions must not publish commit statuses'); }, - }, - issues: { listLabelsOnIssue() {} }, - }, - graphql: async (_query, variables) => { - assert.equal(variables.number, number); - if (issueError) throw new Error('Issue lookup failed'); - return { repository: { pullRequest: { closingIssuesReferences: { - nodes: issues.map(issue => ({ - number: issue.number, - url: `https://github.com/${issue.owner || 'NVIDIA'}/OpenShell/issues/${issue.number}`, - repository: { name: 'OpenShell', owner: { login: issue.owner || 'NVIDIA' } }, - })), - pageInfo: { hasNextPage: false, endCursor: null }, - } } } }; - }, - paginate: async (_method, { issue_number }) => { - if (labelError) throw new Error('Label lookup failed'); - return issues.find(issue => issue.number === issue_number).labels || []; - }, - }; - await checkIssueAcceptance({ - github, - context: { - repo: { owner: 'NVIDIA', repo: 'OpenShell' }, - payload: { pull_request: { number } }, - actor: 'maintainer', sha: 'base-commit', serverUrl: 'https://github.com', runId: 42, - }, - core: { info() {}, setFailed: message => failures.push(message) }, - }); - return failures; -} - -for (const permission of ['write', 'maintain', 'admin']) { - test(`${permission} access allows an issue-less PR`, async () => { - assert.deepEqual(await check({ permission }), []); - }); -} - -for (const body of ['', 'No issue required: typo fix', '']) { - test(`community PR without linked issues fails for body ${JSON.stringify(body)}`, async () => { - const failures = await check({ body }); - assert.equal(failures.length, 1); - assert.match(failures[0], /must link an issue with state:accepted/); - }); -} - -test('all linked issues must be accepted, regardless of PR body or triggering actor', async () => { - const failures = await check({ - body: 'No issue required: small fix', - issues: [{ number: 1, labels: [{ name: 'state:accepted' }] }, { number: 2 }], - }); - assert.equal(failures.length, 1); - assert.match(failures[0], /issues\/2/); -}); - -test('accepted issues in the target repository pass', async () => { - assert.deepEqual(await check({ issues: [{ number: 1, labels: [{ name: 'state:accepted' }] }] }), []); -}); - -test('accepted issues in another repository cannot bypass the gate', async () => { - const failures = await check({ issues: [{ number: 1, owner: 'contributor', labels: [{ name: 'state:accepted' }] }] }); - assert.equal(failures.length, 1); - assert.match(failures[0], /Acceptance must be recorded in NVIDIA\/OpenShell/); -}); - -test('a non-collaborator still needs an accepted issue', async () => { - assert.equal((await check({ permissionError: 404, body: 'No issue required: typo fix' })).length, 1); -}); - -test('permission lookup failures do not grant an exemption', async () => { - await assert.rejects(check({ permissionError: 403 }), /lookup failed/); -}); - -for (const [option, message] of [ - ['pullError', /PR lookup failed/], - ['issueError', /Issue lookup failed/], - ['labelError', /Label lookup failed/], -]) { - test(`${option} fails the workflow invocation`, async () => { - await assert.rejects(check({ [option]: true, issues: [{ number: 1 }] }), message); - }); -} - -test('PRs sharing a head SHA evaluate their own issue references in either order', async () => { - const rejected = { number: 7, headSha: 'shared-head', issues: [{ number: 1 }] }; - const accepted = { - number: 8, headSha: 'shared-head', - issues: [{ number: 2, labels: [{ name: 'state:accepted' }] }], - }; - for (const pair of [[rejected, accepted], [accepted, rejected]]) { - const results = await Promise.all(pair.map(input => check(input))); - pair.forEach((input, index) => { - assert.equal(results[index].length, input.number === 7 ? 1 : 0); - }); - } -}); - -test('a failed refresh rejects after an earlier successful evaluation', async () => { - assert.deepEqual(await check({ permission: 'write' }), []); - await assert.rejects(check({ permission: 'write', pullError: true }), /PR lookup failed/); -}); diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py index 9f87d0a179..412da77461 100644 --- a/tasks/scripts/check_review_readiness.py +++ b/tasks/scripts/check_review_readiness.py @@ -10,6 +10,7 @@ """Validate accepted work and reconcile reviewers from the merged REVIEWERS.md. Preview with --repo OWNER/REPO --pr NUMBER; add --apply to update the PR. +Use --acceptance-only to report acceptance without routing or changing the PR. Unaccepted work is moved to draft before any reviewer requests are changed. Request all matching members with write access, excluding the author. Preserve manual choices, unknown request provenance, and anyone who commented or reviewed. @@ -29,6 +30,10 @@ 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) +ACCEPTANCE_ERROR = ( + "PR is not accepted work: the author needs repository write access " + "or a referenced state:accepted issue they submitted themselves" +) @dataclass(frozen=True) @@ -345,10 +350,7 @@ def check_review_readiness( }""", id=pr["node_id"], ) - raise ValueError( - "PR is not accepted work: the author needs repository write access " - "or a referenced state:accepted issue they submitted themselves" - ) + raise ValueError(ACCEPTANCE_ERROR) categories = parse_reviewers(reviewers.read_text(encoding="utf-8")) selected = select_reviewers(categories, {label["name"] for label in pr["labels"]}) selected = _eligible_reviewers(repository, selected - {pr["user"]["login"].lower()}) @@ -381,20 +383,40 @@ def main(argv: list[str] | None = None) -> int: choices=("opened", "ready_for_review", "labeled", "unlabeled"), help="PR event; acceptance is checked on opened and ready_for_review", ) - parser.add_argument( + mode = parser.add_mutually_exclusive_group() + mode.add_argument( + "--acceptance-only", action="store_true", + help="report acceptance without reading the roster or changing the PR", + ) + mode.add_argument( "--apply", action="store_true", help="update requests or draft unaccepted work" ) 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: - plan = check_review_readiness( - args.reviewers, args.repo, args.pr, apply=args.apply, event=args.event - ) + if args.acceptance_only: + owner, repo = args.repo.split("/") + pr = _graphql( + """query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { user: author { login } body } + } + }""", + owner=owner, repo=repo, number=args.pr, + )["repository"]["pullRequest"] + if not _accepted_work(args.repo, args.pr, pr): + raise ValueError(ACCEPTANCE_ERROR) + result = {"accepted": True} + else: + plan = check_review_readiness( + args.reviewers, args.repo, args.pr, apply=args.apply, event=args.event + ) + result = asdict(plan) if plan is not None else {"skipped": "closed or draft PR"} 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"})) + print(json.dumps(result)) return 0 diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py index 224b5b43d8..8256836665 100644 --- a/tasks/scripts/check_review_readiness_test.py +++ b/tasks/scripts/check_review_readiness_test.py @@ -1,7 +1,7 @@ # 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.""" +"""Offline tests for PR acceptance, roster parsing, and reviewer reconciliation.""" from __future__ import annotations @@ -595,6 +595,124 @@ def test_retry_after_partial_failure_uses_updated_github_metadata(self): class CliTests(unittest.TestCase): + def test_both_cli_modes_use_the_same_acceptance_policy(self): + cases = [ + ("WRITE", "", (), {}, True), + ("READ", "Related to #1", (), {1: ("author", True)}, True), + ("READ", "Closes #1", (1,), {1: ("someone-else", True)}, False), + ("READ", "Closes #1 and #2", (1, 2), + {1: ("author", True), 2: ("author", False)}, True), + ("READ", "Related to #1", (), {1: ("author", False)}, False), + ("READ", "", (), {}, False), + ] + for acceptance_only in (False, True): + for permission, body, linked, issues, expected in cases: + with self.subTest(acceptance_only=acceptance_only, body=body, issues=issues), tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "REVIEWERS.md" + path.write_text(DOCUMENT, encoding="utf-8") + value = pr("area:runtime", body=body) + + def api(endpoint, **kwargs): + if endpoint == PULL: + return value + self.assertEqual(endpoint, "graphql") + query = kwargs["data"]["query"] + self.assertNotIn("mutation", query) + if "user: author" in query: + self.assertEqual(kwargs["data"]["variables"]["number"], 42) + return {"data": {"repository": {"pullRequest": value}}} + if "collaborators" in query: + return AcceptanceTests.member(permission) + if "closingIssuesReferences" in query: + return AcceptanceTests.linked(*linked) + self.assertIn("issueOrPullRequest", query) + author, accepted = issues[kwargs["data"]["variables"]["number"]] + return AcceptanceTests.issue(author, accepted) + + output, errors = io.StringIO(), io.StringIO() + with ( + mock.patch.object(routing, "_api", side_effect=api), + mock.patch.object(routing, "_eligible_reviewers", return_value={"alice"}) as eligible, + mock.patch.object(routing, "_load_reviewers", return_value=reviewers()) as load, + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(errors), + ): + argv = ["--repo", REPO, "--pr", "42", "--reviewers", str(path)] + status = routing.main(argv + (["--acceptance-only"] if acceptance_only else [])) + self.assertEqual(status, 0 if expected else 1) + if expected: + self.assertEqual(json.loads(output.getvalue()), + {"accepted": True} if acceptance_only else {"add": ["alice"], "remove": []}) + else: + self.assertIn(routing.ACCEPTANCE_ERROR, errors.getvalue()) + if acceptance_only: + eligible.assert_not_called() + load.assert_not_called() + + def test_acceptance_only_checks_drafts_and_closed_prs_without_routing(self): + for value in (pr(), pr(draft=True), pr(state="closed")): + for accepted in (False, True): + with self.subTest(value=value, accepted=accepted): + output, errors = io.StringIO(), io.StringIO() + with ( + mock.patch.object(routing, "_graphql", return_value={"repository": {"pullRequest": value}}) as query, + mock.patch.object(routing, "_accepted_work", return_value=accepted) as check, + mock.patch.object(routing, "check_review_readiness") as route, + mock.patch.object(routing, "parse_reviewers") as parse, + mock.patch.object(routing.Path, "read_text", side_effect=AssertionError("Acceptance-only must not read the roster")), + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(errors), + ): + status = routing.main(["--repo", REPO, "--pr", "42", "--acceptance-only"]) + self.assertEqual(status, 0 if accepted else 1) + check.assert_called_once_with(REPO, 42, value) + query.assert_called_once() + self.assertNotIn("mutation", query.call_args.args[0]) + route.assert_not_called() + parse.assert_not_called() + + def test_acceptance_only_lookup_errors_fail_without_routing(self): + for metadata_error in (False, True): + error = subprocess.CalledProcessError(1, "gh") + with ( + self.subTest(metadata_error=metadata_error), + mock.patch.object(routing, "_graphql", side_effect=error if metadata_error else None, + return_value={"repository": {"pullRequest": pr()}}), + mock.patch.object(routing, "_accepted_work", side_effect=error) as check, + mock.patch.object(routing, "check_review_readiness") as route, + contextlib.redirect_stdout(io.StringIO()), + contextlib.redirect_stderr(io.StringIO()), + ): + self.assertEqual(routing.main(["--repo", REPO, "--pr", "42", "--acceptance-only"]), 1) + route.assert_not_called() + if metadata_error: + check.assert_not_called() + + def test_acceptance_only_cannot_be_combined_with_apply(self): + with ( + mock.patch.object(routing, "_api") as api, + contextlib.redirect_stderr(io.StringIO()), + self.assertRaises(SystemExit) as error, + ): + routing.main(["--repo", REPO, "--pr", "42", "--acceptance-only", "--apply"]) + self.assertEqual(error.exception.code, 2) + api.assert_not_called() + + def test_acceptance_only_refreshes_metadata_for_each_pr_and_run(self): + for number in (7, 8, 7): + value = pr(body=f"Related to #{number}") + with ( + self.subTest(number=number), + mock.patch.object(routing, "_graphql", return_value={"repository": {"pullRequest": value}}) as query, + mock.patch.object(routing, "_accepted_work", return_value=True) as check, + contextlib.redirect_stdout(io.StringIO()), + ): + self.assertEqual(routing.main([ + "--repo", REPO, "--pr", str(number), "--acceptance-only", + ]), 0) + self.assertEqual(query.call_args.kwargs["number"], number) + check.assert_called_once_with(REPO, number, value) + def test_rejected_pr_does_not_need_a_readable_valid_roster(self): for contents in (None, "not a registry"): for apply in (False, True): @@ -722,5 +840,26 @@ def test_accepted_pr_with_invalid_registry_fails_before_reviewer_changes(self): self.assertIn("no area/topic", errors.getvalue()) +class WorkflowTests(unittest.TestCase): + def test_legacy_workflow_keeps_its_status_events_and_read_only_permissions(self): + root = Path(__file__).resolve().parents[2] + workflow = (root / ".github/workflows/pr-issue-acceptance.yml").read_text() + self.assertIn("name: PR Issue Acceptance\n", workflow) + self.assertIn(" name: OpenShell / PR Issue Acceptance\n", workflow) + self.assertIn("types: [opened, edited, reopened, synchronize]", workflow) + permissions = workflow.split("permissions:\n", 1)[1].split("\n\n", 1)[0] + self.assertEqual(dict(line.strip().split(": ") for line in permissions.splitlines()), + {"contents": "read", "issues": "read", "pull-requests": "read"}) + self.assertIn("ref: ${{ github.sha }}", workflow) + self.assertIn("persist-credentials: false", workflow) + self.assertIn("sparse-checkout: tasks/scripts/check_review_readiness.py", workflow) + self.assertIn("GH_REPO: ${{ github.repository }}", workflow) + self.assertIn("PR_NUMBER: ${{ github.event.pull_request.number }}", workflow) + command = workflow.split(" run: |\n", 1)[1] + self.assertIn("python3 tasks/scripts/check_review_readiness.py", command) + self.assertIn('--repo "$GH_REPO" --pr "$PR_NUMBER" --acceptance-only', command) + self.assertNotIn("--apply", command) + + if __name__ == "__main__": unittest.main() From b2c7b7eaeeabe3108950a6ec9fff590deec8c74f Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Tue, 27 Oct 2026 02:01:44 +0000 Subject: [PATCH 17/18] refactor(review): narrow acceptance changes after review Signed-off-by: Jim Meyer --- .agents/skills/create-github-pr/SKILL.md | 10 ++--- .agents/skills/review-github-pr/SKILL.md | 15 ++++--- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/pr-issue-acceptance.yml | 3 +- AGENTS.md | 2 +- CONTRIBUTING.md | 6 +-- tasks/scripts/check_review_readiness.py | 42 ++++++++++---------- tasks/scripts/check_review_readiness_test.py | 40 ++++++++++++++++--- 8 files changed, 70 insertions(+), 50 deletions(-) diff --git a/.agents/skills/create-github-pr/SKILL.md b/.agents/skills/create-github-pr/SKILL.md index 9b20dec9de..f564bd8d00 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -24,16 +24,14 @@ to the target repository, not their fork. For the authenticated `gh` user: ```bash gh repo view OWNER/REPO --json nameWithOwner,viewerPermission -gh issue view ISSUE_NUMBER --repo OWNER/REPO --json number,url,labels,author +gh issue view ISSUE_NUMBER --repo OWNER/REPO --json number,url,labels ``` Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may proceed without `state:accepted`; keep the issue link and note the missing label. Everyone else -must reference at least one target-repository issue they submitted themselves -that has `state:accepted`. Related description references and closing links both -qualify; other references need not qualify. Otherwise, preserve local work and -report the missing qualification. A direct request, roadmap placement, other -labels, or vouch status does not bypass this gate. +must link at least one target-repository issue and have `state:accepted` on each. +Otherwise, preserve local work and report which issue needs acceptance. A direct +request, roadmap placement, other labels, or vouch status does not bypass this gate. When acting for another author, verify their permissions. Resolve failed lookups before proceeding; retry sandbox network failures outside the sandbox before diff --git a/.agents/skills/review-github-pr/SKILL.md b/.agents/skills/review-github-pr/SKILL.md index e98962d959..9813402c76 100644 --- a/.agents/skills/review-github-pr/SKILL.md +++ b/.agents/skills/review-github-pr/SKILL.md @@ -46,19 +46,18 @@ Record the **title**, **body**, **headRefName**, and **baseRefName** for use in ### Check Issue Acceptance -Inspect closing issue references (paginate as needed) and issue references in the -PR description. Fetch their current repository, labels, and submitter. Verify the -PR author's permission in the target repository: +Inspect every closing issue reference (paginate as needed) and fetch its current +repository and labels. Verify the PR author's permission in the target repository: ```bash gh api repos/OWNER/REPO/collaborators/AUTHOR/permission --jq .permission ``` -Verified `write`, `maintain`, or `admin` access permits the exception; report it as -context, not a blocking concern. Other authors need at least one referenced issue -in the target repository with `state:accepted` submitted by the PR author. Other -references need not qualify. Report violations under **Potential Concerns**. PR -body text such as `No issue required` does not bypass this gate. +Call out each linked issue missing `state:accepted`, with an issue link. Verified +`write`, `maintain`, or `admin` access permits the exception; report it as context, +not a blocking concern. Other authors need at least one target-repository issue, +and every linked issue must be accepted; report violations under **Potential +Concerns**. PR body text such as `No issue required` does not bypass this gate. Report failed lookups as unverified and continue the code review. For a branch without a PR, note that issue acceptance was not verified. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index f50d747c2d..5cb3dd2efd 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -2,7 +2,7 @@ ## Related Issue - + ## Changes diff --git a/.github/workflows/pr-issue-acceptance.yml b/.github/workflows/pr-issue-acceptance.yml index 1e78c0d544..5c3c27ad3b 100644 --- a/.github/workflows/pr-issue-acceptance.yml +++ b/.github/workflows/pr-issue-acceptance.yml @@ -39,4 +39,5 @@ jobs: PR_NUMBER: ${{ github.event.pull_request.number }} run: | python3 tasks/scripts/check_review_readiness.py \ - --repo "$GH_REPO" --pr "$PR_NUMBER" --acceptance-only + --repo "$GH_REPO" --pr "$PR_NUMBER" --acceptance-only \ + --event-path "$GITHUB_EVENT_PATH" diff --git a/AGENTS.md b/AGENTS.md index 16c0581532..1235fe1183 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -176,7 +176,7 @@ Follow the [branch naming convention in CONTRIBUTING.md](CONTRIBUTING.md#branch- ## Pull Requests -Follow `CONTRIBUTING.md` and `create-github-pr`. Authors without target-repository write access must reference at least one target-repository issue they submitted that has `state:accepted`. Related references and closing links qualify. Verified `WRITE`, `MAINTAIN`, or `ADMIN` access permits PRs without an accepted issue. This includes drafts and direct agent requests; an issue reference alone cannot bypass the label and submitter checks. +Follow `CONTRIBUTING.md` and `create-github-pr`. Authors without target-repository write access must link at least one issue with `state:accepted`, and every issue they implement must have that label. Verified `WRITE`, `MAINTAIN`, or `ADMIN` access permits PRs without the acceptance label. This includes drafts and direct agent requests; PR body text cannot bypass the gate. ## Commits diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39072d30a1..3e93a954f0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -406,11 +406,7 @@ For release notes, describe the user-facing change in a few short, easy-to-read Every PR except an automated dependency update must close an existing issue. In the PR's **Related Issue** section, use `Closes #NNN` for the issue covering that PR's scope. Split multi-PR work into a closable issue for each PR; a tracking issue can link them. Security fixes follow the private disclosure process in [SECURITY.md](SECURITY.md). -Authors without write access to the target repository must reference at least one issue in that repository with `state:accepted` that they submitted themselves. Related references in the PR description, such as `Related to #NNN`, and closing links both qualify. One qualifying issue is sufficient; other references need not qualify. This applies to drafts and direct requests to agents. Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may create a PR without an accepted issue; they must still follow the issue-linking requirement above. Roadmap placement, other labels, and vouch status do not bypass acceptance. - -Both acceptance checks use `tasks/scripts/check_review_readiness.py`. The `PR Issue Acceptance` workflow reports the native `OpenShell / PR Issue Acceptance` job result when a PR is opened, edited, reopened, or updated with new commits, including drafts. It only reports acceptance and does not change the PR. `Review Readiness` checks the same policy when a ready PR is opened or leaves draft; it moves rejected work to draft before requesting reviewers. Area/topic label changes only reconcile reviewers. - -The checks do not publish a shared commit status; lookup and execution errors fail the job. Description references support `#NNN`, `OWNER/REPO#NNN`, and GitHub issue URLs. Rerun `PR Issue Acceptance` after issue labels change. Making the job required for merging and configuring any required Actions event policy are separate repository settings; merge-queue support is not included. +Authors without write access to the target repository must link at least one issue with `state:accepted`, and every linked issue must be accepted. This applies to drafts and direct requests to agents. Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may create a PR without the acceptance label; they must still follow the issue-linking requirement above. PR body text, roadmap placement, other labels, and vouch status do not bypass acceptance. Automated dependency update PRs, including Dependabot PRs, are exempt from the separate issue and closing-reference requirement. Human-authored dependency updates follow the normal issue requirement. The exception does not bypass `PR Issue Acceptance` for authors without repository write access or change applicable verification, review, or private vulnerability disclosure requirements. diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py index 412da77461..afbeb45099 100644 --- a/tasks/scripts/check_review_readiness.py +++ b/tasks/scripts/check_review_readiness.py @@ -7,15 +7,7 @@ # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Validate accepted work and reconcile reviewers from the merged REVIEWERS.md. - -Preview with --repo OWNER/REPO --pr NUMBER; add --apply to update the PR. -Use --acceptance-only to report acceptance without routing or changing the PR. -Unaccepted work is moved to draft before any reviewer requests are changed. -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. -""" +"""Ensure pull requests are ready for review by the appropriate people.""" from __future__ import annotations @@ -32,7 +24,7 @@ PROFILE = re.compile(r"\[@([a-z0-9-]+)\]\(https://github\.com/([a-z0-9-]+)/?\)", re.I) ACCEPTANCE_ERROR = ( "PR is not accepted work: the author needs repository write access " - "or a referenced state:accepted issue they submitted themselves" + "or a referenced state:accepted issue" ) @@ -251,7 +243,7 @@ def _referenced_issues(repository: str, number: int, body: str): def _accepted_work(repository: str, number: int, pr: dict) -> bool: - """Accept write access or a referenced accepted issue filed by the PR author.""" + """Accept write access or a referenced accepted issue.""" owner, repo = repository.split("/") author = pr["user"]["login"].lower() cursor = None @@ -290,7 +282,6 @@ def _accepted_work(repository: str, number: int, pr: dict) -> bool: repository(owner: $owner, name: $repo) { issueOrPullRequest(number: $number) { ... on Issue { - author { login } labels(first: 100, after: $cursor) { nodes { name } pageInfo { hasNextPage endCursor } @@ -307,7 +298,7 @@ def _accepted_work(repository: str, number: int, pr: dict) -> bool: break raise issue = data["repository"]["issueOrPullRequest"] - if not issue or (issue.get("author") or {}).get("login", "").lower() != author: + if not issue or "labels" not in issue: break if any(label["name"] == "state:accepted" for label in issue["labels"]["nodes"]): return True @@ -378,6 +369,10 @@ def main(argv: list[str] | None = None) -> int: 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( + "--event-path", type=Path, + help="PR event payload for --acceptance-only; otherwise fetch current PR metadata", + ) parser.add_argument( "--event", default="opened", choices=("opened", "ready_for_review", "labeled", "unlabeled"), @@ -396,15 +391,18 @@ def main(argv: list[str] | None = None) -> int: parser.error("--repo must be OWNER/REPO and --pr must be positive") try: if args.acceptance_only: - owner, repo = args.repo.split("/") - pr = _graphql( - """query($owner: String!, $repo: String!, $number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $number) { user: author { login } body } - } - }""", - owner=owner, repo=repo, number=args.pr, - )["repository"]["pullRequest"] + if args.event_path: + pr = json.loads(args.event_path.read_text(encoding="utf-8"))["pull_request"] + else: + owner, repo = args.repo.split("/") + pr = _graphql( + """query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { user: author { login } body } + } + }""", + owner=owner, repo=repo, number=args.pr, + )["repository"]["pullRequest"] if not _accepted_work(args.repo, args.pr, pr): raise ValueError(ACCEPTANCE_ERROR) result = {"accepted": True} diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py index 8256836665..e2d160bba3 100644 --- a/tasks/scripts/check_review_readiness_test.py +++ b/tasks/scripts/check_review_readiness_test.py @@ -302,13 +302,14 @@ def test_qualifying_closing_reference_skips_later_pages(self): self.assertTrue(routing._accepted_work(REPO, 42, pr())) self.assertEqual(api.call_count, 3) - def test_description_and_closing_references_require_same_submitter_and_label(self): + def test_description_and_closing_references_require_acceptance_not_same_submitter(self): for body in ( "", "Related to #1", "Related to owner/repo#1", "Related to OWNER/REPO#1", "Related to https://github.com/owner/repo/issues/1", ): for author, accepted, expected in ( - ("AUTHOR", True, True), ("someone-else", True, False), ("author", False, False), + ("AUTHOR", True, True), ("someone-else", True, True), + ("author", False, False), ("someone-else", False, False), ): with ( self.subTest(body=body, author=author, accepted=accepted), @@ -599,15 +600,16 @@ def test_both_cli_modes_use_the_same_acceptance_policy(self): cases = [ ("WRITE", "", (), {}, True), ("READ", "Related to #1", (), {1: ("author", True)}, True), - ("READ", "Closes #1", (1,), {1: ("someone-else", True)}, False), + ("READ", "Closes #1", (1,), {1: ("someone-else", True)}, True), ("READ", "Closes #1 and #2", (1, 2), {1: ("author", True), 2: ("author", False)}, True), ("READ", "Related to #1", (), {1: ("author", False)}, False), ("READ", "", (), {}, False), ] - for acceptance_only in (False, True): + for mode in ("preview", "acceptance-api", "acceptance-event"): + acceptance_only = mode != "preview" for permission, body, linked, issues, expected in cases: - with self.subTest(acceptance_only=acceptance_only, body=body, issues=issues), tempfile.TemporaryDirectory() as directory: + with self.subTest(mode=mode, body=body, issues=issues), tempfile.TemporaryDirectory() as directory: path = Path(directory) / "REVIEWERS.md" path.write_text(DOCUMENT, encoding="utf-8") value = pr("area:runtime", body=body) @@ -619,6 +621,7 @@ def api(endpoint, **kwargs): query = kwargs["data"]["query"] self.assertNotIn("mutation", query) if "user: author" in query: + self.assertNotEqual(mode, "acceptance-event") self.assertEqual(kwargs["data"]["variables"]["number"], 42) return {"data": {"repository": {"pullRequest": value}}} if "collaborators" in query: @@ -638,7 +641,13 @@ def api(endpoint, **kwargs): contextlib.redirect_stderr(errors), ): argv = ["--repo", REPO, "--pr", "42", "--reviewers", str(path)] - status = routing.main(argv + (["--acceptance-only"] if acceptance_only else [])) + if acceptance_only: + argv.append("--acceptance-only") + if mode == "acceptance-event": + event_path = Path(directory) / "event.json" + event_path.write_text(json.dumps({"pull_request": value})) + argv.extend(["--event-path", str(event_path)]) + status = routing.main(argv) self.assertEqual(status, 0 if expected else 1) if expected: self.assertEqual(json.loads(output.getvalue()), @@ -688,6 +697,24 @@ def test_acceptance_only_lookup_errors_fail_without_routing(self): if metadata_error: check.assert_not_called() + def test_invalid_event_payload_fails_without_api_calls_or_routing(self): + for contents in (None, "not JSON"): + with self.subTest(contents=contents), tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "event.json" + if contents is not None: + path.write_text(contents) + with ( + mock.patch.object(routing, "_api") as api, + mock.patch.object(routing, "check_review_readiness") as route, + contextlib.redirect_stderr(io.StringIO()), + ): + self.assertEqual(routing.main([ + "--repo", REPO, "--pr", "42", "--acceptance-only", + "--event-path", str(path), + ]), 1) + api.assert_not_called() + route.assert_not_called() + def test_acceptance_only_cannot_be_combined_with_apply(self): with ( mock.patch.object(routing, "_api") as api, @@ -858,6 +885,7 @@ def test_legacy_workflow_keeps_its_status_events_and_read_only_permissions(self) command = workflow.split(" run: |\n", 1)[1] self.assertIn("python3 tasks/scripts/check_review_readiness.py", command) self.assertIn('--repo "$GH_REPO" --pr "$PR_NUMBER" --acceptance-only', command) + self.assertIn('--event-path "$GITHUB_EVENT_PATH"', command) self.assertNotIn("--apply", command) From 130563eda973991c55df3ca3d6185e2f42e91ab5 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Tue, 27 Oct 2026 05:13:27 +0000 Subject: [PATCH 18/18] feat(review): explain automatic draft conversion once Signed-off-by: Jim Meyer --- tasks/scripts/check_review_readiness.py | 52 +++- tasks/scripts/check_review_readiness_test.py | 248 +++++++++---------- 2 files changed, 160 insertions(+), 140 deletions(-) diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py index afbeb45099..3dbb9b8e51 100644 --- a/tasks/scripts/check_review_readiness.py +++ b/tasks/scripts/check_review_readiness.py @@ -24,7 +24,15 @@ PROFILE = re.compile(r"\[@([a-z0-9-]+)\]\(https://github\.com/([a-z0-9-]+)/?\)", re.I) ACCEPTANCE_ERROR = ( "PR is not accepted work: the author needs repository write access " - "or a referenced state:accepted issue" + "or state:accepted on the PR or a referenced issue" +) +DRAFT_COMMENT_MARKER = "" +DRAFT_COMMENT = ( + "This PR has been automatically moved to draft because it's not associated " + "with an issue that is accepted work. You're welcome to continue to work on " + "it while it's in draft mode; it will be allowed to move to \"Ready for Review\" " + "when the issue or PR have been marked accepted.\n\n" + + DRAFT_COMMENT_MARKER ) @@ -243,7 +251,9 @@ def _referenced_issues(repository: str, number: int, body: str): def _accepted_work(repository: str, number: int, pr: dict) -> bool: - """Accept write access or a referenced accepted issue.""" + """Accept write access or acceptance recorded on the PR or a referenced issue.""" + if any(label["name"] == "state:accepted" for label in pr["labels"]): + return True owner, repo = repository.split("/") author = pr["user"]["login"].lower() cursor = None @@ -341,6 +351,13 @@ def check_review_readiness( }""", id=pr["node_id"], ) + comments = f"repos/{repository}/issues/{number}/comments" + previous = _api(f"{comments}?per_page=100", pages=True) + if not any( + DRAFT_COMMENT_MARKER in (comment.get("body") or "") + for comment in previous + ): + _api(comments, method="POST", data={"body": DRAFT_COMMENT}) raise ValueError(ACCEPTANCE_ERROR) categories = parse_reviewers(reviewers.read_text(encoding="utf-8")) selected = select_reviewers(categories, {label["name"] for label in pr["labels"]}) @@ -395,14 +412,29 @@ def main(argv: list[str] | None = None) -> int: pr = json.loads(args.event_path.read_text(encoding="utf-8"))["pull_request"] else: owner, repo = args.repo.split("/") - pr = _graphql( - """query($owner: String!, $repo: String!, $number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $number) { user: author { login } body } - } - }""", - owner=owner, repo=repo, number=args.pr, - )["repository"]["pullRequest"] + cursor, labels = None, [] + while True: + pr = _graphql( + """query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + user: author { login } + body + labels(first: 100, after: $cursor) { + nodes { name } + pageInfo { hasNextPage endCursor } + } + } + } + }""", + owner=owner, repo=repo, number=args.pr, cursor=cursor, + )["repository"]["pullRequest"] + page = pr["labels"] + labels.extend(page["nodes"]) + if not page["pageInfo"]["hasNextPage"]: + break + cursor = page["pageInfo"]["endCursor"] + pr["labels"] = labels if not _accepted_work(args.repo, args.pr, pr): raise ValueError(ACCEPTANCE_ERROR) result = {"accepted": True} diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py index e2d160bba3..b43b036bc7 100644 --- a/tasks/scripts/check_review_readiness_test.py +++ b/tasks/scripts/check_review_readiness_test.py @@ -42,6 +42,16 @@ def pr(*labels, requested=(), author="author", draft=False, state="open", body=" } +def graphql_pr(value, *, cursor=None): + return {"repository": {"pullRequest": { + **value, + "labels": { + "nodes": value["labels"], + "pageInfo": {"hasNextPage": cursor is not None, "endCursor": cursor}, + }, + }}} + + def reviewers(*, requested=(), engaged=(), manual=()): return {"requested": set(requested), "engaged": set(engaged), "manual": set(manual)} @@ -197,14 +207,6 @@ def test_new_automatic_request_after_removal_is_not_protected(self): class GitHubTests(unittest.TestCase): - def test_graphql_passes_variables_and_unwraps_response(self): - with mock.patch.object(routing, "_api", return_value={"data": {"value": 1}}) as api: - self.assertEqual(routing._graphql("query", number=42), {"value": 1}) - api.assert_called_once_with( - "graphql", method="POST", - data={"query": "query", "variables": {"number": 42}}, - ) - def test_api_reads_all_pages(self): with mock.patch.object(routing.subprocess, "run") as run: run.return_value.stdout = json.dumps( @@ -282,6 +284,11 @@ def test_write_maintain_and_admin_return_without_looking_up_issues(self): api.assert_called_once() self.assertNotIn("pullRequest", api.call_args.kwargs["data"]["query"]) + def test_acceptance_recorded_on_pr_needs_no_further_lookups(self): + with mock.patch.object(routing, "_api") as api: + self.assertTrue(routing._accepted_work(REPO, 42, pr("state:accepted"))) + api.assert_not_called() + def test_collaborator_search_result_must_match_the_pr_author(self): with mock.patch.object(routing, "_api", side_effect=[ self.member("ADMIN", login="someone-else"), self.linked(), @@ -397,22 +404,75 @@ def test_missing_description_reference_falls_back_to_qualifying_closing_link(sel ]): self.assertTrue(routing._accepted_work(REPO, 42, pr(body="Related to #1"))) - def test_creation_and_ready_transition_reject_before_reviewer_changes(self): - for event in ("opened", "ready_for_review"): - for apply in (False, True): - with ( - self.subTest(event=event, apply=apply), - mock.patch.object(routing, "_api", side_effect=[pr(), DRAFTED]) as api, - mock.patch.object(routing, "_accepted_work", return_value=False), - mock.patch.object(routing, "_load_reviewers") as load, - ): - with self.assertRaisesRegex(ValueError, "not accepted work"): - routing.check_review_readiness(ROSTER, REPO, 42, apply=apply, event=event) - load.assert_not_called() - self.assertEqual(api.call_count, 2 if apply else 1) - if apply: - self.assertIn("convertPullRequestToDraft", api.call_args.kwargs["data"]["query"]) - self.assertEqual(api.call_args.kwargs["data"]["variables"], {"id": "PR_node_42"}) + def test_first_draft_conversion_comments_once_and_subsequent_attempts_are_silent(self): + comments = f"repos/{REPO}/issues/42/comments" + posted = {"user": {"type": "Bot"}, "body": ""} + with ( + mock.patch.object(routing, "_api", side_effect=[ + pr(), DRAFTED, [], posted, + pr(), DRAFTED, [posted], + pr(), DRAFTED, [posted], + ]) as api, + mock.patch.object(routing, "_accepted_work", return_value=False), + mock.patch.object(routing, "_load_reviewers") as load, + ): + for attempt in range(3): + with self.assertRaisesRegex(ValueError, "not accepted work"): + routing.check_review_readiness( + ROSTER, REPO, 42, apply=True, event="ready_for_review" + ) + if attempt == 0: + posted["body"] = api.call_args.kwargs["data"]["body"] + self.assertEqual(sum( + "convertPullRequestToDraft" in call.kwargs.get("data", {}).get("query", "") + for call in api.call_args_list + ), 3) + self.assertEqual(sum( + call.args[0] == comments and call.kwargs.get("method") == "POST" + for call in api.call_args_list + ), 1) + load.assert_not_called() + + def test_existing_notice_suppresses_comment_and_all_pages_are_read(self): + comments = f"repos/{REPO}/issues/42/comments" + for previous, should_post in ( + ([{"user": {"type": "User"}, "body": routing.DRAFT_COMMENT}], False), + ([{"user": {"type": "Bot"}, "body": "unrelated comment"}], True), + ([{"user": None, "body": None}], True), + ([{"user": {"type": "Bot"}, "body": None}] * 100 + [ + {"user": {"type": "Bot"}, "body": routing.DRAFT_COMMENT} + ], False), + ): + with ( + self.subTest(previous=previous, should_post=should_post), + mock.patch.object(routing, "_api", side_effect=[ + pr(), DRAFTED, previous, {}, + ]) as api, + mock.patch.object(routing, "_accepted_work", return_value=False), + self.assertRaisesRegex(ValueError, "not accepted work"), + ): + routing.check_review_readiness(ROSTER, REPO, 42, apply=True) + self.assertIn(mock.call(f"{comments}?per_page=100", pages=True), api.call_args_list) + self.assertEqual(any( + call.kwargs.get("method") == "POST" and call.args[0] == comments + for call in api.call_args_list + ), should_post) + + def test_comment_lookup_or_post_failure_still_leaves_pr_drafted(self): + for posting in (False, True): + error = subprocess.CalledProcessError(1, "gh") + with ( + self.subTest(posting=posting), + mock.patch.object(routing, "_api", side_effect=( + [pr(), DRAFTED, [], error] if posting else [pr(), DRAFTED, error] + )) as api, + mock.patch.object(routing, "_accepted_work", return_value=False), + mock.patch.object(routing, "_load_reviewers") as load, + self.assertRaises(subprocess.CalledProcessError), + ): + routing.check_review_readiness(ROSTER, REPO, 42, apply=True) + self.assertIn("convertPullRequestToDraft", api.call_args_list[1].kwargs["data"]["query"]) + load.assert_not_called() def test_lookup_errors_fail_without_drafting_or_requesting_reviewers(self): with ( @@ -496,17 +556,6 @@ def test_label_changes_reconcile_without_rechecking_acceptance(self): self.assertEqual(plan.add, ("dave",)) accepted.assert_not_called() - 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, @@ -598,21 +647,22 @@ def test_retry_after_partial_failure_uses_updated_github_metadata(self): class CliTests(unittest.TestCase): def test_both_cli_modes_use_the_same_acceptance_policy(self): cases = [ - ("WRITE", "", (), {}, True), - ("READ", "Related to #1", (), {1: ("author", True)}, True), - ("READ", "Closes #1", (1,), {1: ("someone-else", True)}, True), + ("WRITE", "", (), {}, (), True), + ("READ", "", (), {}, ("state:accepted",), True), + ("READ", "Related to #1", (), {1: ("author", True)}, (), True), + ("READ", "Closes #1", (1,), {1: ("someone-else", True)}, (), True), ("READ", "Closes #1 and #2", (1, 2), - {1: ("author", True), 2: ("author", False)}, True), - ("READ", "Related to #1", (), {1: ("author", False)}, False), - ("READ", "", (), {}, False), + {1: ("author", True), 2: ("author", False)}, (), True), + ("READ", "Related to #1", (), {1: ("author", False)}, (), False), + ("READ", "", (), {}, (), False), ] for mode in ("preview", "acceptance-api", "acceptance-event"): acceptance_only = mode != "preview" - for permission, body, linked, issues, expected in cases: - with self.subTest(mode=mode, body=body, issues=issues), tempfile.TemporaryDirectory() as directory: + for permission, body, linked, issues, labels, expected in cases: + with self.subTest(mode=mode, body=body, issues=issues, labels=labels), tempfile.TemporaryDirectory() as directory: path = Path(directory) / "REVIEWERS.md" path.write_text(DOCUMENT, encoding="utf-8") - value = pr("area:runtime", body=body) + value = pr("area:runtime", *labels, body=body) def api(endpoint, **kwargs): if endpoint == PULL: @@ -623,7 +673,7 @@ def api(endpoint, **kwargs): if "user: author" in query: self.assertNotEqual(mode, "acceptance-event") self.assertEqual(kwargs["data"]["variables"]["number"], 42) - return {"data": {"repository": {"pullRequest": value}}} + return {"data": graphql_pr(value)} if "collaborators" in query: return AcceptanceTests.member(permission) if "closingIssuesReferences" in query: @@ -658,13 +708,31 @@ def api(endpoint, **kwargs): eligible.assert_not_called() load.assert_not_called() + def test_acceptance_only_reads_later_pr_label_pages(self): + output = io.StringIO() + with ( + mock.patch.object(routing, "_graphql", side_effect=[ + graphql_pr(pr("topic:policy"), cursor="next-page"), + graphql_pr(pr("state:accepted")), + ]) as query, + mock.patch.object(routing, "_api", side_effect=AssertionError("No further lookups")), + mock.patch.object(routing, "check_review_readiness") as route, + contextlib.redirect_stdout(output), + ): + self.assertEqual(routing.main([ + "--repo", REPO, "--pr", "42", "--acceptance-only", + ]), 0) + self.assertEqual(json.loads(output.getvalue()), {"accepted": True}) + self.assertEqual(query.call_args.kwargs["cursor"], "next-page") + route.assert_not_called() + def test_acceptance_only_checks_drafts_and_closed_prs_without_routing(self): for value in (pr(), pr(draft=True), pr(state="closed")): for accepted in (False, True): with self.subTest(value=value, accepted=accepted): output, errors = io.StringIO(), io.StringIO() with ( - mock.patch.object(routing, "_graphql", return_value={"repository": {"pullRequest": value}}) as query, + mock.patch.object(routing, "_graphql", return_value=graphql_pr(value)) as query, mock.patch.object(routing, "_accepted_work", return_value=accepted) as check, mock.patch.object(routing, "check_review_readiness") as route, mock.patch.object(routing, "parse_reviewers") as parse, @@ -686,7 +754,7 @@ def test_acceptance_only_lookup_errors_fail_without_routing(self): with ( self.subTest(metadata_error=metadata_error), mock.patch.object(routing, "_graphql", side_effect=error if metadata_error else None, - return_value={"repository": {"pullRequest": pr()}}), + return_value=graphql_pr(pr())), mock.patch.object(routing, "_accepted_work", side_effect=error) as check, mock.patch.object(routing, "check_review_readiness") as route, contextlib.redirect_stdout(io.StringIO()), @@ -725,21 +793,6 @@ def test_acceptance_only_cannot_be_combined_with_apply(self): self.assertEqual(error.exception.code, 2) api.assert_not_called() - def test_acceptance_only_refreshes_metadata_for_each_pr_and_run(self): - for number in (7, 8, 7): - value = pr(body=f"Related to #{number}") - with ( - self.subTest(number=number), - mock.patch.object(routing, "_graphql", return_value={"repository": {"pullRequest": value}}) as query, - mock.patch.object(routing, "_accepted_work", return_value=True) as check, - contextlib.redirect_stdout(io.StringIO()), - ): - self.assertEqual(routing.main([ - "--repo", REPO, "--pr", str(number), "--acceptance-only", - ]), 0) - self.assertEqual(query.call_args.kwargs["number"], number) - check.assert_called_once_with(REPO, number, value) - def test_rejected_pr_does_not_need_a_readable_valid_roster(self): for contents in (None, "not a registry"): for apply in (False, True): @@ -749,7 +802,7 @@ def test_rejected_pr_does_not_need_a_readable_valid_roster(self): path.write_text(contents, encoding="utf-8") output, errors = io.StringIO(), io.StringIO() with ( - mock.patch.object(routing, "_api", side_effect=[pr(), DRAFTED]) as api, + mock.patch.object(routing, "_api", side_effect=[pr(), DRAFTED, [], {}]) as api, mock.patch.object(routing, "_accepted_work", return_value=False), mock.patch.object(routing, "parse_reviewers") as parse, mock.patch.object(routing, "_load_reviewers") as load, @@ -762,9 +815,9 @@ def test_rejected_pr_does_not_need_a_readable_valid_roster(self): self.assertEqual(output.getvalue(), "") parse.assert_not_called() load.assert_not_called() - self.assertEqual(api.call_count, 2 if apply else 1) + self.assertEqual(api.call_count, 4 if apply else 1) if apply: - self.assertIn("convertPullRequestToDraft", api.call_args.kwargs["data"]["query"]) + self.assertIn("convertPullRequestToDraft", api.call_args_list[1].kwargs["data"]["query"]) def test_accepted_pr_reads_real_roster_and_previews_reviewers(self): with tempfile.TemporaryDirectory() as directory: @@ -802,49 +855,6 @@ def test_closed_and_draft_prs_skip_missing_roster_and_acceptance(self): accepted.assert_not_called() self.assertEqual(json.loads(output.getvalue()), {"skipped": "closed or draft PR"}) - def test_passes_label_event_to_reconciliation(self): - with tempfile.TemporaryDirectory() as directory: - path = Path(directory) / "REVIEWERS.md" - path.write_text(DOCUMENT, encoding="utf-8") - with ( - mock.patch.object( - routing, "check_review_readiness", - return_value=routing.ReviewPlan((), ()), - ) as check, - contextlib.redirect_stdout(io.StringIO()), - ): - self.assertEqual(routing.main([ - "--reviewers", str(path), "--repo", REPO, "--pr", "42", - "--event", "labeled", "--apply", - ]), 0) - self.assertEqual(check.call_args.kwargs, {"apply": True, "event": "labeled"}) - - 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( - path, REPO, 42, apply=True, event="opened" - ) - def test_accepted_pr_with_invalid_registry_fails_before_reviewer_changes(self): with tempfile.TemporaryDirectory() as directory: path = Path(directory) / "REVIEWERS.md" @@ -867,27 +877,5 @@ def test_accepted_pr_with_invalid_registry_fails_before_reviewer_changes(self): self.assertIn("no area/topic", errors.getvalue()) -class WorkflowTests(unittest.TestCase): - def test_legacy_workflow_keeps_its_status_events_and_read_only_permissions(self): - root = Path(__file__).resolve().parents[2] - workflow = (root / ".github/workflows/pr-issue-acceptance.yml").read_text() - self.assertIn("name: PR Issue Acceptance\n", workflow) - self.assertIn(" name: OpenShell / PR Issue Acceptance\n", workflow) - self.assertIn("types: [opened, edited, reopened, synchronize]", workflow) - permissions = workflow.split("permissions:\n", 1)[1].split("\n\n", 1)[0] - self.assertEqual(dict(line.strip().split(": ") for line in permissions.splitlines()), - {"contents": "read", "issues": "read", "pull-requests": "read"}) - self.assertIn("ref: ${{ github.sha }}", workflow) - self.assertIn("persist-credentials: false", workflow) - self.assertIn("sparse-checkout: tasks/scripts/check_review_readiness.py", workflow) - self.assertIn("GH_REPO: ${{ github.repository }}", workflow) - self.assertIn("PR_NUMBER: ${{ github.event.pull_request.number }}", workflow) - command = workflow.split(" run: |\n", 1)[1] - self.assertIn("python3 tasks/scripts/check_review_readiness.py", command) - self.assertIn('--repo "$GH_REPO" --pr "$PR_NUMBER" --acceptance-only', command) - self.assertIn('--event-path "$GITHUB_EVENT_PATH"', command) - self.assertNotIn("--apply", command) - - if __name__ == "__main__": unittest.main()