diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3e800debdf..1333366006 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. +Use exactly one `area:` label and any relevant `topic:` labels on your pull request. The [reviewer routing workflow](.github/workflows/review-readiness.yml) requests 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. GitHub requires requested reviewers to be repository collaborators; listing someone in the roster does not grant access. Routing reports and skips accounts GitHub rejects for this reason, so eligible reviewers still receive requests. 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. diff --git a/tasks/scripts/check_review_readiness.py b/tasks/scripts/check_review_readiness.py index 0e2fff2e7b..d795559fcb 100644 --- a/tasks/scripts/check_review_readiness.py +++ b/tasks/scripts/check_review_readiness.py @@ -206,6 +206,40 @@ def _routing_key(pr: dict): ) +def _non_collaborator_rejection(error: subprocess.CalledProcessError) -> bool: + try: + body = json.loads(error.stdout) + except (TypeError, ValueError): + return False + return ( + isinstance(body, dict) + and str(body.get("status")) == "422" + and isinstance(body.get("message"), str) + and body["message"].startswith( + "Reviews may only be requested from collaborators." + ) + ) + + +def _request_reviewers(endpoint: str, reviewers: tuple[str, ...]) -> tuple[str, ...]: + try: + _api(endpoint, method="POST", data={"reviewers": list(reviewers)}) + except subprocess.CalledProcessError as error: + if not _non_collaborator_rejection(error): + raise + if len(reviewers) > 1: + return tuple( + login for login in reviewers if _request_reviewers(endpoint, (login,)) + ) + print( + f"::warning::Cannot request review from @{reviewers[0]}: " + "GitHub requires repository collaborator access.", + file=sys.stderr, + ) + return () + return reviewers + + def check_review_readiness( categories: dict[str, set[str]], repository: str, @@ -224,7 +258,7 @@ def check_review_readiness( if apply: endpoint = f"{pull}/requested_reviewers" if plan.add: - _api(endpoint, method="POST", data={"reviewers": list(plan.add)}) + plan = ReviewPlan(_request_reviewers(endpoint, plan.add), plan.remove) if plan.remove: # Re-read human choices and engagement immediately before removals. latest = _api(pull) diff --git a/tasks/scripts/check_review_readiness_test.py b/tasks/scripts/check_review_readiness_test.py index 5914788c44..43094e6101 100644 --- a/tasks/scripts/check_review_readiness_test.py +++ b/tasks/scripts/check_review_readiness_test.py @@ -51,6 +51,12 @@ def event(number, login, *, human=False, removed=False): } +def request_error(status, message): + return subprocess.CalledProcessError( + 1, "gh", output=json.dumps({"status": str(status), "message": message}) + ) + + class RegistryTests(unittest.TestCase): def test_reads_only_membership_rows_and_allows_empty_categories(self): text = DOCUMENT.replace( @@ -274,6 +280,126 @@ def test_requests_roster_members_without_permission_lookups(self): ], ) + def test_unavailable_members_do_not_block_other_requests_or_cleanup(self): + categories = {"area:runtime": {"alice", "bob", "carol"}} + before = pr("area:runtime", requested=("old", "manual", "engaged")) + after = pr( + "area:runtime", requested=("old", "manual", "engaged", "alice", "carol") + ) + unavailable = request_error( + 422, "Reviews may only be requested from collaborators." + ) + errors = io.StringIO() + with contextlib.redirect_stderr(errors): + plan, api, _ = self.run_routing( + categories, + snapshots=[ + reviewers( + requested=("old", "manual", "engaged"), + manual=("manual",), + engaged=("engaged",), + ), + reviewers( + requested=("old", "manual", "engaged", "alice", "carol"), + manual=("manual",), + engaged=("engaged",), + ), + ], + responses=[before, unavailable, None, unavailable, None, after, None], + ) + self.assertEqual(plan, routing.ReviewPlan(("alice", "carol"), ("old",))) + self.assertEqual( + api.call_args_list, + [ + mock.call(PULL), + *[ + mock.call( + f"{PULL}/requested_reviewers", + method="POST", + data={"reviewers": members}, + ) + for members in ( + ["alice", "bob", "carol"], + ["alice"], + ["bob"], + ["carol"], + ) + ], + mock.call(PULL), + mock.call( + f"{PULL}/requested_reviewers", + method="DELETE", + data={"reviewers": ["old"]}, + ), + ], + ) + self.assertIn("@bob", errors.getvalue()) + self.assertNotIn("@alice", errors.getvalue()) + self.assertNotIn("@carol", errors.getvalue()) + + def test_single_unavailable_reviewer_is_reported_without_retrying(self): + errors = io.StringIO() + with contextlib.redirect_stderr(errors): + plan, api, _ = self.run_routing( + {"area:runtime": {"alice"}}, + snapshots=[reviewers()], + responses=[ + pr("area:runtime"), + request_error( + 422, "Reviews may only be requested from collaborators." + ), + ], + ) + self.assertEqual(plan, routing.ReviewPlan((), ())) + self.assertEqual(api.call_count, 2) + self.assertIn("@alice", errors.getvalue()) + + def test_other_request_failures_still_fail_without_removing_reviewers(self): + failures = [ + request_error(403, "Resource not accessible by integration"), + request_error(422, "Validation Failed"), + request_error(503, "Service unavailable"), + subprocess.CalledProcessError(1, "gh", output="not JSON"), + ] + for failure in failures: + for retry in (False, True): + with ( + self.subTest(failure=failure.stdout, retry=retry), + mock.patch.object( + routing, + "_api", + side_effect=[ + pr("area:runtime", requested=("old",)), + *( + [ + request_error( + 422, + "Reviews may only be requested from collaborators.", + ), + None, + ] + if retry + else [] + ), + failure, + ], + ) as api, + mock.patch.object( + routing, + "_load_reviewers", + return_value=reviewers(requested=("old",)), + ), + ): + with self.assertRaises(subprocess.CalledProcessError) as raised: + routing.check_review_readiness(CATEGORIES, REPO, 42, apply=True) + self.assertIs(raised.exception, failure) + self.assertFalse( + any( + call.kwargs.get("method") == "DELETE" + for call in api.call_args_list + ) + ) + def test_empty_area_does_not_block_topic_reviewers(self): categories = {"area:runtime": set(), "topic:policy": {"carol"}} plan, _, _ = self.run_routing(