diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index f46c91fd69570..f20dc7c8107e7 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -3562,6 +3562,81 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("reads PR truth when the credential cannot select Team fields", () => + Effect.gen(function* () { + mockedExecute.mockImplementation((call) => { + if (call.kind !== "graphql") return Effect.die("Unexpected REST request"); + return Effect.succeed( + output( + encodeJson( + call.variables?.["teamDetails"] === false + ? coreResponse({ + reviewDecision: "CHANGES_REQUESTED", + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [checkRun({ name: "test", conclusion: "FAILURE" })], + pageInfo: lastPage, + }, + }, + }, + }, + ], + }, + reviewRequests: { + nodes: [ + { + requestedReviewer: { __typename: "User", login: "reviewer", name: null }, + }, + { requestedReviewer: { __typename: "Team" } }, + ], + }, + }) + : { + errors: [ + { + type: "INSUFFICIENT_SCOPES", + locations: [{ line: 1, column: 217 }], + message: + "Your token has not been granted the required scopes to execute this query. The 'slug' field requires one of the following scopes: ['read:org', 'read:discussion'], but your token has only been granted the: ['gist', 'repo', 'workflow'] scopes.", + }, + ], + }, + ), + ), + ); + }); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { ...pullRequestInput, host: "github-team-scope.test" }; + const detail = yield* cli.getPullRequestDetail(input); + expect(detail.reviewDecision).toBe("changes-requested"); + expect(detail.reviewRequestLogins).toEqual(["reviewer"]); + expect(detail.hasTeamReviewRequest).toBe(true); + expect(detail.checksState).toBe("failing"); + expect(mockedExecute).toHaveBeenCalledTimes(2); + yield* cli.getPullRequestDetail(input); + expect(mockedExecute).toHaveBeenCalledTimes(3); + mockedExecute.mockReturnValue(Effect.succeed(output(reviewThreadsPage([], null)))); + yield* cli.listReviewThreadComments(input); + expect(variablesOf("ReviewThreads").at(-1)).toMatchObject({ teamDetails: false }); + mockedExecute.mockReturnValue( + Effect.succeed(output(encodeJson({ errors: [refusedReviewer] }))), + ); + const callsBeforeRefusal = mockedExecute.mock.calls.length; + const failure = yield* Effect.flip(cli.getPullRequestDetail(input)); + expect(failure).toMatchObject({ + _tag: "GitHubApiResponseError", + status: 200, + githubErrors: [refusedReviewer.message], + githubErrorTypes: [refusedReviewer.type], + }); + expect(mockedExecute).toHaveBeenCalledTimes(callsBeforeRefusal + 2); + }), + ); + it.effect.each([false, true])( "continues after the core check page without reading it twice (reviewer refusal: %s)", (refused) => diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index ce38cc726a249..edc5f87963a17 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -320,6 +320,13 @@ const isGraphQlClientError = (error: unknown): error is GraphQLClientError.Graph const { isNotFoundOnly } = GitHubGraphQlClient; +const isInsufficientScopesOnly = (error: unknown): boolean => + isGraphQlClientError(error) && + error.reason._tag === "ResponseError" && + error.reason.status === 200 && + error.reason.errors.length > 0 && + error.reason.errors.every(({ type }) => type === "INSUFFICIENT_SCOPES"); + const isRefusedRequestedReviewersOnly = (error: unknown): boolean => isGraphQlClientError(error) && error.reason._tag === "ResponseError" && @@ -1433,19 +1440,38 @@ export const make = Effect.gen(function* () { timeToLive: "10 minutes", lookup: (_key: string) => Effect.void, }); + const refusedTeamDetails = yield* Cache.make({ + capacity: 512, + timeToLive: "10 minutes", + lookup: (_key: string) => Effect.void, + }); const readRequestedReviewers = Effect.fn("GitHubPullRequestApi.readRequestedReviewers")( function* ( input: { readonly host: string; readonly repository: string; readonly number: number }, wanted: number, - read: (requestedReviewers: number) => Effect.Effect, + read: (requestedReviewers: number, teamDetails: boolean) => Effect.Effect, ) { const host = normalizeHost(input.host); const { fingerprint } = yield* api.credential(host); const key = `${fingerprint}\0${host}\0${input.repository.toLowerCase()}\0${input.number}`; - if (yield* Cache.has(refusedReviewers, key)) return yield* read(0); - return yield* read(wanted).pipe( + const teamKey = `${fingerprint}\0${host}`; + const withTeamDetails = Effect.fn("GitHubPullRequestApi.readTeamDetails")(function* ( + requestedReviewers: number, + ) { + if (yield* Cache.has(refusedTeamDetails, teamKey)) + return yield* read(requestedReviewers, false); + return yield* read(requestedReviewers, true).pipe( + Effect.catchIf(isInsufficientScopesOnly, () => + read(requestedReviewers, false).pipe( + Effect.tap(() => Cache.set(refusedTeamDetails, teamKey, undefined)), + ), + ), + ); + }); + if (yield* Cache.has(refusedReviewers, key)) return yield* withTeamDetails(0); + return yield* withTeamDetails(wanted).pipe( Effect.catchIf(isRefusedRequestedReviewersOnly, () => - Cache.set(refusedReviewers, key, undefined).pipe(Effect.andThen(read(0))), + Cache.set(refusedReviewers, key, undefined).pipe(Effect.andThen(withTeamDetails(0))), ), ); }, @@ -1454,7 +1480,7 @@ export const make = Effect.gen(function* () { const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => { const { owner, name } = parseRepositorySelector(input.repository); const operation = "getPullRequestDetail"; - const read = (requestedReviewers: number) => + const read = (requestedReviewers: number, teamDetails: boolean) => client.PullRequestCore({ owner, name, @@ -1462,6 +1488,7 @@ export const make = Effect.gen(function* () { headRef: `refs/pull/${input.number}/head`, required: asksRequired(input.host), requestedReviewers, + teamDetails, }); return SourceControlRateLimit.Interactive.pipe( Effect.flatMap((allowReserve) => @@ -2219,8 +2246,14 @@ export const make = Effect.gen(function* () { const first: GitHubReviewThreadPage = yield* readRequestedReviewers( input, 50, - (requestedReviewers) => - client.ReviewThreads({ owner, name, number: input.number, requestedReviewers }), + (requestedReviewers, teamDetails) => + client.ReviewThreads({ + owner, + name, + number: input.number, + requestedReviewers, + teamDetails, + }), ).pipe(graphql(target), Effect.map(toReviewThreadPage), Effect.flatMap(found(target))); const entries: GitHubReviewThreadEntry[] = [...first.threads]; const avatarsByLogin = new Map(first.avatarsByLogin); diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index a4c509e944488..fdd6ca818588d 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -116,6 +116,7 @@ interface LabelFields { } interface ReviewRequestFields { + readonly __typename?: string | undefined; readonly login?: string | null | undefined; readonly slug?: string | null | undefined; readonly name?: string | null | undefined; @@ -554,8 +555,9 @@ function toReviewRequestLogins( function hasTeamReviewRequest(raw: ReadonlyArray | undefined): boolean { return (raw ?? []).some( (request) => - trimmed(request.login) === null && - (trimmed(request.slug) !== null || trimmed(request.name) !== null), + request.__typename === "Team" || + (trimmed(request.login) === null && + (trimmed(request.slug) !== null || trimmed(request.name) !== null)), ); } @@ -1041,6 +1043,7 @@ export function toPullRequestCore(result: PullRequestCore.Result): GitHubPullReq : [ { login: "login" in requestedReviewer ? requestedReviewer.login : null, + __typename: requestedReviewer.__typename, slug: "slug" in requestedReviewer ? requestedReviewer.slug : null, name: "teamName" in requestedReviewer diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql b/packages/source-control-github/src/server/pullRequestReads.graphql index eb728b1ee062c..39a2459663e38 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql +++ b/packages/source-control-github/src/server/pullRequestReads.graphql @@ -59,6 +59,7 @@ query PullRequestCore( $headRef: String! $required: Boolean! $requestedReviewers: Int! + $teamDetails: Boolean! ) { repository(owner: $owner, name: $name) { mergeCommitAllowed @@ -131,7 +132,7 @@ query PullRequestCore( } # A team's name and avatar are nullable differently from a person's, so they travel # under their own aliases. `combinedSlug` is the `org/slug` a reviewer row shows. - ... on Team { + ... on Team @include(if: $teamDetails) { slug combinedSlug teamName: name diff --git a/packages/source-control-github/src/server/pullRequestReads.graphql.ts b/packages/source-control-github/src/server/pullRequestReads.graphql.ts index 90ee834c3181e..6c7e7b1a5486e 100644 --- a/packages/source-control-github/src/server/pullRequestReads.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReads.graphql.ts @@ -297,18 +297,18 @@ 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 PullRequestCoreRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam = Schema.Struct({ __typename: Schema.Literal("Team"), /** The slug corresponding to the team. */ - slug: Schema.String, + slug: Schema.optionalKey(Schema.String), /** The slug corresponding to the organization and team. */ - combinedSlug: Schema.String, + combinedSlug: Schema.optionalKey(Schema.String), /** The name of the team. */ - teamName: Schema.String, + teamName: Schema.optionalKey(Schema.String), /** A URL pointing to the team's avatar. */ - teamAvatarUrl: Schema.NullOr(Shared.URI) + teamAvatarUrl: Schema.optionalKey(Schema.NullOr(Shared.URI)) }) -export interface Team extends Schema.Struct.Type {} +export interface PullRequestCoreRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam 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, + PullRequestCoreRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam, RequestedReviewerOther ])) }) @@ -500,10 +500,11 @@ export const PullRequestCore = GraphQL.query("PullRequestCore", { { name: "number", body: "Int!" }, { name: "headRef", body: "String!" }, { name: "required", body: "Boolean!" }, - { name: "requestedReviewers", body: "Int!" } + { name: "requestedReviewers", body: "Int!" }, + { name: "teamDetails", body: "Boolean!" } ], selections: [ - { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed autoMergeAllowed viewerPermission pullRequest(number:$", "number", "){number title url body state isDraft mergeable mergeStateStatus reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository maintainerCanModify headRepositoryOwner{__typename login}headRepository{nameWithOwner viewerPermission}headRef{id}author{__typename login avatarUrl...on User{id name}}autoMergeRequest{mergeMethod}viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch baseRef{compare(headRef:$", "headRef", "){behindBy}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name}...on Bot{login}...on Team{slug combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}labels(first:100){nodes{name color}}commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage endCursor}}}}}}}}"] } + { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed autoMergeAllowed viewerPermission pullRequest(number:$", "number", "){number title url body state isDraft mergeable mergeStateStatus reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository maintainerCanModify headRepositoryOwner{__typename login}headRepository{nameWithOwner viewerPermission}headRef{id}author{__typename login avatarUrl...on User{id name}}autoMergeRequest{mergeMethod}viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch baseRef{compare(headRef:$", "headRef", "){behindBy}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name}...on Bot{login}...on Team@include(if:$", "teamDetails", "){slug combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}labels(first:100){nodes{name color}}commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{__typename...", "CheckContext", "}pageInfo{hasNextPage endCursor}}}}}}}}"] } ], fragments: [ { name: "CheckContext", body: ["on StatusCheckRollupContext{__typename...on StatusContext{context state targetUrl createdAt description isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")}...on CheckRun{name status conclusion startedAt completedAt detailsUrl isRequired(pullRequestNumber:$", "number", ")@include(if:$", "required", ")checkSuite{workflowRun{workflow{name}}}}}"] } @@ -515,7 +516,8 @@ export const PullRequestCore = GraphQL.query("PullRequestCore", { number: Shared.Int, headRef: Schema.String, required: Schema.Boolean, - requestedReviewers: Shared.Int + requestedReviewers: Shared.Int, + teamDetails: Schema.Boolean }, result: Schema.Struct({ /** Lookup a given repository by the owner and repository name. */ @@ -1156,12 +1158,26 @@ export const Actor = Schema.Struct({ }) export interface Actor extends Schema.Struct.Type {} +/** A team of users in an organization. */ +export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam = Schema.Struct({ + __typename: Schema.Literal("Team"), + /** 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. */ + teamName: Schema.String, + /** A URL pointing to the team's avatar. */ + teamAvatarUrl: Schema.NullOr(Shared.URI) +}) +export interface ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam extends Schema.Struct.Type {} + /** A request for a user to review a pull request. */ export const ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesReviewRequest = Schema.Struct({ /** The reviewer that is requested. */ requestedReviewer: Schema.NullOr(Schema.Union([ PullRequestPreviewRepositoryPullRequestAuthorUser, - Team, + ReviewerCandidatesRepositoryPullRequestReviewRequestsNodesRequestedReviewerTeam, NodesBot, RequestedReviewerOther ])) diff --git a/packages/source-control-github/src/server/pullRequestReviews.graphql b/packages/source-control-github/src/server/pullRequestReviews.graphql index 94646874d803d..346fbe91561a2 100644 --- a/packages/source-control-github/src/server/pullRequestReviews.graphql +++ b/packages/source-control-github/src/server/pullRequestReviews.graphql @@ -83,7 +83,13 @@ fragment ReviewThreadPage on PullRequestReviewThreadConnection { # # `viewerCanUpdate` and `viewerDidAuthor` belong to the pull request this query is already # standing on, so what the reader may do with it arrives with the conversation. -query ReviewThreads($owner: String!, $name: String!, $number: Int!, $requestedReviewers: Int!) { +query ReviewThreads( + $owner: String! + $name: String! + $number: Int! + $requestedReviewers: Int! + $teamDetails: Boolean! +) { viewer { login } @@ -140,7 +146,7 @@ query ReviewThreads($owner: String!, $name: String!, $number: Int!, $requestedRe login avatarUrl } - ... on Team { + ... on Team @include(if: $teamDetails) { combinedSlug teamName: name teamAvatarUrl: avatarUrl diff --git a/packages/source-control-github/src/server/pullRequestReviews.graphql.ts b/packages/source-control-github/src/server/pullRequestReviews.graphql.ts index 9a1351f8cd138..55bab8a3e2432 100644 --- a/packages/source-control-github/src/server/pullRequestReviews.graphql.ts +++ b/packages/source-control-github/src/server/pullRequestReviews.graphql.ts @@ -239,11 +239,11 @@ export interface RequestedReviewerBot extends Schema.Struct.Type {} @@ -412,11 +412,12 @@ export const ReviewThreads = GraphQL.query("ReviewThreads", { { name: "owner", body: "String!" }, { name: "name", body: "String!" }, { name: "number", body: "Int!" }, - { name: "requestedReviewers", body: "Int!" } + { name: "requestedReviewers", body: "Int!" }, + { name: "teamDetails", body: "Boolean!" } ], selections: [ { name: "viewer", body: ["{login}"] }, - { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){pullRequest(number:$", "number", "){reviewThreads(first:100){...ReviewThreadPage}viewerCanUpdate viewerDidAuthor author{__typename login avatarUrl}...Reactions comments(first:100){nodes{id lastEditedAt author{__typename login avatarUrl}...Reactions}}reviews(first:100){nodes{id lastEditedAt author{__typename login avatarUrl}...Reactions}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name avatarUrl}...on Bot{login avatarUrl}...on Team{combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}latestReviews(first:50){nodes{state author{__typename login avatarUrl}}}reviewDismissals:timelineItems(itemTypes:[REVIEW_DISMISSED_EVENT]first:100){pageInfo{hasNextPage endCursor}nodes{__typename...on ReviewDismissedEvent{dismissalMessage review{id}}}}commits(last:100){nodes{commit{oid messageHeadline committedDate authors(first:3){nodes{name avatarUrl user{login}}}}}}}}"] } + { name: "repository", body: ["(owner:$", "owner", " name:$", "name", "){pullRequest(number:$", "number", "){reviewThreads(first:100){...ReviewThreadPage}viewerCanUpdate viewerDidAuthor author{__typename login avatarUrl}...Reactions comments(first:100){nodes{id lastEditedAt author{__typename login avatarUrl}...Reactions}}reviews(first:100){nodes{id lastEditedAt author{__typename login avatarUrl}...Reactions}}reviewRequests(first:$", "requestedReviewers", "){nodes{requestedReviewer{__typename...on User{login name avatarUrl}...on Bot{login avatarUrl}...on Team@include(if:$", "teamDetails", "){combinedSlug teamName:name teamAvatarUrl:avatarUrl}}}}latestReviews(first:50){nodes{state author{__typename login avatarUrl}}}reviewDismissals:timelineItems(itemTypes:[REVIEW_DISMISSED_EVENT]first:100){pageInfo{hasNextPage endCursor}nodes{__typename...on ReviewDismissedEvent{dismissalMessage review{id}}}}commits(last:100){nodes{commit{oid messageHeadline committedDate authors(first:3){nodes{name avatarUrl user{login}}}}}}}}"] } ], fragments: [ { name: "ReviewThreadPage", body: ["on PullRequestReviewThreadConnection{totalCount pageInfo{hasNextPage endCursor}nodes{id isResolved isOutdated path line diffSide comments(first:10){totalCount pageInfo{hasNextPage endCursor}nodes{...ThreadComment}}}}"] }, @@ -428,7 +429,8 @@ export const ReviewThreads = GraphQL.query("ReviewThreads", { owner: Schema.String, name: Schema.String, number: Shared.Int, - requestedReviewers: Shared.Int + requestedReviewers: Shared.Int, + teamDetails: Schema.Boolean }, result: Schema.Struct({ /** The currently authenticated user. */