Skip to content

fix(server): support GitHub App tokens in pull request viewer #11273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LouisDeconinck wants to merge 2 commits into from
Closed

fix(server): support GitHub App tokens in pull request viewer #11273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a3e9dd
fix(server): support GitHub App tokens in pull request viewer
LouisDeconinck Sep 11, 2026
c27c6a0
fix(server): route viewer identity lookup through GraphQL budget
LouisDeconinck Sep 17, 2026
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
83 changes: 75 additions & 8 deletions apps/server/src/pullRequest/GitHubPullRequestCli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ it.effect(
Effect.sync(() => {
commands.push(input);
if (input.args[0] === "auth") return output(activeToken);
if (input.args[0] === "api") return output('{"id":123,"login":"same-account"}');
if (input.args[0] === "api")
return output('{"data":{"viewer":{"id":123,"login":"same-account"}}}');
return output("");
}),
}),
Expand Down Expand Up @@ -258,7 +259,9 @@ layer("GitHubPullRequestCli.layer", (it) => {
mockedExecute.mockImplementation((input) =>
input.args[0] === "auth"
? Effect.succeed(output("shared-credential"))
: Effect.yieldNow.pipe(Effect.as(output('{"id":123,"login":"viewer"}'))),
: Effect.yieldNow.pipe(
Effect.as(output('{"data":{"viewer":{"id":123,"login":"viewer"}}}')),
),
);
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
const results = yield* Effect.all(
Expand Down Expand Up @@ -292,7 +295,7 @@ layer("GitHubPullRequestCli.layer", (it) => {
yield* Deferred.succeed(firstStarted, undefined);
return yield* Effect.never;
}
return output('{"id":123,"login":"viewer"}');
return output('{"data":{"viewer":{"id":123,"login":"viewer"}}}');
}),
);
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
Expand Down Expand Up @@ -2567,9 +2570,37 @@ layer("GitHubPullRequestCli.layer", (it) => {
}),
);

it.effect("reads the viewer login through the GraphQL viewer", () =>
Effect.gen(function* () {
// REST GET /user refuses GitHub App installation tokens; the GraphQL viewer answers them.
mockedExecute
.mockReturnValueOnce(Effect.succeed(output("app-installation-credential")))
.mockReturnValueOnce(
Effect.succeed(output('{"data":{"viewer":{"id":789,"login":"acme-app[bot]"}}}')),
);
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

const login = yield* cli.getViewerLogin({ cwd: "/w", host: "github.app-viewer.test" });

assert.strictEqual(login, "acme-app[bot]");
expect(callAt(0).args).toEqual(["auth", "token", "--hostname", "github.app-viewer.test"]);
expect(callAt(1).args.slice(0, 5)).toEqual([
"api",
"graphql",
"--hostname",
"github.app-viewer.test",
"-f",
]);
expect(callAt(1).args.at(-1)).toContain("query={viewer{id:databaseId,login}");
expect(callAt(1).args.at(-1)).toContain("rateLimit { cost limit remaining resetAt }");
}),
);

