fix(github): teams with access show in the reviewer picker

packages/source-control-github/src/server/GitHubPullRequestApi.test.ts

@@ -3770,42 +3770,74 @@ layer("GitHubPullRequestApi.layer", (it) => {
     }),
   );
 
-  it.effect("reads who may review and who already has in one request", () =>
+  const reviewerTeam = {
+    slug: "web",
+    combinedSlug: "acme/web",
+    name: "Web",
+    avatarUrl: null,
+    repositories: { nodes: [{ nameWithOwner: "acme/web" }] },
+  };
+  it.effect.each([
+    {
+      name: "lists teams with access beside the people",
+      teams: {
+        data: {
+          organization: { teams: { pageInfo: { hasNextPage: false }, nodes: [reviewerTeam] } },
+        },
+      },
+      expected: [
+        ["octocat", true],
+        ["acme/web", false],
+        ["hubot", false],
+      ],
+    },
+    {
+      name: "still lists the people when the token cannot read the organization's teams",
+      teams: {
+        data: null,
+        errors: [{ type: "INSUFFICIENT_SCOPES", message: "Your token needs read:org." }],
+      },
+      expected: [
+        ["octocat", true],
+        ["hubot", false],
+      ],
+    },
+  ])("$name", ({ teams, expected }) =>
     Effect.gen(function* () {
-      mockedExecute.mockReturnValue(
-        Effect.succeed(
-          output(
-            encodeJson({
-              data: {
-                repository: {
-                  assignableUsers: {
-                    pageInfo: { hasNextPage: false },
-                    nodes: ["bilal", "octocat", "hubot"].map((login) => ({
-                      login,
-                      name: null,
-                      avatarUrl: `https://avatars/${login}`,
-                    })),
-                  },
-                  pullRequest: {
-                    author: { __typename: "User", login: "bilal" },
-                    reviewRequests: {
-                      nodes: [
-                        {
-                          requestedReviewer: {
-                            __typename: "User",
-                            login: "octocat",
-                            name: null,
-                            avatarUrl: "https://avatars/octocat",
-                          },
+      route(
+        [
+          "ReviewerCandidates",
+          {
+            data: {
+              repository: {
+                assignableUsers: {
+                  pageInfo: { hasNextPage: false },
+                  nodes: ["bilal", "octocat", "hubot"].map((login) => ({
+                    login,
+                    name: null,
+                    avatarUrl: `https://avatars/${login}`,
+                  })),
+                },
+                pullRequest: {
+                  author: { __typename: "User", login: "bilal" },
+                  reviewRequests: {
+                    nodes: [
+                      {
+                        requestedReviewer: {
+                          __typename: "User",
+                          login: "octocat",
+                          name: null,
+                          avatarUrl: "https://avatars/octocat",
                         },
-                      ],
-                    },
+                      },
+                    ],
                   },
                 },
               },
-            }),
-          ),
-        ),
+            },
+          },
+        ],
+        ["ReviewerTeams", teams],
       );
       const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
 
@@ -3816,12 +3848,13 @@ layer("GitHubPullRequestApi.layer", (it) => {
         number: 7,
       });
 
-      assert.strictEqual(mockedExecute.mock.calls.length, 1);
-      expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 });
-      expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([
-        ["octocat", true],
-        ["hubot", false],
+      expect(variablesOf("ReviewerCandidates")).toEqual([
+        { owner: "acme", name: "web", number: 7 },
       ]);
+      expect(variablesOf("ReviewerTeams")).toEqual([{ owner: "acme", name: "web" }]);
+      expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual(
+        expected,
+      );
     }),
   );
 

packages/source-control-github/src/server/GitHubPullRequestApi.ts

@@ -86,6 +86,7 @@ import {
   toPullRequestWatchFingerprint,
   toReviewDismissalsPage,
   toReviewerCandidates,
+  toReviewerTeams,
   toReviewThreadCommentsPage,
   toReviewThreadPage,
   toStackMembership,
@@ -2279,13 +2280,22 @@ export const make = Effect.gen(function* () {
       function (input) {
         const { owner, name } = parseRepositorySelector(input.repository);
         const operation = "listReviewerCandidates";
-        return client
-          .ReviewerCandidates({ owner, name, number: input.number })
-          .pipe(
-            graphql({ ...input, operation, allowReserve: true }),
-            Effect.map(toReviewerCandidates),
-            Effect.flatMap(found({ ...input, operation })),
-          );
+        const target = { ...input, operation, allowReserve: true };
+        return Effect.all(
+          [
+            client.ReviewerCandidates({ owner, name, number: input.number }).pipe(graphql(target)),
+            client.ReviewerTeams({ owner, name }).pipe(
+              graphql(target),
+              Effect.map((result) => toReviewerTeams(result, `${owner}/${name}`)),
+              // Teams need `read:org` and an organization owner; people are still worth listing.
+              Effect.orElseSucceed(() => null),
+            ),
+          ],
+          { concurrency: "unbounded" },
+        ).pipe(
+          Effect.map(([people, teams]) => toReviewerCandidates(people, teams)),
+          Effect.flatMap(found({ ...input, operation })),
+        );
       },
     ),
 

packages/source-control-github/src/server/gitHubPullRequestJson.test.ts

@@ -17,6 +17,7 @@ import {
   toPullRequestFilesViewedPage,
   toPullRequestRows,
   toReviewerCandidates,
+  toReviewerTeams,
   toReviewThreadCommentsPage,
   toCanWrite,
   toReviewThreadPage,
@@ -29,6 +30,7 @@ import {
   PullRequestCore,
   PullRequestFilesViewed,
   ReviewerCandidates,
+  ReviewerTeams,
   ViewerPermissions,
 } from "./pullRequestReads.graphql.ts";
 import { ReviewThreadComments, ReviewThreads } from "./pullRequestReviews.graphql.ts";
@@ -207,6 +209,8 @@ const decodeReviewThreadCommentsJson = (raw: string) =>
 const decodeViewerPermissionsJson = through(ViewerPermissions.result, toViewerPermissions);
 const decodeLabelCandidatesJson = through(LabelCandidates.result, toLabelCandidates);
 const decodeReviewerCandidatesJson = through(ReviewerCandidates.result, toReviewerCandidates);
+const decodeReviewerCandidates = Schema.decodeUnknownSync(ReviewerCandidates.result);
+const decodeReviewerTeams = Schema.decodeSync(ReviewerTeams.result);
 const decodePullRequestFilesViewedJson = through(
   PullRequestFilesViewed.result,
   toPullRequestFilesViewedPage,
@@ -1335,6 +1339,48 @@ describe("reviewer candidate decoding", () => {
       },
     ]);
   });
+
+  it("offers the organization's teams that can reach this repository, each once", () => {
+    const team = (slug: string, repositories: ReadonlyArray<string>) => ({
+      slug,
+      combinedSlug: `acme/${slug}`,
+      name: slug,
+      avatarUrl: null,
+      repositories: { nodes: repositories.map((nameWithOwner) => ({ nameWithOwner })) },
+    });
+    const teams = decodeReviewerTeams({
+      organization: {
+        teams: {
+          pageInfo: { hasNextPage: true },
+          nodes: [
+            team("reviewers", ["acme/web"]),
+            team("frontend", ["ACME/Web"]),
+            team("legacy", ["acme/web-legacy"]),
+          ],
+        },
+      },
+    });
+    const people = JSON.parse(
+      candidatesJson({
+        assignable: [{ login: "octocat" }],
+        requested: [{ slug: "reviewers", combinedSlug: "acme/reviewers", teamName: "Reviewers" }],
+      }),
+    ) as { readonly data: unknown };
+
+    const list = toReviewerCandidates(
+      decodeReviewerCandidates(people.data),
+      toReviewerTeams(teams, "acme/web"),
+    );
+
+    expect(
+      list?.candidates.map(({ kind, login, isRequested }) => [kind, login, isRequested]),
+    ).toEqual([
+      ["team", "acme/reviewers", true],
+      ["team", "acme/frontend", false],
+      ["user", "octocat", false],
+    ]);
+    expect(list?.truncated).toBe(true);
+  });
 });
 
 describe("reviewer request payload", () => {

packages/source-control-github/src/server/gitHubPullRequestJson.ts

@@ -64,6 +64,7 @@ import type {
   RepositoryAuthors,
   RepositoryLabels,
   ReviewerCandidates,
+  ReviewerTeams,
   ViewerPermissions,
 } from "./pullRequestReads.graphql.ts";
 import type {
@@ -1431,9 +1432,40 @@ export interface GitHubBaseComparison {
   readonly viewerCanUpdate: boolean;
 }
 
+/** Teams with access to `repository` (owner/name). Null when the owner is not an organization. */
+export function toReviewerTeams(
+  result: ReviewerTeams.Result,
+  repository: string,
+): PullRequestReviewerCandidateList | null {
+  const teams = result.organization?.teams;
+  if (teams == null) return null;
+  const wanted = repository.toLowerCase();
+  return {
+    candidates: nodesOf(teams).flatMap((team) => {
+      const slug = trimmed(team.slug);
+      const hasAccess = nodesOf(team.repositories).some(
+        (node) => node.nameWithOwner.toLowerCase() === wanted,
+      );
+      if (slug === null || !hasAccess) return [];
+      return [
+        {
+          id: slug,
+          kind: "team" as const,
+          login: trimmed(team.combinedSlug) ?? slug,
+          name: trimmed(team.name),
+          avatarUrl: trimmed(team.avatarUrl),
+          isRequested: false,
+        },
+      ];
+    }),
+    truncated: teams.pageInfo.hasNextPage,
+  };
+}
+
 /** assignableUsers avoids push/read:org requirements. Keep requested outsiders/apps removable. */
 export function toReviewerCandidates(
   result: ReviewerCandidates.Result,
+  teams: PullRequestReviewerCandidateList | null = null,
 ): PullRequestReviewerCandidateList | null {
   const repository = result.repository;
   if (repository == null) return null;
@@ -1457,6 +1489,9 @@ export function toReviewerCandidates(
       isRequested: true,
     });
   }
+  for (const team of teams?.candidates ?? []) {
+    if (!candidates.has(`team ${team.id}`)) candidates.set(`team ${team.id}`, team);
+  }
   for (const node of nodesOf(repository.assignableUsers)) {
     const login = trimmed(node.login);
     if (login === null || login === author || candidates.has(`user ${login}`)) continue;
@@ -1471,7 +1506,7 @@ export function toReviewerCandidates(
   }
   return {
     candidates: [...candidates.values()],
-    truncated: repository.assignableUsers.pageInfo.hasNextPage,
+    truncated: repository.assignableUsers.pageInfo.hasNextPage || teams?.truncated === true,
   };
 }
 

packages/source-control-github/src/server/pullRequestReads.graphql

@@ -404,10 +404,10 @@ query ViewerPermissions($owner: String!, $name: String!, $number: Int!) {
 # the repository) rather than `collaborators`, which the REST API refuses to anyone without push
 # access and which would therefore be empty for exactly the reader most likely to be looking.
 #
-# Teams are asked for only where one has already been requested, so a request to a team can be
-# taken back. The teams a repository could newly be sent to live on the owning organization and
-# need `read:org`, which a repository-scoped token need not carry, and a query GitHub refuses
-# fails whole, taking the people down with the teams.
+# Teams already requested are read here so those requests can be taken back. The teams a review
+# could newly be sent to come from ReviewerTeams, a separate read: they need `read:org`, which a
+# repository-scoped token need not carry, and a query GitHub refuses fails whole, taking the
+# people down with the teams.
 query ReviewerCandidates($owner: String!, $name: String!, $number: Int!) {
   repository(owner: $owner, name: $name) {
     assignableUsers(first: 100) {
@@ -450,6 +450,30 @@ query ReviewerCandidates($owner: String!, $name: String!, $number: Int!) {
   }
 }
 
+# GitHub refuses a review request to a team without access to the repository, and the REST list of
+# a repository's teams needs admin, so each team's repositories are searched for this one by name.
+# `query` matches loosely; the exact repository is picked out from the results.
+query ReviewerTeams($owner: String!, $name: String!) {
+  organization(login: $owner) {
+    teams(first: 100) {
+      pageInfo {
+        hasNextPage
+      }
+      nodes {
+        slug
+        combinedSlug
+        name
+        avatarUrl
+        repositories(query: $name, first: 100) {
+          nodes {
+            nameWithOwner
+          }
+        }
+      }
+    }
+  }
+}
+
 query LabelCandidates($owner: String!, $name: String!, $number: Int!) {
   repository(owner: $owner, name: $name) {
     labels(first: 100, orderBy: { field: NAME, direction: ASC }) {