diff --git a/.agents/skills/create-github-issue/SKILL.md b/.agents/skills/create-github-issue/SKILL.md index 25a9cbc4c9..30152e0ad3 100644 --- a/.agents/skills/create-github-issue/SKILL.md +++ b/.agents/skills/create-github-issue/SKILL.md @@ -137,6 +137,12 @@ GitHub built-in issue types (`Bug`, `Feature`, `Task`) should come from the matc Creating an issue does not accept it. Follow Label Discovery in `CONTRIBUTING.md` to retrieve every page of current `state:*` label definitions and resolve unclear meanings, then follow its triage → validation → human acceptance process. Agents may assess facts, but only humans decide whether to accept work or place it on the roadmap. A direct user request authorizes the requested planning or implementation phase without changing issue disposition. +Follow [Labeling and Notifications](../../../CONTRIBUTING.md#labeling-and-notifications) +for classification labels. Defer area/topic labels until the work is accepted; +filing an issue or identifying its subject is not acceptance. Intake workflow +labels may still record triage status. Classification can trigger subscribers' +GitHub app in Slack and other integration notifications. + ## Useful Options | Option | Description | diff --git a/.agents/skills/create-github-pr/SKILL.md b/.agents/skills/create-github-pr/SKILL.md index 59ab0dade0..957dc8bd60 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -258,6 +258,11 @@ automatically on issue-label events. Created PR [#123](https://github.com/OWNER/REPO/pull/123) ``` +Follow the shared [accepted-work definition](../../../CONTRIBUTING.md#accepted-work) +and [Labeling and Notifications](../../../CONTRIBUTING.md#labeling-and-notifications) +when applying classification labels. Review-readiness applies the acceptance label +to qualifying PRs when they open; this does not replace existing PR requirements. + ### Monitor Workflow Run (Optional) If the user asks to wait for a green CI before posting the RFR, use this snippet to monitor the workflow run: diff --git a/.agents/skills/triage-issue/SKILL.md b/.agents/skills/triage-issue/SKILL.md index a341a56540..cf50164bb9 100644 --- a/.agents/skills/triage-issue/SKILL.md +++ b/.agents/skills/triage-issue/SKILL.md @@ -15,6 +15,11 @@ For a specific issue, assess it when requested. For batch triage, first show the Do not apply an acceptance state, place issues on the roadmap, or make a product investment decision. Do not introduce `agent:*` workflow labels. Follow `SECURITY.md` if the report may disclose a vulnerability; do not copy sensitive details into a public comment. +Follow [Labeling and Notifications](../../../CONTRIBUTING.md#labeling-and-notifications): +defer area/topic classification labels until the work is accepted. They drive +subscriber notifications, so technical classification alone must not send +unaccepted intake to those streams. Continue to record appropriate triage state. + ## Assess one issue 1. Run `gh issue view --json number,title,body,state,labels,author,comments`. Follow Label Discovery in `CONTRIBUTING.md` to retrieve every page of current `state:*` label definitions and resolve unclear meanings. Check for previous triage, new human evidence, an existing owner, and duplicate issues. diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index e9899931bd..169edf36c6 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -40,5 +40,9 @@ /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 +# Accepted-work classification +/tasks/scripts/accepted_work.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/tasks/scripts/accepted_work_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/branch-checks.yml b/.github/workflows/branch-checks.yml index 8e21eb7a69..4e9403c484 100644 --- a/.github/workflows/branch-checks.yml +++ b/.github/workflows/branch-checks.yml @@ -604,6 +604,9 @@ jobs: - name: Test release qualification summary run: mise run test:qualification-summary + + - name: Test accepted-work library and review-readiness + run: mise run test:accepted-work test:review-readiness sdk-go-freeze: # The Go SDK moved to ai-openshell/sdk-go (#5031). Its directory stays only # until the final deprecated sdk/go tag exists, so PRs must not add or change diff --git a/.github/workflows/review-readiness.yml b/.github/workflows/review-readiness.yml index 876d3e490e..bd58a868a3 100644 --- a/.github/workflows/review-readiness.yml +++ b/.github/workflows/review-readiness.yml @@ -9,6 +9,7 @@ on: permissions: contents: read + issues: read pull-requests: write concurrency: @@ -19,11 +20,11 @@ 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:')) + github.event.action == 'opened' || + (!github.event.pull_request.draft && + (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: @@ -31,18 +32,27 @@ jobs: - name: Check out the reviewer roster and helper uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.sha }} + ref: ${{ github.event.repository.default_branch }} sparse-checkout: | REVIEWERS.md + MAINTAINERS.md tasks/scripts/check_review_readiness.py + tasks/scripts/accepted_work.py + tasks/scripts/check_maintainer_approval.py sparse-checkout-cone-mode: false persist-credentials: false - - name: Reconcile reviewer requests + - name: Label newly opened accepted work and reconcile reviewers env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} GH_REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + PR_ACTION: ${{ github.event.action }} + shell: bash run: | - python3 tasks/scripts/check_review_readiness.py \ - --repo "$GH_REPO" --pr "$PR_NUMBER" --apply + set -euo pipefail + args=(--repo "$GH_REPO" --pr "$PR_NUMBER" --apply) + if [ "$PR_ACTION" = opened ]; then + args+=(--label-accepted-work) + fi + python3 tasks/scripts/check_review_readiness.py "${args[@]}" diff --git a/.github/zizmor.yml b/.github/zizmor.yml index 5e19eaefa9..601855c71f 100644 --- a/.github/zizmor.yml +++ b/.github/zizmor.yml @@ -12,4 +12,5 @@ rules: - pr-issue-acceptance.yml:6 - release-canary.yml:3 - required-ci-gates.yml:3 + - review-readiness.yml:6 - vouch-check.yml:3 diff --git a/CI.md b/CI.md index e9384acf15..678f11255e 100644 --- a/CI.md +++ b/CI.md @@ -703,6 +703,7 @@ The bot's full administrator documentation is internal to NVIDIA. The only comma | `.github/actions/pr-merge-base/action.yml` | Composite action that resolves and fetches the merge-base commit for `pull-request/` push workflows. | | `.github/workflows/required-ci-gates.yml` | Posts required PR-head and merge-group statuses for gated CI workflows. This is what branch protection and merge queue should require. | | `.github/workflows/e2e-label-help.yml` | When a `test:e2e*` label is applied, posts a PR comment telling the maintainer the next manual step (re-run an existing workflow run, or `/ok to test ` to refresh the mirror). | +| `.github/workflows/review-readiness.yml` | Labels newly opened accepted-work PRs using the shared read-only acceptance library, including drafts; routes matching reviewers when a PR is ready and classified. | | `.github/workflows/workflow-security.yml` | Runs informational Actionlint and High-severity Zizmor reports on GitHub-hosted runners. | | `.github/workflows/dependency-review.yml` | Reports dependency changes when GitHub Dependency Graph is available; otherwise publishes a neutral warning. | | `.github/workflows/codeql.yml` | Runs nightly informational CodeQL analysis on `main` for Rust and the Go, Python, and TypeScript SDKs and retains SARIF artifacts. | diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3e800debdf..48c3beb575 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,7 @@ Every pull request must be approved by someone listed in [MAINTAINERS.md](MAINTA 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. -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), excluding the author, when a PR opens ready for review, leaves draft status, or gains or loses an area/topic label. Reviewers do not need repository write access to receive requests or review PRs. Manual reviewer choices and people who have commented or reviewed are preserved. Propose roster membership changes through a reviewed PR. +Once your pull request represents [accepted work](#accepted-work), apply exactly one `area:` label and any relevant `topic:` labels. Follow [Labeling and Notifications](#labeling-and-notifications) before applying classification labels. The [reviewer routing workflow](.github/workflows/review-readiness.yml) requests all matching members of [REVIEWERS.md](REVIEWERS.md), excluding the author, when a PR opens ready for review, leaves draft status, or gains or loses an area/topic label. Reviewers do not need repository write access to receive requests or review PRs. 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. @@ -131,6 +131,29 @@ A direct request to an agent authorizes the requested planning or implementation Do not file suspected vulnerabilities as public issues. Follow [SECURITY.md](SECURITY.md). Use the specialized security skills for authorized review or remediation. +### Accepted Work + +Accepted work is work the project intends to pursue. Work submitted by a maintainer listed in the merged [MAINTAINERS.md](MAINTAINERS.md), or someone with repository `WRITE`, `MAINTAIN`, or `ADMIN` access, qualifies immediately. Other issues require a maintainer's acceptance, recorded with `state:accepted`. + +A PR represents accepted work when either: + +- Its author is a maintainer or has repository write access as described above. +- It closes at least one issue in this repository that has `state:accepted`, and the PR author is that issue's submitter or a current assignee. + +Use GitHub closing references such as `Closes #123` or `Fixes #123`. Merely mentioning an accepted issue does not qualify. Acceptance and the author relationship must hold on the same issue. The PR author determines eligibility, not the person triggering a workflow. Other tools and contributor skills use this shared definition; the importable `tasks/scripts/accepted_work.py` library implements the PR check. + +Acceptance classifies work, not implementation quality or approval to merge. Existing issue-linking rules, the separate `PR Issue Acceptance` check (which requires every linked issue to be accepted for community authors), and maintainer/code-owner approvals still apply. Acceptance does not certify that the diff implements the issue, and the existing Dependabot gate exemption does not automatically qualify a PR as accepted work. + +### Labeling and Notifications + +Labels drive notification streams as well as classification. Contributors use the GitHub app in Slack, and other integrations, to follow issues and PRs filtered by area or topic. Applying those labels can notify subscribers immediately, before they have chosen to follow the individual item. + +Apply `area:` and `topic:` classification labels only after the issue or PR meets the shared [accepted-work definition](#accepted-work). Once accepted, apply the relevant classification labels so subscribers can find the work. + +Do not classify an issue merely because it was filed or its subject is clear. Waiting until acceptance keeps label-filtered streams focused on work the project intends to pursue and avoids sending unaccepted intake to subscribers. Workflow labels such as `state:triage-needed` still record intake status; they do not establish acceptance. + +Automatic classification at issue creation and filters that exclude unaccepted work are being discussed separately in [#5261](https://github.com/NVIDIA/OpenShell/issues/5261). Until that policy changes, defer classification labels until acceptance. + ### Label Discovery Retrieve every page of repository labels and their descriptions: @@ -459,6 +482,12 @@ Automated dependency update PRs, including Dependabot PRs, are exempt from the s Authors with `WRITE`, `MAINTAIN`, or `ADMIN` access may open a PR that only fixes a broken build or flaky tests without a separate issue. Describe the failure in the PR instead. Authors without write access follow the normal issue and acceptance requirements. +### Automatic Acceptance Labeling + +The existing [review-readiness workflow](.github/workflows/review-readiness.yml) checks [accepted work](#accepted-work) when a PR is opened and adds `state:accepted` if it qualifies. This includes drafts and PRs awaiting area/topic classification. Reviewer requests still wait until a PR is ready and has an area label. + +This is an additive check at creation, not continuous reconciliation: later edits, permission changes, and issue-label or assignment changes do not refresh or remove the PR label. The shared library is read-only; review-readiness applies the label. The label does not replace the issue-acceptance gate or merge approvals. + ### Branch Names Use `/-/` for contributor branches. Choose the Conventional Commit type that describes the work, such as `feat`, `fix`, `refactor`, `chore`, `perf`, `docs`, `test`, or `ci`. The description is a short lowercase slug separated by hyphens; the final component is your GitHub login, not initials. diff --git a/tasks/scripts/accepted_work.py b/tasks/scripts/accepted_work.py new file mode 100644 index 0000000000..0644384946 --- /dev/null +++ b/tasks/scripts/accepted_work.py @@ -0,0 +1,148 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Shared accepted-work classification; see CONTRIBUTING.md#accepted-work. + +Use is_accepted_work with supplied facts, or check_pr_accepted_work with a +GitHub API callable. The latter accepts the same path/method/data/pages arguments +as check_review_readiness._api. Both return a bool; API errors propagate. +This library has no CLI, label writes, or workflow lifecycle of its own. +""" + +from __future__ import annotations + +import subprocess +from dataclasses import dataclass +from typing import TYPE_CHECKING +from urllib.parse import quote + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Iterator + +ACCEPTED_LABEL = "state:accepted" +WRITE_PERMISSIONS = frozenset({"write", "maintain", "admin"}) +CLOSING_ISSUES_QUERY = """ +query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + closingIssuesReferences(first: 100, after: $cursor) { + nodes { number repository { nameWithOwner } } + pageInfo { hasNextPage endCursor } + } + } + } +} +""" + + +@dataclass(frozen=True) +class ClosingIssue: + """Facts about an issue GitHub reports that the PR closes.""" + + repository: str + accepted: bool + author: str | None + assignees: frozenset[str] = frozenset() + + +def is_accepted_work( + author: str, + *, + repository: str, + maintainers: set[str], + permission: str, + closing_issues: Iterable[ClosingIssue] = (), +) -> bool: + """Classify supplied facts without IO; the triggering actor is irrelevant.""" + author = author.lower() + if author in {login.lower() for login in maintainers}: + return True + if permission in WRITE_PERMISSIONS: + return True + for issue in closing_issues: + if issue.repository.lower() != repository.lower() or not issue.accepted: + continue + participants = {login.lower() for login in issue.assignees} + if issue.author: + participants.add(issue.author.lower()) + if author in participants: + return True + return False + + +def _closing_issue_facts( + repository: str, number: int, api: Callable +) -> Iterator[ClosingIssue]: + owner, repo = repository.split("/") + cursor = None + while True: + result = api( + "graphql", + method="POST", + data={ + "query": CLOSING_ISSUES_QUERY, + "variables": { + "owner": owner, + "repo": repo, + "number": number, + "cursor": cursor, + }, + }, + ) + if result.get("errors"): + raise ValueError("GitHub could not resolve closing issues") + pull = result["data"]["repository"]["pullRequest"] + connection = pull["closingIssuesReferences"] + for reference in connection["nodes"]: + source = reference["repository"]["nameWithOwner"] + if source.lower() != repository.lower(): + continue + root = f"repos/{repository}/issues/{reference['number']}" + issue = api(root) + if "pull_request" in issue: + continue + labels = api(f"{root}/labels?per_page=100", pages=True) + yield ClosingIssue( + repository=source, + accepted=any(label["name"] == ACCEPTED_LABEL for label in labels), + author=issue["user"]["login"] if issue["user"] else None, + assignees=frozenset(user["login"] for user in issue["assignees"]), + ) + if not connection["pageInfo"]["hasNextPage"]: + return + next_cursor = connection["pageInfo"]["endCursor"] + if not next_cursor or next_cursor == cursor: + raise ValueError("GitHub returned an invalid closing-issue cursor") + cursor = next_cursor + + +def check_pr_accepted_work( + repository: str, pr: dict, maintainers: set[str], *, api: Callable +) -> bool: + """Load current facts using a read-only API callable and classify this PR. + + Callers supply maintainer membership from a trusted merged roster. A 404 + collaborator lookup means no repository access; other lookup errors propagate. + Maintainer and write-access authors need no closing-issue lookup. + """ + if not maintainers: + raise ValueError("Could not parse any maintainers from MAINTAINERS.md") + author = pr["user"]["login"] + permission = "none" + if author.lower() not in {login.lower() for login in maintainers}: + try: + result = api( + f"repos/{repository}/collaborators/" + f"{quote(author, safe='')}/permission" + ) + permission = result["permission"] + except subprocess.CalledProcessError as exc: + if "(HTTP 404)" not in (exc.stderr or ""): + raise + return is_accepted_work( + author, + repository=repository, + maintainers=maintainers, + permission=permission, + closing_issues=_closing_issue_facts(repository, pr["number"], api), + ) diff --git a/tasks/scripts/accepted_work_test.py b/tasks/scripts/accepted_work_test.py new file mode 100644 index 0000000000..f746dae4b8 --- /dev/null +++ b/tasks/scripts/accepted_work_test.py @@ -0,0 +1,251 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Offline tests for the shared accepted-work predicate and GitHub adapter.""" + +from __future__ import annotations + +import subprocess +import unittest +from unittest import mock + +from accepted_work import ( + ACCEPTED_LABEL, + ClosingIssue, + check_pr_accepted_work, + is_accepted_work, +) + +REPO = "NVIDIA/OpenShell" +PR = {"number": 7, "user": {"login": "contributor"}} + + +def facts(*, repository=REPO, accepted=True, author="submitter", assignees=()): + return ClosingIssue(repository, accepted, author, frozenset(assignees)) + + +def connection(*numbers, more=False, cursor=None, repository=REPO): + linked = { + "nodes": [ + {"number": number, "repository": {"nameWithOwner": repository}} + for number in numbers + ], + "pageInfo": {"hasNextPage": more, "endCursor": cursor}, + } + pull = {"closingIssuesReferences": linked} + return {"data": {"repository": {"pullRequest": pull}}} + + +def issue(*, author="contributor", assignees=()): + return { + "user": {"login": author} if author else None, + "assignees": [{"login": login} for login in assignees], + } + + +class PredicateTests(unittest.TestCase): + def decide( + self, *, author="contributor", maintainers=None, permission="read", issues=() + ): + return is_accepted_work( + author, + repository=REPO, + maintainers=maintainers or {"maintainer"}, + permission=permission, + closing_issues=issues, + ) + + def test_maintainers_and_write_roles_qualify_without_issues(self): + self.assertTrue(self.decide(author="MAINTAINER")) + for permission in ("write", "maintain", "admin"): + with self.subTest(permission=permission): + self.assertTrue(self.decide(permission=permission)) + + def test_read_triage_none_and_unknown_roles_do_not_qualify(self): + for permission in ("read", "triage", "none", "unknown"): + with self.subTest(permission=permission): + self.assertFalse(self.decide(permission=permission)) + + def test_issue_submitter_and_assignee_qualify_case_insensitively(self): + for value in ( + facts(author="CONTRIBUTOR"), + facts(assignees=("other", "CONTRIBUTOR")), + facts(author=None, assignees=("contributor",)), + ): + with self.subTest(issue=value): + self.assertTrue(self.decide(issues=[value])) + + def test_accepted_issue_without_author_relationship_does_not_qualify(self): + self.assertFalse(self.decide(issues=[facts()])) + + def test_author_relationship_without_acceptance_does_not_qualify(self): + for value in ( + facts(author="contributor", accepted=False), + facts(assignees=("contributor",), accepted=False), + ): + with self.subTest(issue=value): + self.assertFalse(self.decide(issues=[value])) + + def test_acceptance_and_relationship_must_be_on_the_same_issue(self): + self.assertFalse(self.decide(issues=[ + facts(), facts(author="contributor", accepted=False) + ])) + self.assertTrue(self.decide(issues=[ + facts(accepted=False), facts(assignees=("contributor",)) + ])) + + def test_only_target_repository_issues_qualify(self): + self.assertFalse(self.decide(issues=[ + facts(repository="purp/OpenShell", author="contributor") + ])) + self.assertTrue(self.decide(issues=[ + facts(repository="nvidia/openshell", author="contributor") + ])) + + def test_bots_do_not_receive_an_implicit_exemption(self): + self.assertFalse(self.decide(author="dependabot[bot]")) + + def test_privileged_decision_does_not_consume_issue_metadata(self): + def unavailable(): + yield from () + raise AssertionError("must not load issues") + + self.assertTrue(self.decide(permission="write", issues=unavailable())) + self.assertTrue(self.decide(author="maintainer", issues=unavailable())) + + +class GitHubAdapterTests(unittest.TestCase): + def check(self, responses, *, pull=None, maintainers=None): + api = mock.Mock(side_effect=responses) + result = check_pr_accepted_work( + REPO, pull or PR, maintainers or {"maintainer"}, api=api + ) + return result, api + + def test_maintainer_skips_all_api_reads(self): + result, api = self.check( + [], pull={"number": 7, "user": {"login": "MAINTAINER"}} + ) + self.assertTrue(result) + api.assert_not_called() + + def test_write_author_only_reads_their_permissions(self): + result, api = self.check([{"permission": "write"}]) + self.assertTrue(result) + api.assert_called_once_with( + f"repos/{REPO}/collaborators/contributor/permission" + ) + + def test_empty_roster_is_an_error_before_any_api_reads(self): + api = mock.Mock() + with self.assertRaisesRegex(ValueError, "Could not parse"): + check_pr_accepted_work(REPO, PR, set(), api=api) + api.assert_not_called() + + def test_checks_current_issue_author_assignees_and_paginated_labels(self): + result, api = self.check([ + {"permission": "read"}, connection(42), + issue(author="other", assignees=("contributor",)), + [{"name": "area:build"}, {"name": ACCEPTED_LABEL}], + ]) + self.assertTrue(result) + self.assertEqual( + api.call_args_list[-2:], + [ + mock.call(f"repos/{REPO}/issues/42"), + mock.call(f"repos/{REPO}/issues/42/labels?per_page=100", pages=True), + ], + ) + + def test_no_native_closing_reference_does_not_qualify(self): + pull = {**PR, "body": "Accepted work: #42. Maintainer requested."} + result, _ = self.check([{"permission": "read"}, connection()], pull=pull) + self.assertFalse(result) + + def test_cross_repository_references_are_not_fetched(self): + result, api = self.check([ + {"permission": "read"}, connection(42, repository="purp/OpenShell") + ]) + self.assertFalse(result) + self.assertEqual(api.call_count, 2) + + def test_prs_are_not_issues_for_acceptance(self): + result, api = self.check([ + {"permission": "read"}, connection(42), + {**issue(), "pull_request": {"url": "pull"}}, + ]) + self.assertFalse(result) + self.assertEqual(api.call_count, 3) + + def test_reaches_a_qualifying_issue_on_a_later_page(self): + result, api = self.check([ + {"permission": "read"}, connection(more=True, cursor="next"), + connection(42), issue(), [{"name": ACCEPTED_LABEL}], + ]) + self.assertTrue(result) + graphql_calls = [ + call for call in api.call_args_list if call.args[0] == "graphql" + ] + self.assertEqual(graphql_calls[0].kwargs["data"]["variables"]["cursor"], None) + self.assertEqual(graphql_calls[1].kwargs["data"]["variables"]["cursor"], "next") + self.assertEqual(graphql_calls[0].kwargs["data"]["variables"]["number"], 7) + + def test_invalid_cursor_is_an_error(self): + with self.assertRaisesRegex(ValueError, "cursor"): + self.check([{"permission": "read"}, connection(more=True)]) + + def test_noncollaborator_404_still_qualifies_through_an_issue(self): + error = subprocess.CalledProcessError( + 1, "gh", stderr="gh: Not Found (HTTP 404)" + ) + result, _ = self.check([ + error, connection(42), issue(), [{"name": ACCEPTED_LABEL}] + ]) + self.assertTrue(result) + + def test_permission_errors_propagate(self): + for status in (401, 403, 429, 500): + error = subprocess.CalledProcessError(1, "gh", stderr=f"(HTTP {status})") + with ( + self.subTest(status=status), + self.assertRaises(subprocess.CalledProcessError), + ): + self.check([error]) + + def test_graphql_errors_are_errors_not_a_negative_decision(self): + for response in ( + {"errors": [{"message": "denied"}]}, + {"data": {"repository": None}}, + ): + with ( + self.subTest(response=response), + self.assertRaises((ValueError, TypeError)), + ): + self.check([{"permission": "read"}, response]) + + def test_issue_and_label_errors_propagate(self): + error = subprocess.CalledProcessError(1, "gh", stderr="(HTTP 403)") + for responses in ( + [{"permission": "read"}, connection(42), error], + [{"permission": "read"}, connection(42), issue(), error], + ): + with ( + self.subTest(responses=responses), + self.assertRaises(subprocess.CalledProcessError), + ): + self.check(responses) + + def test_metadata_reads_never_call_label_mutation_endpoints(self): + _, api = self.check([ + {"permission": "read"}, connection(42), issue(), + [{"name": ACCEPTED_LABEL}], + ]) + for call in api.call_args_list: + if call.args[0] == "graphql": + self.assertIn("query(", call.kwargs["data"]["query"]) + else: + self.assertEqual(call.kwargs.get("method", "GET"), "GET") + + +if __name__ == "__main__": + unittest.main() diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py index 0e2fff2e7b..824f1ac394 100644 --- a/tasks/scripts/check_review_readiness.py +++ b/tasks/scripts/check_review_readiness.py @@ -10,6 +10,7 @@ """Reconcile PR reviewers from the merged REVIEWERS.md and current PR metadata. Preview with --repo OWNER/REPO --pr NUMBER; add --apply to update requests. +For newly opened PRs, --label-accepted-work also adds state:accepted if qualified. Request all matching roster members, 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. @@ -22,9 +23,12 @@ import re import subprocess import sys -from dataclasses import asdict, dataclass +from dataclasses import asdict, dataclass, replace from pathlib import Path +from accepted_work import ACCEPTED_LABEL, check_pr_accepted_work +from check_maintainer_approval import parse_maintainers + 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) @@ -34,6 +38,8 @@ class ReviewPlan: add: tuple[str, ...] remove: tuple[str, ...] + accepted_work: bool | None = None + add_labels: tuple[str, ...] = () def parse_reviewers(markdown: str) -> dict[str, set[str]]: @@ -212,15 +218,43 @@ def check_review_readiness( number: int, *, apply=False, + label_accepted_work=False, + maintainers: set[str] | None = None, ) -> ReviewPlan | None: - """Plan or apply routing. Callers must serialize runs for the same PR.""" + """Plan labeling and routing; callers serialize runs for the same PR. + + Enable label_accepted_work for newly opened PRs, including drafts. Acceptance + is independent of readiness and classification; only reviewer routing needs + a ready PR with an area label. Labeling never removes an existing label. + """ pull = f"repos/{repository}/pulls/{number}" pr = _api(pull) - if pr["state"] != "open" or pr["draft"]: + if pr["state"] != "open": return None - selected = select_reviewers(categories, {label["name"] for label in pr["labels"]}) + accepted = None + add_labels = () + labels = {label["name"] for label in pr["labels"]} + if label_accepted_work: + accepted = check_pr_accepted_work(repository, pr, maintainers or set(), api=_api) + if accepted and ACCEPTED_LABEL not in labels: + add_labels = (ACCEPTED_LABEL,) + if apply: + _api( + f"repos/{repository}/issues/{number}/labels", + method="POST", + data={"labels": list(add_labels)}, + ) + if pr["draft"] or not any(label.startswith("area:") for label in labels): + if pr["draft"] and not label_accepted_work: + return None + return ReviewPlan((), (), accepted, add_labels) + selected = select_reviewers(categories, labels) selected -= {pr["user"]["login"].lower()} - plan = plan_review_requests(selected, **_load_reviewers(repository, number, pr)) + plan = replace( + plan_review_requests(selected, **_load_reviewers(repository, number, pr)), + accepted_work=accepted, + add_labels=add_labels, + ) if apply: endpoint = f"{pull}/requested_reviewers" if plan.add: @@ -233,7 +267,7 @@ def check_review_readiness( refreshed = plan_review_requests( selected, **_load_reviewers(repository, number, latest) ) - plan = ReviewPlan(plan.add, refreshed.remove) + plan = replace(plan, remove=refreshed.remove) if plan.remove: _api(endpoint, method="DELETE", data={"reviewers": list(plan.remove)}) return plan @@ -242,6 +276,12 @@ def check_review_readiness( 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("--maintainers", type=Path, default=Path("MAINTAINERS.md")) + parser.add_argument( + "--label-accepted-work", + action="store_true", + help="check and label accepted work on a newly opened PR", + ) 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( @@ -252,8 +292,27 @@ def main(argv: list[str] | None = None) -> int: parser.error("--repo must be OWNER/REPO and --pr must be positive") try: categories = parse_reviewers(args.reviewers.read_text(encoding="utf-8")) - plan = check_review_readiness(categories, args.repo, args.pr, apply=args.apply) - except (OSError, UnicodeError, ValueError, subprocess.CalledProcessError) as exc: + maintainers = ( + parse_maintainers(args.maintainers.read_text(encoding="utf-8")) + if args.label_accepted_work + else None + ) + plan = check_review_readiness( + categories, + args.repo, + args.pr, + apply=args.apply, + label_accepted_work=args.label_accepted_work, + maintainers=maintainers, + ) + except ( + OSError, + UnicodeError, + ValueError, + KeyError, + TypeError, + subprocess.CalledProcessError, + ) as exc: print(f"Review readiness: {exc}", file=sys.stderr) return 1 print( diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py index 5914788c44..1d4df4300e 100644 --- a/tasks/scripts/check_review_readiness_test.py +++ b/tasks/scripts/check_review_readiness_test.py @@ -30,6 +30,7 @@ def pr(*labels, requested=(), author="author", draft=False, state="open"): return { + "number": 42, "state": state, "draft": draft, "user": {"login": author}, @@ -385,6 +386,165 @@ def test_retry_after_partial_failure_uses_updated_github_metadata(self): ) +class AcceptedWorkIntegrationTests(unittest.TestCase): + def run_check(self, value, *, accepted=True, apply=True, opened=True): + with ( + mock.patch.object(routing, "_api", side_effect=[value, None]) as api, + mock.patch.object( + routing, "check_pr_accepted_work", return_value=accepted + ) as check, + mock.patch.object( + routing, "_load_reviewers", return_value=reviewers() + ) as load, + ): + plan = routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=apply, + label_accepted_work=opened, + maintainers={"maintainer"}, + ) + return plan, api, check, load + + def test_new_draft_is_labeled_without_requesting_reviewers(self): + plan, api, check, load = self.run_check(pr(draft=True)) + self.assertEqual(plan, routing.ReviewPlan((), (), True, ("state:accepted",))) + check.assert_called_once_with( + REPO, pr(draft=True), {"maintainer"}, api=api + ) + api.assert_has_calls([ + mock.call(PULL), + mock.call( + "repos/owner/repo/issues/42/labels", method="POST", + data={"labels": ["state:accepted"]}, + ), + ]) + self.assertEqual(api.call_count, 2) + load.assert_not_called() + + def test_new_ready_pr_can_be_labeled_before_area_classification(self): + plan, api, _, load = self.run_check(pr()) + self.assertEqual(plan.add_labels, ("state:accepted",)) + self.assertEqual(api.call_count, 2) + load.assert_not_called() + + def test_nonqualifying_pr_is_not_labeled(self): + plan, api, _, _ = self.run_check(pr(), accepted=False) + self.assertFalse(plan.accepted_work) + self.assertEqual(plan.add_labels, ()) + api.assert_called_once_with(PULL) + + def test_existing_labels_are_preserved_and_acceptance_is_not_readded(self): + plan, api, _, _ = self.run_check(pr("state:accepted", "unrelated")) + self.assertTrue(plan.accepted_work) + self.assertEqual(plan.add_labels, ()) + api.assert_called_once_with(PULL) + plan, api, _, _ = self.run_check(pr("state:accepted"), accepted=False) + self.assertEqual(plan.add_labels, ()) + api.assert_called_once_with(PULL) + + def test_preview_reports_label_addition_without_writing(self): + plan, api, _, _ = self.run_check(pr(draft=True), apply=False) + self.assertEqual(plan.add_labels, ("state:accepted",)) + api.assert_called_once_with(PULL) + + def test_later_events_do_not_recheck_or_change_acceptance(self): + plan, api, check, _ = self.run_check(pr(), opened=False) + self.assertIsNone(plan.accepted_work) + check.assert_not_called() + api.assert_called_once_with(PULL) + + def test_closed_pr_is_never_classified_or_labeled(self): + plan, api, check, load = self.run_check(pr(state="closed")) + self.assertIsNone(plan) + check.assert_not_called() + load.assert_not_called() + api.assert_called_once_with(PULL) + + def test_labels_and_requests_reviewers_for_a_classified_ready_pr(self): + with ( + mock.patch.object( + routing, "_api", side_effect=[pr("area:build"), None, None] + ) as api, + mock.patch.object(routing, "check_pr_accepted_work", return_value=True), + mock.patch.object(routing, "_load_reviewers", return_value=reviewers()), + ): + plan = routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=True, + label_accepted_work=True, maintainers={"maintainer"}, + ) + self.assertEqual( + plan, routing.ReviewPlan(("dave",), (), True, ("state:accepted",)) + ) + self.assertEqual( + api.call_args.kwargs, + {"method": "POST", "data": {"reviewers": ["dave"]}}, + ) + + def test_lookup_failure_does_not_label_or_request_reviewers(self): + with ( + mock.patch.object(routing, "_api", return_value=pr("area:build")) as api, + mock.patch.object( + routing, "check_pr_accepted_work", side_effect=ValueError("lookup failed") + ), + self.assertRaisesRegex(ValueError, "lookup failed"), + ): + routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=True, + label_accepted_work=True, maintainers={"maintainer"}, + ) + api.assert_called_once_with(PULL) + + def test_real_library_qualifies_a_maintainer_without_permission_reads(self): + with mock.patch.object( + routing, "_api", side_effect=[pr(author="MAINTAINER", draft=True), None] + ) as api: + plan = routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=True, + label_accepted_work=True, maintainers={"maintainer"}, + ) + self.assertTrue(plan.accepted_work) + self.assertEqual(api.call_count, 2) + + + def test_label_result_survives_the_refresh_before_reviewer_removal(self): + before = pr("area:build", requested=("alice", "dave")) + after = pr("area:build", "state:accepted", requested=("alice", "dave")) + with ( + mock.patch.object( + routing, "_api", side_effect=[before, None, after, None] + ) as api, + mock.patch.object(routing, "check_pr_accepted_work", return_value=True), + mock.patch.object( + routing, "_load_reviewers", + return_value=reviewers(requested=("alice", "dave")), + ), + ): + plan = routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=True, + label_accepted_work=True, maintainers={"maintainer"}, + ) + self.assertEqual( + plan, routing.ReviewPlan((), ("alice",), True, ("state:accepted",)) + ) + self.assertEqual( + api.call_args.kwargs, + {"method": "DELETE", "data": {"reviewers": ["alice"]}}, + ) + + def test_label_write_failure_does_not_request_reviewers(self): + error = subprocess.CalledProcessError(1, "gh", stderr="(HTTP 403)") + with ( + mock.patch.object(routing, "_api", side_effect=[pr("area:build"), error]), + mock.patch.object(routing, "check_pr_accepted_work", return_value=True), + mock.patch.object(routing, "_load_reviewers") as load, + self.assertRaises(subprocess.CalledProcessError), + ): + routing.check_review_readiness( + CATEGORIES, REPO, 42, apply=True, + label_accepted_work=True, maintainers={"maintainer"}, + ) + load.assert_not_called() + + class CliTests(unittest.TestCase): def test_emits_plan_and_passes_apply_option(self): with tempfile.TemporaryDirectory() as directory: @@ -414,10 +574,40 @@ def test_emits_plan_and_passes_apply_option(self): 0, ) self.assertEqual( - json.loads(output.getvalue()), {"add": ["alice"], "remove": []} + json.loads(output.getvalue()), { + "add": ["alice"], "remove": [], + "accepted_work": None, "add_labels": [], + } + ) + check.assert_called_once_with( + {"area:runtime": {"alice"}}, REPO, 42, apply=True, + label_accepted_work=False, maintainers=None, + ) + + def test_opened_pr_option_loads_trusted_maintainer_roster(self): + with tempfile.TemporaryDirectory() as directory: + roster = Path(directory) / "REVIEWERS.md" + roster.write_text(DOCUMENT, encoding="utf-8") + maintainers = Path(directory) / "MAINTAINERS.md" + maintainers.write_text("[@maintainer](https://github.com/maintainer)") + with ( + mock.patch.object( + routing, "check_review_readiness", + return_value=routing.ReviewPlan((), (), True, ("state:accepted",)), + ) as check, + contextlib.redirect_stdout(io.StringIO()) as output, + ): + result = routing.main([ + "--repo", REPO, "--pr", "42", "--reviewers", str(roster), + "--maintainers", str(maintainers), "--label-accepted-work", + ]) + self.assertEqual(result, 0) + self.assertEqual( + json.loads(output.getvalue())["add_labels"], ["state:accepted"] ) check.assert_called_once_with( - {"area:runtime": {"alice"}}, REPO, 42, apply=True + {"area:runtime": {"alice"}}, REPO, 42, apply=False, + label_accepted_work=True, maintainers={"maintainer"}, ) def test_invalid_registry_fails_before_contacting_github(self): diff --git a/tasks/test.toml b/tasks/test.toml index 4309ad95b7..c9dab418a7 100644 --- a/tasks/test.toml +++ b/tasks/test.toml @@ -25,6 +25,7 @@ depends = [ "test:maintainer-approval", "test:cargo-lockfile-alignment", "test:review-readiness", + "test:accepted-work", ] ["test:cargo-lockfile-alignment"] @@ -102,6 +103,11 @@ 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 +["test:accepted-work"] +description = "Test the shared accepted-work library" +run = "uv run --no-project python -m unittest discover -s tasks/scripts -p accepted_work_test.py" +hide = true + [e2e] description = "Run all end-to-end tests (Rust + Python + MCP)" depends = ["e2e:rust", "e2e:python", "e2e:mcp"]