From 17c5050216e658d77d0ca23f6ee680fd31789a32 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Tue, 22 Sep 2026 23:44:49 +0200 Subject: [PATCH] Show a fork checkout's pull request in the workspace (#5221) * Resolve a fork checkout's pull request by its head repository A fork checkout's pull request lives in the parent repository, and its head repository is the only thing that tells it apart from every other fork's pull request on the same branch name. The per-checkout lookup asked `gh pr list --repo --head :`, which `gh` cannot answer: it forwards the whole value as a head branch name, so the query always came back empty. Every synchronous refresh then overwrote the status the background poll had resolved, and the workspace showed no pull request. Fork checkouts now resolve through the REST pulls endpoint, the one lookup that takes a head repository, and read each resulting pull request through the ordinary view path. That also removes the disambiguation the bare branch name would have needed: `--head main` already returns thirty pull requests upstream, far past the ten-item candidate page, so a fork checkout on a common branch name would still have lost its own pull request. The three existing fork tests asserted the unanswerable invocation while their fake runner answered it regardless of the head value. They now run against a runner that matches head refs the way `gh` does. * Let a failed fork lookup surface, and keep the checked-out attempt Two corrections to the fork pull request lookup. An operational failure on the REST lookup, a rate limit, a timeout, a DNS failure, was caught and returned as an empty list, so a refresh reported "no pull request" for a checkout that has one. The lookup now only handles what it can act on and lets the rest reach the caller, the same way the non-fork list path does. A fork branch reused for several attempts can carry the checkout's head commit on an older pull request than the three the lookup reads. The page is now requested in full and the attempts carrying the checked-out commit are read first, so the bound on full reads no longer decides which attempt the workspace can see. * Use a function declaration for the head-commit predicate * Use a function declaration for the test pull request helper --------- Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- .../src/services/github-service.test.ts | 337 ++++++++++++------ .../server/src/services/github-service.ts | 111 +++++- 2 files changed, 331 insertions(+), 117 deletions(-) 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,