From 38d9bd518bfc01482aabb9d476300e7c5df1c39b Mon Sep 17 00:00:00 2001 From: Illia Panasenko Date: Thu, 12 Nov 2026 11:08:35 +0100 Subject: [PATCH 1/2] fix(github): teams with access show in the reviewer picker Co-Authored-By: Claude Opus 5.5 --- .../src/server/GitHubPullRequestApi.test.ts | 148 ++++++++++-------- .../src/server/GitHubPullRequestApi.ts | 27 +++- .../src/server/gitHubPullRequestJson.test.ts | 47 ++++++ .../src/server/gitHubPullRequestJson.ts | 42 ++++- .../src/server/pullRequestReads.graphql | 32 +++- .../src/server/pullRequestReads.graphql.ts | 83 +++++++++- 6 files changed, 291 insertions(+), 88 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index c83692d287fa1..b27dc047122c5 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -4104,42 +4104,70 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - it.effect("reads who may review and who already has in one request", () => - 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", - }, - }, - ], - }, - }, - }, + const reviewerTeam = { + slug: "web", + combinedSlug: "acme/web", + name: "Web", + avatarUrl: null, + repositories: { nodes: [{ nameWithOwner: "acme/web" }] }, + }; + const reviewerTeams = (teams: ReadonlyArray) => ({ + data: { organization: { teams: { pageInfo: { hasNextPage: false }, nodes: teams } } }, + }); + const reviewerPeople = (assignable: ReadonlyArray, requested: ReadonlyArray) => ({ + data: { + repository: { + assignableUsers: { + pageInfo: { hasNextPage: false }, + nodes: assignable.map((login) => ({ + login, + name: null, + avatarUrl: `https://avatars/${login}`, + })), + }, + pullRequest: { + author: { __typename: "User", login: "bilal" }, + reviewRequests: { + nodes: requested.map((login) => ({ + requestedReviewer: { + __typename: "User", + login, + name: null, + avatarUrl: `https://avatars/${login}`, }, - }), - ), - ), + })), + }, + }, + }, + }, + }); + + it.effect.each([ + { + name: "lists teams with access beside the people", + teams: reviewerTeams([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* () { + route( + ["ReviewerCandidates", reviewerPeople(["bilal", "octocat", "hubot"], ["octocat"])], + ["ReviewerTeams", teams], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -4150,45 +4178,21 @@ 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")).toMatchObject([ + { owner: "acme", name: "web", number: 7 }, ]); + expect(variablesOf("ReviewerTeams")).toMatchObject([{ owner: "acme", name: "web" }]); + expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual( + expected, + ); }), ); it.effect("asks GitHub to search everyone assignable when the reviewer menu has a query", () => Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - assignableUsers: { - pageInfo: { hasNextPage: false }, - nodes: [{ login: "octocat", name: null, avatarUrl: "https://avatars/octocat" }], - }, - pullRequest: { - author: { __typename: "User", login: "bilal" }, - reviewRequests: { - nodes: ["octocat", "unrelated"].map((login) => ({ - requestedReviewer: { - __typename: "User", - login, - name: null, - avatarUrl: "https://avatars/user", - }, - })), - }, - }, - }, - }, - }), - ), - ), + route( + ["ReviewerCandidates", reviewerPeople(["octocat"], ["octocat", "unrelated"])], + ["ReviewerTeams", reviewerTeams([reviewerTeam])], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -4200,9 +4204,15 @@ layer("GitHubPullRequestApi.layer", (it) => { query: "octo", }); - expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7, query: "octo" }); + expect(variablesOf("ReviewerCandidates")).toMatchObject([ + { owner: "acme", name: "web", number: 7, query: "octo" }, + ]); + expect(variablesOf("ReviewerTeams")).toMatchObject([ + { owner: "acme", name: "web", query: "octo" }, + ]); expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([ ["octocat", true], + ["acme/web", false], ]); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 45275bef0ae44..1d359c901c1ce 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -99,6 +99,7 @@ import { toPullRequestWatchFingerprint, toReviewDismissalsPage, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toLaterReviewThreadPage, toReviewThreadPage, @@ -2382,13 +2383,25 @@ 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, query: input.query ?? null }) - .pipe( - graphql({ ...input, operation, allowReserve: true }), - Effect.map((result) => toReviewerCandidates(result, input.query)), - Effect.flatMap(found({ ...input, operation })), - ); + const target = { ...input, operation, allowReserve: true }; + const query = input.query ?? null; + return Effect.all( + [ + client + .ReviewerCandidates({ owner, name, number: input.number, query }) + .pipe(graphql(target)), + client.ReviewerTeams({ owner, name, query }).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, input.query, teams)), + Effect.flatMap(found({ ...input, operation })), + ); }, ), diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index 685afdc355c08..54071ed7c4e7a 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -19,6 +19,7 @@ import { toPullRequestFilesViewedPage, toPullRequestRows, toReviewerCandidates, + toReviewerTeams, toReviewThreadCommentsPage, toCanWrite, toReviewThreadPage, @@ -31,6 +32,7 @@ import { PullRequestCore, PullRequestFilesViewed, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import { ReviewThreadComments, ReviewThreads } from "./pullRequestReviews.graphql.ts"; @@ -212,6 +214,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, @@ -1446,6 +1450,49 @@ describe("reviewer candidate decoding", () => { }, ]); }); + + it("offers the organization's teams that can reach this repository, each once", () => { + const team = (slug: string, repositories: ReadonlyArray) => ({ + 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), + undefined, + 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", () => { diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index b97454d1c74bb..9bc943f60ffe5 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -69,6 +69,7 @@ import type { RepositoryReferences, ReferenceCandidate, ReviewerCandidates, + ReviewerTeams, ViewerPermissions, } from "./pullRequestReads.graphql.ts"; import type { @@ -1542,10 +1543,41 @@ 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, query?: string, + teams: PullRequestReviewerCandidateList | null = null, ): PullRequestReviewerCandidateList | null { const repository = result.repository; if (repository == null) return null; @@ -1570,9 +1602,13 @@ export function toReviewerCandidates( }); } const hostMatches = new Set(); + for (const team of teams?.candidates ?? []) { + hostMatches.add(`team ${team.id}`); + 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) hostMatches.add(login); + if (login !== null) hostMatches.add(`user ${login}`); if (login === null || login === author || candidates.has(`user ${login}`)) continue; candidates.set(`user ${login}`, { id: login, @@ -1585,9 +1621,9 @@ export function toReviewerCandidates( } return { candidates: [...candidates.values()].filter( - (candidate) => !query?.trim() || (candidate.kind === "user" && hostMatches.has(candidate.id)), + (candidate) => !query?.trim() || hostMatches.has(`${candidate.kind} ${candidate.id}`), ), - truncated: repository.assignableUsers.pageInfo.hasNextPage, + truncated: repository.assignableUsers.pageInfo.hasNextPage || teams?.truncated === true, }; } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index 96588a2a5872f..3764a3fe6a6e5 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -408,10 +408,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!, $query: String) { repository(owner: $owner, name: $name) { assignableUsers(first: 100, query: $query) { @@ -454,6 +454,30 @@ query ReviewerCandidates($owner: String!, $name: String!, $number: Int!, $query: } } +# 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!, $query: String) { + organization(login: $owner) { + teams(first: 100, query: $query) { + 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 }) { diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index ebb43f6d7c756..e7cdd6c185d12 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -297,7 +297,7 @@ export const RequestedReviewerBot = Schema.Struct({ export interface RequestedReviewerBot extends Schema.Struct.Type {} /** A team of users in an organization. */ -export const Team = Schema.Struct({ +export const RequestedReviewerTeam = Schema.Struct({ __typename: Schema.Literal("Team"), /** The slug corresponding to the team. */ slug: Schema.String, @@ -308,7 +308,7 @@ export const Team = Schema.Struct({ /** A URL pointing to the team's avatar. */ teamAvatarUrl: Schema.NullOr(Shared.URI) }) -export interface Team extends Schema.Struct.Type {} +export interface RequestedReviewerTeam extends Schema.Struct.Type {} /** Types that can be requested reviewers. */ export const RequestedReviewerOther = Schema.Struct({ @@ -322,7 +322,7 @@ export const PullRequestCoreRepositoryPullRequestReviewRequestsNodesReviewReques requestedReviewer: Schema.NullOr(Schema.Union([ RequestedReviewerUser, RequestedReviewerBot, - Team, + RequestedReviewerTeam, RequestedReviewerOther ])) }) @@ -1167,7 +1167,7 @@ export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesReviewReq /** The reviewer that is requested. */ requestedReviewer: Schema.NullOr(Schema.Union([ PullRequestPreviewRepositoryPullRequestAuthorUser, - Team, + RequestedReviewerTeam, NodesBot, RequestedReviewerOther ])) @@ -1229,6 +1229,79 @@ export declare namespace ReviewerCandidates { export type Result = typeof ReviewerCandidates.result.Type } +/** A repository contains the content for a project. */ +export const NodesRepository = Schema.Struct({ + /** The repository's name with owner. */ + nameWithOwner: Schema.String +}) +export interface NodesRepository extends Schema.Struct.Type {} + +/** The connection type for Repository. */ +export const TeamRepositoryConnection = Schema.Struct({ + /** A list of nodes. */ + nodes: Schema.NullOr(Schema.Array(Schema.NullOr(NodesRepository))) +}) +export interface TeamRepositoryConnection extends Schema.Struct.Type {} + +/** A team of users in an organization. */ +export const NodesTeam = Schema.Struct({ + /** The slug corresponding to the team. */ + slug: Schema.String, + /** The slug corresponding to the organization and team. */ + combinedSlug: Schema.String, + /** The name of the team. */ + name: Schema.String, + /** A URL pointing to the team's avatar. */ + avatarUrl: Schema.NullOr(Shared.URI), + /** A list of repositories this team has access to. */ + repositories: TeamRepositoryConnection +}) +export interface NodesTeam extends Schema.Struct.Type {} + +/** The connection type for Team. */ +export const TeamConnection = Schema.Struct({ + /** Information to aid in pagination. */ + pageInfo: AssignableUsersPageInfoPageInfo, + /** A list of nodes. */ + nodes: Schema.NullOr(Schema.Array(Schema.NullOr(NodesTeam))) +}) +export interface TeamConnection extends Schema.Struct.Type {} + +/** An account on GitHub, with one or more owners, that has repositories, members and teams. */ +export const Organization = Schema.Struct({ + /** A list of teams in this organization. */ + teams: TeamConnection +}) +export interface Organization extends Schema.Struct.Type {} + +/** `query ReviewerTeams` */ +export const ReviewerTeams = GraphQL.query("ReviewerTeams", { + document: { + variables: [ + { name: "owner", body: "String!" }, + { name: "name", body: "String!" }, + { name: "query", body: "String" } + ], + selections: [ + { name: "organization", body: ["(login:$", "owner", "){teams(first:100 query:$", "query", "){pageInfo{hasNextPage}nodes{slug combinedSlug name avatarUrl repositories(query:$", "name", " first:100){nodes{nameWithOwner}}}}}"] } + ], + fragments: [] + }, + variables: { + owner: Schema.String, + name: Schema.String, + query: Schema.optional(Schema.NullOr(Schema.String)) + }, + result: Schema.Struct({ + /** Lookup a organization by login. */ + organization: Schema.NullOr(Organization) + }) +}) +export declare namespace ReviewerTeams { + export type Variables = typeof ReviewerTeams.variables.Type + export type Result = typeof ReviewerTeams.result.Type +} + /** The connection type for Label. */ export const LabelCandidatesRepositoryLabelsLabelConnection = Schema.Struct({ /** Information to aid in pagination. */ @@ -1802,4 +1875,4 @@ export declare namespace PullRequestSourceBranch { } /** Every operation in pullRequestReads.graphql. */ -export const PullRequestReadsGraphQLGroup = GraphQLGroup.make(RoutingIdentityViewer, ActorAvatars, PullRequestCore, PullRequestCheckContexts, PullRequestPreview, PullRequestActivity, PullRequestCommitStats, PullRequestHeads, PullRequestNodeId, PullRequestSubject, ViewerPermissions, ReviewerCandidates, LabelCandidates, RepositoryLabels, RepositoryReferences, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch) +export const PullRequestReadsGraphQLGroup = GraphQLGroup.make(RoutingIdentityViewer, ActorAvatars, PullRequestCore, PullRequestCheckContexts, PullRequestPreview, PullRequestActivity, PullRequestCommitStats, PullRequestHeads, PullRequestNodeId, PullRequestSubject, ViewerPermissions, ReviewerCandidates, ReviewerTeams, LabelCandidates, RepositoryLabels, RepositoryReferences, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch) From 53e7afcf7dbecf5fa6893f5c584014dfdb3fb19a Mon Sep 17 00:00:00 2001 From: Illia Panasenko Date: Thu, 12 Nov 2026 11:34:19 +0100 Subject: [PATCH 2/2] test(github): typed reviewer search fixture returns a team GitHub would match Co-Authored-By: Claude Opus 5.5 --- .../src/server/GitHubPullRequestApi.test.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index b27dc047122c5..f947282729d2d 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -4192,7 +4192,17 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { route( ["ReviewerCandidates", reviewerPeople(["octocat"], ["octocat", "unrelated"])], - ["ReviewerTeams", reviewerTeams([reviewerTeam])], + [ + "ReviewerTeams", + reviewerTeams([ + { + ...reviewerTeam, + slug: "octonauts", + combinedSlug: "acme/octonauts", + name: "Octonauts", + }, + ]), + ], ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -4212,7 +4222,7 @@ layer("GitHubPullRequestApi.layer", (it) => { ]); expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([ ["octocat", true], - ["acme/web", false], + ["acme/octonauts", false], ]); }), );