Skip to content

Commit e867091

Browse files
ScottN-PVclaude
andcommitted
fix(github): read pull requests when GitHub refuses a requested reviewer
A GitHub App that may read a pull request but not one of its requested reviewers gets a FORBIDDEN error at reviewRequests.nodes.<n>.requestedReviewer, and that one error failed the whole detail and review-thread reads, so watching stopped after eight failures. Both reads now ask again for no reviewer requests when that is the only refusal; any other error still fails them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent bf8ffe8 commit e867091

6 files changed

Lines changed: 506 additions & 16 deletions

File tree

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,43 @@ describe("GitHubApi", () => {
233233
}).pipe(Effect.provide(layer));
234234
});
235235

236+
it.effect("names each refused field of a partial GraphQL answer", () => {
237+
const reviewerPath = [
238+
"repository",
239+
"pullRequest",
240+
"reviewRequests",
241+
"nodes",
242+
0,
243+
"requestedReviewer",
244+
];
245+
const { layer } = harness(() =>
246+
json({
247+
data: { repository: { pullRequest: { number: 7, reviewRequests: { nodes: [null] } } } },
248+
errors: [
249+
{
250+
type: "FORBIDDEN",
251+
path: reviewerPath,
252+
extensions: { saml_failure: false, ip_allow_list_failure: false },
253+
message: "Resource not accessible by integration",
254+
},
255+
{ message: "Something went wrong", path: "repository" },
256+
],
257+
}),
258+
);
259+
return Effect.gen(function* () {
260+
const api = yield* GitHubApi.GitHubApi;
261+
const error = yield* Effect.flip(
262+
api.graphql({ host: "github.com", operation: "detail", query: "query { viewer { id } }" }),
263+
);
264+
expect(error._tag).toBe("GitHubApiResponseError");
265+
// An error with no type or an unreadable path is kept, so a caller cannot miss it.
266+
expect(error).toHaveProperty("graphqlErrors", [
267+
{ type: "FORBIDDEN", path: reviewerPath },
268+
{ type: undefined, path: undefined },
269+
]);
270+
}).pipe(Effect.provide(layer));
271+
});
272+
236273
it.effect(
237274
"reads a NOT_FOUND answer with another error as a failure, not a missing resource",
238275
() => {

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

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,20 @@ export class GitHubApiNotFoundError extends Schema.TaggedError<GitHubApiNotFound
7575
}
7676
}
7777

78+
/** The field a GraphQL error refers to, as keys and list indexes from the top of the answer. */
79+
const GraphQlErrorPath = Schema.Array(Schema.Union([Schema.String, Schema.Int]));
80+
const isGraphQlErrorPath = Schema.is(GraphQlErrorPath);
81+
82+
const GraphQlError = Schema.Struct({
83+
type: Schema.optional(Schema.String),
84+
path: Schema.optional(GraphQlErrorPath),
85+
});
86+
7887
/**
7988
* GitHub answered with a failure that is none of the above. `githubErrors` carries GitHub's own
8089
* error messages, from a GraphQL `errors` list or a REST `message`/`errors` body: they name the
8190
* field and the reason ("A pull request already exists for acme:feature"), never a token.
91+
* `graphqlErrors` has one entry per GraphQL error, so a caller can tell which fields were refused.
8292
*/
8393
export class GitHubApiResponseError extends Schema.TaggedError<GitHubApiResponseError>()(
8494
"GitHubApiResponseError",
@@ -87,6 +97,7 @@ export class GitHubApiResponseError extends Schema.TaggedError<GitHubApiResponse
8797
operation: Schema.String,
8898
status: Schema.Int,
8999
githubErrors: Schema.optionalKey(Schema.Array(Schema.String)),
100+
graphqlErrors: Schema.optionalKey(Schema.Array(GraphQlError)),
90101
},
91102
) {
92103
override get message(): string {
@@ -229,6 +240,8 @@ const decodeGraphQlErrors = Schema.decodeUnknownOption(
229240
Schema.Struct({
230241
type: Schema.optional(Schema.String),
231242
message: Schema.optional(Schema.String),
243+
// Read loosely: an unexpected path must not stop the error itself from being seen.
244+
path: Schema.optional(Schema.Unknown),
232245
}),
233246
),
234247
}),
@@ -281,7 +294,10 @@ type Answer = Data.TaggedEnum<{
281294
RateLimited: {};
282295
Unauthorized: {};
283296
NotFound: {};
284-
Failed: { readonly messages: ReadonlyArray<string> | undefined };
297+
Failed: {
298+
readonly messages: ReadonlyArray<string> | undefined;
299+
readonly graphqlErrors?: ReadonlyArray<typeof GraphQlError.Type>;
300+
};
285301
}>;
286302
const Answer = Data.taggedEnum<Answer>();
287303

@@ -326,7 +342,15 @@ function classify(input: {
326342
) {
327343
return Answer.NotFound();
328344
}
329-
if (errors !== undefined) return Answer.Failed({ messages });
345+
if (errors !== undefined) {
346+
return Answer.Failed({
347+
messages,
348+
graphqlErrors: errors.map(({ type, path }) => ({
349+
type,
350+
path: isGraphQlErrorPath(path) ? path : undefined,
351+
})),
352+
});
353+
}
330354
if ((status >= 200 && status < 300) || (status === 304 && input.acceptNotModified)) {
331355
return Answer.Ok();
332356
}
@@ -486,12 +510,13 @@ export const make = Effect.gen(function* () {
486510
.invalidate(host)
487511
.pipe(Effect.andThen(Effect.fail(new GitHubApiAuthenticationError(context)))),
488512
NotFound: () => Effect.fail(new GitHubApiNotFoundError(context)),
489-
Failed: ({ messages }) =>
513+
Failed: ({ messages, graphqlErrors }) =>
490514
Effect.fail(
491515
new GitHubApiResponseError({
492516
...context,
493517
status,
494518
...(messages === undefined ? {} : { githubErrors: messages }),
519+
...(graphqlErrors === undefined ? {} : { graphqlErrors }),
495520
}),
496521
),
497522
},

0 commit comments

Comments
 (0)