it.effect("fails when the authenticated account has no login", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValueOnce(Effect.succeed(output(" ")));
mockedExecute
.mockReturnValueOnce(Effect.succeed(output("no-login-credential")))
.mockReturnValueOnce(Effect.succeed(output('{"data":{"viewer":{"id":123,"login":" "}}}')));
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

const error = yield* Effect.flip(cli.getViewerLogin({ cwd: "/w", host: "github.com" }));
Expand All @@ -2582,14 +2613,23 @@ layer("GitHubPullRequestCli.layer", (it) => {
Effect.gen(function* () {
mockedExecute
.mockReturnValueOnce(Effect.succeed(output("enterprise-test-credential")))
.mockReturnValueOnce(Effect.succeed(output('{"id":456,"login":"enterprise-user"}')));
.mockReturnValueOnce(
Effect.succeed(output('{"data":{"viewer":{"id":456,"login":"enterprise-user"}}}')),
);
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

const login = yield* cli.getViewerLogin({ cwd: "/w", host: "github.acme.com" });

expect(login).toBe("enterprise-user");
expect(callAt(0).args).toEqual(["auth", "token", "--hostname", "github.acme.com"]);
expect(callAt(1).args).toEqual(["api", "user", "--hostname", "github.acme.com"]);
expect(callAt(1).args.slice(0, 5)).toEqual([
"api",
"graphql",
"--hostname",
"github.acme.com",
"-f",
]);
expect(callAt(1).args.at(-1)).toContain("query={viewer{id:databaseId,login}");
}),
);

Expand All @@ -2599,7 +2639,9 @@ layer("GitHubPullRequestCli.layer", (it) => {
const input = { cwd: "/w", host: "github.identity-cache.test" };
mockedExecute
.mockReturnValueOnce(Effect.succeed(output("test-credential-a")))
.mockReturnValueOnce(Effect.succeed(output('{"id":123,"login":"maria-rcks"}')));
.mockReturnValueOnce(
Effect.succeed(output('{"data":{"viewer":{"id":123,"login":"maria-rcks"}}}')),
);
expect(yield* cli.getRoutingIdentity(input)).toEqual({
accountId: "123",
viewer: "maria-rcks",
Expand Down Expand Up @@ -2637,14 +2679,39 @@ layer("GitHubPullRequestCli.layer", (it) => {

mockedExecute
.mockReturnValueOnce(Effect.succeed(output("test-credential-b")))
.mockReturnValueOnce(Effect.succeed(output('{"id":456,"login":"maria-rcks"}')));
.mockReturnValueOnce(
Effect.succeed(output('{"data":{"viewer":{"id":456,"login":"maria-rcks"}}}')),
);
expect(yield* cli.getRoutingIdentity(input)).toEqual({
accountId: "456",
viewer: "maria-rcks",
});
}),
);

it.effect("fails the viewer read when the identity lookup is refused", () =>
Effect.gen(function* () {
mockedExecute
.mockReturnValueOnce(Effect.succeed(output("refused-credential")))
.mockReturnValueOnce(
Effect.fail(
new GitHubCli.GitHubCliCommandError({
command: "gh",
cwd: "/w",
cause: new Error("HTTP 403"),
}),
),
);
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

const error = yield* Effect.flip(
cli.getViewerLogin({ cwd: "/w", host: "github.viewer-refused.test" }),
);

assert.strictEqual(error._tag, "GitHubViewerLoginUnavailableError");
}),
);

it.effect("sends a whole review as one request body over stdin", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValue(Effect.succeed(output("{}")));
Expand Down
23 changes: 19 additions & 4 deletions apps/server/src/pullRequest/GitHubPullRequestCli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,8 +1024,12 @@ export const make = Effect.gen(function* () {
const decodeRoutingIdentity = Schema.decodeUnknownEffect(
Schema.fromJsonString(
Schema.Struct({
id: PositiveInt,
login: TrimmedNonEmptyString,
data: Schema.Struct({
viewer: Schema.Struct({
id: PositiveInt,

@macroscopeapp macroscopeapp Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High pullRequest/GitHubPullRequestCli.ts:1029

A valid GraphQL response with viewer.databaseId: null is decoded as GitHubViewerLoginUnavailableError, so authenticated accounts such as app[bot] cannot be routed even though viewer.login is present. Because databaseId is nullable, PositiveInt rejects this response; decode the field as nullable and explicitly use a non-null routing identity or handle the missing ID without discarding the login.

🤖 Copy this AI Prompt to have your agent fix this: 
In file @apps/server/src/pullRequest/GitHubPullRequestCli.ts around line 1029:

A valid GraphQL response with `viewer.databaseId: null` is decoded as `GitHubViewerLoginUnavailableError`, so authenticated accounts such as `app[bot]` cannot be routed even though `viewer.login` is present. Because `databaseId` is nullable, `PositiveInt` rejects this response; decode the field as nullable and explicitly use a non-null routing identity or handle the missing ID without discarding the login.

login: TrimmedNonEmptyString,
}),
}),
}),
),
);
Expand Down Expand Up @@ -1067,10 +1071,17 @@ export const make = Effect.gen(function* () {
if (cached !== undefined && now - cached.at < 10 * 60_000)
return { ...credential, ...cached.value };
// Pin this read so an auth switch cannot poison its cache entry.
// REST GET /user refuses GitHub App installation tokens; the GraphQL viewer
// answers both those and user tokens with the same login. The read goes
// through the budget like every other GraphQL call, so the response stays
// unfiltered for observe to learn the rate-limit snapshot.
const document = yield* graphQlBudget.query(host, "{viewer{id:databaseId,login}}", {
allowReserve: true,
});
const response = yield* github
.execute({
cwd: input.cwd,
args: ["api", "user", "--hostname", host],
args: ["api", "graphql", "--hostname", host, "-f", `query=${document}`],
env: {
GH_HOST: host,
GH_TOKEN: token,
Expand All @@ -1081,10 +1092,14 @@ export const make = Effect.gen(function* () {
},
})
.pipe(Effect.mapError(unavailable));
yield* graphQlBudget.observe(host, response.stdout);
const identity = yield* decodeRoutingIdentity(response.stdout).pipe(
Effect.mapError(unavailable),
);
const value = { accountId: String(identity.id), viewer: identity.login };
const value = {
accountId: String(identity.data.viewer.id),
viewer: identity.data.viewer.login,
};
if (routingIdentities.size >= 128)
routingIdentities.delete(routingIdentities.keys().next().value!);
routingIdentities.set(key, { at: now, value });
Expand Down
Loading