From ce76a6f7dc6c4de8df0e14d7ae67def64223e62b Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Wed, 4 Nov 2026 16:59:45 +0000 Subject: [PATCH 1/5] feat(review): automatically label PRs for accepted work Signed-off-by: Jim Meyer --- .agents/skills/create-github-pr/SKILL.md | 9 + .github/CODEOWNERS | 4 + .github/workflows/accepted-work.yml | 71 ++++ .github/workflows/branch-checks.yml | 3 + CI.md | 1 + CONTRIBUTING.md | 13 + tasks/scripts/label_accepted_work.py | 209 ++++++++++++ tasks/scripts/label_accepted_work_test.py | 389 ++++++++++++++++++++++ tasks/test.toml | 6 + 9 files changed, 705 insertions(+) create mode 100644 .github/workflows/accepted-work.yml create mode 100644 tasks/scripts/label_accepted_work.py create mode 100644 tasks/scripts/label_accepted_work_test.py diff --git a/.agents/skills/create-github-pr/SKILL.md b/.agents/skills/create-github-pr/SKILL.md index 59ab0dade0..2a26393cb2 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -258,6 +258,15 @@ automatically on issue-label events. Created PR [#123](https://github.com/OWNER/REPO/pull/123) ``` +### Accepted-Work Classification + +The `Accepted Work` workflow manages the PR's acceptance label using the definition +in [CONTRIBUTING.md](../../../CONTRIBUTING.md#accepted-work). For an author without +maintainer membership or target-repository write access, the same closing issue +must be accepted and list the PR author as its submitter or a current assignee. +The label does not replace the issue-acceptance gate or merge approvals. Do not +manually apply it as a substitute for satisfying the automated rule. + ### 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/.github/CODEOWNERS b/.github/CODEOWNERS index e9899931bd..78f5034728 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/label_accepted_work.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/tasks/scripts/label_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/accepted-work.yml b/.github/workflows/accepted-work.yml new file mode 100644 index 0000000000..d7712b66c6 --- /dev/null +++ b/.github/workflows/accepted-work.yml @@ -0,0 +1,71 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +name: Accepted Work + +on: + pull_request_target: + types: [opened, edited, reopened, synchronize, ready_for_review, labeled, unlabeled] + issues: + types: [labeled, unlabeled, assigned, unassigned, deleted, transferred] + push: + branches: [main] + paths: + - MAINTAINERS.md + - tasks/scripts/label_accepted_work.py + - tasks/scripts/check_maintainer_approval.py + - .github/workflows/accepted-work.yml + schedule: + # Refresh permissions and closing links that have no dedicated webhook. + - cron: '23 9 * * *' + workflow_dispatch: + +permissions: + contents: read + issues: write + pull-requests: read + +# Issue changes and periodic refreshes reconcile all open PRs. Serialize them +# with individual PR events to avoid older decisions overwriting newer ones. +concurrency: + group: accepted-work + cancel-in-progress: false + +jobs: + label: + name: OpenShell / Accepted Work + if: >- + github.repository_owner == 'NVIDIA' && + (github.event_name != 'issues' || + github.event.action != 'labeled' && github.event.action != 'unlabeled' || + github.event.label.name == 'state:accepted') + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + # All event types use trusted default-branch code and membership. + - name: Check out accepted-work helper and maintainer list + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: main + sparse-checkout: | + MAINTAINERS.md + tasks/scripts/label_accepted_work.py + tasks/scripts/check_maintainer_approval.py + sparse-checkout-cone-mode: false + persist-credentials: false + + - name: Reconcile accepted-work labels + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + shell: bash + run: | + set -euo pipefail + if [ -n "$PR_NUMBER" ]; then + python3 tasks/scripts/label_accepted_work.py \ + --repo "$GH_REPO" --pr "$PR_NUMBER" --apply + else + python3 tasks/scripts/label_accepted_work.py \ + --repo "$GH_REPO" --all-open --apply + fi diff --git a/.github/workflows/branch-checks.yml b/.github/workflows/branch-checks.yml index 8e21eb7a69..ec34fbb879 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 labeling + run: mise run test:accepted-work 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/CI.md b/CI.md index e9384acf15..0491d75526 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/accepted-work.yml` | Reconciles the informational `state:accepted` PR label from trusted maintainer membership, repository write access, or accepted closing issues submitted by or assigned to the PR author. Runs on PR and issue changes, maintainer/helper changes, daily refresh, and manual dispatch. | | `.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..cde84a1762 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -459,6 +459,19 @@ 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. +### Accepted Work + +The `Accepted Work` workflow manages `state:accepted` on open PRs, including drafts. A PR represents accepted work when either: + +- Its author is listed in the merged [MAINTAINERS.md](MAINTAINERS.md) or has `WRITE`, `MAINTAIN`, or `ADMIN` access to this repository. +- 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. A contributor implementing someone else's accepted issue must be assigned to it to qualify through that issue. Acceptance and the author relationship must hold on the same issue; an accepted issue unrelated to the PR author does not qualify them. + +The workflow uses the PR author, not the person triggering it. It refreshes the label on PR changes, accepted-issue label and assignment changes, and changes to the merged maintainer list. A daily refresh and manual workflow dispatch reconcile all open PRs, including permission changes and closing-link changes without a dedicated event. The label is removed when a PR no longer qualifies; do not apply it manually to bypass the rule. Closed PRs retain their last label. + +This label 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. The label does not certify that the diff implements the issue, and the existing Dependabot gate exemption does not automatically qualify a PR for the label. + ### 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/label_accepted_work.py b/tasks/scripts/label_accepted_work.py new file mode 100644 index 0000000000..f544a36fb0 --- /dev/null +++ b/tasks/scripts/label_accepted_work.py @@ -0,0 +1,209 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# dependencies = [] +# /// + +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Reconcile the informational state:accepted label on open PRs. + +Read trusted MAINTAINERS.md and GitHub metadata, never code from a PR head. +Preview with --repo OWNER/REPO and --pr NUMBER or --all-open; add --apply to label. +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path +from urllib.parse import quote + +from check_maintainer_approval import parse_maintainers + +ACCEPTED = "state:accepted" +CLOSING_ISSUES = """ +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 } + } + } + } +} +""" + + +def _api(path: str, *, method="GET", data=None, pages=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 + + +class AcceptedWork: + """Evaluate current PR authors and issue relationships; cache within a run.""" + + def __init__(self, repository: str, maintainers: set[str]): + if not maintainers: + raise ValueError("Could not parse any maintainers from MAINTAINERS.md") + self.repository = repository + self.maintainers = {login.lower() for login in maintainers} + self.permissions: dict[str, bool] = {} + self.issues: dict[int, dict] = {} + + def privileged(self, author: str) -> bool: + if author in self.maintainers: + return True + if author not in self.permissions: + try: + value = _api( + f"repos/{self.repository}/collaborators/" + f"{quote(author, safe='')}/permission" + ) + except subprocess.CalledProcessError as exc: + # GitHub returns 404 for users who are not collaborators. + # Other errors must stop reconciliation, not revoke acceptance. + if "(HTTP 404)" not in (exc.stderr or ""): + raise + value = {"permission": "none"} + self.permissions[author] = value["permission"] in { + "write", + "maintain", + "admin", + } + return self.permissions[author] + + def closing_issues(self, number: int): + owner, repo = self.repository.split("/") + cursor = None + while True: + value = _api( + "graphql", + method="POST", + data={ + "query": CLOSING_ISSUES, + "variables": { + "owner": owner, + "repo": repo, + "number": number, + "cursor": cursor, + }, + }, + ) + if value.get("errors"): + raise ValueError("GitHub could not resolve closing issues") + pull = value["data"]["repository"]["pullRequest"] + connection = pull["closingIssuesReferences"] + yield from connection["nodes"] + if not connection["pageInfo"]["hasNextPage"]: + return + cursor = connection["pageInfo"]["endCursor"] + if not cursor: + raise ValueError("GitHub returned a missing closing-issue cursor") + + def accepted(self, pr: dict) -> bool: + author = pr["user"]["login"].lower() + if self.privileged(author): + return True + for reference in self.closing_issues(pr["number"]): + repository = reference["repository"]["nameWithOwner"] + if repository.lower() != self.repository.lower(): + continue + number = reference["number"] + if number not in self.issues: + root = f"repos/{self.repository}/issues/{number}" + issue = _api(root) + issue["labels"] = _api(f"{root}/labels?per_page=100", pages=True) + self.issues[number] = issue + issue = self.issues[number] + if "pull_request" in issue: + continue + participants = { + user["login"].lower() + for user in [issue["user"], *issue["assignees"]] + if user + } + if author in participants and any( + label["name"] == ACCEPTED for label in issue["labels"] + ): + return True + return False + + def reconcile(self, prs: list[dict], *, apply=False) -> list[dict]: + # Resolve all reads before mutations, so lookup failures leave labels alone. + plans = [] + for pr in prs: + if pr["state"] != "open": + continue + accepted = self.accepted(pr) + labeled = any(label["name"] == ACCEPTED for label in pr["labels"]) + action = "none" if accepted == labeled else "add" if accepted else "remove" + plans.append( + {"number": pr["number"], "accepted": accepted, "action": action} + ) + if apply: + for plan in plans: + root = f"repos/{self.repository}/issues/{plan['number']}/labels" + if plan["action"] == "add": + _api(root, method="POST", data={"labels": [ACCEPTED]}) + elif plan["action"] == "remove": + _api(f"{root}/{quote(ACCEPTED, safe='')}", method="DELETE") + return plans + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo", required=True) + parser.add_argument("--maintainers", type=Path, default=Path("MAINTAINERS.md")) + target = parser.add_mutually_exclusive_group(required=True) + target.add_argument("--pr", type=int) + target.add_argument("--all-open", action="store_true") + parser.add_argument("--apply", action="store_true") + args = parser.parse_args(argv) + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", args.repo): + parser.error("--repo must be OWNER/REPO") + if args.pr is not None and args.pr <= 0: + parser.error("--pr must be positive") + try: + evaluator = AcceptedWork( + args.repo, parse_maintainers(args.maintainers.read_text(encoding="utf-8")) + ) + root = f"repos/{args.repo}/pulls" + prs = ( + [_api(f"{root}/{args.pr}")] + if args.pr is not None + else _api(f"{root}?state=open&per_page=100", pages=True) + ) + print(json.dumps(evaluator.reconcile(prs, apply=args.apply))) + except ( + OSError, + UnicodeError, + ValueError, + KeyError, + TypeError, + subprocess.CalledProcessError, + ) as exc: + print(f"Accepted work: {exc}", file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tasks/scripts/label_accepted_work_test.py b/tasks/scripts/label_accepted_work_test.py new file mode 100644 index 0000000000..458780b269 --- /dev/null +++ b/tasks/scripts/label_accepted_work_test.py @@ -0,0 +1,389 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Offline tests for deterministic accepted-work labeling.""" + +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 label_accepted_work as labeling + +REPO = "NVIDIA/OpenShell" +ROOT = f"repos/{REPO}" + + +def pr(*, author="contributor", number=7, labels=(), state="open", draft=False): + return { + "number": number, + "user": {"login": author}, + "labels": [{"name": name} for name in labels], + "state": state, + "draft": draft, + } + + +def issue(*, author="submitter", assignees=(), labels=(labeling.ACCEPTED,)): + return { + "user": {"login": author} if author else None, + "assignees": [{"login": login} for login in assignees], + "labels": [{"name": name} for name in labels], + } + + +def reference(number=1, repository=REPO): + return {"number": number, "repository": {"nameWithOwner": repository}} + + +def connection(nodes, *, more=False, cursor=None): + return { + "data": { + "repository": { + "pullRequest": { + "closingIssuesReferences": { + "nodes": nodes, + "pageInfo": {"hasNextPage": more, "endCursor": cursor}, + } + } + } + } + } + + +class AcceptanceTests(unittest.TestCase): + def evaluate(self, *, pull=None, permission="read", references=None, issues=None): + issues = issues or {1: issue()} + references = references if references is not None else [reference()] + + def api(path, **kwargs): + if "/collaborators/" in path: + if isinstance(permission, Exception): + raise permission + return {"permission": permission} + if path == "graphql": + self.assertEqual(kwargs["data"]["variables"]["number"], 7) + return connection(references) + for number, value in issues.items(): + if path == f"{ROOT}/issues/{number}": + return value.copy() + if path == f"{ROOT}/issues/{number}/labels?per_page=100": + self.assertTrue(kwargs["pages"]) + return value["labels"] + self.fail(f"Unexpected API call: {path}") + + with mock.patch.object(labeling, "_api", side_effect=api) as calls: + result = labeling.AcceptedWork(REPO, {"maintainer"}).accepted(pull or pr()) + return result, calls + + def test_maintainer_needs_no_permission_or_issue_lookup(self): + result, calls = self.evaluate(pull=pr(author="MAINTAINER")) + self.assertTrue(result) + calls.assert_not_called() + + def test_empty_maintainer_roster_is_an_error(self): + with self.assertRaisesRegex(ValueError, "Could not parse"): + labeling.AcceptedWork(REPO, set()) + + def test_write_maintain_and_admin_qualify_without_an_issue(self): + for permission in ("write", "maintain", "admin"): + with self.subTest(permission=permission): + result, calls = self.evaluate(permission=permission, references=[]) + self.assertTrue(result) + self.assertEqual(calls.call_count, 1) + + def test_read_triage_and_no_access_need_an_issue_relationship(self): + for permission in ("read", "triage", "none"): + with self.subTest(permission=permission): + result, _ = self.evaluate(permission=permission) + self.assertFalse(result) + + def test_accepted_issue_submitter_qualifies_case_insensitively(self): + result, _ = self.evaluate(issues={1: issue(author="CONTRIBUTOR")}) + self.assertTrue(result) + + def test_current_assignee_qualifies_case_insensitively(self): + result, _ = self.evaluate(issues={1: issue(assignees=("other", "CONTRIBUTOR"))}) + self.assertTrue(result) + + def test_unrelated_accepted_issue_does_not_qualify(self): + result, _ = self.evaluate() + self.assertFalse(result) + + def test_submitter_and_assignee_of_unaccepted_issue_do_not_qualify(self): + for value in ( + issue(author="contributor", labels=()), + issue(assignees=("contributor",), labels=("state:validated",)), + ): + with self.subTest(issue=value): + result, _ = self.evaluate(issues={1: value}) + self.assertFalse(result) + + def test_deleted_submitter_does_not_block_current_assignee(self): + result, _ = self.evaluate( + issues={1: issue(author=None, assignees=("contributor",))} + ) + self.assertTrue(result) + + def test_cross_repository_accepted_issue_does_not_qualify(self): + result, calls = self.evaluate( + references=[reference(repository="purp/OpenShell")] + ) + self.assertFalse(result) + self.assertEqual(calls.call_count, 2) + + def test_target_repository_identity_is_case_insensitive(self): + result, _ = self.evaluate( + references=[reference(repository="nvidia/openshell")], + issues={1: issue(author="contributor")}, + ) + self.assertTrue(result) + + def test_at_least_one_issue_must_have_both_acceptance_and_relationship(self): + result, _ = self.evaluate( + references=[reference(1), reference(2)], + issues={1: issue(), 2: issue(author="contributor", labels=())}, + ) + self.assertFalse(result) + result, _ = self.evaluate( + references=[reference(1), reference(2)], + issues={1: issue(labels=()), 2: issue(assignees=("contributor",))}, + ) + self.assertTrue(result) + + def test_arbitrary_mentions_and_body_claims_do_not_qualify(self): + pull = pr() + pull["body"] = "Accepted work: #1. No issue required: maintainer requested." + result, _ = self.evaluate(pull=pull, references=[]) + self.assertFalse(result) + + def test_another_pr_is_not_a_qualifying_issue(self): + value = issue(author="contributor") + value["pull_request"] = {"url": "pull"} + result, _ = self.evaluate(issues={1: value}) + self.assertFalse(result) + + def test_bots_have_no_automatic_exemption(self): + result, _ = self.evaluate(pull=pr(author="dependabot[bot]"), references=[]) + self.assertFalse(result) + + def test_noncollaborator_404_still_qualifies_as_submitter(self): + error = subprocess.CalledProcessError( + 1, "gh", stderr="gh: Not Found (HTTP 404)" + ) + result, _ = self.evaluate( + permission=error, issues={1: issue(author="contributor")} + ) + self.assertTrue(result) + + def test_other_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.evaluate(permission=error) + + def test_closing_issue_pagination_reaches_later_pages(self): + responses = [ + connection([reference(1)], more=True, cursor="next"), + connection([reference(2)]), + ] + with mock.patch.object(labeling, "_api", side_effect=responses) as api: + refs = list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) + self.assertEqual(refs, [reference(1), reference(2)]) + self.assertEqual(api.call_args.kwargs["data"]["variables"]["cursor"], "next") + + def test_graphql_errors_do_not_become_nonacceptance(self): + values = [{"errors": [{"message": "denied"}]}, {"data": {"repository": None}}] + for value in values: + with ( + self.subTest(value=value), + mock.patch.object(labeling, "_api", return_value=value), + self.assertRaises((ValueError, TypeError)), + ): + list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) + + def test_missing_pagination_cursor_fails(self): + with mock.patch.object( + labeling, "_api", return_value=connection([], more=True) + ): + with self.assertRaisesRegex(ValueError, "cursor"): + list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) + + def test_permissions_and_shared_issues_are_cached_within_one_sweep(self): + evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) + responses = [ + {"permission": "read"}, + connection([reference()]), + issue(author="contributor"), + [{"name": labeling.ACCEPTED}], + connection([reference()]), + ] + with mock.patch.object(labeling, "_api", side_effect=responses) as api: + self.assertTrue(evaluator.accepted(pr())) + self.assertTrue(evaluator.accepted(pr(number=8))) + self.assertEqual(api.call_count, 5) + + +class ReconciliationTests(unittest.TestCase): + def test_adds_only_accepted_label_and_preserves_other_labels(self): + evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) + with mock.patch.object(labeling, "_api") as api: + plan = evaluator.reconcile( + [pr(author="maintainer", labels=("area:build",))], apply=True + ) + self.assertEqual(plan, [{"number": 7, "accepted": True, "action": "add"}]) + api.assert_called_once_with( + f"{ROOT}/issues/7/labels", + method="POST", + data={"labels": [labeling.ACCEPTED]}, + ) + + def test_removes_stale_or_manually_applied_acceptance_only(self): + evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) + with ( + mock.patch.object(evaluator, "accepted", return_value=False), + mock.patch.object(labeling, "_api") as api, + ): + evaluator.reconcile( + [pr(labels=(labeling.ACCEPTED, "area:build"))], apply=True + ) + api.assert_called_once_with( + f"{ROOT}/issues/7/labels/state%3Aaccepted", method="DELETE" + ) + + def test_idempotent_for_labeled_accepted_and_unlabeled_unaccepted(self): + evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) + with ( + mock.patch.object(evaluator, "accepted", side_effect=[True, False]), + mock.patch.object(labeling, "_api") as api, + ): + plans = evaluator.reconcile( + [pr(labels=(labeling.ACCEPTED,)), pr(number=8)], apply=True + ) + self.assertEqual([plan["action"] for plan in plans], ["none", "none"]) + api.assert_not_called() + + def test_preview_never_mutates(self): + with mock.patch.object(labeling, "_api") as api: + plan = labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( + [pr(author="maintainer")] + ) + self.assertEqual(plan[0]["action"], "add") + api.assert_not_called() + + def test_drafts_qualify_and_closed_prs_are_skipped(self): + with mock.patch.object(labeling, "_api") as api: + plan = labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( + [pr(author="maintainer", draft=True), pr(number=8, state="closed")] + ) + self.assertEqual(len(plan), 1) + self.assertTrue(plan[0]["accepted"]) + api.assert_not_called() + + def test_lookup_failure_does_not_apply_any_pending_changes(self): + evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) + with ( + mock.patch.object( + evaluator, "accepted", side_effect=[True, ValueError("lookup failed")] + ), + mock.patch.object(labeling, "_api") as api, + ): + with self.assertRaisesRegex(ValueError, "lookup failed"): + evaluator.reconcile([pr(), pr(number=8)], apply=True) + api.assert_not_called() + + def test_mutation_failure_propagates(self): + with mock.patch.object(labeling, "_api", side_effect=OSError("denied")): + with self.assertRaisesRegex(OSError, "denied"): + labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( + [pr(author="maintainer")], apply=True + ) + + def test_fresh_run_reflects_acceptance_assignment_and_permission_changes(self): + for first, second in ( + (issue(assignees=("contributor",)), issue()), + (issue(author="contributor"), issue(author="contributor", labels=())), + ): + with self.subTest(first=first): + accepted, _ = AcceptanceTests().evaluate(issues={1: first}) + removed, _ = AcceptanceTests().evaluate(issues={1: second}) + self.assertTrue(accepted) + self.assertFalse(removed) + self.assertTrue(AcceptanceTests().evaluate(permission="write")[0]) + self.assertFalse(AcceptanceTests().evaluate(permission="read")[0]) + + +class ApiAndCliTests(unittest.TestCase): + def test_api_flattens_all_pages_and_posts_json(self): + with mock.patch.object(labeling.subprocess, "run") as run: + run.return_value.stdout = '[[{"number": 1}], [{"number": 2}]]' + self.assertEqual( + labeling._api("endpoint", pages=True), [{"number": 1}, {"number": 2}] + ) + self.assertIn("--paginate", run.call_args.args[0]) + self.assertIn("--slurp", run.call_args.args[0]) + labeling._api("graphql", method="POST", data={"query": "query"}) + self.assertEqual( + json.loads(run.call_args.kwargs["input"]), {"query": "query"} + ) + self.assertTrue(run.call_args.kwargs["check"]) + + def test_cli_single_pr_or_paginated_sweep_defaults_to_preview(self): + for target in (["--pr", "7"], ["--all-open"]): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "MAINTAINERS.md" + path.write_text("[@maintainer](https://github.com/maintainer)") + with ( + mock.patch.object( + labeling, "_api", return_value=pr() if "--pr" in target else [pr()] + ) as api, + mock.patch.object( + labeling.AcceptedWork, "reconcile", return_value=[] + ) as reconcile, + contextlib.redirect_stdout(io.StringIO()), + ): + self.assertEqual( + labeling.main( + ["--repo", REPO, "--maintainers", str(path), *target] + ), + 0, + ) + reconcile.assert_called_once_with([pr()], apply=False) + if "--all-open" in target: + api.assert_called_once_with( + f"{ROOT}/pulls?state=open&per_page=100", pages=True + ) + else: + api.assert_called_once_with(f"{ROOT}/pulls/7") + + def test_invalid_roster_fails_before_github_calls(self): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "MAINTAINERS.md" + path.write_text("# No maintainers") + with ( + mock.patch.object(labeling, "_api") as api, + contextlib.redirect_stderr(io.StringIO()), + ): + self.assertEqual( + labeling.main( + [ + "--repo", REPO, "--maintainers", str(path), + "--pr", "7", "--apply", + ] + ), + 1, + ) + api.assert_not_called() + + +if __name__ == "__main__": + unittest.main() diff --git a/tasks/test.toml b/tasks/test.toml index 4309ad95b7..0cc869c0e3 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 accepted-work PR labeling" +run = "uv run --no-project python -m unittest discover -s tasks/scripts -p label_accepted_work_test.py" +hide = true + [e2e] description = "Run all end-to-end tests (Rust + Python + MCP)" depends = ["e2e:rust", "e2e:python", "e2e:mcp"] From da84db414b8c52beaa6abbf87b5bb83611e4062d Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Wed, 4 Nov 2026 17:02:43 +0000 Subject: [PATCH 2/5] ci(review): document trusted accepted-work trigger Signed-off-by: Jim Meyer --- .github/zizmor.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/zizmor.yml b/.github/zizmor.yml index 5e19eaefa9..1371bd9511 100644 --- a/.github/zizmor.yml +++ b/.github/zizmor.yml @@ -6,6 +6,7 @@ rules: ignore: # These base-branch workflows never check out or execute pull request # head code. Keep each suppression scoped to its reviewed trigger block. + - accepted-work.yml:6 - dco.yml:3 - e2e-label-help.yml:13 - maintainers-change-alert.yml:6 From 13b54bb4bb9a41c31c80e116dcc2f4120cbbbffa Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Wed, 4 Nov 2026 17:04:02 +0000 Subject: [PATCH 3/5] test(review): tidy accepted-work regression fixtures Signed-off-by: Jim Meyer --- tasks/scripts/label_accepted_work_test.py | 49 ++++++++++++----------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tasks/scripts/label_accepted_work_test.py b/tasks/scripts/label_accepted_work_test.py index 458780b269..b957ab5d38 100644 --- a/tasks/scripts/label_accepted_work_test.py +++ b/tasks/scripts/label_accepted_work_test.py @@ -43,18 +43,12 @@ def reference(number=1, repository=REPO): def connection(nodes, *, more=False, cursor=None): - return { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": { - "nodes": nodes, - "pageInfo": {"hasNextPage": more, "endCursor": cursor}, - } - } - } - } + linked = { + "nodes": nodes, + "pageInfo": {"hasNextPage": more, "endCursor": cursor}, } + pull = {"closingIssuesReferences": linked} + return {"data": {"repository": {"pullRequest": pull}}} class AcceptanceTests(unittest.TestCase): @@ -212,11 +206,11 @@ def test_graphql_errors_do_not_become_nonacceptance(self): list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) def test_missing_pagination_cursor_fails(self): - with mock.patch.object( - labeling, "_api", return_value=connection([], more=True) + with ( + mock.patch.object(labeling, "_api", return_value=connection([], more=True)), + self.assertRaisesRegex(ValueError, "cursor"), ): - with self.assertRaisesRegex(ValueError, "cursor"): - list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) + list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) def test_permissions_and_shared_issues_are_cached_within_one_sweep(self): evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) @@ -296,17 +290,19 @@ def test_lookup_failure_does_not_apply_any_pending_changes(self): evaluator, "accepted", side_effect=[True, ValueError("lookup failed")] ), mock.patch.object(labeling, "_api") as api, + self.assertRaisesRegex(ValueError, "lookup failed"), ): - with self.assertRaisesRegex(ValueError, "lookup failed"): - evaluator.reconcile([pr(), pr(number=8)], apply=True) + evaluator.reconcile([pr(), pr(number=8)], apply=True) api.assert_not_called() def test_mutation_failure_propagates(self): - with mock.patch.object(labeling, "_api", side_effect=OSError("denied")): - with self.assertRaisesRegex(OSError, "denied"): - labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( - [pr(author="maintainer")], apply=True - ) + with ( + mock.patch.object(labeling, "_api", side_effect=OSError("denied")), + self.assertRaisesRegex(OSError, "denied"), + ): + labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( + [pr(author="maintainer")], apply=True + ) def test_fresh_run_reflects_acceptance_assignment_and_permission_changes(self): for first, second in ( @@ -376,8 +372,13 @@ def test_invalid_roster_fails_before_github_calls(self): self.assertEqual( labeling.main( [ - "--repo", REPO, "--maintainers", str(path), - "--pr", "7", "--apply", + "--repo", + REPO, + "--maintainers", + str(path), + "--pr", + "7", + "--apply", ] ), 1, From efa596ab3ea2396c7e2ef0e9d8833f8af8ac6787 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Wed, 4 Nov 2026 17:07:27 +0000 Subject: [PATCH 4/5] docs(review): explain accepted-work labeling and notifications Signed-off-by: Jim Meyer --- .agents/skills/create-github-issue/SKILL.md | 6 ++++++ .agents/skills/create-github-pr/SKILL.md | 5 +++++ .agents/skills/triage-issue/SKILL.md | 5 +++++ CONTRIBUTING.md | 12 +++++++++++- 4 files changed, 27 insertions(+), 1 deletion(-) 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 2a26393cb2..cdf40a1a54 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -267,6 +267,11 @@ must be accepted and list the PR author as its submitter or a current assignee. The label does not replace the issue-acceptance gate or merge approvals. Do not manually apply it as a substitute for satisfying the automated rule. +Apply area/topic classification labels only once the PR represents accepted +work, following [Labeling and Notifications](../../../CONTRIBUTING.md#labeling-and-notifications). +These labels drive filtered subscriber notifications, including the GitHub app +in Slack; do not apply them to unaccepted work just because its scope is known. + ### 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/CONTRIBUTING.md b/CONTRIBUTING.md index cde84a1762..fe20b85821 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,16 @@ 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. +### 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 represents accepted work. Work submitted by a maintainer or someone with repository `WRITE`, `MAINTAIN`, or `ADMIN` access qualifies immediately. Other issues need a maintainer's acceptance; other PRs must satisfy the [accepted-work definition](#accepted-work), including the issue submitter or assignee relationship. 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: From 87a3b8e4e0006a9c3490abfe9cbbcac141febc59 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Wed, 4 Nov 2026 20:04:26 +0000 Subject: [PATCH 5/5] refactor(review): share accepted-work checks with review-readiness Signed-off-by: Jim Meyer --- .agents/skills/create-github-pr/SKILL.md | 17 +- .github/CODEOWNERS | 4 +- .github/workflows/accepted-work.yml | 71 ---- .github/workflows/branch-checks.yml | 4 +- .github/workflows/review-readiness.yml | 28 +- .github/zizmor.yml | 2 +- CI.md | 2 +- CONTRIBUTING.md | 28 +- tasks/scripts/accepted_work.py | 148 +++++++ tasks/scripts/accepted_work_test.py | 251 ++++++++++++ tasks/scripts/check_review_readiness.py | 75 +++- tasks/scripts/check_review_readiness_test.py | 194 ++++++++- tasks/scripts/label_accepted_work.py | 209 ---------- tasks/scripts/label_accepted_work_test.py | 390 ------------------- tasks/test.toml | 4 +- 15 files changed, 706 insertions(+), 721 deletions(-) delete mode 100644 .github/workflows/accepted-work.yml create mode 100644 tasks/scripts/accepted_work.py create mode 100644 tasks/scripts/accepted_work_test.py delete mode 100644 tasks/scripts/label_accepted_work.py delete mode 100644 tasks/scripts/label_accepted_work_test.py diff --git a/.agents/skills/create-github-pr/SKILL.md b/.agents/skills/create-github-pr/SKILL.md index cdf40a1a54..957dc8bd60 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -258,19 +258,10 @@ automatically on issue-label events. Created PR [#123](https://github.com/OWNER/REPO/pull/123) ``` -### Accepted-Work Classification - -The `Accepted Work` workflow manages the PR's acceptance label using the definition -in [CONTRIBUTING.md](../../../CONTRIBUTING.md#accepted-work). For an author without -maintainer membership or target-repository write access, the same closing issue -must be accepted and list the PR author as its submitter or a current assignee. -The label does not replace the issue-acceptance gate or merge approvals. Do not -manually apply it as a substitute for satisfying the automated rule. - -Apply area/topic classification labels only once the PR represents accepted -work, following [Labeling and Notifications](../../../CONTRIBUTING.md#labeling-and-notifications). -These labels drive filtered subscriber notifications, including the GitHub app -in Slack; do not apply them to unaccepted work just because its scope is known. +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) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 78f5034728..169edf36c6 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -41,8 +41,8 @@ /tasks/scripts/check_issue_acceptance.test.cjs @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr # Accepted-work classification -/tasks/scripts/label_accepted_work.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr -/tasks/scripts/label_accepted_work_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/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/accepted-work.yml b/.github/workflows/accepted-work.yml deleted file mode 100644 index d7712b66c6..0000000000 --- a/.github/workflows/accepted-work.yml +++ /dev/null @@ -1,71 +0,0 @@ -# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 - -name: Accepted Work - -on: - pull_request_target: - types: [opened, edited, reopened, synchronize, ready_for_review, labeled, unlabeled] - issues: - types: [labeled, unlabeled, assigned, unassigned, deleted, transferred] - push: - branches: [main] - paths: - - MAINTAINERS.md - - tasks/scripts/label_accepted_work.py - - tasks/scripts/check_maintainer_approval.py - - .github/workflows/accepted-work.yml - schedule: - # Refresh permissions and closing links that have no dedicated webhook. - - cron: '23 9 * * *' - workflow_dispatch: - -permissions: - contents: read - issues: write - pull-requests: read - -# Issue changes and periodic refreshes reconcile all open PRs. Serialize them -# with individual PR events to avoid older decisions overwriting newer ones. -concurrency: - group: accepted-work - cancel-in-progress: false - -jobs: - label: - name: OpenShell / Accepted Work - if: >- - github.repository_owner == 'NVIDIA' && - (github.event_name != 'issues' || - github.event.action != 'labeled' && github.event.action != 'unlabeled' || - github.event.label.name == 'state:accepted') - runs-on: ubuntu-latest - timeout-minutes: 15 - steps: - # All event types use trusted default-branch code and membership. - - name: Check out accepted-work helper and maintainer list - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - ref: main - sparse-checkout: | - MAINTAINERS.md - tasks/scripts/label_accepted_work.py - tasks/scripts/check_maintainer_approval.py - sparse-checkout-cone-mode: false - persist-credentials: false - - - name: Reconcile accepted-work labels - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - GH_REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - shell: bash - run: | - set -euo pipefail - if [ -n "$PR_NUMBER" ]; then - python3 tasks/scripts/label_accepted_work.py \ - --repo "$GH_REPO" --pr "$PR_NUMBER" --apply - else - python3 tasks/scripts/label_accepted_work.py \ - --repo "$GH_REPO" --all-open --apply - fi diff --git a/.github/workflows/branch-checks.yml b/.github/workflows/branch-checks.yml index ec34fbb879..4e9403c484 100644 --- a/.github/workflows/branch-checks.yml +++ b/.github/workflows/branch-checks.yml @@ -605,8 +605,8 @@ jobs: - name: Test release qualification summary run: mise run test:qualification-summary - - name: Test accepted-work labeling - run: mise run test:accepted-work + - 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 1371bd9511..601855c71f 100644 --- a/.github/zizmor.yml +++ b/.github/zizmor.yml @@ -6,11 +6,11 @@ rules: ignore: # These base-branch workflows never check out or execute pull request # head code. Keep each suppression scoped to its reviewed trigger block. - - accepted-work.yml:6 - dco.yml:3 - e2e-label-help.yml:13 - maintainers-change-alert.yml:6 - 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 0491d75526..678f11255e 100644 --- a/CI.md +++ b/CI.md @@ -703,7 +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/accepted-work.yml` | Reconciles the informational `state:accepted` PR label from trusted maintainer membership, repository write access, or accepted closing issues submitted by or assigned to the PR author. Runs on PR and issue changes, maintainer/helper changes, daily refresh, and manual dispatch. | +| `.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 fe20b85821..48c3beb575 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -131,11 +131,24 @@ 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 represents accepted work. Work submitted by a maintainer or someone with repository `WRITE`, `MAINTAIN`, or `ADMIN` access qualifies immediately. Other issues need a maintainer's acceptance; other PRs must satisfy the [accepted-work definition](#accepted-work), including the issue submitter or assignee relationship. Once accepted, apply the relevant classification labels so subscribers can find the work. +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. @@ -469,18 +482,11 @@ 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. -### Accepted Work - -The `Accepted Work` workflow manages `state:accepted` on open PRs, including drafts. A PR represents accepted work when either: - -- Its author is listed in the merged [MAINTAINERS.md](MAINTAINERS.md) or has `WRITE`, `MAINTAIN`, or `ADMIN` access to this repository. -- 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. A contributor implementing someone else's accepted issue must be assigned to it to qualify through that issue. Acceptance and the author relationship must hold on the same issue; an accepted issue unrelated to the PR author does not qualify them. +### Automatic Acceptance Labeling -The workflow uses the PR author, not the person triggering it. It refreshes the label on PR changes, accepted-issue label and assignment changes, and changes to the merged maintainer list. A daily refresh and manual workflow dispatch reconcile all open PRs, including permission changes and closing-link changes without a dedicated event. The label is removed when a PR no longer qualifies; do not apply it manually to bypass the rule. Closed PRs retain their last label. +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 label 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. The label does not certify that the diff implements the issue, and the existing Dependabot gate exemption does not automatically qualify a PR for the 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 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/scripts/label_accepted_work.py b/tasks/scripts/label_accepted_work.py deleted file mode 100644 index f544a36fb0..0000000000 --- a/tasks/scripts/label_accepted_work.py +++ /dev/null @@ -1,209 +0,0 @@ -#!/usr/bin/env python3 -# /// script -# requires-python = ">=3.11" -# dependencies = [] -# /// - -# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 - -"""Reconcile the informational state:accepted label on open PRs. - -Read trusted MAINTAINERS.md and GitHub metadata, never code from a PR head. -Preview with --repo OWNER/REPO and --pr NUMBER or --all-open; add --apply to label. -""" - -from __future__ import annotations - -import argparse -import json -import re -import subprocess -import sys -from pathlib import Path -from urllib.parse import quote - -from check_maintainer_approval import parse_maintainers - -ACCEPTED = "state:accepted" -CLOSING_ISSUES = """ -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 } - } - } - } -} -""" - - -def _api(path: str, *, method="GET", data=None, pages=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 - - -class AcceptedWork: - """Evaluate current PR authors and issue relationships; cache within a run.""" - - def __init__(self, repository: str, maintainers: set[str]): - if not maintainers: - raise ValueError("Could not parse any maintainers from MAINTAINERS.md") - self.repository = repository - self.maintainers = {login.lower() for login in maintainers} - self.permissions: dict[str, bool] = {} - self.issues: dict[int, dict] = {} - - def privileged(self, author: str) -> bool: - if author in self.maintainers: - return True - if author not in self.permissions: - try: - value = _api( - f"repos/{self.repository}/collaborators/" - f"{quote(author, safe='')}/permission" - ) - except subprocess.CalledProcessError as exc: - # GitHub returns 404 for users who are not collaborators. - # Other errors must stop reconciliation, not revoke acceptance. - if "(HTTP 404)" not in (exc.stderr or ""): - raise - value = {"permission": "none"} - self.permissions[author] = value["permission"] in { - "write", - "maintain", - "admin", - } - return self.permissions[author] - - def closing_issues(self, number: int): - owner, repo = self.repository.split("/") - cursor = None - while True: - value = _api( - "graphql", - method="POST", - data={ - "query": CLOSING_ISSUES, - "variables": { - "owner": owner, - "repo": repo, - "number": number, - "cursor": cursor, - }, - }, - ) - if value.get("errors"): - raise ValueError("GitHub could not resolve closing issues") - pull = value["data"]["repository"]["pullRequest"] - connection = pull["closingIssuesReferences"] - yield from connection["nodes"] - if not connection["pageInfo"]["hasNextPage"]: - return - cursor = connection["pageInfo"]["endCursor"] - if not cursor: - raise ValueError("GitHub returned a missing closing-issue cursor") - - def accepted(self, pr: dict) -> bool: - author = pr["user"]["login"].lower() - if self.privileged(author): - return True - for reference in self.closing_issues(pr["number"]): - repository = reference["repository"]["nameWithOwner"] - if repository.lower() != self.repository.lower(): - continue - number = reference["number"] - if number not in self.issues: - root = f"repos/{self.repository}/issues/{number}" - issue = _api(root) - issue["labels"] = _api(f"{root}/labels?per_page=100", pages=True) - self.issues[number] = issue - issue = self.issues[number] - if "pull_request" in issue: - continue - participants = { - user["login"].lower() - for user in [issue["user"], *issue["assignees"]] - if user - } - if author in participants and any( - label["name"] == ACCEPTED for label in issue["labels"] - ): - return True - return False - - def reconcile(self, prs: list[dict], *, apply=False) -> list[dict]: - # Resolve all reads before mutations, so lookup failures leave labels alone. - plans = [] - for pr in prs: - if pr["state"] != "open": - continue - accepted = self.accepted(pr) - labeled = any(label["name"] == ACCEPTED for label in pr["labels"]) - action = "none" if accepted == labeled else "add" if accepted else "remove" - plans.append( - {"number": pr["number"], "accepted": accepted, "action": action} - ) - if apply: - for plan in plans: - root = f"repos/{self.repository}/issues/{plan['number']}/labels" - if plan["action"] == "add": - _api(root, method="POST", data={"labels": [ACCEPTED]}) - elif plan["action"] == "remove": - _api(f"{root}/{quote(ACCEPTED, safe='')}", method="DELETE") - return plans - - -def main(argv: list[str] | None = None) -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--repo", required=True) - parser.add_argument("--maintainers", type=Path, default=Path("MAINTAINERS.md")) - target = parser.add_mutually_exclusive_group(required=True) - target.add_argument("--pr", type=int) - target.add_argument("--all-open", action="store_true") - parser.add_argument("--apply", action="store_true") - args = parser.parse_args(argv) - if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", args.repo): - parser.error("--repo must be OWNER/REPO") - if args.pr is not None and args.pr <= 0: - parser.error("--pr must be positive") - try: - evaluator = AcceptedWork( - args.repo, parse_maintainers(args.maintainers.read_text(encoding="utf-8")) - ) - root = f"repos/{args.repo}/pulls" - prs = ( - [_api(f"{root}/{args.pr}")] - if args.pr is not None - else _api(f"{root}?state=open&per_page=100", pages=True) - ) - print(json.dumps(evaluator.reconcile(prs, apply=args.apply))) - except ( - OSError, - UnicodeError, - ValueError, - KeyError, - TypeError, - subprocess.CalledProcessError, - ) as exc: - print(f"Accepted work: {exc}", file=sys.stderr) - return 1 - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/tasks/scripts/label_accepted_work_test.py b/tasks/scripts/label_accepted_work_test.py deleted file mode 100644 index b957ab5d38..0000000000 --- a/tasks/scripts/label_accepted_work_test.py +++ /dev/null @@ -1,390 +0,0 @@ -# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 - -"""Offline tests for deterministic accepted-work labeling.""" - -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 label_accepted_work as labeling - -REPO = "NVIDIA/OpenShell" -ROOT = f"repos/{REPO}" - - -def pr(*, author="contributor", number=7, labels=(), state="open", draft=False): - return { - "number": number, - "user": {"login": author}, - "labels": [{"name": name} for name in labels], - "state": state, - "draft": draft, - } - - -def issue(*, author="submitter", assignees=(), labels=(labeling.ACCEPTED,)): - return { - "user": {"login": author} if author else None, - "assignees": [{"login": login} for login in assignees], - "labels": [{"name": name} for name in labels], - } - - -def reference(number=1, repository=REPO): - return {"number": number, "repository": {"nameWithOwner": repository}} - - -def connection(nodes, *, more=False, cursor=None): - linked = { - "nodes": nodes, - "pageInfo": {"hasNextPage": more, "endCursor": cursor}, - } - pull = {"closingIssuesReferences": linked} - return {"data": {"repository": {"pullRequest": pull}}} - - -class AcceptanceTests(unittest.TestCase): - def evaluate(self, *, pull=None, permission="read", references=None, issues=None): - issues = issues or {1: issue()} - references = references if references is not None else [reference()] - - def api(path, **kwargs): - if "/collaborators/" in path: - if isinstance(permission, Exception): - raise permission - return {"permission": permission} - if path == "graphql": - self.assertEqual(kwargs["data"]["variables"]["number"], 7) - return connection(references) - for number, value in issues.items(): - if path == f"{ROOT}/issues/{number}": - return value.copy() - if path == f"{ROOT}/issues/{number}/labels?per_page=100": - self.assertTrue(kwargs["pages"]) - return value["labels"] - self.fail(f"Unexpected API call: {path}") - - with mock.patch.object(labeling, "_api", side_effect=api) as calls: - result = labeling.AcceptedWork(REPO, {"maintainer"}).accepted(pull or pr()) - return result, calls - - def test_maintainer_needs_no_permission_or_issue_lookup(self): - result, calls = self.evaluate(pull=pr(author="MAINTAINER")) - self.assertTrue(result) - calls.assert_not_called() - - def test_empty_maintainer_roster_is_an_error(self): - with self.assertRaisesRegex(ValueError, "Could not parse"): - labeling.AcceptedWork(REPO, set()) - - def test_write_maintain_and_admin_qualify_without_an_issue(self): - for permission in ("write", "maintain", "admin"): - with self.subTest(permission=permission): - result, calls = self.evaluate(permission=permission, references=[]) - self.assertTrue(result) - self.assertEqual(calls.call_count, 1) - - def test_read_triage_and_no_access_need_an_issue_relationship(self): - for permission in ("read", "triage", "none"): - with self.subTest(permission=permission): - result, _ = self.evaluate(permission=permission) - self.assertFalse(result) - - def test_accepted_issue_submitter_qualifies_case_insensitively(self): - result, _ = self.evaluate(issues={1: issue(author="CONTRIBUTOR")}) - self.assertTrue(result) - - def test_current_assignee_qualifies_case_insensitively(self): - result, _ = self.evaluate(issues={1: issue(assignees=("other", "CONTRIBUTOR"))}) - self.assertTrue(result) - - def test_unrelated_accepted_issue_does_not_qualify(self): - result, _ = self.evaluate() - self.assertFalse(result) - - def test_submitter_and_assignee_of_unaccepted_issue_do_not_qualify(self): - for value in ( - issue(author="contributor", labels=()), - issue(assignees=("contributor",), labels=("state:validated",)), - ): - with self.subTest(issue=value): - result, _ = self.evaluate(issues={1: value}) - self.assertFalse(result) - - def test_deleted_submitter_does_not_block_current_assignee(self): - result, _ = self.evaluate( - issues={1: issue(author=None, assignees=("contributor",))} - ) - self.assertTrue(result) - - def test_cross_repository_accepted_issue_does_not_qualify(self): - result, calls = self.evaluate( - references=[reference(repository="purp/OpenShell")] - ) - self.assertFalse(result) - self.assertEqual(calls.call_count, 2) - - def test_target_repository_identity_is_case_insensitive(self): - result, _ = self.evaluate( - references=[reference(repository="nvidia/openshell")], - issues={1: issue(author="contributor")}, - ) - self.assertTrue(result) - - def test_at_least_one_issue_must_have_both_acceptance_and_relationship(self): - result, _ = self.evaluate( - references=[reference(1), reference(2)], - issues={1: issue(), 2: issue(author="contributor", labels=())}, - ) - self.assertFalse(result) - result, _ = self.evaluate( - references=[reference(1), reference(2)], - issues={1: issue(labels=()), 2: issue(assignees=("contributor",))}, - ) - self.assertTrue(result) - - def test_arbitrary_mentions_and_body_claims_do_not_qualify(self): - pull = pr() - pull["body"] = "Accepted work: #1. No issue required: maintainer requested." - result, _ = self.evaluate(pull=pull, references=[]) - self.assertFalse(result) - - def test_another_pr_is_not_a_qualifying_issue(self): - value = issue(author="contributor") - value["pull_request"] = {"url": "pull"} - result, _ = self.evaluate(issues={1: value}) - self.assertFalse(result) - - def test_bots_have_no_automatic_exemption(self): - result, _ = self.evaluate(pull=pr(author="dependabot[bot]"), references=[]) - self.assertFalse(result) - - def test_noncollaborator_404_still_qualifies_as_submitter(self): - error = subprocess.CalledProcessError( - 1, "gh", stderr="gh: Not Found (HTTP 404)" - ) - result, _ = self.evaluate( - permission=error, issues={1: issue(author="contributor")} - ) - self.assertTrue(result) - - def test_other_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.evaluate(permission=error) - - def test_closing_issue_pagination_reaches_later_pages(self): - responses = [ - connection([reference(1)], more=True, cursor="next"), - connection([reference(2)]), - ] - with mock.patch.object(labeling, "_api", side_effect=responses) as api: - refs = list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) - self.assertEqual(refs, [reference(1), reference(2)]) - self.assertEqual(api.call_args.kwargs["data"]["variables"]["cursor"], "next") - - def test_graphql_errors_do_not_become_nonacceptance(self): - values = [{"errors": [{"message": "denied"}]}, {"data": {"repository": None}}] - for value in values: - with ( - self.subTest(value=value), - mock.patch.object(labeling, "_api", return_value=value), - self.assertRaises((ValueError, TypeError)), - ): - list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) - - def test_missing_pagination_cursor_fails(self): - with ( - mock.patch.object(labeling, "_api", return_value=connection([], more=True)), - self.assertRaisesRegex(ValueError, "cursor"), - ): - list(labeling.AcceptedWork(REPO, {"maintainer"}).closing_issues(7)) - - def test_permissions_and_shared_issues_are_cached_within_one_sweep(self): - evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) - responses = [ - {"permission": "read"}, - connection([reference()]), - issue(author="contributor"), - [{"name": labeling.ACCEPTED}], - connection([reference()]), - ] - with mock.patch.object(labeling, "_api", side_effect=responses) as api: - self.assertTrue(evaluator.accepted(pr())) - self.assertTrue(evaluator.accepted(pr(number=8))) - self.assertEqual(api.call_count, 5) - - -class ReconciliationTests(unittest.TestCase): - def test_adds_only_accepted_label_and_preserves_other_labels(self): - evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) - with mock.patch.object(labeling, "_api") as api: - plan = evaluator.reconcile( - [pr(author="maintainer", labels=("area:build",))], apply=True - ) - self.assertEqual(plan, [{"number": 7, "accepted": True, "action": "add"}]) - api.assert_called_once_with( - f"{ROOT}/issues/7/labels", - method="POST", - data={"labels": [labeling.ACCEPTED]}, - ) - - def test_removes_stale_or_manually_applied_acceptance_only(self): - evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) - with ( - mock.patch.object(evaluator, "accepted", return_value=False), - mock.patch.object(labeling, "_api") as api, - ): - evaluator.reconcile( - [pr(labels=(labeling.ACCEPTED, "area:build"))], apply=True - ) - api.assert_called_once_with( - f"{ROOT}/issues/7/labels/state%3Aaccepted", method="DELETE" - ) - - def test_idempotent_for_labeled_accepted_and_unlabeled_unaccepted(self): - evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) - with ( - mock.patch.object(evaluator, "accepted", side_effect=[True, False]), - mock.patch.object(labeling, "_api") as api, - ): - plans = evaluator.reconcile( - [pr(labels=(labeling.ACCEPTED,)), pr(number=8)], apply=True - ) - self.assertEqual([plan["action"] for plan in plans], ["none", "none"]) - api.assert_not_called() - - def test_preview_never_mutates(self): - with mock.patch.object(labeling, "_api") as api: - plan = labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( - [pr(author="maintainer")] - ) - self.assertEqual(plan[0]["action"], "add") - api.assert_not_called() - - def test_drafts_qualify_and_closed_prs_are_skipped(self): - with mock.patch.object(labeling, "_api") as api: - plan = labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( - [pr(author="maintainer", draft=True), pr(number=8, state="closed")] - ) - self.assertEqual(len(plan), 1) - self.assertTrue(plan[0]["accepted"]) - api.assert_not_called() - - def test_lookup_failure_does_not_apply_any_pending_changes(self): - evaluator = labeling.AcceptedWork(REPO, {"maintainer"}) - with ( - mock.patch.object( - evaluator, "accepted", side_effect=[True, ValueError("lookup failed")] - ), - mock.patch.object(labeling, "_api") as api, - self.assertRaisesRegex(ValueError, "lookup failed"), - ): - evaluator.reconcile([pr(), pr(number=8)], apply=True) - api.assert_not_called() - - def test_mutation_failure_propagates(self): - with ( - mock.patch.object(labeling, "_api", side_effect=OSError("denied")), - self.assertRaisesRegex(OSError, "denied"), - ): - labeling.AcceptedWork(REPO, {"maintainer"}).reconcile( - [pr(author="maintainer")], apply=True - ) - - def test_fresh_run_reflects_acceptance_assignment_and_permission_changes(self): - for first, second in ( - (issue(assignees=("contributor",)), issue()), - (issue(author="contributor"), issue(author="contributor", labels=())), - ): - with self.subTest(first=first): - accepted, _ = AcceptanceTests().evaluate(issues={1: first}) - removed, _ = AcceptanceTests().evaluate(issues={1: second}) - self.assertTrue(accepted) - self.assertFalse(removed) - self.assertTrue(AcceptanceTests().evaluate(permission="write")[0]) - self.assertFalse(AcceptanceTests().evaluate(permission="read")[0]) - - -class ApiAndCliTests(unittest.TestCase): - def test_api_flattens_all_pages_and_posts_json(self): - with mock.patch.object(labeling.subprocess, "run") as run: - run.return_value.stdout = '[[{"number": 1}], [{"number": 2}]]' - self.assertEqual( - labeling._api("endpoint", pages=True), [{"number": 1}, {"number": 2}] - ) - self.assertIn("--paginate", run.call_args.args[0]) - self.assertIn("--slurp", run.call_args.args[0]) - labeling._api("graphql", method="POST", data={"query": "query"}) - self.assertEqual( - json.loads(run.call_args.kwargs["input"]), {"query": "query"} - ) - self.assertTrue(run.call_args.kwargs["check"]) - - def test_cli_single_pr_or_paginated_sweep_defaults_to_preview(self): - for target in (["--pr", "7"], ["--all-open"]): - with tempfile.TemporaryDirectory() as directory: - path = Path(directory) / "MAINTAINERS.md" - path.write_text("[@maintainer](https://github.com/maintainer)") - with ( - mock.patch.object( - labeling, "_api", return_value=pr() if "--pr" in target else [pr()] - ) as api, - mock.patch.object( - labeling.AcceptedWork, "reconcile", return_value=[] - ) as reconcile, - contextlib.redirect_stdout(io.StringIO()), - ): - self.assertEqual( - labeling.main( - ["--repo", REPO, "--maintainers", str(path), *target] - ), - 0, - ) - reconcile.assert_called_once_with([pr()], apply=False) - if "--all-open" in target: - api.assert_called_once_with( - f"{ROOT}/pulls?state=open&per_page=100", pages=True - ) - else: - api.assert_called_once_with(f"{ROOT}/pulls/7") - - def test_invalid_roster_fails_before_github_calls(self): - with tempfile.TemporaryDirectory() as directory: - path = Path(directory) / "MAINTAINERS.md" - path.write_text("# No maintainers") - with ( - mock.patch.object(labeling, "_api") as api, - contextlib.redirect_stderr(io.StringIO()), - ): - self.assertEqual( - labeling.main( - [ - "--repo", - REPO, - "--maintainers", - str(path), - "--pr", - "7", - "--apply", - ] - ), - 1, - ) - api.assert_not_called() - - -if __name__ == "__main__": - unittest.main() diff --git a/tasks/test.toml b/tasks/test.toml index 0cc869c0e3..c9dab418a7 100644 --- a/tasks/test.toml +++ b/tasks/test.toml @@ -104,8 +104,8 @@ run = "uv run --no-project python -m unittest discover -s tasks/scripts -p check hide = true ["test:accepted-work"] -description = "Test accepted-work PR labeling" -run = "uv run --no-project python -m unittest discover -s tasks/scripts -p label_accepted_work_test.py" +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]