From 3ce2dbb1eadbb24cb102af1e3a06df7d2463a1b7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Wed, 13 May 2026 14:55:52 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20solve=20#4513=20=E2=80=94=20wrong=20PR?= =?UTF-8?q?=20shown=20next=20to=20V2=20sidebar=20workspaces?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V2's PR resolution attached the first matching candidate from the GitHub REST list endpoint (sorted by updated desc), so a recently-closed PR could shadow an open one on the same branch, and a long-closed historical PR could linger next to `master` after the branch head had moved past it. Root cause: - `normalizePullRequestCandidates` used `.find` and accepted the first matching head, with no preference for OPEN over CLOSED/MERGED. - The per-workspace assignment in `performProjectRefresh` attached any matched PR unconditionally; V1's `shouldAcceptPRMatch` historical-vs-headSha gate had no V2 counterpart. Fix: - Sort candidates by state rank (OPEN > MERGED > CLOSED), tiebreak on updatedAt, before returning the picked node. - Introduce `shouldAttachHistoricalMatch` and use it before linking — a CLOSED or MERGED PR only attaches when its headRefOid matches the workspace headSha (mirrors V1). - Surface the picked node's state + headSha through `fetchRepoPullRequests` so the per-workspace decision has the data it needs. Tests: - `github-query.test.ts`: open PR is selected even when the closed one has a newer updatedAt. - `pull-requests.test.ts`: stale closed PR on `master` does not attach when workspace HEAD has moved past the PR's head commit. Closes #4513 --- .../pull-requests/pull-requests.test.ts | 51 ++++++++++++++++ .../runtime/pull-requests/pull-requests.ts | 34 +++++++++-- .../utils/github-query/github-query.test.ts | 60 +++++++++++++++++++ .../utils/github-query/github-query.ts | 42 +++++++++---- 4 files changed, 169 insertions(+), 18 deletions(-) diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index 68a73096f66..12eecfefccc 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -351,6 +351,57 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { ]); }); + test("does not attach a stale closed PR when workspace HEAD has moved past it (#4513)", async () => { + const state = makeState("master"); + state.workspace = { + ...state.workspace, + headSha: "current-master-sha", + upstreamOwner: "base-owner", + upstreamRepo: "base-repo", + upstreamBranch: "master", + pullRequestId: null, + }; + const manager = createManager(state, { + execGh: async (args) => { + const path = args.find((arg) => arg.startsWith("repos/")); + if (path === "repos/base-owner/base-repo/pulls") { + return [ + { + number: 42, + title: "Old closed PR on master", + html_url: "https://github.com/base-owner/base-repo/pull/42", + state: "closed", + draft: false, + merged_at: null, + updated_at: "2025-01-01T00:00:00Z", + head: { + ref: "master", + sha: "stale-closed-sha", + repo: { + name: "base-repo", + owner: { login: "base-owner" }, + }, + }, + base: { + repo: { + full_name: "base-owner/base-repo", + }, + }, + }, + ]; + } + if (path?.endsWith("/reviews")) return []; + if (path?.endsWith("/check-runs")) return { check_runs: [] }; + if (path?.endsWith("/statuses")) return []; + return null; + }, + }); + + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + + expect(state.workspace.pullRequestId).toBeNull(); + }); + test("preserves existing pullRequestId when head lookup fails", async () => { const state = makeState("fix/sidebar"); state.workspace = { diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index 37ee51783ee..393fa8c4d4a 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -243,6 +243,19 @@ function mapCheckoutPullRequestState( return "open"; } +// Historical PRs (closed / merged) only attach when the workspace HEAD still +// points at the PR head commit — otherwise a long-closed PR on a long-moved +// branch (e.g. a closed PR on `master`) keeps showing up on every poll. +// Matches V1 `shouldAcceptPRMatch` in `pr-resolution.ts`. +function shouldAttachHistoricalMatch( + match: { state: PullRequestState; headSha: string }, + workspaceHeadSha: string | null, +): boolean { + if (match.state === "open" || match.state === "draft") return true; + if (!workspaceHeadSha) return false; + return match.headSha.toLowerCase() === workspaceHeadSha.trim().toLowerCase(); +} + function deriveCheckoutPullRequestUpstream( repo: NormalizedRepoIdentity, pr: CheckoutPullRequestMetadata, @@ -669,7 +682,7 @@ export class PullRequestRuntimeManager { continue; } const match = keyToPullRequest.get(key); - if (match) { + if (match && shouldAttachHistoricalMatch(match, workspace.headSha)) { this.db .update(workspaces) .set({ pullRequestId: match.id }) @@ -926,10 +939,16 @@ export class PullRequestRuntimeManager { wantedRefs: Map, options: { bypassCache?: boolean } = {}, ): Promise<{ - matched: Map; + matched: Map< + string, + { id: string; state: PullRequestState; headSha: string } + >; failedKeys: Set; }> { - const matched = new Map(); + const matched = new Map< + string, + { id: string; state: PullRequestState; headSha: string } + >(); const failedKeys = new Set(); if (wantedRefs.size === 0) return { matched, failedKeys }; @@ -1031,6 +1050,7 @@ export class PullRequestRuntimeManager { const reviewDecision = reviewDecisionByNumber.has(node.number) ? mapReviewDecision(reviewDecisionByNumber.get(node.number) ?? null) : coerceReviewDecision(existing?.reviewDecision ?? null); + const mappedState = mapPullRequestState(node.state, node.isDraft); const rowId = this.upsertPullRequestRow({ existing, projectId, @@ -1038,7 +1058,7 @@ export class PullRequestRuntimeManager { repo, url: node.url, title: node.title, - state: mapPullRequestState(node.state, node.isDraft), + state: mappedState, isDraft: node.isDraft, headBranch: node.headRefName, headSha: node.headRefOid, @@ -1050,7 +1070,11 @@ export class PullRequestRuntimeManager { now, }); - matched.set(key, { id: rowId }); + matched.set(key, { + id: rowId, + state: mappedState, + headSha: node.headRefOid, + }); } return { matched, failedKeys }; diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts index a97e2062f25..c64a9def16c 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts @@ -91,6 +91,66 @@ describe("GitHub pull request REST queries", () => { ]); }); + test("prefers an open PR over a more recently updated closed PR (#4513)", async () => { + const { execGh } = createExecGh([ + [ + { + number: 100, + title: "Stale closed", + html_url: "https://github.com/superset-sh/superset/pull/100", + state: "closed", + draft: false, + merged_at: null, + updated_at: "2026-05-09T12:00:00Z", + head: { + ref: "feat/x", + sha: "closed-sha", + repo: { + name: "superset", + owner: { login: "superset-sh" }, + }, + }, + base: { + repo: { + full_name: "superset-sh/superset", + }, + }, + }, + { + number: 101, + title: "Active open", + html_url: "https://github.com/superset-sh/superset/pull/101", + state: "open", + draft: false, + merged_at: null, + updated_at: "2026-05-08T12:00:00Z", + head: { + ref: "feat/x", + sha: "open-sha", + repo: { + name: "superset", + owner: { login: "superset-sh" }, + }, + }, + base: { + repo: { + full_name: "superset-sh/superset", + }, + }, + }, + ], + ]); + + const result = await fetchPullRequestByHeadFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + { owner: "superset-sh", repo: "superset", branch: "feat/x" }, + ); + + expect(result?.number).toBe(101); + expect(result?.state).toBe("OPEN"); + }); + test("filters REST head candidates by exact upstream repository", async () => { const { execGh } = createExecGh([ [ diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts index ab1b33eb05b..a272246b572 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts @@ -118,24 +118,40 @@ function headKey( return `${owner.toLowerCase()}/${repo.toLowerCase()}#${branch}`; } +function pullRequestStateRank(state: PullRequestState): number { + if (state === "OPEN") return 2; + if (state === "MERGED") return 1; + return 0; +} + function normalizePullRequestCandidates( raw: unknown, head: GitHubPullRequestHeadRef, ): GitHubPullRequestNode | null { const requestedKey = headKey(head.owner, head.repo, head.branch); - return ( - asArray(raw) - .map((item) => normalizePullRequest(item)) - .find( - (node) => - node && - headKey( - node.headRepositoryOwner?.login, - node.headRepository?.name, - node.headRefName, - ) === requestedKey, - ) ?? null - ); + const matches = asArray(raw) + .map((item) => normalizePullRequest(item)) + .filter( + (node): node is GitHubPullRequestNode => + node !== null && + headKey( + node.headRepositoryOwner?.login, + node.headRepository?.name, + node.headRefName, + ) === requestedKey, + ); + + // Prefer OPEN > MERGED > CLOSED so an active PR isn't shadowed by a more + // recently updated closed one. Ties fall back to updatedAt desc (matching + // the API's sort=updated&direction=desc). + matches.sort((left, right) => { + const stateDelta = + pullRequestStateRank(right.state) - pullRequestStateRank(left.state); + if (stateDelta !== 0) return stateDelta; + return Date.parse(right.updatedAt) - Date.parse(left.updatedAt); + }); + + return matches[0] ?? null; } function mapReviewDecision(