From 8e78668fee302b854cec5edbc7529575fbe05d49 Mon Sep 17 00:00:00 2001 From: Illia Panasenko Date: Wed, 4 Nov 2026 22:22:11 +0100 Subject: [PATCH] fix(github): teams with access show in the reviewer picker The reviewer picker listed only people, plus teams already requested, so a team could not be asked for a review from T3 Code. A separate ReviewerTeams query reads the organization's teams and keeps those with access to the repository. It needs read:org and an organization owner, so it runs beside the people query and is dropped on failure instead of failing the picker. Done with Claude Opus 5.5 in Claude Code, run through T3 Code. --- .../src/server/GitHubPullRequestApi.test.ts | 105 ++++++++++++------ .../src/server/GitHubPullRequestApi.ts | 24 ++-- .../src/server/gitHubPullRequestJson.test.ts | 46 ++++++++ .../src/server/gitHubPullRequestJson.ts | 37 +++++- .../src/server/pullRequestReads.graphql | 32 +++++- .../src/server/pullRequestReads.graphql.ts | 81 +++++++++++++- 6 files changed, 272 insertions(+), 53 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 5a5c46f28d99a..1b1753aa821eb 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/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, + ); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 2defbb6684919..8fc3b23904f17 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/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 })), + ); }, ), diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index c816f76a7801d..d63bdaf1ee577 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/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) => ({ + 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", () => { diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index 7b8851f7bed2d..4fa0f8b49c4f6 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/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, }; } diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index 78e5efe0e4558..bc6e977044442 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/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 }) { diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index 5bc09fa4e2ce4..701d5ce01753e 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -255,7 +255,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, @@ -266,7 +266,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({ @@ -280,7 +280,7 @@ export const PullRequestCoreRepositoryPullRequestReviewRequestsNodesReviewReques requestedReviewer: Schema.NullOr(Schema.Union([ RequestedReviewerUser, RequestedReviewerBot, - Team, + RequestedReviewerTeam, RequestedReviewerOther ])) }) @@ -1156,7 +1156,7 @@ export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesReviewReq /** The reviewer that is requested. */ requestedReviewer: Schema.NullOr(Schema.Union([ PullRequestPreviewRepositoryPullRequestAuthorUser, - Team, + RequestedReviewerTeam, NodesBot, RequestedReviewerOther ])) @@ -1216,6 +1216,77 @@ 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: PullRequestCoreRepositoryPullRequestCommitsNodesCommitStatusCheckRollupContextsPageInfoPageInfo, + /** 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!" } + ], + selections: [ + { name: "organization", body: ["(login:$", "owner", "){teams(first:100){pageInfo{hasNextPage}nodes{slug combinedSlug name avatarUrl repositories(query:$", "name", " first:100){nodes{nameWithOwner}}}}}"] } + ], + fragments: [] + }, + variables: { + owner: Schema.String, + name: 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 +} + /** A label for categorizing Issues, Pull Requests, Milestones, or Discussions with a given Repository. */ export const RepositoryLabelsNodesLabel = Schema.Struct({ /** Identifies the label name. */ @@ -1724,4 +1795,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, 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, RepositoryAuthors, PullRequestFilesViewed, PullRequestFileStats, PullRequestMergeMessage, PullRequestActionState, PullRequestBranchToDelete, PullRequestSourceBranch)