diff --git a/apps/server/scripts/measure-pr-preview.ts b/apps/server/scripts/measure-pr-preview.ts index f967d1f26a8eebd636e1be1c89b35ca3d44303b7..ae291cd17cc69884cd0f05ead65c24232b03cb1b 100644 --- a/apps/server/scripts/measure-pr-preview.ts +++ b/apps/server/scripts/measure-pr-preview.ts @@ -1,18 +1,21 @@ // Run with: node apps/server/scripts/measure-pr-preview.ts owner/repo 123 456 // Numbers form a session with shared repository-permission caches. Browser and // service caches are excluded. Uses real GitHub reads, without a server or database. -// CLI-generated GraphQL -// queries are replayed with rateLimit.cost, outside the timed section, to measure -// their cost without confusing other applications' traffic with this process's. +// Every GraphQL read carries `rateLimit`, so its cost is read off its own answer. import * as NodeServices from "@effect/platform-node/NodeServices"; import * as Effect from "effect/Effect"; import * as Console from "effect/Console"; import * as Layer from "effect/Layer"; +import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; +import { FetchHttpClient } from "effect/http"; import * as GitHubPullRequestCli from "../src/pullRequest/GitHubPullRequestCli.ts"; import * as GitHubPullRequestProvider from "../src/pullRequest/GitHubPullRequestProvider.ts"; -import * as GitHubCli from "../src/sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../src/sourceControl/GitHubApi.ts"; +import * as GitHubCredentials from "../src/sourceControl/GitHubCredentials.ts"; +import * as GitHubGraphQlBudget from "../src/sourceControl/githubGraphQlBudget.ts"; +import * as SourceControlRateLimit from "../src/sourceControl/SourceControlRateLimit.ts"; import * as VcsProcess from "../src/vcs/VcsProcess.ts"; const [repository, ...numbers] = process.argv.slice(2); @@ -20,77 +23,58 @@ if (!repository || numbers.length === 0 || numbers.some((number) => !/^\d+$/.tes throw new Error("Usage: node apps/server/scripts/measure-pr-preview.ts owner/repo number..."); } -type Read = { graphqlRequests: number; restRequests: number; cost: number; debug: string }; +type Read = { graphqlRequests: number; restRequests: number; cost: number }; const reads: Read[] = []; -const layerMeasuredProcess = Layer.effect( - VcsProcess.VcsProcess, +const decodeCost = Schema.decodeUnknownOption( + Schema.fromJsonString( + Schema.Struct({ + data: Schema.Struct({ rateLimit: Schema.Struct({ cost: Schema.Number }) }), + }), + ), +); +const measuredApi = Layer.effect( + GitHubApi.GitHubApi, Effect.gen(function* () { - const vcs = yield* VcsProcess.VcsProcess; - return VcsProcess.VcsProcess.of({ - run: (input) => - vcs.run({ ...input, env: { ...input.env, GH_DEBUG: input.env?.GH_DEBUG ?? "api" } }).pipe( - Effect.tap((output) => - Effect.sync(() => { - const graphqlRequests = [...output.stderr.matchAll(/^> POST \/graphql /gm)].length; - const requests = [...output.stderr.matchAll(/^> (?:GET|POST) /gm)].length; - const cost = /"rateLimit"\s*:\s*\{[^}]*"cost"\s*:\s*(\d+)/.exec(output.stdout)?.[1]; + const api = yield* GitHubApi.make; + return GitHubApi.GitHubApi.of({ + ...api, + graphql: (input) => + api.graphql(input).pipe( + Effect.tap((body) => + Effect.sync(() => reads.push({ - graphqlRequests, - restRequests: requests - graphqlRequests, - cost: Number(cost ?? 0), - debug: output.stderr, - }); - }), + graphqlRequests: 1, + restRequests: 0, + cost: Option.match(decodeCost(body), { + onNone: () => 0, + onSome: (decoded) => decoded.data.rateLimit.cost, + }), + }), + ), ), ), + rest: (input) => + api + .rest(input) + .pipe( + Effect.tap(() => + Effect.sync(() => reads.push({ graphqlRequests: 0, restRequests: 1, cost: 0 })), + ), + ), }); }), -).pipe(Layer.provide(VcsProcess.layer), Layer.provide(NodeServices.layer)); - -const layerServices = GitHubPullRequestCli.layer.pipe( - Layer.provide(GitHubCli.layer), - Layer.provideMerge(layerMeasuredProcess), +).pipe( + Layer.provide(GitHubCredentials.layer), + Layer.provide(GitHubGraphQlBudget.layer), + Layer.provide(SourceControlRateLimit.layer), + Layer.provide(FetchHttpClient.layer), + Layer.provide(VcsProcess.layer), Layer.provide(NodeServices.layer), ); -const decodeJson = Schema.decodeSync(Schema.fromJsonString(Schema.Unknown)); +const services = GitHubPullRequestCli.layer.pipe(Layer.provideMerge(measuredApi)); + const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); -const graphqlCost = Effect.fn("measurePrPreview.graphqlCost")(function* (read: Read) { - // The REST quota probe synthesizes rateLimit.cost for budget admission. - // It does not spend GraphQL points. - if (!read.graphqlRequests) return 0; - if (read.cost) return read.cost; - const vcs = yield* VcsProcess.VcsProcess; - let cost = 0; - let found = 0; - for (const match of read.debug.matchAll( - /GraphQL query:\n([\s\S]*?)\nGraphQL variables: (\{[^\n]*\})/g, - )) { - const query = match[1]!.replace(/(\bquery\b[^{]*\{)/, "$1 rateLimit { cost }"); - const output = yield* vcs.run({ - operation: "measurePrPreview.cost", - command: "gh", - cwd: process.cwd(), - args: [ - "api", - "graphql", - "--hostname", - "github.com", - "--input", - "-", - "--jq", - ".data.rateLimit.cost", - ], - stdin: encodeJson({ query, variables: decodeJson(match[2]!) }), - env: { GH_DEBUG: "" }, - }); - cost += Number(output.stdout.trim()); - found++; - } - if (found !== read.graphqlRequests) - throw new Error("Could not account for every GraphQL request"); - return cost; -}); for (const mode of ["detail", "preview"] as const) { // Start each side cold, then retain the caches shared across PRs in one session. @@ -108,8 +92,7 @@ for (const mode of ["detail", "preview"] as const) { : provider.getChangeRequestPreview!(input); const elapsedMs = Math.round(performance.now() - start); const measured = [...reads]; - const costs = yield* Effect.forEach(measured, graphqlCost); - const points = costs.reduce((total, cost) => total + cost, 0); + const points = measured.reduce((total, read) => total + read.cost, 0); return { repository, number, @@ -123,7 +106,7 @@ for (const mode of ["detail", "preview"] as const) { }), ); }), - ).pipe(Effect.provide(layerServices)), + ).pipe(Effect.provide(services)), ); for (const row of rows) await Effect.runPromise(Console.log(encodeJson(row))); } diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 44730cf133500396a085a95b2c7340d637b59521..33f0df8e8bcbaeec348d454439b08d07e9cab301 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -7,13 +7,17 @@ import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import * as Schema from "effect/Schema"; import * as TestClock from "effect/testing/TestClock"; -import { ChildProcessSpawner } from "effect/process"; +import * as Redacted from "effect/Redacted"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; -import * as VcsProcess from "../vcs/VcsProcess.ts"; -import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; +import { HttpClient, HttpClientResponse } from "effect/http"; + +import { AllowGitHubReserve } from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; +import * as GitHubCredentials from "../sourceControl/GitHubCredentials.ts"; import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; +import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; +import { KnownWorkflowRuns } from "./gitHubConditionalChecks.ts"; import { BASE_COMPARISON_GRAPHQL_QUERY } from "./gitHubPullRequestJson.ts"; const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); @@ -48,60 +52,141 @@ const coreResponse = (pullRequest: Readonly> = {}) => ({ }, }); -const mockedExecute = vi.fn(); -const mockedStackMemberships = vi.fn(() => +/** One request the CLI made, as the transport received it. */ +type ApiCall = + | ({ readonly kind: "graphql" } & GitHubApi.GitHubGraphQlInput) + | ({ readonly kind: "rest" } & GitHubApi.GitHubRestInput); + +type ApiAnswer = Effect.Effect; + +const mockedExecute = vi.fn<(call: ApiCall) => ApiAnswer>(); +const defaultCredential = (host: string) => + Effect.succeed({ token: Redacted.make("token"), fingerprint: `${host}:token` }); +const mockedCredential = vi.fn(defaultCredential); +const mockedStackMemberships = vi.fn<(call: ApiCall) => ApiAnswer>(() => Effect.succeed(output('{"data":{}}')), ); -const mockedGetPullRequest = vi.fn(); + +/** + * A transport that answers from the mocks above but spends the real GraphQL budget, the way + * GitHubApi does, so the reserve and pause behaviour is still the module's to prove. + */ +const mockApi = Layer.effect( + GitHubApi.GitHubApi, + Effect.gen(function* () { + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + return GitHubApi.GitHubApi.of({ + graphql: (input) => + budget + .query(input.host, input.query, input.allowReserve ? { allowReserve: true } : undefined) + .pipe( + Effect.flatMap((query) => + (input.query.includes("query PullRequestStackMemberships") + ? mockedStackMemberships + : mockedExecute)({ kind: "graphql", ...input, query }), + ), + Effect.map((answer) => answer.body), + Effect.tap((body) => budget.observe(input.host, body)), + ), + rest: (input) => mockedExecute({ kind: "rest", ...input }), + credential: (host) => mockedCredential(host), + }); + }), +); const layer = it.layer( GitHubPullRequestCli.layer.pipe( - Layer.provide( - Layer.mock(GitHubCli.GitHubCli)({ - execute: (input) => - input.args.some((arg) => arg.includes("query PullRequestStackMemberships")) - ? mockedStackMemberships(input) - : mockedExecute(input), - getPullRequest: mockedGetPullRequest, - }), - ), + Layer.provideMerge(mockApi), Layer.provideMerge(GitHubGraphQlBudget.layer), Layer.provide(NodeCrypto.layer), ), ); -function output(stdout: string, stdoutTruncated = false, stdoutInvalidUtf8 = false) { - return { - exitCode: ChildProcessSpawner.ExitCode(0), - stdout, - stderr: "", - stdoutTruncated, - stderrTruncated: false, - stdoutInvalidUtf8, - }; +/** A successful answer; GraphQL reads take its body. */ +function output( + body: string, + truncated = false, + invalidUtf8 = false, +): GitHubApi.GitHubRestResponse { + return { status: 200, headers: {}, body, truncated, invalidUtf8 }; } -function pullRequests( +/** + * Listing rows as GraphQL answers them. Overrides may use the flat shapes a test reads most + * easily: `reviewRequests` as `[{ login }]` or `[{ slug }]`, and `checks` as the rollup state. + */ +function rows( count: number, firstNumber: number, overrides: (number: number) => Readonly> = () => ({}), -): string { - return JSON.stringify( - Array.from({ length: count }, (_, index) => ({ - number: firstNumber + index, - title: `Pull request ${firstNumber + index}`, - url: `https://github.com/acme/web/pull/${firstNumber + index}`, +): ReadonlyArray> { + return Array.from({ length: count }, (_, index) => { + const number = firstNumber + index; + const { reviewRequests, checks, ...rest } = overrides(number) as { + reviewRequests?: ReadonlyArray>; + checks?: string; + } & Record; + return { + number, + title: `Pull request ${number}`, + url: `https://github.com/acme/web/pull/${number}`, headRefName: "feat/page", baseRefName: "main", createdAt: "2026-07-01T00:00:00Z", updatedAt: "2026-07-02T00:00:00Z", - ...overrides(firstNumber + index), - })), - ); + repository: { nameWithOwner: "acme/web" }, + ...(reviewRequests === undefined + ? {} + : { + reviewRequests: { + nodes: reviewRequests.map((requestedReviewer) => ({ requestedReviewer })), + }, + }), + ...(checks === undefined + ? {} + : { commits: { nodes: [{ commit: { statusCheckRollup: { state: checks } } }] } }), + ...rest, + }; + }); } +/** A page of the searched listing. */ +function pullRequests( + count: number, + firstNumber: number, + overrides?: (number: number) => Readonly>, +): string { + return encodeJson({ + data: { + search: { pageInfo: { hasNextPage: false }, nodes: rows(count, firstNumber, overrides) }, + }, + }); +} + +/** A page of a repository's own list, which is what the search-free fallback reads. */ +function listedPullRequests( + count: number, + firstNumber: number, + overrides?: (number: number) => Readonly>, +): string { + return encodeJson({ + data: { + repository: { + pullRequests: { + pageInfo: { hasNextPage: false }, + nodes: rows(count, firstNumber, overrides), + }, + }, + }, + }); +} + +/** A search that found nothing, which is also what GitHub says for a repository it does not index. */ +const emptySearch = () => output(pullRequests(0, 1)); +const emptyList = () => output(listedPullRequests(0, 1)); + function pullRequestFiles(count: number, firstIndex: number): string { - return JSON.stringify( + return encodeJson( Array.from({ length: count }, (_, index) => ({ filename: `src/file${firstIndex + index}.ts`, status: "modified", @@ -139,7 +224,7 @@ function reviewThreadsPage( nodes: ReadonlyArray>, endCursor: string | null, ): string { - return JSON.stringify({ + return encodeJson({ data: { repository: { pullRequest: { @@ -160,7 +245,7 @@ function threadCommentsPage( totalCount: number, pullRequestId = "PR_7", ): string { - return JSON.stringify({ + return encodeJson({ data: { repository: { pullRequest: { id: "PR_7" } }, node: { @@ -171,26 +256,43 @@ function threadCommentsPage( }); } -/** What `gh pr diff` answers on a pull request GitHub will not serve a diff for. */ -const diffRefused = new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/w", - cause: new Error("HTTP 406: the diff exceeded the maximum number of files (300)"), +/** What GitHub answers for a pull request it will not serve a whole diff for. */ +const diffRefused = new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "getPullRequestDiff", + status: 406, }); -/** The whole invocation the nth call made, so both argv and stdin can be asserted. */ -function callAt(index: number) { +/** The nth request the CLI made. */ +function callAt(index: number): ApiCall { const call = mockedExecute.mock.calls[index]; assert.isDefined(call); return call[0]; } -/** The one argument `--search` carries, which is where every listing filter ends up. */ +/** The GraphQL variables of the nth request. */ +function varsAt(index: number): Readonly> { + const call = callAt(index); + assert.strictEqual(call.kind, "graphql"); + return call.kind === "graphql" ? (call.variables ?? {}) : {}; +} + +/** The GraphQL document of the nth request. */ +function queryAt(index: number): string { + const call = callAt(index); + return call.kind === "graphql" ? call.query : ""; +} + +/** The REST path of the nth request. */ +function pathAt(index: number): string { + const call = callAt(index); + return call.kind === "rest" ? call.path : ""; +} + +/** The search the nth request carried, or undefined for a read that did not search. */ function searchOfCall(index: number): string | undefined { - const args = callAt(index).args; - const flag = args.indexOf("--search"); - // Absent is its own answer: a read that carries no `--search` at all is what the fallback is. - return flag === -1 ? undefined : args[flag + 1]; + const q = callAt(index).kind === "graphql" ? varsAt(index)["q"] : undefined; + return typeof q === "string" ? q : undefined; } /** One row as a search answers it, which is the listing's row one connection deeper. */ @@ -214,19 +316,143 @@ function searchItem(number: number, repository: string, updatedAt: string) { } function searchPage(nodes: ReadonlyArray, hasNextPage = false) { - return output(JSON.stringify({ data: { search: { pageInfo: { hasNextPage }, nodes } } })); + return output(encodeJson({ data: { search: { pageInfo: { hasNextPage }, nodes } } })); } -/** The search a batched read sent, which travels in the request body rather than in argv. */ -function searchQueryOfCall(index: number): string | undefined { - const body = JSON.parse(callAt(index).stdin ?? "{}") as { variables?: { q?: string } }; - return body.variables?.q; +const searchQueryOfCall = searchOfCall; + +/** + * Answers every request with the body of the first route whose needle its GraphQL document or + * REST path contains, and anything unrouted with an empty object, which a mutation never reads. + */ +function route(...routes: ReadonlyArray): void { + mockedExecute.mockImplementation((call) => { + const target = call.kind === "graphql" ? call.query : call.path; + const match = routes.find(([needle]) => target.includes(needle)); + return Effect.succeed(output(match === undefined ? "{}" : encodeJson(match[1]))); + }); +} + +/** The variables of every GraphQL request whose document contains `needle`, in order. */ +function variablesOf(needle: string): ReadonlyArray>> { + return mockedExecute.mock.calls.flatMap(([call]) => + call.kind === "graphql" && call.query.includes(needle) ? [call.variables ?? {}] : [], + ); +} + +/** Every REST request whose path contains `needle`, in order. */ +function restCallsTo(needle: string): ReadonlyArray { + return mockedExecute.mock.calls.flatMap(([call]) => + call.kind === "rest" && call.path.includes(needle) ? [call] : [], + ); +} + +/** + * The check that a client-given node hangs off the pull request it names. Routed before the node + * id lookup, whose document it contains. + */ +const SUBJECT_SCOPE_QUERY = "node(id: $subjectId)"; +const subjectScope = (subjectId: string, pullRequestId: string) => ({ + data: { + repository: { pullRequest: { id: pullRequestId } }, + node: { id: subjectId, pullRequest: { id: pullRequestId } }, + }, +}); + +/** One page of a head commit's check contexts, the read a rollup past one page is walked by. */ +const checkContextsPage = ( + nodes: ReadonlyArray>, + endCursor: string | null, + headRefOid = "abc123", +) => + encodeJson({ + data: { + repository: { + pullRequest: { + headRefOid, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { nodes, pageInfo: { hasNextPage: endCursor !== null, endCursor } }, + }, + }, + }, + ], + }, + }, + }, + }, + }); + +/** A pull request's base and head, as the REST read a file expansion starts from answers them. */ +const pullRequestRefs = encodeJson({ base: { sha: "a1b2c3d" }, head: { sha: "b1c2d3e" } }); + +/** A cross-repository pull request whose head waits on a maintainer to run its workflows. */ +const crossRepositoryDetail = (headRefOid = "abc123") => + coreResponse({ + headRefName: "feat/page", + headRefOid, + isCrossRepository: true, + headRepositoryOwner: { login: "octocat" }, + }); + +/** Open pull requests on one head branch, every one of them from the same fork head. */ +const heads = (numbers: ReadonlyArray) => ({ + data: { + repository: { + pullRequests: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: numbers.map((number) => ({ + number, + headRefOid: "abc123", + isCrossRepository: true, + headRepositoryOwner: { login: "octocat" }, + })), + }, + }, + }, +}); + +const workflowRuns = (ids: ReadonlyArray) => ({ + workflow_runs: ids.map((id) => ({ + id, + name: id === 10 ? "build" : `run ${id}`, + html_url: `https://example.com/${id}`, + })), +}); + +/** Answers the reads an approval makes: the detail, the heads, the runs; approvals return nothing. */ +function workflowApprovalRoutes( + detail: () => unknown, + headsAnswer: unknown, + runsAnswer: unknown, +): void { + mockedExecute.mockImplementation((call) => + Effect.sync(() => + output( + call.kind === "rest" + ? call.path.endsWith("/approve") + ? "" + : encodeJson(runsAnswer) + : call.query.includes("headRefName: $head") + ? encodeJson(headsAnswer) + : encodeJson(detail()), + ), + ), + ); } +/** The pull request node id lookup, which the layer caches for every test after the first. */ +const NODE_ID_QUERY = "pullRequest(number: $number) { id }"; +const nodeIdAnswer = (id: string) => ({ data: { repository: { pullRequest: { id } } } }); + afterEach(() => { mockedExecute.mockReset(); mockedStackMemberships.mockReset(); - mockedGetPullRequest.mockReset(); + mockedCredential.mockReset(); + mockedCredential.mockImplementation(defaultCredential); }); it.effect( @@ -234,22 +460,41 @@ it.effect( () => Effect.gen(function* () { let activeToken = "broad-credential"; - const commands: VcsProcess.VcsProcessInput[] = []; - const github = yield* GitHubCli.make.pipe( - Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, SourceControlRateLimit.layer)), - Effect.provideService(VcsProcess.VcsProcess, { - run: (input) => - 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"}'); - return output(""); - }), + const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = + []; + const credentials = Layer.succeed( + GitHubCredentials.GitHubCredentials, + GitHubCredentials.GitHubCredentials.of({ + get: (host) => + Effect.sync(() => ({ + host, + token: Redacted.make(activeToken), + source: "gh" as const, + fingerprint: `${host}:${activeToken.length}${activeToken.at(-1)}`, + })), + invalidate: () => Effect.void, }), ); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.sync(() => { + requests.push({ url: request.url, authorization: request.headers["authorization"] }); + const body = request.url.endsWith("/user") + ? { id: 123, login: "same-account" } + : { data: { repository: { pullRequest: { id: "PR_1" } }, addComment: {} } }; + return HttpClientResponse.fromWeb(request, new Response(encodeJson(body))); + }), + ), + ); const cli = yield* GitHubPullRequestCli.make.pipe( - Effect.provideService(GitHubCli.GitHubCli, github), - Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, NodeCrypto.layer)), + Effect.provide( + GitHubApi.layer.pipe( + Layer.provide(Layer.mergeAll(credentials, http)), + Layer.provide(GitHubGraphQlBudget.layer), + Layer.provide(SourceControlRateLimit.layer), + ), + ), ); const input = { cwd: "/repo", host: "github.com" }; const first = yield* cli.withVerifiedCredential(input, (identity) => @@ -270,16 +515,17 @@ it.effect( expect(first.credentialFingerprint).not.toBe(second.credentialFingerprint); expect(encodeJson([first, second])).not.toContain("broad-credential"); expect(encodeJson([first, second])).not.toContain("restricted-credential"); - expect(commands.find((command) => command.args[0] === "pr")?.env).toMatchObject({ - GH_TOKEN: "broad-credential", - GITHUB_TOKEN: "broad-credential", - GH_DEBUG: "", - }); + // The comment was written under the credential the page verified, not the switched one. expect( - commands - .filter((command) => command.args[0] === "api") - .map((command) => command.env?.GH_TOKEN), - ).toEqual(["broad-credential", "restricted-credential"]); + requests + .filter((request) => request.url.endsWith("/graphql")) + .map((request) => request.authorization), + ).toEqual(["Bearer broad-credential", "Bearer broad-credential"]); + expect( + requests + .filter((request) => request.url.endsWith("/user")) + .map((request) => request.authorization), + ).toEqual(["Bearer broad-credential", "Bearer restricted-credential"]); expect(yield* cli.getRoutingIdentity(input)).toEqual({ accountId: "123", viewer: "same-account", @@ -394,18 +640,14 @@ layer("GitHubPullRequestCli.layer", (it) => { }, }); expect(mockedExecute).toHaveBeenCalledTimes(1); - expect(callAt(0).args).toEqual( - expect.arrayContaining(["api", "graphql", "--hostname", "github.example"]), - ); + expect(callAt(0)).toMatchObject({ kind: "graphql", host: "github.example" }); }), ); it.effect("coalesces concurrent identity verification for the same host and credential", () => Effect.gen(function* () { - mockedExecute.mockImplementation((input) => - input.args[0] === "auth" - ? Effect.succeed(output("shared-credential")) - : Effect.yieldNow.pipe(Effect.as(output('{"id":123,"login":"viewer"}'))), + mockedExecute.mockImplementation(() => + Effect.yieldNow.pipe(Effect.as(output('{"id":123,"login":"viewer"}'))), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const results = yield* Effect.all( @@ -417,7 +659,8 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(results).toEqual( Array.from({ length: 4 }, () => ({ accountId: "123", viewer: "viewer" })), ); - expect(mockedExecute.mock.calls.filter(([input]) => input.args[0] === "api")).toHaveLength(1); + expect(mockedExecute).toHaveBeenCalledTimes(1); + expect(pathAt(0)).toBe("user"); }), ); @@ -426,15 +669,9 @@ layer("GitHubPullRequestCli.layer", (it) => { () => Effect.gen(function* () { const firstStarted = yield* Deferred.make(); - const secondStarted = yield* Deferred.make(); - let tokens = 0; let verifications = 0; - mockedExecute.mockImplementation((input) => + mockedExecute.mockImplementation(() => Effect.gen(function* () { - if (input.args[0] === "auth") { - if (++tokens === 2) yield* Deferred.succeed(secondStarted, undefined); - return output("cancel-credential"); - } if (++verifications === 1) { yield* Deferred.succeed(firstStarted, undefined); return yield* Effect.never; @@ -446,8 +683,9 @@ layer("GitHubPullRequestCli.layer", (it) => { const input = { cwd: "/w", host: "github.identity-cancel.test" }; const first = yield* cli.getRoutingIdentity(input).pipe(Effect.forkChild); yield* Deferred.await(firstStarted); + // The second reader is queued on the same credential's lock behind the first. const second = yield* cli.getRoutingIdentity(input).pipe(Effect.forkChild); - yield* Deferred.await(secondStarted); + yield* Effect.yieldNow; yield* Fiber.interrupt(first); expect(yield* Fiber.join(second)).toEqual({ accountId: "123", viewer: "viewer" }); expect(verifications).toBe(2); @@ -479,8 +717,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { s0: { pullRequest: node(7) }, s1: { pullRequest: node(8) } }, }), ), @@ -526,7 +763,7 @@ layer("GitHubPullRequestCli.layer", (it) => { ); assert.strictEqual(eight?.headBranch, "feat/8"); expect(mockedExecute).toHaveBeenCalledOnce(); - const document = callAt(0).args.at(-1) ?? ""; + const document = queryAt(0); expect(document).toContain( 's0: repository(owner: "acme", name: "web") { pullRequest(number: 7)', ); @@ -575,7 +812,8 @@ layer("GitHubPullRequestCli.layer", (it) => { // GitHub had no answer for #8, so its watch reads it in full. expect(eight).toBeNull(); expect(mockedExecute).toHaveBeenCalledOnce(); - expect(callAt(0).args.at(-1) ?? "").toContain( + const call = callAt(0); + expect(call.kind === "graphql" ? call.query : "").toContain( 'w1: repository(owner: "acme", name: "web") { pullRequest(number: 8)', ); }), @@ -588,34 +826,28 @@ layer("GitHubPullRequestCli.layer", (it) => { .mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - number: 7, - title: "Reuse the summary", - url: "https://github.com/acme/web/pull/7", - author: { login: "octocat", name: "Octo Cat" }, - baseRefName: "main", - headRefName: "feat/summary", - state: "OPEN", - isDraft: false, - mergeable: "MERGEABLE", - reviewDecision: "APPROVED", - additions: 12, - deletions: 3, - changedFiles: 2, - createdAt: "2026-08-20T00:00:00.000Z", - updatedAt: "2026-08-24T12:34:56.000Z", - reviewRequests: [], - labels: [], - statusCheckRollup: [ - { - __typename: "CheckRun", - status: "COMPLETED", - conclusion: "SUCCESS", - name: "ci", + encodeJson({ + data: { + s0: { + pullRequest: { + number: 7, + title: "Reuse the summary", + url: "https://github.com/acme/web/pull/7", + author: { login: "octocat", name: "Octo Cat" }, + baseRefName: "main", + headRefName: "feat/summary", + state: "OPEN", + isDraft: false, + mergeable: "MERGEABLE", + reviewDecision: "APPROVED", + additions: 12, + deletions: 3, + changedFiles: 2, + updatedAt: "2026-08-24T12:34:56.000Z", + commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, + }, }, - ], - body: "", + }, }), ), ), @@ -631,15 +863,7 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.strictEqual(summary.headBranch, "feat/summary"); assert.strictEqual(summary.checksState, "passing"); assert.strictEqual(mockedExecute.mock.calls.length, 2); - expect(callAt(1).args).toEqual([ - "pr", - "view", - "7", - "--repo", - "github.com/acme/web", - "--json", - expect.stringContaining("statusCheckRollup"), - ]); + expect(varsAt(1)).toEqual({ owner: "acme", name: "web", number: 7 }); }), ); @@ -648,8 +872,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify([ + encodeJson([ { id: 42, number: 3, @@ -688,12 +911,11 @@ layer("GitHubPullRequestCli.layer", (it) => { { number: 7, headBranch: "feat/two", state: "open" }, ], }); - assert.deepStrictEqual(callAt(0).args, [ - "api", - "--hostname", - "ghe.example.com", - "repos/acme/web/stacks?pull_request=7", - ]); + expect(callAt(0)).toMatchObject({ + kind: "rest", + host: "ghe.example.com", + path: "repos/acme/web/stacks?pull_request=7", + }); }), ); @@ -707,13 +929,11 @@ layer("GitHubPullRequestCli.layer", (it) => { { number: 7, head: { ref: "feat/two", sha: "abc123" }, state: "open", merged_at: null }, ], }; - // @effect-diagnostics-next-line preferSchemaOverJson:off - mockedExecute.mockReturnValueOnce(Effect.succeed(output(JSON.stringify([minimal])))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(encodeJson([minimal])))); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ ...minimal, pull_requests: [{ ...minimal.pull_requests[0], title: "Second layer", draft: false }], }), @@ -733,12 +953,7 @@ layer("GitHubPullRequestCli.layer", (it) => { headSha: "abc123", isDraft: false, }); - expect(callAt(1).args).toEqual([ - "api", - "--hostname", - "github.com", - "repos/acme/web/stacks/3", - ]); + expect(pathAt(1)).toBe("repos/acme/web/stacks/3"); }), ); @@ -760,13 +975,11 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("reads a host that refuses the stacks preview as not stacked", () => Effect.gen(function* () { - // The CLI classifies a missing preview endpoint as not found. mockedExecute.mockReturnValueOnce( Effect.fail( - new GitHubCli.GitHubPullRequestNotFoundError({ - command: "gh", - cwd: "/w", - cause: new Error("HTTP 404: Not Found (https://api.github.com/repos/acme/web/stacks)"), + new GitHubApi.GitHubApiNotFoundError({ + host: "github.com", + operation: "getPullRequestStack", }), ), ); @@ -783,14 +996,13 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("does not read a signed-out gh as an unstacked pull request", () => + it.effect("does not read a refused credential as an unstacked pull request", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( Effect.fail( - new GitHubCli.GitHubCliAuthenticationError({ - command: "gh", - cwd: "/w", - cause: new Error("gh auth login"), + new GitHubApi.GitHubApiAuthenticationError({ + host: "github.com", + operation: "getPullRequestStack", }), ), ); @@ -805,16 +1017,16 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - assert.strictEqual(error._tag, "GitHubCliAuthenticationError"); + assert.strictEqual(error._tag, "GitHubApiAuthenticationError"); }), ); it.effect("preserves transient stack failures instead of reporting no stack", () => Effect.gen(function* () { - const failure = new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/w", - cause: new Error("HTTP 503"), + const failure = new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "getPullRequestStack", + status: 503, }); mockedExecute.mockReturnValueOnce(Effect.fail(failure)); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -867,13 +1079,9 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.strictEqual(batch.items.length, 3); assert.isFalse(batch.truncated); - const args = callAt(0).args; - expect(args).toContain("--repo"); - expect(args).toContain("github.com/acme/web"); - expect(args).toContain("--state"); - expect(args).toContain("open"); - expect(args).toContain("--limit"); - expect(args).toContain("11"); + expect(callAt(0)).toMatchObject({ kind: "graphql", host: "github.com" }); + expect(searchOfCall(0)).toBe("is:pr is:open sort:updated-desc repo:acme/web"); + expect(queryAt(0)).toContain("first: 11"); }), ); @@ -899,7 +1107,13 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("excludes merged pull requests from the Closed tab", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -912,14 +1126,20 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 10, }); - // `--state closed` includes merged pull requests, so the tab narrows through search. - expect(searchOfCall(0)).toBe("is:unmerged sort:updated-desc"); + // A closed pull request may also be a merged one, so the tab narrows through search. + expect(searchOfCall(0)).toBe("is:pr is:closed is:unmerged sort:updated-desc repo:acme/web"); }), ); it.effect("narrows to the author on the authored tab", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -932,15 +1152,19 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 10, }); - const args = callAt(0).args; - expect(args).toContain("--author"); - expect(args).toContain("bilal"); + expect(searchOfCall(0)).toBe("is:pr is:open author:bilal sort:updated-desc repo:acme/web"); }), ); it.effect("narrows through search on the reviewing tab", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -953,7 +1177,9 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 10, }); - expect(searchOfCall(0)).toBe("review-requested:bilal sort:updated-desc"); + expect(searchOfCall(0)).toBe( + "is:pr is:open review-requested:bilal sort:updated-desc repo:acme/web", + ); }), ); @@ -1008,7 +1234,7 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("keeps a searched-for qualifier inside the phrase, and out of argv", () => + it.effect("keeps a searched-for qualifier inside the phrase", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(searchPage([]))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -1024,13 +1250,11 @@ layer("GitHubPullRequestCli.layer", (it) => { query: 'x" is:merged repo:evil/repo', }); - // Quoted and escaped, so the words a reader typed narrow the listing rather than widening - // it — and the whole document travels over stdin rather than in a visible argv. + // Quoted and escaped, so the words a reader typed narrow the listing rather than widening it. assert.strictEqual( searchQueryOfCall(0), 'is:pr is:open "x\\" is:merged repo:evil/repo" sort:updated-desc repo:acme/web', ); - expect(callAt(0).args).not.toContain("-f"); }), ); @@ -1139,12 +1363,12 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("enriches only the visible fallback rows after filtering and widening", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(pullRequests(3, 1, () => ({ isDraft: true })))), + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true })))), ); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(pullRequests(6, 1, (number) => ({ isDraft: number < 4 })))), + Effect.succeed(output(listedPullRequests(6, 1, (number) => ({ isDraft: number < 4 })))), ); mockedStackMemberships.mockReturnValueOnce( Effect.succeed( @@ -1180,7 +1404,8 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(batch.truncated).toBe(true); expect(batch.continues).toBe(false); expect(mockedStackMemberships).toHaveBeenCalledTimes(1); - const query = mockedStackMemberships.mock.calls[0]?.[0].args.at(-1) ?? ""; + const membership = mockedStackMemberships.mock.calls[0]?.[0]; + const query = membership?.kind === "graphql" ? membership.query : ""; expect(query).toContain("pullRequest(number: 4)"); expect(query).toContain("pullRequest(number: 5)"); expect(query).not.toContain("pullRequest(number: 1)"); @@ -1192,12 +1417,12 @@ layer("GitHubPullRequestCli.layer", (it) => { Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(27, 1)))); mockedStackMemberships.mockImplementation((input) => - input.args.at(-1)?.includes("pullRequest(number: 26)") + input.kind === "graphql" && input.query.includes("pullRequest(number: 26)") ? Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/w", - cause: new Error("HTTP 502"), + new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "listPullRequestStackMemberships", + status: 502, }), ) : Effect.succeed( @@ -1238,8 +1463,8 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("skips membership enrichment for empty pages and enterprise hosts", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptyList())); mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(1, 7)))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const input = { @@ -1268,7 +1493,7 @@ layer("GitHubPullRequestCli.layer", (it) => { // Every chunk answers for its first alias only, so a row GitHub said nothing about is // dropped rather than shown as a change of no size. Effect.succeed( - output(JSON.stringify({ data: { s0: { pullRequest: { additions: 4, deletions: 1 } } } })), + output(encodeJson({ data: { s0: { pullRequest: { additions: 4, deletions: 1 } } } })), ), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -1285,7 +1510,7 @@ layer("GitHubPullRequestCli.layer", (it) => { { repository: "acme/web", number: 1, additions: 4, deletions: 1 }, { repository: "acme/web", number: 26, additions: 4, deletions: 1 }, ]); - const document = callAt(0).args.at(-1) ?? ""; + const document = queryAt(0); expect(document).toContain('s0: repository(owner: "acme", name: "web")'); expect(document).toContain("pullRequest(number: 25)"); }), @@ -1310,7 +1535,13 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("hands a search to GitHub rather than to the rows already read", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1326,13 +1557,21 @@ layer("GitHubPullRequestCli.layer", (it) => { // The recency qualifier rides along, because free text would otherwise reorder the page // by relevance and truncation would drop the newest matches. - expect(searchOfCall(0)).toBe('"pull requests page" sort:updated-desc'); + expect(searchOfCall(0)).toBe( + 'is:pr is:open "pull requests page" sort:updated-desc repo:acme/web', + ); }), ); it.effect("joins a search onto the tab's own qualifiers instead of replacing them", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1346,16 +1585,21 @@ layer("GitHubPullRequestCli.layer", (it) => { query: "page", }); - // One `--search` is all gh reads, so a second would silently drop the first. - const args = callAt(0).args; - assert.strictEqual(args.filter((arg) => arg === "--search").length, 1); - expect(searchOfCall(0)).toBe('review-requested:bilal is:unmerged "page" sort:updated-desc'); + expect(searchOfCall(0)).toBe( + 'is:pr is:closed is:unmerged review-requested:bilal "page" sort:updated-desc repo:acme/web', + ); }), ); it.effect("carries the further narrowings into the search as qualifiers", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1379,15 +1623,21 @@ layer("GitHubPullRequestCli.layer", (it) => { // Quotes around anything a reader typed, and the one character that could end a quoted // value early dropped rather than escaped. expect(searchOfCall(0)).toBe( - 'label:"needs design" label:"quote" -label:"wip" author:"octocat" draft:false ' + - "review:changes_requested status:failure sort:updated-desc", + 'is:pr is:open label:"needs design" label:"quote" -label:"wip" author:"octocat" ' + + "draft:false review:changes_requested status:failure sort:updated-desc repo:acme/web", ); }), ); it.effect('resolves an author filter of "me" to the viewer, not the literal word', () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1401,13 +1651,19 @@ layer("GitHubPullRequestCli.layer", (it) => { filters: { author: "me" }, }); - expect(searchOfCall(0)).toBe('author:"bilal" sort:updated-desc'); + expect(searchOfCall(0)).toBe('is:pr is:open author:"bilal" sort:updated-desc repo:acme/web'); }), ); it.effect("sends one label qualifier per group, its names joined the way GitHub ors them", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1422,8 +1678,9 @@ layer("GitHubPullRequestCli.layer", (it) => { }); // One qualifier satisfied by either size, and a second one that must hold as well. - expect(searchOfCall(0)).toBe('label:"size:S","size:XS" label:"bug" sort:updated-desc'); - expect(callAt(0).args).toContain('label:"size:S","size:XS" label:"bug" sort:updated-desc'); + expect(searchOfCall(0)).toBe( + 'is:pr is:open label:"size:S","size:XS" label:"bug" sort:updated-desc repo:acme/web', + ); }), ); @@ -1431,15 +1688,12 @@ layer("GitHubPullRequestCli.layer", (it) => { "falls back for a repository the index does not cover under a checks filter, keeping only the matching rows", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - pullRequests(2, 1, (number) => ({ - statusCheckRollup: - number === 1 - ? [{ name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }] - : [{ name: "test", status: "COMPLETED", conclusion: "FAILURE" }], + listedPullRequests(2, 1, (number) => ({ + checks: number === 1 ? "SUCCESS" : "FAILURE", })), ), ), @@ -1471,15 +1725,9 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("fails a checks filter for a row whose checks are still pending", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - pullRequests(1, 1, () => ({ - statusCheckRollup: [{ name: "build", status: "IN_PROGRESS" }], - })), - ), - ), + Effect.succeed(output(listedPullRequests(1, 1, () => ({ checks: "PENDING" })))), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -1504,9 +1752,9 @@ layer("GitHubPullRequestCli.layer", (it) => { "falls back for a repository the index does not cover even under a judgeable filter", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(pullRequests(2, 1, (number) => ({ isDraft: number === 1 })))), + Effect.succeed(output(listedPullRequests(2, 1, (number) => ({ isDraft: number === 1 })))), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -1555,7 +1803,13 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("quotes a search, so it cannot add a qualifier or a flag of its own", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1572,15 +1826,20 @@ layer("GitHubPullRequestCli.layer", (it) => { // Every word stays inside one phrase: nothing before it, nothing after it, and the // leading dashes are text rather than the start of another argument. expect(searchOfCall(0)).toBe( - String.raw`"-- is:merged label:secret \"widen me\"" sort:updated-desc`, + String.raw`is:pr is:open "-- is:merged label:secret \"widen me\"" sort:updated-desc repo:acme/web`, ); - expect(callAt(0).args).not.toContain("is:merged"); }), ); it.effect("escapes a backslash before the quote it would otherwise let out", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1596,13 +1855,21 @@ layer("GitHubPullRequestCli.layer", (it) => { // GitHub reads `\\` as one backslash and `\"` as one quote, so the phrase ends where // this says it does; escaping the quote alone would have closed it early. - expect(searchOfCall(0)).toBe(String.raw`"a\\\" is:merged" sort:updated-desc`); + expect(searchOfCall(0)).toBe( + String.raw`is:pr is:open "a\\\" is:merged" sort:updated-desc repo:acme/web`, + ); }), ); it.effect("asks for nothing but the order when the reader typed only spaces", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1618,7 +1885,7 @@ layer("GitHubPullRequestCli.layer", (it) => { // An empty phrase would match nothing rather than everything, so it is left out; the // order the page reads rows in is asked for whether or not anything was typed. - expect(searchOfCall(0)).toBe("sort:updated-desc"); + expect(searchOfCall(0)).toBe("is:pr is:open sort:updated-desc repo:acme/web"); }), ); @@ -1640,7 +1907,9 @@ layer("GitHubPullRequestCli.layer", (it) => { // Inclusive, so the rows already sent at that instant come back for the caller to drop — // which is what keeps the ones beside them from being skipped. - expect(searchOfCall(0)).toBe("updated:<=2026-07-02T00:00:00Z sort:updated-desc"); + expect(searchOfCall(0)).toBe( + "is:pr is:open updated:<=2026-07-02T00:00:00Z sort:updated-desc repo:acme/web", + ); assert.isTrue(batch.continues); }), ); @@ -1650,7 +1919,7 @@ layer("GitHubPullRequestCli.layer", (it) => { // The fallback is for a repository the index does not cover. Under a text search an empty // answer means the text matched nothing, and listing everything instead would fill the // page with rows the reader did not search for. - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const batch = yield* cli.listPullRequests({ @@ -1669,12 +1938,12 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("reads a repository GitHub will not search the way gh lists one", () => + it.effect("reads a repository GitHub will not search from its own list", () => Effect.gen(function* () { // GitHub answers for a repository outside its search index with no rows and no error. - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(pullRequests(3, 1, () => ({ state: "CLOSED" })))), + Effect.succeed(output(listedPullRequests(3, 1, () => ({ state: "CLOSED" })))), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -1689,20 +1958,21 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(batch.items.length, 3); - // The fallback itself uses no search, then narrows the decoded rows locally. They still - // arrive in gh's own order, so nothing can carry on from them. + // The fallback itself uses no search, then narrows the decoded rows locally. They arrive + // newest-created first, so nothing can carry on from them. expect(searchOfCall(1)).toBeUndefined(); + expect(varsAt(1)).toMatchObject({ owner: "acme", name: "web", states: ["CLOSED", "MERGED"] }); assert.isFalse(batch.continues); }), ); it.effect("keeps state and involvement filters on the search-free fallback", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - pullRequests(4, 1, (number) => ({ + listedPullRequests(4, 1, (number) => ({ state: number === 4 ? "OPEN" : "CLOSED", ...(number === 3 ? { mergedAt: "2026-07-03T00:00:00Z" } : {}), reviewRequests: @@ -1734,12 +2004,14 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("grows the search-free fallback until it fills the filtered page", () => Effect.gen(function* () { const unrelated = () => ({ reviewRequests: [{ login: "somebody-else" }] }); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("[]"))); - mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(3, 1, unrelated)))); + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 1, unrelated))), + ); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - pullRequests(4, 1, (number) => + listedPullRequests(4, 1, (number) => number === 4 ? { reviewRequests: [{ login: "bilal" }] } : unrelated(), ), ), @@ -1758,25 +2030,34 @@ layer("GitHubPullRequestCli.layer", (it) => { }); expect(batch.items.map((item) => item.number)).toEqual([4]); - const firstFallbackArgs = callAt(1).args; - const secondFallbackArgs = callAt(2).args; - expect(firstFallbackArgs[firstFallbackArgs.indexOf("--limit") + 1]).toBe("3"); - expect(secondFallbackArgs[secondFallbackArgs.indexOf("--limit") + 1]).toBe("6"); + expect(queryAt(1)).toContain("first: 3,"); + expect(queryAt(2)).toContain("first: 6,"); assert.isFalse(batch.truncated); }), ); it.effect("bounds a sparse search-free fallback and reports the unread tail", () => Effect.gen(function* () { - mockedExecute.mockImplementation((_input) => { - if (mockedExecute.mock.calls.length === 1) return Effect.succeed(output("[]")); - const args = callAt(mockedExecute.mock.calls.length - 1).args; - const limit = Number(args[args.indexOf("--limit") + 1]); + // A repository with far more pull requests than the bound, none of them for this reader. + let listed = 0; + mockedExecute.mockImplementation((call) => { + if (call.kind !== "graphql" || !call.query.includes("pullRequests(")) { + return Effect.succeed(emptySearch()); + } + const first = Number(/first: (\d+)/.exec(call.query)?.[1]); + const nodes = rows(first, listed + 1, () => ({ + reviewRequests: [{ login: "somebody-else" }], + })); + listed += first; return Effect.succeed( output( - pullRequests(limit, 1, () => ({ - reviewRequests: [{ login: "somebody-else" }], - })), + encodeJson({ + data: { + repository: { + pullRequests: { pageInfo: { hasNextPage: true, endCursor: `c${listed}` }, nodes }, + }, + }, + }), ), ); }); @@ -1792,8 +2073,10 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 2, }); - const finalArgs = callAt(mockedExecute.mock.calls.length - 1).args; - expect(finalArgs[finalArgs.indexOf("--limit") + 1]).toBe("1000"); + // The last widening reads a thousand rows, a page of a hundred at a time, and stops there. + const last = mockedExecute.mock.calls.length - 1; + expect(varsAt(last)["after"]).toBe(`c${listed - 100}`); + expect(listed - (3 + 6 + 12 + 24 + 48 + 96 + 192 + 384 + 768)).toBe(1000); assert.strictEqual(batch.items.length, 0); assert.isTrue(batch.truncated); }), @@ -1801,7 +2084,13 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("takes an empty slice for a repository that has run out, not one to read again", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -1821,9 +2110,32 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + /** The state a merge or branch update reads first: behind its base and blocked on checks. */ + const actionState = (pullRequest: Readonly> = {}) => ({ + data: { + repository: { + pullRequest: { + id: "PR_7", + headRefOid: "abc123", + isMergeQueueEnabled: false, + mergeStateStatus: "BLOCKED", + baseRef: { compare: { behindBy: 2 } }, + ...pullRequest, + }, + }, + }, + }); + const mergeMessage = (body: string, isMergeQueueEnabled = false) => ({ + data: { + repository: { + pullRequest: { isMergeQueueEnabled, headRefOid: "abc123", viewerMergeBodyText: body }, + }, + }, + }); + it.effect("updates a stale branch with a merge commit unless asked to rebase", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route(["query PullRequestActionState", actionState()]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -1833,9 +2145,6 @@ layer("GitHubPullRequestCli.layer", (it) => { number: 7, action: "update-branch", }); - // GitHub's own default, and `gh`'s: a merge commit unless the rebase flag says otherwise. - expect(callAt(0).args).toEqual(["pr", "update-branch", "7", "--repo", "github.com/acme/web"]); - yield* cli.runPullRequestAction({ cwd: "/w", repository: "acme/web", @@ -1844,20 +2153,18 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "update-branch", updateMethod: "rebase", }); - expect(callAt(1).args).toEqual([ - "pr", - "update-branch", - "7", - "--repo", - "github.com/acme/web", - "--rebase", + + // GitHub's own default: a merge commit unless asked to rebase, pinned to the head it read. + expect(variablesOf("updatePullRequestBranch(")).toEqual([ + { pullRequestId: "PR_7", expectedHeadOid: "abc123", updateMethod: "MERGE" }, + { pullRequestId: "PR_7", expectedHeadOid: "abc123", updateMethod: "REBASE" }, ]); }), ); it.effect("merges with the strategy it was asked for", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route(["query PullRequestActionState", actionState()]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -1869,14 +2176,10 @@ layer("GitHubPullRequestCli.layer", (it) => { mergeMethod: "squash", }); - expect(callAt(0).args).toEqual([ - "pr", - "merge", - "7", - "--repo", - "github.com/acme/web", - "--squash", + expect(variablesOf("mergePullRequest(")).toEqual([ + { input: { pullRequestId: "PR_7", mergeMethod: "SQUASH" } }, ]); + expect(variablesOf("enablePullRequestAutoMerge(")).toEqual([]); }), ); @@ -1884,26 +2187,15 @@ layer("GitHubPullRequestCli.layer", (it) => { "removes agent credits from the proposed message for %s", (action) => Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - pullRequest: { - isMergeQueueEnabled: false, - headRefOid: "abc123", - viewerMergeBodyText: - "Details\n\nCo-authored-by: Alice \nCo-authored-by: Claude ", - }, - }, - }, - }), - ), + route( + [ + "query PullRequestMergeMessage", + mergeMessage( + "Details\n\nCo-authored-by: Alice \nCo-authored-by: Claude ", ), - ) - .mockReturnValueOnce(Effect.succeed(output(""))); + ], + ["query PullRequestActionState", actionState()], + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ cwd: "/w", @@ -1914,12 +2206,20 @@ layer("GitHubPullRequestCli.layer", (it) => { mergeMethod: "squash", removeAgentCreditsOnMerge: true, }); - expect(callAt(0).args).toContain("method=SQUASH"); - expect(callAt(1).args.slice(-2)).toEqual(["--body-file", "-"]); - expect(callAt(1).args).toContain("--match-head-commit"); - expect(callAt(1).args).toContain("abc123"); - expect(callAt(1).stdin).toBe("Details\n\nCo-authored-by: Alice "); - expect(callAt(1).args.join(" ")).not.toContain("alice@example.com"); + expect(variablesOf("query PullRequestMergeMessage")[0]?.["method"]).toBe("SQUASH"); + expect( + variablesOf(action === "merge" ? "mergePullRequest(" : "enablePullRequestAutoMerge("), + ).toEqual([ + { + input: { + pullRequestId: "PR_7", + mergeMethod: "SQUASH", + // Pinned to the head the message was read from, so it cannot describe other commits. + expectedHeadOid: "abc123", + commitBody: "Details\n\nCo-authored-by: Alice ", + }, + }, + ]); }), ); @@ -1936,25 +2236,10 @@ layer("GitHubPullRequestCli.layer", (it) => { }, ] as const)("keeps GitHub's default message for $description", ({ body, queued }) => Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - pullRequest: { - isMergeQueueEnabled: queued, - headRefOid: "abc123", - viewerMergeBodyText: body, - }, - }, - }, - }), - ), - ), - ) - .mockReturnValueOnce(Effect.succeed(output(""))); + route( + ["query PullRequestMergeMessage", mergeMessage(body, queued)], + ["query PullRequestActionState", actionState({ isMergeQueueEnabled: queued })], + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ cwd: "/w", @@ -1964,33 +2249,23 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "merge", removeAgentCreditsOnMerge: true, }); - expect(callAt(0).args).toContain("method=MERGE"); - expect(callAt(1).args).not.toContain("--body-file"); - expect(callAt(1).stdin).toBeUndefined(); + expect(variablesOf("query PullRequestMergeMessage")[0]?.["method"]).toBe("MERGE"); + // A merge queue takes the pull request through auto-merge, with its own message. + expect(variablesOf(queued ? "enablePullRequestAutoMerge(" : "mergePullRequest(")).toEqual([ + { input: { pullRequestId: "PR_7", mergeMethod: "MERGE" } }, + ]); }), ); it.effect("passes an explicitly empty body when the proposed message only credits an agent", () => Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce( - Effect.succeed( - output( - encodeJson({ - data: { - repository: { - pullRequest: { - isMergeQueueEnabled: false, - headRefOid: "abc123", - viewerMergeBodyText: "Co-authored-by: Claude ", - }, - }, - }, - }), - ), - ), - ) - .mockReturnValueOnce(Effect.succeed(output(""))); + route( + [ + "query PullRequestMergeMessage", + mergeMessage("Co-authored-by: Claude "), + ], + ["query PullRequestActionState", actionState()], + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ cwd: "/w", @@ -2000,14 +2275,22 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "merge", removeAgentCreditsOnMerge: true, }); - expect(callAt(1).stdin).toBe(""); - expect(callAt(1).args).toContain("--body-file"); + expect(variablesOf("mergePullRequest(")).toEqual([ + { + input: { + pullRequestId: "PR_7", + mergeMethod: "MERGE", + expectedHeadOid: "abc123", + commitBody: "", + }, + }, + ]); }), ); it.effect("does not fetch a message for rebase merges", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route(["query PullRequestActionState", actionState()]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ cwd: "/w", @@ -2018,15 +2301,18 @@ layer("GitHubPullRequestCli.layer", (it) => { mergeMethod: "rebase", removeAgentCreditsOnMerge: true, }); - expect(mockedExecute).toHaveBeenCalledTimes(1); - expect(callAt(0).args).toContain("--rebase"); + expect(variablesOf("query PullRequestMergeMessage")).toEqual([]); + expect(variablesOf("mergePullRequest(")).toEqual([ + { input: { pullRequestId: "PR_7", mergeMethod: "REBASE" } }, + ]); }), ); it.effect("refuses to merge when the proposed message cannot be read", () => Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed(output('{"data":{"repository":{"pullRequest":null}}}')), + route( + ["query PullRequestMergeMessage", { data: { repository: { pullRequest: null } } }], + ["query PullRequestActionState", actionState()], ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const result = yield* Effect.result( @@ -2040,13 +2326,13 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); expect(result._tag).toBe("Failure"); - expect(mockedExecute).toHaveBeenCalledTimes(1); + expect(variablesOf("mergePullRequest(")).toEqual([]); }), ); it.effect("arms auto-merge with the same strategy a merge would have used", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route(["query PullRequestActionState", actionState()]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -2057,16 +2343,6 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "enable-auto-merge", mergeMethod: "squash", }); - expect(callAt(0).args).toEqual([ - "pr", - "merge", - "7", - "--repo", - "github.com/acme/web", - "--auto", - "--squash", - ]); - // No strategy asked for is GitHub's own default, exactly as it is for a merge now. yield* cli.runPullRequestAction({ cwd: "/w", @@ -2075,21 +2351,39 @@ layer("GitHubPullRequestCli.layer", (it) => { number: 7, action: "enable-auto-merge", }); - expect(callAt(1).args).toEqual([ - "pr", - "merge", - "7", - "--repo", - "github.com/acme/web", - "--auto", - "--merge", + + expect(variablesOf("enablePullRequestAutoMerge(")).toEqual([ + { input: { pullRequestId: "PR_7", mergeMethod: "SQUASH" } }, + { input: { pullRequestId: "PR_7", mergeMethod: "MERGE" } }, ]); + expect(variablesOf("mergePullRequest(")).toEqual([]); + }), + ); + + it.effect("merges at once when auto-merge is asked of a pull request that is ready now", () => + Effect.gen(function* () { + route(["query PullRequestActionState", actionState({ mergeStateStatus: "CLEAN" })]); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "enable-auto-merge", + mergeMethod: "squash", + }); + + expect(variablesOf("mergePullRequest(")).toEqual([ + { input: { pullRequestId: "PR_7", mergeMethod: "SQUASH" } }, + ]); + expect(variablesOf("enablePullRequestAutoMerge(")).toEqual([]); }), ); it.effect("takes auto-merge back off without naming a strategy", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route([NODE_ID_QUERY, nodeIdAnswer("PR_7")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -2101,30 +2395,13 @@ layer("GitHubPullRequestCli.layer", (it) => { mergeMethod: "squash", }); - expect(callAt(0).args).toEqual([ - "pr", - "merge", - "7", - "--repo", - "github.com/acme/web", - "--disable-auto", - ]); + expect(variablesOf("disablePullRequestAutoMerge(")).toEqual([{ pullRequestId: "PR_7" }]); }), ); it.effect("opens a pull request that reverts a merged pull request", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh GraphQL response. - JSON.stringify({ - data: { repository: { pullRequest: { id: "PR_7" } } }, - }), - ), - ), - ); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("{}"))); + route([NODE_ID_QUERY, nodeIdAnswer("PR_7")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -2135,19 +2412,7 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "revert", }); - expect(callAt(0).args).toContain("owner=acme"); - expect(callAt(0).args).toContain("name=web"); - expect(callAt(0).args).toContain("number=7"); - expect(callAt(1).args).toEqual([ - "api", - "graphql", - "--hostname", - "github.com", - "--input", - "-", - ]); - expect(callAt(1).stdin).toContain("revertPullRequest"); - expect(callAt(1).stdin).toContain('"pullRequestId":"PR_7"'); + expect(variablesOf("revertPullRequest(")).toEqual([{ pullRequestId: "PR_7" }]); }), ); @@ -2189,55 +2454,7 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("finds and approves every workflow waiting on a maintainer", () => Effect.gen(function* () { - const detail = output( - encodeJson( - coreResponse({ - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }), - ), - ); - const heads = output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify([ - { - number: 7, - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - }, - ]), - ); - const runs = output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify([ - { databaseId: 10, workflowName: "build", url: "https://example.com/10" }, - { databaseId: 11, workflowName: "test", url: "https://example.com/11" }, - ]), - ); - for (const result of [ - detail, - heads, - runs, - detail, - heads, - runs, - output(""), - detail, - heads, - runs, - output(""), - ]) { - mockedExecute.mockReturnValueOnce(Effect.succeed(result)); - } + workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7]), workflowRuns([10, 11])); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -2248,98 +2465,70 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "approve-workflows", }); - expect(callAt(1).args).toEqual([ - "pr", - "list", - "--repo", - "github.com/acme/web", - "--state", - "open", - "--head", - "feat/page", - "--limit", - "1001", - "--json", - "number,headRefOid,isCrossRepository,headRepositoryOwner", + // Open pull requests on this head branch, and runs scoped to this exact head. + expect(variablesOf("headRefName: $head")[0]).toEqual({ + owner: "acme", + name: "web", + head: "feat/page", + after: null, + }); + expect(restCallsTo("actions/runs?")[0]?.path).toBe( + "repos/acme/web/actions/runs?head_sha=abc123&branch=feat%2Fpage&event=pull_request&status=action_required&per_page=100&page=1", + ); + // Each run is approved only after the head is read again and still lists it. + expect(restCallsTo("/approve").map((call) => [call.method, call.path])).toEqual([ + ["POST", "repos/acme/web/actions/runs/10/approve"], + ["POST", "repos/acme/web/actions/runs/11/approve"], ]); - expect(callAt(2).args).toEqual([ - "run", - "list", - "--repo", - "github.com/acme/web", - "--commit", - "abc123", - "--branch", - "feat/page", - "--event", - "pull_request", - "--status", - "action_required", - "--limit", - "1001", - "--json", - "databaseId,workflowName,url", - ]); - expect([callAt(6).args, callAt(10).args]).toEqual([ - [ - "api", - "--method", - "POST", - "--hostname", - "github.com", - "repos/acme/web/actions/runs/10/approve", - "--silent", - ], - [ - "api", - "--method", - "POST", - "--hostname", - "github.com", - "repos/acme/web/actions/runs/11/approve", - "--silent", - ], - ]); - expect(mockedExecute).toHaveBeenCalledTimes(11); + assert.strictEqual(restCallsTo("actions/runs?").length, 3); + }), + ); + + it.effect("counts revalidated runs that wait on a maintainer as GitHub reports them", () => + Effect.gen(function* () { + workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7]), workflowRuns([])); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const runs = yield* cli + .listWorkflowRunsRequiringApproval({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: "octocat", + isCrossRepository: true, + }) + .pipe( + Effect.provideService(KnownWorkflowRuns, { + headSha: "abc123", + runs: [ + // Live shape: waiting on approval is completed + action_required. + { + id: 10, + status: "completed", + conclusion: "action_required", + head_branch: "feat/page", + }, + { id: 11, status: "completed", conclusion: "success", head_branch: "feat/page" }, + ], + }), + ); + + expect(runs.map((run) => run.id)).toEqual([10]); + assert.strictEqual(restCallsTo("actions/runs?").length, 0); }), ); it.effect("refuses a stale workflow approval after the pull request head changes", () => Effect.gen(function* () { - const detail = { - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }; - for (const value of [ - coreResponse(detail), - [ - { - number: 7, - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - }, - ], - [{ databaseId: 10, workflowName: "build", url: "https://example.com/10" }], - coreResponse({ ...detail, headRefOid: "def456" }), - ]) { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify(value), - ), - ), - ); - } + let detailReads = 0; + workflowApprovalRoutes( + () => crossRepositoryDetail(++detailReads === 1 ? "abc123" : "def456"), + heads([7]), + workflowRuns([10]), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const error = yield* Effect.flip( @@ -2356,7 +2545,8 @@ layer("GitHubPullRequestCli.layer", (it) => { _tag: "GitHubWorkflowApprovalHeadChangedError", number: 7, }); - expect(mockedExecute).toHaveBeenCalledTimes(4); + assert.strictEqual(detailReads, 2); + expect(restCallsTo("/approve")).toEqual([]); }), ); @@ -2364,21 +2554,15 @@ layer("GitHubPullRequestCli.layer", (it) => { Effect.gen(function* () { const headsStarted = yield* Deferred.make(); const runsStarted = yield* Deferred.make(); - mockedExecute.mockImplementation(({ args }) => - args[0] === "pr" + mockedExecute.mockImplementation((call) => + call.kind === "graphql" ? Deferred.succeed(headsStarted, undefined).pipe( Effect.andThen(Deferred.await(runsStarted)), - Effect.as( - output( - '[{"number":7,"headRefOid":"abc123","isCrossRepository":true,"headRepositoryOwner":{"login":"octocat"}}]', - ), - ), + Effect.as(output(encodeJson(heads([7])))), ) : Deferred.succeed(runsStarted, undefined).pipe( Effect.andThen(Deferred.await(headsStarted)), - Effect.as( - output('[{"databaseId":10,"workflowName":"build","url":"https://example.com/10"}]'), - ), + Effect.as(output(encodeJson(workflowRuns([10])))), ), ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -2400,22 +2584,7 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("refuses workflow approval when one head belongs to several pull requests", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify( - [7, 8].map((number) => ({ - number, - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - })), - ), - ), - ), - ); + workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7, 8]), workflowRuns([])); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const error = yield* Effect.flip( @@ -2438,8 +2607,7 @@ layer("GitHubPullRequestCli.layer", (it) => { observedCount: 2, limit: 1_000, }); - expect(error.detail).toContain("instead of uniquely matching #7"); - expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(error.message).toContain("instead of uniquely matching #7"); }), ); @@ -2487,29 +2655,18 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("surfaces a workflow run list beyond the safe approval bound", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( + let runPages = 0; + mockedExecute.mockImplementation((call) => { + if (call.kind === "graphql") return Effect.succeed(output(encodeJson(heads([7])))); + const page = runPages++; + return Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify([ - { - number: 7, - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - }, - ]), + encodeJson( + workflowRuns(Array.from({ length: 100 }, (_, index) => page * 100 + index + 1)), + ), ), - ), - ); - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify(Array.from({ length: 1_001 }, (_, id) => ({ databaseId: id + 1 }))), - ), - ), - ); + ); + }); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const error = yield* Effect.flip( @@ -2525,21 +2682,22 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + // Paged a hundred at a time, it stops at the first page past the bound. expect(error).toMatchObject({ _tag: "GitHubWorkflowApprovalRefusedError", reason: "run-list-truncated", number: 7, - observedCount: 1_001, + observedCount: 1_100, limit: 1_000, }); - expect(error.detail).toContain("more than 1000 workflow runs"); - expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(error.message).toContain("more than 1000 workflow runs"); + assert.strictEqual(runPages, 11); }), ); - it.effect("returns a pull request to draft by undoing ready", () => + it.effect("returns a pull request to draft by converting it", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route([NODE_ID_QUERY, nodeIdAnswer("PR_7")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.runPullRequestAction({ @@ -2550,21 +2708,14 @@ layer("GitHubPullRequestCli.layer", (it) => { action: "draft", }); - // gh has no `draft` command; going back is `ready --undo`. - expect(callAt(0).args).toEqual([ - "pr", - "ready", - "7", - "--repo", - "github.com/acme/web", - "--undo", - ]); + expect(variablesOf("convertPullRequestToDraft(")).toEqual([{ pullRequestId: "PR_7" }]); + expect(variablesOf("markPullRequestReadyForReview(")).toEqual([]); }), ); - it.effect("sends a comment body over stdin, never in argv", () => + it.effect("sends a comment body as a variable, never inside the document", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output(""))); + route([NODE_ID_QUERY, nodeIdAnswer("PR_7")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.commentOnPullRequest({ @@ -2575,24 +2726,20 @@ layer("GitHubPullRequestCli.layer", (it) => { body: "Looks good.", }); - // argv shows up in process listings and in process-runner failure messages. - expect(callAt(0).args).toEqual([ - "pr", - "comment", - "7", - "--repo", - "github.com/acme/web", - "--body-file", - "-", - ]); - expect(callAt(0).stdin).toBe("Looks good."); - expect(callAt(0).args).not.toContain("Looks good."); + expect(variablesOf("addComment(")).toEqual([{ subjectId: "PR_7", body: "Looks good." }]); + expect(queryAt(mockedExecute.mock.calls.length - 1)).not.toContain("Looks good."); }), ); it.effect("names the host on every repository it addresses", () => Effect.gen(function* () { - mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); + mockedExecute.mockImplementation((call) => + Effect.succeed( + call.kind === "graphql" && call.query.includes("pullRequests(") + ? emptyList() + : emptySearch(), + ), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.listPullRequests({ @@ -2605,8 +2752,12 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 10, }); - // A bare `owner/repo` resolves against github.com, which is a different repository. - expect(callAt(0).args).toContain("github.acme.dev/acme/web"); + // A request sent to github.com would read a different repository of the same name. + assert.isAbove(mockedExecute.mock.calls.length, 0); + expect(new Set(mockedExecute.mock.calls.map(([call]) => call.host))).toEqual( + new Set(["github.acme.dev"]), + ); + expect(searchOfCall(0)).toContain("repo:acme/web"); }), ); @@ -2615,8 +2766,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { reviewThreads: { totalCount: 0, nodes: [] } } }, }, @@ -2633,11 +2783,8 @@ layer("GitHubPullRequestCli.layer", (it) => { number: 7, }); - const args = callAt(0).args; - expect(args).toContain("--hostname"); - expect(args).toContain("github.acme.dev"); - expect(args).toContain("owner=acme"); - expect(args).toContain("name=web"); + expect(callAt(0).host).toBe("github.acme.dev"); + expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 }); }), ); @@ -2653,13 +2800,18 @@ layer("GitHubPullRequestCli.layer", (it) => { number: 7, }); + assert.strictEqual(diff.patch, "diff --git a/a b/a"); assert.isNull(diff.nextCursor); assert.isFalse(diff.truncated); // The common case pays for one request and not the files API on top of it. assert.strictEqual(mockedExecute.mock.calls.length, 1); - // `--patch` asks gh for a format-patch stream, which repeats a file once per commit. - // The review needs GitHub's combined pull-request diff: one section per changed file. - expect(callAt(0).args).not.toContain("--patch"); + // GitHub's combined pull-request diff: one section per changed file, not one per commit. + const call = callAt(0); + assert.strictEqual(call.kind, "rest"); + if (call.kind === "rest") { + assert.strictEqual(call.path, "repos/acme/web/pulls/7"); + assert.strictEqual(call.accept, "application/vnd.github.diff"); + } }), ); @@ -2682,10 +2834,8 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.isNull(diff.nextCursor); expect(diff.patch).toContain("diff --git a/src/file1.ts b/src/file1.ts"); expect(diff.patch).toContain("diff --git a/src/file2.ts b/src/file2.ts"); - const args = callAt(1).args; - expect(args).toContain("--hostname"); - expect(args).toContain("github.acme.dev"); - expect(args).toContain("repos/acme/web/pulls/7/files?per_page=100&page=1"); + assert.strictEqual(callAt(1).host, "github.acme.dev"); + assert.strictEqual(pathAt(1), "repos/acme/web/pulls/7/files?per_page=100&page=1"); }), ); @@ -2709,7 +2859,7 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("carries on from a cursor without asking `gh pr diff` again", () => + it.effect("carries on from a cursor without asking for the whole diff again", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.fail(diffRefused)); mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestFiles(100, 0)))); @@ -2725,7 +2875,7 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(second.patch).toContain("diff --git a/src/file100.ts b/src/file100.ts"); // The second slice is one request: the cursor already says where to read. assert.strictEqual(mockedExecute.mock.calls.length, 3); - expect(callAt(2).args).toContain("repos/acme/web/pulls/7/files?per_page=100&page=2"); + assert.strictEqual(pathAt(2), "repos/acme/web/pulls/7/files?per_page=100&page=2"); }), ); @@ -2748,9 +2898,11 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("reads a named commit from the commit endpoint rather than from `gh pr diff`", () => + it.effect("reads a named commit from the commit endpoint rather than the whole diff", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestFiles(2, 1)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(`{"files":${pullRequestFiles(2, 1)}}`)), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const diff = yield* cli.getPullRequestDiff({ @@ -2761,22 +2913,23 @@ layer("GitHubPullRequestCli.layer", (it) => { commit: "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0", }); - // One request: the commit's own changes never take the `gh pr diff` road. + // One request: the commit's own changes never take the whole-diff road. assert.strictEqual(mockedExecute.mock.calls.length, 1); assert.isNull(diff.nextCursor); + // The commit endpoint wraps its files in an object, which the decoder unwraps. expect(diff.patch).toContain("diff --git a/src/file1.ts b/src/file1.ts"); - const args = callAt(0).args; - expect(args).toContain( + assert.strictEqual( + pathAt(0), "repos/acme/web/commits/a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0?per_page=100&page=1", ); - // The commit endpoint wraps its files in an object, which jq unwraps for the decoder. - expect(args).toContain(".files // []"); }), ); it.effect("pages inside a commit the way it pages the pull request's own files", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestFiles(100, 0)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(`{"files":${pullRequestFiles(100, 0)}}`)), + ); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const target = { cwd: "/w", @@ -2788,11 +2941,14 @@ layer("GitHubPullRequestCli.layer", (it) => { const first = yield* cli.getPullRequestDiff(target); assert.isNotNull(first.nextCursor); - mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestFiles(4, 100)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(`{"files":${pullRequestFiles(4, 100)}}`)), + ); const second = yield* cli.getPullRequestDiff({ ...target, cursor: first.nextCursor }); assert.isNull(second.nextCursor); - expect(callAt(1).args).toContain("repos/acme/web/commits/a1b2c3d?per_page=100&page=2"); + expect(second.patch).toContain("src/file100.ts"); + assert.strictEqual(pathAt(1), "repos/acme/web/commits/a1b2c3d?per_page=100&page=2"); }), ); @@ -2817,7 +2973,9 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("expands a new file from a root commit without requiring a parent", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("\ta1b2c3d\n"))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(encodeJson({ sha: "a1b2c3d", parents: [] }))), + ); mockedExecute.mockReturnValueOnce(Effect.succeed(output("root contents\n"))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -2834,7 +2992,8 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(contents).toEqual({ oldContents: "", newContents: "root contents\n" }); assert.strictEqual(mockedExecute.mock.calls.length, 2); - expect(callAt(1).args.join(" ")).toContain("contents/src/root.ts?ref=a1b2c3d"); + assert.strictEqual(pathAt(0), "repos/acme/web/commits/a1b2c3d"); + assert.strictEqual(pathAt(1), "repos/acme/web/contents/src/root.ts?ref=a1b2c3d"); }), ); @@ -2866,7 +3025,7 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("reports an oversized diff file with its path and reason", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestRefs))); mockedExecute.mockReturnValueOnce(Effect.succeed(output("partial", true))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -2887,15 +3046,15 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.strictEqual(error.path, "src/large.ts"); assert.strictEqual(error.reason, "oversized"); } + // A deleted file is read at the base revision only. + assert.strictEqual(pathAt(1), "repos/acme/web/contents/src/large.ts?ref=a1b2c3d"); }), ); it.effect("reports undecodable diff file contents as binary", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); - mockedExecute.mockReturnValueOnce( - Effect.succeed(output("binary\uFFFDcontents", false, true)), - ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestRefs))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("binary�contents", false, true))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const error = yield* Effect.flip( @@ -2920,8 +3079,8 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("returns valid text containing a literal replacement character", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("before\uFFFDafter"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequestRefs))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("before�after"))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const contents = yield* cli.getPullRequestDiffFileContents({ @@ -2934,7 +3093,7 @@ layer("GitHubPullRequestCli.layer", (it) => { newPath: "docs/encoding.md", }); - assert.strictEqual(contents.oldContents, "before\uFFFDafter"); + assert.strictEqual(contents.oldContents, "before�after"); }), ); @@ -2996,8 +3155,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { nodes: [{ login: "octocat", avatarUrl: "https://avatars/octocat" }], rateLimit: { @@ -3020,8 +3178,8 @@ layer("GitHubPullRequestCli.layer", (it) => { ids: ["MDQ6VXNlcjE="], }); - expect(callAt(0).args).toContain("ids[]=MDQ6VXNlcjE="); - expect(callAt(0).args.at(-1)).toContain("rateLimit { cost limit remaining resetAt }"); + expect(varsAt(0)).toEqual({ ids: ["MDQ6VXNlcjE="] }); + expect(queryAt(0)).toContain("rateLimit { cost limit remaining resetAt }"); expect(avatars.get("octocat")).toBe("https://avatars/octocat"); }), ); @@ -3039,16 +3197,16 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("looks up the authenticated account on the requested enterprise host", () => Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce(Effect.succeed(output("enterprise-test-credential"))) - .mockReturnValueOnce(Effect.succeed(output('{"id":456,"login":"enterprise-user"}'))); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output('{"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(mockedCredential).toHaveBeenCalledWith("github.acme.com"); + expect(callAt(0)).toMatchObject({ kind: "rest", host: "github.acme.com", path: "user" }); }), ); @@ -3056,47 +3214,42 @@ layer("GitHubPullRequestCli.layer", (it) => { Effect.gen(function* () { const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; 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"}'))); + mockedCredential.mockImplementation((host) => + Effect.succeed({ token: Redacted.make("test-credential-a"), fingerprint: `${host}:a` }), + ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output('{"id":123,"login":"maria-rcks"}'))); expect(yield* cli.getRoutingIdentity(input)).toEqual({ accountId: "123", viewer: "maria-rcks", }); - expect(callAt(1).env).toMatchObject({ - GH_ENTERPRISE_TOKEN: "test-credential-a", - GH_DEBUG: "", - }); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("test-credential-a"))); + // The same credential again is answered from what was verified, without asking GitHub. expect(yield* cli.getRoutingIdentity(input)).toEqual({ accountId: "123", viewer: "maria-rcks", }); - expect(mockedExecute).toHaveBeenCalledTimes(3); + expect(mockedExecute).toHaveBeenCalledTimes(1); - mockedExecute - .mockReturnValueOnce(Effect.succeed(output("test-credential-b"))) - .mockReturnValueOnce( - Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/w", - cause: new Error("upstream failed with test-credential-b"), - }), - ), - ); + // A switched credential is not trusted on the strength of the old one's answer. + mockedCredential.mockImplementation((host) => + Effect.succeed({ token: Redacted.make("test-credential-b"), fingerprint: `${host}:b` }), + ); + mockedExecute.mockReturnValueOnce( + Effect.fail( + new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "getRoutingIdentity", + status: 502, + }), + ), + ); + // GitHub's own refusal is reported as itself, so the page can say what went wrong. const failure = yield* cli.getRoutingIdentity(input).pipe(Effect.flip); - expect(failure._tag).toBe("GitHubViewerLoginUnavailableError"); + expect(failure._tag).toBe("GitHubApiResponseError"); expect(String(failure)).not.toContain("test-credential-b"); - expect(callAt(4).env).toMatchObject({ - GH_ENTERPRISE_TOKEN: "test-credential-b", - GH_DEBUG: "", - }); + expect(mockedExecute).toHaveBeenCalledTimes(2); - mockedExecute - .mockReturnValueOnce(Effect.succeed(output("test-credential-b"))) - .mockReturnValueOnce(Effect.succeed(output('{"id":456,"login":"maria-rcks"}'))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output('{"id":456,"login":"maria-rcks"}'))); expect(yield* cli.getRoutingIdentity(input)).toEqual({ accountId: "456", viewer: "maria-rcks", @@ -3104,7 +3257,7 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("sends a whole review as one request body over stdin", () => + it.effect("sends a whole review as one request", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("{}"))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -3119,28 +3272,23 @@ layer("GitHubPullRequestCli.layer", (it) => { comments: [{ path: "src/a.ts", position: { kind: "added", newLine: 4 }, body: "nit" }], }); - expect(callAt(0).args).toEqual([ - "api", - "--method", - "POST", - "--hostname", - "github.com", - "repos/acme/web/pulls/7/reviews", - "--input", - "-", - ]); // One request, so nothing is on the pull request until the verdict is. assert.strictEqual(mockedExecute.mock.calls.length, 1); - // @effect-diagnostics-next-line preferSchemaOverJson:off - expect(JSON.parse(callAt(0).stdin ?? "")).toEqual({ - event: "APPROVE", - body: "Looks right.", - comments: [{ path: "src/a.ts", line: 4, side: "RIGHT", body: "nit" }], - }); + expect(restCallsTo("/reviews")).toMatchObject([ + { + method: "POST", + path: "repos/acme/web/pulls/7/reviews", + body: { + event: "APPROVE", + body: "Looks right.", + comments: [{ path: "src/a.ts", line: 4, side: "RIGHT", body: "nit" }], + }, + }, + ]); }), ); - it.effect("sends a reply body over stdin, never in argv", () => + it.effect("sends a reply body as a variable, never inside the document", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("{}"))); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; @@ -3153,23 +3301,10 @@ layer("GitHubPullRequestCli.layer", (it) => { body: "Fixed in 42ff8ec.", }); - // A reply is the reader's own words, so it travels the same way a comment body does. - expect(callAt(0).args).toEqual([ - "api", - "graphql", - "--hostname", - "github.com", - "--input", - "-", + expect(variablesOf("addPullRequestReviewThreadReply(")).toEqual([ + { threadId: "PRRT_1", body: "Fixed in 42ff8ec." }, ]); - // @effect-diagnostics-next-line preferSchemaOverJson:off - const request = JSON.parse(callAt(0).stdin ?? "") as { - query: string; - variables: Record; - }; - expect(request.query).toContain("addPullRequestReviewThreadReply"); - expect(request.variables).toEqual({ threadId: "PRRT_1", body: "Fixed in 42ff8ec." }); - expect(callAt(0).args.join(" ")).not.toContain("Fixed in 42ff8ec."); + expect(queryAt(0)).not.toContain("Fixed in 42ff8ec."); }), ); @@ -3193,30 +3328,18 @@ layer("GitHubPullRequestCli.layer", (it) => { resolved: false, }); - const parse = (index: number) => JSON.parse(callAt(index).stdin ?? "") as { query: string }; - expect(parse(0).query).toContain("resolveReviewThread("); - expect(parse(1).query).toContain("unresolveReviewThread("); + expect(queryAt(0)).toContain("resolveReviewThread("); + expect(queryAt(0)).not.toContain("unresolveReviewThread("); + expect(queryAt(1)).toContain("unresolveReviewThread("); + expect([varsAt(0), varsAt(1)]).toEqual([{ threadId: "PRRT_1" }, { threadId: "PRRT_1" }]); // A GitHub Enterprise thread is resolved on its own host, not on github.com. - expect(callAt(0).args).toContain("github.acme.dev"); + expect([callAt(0).host, callAt(1).host]).toEqual(["github.acme.dev", "github.acme.dev"]); }), ); it.effect("confirms a given subject belongs to the named pull request, then reacts to it", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - data: { - repository: { pullRequest: { id: "PR_kwDOA" } }, - node: { id: "IC_1", pullRequest: { id: "PR_kwDOA" } }, - }, - }), - ), - ), - ); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("{}"))); + route([SUBJECT_SCOPE_QUERY, subjectScope("IC_1", "PR_kwDOA")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.setReaction({ @@ -3230,18 +3353,10 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 2); - const scopeCheck = callAt(0).args; - expect(scopeCheck).toContain("owner=acme"); - expect(scopeCheck).toContain("name=web"); - expect(scopeCheck).toContain("number=7"); - expect(scopeCheck).toContain("subjectId=IC_1"); - // @effect-diagnostics-next-line preferSchemaOverJson:off - const request = JSON.parse(callAt(1).stdin ?? "") as { - query: string; - variables: Record; - }; - expect(request.query).toContain("addReaction("); - expect(request.variables).toEqual({ subjectId: "IC_1", content: "HEART" }); + expect(variablesOf(SUBJECT_SCOPE_QUERY)).toEqual([ + { owner: "acme", name: "web", number: 7, subjectId: "IC_1" }, + ]); + expect(variablesOf("addReaction(")).toEqual([{ subjectId: "IC_1", content: "HEART" }]); }), ); @@ -3250,8 +3365,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { id: "PR_thisOne" } }, // A comment on pull request #99 of a different repository, named as though it @@ -3284,15 +3398,7 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("looks up the pull request's own node id when no subject was given", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ data: { repository: { pullRequest: { id: "PR_kwDOA" } } } }), - ), - ), - ); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("{}"))); + route([NODE_ID_QUERY, nodeIdAnswer("PR_kwDOA")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; // Its own pull request: a node id looked up once is remembered for the life of the service. @@ -3306,36 +3412,14 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 2); - const lookup = callAt(0).args; - expect(lookup).toContain("owner=acme"); - expect(lookup).toContain("name=web"); - expect(lookup).toContain("number=21"); - // @effect-diagnostics-next-line preferSchemaOverJson:off - const request = JSON.parse(callAt(1).stdin ?? "") as { - query: string; - variables: Record; - }; - expect(request.query).toContain("addReaction("); - expect(request.variables).toEqual({ subjectId: "PR_kwDOA", content: "ROCKET" }); + expect(variablesOf(NODE_ID_QUERY)).toEqual([{ owner: "acme", name: "web", number: 21 }]); + expect(variablesOf("addReaction(")).toEqual([{ subjectId: "PR_kwDOA", content: "ROCKET" }]); }), ); it.effect("takes a reaction back through the remove mutation", () => Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - data: { - repository: { pullRequest: { id: "PR_kwDOA" } }, - node: { id: "IC_1", pullRequest: { id: "PR_kwDOA" } }, - }, - }), - ), - ), - ); - mockedExecute.mockReturnValueOnce(Effect.succeed(output("{}"))); + route([SUBJECT_SCOPE_QUERY, subjectScope("IC_1", "PR_kwDOA")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; yield* cli.setReaction({ @@ -3348,22 +3432,14 @@ layer("GitHubPullRequestCli.layer", (it) => { reacted: false, }); - // @effect-diagnostics-next-line preferSchemaOverJson:off - const request = JSON.parse(callAt(1).stdin ?? "") as { query: string }; - expect(request.query).toContain("removeReaction("); + expect(variablesOf("removeReaction(")).toEqual([{ subjectId: "IC_1", content: "HEART" }]); + expect(variablesOf("addReaction(")).toEqual([]); }), ); it.effect("rewrites only the words a request named", () => Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ data: { repository: { pullRequest: { id: "PR_kwDOA" } } } }), - ), - ), - ); + route([NODE_ID_QUERY, nodeIdAnswer("PR_kwDOA")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const rewrite = (fields: { readonly title?: string; readonly body?: string }) => cli.updatePullRequest({ @@ -3379,38 +3455,18 @@ layer("GitHubPullRequestCli.layer", (it) => { yield* rewrite({ title: "Both", body: "at once." }); // One node id lookup for the pull request, then a mutation per rewrite. - const variablesAt = (index: number) => - (JSON.parse(callAt(index).stdin ?? "") as { variables: Record }).variables; - expect(variablesAt(1)).toEqual({ pullRequestId: "PR_kwDOA", title: "A better title" }); - expect(variablesAt(2)).toEqual({ - pullRequestId: "PR_kwDOA", - body: "A better description.", - }); - expect(variablesAt(3)).toEqual({ - pullRequestId: "PR_kwDOA", - title: "Both", - body: "at once.", - }); - // The reader's own words, so they travel the way every other body does. - expect(callAt(3).args.join(" ")).not.toContain("at once."); + assert.strictEqual(variablesOf(NODE_ID_QUERY).length, 1); + expect(variablesOf("updatePullRequest(")).toEqual([ + { pullRequestId: "PR_kwDOA", title: "A better title" }, + { pullRequestId: "PR_kwDOA", body: "A better description." }, + { pullRequestId: "PR_kwDOA", title: "Both", body: "at once." }, + ]); }), ); it.effect("rewrites a remark through the mutation its kind needs", () => Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - data: { - repository: { pullRequest: { id: "PR_kwDOA" } }, - node: { id: "IC_1", pullRequest: { id: "PR_kwDOA" } }, - }, - }), - ), - ), - ); + route([SUBJECT_SCOPE_QUERY, subjectScope("IC_1", "PR_kwDOA")]); const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; const rewrite = (kind: "issue-comment" | "review-comment") => cli.updateComment({ @@ -3426,16 +3482,16 @@ layer("GitHubPullRequestCli.layer", (it) => { yield* rewrite("issue-comment"); yield* rewrite("review-comment"); - const parse = (index: number) => - JSON.parse(callAt(index).stdin ?? "") as { - query: string; - variables: Record; - }; - expect(callAt(0).args).toContain("subjectId=IC_1"); - expect(parse(1).query).toContain("updateIssueComment("); - expect(parse(1).variables).toEqual({ commentId: "IC_1", body: "Reworded." }); - expect(parse(3).query).toContain("updatePullRequestReviewComment("); - expect(parse(3).variables).toEqual({ commentId: "IC_1", body: "Reworded." }); + expect(variablesOf(SUBJECT_SCOPE_QUERY).map((variables) => variables["subjectId"])).toEqual([ + "IC_1", + "IC_1", + ]); + expect(variablesOf("updateIssueComment(")).toEqual([ + { commentId: "IC_1", body: "Reworded." }, + ]); + expect(variablesOf("updatePullRequestReviewComment(")).toEqual([ + { commentId: "IC_1", body: "Reworded." }, + ]); }), ); @@ -3444,8 +3500,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { id: "PR_thisOne" } }, node: { id: "IC_99", pullRequest: { id: "PR_someOtherOne" } }, @@ -3469,7 +3524,8 @@ layer("GitHubPullRequestCli.layer", (it) => { ); assert.strictEqual(error._tag, "GitHubSubjectScopeError"); - expect(error.message).toContain("updateComment"); + if (error._tag === "GitHubSubjectScopeError") + assert.strictEqual(error.operation, "updateComment"); // Refused before any mutation was sent. assert.strictEqual(mockedExecute.mock.calls.length, 1); }), @@ -3500,14 +3556,8 @@ layer("GitHubPullRequestCli.layer", (it) => { output( encodeJson( coreResponse({ - number: 7, title: "Progressive detail", - url: "https://github.com/acme/web/pull/7", author: { login: "octocat" }, - headRefName: "feature", - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", body: "Core body", changedFiles: 2, }), @@ -3518,12 +3568,17 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - author: { login: "octocat" }, - comments: [], - reviews: [], - commits: [], + encodeJson({ + data: { + repository: { + pullRequest: { + author: { __typename: "User", login: "octocat", avatarUrl: "https://a/o" }, + commits: { nodes: [] }, + comments: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] }, + reviews: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] }, + }, + }, + }, }), ), ), @@ -3541,15 +3596,19 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(detail.body).toBe("Core body"); expect(activity.author?.login).toBe("octocat"); - expect(callAt(0).args).toContain("headRef=refs/pull/7/head"); - expect(callAt(0).args.at(-1)).toContain("viewerCanUpdateBranch"); + expect(varsAt(0)).toMatchObject({ number: 7, headRef: "refs/pull/7/head" }); + expect(queryAt(0)).toContain("viewerCanUpdateBranch"); + expect(queryAt(0)).not.toContain("reviews("); expect(detail.viewerAccess.mergeCapabilities).toEqual({ merge: true, squash: false, rebase: true, }); expect(detail.comparison).toEqual({ behindBy: 2, viewerCanUpdate: true }); - expect(callAt(1).args.at(-1)).toBe("author,comments,reviews,commits"); + // Conversation activity is its own read, and asks for the head of the conversation once. + expect(queryAt(1)).toContain("reviews("); + expect(varsAt(1)).toMatchObject({ head: true, withComments: true, withReviews: true }); + assert.strictEqual(mockedExecute.mock.calls.length, 2); }), ); @@ -3621,35 +3680,40 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("reads every check when the combined response has another page", () => Effect.gen(function* () { - const response = coreResponse({ - commits: { - nodes: [ - { - commit: { - statusCheckRollup: { - contexts: { - nodes: [{ name: "first", status: "COMPLETED", conclusion: "SUCCESS" }], - pageInfo: { hasNextPage: true }, - }, + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + encodeJson( + coreResponse({ + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [{ name: "first", status: "COMPLETED", conclusion: "SUCCESS" }], + pageInfo: { hasNextPage: true, endCursor: "c1" }, + }, + }, + }, + }, + ], }, - }, - }, - ], - }, - }); - mockedExecute.mockReturnValueOnce(Effect.succeed(output(encodeJson(response)))); + }), + ), + ), + ), + ); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - encodeJson({ - ...response.data.repository.pullRequest, - reviewRequests: [], - labels: [], - statusCheckRollup: [ + checkContextsPage( + [ { name: "first", status: "COMPLETED", conclusion: "SUCCESS" }, { name: "last", status: "COMPLETED", conclusion: "FAILURE" }, ], - }), + null, + ), ), ), ); @@ -3663,7 +3727,9 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(detail.checks).toHaveLength(2); expect(detail.checksState).toBe("failing"); expect(detail.checksTruncated).toBe(false); - expect(callAt(1).args.slice(0, 2)).toEqual(["pr", "view"]); + // The whole rollup is walked from its start, so no check is counted twice or skipped. + expect(varsAt(1)).toMatchObject({ number: 7, after: null }); + assert.strictEqual(mockedExecute.mock.calls.length, 2); }), ); @@ -3744,9 +3810,7 @@ layer("GitHubPullRequestCli.layer", (it) => { const error = yield* Effect.flip(cli.getPullRequestDetail(input)); expect(error._tag).toBe("SourceControlRateLimitPausedError"); expect(mockedExecute).toHaveBeenCalledOnce(); - yield* cli - .getPullRequestDetail(input) - .pipe(Effect.provideService(GitHubCli.AllowGitHubReserve, true)); + yield* cli.getPullRequestDetail(input).pipe(Effect.provideService(AllowGitHubReserve, true)); expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); @@ -3769,13 +3833,13 @@ layer("GitHubPullRequestCli.layer", (it) => { // What matters is that it fails at all: an empty patch with no cursor would render as a // change with no files and report the rest of it as already read. The refusal that sent // the read down this road is the one reported, by design. - assert.strictEqual(error._tag, "GitHubCliCommandError"); + assert.strictEqual(error, diffRefused); }), ); it.effect("pages an oversized patch by file rather than handing back a severed one", () => Effect.gen(function* () { - // `gh pr diff` succeeded but its output was cut at a byte, which lands mid-file. + // The whole diff came back, but cut at a byte, which lands mid-file. mockedExecute.mockReturnValueOnce( Effect.succeed(output("diff --git a/a b/a\n@@ -1 +1 @@", true)), ); @@ -3790,8 +3854,9 @@ layer("GitHubPullRequestCli.layer", (it) => { }); // The severed patch is thrown away; what comes back is assembled from whole files. - expect(callAt(1).args.join(" ")).toContain("/pulls/7/files"); + assert.strictEqual(pathAt(1), "repos/acme/web/pulls/7/files?per_page=100&page=1"); expect(slice.patch).toContain("src/file1.ts"); + expect(slice.patch).not.toContain("diff --git a/a b/a"); assert.strictEqual(mockedExecute.mock.calls.length, 2); }), ); @@ -3813,9 +3878,9 @@ layer("GitHubPullRequestCli.layer", (it) => { number: 7, }); - // The first page asks from the beginning, which gh only sends as a typed JSON null. - expect(callAt(0).args).toContain("cursor=null"); - expect(callAt(1).args).toContain("cursor=Y3Vyc29yOjE"); + // The first page asks from the beginning; the next carries on from where it stopped. + expect(varsAt(0)["cursor"]).toBeNull(); + expect(varsAt(1)["cursor"]).toBe("Y3Vyc29yOjE"); expect(conversation.comments.map((comment) => comment.id)).toEqual(["c1", "c2"]); assert.isFalse(conversation.truncated); }), @@ -3897,11 +3962,13 @@ layer("GitHubPullRequestCli.layer", (it) => { cursor: "Y3Vyc29yOjI", }); - expect(callAt(0).args).toContain("owner=acme"); - expect(callAt(0).args).toContain("name=web"); - expect(callAt(0).args).toContain("number=7"); - expect(callAt(0).args).toContain("threadId=PRRT_1"); - expect(callAt(0).args).toContain("cursor=Y3Vyc29yOjI"); + expect(varsAt(0)).toMatchObject({ + owner: "acme", + name: "web", + number: 7, + threadId: "PRRT_1", + cursor: "Y3Vyc29yOjI", + }); assert.strictEqual(mockedExecute.mock.calls.length, 1); expect(page.comments.map((comment) => comment.id)).toEqual(["c2", "c3"]); expect(page.nextCursor).toBeNull(); @@ -3937,8 +4004,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValue( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { mergeCommitAllowed: true, @@ -3963,10 +4029,8 @@ layer("GitHubPullRequestCli.layer", (it) => { // One request, because both answers hang off the same repository object. assert.strictEqual(mockedExecute.mock.calls.length, 1); - expect(callAt(0).args).toContain("number=7"); - expect(callAt(0).args.at(-1)).toContain( - "mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed", - ); + expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 }); + expect(queryAt(0)).toContain("mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed"); expect(access).toEqual({ mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, @@ -3977,13 +4041,12 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("sends the base comparison's variables as gh flags, not as bare words", () => + it.effect("sends the base comparison's head as a variable, not inside the document", () => Effect.gen(function* () { mockedExecute.mockReturnValue( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { @@ -4006,26 +4069,14 @@ layer("GitHubPullRequestCli.layer", (it) => { headRef: "fork:feat/page", }); - // The tuples are flattened straight into argv, so a variable without its flag is a - // positional argument gh refuses outright. - const args = callAt(0).args; - expect(args.slice(0, -2)).toEqual([ - "api", - "graphql", - "--hostname", - "github.com", - "-f", - "owner=acme", - "-f", - "name=web", - "-F", - "number=7", - "-f", - "headRef=fork:feat/page", - ]); + expect(varsAt(0)).toEqual({ + owner: "acme", + name: "web", + number: 7, + headRef: "fork:feat/page", + }); expect(comparison).toEqual({ behindBy: 4, viewerCanUpdate: true }); - expect(args.at(-2)).toBe("-f"); - expect(args.at(-1)).toContain(`query=${BASE_COMPARISON_GRAPHQL_QUERY.slice(0, -2)}`); + expect(queryAt(0)).toContain(BASE_COMPARISON_GRAPHQL_QUERY.slice(0, -2)); }), ); @@ -4034,8 +4085,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValue( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { @@ -4064,7 +4114,7 @@ layer("GitHubPullRequestCli.layer", (it) => { } as const; yield* cli.getPullRequestBaseComparison(input); - expect(callAt(0).args.at(-1)).toContain("rateLimit { cost limit remaining resetAt }"); + expect(queryAt(0)).toContain("rateLimit { cost limit remaining resetAt }"); const error = yield* Effect.flip(cli.getPullRequestBaseComparison(input)); @@ -4083,8 +4133,7 @@ layer("GitHubPullRequestCli.layer", (it) => { .mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { @@ -4106,8 +4155,7 @@ layer("GitHubPullRequestCli.layer", (it) => { .mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { mergeCommitAllowed: true, @@ -4167,21 +4215,11 @@ layer("GitHubPullRequestCli.layer", (it) => { requested: true, }); - const call = callAt(0); - expect(call.args).toEqual([ - "api", - "--method", - "POST", - "--hostname", - "github.com", - "repos/acme/web/pulls/7/requested_reviewers", - "--input", - "-", - ]); - // @effect-diagnostics-next-line preferSchemaOverJson:off - expect(JSON.parse(call.stdin ?? "")).toEqual({ - reviewers: ["octocat"], - team_reviewers: ["reviewers"], + expect(callAt(0)).toMatchObject({ + kind: "rest", + method: "POST", + path: "repos/acme/web/pulls/7/requested_reviewers", + body: { reviewers: ["octocat"], team_reviewers: ["reviewers"] }, }); }), ); @@ -4200,13 +4238,11 @@ layer("GitHubPullRequestCli.layer", (it) => { requested: false, }); - const call = callAt(0); - expect(call.args).toContain("DELETE"); - expect(call.args).toContain("repos/acme/web/pulls/7/requested_reviewers"); - // @effect-diagnostics-next-line preferSchemaOverJson:off - expect(JSON.parse(call.stdin ?? "")).toEqual({ - reviewers: ["octocat"], - team_reviewers: [], + expect(callAt(0)).toMatchObject({ + kind: "rest", + method: "DELETE", + path: "repos/acme/web/pulls/7/requested_reviewers", + body: { reviewers: ["octocat"], team_reviewers: [] }, }); }), ); @@ -4216,8 +4252,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValue( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { assignableUsers: { @@ -4246,7 +4281,7 @@ layer("GitHubPullRequestCli.layer", (it) => { // The people, who has been asked and who opened the pull request all hang off the same // repository object, so the menu costs one request. assert.strictEqual(mockedExecute.mock.calls.length, 1); - expect(callAt(0).args).toContain("number=7"); + expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 }); expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([ ["octocat", true], ["hubot", false], @@ -4269,19 +4304,12 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 1); - const call = callAt(0); - expect(call.args).toEqual([ - "api", - "--method", - "POST", - "--hostname", - "github.com", - "repos/acme/web/issues/7/labels", - "--input", - "-", - ]); - // @effect-diagnostics-next-line preferSchemaOverJson:off - asserting the raw gh request body. - expect(JSON.parse(call.stdin ?? "")).toEqual({ labels: ["bug", "size:XL"] }); + expect(callAt(0)).toMatchObject({ + kind: "rest", + method: "POST", + path: "repos/acme/web/issues/7/labels", + body: { labels: ["bug", "size:XL"] }, + }); }), ); @@ -4300,9 +4328,14 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 2); - expect(callAt(0).args).toContain("repos/acme/web/issues/7/labels/good%20first%20issue"); - expect(callAt(0).args).toContain("DELETE"); - expect(callAt(1).args).toContain("repos/acme/web/issues/7/labels/area%2Fweb"); + expect(callAt(0)).toMatchObject({ + method: "DELETE", + path: "repos/acme/web/issues/7/labels/good%20first%20issue", + }); + expect(callAt(1)).toMatchObject({ + method: "DELETE", + path: "repos/acme/web/issues/7/labels/area%2Fweb", + }); }), ); @@ -4311,7 +4344,7 @@ layer("GitHubPullRequestCli.layer", (it) => { const page = (index: number, hasNextPage: boolean) => Effect.succeed( output( - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { @@ -4343,9 +4376,11 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.strictEqual(mockedExecute.mock.calls.length, 3); // The first page asks from the start; each one after it carries the cursor before it. - assert.isFalse(callAt(0).args.some((arg) => arg.startsWith("after="))); - expect(callAt(1).args).toContain("after=cursor-0"); - expect(callAt(2).args).toContain("after=cursor-1"); + expect([varsAt(0)["after"], varsAt(1)["after"], varsAt(2)["after"]]).toEqual([ + null, + "cursor-0", + "cursor-1", + ]); assert.isFalse(viewed.truncated); expect(viewed.files.map((file) => [file.path, file.state])).toEqual([ ["src/file0.ts", "viewed"], @@ -4363,8 +4398,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValue( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ + encodeJson({ data: { repository: { pullRequest: { @@ -4399,8 +4433,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute .mockReturnValueOnce( Effect.succeed( - // @effect-diagnostics-next-line preferSchemaOverJson:off - output(JSON.stringify({ data: { repository: { pullRequest: { id: "PR_1" } } } })), + output(encodeJson({ data: { repository: { pullRequest: { id: "PR_1" } } } })), ), ) .mockReturnValueOnce(Effect.succeed(output("{}"))); @@ -4419,14 +4452,9 @@ layer("GitHubPullRequestCli.layer", (it) => { // One request to learn the pull request's node id, one for every press together. assert.strictEqual(mockedExecute.mock.calls.length, 2); - // @effect-diagnostics-next-line preferSchemaOverJson:off - const sent = JSON.parse(callAt(1).stdin ?? "") as { - query: string; - variables: Record; - }; - expect(sent.query).toContain("f0: markFileAsViewed"); - expect(sent.query).toContain("f1: unmarkFileAsViewed"); - expect(sent.variables).toEqual({ + expect(queryAt(1)).toContain("f0: markFileAsViewed"); + expect(queryAt(1)).toContain("f1: unmarkFileAsViewed"); + expect(varsAt(1)).toEqual({ pullRequestId: "PR_1", path0: "src/a.ts", path1: "src/b.ts", @@ -4455,8 +4483,7 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute .mockReturnValueOnce( Effect.succeed( - // @effect-diagnostics-next-line preferSchemaOverJson:off - output(JSON.stringify({ data: { repository: { pullRequest: { id: "PR_24" } } } })), + output(encodeJson({ data: { repository: { pullRequest: { id: "PR_24" } } } })), ), ) .mockReturnValue(Effect.succeed(output("{}"))); @@ -4475,11 +4502,12 @@ layer("GitHubPullRequestCli.layer", (it) => { // One lookup, then a mutation per write, every one of them addressed by the id it answered. assert.strictEqual(mockedExecute.mock.calls.length, 4); - expect(callAt(0).args).toContain("number=24"); - const idSentAt = (index: number) => - (JSON.parse(callAt(index).stdin ?? "") as { variables: { pullRequestId: string } }) - .variables.pullRequestId; - expect([idSentAt(1), idSentAt(2), idSentAt(3)]).toEqual(["PR_24", "PR_24", "PR_24"]); + expect(varsAt(0)).toEqual({ owner: "acme", name: "web", number: 24 }); + expect([1, 2, 3].map((index) => varsAt(index)["pullRequestId"])).toEqual([ + "PR_24", + "PR_24", + "PR_24", + ]); }), ); @@ -4489,8 +4517,7 @@ layer("GitHubPullRequestCli.layer", (it) => { .mockReturnValueOnce(Effect.succeed(output('{"message":"not found"}'))) .mockReturnValueOnce( Effect.succeed( - // @effect-diagnostics-next-line preferSchemaOverJson:off - output(JSON.stringify({ data: { repository: { pullRequest: { id: "PR_25" } } } })), + output(encodeJson({ data: { repository: { pullRequest: { id: "PR_25" } } } })), ), ) .mockReturnValueOnce(Effect.succeed(output("{}"))); @@ -4510,10 +4537,7 @@ layer("GitHubPullRequestCli.layer", (it) => { yield* write(); assert.strictEqual(mockedExecute.mock.calls.length, 3); - const idSentAt = (index: number) => - (JSON.parse(callAt(index).stdin ?? "") as { variables: { pullRequestId: string } }) - .variables.pullRequestId; - expect(idSentAt(2)).toEqual("PR_25"); + expect(varsAt(2)["pullRequestId"]).toEqual("PR_25"); }), ); it.effect("keeps the pull request being ticked through, not the one looked up first", () => @@ -4525,9 +4549,10 @@ layer("GitHubPullRequestCli.layer", (it) => { const HOT = 9_000; const lookupsOf = new Map(); mockedExecute.mockImplementation((input) => { - const asked = input.args.find((arg) => arg.startsWith("number=")); - if (asked === undefined) return Effect.succeed(output("{}")); - const number = Number(asked.slice("number=".length)); + if (input.kind !== "graphql" || !input.query.includes(NODE_ID_QUERY)) { + return Effect.succeed(output("{}")); + } + const number = Number(input.variables?.["number"]); lookupsOf.set(number, (lookupsOf.get(number) ?? 0) + 1); return Effect.succeed( output(encodeJson({ data: { repository: { pullRequest: { id: `PR_${number}` } } } })), diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index a860ccdc04708284ff95093285c7472e2fd12a9c..d393db9fa78b8f47f440d896ab5a6fb592f7b2a8 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -1,21 +1,19 @@ import { removeAgentCredits } from "./mergeMessage.ts"; -import { makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; +import { KnownWorkflowRuns, makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; import { runGitHubStackAction, type GitHubStackActionError } from "./githubStackActions.ts"; import * as Cause from "effect/Cause"; import * as Context from "effect/Context"; import * as Clock from "effect/Clock"; -import * as Crypto from "effect/Crypto"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Exit from "effect/Exit"; import * as Option from "effect/Option"; -import * as Redacted from "effect/Redacted"; import * as Request from "effect/Request"; import * as RequestResolver from "effect/RequestResolver"; import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import * as Semaphore from "effect/Semaphore"; -import * as Hex from "effect/encoding/Hex"; import { resolvePullRequestAuthorFilter, PositiveInt, @@ -23,6 +21,8 @@ import { type PullRequestAction, type PullRequestStackHead, type PullRequestActor, + type PullRequestComment, + type PullRequestCommit, type PullRequestFileViewed, type PullRequestInvolvement, type PullRequestListFilters, @@ -41,19 +41,28 @@ import { type PullRequestPreview, } from "@t3tools/contracts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; -import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; +import { AllowGitHubReserve } from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import { ACTOR_AVATARS_GRAPHQL_QUERY, ADD_REACTION_GRAPHQL_MUTATION, - buildReviewSubmissionJson, - buildReviewerRequestJson, + buildReviewSubmission, + buildReviewerRequest, buildSetFilesViewedGraphQlMutation, decodeActorAvatarsJson, decodePullRequestActivityJson, - decodePullRequestDetailJson, + decodePullRequestCheckContextsJson, decodePullRequestCoreJson, + decodeCommitFilesJson, + pullRequestCheckContextsGraphQlQuery, + pullRequestChecksFromContexts, + pullRequestListGraphQlQuery, + pullRequestSummaryGraphQlQuery, + PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY, + PULL_REQUEST_HEADS_GRAPHQL_QUERY, + decodeWorkflowRunsJson, + type GitHubCheckContext, pullRequestCoreGraphQlQuery, type GitHubPullRequestCore, type GitHubPullRequestSummary, @@ -73,7 +82,7 @@ import { decodeReactionSubjectScopeJson, decodeReviewerCandidatesJson, decodeLabelCandidatesJson, - buildLabelRequestJson, + buildLabelRequest, LABEL_CANDIDATES_GRAPHQL_QUERY, decodeReviewDismissalsJson, decodeReviewThreadCommentsJson, @@ -83,14 +92,10 @@ import { buildPullRequestWatchFingerprintsGraphQlQuery, buildPullRequestStackMembershipsGraphQlQuery, decodePullRequestStackMembershipsJson, - encodeGraphQlRequestJson, pullRequestSearchGraphQlQuery, PULL_REQUEST_SEARCH_MAX_ROWS, - PULL_REQUEST_ACTIVITY_JSON_FIELDS, BASE_COMPARISON_GRAPHQL_QUERY, decodeBaseComparisonJson, - PULL_REQUEST_DETAIL_JSON_FIELDS, - PULL_REQUEST_LIST_JSON_FIELDS, PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, REACTION_SUBJECT_PULL_REQUEST_GRAPHQL_QUERY, @@ -110,9 +115,10 @@ import { UPDATE_REVIEW_COMMENT_GRAPHQL_MUTATION, VIEWER_PERMISSIONS_GRAPHQL_QUERY, decodeViewerPermissionsJson, - decodeWorkflowRunApprovalsJson, type GitHubBaseComparison, type GitHubPullRequestActivity, + type GitHubPullRequestActivityPage, + type GitHubWorkflowRunPage, type GitHubPullRequestHead, type GitHubPullRequestListItem, type GitHubPullRequestSearchItem, @@ -139,13 +145,9 @@ export class GitHubPullRequestReadError extends Schema.TaggedError> = { + all: ["OPEN", "CLOSED", "MERGED"], + open: ["OPEN"], + // Closed includes merged here, the way GitHub's own list reads it; merged rows are filtered out + // locally by `matchesUnsortedListing`. + closed: ["CLOSED", "MERGED"], + merged: ["MERGED"], +}; + +interface GitHubPullRequestListPage { + readonly items: ReadonlyArray; + readonly rawCount: number; + readonly endCursor: string | null; +} + /** A search-free fallback may scan older rows for local filters, but never the whole repository. */ const PULL_REQUEST_FALLBACK_MAX_ROWS = 1_000; @@ -413,6 +387,9 @@ export const NODE_ID_CACHE_CAPACITY = 128; */ const REVIEW_THREAD_PAGES = 10; +/** Pages of a hundred check contexts to walk for one head before the rollup says it was cut. */ +const CHECK_CONTEXT_PAGES = 10; + export interface GitHubPullRequestListBatch { readonly items: ReadonlyArray; readonly truncated: boolean; @@ -960,50 +937,6 @@ function matchesFilters( ); } -function involvementArgs(input: { - readonly state: PullRequestListState; - readonly involvement: PullRequestInvolvement; - readonly viewer: string; - readonly query?: string | undefined; - /** Where to carry on from, which only a search can express. */ - readonly cursor?: ProviderListCursor | undefined; - /** - * Ask GitHub for the order the page reads its rows in. False on the fallback read, which - * cannot use search at all and takes whatever order `gh pr list` answers in. - */ - readonly sorted: boolean; - readonly filters?: PullRequestListFilters | undefined; -}): ReadonlyArray { - // `--state closed` includes merged pull requests, so the Closed tab additionally excludes - // them through search; `--author` and `review-requested:` are GitHub's own filters. `gh` - // takes one `--search`, so the reader's text joins the qualifiers rather than replacing them. - const query = input.query?.trim() ?? ""; - // The fallback read exists because this repository's search index answered nothing, so it goes - // nowhere near search: no order, cursor or qualifiers. Its decoded rows are narrowed by state - // and involvement below, since widening either would put unrelated pull requests on the page. - const searchTerms = !input.sorted - ? [] - : [ - ...(input.involvement === "reviewing" ? [`review-requested:${input.viewer}`] : []), - ...(input.state === "closed" ? ["is:unmerged"] : []), - ...(query.length === 0 ? [] : [searchPhrase(query)]), - // The instant the last slice ended on, and everything before it. Inclusive, because rows - // sharing one instant are ordinary and the caller drops the ones it has already sent — - // asking for strictly older would lose the rest of them instead. - ...(input.cursor === undefined ? [] : [`updated:<=${input.cursor.updatedBefore}`]), - ...filterQualifiers(input.filters, input.viewer), - // `gh pr list` answers newest-created first, which is not the order the page reads rows in - // and not an order a continuation can carry on from: a change request opened last year and - // touched this morning belongs at the top of the list and at the front of the first slice. - // Free text would otherwise come back in best-match order, which is worse again. - "sort:updated-desc", - ]; - return [ - ...(input.involvement === "authored" ? ["--author", input.viewer] : []), - ...(searchTerms.length > 0 ? ["--search", searchTerms.join(" ")] : []), - ]; -} - /** The search-free fallback is wider than the request, so narrow its decoded rows locally. */ function matchesUnsortedListing( item: GitHubPullRequestListItem, @@ -1029,13 +962,11 @@ function matchesUnsortedListing( const SEARCH_REPOSITORY = /^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/; /** - * The same listing as one GitHub search across several repositories, which is the only way to - * read a whole host in one request. + * A listing as one GitHub search, across one repository or several: the only way to read a + * whole host in one request, and the only order (`updated`) a continuation can carry on from. * - * Every narrowing `involvementArgs` hands to `gh pr list` as a flag is a qualifier here instead, - * because a search has no flags to borrow: `--author X` is `author:X`, `--state open` is - * `is:open`, and `--state closed` — which includes merged pull requests — is `is:closed - * is:unmerged`. The two belong together; a tab added to one wants adding to the other. + * `--state closed` in GitHub's own list includes merged pull requests, so the Closed tab is + * `is:closed is:unmerged` here. * * Null where a repository is not `owner/name`. A name is written into the query as itself, and a * name holding a space could otherwise end the `repo:` qualifier and start a qualifier of its @@ -1073,15 +1004,6 @@ function searchQuery(input: { ].join(" "); } -/** - * The `after` a paged read carries. gh sends a JSON null only through a typed field, and an - * untyped `cursor=` would send the empty string, which GitHub refuses as a cursor rather than - * reading as "start at the beginning". - */ -function cursorVariable(cursor: string | null): readonly [string, string] { - return cursor === null ? ["-F", "cursor=null"] : ["-f", `cursor=${cursor}`]; -} - const MERGE_MESSAGE_GRAPHQL_QUERY = ` query PullRequestMergeMessage($owner: String!, $name: String!, $number: Int!, $method: PullRequestMergeMethod!) { repository(owner: $owner, name: $name) { @@ -1093,6 +1015,18 @@ query PullRequestMergeMessage($owner: String!, $name: String!, $number: Int!, $m } }`; +/** The two revisions a file is expanded between: a pull request's base and head, or a commit's parent and itself. */ +const decodeRevisionRefs = Schema.decodeUnknownOption( + Schema.fromJsonString( + Schema.Struct({ + sha: Schema.optional(Schema.String), + parents: Schema.optional(Schema.Array(Schema.Struct({ sha: Schema.String }))), + base: Schema.optional(Schema.Struct({ sha: Schema.String })), + head: Schema.optional(Schema.Struct({ sha: Schema.String })), + }), + ), +); + const decodeMergeMessageResponse = Schema.decodeUnknownResult( Schema.fromJsonString( Schema.Struct({ @@ -1111,45 +1045,93 @@ const decodeMergeMessageResponse = Schema.decodeUnknownResult( const decodeMergeMessage = (raw: string) => Result.map(decodeMergeMessageResponse(raw), (response) => response.data.repository.pullRequest); -function actionArgs( - action: PullRequestAction, - mergeMethod: PullRequestMergeMethod | undefined, - updateMethod: PullRequestUpdateMethod | undefined, -): ReadonlyArray { - switch (action) { - case "merge": - return ["merge", `--${mergeMethod ?? "merge"}`]; - // `--auto` arms the same command instead of running it, and still needs the strategy: GitHub - // stores the strategy with the standing instruction rather than choosing one at merge time. - case "enable-auto-merge": - return ["merge", "--auto", `--${mergeMethod ?? "merge"}`]; - case "disable-auto-merge": - return ["merge", "--disable-auto"]; - // `gh` updates with a merge commit unless asked to rebase, which is GitHub's own default. - case "update-branch": - return ["update-branch", ...(updateMethod === "rebase" ? ["--rebase"] : [])]; - case "ready": - return ["ready"]; - case "draft": - return ["ready", "--undo"]; - case "close": - return ["close"]; - case "reopen": - return ["reopen"]; - case "revert": - throw new Error("Revert requires a GraphQL mutation"); - // Handled separately because it may approve several workflow runs rather than mutate the - // pull request itself. - case "approve-workflows": - throw new Error("Workflow approval requires run discovery"); +/** What `gh pr merge` and `gh pr update-branch` read before they act. */ +const ACTION_STATE_GRAPHQL_QUERY = ` +query PullRequestActionState($owner: String!, $name: String!, $number: Int!, $headRef: String!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + id + headRefOid + isMergeQueueEnabled + mergeStateStatus + baseRef { compare(headRef: $headRef) { behindBy } } + } } -} +}`; + +const decodeActionStateResponse = Schema.decodeUnknownResult( + Schema.fromJsonString( + Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + id: Schema.String, + headRefOid: Schema.String, + isMergeQueueEnabled: Schema.optional(Schema.Boolean), + mergeStateStatus: Schema.optional(Schema.NullOr(Schema.String)), + baseRef: Schema.optional( + Schema.NullOr( + Schema.Struct({ compare: Schema.NullOr(Schema.Struct({ behindBy: Schema.Int })) }), + ), + ), + }), + }), + }), + }), + ), +); +const decodeActionState = (raw: string) => + Result.map(decodeActionStateResponse(raw), (response) => response.data.repository.pullRequest); + +const MERGE_PULL_REQUEST_GRAPHQL_MUTATION = `mutation($input: MergePullRequestInput!) { + mergePullRequest(input: $input) { clientMutationId } +}`; +const ENABLE_AUTO_MERGE_GRAPHQL_MUTATION = `mutation($input: EnablePullRequestAutoMergeInput!) { + enablePullRequestAutoMerge(input: $input) { clientMutationId } +}`; +const DISABLE_AUTO_MERGE_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { + disablePullRequestAutoMerge(input: { pullRequestId: $pullRequestId }) { clientMutationId } +}`; +const UPDATE_BRANCH_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!, $expectedHeadOid: GitObjectID!, $updateMethod: PullRequestBranchUpdateMethod!) { + updatePullRequestBranch(input: { pullRequestId: $pullRequestId, expectedHeadOid: $expectedHeadOid, updateMethod: $updateMethod }) { clientMutationId } +}`; +const READY_FOR_REVIEW_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { + markPullRequestReadyForReview(input: { pullRequestId: $pullRequestId }) { clientMutationId } +}`; +const CONVERT_TO_DRAFT_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { + convertPullRequestToDraft(input: { pullRequestId: $pullRequestId }) { clientMutationId } +}`; +const CLOSE_PULL_REQUEST_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { + closePullRequest(input: { pullRequestId: $pullRequestId }) { clientMutationId } +}`; +const REOPEN_PULL_REQUEST_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { + reopenPullRequest(input: { pullRequestId: $pullRequestId }) { clientMutationId } +}`; +const ADD_COMMENT_GRAPHQL_MUTATION = `mutation($subjectId: ID!, $body: String!) { + addComment(input: { subjectId: $subjectId, body: $body }) { clientMutationId } +}`; + +const GRAPHQL_MERGE_METHODS = { + merge: "MERGE", + squash: "SQUASH", + rebase: "REBASE", +} as const satisfies Record; + +/** States in which `gh pr merge --auto` merges at once instead of arming auto-merge. */ +const IMMEDIATELY_MERGEABLE = new Set(["CLEAN", "HAS_HOOKS", "UNSTABLE"]); + +/** The mutations that only need the pull request's node id. */ +const SIMPLE_ACTION_MUTATIONS = { + "disable-auto-merge": DISABLE_AUTO_MERGE_GRAPHQL_MUTATION, + ready: READY_FOR_REVIEW_GRAPHQL_MUTATION, + draft: CONVERT_TO_DRAFT_GRAPHQL_MUTATION, + close: CLOSE_PULL_REQUEST_GRAPHQL_MUTATION, + reopen: REOPEN_PULL_REQUEST_GRAPHQL_MUTATION, +} as const satisfies Partial>; /** @public Service construction is part of the canonical Effect module API. */ export const make = Effect.gen(function* () { - const github = yield* GitHubCli.GitHubCli; - const graphQlBudget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; - const crypto = yield* Crypto.Crypto; + const api = yield* GitHubApi.GitHubApi; const revalidateChecks = yield* makeChecksRevalidator; const routingIdentities = new Map< string, @@ -1172,25 +1154,9 @@ export const make = Effect.gen(function* () { const unavailable = () => new GitHubViewerLoginUnavailableError({ command: "gh", cwd: input.cwd }); const host = input.host.toLowerCase(); - const pinned = yield* GitHubCli.PinnedGitHubCredential; - if (pinned !== null && pinned.host !== host) return yield* unavailable(); - // Only the digest is retained. Never attach credential lookup output to an error. - const token = - pinned !== null - ? Redacted.value(pinned.token) - : (yield* github - .execute({ - cwd: input.cwd, - args: ["auth", "token", "--hostname", host], - env: { GH_DEBUG: "" }, - }) - .pipe(Effect.mapError(unavailable))).stdout.trim(); - if (!token) return yield* unavailable(); - const tokenHash = yield* crypto - .digest("SHA-256", new TextEncoder().encode(token)) - .pipe(Effect.map(Hex.encode), Effect.orDie); - const key = `${host}:${tokenHash}`; - const credential = { host, token: Redacted.make(token), credentialFingerprint: key }; + // A missing or signed-out credential keeps its own error, so the page can say which. + const { token, fingerprint: key } = yield* api.credential(host); + const credential = { host, token, credentialFingerprint: key }; // A cold page may ask several times. Wait per credential and check again after the // first verification; cancellation releases the next waiter without losing its request. return yield* Effect.acquireUseRelease( @@ -1208,21 +1174,13 @@ 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. - const response = yield* github - .execute({ - cwd: input.cwd, - args: ["api", "user", "--hostname", host], - env: { - GH_HOST: host, - GH_TOKEN: token, - GITHUB_TOKEN: token, - GH_ENTERPRISE_TOKEN: token, - GITHUB_ENTERPRISE_TOKEN: token, - GH_DEBUG: "", - }, - }) - .pipe(Effect.mapError(unavailable)); - const identity = yield* decodeRoutingIdentity(response.stdout).pipe( + const response = yield* api + .rest({ host, operation: "getRoutingIdentity", path: "user", allowReserve: true }) + .pipe( + Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), + Effect.provideService(SourceControlRateLimit.CredentialScope, key), + ); + const identity = yield* decodeRoutingIdentity(response.body).pipe( Effect.mapError(unavailable), ); const value = { accountId: String(identity.id), viewer: identity.login }; @@ -1248,7 +1206,7 @@ export const make = Effect.gen(function* () { Effect.flatMap(({ host, token, accountId, viewer, credentialFingerprint }) => use({ accountId, viewer, credentialFingerprint }).pipe( Effect.provideService(SourceControlRateLimit.CredentialScope, credentialFingerprint), - Effect.provideService(GitHubCli.PinnedGitHubCredential, { + Effect.provideService(GitHubApi.PinnedGitHubCredential, { host, token, credentialFingerprint, @@ -1293,11 +1251,7 @@ export const make = Effect.gen(function* () { host: input.host, operation: input.operation, allowReserve: true, - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ], + variables: { owner, name, number: input.number }, query: PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, decode: decodePullRequestNodeIdJson, }).pipe( @@ -1333,46 +1287,35 @@ export const make = Effect.gen(function* () { host: input.host, operation: input.operation, allowReserve: true, - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `subjectId=${input.subjectId}`], - ], + variables: { owner, name, number: input.number, subjectId: input.subjectId }, query: REACTION_SUBJECT_PULL_REQUEST_GRAPHQL_QUERY, decode: decodeReactionSubjectScopeJson, }); }; - // `gh` resolves a bare `owner/repo` against whichever host it defaults to, which is - // github.com. Naming the host makes a GitHub Enterprise repository resolve to its own - // install rather than to a same-named repository on github.com. - const repositoryArgs = (input: { readonly host: string; readonly repository: string }) => [ - "--repo", - `${input.host}/${input.repository}`, - ]; - - /** - * A GraphQL mutation whose answer is not read back. `gh` exits non-zero on a GraphQL error, - * so a failed mutation is already a failed command rather than a body to inspect. - * - * The query and its variables travel over stdin as one document: a variable can carry a - * body the reader wrote, and argv is visible in process listings and echoed back inside - * process-runner failure messages. - */ + /** A GraphQL mutation whose answer is not read back; a GraphQL error fails it. */ const graphql = (input: { - readonly cwd: string; readonly host: string; + readonly operation: string; readonly query: string; - readonly variables: Readonly>; - }) => - github - .execute({ - cwd: input.cwd, - args: ["api", "graphql", "--hostname", input.host, "--input", "-"], - stdin: encodeGraphQlRequestJson({ query: input.query, variables: input.variables }), - }) - .pipe(Effect.asVoid); + readonly variables: Readonly>; + }) => api.graphql(input).pipe(Effect.asVoid); + + const readError = (cwd: string, operation: string, cause: unknown) => + new GitHubPullRequestReadError({ command: "gh", cwd, operation, cause }); + + /** Decodes an answer, reporting a failure against the read that made it. */ + const decodeWith = ( + cwd: string, + operation: string, + decode: (raw: string) => Result.Result, + raw: string, + ): Effect.Effect => { + const decoded = decode(raw.trim()); + return Result.isSuccess(decoded) + ? Effect.succeed(decoded.success) + : Effect.fail(readError(cwd, operation, decoded.failure)); + }; /** A GraphQL read whose answer is decoded, reporting a failure against the read that made it. */ const graphqlRead = (input: { @@ -1380,75 +1323,45 @@ export const make = Effect.gen(function* () { readonly host: string; readonly operation: string; readonly allowReserve?: boolean | undefined; - /** Variables as `-f` flags, for values this module composed itself. */ - readonly variables?: ReadonlyArray; - /** - * Variables carrying words the reader typed. Document and variables travel over stdin - * together, because argv is visible in process listings and is echoed back inside a - * process-runner failure message. - */ - readonly privateVariables?: Readonly>; + readonly variables?: Readonly>; readonly query: string; readonly decode: (raw: string) => Result.Result; - }): Effect.Effect => { - return graphQlBudget - .query( - input.host, - input.query, - input.allowReserve === true ? { allowReserve: true } : undefined, - ) + }): Effect.Effect => + api + .graphql({ + host: input.host, + operation: input.operation, + query: input.query, + ...(input.variables === undefined ? {} : { variables: input.variables }), + ...(input.allowReserve === true ? { allowReserve: true } : {}), + }) + .pipe(Effect.flatMap((raw) => decodeWith(input.cwd, input.operation, input.decode, raw))); + + /** A REST read whose JSON answer is decoded the same way. */ + const restRead = (input: { + readonly cwd: string; + readonly host: string; + readonly operation: string; + readonly path: string; + readonly decode: (raw: string) => Result.Result; + }): Effect.Effect => + api + .rest({ host: input.host, operation: input.operation, path: input.path }) .pipe( - Effect.flatMap((query) => - github.execute( - input.privateVariables === undefined - ? { - cwd: input.cwd, - args: [ - "api", - "graphql", - "--hostname", - input.host, - ...(input.variables ?? []).flat(), - "-f", - `query=${query}`, - ], - } - : { - cwd: input.cwd, - args: ["api", "graphql", "--hostname", input.host, "--input", "-"], - stdin: encodeGraphQlRequestJson({ - query, - variables: input.privateVariables, - }), - }, - ), + Effect.flatMap((response) => + decodeWith(input.cwd, input.operation, input.decode, response.body), ), - Effect.tap((result) => graphQlBudget.observe(input.host, result.stdout)), - Effect.flatMap((result) => { - const decoded = input.decode(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: input.operation, - cause: decoded.failure, - }), - ); - }), ); - }; /** - * One page of the patch, read from the files API. GitHub refuses `pr diff` outright past 300 - * changed files, and still serves those files' hunks here. + * One page of the patch, read from the files API. GitHub refuses a whole diff past 300 changed + * files, and still serves those files' hunks here. * * A page is a whole number of files, so each one parses on its own; the caller carries on from * `nextCursor` for as long as GitHub keeps handing pages back. * * A named commit is read from the commit endpoint, which lists the same file entries and pages - * them the same way — only wrapped in an object, which jq unwraps before they are decoded. + * them the same way, only wrapped in an object. */ const diffFilesPage = (input: { readonly cwd: string; @@ -1460,61 +1373,51 @@ export const make = Effect.gen(function* () { }): Effect.Effect => { const { owner, name } = parseRepositorySelector(input.repository); const paging = `per_page=${DIFF_FILES_PAGE_SIZE}&page=${input.page}`; - return github - .execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, + return api + .rest({ + host: input.host, + operation: "getPullRequestDiff", + path: input.commit === undefined ? `repos/${owner}/${name}/pulls/${input.number}/files?${paging}` : `repos/${owner}/${name}/commits/${input.commit}?${paging}`, - // An empty commit carries no `files` at all, which is a commit with nothing in it - // rather than an answer that could not be read. - ...(input.commit === undefined ? [] : ["--jq", ".files // []"]), - ], - maxOutputBytes: DIFF_MAX_OUTPUT_BYTES, - timeoutMs: DIFF_TIMEOUT_MS, + maxResponseBytes: DIFF_MAX_OUTPUT_BYTES, + timeout: DIFF_TIMEOUT, }) .pipe( - Effect.flatMap((result) => { + Effect.flatMap((response) => { // Checked before decoding: a byte-truncated response is a JSON prefix, which would // fail to parse. Nothing of this page can be shown, and an empty patch would render // as a change with no files rather than as the failure it is; slices already handed // over stay with the reader either way. - if (result.stdoutTruncated) { + if (response.truncated) { return Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestDiff", - cause: new Error(`Page ${input.page} of the changed files was too large to read.`), - }), - ); - } - const decoded = decodePullRequestFilesJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestDiff", - cause: decoded.failure, - }), + readError( + input.cwd, + "getPullRequestDiff", + new Error(`Page ${input.page} of the changed files was too large to read.`), + ), ); } + return decodeWith( + input.cwd, + "getPullRequestDiff", + input.commit === undefined ? decodePullRequestFilesJson : decodeCommitFilesJson, + response.body, + ); + }), + Effect.map((files) => { // Counted before decoding, so a page whose files all failed to decode still moves on // rather than pointing the reader back at the page it just read. - const morePages = decoded.success.rawCount >= DIFF_FILES_PAGE_SIZE; - return Effect.succeed({ - patch: decoded.success.patch, - truncated: decoded.success.truncated, + const morePages = files.rawCount >= DIFF_FILES_PAGE_SIZE; + return { + patch: files.patch, + truncated: files.truncated, nextCursor: morePages ? String(input.page + 1) : null, - ...(decoded.success.omittedFileStats.length === 0 + ...(files.omittedFileStats.length === 0 ? {} - : { omittedFileStats: decoded.success.omittedFileStats }), - }); + : { omittedFileStats: files.omittedFileStats }), + }; }), ); }; @@ -1526,33 +1429,32 @@ export const make = Effect.gen(function* () { return yield* new GitHubDiffCommitError({ command: "gh", cwd: input.cwd }); } const { owner, name } = parseRepositorySelector(input.repository); - const refsResult = yield* github.execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, + const refsResponse = yield* api.rest({ + host: input.host, + operation: "getPullRequestDiffFileContents", + path: input.commit === undefined ? `repos/${owner}/${name}/pulls/${input.number}` : `repos/${owner}/${name}/commits/${input.commit}`, - "--jq", - input.commit === undefined - ? "[.base.sha, .head.sha] | @tsv" - : "[.parents[0].sha, .sha] | @tsv", - ], - maxOutputBytes: 1024, - timeoutMs: DIFF_TIMEOUT_MS, + timeout: DIFF_TIMEOUT, }); - // Keep a leading tab: a root commit has no parent, and jq represents that absent old - // revision as the empty field before the tab. Every file in it is new, so that is a - // usable answer whenever the caller does not need the old side. - const [baseRef, headRef, ...extraRefs] = refsResult.stdout.trimEnd().split("\t"); + const refs = decodeRevisionRefs(refsResponse.body); + // A root commit has no parent, which is an absent old revision. Every file in it is new, + // so that is a usable answer whenever the caller does not need the old side. + const baseRef = Option.isSome(refs) + ? input.commit === undefined + ? refs.value.base?.sha + : (refs.value.parents?.[0]?.sha ?? "") + : undefined; + const headRef = Option.isSome(refs) + ? input.commit === undefined + ? refs.value.head?.sha + : refs.value.sha + : undefined; const rootCommitNewFile = input.commit !== undefined && input.changeType === "new" && baseRef === ""; if ( - refsResult.stdoutTruncated || - !headRef || - extraRefs.length > 0 || + headRef === undefined || (!rootCommitNewFile && (baseRef === undefined || !isCommitSha(baseRef))) || !isCommitSha(headRef) ) { @@ -1565,43 +1467,38 @@ export const make = Effect.gen(function* () { } const readFile = (revision: string, filePath: string) => - github - .execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - "--header", - "Accept: application/vnd.github.raw+json", - `repos/${owner}/${name}/contents/${filePath - .split("/") - .map(encodeURIComponent) - .join("/")}?ref=${encodeURIComponent(revision)}`, - ], - maxOutputBytes: DIFF_FILE_MAX_OUTPUT_BYTES, - timeoutMs: DIFF_TIMEOUT_MS, + api + .rest({ + host: input.host, + operation: "getPullRequestDiffFileContents", + accept: "application/vnd.github.raw+json", + path: `repos/${owner}/${name}/contents/${filePath + .split("/") + .map(encodeURIComponent) + .join("/")}?ref=${encodeURIComponent(revision)}`, + maxResponseBytes: DIFF_FILE_MAX_OUTPUT_BYTES, + timeout: DIFF_TIMEOUT, }) .pipe( - Effect.flatMap((result) => - result.stdoutTruncated || - result.stdout.includes("\0") || - result.stdoutInvalidUtf8 === true + Effect.flatMap((response) => + response.truncated || response.body.includes("\0") || response.invalidUtf8 ? Effect.fail( new GitHubDiffFileContentsUnavailableError({ command: "gh", cwd: input.cwd, path: filePath, - reason: result.stdoutTruncated ? "oversized" : "binary", + reason: response.truncated ? "oversized" : "binary", }), ) - : Effect.succeed(result.stdout), + : Effect.succeed(response.body), ), ); const [oldContents, newContents] = yield* Effect.all( [ - input.changeType === "new" ? Effect.succeed("") : readFile(baseRef, input.oldPath), + input.changeType === "new" + ? Effect.succeed("") + : readFile(baseRef ?? "", input.oldPath), input.changeType === "deleted" ? Effect.succeed("") : readFile(headRef, input.newPath), ], { concurrency: 2 }, @@ -1609,262 +1506,266 @@ export const make = Effect.gen(function* () { return { oldContents, newContents }; }); - const readLegacyDetail = ( + /** + * Every check context of the head commit, a page at a time, for a rollup the detail read cut at + * its first hundred. Each page names the head it read, so a push mid-walk fails rather than + * mixing two revisions' checks. + */ + const readAllCheckContexts = ( input: Parameters[0], + allowReserve: boolean, ) => - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "view", - String(input.number), - ...repositoryArgs(input), - "--json", - PULL_REQUEST_DETAIL_JSON_FIELDS, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodePullRequestDetailJson(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestDetail", - cause: decoded.failure, - }), - ); - }), - ); + Effect.gen(function* () { + const { owner, name } = parseRepositorySelector(input.repository); + const contexts: GitHubCheckContext[] = []; + let headSha: string | null = null; + let after: string | null = null; + for (let page = 0; page < CHECK_CONTEXT_PAGES; page++) { + const read: { + readonly headSha: string; + readonly contexts: ReadonlyArray; + readonly nextCursor: string | null; + } = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestDetail", + allowReserve, + variables: { owner, name, number: input.number, after }, + query: pullRequestCheckContextsGraphQlQuery(input.host), + decode: decodePullRequestCheckContextsJson, + }); + if (headSha !== null && read.headSha !== headSha) { + return yield* readError( + input.cwd, + "getPullRequestDetail", + new Error("Pull request head changed while reading checks."), + ); + } + headSha = read.headSha; + contexts.push(...read.contexts); + after = read.nextCursor; + if (after === null) break; + } + return { headSha, contexts, truncated: after !== null }; + }); const getPullRequestDetail: GitHubPullRequestCli["Service"]["getPullRequestDetail"] = (input) => { const { owner, name } = parseRepositorySelector(input.repository); - return GitHubCli.AllowGitHubReserve.pipe( + return AllowGitHubReserve.pipe( Effect.flatMap((allowReserve) => graphqlRead({ allowReserve, cwd: input.cwd, host: input.host, operation: "getPullRequestDetail", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `headRef=refs/pull/${input.number}/head`], - ], + variables: { + owner, + name, + number: input.number, + headRef: `refs/pull/${input.number}/head`, + }, query: pullRequestCoreGraphQlQuery(input.host), decode: decodePullRequestCoreJson, - }), - ), - // gh already pages check contexts. Keep its complete, deduplicated result for - // large check suites instead of letting the first 100 checks imply success. - Effect.filterOrElse( - (core) => !core.checksTruncated, - (core) => - readLegacyDetail(input).pipe( - Effect.filterOrFail( - (detail) => detail.headSha === core.headSha, - () => - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestDetail", - cause: new Error("Pull request head changed while reading checks."), - }), - ), - Effect.map((detail) => ({ - ...core, - checks: detail.checks, - checksState: detail.checksState, - checksTruncated: false, - })), + }).pipe( + // Past a hundred checks the first page would let the rest imply success, so the whole + // rollup is walked instead. + Effect.filterOrElse( + (core) => !core.checksTruncated, + (core) => + readAllCheckContexts(input, allowReserve).pipe( + Effect.filterOrFail( + (all) => all.headSha === core.headSha, + () => + readError( + input.cwd, + "getPullRequestDetail", + new Error("Pull request head changed while reading checks."), + ), + ), + Effect.map((all) => ({ + ...core, + ...pullRequestChecksFromContexts(all.contexts), + checksTruncated: all.truncated, + })), + ), ), + ), ), ); }; const workflowApprovalLimit = 1_000; - const workflowApprovalProbeLimit = String(workflowApprovalLimit + 1); const workflowApprovalReadError = (cwd: string, cause: unknown) => - new GitHubPullRequestReadError({ - command: "gh", - cwd, - operation: "listWorkflowRunsRequiringApproval", - cause, + readError(cwd, "listWorkflowRunsRequiringApproval", cause); + + /** Every open pull request whose head branch carries this name, up to one past the limit. */ + const listHeadsByBranch = (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly headBranch: string; + }) => + Effect.gen(function* () { + const { owner, name } = parseRepositorySelector(input.repository); + const heads: GitHubPullRequestHead[] = []; + let after: string | null = null; + do { + const page: { + readonly heads: ReadonlyArray; + readonly nextCursor: string | null; + } = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listWorkflowRunsRequiringApproval", + allowReserve: true, + variables: { owner, name, head: input.headBranch, after }, + query: PULL_REQUEST_HEADS_GRAPHQL_QUERY, + decode: decodePullRequestHeadsJson, + }); + heads.push(...page.heads); + after = page.nextCursor; + } while (after !== null && heads.length <= workflowApprovalLimit); + return heads; }); + + /** + * The runs waiting on a maintainer for this exact head, up to one past the limit. `head_sha` is + * what scopes them; `head_branch` is checked as well, the way `gh run list --branch` did. + */ + const listActionRequiredRuns = (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly headSha: string; + readonly headBranch: string; + }) => + Effect.gen(function* () { + // The checks revalidator may have just confirmed every run of this head as current. + const known = yield* KnownWorkflowRuns; + if (known !== null && known.headSha === input.headSha) { + return known.runs.flatMap((run) => + // A run waiting on a maintainer reports `completed` with an `action_required` + // conclusion; the `status=action_required` query matches either. + (run.conclusion === "action_required" || run.status === "action_required") && + run.head_branch === input.headBranch + ? [ + { + id: run.id, + name: run.name?.trim() || `Workflow run ${run.id}`, + url: run.html_url?.trim() || null, + }, + ] + : [], + ); + } + const { owner, name } = parseRepositorySelector(input.repository); + const runs: GitHubWorkflowRunApproval[] = []; + for (let page = 1; runs.length <= workflowApprovalLimit; page++) { + const read: GitHubWorkflowRunPage = yield* restRead({ + cwd: input.cwd, + host: input.host, + operation: "listWorkflowRunsRequiringApproval", + path: `repos/${owner}/${name}/actions/runs?head_sha=${encodeURIComponent(input.headSha)}&branch=${encodeURIComponent(input.headBranch)}&event=pull_request&status=action_required&per_page=100&page=${page}`, + decode: decodeWorkflowRunsJson, + }).pipe(Effect.mapError((error) => workflowApprovalReadError(input.cwd, error))); + runs.push(...read.runs); + if (read.rawCount < 100) break; + } + return runs; + }); + const listWorkflowRunsRequiringApproval: GitHubPullRequestCli["Service"]["listWorkflowRunsRequiringApproval"] = (input) => Effect.all( [ - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "list", - ...repositoryArgs(input), - "--state", - "open", - "--head", - input.headBranch, - "--limit", - workflowApprovalProbeLimit, - "--json", - "number,headRefOid,isCrossRepository,headRepositoryOwner", - ], - }) - .pipe( - Effect.flatMap( - ( - result, - ): Effect.Effect< - GitHubPullRequestHead, - GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError - > => { - const decoded = decodePullRequestHeadsJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); - } - const exactHeads = decoded.success.filter( - (pullRequest) => - pullRequest.headSha === input.headSha && - pullRequest.isCrossRepository === true && - pullRequest.headRepositoryOwner?.toLowerCase() === - input.headRepositoryOwner.toLowerCase(), + listHeadsByBranch(input).pipe( + Effect.flatMap( + ( + heads, + ): Effect.Effect< + GitHubPullRequestHead, + GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError + > => { + const exactHeads = heads.filter( + (pullRequest) => + pullRequest.headSha === input.headSha && + pullRequest.isCrossRepository === true && + pullRequest.headRepositoryOwner?.toLowerCase() === + input.headRepositoryOwner.toLowerCase(), + ); + if (heads.length > workflowApprovalLimit) { + return Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "head-list-truncated", + observedCount: heads.length, + limit: workflowApprovalLimit, + }), ); - if (decoded.success.length > workflowApprovalLimit) { - return Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "head-list-truncated", - observedCount: decoded.success.length, - limit: workflowApprovalLimit, - }), - ); - } - if (exactHeads.length !== 1 || exactHeads[0]?.number !== input.number) { - return Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "head-not-unique", - observedCount: exactHeads.length, - limit: workflowApprovalLimit, - }), - ); - } - return Effect.succeed(exactHeads[0]); - }, - ), + } + if (exactHeads.length !== 1 || exactHeads[0]?.number !== input.number) { + return Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "head-not-unique", + observedCount: exactHeads.length, + limit: workflowApprovalLimit, + }), + ); + } + return Effect.succeed(exactHeads[0]); + }, ), - github - .execute({ - cwd: input.cwd, - args: [ - "run", - "list", - ...repositoryArgs(input), - "--commit", - input.headSha, - "--branch", - input.headBranch, - "--event", - "pull_request", - "--status", - "action_required", - "--limit", - workflowApprovalProbeLimit, - "--json", - "databaseId,workflowName,url", - ], - }) - .pipe( - Effect.flatMap( - ( - result, - ): Effect.Effect< - ReadonlyArray, - GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError - > => { - const decoded = decodeWorkflowRunApprovalsJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); - } - return decoded.success.length > workflowApprovalLimit - ? Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "run-list-truncated", - observedCount: decoded.success.length, - limit: workflowApprovalLimit, - }), - ) - : Effect.succeed(decoded.success); - }, - ), + ), + listActionRequiredRuns(input).pipe( + Effect.flatMap((runs) => + runs.length > workflowApprovalLimit + ? Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "run-list-truncated", + observedCount: runs.length, + limit: workflowApprovalLimit, + }), + ) + : Effect.succeed(runs), ), + ), ], { concurrency: 2 }, ).pipe(Effect.map(([, runs]) => runs)); - // One `gh pr view` either way; asking for the detail fields costs nothing extra and hands - // the thread overview its author, diff stat, review decision and checks in the same read. - const viewPullRequestSummary = (input: PullRequestSummaryRead) => - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "view", - String(input.number), - ...repositoryArgs(input), - "--json", - PULL_REQUEST_DETAIL_JSON_FIELDS, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodePullRequestDetailJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestSummary", - cause: decoded.failure, - }), - ); - } - const detail = decoded.success; - return Effect.succeed({ - number: detail.number, - title: detail.title, - url: detail.url, - headBranch: detail.headBranch, - baseBranch: detail.baseBranch, - state: detail.state, - updatedAt: detail.updatedAt, - closedAt: detail.closedAt ?? null, - mergedAt: detail.mergedAt ?? null, - isDraft: detail.isDraft, - author: detail.author, - additions: detail.additions, - deletions: detail.deletions, - changedFiles: detail.changedFiles, - reviewDecision: detail.reviewDecision, - checksState: detail.checksState, - mergeability: detail.mergeability, - }); - }), - ); + /** One pull request's summary, through the same aliased query the batch uses. */ + const viewPullRequestSummary = (input: PullRequestSummaryRead) => { + const { owner, name } = parseRepositorySelector(input.repository); + return graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestSummary", + variables: { owner, name, number: input.number }, + query: pullRequestSummaryGraphQlQuery(input.host === "github.com"), + decode: decodePullRequestSummariesJson, + }).pipe( + Effect.flatMap((summaries) => { + const summary = summaries.get(0); + return summary === undefined + ? Effect.fail( + readError( + input.cwd, + "getPullRequestSummary", + new Error(`GitHub answered nothing for ${input.repository}#${input.number}.`), + ), + ) + : Effect.succeed(summary); + }), + ); + }; /** * Summaries asked for together, on one host under one credential, share aliased GraphQL reads @@ -1876,7 +1777,7 @@ export const make = Effect.gen(function* () { key: ({ request, context }) => JSON.stringify([ request.host.toLowerCase(), - Context.getOrElse(context, GitHubCli.PinnedGitHubCredential, () => null) + Context.getOrElse(context, GitHubApi.PinnedGitHubCredential, () => null) ?.credentialFingerprint ?? null, Context.getOrElse(context, SourceControlRateLimit.CredentialScope, () => ""), ]), @@ -1956,7 +1857,7 @@ export const make = Effect.gen(function* () { key: ({ request, context }) => JSON.stringify([ request.host.toLowerCase(), - Context.getOrElse(context, GitHubCli.PinnedGitHubCredential, () => null) + Context.getOrElse(context, GitHubApi.PinnedGitHubCredential, () => null) ?.credentialFingerprint ?? null, Context.getOrElse(context, SourceControlRateLimit.CredentialScope, () => ""), ]), @@ -2010,70 +1911,88 @@ export const make = Effect.gen(function* () { listPullRequests: (input) => { const fallbackMaxRows = Math.max(input.limit + 1, PULL_REQUEST_FALLBACK_MAX_ROWS); + const { owner, name } = parseRepositorySelector(input.repository); + const query = searchQuery({ ...input, repositories: [input.repository] }); + if (query === null) { + return Effect.fail( + new GitHubRepositorySelectorError({ + command: "gh", + cwd: input.cwd, + operation: "listPullRequests", + }), + ); + } + /** Pages of up to a hundred until `rows` have arrived or GitHub has no more. */ + const collect = ( + rows: number, + page: ( + after: string | null, + rows: number, + ) => Effect.Effect, + ) => + Effect.gen(function* () { + const items: GitHubPullRequestListItem[] = []; + let rawCount = 0; + let after: string | null = null; + do { + const read: GitHubPullRequestListPage = yield* page(after, rows - rawCount); + items.push(...read.items); + rawCount += read.rawCount; + after = read.endCursor; + } while (after !== null && rawCount < rows); + return { items, rawCount }; + }); const read = ( continues: boolean, requestedRows = input.limit + 1, ): Effect.Effect => - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "list", - ...repositoryArgs(input), - ...involvementArgs({ ...input, sorted: continues }), - "--state", - input.state, - "--limit", - // One extra row reveals that the repository has more than the page shows. - String(requestedRows), - "--json", - PULL_REQUEST_LIST_JSON_FIELDS, - ], - }) - .pipe( - Effect.flatMap((result) => { - const raw = result.stdout.trim(); - if (raw.length === 0) { - return Effect.succeed({ items: [], truncated: false, continues }); - } - const decoded = decodePullRequestListJson(raw); - if (Result.isSuccess(decoded)) { - const items = continues - ? decoded.success.items - : decoded.success.items.filter((item) => matchesUnsortedListing(item, input)); - if ( - !continues && - items.length < input.limit && - decoded.success.rawCount >= requestedRows && - requestedRows < fallbackMaxRows - ) { - const nextRows = Math.min(requestedRows * 2, fallbackMaxRows); - if (nextRows > requestedRows) return read(false, nextRows); - } - return Effect.succeed({ - items: items.slice(0, input.limit), - // One row over the page size is the probe for a next page, and it is - // counted before decoding: a skipped malformed row must not end paging. - truncated: continues - ? decoded.success.rawCount > input.limit - : items.length > input.limit || decoded.success.rawCount >= requestedRows, - continues, - }); - } - return Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "listPullRequests", - cause: decoded.failure, - }), - ); - }), - ); + collect(requestedRows, (after, rows) => + continues + ? graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + // The reader's own words are in the query. + variables: { q: query, after }, + query: pullRequestSearchGraphQlQuery(rows, false, true), + decode: decodePullRequestSearchJson, + }) + : graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + variables: { owner, name, states: LIST_STATES[input.state], after }, + query: pullRequestListGraphQlQuery(rows), + decode: decodePullRequestListJson, + }), + ).pipe( + Effect.flatMap(({ items: rawItems, rawCount }) => { + const items = continues + ? rawItems + : rawItems.filter((item) => matchesUnsortedListing(item, input)); + if ( + !continues && + items.length < input.limit && + rawCount >= requestedRows && + requestedRows < fallbackMaxRows + ) { + const nextRows = Math.min(requestedRows * 2, fallbackMaxRows); + if (nextRows > requestedRows) return read(false, nextRows); + } + return Effect.succeed({ + items: items.slice(0, input.limit), + // One row over the page size is the probe for a next page, and it is + // counted before decoding: a skipped malformed row must not end paging. + truncated: continues + ? rawCount > input.limit + : items.length > input.limit || rawCount >= requestedRows, + continues, + }); + }), + ); // GitHub does not index every repository for search, and one it will not search answers - // with no rows rather than with an error — so an empty listing is read again the way `gh` - // lists without one. Those rows come back newest-created first, an order no `updated:` + // with no rows rather than with an error — so an empty listing is read again from the + // repository's own list. Those rows come back newest-created first, an order no `updated:` // qualifier can carry on from, so that page says it cannot be continued and the reader // reaches the rest of it by asking for a larger page, as every listing used to. // @@ -2157,8 +2076,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "searchPullRequests", - // The reader's own words are in the query, so it travels over stdin rather than in argv. - privateVariables: { q: query }, + variables: { q: query, after: null }, query: pullRequestSearchGraphQlQuery(rows, input.host === "github.com"), decode: decodePullRequestSearchJson, }).pipe( @@ -2217,11 +2135,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "getPullRequestPreview", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ], + variables: { owner, name, number: input.number }, query: PULL_REQUEST_PREVIEW_GRAPHQL_QUERY, decode: decodePullRequestPreviewJson, }); @@ -2230,65 +2144,30 @@ export const make = Effect.gen(function* () { getPullRequestStack: (input) => { const { owner, name } = parseRepositorySelector(input.repository); - return github - .execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - `repos/${owner}/${name}/stacks?pull_request=${input.number}`, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodePullRequestStacksJson(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestStack", - cause: decoded.failure, - }), - ); - }), - // @effect-diagnostics-next-line flatMapConditionalToFilterOrFail:off - the fallback needs a non-null stack, which a predicate that also reads includeDetails cannot refine. - Effect.flatMap((stack) => { - if (!input.includeDetails || stack === null) return Effect.succeed(stack); - return github - .execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - `repos/${owner}/${name}/stacks/${stack.number}`, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodePullRequestStacksJson(`[${result.stdout.trim()}]`); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestStack", - cause: decoded.failure, - }), - ); - }), - ); - }), - // Hosts without the stacks preview return 404. Other failures must preserve the - // previously synced stack and let the caller retry. - Effect.catchTags({ - GitHubPullRequestNotFoundError: () => Effect.succeed(null), - }), - ); + return restRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestStack", + path: `repos/${owner}/${name}/stacks?pull_request=${input.number}`, + decode: decodePullRequestStacksJson, + }).pipe( + // @effect-diagnostics-next-line flatMapConditionalToFilterOrFail:off - the fallback needs a non-null stack, which a predicate that also reads includeDetails cannot refine. + Effect.flatMap((stack) => { + if (!input.includeDetails || stack === null) return Effect.succeed(stack); + return restRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestStack", + path: `repos/${owner}/${name}/stacks/${stack.number}`, + decode: (raw) => decodePullRequestStacksJson(`[${raw}]`), + }); + }), + // Hosts without the stacks preview return 404. Other failures must preserve the + // previously synced stack and let the caller retry. + Effect.catchTags({ + GitHubApiNotFoundError: () => Effect.succeed(null), + }), + ); }, getPullRequestBaseComparison: (input) => { @@ -2298,45 +2177,59 @@ export const make = Effect.gen(function* () { host: input.host, operation: "getPullRequestBaseComparison", ...(input.allowReserve === true ? { allowReserve: true } : {}), - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `headRef=${input.headRef}`], - ], + variables: { owner, name, number: input.number, headRef: input.headRef }, query: BASE_COMPARISON_GRAPHQL_QUERY, decode: decodeBaseComparisonJson, }); }, getPullRequestActivity: (input) => - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "view", - String(input.number), - ...repositoryArgs(input), - "--json", - PULL_REQUEST_ACTIVITY_JSON_FIELDS, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodePullRequestActivityJson(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getPullRequestActivity", - cause: decoded.failure, - }), - ); - }), - ), + Effect.gen(function* () { + const { owner, name } = parseRepositorySelector(input.repository); + let author: PullRequestActor | null = null; + let commits: ReadonlyArray = []; + const remarks: PullRequestComment[] = []; + let commentsAfter: string | null = null; + let reviewsAfter: string | null = null; + let withComments = true; + let withReviews = true; + // Both remark lists page on their own; a page asks only for the ones with more to give. + for (let page = 0; page < REVIEW_THREAD_PAGES && (withComments || withReviews); page++) { + const read: GitHubPullRequestActivityPage = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestActivity", + variables: { + owner, + name, + number: input.number, + head: page === 0, + withComments, + commentsAfter, + withReviews, + reviewsAfter, + }, + query: PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY, + decode: decodePullRequestActivityJson, + }); + if (page === 0) { + author = read.author ?? null; + commits = read.commits ?? []; + } + remarks.push(...read.remarks); + commentsAfter = read.nextCommentsCursor; + reviewsAfter = read.nextReviewsCursor; + withComments = withComments && commentsAfter !== null; + withReviews = withReviews && reviewsAfter !== null; + } + return { + author, + comments: remarks.toSorted((left, right) => + left.createdAt.localeCompare(right.createdAt), + ), + commits, + } satisfies GitHubPullRequestActivity; + }), getPullRequestDiff: (input) => { const filesPage = (page: number) => @@ -2363,31 +2256,34 @@ export const make = Effect.gen(function* () { if (input.commit !== undefined) { return filesPage(1); } - return github - .execute({ - cwd: input.cwd, - args: ["pr", "diff", String(input.number), ...repositoryArgs(input), "--color", "never"], - maxOutputBytes: DIFF_MAX_OUTPUT_BYTES, - timeoutMs: DIFF_TIMEOUT_MS, + const { owner, name } = parseRepositorySelector(input.repository); + return api + .rest({ + host: input.host, + operation: "getPullRequestDiff", + path: `repos/${owner}/${name}/pulls/${input.number}`, + accept: "application/vnd.github.diff", + maxResponseBytes: DIFF_MAX_OUTPUT_BYTES, + timeout: DIFF_TIMEOUT, }) .pipe( - Effect.flatMap((result) => + Effect.flatMap((response) => // A patch cut at a byte boundary ends mid-file, which is neither a whole slice nor // something the reader can carry on from. The files API can serve the same change a // whole number of files at a time, so an oversized patch takes that road as well. - result.stdoutTruncated + response.truncated ? filesPage(1) : // One read served the whole patch, so there is no next slice to ask for. - Effect.succeed({ patch: result.stdout, truncated: false, nextCursor: null }), + Effect.succeed({ patch: response.body, truncated: false, nextCursor: null }), ), // GitHub answers 406 rather than a diff past 300 changed files, so the patch is read // from the files API instead, a page per call. Only once the direct read has failed: a // pull request GitHub will serve a diff for must not pay for a second request. A // fallback that fails too reports the original refusal, which is the one that explains - // the page. Narrowed to a command that ran and was refused: a missing `gh` or a - // signed-out one fails the same way for every request. + // the page. Narrowed to a request GitHub answered and refused: a missing credential or + // a rate limit fails the same way for every request. Effect.catchTags({ - GitHubCliCommandError: (error) => filesPage(1).pipe(Effect.mapError(() => error)), + GitHubApiResponseError: (error) => filesPage(1).pipe(Effect.mapError(() => error)), }), ); }, @@ -2400,13 +2296,13 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "getReviewThreadComments", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `threadId=${input.threadId}`], - cursorVariable(input.cursor), - ], + variables: { + owner, + name, + number: input.number, + threadId: input.threadId, + cursor: input.cursor, + }, query: REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY, decode: decodeReviewThreadCommentsJson, }).pipe( @@ -2434,12 +2330,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "listReviewThreadComments", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - cursorVariable(cursor), - ], + variables: { owner, name, number: input.number, cursor }, query: REVIEW_THREADS_GRAPHQL_QUERY, decode: decodeReviewThreadsJson, }); @@ -2496,12 +2387,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "listReviewThreadComments", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `cursor=${dismissalCursor}`], - ], + variables: { owner, name, number: input.number, cursor: dismissalCursor }, query: REVIEW_DISMISSALS_GRAPHQL_QUERY, decode: decodeReviewDismissalsJson, }); @@ -2547,7 +2433,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "listActorAvatars", - variables: input.ids.map((id) => ["-f", `ids[]=${id}`]), + variables: { ids: input.ids }, query: ACTOR_AVATARS_GRAPHQL_QUERY, decode: decodeActorAvatarsJson, }); @@ -2560,11 +2446,7 @@ export const make = Effect.gen(function* () { host: input.host, operation: "getViewerAccess", ...(input.allowReserve === true ? { allowReserve: true } : {}), - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ], + variables: { owner, name, number: input.number }, query: VIEWER_PERMISSIONS_GRAPHQL_QUERY, decode: decodeViewerPermissionsJson, }); @@ -2577,11 +2459,7 @@ export const make = Effect.gen(function* () { host: input.host, operation: "listReviewerCandidates", allowReserve: true, - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ], + variables: { owner, name, number: input.number }, query: REVIEWER_CANDIDATES_GRAPHQL_QUERY, decode: decodeReviewerCandidatesJson, }); @@ -2589,24 +2467,15 @@ export const make = Effect.gen(function* () { setReviewerRequest: (input) => { const { owner, name } = parseRepositorySelector(input.repository); - return github - .execute({ - cwd: input.cwd, - // Posting to a login GitHub has already been asked about is what a re-request is, so - // there is nothing to say here about somebody who has reviewed once already. The body - // travels over stdin for the reason every other one does: argv is visible in process - // listings and echoed back inside process-runner failure messages. - args: [ - "api", - "--method", - input.requested ? "POST" : "DELETE", - "--hostname", - input.host, - `repos/${owner}/${name}/pulls/${input.number}/requested_reviewers`, - "--input", - "-", - ], - stdin: buildReviewerRequestJson(input.reviewers), + // Posting to a login GitHub has already been asked about is what a re-request is, so + // there is nothing to say here about somebody who has reviewed once already. + return api + .rest({ + host: input.host, + operation: "setReviewerRequest", + method: input.requested ? "POST" : "DELETE", + path: `repos/${owner}/${name}/pulls/${input.number}/requested_reviewers`, + body: buildReviewerRequest(input.reviewers), }) .pipe(Effect.asVoid); }, @@ -2618,11 +2487,7 @@ export const make = Effect.gen(function* () { host: input.host, operation: "listLabelCandidates", allowReserve: true, - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ], + variables: { owner, name, number: input.number }, query: LABEL_CANDIDATES_GRAPHQL_QUERY, decode: decodeLabelCandidatesJson, }); @@ -2635,27 +2500,24 @@ export const make = Effect.gen(function* () { // path. The name goes into the path encoded, because a label may carry a space or a slash. const issue = `repos/${owner}/${name}/issues/${input.number}/labels`; if (input.applied) { - return github - .execute({ - cwd: input.cwd, - args: ["api", "--method", "POST", "--hostname", input.host, issue, "--input", "-"], - stdin: buildLabelRequestJson(input.labels), + return api + .rest({ + host: input.host, + operation: "setLabels", + method: "POST", + path: issue, + body: buildLabelRequest(input.labels), }) .pipe(Effect.asVoid); } return Effect.forEach( input.labels, (label) => - github.execute({ - cwd: input.cwd, - args: [ - "api", - "--method", - "DELETE", - "--hostname", - input.host, - `${issue}/${encodeURIComponent(label)}`, - ], + api.rest({ + host: input.host, + operation: "setLabels", + method: "DELETE", + path: `${issue}/${encodeURIComponent(label)}`, }), { concurrency: 1, discard: true }, ); @@ -2664,14 +2526,14 @@ export const make = Effect.gen(function* () { runPullRequestAction: (input) => { if (input.stackNumber !== undefined) return runGitHubStackAction({ ...input, stackNumber: input.stackNumber }).pipe( - Effect.provideService(GitHubCli.GitHubCli, github), + Effect.provideService(GitHubApi.GitHubApi, api), ); if (input.action === "revert") { return pullRequestNodeId({ ...input, operation: "revertPullRequest" }).pipe( Effect.flatMap((pullRequestId) => graphql({ - cwd: input.cwd, host: input.host, + operation: "revertPullRequest", query: REVERT_PULL_REQUEST_GRAPHQL_MUTATION, variables: { pullRequestId }, }), @@ -2742,18 +2604,12 @@ export const make = Effect.gen(function* () { }), Effect.flatMap((currentRuns) => currentRuns.some((current) => current.id === run.id) - ? github - .execute({ - cwd: input.cwd, - args: [ - "api", - "--method", - "POST", - "--hostname", - input.host, - `repos/${owner}/${name}/actions/runs/${run.id}/approve`, - "--silent", - ], + ? api + .rest({ + host: input.host, + operation: "approveWorkflowRun", + method: "POST", + path: `repos/${owner}/${name}/actions/runs/${run.id}/approve`, }) .pipe(Effect.asVoid) : Effect.void, @@ -2766,32 +2622,67 @@ export const make = Effect.gen(function* () { }), ); } - const [subcommand, ...flags] = actionArgs( - input.action, - input.mergeMethod, - input.updateMethod, - ); + const action = input.action; + if (action in SIMPLE_ACTION_MUTATIONS) { + return pullRequestNodeId({ ...input, operation: "runPullRequestAction" }).pipe( + Effect.flatMap((pullRequestId) => + graphql({ + host: input.host, + operation: "runPullRequestAction", + query: SIMPLE_ACTION_MUTATIONS[action as keyof typeof SIMPLE_ACTION_MUTATIONS], + variables: { pullRequestId }, + }), + ), + ); + } return Effect.gen(function* () { + const { owner, name } = parseRepositorySelector(input.repository); + // Read fresh rather than from the node id cache: merging and updating act on the head + // as it stands now, and the merge queue decides which mutation a merge is. + const state = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "runPullRequestAction", + allowReserve: true, + query: ACTION_STATE_GRAPHQL_QUERY, + variables: { + owner, + name, + number: input.number, + headRef: `refs/pull/${input.number}/head`, + }, + decode: decodeActionState, + }); + if (action === "update-branch") { + // Already current is done, the way `gh pr update-branch` reports it. + if (state.baseRef?.compare?.behindBy === 0) return; + // GitHub updates with a merge commit unless asked to rebase, which is its own default. + return yield* graphql({ + host: input.host, + operation: "runPullRequestAction", + query: UPDATE_BRANCH_GRAPHQL_MUTATION, + variables: { + pullRequestId: state.id, + expectedHeadOid: state.headRefOid, + updateMethod: input.updateMethod === "rebase" ? "REBASE" : "MERGE", + }, + }); + } let body: string | undefined; let expectedHead: string | undefined; - if ( - input.removeAgentCreditsOnMerge === true && - (input.action === "merge" || input.action === "enable-auto-merge") && - input.mergeMethod !== "rebase" - ) { - const { owner, name } = parseRepositorySelector(input.repository); + if (input.removeAgentCreditsOnMerge === true && input.mergeMethod !== "rebase") { const message = yield* graphqlRead({ cwd: input.cwd, host: input.host, operation: "runPullRequestAction", allowReserve: true, query: MERGE_MESSAGE_GRAPHQL_QUERY, - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ["-f", `method=${input.mergeMethod === "squash" ? "SQUASH" : "MERGE"}`], - ], + variables: { + owner, + name, + number: input.number, + method: input.mergeMethod === "squash" ? "SQUASH" : "MERGE", + }, decode: decodeMergeMessage, }); // GitHub's merge queue chooses its own message and ignores custom text. @@ -2803,59 +2694,53 @@ export const make = Effect.gen(function* () { } } } - yield* github.execute({ - cwd: input.cwd, - args: [ - "pr", - subcommand!, - String(input.number), - ...repositoryArgs(input), - ...flags, - ...(expectedHead === undefined ? [] : ["--match-head-commit", expectedHead]), - ...(body === undefined ? [] : ["--body-file", "-"]), - ], - ...(body === undefined ? {} : { stdin: body }), + // A merge queue takes a pull request through auto-merge rather than a direct merge, and + // `--auto` on a pull request that is mergeable right now simply merges it, as `gh` does. + // GitHub stores the strategy with a standing instruction rather than choosing one at + // merge time, so arming still names it. + const auto = + state.isMergeQueueEnabled === true || + (action === "enable-auto-merge" && + !IMMEDIATELY_MERGEABLE.has(state.mergeStateStatus?.toUpperCase() ?? "")); + yield* graphql({ + host: input.host, + operation: "runPullRequestAction", + query: auto ? ENABLE_AUTO_MERGE_GRAPHQL_MUTATION : MERGE_PULL_REQUEST_GRAPHQL_MUTATION, + variables: { + input: { + pullRequestId: state.id, + mergeMethod: GRAPHQL_MERGE_METHODS[input.mergeMethod ?? "merge"], + ...(expectedHead === undefined ? {} : { expectedHeadOid: expectedHead }), + ...(body === undefined ? {} : { commitBody: body }), + }, + }, }); }); }, commentOnPullRequest: (input) => - github - .execute({ - cwd: input.cwd, - // The body travels over stdin: argv is visible in process listings and is echoed - // back inside process-runner failure messages. - args: [ - "pr", - "comment", - String(input.number), - ...repositoryArgs(input), - "--body-file", - "-", - ], - stdin: input.body, - }) - .pipe(Effect.asVoid), + pullRequestNodeId({ ...input, operation: "commentOnPullRequest" }).pipe( + Effect.flatMap((subjectId) => + graphql({ + host: input.host, + operation: "commentOnPullRequest", + query: ADD_COMMENT_GRAPHQL_MUTATION, + variables: { subjectId, body: input.body }, + }), + ), + ), submitReview: (input) => { const { owner, name } = parseRepositorySelector(input.repository); - return github - .execute({ - cwd: input.cwd, - // The whole review is one request, so nothing is visible to anyone else until the - // verdict is sent. The payload travels over stdin for the same reason a comment - // body does: argv is visible in process listings and echoed back in failures. - args: [ - "api", - "--method", - "POST", - "--hostname", - input.host, - `repos/${owner}/${name}/pulls/${input.number}/reviews`, - "--input", - "-", - ], - stdin: buildReviewSubmissionJson({ + // The whole review is one request, so nothing is visible to anyone else until the verdict + // is sent. + return api + .rest({ + host: input.host, + operation: "submitReview", + method: "POST", + path: `repos/${owner}/${name}/pulls/${input.number}/reviews`, + body: buildReviewSubmission({ verdict: input.verdict, body: input.body, comments: input.comments, @@ -2866,8 +2751,8 @@ export const make = Effect.gen(function* () { replyToReviewThread: (input) => graphql({ - cwd: input.cwd, host: input.host, + operation: "replyToReviewThread", query: REVIEW_THREAD_REPLY_GRAPHQL_MUTATION, variables: { threadId: input.threadId, body: input.body }, }), @@ -2883,14 +2768,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, host: input.host, operation: "getPullRequestFilesViewed", - variables: [ - ["-f", `owner=${owner}`], - ["-f", `name=${name}`], - ["-F", `number=${input.number}`], - ...(after === null - ? [] - : ([["-f", `after=${after}`]] as ReadonlyArray)), - ], + variables: { owner, name, number: input.number, after }, query: PULL_REQUEST_FILES_VIEWED_GRAPHQL_QUERY, decode: decodePullRequestFilesViewedJson, }).pipe( @@ -2913,8 +2791,8 @@ export const make = Effect.gen(function* () { return pullRequestNodeId({ ...input, operation: "setPullRequestFilesViewed" }).pipe( Effect.flatMap((pullRequestId) => graphql({ - cwd: input.cwd, host: input.host, + operation: "setPullRequestFilesViewed", query: mutation.query, variables: { pullRequestId, ...mutation.variables }, }), @@ -2924,8 +2802,8 @@ export const make = Effect.gen(function* () { setReviewThreadResolution: (input) => graphql({ - cwd: input.cwd, host: input.host, + operation: "setReviewThreadResolution", query: input.resolved ? RESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION : UNRESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION, @@ -2957,8 +2835,8 @@ export const make = Effect.gen(function* () { return subjectId.pipe( Effect.flatMap((subjectId) => graphql({ - cwd: input.cwd, host: input.host, + operation: "setReaction", query: input.reacted ? ADD_REACTION_GRAPHQL_MUTATION : REMOVE_REACTION_GRAPHQL_MUTATION, variables: { subjectId, content: gitHubReactionContent(input.content) }, }), @@ -2970,8 +2848,8 @@ export const make = Effect.gen(function* () { pullRequestNodeId({ ...input, operation: "updatePullRequest" }).pipe( Effect.flatMap((pullRequestId) => graphql({ - cwd: input.cwd, host: input.host, + operation: "updatePullRequest", query: UPDATE_PULL_REQUEST_GRAPHQL_MUTATION, // A field the caller did not name is left out of the request entirely, so GitHub // keeps the words that are there rather than being asked for an empty one. @@ -3006,8 +2884,8 @@ export const make = Effect.gen(function* () { ), Effect.flatMap((commentId) => graphql({ - cwd: input.cwd, host: input.host, + operation: "updateComment", query: input.kind === "issue-comment" ? UPDATE_ISSUE_COMMENT_GRAPHQL_MUTATION diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 9c9e3bb47648c394780e073d58f48901e4710530..6905b382688008599be9a4a180bdc7de8775cec3 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -6,7 +6,7 @@ import * as Result from "effect/Result"; import type { PullRequestReaction } from "@t3tools/contracts"; import { decodePullRequestDetailJson } from "./gitHubPullRequestJson.ts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts"; @@ -331,7 +331,7 @@ describe("gitHubViewerPermissions", () => { number: 7, }) .pipe( - Effect.provideService(GitHubCli.PinnedGitHubCredential, { + Effect.provideService(GitHubApi.PinnedGitHubCredential, { host: "github.com", token: Redacted.make("credential"), credentialFingerprint: fingerprint, @@ -378,7 +378,7 @@ describe("gitHubViewerPermissions", () => { commits: [], }).pipe( Effect.flatMap((detail) => - GitHubCli.PinnedGitHubCredential.pipe( + GitHubApi.PinnedGitHubCredential.pipe( Effect.map((credential) => ({ ...detail, viewerAccess: { @@ -672,10 +672,9 @@ it.effect("propagates workflow discovery rate limits", () => getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), listWorkflowRunsRequiringApproval: () => Effect.fail( - new GitHubCli.GitHubCliRateLimitError({ - command: "gh", - cwd: "/w", - cause: new Error("rate limited"), + new GitHubApi.GitHubApiRateLimitError({ + host: "github.com", + operation: "listWorkflowRunsRequiringApproval", }), ), getViewerAccess: () => diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 567dd2925f5cdc2a653e5fd3fd92a3c44b7fae54..940b04715c84ea3dcde9f5ac3f14ed6a75f2d2b4 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -101,22 +101,28 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest }; } -/** The CLI tags that mean the tool itself is unusable, rather than one request failing. */ +/** The tags that mean GitHub is out of reach for this account, rather than one request failing. */ export function gitHubProviderFailure( error: GitHubPullRequestCli.GitHubPullRequestCliError, ): PullRequestProviderFailure { - if (error._tag === "GitHubCliUnavailableError") return { reason: "missing-tool" }; - if (error._tag === "GitHubCliAuthenticationError") return { reason: "unauthenticated" }; - if (error._tag === "GitHubCliRateLimitError") - return { - reason: "rate-limited", - ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), - }; - if (error._tag === "SourceControlRateLimitPausedError") { - return { reason: "rate-limited", retryAt: error.retryAt }; + switch (error._tag) { + case "GitHubCliMissingError": + return { reason: "missing-tool" }; + case "GitHubNotSignedInError": + case "GitHubApiAuthenticationError": + return { reason: "unauthenticated" }; + case "GitHubApiRateLimitError": + return { + reason: "rate-limited", + ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), + }; + case "SourceControlRateLimitPausedError": + return { reason: "rate-limited", retryAt: error.retryAt }; + case "GitHubApiNotFoundError": + return { reason: "not-found" }; + default: + return { reason: "failed" }; } - if (error._tag === "GitHubPullRequestNotFoundError") return { reason: "not-found" }; - return { reason: "failed" }; } /** @@ -196,7 +202,7 @@ export const make = Effect.gen(function* () { provider: "github", operation, ...gitHubProviderFailure(error), - detail: error.detail, + detail: error.message, cause: error, }); @@ -224,7 +230,7 @@ export const make = Effect.gen(function* () { .pipe( Effect.matchEffect({ onFailure: (error) => - error._tag === "GitHubCliRateLimitError" || + error._tag === "GitHubApiRateLimitError" || error._tag === "SourceControlRateLimitPausedError" ? Effect.fail(error) : Effect.succeed({ diff --git a/apps/server/src/pullRequest/PullRequestProviderRateLimit.test.ts b/apps/server/src/pullRequest/PullRequestProviderRateLimit.test.ts index 5f837acc82ac5aa771da94e2d1074e4440b1568f..873cc32a2c7f102cdd34046d16c2f9167b790a23 100644 --- a/apps/server/src/pullRequest/PullRequestProviderRateLimit.test.ts +++ b/apps/server/src/pullRequest/PullRequestProviderRateLimit.test.ts @@ -2,7 +2,7 @@ import { assert, it } from "@effect/vitest"; import * as AzureDevOpsCli from "../sourceControl/AzureDevOpsCli.ts"; import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import * as GitLabCli from "../sourceControl/GitLabCli.ts"; import { azureDevOpsProviderFailure } from "./AzureDevOpsPullRequestProvider.ts"; @@ -15,7 +15,7 @@ const cause = new Error("redacted provider failure"); it("classifies rate limits from every pull-request provider", () => { assert.deepStrictEqual( gitHubProviderFailure( - new GitHubCli.GitHubCliRateLimitError({ command: "gh", cwd: "/repo", cause }), + new GitHubApi.GitHubApiRateLimitError({ host: "github.com", operation: "execute" }), ), { reason: "rate-limited" }, ); diff --git a/apps/server/src/pullRequest/PullRequestProviderRegistry.ts b/apps/server/src/pullRequest/PullRequestProviderRegistry.ts index 9e8727ed7a77c30c2093d36eab4aefc8dd3e24c7..60de91d0e99af4b99ffce017b52b39365a2efcc2 100644 --- a/apps/server/src/pullRequest/PullRequestProviderRegistry.ts +++ b/apps/server/src/pullRequest/PullRequestProviderRegistry.ts @@ -5,8 +5,10 @@ import type { SourceControlProviderKind } from "@t3tools/contracts"; import * as AzureDevOpsCli from "../sourceControl/AzureDevOpsCli.ts"; import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; +import * as GitHubCredentials from "../sourceControl/GitHubCredentials.ts"; import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; +import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; import * as GitLabCli from "../sourceControl/GitLabCli.ts"; import * as ForgejoCli from "../sourceControl/ForgejoCli.ts"; import * as ForgejoPullRequestProvider from "./ForgejoPullRequestProvider.ts"; @@ -60,8 +62,14 @@ export const make = Effect.map( export const layer = Layer.effect(PullRequestProviderRegistry, make).pipe( Layer.provide( GitHubPullRequestCli.layer.pipe( - Layer.provide(GitHubCli.layer), - Layer.provide(GitHubGraphQlBudget.layer), + Layer.provide( + GitHubApi.layer.pipe( + Layer.provide(GitHubCredentials.layer), + // The same layers GitHubCli merges, so both share one budget and one pause per host. + Layer.provide(GitHubGraphQlBudget.layer), + Layer.provide(SourceControlRateLimit.layer), + ), + ), ), ), Layer.provide(GitLabPullRequestCli.layer.pipe(Layer.provide(GitLabCli.layer))), diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts index 7ef2acebad94fafa6196a5033025dbc6e4327a34..79048b1d74535823194a7c0816755bca84ffd8ad 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.test.ts @@ -4,11 +4,10 @@ import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Schema from "effect/Schema"; import * as Redacted from "effect/Redacted"; -import { ChildProcessSpawner } from "effect/process"; import type { PullRequestCheck } from "@t3tools/contracts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; -import { makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; +import { KnownWorkflowRuns, makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); @@ -27,31 +26,46 @@ it.effect( let changed = ""; let reads = 0; const requests: string[] = []; + const known: Array = []; const revalidate = yield* makeChecksRevalidator.pipe( Effect.provide( - Layer.mock(GitHubCli.GitHubCli)({ - execute: ({ args }) => + Layer.mock(GitHubApi.GitHubApi)({ + rest: ({ path, ifNoneMatch }) => Effect.sync(() => { - const endpoint = args[1]!; - requests.push(endpoint); - const head = endpoint.endsWith("/pulls/1"); + requests.push(path); + const head = path.endsWith("/pulls/1"); const modified = - !args.includes("-H") || (endpoint.includes(changed) && changed !== ""); - const next = endpoint.includes("check-runs") && endpoint.endsWith("page=1"); + ifNoneMatch === undefined || (path.includes(changed) && changed !== ""); + const next = path.includes("check-runs") && path.endsWith("page=1"); + const runs = path.includes("/actions/runs"); return { - exitCode: ChildProcessSpawner.ExitCode(modified ? 0 : 1), - stdout: modified - ? `HTTP/2.0 200 OK\r\nEtag: "${sha}-${changed}"\r\n${next ? 'Link: ; rel="next"\r\n' : ""}\r\n${head ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 2 }) : ""}` - : "HTTP/2.0 304 Not Modified\r\n\r\n", - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, + status: modified ? 200 : 304, + headers: modified + ? { + etag: `"${sha}-${changed}"`, + ...(next ? { link: '; rel="next"' } : {}), + } + : {}, + body: !modified + ? "" + : head + ? encodeJson({ head: { sha, repo: { id: 2 } }, base: { repo: { id: 1 } } }) + : runs + ? encodeJson({ + workflow_runs: [ + { id: 9, status: "completed", conclusion: "action_required" }, + ], + }) + : "{}", + truncated: false, + invalidUtf8: false, }; }), }), ), ); - const read = Effect.sync(() => { + const read = Effect.gen(function* () { + known.push(yield* KnownWorkflowRuns); reads++; return { state: "open" as const, @@ -64,10 +78,15 @@ it.effect( }); const poll = (identity = credential) => revalidate(reference, read).pipe( - Effect.provideService(GitHubCli.PinnedGitHubCredential, identity), + Effect.provideService(GitHubApi.PinnedGitHubCredential, identity), ); yield* poll(); expect(reads).toBe(1); + // The fork's runs were just confirmed, so the read is handed them instead of listing them. + expect(known[0]).toEqual({ + headSha: sha, + runs: [{ id: 9, status: "completed", conclusion: "action_required" }], + }); requests.length = 0; yield* poll(); expect(reads).toBe(1); @@ -113,27 +132,32 @@ it.effect("does not retain failed or incomplete reads, and supports hosts withou let reads = 0; const revalidate = yield* makeChecksRevalidator.pipe( Effect.provide( - Layer.mock(GitHubCli.GitHubCli)({ - execute: ({ args }) => + Layer.mock(GitHubApi.GitHubApi)({ + rest: ({ path, ifNoneMatch }) => unavailable ? Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/repo", - cause: undefined, - httpStatus: 502, + new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "revalidateChecks", + status: 502, }), ) - : Effect.succeed({ - exitCode: ChildProcessSpawner.ExitCode(0), - stdout: - args.includes("-H") && etags - ? "HTTP/2.0 304 Not Modified\n\n" - : `HTTP/2.0 200 OK\n${etags ? 'Etag: "one"\n' : ""}\n${args[1]!.endsWith("/pulls/1") ? encodeJson({ head: { sha }, base: { repo: { id: 1 } }, headRepositoryId: 1 }) : ""}`, - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, - }), + : Effect.succeed( + ifNoneMatch !== undefined && etags + ? { status: 304, headers: {}, body: "", truncated: false, invalidUtf8: false } + : { + status: 200, + headers: etags ? { etag: '"one"' } : {}, + body: path.endsWith("/pulls/1") + ? encodeJson({ + head: { sha, repo: { id: 1 } }, + base: { repo: { id: 1 } }, + }) + : "{}", + truncated: false, + invalidUtf8: false, + }, + ), }), ), ); @@ -141,7 +165,11 @@ it.effect("does not retain failed or incomplete reads, and supports hosts withou reads++; return fail ? Effect.fail( - new GitHubCli.GitHubCliCommandError({ command: "gh", cwd: "/repo", cause: undefined }), + new GitHubApi.GitHubApiResponseError({ + host: "github.com", + operation: "read", + status: 500, + }), ) : Effect.succeed({ state: "open" as const, @@ -152,7 +180,7 @@ it.effect("does not retain failed or incomplete reads, and supports hosts withou }); const poll = () => revalidate(reference, read).pipe( - Effect.provideService(GitHubCli.PinnedGitHubCredential, credential), + Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), ); yield* poll().pipe(Effect.flip); fail = false; @@ -190,15 +218,13 @@ it.effect("falls back when REST checks are unavailable without retrying unsuppor let reads = 0; const revalidate = yield* makeChecksRevalidator.pipe( Effect.provide( - Layer.mock(GitHubCli.GitHubCli)({ - execute: () => { + Layer.mock(GitHubApi.GitHubApi)({ + rest: () => { probes++; return Effect.fail( - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: "/repo", - cause: undefined, - httpStatus: 404, + new GitHubApi.GitHubApiNotFoundError({ + host: "github.com", + operation: "revalidateChecks", }), ); }, @@ -211,7 +237,7 @@ it.effect("falls back when REST checks are unavailable without retrying unsuppor }); for (let tick = 0; tick < 2; tick++) yield* revalidate(reference, read).pipe( - Effect.provideService(GitHubCli.PinnedGitHubCredential, credential), + Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), ); expect(probes).toBe(1); expect(reads).toBe(2); diff --git a/apps/server/src/pullRequest/gitHubConditionalChecks.ts b/apps/server/src/pullRequest/gitHubConditionalChecks.ts index 04d2f8706c52be277479d1fbcc9ca1c9c0b1ad4f..8172bf544074c6aa7776bfaf2a35869294caeebe 100644 --- a/apps/server/src/pullRequest/gitHubConditionalChecks.ts +++ b/apps/server/src/pullRequest/gitHubConditionalChecks.ts @@ -1,32 +1,63 @@ import * as Clock from "effect/Clock"; import * as Cache from "effect/Cache"; +import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; +import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; import * as Semaphore from "effect/Semaphore"; import { PositiveInt, type PullRequestChecks } from "@t3tools/contracts"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import type { GitHubPullRequestDetail } from "./gitHubPullRequestJson.ts"; import type { GitHubPullRequestCliError } from "./GitHubPullRequestCli.ts"; import type { ProviderRepositoryRef } from "./PullRequestProvider.ts"; -const decodeHeadSchema = Schema.Struct({ - head: Schema.Struct({ sha: Schema.String.check(Schema.isPattern(/^[a-f0-9]{40,64}$/)) }), +const HeadSchema = Schema.Struct({ + head: Schema.Struct({ + sha: Schema.String.check(Schema.isPattern(/^[a-f0-9]{40,64}$/)), + // Null once the fork a pull request came from has been deleted. + repo: Schema.NullOr(Schema.Struct({ id: PositiveInt })), + }), base: Schema.Struct({ repo: Schema.Struct({ id: PositiveInt }) }), - headRepositoryId: Schema.NullOr(PositiveInt), }); -const decodeHead = Schema.decodeUnknownEffect(Schema.fromJsonString(decodeHeadSchema)); +const decodeHead = Schema.decodeUnknownEffect(Schema.fromJsonString(HeadSchema)); -type Validator = { etag: string | undefined; next: boolean }; +const WorkflowRunSchema = Schema.Struct({ + id: Schema.Int, + name: Schema.optional(Schema.NullOr(Schema.String)), + html_url: Schema.optional(Schema.NullOr(Schema.String)), + status: Schema.optional(Schema.NullOr(Schema.String)), + conclusion: Schema.optional(Schema.NullOr(Schema.String)), + head_branch: Schema.optional(Schema.NullOr(Schema.String)), +}); +const decodeWorkflowRuns = Schema.decodeUnknownOption( + Schema.fromJsonString(Schema.Struct({ workflow_runs: Schema.Array(WorkflowRunSchema) })), +); + +export type KnownWorkflowRun = typeof WorkflowRunSchema.Type; + +/** + * Every `pull_request` workflow run of one head, as the revalidator just confirmed them against + * GitHub. A read running under it filters these rather than asking for the runs again. + */ +export const KnownWorkflowRuns = Context.Reference<{ + readonly headSha: string; + readonly runs: ReadonlyArray; +} | null>("t3/pullRequest/KnownWorkflowRuns", { defaultValue: () => null }); + +type Validator = { etag: string | undefined; next: boolean; body: string }; + +/** A host that answers these with one of them has no conditional REST for it at all. */ +const UNSUPPORTED_STATUSES = new Set([404, 405, 501]); export const makeChecksRevalidator = Effect.gen(function* () { - const github = yield* GitHubCli.GitHubCli; + const api = yield* GitHubApi.GitHubApi; const entries = yield* Cache.makeWith( (_key: string) => Effect.sync(() => ({ gate: Semaphore.makeUnsafe(1), validators: new Map(), - head: null as Schema.Schema.Type | null, + head: null as typeof HeadSchema.Type | null, value: null as PullRequestChecks | null, readAt: 0, supported: true, @@ -43,71 +74,64 @@ export const makeChecksRevalidator = Effect.gen(function* () { >, ) => Effect.gen(function* () { - const credential = yield* GitHubCli.PinnedGitHubCredential; + const credential = yield* GitHubApi.PinnedGitHubCredential; if (credential === null) return yield* read; const key = `${credential.credentialFingerprint}\0${input.host}\0${input.repository}\0${input.number}`; const entry = yield* Cache.get(entries, key); return yield* entry.gate.withPermit( Effect.gen(function* () { if (!entry.supported) return yield* read; - const fail = () => - new GitHubCli.GitHubCliCommandError({ - command: "gh", - cwd: input.cwd, - cause: new Error("GitHub returned an invalid conditional checks response."), + const fail = (status: number) => + new GitHubApi.GitHubApiResponseError({ + host: input.host, + operation: "revalidateChecks", + status, }); + /** Whether the endpoint has a next page, or null where the host cannot say. */ const get = (endpoint: string, head = false) => Effect.gen(function* () { const previous = entry.validators.get(endpoint); - const result = yield* github - .execute({ - cwd: input.cwd, - args: [ - "api", - endpoint, - "--hostname", - input.host, - "--include", - ...(head - ? [ - "--jq", - "{head:{sha:.head.sha},base:{repo:{id:.base.repo.id}},headRepositoryId:.head.repo.id}", - ] - : ["--silent"]), - ...(previous?.etag ? ["-H", `If-None-Match: ${previous.etag}`] : []), - ], - acceptNotModified: true, + const response = yield* api + .rest({ + host: input.host, + operation: "revalidateChecks", + path: endpoint, + ...(previous?.etag ? { ifNoneMatch: previous.etag } : {}), }) .pipe( Effect.catchTags({ - GitHubCliCommandError: (error) => + GitHubApiNotFoundError: () => Effect.sync(() => { - entry.supported = ![404, 405, 501].includes(error.httpStatus ?? 0); + entry.supported = false; + return null; + }), + GitHubApiResponseError: (error) => + Effect.sync(() => { + entry.supported = !UNSUPPORTED_STATUSES.has(error.status); return null; }), }), ); - if (result === null) { + if (response === null) { entry.value = null; return null; } - if (result.stdoutTruncated || result.stdoutInvalidUtf8) return yield* fail(); - const split = result.stdout.search(/\r?\n\r?\n/); - const headers = split < 0 ? result.stdout : result.stdout.slice(0, split); - const status = /^HTTP\/\S+ (\d+)/.exec(headers)?.[1]; - if (status === "304" && previous) return previous.next; - if (status !== "200") return yield* fail(); + if (response.status === 304 && previous) return previous.next; + if (response.status !== 200 || response.truncated) { + return yield* fail(response.status); + } entry.value = null; if (head) { - const decoded = yield* decodeHead(result.stdout.slice(split).trim()).pipe( - Effect.mapError(fail), + const decoded = yield* decodeHead(response.body).pipe( + Effect.mapError(() => fail(response.status)), ); if (entry.head?.head.sha !== decoded.head.sha) entry.validators.clear(); entry.head = decoded; } const validator = { - etag: /^etag:\s*(.+)$/im.exec(headers)?.[1]?.trim(), - next: /^link:.*rel="next"/im.test(headers), + etag: response.headers["etag"]?.trim(), + next: /rel="next"/.test(response.headers["link"] ?? ""), + body: response.body, }; entry.validators.set(endpoint, validator); return validator.next; @@ -115,27 +139,44 @@ export const makeChecksRevalidator = Effect.gen(function* () { const root = `repos/${input.repository}`; if ((yield* get(`${root}/pulls/${input.number}`, true)) === null) return yield* read; const head = entry.head; - if (head === null) return yield* fail(); + if (head === null) return yield* fail(200); + const runsEndpoint = + head.head.repo?.id !== head.base.repo.id + ? `${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request` + : null; const endpoints = [ `${root}/commits/${head.head.sha}/check-runs?filter=all`, `${root}/commits/${head.head.sha}/status`, - ...(head.headRepositoryId !== head.base.repo.id - ? [`${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request`] - : []), + ...(runsEndpoint === null ? [] : [runsEndpoint]), ]; + const runPages: Array = []; for (const endpoint of endpoints) { for (let page = 1; ; page++) { - if (page > 100) return yield* fail(); - const next = yield* get( - `${endpoint}${endpoint.includes("?") ? "&" : "?"}per_page=100&page=${page}`, - ); + if (page > 100) return yield* fail(200); + const paged = `${endpoint}${endpoint.includes("?") ? "&" : "?"}per_page=100&page=${page}`; + const next = yield* get(paged); if (next === null) return yield* read; + if (endpoint === runsEndpoint) runPages.push(paged); if (!next) break; } } const now = yield* Clock.currentTimeMillis; if (entry.value !== null && now - entry.readAt < 5 * 60_000) return entry.value; - const fresh = yield* read; + // Every page of the head's runs was just confirmed current, so the read filters them + // rather than listing the runs a second time. + const pages = runPages.map((endpoint) => + decodeWorkflowRuns(entry.validators.get(endpoint)?.body ?? ""), + ); + const known = + runsEndpoint !== null && pages.every(Option.isSome) + ? { + headSha: head.head.sha, + runs: pages.flatMap((page) => + Option.isSome(page) ? page.value.workflow_runs : [], + ), + } + : null; + const fresh = yield* read.pipe(Effect.provideService(KnownWorkflowRuns, known)); const value = { state: fresh.state, checks: fresh.checks, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index b0225545c1f43e4cd6c5aa51769344f2ade88894..2e008fe4c6365ab531e6ec0039e81128b1cf68e5 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -6,10 +6,10 @@ import { buildPullRequestWatchFingerprintsGraphQlQuery, decodePullRequestSummariesJson, decodePullRequestWatchFingerprintsJson, - buildReviewSubmissionJson, + buildReviewSubmission, buildPullRequestStackMembershipsGraphQlQuery, decodePullRequestStackMembershipsJson, - buildReviewerRequestJson, + buildReviewerRequest, buildSetFilesViewedGraphQlMutation, decodeBaseComparisonJson, decodePullRequestActivityJson, @@ -25,26 +25,50 @@ import { decodeReviewThreadCommentsJson, decodeReviewThreadsJson, decodeViewerPermissionsJson, - decodeWorkflowRunApprovalsJson, + decodeWorkflowRunsJson, reviewThreadConversation, REVIEW_THREADS_GRAPHQL_QUERY, pullRequestCoreGraphQlQuery, pullRequestSearchGraphQlQuery, } from "./gitHubPullRequestJson.ts"; +/** + * Rows as a repository's own GraphQL list answers them. Entries may use flat shapes for brevity: + * `reviewRequests` and `latestReviews` as arrays, and `checks` as the rollup state. + */ function listJson(entries: ReadonlyArray>): string { - return JSON.stringify( - entries.map((entry) => ({ - number: 1, - title: "Add the pull requests page", - url: "https://github.com/pingdotgg/t3code/pull/1", - headRefName: "feat/page", - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - ...entry, - })), - ); + return JSON.stringify({ + data: { + repository: { + pullRequests: { + pageInfo: { hasNextPage: false }, + nodes: entries.map(({ reviewRequests, latestReviews, checks, ...entry }) => ({ + number: 1, + title: "Add the pull requests page", + url: "https://github.com/pingdotgg/t3code/pull/1", + headRefName: "feat/page", + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + ...(reviewRequests === undefined + ? {} + : { + reviewRequests: { + nodes: (reviewRequests as ReadonlyArray).map((requestedReviewer) => ({ + requestedReviewer, + })), + }, + }), + ...(latestReviews === undefined ? {} : { latestReviews: { nodes: latestReviews } }), + ...(checks === undefined + ? {} + : { commits: { nodes: [{ commit: { statusCheckRollup: { state: checks } } }] } }), + ...entry, + })), + }, + }, + }, + }); } function expectSuccess(result: Result.Result): A { @@ -136,38 +160,15 @@ describe("pull request list decoding", () => { ]); }); - it("rolls the head commit's checks up to the one word a row has space for", () => { + it("rolls the head commit's rollup up to the one word a row has space for", () => { const batch = expectSuccess( decodePullRequestListJson( listJson([ - // A failure outranks a run still going, and a completed run has to be read through its - // conclusion rather than its status. - { - statusCheckRollup: [ - { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, - { name: "build", status: "IN_PROGRESS" }, - { name: "test", status: "COMPLETED", conclusion: "FAILURE" }, - ], - }, - { - statusCheckRollup: [ - { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, - { name: "build", status: "QUEUED" }, - ], - }, - { statusCheckRollup: [{ name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }] }, - // A commit status reports one `state` and no `status` at all. - { statusCheckRollup: [{ context: "ci/legacy", state: "ERROR" }] }, - // Neither a pass, a failure nor a wait is no verdict rather than a green tick. - { statusCheckRollup: [{ name: "lint", status: "COMPLETED", conclusion: "SKIPPED" }] }, - // Cancelled reads as failing here and in the detail header, so the two never flap. - { - statusCheckRollup: [ - { name: "lint", status: "COMPLETED", conclusion: "SUCCESS" }, - { name: "test", status: "COMPLETED", conclusion: "CANCELLED" }, - ], - }, - { statusCheckRollup: [] }, + { checks: "FAILURE" }, + { checks: "PENDING" }, + { checks: "SUCCESS" }, + { checks: "ERROR" }, + { checks: "EXPECTED" }, {}, ]), ), @@ -177,16 +178,15 @@ describe("pull request list decoding", () => { "pending", "passing", "failing", - null, - "failing", - null, + "pending", null, ]); }); it("skips malformed entries but still counts them, so paging does not stop early", () => { - const raw = `[${listJson([{}]).slice(1, -1)},{"number":"not-a-number"}]`; - const batch = expectSuccess(decodePullRequestListJson(raw)); + const batch = expectSuccess( + decodePullRequestListJson(listJson([{}, { number: "not-a-number" }])), + ); expect(batch.items).toHaveLength(1); expect(batch.rawCount).toBe(2); }); @@ -279,6 +279,43 @@ describe("pull request detail decoding", () => { ], }); + /** The same conversation as the GraphQL activity query answers it. */ + const activityJson = (raw: Record) => + JSON.stringify({ + data: { + repository: { + pullRequest: { + comments: { nodes: raw["comments"] ?? [] }, + reviews: { nodes: raw["reviews"] ?? [] }, + commits: { + nodes: ((raw["commits"] ?? []) as ReadonlyArray>).map( + ({ authors, ...commit }) => ({ + commit: { + ...commit, + authors: { + nodes: ((authors ?? []) as ReadonlyArray>).map( + ({ login, ...author }) => ({ + ...author, + ...(login === undefined ? {} : { user: { login } }), + }), + ), + }, + }, + }), + ), + }, + }, + }, + }, + }); + const activity = (raw: Record) => + Result.map(decodePullRequestActivityJson(activityJson(raw)), (page) => ({ + comments: page.remarks.toSorted((left, right) => + left.createdAt.localeCompare(right.createdAt), + ), + commits: page.commits ?? [], + })); + it("maps check-run status and commit-status state onto one vocabulary", () => { const detail = expectSuccess(decodePullRequestDetailJson(detailJson)); expect(detail.checks.map((check) => [check.name, check.status])).toEqual([ @@ -333,13 +370,15 @@ describe("pull request detail decoding", () => { it("decodes workflow runs that can be approved", () => { expect( expectSuccess( - decodeWorkflowRunApprovalsJson( - JSON.stringify([ - { databaseId: 10, workflowName: "contributor tests", url: "https://example.com/10" }, - { databaseId: 11, workflowName: null, url: null }, - ]), + decodeWorkflowRunsJson( + JSON.stringify({ + workflow_runs: [ + { id: 10, name: "contributor tests", html_url: "https://example.com/10" }, + { id: 11, name: null, html_url: null }, + ], + }), ), - ), + ).runs, ).toEqual([ { id: 10, name: "contributor tests", url: "https://example.com/10" }, { id: 11, name: "Workflow run 11", url: null }, @@ -398,14 +437,14 @@ describe("pull request detail decoding", () => { }); it("merges reviews with comments in time order and keeps a bodyless approval", () => { - const detail = expectSuccess(decodePullRequestActivityJson(detailJson)); + const detail = expectSuccess(activity(JSON.parse(detailJson) as Record)); // r2 approved without writing anything, which is still the event worth seeing. expect(detail.comments.map((comment) => comment.id)).toEqual(["r1", "c1", "r2"]); expect(detail.comments.at(-1)?.reviewState).toBe("APPROVED"); }); it("keeps every attributed commit author, including an unlinked signature", () => { - const detail = expectSuccess(decodePullRequestActivityJson(detailJson)); + const detail = expectSuccess(activity(JSON.parse(detailJson) as Record)); expect(detail.commits[0]?.authors).toEqual([ { login: "octocat", name: "Octo Cat", avatarUrl: null }, { login: "Pair Author", name: "Pair Author", avatarUrl: null }, @@ -415,22 +454,20 @@ describe("pull request detail decoding", () => { it("drops the bodyless review GitHub opens to hold line comments", () => { const raw = JSON.parse(detailJson) as Record; const detail = expectSuccess( - decodePullRequestActivityJson( - JSON.stringify({ - ...raw, - reviews: [ - // What a reviewer leaving inline comments produces: a container with a state but - // nothing to read. Its comments come from the review threads instead. - { id: "r4", body: "", state: "COMMENTED", submittedAt: "2026-07-07T00:00:00Z" }, - { - id: "r5", - body: "Looks good.", - state: "COMMENTED", - submittedAt: "2026-07-08T00:00:00Z", - }, - ], - }), - ), + activity({ + ...raw, + reviews: [ + // What a reviewer leaving inline comments produces: a container with a state but + // nothing to read. Its comments come from the review threads instead. + { id: "r4", body: "", state: "COMMENTED", submittedAt: "2026-07-07T00:00:00Z" }, + { + id: "r5", + body: "Looks good.", + state: "COMMENTED", + submittedAt: "2026-07-08T00:00:00Z", + }, + ], + }), ); expect(detail.comments.map((comment) => comment.id)).toEqual(["c1", "r5"]); @@ -441,12 +478,10 @@ describe("pull request detail decoding", () => { (state) => { const raw = JSON.parse(detailJson) as Record; const detail = expectSuccess( - decodePullRequestActivityJson( - JSON.stringify({ - ...raw, - reviews: [{ id: "r6", body: "", state, submittedAt: "2026-07-07T00:00:00Z" }], - }), - ), + activity({ + ...raw, + reviews: [{ id: "r6", body: "", state, submittedAt: "2026-07-07T00:00:00Z" }], + }), ); expect(detail.comments.map((comment) => comment.id)).toContain("r6"); @@ -456,12 +491,10 @@ describe("pull request detail decoding", () => { it("drops a review that carries neither a body nor a state", () => { const raw = JSON.parse(detailJson) as Record; const detail = expectSuccess( - decodePullRequestActivityJson( - JSON.stringify({ - ...raw, - reviews: [{ id: "r3", body: " ", submittedAt: "2026-07-07T00:00:00Z" }], - }), - ), + activity({ + ...raw, + reviews: [{ id: "r3", body: " ", submittedAt: "2026-07-07T00:00:00Z" }], + }), ); expect(detail.comments.map((comment) => comment.id)).toEqual(["c1"]); }); @@ -1432,18 +1465,16 @@ describe("reviewer candidate decoding", () => { describe("reviewer request payload", () => { it("sends people and teams in the two lists GitHub keeps them in", () => { expect( - JSON.parse( - buildReviewerRequestJson([ - { id: "octocat", kind: "user" }, - { id: "reviewers", kind: "team" }, - { id: "hubot", kind: "user" }, - ]), - ), + buildReviewerRequest([ + { id: "octocat", kind: "user" }, + { id: "reviewers", kind: "team" }, + { id: "hubot", kind: "user" }, + ]), ).toEqual({ reviewers: ["octocat", "hubot"], team_reviewers: ["reviewers"] }); }); it("sends both lists even where one of them is empty, which is what GitHub reads", () => { - expect(JSON.parse(buildReviewerRequestJson([{ id: "octocat", kind: "user" }]))).toEqual({ + expect(buildReviewerRequest([{ id: "octocat", kind: "user" }])).toEqual({ reviewers: ["octocat"], team_reviewers: [], }); @@ -1452,24 +1483,22 @@ describe("reviewer request payload", () => { describe("review submission payload", () => { it("sends the verdict, the summary and every line comment in one body", () => { - const payload = JSON.parse( - buildReviewSubmissionJson({ - verdict: "request-changes", - body: "Two things.", - comments: [ - { - path: "src/a.ts", - position: { kind: "added", newLine: 12 }, - body: "rename this", - }, - { - path: "src/b.ts", - position: { kind: "deleted", oldLine: 3 }, - body: "why remove?", - }, - ], - }), - ) as Record; + const payload = buildReviewSubmission({ + verdict: "request-changes", + body: "Two things.", + comments: [ + { + path: "src/a.ts", + position: { kind: "added", newLine: 12 }, + body: "rename this", + }, + { + path: "src/b.ts", + position: { kind: "deleted", oldLine: 3 }, + body: "why remove?", + }, + ], + }); expect(payload).toEqual({ event: "REQUEST_CHANGES", body: "Two things.", @@ -1481,9 +1510,11 @@ describe("review submission payload", () => { }); it("sends an approval with no words and no comments", () => { - expect( - JSON.parse(buildReviewSubmissionJson({ verdict: "approve", body: "", comments: [] })), - ).toEqual({ event: "APPROVE", body: "", comments: [] }); + expect(buildReviewSubmission({ verdict: "approve", body: "", comments: [] })).toEqual({ + event: "APPROVE", + body: "", + comments: [], + }); }); }); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 0a2f5fe4a60459956f6b411b89dd0ddcddf0dc63..1409e92a1fadf809b683214dcdcb0a5313365a1e 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -155,6 +155,9 @@ const RawSearchItemSchema = Schema.Struct({ latestReviews: Schema.optional( Schema.NullOr(Schema.Struct({ nodes: Schema.Array(Schema.NullOr(RawLatestReviewSchema)) })), ), + /** Asked for by the per-repository listing, and left out of the cross-repository search. */ + additions: Schema.optional(Schema.Int), + deletions: Schema.optional(Schema.Int), createdAt: Schema.String, updatedAt: Schema.String, mergedAt: Schema.optional(Schema.NullOr(Schema.String)), @@ -167,7 +170,14 @@ const RawSearchItemSchema = Schema.Struct({ Schema.Array( Schema.NullOr( Schema.Struct({ - requestedReviewer: Schema.optional(Schema.NullOr(RawActorSchema)), + requestedReviewer: Schema.optional( + Schema.NullOr( + Schema.Struct({ + ...RawActorSchema.fields, + slug: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), }), ), ), @@ -215,14 +225,27 @@ const RawSearchItemSchema = Schema.Struct({ ), }); +const RawRowConnectionSchema = Schema.Struct({ + pageInfo: Schema.optional( + Schema.NullOr( + Schema.Struct({ + hasNextPage: Schema.Boolean, + endCursor: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), + // Row by row: a node that is not a pull request — or one field GitHub changes — is skipped + // rather than blanking every repository at once. + nodes: Schema.optional(Schema.NullOr(Schema.Array(Schema.Unknown))), +}); + const RawSearchSchema = Schema.Struct({ + data: Schema.Struct({ search: RawRowConnectionSchema }), +}); + +const RawRepositoryPullRequestsSchema = Schema.Struct({ data: Schema.Struct({ - search: Schema.Struct({ - pageInfo: Schema.optional(Schema.NullOr(Schema.Struct({ hasNextPage: Schema.Boolean }))), - // Row by row, like the listing's own: a node that is not a pull request — or one field - // GitHub changes — is skipped rather than blanking every repository at once. - nodes: Schema.optional(Schema.NullOr(Schema.Array(Schema.Unknown))), - }), + repository: Schema.NullOr(Schema.Struct({ pullRequests: RawRowConnectionSchema })), }), }); @@ -414,31 +437,103 @@ const RawDetailSchema = Schema.Struct({ ), }); -const RawWorkflowRunApprovalSchema = Schema.Struct({ - databaseId: Schema.Int, - workflowName: Schema.optional(Schema.NullOr(Schema.String)), - url: Schema.optional(Schema.NullOr(Schema.String)), +/** `GET /repos/{owner}/{repo}/actions/runs`, one page of it. */ +const RawWorkflowRunsSchema = Schema.Struct({ + workflow_runs: Schema.Array( + Schema.Struct({ + id: Schema.Int, + name: Schema.optional(Schema.NullOr(Schema.String)), + html_url: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), }); -const RawPullRequestHeadSchema = Schema.Struct({ - number: Schema.Int, - headRefOid: Schema.String, - isCrossRepository: Schema.optional(Schema.Boolean), - headRepositoryOwner: Schema.optional(Schema.NullOr(Schema.Struct({ login: Schema.String }))), +const RawPullRequestHeadsSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.NullOr( + Schema.Struct({ + pullRequests: Schema.Struct({ + pageInfo: Schema.optional(RawPageInfoSchemaFields()), + nodes: Schema.Array( + Schema.Struct({ + number: Schema.Int, + headRefOid: Schema.String, + isCrossRepository: Schema.optional(Schema.Boolean), + headRepositoryOwner: Schema.optional( + Schema.NullOr(Schema.Struct({ login: Schema.String })), + ), + }), + ), + }), + }), + ), + }), }); +const RawActivityConnection = (node: S) => + Schema.optional( + Schema.NullOr( + Schema.Struct({ + pageInfo: Schema.optional(RawPageInfoSchemaFields()), + nodes: Schema.Array(node), + }), + ), + ); + const RawActivitySchema = Schema.Struct({ - author: Schema.optional(Schema.NullOr(RawActorSchema)), - comments: Schema.optional(Schema.Array(RawCommentSchema)), - reviews: Schema.optional(Schema.Array(RawReviewSchema)), - commits: Schema.optional(Schema.Array(RawCommitSchema)), + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + author: Schema.optional(Schema.NullOr(RawActorSchema)), + comments: RawActivityConnection(RawCommentSchema), + reviews: RawActivityConnection(RawReviewSchema), + commits: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ + commit: Schema.Struct({ + oid: Schema.String, + messageHeadline: Schema.optional(Schema.String), + committedDate: Schema.String, + authors: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ + email: Schema.optional(Schema.NullOr(Schema.String)), + name: Schema.optional(Schema.NullOr(Schema.String)), + user: Schema.optional( + Schema.NullOr( + Schema.Struct({ + login: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), + }), + ), + }), + ), + ), + }), + }), + ), + }), + ), + ), + }), + }), + }), }); /** Where a connection carries on from, which is what every paged read below follows. */ -const RawPageInfoSchema = Schema.Struct({ - hasNextPage: Schema.optional(Schema.Boolean), - endCursor: Schema.optional(Schema.NullOr(Schema.String)), -}); +function RawPageInfoSchemaFields() { + return Schema.Struct({ + hasNextPage: Schema.optional(Schema.Boolean), + endCursor: Schema.optional(Schema.NullOr(Schema.String)), + }); +} +const RawPageInfoSchema = RawPageInfoSchemaFields(); /** * What GitHub says the viewer may do with a pull request. Both are optional so that an install @@ -707,19 +802,25 @@ export function decodeActorAvatarsJson( return Result.succeed(avatarsByLogin); } -export const PULL_REQUEST_LIST_JSON_FIELDS = - "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,latestReviews,labels,statusCheckRollup"; - -export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,isCrossRepository,headRepositoryOwner,headRefOid,autoMergeRequest`; - /** * Pull refs let the comparison share the detail read without first resolving a fork branch. * `isRequired` is asked for on github.com only: an older Enterprise server may not know it, and * an unknown field fails the whole read. */ -export const pullRequestCoreGraphQlQuery = (host: string) => { +function checkContextNodesSelection(host: string): string { const required = host.toLowerCase() === "github.com" ? " isRequired(pullRequestNumber: $number)" : ""; + return `nodes { + __typename + ... on StatusContext { context state targetUrl createdAt description${required} } + ... on CheckRun { + name status conclusion startedAt completedAt detailsUrl${required} + checkSuite { workflowRun { workflow { name } } } + } + }`; +} + +export const pullRequestCoreGraphQlQuery = (host: string) => { return `query($owner: String!, $name: String!, $number: Int!, $headRef: String!) { repository(owner: $owner, name: $name) { mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission @@ -738,14 +839,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => { labels(first: 100) { nodes { name color } } commits(last: 1) { nodes { commit { statusCheckRollup { contexts(first: 100) { - nodes { - __typename - ... on StatusContext { context state targetUrl createdAt description${required} } - ... on CheckRun { - name status conclusion startedAt completedAt detailsUrl${required} - checkSuite { workflowRun { workflow { name } } } - } - } + ${checkContextNodesSelection(host)} pageInfo { hasNextPage } } } } } } @@ -791,7 +885,29 @@ export function decodePullRequestPreviewJson( })); } -export const PULL_REQUEST_ACTIVITY_JSON_FIELDS = "author,comments,reviews,commits"; +/** + * The conversation a pull request carries outside its review threads: who opened it, its issue + * comments, its reviews and its newest commits. The two remark connections page independently, so + * each is switched on only while it has more to give; the first read asks for everything. + */ +export const PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $head: Boolean!, $withComments: Boolean!, $commentsAfter: String, $withReviews: Boolean!, $reviewsAfter: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + author @include(if: $head) { __typename login avatarUrl ... on User { name } } + commits(last: 100) @include(if: $head) { + nodes { commit { oid messageHeadline committedDate authors(first: 10) { nodes { name email user { login } } } } } + } + comments(first: 100, after: $commentsAfter) @include(if: $withComments) { + pageInfo { hasNextPage endCursor } + nodes { id body createdAt lastEditedAt url author { __typename login avatarUrl ... on User { name } } } + } + reviews(first: 100, after: $reviewsAfter) @include(if: $withReviews) { + pageInfo { hasNextPage endCursor } + nodes { id body state submittedAt lastEditedAt url author { __typename login avatarUrl ... on User { name } } } + } + } + } +}`; /** GitHub's own ceiling on a connection page, which is what both thread reads ask for. */ const GRAPHQL_PAGE_SIZE = 100; @@ -818,13 +934,45 @@ export const PULL_REQUEST_SEARCH_MAX_ROWS = GRAPHQL_PAGE_SIZE; * than twenty labels shows twenty, and one that has asked more than twenty people for a review * is already past what a row can say. */ -export function pullRequestSearchGraphQlQuery(rows: number, includeStacks = false): string { - return `query($q: String!) { - search(query: $q, type: ISSUE, first: ${Math.min(Math.max(Math.trunc(rows), 1), PULL_REQUEST_SEARCH_MAX_ROWS)}) { - pageInfo { hasNextPage } +export function pullRequestSearchGraphQlQuery( + rows: number, + includeStacks = false, + includeStats = false, +): string { + return `query($q: String!, $after: String) { + search(query: $q, type: ISSUE, first: ${pageRows(rows)}, after: $after) { + pageInfo { hasNextPage endCursor } nodes { ... on PullRequest { - ${includeStacks ? "stack { number size baseRefName } stackEntry { position }" : ""} + ${pullRequestRowSelection(includeStacks, includeStats)} + } + } + } +}`; +} + +/** + * A repository's pull requests without search, newest created first: the order `gh pr list` lists + * in when it does not search, for a repository GitHub's search index does not cover. + */ +export function pullRequestListGraphQlQuery(rows: number, includeStacks = false): string { + return `query($owner: String!, $name: String!, $states: [PullRequestState!], $after: String) { + repository(owner: $owner, name: $name) { + pullRequests(first: ${pageRows(rows)}, after: $after, states: $states, orderBy: { field: CREATED_AT, direction: DESC }) { + pageInfo { hasNextPage endCursor } + nodes { ${pullRequestRowSelection(includeStacks, true)} } + } + } +}`; +} + +function pageRows(rows: number): number { + return Math.min(Math.max(Math.trunc(rows), 1), PULL_REQUEST_SEARCH_MAX_ROWS); +} + +/** One listing row, the same whether it came from a search or from a repository's own list. */ +function pullRequestRowSelection(includeStacks: boolean, includeStats: boolean): string { + return `${includeStacks ? "stack { number size baseRefName } stackEntry { position }" : ""} number title url @@ -836,17 +984,14 @@ export function pullRequestSearchGraphQlQuery(rows: number, includeStacks = fals mergeable reviewDecision latestReviews(first: 20) { nodes { state author { login } } } + ${includeStats ? "additions deletions" : ""} createdAt updatedAt mergedAt repository { nameWithOwner } - reviewRequests(first: 20) { nodes { requestedReviewer { ... on User { login } } } } + reviewRequests(first: 20) { nodes { requestedReviewer { ... on User { login } ... on Team { slug } } } } labels(first: 20) { nodes { name color } } - commits(last: 1) { nodes { commit { statusCheckRollup { state } } } } - } - } - } -}`; + commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }`; } /** @@ -1094,24 +1239,6 @@ export const UPDATE_REVIEW_COMMENT_GRAPHQL_MUTATION = `mutation($commentId: ID!, } }`; -/** - * A GraphQL request as `gh api graphql --input -` takes it. Variables travel in the document - * rather than as `-f name=value` flags, so a reader's own words never reach argv. - */ -const GraphQlRequestSchema = Schema.Struct({ - query: Schema.String, - variables: Schema.Record(Schema.String, Schema.String), -}); - -const encodeGraphQlRequest = Schema.encodeSync(Schema.fromJsonString(GraphQlRequestSchema)); - -export function encodeGraphQlRequestJson(input: { - readonly query: string; - readonly variables: Readonly>; -}): string { - return encodeGraphQlRequest({ query: input.query, variables: { ...input.variables } }); -} - /** The body of `POST /repos/{owner}/{repo}/pulls/{number}/reviews`, which sends a review whole. */ const ReviewSubmissionSchema = Schema.Struct({ event: Schema.Literals(["COMMENT", "APPROVE", "REQUEST_CHANGES"]), @@ -1126,8 +1253,6 @@ const ReviewSubmissionSchema = Schema.Struct({ ), }); -const encodeReviewSubmission = Schema.encodeSync(Schema.fromJsonString(ReviewSubmissionSchema)); - const REVIEW_EVENTS: Record = { comment: "COMMENT", approve: "APPROVE", @@ -1166,12 +1291,12 @@ function gitHubReviewPosition(position: PullRequestReviewPosition): { } /** The whole review as one request body, which is how GitHub keeps it invisible until sent. */ -export function buildReviewSubmissionJson(input: { +export function buildReviewSubmission(input: { readonly verdict: PullRequestReviewVerdict; readonly body: string; readonly comments: ReadonlyArray; -}): string { - return encodeReviewSubmission({ +}): typeof ReviewSubmissionSchema.Type { + return { event: REVIEW_EVENTS[input.verdict], body: input.body, comments: input.comments.map((comment) => ({ @@ -1179,7 +1304,7 @@ export function buildReviewSubmissionJson(input: { ...gitHubReviewPosition(comment.position), body: comment.body, })), - }); + }; } export interface GitHubPullRequestListItem { @@ -1644,23 +1769,15 @@ function toDetail(raw: Schema.Schema.Type): GitHubPullRe }; } -function toActivity(raw: Schema.Schema.Type): GitHubPullRequestActivity { - return { - author: toActor(raw.author), - comments: toComments(raw), - commits: toCommits(raw.commits), - }; -} - const decodeUnknownList = decodeJsonResult(Schema.Array(Schema.Unknown)); -const decodeListEntry = Schema.decodeUnknownExit(RawListItemSchema); const decodeSearch = decodeJsonResult(RawSearchSchema); const decodeSearchItem = Schema.decodeUnknownExit(RawSearchItemSchema); const decodeStats = decodeJsonResult(RawStatsSchema); const decodeDetail = decodeJsonResult(RawDetailSchema); -const decodeWorkflowRunApprovals = decodeJsonResult(Schema.Array(RawWorkflowRunApprovalSchema)); -const decodePullRequestHeads = decodeJsonResult(Schema.Array(RawPullRequestHeadSchema)); +const decodeWorkflowRuns = decodeJsonResult(RawWorkflowRunsSchema); +const decodePullRequestHeads = decodeJsonResult(RawPullRequestHeadsSchema); const decodeActivity = decodeJsonResult(RawActivitySchema); +const decodeRepositoryPullRequests = decodeJsonResult(RawRepositoryPullRequestsSchema); const decodeFileEntry = Schema.decodeUnknownExit(RawPullRequestFileSchema); const decodeReviewThreads = decodeJsonResult(RawReviewThreadsSchema); const decodeReviewThreadComments = decodeJsonResult(RawReviewThreadCommentsSchema); @@ -1669,27 +1786,78 @@ type DecodeFailure = Cause.Cause; export interface GitHubPullRequestListBatch { readonly items: ReadonlyArray; - /** Rows gh returned, counted before decoding, so a skipped row cannot hide a next page. */ + /** Rows GitHub returned, counted before decoding, so a skipped row cannot hide a next page. */ readonly rawCount: number; + /** Where the next page starts, or null once GitHub has handed over every row. */ + readonly endCursor: string | null; } -/** Malformed entries are skipped rather than failing the batch: one unexpected pull request - * must not blank the whole list. */ +/** + * One page of a repository's own pull request list. Malformed rows are skipped rather than + * failing the page: one unexpected pull request must not blank the whole list. A repository the + * viewer cannot see answers as an empty page, which is what `gh pr list` printed for it too. + */ export function decodePullRequestListJson( raw: string, ): Result.Result { - const decoded = decodeUnknownList(raw); - if (!Result.isSuccess(decoded)) { - return Result.fail(decoded.failure); - } + const decoded = decodeRepositoryPullRequests(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const connection = decoded.success.data.repository?.pullRequests; + const nodes = connection?.nodes ?? []; const items: GitHubPullRequestListItem[] = []; - for (const entry of decoded.success) { - const item = decodeListEntry(entry); - if (Exit.isSuccess(item)) { - items.push(toListItem(item.value)); - } + for (const entry of nodes) { + const row = toRow(entry); + if (row !== null) items.push(row.item); } - return Result.succeed({ items, rawCount: decoded.success.length }); + return Result.succeed({ + items, + rawCount: nodes.length, + endCursor: + connection?.pageInfo?.hasNextPage === true ? trimmed(connection.pageInfo.endCursor) : null, + }); +} + +/** + * A row as GraphQL answers it: reviewers and labels arrive as connections, and the checks as + * GitHub's one-word rollup rather than the whole check list. Null for a node that is not a pull + * request, or one whose fields no longer decode. + */ +function toRow( + entry: unknown, +): { readonly item: GitHubPullRequestListItem; readonly repository: string | null } | null { + const decodedNode = decodeSearchItem(entry); + if (!Exit.isSuccess(decodedNode)) return null; + const node = decodedNode.value; + const stack = toStackMembership(node); + const reviewRequests = (node.reviewRequests?.nodes ?? []).flatMap( + (request): ReadonlyArray<{ readonly login?: string; readonly slug?: string }> => { + const reviewer = request?.requestedReviewer; + const login = trimmed(reviewer?.login); + if (login !== null) return [{ login }]; + const slug = trimmed(reviewer?.slug); + return slug === null ? [] : [{ slug }]; + }, + ); + return { + item: { + ...toListItem({ + ...node, + latestReviews: (node.latestReviews?.nodes ?? []).flatMap((review) => + review === null ? [] : [review], + ), + reviewRequests, + labels: (node.labels?.nodes ?? []).flatMap((label) => (label === null ? [] : [label])), + // The rollup arrives as one enum. Dressed as a single check here so it is read the same + // way the detail's checks are. + statusCheckRollup: (node.commits?.nodes ?? []).flatMap((commitNode) => { + const state = trimmed(commitNode?.commit?.statusCheckRollup?.state); + return state === null ? [] : [{ state }]; + }), + }), + ...(stack === undefined ? {} : { stack }), + }, + repository: trimmed(node.repository?.nameWithOwner), + }; } export interface GitHubPullRequestSearchItem extends GitHubPullRequestListItem { @@ -1703,12 +1871,12 @@ export interface GitHubPullRequestSearchBatch { readonly rawCount: number; /** More rows than this slice asked for, which is truncation for every repository in it. */ readonly hasNextPage: boolean; + /** Where the next page of the same search starts. */ + readonly endCursor: string | null; } /** - * A search answers with the same pull request the listing does, one connection deeper: reviewers - * and labels arrive as connections, and the row names the repository it came from. Flattened to - * the shape `gh pr list --json` hands over so both reads decode into one type. + * A search answers with the same row a repository's list does, each naming its repository. * * Rows that are not pull requests decode as empty and are skipped, the way a malformed listing * row is — `is:pr` already excludes them, and one surprise must not blank a whole host. @@ -1720,41 +1888,19 @@ export function decodePullRequestSearchJson( if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); } - const nodes = decoded.success.data.search.nodes ?? []; + const search = decoded.success.data.search; + const nodes = search.nodes ?? []; const items: GitHubPullRequestSearchItem[] = []; for (const entry of nodes) { - const decodedNode = decodeSearchItem(entry); - if (!Exit.isSuccess(decodedNode)) continue; - const node = decodedNode.value; - const repository = trimmed(node.repository?.nameWithOwner); - if (repository === null) continue; - const stack = toStackMembership(node); - items.push({ - ...toListItem({ - ...node, - latestReviews: (node.latestReviews?.nodes ?? []).flatMap((review) => - review === null ? [] : [review], - ), - reviewRequests: (node.reviewRequests?.nodes ?? []).flatMap((request) => { - const login = trimmed(request?.requestedReviewer?.login); - return login === null ? [] : [{ login }]; - }), - labels: (node.labels?.nodes ?? []).flatMap((label) => (label === null ? [] : [label])), - // The search asks for the verdict rather than the checks behind it, so it arrives as one - // enum. Dressed as a single check here so the rollup is read the same way on both paths. - statusCheckRollup: (node.commits?.nodes ?? []).flatMap((commitNode) => { - const state = trimmed(commitNode?.commit?.statusCheckRollup?.state); - return state === null ? [] : [{ state }]; - }), - }), - ...(stack === undefined ? {} : { stack }), - repository, - }); + const row = toRow(entry); + if (row === null || row.repository === null) continue; + items.push({ ...row.item, repository: row.repository }); } return Result.succeed({ items, rawCount: nodes.length, - hasNextPage: decoded.success.data.search.pageInfo?.hasNextPage ?? false, + hasNextPage: search.pageInfo?.hasNextPage ?? false, + endCursor: search.pageInfo?.hasNextPage === true ? trimmed(search.pageInfo.endCursor) : null, }); } @@ -1900,6 +2046,13 @@ export function buildPullRequestSummariesGraphQlQuery( return `query PullRequestSummaries {\n${selections.join("\n")}\n}`; } +/** One pull request's summary, aliased the way the batch is so the same decoder reads it. */ +export function pullRequestSummaryGraphQlQuery(includeStacks = false): string { + return `query($owner: String!, $name: String!, $number: Int!) { + s0: repository(owner: $owner, name: $name) { pullRequest(number: $number) { ${PULL_REQUEST_SUMMARY_SELECTION}${includeStacks ? ` ${STACK_MEMBERSHIP_SELECTION}` : ""} } } +}`; +} + const RawSummarySchema = Schema.Struct({ ...RawSearchItemSchema.fields, changedFiles: Schema.optional(Schema.NullOr(Schema.Int)), @@ -2147,11 +2300,7 @@ export function decodePullRequestCoreJson( requestedReviewer === null ? [] : [requestedReviewer], ), labels: pr.labels.nodes, - statusCheckRollup: - contexts?.nodes.map((check) => ({ - ...check, - workflowName: check.checkSuite?.workflowRun?.workflow?.name ?? null, - })) ?? [], + statusCheckRollup: toCheckContexts(contexts?.nodes ?? []), }), viewerAccess: { canWrite: toCanWrite(repository.viewerPermission), @@ -2183,27 +2332,50 @@ export function decodePullRequestDetailJson( : Result.fail(decoded.failure); } -export function decodeWorkflowRunApprovalsJson( +export interface GitHubWorkflowRunPage { + readonly runs: ReadonlyArray; + /** Runs on the page, counted before decoding, which is what decides whether to page on. */ + readonly rawCount: number; +} + +/** One page of `actions/runs`. */ +export function decodeWorkflowRunsJson( raw: string, -): Result.Result, DecodeFailure> { - const decoded = decodeWorkflowRunApprovals(raw); +): Result.Result { + const decoded = decodeWorkflowRuns(raw); if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); - return Result.succeed( - decoded.success.map((run) => ({ - id: run.databaseId, - name: trimmed(run.workflowName) ?? `Workflow run ${run.databaseId}`, - url: trimmed(run.url), + return Result.succeed({ + runs: decoded.success.workflow_runs.map((run) => ({ + id: run.id, + name: trimmed(run.name) ?? `Workflow run ${run.id}`, + url: trimmed(run.html_url), })), - ); + rawCount: decoded.success.workflow_runs.length, + }); } -export function decodePullRequestHeadsJson( - raw: string, -): Result.Result, DecodeFailure> { +/** Open pull requests whose head branch has one name, from whichever repository it lives in. */ +export const PULL_REQUEST_HEADS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $head: String!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequests(first: 100, after: $after, states: [OPEN], headRefName: $head) { + pageInfo { hasNextPage endCursor } + nodes { number headRefOid isCrossRepository headRepositoryOwner { login } } + } + } +}`; + +export function decodePullRequestHeadsJson(raw: string): Result.Result< + { + readonly heads: ReadonlyArray; + readonly nextCursor: string | null; + }, + DecodeFailure +> { const decoded = decodePullRequestHeads(raw); if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); - return Result.succeed( - decoded.success.map((pullRequest) => ({ + const connection = decoded.success.data.repository?.pullRequests; + return Result.succeed({ + heads: (connection?.nodes ?? []).map((pullRequest) => ({ number: pullRequest.number, headSha: pullRequest.headRefOid, ...(typeof pullRequest.isCrossRepository === "boolean" @@ -2211,16 +2383,145 @@ export function decodePullRequestHeadsJson( : {}), headRepositoryOwner: trimmed(pullRequest.headRepositoryOwner?.login), })), - ); + nextCursor: nextCursorOf(connection?.pageInfo), + }); } +export interface GitHubPullRequestActivityPage { + /** Only on the first page, which is the one that asks for them. */ + readonly author?: PullRequestActor | null; + readonly commits?: ReadonlyArray; + /** Issue comments and reviews, each list unsorted. */ + readonly remarks: ReadonlyArray; + readonly nextCommentsCursor: string | null; + readonly nextReviewsCursor: string | null; +} + +/** One page of `PULL_REQUEST_ACTIVITY_GRAPHQL_QUERY`. */ export function decodePullRequestActivityJson( raw: string, -): Result.Result { +): Result.Result { const decoded = decodeActivity(raw); - return Result.isSuccess(decoded) - ? Result.succeed(toActivity(decoded.success)) - : Result.fail(decoded.failure); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const pr = decoded.success.data.repository.pullRequest; + return Result.succeed({ + ...(pr.author === undefined ? {} : { author: toActor(pr.author) }), + ...(pr.commits === undefined + ? {} + : { + commits: toCommits( + (pr.commits?.nodes ?? []).map(({ commit }) => ({ + ...commit, + authors: (commit.authors?.nodes ?? []).map((author) => ({ + ...author, + login: author.user?.login ?? null, + })), + })), + ), + }), + remarks: toComments({ + comments: pr.comments?.nodes ?? [], + reviews: pr.reviews?.nodes ?? [], + }), + nextCommentsCursor: nextCursorOf(pr.comments?.pageInfo), + nextReviewsCursor: nextCursorOf(pr.reviews?.pageInfo), + }); +} + +/** Every check context of the head commit, a page at a time, for a rollup past one page. */ +export const pullRequestCheckContextsGraphQlQuery = (host: string) => + `query($owner: String!, $name: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + headRefOid + commits(last: 1) { + nodes { commit { statusCheckRollup { contexts(first: 100, after: $after) { + ${checkContextNodesSelection(host)} + pageInfo { hasNextPage endCursor } + } } } } + } + } + } +}`; + +const RawCheckContextNodeSchema = Schema.Struct({ + ...RawCheckSchema.fields, + checkSuite: Schema.optional( + Schema.NullOr( + Schema.Struct({ + workflowRun: Schema.NullOr( + Schema.Struct({ workflow: Schema.NullOr(Schema.Struct({ name: Schema.String })) }), + ), + }), + ), + ), +}); + +const decodeCheckContexts = decodeJsonResult( + Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + headRefOid: Schema.String, + commits: Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ + commit: Schema.Struct({ + statusCheckRollup: Schema.NullOr( + Schema.Struct({ + contexts: Schema.Struct({ + nodes: Schema.Array(RawCheckContextNodeSchema), + pageInfo: RawPageInfoSchema, + }), + }), + ), + }), + }), + ), + }), + }), + }), + }), + }), +); + +/** A check context as the rollup reports it; opaque outside this module. */ +export type GitHubCheckContext = Schema.Schema.Type; + +function toCheckContexts( + nodes: ReadonlyArray>, +): ReadonlyArray { + return nodes.map((check) => ({ + ...check, + workflowName: check.checkSuite?.workflowRun?.workflow?.name ?? null, + })); +} + +export function decodePullRequestCheckContextsJson(raw: string): Result.Result< + { + readonly headSha: string; + readonly contexts: ReadonlyArray; + readonly nextCursor: string | null; + }, + DecodeFailure +> { + const decoded = decodeCheckContexts(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const pr = decoded.success.data.repository.pullRequest; + const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts; + return Result.succeed({ + headSha: pr.headRefOid, + contexts: toCheckContexts(contexts?.nodes ?? []), + nextCursor: nextCursorOf(contexts?.pageInfo), + }); +} + +/** The checks and their one-word verdict, from every context of the head commit. */ +export function pullRequestChecksFromContexts(contexts: ReadonlyArray): { + readonly checks: ReadonlyArray; + readonly checksState: PullRequestChecksState | null; +} { + return { checks: toChecks(contexts), checksState: rollupChecksState(contexts) }; } export interface GitHubReviewThreadComments { @@ -2778,17 +3079,15 @@ const ReviewerRequestSchema = Schema.Struct({ team_reviewers: Schema.Array(Schema.String), }); -const encodeReviewerRequest = Schema.encodeSync(Schema.fromJsonString(ReviewerRequestSchema)); - -export function buildReviewerRequestJson( +export function buildReviewerRequest( reviewers: ReadonlyArray<{ readonly id: string; readonly kind: PullRequestReviewerKind }>, -): string { - return encodeReviewerRequest({ +): typeof ReviewerRequestSchema.Type { + return { reviewers: reviewers.flatMap((reviewer) => (reviewer.kind === "user" ? [reviewer.id] : [])), team_reviewers: reviewers.flatMap((reviewer) => reviewer.kind === "team" ? [reviewer.id] : [], ), - }); + }; } export const LABEL_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { @@ -2876,12 +3175,10 @@ export function decodeLabelCandidatesJson( } /** The body of `POST /repos/{owner}/{repo}/issues/{number}/labels`, which adds to what is there. */ -const LabelRequestSchema = Schema.Struct({ labels: Schema.Array(Schema.String) }); - -const encodeLabelRequest = Schema.encodeSync(Schema.fromJsonString(LabelRequestSchema)); - -export function buildLabelRequestJson(labels: ReadonlyArray): string { - return encodeLabelRequest({ labels }); +export function buildLabelRequest(labels: ReadonlyArray): { + readonly labels: ReadonlyArray; +} { + return { labels }; } /** @@ -2965,14 +3262,28 @@ export interface GitHubPullRequestFilesPatch { export function decodePullRequestFilesJson( raw: string, ): Result.Result { - const decoded = decodeUnknownList(raw); - if (!Result.isSuccess(decoded)) { - return Result.fail(decoded.failure); - } + return Result.map(decodeUnknownList(raw), toFilesPatch); +} + +/** + * The files of one commit, which the commit endpoint lists and pages the same way, only wrapped + * in an object. An empty commit carries no `files` at all, which is a commit with nothing in it. + */ +export function decodeCommitFilesJson( + raw: string, +): Result.Result { + return Result.map(decodeCommitFiles(raw), (commit) => toFilesPatch(commit.files ?? [])); +} + +const decodeCommitFiles = decodeJsonResult( + Schema.Struct({ files: Schema.optional(Schema.NullOr(Schema.Array(Schema.Unknown))) }), +); + +function toFilesPatch(entries: ReadonlyArray): GitHubPullRequestFilesPatch { const sections: string[] = []; const omittedFileStats: PullRequestOmittedFileStat[] = []; let truncated = false; - for (const entry of decoded.success) { + for (const entry of entries) { const file = decodeFileEntry(entry); if (Exit.isFailure(file)) continue; const value = file.value; @@ -3009,12 +3320,12 @@ export function decodePullRequestFilesJson( ].join("\n"); sections.push(hunks.length === 0 ? `${header}\n` : `${header}\n${hunks.replace(/\n?$/, "\n")}`); } - return Result.succeed({ + return { patch: sections.join(""), truncated, - rawCount: decoded.success.length, + rawCount: entries.length, omittedFileStats, - }); + }; } /** diff --git a/apps/server/src/pullRequest/githubStackActions.test.ts b/apps/server/src/pullRequest/githubStackActions.test.ts index 61ad7da3c7b0761c6a3bd1c94f4e6f3fea0c1e52..af6794616f7c08d831bd92904d91bf855d168f9b 100644 --- a/apps/server/src/pullRequest/githubStackActions.test.ts +++ b/apps/server/src/pullRequest/githubStackActions.test.ts @@ -2,15 +2,62 @@ import { expect, it } from "@effect/vitest"; import * as Layer from "effect/Layer"; import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; +import * as Schema from "effect/Schema"; import * as TestClock from "effect/testing/TestClock"; -import { ChildProcessSpawner } from "effect/process"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import { runGitHubStackAction as runStackAction } from "./githubStackActions.ts"; -const runGitHubStackAction = ( - execute: GitHubCli.GitHubCli["Service"]["execute"], - input: Parameters[0], -) => runStackAction(input).pipe(Effect.provide(Layer.mock(GitHubCli.GitHubCli)({ execute }))); +/** + * One request as the fake saw it, flattened to words: the path or document, then each variable + * or body field as `name=value`, so an assertion reads as "this request carried that value". + */ +type Call = ReadonlyArray; +type Send = ( + call: Call, + kind: "graphql" | "rest", +) => Effect.Effect; + +const runGitHubStackAction = (send: Send, input: Parameters[0]) => + runStackAction(input).pipe( + Effect.provide( + Layer.mock(GitHubApi.GitHubApi)({ + graphql: (request) => { + // GitHub refuses a document that declares a variable it never uses. + const declared = [...request.query.matchAll(/\$(\w+)\s*:/g)].map((match) => match[1]!); + const unused = declared.filter( + (name) => !new RegExp(`\\$${name}(?!\\w)(?!\\s*:)`).test(request.query), + ); + if (unused.length > 0) { + return Effect.die(new Error(`Variables declared but not used: ${unused.join(", ")}`)); + } + return send(words(request.query, request.variables), "graphql"); + }, + rest: (request) => + send(words(request.path, request.body), "rest").pipe( + Effect.map((body) => ({ + status: 200, + headers: {}, + body, + truncated: false, + invalidUtf8: false, + })), + ), + }), + ), + ); + +const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); + +function words(head: string, fields: unknown): Call { + return [ + head, + ...Object.entries((fields ?? {}) as Record).map( + ([key, value]) => `${key}=${typeof value === "string" ? value : encodeJson(value)}`, + ), + ]; +} + +const isMutation = (call: Call) => call[0]!.startsWith("mutation"); const stack = [ { @@ -71,21 +118,13 @@ const rebased = { const rebaseResponses = [branch(2, "bbb"), rebased, branch(3, "ccc", 1, ["rebased-sha"]), rebased]; function fake(responses: readonly unknown[]) { - const calls: ReadonlyArray[] = []; - const execute: GitHubCli.GitHubCli["Service"]["execute"] = (request) => + const calls: Call[] = []; + const execute: Send = (call) => Effect.sync(() => { - calls.push(request.args); + calls.push(call); const value = responses[calls.length - 1]; if (value === undefined) throw new Error("Unexpected GitHub request"); - return { - exitCode: ChildProcessSpawner.ExitCode(0), - // @effect-diagnostics-next-line preferSchemaOverJson:off - stdout: JSON.stringify(value), - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, - stdoutInvalidUtf8: false, - }; + return encodeJson(value); }); return { execute, calls }; } @@ -215,29 +254,25 @@ it.effect("rebases unmerged layers bottom to top without local git commands", () Effect.gen(function* () { const api = fake([stack, access, ...rebaseResponses]); yield* runGitHubStackAction(api.execute, { ...input, action: "update-branch" }); - const mutations = api.calls.filter((args) => - args.some((arg) => arg.startsWith("query=mutation")), - ); + const mutations = api.calls.filter(isMutation); expect(mutations).toHaveLength(2); expect(mutations[0]).toContain("id=PR_2"); expect(mutations[0]).toContain("sha=bbb"); expect(mutations[1]).toContain("id=PR_3"); expect(mutations[1]).toContain("sha=ccc"); - expect(api.calls.every((args) => args[0] === "api")).toBe(true); }), ); it.effect("does not update later layers after a rebase failure", () => Effect.gen(function* () { const api = fake([stack, access, branch(2, "bbb")]); - const execute: typeof api.execute = (request) => - !request.args.some((arg) => arg.startsWith("query=mutation")) - ? api.execute(request) + const execute: Send = (call, kind) => + !isMutation(call) + ? api.execute(call, kind) : Effect.fail( - new GitHubCli.GitHubCliAuthenticationError({ - command: "gh", - cwd: "/repo", - cause: new Error("denied"), + new GitHubApi.GitHubApiAuthenticationError({ + host: "github.com", + operation: "runGitHubStackAction", }), ); const result = yield* runGitHubStackAction(execute, { ...input, action: "update-branch" }).pipe( @@ -272,7 +307,6 @@ it.effect("refuses the entire rebase before mutation when a later fork denies wr failure: { _tag: "GitHubStackPermissionError" }, }); expect(api.calls).toHaveLength(2); - expect(api.calls.every((args) => args[0] === "api")).toBe(true); }), ); @@ -333,9 +367,7 @@ it.effect("skips current layers without submitting a rebase mutation", () => Effect.gen(function* () { const api = fake([stack, access, branch(2, "bbb", 0), branch(3, "ccc", 0, ["bbb"])]); yield* runGitHubStackAction(api.execute, { ...input, action: "update-branch" }); - expect(api.calls.some((args) => args.some((arg) => arg.startsWith("query=mutation")))).toBe( - false, - ); + expect(api.calls.some((args) => isMutation(args))).toBe(false); }), ); @@ -380,9 +412,7 @@ it.effect("reports partial progress when a later head changes during the rebase" if (result._tag === "Failure") { expect(result.failure.message).toContain("Earlier updates remain on GitHub"); } - expect( - api.calls.filter((args) => args.some((arg) => arg.startsWith("query=mutation"))), - ).toHaveLength(1); + expect(api.calls.filter((args) => isMutation(args))).toHaveLength(1); }), ); @@ -403,11 +433,7 @@ it.effect.each([false, true])("rejects a push to a processed layer, rebased=%s", _tag: "Failure", failure: { _tag: "GitHubStackChangedError", number: 2, completed: 1 }, }); - expect(api.calls.at(-1)?.some((arg) => arg.includes('processed:nodes(ids:["PR_2"])'))).toBe( - true, - ); - expect( - api.calls.filter((args) => args.some((arg) => arg.startsWith("query=mutation"))), - ).toHaveLength(rebasedParent ? 1 : 0); + expect(api.calls.at(-1)?.includes('ids=["PR_2"]')).toBe(true); + expect(api.calls.filter((args) => isMutation(args))).toHaveLength(rebasedParent ? 1 : 0); }), ); diff --git a/apps/server/src/pullRequest/githubStackActions.ts b/apps/server/src/pullRequest/githubStackActions.ts index 6e72836e6e4015c88dac87686de80073fb499e88..0ddb57a274bc9372dd5849a6150152c16a3a6c81 100644 --- a/apps/server/src/pullRequest/githubStackActions.ts +++ b/apps/server/src/pullRequest/githubStackActions.ts @@ -8,7 +8,7 @@ import * as Clock from "effect/Clock"; import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; -import * as GitHubCli from "../sourceControl/GitHubCli.ts"; +import * as GitHubApi from "../sourceControl/GitHubApi.ts"; import { decodePullRequestStacksJson } from "./gitHubPullRequestJson.ts"; const stackErrorIdentity = { @@ -21,10 +21,6 @@ export class GitHubStackChangedError extends Schema.TaggedError 0 ? `The stack changed at PR #${this.number} after ${this.completed} layers. Earlier updates remain on GitHub. Refresh it before trying again.` @@ -36,10 +32,6 @@ export class GitHubStackUnsupportedError extends Schema.TaggedError - `pr${layer.number}:pullRequest(number:${layer.number}){headRepository{viewerPermission} maintainerCanModify}`, - ) - .join(" ")}}}`, - ], + const permissions = yield* api.graphql({ + host: input.host, + operation: "runGitHubStackAction", + allowReserve: true, + variables: { owner, name }, + query: `query($owner:String!,$name:String!){repository(owner:$owner,name:$name){${open + .map( + (layer) => + `pr${layer.number}:pullRequest(number:${layer.number}){headRepository{viewerPermission} maintainerCanModify}`, + ) + .join(" ")}}}`, }); - const access = yield* decodeBranchAccess(permissions.stdout).pipe( + const access = yield* decodeBranchAccess(permissions).pipe( Effect.mapError((cause) => new GitHubStackResponseInvalidError({ ...identity, cause })), ); // viewerCanUpdateBranch is false for an already-current layer, even if rebasing its parent @@ -282,35 +245,27 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* const processed: Array<{ id: string; number: number; headSha: string }> = []; for (const [index, layer] of open.entries()) { yield* Effect.gen(function* () { - const read = yield* github.execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - "graphql", - "-f", - `owner=${owner}`, - "-f", - `name=${name}`, - "-F", - `number=${layer.number}`, - "-f", - `sha=${layer.headSha}`, - "-f", - `query=query($owner:String!,$name:String!,$number:Int!,$sha:String!){${ - processed.length === 0 - ? "" - : `processed:nodes(ids:${encodeNodeIds(processed.map((head) => head.id))}){... on PullRequest{headRefOid}}` - } repository(owner:$owner,name:$name){pullRequest(number:$number){id headRefOid baseRef{compare(headRef:$sha){behindBy}}}}}`, - ], + const read = yield* api.graphql({ + host: input.host, + operation: "runGitHubStackAction", + allowReserve: true, + variables: { + owner, + name, + number: layer.number, + sha: layer.headSha, + ids: processed.map((head) => head.id), + }, + // GitHub rejects a declared variable the document never uses, so `$ids` is always + // selected; an empty list asks for nothing. + query: `query($owner:String!,$name:String!,$number:Int!,$sha:String!,$ids:[ID!]!){processed:nodes(ids:$ids){... on PullRequest{headRefOid}} repository(owner:$owner,name:$name){pullRequest(number:$number){id headRefOid baseRef{compare(headRef:$sha){behindBy}}}}}`, }); const { data: { processed: observed, repository: { pullRequest: pr }, }, - } = yield* decodeRebaseBranch(read.stdout); + } = yield* decodeRebaseBranch(read); // A push to an earlier layer must not silently become the next layer's new base. const changed = processed.find( (head, index) => observed?.[index]?.headRefOid !== head.headSha, @@ -332,22 +287,14 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* return; } // Pass the reviewed revision to GitHub, including when a push races this read. - const updated = yield* github.execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - "graphql", - "-f", - `id=${pr.id}`, - "-f", - `sha=${layer.headSha}`, - "-f", - "query=mutation($id:ID!,$sha:GitObjectID!){updatePullRequestBranch(input:{pullRequestId:$id,expectedHeadOid:$sha,updateMethod:REBASE}){pullRequest{headRefOid}}}", - ], + const updated = yield* api.graphql({ + host: input.host, + operation: "runGitHubStackAction", + variables: { id: pr.id, sha: layer.headSha }, + query: + "mutation($id:ID!,$sha:GitObjectID!){updatePullRequestBranch(input:{pullRequestId:$id,expectedHeadOid:$sha,updateMethod:REBASE}){pullRequest{headRefOid}}}", }); - const response = yield* decodeRebaseResponse(updated.stdout); + const response = yield* decodeRebaseResponse(updated); processed.push({ id: pr.id, number: layer.number, @@ -374,24 +321,18 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* decodeMergeResponse(raw).pipe( Effect.mapError((cause) => new GitHubStackResponseInvalidError({ ...identity, cause })), ); - const request = yield* github.execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - "--method", - "PUT", - `${endpoint}/pulls/${input.number}/merge-async`, - "-f", - `merge_method=${input.mergeMethod ?? "merge"}`, - "-f", - "merge_action=default", - "-f", - `sha=${target.headSha}`, - ], + const request = yield* api.rest({ + host: input.host, + operation: "runGitHubStackAction", + method: "PUT", + path: `${endpoint}/pulls/${input.number}/merge-async`, + body: { + merge_method: input.mergeMethod ?? "merge", + merge_action: "default", + sha: target.headSha, + }, }); - let result = yield* decode(request.stdout); + let result = yield* decode(request.body); const deadline = (yield* Clock.currentTimeMillis) + 5 * 60_000; for ( let attempt = 0; @@ -401,16 +342,12 @@ export const runGitHubStackAction = Effect.fn("runGitHubStackAction")(function* const uuid = result.details.uuid; if (!uuid) return yield* new GitHubStackResponseInvalidError({ ...identity }); yield* Effect.sleep(Math.min(1_000 * 2 ** attempt, 10_000)); - const poll = yield* github.execute({ - cwd: input.cwd, - args: [ - "api", - "--hostname", - input.host, - `${endpoint}/pulls/${input.number}/merge-async/${encodeURIComponent(uuid)}`, - ], + const poll = yield* api.rest({ + host: input.host, + operation: "runGitHubStackAction", + path: `${endpoint}/pulls/${input.number}/merge-async/${encodeURIComponent(uuid)}`, }); - result = yield* decode(poll.stdout); + result = yield* decode(poll.body); } if (result.status === "pending") return yield* new GitHubStackMergePendingError({ ...identity }); if (result.status === "failed") diff --git a/apps/server/src/sourceControl/GitHubApi.ts b/apps/server/src/sourceControl/GitHubApi.ts index 7b1a9f316a9ce8aba4b8e195776068533e0ddfb3..a6cc60b50aa8038ab56af59dad0e3da4769a3032 100644 --- a/apps/server/src/sourceControl/GitHubApi.ts +++ b/apps/server/src/sourceControl/GitHubApi.ts @@ -117,6 +117,8 @@ export interface GitHubRestResponse { readonly headers: Readonly>; readonly body: string; readonly truncated: boolean; + /** The body was not valid UTF-8, which a raw file read takes to mean binary. */ + readonly invalidUtf8: boolean; } export interface GitHubRestInput { @@ -131,6 +133,8 @@ export interface GitHubRestInput { /** Revalidates a cached answer. A 304 is returned rather than failed, and is free. */ readonly ifNoneMatch?: string; readonly maxResponseBytes?: number; + /** Defaults to 30 seconds; a whole pull request's patch may need longer. */ + readonly timeout?: Duration.Input; /** Overrides `AllowGitHubReserve` for this one request. */ readonly allowReserve?: boolean; } @@ -381,11 +385,11 @@ export const make = Effect.gen(function* () { readonly operation: string; readonly request: HttpClientRequest.HttpClientRequest; readonly maxResponseBytes: number; + readonly timeout?: Duration.Input | undefined; readonly allowReserve: boolean; readonly acceptNotModified: boolean; /** Reads the body for GraphQL `errors`, which GitHub sends with HTTP 200. */ readonly graphql?: boolean; - readonly timeout?: Duration.Duration | undefined; }) { const host = normalizeHost(input.host); // Only the path: a query string can carry a SHA or a branch, and never needs to be in a trace. @@ -435,7 +439,7 @@ export const make = Effect.gen(function* () { // 204, 304 and many refusals carry no body at all, which is an empty answer. Effect.catchIf( (error) => error.reason._tag === "EmptyBodyError", - () => Effect.succeed({ text: "", truncated: false }), + () => Effect.succeed({ text: "", truncated: false, invalidUtf8: false }), ), ); return { response, collected }; @@ -468,6 +472,7 @@ export const make = Effect.gen(function* () { headers, body: collected.text, truncated: collected.truncated, + invalidUtf8: collected.invalidUtf8, }), ), RateLimited: () => @@ -526,6 +531,7 @@ export const make = Effect.gen(function* () { operation: input.operation, request, maxResponseBytes: input.maxResponseBytes ?? DEFAULT_MAX_RESPONSE_BYTES, + timeout: input.timeout, allowReserve: input.allowReserve ?? interactive, acceptNotModified: input.ifNoneMatch !== undefined, }), diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index 0097794c7b9d3957e2a181844b03416357181b2d..88ddf1349e86d78c03cb9aad80aba977fd125e13 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -38,10 +38,7 @@ const DEFAULT_TIMEOUT_MS = 30_000; /** Server-local credential scope; never put its value in RPC payloads or cache keys. */ export const PinnedGitHubCredential = GitHubApi.PinnedGitHubCredential; -export const AllowGitHubReserve = Context.Reference( - "t3/sourceControl/AllowGitHubReserve", - { defaultValue: () => false }, -); +export const AllowGitHubReserve = GitHubApi.AllowGitHubReserve; function commandHosts(args: ReadonlyArray): Array { const hosts: Array = [];