From 24ecf57b67a10378ffffdcbfe3a58a251a04b085 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 25 Apr 2026 00:00:00 -0700 Subject: [PATCH 01/10] Recover PR checkout from archived refs --- .../$pendingId/buildIntentPayload.test.ts | 18 +- .../pending/$pendingId/buildIntentPayload.ts | 12 +- .../useCheckoutDashboardWorkspace.ts | 1 + .../workspace-creation/procedures/checkout.ts | 61 +++-- .../get-github-pull-request-content.ts | 3 +- .../trpc/router/workspace-creation/schemas.ts | 2 + .../utils/pr-branch-name.test.ts | 11 + .../utils/pr-branch-name.ts | 6 + .../utils/pr-checkout-recovery.test.ts | 218 ++++++++++++++++++ .../utils/pr-checkout-recovery.ts | 167 ++++++++++++++ 10 files changed, 466 insertions(+), 33 deletions(-) create mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts create mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts index 9d51cc26f84..d361e5a02f2 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts @@ -192,6 +192,7 @@ describe("buildPrCheckoutPayload", () => { url: "https://github.com/o/r/pull/42", title: "Fix typo", branch: "fix/typo", + headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseBranch: "main", headRepositoryOwner: "kietho", isCrossRepository: true, @@ -217,6 +218,7 @@ describe("buildPrCheckoutPayload", () => { url: "https://github.com/o/r/pull/42", title: "Fix typo", headRefName: "fix/typo", + headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseRefName: "main", headRepositoryOwner: "kietho", isCrossRepository: true, @@ -285,15 +287,15 @@ describe("buildPrCheckoutPayload", () => { expect(payload.pr?.state).toBe("open"); }); - test("throws clear error for cross-repo PR with deleted fork (null owner)", () => { + test("allows cross-repo PR with deleted fork so host-service can recover from refs/pull", () => { const pending = makePending({ intent: "pr-checkout" }); - expect(() => - buildPrCheckoutPayload("pid", pending, { - ...prContent, - headRepositoryOwner: null, - isCrossRepository: true, - }), - ).toThrow("head fork repository has been deleted"); + const payload = buildPrCheckoutPayload("pid", pending, { + ...prContent, + headRepositoryOwner: null, + isCrossRepository: true, + }); + expect(payload.pr?.headRepositoryOwner).toBe(""); + expect(payload.pr?.isCrossRepository).toBe(true); }); test("same-repo PR with null owner is fine (owner not needed)", () => { diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts index 81c2f57df71..eff2f13f2a4 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts @@ -97,22 +97,13 @@ export function buildPrCheckoutPayload( url: string; title: string; branch: string; // headRefName + headRefOid: string; baseBranch: string; // baseRefName headRepositoryOwner: string | null; isCrossRepository: boolean; state: string; }, ): CheckoutWorkspaceInput { - // Null owner on a cross-repo PR means the head fork repo has been - // deleted. We can't derive `/` without it, and - // `gh pr checkout` wouldn't have a fork to configure push against. - // Fail early with a clear error rather than a cryptic server-side - // "headRepositoryOwner is required". - if (prContent.isCrossRepository && !prContent.headRepositoryOwner) { - throw new Error( - `Cannot check out PR #${prContent.number}: the head fork repository has been deleted.`, - ); - } const linked = mapLinkedContextFromPending(pending); const normalizedState: "open" | "closed" | "merged" = prContent.state === "closed" @@ -130,6 +121,7 @@ export function buildPrCheckoutPayload( url: prContent.url, title: prContent.title, headRefName: prContent.branch, + headRefOid: prContent.headRefOid, baseRefName: prContent.baseBranch, // Same-repo PRs don't need an owner for branch derivation; pass an // empty string rather than leaking null into the server input. diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts index 30a9bf145da..4ea7e56317f 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts @@ -18,6 +18,7 @@ export interface CheckoutWorkspaceInput { url: string; title: string; headRefName: string; + headRefOid: string; baseRefName: string; headRepositoryOwner: string; isCrossRepository: boolean; diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index 1483cd3652b..c2e08afba38 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -13,6 +13,10 @@ import { clearProgress, setProgress } from "../shared/progress-store"; import { safeResolveWorktreePath } from "../shared/worktree-paths"; import { execGh } from "../utils/exec-gh"; import { derivePrLocalBranchName } from "../utils/pr-branch-name"; +import { + getErrorMessage, + recoverPrCheckoutAfterGhFailure, +} from "../utils/pr-checkout-recovery"; export const checkout = protectedProcedure .input(checkoutInputSchema) @@ -101,6 +105,7 @@ export const checkout = protectedProcedure }); } + let prCheckoutRecoveryWarning: string | null = null; try { await execGh( [ @@ -114,21 +119,46 @@ export const checkout = protectedProcedure { cwd: worktreePath, timeout: 120_000 }, ); } catch (err) { - await git - .raw(["worktree", "remove", "--force", worktreePath]) - .catch((rollbackErr) => { - console.warn( - "[workspaceCreation.checkout] failed to rollback PR worktree", - { worktreePath, err: rollbackErr }, - ); + try { + const recovery = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath, + branch, + prNumber: input.pr.number, + remoteName: localProject.remoteName ?? "origin", + expectedHeadOid: input.pr.headRefOid, + error: err, }); - clearProgress(input.pendingId); - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `gh pr checkout failed: ${ - err instanceof Error ? err.message : String(err) - }`, - }); + if (!recovery.recovered) { + throw err; + } + prCheckoutRecoveryWarning = recovery.warning; + } catch (recoveryErr) { + await git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch((rollbackErr) => { + console.warn( + "[workspaceCreation.checkout] failed to rollback PR worktree", + { worktreePath, err: rollbackErr }, + ); + }); + clearProgress(input.pendingId); + const recoveryMessage = + recoveryErr === err + ? "" + : ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryErr)}`; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, + }); + } + } + + if (prCheckoutRecoveryWarning) { + console.warn( + "[workspaceCreation.checkout] recovered failed gh pr checkout", + { prNumber: input.pr.number, branch }, + ); } // Push ergonomics. `gh pr checkout` sets per-branch push config @@ -141,6 +171,9 @@ export const checkout = protectedProcedure ); const extraWarnings: string[] = []; + if (prCheckoutRecoveryWarning) { + extraWarnings.push(prCheckoutRecoveryWarning); + } if (input.pr.state !== "open") { extraWarnings.push( `PR is ${input.pr.state} — commits are included, but the PR may not merge.`, diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts index 63ea9a19ea4..f78ea329567 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts @@ -19,7 +19,7 @@ export const getGitHubPullRequestContent = protectedProcedure "--repo", `${repo.owner}/${repo.name}`, "--json", - "number,title,body,url,state,author,headRefName,baseRefName,headRepositoryOwner,isCrossRepository,isDraft,createdAt,updatedAt", + "number,title,body,url,state,author,headRefName,headRefOid,baseRefName,headRepositoryOwner,isCrossRepository,isDraft,createdAt,updatedAt", ]); const data = pullRequestContentSchema.parse(raw); return { @@ -29,6 +29,7 @@ export const getGitHubPullRequestContent = protectedProcedure url: data.url, state: data.state.toLowerCase(), branch: data.headRefName, + headRefOid: data.headRefOid, baseBranch: data.baseRefName, headRepositoryOwner: data.headRepositoryOwner?.login ?? null, isCrossRepository: data.isCrossRepository, diff --git a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts index bf82f949bc0..776da0c9f35 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts @@ -65,6 +65,7 @@ const checkoutPrSchema = z.object({ url: z.string().url(), title: z.string(), headRefName: z.string(), + headRefOid: z.string().min(1), baseRefName: z.string(), headRepositoryOwner: z.string(), isCrossRepository: z.boolean(), @@ -143,6 +144,7 @@ export const pullRequestContentSchema = z.object({ url: z.string(), state: z.string(), headRefName: z.string(), + headRefOid: z.string().min(1), baseRefName: z.string(), // `gh pr view` returns null when the PR's head fork repository has been // deleted. Nullable so the schema parse doesn't fail; consumers decide diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts index b1547db43a5..c38e564c375 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts @@ -111,4 +111,15 @@ describe("derivePrLocalBranchName", () => { }), ).toThrow("headRepositoryOwner is required"); }); + + test("cross-repo with empty owner falls back to pr number when available", () => { + expect( + derivePrLocalBranchName({ + number: 3711, + headRefName: "foo", + headRepositoryOwner: "", + isCrossRepository: true, + }), + ).toBe("pr/3711"); + }); }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts index a6d702c91bf..87d2cc36388 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts @@ -4,10 +4,13 @@ * Same-repo PRs use the head ref as-is. Cross-repo (fork) PRs get the * fork owner as a lowercase prefix — avoids collisions with local/upstream * branches of the same name and namespaces by author. + * If GitHub no longer reports the fork owner, fall back to `pr/` so + * the checkout can still recover from GitHub's synthetic PR ref. * * Mirrors v1 (`apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:1630`). */ export function derivePrLocalBranchName(pr: { + number?: number; headRefName: string; headRepositoryOwner: string; isCrossRepository: boolean; @@ -19,6 +22,9 @@ export function derivePrLocalBranchName(pr: { if (pr.isCrossRepository) { const owner = pr.headRepositoryOwner.trim().toLowerCase(); if (!owner) { + if (pr.number) { + return `pr/${pr.number}`; + } throw new Error( "derivePrLocalBranchName: headRepositoryOwner is required for cross-repo PRs", ); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts new file mode 100644 index 00000000000..731067f2383 --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts @@ -0,0 +1,218 @@ +import { describe, expect, mock, test } from "bun:test"; +import type { GitClient } from "../shared/types"; +import { + getPrCheckoutRecoveryKind, + getSyntheticPrHeadRef, + recoverPrCheckoutAfterGhFailure, +} from "./pr-checkout-recovery"; + +const EXPECTED_HEAD_OID = "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a"; + +function createMockGit(fetchHeadOid = EXPECTED_HEAD_OID) { + const raw = mock(async (args: string[]) => { + if (args.includes("rev-parse")) { + return `${fetchHeadOid}\n`; + } + return ""; + }); + return { + git: { raw } as unknown as GitClient, + raw, + }; +} + +describe("getPrCheckoutRecoveryKind", () => { + test("recovers missing PR head branch via the synthetic PR ref", () => { + expect( + getPrCheckoutRecoveryKind( + new Error( + "Command failed: gh pr checkout 3711 --branch debug-pty-termination --force\nfatal: couldn't find remote ref refs/heads/debug-pty-termination\nfailed to run git: exit status 128", + ), + ), + ).toBe("synthetic-pr-ref"); + }); + + test("recovers deleted or inaccessible fork via the synthetic PR ref", () => { + expect( + getPrCheckoutRecoveryKind( + new Error("fatal: could not read from remote repository"), + ), + ).toBe("synthetic-pr-ref"); + expect( + getPrCheckoutRecoveryKind(new Error("remote: Repository not found")), + ).toBe("synthetic-pr-ref"); + }); + + test("recovers gh tracking failures via FETCH_HEAD", () => { + expect( + getPrCheckoutRecoveryKind( + new Error("fatal: 'origin/user/feature' is not a branch"), + ), + ).toBe("fetch-head"); + }); + + test("does not recover auth or PR lookup failures", () => { + expect( + getPrCheckoutRecoveryKind(new Error("not logged in to GitHub")), + ).toBeNull(); + expect( + getPrCheckoutRecoveryKind( + new Error("could not find any pull requests matching 99999"), + ), + ).toBeNull(); + }); +}); + +describe("getSyntheticPrHeadRef", () => { + test("builds the GitHub PR head ref", () => { + expect(getSyntheticPrHeadRef(3711)).toBe("refs/pull/3711/head"); + }); +}); + +describe("recoverPrCheckoutAfterGhFailure", () => { + test("fetches and verifies refs/pull/N/head before checking out missing head branches", async () => { + const { git, raw } = createMockGit(); + const result = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath: "/tmp/worktree", + branch: "debug-pty-termination", + prNumber: 3711, + remoteName: "origin", + expectedHeadOid: EXPECTED_HEAD_OID, + error: new Error( + "fatal: couldn't find remote ref refs/heads/debug-pty-termination", + ), + }); + + expect(result.recovered).toBe(true); + expect(raw).toHaveBeenNthCalledWith(1, [ + "-C", + "/tmp/worktree", + "fetch", + "--no-tags", + "--quiet", + "origin", + "refs/pull/3711/head", + ]); + expect(raw).toHaveBeenNthCalledWith(2, [ + "-C", + "/tmp/worktree", + "rev-parse", + "--verify", + "FETCH_HEAD^{commit}", + ]); + expect(raw).toHaveBeenNthCalledWith(3, [ + "-C", + "/tmp/worktree", + "checkout", + "-B", + "debug-pty-termination", + "--no-track", + "FETCH_HEAD", + ]); + }); + + test("uses configured base remote when fetching synthetic PR refs", async () => { + const { git, raw } = createMockGit(); + await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath: "/tmp/worktree", + branch: "debug-pty-termination", + prNumber: 3711, + remoteName: "upstream", + expectedHeadOid: EXPECTED_HEAD_OID, + error: new Error( + "fatal: couldn't find remote ref refs/heads/debug-pty-termination", + ), + }); + + expect(raw).toHaveBeenNthCalledWith(1, [ + "-C", + "/tmp/worktree", + "fetch", + "--no-tags", + "--quiet", + "upstream", + "refs/pull/3711/head", + ]); + }); + + test("uses verified FETCH_HEAD directly for gh tracking failures", async () => { + const { git, raw } = createMockGit(); + const result = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath: "/tmp/worktree", + branch: "user/feature", + prNumber: 42, + remoteName: "origin", + expectedHeadOid: EXPECTED_HEAD_OID, + error: new Error("fatal: 'origin/user/feature' is not a branch"), + }); + + expect(result.recovered).toBe(true); + expect(raw).toHaveBeenCalledTimes(2); + expect(raw).toHaveBeenNthCalledWith(1, [ + "-C", + "/tmp/worktree", + "rev-parse", + "--verify", + "FETCH_HEAD^{commit}", + ]); + expect(raw).toHaveBeenNthCalledWith(2, [ + "-C", + "/tmp/worktree", + "checkout", + "-B", + "user/feature", + "--no-track", + "FETCH_HEAD", + ]); + }); + + test("does not checkout when fetched PR ref does not match GitHub headRefOid", async () => { + const { git, raw } = createMockGit( + "1111111111111111111111111111111111111111", + ); + + await expect( + recoverPrCheckoutAfterGhFailure({ + git, + worktreePath: "/tmp/worktree", + branch: "debug-pty-termination", + prNumber: 3711, + remoteName: "origin", + expectedHeadOid: EXPECTED_HEAD_OID, + error: new Error( + "fatal: couldn't find remote ref refs/heads/debug-pty-termination", + ), + }), + ).rejects.toThrow("did not match GitHub headRefOid"); + + expect(raw).toHaveBeenCalledTimes(2); + expect(raw).not.toHaveBeenCalledWith([ + "-C", + "/tmp/worktree", + "checkout", + "-B", + "debug-pty-termination", + "--no-track", + "FETCH_HEAD", + ]); + }); + + test("returns false for unrecoverable errors", async () => { + const { git, raw } = createMockGit(); + const result = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath: "/tmp/worktree", + branch: "feature", + prNumber: 42, + remoteName: "origin", + expectedHeadOid: EXPECTED_HEAD_OID, + error: new Error("not logged in to GitHub"), + }); + + expect(result).toEqual({ recovered: false }); + expect(raw).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts new file mode 100644 index 00000000000..e245bcc13d8 --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts @@ -0,0 +1,167 @@ +import type { GitClient } from "../shared/types"; + +export type PrCheckoutRecoveryKind = "fetch-head" | "synthetic-pr-ref"; + +export type PrCheckoutRecoveryResult = + | { recovered: true; warning: string } + | { recovered: false }; + +export function getErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +export function getPrCheckoutRecoveryKind( + error: unknown, +): PrCheckoutRecoveryKind | null { + const message = getErrorMessage(error).toLowerCase(); + + if (message.includes("is not a branch")) { + return "fetch-head"; + } + + if ( + message.includes("couldn't find remote ref") || + message.includes("could not read from remote repository") || + message.includes("does not appear to be a git repository") || + message.includes("repository not found") + ) { + return "synthetic-pr-ref"; + } + + return null; +} + +export function getSyntheticPrHeadRef(prNumber: number): string { + return `refs/pull/${prNumber}/head`; +} + +async function revParseFetchHead({ + git, + worktreePath, +}: { + git: GitClient; + worktreePath: string; +}): Promise { + const oid = await git.raw([ + "-C", + worktreePath, + "rev-parse", + "--verify", + "FETCH_HEAD^{commit}", + ]); + return oid.trim(); +} + +async function assertFetchHeadMatchesExpectedOid({ + git, + worktreePath, + expectedHeadOid, +}: { + git: GitClient; + worktreePath: string; + expectedHeadOid: string; +}): Promise { + const actualOid = await revParseFetchHead({ git, worktreePath }); + if (actualOid.toLowerCase() !== expectedHeadOid.trim().toLowerCase()) { + throw new Error( + `Fetched PR head ${actualOid} did not match GitHub headRefOid ${expectedHeadOid}`, + ); + } +} + +export async function fetchSyntheticPrHead({ + git, + worktreePath, + remoteName, + prNumber, +}: { + git: GitClient; + worktreePath: string; + remoteName: string; + prNumber: number; +}): Promise { + await git.raw([ + "-C", + worktreePath, + "fetch", + "--no-tags", + "--quiet", + remoteName, + getSyntheticPrHeadRef(prNumber), + ]); +} + +export async function checkoutFetchHeadAsBranch({ + git, + worktreePath, + branch, +}: { + git: GitClient; + worktreePath: string; + branch: string; +}): Promise { + await git.raw([ + "-C", + worktreePath, + "checkout", + "-B", + branch, + "--no-track", + "FETCH_HEAD", + ]); +} + +/** + * Recover from `gh pr checkout` failures that still have a safe git fallback. + * + * GitHub Desktop uses the same broad strategy: resolve/fetch the PR ref first, + * then create a local branch from that ref instead of depending only on a + * named head branch. This is especially important after a PR has been merged + * and the source branch or fork has been deleted. + */ +export async function recoverPrCheckoutAfterGhFailure({ + git, + worktreePath, + branch, + prNumber, + remoteName, + expectedHeadOid, + error, +}: { + git: GitClient; + worktreePath: string; + branch: string; + prNumber: number; + remoteName: string; + expectedHeadOid: string; + error: unknown; +}): Promise { + const kind = getPrCheckoutRecoveryKind(error); + if (!kind) return { recovered: false }; + + if (kind === "synthetic-pr-ref") { + await fetchSyntheticPrHead({ git, worktreePath, remoteName, prNumber }); + await assertFetchHeadMatchesExpectedOid({ + git, + worktreePath, + expectedHeadOid, + }); + await checkoutFetchHeadAsBranch({ git, worktreePath, branch }); + return { + recovered: true, + warning: `The PR head branch was unavailable, so Superset checked out GitHub's archived PR ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, + }; + } + + await assertFetchHeadMatchesExpectedOid({ + git, + worktreePath, + expectedHeadOid, + }); + await checkoutFetchHeadAsBranch({ git, worktreePath, branch }); + return { + recovered: true, + warning: + "gh pr checkout could not attach upstream tracking for this PR branch, so Superset checked out FETCH_HEAD with no upstream.", + }; +} From 0d1db3e64dac1aa560720198c059ec836bbb66c9 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 25 Apr 2026 00:23:57 -0700 Subject: [PATCH 02/10] [codex] Link PR checkouts to sidebar metadata --- .../$pendingId/buildIntentPayload.test.ts | 6 + .../pending/$pendingId/buildIntentPayload.ts | 6 + .../useCheckoutDashboardWorkspace.ts | 3 + .../src/runtime/pull-requests/index.ts | 1 + .../pull-requests/pull-requests.test.ts | 215 ++++++++++++++++ .../runtime/pull-requests/pull-requests.ts | 235 +++++++++++++++--- .../workspace-creation/procedures/checkout.ts | 19 +- .../get-github-pull-request-content.ts | 3 +- .../trpc/router/workspace-creation/schemas.ts | 4 + .../shared/finish-checkout.ts | 20 ++ 10 files changed, 474 insertions(+), 38 deletions(-) create mode 100644 packages/host-service/src/runtime/pull-requests/pull-requests.test.ts diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts index d361e5a02f2..84e4810697c 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts @@ -195,8 +195,11 @@ describe("buildPrCheckoutPayload", () => { headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseBranch: "main", headRepositoryOwner: "kietho", + headRepositoryName: "r", isCrossRepository: true, + isDraft: false, state: "open", + updatedAt: "2026-04-13T00:00:00Z", body: "body text", }; @@ -221,8 +224,11 @@ describe("buildPrCheckoutPayload", () => { headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseRefName: "main", headRepositoryOwner: "kietho", + headRepositoryName: "r", isCrossRepository: true, + isDraft: false, state: "open", + updatedAt: "2026-04-13T00:00:00Z", }); expect(payload.branch).toBeUndefined(); }); diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts index eff2f13f2a4..414a249c680 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts @@ -100,8 +100,11 @@ export function buildPrCheckoutPayload( headRefOid: string; baseBranch: string; // baseRefName headRepositoryOwner: string | null; + headRepositoryName?: string | null; isCrossRepository: boolean; + isDraft?: boolean; state: string; + updatedAt?: string; }, ): CheckoutWorkspaceInput { const linked = mapLinkedContextFromPending(pending); @@ -126,8 +129,11 @@ export function buildPrCheckoutPayload( // Same-repo PRs don't need an owner for branch derivation; pass an // empty string rather than leaking null into the server input. headRepositoryOwner: prContent.headRepositoryOwner ?? "", + headRepositoryName: prContent.headRepositoryName ?? null, isCrossRepository: prContent.isCrossRepository, + isDraft: prContent.isDraft ?? false, state: normalizedState, + updatedAt: prContent.updatedAt, }, composer: { prompt: pending.prompt || undefined, diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts index 4ea7e56317f..04b597d2041 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts @@ -21,8 +21,11 @@ export interface CheckoutWorkspaceInput { headRefOid: string; baseRefName: string; headRepositoryOwner: string; + headRepositoryName?: string | null; isCrossRepository: boolean; + isDraft?: boolean; state: "open" | "closed" | "merged"; + updatedAt?: string; }; composer: { prompt?: string; diff --git a/packages/host-service/src/runtime/pull-requests/index.ts b/packages/host-service/src/runtime/pull-requests/index.ts index 4ea1fd133a5..648909b52cb 100644 --- a/packages/host-service/src/runtime/pull-requests/index.ts +++ b/packages/host-service/src/runtime/pull-requests/index.ts @@ -1,4 +1,5 @@ export { + type CheckoutPullRequestMetadata, PullRequestRuntimeManager, type PullRequestRuntimeManagerOptions, type PullRequestStateSnapshot, 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 new file mode 100644 index 00000000000..73a2518fb3c --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -0,0 +1,215 @@ +import { describe, expect, test } from "bun:test"; +import { pullRequests, workspaces } from "../../db/schema"; +import { PullRequestRuntimeManager } from "./pull-requests"; + +const PROJECT_ID = "project-1"; +const WORKSPACE_ID = "workspace-1"; + +interface FakeProject { + id: string; + repoPath: string; + repoProvider: "github"; + repoOwner: string; + repoName: string; + repoUrl: string; + remoteName: string; +} + +interface FakeWorkspace { + id: string; + projectId: string; + worktreePath: string; + branch: string; + headSha: string | null; + upstreamOwner: string | null; + upstreamRepo: string | null; + upstreamBranch: string | null; + pullRequestId: string | null; +} + +interface FakePullRequest { + id: string; + projectId: string; + repoProvider: "github"; + repoOwner: string; + repoName: string; + prNumber: number; + url: string; + title: string; + state: string; + isDraft: boolean; + headBranch: string; + headSha: string; + reviewDecision: string | null; + checksStatus: string; + checksJson: string; + lastFetchedAt: number | null; + error: string | null; + createdAt: number; + updatedAt: number; +} + +interface FakeState { + project: FakeProject; + workspace: FakeWorkspace; + pullRequest: FakePullRequest | undefined; +} + +function makeState(branch: string): FakeState { + return { + project: { + id: PROJECT_ID, + repoPath: "/repo", + repoProvider: "github", + repoOwner: "base-owner", + repoName: "base-repo", + repoUrl: "https://github.com/base-owner/base-repo.git", + remoteName: "origin", + }, + workspace: { + id: WORKSPACE_ID, + projectId: PROJECT_ID, + worktreePath: `/repo/.worktrees/${branch}`, + branch, + headSha: null, + upstreamOwner: null, + upstreamRepo: null, + upstreamBranch: null, + pullRequestId: null, + }, + pullRequest: undefined, + }; +} + +function createFakeDb(state: FakeState) { + return { + query: { + projects: { + findFirst: () => ({ sync: () => state.project }), + }, + pullRequests: { + findFirst: () => ({ sync: () => state.pullRequest }), + }, + }, + insert: (table: unknown) => ({ + values: (values: FakePullRequest) => ({ + run: () => { + if (table === pullRequests) { + state.pullRequest = values; + } + }, + }), + }), + update: (table: unknown) => ({ + set: (values: Partial | Partial) => ({ + where: () => ({ + run: () => { + if (table === workspaces) { + state.workspace = { + ...state.workspace, + ...(values as Partial), + }; + } + if (table === pullRequests && state.pullRequest) { + state.pullRequest = { + ...state.pullRequest, + ...(values as Partial), + }; + } + }, + }), + }), + }), + select: (shape?: unknown) => ({ + from: (table: unknown) => ({ + where: () => ({ + all: () => { + if (table !== workspaces) return []; + if (shape) return [{ projectId: state.workspace.projectId }]; + return [state.workspace]; + }, + }), + all: () => { + if (table !== workspaces) return []; + if (shape) return [{ projectId: state.workspace.projectId }]; + return [state.workspace]; + }, + }), + }), + }; +} + +function createManager(state: FakeState) { + return new PullRequestRuntimeManager({ + db: createFakeDb(state) as never, + git: async () => { + throw new Error("git should not be used when project metadata is set"); + }, + github: async () => { + throw new Error("github should not be used for direct PR linking"); + }, + }); +} + +describe("PullRequestRuntimeManager direct checkout PR linking", () => { + test("links a fork PR workspace to the selected PR and records fork upstream", async () => { + const state = makeState("fork-owner/fix-typo"); + const manager = createManager(state); + + const prId = await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Fix typo", + state: "open", + isDraft: false, + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: "fork-owner", + headRepositoryName: "fork-repo", + isCrossRepository: true, + }, + }); + + expect(state.workspace.pullRequestId).toBe(prId); + expect(state.workspace.upstreamOwner).toBe("fork-owner"); + expect(state.workspace.upstreamRepo).toBe("fork-repo"); + expect(state.workspace.upstreamBranch).toBe("fix-typo"); + expect(state.pullRequest?.prNumber).toBe(42); + expect(state.pullRequest?.repoOwner).toBe("base-owner"); + expect(state.pullRequest?.repoName).toBe("base-repo"); + expect(state.pullRequest?.headBranch).toBe("fix-typo"); + }); + + test("keeps a deleted-fork PR link when no upstream can be recorded", async () => { + const state = makeState("pr/42"); + const manager = createManager(state); + + const prId = await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Deleted fork", + state: "merged", + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: null, + headRepositoryName: null, + isCrossRepository: true, + }, + }); + + expect(state.workspace.pullRequestId).toBe(prId); + expect(state.workspace.upstreamOwner).toBeNull(); + expect(state.workspace.upstreamRepo).toBeNull(); + expect(state.workspace.upstreamBranch).toBeNull(); + + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + + expect(state.workspace.pullRequestId).toBe(prId); + }); +}); 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 0a33bed5740..cf402ca7d82 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -190,6 +190,45 @@ interface NormalizedRepoIdentity { remoteName: string; } +type PullRequestRow = typeof pullRequests.$inferSelect; + +export interface CheckoutPullRequestMetadata { + number: number; + url: string; + title: string; + state: "open" | "closed" | "merged"; + isDraft?: boolean; + headRefName: string; + headRefOid: string; + headRepositoryOwner?: string | null; + headRepositoryName?: string | null; + isCrossRepository: boolean; +} + +function mapCheckoutPullRequestState( + state: CheckoutPullRequestMetadata["state"], + isDraft: boolean, +): PullRequestState { + if (state === "merged") return "merged"; + if (state === "closed") return "closed"; + if (isDraft) return "draft"; + return "open"; +} + +function deriveCheckoutPullRequestUpstream( + repo: NormalizedRepoIdentity, + pr: CheckoutPullRequestMetadata, +): { owner: string; name: string; branch: string } | null { + if (!pr.isCrossRepository) { + return { owner: repo.owner, name: repo.name, branch: pr.headRefName }; + } + + const owner = pr.headRepositoryOwner?.trim(); + const name = pr.headRepositoryName?.trim(); + if (!owner || !name) return null; + return { owner, name, branch: pr.headRefName }; +} + export class PullRequestRuntimeManager { private readonly db: HostDb; private readonly git: GitFactory; @@ -291,6 +330,57 @@ export class PullRequestRuntimeManager { ); } + async linkWorkspaceToCheckoutPullRequest({ + workspaceId, + projectId, + pullRequest, + }: { + workspaceId: string; + projectId: string; + pullRequest: CheckoutPullRequestMetadata; + }): Promise { + const repo = await this.getProjectRepository(projectId); + if (!repo) return null; + + const existing = this.findPullRequestRow(repo, pullRequest.number); + const existingChecks = parseChecksJson(existing?.checksJson ?? null); + const now = Date.now(); + const isDraft = pullRequest.isDraft ?? false; + const rowId = this.upsertPullRequestRow({ + existing, + projectId, + repo, + prNumber: pullRequest.number, + url: pullRequest.url, + title: pullRequest.title, + state: mapCheckoutPullRequestState(pullRequest.state, isDraft), + isDraft, + headBranch: pullRequest.headRefName, + headSha: pullRequest.headRefOid, + reviewDecision: coerceReviewDecision(existing?.reviewDecision ?? null), + checksStatus: coerceChecksStatus(existing?.checksStatus ?? null), + checksJson: JSON.stringify(existingChecks), + lastFetchedAt: existing?.lastFetchedAt ?? now, + error: null, + now, + }); + + const upstream = deriveCheckoutPullRequestUpstream(repo, pullRequest); + this.db + .update(workspaces) + .set({ + pullRequestId: rowId, + headSha: pullRequest.headRefOid, + upstreamOwner: upstream?.owner ?? null, + upstreamRepo: upstream?.name ?? null, + upstreamBranch: upstream?.branch ?? null, + }) + .where(eq(workspaces.id, workspaceId)) + .run(); + + return rowId; + } + private async syncWorkspaceBranches(): Promise { const allWorkspaces = this.db.select().from(workspaces).all(); const changedProjectIds = new Set(); @@ -307,6 +397,9 @@ export class PullRequestRuntimeManager { const upstreamOwner = upstream?.owner ?? null; const upstreamRepo = upstream?.name ?? null; const upstreamBranch = upstream?.branch ?? null; + const branchChanged = branch !== workspace.branch; + const pullRequestId = + branchChanged && !upstream ? null : workspace.pullRequestId; if ( branch === workspace.branch && @@ -326,6 +419,7 @@ export class PullRequestRuntimeManager { upstreamOwner, upstreamRepo, upstreamBranch, + pullRequestId, }) .where(eq(workspaces.id, workspace.id)) .run(); @@ -434,7 +528,13 @@ export class PullRequestRuntimeManager { workspace.upstreamRepo, workspace.upstreamBranch ?? workspace.branch, ); - const match = key ? keyToPullRequest.get(key) : undefined; + if (!key) { + // PR checkouts recovered from GitHub's archived refs intentionally + // have no upstream. Keep their explicit PR link instead of erasing it + // during the periodic branch-inference refresh. + continue; + } + const match = keyToPullRequest.get(key); this.db .update(workspaces) .set({ pullRequestId: match?.id ?? null }) @@ -501,6 +601,98 @@ export class PullRequestRuntimeManager { }; } + private findPullRequestRow( + repo: NormalizedRepoIdentity, + prNumber: number, + ): PullRequestRow | undefined { + return this.db.query.pullRequests + .findFirst({ + where: and( + eq(pullRequests.repoProvider, repo.provider), + eq(pullRequests.repoOwner, repo.owner), + eq(pullRequests.repoName, repo.name), + eq(pullRequests.prNumber, prNumber), + ), + }) + .sync(); + } + + private upsertPullRequestRow({ + existing, + projectId, + repo, + prNumber, + url, + title, + state, + isDraft, + headBranch, + headSha, + reviewDecision, + checksStatus, + checksJson, + lastFetchedAt, + error, + now, + }: { + existing: PullRequestRow | undefined; + projectId: string; + repo: NormalizedRepoIdentity; + prNumber: number; + url: string; + title: string; + state: PullRequestState; + isDraft: boolean; + headBranch: string; + headSha: string; + reviewDecision: ReviewDecision; + checksStatus: ChecksStatus; + checksJson: string; + lastFetchedAt: number | null; + error: string | null; + now: number; + }): string { + const rowId = existing?.id ?? randomUUID(); + const data = { + projectId, + repoProvider: repo.provider, + repoOwner: repo.owner, + repoName: repo.name, + prNumber, + url, + title, + state, + isDraft, + headBranch, + headSha, + reviewDecision, + checksStatus, + checksJson, + lastFetchedAt, + error, + updatedAt: now, + }; + + if (existing) { + this.db + .update(pullRequests) + .set(data) + .where(eq(pullRequests.id, rowId)) + .run(); + } else { + this.db + .insert(pullRequests) + .values({ + id: rowId, + createdAt: now, + ...data, + }) + .run(); + } + + return rowId; + } + private async fetchRepoPullRequests( projectId: string, repo: NormalizedRepoIdentity, @@ -537,27 +729,15 @@ export class PullRequestRuntimeManager { const now = Date.now(); for (const [key, node] of latestByKey) { - const existing = this.db.query.pullRequests - .findFirst({ - where: and( - eq(pullRequests.repoProvider, repo.provider), - eq(pullRequests.repoOwner, repo.owner), - eq(pullRequests.repoName, repo.name), - eq(pullRequests.prNumber, node.number), - ), - }) - .sync(); - - const rowId = existing?.id ?? randomUUID(); + const existing = this.findPullRequestRow(repo, node.number); const checks = parseCheckContexts( node.statusCheckRollup?.contexts?.nodes ?? [], ); - const data = { + const rowId = this.upsertPullRequestRow({ + existing, projectId, - repoProvider: repo.provider, - repoOwner: repo.owner, - repoName: repo.name, prNumber: node.number, + repo, url: node.url, title: node.title, state: mapPullRequestState(node.state, node.isDraft), @@ -569,25 +749,8 @@ export class PullRequestRuntimeManager { checksJson: JSON.stringify(checks), lastFetchedAt: now, error: null, - updatedAt: now, - }; - - if (existing) { - this.db - .update(pullRequests) - .set(data) - .where(eq(pullRequests.id, rowId)) - .run(); - } else { - this.db - .insert(pullRequests) - .values({ - id: rowId, - createdAt: now, - ...data, - }) - .run(); - } + now, + }); keyToRow.set(key, { id: rowId }); } diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index c2e08afba38..de64c5c869f 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -43,11 +43,27 @@ export const checkout = protectedProcedure }) .sync(); if (existing) { + const warnings: string[] = []; + try { + await ctx.runtime.pullRequests.linkWorkspaceToCheckoutPullRequest({ + workspaceId: existing.id, + projectId: input.projectId, + pullRequest: input.pr, + }); + } catch (err) { + console.warn( + "[workspaceCreation.checkout] failed to link existing workspace PR metadata", + { workspaceId: existing.id, err }, + ); + warnings.push( + "Existing workspace found, but Superset could not link pull request status automatically.", + ); + } clearProgress(input.pendingId); return { workspace: { id: existing.id }, terminals: [], - warnings: [], + warnings, alreadyExists: true as const, }; } @@ -195,6 +211,7 @@ export const checkout = protectedProcedure runSetupScript: input.composer.runSetupScript ?? false, git, extraWarnings, + pullRequest: input.pr, }); } diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts index f78ea329567..44dfef4f5a0 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts @@ -19,7 +19,7 @@ export const getGitHubPullRequestContent = protectedProcedure "--repo", `${repo.owner}/${repo.name}`, "--json", - "number,title,body,url,state,author,headRefName,headRefOid,baseRefName,headRepositoryOwner,isCrossRepository,isDraft,createdAt,updatedAt", + "number,title,body,url,state,author,headRefName,headRefOid,baseRefName,headRepositoryOwner,headRepository,isCrossRepository,isDraft,createdAt,updatedAt", ]); const data = pullRequestContentSchema.parse(raw); return { @@ -32,6 +32,7 @@ export const getGitHubPullRequestContent = protectedProcedure headRefOid: data.headRefOid, baseBranch: data.baseRefName, headRepositoryOwner: data.headRepositoryOwner?.login ?? null, + headRepositoryName: data.headRepository?.name ?? null, isCrossRepository: data.isCrossRepository, author: data.author?.login ?? null, isDraft: data.isDraft, diff --git a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts index 776da0c9f35..b76e90399f9 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts @@ -68,8 +68,11 @@ const checkoutPrSchema = z.object({ headRefOid: z.string().min(1), baseRefName: z.string(), headRepositoryOwner: z.string(), + headRepositoryName: z.string().nullable().optional(), isCrossRepository: z.boolean(), + isDraft: z.boolean().optional(), state: z.enum(["open", "closed", "merged"]), + updatedAt: z.string().optional(), }); export const checkoutInputSchema = z @@ -151,6 +154,7 @@ export const pullRequestContentSchema = z.object({ // how to handle a missing owner (client surfaces a clear error for // cross-repo PRs — same-repo PRs shouldn't see null in practice). headRepositoryOwner: z.object({ login: z.string() }).nullable(), + headRepository: z.object({ name: z.string() }).nullable(), isCrossRepository: z.boolean(), isDraft: z.boolean(), author: z.object({ login: z.string() }).optional(), diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts index d35af97c39e..95ac79577e4 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts @@ -1,6 +1,7 @@ import { getDeviceName, getHashedDeviceId } from "@superset/shared/device-info"; import { TRPCError } from "@trpc/server"; import { workspaces } from "../../../../db/schema"; +import type { CheckoutPullRequestMetadata } from "../../../../runtime/pull-requests"; import type { HostServiceContext } from "../../../../types"; import { clearProgress, setProgress } from "./progress-store"; import { startSetupTerminalIfPresent } from "./setup-terminal"; @@ -27,6 +28,7 @@ export async function finishCheckout( runSetupScript: boolean; git: GitClient; extraWarnings: string[]; + pullRequest?: CheckoutPullRequestMetadata; }, ): Promise { setProgress(args.pendingId, "registering"); @@ -142,6 +144,24 @@ export async function finishCheckout( const terminals: TerminalDescriptor[] = []; const warnings: string[] = [...args.extraWarnings]; + if (args.pullRequest) { + try { + await ctx.runtime.pullRequests.linkWorkspaceToCheckoutPullRequest({ + workspaceId: cloudRow.id, + projectId: args.projectId, + pullRequest: args.pullRequest, + }); + } catch (err) { + console.warn( + "[workspaceCreation.checkout] failed to link checkout PR metadata", + { workspaceId: cloudRow.id, err }, + ); + warnings.push( + "Workspace was created, but Superset could not link pull request status automatically.", + ); + } + } + if (args.runSetupScript) { const { terminal, warning } = startSetupTerminalIfPresent({ ctx, From bffb651a2e569b2337d1a2ff5bd3ba68dae9564c Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 25 Apr 2026 12:47:34 -0700 Subject: [PATCH 03/10] [codex] Tighten PR checkout sidebar linking --- .../$pendingId/buildIntentPayload.test.ts | 2 - .../pending/$pendingId/buildIntentPayload.ts | 2 - .../useCheckoutDashboardWorkspace.ts | 1 - .../pull-requests/pull-requests.test.ts | 26 +++++++++++ .../runtime/pull-requests/pull-requests.ts | 43 ++++++++++++++++--- .../trpc/router/workspace-creation/schemas.ts | 1 - 6 files changed, 64 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts index 84e4810697c..fd880a4ab2e 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts @@ -199,7 +199,6 @@ describe("buildPrCheckoutPayload", () => { isCrossRepository: true, isDraft: false, state: "open", - updatedAt: "2026-04-13T00:00:00Z", body: "body text", }; @@ -228,7 +227,6 @@ describe("buildPrCheckoutPayload", () => { isCrossRepository: true, isDraft: false, state: "open", - updatedAt: "2026-04-13T00:00:00Z", }); expect(payload.branch).toBeUndefined(); }); diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts index 414a249c680..d38fe856543 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts @@ -104,7 +104,6 @@ export function buildPrCheckoutPayload( isCrossRepository: boolean; isDraft?: boolean; state: string; - updatedAt?: string; }, ): CheckoutWorkspaceInput { const linked = mapLinkedContextFromPending(pending); @@ -133,7 +132,6 @@ export function buildPrCheckoutPayload( isCrossRepository: prContent.isCrossRepository, isDraft: prContent.isDraft ?? false, state: normalizedState, - updatedAt: prContent.updatedAt, }, composer: { prompt: pending.prompt || undefined, diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts index 04b597d2041..e18208414a0 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts @@ -25,7 +25,6 @@ export interface CheckoutWorkspaceInput { isCrossRepository: boolean; isDraft?: boolean; state: "open" | "closed" | "merged"; - updatedAt?: string; }; composer: { prompt?: string; 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 73a2518fb3c..e74050c70e0 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 @@ -212,4 +212,30 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { expect(state.workspace.pullRequestId).toBe(prId); }); + + test("clears a no-upstream PR link when workspace HEAD no longer matches the PR", async () => { + const state = makeState("pr/42"); + const manager = createManager(state); + + await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Deleted fork", + state: "merged", + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: null, + headRepositoryName: null, + isCrossRepository: true, + }, + }); + state.workspace.headSha = "def456"; + + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + + expect(state.workspace.pullRequestId).toBeNull(); + }); }); 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 cf402ca7d82..370d632490e 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -397,16 +397,19 @@ export class PullRequestRuntimeManager { const upstreamOwner = upstream?.owner ?? null; const upstreamRepo = upstream?.name ?? null; const upstreamBranch = upstream?.branch ?? null; - const branchChanged = branch !== workspace.branch; const pullRequestId = - branchChanged && !upstream ? null : workspace.pullRequestId; + upstream || + this.pullRequestHeadMatches(workspace.pullRequestId, headSha) + ? workspace.pullRequestId + : null; if ( branch === workspace.branch && headSha === workspace.headSha && upstreamOwner === workspace.upstreamOwner && upstreamRepo === workspace.upstreamRepo && - upstreamBranch === workspace.upstreamBranch + upstreamBranch === workspace.upstreamBranch && + pullRequestId === workspace.pullRequestId ) { continue; } @@ -530,8 +533,23 @@ export class PullRequestRuntimeManager { ); if (!key) { // PR checkouts recovered from GitHub's archived refs intentionally - // have no upstream. Keep their explicit PR link instead of erasing it - // during the periodic branch-inference refresh. + // have no upstream. Keep the explicit PR link only while the + // workspace HEAD still matches the selected PR head. + if ( + this.pullRequestHeadMatches( + workspace.pullRequestId, + workspace.headSha, + ) + ) { + continue; + } + if (workspace.pullRequestId) { + this.db + .update(workspaces) + .set({ pullRequestId: null }) + .where(eq(workspaces.id, workspace.id)) + .run(); + } continue; } const match = keyToPullRequest.get(key); @@ -617,6 +635,21 @@ export class PullRequestRuntimeManager { .sync(); } + private findPullRequestRowById(id: string): PullRequestRow | undefined { + return this.db.query.pullRequests + .findFirst({ where: eq(pullRequests.id, id) }) + .sync(); + } + + private pullRequestHeadMatches( + pullRequestId: string | null, + headSha: string | null, + ): boolean { + if (!pullRequestId || !headSha) return false; + const pr = this.findPullRequestRowById(pullRequestId); + return pr?.headSha.toLowerCase() === headSha.trim().toLowerCase(); + } + private upsertPullRequestRow({ existing, projectId, diff --git a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts index b76e90399f9..71c0e3475ad 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts @@ -72,7 +72,6 @@ const checkoutPrSchema = z.object({ isCrossRepository: z.boolean(), isDraft: z.boolean().optional(), state: z.enum(["open", "closed", "merged"]), - updatedAt: z.string().optional(), }); export const checkoutInputSchema = z From 3b898ba2cdda72f23f3e264bc41663876b00ba13 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 17:01:01 -0700 Subject: [PATCH 04/10] test(host-service): integration tests for PR checkout recovery against real git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exercises `recoverPrCheckoutAfterGhFailure` end-to-end against an on-disk bare remote with a synthetic refs/pull/N/head ref and the named branch deleted — the scenario the recovery code is designed for. Unit tests mock git.raw; these tests catch breakage in the actual git plumbing (fetch wire format, simple-git arg shape, --no-track honoring). Verified the synthetic-pr-ref recovery, OID mismatch abort, fetch-head fallback, and unrecoverable-error passthrough each fail when the recovery body is stubbed to return {recovered: false}. --- .../pr-checkout-recovery.integration.test.ts | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts diff --git a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts new file mode 100644 index 00000000000..371019b1be4 --- /dev/null +++ b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts @@ -0,0 +1,261 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import simpleGit, { type SimpleGit } from "simple-git"; +import { recoverPrCheckoutAfterGhFailure } from "../../src/trpc/router/workspace-creation/utils/pr-checkout-recovery"; +import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; + +/** + * End-to-end exercise of `recoverPrCheckoutAfterGhFailure` against real + * `git fetch` / `git checkout`. The unit tests use a mocked `git.raw`; + * these tests run against an on-disk bare repo with a synthetic + * `refs/pull//head` ref to verify the actual git plumbing works — + * the most likely place for silent breakage when GitHub's wire format + * or simple-git's behavior shifts. + */ + +interface BareRemoteFixture { + bareRepoPath: string; + dispose: () => void; + /** Push `refs/pull//head` to the bare repo at `commitSha`. */ + publishSyntheticPrRef: (prNumber: number, commitSha: string) => Promise; +} + +async function createBareRemote(): Promise { + const bareRepoPath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-test-bare-")), + ); + await simpleGit().init(["--bare", "--initial-branch=main", bareRepoPath]); + return { + bareRepoPath, + dispose: () => rmSync(bareRepoPath, { recursive: true, force: true }), + publishSyntheticPrRef: async (prNumber, commitSha) => { + await simpleGit().raw([ + "-C", + bareRepoPath, + "update-ref", + `refs/pull/${prNumber}/head`, + commitSha, + ]); + }, + }; +} + +/** + * Set up: `local` working repo with `origin` pointing at a bare remote that + * already has a `refs/pull//head` ref but the named branch deleted. + * This is the scenario the recovery code is designed for: PR was merged, + * source branch was auto-deleted, but GitHub still keeps the head commit + * accessible via the synthetic ref. + */ +async function createDeletedBranchScenario(prNumber: number): Promise<{ + local: GitFixture; + bare: BareRemoteFixture; + prHeadOid: string; + worktreePath: string; + worktreeGit: SimpleGit; + dispose: () => void; +}> { + const local = await createGitFixture(); + const bare = await createBareRemote(); + + // Wire local → bare as `origin`. Push `main` so the bare repo has a + // resolvable HEAD. Then create + push a PR commit on a separate + // branch, capture its SHA, and *delete* the branch from the remote + // while keeping the commit reachable through the synthetic PR ref. + await local.git.addRemote("origin", bare.bareRepoPath); + await local.git.push("origin", "main", ["--set-upstream"]); + + await local.git.checkoutBranch("feature/will-be-deleted", "main"); + const prHeadOid = await local.commit("PR head commit", { + "feature.txt": "from the PR", + }); + await local.git.push("origin", "feature/will-be-deleted"); + + // Delete the named branch from the remote (simulates GitHub's + // "delete branch on merge"). The commit is now only reachable via + // the synthetic PR ref. + await local.git.raw(["push", "origin", "--delete", "feature/will-be-deleted"]); + await bare.publishSyntheticPrRef(prNumber, prHeadOid); + + // Switch back to main locally so the feature branch isn't checked out + // anywhere when we add a worktree, and prune so the local fork doesn't + // hold the only reference to the PR commit. + await local.git.checkout("main"); + await local.git.deleteLocalBranch("feature/will-be-deleted", true); + + // Detached worktree where recovery will create the local branch — + // mirrors what `checkout.ts` sets up before the recovery call. + const worktreePath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-test-worktree-")), + ); + rmSync(worktreePath, { recursive: true, force: true }); + await local.git.raw(["worktree", "add", "--detach", worktreePath, "main"]); + + const worktreeGit = simpleGit(worktreePath); + + return { + local, + bare, + prHeadOid, + worktreePath, + worktreeGit, + dispose: () => { + rmSync(worktreePath, { recursive: true, force: true }); + local.dispose(); + bare.dispose(); + }, + }; +} + +describe("recoverPrCheckoutAfterGhFailure (real git)", () => { + let scenario: Awaited>; + + beforeEach(async () => { + scenario = await createDeletedBranchScenario(4242); + }); + + afterEach(() => { + scenario?.dispose(); + }); + + test("synthetic-pr-ref recovery: fetches refs/pull/N/head and creates the local branch at the verified OID", async () => { + const result = await recoverPrCheckoutAfterGhFailure({ + git: scenario.local.git, + worktreePath: scenario.worktreePath, + branch: "feature/will-be-deleted", + prNumber: 4242, + remoteName: "origin", + expectedHeadOid: scenario.prHeadOid, + error: new Error( + "fatal: couldn't find remote ref refs/heads/feature/will-be-deleted", + ), + }); + + expect(result.recovered).toBe(true); + if (!result.recovered) throw new Error("recovery should have succeeded"); + expect(result.warning).toContain("refs/pull/4242/head"); + + // Branch was actually created and points at the PR head commit. + const head = ( + await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) + ).trim(); + expect(head).toBe(scenario.prHeadOid); + + const branch = ( + await scenario.worktreeGit.raw(["symbolic-ref", "--short", "HEAD"]) + ).trim(); + expect(branch).toBe("feature/will-be-deleted"); + + // `--no-track` was honored — recovered branch has no upstream so + // `git push` won't accidentally try to push to the deleted ref. + const upstream = await scenario.worktreeGit + .raw(["rev-parse", "--abbrev-ref", "feature/will-be-deleted@{u}"]) + .catch((err: Error) => err.message); + expect(typeof upstream).toBe("string"); + expect(upstream as string).toMatch(/no upstream|no such ref|^@\{u\}$/i); + }); + + test("OID mismatch aborts recovery without checking out", async () => { + const wrongOid = "0000000000000000000000000000000000000000"; + const mainOid = ( + await scenario.local.git.raw(["rev-parse", "main"]) + ).trim(); + + await expect( + recoverPrCheckoutAfterGhFailure({ + git: scenario.local.git, + worktreePath: scenario.worktreePath, + branch: "feature/will-be-deleted", + prNumber: 4242, + remoteName: "origin", + expectedHeadOid: wrongOid, + error: new Error( + "fatal: couldn't find remote ref refs/heads/feature/will-be-deleted", + ), + }), + ).rejects.toThrow(/did not match GitHub headRefOid/); + + // Worktree HEAD must still point at main's commit — the detached + // state from setup. Recovery aborted before `checkout -B` ran, so + // no `feature/will-be-deleted` branch should exist either. + const head = ( + await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) + ).trim(); + expect(head).toBe(mainOid); + + const branchExists = await scenario.worktreeGit + .raw(["show-ref", "--verify", "refs/heads/feature/will-be-deleted"]) + .then(() => true) + .catch(() => false); + expect(branchExists).toBe(false); + }); + + test("fetch-head recovery: gh attached the ref but failed to set up tracking — checkout FETCH_HEAD as new branch", async () => { + // Simulate the state gh leaves behind when --branch tracking fails: + // FETCH_HEAD is set (from a prior fetch) but no local branch exists. + // We pre-fetch the synthetic ref so FETCH_HEAD has a real SHA. + await scenario.worktreeGit.raw([ + "fetch", + "--no-tags", + "--quiet", + "origin", + "refs/pull/4242/head", + ]); + + const result = await recoverPrCheckoutAfterGhFailure({ + git: scenario.worktreeGit, + worktreePath: scenario.worktreePath, + branch: "user/feature", + prNumber: 4242, + remoteName: "origin", + expectedHeadOid: scenario.prHeadOid, + error: new Error("fatal: 'origin/user/feature' is not a branch"), + }); + + expect(result.recovered).toBe(true); + if (!result.recovered) throw new Error("recovery should have succeeded"); + + const branch = ( + await scenario.worktreeGit.raw(["symbolic-ref", "--short", "HEAD"]) + ).trim(); + expect(branch).toBe("user/feature"); + + const head = ( + await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) + ).trim(); + expect(head).toBe(scenario.prHeadOid); + }); + + test("unrecoverable error returns recovered:false and leaves worktree untouched", async () => { + const mainOid = ( + await scenario.local.git.raw(["rev-parse", "main"]) + ).trim(); + + const result = await recoverPrCheckoutAfterGhFailure({ + git: scenario.local.git, + worktreePath: scenario.worktreePath, + branch: "feature/will-be-deleted", + prNumber: 4242, + remoteName: "origin", + expectedHeadOid: scenario.prHeadOid, + error: new Error("not logged in to GitHub"), + }); + + expect(result.recovered).toBe(false); + + // Worktree still detached at main's commit; no recovery branch ref + // was written. + const head = ( + await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) + ).trim(); + expect(head).toBe(mainOid); + + const branchExists = await scenario.worktreeGit + .raw(["show-ref", "--verify", "refs/heads/feature/will-be-deleted"]) + .then(() => true) + .catch(() => false); + expect(branchExists).toBe(false); + }); +}); From 3dcb8d5bd6d1a33da7430003240b701ef69e832a Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 17:10:11 -0700 Subject: [PATCH 05/10] test(host-service): use simple-git push API correctly for branch deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git.push(remote, branch, customArgs) — extras must come via the array, not positional strings. The earlier git.raw() form worked but obscured intent. --- .../test/integration/pr-checkout-recovery.integration.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts index 371019b1be4..2048984cec4 100644 --- a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts +++ b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts @@ -76,7 +76,7 @@ async function createDeletedBranchScenario(prNumber: number): Promise<{ // Delete the named branch from the remote (simulates GitHub's // "delete branch on merge"). The commit is now only reachable via // the synthetic PR ref. - await local.git.raw(["push", "origin", "--delete", "feature/will-be-deleted"]); + await local.git.push("origin", undefined, ["--delete", "feature/will-be-deleted"]); await bare.publishSyntheticPrRef(prNumber, prHeadOid); // Switch back to main locally so the feature branch isn't checked out From 3ad8f8f551cff18c763354437bc747594f48fe1d Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 17:21:59 -0700 Subject: [PATCH 06/10] fix(host-service): unbreak integration tests run from repo root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two modules were eagerly loading the validated env (`@t3-oss/env-core`) at import time. Both are pulled in by the trpc terminal router, so any test that boots `createApp` crashed during module evaluation when ORGANIZATION_ID etc. were unset — which is the case from the repo root, where the package's bunfig preload doesn't run. - terminal.ts (router): switch `env.ORGANIZATION_ID` → `ctx.organizationId`, which is already plumbed through every protected procedure. - daemon-client-singleton.ts: read `process.env.ORGANIZATION_ID` directly with a runtime check. The singleton is process-wide so ctx isn't available. - terminal.daemon.test.ts: pass `organizationId` in the fake ctx now that the procedure reads it from there instead of env. - pull-requests.test.ts: `syncWorkspaceBranches` writes `pullRequestId` in the same update — expect it in the assertion. --- .../src/terminal/daemon-client-singleton.ts | 19 ++++++++++--- .../router/terminal/terminal.daemon.test.ts | 3 +++ .../src/trpc/router/terminal/terminal.ts | 27 ++++++++++--------- .../host-service/test/pull-requests.test.ts | 1 + 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/packages/host-service/src/terminal/daemon-client-singleton.ts b/packages/host-service/src/terminal/daemon-client-singleton.ts index d169d993619..92d8ee6adfd 100644 --- a/packages/host-service/src/terminal/daemon-client-singleton.ts +++ b/packages/host-service/src/terminal/daemon-client-singleton.ts @@ -10,9 +10,22 @@ // failure model. import { getSupervisor, waitForDaemonReady } from "../daemon/index.ts"; -import { env } from "../env.ts"; import { DaemonClient } from "./DaemonClient/index.ts"; +// Read org id directly from process.env rather than importing the validated +// `env` module — this singleton is eagerly loaded by the trpc terminal +// router, so importing `env` here makes every test that boots the router +// crash at import time when the production env vars aren't set. +function getOrganizationId(): string { + const id = process.env.ORGANIZATION_ID; + if (!id) { + throw new Error( + "ORGANIZATION_ID is not set; pty-daemon cannot be addressed.", + ); + } + return id; +} + let cached: DaemonClient | null = null; let connecting: Promise | null = null; @@ -38,8 +51,8 @@ async function ptyDaemonSocketPath(): Promise { const testOverride = process.env.SUPERSET_PTY_DAEMON_SOCKET; if (testOverride) return testOverride; - await waitForDaemonReady(env.ORGANIZATION_ID); - const sockPath = getSupervisor().getSocketPath(env.ORGANIZATION_ID); + await waitForDaemonReady(getOrganizationId()); + const sockPath = getSupervisor().getSocketPath(getOrganizationId()); if (!sockPath) { throw new Error( "pty-daemon is not available: supervisor returned no socket path. " + diff --git a/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts b/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts index 87bfa337240..1420ee7bc3c 100644 --- a/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts +++ b/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts @@ -24,10 +24,13 @@ process.env.SUPERSET_API_URL = "https://cloud.example.com"; const { appRouter } = await import("../router.ts"); +const TEST_ORG_ID = "00000000-0000-4000-8000-000000000000"; + function makeCaller(authenticated = true) { // Cast to whatever; we only invoke procedures that don't touch db/git/etc. return appRouter.createCaller({ isAuthenticated: authenticated, + organizationId: TEST_ORG_ID, } as unknown as Parameters[0]); } diff --git a/packages/host-service/src/trpc/router/terminal/terminal.ts b/packages/host-service/src/trpc/router/terminal/terminal.ts index dd97a8c840e..216b8b7ae17 100644 --- a/packages/host-service/src/trpc/router/terminal/terminal.ts +++ b/packages/host-service/src/trpc/router/terminal/terminal.ts @@ -3,7 +3,6 @@ import { eq } from "drizzle-orm"; import { z } from "zod"; import { getSupervisor, waitForDaemonReady } from "../../../daemon"; import { terminalSessions, workspaces } from "../../../db/schema"; -import { env } from "../../../env"; import { createTerminalSessionInternal, disposeSession, @@ -13,23 +12,25 @@ import { import { protectedProcedure, router } from "../../index"; // Daemon control surface — sibling to the per-workspace terminal ops above. -// Org-scoped (one daemon per host-service); reads org id from env. +// Org-scoped (one daemon per host-service); org id comes from request ctx +// rather than env so this module can be imported in tests where env vars +// aren't set. // Supervisor lives in this same process so calls go through the in-process // singleton, not over the wire. const daemonRouter = router({ - getUpdateStatus: protectedProcedure.query(() => - getSupervisor().getUpdateStatus(env.ORGANIZATION_ID), + getUpdateStatus: protectedProcedure.query(({ ctx }) => + getSupervisor().getUpdateStatus(ctx.organizationId), ), - listSessions: protectedProcedure.query(async () => { + listSessions: protectedProcedure.query(async ({ ctx }) => { // Wait for the bootstrap so the supervisor has a socket path. - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().listSessions(env.ORGANIZATION_ID); + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().listSessions(ctx.organizationId); }), - restart: protectedProcedure.mutation(async () => { - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().restart(env.ORGANIZATION_ID); + restart: protectedProcedure.mutation(async ({ ctx }) => { + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().restart(ctx.organizationId); }), /** @@ -40,9 +41,9 @@ const daemonRouter = router({ * "Update" path (vs `restart` which kills sessions). On failure, the * UI offers force-restart as a fallback. */ - update: protectedProcedure.mutation(async () => { - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().update(env.ORGANIZATION_ID); + update: protectedProcedure.mutation(async ({ ctx }) => { + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().update(ctx.organizationId); }), }); diff --git a/packages/host-service/test/pull-requests.test.ts b/packages/host-service/test/pull-requests.test.ts index da65d076890..164c9acb714 100644 --- a/packages/host-service/test/pull-requests.test.ts +++ b/packages/host-service/test/pull-requests.test.ts @@ -67,6 +67,7 @@ describe("PullRequestRuntimeManager branch sync", () => { upstreamOwner: null, upstreamRepo: null, upstreamBranch: null, + pullRequestId: null, }); expect(refreshProjectMock).toHaveBeenCalledWith("project-1"); }); From 887ce3945588043b068f58d48fa5852acce11ef4 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 17:45:05 -0700 Subject: [PATCH 07/10] address PR review: tighten patterns, log fully, fix wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot-flagged issues from PR #3725: - pr-checkout-recovery.ts: tighten the fetch-head trigger from a bare 'is not a branch' substring to a regex matching '/' is not a branch. The loose form would match unrelated git errors ('HEAD~5' is not a branch, pathspec failures) and let the fallback consume a stale FETCH_HEAD before the OID check rejects it. - pr-checkout-recovery.ts: 'archived PR ref' → 'PR head ref' in the user warning. refs/pull//head is GitHub's synthetic ref for every PR, not specifically archived; 'archived' suggested the PR itself was closed. - checkout.ts: include the recovery warning text in the server log so ops can see *which* recovery path fired without inferring from PR state. - pull-requests.ts: log a warn when linkWorkspaceToCheckoutPullRequest bails because project repo metadata is unavailable. Was returning null silently; the caller's try/catch only converts thrown errors into user warnings, so the failure was invisible. - new test guards the tightened regex against the false-positive cases. --- .../src/runtime/pull-requests/pull-requests.ts | 8 +++++++- .../workspace-creation/procedures/checkout.ts | 6 +++++- .../utils/pr-checkout-recovery.test.ts | 14 ++++++++++++++ .../utils/pr-checkout-recovery.ts | 9 +++++++-- 4 files changed, 33 insertions(+), 4 deletions(-) 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 daf1fc8375f..728227f845e 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -352,7 +352,13 @@ export class PullRequestRuntimeManager { pullRequest: CheckoutPullRequestMetadata; }): Promise { const repo = await this.getProjectRepository(projectId); - if (!repo) return null; + if (!repo) { + console.warn( + "[host-service:pull-request-runtime] linkWorkspaceToCheckoutPullRequest: skipping; project repo metadata unavailable", + { projectId, workspaceId, prNumber: pullRequest.number }, + ); + return null; + } const existing = this.findPullRequestRow(repo, pullRequest.number); const existingChecks = parseChecksJson(existing?.checksJson ?? null); diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index 0009273e4d0..f63b8fdd281 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -179,7 +179,11 @@ export const checkout = protectedProcedure if (prCheckoutRecoveryWarning) { console.warn( "[workspaceCreation.checkout] recovered failed gh pr checkout", - { prNumber: input.pr.number, branch }, + { + prNumber: input.pr.number, + branch, + warning: prCheckoutRecoveryWarning, + }, ); } diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts index 731067f2383..dc373399d85 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts @@ -51,6 +51,20 @@ describe("getPrCheckoutRecoveryKind", () => { ).toBe("fetch-head"); }); + test("does not match unrelated 'is not a branch' errors", () => { + // Plain ref expressions and pathspec failures both use the same phrase + // but are unrelated to gh's tracking-ref failure. Falling through to + // the fetch-head path here would consume a stale FETCH_HEAD. + expect( + getPrCheckoutRecoveryKind(new Error("fatal: 'HEAD~5' is not a branch")), + ).toBeNull(); + expect( + getPrCheckoutRecoveryKind( + new Error("error: pathspec 'foo' is not a branch"), + ), + ).toBeNull(); + }); + test("does not recover auth or PR lookup failures", () => { expect( getPrCheckoutRecoveryKind(new Error("not logged in to GitHub")), diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts index e245bcc13d8..69c8e7d2a05 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts @@ -15,7 +15,12 @@ export function getPrCheckoutRecoveryKind( ): PrCheckoutRecoveryKind | null { const message = getErrorMessage(error).toLowerCase(); - if (message.includes("is not a branch")) { + // Match the precise gh-tracking-failure phrasing — `'/' is + // not a branch`. Plain `"is not a branch"` matches unrelated git errors + // (`'HEAD~5' is not a branch`, pathspec failures) that could let the + // fetch-head fallback consume a stale FETCH_HEAD from a prior fetch. The + // OID check still rejects mismatches, but only after a confusing path. + if (/'[^']+\/[^']+' is not a branch/.test(message)) { return "fetch-head"; } @@ -149,7 +154,7 @@ export async function recoverPrCheckoutAfterGhFailure({ await checkoutFetchHeadAsBranch({ git, worktreePath, branch }); return { recovered: true, - warning: `The PR head branch was unavailable, so Superset checked out GitHub's archived PR ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, + warning: `The PR head branch was unavailable, so Superset checked out GitHub's PR head ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, }; } From 96ad3550f22d4a908ba4e7ed7f5e9e5b7282e336 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 21:58:20 -0700 Subject: [PATCH 08/10] style: biome formatter on pr-checkout-recovery integration test --- .../pr-checkout-recovery.integration.test.ts | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts index 2048984cec4..ed0e86410b6 100644 --- a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts +++ b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts @@ -76,7 +76,10 @@ async function createDeletedBranchScenario(prNumber: number): Promise<{ // Delete the named branch from the remote (simulates GitHub's // "delete branch on merge"). The commit is now only reachable via // the synthetic PR ref. - await local.git.push("origin", undefined, ["--delete", "feature/will-be-deleted"]); + await local.git.push("origin", undefined, [ + "--delete", + "feature/will-be-deleted", + ]); await bare.publishSyntheticPrRef(prNumber, prHeadOid); // Switch back to main locally so the feature branch isn't checked out @@ -138,9 +141,7 @@ describe("recoverPrCheckoutAfterGhFailure (real git)", () => { expect(result.warning).toContain("refs/pull/4242/head"); // Branch was actually created and points at the PR head commit. - const head = ( - await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) - ).trim(); + const head = (await scenario.worktreeGit.raw(["rev-parse", "HEAD"])).trim(); expect(head).toBe(scenario.prHeadOid); const branch = ( @@ -180,9 +181,7 @@ describe("recoverPrCheckoutAfterGhFailure (real git)", () => { // Worktree HEAD must still point at main's commit — the detached // state from setup. Recovery aborted before `checkout -B` ran, so // no `feature/will-be-deleted` branch should exist either. - const head = ( - await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) - ).trim(); + const head = (await scenario.worktreeGit.raw(["rev-parse", "HEAD"])).trim(); expect(head).toBe(mainOid); const branchExists = await scenario.worktreeGit @@ -222,9 +221,7 @@ describe("recoverPrCheckoutAfterGhFailure (real git)", () => { ).trim(); expect(branch).toBe("user/feature"); - const head = ( - await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) - ).trim(); + const head = (await scenario.worktreeGit.raw(["rev-parse", "HEAD"])).trim(); expect(head).toBe(scenario.prHeadOid); }); @@ -247,9 +244,7 @@ describe("recoverPrCheckoutAfterGhFailure (real git)", () => { // Worktree still detached at main's commit; no recovery branch ref // was written. - const head = ( - await scenario.worktreeGit.raw(["rev-parse", "HEAD"]) - ).trim(); + const head = (await scenario.worktreeGit.raw(["rev-parse", "HEAD"])).trim(); expect(head).toBe(mainOid); const branchExists = await scenario.worktreeGit From d8387449f9d2145a93f6888d0c9651fdd227acd3 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 22:01:54 -0700 Subject: [PATCH 09/10] address PR review nits: clearer recovery control flow + doc fetch-head contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two coderabbit nitpicks worth taking: - checkout.ts: replace `recoveryErr === err` reference-identity check with an explicit `recoveryError` variable. The two failure modes (recovery declined vs. recovery itself threw) were collapsed into one catch block and distinguished only by object identity — a future change that wraps or rethrows the same error reference would silently break the message. - pr-checkout-recovery.ts: document that the `fetch-head` recovery path depends on gh having pre-populated FETCH_HEAD before failing. The `synthetic-pr-ref` path fetches itself; the asymmetry was implicit. --- .../workspace-creation/procedures/checkout.ts | 25 +++++++++++++------ .../utils/pr-checkout-recovery.ts | 9 +++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index f63b8fdd281..bd98af21057 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -141,6 +141,13 @@ export const checkout = protectedProcedure { cwd: worktreePath, timeout: 120_000 }, ); } catch (err) { + // Two failure modes to distinguish: + // 1. recovery declined the error (returned recovered:false) → + // surface the original gh failure as-is. + // 2. recovery attempted but threw → surface both errors. + // Tracking with an explicit error variable instead of catching + // `throw err` and comparing references via `recoveryErr === err`. + let recoveryError: unknown = null; try { const recovery = await recoverPrCheckoutAfterGhFailure({ git, @@ -151,11 +158,14 @@ export const checkout = protectedProcedure expectedHeadOid: input.pr.headRefOid, error: err, }); - if (!recovery.recovered) { - throw err; + if (recovery.recovered) { + prCheckoutRecoveryWarning = recovery.warning; } - prCheckoutRecoveryWarning = recovery.warning; - } catch (recoveryErr) { + } catch (e) { + recoveryError = e; + } + + if (!prCheckoutRecoveryWarning) { await git .raw(["worktree", "remove", "--force", worktreePath]) .catch((rollbackErr) => { @@ -165,10 +175,9 @@ export const checkout = protectedProcedure ); }); clearProgress(input.pendingId); - const recoveryMessage = - recoveryErr === err - ? "" - : ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryErr)}`; + const recoveryMessage = recoveryError + ? ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryError)}` + : ""; throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts index 69c8e7d2a05..2534d08488e 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts @@ -123,6 +123,15 @@ export async function checkoutFetchHeadAsBranch({ * then create a local branch from that ref instead of depending only on a * named head branch. This is especially important after a PR has been merged * and the source branch or fork has been deleted. + * + * The two recovery paths diverge in how `FETCH_HEAD` gets populated: + * - `synthetic-pr-ref`: we run an explicit `git fetch refs/pull/N/head`, + * so `FETCH_HEAD` is freshly written by us before the OID check. + * - `fetch-head`: we rely on `gh pr checkout` having already fetched the + * PR head before it failed at the `--branch` tracking step, leaving a + * valid `FETCH_HEAD` behind. The OID check against `expectedHeadOid` + * is the safety net — a stale or unrelated `FETCH_HEAD` from a prior + * unrelated fetch will mismatch and abort the recovery. */ export async function recoverPrCheckoutAfterGhFailure({ git, From d04b8181aeff97f097a2ffbbbfb21a083d96d1c6 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 2 May 2026 22:08:05 -0700 Subject: [PATCH 10/10] deslop: trim historical refactor justification from catch-block comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment explained the structure against the prior `recoveryErr === err` form. Future readers don't see that prior form — keep only the WHY (the two states need distinguishing). --- .../router/workspace-creation/procedures/checkout.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index bd98af21057..c886b78b920 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -141,12 +141,10 @@ export const checkout = protectedProcedure { cwd: worktreePath, timeout: 120_000 }, ); } catch (err) { - // Two failure modes to distinguish: - // 1. recovery declined the error (returned recovered:false) → - // surface the original gh failure as-is. - // 2. recovery attempted but threw → surface both errors. - // Tracking with an explicit error variable instead of catching - // `throw err` and comparing references via `recoveryErr === err`. + // Distinguish recovery declined (recoveryError null, + // prCheckoutRecoveryWarning null) from recovery threw + // (recoveryError set) so the surfaced message includes the + // secondary failure only when it adds information. let recoveryError: unknown = null; try { const recovery = await recoverPrCheckoutAfterGhFailure({