diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index a62d1ed21970..79c63f348195 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -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(""); }), }), @@ -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( @@ -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; @@ -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" })); @@ -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}"); }), ); @@ -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", @@ -2637,7 +2679,9 @@ 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", @@ -2645,6 +2689,29 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + 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("{}"))); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 2558f6695f1a..572b3e879ca7 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -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, + login: TrimmedNonEmptyString, + }), + }), }), ), ); @@ -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, @@ -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 });