Skip to content

Commit e79ae42

Browse files
committed
fix(github): read watched PRs without organization scopes
1 parent d4247a5 commit e79ae42

7 files changed

Lines changed: 157 additions & 29 deletions

File tree

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3562,6 +3562,73 @@ layer("GitHubPullRequestApi.layer", (it) => {
35623562
}),
35633563
);
35643564

3565+
it.effect("reads PR truth when the credential cannot select Team fields", () =>
3566+
Effect.gen(function* () {
3567+
mockedExecute.mockImplementation((call) => {
3568+
if (call.kind !== "graphql") return Effect.die("Unexpected REST request");
3569+
return Effect.succeed(
3570+
output(
3571+
encodeJson(
3572+
call.variables?.["teamDetails"] === false
3573+
? coreResponse({
3574+
reviewDecision: "CHANGES_REQUESTED",
3575+
commits: {
3576+
nodes: [
3577+
{
3578+
commit: {
3579+
statusCheckRollup: {
3580+
contexts: {
3581+
nodes: [checkRun({ name: "test", conclusion: "FAILURE" })],
3582+
pageInfo: lastPage,
3583+
},
3584+
},
3585+
},
3586+
},
3587+
],
3588+
},
3589+
reviewRequests: {
3590+
nodes: [
3591+
{
3592+
requestedReviewer: { __typename: "User", login: "reviewer", name: null },
3593+
},
3594+
{ requestedReviewer: { __typename: "Team" } },
3595+
],
3596+
},
3597+
})
3598+
: {
3599+
errors: [
3600+
{
3601+
type: "INSUFFICIENT_SCOPES",
3602+
locations: [{ line: 1, column: 217 }],
3603+
message:
3604+
"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.",
3605+
},
3606+
],
3607+
},
3608+
),
3609+
),
3610+
);
3611+
});
3612+
const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
3613+
const input = { ...pullRequestInput, host: "github-team-scope.test" };
3614+
const detail = yield* cli.getPullRequestDetail(input);
3615+
expect(detail.reviewDecision).toBe("changes-requested");
3616+
expect(detail.reviewRequestLogins).toEqual(["reviewer"]);
3617+
expect(detail.hasTeamReviewRequest).toBe(true);
3618+
expect(detail.checksState).toBe("failing");
3619+
expect(mockedExecute).toHaveBeenCalledTimes(2);
3620+
yield* cli.getPullRequestDetail(input);
3621+
expect(mockedExecute).toHaveBeenCalledTimes(3);
3622+
mockedExecute.mockReturnValue(Effect.succeed(output(reviewThreadsPage([], null))));
3623+
yield* cli.listReviewThreadComments(input);
3624+
expect(variablesOf("ReviewThreads").at(-1)).toMatchObject({ teamDetails: false });
3625+
mockedExecute.mockReturnValue(
3626+
Effect.succeed(output(encodeJson({ errors: [refusedReviewer] }))),
3627+
);
3628+
yield* Effect.flip(cli.getPullRequestDetail(input));
3629+
}),
3630+
);
3631+
35653632
it.effect.each([false, true])(
35663633
"continues after the core check page without reading it twice (reviewer refusal: %s)",
35673634
(refused) =>

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

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,13 @@ const isGraphQlClientError = (error: unknown): error is GraphQLClientError.Graph
320320

321321
const { isNotFoundOnly } = GitHubGraphQlClient;
322322

323+
const isInsufficientScopesOnly = (error: unknown): boolean =>
324+
isGraphQlClientError(error) &&
325+
error.reason._tag === "ResponseError" &&
326+
error.reason.status === 200 &&
327+
error.reason.errors.length > 0 &&
328+
error.reason.errors.every(({ type }) => type === "INSUFFICIENT_SCOPES");
329+
323330
const isRefusedRequestedReviewersOnly = (error: unknown): boolean =>
324331
isGraphQlClientError(error) &&
325332
error.reason._tag === "ResponseError" &&
@@ -1433,19 +1440,38 @@ export const make = Effect.gen(function* () {
14331440
timeToLive: "10 minutes",
14341441
lookup: (_key: string) => Effect.void,
14351442
});
1443+
const refusedTeamDetails = yield* Cache.make({
1444+
capacity: 512,
1445+
timeToLive: "10 minutes",
1446+
lookup: (_key: string) => Effect.void,
1447+
});
14361448
const readRequestedReviewers = Effect.fn("GitHubPullRequestApi.readRequestedReviewers")(
14371449
function* <A, E, R>(
14381450
input: { readonly host: string; readonly repository: string; readonly number: number },
14391451
wanted: number,
1440-
read: (requestedReviewers: number) => Effect.Effect<A, E, R>,
1452+
read: (requestedReviewers: number, teamDetails: boolean) => Effect.Effect<A, E, R>,
14411453
) {
14421454
const host = normalizeHost(input.host);
14431455
const { fingerprint } = yield* api.credential(host);
14441456
const key = `${fingerprint}\0${host}\0${input.repository.toLowerCase()}\0${input.number}`;
1445-
if (yield* Cache.has(refusedReviewers, key)) return yield* read(0);
1446-
return yield* read(wanted).pipe(
1457+
const teamKey = `${fingerprint}\0${host}`;
1458+
const withTeamDetails = Effect.fn("GitHubPullRequestApi.readTeamDetails")(function* (
1459+
requestedReviewers: number,
1460+
) {
1461+
if (yield* Cache.has(refusedTeamDetails, teamKey))
1462+
return yield* read(requestedReviewers, false);
1463+
return yield* read(requestedReviewers, true).pipe(
1464+
Effect.catchIf(isInsufficientScopesOnly, () =>
1465+
read(requestedReviewers, false).pipe(
1466+
Effect.tap(() => Cache.set(refusedTeamDetails, teamKey, undefined)),
1467+
),
1468+
),
1469+
);
1470+
});
1471+
if (yield* Cache.has(refusedReviewers, key)) return yield* withTeamDetails(0);
1472+
return yield* withTeamDetails(wanted).pipe(
14471473
Effect.catchIf(isRefusedRequestedReviewersOnly, () =>
1448-
Cache.set(refusedReviewers, key, undefined).pipe(Effect.andThen(read(0))),
1474+
Cache.set(refusedReviewers, key, undefined).pipe(Effect.andThen(withTeamDetails(0))),
14491475
),
14501476
);
14511477
},
@@ -1454,14 +1480,15 @@ export const make = Effect.gen(function* () {
14541480
const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => {
14551481
const { owner, name } = parseRepositorySelector(input.repository);
14561482
const operation = "getPullRequestDetail";
1457-
const read = (requestedReviewers: number) =>
1483+
const read = (requestedReviewers: number, teamDetails: boolean) =>
14581484
client.PullRequestCore({
14591485
owner,
14601486
name,
14611487
number: input.number,
14621488
headRef: `refs/pull/${input.number}/head`,
14631489
required: asksRequired(input.host),
14641490
requestedReviewers,
1491+
teamDetails,
14651492
});
14661493
return SourceControlRateLimit.Interactive.pipe(
14671494
Effect.flatMap((allowReserve) =>
@@ -2219,8 +2246,14 @@ export const make = Effect.gen(function* () {
22192246
const first: GitHubReviewThreadPage = yield* readRequestedReviewers(
22202247
input,
22212248
50,
2222-
(requestedReviewers) =>
2223-
client.ReviewThreads({ owner, name, number: input.number, requestedReviewers }),
2249+
(requestedReviewers, teamDetails) =>
2250+
client.ReviewThreads({
2251+
owner,
2252+
name,
2253+
number: input.number,
2254+
requestedReviewers,
2255+
teamDetails,
2256+
}),
22242257
).pipe(graphql(target), Effect.map(toReviewThreadPage), Effect.flatMap(found(target)));
22252258
const entries: GitHubReviewThreadEntry[] = [...first.threads];
22262259
const avatarsByLogin = new Map(first.avatarsByLogin);

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ interface LabelFields {
116116
}
117117

118118
interface ReviewRequestFields {
119+
readonly __typename?: string | undefined;
119120
readonly login?: string | null | undefined;
120121
readonly slug?: string | null | undefined;
121122
readonly name?: string | null | undefined;
@@ -554,8 +555,9 @@ function toReviewRequestLogins(
554555
function hasTeamReviewRequest(raw: ReadonlyArray<ReviewRequestFields> | undefined): boolean {
555556
return (raw ?? []).some(
556557
(request) =>
557-
trimmed(request.login) === null &&
558-
(trimmed(request.slug) !== null || trimmed(request.name) !== null),
558+
request.__typename === "Team" ||
559+
(trimmed(request.login) === null &&
560+
(trimmed(request.slug) !== null || trimmed(request.name) !== null)),
559561
);
560562
}
561563

@@ -1041,6 +1043,7 @@ export function toPullRequestCore(result: PullRequestCore.Result): GitHubPullReq
10411043
: [
10421044
{
10431045
login: "login" in requestedReviewer ? requestedReviewer.login : null,
1046+
__typename: requestedReviewer.__typename,
10441047
slug: "slug" in requestedReviewer ? requestedReviewer.slug : null,
10451048
name:
10461049
"teamName" in requestedReviewer

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ query PullRequestCore(
5959
$headRef: String!
6060
$required: Boolean!
6161
$requestedReviewers: Int!
62+
$teamDetails: Boolean!
6263
) {
6364
repository(owner: $owner, name: $name) {
6465
mergeCommitAllowed
@@ -131,7 +132,7 @@ query PullRequestCore(
131132
}
132133
# A team's name and avatar are nullable differently from a person's, so they travel
133134
# under their own aliases. `combinedSlug` is the `org/slug` a reviewer row shows.
134-
... on Team {
135+
... on Team @include(if: $teamDetails) {
135136
slug
136137
combinedSlug
137138
teamName: name

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

Lines changed: 27 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/source-control-github/src/server/pullRequestReviews.graphql‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,13 @@ fragment ReviewThreadPage on PullRequestReviewThreadConnection {
8383
#
8484
# `viewerCanUpdate` and `viewerDidAuthor` belong to the pull request this query is already
8585
# standing on, so what the reader may do with it arrives with the conversation.
86-
query ReviewThreads($owner: String!, $name: String!, $number: Int!, $requestedReviewers: Int!) {
86+
query ReviewThreads(
87+
$owner: String!
88+
$name: String!
89+
$number: Int!
90+
$requestedReviewers: Int!
91+
$teamDetails: Boolean!
92+
) {
8793
viewer {
8894
login
8995
}
@@ -140,7 +146,7 @@ query ReviewThreads($owner: String!, $name: String!, $number: Int!, $requestedRe
140146
login
141147
avatarUrl
142148
}
143-
... on Team {
149+
... on Team @include(if: $teamDetails) {
144150
combinedSlug
145151
teamName: name
146152
teamAvatarUrl: avatarUrl

‎packages/source-control-github/src/server/pullRequestReviews.graphql.ts‎

Lines changed: 8 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)