diff --git a/packages/server/src/services/github-service.test.ts b/packages/server/src/services/github-service.test.ts index 194a0c7487..ab71115f36 100644 --- a/packages/server/src/services/github-service.test.ts +++ b/packages/server/src/services/github-service.test.ts @@ -25,6 +25,7 @@ const EXPECTED_GITHUB_ERROR_BACKOFF_CAP_MS = 300_000; const CURRENT_PR_STATUS_BASE_FIELDS = "number,url,title,state,isDraft,baseRefName,headRefName,headRefOid,mergedAt,reviewDecision,mergeable,headRepositoryOwner"; const CURRENT_PR_STATUS_FIELDS = `${CURRENT_PR_STATUS_BASE_FIELDS},statusCheckRollup`; +const FORK_PR_QUERY = "state=all&per_page=100&head=forkOwner%3Afeature%2Ffork"; interface RunnerCall { args: string[]; @@ -3324,34 +3325,33 @@ describe("ForgeService", () => { name: "parentRepo", parent: { owner: { login: "parentOwner" }, name: "parentRepo" }, }), - JSON.stringify([ - { - number: 41, - url: "https://github.com/parentOwner/parentRepo/pull/41", - title: "Wrong fork owner", - state: "OPEN", - isDraft: false, - baseRefName: "main", - headRefName: "feature/fork", - mergedAt: null, - statusCheckRollup: [], - reviewDecision: "REVIEW_REQUIRED", - headRepositoryOwner: { login: "otherFork" }, - }, - { - number: 42, - url: "https://github.com/parentOwner/parentRepo/pull/42", - title: "Real fork PR", - state: "OPEN", - isDraft: false, - baseRefName: "main", - headRefName: "feature/fork", - mergedAt: null, - statusCheckRollup: [], - reviewDecision: "REVIEW_REQUIRED", - headRepositoryOwner: { login: "forkOwner" }, - }, - ]), + JSON.stringify([{ number: 41 }, { number: 42 }]), + JSON.stringify({ + number: 41, + url: "https://github.com/parentOwner/parentRepo/pull/41", + title: "Wrong fork owner", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "feature/fork", + mergedAt: null, + statusCheckRollup: [], + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: "otherFork" }, + }), + JSON.stringify({ + number: 42, + url: "https://github.com/parentOwner/parentRepo/pull/42", + title: "Real fork PR", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "feature/fork", + mergedAt: null, + statusCheckRollup: [], + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: "forkOwner" }, + }), ]); const service = createGitHubService({ runner: runner.runner, @@ -3371,27 +3371,182 @@ describe("ForgeService", () => { title: "Real fork PR", headRefName: "feature/fork", }); - expect(runner.calls.slice(0, 3).map((call) => call.args)).toEqual([ + expect(runner.calls.slice(0, 5).map((call) => call.args)).toEqual([ ["pr", "view", "--json", CURRENT_PR_STATUS_FIELDS], ["repo", "view", "--json", "owner,name,parent"], - [ - "pr", - "list", - "--repo", - "parentOwner/parentRepo", - "--state", - "all", - "--head", - "forkOwner:feature/fork", - "--limit", - "10", - "--json", - CURRENT_PR_STATUS_FIELDS, - ], + ["api", `repos/parentOwner/parentRepo/pulls?${FORK_PR_QUERY}`], + ["pr", "view", "41", "--repo", "parentOwner/parentRepo", "--json", CURRENT_PR_STATUS_FIELDS], + ["pr", "view", "42", "--repo", "parentOwner/parentRepo", "--json", CURRENT_PR_STATUS_FIELDS], ]); }); - it("retries scoped PR list without statusCheckRollup when token permissions are insufficient", async () => { + it("reads the fork PR carrying the checked-out commit past newer attempts on the branch", async () => { + const checkoutSha = "3333333333333333333333333333333333333333"; + const viewed: number[] = []; + const runner: GitHubCommandRunner = async (args) => { + if (args[0] === "repo" && args[1] === "view") { + return { + stdout: JSON.stringify({ + owner: { login: "forkOwner" }, + name: "parentRepo", + parent: { owner: { login: "parentOwner" }, name: "parentRepo" }, + }), + stderr: "", + }; + } + if (args[0] === "api") { + // Newest first, and the checkout is sitting on the oldest attempt. + return { + stdout: JSON.stringify([ + { number: 50, head: { sha: "5".repeat(40) } }, + { number: 49, head: { sha: "4".repeat(40) } }, + { number: 48, head: { sha: "8".repeat(40) } }, + { number: 47, head: { sha: checkoutSha } }, + ]), + stderr: "", + }; + } + if (args[0] === "pr" && args[1] === "view" && /^\d+$/.test(args[2] ?? "")) { + const number = Number(args[2]); + viewed.push(number); + return { + stdout: currentPullRequestJson({ + number, + url: `https://github.com/parentOwner/parentRepo/pull/${number}`, + state: "CLOSED", + headRefOid: number === 47 ? checkoutSha : `${number}`.repeat(20), + headRepositoryOwner: { login: "forkOwner" }, + }), + stderr: "", + }; + } + throw noPullRequestError(args); + }; + const service = createGitHubService({ + runner, + resolveGhPath: async () => "/usr/bin/gh", + now: () => 100, + }); + + const status = await service.getCurrentPullRequestStatus({ + cwd: "/repo", + headRef: "feature/fork", + headSha: checkoutSha, + }); + + expect(status).toMatchObject({ + number: 47, + repoOwner: "parentOwner", + repoName: "parentRepo", + }); + expect(viewed).toContain(47); + expect(viewed).toHaveLength(3); + }); + + it("propagates a failed fork PR lookup instead of reporting no pull request", async () => { + const rateLimitError = new GitHubCommandError({ + args: ["api", "repos/parentOwner/parentRepo/pulls"], + cwd: "/repo", + exitCode: 1, + stderr: "API rate limit exceeded", + }); + const runner = createScriptedRunner([ + { error: noPullRequestError() }, + JSON.stringify({ + owner: { login: "forkOwner" }, + name: "parentRepo", + parent: { owner: { login: "parentOwner" }, name: "parentRepo" }, + }), + { error: rateLimitError }, + ]); + const service = createGitHubService({ + runner: runner.runner, + resolveGhPath: async () => "/usr/bin/gh", + now: () => 100, + }); + + await expect( + service.getCurrentPullRequestStatus({ cwd: "/repo", headRef: "feature/fork" }), + ).rejects.toThrow(GitHubCommandError); + }); + + it("resolves a fork PR whose branch name is crowded in the parent repository", async () => { + // Mirrors what `gh` really does: `pr list --head` matches the string + // against headRefName, so an "owner:branch" head never matches, and a + // popular branch name fills the candidate page with other forks' PRs. + const calls: RunnerCall[] = []; + function parentRepoPullRequest( + number: number, + headRepositoryOwner: string, + ): Record { + return { + number, + url: `https://github.com/parentOwner/parentRepo/pull/${number}`, + title: `PR ${number}`, + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "main", + headRefOid: `${number}`.padStart(40, "0"), + mergedAt: null, + statusCheckRollup: [], + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: headRepositoryOwner }, + }; + } + const crowd = Array.from({ length: 10 }, (_, index) => + parentRepoPullRequest(100 + index, `otherFork${index}`), + ); + const runner: GitHubCommandRunner = async (args, options) => { + calls.push({ args, cwd: options.cwd }); + if (args[0] === "repo" && args[1] === "view") { + return { + stdout: JSON.stringify({ + owner: { login: "forkOwner" }, + name: "parentRepo", + parent: { owner: { login: "parentOwner" }, name: "parentRepo" }, + }), + stderr: "", + }; + } + if (args[0] === "pr" && args[1] === "view" && args[2] === "4984") { + return { stdout: JSON.stringify(parentRepoPullRequest(4984, "forkOwner")), stderr: "" }; + } + if (args[0] === "pr" && args[1] === "view") { + throw noPullRequestError(args); + } + if (args[0] === "pr" && args[1] === "list") { + const headRef = args[args.indexOf("--head") + 1]; + const limit = Number(args[args.indexOf("--limit") + 1]); + const matching = crowd.filter((candidate) => candidate.headRefName === headRef); + return { stdout: JSON.stringify(matching.slice(0, limit)), stderr: "" }; + } + if (args[0] === "api" && args[1]?.startsWith("repos/")) { + expect(args[1]).toContain("head=forkOwner%3Amain"); + return { stdout: JSON.stringify([{ number: 4984 }]), stderr: "" }; + } + return { stdout: "{}", stderr: "" }; + }; + const service = createGitHubService({ + runner, + resolveGhPath: async () => "/usr/bin/gh", + now: () => 100, + }); + + const status = await service.getCurrentPullRequestStatus({ + cwd: "/repo", + headRef: "main", + }); + + expect(status).toMatchObject({ + number: 4984, + repoOwner: "parentOwner", + repoName: "parentRepo", + headRefName: "main", + }); + }); + + it("retries the fork PR view without statusCheckRollup when token permissions are insufficient", async () => { const runner = createScriptedRunner([ currentPullRequestJson({ number: 7, @@ -3404,36 +3559,30 @@ describe("ForgeService", () => { name: "parentRepo", parent: { owner: { login: "parentOwner" }, name: "parentRepo" }, }), + JSON.stringify([{ number: 42 }]), { error: statusCheckRollupPermissionError([ "pr", - "list", + "view", + "42", "--repo", "parentOwner/parentRepo", - "--state", - "all", - "--head", - "forkOwner:feature/fork", "--json", CURRENT_PR_STATUS_FIELDS, - "--limit", - "10", ]), }, - JSON.stringify([ - { - number: 42, - url: "https://github.com/parentOwner/parentRepo/pull/42", - title: "Real fork PR", - state: "OPEN", - isDraft: false, - baseRefName: "main", - headRefName: "feature/fork", - mergedAt: null, - reviewDecision: "REVIEW_REQUIRED", - headRepositoryOwner: { login: "forkOwner" }, - }, - ]), + JSON.stringify({ + number: 42, + url: "https://github.com/parentOwner/parentRepo/pull/42", + title: "Real fork PR", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "feature/fork", + mergedAt: null, + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: "forkOwner" }, + }), ]); const service = createGitHubService({ runner: runner.runner, @@ -3454,34 +3603,17 @@ describe("ForgeService", () => { checks: [], checksStatus: "none", }); - expect(runner.calls.slice(0, 4).map((call) => call.args)).toEqual([ + expect(runner.calls.slice(0, 5).map((call) => call.args)).toEqual([ ["pr", "view", "--json", CURRENT_PR_STATUS_FIELDS], ["repo", "view", "--json", "owner,name,parent"], + ["api", `repos/parentOwner/parentRepo/pulls?${FORK_PR_QUERY}`], + ["pr", "view", "42", "--repo", "parentOwner/parentRepo", "--json", CURRENT_PR_STATUS_FIELDS], [ "pr", - "list", + "view", + "42", "--repo", "parentOwner/parentRepo", - "--state", - "all", - "--head", - "forkOwner:feature/fork", - "--limit", - "10", - "--json", - CURRENT_PR_STATUS_FIELDS, - ], - [ - "pr", - "list", - "--repo", - "parentOwner/parentRepo", - "--state", - "all", - "--head", - "forkOwner:feature/fork", - "--limit", - "10", "--json", CURRENT_PR_STATUS_BASE_FIELDS, ], @@ -3629,21 +3761,20 @@ describe("ForgeService", () => { name: "repo", parent: { owner: { login: "parentOwner" }, name: "repo" }, }), - JSON.stringify([ - { - number: 88, - url: "https://github.com/parentOwner/repo/pull/88", - title: "Fork-only PR", - state: "OPEN", - isDraft: false, - baseRefName: "main", - headRefName: "feature/fork", - mergedAt: null, - statusCheckRollup: [], - reviewDecision: null, - headRepositoryOwner: { login: "forkOwner" }, - }, - ]), + JSON.stringify([{ number: 88 }]), + JSON.stringify({ + number: 88, + url: "https://github.com/parentOwner/repo/pull/88", + title: "Fork-only PR", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "feature/fork", + mergedAt: null, + statusCheckRollup: [], + reviewDecision: null, + headRepositoryOwner: { login: "forkOwner" }, + }), ]); const service = createGitHubService({ runner: runner.runner, @@ -3662,7 +3793,7 @@ describe("ForgeService", () => { repoName: "repo", headRefName: "feature/fork", }); - expect(runner.calls[2]?.args).toContain("forkOwner:feature/fork"); + expect(runner.calls[2]?.args).toEqual(["api", `repos/parentOwner/repo/pulls?${FORK_PR_QUERY}`]); }); it("propagates DNS errors while resolving the current PR view", async () => { diff --git a/packages/server/src/services/github-service.ts b/packages/server/src/services/github-service.ts index 4e857d5062..b6de0cbd6e 100644 --- a/packages/server/src/services/github-service.ts +++ b/packages/server/src/services/github-service.ts @@ -117,6 +117,11 @@ export const GITHUB_POLL_ALIGNMENT_MS = 5_000; const GITHUB_GRAPHQL_RESERVE_RATIO = 0.4; const GITHUB_GRAPHQL_RESET_GRACE_MS = 1_000; const BATCH_PR_CANDIDATE_LIMIT = 10; +// How many of a fork branch's own pull requests are read in full. One is the +// normal case; the rest are earlier attempts on the same branch, and the one +// carrying the checkout's head commit is read whichever attempt it belongs to. +const FORK_PR_VIEW_LIMIT = 3; +const FORK_PR_PAGE_SIZE = 100; const FROZEN_PR_CHECKS_CACHE_MAX_ENTRIES = 512; const GITHUB_ENV = { GIT_TERMINAL_PROMPT: "0", @@ -457,6 +462,16 @@ const GitHubRepoViewSchema = z.object({ .optional(), }); +const ForkPullRequestRefsSchema = z.array( + z.object({ + number: z.number(), + head: z + .object({ sha: z.string().catch("") }) + .nullable() + .optional(), + }), +); + const PullRequestCheckoutTargetSchema = z.object({ data: z.object({ repository: z.object({ @@ -2964,8 +2979,6 @@ async function resolveCurrentPullRequestView(options: { return viewMatch.status; } - let listHeadRef = options.headRef; - let listRepo: string | undefined; let headRepositoryOwner = options.headRepositoryOwner; if (!headRepositoryOwner) { @@ -2977,17 +2990,19 @@ async function resolveCurrentPullRequestView(options: { return null; } if (parentOwner && parentName) { - listHeadRef = `${forkOwner}:${options.headRef}`; - listRepo = `${parentOwner}/${parentName}`; + return resolveForkPullRequestView({ + ...options, + forkOwner, + repo: `${parentOwner}/${parentName}`, + }); } headRepositoryOwner = forkOwner; } const candidates = await listCurrentPullRequestCandidates({ cwd: options.cwd, - headRef: listHeadRef, + headRef: options.headRef, run: options.run, - repo: listRepo, }); const match = pickPullRequestCandidate({ candidates, @@ -2998,6 +3013,77 @@ async function resolveCurrentPullRequestView(options: { return match?.status ?? null; } +// A fork checkout's pull request lives in the parent repository, where only +// its head repository tells it apart from every other fork's pull request on +// the same branch name. `gh pr list --head` cannot ask that question: it +// forwards the whole value as a head branch name, so "owner:branch" matches +// nothing (cli/cli#10945), and the bare branch name loses a common name like +// `main` among the other forks crowding the candidate page. The REST pulls +// endpoint is the only lookup that takes a head repository, so it names the +// pull requests and the ordinary view path reads each one's status. +async function resolveForkPullRequestView(options: { + cwd: string; + headRef: string; + headSha?: string; + forkOwner: string; + repo: string; + run: (args: string[], options: GitHubCommandRunnerOptions) => Promise; +}): Promise { + const numbers = await listForkPullRequestNumbers(options); + const candidates = await Promise.all( + numbers.map((number) => + tryCurrentPullRequestView({ + cwd: options.cwd, + headRef: options.headRef, + run: options.run, + args: ["pr", "view", String(number), "--repo", options.repo], + }), + ), + ); + const match = pickPullRequestCandidate({ + candidates: candidates.filter( + (candidate): candidate is ResolvedPullRequestCandidate => candidate !== null, + ), + headRef: options.headRef, + headSha: options.headSha, + headRepositoryOwner: options.forkOwner, + }); + return match?.status ?? null; +} + +// The endpoint answers with the fork branch's pull requests, newest first, so +// a branch reused for several attempts can carry the checkout's head commit on +// an older one. Those are read first, and the rest of the window is filled +// newest-first, so the full read stays bounded without dropping the attempt the +// checkout is actually sitting on. +async function listForkPullRequestNumbers(options: { + cwd: string; + headRef: string; + headSha?: string; + forkOwner: string; + repo: string; + run: (args: string[], options: GitHubCommandRunnerOptions) => Promise; +}): Promise { + const query = new URLSearchParams({ + state: "all", + per_page: String(FORK_PR_PAGE_SIZE), + head: `${options.forkOwner}:${options.headRef}`, + }); + const args = ["api", `repos/${options.repo}/pulls?${query.toString()}`]; + const stdout = await options.run(args, { cwd: options.cwd }); + const refs = parseGitHubJsonOutput(stdout, ForkPullRequestRefsSchema, { + args, + cwd: options.cwd, + emptyFallback: "[]", + }); + function carriesHeadSha(ref: (typeof refs)[number]): boolean { + return options.headSha !== undefined && ref.head?.sha === options.headSha; + } + return [...refs.filter(carriesHeadSha), ...refs.filter((ref) => !carriesHeadSha(ref))] + .slice(0, FORK_PR_VIEW_LIMIT) + .map((ref) => ref.number); +} + async function addCurrentPullRequestGithubFacts(options: { cwd: string; status: CurrentPullRequestStatus | null; @@ -3057,16 +3143,18 @@ async function loadPullRequestGithubFacts(options: { async function tryCurrentPullRequestView(options: { cwd: string; headRef: string; + args?: string[]; run: (args: string[], options: GitHubCommandRunnerOptions) => Promise; }): Promise { + const args = options.args ?? ["pr", "view"]; try { const stdout = await runCurrentPullRequestStatusCommand({ cwd: options.cwd, run: options.run, - args: ["pr", "view"], + args, }); return parseCurrentPullRequestCandidate(stdout, options.headRef, { - args: ["pr", "view", "--json", CURRENT_PR_STATUS_FIELDS], + args: [...args, "--json", CURRENT_PR_STATUS_FIELDS], cwd: options.cwd, }); } catch (error) { @@ -3081,13 +3169,8 @@ async function listCurrentPullRequestCandidates(options: { cwd: string; headRef: string; run: (args: string[], options: GitHubCommandRunnerOptions) => Promise; - repo?: string; }): Promise { - const args = ["pr", "list"]; - if (options.repo) { - args.push("--repo", options.repo); - } - args.push("--state", "all", "--head", options.headRef, "--limit", "10"); + const args = ["pr", "list", "--state", "all", "--head", options.headRef, "--limit", "10"]; try { const stdout = await runCurrentPullRequestStatusCommand({ cwd: options.cwd,