Fix PR worktree status lookup
This commit is contained in:
@@ -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() },
|
||||
|
||||
@@ -481,6 +481,7 @@ export interface GitHubService {
|
||||
options: {
|
||||
cwd: string;
|
||||
headRef: string;
|
||||
headRepositoryOwner?: string;
|
||||
} & GitHubReadOptions,
|
||||
): Promise<GitHubCurrentPullRequestStatus | null>;
|
||||
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<string>;
|
||||
}): Promise<GitHubCurrentPullRequestStatus | null> {
|
||||
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: {
|
||||
|
||||
@@ -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 });
|
||||
|
||||
@@ -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<string, Promise<PullRequestStatusResult>>();
|
||||
const lastSuccessfulPullRequestStatus = new Map<string, PullRequestStatusResult>();
|
||||
let shortstatCacheTtlMs = DEFAULT_SHORTSTAT_CACHE_TTL_MS;
|
||||
let shortstatCache = createShortstatCache(shortstatCacheTtlMs);
|
||||
const shortstatInFlight = new Map<string, Promise<CheckoutShortstat | null>>();
|
||||
@@ -38,6 +41,11 @@ interface CheckoutReadCacheOptions {
|
||||
reason?: string;
|
||||
}
|
||||
|
||||
interface PullRequestStatusLookupTarget {
|
||||
headRef: string;
|
||||
headRepositoryOwner?: string;
|
||||
}
|
||||
|
||||
function createPullRequestStatusCache(ttlMs: number) {
|
||||
return new TTLCache<string, PullRequestStatusResult>({
|
||||
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<boolean> {
|
||||
return url !== null;
|
||||
}
|
||||
|
||||
async function getGitConfigValue(cwd: string, key: string): Promise<string | null> {
|
||||
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<PullRequestStatusLookupTarget> {
|
||||
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<string | null> {
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user