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