diff --git a/packages/server/src/services/github-service.test.ts b/packages/server/src/services/github-service.test.ts index a98732a69..2f8585529 100644 --- a/packages/server/src/services/github-service.test.ts +++ b/packages/server/src/services/github-service.test.ts @@ -1220,6 +1220,78 @@ describe("GitHubService", () => { ]); }); + it("selects the requested fork owner when resolving a scoped PR worktree branch", async () => { + const runner = createScriptedRunner([ + { error: noPullRequestError() }, + JSON.stringify([ + { + number: 77, + url: "https://github.com/repoOwner/repo/pull/77", + title: "Unrelated fork main branch", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "main", + mergedAt: null, + statusCheckRollup: [], + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: "otherForkOwner" }, + }, + { + number: 345, + url: "https://github.com/repoOwner/repo/pull/345", + title: "Requested fork main branch", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "main", + mergedAt: null, + statusCheckRollup: [], + reviewDecision: "REVIEW_REQUIRED", + headRepositoryOwner: { login: "chethanuk" }, + }, + ]), + ]); + const service = createGitHubService({ + runner: runner.runner, + resolveGhPath: async () => "/usr/bin/gh", + now: () => 100, + }); + + const status = await service.getCurrentPullRequestStatus({ + cwd: "/repo", + headRef: "main", + headRepositoryOwner: "chethanuk", + }); + + expect(status).toMatchObject({ + number: 345, + repoOwner: "repoOwner", + repoName: "repo", + headRefName: "main", + }); + expect(runner.calls.map((call) => call.args)).toEqual([ + [ + "pr", + "view", + "--json", + "number,url,title,state,isDraft,baseRefName,headRefName,mergedAt,statusCheckRollup,reviewDecision,headRepositoryOwner", + ], + [ + "pr", + "list", + "--state", + "all", + "--head", + "main", + "--json", + "number,url,title,state,isDraft,baseRefName,headRefName,mergedAt,statusCheckRollup,reviewDecision,headRepositoryOwner", + "--limit", + "10", + ], + ]); + }); + it("finds a fork PR in the parent repo when the direct current branch view is unavailable", async () => { const runner = createScriptedRunner([ { error: noPullRequestError() }, diff --git a/packages/server/src/services/github-service.ts b/packages/server/src/services/github-service.ts index bc5f993a6..7ab8846eb 100644 --- a/packages/server/src/services/github-service.ts +++ b/packages/server/src/services/github-service.ts @@ -481,6 +481,7 @@ export interface GitHubService { options: { cwd: string; headRef: string; + headRepositoryOwner?: string; } & GitHubReadOptions, ): Promise; getPullRequestTimeline( @@ -854,12 +855,16 @@ export function createGitHubService(options: CreateGitHubServiceOptions = {}): G return cached({ cwd: options.cwd, method: "getCurrentPullRequestStatus", - args: { headRef: options.headRef }, + args: { + headRef: options.headRef, + headRepositoryOwner: options.headRepositoryOwner, + }, readOptions: options, load: async () => { return resolveCurrentPullRequestView({ cwd: options.cwd, headRef: options.headRef, + headRepositoryOwner: options.headRepositoryOwner, run, }); }, @@ -1281,33 +1286,51 @@ function isNoPullRequestFoundError(error: unknown): boolean { async function resolveCurrentPullRequestView(options: { cwd: string; headRef: string; + headRepositoryOwner?: string; run: (args: string[], options: GitHubCommandRunnerOptions) => Promise; }): Promise { const viewCandidate = await tryCurrentPullRequestView(options); - if (viewCandidate && isCandidateForHeadRef(viewCandidate, options.headRef)) { - return viewCandidate.status; + const viewMatch = viewCandidate + ? pickPullRequestCandidate({ + candidates: [viewCandidate], + headRef: options.headRef, + headRepositoryOwner: options.headRepositoryOwner, + }) + : null; + if (viewMatch) { + return viewMatch.status; } - const repo = await getGitHubRepoView(options); - const forkOwner = repo?.owner?.login; - const parentOwner = repo?.parent?.owner?.login; - const parentName = repo?.parent?.name; - if (!forkOwner || !parentOwner || !parentName) { - return null; + let listHeadRef = options.headRef; + let listRepo: string | undefined; + let headRepositoryOwner = options.headRepositoryOwner; + + if (!headRepositoryOwner) { + const repo = await getGitHubRepoView(options); + const forkOwner = repo?.owner?.login; + const parentOwner = repo?.parent?.owner?.login; + const parentName = repo?.parent?.name; + if (!forkOwner || !parentOwner || !parentName) { + return null; + } + + listHeadRef = `${forkOwner}:${options.headRef}`; + listRepo = `${parentOwner}/${parentName}`; + headRepositoryOwner = forkOwner; } - const parentCandidates = await listCurrentPullRequestCandidates({ + const candidates = await listCurrentPullRequestCandidates({ cwd: options.cwd, - headRef: `${forkOwner}:${options.headRef}`, + headRef: listHeadRef, run: options.run, - repo: `${parentOwner}/${parentName}`, + repo: listRepo, }); - const parentMatch = pickPullRequestCandidate({ - candidates: parentCandidates, + const match = pickPullRequestCandidate({ + candidates, headRef: options.headRef, - headRepositoryOwner: forkOwner, + headRepositoryOwner, }); - return parentMatch?.status ?? null; + return match?.status ?? null; } async function tryCurrentPullRequestView(options: { diff --git a/packages/server/src/utils/checkout-git.test.ts b/packages/server/src/utils/checkout-git.test.ts index 8830602de..7d222fba1 100644 --- a/packages/server/src/utils/checkout-git.test.ts +++ b/packages/server/src/utils/checkout-git.test.ts @@ -31,6 +31,7 @@ import { warmCheckoutShortstatInBackground, } from "./checkout-git.js"; import { + GitHubCommandError, GitHubCliMissingError, type GitHubCurrentPullRequestStatus, type GitHubService, @@ -1428,6 +1429,45 @@ const x = 1; }); }); + it("uses the tracked fork branch for PR worktree status lookup", async () => { + execSync("git checkout -b chethanuk/main", { cwd: repoDir }); + execSync("git remote add origin https://github.com/getpaseo/paseo.git", { cwd: repoDir }); + execSync("git remote add paseo-pr-345 git@github.com:chethanuk/paseo.git", { cwd: repoDir }); + execSync("git config branch.chethanuk/main.remote paseo-pr-345", { cwd: repoDir }); + execSync("git config branch.chethanuk/main.merge refs/heads/main", { cwd: repoDir }); + + const requestedTargets: Array<{ headRef: string; headRepositoryOwner?: string }> = []; + const github = createGitHubServiceForStatus( + createPullRequestStatus({ + number: 345, + url: "https://github.com/getpaseo/paseo/pull/345", + headRefName: "main", + }), + { + onStatus: () => {}, + }, + ); + github.getCurrentPullRequestStatus = async (options) => { + requestedTargets.push({ + headRef: options.headRef, + ...(options.headRepositoryOwner + ? { headRepositoryOwner: options.headRepositoryOwner } + : {}), + }); + return createPullRequestStatus({ + number: 345, + url: "https://github.com/getpaseo/paseo/pull/345", + headRefName: options.headRef, + }); + }; + + const status = await getPullRequestStatus(repoDir, github); + + expect(requestedTargets).toEqual([{ headRef: "main", headRepositoryOwner: "chethanuk" }]); + expect(status.status?.number).toBe(345); + expect(status.status?.headRefName).toBe("main"); + }); + it("returns closed-unmerged PR status without marking it as merged", async () => { execSync("git checkout -b feature", { cwd: repoDir }); execSync("git remote add origin https://github.com/getpaseo/paseo.git", { cwd: repoDir }); @@ -1497,6 +1537,75 @@ const x = 1; } }); + it("keeps stale PR status when a refresh hits a transient GitHub error", async () => { + execSync("git checkout -b feature", { cwd: repoDir }); + execSync("git remote add origin https://github.com/getpaseo/paseo.git", { cwd: repoDir }); + + __setPullRequestStatusCacheTtlForTests(50); + try { + let callCount = 0; + const github = createGitHubServiceForStatus(null); + github.getCurrentPullRequestStatus = async () => { + callCount += 1; + if (callCount === 1) { + return createPullRequestStatus({ + url: "https://github.com/getpaseo/paseo/pull/123", + }); + } + throw new GitHubCommandError({ + args: ["pr", "view"], + cwd: repoDir, + exitCode: 1, + stderr: "could not resolve host: github.com", + }); + }; + + const fresh = await getPullRequestStatus(repoDir, github); + await sleep(80); + const stale = await getPullRequestStatus(repoDir, github); + + expect(stale).toEqual(fresh); + expect(stale.githubFeaturesEnabled).toBe(true); + expect(stale.status?.url).toContain("/pull/123"); + expect(callCount).toBe(2); + } finally { + __resetPullRequestStatusCacheForTests(); + } + }); + + it("clears stale PR status after a successful no-PR refresh", async () => { + execSync("git checkout -b feature", { cwd: repoDir }); + execSync("git remote add origin https://github.com/getpaseo/paseo.git", { cwd: repoDir }); + + __setPullRequestStatusCacheTtlForTests(50); + try { + let callCount = 0; + const github = createGitHubServiceForStatus(null); + github.getCurrentPullRequestStatus = async () => { + callCount += 1; + if (callCount === 1) { + return createPullRequestStatus({ + url: "https://github.com/getpaseo/paseo/pull/123", + }); + } + return null; + }; + + const fresh = await getPullRequestStatus(repoDir, github); + await sleep(80); + const cleared = await getPullRequestStatus(repoDir, github); + + expect(fresh.status?.url).toContain("/pull/123"); + expect(cleared).toEqual({ + githubFeaturesEnabled: true, + status: null, + }); + expect(callCount).toBe(2); + } finally { + __resetPullRequestStatusCacheForTests(); + } + }); + it("dedupes concurrent PR status lookups for the same cwd", async () => { execSync("git checkout -b feature", { cwd: repoDir }); execSync("git remote add origin https://github.com/getpaseo/paseo.git", { cwd: repoDir }); diff --git a/packages/server/src/utils/checkout-git.ts b/packages/server/src/utils/checkout-git.ts index 3a1743af5..d02134fa3 100644 --- a/packages/server/src/utils/checkout-git.ts +++ b/packages/server/src/utils/checkout-git.ts @@ -4,9 +4,11 @@ import { open as openFile, readFile, stat as statFile } from "fs/promises"; import { TTLCache } from "@isaacs/ttlcache"; import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js"; import { parseAndHighlightDiff } from "../server/utils/diff-highlighter.js"; +import { parseGitHubRepoFromRemote } from "../server/workspace-git-metadata.js"; import { GitHubAuthenticationError, GitHubCliMissingError, + GitHubCommandError, createGitHubService, resolveGitHubRepo, type GitHubRepoRemoteUrlResolver, @@ -29,6 +31,7 @@ const SHORTSTAT_CACHE_MAX = 1_000; let pullRequestStatusCacheTtlMs = DEFAULT_PULL_REQUEST_STATUS_CACHE_TTL_MS; let pullRequestStatusCache = createPullRequestStatusCache(pullRequestStatusCacheTtlMs); const pullRequestStatusInFlight = new Map>(); +const lastSuccessfulPullRequestStatus = new Map(); let shortstatCacheTtlMs = DEFAULT_SHORTSTAT_CACHE_TTL_MS; let shortstatCache = createShortstatCache(shortstatCacheTtlMs); const shortstatInFlight = new Map>(); @@ -38,6 +41,11 @@ interface CheckoutReadCacheOptions { reason?: string; } +interface PullRequestStatusLookupTarget { + headRef: string; + headRepositoryOwner?: string; +} + function createPullRequestStatusCache(ttlMs: number) { return new TTLCache({ ttl: ttlMs, @@ -58,6 +66,17 @@ function getPullRequestStatusCacheKey(cwd: string): string { return resolve(cwd); } +function rememberPullRequestStatus(cacheKey: string, status: PullRequestStatusResult): void { + lastSuccessfulPullRequestStatus.set(cacheKey, status); + if (lastSuccessfulPullRequestStatus.size <= PULL_REQUEST_STATUS_CACHE_MAX) { + return; + } + const oldest = lastSuccessfulPullRequestStatus.keys().next(); + if (!oldest.done) { + lastSuccessfulPullRequestStatus.delete(oldest.value); + } +} + function getShortstatCacheKey(cwd: string): string { return resolve(cwd); } @@ -68,6 +87,7 @@ export function __resetPullRequestStatusCacheForTests(): void { pullRequestStatusCacheTtlMs = DEFAULT_PULL_REQUEST_STATUS_CACHE_TTL_MS; pullRequestStatusCache = createPullRequestStatusCache(pullRequestStatusCacheTtlMs); pullRequestStatusInFlight.clear(); + lastSuccessfulPullRequestStatus.clear(); } export function __setPullRequestStatusCacheTtlForTests(ttlMs: number): void { @@ -76,6 +96,7 @@ export function __setPullRequestStatusCacheTtlForTests(ttlMs: number): void { pullRequestStatusCacheTtlMs = ttlMs; pullRequestStatusCache = createPullRequestStatusCache(ttlMs); pullRequestStatusInFlight.clear(); + lastSuccessfulPullRequestStatus.clear(); } export function __resetCheckoutShortstatCacheForTests(): void { @@ -905,6 +926,52 @@ export async function hasOriginRemote(cwd: string): Promise { return url !== null; } +async function getGitConfigValue(cwd: string, key: string): Promise { + try { + const { stdout } = await runGitCommand(["config", "--get", key], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + const value = stdout.trim(); + return value.length > 0 ? value : null; + } catch { + return null; + } +} + +function parseBranchMergeHeadRef(mergeRef: string | null): string | null { + const prefix = "refs/heads/"; + if (!mergeRef?.startsWith(prefix)) { + return null; + } + const headRef = mergeRef.slice(prefix.length).trim(); + return headRef.length > 0 ? headRef : null; +} + +async function resolvePullRequestStatusLookupTarget( + cwd: string, + currentBranch: string, +): Promise { + const remoteName = await getGitConfigValue(cwd, `branch.${currentBranch}.remote`); + if (!remoteName?.startsWith("paseo-pr-")) { + return { headRef: currentBranch }; + } + + const mergeRef = await getGitConfigValue(cwd, `branch.${currentBranch}.merge`); + const trackedHeadRef = parseBranchMergeHeadRef(mergeRef); + if (!trackedHeadRef) { + return { headRef: currentBranch }; + } + + const remoteUrl = await getGitConfigValue(cwd, `remote.${remoteName}.url`); + const remoteRepo = remoteUrl ? parseGitHubRepoFromRemote(remoteUrl) : null; + const headRepositoryOwner = remoteRepo?.split("/")[0]; + return { + headRef: trackedHeadRef, + ...(headRepositoryOwner ? { headRepositoryOwner } : {}), + }; +} + export async function resolveAbsoluteGitDir(cwd: string): Promise { try { const { stdout } = await runGitCommand(["rev-parse", "--absolute-git-dir"], { @@ -2106,8 +2173,18 @@ export async function getPullRequestStatus( const lookup = getPullRequestStatusUncached(cwd, github, options) .then((status) => { pullRequestStatusCache.set(cacheKey, status); + rememberPullRequestStatus(cacheKey, status); return status; }) + .catch((error) => { + if (error instanceof GitHubCommandError) { + const stale = lastSuccessfulPullRequestStatus.get(cacheKey); + if (stale) { + return stale; + } + } + throw error; + }) .finally(() => { pullRequestStatusInFlight.delete(cacheKey); }); @@ -2130,9 +2207,10 @@ async function getPullRequestStatusUncached( }; } try { + const lookupTarget = await resolvePullRequestStatusLookupTarget(cwd, head); const status = await github.getCurrentPullRequestStatus({ cwd, - headRef: head, + ...lookupTarget, reason: options?.reason, }); return {