From 10590993f41cb66a2a82f781da0c74ec5fb80f16 Mon Sep 17 00:00:00 2001 From: David Blass Date: Sat, 9 May 2026 00:58:55 +0000 Subject: [PATCH] checkout_pr: retry missing pull/N/head ref with PR-state guard (#627) * checkout_pr: retry missing pull/N/head ref with PR-state guard Co-authored-by: Cursor * checkout_pr tests: satisfy ToolState required fields Co-authored-by: Cursor * checkout_pr: tighten retry-helper semantics (anneal round 1) Co-authored-by: Cursor * checkout_pr: use retry util, drop retry tests * Update action/mcp/checkout.ts Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> --------- Co-authored-by: Cursor Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> --- mcp/checkout.ts | 91 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 5201f28..18e64c7 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -8,6 +8,7 @@ import { countLines, createDiffCoverageState } from "../utils/diffCoverage.ts"; import { $git } from "../utils/gitAuth.ts"; import { executeLifecycleHook } from "../utils/lifecycle.ts"; import { computeIncrementalDiff } from "../utils/rangeDiff.ts"; +import { retry } from "../utils/retry.ts"; import { $ } from "../utils/shell.ts"; import { rejectIfLeadingDash } from "./git.ts"; import { commentableLinesForFile } from "./review.ts"; @@ -281,6 +282,12 @@ type CheckoutPrBranchParams = GitContext & { // legitimate git op that's holding the lock. const STALE_LOCK_AGE_MS = 30_000; +// PR head refs (refs/pull/N/head) sometimes lag the pull_request.opened +// webhook by a few seconds. retry the missing-ref case with backoff +// before giving up — see issue #591. +const PULL_REF_RETRY_DELAYS_MS = [2_000, 5_000, 10_000]; +const PULL_REF_MISSING_PATTERN = /couldn't find remote ref pull\/\d+\/head/i; + const GIT_LOCK_PATHS = [ ".git/shallow.lock", ".git/index.lock", @@ -308,6 +315,62 @@ function cleanupStaleGitLocks(): void { } } +/** + * Returns false when a PR's current state diverges from what we dispatched + * on (closed/merged, or head SHA differs from pr.headSha). Used to short- + * circuit the pull/N/head retry loop when the ref is missing because the + * PR has moved on, not because of a webhook race. + * + * Network failures here are treated as "still valid" — we'd rather burn the + * retry budget than wrongly abort on a transient API blip. + * + * Note: this answers "should we keep trying?", NOT "will the next fetch + * succeed?". `pulls.get` (REST API) and `pull/N/head` (git ref) are served + * by independent GitHub replicas with their own propagation lag, so + * `pulls.get` reporting an open PR with a matching head SHA does not + * guarantee the git ref is yet visible — and vice versa (see issue #591 + * for the original webhook-vs-ref replication-lag context). + */ +async function isPullRequestStillDispatchable(args: { + octokit: Octokit; + owner: string; + repo: string; + pr: PrData; +}): Promise { + try { + const { data } = await args.octokit.rest.pulls.get({ + owner: args.owner, + repo: args.repo, + pull_number: args.pr.number, + }); + if (data.state !== "open") return false; + if (data.head.sha !== args.pr.headSha) return false; + return true; + } catch { + // lenient — don't abort on API hiccups + return true; + } +} + +/** + * Throws the friendly clean-abort error when the PR has moved on since + * dispatch. Wraps `isPullRequestStillDispatchable` so the abort message + * lives in one place and is invoked from the inner `catch` around the + * `pull/N/head` fetch on every missing-ref failure. + */ +async function abortIfPullRequestMoved(args: { + octokit: Octokit; + owner: string; + repo: string; + pr: PrData; +}): Promise { + const stillValid = await isPullRequestStillDispatchable(args); + if (stillValid) return; + throw new Error( + `PR #${args.pr.number} is no longer in the state it was at dispatch (likely closed, merged, or force-pushed between webhook fire and run start). aborting checkout — re-trigger the run if this PR is still active.` + ); +} + /** * Shared helper to checkout a PR branch and configure fork remotes. * Assumes origin remote is already configured with authentication. @@ -365,9 +428,31 @@ export async function checkoutPrBranch( // fetch PR branch using pull/{n}/head refspec (works for both fork and same-repo PRs) log.debug(`» fetching PR #${pr.number} (${localBranch})...`); - await $git("fetch", ["--no-tags", "origin", `+pull/${pr.number}/head:${localBranch}`], { - token: gitToken, - }); + await retry( + async () => { + try { + await $git("fetch", ["--no-tags", "origin", `+pull/${pr.number}/head:${localBranch}`], { + token: gitToken, + }); + } catch (e) { + // on the webhook race, check whether the PR still matches what we + // dispatched on. if it's been closed/merged or the head SHA moved, + // no amount of retrying will populate the expected ref — surface a + // clean abort error instead of burning the full retry budget. + const msg = e instanceof Error ? e.message : String(e); + if (PULL_REF_MISSING_PATTERN.test(msg)) { + await abortIfPullRequestMoved({ octokit, owner, repo: name, pr }); + } + throw e; + } + }, + { + delaysMs: PULL_REF_RETRY_DELAYS_MS, + label: `pull/${pr.number}/head fetch`, + shouldRetry: (e) => + PULL_REF_MISSING_PATTERN.test(e instanceof Error ? e.message : String(e)), + } + ); // checkout the branch $("git", ["checkout", localBranch], { log: false });