fix(review): route reviewers without requiring write access (#5257)

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

Author: purp

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) with repository write access, excluding the author, when a PR opens ready for review, leaves draft status, or gains or loses an area/topic label. Manual reviewer choices and people who have commented or reviewed are preserved. Propose roster membership changes through a reviewed PR.
+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.
 
 If your pull request has been idle, ask for a reviewer in the pull request or in the CNCF Slack channel rather than waiting.
 

tasks/scripts/check_review_readiness.py

@@ -10,7 +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.
-Request all matching members with write access, excluding the author. Preserve
+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.
 """
@@ -194,15 +194,6 @@ def _load_reviewers(repository: str, number: int, pr: dict) -> dict[str, set[str
     }
 
 
-def _eligible_reviewers(repository: str, logins: set[str]) -> set[str]:
-    eligible = set()
-    for login in sorted(logins):
-        value = _api(f"repos/{repository}/collaborators/{login}/permission")
-        if value.get("permission") in {"write", "maintain", "admin"}:
-            eligible.add(login)
-    return eligible
-
-
 def _routing_key(pr: dict):
     return (
         pr["state"],
@@ -228,7 +219,7 @@ def check_review_readiness(
     if pr["state"] != "open" or pr["draft"]:
         return None
     selected = select_reviewers(categories, {label["name"] for label in pr["labels"]})
-    selected = _eligible_reviewers(repository, selected - {pr["user"]["login"].lower()})
+    selected -= {pr["user"]["login"].lower()}
     plan = plan_review_requests(selected, **_load_reviewers(repository, number, pr))
     if apply:
         endpoint = f"{pull}/requested_reviewers"

tasks/scripts/check_review_readiness_test.py

@@ -227,17 +227,6 @@ def test_loads_all_engagement_and_timeline_feeds(self):
         self.assertTrue(all(call.kwargs["pages"] for call in api.call_args_list))
         self.assertIn("/timeline?per_page=100", api.call_args.args[0])
 
-    def test_only_current_write_maintain_admin_permissions_are_eligible(self):
-        responses = [
-            {"permission": role}
-            for role in ("admin", "maintain", "read", "triage", "write")
-        ]
-        with mock.patch.object(routing, "_api", side_effect=responses):
-            self.assertEqual(
-                routing._eligible_reviewers(REPO, {"a", "b", "c", "d", "e"}),
-                {"a", "b", "e"},
-            )
-
 
 class ReconciliationTests(unittest.TestCase):
     def run_routing(self, categories, *, snapshots, responses, apply=True):
@@ -246,31 +235,48 @@ def run_routing(self, categories, *, snapshots, responses, apply=True):
             mock.patch.object(
                 routing, "_load_reviewers", side_effect=snapshots
             ) as load,
-            mock.patch.object(
-                routing, "_eligible_reviewers", side_effect=lambda _repo, logins: logins
-            ) as eligible,
         ):
             plan = routing.check_review_readiness(categories, REPO, 42, apply=apply)
-        return plan, api, load, eligible
+        return plan, api, load
 
     def test_initial_assignment_excludes_author_without_extra_refresh(self):
-        plan, api, load, eligible = self.run_routing(
+        plan, api, load = self.run_routing(
             CATEGORIES,
             snapshots=[reviewers()],
             responses=[pr("area:runtime", "topic:policy", author="ALICE"), None],
         )
         self.assertEqual(plan, routing.ReviewPlan(("bob", "carol"), ()))
-        eligible.assert_called_once_with(REPO, {"bob", "carol"})
         load.assert_called_once()
         self.assertEqual(api.call_count, 2)
         self.assertEqual(
             api.call_args.kwargs,
             {"method": "POST", "data": {"reviewers": ["bob", "carol"]}},
         )
 
+    def test_requests_roster_members_without_permission_lookups(self):
+        members = {"read-member", "triage-member", "outside-collaborator"}
+        categories = {"area:runtime": members}
+        plan, api, _ = self.run_routing(
+            categories,
+            snapshots=[reviewers()],
+            responses=[pr("area:runtime"), None],
+        )
+        self.assertEqual(plan, routing.ReviewPlan(tuple(sorted(members)), ()))
+        self.assertEqual(
+            api.call_args_list,
+            [
+                mock.call(PULL),
+                mock.call(
+                    f"{PULL}/requested_reviewers",
+                    method="POST",
+                    data={"reviewers": sorted(members)},
+                ),
+            ],
+        )
+
     def test_empty_area_does_not_block_topic_reviewers(self):
         categories = {"area:runtime": set(), "topic:policy": {"carol"}}
-        plan, _, _, _ = self.run_routing(
+        plan, _, _ = self.run_routing(
             categories,
             snapshots=[reviewers()],
             responses=[pr("area:runtime", "topic:policy")],
@@ -281,16 +287,15 @@ def test_empty_area_does_not_block_topic_reviewers(self):
     def test_draft_and_closed_prs_skip_all_routing(self):
         for value in (pr(draft=True), pr(state="closed")):
             with self.subTest(value=value):
-                plan, api, load, eligible = self.run_routing(
+                plan, api, load = self.run_routing(
                     CATEGORIES, snapshots=[], responses=[value]
                 )
                 self.assertIsNone(plan)
                 api.assert_called_once_with(PULL)
                 load.assert_not_called()
-                eligible.assert_not_called()
 
     def test_preview_makes_no_changes(self):
-        plan, api, _, _ = self.run_routing(
+        plan, api, _ = self.run_routing(
             CATEGORIES,
             snapshots=[reviewers()],
             responses=[pr("area:build")],
@@ -310,7 +315,7 @@ def test_refresh_before_removal_protects_new_manual_choices_and_engagement(self)
                 manual=("carol",),
             ),
         ]
-        plan, api, _, _ = self.run_routing(
+        plan, api, _ = self.run_routing(
             CATEGORIES, snapshots=snapshots, responses=[before, None, after, None]
         )
         self.assertEqual(plan, routing.ReviewPlan(("dave",), ("alice",)))
@@ -338,9 +343,6 @@ def test_changed_routing_or_draft_status_stops_removals(self):
                     "_load_reviewers",
                     return_value=reviewers(requested=("alice", "dave")),
                 ),
-                mock.patch.object(
-                    routing, "_eligible_reviewers", return_value={"dave"}
-                ),
             ):
                 with self.assertRaisesRegex(ValueError, "routing changed"):
                     routing.check_review_readiness(CATEGORIES, REPO, 42, apply=True)
@@ -356,7 +358,7 @@ def test_new_commit_and_unrelated_label_do_not_interrupt_routing(self):
         after = pr("area:build", "kind:bug", requested=("alice", "dave"))
         before["head"], after["head"] = {"sha": "old"}, {"sha": "new"}
         snapshots = [reviewers(requested=("alice", "dave"))] * 2
-        plan, _, _, _ = self.run_routing(
+        plan, _, _ = self.run_routing(
             CATEGORIES, snapshots=snapshots, responses=[before, after, None]
         )
         self.assertEqual(plan.remove, ("alice",))
@@ -374,7 +376,7 @@ def test_retry_after_partial_failure_uses_updated_github_metadata(self):
                 snapshots=snapshots,
                 responses=[before, None, after, subprocess.CalledProcessError(1, "gh")],
             )
-        plan, api, _, _ = self.run_routing(
+        plan, api, _ = self.run_routing(
             CATEGORIES, snapshots=[snapshots[1]] * 2, responses=[after, after, None]
         )
         self.assertEqual(plan, routing.ReviewPlan((), ("alice",)))