Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
47 changes: 40 additions & 7 deletions packages/source-control-github/src/server/GitHubPullRequestApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" &&
Expand Down Expand Up @@ -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* <A, E, R>(
input: { readonly host: string; readonly repository: string; readonly number: number },
wanted: number,
read: (requestedReviewers: number) => Effect.Effect<A, E, R>,
read: (requestedReviewers: number, teamDetails: boolean) => Effect.Effect<A, E, R>,
) {
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))),
),
);
},
Expand All @@ -1454,14 +1480,15 @@ 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,
number: input.number,
headRef: `refs/pull/${input.number}/head`,
required: asksRequired(input.host),
requestedReviewers,
teamDetails,
});
return SourceControlRateLimit.Interactive.pipe(
Effect.flatMap((allowReserve) =>
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -554,8 +555,9 @@ function toReviewRequestLogins(
function hasTeamReviewRequest(raw: ReadonlyArray<ReviewRequestFields> | 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)),
);
}

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ query PullRequestCore(
$headRef: String!
$required: Boolean!
$requestedReviewers: Int!
$teamDetails: Boolean!
) {
repository(owner: $owner, name: $name) {
mergeCommitAllowed
Expand Down Expand Up @@ -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
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading