From 415f4f278b815b91e15ddc164a4e27df6e4af653 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 16 May 2026 14:12:10 -0700 Subject: [PATCH 1/8] fix PR worktree checkout materialization --- apps/docs/content/docs/cli/cli-reference.mdx | 2 +- .../src/commands/workspaces/create/command.ts | 2 +- .../utils/pr-branch-materialize.test.ts | 148 ++++++++ .../utils/pr-branch-materialize.ts | 243 ++++++++++++ .../src/trpc/router/workspaces/workspaces.ts | 75 ++-- .../pr-branch-materialize.integration.test.ts | 174 +++++++++ .../workspace-create-pr.integration.test.ts | 165 ++++++++ .../mcp-v2/src/tools/workspaces/create.ts | 2 +- packages/sdk/src/resources/workspaces.ts | 2 +- .../20260516-pr-worktree-checkout-flow.md | 354 ++++++++++++++++++ skills/superset/SKILL.md | 2 +- 11 files changed, 1115 insertions(+), 54 deletions(-) create mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts create mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts create mode 100644 packages/host-service/test/integration/pr-branch-materialize.integration.test.ts create mode 100644 packages/host-service/test/integration/workspace-create-pr.integration.test.ts create mode 100644 plans/done/20260516-pr-worktree-checkout-flow.md diff --git a/apps/docs/content/docs/cli/cli-reference.mdx b/apps/docs/content/docs/cli/cli-reference.mdx index 13a02fe1ef2..f40e82f914d 100644 --- a/apps/docs/content/docs/cli/cli-reference.mdx +++ b/apps/docs/content/docs/cli/cli-reference.mdx @@ -536,7 +536,7 @@ List workspaces accessible to the active organization. With neither { flag: "--project ", required: true, description: "Project ID." }, { flag: "--name ", required: true, description: "Workspace name." }, { flag: "--branch ", description: "Workspace branch. Required unless `--pr` is set." }, - { flag: "--pr ", description: "Pull request number. Derives the branch via `gh pr checkout`." }, + { flag: "--pr ", description: "Pull request number. Checks out the verified PR head." }, { flag: "--base-branch ", description: "Branch to fork from when `--branch` does not exist. Defaults to the project default branch." }, { flag: "--host ", description: "Target host machineId. Mutually exclusive with `--local`; one is required." }, { flag: "--local", description: "Target this machine. Mutually exclusive with `--host`; one is required." }, diff --git a/packages/cli/src/commands/workspaces/create/command.ts b/packages/cli/src/commands/workspaces/create/command.ts index b1e947ba67b..7dd64c9058d 100644 --- a/packages/cli/src/commands/workspaces/create/command.ts +++ b/packages/cli/src/commands/workspaces/create/command.ts @@ -11,7 +11,7 @@ export default command({ project: string().required().desc("Project ID"), name: string().required().desc("Workspace name"), branch: string().desc("Git branch (required unless --pr is set)"), - pr: number().desc("PR number — derives branch via gh pr checkout"), + pr: number().desc("PR number — checks out the verified PR head"), baseBranch: string().desc( "Branch to fork from when `branch` does not exist (defaults to project default)", ), diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts new file mode 100644 index 00000000000..ecc05def0a0 --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, mock, test } from "bun:test"; +import type { GitClient } from "../shared/types"; +import { materializePrBranch } from "./pr-branch-materialize"; + +const EXPECTED_HEAD_OID = "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a"; + +function createMockGit() { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + const ref = args[2] ?? ""; + if (ref.startsWith("refs/heads/")) { + throw new Error("branch does not exist"); + } + return `${EXPECTED_HEAD_OID}\n`; + } + return ""; + }); + return { + git: { raw } as unknown as GitClient, + raw, + }; +} + +describe("materializePrBranch", () => { + test("same-repo PR fetches and tracks the source branch before creating the local branch", async () => { + const { git, raw } = createMockGit(); + + const result = await materializePrBranch({ + git, + branch: "feature/x", + remoteName: "upstream", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }); + + expect(result).toMatchObject({ + createdBranch: true, + sourceKind: "head-branch", + startPoint: "refs/remotes/upstream/feature/x", + trackingRemote: "upstream", + trackingMergeRef: "refs/heads/feature/x", + }); + expect(raw).toHaveBeenNthCalledWith(1, [ + "fetch", + "--no-tags", + "--quiet", + "upstream", + "+refs/heads/feature/x:refs/remotes/upstream/feature/x", + ]); + expect(raw).toHaveBeenNthCalledWith(4, [ + "branch", + "--no-track", + "--", + "feature/x", + "refs/remotes/upstream/feature/x", + ]); + expect(raw).toHaveBeenNthCalledWith(5, [ + "config", + "branch.feature/x.remote", + "upstream", + ]); + expect(raw).toHaveBeenNthCalledWith(6, [ + "config", + "branch.feature/x.merge", + "refs/heads/feature/x", + ]); + }); + + test("cross-repo PR fetches the synthetic PR ref and configures it as the merge ref", async () => { + const { git, raw } = createMockGit(); + + const result = await materializePrBranch({ + git, + branch: "alice/feature/x", + remoteName: "origin", + pr: { + number: 456, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: true, + }, + }); + + expect(result).toMatchObject({ + createdBranch: true, + sourceKind: "synthetic-pr-ref", + startPoint: "FETCH_HEAD", + trackingRemote: "origin", + trackingMergeRef: "refs/pull/456/head", + }); + expect(raw).toHaveBeenNthCalledWith(1, [ + "fetch", + "--no-tags", + "--quiet", + "origin", + "refs/pull/456/head", + ]); + expect(raw).toHaveBeenNthCalledWith(4, [ + "branch", + "--no-track", + "--", + "alice/feature/x", + "FETCH_HEAD", + ]); + expect(raw).toHaveBeenNthCalledWith(6, [ + "config", + "branch.alice/feature/x.merge", + "refs/pull/456/head", + ]); + }); + + test("aborts before branch creation when the fetched ref does not match GitHub headRefOid", async () => { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + return "1111111111111111111111111111111111111111\n"; + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + await expect( + materializePrBranch({ + git, + branch: "feature/x", + remoteName: "origin", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }), + ).rejects.toThrow("did not match GitHub headRefOid"); + + expect(raw).toHaveBeenCalledTimes(2); + expect(raw).not.toHaveBeenCalledWith([ + "branch", + "--no-track", + "--", + "feature/x", + "refs/remotes/origin/feature/x", + ]); + }); +}); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts new file mode 100644 index 00000000000..455c4e68ea7 --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -0,0 +1,243 @@ +import type { GitClient } from "../shared/types"; + +export type PrBranchSourceKind = "head-branch" | "synthetic-pr-ref"; + +export interface PrBranchMetadata { + number: number; + headRefName: string; + headRefOid: string; + isCrossRepository: boolean; +} + +export interface MaterializePrBranchResult { + branch: string; + createdBranch: boolean; + sourceKind: PrBranchSourceKind; + startPoint: string; + trackingRemote: string; + trackingMergeRef: string; + warning?: string; +} + +interface PrBranchSource { + kind: PrBranchSourceKind; + startPoint: string; + mergeRef: string; + warning?: string; +} + +class SameRepoBranchFetchError extends Error { + constructor( + message: string, + readonly originalError: unknown, + ) { + super(message); + this.name = "SameRepoBranchFetchError"; + } +} + +export function getSyntheticPrHeadRef(prNumber: number): string { + return `refs/pull/${prNumber}/head`; +} + +function normalizeOid(oid: string): string { + return oid.trim().toLowerCase(); +} + +async function revParseCommit(git: GitClient, ref: string): Promise { + const oid = await git.raw(["rev-parse", "--verify", `${ref}^{commit}`]); + const trimmed = oid.trim(); + if (!/^[0-9a-f]{40,}$/i.test(trimmed)) { + throw new Error(`Expected ${ref} to resolve to a commit, got "${trimmed}"`); + } + return trimmed; +} + +async function assertRefMatchesExpectedOid(args: { + git: GitClient; + ref: string; + expectedHeadOid: string; +}): Promise { + const actualOid = await revParseCommit(args.git, args.ref); + if (normalizeOid(actualOid) !== normalizeOid(args.expectedHeadOid)) { + throw new Error( + `Fetched PR head ${actualOid} did not match GitHub headRefOid ${args.expectedHeadOid}`, + ); + } + return actualOid; +} + +async function getLocalBranchHead( + git: GitClient, + branch: string, +): Promise { + try { + return await revParseCommit(git, `refs/heads/${branch}`); + } catch { + return null; + } +} + +async function fetchSameRepoPrBranch(args: { + git: GitClient; + remoteName: string; + pr: PrBranchMetadata; +}): Promise { + const remoteTrackingRef = `refs/remotes/${args.remoteName}/${args.pr.headRefName}`; + try { + await args.git.raw([ + "fetch", + "--no-tags", + "--quiet", + args.remoteName, + `+refs/heads/${args.pr.headRefName}:${remoteTrackingRef}`, + ]); + } catch (err) { + throw new SameRepoBranchFetchError( + `Failed to fetch ${args.pr.headRefName} from ${args.remoteName}`, + err, + ); + } + await assertRefMatchesExpectedOid({ + git: args.git, + ref: remoteTrackingRef, + expectedHeadOid: args.pr.headRefOid, + }); + return { + kind: "head-branch", + startPoint: remoteTrackingRef, + mergeRef: `refs/heads/${args.pr.headRefName}`, + }; +} + +async function fetchSyntheticPrBranch(args: { + git: GitClient; + remoteName: string; + pr: PrBranchMetadata; + warning?: string; +}): Promise { + const syntheticRef = getSyntheticPrHeadRef(args.pr.number); + await args.git.raw([ + "fetch", + "--no-tags", + "--quiet", + args.remoteName, + syntheticRef, + ]); + await assertRefMatchesExpectedOid({ + git: args.git, + ref: "FETCH_HEAD", + expectedHeadOid: args.pr.headRefOid, + }); + return { + kind: "synthetic-pr-ref", + startPoint: "FETCH_HEAD", + mergeRef: syntheticRef, + warning: args.warning, + }; +} + +export async function configurePrBranchTracking(args: { + git: GitClient; + branch: string; + remoteName: string; + mergeRef: string; + pushRemote?: string; +}): Promise { + await args.git.raw([ + "config", + `branch.${args.branch}.remote`, + args.remoteName, + ]); + await args.git.raw(["config", `branch.${args.branch}.merge`, args.mergeRef]); + if (args.pushRemote) { + await args.git.raw([ + "config", + `branch.${args.branch}.pushRemote`, + args.pushRemote, + ]); + } +} + +export async function materializePrBranch(args: { + git: GitClient; + branch: string; + remoteName: string; + pr: PrBranchMetadata; +}): Promise { + let source: PrBranchSource; + + if (args.pr.isCrossRepository) { + source = await fetchSyntheticPrBranch({ + git: args.git, + remoteName: args.remoteName, + pr: args.pr, + }); + } else { + try { + source = await fetchSameRepoPrBranch({ + git: args.git, + remoteName: args.remoteName, + pr: args.pr, + }); + } catch (err) { + if (!(err instanceof SameRepoBranchFetchError)) { + throw err; + } + source = await fetchSyntheticPrBranch({ + git: args.git, + remoteName: args.remoteName, + pr: args.pr, + warning: `The PR head branch "${args.pr.headRefName}" was unavailable from ${args.remoteName}, so Superset fetched ${getSyntheticPrHeadRef(args.pr.number)} instead. Original error: ${err.originalError instanceof Error ? err.originalError.message : String(err.originalError)}`, + }); + } + } + + const existingOid = await getLocalBranchHead(args.git, args.branch); + if (existingOid !== null) { + if (normalizeOid(existingOid) !== normalizeOid(args.pr.headRefOid)) { + throw new Error( + `Local branch "${args.branch}" exists and points at ${existingOid}, not PR head ${args.pr.headRefOid}`, + ); + } + await configurePrBranchTracking({ + git: args.git, + branch: args.branch, + remoteName: args.remoteName, + mergeRef: source.mergeRef, + }); + return { + branch: args.branch, + createdBranch: false, + sourceKind: source.kind, + startPoint: source.startPoint, + trackingRemote: args.remoteName, + trackingMergeRef: source.mergeRef, + warning: source.warning, + }; + } + + await args.git.raw([ + "branch", + "--no-track", + "--", + args.branch, + source.startPoint, + ]); + await configurePrBranchTracking({ + git: args.git, + branch: args.branch, + remoteName: args.remoteName, + mergeRef: source.mergeRef, + }); + + return { + branch: args.branch, + createdBranch: true, + sourceKind: source.kind, + startPoint: source.startPoint, + trackingRemote: args.remoteName, + trackingMergeRef: source.mergeRef, + warning: source.warning, + }; +} diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index 323a2832dba..b24299fb73b 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -32,13 +32,10 @@ import { type GeneratedWorkspaceNames, generateWorkspaceNamesFromPrompt, } from "../workspace-creation/utils/ai-workspace-names"; -import { execGh } from "../workspace-creation/utils/exec-gh"; +import type { ExecGh } from "../workspace-creation/utils/exec-gh"; import { listBranchNames } from "../workspace-creation/utils/list-branch-names"; +import { materializePrBranch } from "../workspace-creation/utils/pr-branch-materialize"; import { derivePrLocalBranchName } from "../workspace-creation/utils/pr-branch-name"; -import { - getErrorMessage, - recoverPrCheckoutAfterGhFailure, -} from "../workspace-creation/utils/pr-checkout-recovery"; import { resolveStartPoint } from "../workspace-creation/utils/resolve-start-point"; import { deduplicateBranchName } from "../workspace-creation/utils/sanitize-branch"; @@ -128,8 +125,9 @@ interface PrMetadata { async function fetchPrMetadata(args: { cwd: string; prNumber: number; + execGh: ExecGh; }): Promise { - const result = await execGh( + const result = await args.execGh( [ "pr", "view", @@ -545,6 +543,7 @@ export const workspacesRouter = router({ prMetadata = await fetchPrMetadata({ cwd: localProject.repoPath, prNumber: input.pr, + execGh: ctx.execGh, }); resolvedBranch = derivePrLocalBranchName(prMetadata); @@ -637,58 +636,36 @@ export const workspacesRouter = router({ }); } } else { + let attemptedWorktreeAdd = false; try { - await git.raw(["worktree", "add", "--detach", worktreePath]); + const materialized = await materializePrBranch({ + git, + branch: resolvedBranch, + remoteName: localProject.remoteName ?? "origin", + pr: prMetadata, + }); + if (materialized.warning) { + console.warn(`[workspaces.create] ${materialized.warning}`); + } + attemptedWorktreeAdd = true; + await git.raw([ + "worktree", + "add", + worktreePath, + resolvedBranch, + ]); } catch (err) { + if (attemptedWorktreeAdd) { + await rollbackWorktree(); + } throw new TRPCError({ code: "CONFLICT", message: err instanceof Error ? err.message - : "Failed to add detached worktree", + : "Failed to prepare PR worktree", }); } - - try { - await execGh( - [ - "pr", - "checkout", - String(input.pr), - "--branch", - resolvedBranch, - "--force", - ], - { cwd: worktreePath, timeout: 120_000 }, - ); - } catch (err) { - let recoveryError: unknown = null; - let recovered = false; - try { - const recovery = await recoverPrCheckoutAfterGhFailure({ - git, - worktreePath, - branch: resolvedBranch, - prNumber: input.pr, - remoteName: localProject.remoteName ?? "origin", - expectedHeadOid: prMetadata.headRefOid, - error: err, - }); - recovered = recovery.recovered; - } catch (e) { - recoveryError = e; - } - if (!recovered) { - await rollbackWorktree(); - const recoveryMessage = recoveryError - ? ` Recovery via refs/pull/${input.pr}/head also failed: ${getErrorMessage(recoveryError)}` - : ""; - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `gh pr checkout failed: ${err instanceof Error ? err.message : String(err)}${recoveryMessage}`, - }); - } - } } await enablePushAutoSetupRemote( diff --git a/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts new file mode 100644 index 00000000000..7dc38b19691 --- /dev/null +++ b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts @@ -0,0 +1,174 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { + chmodSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import simpleGit, { type SimpleGit } from "simple-git"; +import { materializePrBranch } from "../../src/trpc/router/workspace-creation/utils/pr-branch-materialize"; +import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; + +interface BareRemoteFixture { + bareRepoPath: string; + dispose: () => void; +} + +async function createBareRemote(): Promise { + const bareRepoPath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-pr-branch-bare-")), + ); + await simpleGit().init(["--bare", "--initial-branch=main", bareRepoPath]); + return { + bareRepoPath, + dispose: () => rmSync(bareRepoPath, { recursive: true, force: true }), + }; +} + +async function createPrScenario(prNumber: number): Promise<{ + local: GitFixture; + bare: BareRemoteFixture; + prHeadOid: string; + dispose: () => void; +}> { + const local = await createGitFixture(); + const bare = await createBareRemote(); + + await local.commit("main lockfile", { + "package-lock.json": "main lockfile\n", + }); + await local.git.addRemote("origin", bare.bareRepoPath); + await local.git.push("origin", "main", ["--set-upstream"]); + + await local.git.checkoutBranch("feature/pr-lockfile", "main"); + const prHeadOid = await local.commit("PR lockfile", { + "package-lock.json": "pr lockfile\n", + "feature.txt": "from the PR\n", + }); + await local.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await local.git.checkout("main"); + await local.git.deleteLocalBranch("feature/pr-lockfile", true); + + return { + local, + bare, + prHeadOid, + dispose: () => { + local.dispose(); + bare.dispose(); + }, + }; +} + +function installDirtyPostCheckoutHook(repoPath: string): void { + const hookPath = join(repoPath, ".git", "hooks", "post-checkout"); + writeFileSync( + hookPath, + [ + "#!/bin/sh", + "printf 'dirty lockfile from post-checkout hook\\n' > package-lock.json", + "", + ].join("\n"), + ); + chmodSync(hookPath, 0o755); +} + +describe("materializePrBranch (real git)", () => { + let scenario: Awaited>; + + beforeEach(async () => { + scenario = await createPrScenario(5252); + }); + + afterEach(() => { + scenario?.dispose(); + }); + + test("materialize-first worktree creation survives hooks that dirty tracked files during checkout", async () => { + installDirtyPostCheckoutHook(scenario.local.repoPath); + + const materialized = await materializePrBranch({ + git: scenario.local.git, + branch: "contributor/feature-pr-lockfile", + remoteName: "origin", + pr: { + number: 5252, + headRefName: "feature/pr-lockfile", + headRefOid: scenario.prHeadOid, + isCrossRepository: true, + }, + }); + expect(materialized.sourceKind).toBe("synthetic-pr-ref"); + + const oldFlowPath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-old-pr-worktree-")), + ); + rmSync(oldFlowPath, { recursive: true, force: true }); + try { + await scenario.local.git.raw([ + "worktree", + "add", + "--detach", + oldFlowPath, + "main", + ]); + const oldCheckoutError = await simpleGit(oldFlowPath) + .raw(["checkout", "contributor/feature-pr-lockfile"]) + .then(() => null) + .catch((err: Error) => err); + expect(oldCheckoutError).toBeInstanceOf(Error); + expect(oldCheckoutError?.message).toMatch( + /would be overwritten by checkout/, + ); + } finally { + await scenario.local.git + .raw(["worktree", "remove", "--force", oldFlowPath]) + .catch(() => {}); + rmSync(oldFlowPath, { recursive: true, force: true }); + } + + const worktreePath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-new-pr-worktree-")), + ); + rmSync(worktreePath, { recursive: true, force: true }); + try { + await scenario.local.git.raw([ + "worktree", + "add", + worktreePath, + "contributor/feature-pr-lockfile", + ]); + + const worktreeGit: SimpleGit = simpleGit(worktreePath); + const head = (await worktreeGit.raw(["rev-parse", "HEAD"])).trim(); + expect(head).toBe(scenario.prHeadOid); + + const branch = ( + await worktreeGit.raw(["symbolic-ref", "--short", "HEAD"]) + ).trim(); + expect(branch).toBe("contributor/feature-pr-lockfile"); + + const lockStatus = ( + await worktreeGit.raw([ + "status", + "--porcelain", + "--", + "package-lock.json", + ]) + ).trim(); + expect(lockStatus).toContain("package-lock.json"); + } finally { + await scenario.local.git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch(() => {}); + rmSync(worktreePath, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts new file mode 100644 index 00000000000..0f481ff753d --- /dev/null +++ b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts @@ -0,0 +1,165 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { + chmodSync, + existsSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { eq } from "drizzle-orm"; +import simpleGit from "simple-git"; +import { workspaces } from "../../src/db/schema"; +import { cloudFlows } from "../helpers/cloud-fakes"; +import { createProjectScenario } from "../helpers/scenarios"; + +interface BareRemoteFixture { + bareRepoPath: string; + dispose: () => void; +} + +async function createBareRemote(): Promise { + const bareRepoPath = realpathSync( + mkdtempSync(join(tmpdir(), "host-service-workspace-pr-bare-")), + ); + await simpleGit().init(["--bare", "--initial-branch=main", bareRepoPath]); + return { + bareRepoPath, + dispose: () => rmSync(bareRepoPath, { recursive: true, force: true }), + }; +} + +function installDirtyPostCheckoutHook(repoPath: string): void { + const hookPath = join(repoPath, ".git", "hooks", "post-checkout"); + writeFileSync( + hookPath, + [ + "#!/bin/sh", + "printf 'dirty lockfile from post-checkout hook\\n' > package-lock.json", + "", + ].join("\n"), + ); + chmodSync(hookPath, 0o755); +} + +describe("workspaces.create PR checkout integration", () => { + let dispose: (() => Promise) | undefined; + + afterEach(async () => { + if (dispose) { + await dispose(); + dispose = undefined; + } + }); + + test("creates a PR worktree from the verified PR head without running gh pr checkout", async () => { + const prNumber = 6060; + const ghCalls: Array<{ args: string[]; cwd?: string }> = []; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args, options?: unknown) => { + ghCalls.push({ + args, + cwd: (options as { cwd?: string } | undefined)?.cwd, + }); + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "PR with lockfile hook", + headRefName: "feature/pr-lockfile", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + isCrossRepository: true, + state: "OPEN", + }; + } + if (args[0] === "pr" && args[1] === "checkout") { + throw new Error("workspaces.create must not run gh pr checkout"); + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + let worktreePath: string | undefined; + dispose = async () => { + if (worktreePath) { + await scenario.repo.git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch(() => {}); + rmSync(worktreePath, { recursive: true, force: true }); + } + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main lockfile", { + "package-lock.json": "main lockfile\n", + }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/pr-lockfile", "main"); + prHeadOid = await scenario.repo.commit("PR lockfile", { + "package-lock.json": "pr lockfile\n", + "feature.txt": "from the PR\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/pr-lockfile", true); + installDirtyPostCheckoutHook(scenario.repo.repoPath); + + const result = await scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "PR workspace", + pr: prNumber, + }); + + const expectedBranch = "contributor/feature/pr-lockfile"; + expect(result.workspace.branch).toBe(expectedBranch); + expect( + ghCalls.some((call) => call.args[0] === "pr" && call.args[1] === "view"), + ).toBe(true); + expect( + ghCalls.some( + (call) => call.args[0] === "pr" && call.args[1] === "checkout", + ), + ).toBe(false); + + const persisted = scenario.host.db + .select() + .from(workspaces) + .where(eq(workspaces.branch, expectedBranch)) + .get(); + worktreePath = persisted?.worktreePath; + expect(worktreePath).toBeTruthy(); + if (!worktreePath) { + throw new Error("expected PR workspace path to be persisted"); + } + expect(existsSync(worktreePath)).toBe(true); + + const worktreeGit = simpleGit(worktreePath); + const head = (await worktreeGit.raw(["rev-parse", "HEAD"])).trim(); + expect(head).toBe(prHeadOid); + + const lockStatus = ( + await worktreeGit.raw([ + "status", + "--porcelain", + "--", + "package-lock.json", + ]) + ).trim(); + expect(lockStatus).toContain("package-lock.json"); + }); +}); diff --git a/packages/mcp-v2/src/tools/workspaces/create.ts b/packages/mcp-v2/src/tools/workspaces/create.ts index 2b10c3c323b..ec5d1036c29 100644 --- a/packages/mcp-v2/src/tools/workspaces/create.ts +++ b/packages/mcp-v2/src/tools/workspaces/create.ts @@ -40,7 +40,7 @@ export function register(server: McpServer): void { .positive() .optional() .describe( - "Pull request number — server runs `gh pr checkout` and derives the branch.", + "Pull request number — server checks out the verified PR head and derives the branch.", ), baseBranch: z .string() diff --git a/packages/sdk/src/resources/workspaces.ts b/packages/sdk/src/resources/workspaces.ts index 4d3f0fa248e..17f28aa3c80 100644 --- a/packages/sdk/src/resources/workspaces.ts +++ b/packages/sdk/src/resources/workspaces.ts @@ -161,7 +161,7 @@ export interface WorkspaceCreateParams { name: string; /** Git branch the workspace tracks. Required unless `pr` is set. */ branch?: string; - /** Pull request number — server runs `gh pr checkout` and derives the branch. */ + /** Pull request number — server checks out the verified PR head and derives the branch. */ pr?: number; /** Branch to fork from when `branch` does not exist. Ignored with `pr`. */ baseBranch?: string; diff --git a/plans/done/20260516-pr-worktree-checkout-flow.md b/plans/done/20260516-pr-worktree-checkout-flow.md new file mode 100644 index 00000000000..6e79c7c816f --- /dev/null +++ b/plans/done/20260516-pr-worktree-checkout-flow.md @@ -0,0 +1,354 @@ +# PR Worktree Checkout Flow + +## Purpose + +Document Superset's current PR workspace checkout setup, explain how the +`package-lock.json would be overwritten by checkout` failure can happen, and +define the more correct v2 flow. + +The short version: for worktree mode, we should stop running `gh pr checkout` +inside a placeholder detached worktree. We should resolve and materialize the PR +branch first, verify the expected commit, configure branch metadata, then create +the worktree once with `git worktree add `. + +## Current Superset Setup + +### v2 host service + +The canonical v2 path is +`packages/host-service/src/trpc/router/workspaces/workspaces.ts`. + +- `fetchPrMetadata` shells out to `gh pr view --json number,url,title,headRefName,headRefOid,baseRefName,headRepositoryOwner,isCrossRepository,state` (`workspaces.ts:128`). +- `derivePrLocalBranchName` maps same-repo PRs to `headRefName` and cross-repo PRs to `/`, falling back to `pr/` if GitHub no longer reports a fork owner (`packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts:12`). +- `workspaces.create` prunes stale worktree registrations, checks for an existing Superset workspace, checks whether a local branch already points at `headRefOid`, and adopts a matching existing worktree when possible (`workspaces.ts:544`). +- If the local branch already exists and matches the PR head, v2 creates a worktree directly from that branch with `git worktree add ` (`workspaces.ts:622`). +- If the local branch does not exist, v2 creates a detached placeholder worktree with `git worktree add --detach `, then runs `gh pr checkout --branch --force` with `cwd` set to that placeholder (`workspaces.ts:640` and `workspaces.ts:653`). +- After checkout, v2 enables `push.autoSetupRemote` and registers local/cloud workspace records. + +Recovery after `gh pr checkout` lives in +`packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts`. +It only handles selected `gh` failures: + +- `'/' is not a branch`, where `gh` likely fetched a valid `FETCH_HEAD` but failed while attaching tracking (`pr-checkout-recovery.ts:18`). +- Missing/unreadable remote/ref errors, where Superset explicitly fetches `refs/pull//head` (`pr-checkout-recovery.ts:27`). +- Both paths verify `FETCH_HEAD` against `headRefOid` before `git checkout -B --no-track FETCH_HEAD` (`pr-checkout-recovery.ts:60` and `pr-checkout-recovery.ts:99`). + +It does not recover from a dirty placeholder worktree. + +### v1 desktop + +The legacy desktop path has the same high-level shape in +`apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`. + +- `createWorktreeFromPr` is documented as creating a worktree from a PR by using + `gh pr checkout` inside the new worktree (`git.ts:1736`). +- If the local branch exists, it runs `git worktree add ` (`git.ts:1759`). +- Otherwise it runs `git worktree add --detach ` and then + `gh pr checkout --branch --force` in that worktree (`git.ts:1772` and `git.ts:1780`). +- It has a narrower fallback for the `"is not a branch"` tracking error, also using `FETCH_HEAD` (`git.ts:1793`). + +v2 is the path to fix first, but v1 has the same underlying vulnerability. + +### Public surface and docs + +Several user-facing or SDK-facing descriptions still say that PR workspace +creation is implemented by `gh pr checkout`: + +- `packages/cli/src/commands/workspaces/create/command.ts:14` +- `packages/mcp-v2/src/tools/workspaces/create.ts:43` +- `packages/sdk/src/resources/workspaces.ts:164` +- `apps/docs/content/docs/cli/cli-reference.mdx:539` +- `skills/superset/SKILL.md:39` + +The implementation plan in +`apps/desktop/plans/20260416-v2-pr-checkout-endpoint.md` explicitly chose the +detached-worktree-plus-`gh pr checkout` approach because it matched v1 and let +`gh` set fork/upstream metadata (`20260416-v2-pr-checkout-endpoint.md:146` and +`:349`). That was a reasonable shortcut, but it is the source of this failure +class. + +## How The Failure Happens + +The failing command looked like: + +```text +gh pr checkout 1038 --branch MiroBenicio/agents-md-add-typecheck --force +error: Your local changes to the following files would be overwritten by checkout: +package-lock.json +``` + +This should not be caused by unrelated dirtiness in the user's main checkout if +`cwd` is truly the new worktree. Git worktrees have independent working trees. + +The likely chain is: + +1. Superset runs `git worktree add --detach `. +2. That command performs a real checkout into ``. +3. During or immediately after that checkout, local hooks, filters, generated + files, package-manager side effects, or watchers modify a tracked file in the + new placeholder worktree. In the observed case the file was `package-lock.json`. +4. Superset then runs `gh pr checkout ... --force` in that already-dirty + placeholder worktree. +5. `gh pr checkout` still uses normal `git checkout`/`git fetch`/`git reset` + commands internally. Its `--force` is not a general "discard any dirty + placeholder worktree before switching" operation, so Git refuses to overwrite + the dirty tracked file and the PR checkout aborts. + +I reproduced the same Git error shape locally with a `post-checkout` hook that +rewrites `package-lock.json` during the detached worktree creation step. + +If this error happened without the new worktree becoming dirty, the other +possibility is that `gh pr checkout` was run with the wrong `cwd`. The current +v2 code passes `{ cwd: worktreePath }`, so the dirty-placeholder explanation is +the one that matches the code path. + +## External Comparisons + +### Worktrunk + +Worktrunk is the strongest comparison because it has first-class `pr:` +and `mr:` worktree switching. + +Local clone: `~/workplace/worktrunk` at +`d25e87c3 chore: update tend workflows (0.0.19 -> 0.0.21) (#2772)`. + +Relevant behavior: + +- Help text says same-repo PRs/MRs switch to the branch directly; fork PRs/MRs + fetch the platform ref (`refs/pull/N/head` or `refs/merge-requests/N/head`) + and configure `pushRemote` to the fork URL (`src/cli/mod.rs:622`). +- The remote-ref module documents the shared workflow: fetch metadata, check + whether a local branch already tracks the ref, then create/configure the + branch if needed (`src/git/remote_ref/mod.rs:1`). +- Same-repo PRs are fetched with an explicit refspec so a remote-tracking ref + exists even in limited-fetch clones: + `+refs/heads/:refs/remotes//` + (`src/commands/worktree/switch.rs:419`). +- Fork PRs fetch the PR/MR ref, create the local branch from `FETCH_HEAD`, + configure `branch..remote`, `branch..merge`, and optionally + `branch..pushRemote`, then run `git worktree add -- ` + (`src/commands/worktree/switch.rs:695` and `:743`). + +The important pattern is that Worktrunk does not create a detached placeholder +worktree and then run `gh pr checkout` inside it. It materializes and configures +the branch first, then creates the worktree from that branch. + +### T3Code + +Local clone: `~/workplace/t3code`. + +T3Code uses `gh pr checkout --force` only for an intentional local checkout +mode (`apps/server/src/git/Layers/GitManager.ts:1428`). In worktree mode it: + +- Resolves PR metadata and derives a local PR branch (`GitManager.ts:1464`). +- Materializes the PR branch before creating the worktree + (`GitManager.ts:1517`). +- Creates the worktree with `git worktree add ` + (`GitCore.ts:1896`). +- Falls back to fetching `+refs/pull//head:refs/heads/` when richer + head-repo fetching is unavailable (`GitCore.ts:1919`). + +This is effectively the same architecture Superset should use. + +### GitHub CLI + +Local clone: `~/workplace/cli`. + +`gh pr checkout` is current-worktree oriented: + +- For existing remotes it fetches `refs/heads/` into a remote-tracking ref, + then checks out, merges/resets, or creates a tracking local branch + (`pkg/cmd/pr/checkout/checkout.go:170`). +- For missing remotes/forks it fetches `refs/pull//head`, checks out the + local branch, then configures `branch..remote`, `pushRemote`, and + `merge` (`checkout.go:204`). + +There is an open upstream feature request for a native worktree mode: +https://github.com/cli/cli/issues/5793. That reinforces that `gh pr checkout` +does not currently expose the worktree primitive Superset needs. + +### Other tools + +- OpenClaw's review helper creates a temporary worktree from `origin/main`, then + fetches `pull//head:pr-` and checks out that ref detached + (`~/workplace/openclaw/scripts/pr:80` and `:270`). That is useful for review + mode but still has a two-checkout placeholder risk. +- Workz and Worktree Manager are not PR-specific, but their generic pattern is + the same safer worktree pattern: choose or create a branch/ref first, then run + one `git worktree add` from that start point (`~/workplace/workz/src/git.rs:76`, + `~/workplace/worktree-manager/src/worktree.ts:62`, and `:145`). + +## Recommended v2 Flow + +Replace the "detached placeholder, then `gh pr checkout`" path with a +materialize-first PR worktree flow. + +### 1. Keep `gh pr view` for metadata + +Continue using `gh pr view` because it gives authenticated GitHub metadata and +fits the existing dependency model. Extend the metadata only as needed: + +- Keep `number`, `url`, `title`, `headRefName`, `headRefOid`, `baseRefName`, + `headRepositoryOwner`, `isCrossRepository`, and `state`. +- Add `headRepository` and `maintainerCanModify` if we want parity with + GitHub CLI's fork push configuration. + +### 2. Derive and validate the local branch + +Reuse `derivePrLocalBranchName`. + +Before creating a worktree: + +- If a Superset workspace already exists for that branch, return it. +- If the local branch exists and its head equals `headRefOid`, adopt it or add a + worktree from it. +- If the local branch exists and its head differs from `headRefOid`, keep the + current conflict behavior. +- If the branch is already checked out in another worktree, adopt that worktree + when it matches the PR head. + +This part of v2 is already mostly right. + +### 3. Materialize the PR branch before the worktree + +For same-repo PRs: + +1. Resolve the base remote name (`localProject.remoteName ?? "origin"` today). +2. Fetch the source branch explicitly: + + ```text + git fetch --no-tags --quiet +refs/heads/:refs/remotes// + ``` + +3. Verify `refs/remotes//` equals `headRefOid`. +4. Create or update the local branch to that verified commit if it does not + already exist. +5. Configure upstream to `/`. + +For cross-repo PRs or deleted head branches: + +1. Fetch GitHub's synthetic PR head ref from the base remote: + + ```text + git fetch --no-tags --quiet refs/pull//head + ``` + +2. Verify `FETCH_HEAD^{commit}` equals `headRefOid`. +3. Create the local branch from `FETCH_HEAD`. +4. Configure: + - `branch..remote = ` + - `branch..merge = refs/pull//head` for synthetic-ref fallback, + or `refs/heads/` when a real fork remote is available. + - `branch..pushRemote = ` when we have fork clone URL + metadata and pushing to the fork should work. + +This preserves the safety check v2 already has in recovery: every fetched PR +head must match GitHub's `headRefOid` before we create or update a branch. + +### 4. Create the worktree once + +After the branch exists and points at the verified PR head: + +```text +git worktree add +``` + +or, when creating a brand-new branch atomically from a verified ref: + +```text +git worktree add -b +``` + +There should be no intermediate detached checkout in the target path. + +After creation, optionally run `git status --porcelain` in the new worktree. If +hooks or filters still dirty files during the single worktree checkout, surface +that as a warning on the workspace instead of treating it as a PR checkout +failure. The branch is already correct at that point. + +### 5. Keep rollback targeted + +If branch materialization fails, no worktree exists yet. + +If `git worktree add` fails after a new branch was created only for this +operation, delete that branch if and only if it still points at the verified PR +head and is not checked out anywhere. Do not delete or reset pre-existing local +branches. + +## Implementation Shape + +Add small, testable helpers under +`packages/host-service/src/trpc/router/workspace-creation/utils/`: + +- `resolvePrCheckoutPlan` - classify same-repo vs cross-repo, choose remote/ref, + and describe required branch config. +- `fetchAndVerifyPrHead` - fetch same-repo branch or synthetic PR ref and verify + the resulting OID against `headRefOid`. +- `materializePrBranch` - create/configure the local branch without checking it + out in any worktree. +- `configurePrBranchTracking` - set `branch..remote`, `.merge`, and + `.pushRemote` consistently. + +Then update only the new-branch PR path in +`packages/host-service/src/trpc/router/workspaces/workspaces.ts`: + +1. Replace `git worktree add --detach` plus `gh pr checkout` with + `materializePrBranch(...)`. +2. Run `git worktree add `. +3. Keep existing workspace registration, base-branch config, `push.autoSetupRemote`, + and rollback behavior. +4. Remove or narrow `recoverPrCheckoutAfterGhFailure` usage. The useful + synthetic-ref fetch and OID verification logic should move into the normal + materialization path. + +Once v2 is changed, update CLI/MCP/SDK/docs descriptions so they say Superset +checks out PRs by resolving the PR head and creating a worktree from the +verified branch, not by running `gh pr checkout`. + +## Tests To Add + +- Same-repo PR: fetches source branch with explicit refspec, verifies + `headRefOid`, creates branch/worktree. +- Fork PR: fetches `refs/pull//head`, verifies `headRefOid`, configures + branch metadata, creates branch/worktree. +- Deleted fork/head branch: synthetic PR ref still works when GitHub exposes the + PR head ref. +- Existing local branch with matching OID: no fetch rewrite needed; add/adopt + worktree. +- Existing local branch with mismatched OID: conflict with current cleanup hint. +- Branch already checked out in another worktree: adopt matching branch, reject + mismatched branch. +- Dirty-placeholder regression: install a `post-checkout` hook that modifies a + tracked file like `package-lock.json`; the new flow should not run a second + checkout in that dirty placeholder and should create the PR workspace + successfully. +- OID mismatch after fetch: abort before branch/worktree creation. + +## Non-goals + +- Do not stash user changes to make PR checkout work. +- Do not mutate the primary checkout. +- Do not call `gh pr checkout` in worktree mode. +- Do not force-reset a pre-existing user branch that points at a different + commit. +- Do not solve every fork push edge case in the first pass unless we add the + necessary `headRepository` metadata. + +## Implementation Status + +Implemented for the canonical v2 host-service path: + +- `workspaces.create` now fetches and verifies the PR head, materializes the + local branch, then creates the worktree from that branch. +- The PR path no longer shells out to `gh pr checkout` in worktree mode. +- `workspaces.create` uses `ctx.execGh` for PR metadata so integration tests can + mock GitHub CLI behavior. +- Regression coverage now includes command-level helper tests, real-git + materialization tests, and a `workspaces.create` integration test that proves + the dirty-placeholder failure class no longer blocks PR workspace creation. + +Remaining follow-ups: + +- Decide whether to backport the same shape to legacy v1 desktop code. +- Add richer fork push metadata (`headRepository`, `maintainerCanModify`, + `pushRemote`) if we want GitHub CLI parity for fork push UX. diff --git a/skills/superset/SKILL.md b/skills/superset/SKILL.md index 3bcf0e31c39..d04449a714a 100644 --- a/skills/superset/SKILL.md +++ b/skills/superset/SKILL.md @@ -36,7 +36,7 @@ superset workspaces update --name "..." superset workspaces delete [...] ``` -Provide exactly one of `--branch` or `--pr`. With `--pr`, the host runs `gh pr checkout`. `--base-branch ` is the fork point when `--branch` doesn't exist yet. +Provide exactly one of `--branch` or `--pr`. With `--pr`, the host checks out the verified PR head and derives the branch. `--base-branch ` is the fork point when `--branch` doesn't exist yet. ## Agents From 57c5247b028afc2043cfbf88d219a108ea04097e Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 16 May 2026 14:50:26 -0700 Subject: [PATCH 2/8] address PR checkout review issues --- .../utils/pr-branch-materialize.test.ts | 77 ++++++++++++++++--- .../utils/pr-branch-materialize.ts | 60 +++++++++++---- .../src/trpc/router/workspaces/workspaces.ts | 21 ++++- .../pr-branch-materialize.integration.test.ts | 61 ++++++++++++++- 4 files changed, 190 insertions(+), 29 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index ecc05def0a0..8e765f173fe 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -1,6 +1,10 @@ import { describe, expect, mock, test } from "bun:test"; import type { GitClient } from "../shared/types"; -import { materializePrBranch } from "./pr-branch-materialize"; +import { + deleteMaterializedPrBranchIfSafe, + getSyntheticPrVerifiedRef, + materializePrBranch, +} from "./pr-branch-materialize"; const EXPECTED_HEAD_OID = "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a"; @@ -72,23 +76,25 @@ describe("materializePrBranch", () => { test("cross-repo PR fetches the synthetic PR ref and configures it as the merge ref", async () => { const { git, raw } = createMockGit(); + const pr = { + number: 456, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: true, + }; + const verifiedRef = getSyntheticPrVerifiedRef(pr); const result = await materializePrBranch({ git, branch: "alice/feature/x", remoteName: "origin", - pr: { - number: 456, - headRefName: "feature/x", - headRefOid: EXPECTED_HEAD_OID, - isCrossRepository: true, - }, + pr, }); expect(result).toMatchObject({ createdBranch: true, sourceKind: "synthetic-pr-ref", - startPoint: "FETCH_HEAD", + startPoint: verifiedRef, trackingRemote: "origin", trackingMergeRef: "refs/pull/456/head", }); @@ -97,14 +103,14 @@ describe("materializePrBranch", () => { "--no-tags", "--quiet", "origin", - "refs/pull/456/head", + `+refs/pull/456/head:${verifiedRef}`, ]); expect(raw).toHaveBeenNthCalledWith(4, [ "branch", "--no-track", "--", "alice/feature/x", - "FETCH_HEAD", + verifiedRef, ]); expect(raw).toHaveBeenNthCalledWith(6, [ "config", @@ -113,6 +119,57 @@ describe("materializePrBranch", () => { ]); }); + test("deletes a materialized branch only when it still points at the verified PR head", async () => { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + return `${EXPECTED_HEAD_OID}\n`; + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + await expect( + deleteMaterializedPrBranchIfSafe({ + git, + branch: "alice/feature/x", + expectedHeadOid: EXPECTED_HEAD_OID, + }), + ).resolves.toBe(true); + + expect(raw).toHaveBeenNthCalledWith(2, [ + "branch", + "-D", + "--", + "alice/feature/x", + ]); + }); + + test("does not delete a branch that moved away from the verified PR head", async () => { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + return "1111111111111111111111111111111111111111\n"; + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + await expect( + deleteMaterializedPrBranchIfSafe({ + git, + branch: "alice/feature/x", + expectedHeadOid: EXPECTED_HEAD_OID, + }), + ).resolves.toBe(false); + + expect(raw).toHaveBeenCalledTimes(1); + expect(raw).not.toHaveBeenCalledWith([ + "branch", + "-D", + "--", + "alice/feature/x", + ]); + }); + test("aborts before branch creation when the fetched ref does not match GitHub headRefOid", async () => { const raw = mock(async (args: string[]) => { if (args[0] === "rev-parse") { diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts index 455c4e68ea7..3904f9bbf48 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -40,6 +40,10 @@ export function getSyntheticPrHeadRef(prNumber: number): string { return `refs/pull/${prNumber}/head`; } +export function getSyntheticPrVerifiedRef(pr: PrBranchMetadata): string { + return `refs/superset/pr-fetch/${pr.number}/${normalizeOid(pr.headRefOid)}`; +} + function normalizeOid(oid: string): string { return oid.trim().toLowerCase(); } @@ -117,21 +121,22 @@ async function fetchSyntheticPrBranch(args: { warning?: string; }): Promise { const syntheticRef = getSyntheticPrHeadRef(args.pr.number); + const verifiedRef = getSyntheticPrVerifiedRef(args.pr); await args.git.raw([ "fetch", "--no-tags", "--quiet", args.remoteName, - syntheticRef, + `+${syntheticRef}:${verifiedRef}`, ]); await assertRefMatchesExpectedOid({ git: args.git, - ref: "FETCH_HEAD", + ref: verifiedRef, expectedHeadOid: args.pr.headRefOid, }); return { kind: "synthetic-pr-ref", - startPoint: "FETCH_HEAD", + startPoint: verifiedRef, mergeRef: syntheticRef, warning: args.warning, }; @@ -159,6 +164,20 @@ export async function configurePrBranchTracking(args: { } } +export async function deleteMaterializedPrBranchIfSafe(args: { + git: GitClient; + branch: string; + expectedHeadOid: string; +}): Promise { + const localOid = await getLocalBranchHead(args.git, args.branch); + if (localOid === null) return false; + if (normalizeOid(localOid) !== normalizeOid(args.expectedHeadOid)) { + return false; + } + await args.git.raw(["branch", "-D", "--", args.branch]); + return true; +} + export async function materializePrBranch(args: { git: GitClient; branch: string; @@ -217,19 +236,28 @@ export async function materializePrBranch(args: { }; } - await args.git.raw([ - "branch", - "--no-track", - "--", - args.branch, - source.startPoint, - ]); - await configurePrBranchTracking({ - git: args.git, - branch: args.branch, - remoteName: args.remoteName, - mergeRef: source.mergeRef, - }); + try { + await args.git.raw([ + "branch", + "--no-track", + "--", + args.branch, + source.startPoint, + ]); + await configurePrBranchTracking({ + git: args.git, + branch: args.branch, + remoteName: args.remoteName, + mergeRef: source.mergeRef, + }); + } catch (err) { + await deleteMaterializedPrBranchIfSafe({ + git: args.git, + branch: args.branch, + expectedHeadOid: args.pr.headRefOid, + }).catch(() => {}); + throw err; + } return { branch: args.branch, diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index b24299fb73b..ab2cd61fd90 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -34,7 +34,11 @@ import { } from "../workspace-creation/utils/ai-workspace-names"; import type { ExecGh } from "../workspace-creation/utils/exec-gh"; import { listBranchNames } from "../workspace-creation/utils/list-branch-names"; -import { materializePrBranch } from "../workspace-creation/utils/pr-branch-materialize"; +import { + deleteMaterializedPrBranchIfSafe, + type MaterializePrBranchResult, + materializePrBranch, +} from "../workspace-creation/utils/pr-branch-materialize"; import { derivePrLocalBranchName } from "../workspace-creation/utils/pr-branch-name"; import { resolveStartPoint } from "../workspace-creation/utils/resolve-start-point"; import { deduplicateBranchName } from "../workspace-creation/utils/sanitize-branch"; @@ -637,8 +641,9 @@ export const workspacesRouter = router({ } } else { let attemptedWorktreeAdd = false; + let materialized: MaterializePrBranchResult | null = null; try { - const materialized = await materializePrBranch({ + materialized = await materializePrBranch({ git, branch: resolvedBranch, remoteName: localProject.remoteName ?? "origin", @@ -658,6 +663,18 @@ export const workspacesRouter = router({ if (attemptedWorktreeAdd) { await rollbackWorktree(); } + if (materialized?.createdBranch) { + await deleteMaterializedPrBranchIfSafe({ + git, + branch: resolvedBranch, + expectedHeadOid: prMetadata.headRefOid, + }).catch((cleanupErr) => { + console.warn( + "[workspaces.create] failed to rollback PR branch", + { branch: resolvedBranch, err: cleanupErr }, + ); + }); + } throw new TRPCError({ code: "CONFLICT", message: diff --git a/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts index 7dc38b19691..511f7d3c227 100644 --- a/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts +++ b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts @@ -9,7 +9,11 @@ import { import { tmpdir } from "node:os"; import { join } from "node:path"; import simpleGit, { type SimpleGit } from "simple-git"; -import { materializePrBranch } from "../../src/trpc/router/workspace-creation/utils/pr-branch-materialize"; +import { + deleteMaterializedPrBranchIfSafe, + getSyntheticPrVerifiedRef, + materializePrBranch, +} from "../../src/trpc/router/workspace-creation/utils/pr-branch-materialize"; import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; interface BareRemoteFixture { @@ -106,6 +110,14 @@ describe("materializePrBranch (real git)", () => { }, }); expect(materialized.sourceKind).toBe("synthetic-pr-ref"); + expect(materialized.startPoint).toBe( + getSyntheticPrVerifiedRef({ + number: 5252, + headRefName: "feature/pr-lockfile", + headRefOid: scenario.prHeadOid, + isCrossRepository: true, + }), + ); const oldFlowPath = realpathSync( mkdtempSync(join(tmpdir(), "host-service-old-pr-worktree-")), @@ -171,4 +183,51 @@ describe("materializePrBranch (real git)", () => { rmSync(worktreePath, { recursive: true, force: true }); } }); + + test("safe cleanup deletes only branches still at the verified PR head", async () => { + await materializePrBranch({ + git: scenario.local.git, + branch: "contributor/cleanup-pr-lockfile", + remoteName: "origin", + pr: { + number: 5252, + headRefName: "feature/pr-lockfile", + headRefOid: scenario.prHeadOid, + isCrossRepository: true, + }, + }); + + await expect( + deleteMaterializedPrBranchIfSafe({ + git: scenario.local.git, + branch: "contributor/cleanup-pr-lockfile", + expectedHeadOid: "1111111111111111111111111111111111111111", + }), + ).resolves.toBe(false); + const branchHeadBeforeCleanup = ( + await scenario.local.git.raw([ + "rev-parse", + "--verify", + "refs/heads/contributor/cleanup-pr-lockfile", + ]) + ).trim(); + expect(branchHeadBeforeCleanup).toBe(scenario.prHeadOid); + + await expect( + deleteMaterializedPrBranchIfSafe({ + git: scenario.local.git, + branch: "contributor/cleanup-pr-lockfile", + expectedHeadOid: scenario.prHeadOid, + }), + ).resolves.toBe(true); + const branchStillExists = await scenario.local.git + .raw([ + "rev-parse", + "--verify", + "refs/heads/contributor/cleanup-pr-lockfile", + ]) + .then(() => true) + .catch(() => false); + expect(branchStillExists).toBe(false); + }); }); From 02623a60a2d3846d9161aef9e70e03ed82259548 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 16 May 2026 15:54:50 -0700 Subject: [PATCH 3/8] address PR checkout review follow-ups --- .../utils/pr-branch-materialize.test.ts | 82 ++++++ .../utils/pr-branch-materialize.ts | 20 +- .../utils/pr-checkout-recovery.test.ts | 232 ---------------- .../utils/pr-checkout-recovery.ts | 181 ------------- .../src/trpc/router/workspaces/workspaces.ts | 6 +- .../pr-checkout-recovery.integration.test.ts | 256 ------------------ 6 files changed, 100 insertions(+), 677 deletions(-) delete mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts delete mode 100644 packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts delete mode 100644 packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index 8e765f173fe..6df63a4374e 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -202,4 +202,86 @@ describe("materializePrBranch", () => { "refs/remotes/origin/feature/x", ]); }); + + test("does not delete a branch when branch creation itself fails", async () => { + let localBranchLookupCount = 0; + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + const ref = args[2] ?? ""; + if (ref === "refs/heads/feature/x^{commit}") { + localBranchLookupCount += 1; + if (localBranchLookupCount === 1) { + throw new Error("branch does not exist before create"); + } + return `${EXPECTED_HEAD_OID}\n`; + } + return `${EXPECTED_HEAD_OID}\n`; + } + if (args[0] === "branch" && args[1] === "--no-track") { + throw new Error("branch was created concurrently"); + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + await expect( + materializePrBranch({ + git, + branch: "feature/x", + remoteName: "origin", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }), + ).rejects.toThrow("branch was created concurrently"); + + expect(localBranchLookupCount).toBe(1); + expect(raw).not.toHaveBeenCalledWith(["branch", "-D", "--", "feature/x"]); + }); + + test("surfaces rollback failure details after creating a branch", async () => { + let localBranchLookupCount = 0; + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + const ref = args[2] ?? ""; + if (ref === "refs/heads/feature/x^{commit}") { + localBranchLookupCount += 1; + if (localBranchLookupCount === 1) { + throw new Error("branch does not exist before create"); + } + return `${EXPECTED_HEAD_OID}\n`; + } + return `${EXPECTED_HEAD_OID}\n`; + } + if (args[0] === "config") { + throw new Error("config failed"); + } + if (args[0] === "branch" && args[1] === "-D") { + throw new Error("cleanup denied"); + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + await expect( + materializePrBranch({ + git, + branch: "feature/x", + remoteName: "origin", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }), + ).rejects.toThrow( + 'Failed to materialize PR branch "feature/x": config failed. Failed to roll back created branch: cleanup denied', + ); + + expect(raw).toHaveBeenCalledWith(["branch", "-D", "--", "feature/x"]); + }); }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts index 3904f9bbf48..fb811be3443 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -236,6 +236,7 @@ export async function materializePrBranch(args: { }; } + let branchCreated = false; try { await args.git.raw([ "branch", @@ -244,6 +245,7 @@ export async function materializePrBranch(args: { args.branch, source.startPoint, ]); + branchCreated = true; await configurePrBranchTracking({ git: args.git, branch: args.branch, @@ -251,11 +253,19 @@ export async function materializePrBranch(args: { mergeRef: source.mergeRef, }); } catch (err) { - await deleteMaterializedPrBranchIfSafe({ - git: args.git, - branch: args.branch, - expectedHeadOid: args.pr.headRefOid, - }).catch(() => {}); + if (branchCreated) { + try { + await deleteMaterializedPrBranchIfSafe({ + git: args.git, + branch: args.branch, + expectedHeadOid: args.pr.headRefOid, + }); + } catch (cleanupErr) { + throw new Error( + `Failed to materialize PR branch "${args.branch}": ${err instanceof Error ? err.message : String(err)}. Failed to roll back created branch: ${cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr)}`, + ); + } + } throw err; } 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 deleted file mode 100644 index dc373399d85..00000000000 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts +++ /dev/null @@ -1,232 +0,0 @@ -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 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")), - ).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 deleted file mode 100644 index 2534d08488e..00000000000 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts +++ /dev/null @@ -1,181 +0,0 @@ -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(); - - // 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"; - } - - 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. - * - * 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, - 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 PR head 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.", - }; -} diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index ab2cd61fd90..658e57fb5b0 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -640,7 +640,7 @@ export const workspacesRouter = router({ }); } } else { - let attemptedWorktreeAdd = false; + let worktreeAddStarted = false; let materialized: MaterializePrBranchResult | null = null; try { materialized = await materializePrBranch({ @@ -652,7 +652,7 @@ export const workspacesRouter = router({ if (materialized.warning) { console.warn(`[workspaces.create] ${materialized.warning}`); } - attemptedWorktreeAdd = true; + worktreeAddStarted = true; await git.raw([ "worktree", "add", @@ -660,7 +660,7 @@ export const workspacesRouter = router({ resolvedBranch, ]); } catch (err) { - if (attemptedWorktreeAdd) { + if (worktreeAddStarted) { await rollbackWorktree(); } if (materialized?.createdBranch) { 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 deleted file mode 100644 index ed0e86410b6..00000000000 --- a/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts +++ /dev/null @@ -1,256 +0,0 @@ -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.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 - // 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 955d0800f650621e521a6972ffb53136c60e5f77 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 16 May 2026 16:13:43 -0700 Subject: [PATCH 4/8] fix fork PR checkout follow-ups --- .../workspace-creates/useWorkspaceCreates.ts | 4 + .../utils/pr-branch-materialize.test.ts | 124 ++++++++++-- .../utils/pr-branch-materialize.ts | 190 +++++++++++++++--- .../src/trpc/router/workspaces/workspaces.ts | 15 +- .../workspace-create-pr.integration.test.ts | 82 ++++++++ .../mcp-v2/src/tools/workspaces/create.ts | 1 + packages/sdk/src/resources/workspaces.ts | 1 + .../20260516-pr-worktree-checkout-flow.md | 31 ++- 8 files changed, 384 insertions(+), 64 deletions(-) diff --git a/apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts b/apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts index 2b2ce94cb2a..e3ce2d78367 100644 --- a/apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts +++ b/apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts @@ -1,4 +1,5 @@ import type { WorkspaceState } from "@superset/panes"; +import { toast } from "@superset/ui/sonner"; import { useCallback } from "react"; import { resolveHostUrl } from "renderer/hooks/host-service/useHostTargetUrl"; import { useRelayUrl } from "renderer/hooks/useRelayUrl"; @@ -139,6 +140,9 @@ export function useWorkspaceCreates(): UseWorkspaceCreatesApi { if (result.alreadyExists && result.workspace.id !== workspaceId) { useWorkspaceCreatesStore.getState().remove(workspaceId); } + for (const warning of result.warnings ?? []) { + toast.warning(warning); + } return { ok: true, workspaceId: result.workspace.id, diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index 6df63a4374e..ed0810f389c 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -74,13 +74,15 @@ describe("materializePrBranch", () => { ]); }); - test("cross-repo PR fetches the synthetic PR ref and configures it as the merge ref", async () => { + test("cross-repo PR configures fork push defaults from the synthetic PR ref", async () => { const { git, raw } = createMockGit(); const pr = { number: 456, headRefName: "feature/x", headRefOid: EXPECTED_HEAD_OID, isCrossRepository: true, + headRepositoryOwner: "alice", + headRepositoryName: "fork", }; const verifiedRef = getSyntheticPrVerifiedRef(pr); @@ -95,8 +97,8 @@ describe("materializePrBranch", () => { createdBranch: true, sourceKind: "synthetic-pr-ref", startPoint: verifiedRef, - trackingRemote: "origin", - trackingMergeRef: "refs/pull/456/head", + trackingRemote: "superset-pr-456", + trackingMergeRef: "refs/heads/feature/x", }); expect(raw).toHaveBeenNthCalledWith(1, [ "fetch", @@ -113,12 +115,61 @@ describe("materializePrBranch", () => { verifiedRef, ]); expect(raw).toHaveBeenNthCalledWith(6, [ + "remote", + "add", + "superset-pr-456", + "https://github.com/alice/fork.git", + ]); + expect(raw).toHaveBeenNthCalledWith(7, [ + "update-ref", + "refs/remotes/superset-pr-456/feature/x", + EXPECTED_HEAD_OID, + ]); + expect(raw).toHaveBeenNthCalledWith(8, [ + "config", + "branch.alice/feature/x.remote", + "superset-pr-456", + ]); + expect(raw).toHaveBeenNthCalledWith(9, [ "config", "branch.alice/feature/x.merge", - "refs/pull/456/head", + "refs/heads/feature/x", + ]); + expect(raw).toHaveBeenNthCalledWith(10, [ + "config", + "branch.alice/feature/x.pushRemote", + "superset-pr-456", + ]); + expect(raw).toHaveBeenNthCalledWith(11, [ + "config", + "--replace-all", + "remote.superset-pr-456.push", + "HEAD:refs/heads/feature/x", ]); }); + test("cross-repo PR warns when fork repository metadata is missing", async () => { + const { git } = createMockGit(); + + const result = await materializePrBranch({ + git, + branch: "alice/feature/x", + remoteName: "origin", + pr: { + number: 456, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: true, + }, + }); + + expect(result).toMatchObject({ + trackingRemote: "origin", + trackingMergeRef: "refs/pull/456/head", + }); + expect(result.warning).toContain("Plain git push may require"); + }); + test("deletes a materialized branch only when it still points at the verified PR head", async () => { const raw = mock(async (args: string[]) => { if (args[0] === "rev-parse") { @@ -170,6 +221,42 @@ describe("materializePrBranch", () => { ]); }); + test("adopts an existing local branch that already points at the PR head", async () => { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + return `${EXPECTED_HEAD_OID}\n`; + } + return ""; + }); + const git = { raw } as unknown as GitClient; + + const result = await materializePrBranch({ + git, + branch: "feature/x", + remoteName: "origin", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }); + + expect(result.createdBranch).toBe(false); + expect(raw).not.toHaveBeenCalledWith([ + "branch", + "--no-track", + "--", + "feature/x", + "refs/remotes/origin/feature/x", + ]); + expect(raw).toHaveBeenCalledWith([ + "config", + "branch.feature/x.remote", + "origin", + ]); + }); + test("aborts before branch creation when the fetched ref does not match GitHub headRefOid", async () => { const raw = mock(async (args: string[]) => { if (args[0] === "rev-parse") { @@ -203,7 +290,7 @@ describe("materializePrBranch", () => { ]); }); - test("does not delete a branch when branch creation itself fails", async () => { + test("adopts a matching branch created by a concurrent caller", async () => { let localBranchLookupCount = 0; const raw = mock(async (args: string[]) => { if (args[0] === "rev-parse") { @@ -224,21 +311,20 @@ describe("materializePrBranch", () => { }); const git = { raw } as unknown as GitClient; - await expect( - materializePrBranch({ - git, - branch: "feature/x", - remoteName: "origin", - pr: { - number: 123, - headRefName: "feature/x", - headRefOid: EXPECTED_HEAD_OID, - isCrossRepository: false, - }, - }), - ).rejects.toThrow("branch was created concurrently"); + const result = await materializePrBranch({ + git, + branch: "feature/x", + remoteName: "origin", + pr: { + number: 123, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: false, + }, + }); - expect(localBranchLookupCount).toBe(1); + expect(result.createdBranch).toBe(false); + expect(localBranchLookupCount).toBe(2); expect(raw).not.toHaveBeenCalledWith(["branch", "-D", "--", "feature/x"]); }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts index fb811be3443..952928b5f82 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -7,6 +7,8 @@ export interface PrBranchMetadata { headRefName: string; headRefOid: string; isCrossRepository: boolean; + headRepositoryOwner?: string | null; + headRepositoryName?: string | null; } export interface MaterializePrBranchResult { @@ -22,10 +24,23 @@ export interface MaterializePrBranchResult { interface PrBranchSource { kind: PrBranchSourceKind; startPoint: string; + trackingRemote: string; mergeRef: string; + remoteUrl?: string; + pushRemote?: string; + pushRef?: string; + remoteTrackingBranch?: string; + remoteTrackingOid?: string; warning?: string; } +export class PrBranchConflictError extends Error { + constructor(message: string) { + super(message); + this.name = "PrBranchConflictError"; + } +} + class SameRepoBranchFetchError extends Error { constructor( message: string, @@ -48,6 +63,24 @@ function normalizeOid(oid: string): string { return oid.trim().toLowerCase(); } +function normalizeRemoteUrl(url: string): string { + return url + .trim() + .replace(/\.git$/, "") + .toLowerCase(); +} + +function getForkRemoteName(prNumber: number): string { + return `superset-pr-${prNumber}`; +} + +function getHeadRepositoryUrl(pr: PrBranchMetadata): string | null { + const owner = pr.headRepositoryOwner?.trim(); + const name = pr.headRepositoryName?.trim(); + if (!owner || !name) return null; + return `https://github.com/${owner}/${name}.git`; +} + async function revParseCommit(git: GitClient, ref: string): Promise { const oid = await git.raw(["rev-parse", "--verify", `${ref}^{commit}`]); const trimmed = oid.trim(); @@ -82,6 +115,41 @@ async function getLocalBranchHead( } } +async function getRemoteUrl( + git: GitClient, + remoteName: string, +): Promise { + try { + return (await git.raw(["remote", "get-url", remoteName])).trim() || null; + } catch { + return null; + } +} + +async function ensureRemoteUrl(args: { + git: GitClient; + remoteName: string; + remoteUrl: string; +}): Promise { + for (let attempt = 0; attempt < 10; attempt += 1) { + const candidate = + attempt === 0 ? args.remoteName : `${args.remoteName}-${attempt + 1}`; + const existingUrl = await getRemoteUrl(args.git, candidate); + if (existingUrl === null) { + await args.git.raw(["remote", "add", candidate, args.remoteUrl]); + return candidate; + } + if ( + normalizeRemoteUrl(existingUrl) === normalizeRemoteUrl(args.remoteUrl) + ) { + return candidate; + } + } + throw new Error( + `Could not configure a remote for ${args.remoteUrl}; remote names based on "${args.remoteName}" are already in use`, + ); +} + async function fetchSameRepoPrBranch(args: { git: GitClient; remoteName: string; @@ -110,6 +178,7 @@ async function fetchSameRepoPrBranch(args: { return { kind: "head-branch", startPoint: remoteTrackingRef, + trackingRemote: args.remoteName, mergeRef: `refs/heads/${args.pr.headRefName}`, }; } @@ -134,11 +203,29 @@ async function fetchSyntheticPrBranch(args: { ref: verifiedRef, expectedHeadOid: args.pr.headRefOid, }); + const forkRemoteUrl = args.pr.isCrossRepository + ? getHeadRepositoryUrl(args.pr) + : null; + const forkRemoteName = getForkRemoteName(args.pr.number); return { kind: "synthetic-pr-ref", startPoint: verifiedRef, - mergeRef: syntheticRef, - warning: args.warning, + trackingRemote: forkRemoteUrl ? forkRemoteName : args.remoteName, + mergeRef: forkRemoteUrl + ? `refs/heads/${args.pr.headRefName}` + : syntheticRef, + remoteUrl: forkRemoteUrl ?? undefined, + pushRemote: forkRemoteUrl ? forkRemoteName : undefined, + pushRef: forkRemoteUrl + ? `HEAD:refs/heads/${args.pr.headRefName}` + : undefined, + remoteTrackingBranch: forkRemoteUrl ? args.pr.headRefName : undefined, + remoteTrackingOid: forkRemoteUrl ? args.pr.headRefOid : undefined, + warning: + args.warning ?? + (args.pr.isCrossRepository && !forkRemoteUrl + ? `Superset checked out PR #${args.pr.number} through ${syntheticRef}, but GitHub did not return the fork repository. Plain git push may require manual remote configuration.` + : undefined), }; } @@ -147,21 +234,53 @@ export async function configurePrBranchTracking(args: { branch: string; remoteName: string; mergeRef: string; + remoteUrl?: string; pushRemote?: string; -}): Promise { + pushRef?: string; + remoteTrackingBranch?: string; + remoteTrackingOid?: string; +}): Promise { + const trackingRemote = args.remoteUrl + ? await ensureRemoteUrl({ + git: args.git, + remoteName: args.remoteName, + remoteUrl: args.remoteUrl, + }) + : args.remoteName; + const pushRemote = + args.pushRemote && args.pushRemote === args.remoteName + ? trackingRemote + : args.pushRemote; + + if (args.remoteTrackingBranch && args.remoteTrackingOid) { + await args.git.raw([ + "update-ref", + `refs/remotes/${trackingRemote}/${args.remoteTrackingBranch}`, + args.remoteTrackingOid, + ]); + } await args.git.raw([ "config", `branch.${args.branch}.remote`, - args.remoteName, + trackingRemote, ]); await args.git.raw(["config", `branch.${args.branch}.merge`, args.mergeRef]); - if (args.pushRemote) { + if (pushRemote) { await args.git.raw([ "config", `branch.${args.branch}.pushRemote`, - args.pushRemote, + pushRemote, ]); } + if (pushRemote && args.pushRef) { + await args.git.raw([ + "config", + "--replace-all", + `remote.${pushRemote}.push`, + args.pushRef, + ]); + } + return trackingRemote; } export async function deleteMaterializedPrBranchIfSafe(args: { @@ -212,28 +331,37 @@ export async function materializePrBranch(args: { } } - const existingOid = await getLocalBranchHead(args.git, args.branch); - if (existingOid !== null) { - if (normalizeOid(existingOid) !== normalizeOid(args.pr.headRefOid)) { - throw new Error( - `Local branch "${args.branch}" exists and points at ${existingOid}, not PR head ${args.pr.headRefOid}`, - ); - } - await configurePrBranchTracking({ + const configureTracking = async (createdBranch: boolean) => { + const trackingRemote = await configurePrBranchTracking({ git: args.git, branch: args.branch, - remoteName: args.remoteName, + remoteName: source.trackingRemote, mergeRef: source.mergeRef, + remoteUrl: source.remoteUrl, + pushRemote: source.pushRemote, + pushRef: source.pushRef, + remoteTrackingBranch: source.remoteTrackingBranch, + remoteTrackingOid: source.remoteTrackingOid, }); return { branch: args.branch, - createdBranch: false, + createdBranch, sourceKind: source.kind, startPoint: source.startPoint, - trackingRemote: args.remoteName, + trackingRemote, trackingMergeRef: source.mergeRef, warning: source.warning, }; + }; + + const existingOid = await getLocalBranchHead(args.git, args.branch); + if (existingOid !== null) { + if (normalizeOid(existingOid) !== normalizeOid(args.pr.headRefOid)) { + throw new PrBranchConflictError( + `Local branch "${args.branch}" exists and points at ${existingOid}, not PR head ${args.pr.headRefOid}`, + ); + } + return await configureTracking(false); } let branchCreated = false; @@ -246,13 +374,19 @@ export async function materializePrBranch(args: { source.startPoint, ]); branchCreated = true; - await configurePrBranchTracking({ - git: args.git, - branch: args.branch, - remoteName: args.remoteName, - mergeRef: source.mergeRef, - }); + return await configureTracking(true); } catch (err) { + if (!branchCreated) { + const concurrentOid = await getLocalBranchHead(args.git, args.branch); + if (concurrentOid !== null) { + if (normalizeOid(concurrentOid) === normalizeOid(args.pr.headRefOid)) { + return await configureTracking(false); + } + throw new PrBranchConflictError( + `Local branch "${args.branch}" exists and points at ${concurrentOid}, not PR head ${args.pr.headRefOid}`, + ); + } + } if (branchCreated) { try { await deleteMaterializedPrBranchIfSafe({ @@ -268,14 +402,4 @@ export async function materializePrBranch(args: { } throw err; } - - return { - branch: args.branch, - createdBranch: true, - sourceKind: source.kind, - startPoint: source.startPoint, - trackingRemote: args.remoteName, - trackingMergeRef: source.mergeRef, - warning: source.warning, - }; } diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index 658e57fb5b0..c0b021ad820 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -38,6 +38,7 @@ import { deleteMaterializedPrBranchIfSafe, type MaterializePrBranchResult, materializePrBranch, + PrBranchConflictError, } from "../workspace-creation/utils/pr-branch-materialize"; import { derivePrLocalBranchName } from "../workspace-creation/utils/pr-branch-name"; import { resolveStartPoint } from "../workspace-creation/utils/resolve-start-point"; @@ -122,6 +123,7 @@ interface PrMetadata { headRefOid: string; baseRefName: string; headRepositoryOwner: string; + headRepositoryName: string; isCrossRepository: boolean; state: "open" | "closed" | "merged"; } @@ -137,7 +139,7 @@ async function fetchPrMetadata(args: { "view", String(args.prNumber), "--json", - "number,url,title,headRefName,headRefOid,baseRefName,headRepositoryOwner,isCrossRepository,state", + "number,url,title,headRefName,headRefOid,baseRefName,headRepositoryOwner,headRepository,isCrossRepository,state", ], { cwd: args.cwd, timeout: 30_000 }, ); @@ -149,6 +151,7 @@ async function fetchPrMetadata(args: { headRefOid: string; baseRefName: string; headRepositoryOwner: { login: string } | null; + headRepository: { name: string } | null; isCrossRepository: boolean; state: string; }; @@ -167,6 +170,7 @@ async function fetchPrMetadata(args: { headRefOid: parsed.headRefOid, baseRefName: parsed.baseRefName, headRepositoryOwner: parsed.headRepositoryOwner?.login ?? "", + headRepositoryName: parsed.headRepository?.name ?? "", isCrossRepository: parsed.isCrossRepository, state, }; @@ -542,6 +546,7 @@ export const workspacesRouter = router({ let alreadyExists = false; let workspaceRow: CloudWorkspace; let prMetadata: PrMetadata | null = null; + const warnings: string[] = []; if (input.pr !== undefined) { prMetadata = await fetchPrMetadata({ @@ -651,6 +656,7 @@ export const workspacesRouter = router({ }); if (materialized.warning) { console.warn(`[workspaces.create] ${materialized.warning}`); + warnings.push(materialized.warning); } worktreeAddStarted = true; await git.raw([ @@ -676,7 +682,10 @@ export const workspacesRouter = router({ }); } throw new TRPCError({ - code: "CONFLICT", + code: + worktreeAddStarted || err instanceof PrBranchConflictError + ? "CONFLICT" + : "INTERNAL_SERVER_ERROR", message: err instanceof Error ? err.message @@ -942,6 +951,7 @@ export const workspacesRouter = router({ }); if (warning) { console.warn(`[workspaces.create] setup warning: ${warning}`); + warnings.push(warning); } if (terminal) { terminalsResult.push({ @@ -962,6 +972,7 @@ export const workspacesRouter = router({ terminals: terminalsResult, agents: agentsResult, alreadyExists, + warnings, }; }), diff --git a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts index 0f481ff753d..d8c572ba885 100644 --- a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts +++ b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts @@ -9,6 +9,7 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { TRPCClientError } from "@trpc/client"; import { eq } from "drizzle-orm"; import simpleGit from "simple-git"; import { workspaces } from "../../src/db/schema"; @@ -76,6 +77,7 @@ describe("workspaces.create PR checkout integration", () => { headRefOid: prHeadOid, baseRefName: "main", headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, isCrossRepository: true, state: "OPEN", }; @@ -151,6 +153,20 @@ describe("workspaces.create PR checkout integration", () => { const worktreeGit = simpleGit(worktreePath); const head = (await worktreeGit.raw(["rev-parse", "HEAD"])).trim(); expect(head).toBe(prHeadOid); + expect( + ( + await scenario.repo.git.raw([ + "config", + "branch.contributor/feature/pr-lockfile.pushRemote", + ]) + ).trim(), + ).toBe("superset-pr-6060"); + expect( + ( + await scenario.repo.git.raw(["config", "remote.superset-pr-6060.push"]) + ).trim(), + ).toBe("HEAD:refs/heads/feature/pr-lockfile"); + expect(result.warnings).toEqual([]); const lockStatus = ( await worktreeGit.raw([ @@ -162,4 +178,70 @@ describe("workspaces.create PR checkout integration", () => { ).trim(); expect(lockStatus).toContain("package-lock.json"); }); + + test("reports PR head verification failures as internal errors", async () => { + const prNumber = 7070; + const staleHeadOid = "1111111111111111111111111111111111111111"; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Stale PR metadata", + headRefName: "feature/stale", + headRefOid: staleHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, + isCrossRepository: true, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + dispose = async () => { + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/stale", "main"); + prHeadOid = await scenario.repo.commit("actual PR head", { + "feature.txt": "actual\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/stale", true); + + const error = await scenario.host.trpc.workspaces.create + .mutate({ + projectId: scenario.projectId, + name: "Stale PR workspace", + pr: prNumber, + }) + .catch((err: unknown) => err); + + expect(error).toBeInstanceOf(TRPCClientError); + expect((error as { data?: { code?: string } }).data?.code).toBe( + "INTERNAL_SERVER_ERROR", + ); + expect(error).toHaveProperty("message"); + expect(String((error as Error).message)).toContain( + "did not match GitHub headRefOid", + ); + }); }); diff --git a/packages/mcp-v2/src/tools/workspaces/create.ts b/packages/mcp-v2/src/tools/workspaces/create.ts index ec5d1036c29..1a8f5df95c0 100644 --- a/packages/mcp-v2/src/tools/workspaces/create.ts +++ b/packages/mcp-v2/src/tools/workspaces/create.ts @@ -79,6 +79,7 @@ export function register(server: McpServer): void { | { ok: false; error: string } >; alreadyExists: boolean; + warnings: string[]; }>( { relayUrl: ctx.relayUrl, diff --git a/packages/sdk/src/resources/workspaces.ts b/packages/sdk/src/resources/workspaces.ts index 17f28aa3c80..03fc10fd56a 100644 --- a/packages/sdk/src/resources/workspaces.ts +++ b/packages/sdk/src/resources/workspaces.ts @@ -202,6 +202,7 @@ export interface WorkspaceCreateResult { terminals: Array<{ terminalId: string; label?: string }>; agents: WorkspaceCreateAgentResult[]; alreadyExists: boolean; + warnings: string[]; } export interface WorkspaceUpdateParams { diff --git a/plans/done/20260516-pr-worktree-checkout-flow.md b/plans/done/20260516-pr-worktree-checkout-flow.md index 6e79c7c816f..28d9fd9edd5 100644 --- a/plans/done/20260516-pr-worktree-checkout-flow.md +++ b/plans/done/20260516-pr-worktree-checkout-flow.md @@ -230,17 +230,21 @@ For cross-repo PRs or deleted head branches: 1. Fetch GitHub's synthetic PR head ref from the base remote: ```text - git fetch --no-tags --quiet refs/pull//head + git fetch --no-tags --quiet +refs/pull//head:refs/superset/pr-fetch// ``` -2. Verify `FETCH_HEAD^{commit}` equals `headRefOid`. -3. Create the local branch from `FETCH_HEAD`. +2. Verify `refs/superset/pr-fetch//^{commit}` equals + `headRefOid`. This avoids `FETCH_HEAD`, which is shared across concurrent + fetches in the same clone. +3. Create the local branch from the verified named ref. 4. Configure: - - `branch..remote = ` + - `branch..remote = ` - `branch..merge = refs/pull//head` for synthetic-ref fallback, or `refs/heads/` when a real fork remote is available. - - `branch..pushRemote = ` when we have fork clone URL - metadata and pushing to the fork should work. + - `branch..pushRemote = ` and + `remote..push = HEAD:refs/heads/` for cross-repo + PRs with `headRepository` metadata, so plain `git push` targets the fork + branch instead of GitHub's read-only synthetic PR ref. This preserves the safety check v2 already has in recovery: every fetched PR head must match GitHub's `headRefOid` before we create or update a branch. @@ -331,8 +335,8 @@ verified branch, not by running `gh pr checkout`. - Do not call `gh pr checkout` in worktree mode. - Do not force-reset a pre-existing user branch that points at a different commit. -- Do not solve every fork push edge case in the first pass unless we add the - necessary `headRepository` metadata. +- Do not point fork PR pushes at GitHub's read-only `refs/pull//head` + synthetic ref. ## Implementation Status @@ -343,6 +347,10 @@ Implemented for the canonical v2 host-service path: - The PR path no longer shells out to `gh pr checkout` in worktree mode. - `workspaces.create` uses `ctx.execGh` for PR metadata so integration tests can mock GitHub CLI behavior. +- Cross-repo PRs with `headRepository` metadata get a Superset-managed fork + remote plus a push refspec so plain `git push` targets the contributor branch. +- Materialization warnings are returned from `workspaces.create` and surfaced by + the desktop workspace-create flow. - Regression coverage now includes command-level helper tests, real-git materialization tests, and a `workspaces.create` integration test that proves the dirty-placeholder failure class no longer blocks PR workspace creation. @@ -350,5 +358,8 @@ Implemented for the canonical v2 host-service path: Remaining follow-ups: - Decide whether to backport the same shape to legacy v1 desktop code. -- Add richer fork push metadata (`headRepository`, `maintainerCanModify`, - `pushRemote`) if we want GitHub CLI parity for fork push UX. +- Add a small prune/sweep for stale `refs/superset/pr-fetch/*` verified refs + from old PR force-pushes. +- Add richer fork permission handling (`maintainerCanModify`, deleted forks, + inaccessible forks) if we need exact GitHub CLI parity for every fork push + edge case. From ab211411d71f3d5ebe0366040f4581cf386a9c47 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 17 May 2026 10:14:13 -0700 Subject: [PATCH 5/8] cover PR workspace edge cases --- .../src/trpc/router/workspaces/workspaces.ts | 339 ++++++++------- .../workspace-create-pr.integration.test.ts | 393 +++++++++++++++++- 2 files changed, 570 insertions(+), 162 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index c0b021ad820..920f71d0610 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -83,6 +83,29 @@ const createInputSchema = z message: "`worktreePath` and `pr` cannot both be set", }); +const workspaceCreateLocks = new Map>(); + +async function acquireWorkspaceCreateLock(key: string): Promise<() => void> { + const previous = workspaceCreateLocks.get(key) ?? Promise.resolve(); + let releaseCurrent!: () => void; + const current = new Promise((resolve) => { + releaseCurrent = resolve; + }); + const entry = previous.catch(() => {}).then(() => current); + workspaceCreateLocks.set(key, entry); + await previous.catch(() => {}); + + let released = false; + return () => { + if (released) return; + released = true; + releaseCurrent(); + if (workspaceCreateLocks.get(key) === entry) { + workspaceCreateLocks.delete(key); + } + }; +} + type AgentLaunchResult = | ({ ok: true } & AgentRunResult) | { ok: false; error: string }; @@ -549,178 +572,190 @@ export const workspacesRouter = router({ const warnings: string[] = []; if (input.pr !== undefined) { - prMetadata = await fetchPrMetadata({ - cwd: localProject.repoPath, - prNumber: input.pr, - execGh: ctx.execGh, - }); - resolvedBranch = derivePrLocalBranchName(prMetadata); - - const existing = await findExistingWorkspaceByBranch( - ctx, - input.projectId, - resolvedBranch, + const releaseCreateLock = await acquireWorkspaceCreateLock( + `pr:${input.projectId}:${input.pr}`, ); - if (existing) { - workspaceRow = existing; - alreadyExists = true; - } else { - const localOid = await getLocalBranchHead(git, resolvedBranch); - const adoptLocalBranch = - localOid !== null && - localOid.toLowerCase() === - prMetadata.headRefOid.trim().toLowerCase(); - // If the local branch already lives in a worktree somewhere, - // `git worktree add` will refuse. Look it up first so the - // OID-mismatch error can point at the actual worktree, and - // the matching-OID case can adopt instead of duplicating. - const existingWorktreePath = ( - await listWorktreeBranches(git) - ).worktreeMap.get(resolvedBranch); - - if (localOid !== null && !adoptLocalBranch) { - const cleanupHint = existingWorktreePath - ? `Inspect with \`git log ${resolvedBranch}\`, then \`git worktree remove ${existingWorktreePath}\` and \`git branch -D ${resolvedBranch}\` if safe.` - : `Inspect with \`git log ${resolvedBranch}\`, then \`git branch -D ${resolvedBranch}\` if safe.`; - throw new TRPCError({ - code: "CONFLICT", - message: `Local branch "${resolvedBranch}" exists outside Superset and points at a different commit than PR #${input.pr} (local ${localOid.slice(0, 7)}, PR ${prMetadata.headRefOid.slice(0, 7)}). ${cleanupHint}`, - }); - } + try { + prMetadata = await fetchPrMetadata({ + cwd: localProject.repoPath, + prNumber: input.pr, + execGh: ctx.execGh, + }); + resolvedBranch = derivePrLocalBranchName(prMetadata); - if (adoptLocalBranch && existingWorktreePath) { - worktreePath = existingWorktreePath; - const result = await adoptExistingWorktree({ - ctx, - git, - projectId: input.projectId, - branch: resolvedBranch, - worktreePath, - workspaceName: input.name ?? prMetadata.title ?? resolvedBranch, - baseBranch: prMetadata.baseRefName, - idempotencyId: input.id, - taskId: input.taskId, - hostPromise, - }); - workspaceRow = result.workspace; - alreadyExists = result.alreadyExists; - await enablePushAutoSetupRemote( - git, - worktreePath, - "[workspaces.create]", - ); + const existing = await findExistingWorkspaceByBranch( + ctx, + input.projectId, + resolvedBranch, + ); + if (existing) { + workspaceRow = existing; + alreadyExists = true; } else { - worktreePath = safeResolveWorktreePath( - localProject.id, - resolvedBranch, - ); - mkdirSync(dirname(worktreePath), { recursive: true }); + const localOid = await getLocalBranchHead(git, resolvedBranch); + const adoptLocalBranch = + localOid !== null && + localOid.toLowerCase() === + prMetadata.headRefOid.trim().toLowerCase(); + // If the local branch already lives in a worktree somewhere, + // `git worktree add` will refuse. Look it up first so the + // OID-mismatch error can point at the actual worktree, and + // the matching-OID case can adopt instead of duplicating. + const existingWorktreePath = ( + await listWorktreeBranches(git) + ).worktreeMap.get(resolvedBranch); - const rollbackWorktree = async () => { - try { - await git.raw(["worktree", "remove", "--force", worktreePath]); - } catch (err) { - console.warn( - "[workspaces.create] failed to rollback PR worktree", - { worktreePath, err }, - ); - } - }; + if (localOid !== null && !adoptLocalBranch) { + const cleanupHint = existingWorktreePath + ? `Inspect with \`git log ${resolvedBranch}\`, then \`git worktree remove ${existingWorktreePath}\` and \`git branch -D ${resolvedBranch}\` if safe.` + : `Inspect with \`git log ${resolvedBranch}\`, then \`git branch -D ${resolvedBranch}\` if safe.`; + throw new TRPCError({ + code: "CONFLICT", + message: `Local branch "${resolvedBranch}" exists outside Superset and points at a different commit than PR #${input.pr} (local ${localOid.slice(0, 7)}, PR ${prMetadata.headRefOid.slice(0, 7)}). ${cleanupHint}`, + }); + } - if (adoptLocalBranch) { - try { - await git.raw([ - "worktree", - "add", - worktreePath, - resolvedBranch, - ]); - } catch (err) { - throw new TRPCError({ - code: "CONFLICT", - message: - err instanceof Error - ? err.message - : "Failed to add worktree for existing branch", - }); - } + if (adoptLocalBranch && existingWorktreePath) { + worktreePath = existingWorktreePath; + const result = await adoptExistingWorktree({ + ctx, + git, + projectId: input.projectId, + branch: resolvedBranch, + worktreePath, + workspaceName: input.name ?? prMetadata.title ?? resolvedBranch, + baseBranch: prMetadata.baseRefName, + idempotencyId: input.id, + taskId: input.taskId, + hostPromise, + }); + workspaceRow = result.workspace; + alreadyExists = result.alreadyExists; + await enablePushAutoSetupRemote( + git, + worktreePath, + "[workspaces.create]", + ); } else { - let worktreeAddStarted = false; - let materialized: MaterializePrBranchResult | null = null; - try { - materialized = await materializePrBranch({ - git, - branch: resolvedBranch, - remoteName: localProject.remoteName ?? "origin", - pr: prMetadata, - }); - if (materialized.warning) { - console.warn(`[workspaces.create] ${materialized.warning}`); - warnings.push(materialized.warning); + worktreePath = safeResolveWorktreePath( + localProject.id, + resolvedBranch, + ); + mkdirSync(dirname(worktreePath), { recursive: true }); + + const rollbackWorktree = async () => { + try { + await git.raw([ + "worktree", + "remove", + "--force", + worktreePath, + ]); + } catch (err) { + console.warn( + "[workspaces.create] failed to rollback PR worktree", + { worktreePath, err }, + ); } - worktreeAddStarted = true; - await git.raw([ - "worktree", - "add", - worktreePath, - resolvedBranch, - ]); - } catch (err) { - if (worktreeAddStarted) { - await rollbackWorktree(); + }; + + if (adoptLocalBranch) { + try { + await git.raw([ + "worktree", + "add", + worktreePath, + resolvedBranch, + ]); + } catch (err) { + throw new TRPCError({ + code: "CONFLICT", + message: + err instanceof Error + ? err.message + : "Failed to add worktree for existing branch", + }); } - if (materialized?.createdBranch) { - await deleteMaterializedPrBranchIfSafe({ + } else { + let worktreeAddStarted = false; + let materialized: MaterializePrBranchResult | null = null; + try { + materialized = await materializePrBranch({ git, branch: resolvedBranch, - expectedHeadOid: prMetadata.headRefOid, - }).catch((cleanupErr) => { - console.warn( - "[workspaces.create] failed to rollback PR branch", - { branch: resolvedBranch, err: cleanupErr }, - ); + remoteName: localProject.remoteName ?? "origin", + pr: prMetadata, + }); + if (materialized.warning) { + console.warn(`[workspaces.create] ${materialized.warning}`); + warnings.push(materialized.warning); + } + worktreeAddStarted = true; + await git.raw([ + "worktree", + "add", + worktreePath, + resolvedBranch, + ]); + } catch (err) { + if (worktreeAddStarted) { + await rollbackWorktree(); + } + if (materialized?.createdBranch) { + await deleteMaterializedPrBranchIfSafe({ + git, + branch: resolvedBranch, + expectedHeadOid: prMetadata.headRefOid, + }).catch((cleanupErr) => { + console.warn( + "[workspaces.create] failed to rollback PR branch", + { branch: resolvedBranch, err: cleanupErr }, + ); + }); + } + throw new TRPCError({ + code: + worktreeAddStarted || err instanceof PrBranchConflictError + ? "CONFLICT" + : "INTERNAL_SERVER_ERROR", + message: + err instanceof Error + ? err.message + : "Failed to prepare PR worktree", }); } - throw new TRPCError({ - code: - worktreeAddStarted || err instanceof PrBranchConflictError - ? "CONFLICT" - : "INTERNAL_SERVER_ERROR", - message: - err instanceof Error - ? err.message - : "Failed to prepare PR worktree", - }); } - } - await enablePushAutoSetupRemote( - git, - worktreePath, - "[workspaces.create]", - ); - - workspaceRow = await registerCloudAndLocal({ - ctx, - id: input.id, - projectId: input.projectId, - name: input.name ?? prMetadata.title ?? resolvedBranch, - branch: resolvedBranch, - worktreePath, - taskId: input.taskId, - rollbackWorktree, - hostPromise, - }); - - if (prMetadata.baseRefName) { - await recordBaseBranchConfig({ + await enablePushAutoSetupRemote( git, worktreePath, + "[workspaces.create]", + ); + + workspaceRow = await registerCloudAndLocal({ + ctx, + id: input.id, + projectId: input.projectId, + name: input.name ?? prMetadata.title ?? resolvedBranch, branch: resolvedBranch, - baseBranch: prMetadata.baseRefName, + worktreePath, + taskId: input.taskId, + rollbackWorktree, + hostPromise, }); + + if (prMetadata.baseRefName) { + await recordBaseBranchConfig({ + git, + worktreePath, + branch: resolvedBranch, + baseBranch: prMetadata.baseRefName, + }); + } } } + } finally { + releaseCreateLock(); } } else if (input.worktreePath) { // Read the branch from git rather than trusting `input.branch` diff --git a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts index d8c572ba885..c63fbab6435 100644 --- a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts +++ b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; import { chmodSync, existsSync, @@ -11,7 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { TRPCClientError } from "@trpc/client"; import { eq } from "drizzle-orm"; -import simpleGit from "simple-git"; +import simpleGit, { type SimpleGit } from "simple-git"; import { workspaces } from "../../src/db/schema"; import { cloudFlows } from "../helpers/cloud-fakes"; import { createProjectScenario } from "../helpers/scenarios"; @@ -45,6 +46,24 @@ function installDirtyPostCheckoutHook(repoPath: string): void { chmodSync(hookPath, 0o755); } +async function removeWorktree(git: SimpleGit, worktreePath: string) { + await git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch(() => {}); + rmSync(worktreePath, { recursive: true, force: true }); +} + +function getWorkspaceRow( + scenario: Awaited>, + branch: string, +) { + return scenario.host.db + .select() + .from(workspaces) + .where(eq(workspaces.branch, branch)) + .get(); +} + describe("workspaces.create PR checkout integration", () => { let dispose: (() => Promise) | undefined; @@ -90,14 +109,13 @@ describe("workspaces.create PR checkout integration", () => { }, }); const bare = await createBareRemote(); + const forkBare = await createBareRemote(); let worktreePath: string | undefined; dispose = async () => { if (worktreePath) { - await scenario.repo.git - .raw(["worktree", "remove", "--force", worktreePath]) - .catch(() => {}); - rmSync(worktreePath, { recursive: true, force: true }); + await removeWorktree(scenario.repo.git, worktreePath); } + forkBare.dispose(); bare.dispose(); await scenario.dispose(); }; @@ -106,12 +124,22 @@ describe("workspaces.create PR checkout integration", () => { "package-lock.json": "main lockfile\n", }); await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.raw([ + "config", + `url.${forkBare.bareRepoPath}.insteadOf`, + "https://github.com/Contributor/hello-fork.git", + ]); await scenario.repo.git.push("origin", "main", ["--set-upstream"]); await scenario.repo.git.checkoutBranch("feature/pr-lockfile", "main"); prHeadOid = await scenario.repo.commit("PR lockfile", { "package-lock.json": "pr lockfile\n", "feature.txt": "from the PR\n", }); + await scenario.repo.git.raw([ + "push", + forkBare.bareRepoPath, + `${prHeadOid}:refs/heads/feature/pr-lockfile`, + ]); await scenario.repo.git.raw([ "push", "origin", @@ -138,11 +166,7 @@ describe("workspaces.create PR checkout integration", () => { ), ).toBe(false); - const persisted = scenario.host.db - .select() - .from(workspaces) - .where(eq(workspaces.branch, expectedBranch)) - .get(); + const persisted = getWorkspaceRow(scenario, expectedBranch); worktreePath = persisted?.worktreePath; expect(worktreePath).toBeTruthy(); if (!worktreePath) { @@ -166,6 +190,8 @@ describe("workspaces.create PR checkout integration", () => { await scenario.repo.git.raw(["config", "remote.superset-pr-6060.push"]) ).trim(), ).toBe("HEAD:refs/heads/feature/pr-lockfile"); + const dryRunOutput = await worktreeGit.raw(["push", "--dry-run"]); + expect(typeof dryRunOutput).toBe("string"); expect(result.warnings).toEqual([]); const lockStatus = ( @@ -244,4 +270,351 @@ describe("workspaces.create PR checkout integration", () => { "did not match GitHub headRefOid", ); }); + + test("same-repo PR tracks the real head branch without synthetic fallback", async () => { + const prNumber = 8080; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Same repo PR", + headRefName: "feature/same-repo", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "octocat" }, + headRepository: { name: "hello" }, + isCrossRepository: false, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + let worktreePath: string | undefined; + dispose = async () => { + if (worktreePath) { + await removeWorktree(scenario.repo.git, worktreePath); + } + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/same-repo", "main"); + prHeadOid = await scenario.repo.commit("same repo PR head", { + "feature.txt": "same repo\n", + }); + await scenario.repo.git.push("origin", "feature/same-repo"); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/same-repo", true); + + const result = await scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "Same repo PR workspace", + pr: prNumber, + }); + + worktreePath = getWorkspaceRow(scenario, "feature/same-repo")?.worktreePath; + expect(worktreePath).toBeTruthy(); + if (!worktreePath) throw new Error("expected worktree path"); + expect(result.workspace.branch).toBe("feature/same-repo"); + expect(result.warnings).toEqual([]); + expect( + ( + await scenario.repo.git.raw([ + "config", + "branch.feature/same-repo.remote", + ]) + ).trim(), + ).toBe("origin"); + expect( + ( + await scenario.repo.git.raw([ + "config", + "branch.feature/same-repo.merge", + ]) + ).trim(), + ).toBe("refs/heads/feature/same-repo"); + expect( + (await simpleGit(worktreePath).raw(["rev-parse", "HEAD"])).trim(), + ).toBe(prHeadOid); + }); + + test("same-repo PR falls back to synthetic ref with a user-visible warning when the head branch is gone", async () => { + const prNumber = 8081; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Deleted head branch PR", + headRefName: "feature/deleted-head", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "octocat" }, + headRepository: { name: "hello" }, + isCrossRepository: false, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + let worktreePath: string | undefined; + dispose = async () => { + if (worktreePath) { + await removeWorktree(scenario.repo.git, worktreePath); + } + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/deleted-head", "main"); + prHeadOid = await scenario.repo.commit("deleted head PR", { + "feature.txt": "deleted\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/deleted-head", true); + + const result = await scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "Deleted branch PR workspace", + pr: prNumber, + }); + + worktreePath = getWorkspaceRow( + scenario, + "feature/deleted-head", + )?.worktreePath; + expect(worktreePath).toBeTruthy(); + if (!worktreePath) throw new Error("expected worktree path"); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain("was unavailable from origin"); + expect( + ( + await scenario.repo.git.raw([ + "config", + "branch.feature/deleted-head.merge", + ]) + ).trim(), + ).toBe(`refs/pull/${prNumber}/head`); + expect( + (await simpleGit(worktreePath).raw(["rev-parse", "HEAD"])).trim(), + ).toBe(prHeadOid); + }); + + test("rejects an existing local branch at the wrong commit without creating a workspace", async () => { + const prNumber = 9090; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Conflicting local branch", + headRefName: "feature/conflict", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, + isCrossRepository: true, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + dispose = async () => { + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/conflict", "main"); + prHeadOid = await scenario.repo.commit("actual PR head", { + "feature.txt": "actual\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/conflict", true); + await scenario.repo.git.checkoutBranch( + "contributor/feature/conflict", + "main", + ); + await scenario.repo.commit("wrong local branch head", { + "wrong.txt": "wrong\n", + }); + await scenario.repo.git.checkout("main"); + + const error = await scenario.host.trpc.workspaces.create + .mutate({ + projectId: scenario.projectId, + name: "Conflict PR workspace", + pr: prNumber, + }) + .catch((err: unknown) => err); + + expect(error).toBeInstanceOf(TRPCClientError); + expect((error as { data?: { code?: string } }).data?.code).toBe("CONFLICT"); + expect(getWorkspaceRow(scenario, "contributor/feature/conflict")).toBe( + undefined, + ); + }); + + test("serializes concurrent creates for the same PR and reuses the first workspace", async () => { + const prNumber = 9191; + let prHeadOid = ""; + const cloudRows = new Map< + string, + { + id: string; + projectId: string; + branch: string; + name: string; + type: "worktree"; + } + >(); + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: { + "host.ensure.mutate": () => ({ machineId: "m1" }), + "v2Workspace.create.mutate": (input: unknown) => { + const i = input as { + id?: string; + projectId: string; + branch: string; + name: string; + }; + const row = { + id: i.id ?? randomUUID(), + projectId: i.projectId, + branch: i.branch, + name: i.name, + type: "worktree" as const, + }; + cloudRows.set(row.id, row); + return row; + }, + "v2Workspace.getFromHost.query": (input: unknown) => { + const i = input as { id: string }; + return cloudRows.get(i.id) ?? null; + }, + }, + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Concurrent PR", + headRefName: "feature/concurrent", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, + isCrossRepository: true, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + const worktreePaths = new Set(); + dispose = async () => { + for (const worktreePath of worktreePaths) { + await removeWorktree(scenario.repo.git, worktreePath); + } + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/concurrent", "main"); + prHeadOid = await scenario.repo.commit("concurrent PR head", { + "feature.txt": "concurrent\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/concurrent", true); + + const [first, second] = await Promise.all([ + scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "Concurrent PR workspace", + pr: prNumber, + }), + scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "Concurrent PR workspace", + pr: prNumber, + }), + ]); + + expect(first.workspace.id).toBe(second.workspace.id); + expect([first.alreadyExists, second.alreadyExists].sort()).toEqual([ + false, + true, + ]); + const row = getWorkspaceRow(scenario, "contributor/feature/concurrent"); + expect(row).toBeTruthy(); + if (!row) throw new Error("expected concurrent workspace row"); + worktreePaths.add(row.worktreePath); + expect(existsSync(row.worktreePath)).toBe(true); + expect( + (await simpleGit(row.worktreePath).raw(["rev-parse", "HEAD"])).trim(), + ).toBe(prHeadOid); + expect( + scenario.host.apiCalls.filter( + (call) => + call.path === "v2Workspace.create.mutate" && + (call.input as { branch?: string }).branch === + "contributor/feature/concurrent", + ), + ).toHaveLength(1); + }); }); From 835660a2160b1783def3e84f6246c91c0bb76756 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 17 May 2026 11:27:31 -0700 Subject: [PATCH 6/8] address PR checkout edge cases --- .../utils/pr-branch-materialize.test.ts | 53 +++++ .../utils/pr-branch-materialize.ts | 144 +++++++++---- .../src/trpc/router/workspaces/workspaces.ts | 92 +++++--- .../workspace-create-pr.integration.test.ts | 203 ++++++++++++++++++ 4 files changed, 420 insertions(+), 72 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index ed0810f389c..9c692e5b548 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -4,6 +4,7 @@ import { deleteMaterializedPrBranchIfSafe, getSyntheticPrVerifiedRef, materializePrBranch, + normalizePrBranchTracking, } from "./pr-branch-materialize"; const EXPECTED_HEAD_OID = "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a"; @@ -170,6 +171,58 @@ describe("materializePrBranch", () => { expect(result.warning).toContain("Plain git push may require"); }); + test("normalizes fork push defaults for an existing matching branch", async () => { + const raw = mock(async (args: string[]) => { + if (args[0] === "rev-parse") { + return `${EXPECTED_HEAD_OID}\n`; + } + return ""; + }); + const git = { raw } as unknown as GitClient; + const pr = { + number: 456, + headRefName: "feature/x", + headRefOid: EXPECTED_HEAD_OID, + isCrossRepository: true, + headRepositoryOwner: "alice", + headRepositoryName: "fork", + }; + const verifiedRef = getSyntheticPrVerifiedRef(pr); + + const result = await normalizePrBranchTracking({ + git, + branch: "alice/feature/x", + remoteName: "origin", + pr, + }); + + expect(result).toMatchObject({ + createdBranch: false, + sourceKind: "synthetic-pr-ref", + startPoint: verifiedRef, + trackingRemote: "superset-pr-456", + trackingMergeRef: "refs/heads/feature/x", + }); + expect(raw).not.toHaveBeenCalledWith([ + "branch", + "--no-track", + "--", + "alice/feature/x", + verifiedRef, + ]); + expect(raw).toHaveBeenCalledWith([ + "config", + "branch.alice/feature/x.pushRemote", + "superset-pr-456", + ]); + expect(raw).toHaveBeenCalledWith([ + "config", + "--replace-all", + "remote.superset-pr-456.push", + "HEAD:refs/heads/feature/x", + ]); + }); + test("deletes a materialized branch only when it still points at the verified PR head", async () => { const raw = mock(async (args: string[]) => { if (args[0] === "rev-parse") { diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts index 952928b5f82..ee3f5d4aaf5 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -297,62 +297,99 @@ export async function deleteMaterializedPrBranchIfSafe(args: { return true; } -export async function materializePrBranch(args: { +async function resolvePrBranchSource(args: { git: GitClient; - branch: string; remoteName: string; pr: PrBranchMetadata; -}): Promise { - let source: PrBranchSource; - +}): Promise { if (args.pr.isCrossRepository) { - source = await fetchSyntheticPrBranch({ + return await fetchSyntheticPrBranch({ git: args.git, remoteName: args.remoteName, pr: args.pr, }); - } else { - try { - source = await fetchSameRepoPrBranch({ - git: args.git, - remoteName: args.remoteName, - pr: args.pr, - }); - } catch (err) { - if (!(err instanceof SameRepoBranchFetchError)) { - throw err; - } - source = await fetchSyntheticPrBranch({ - git: args.git, - remoteName: args.remoteName, - pr: args.pr, - warning: `The PR head branch "${args.pr.headRefName}" was unavailable from ${args.remoteName}, so Superset fetched ${getSyntheticPrHeadRef(args.pr.number)} instead. Original error: ${err.originalError instanceof Error ? err.originalError.message : String(err.originalError)}`, - }); - } } - const configureTracking = async (createdBranch: boolean) => { - const trackingRemote = await configurePrBranchTracking({ + try { + return await fetchSameRepoPrBranch({ git: args.git, - branch: args.branch, - remoteName: source.trackingRemote, - mergeRef: source.mergeRef, - remoteUrl: source.remoteUrl, - pushRemote: source.pushRemote, - pushRef: source.pushRef, - remoteTrackingBranch: source.remoteTrackingBranch, - remoteTrackingOid: source.remoteTrackingOid, + remoteName: args.remoteName, + pr: args.pr, }); - return { - branch: args.branch, - createdBranch, - sourceKind: source.kind, - startPoint: source.startPoint, - trackingRemote, - trackingMergeRef: source.mergeRef, - warning: source.warning, - }; + } catch (err) { + if (!(err instanceof SameRepoBranchFetchError)) { + throw err; + } + return await fetchSyntheticPrBranch({ + git: args.git, + remoteName: args.remoteName, + pr: args.pr, + warning: `The PR head branch "${args.pr.headRefName}" was unavailable from ${args.remoteName}, so Superset fetched ${getSyntheticPrHeadRef(args.pr.number)} instead. Original error: ${err.originalError instanceof Error ? err.originalError.message : String(err.originalError)}`, + }); + } +} + +async function configureTrackingFromSource(args: { + git: GitClient; + branch: string; + source: PrBranchSource; + createdBranch: boolean; +}): Promise { + const trackingRemote = await configurePrBranchTracking({ + git: args.git, + branch: args.branch, + remoteName: args.source.trackingRemote, + mergeRef: args.source.mergeRef, + remoteUrl: args.source.remoteUrl, + pushRemote: args.source.pushRemote, + pushRef: args.source.pushRef, + remoteTrackingBranch: args.source.remoteTrackingBranch, + remoteTrackingOid: args.source.remoteTrackingOid, + }); + return { + branch: args.branch, + createdBranch: args.createdBranch, + sourceKind: args.source.kind, + startPoint: args.source.startPoint, + trackingRemote, + trackingMergeRef: args.source.mergeRef, + warning: args.source.warning, }; +} + +export async function normalizePrBranchTracking(args: { + git: GitClient; + branch: string; + remoteName: string; + pr: PrBranchMetadata; +}): Promise { + const source = await resolvePrBranchSource(args); + const existingOid = await getLocalBranchHead(args.git, args.branch); + if (existingOid === null) { + throw new PrBranchConflictError( + `Local branch "${args.branch}" no longer exists while preparing PR #${args.pr.number}`, + ); + } + if (normalizeOid(existingOid) !== normalizeOid(args.pr.headRefOid)) { + throw new PrBranchConflictError( + `Local branch "${args.branch}" exists and points at ${existingOid}, not PR head ${args.pr.headRefOid}`, + ); + } + return await configureTrackingFromSource({ + git: args.git, + branch: args.branch, + source, + createdBranch: false, + }); +} + +export async function materializePrBranch(args: { + git: GitClient; + branch: string; + remoteName: string; + pr: PrBranchMetadata; +}): Promise { + const source = await resolvePrBranchSource(args); const existingOid = await getLocalBranchHead(args.git, args.branch); if (existingOid !== null) { @@ -361,7 +398,12 @@ export async function materializePrBranch(args: { `Local branch "${args.branch}" exists and points at ${existingOid}, not PR head ${args.pr.headRefOid}`, ); } - return await configureTracking(false); + return await configureTrackingFromSource({ + git: args.git, + branch: args.branch, + source, + createdBranch: false, + }); } let branchCreated = false; @@ -374,13 +416,23 @@ export async function materializePrBranch(args: { source.startPoint, ]); branchCreated = true; - return await configureTracking(true); + return await configureTrackingFromSource({ + git: args.git, + branch: args.branch, + source, + createdBranch: true, + }); } catch (err) { if (!branchCreated) { const concurrentOid = await getLocalBranchHead(args.git, args.branch); if (concurrentOid !== null) { if (normalizeOid(concurrentOid) === normalizeOid(args.pr.headRefOid)) { - return await configureTracking(false); + return await configureTrackingFromSource({ + git: args.git, + branch: args.branch, + source, + createdBranch: false, + }); } throw new PrBranchConflictError( `Local branch "${args.branch}" exists and points at ${concurrentOid}, not PR head ${args.pr.headRefOid}`, diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index 920f71d0610..96f75531564 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -38,6 +38,7 @@ import { deleteMaterializedPrBranchIfSafe, type MaterializePrBranchResult, materializePrBranch, + normalizePrBranchTracking, PrBranchConflictError, } from "../workspace-creation/utils/pr-branch-materialize"; import { derivePrLocalBranchName } from "../workspace-creation/utils/pr-branch-name"; @@ -568,7 +569,6 @@ export const workspacesRouter = router({ let worktreePath: string; let alreadyExists = false; let workspaceRow: CloudWorkspace; - let prMetadata: PrMetadata | null = null; const warnings: string[] = []; if (input.pr !== undefined) { @@ -576,7 +576,7 @@ export const workspacesRouter = router({ `pr:${input.projectId}:${input.pr}`, ); try { - prMetadata = await fetchPrMetadata({ + const prMetadata = await fetchPrMetadata({ cwd: localProject.repoPath, prNumber: input.pr, execGh: ctx.execGh, @@ -604,6 +604,37 @@ export const workspacesRouter = router({ const existingWorktreePath = ( await listWorktreeBranches(git) ).worktreeMap.get(resolvedBranch); + const recordMaterializedWarning = ( + materialized: MaterializePrBranchResult, + ) => { + if (materialized.warning) { + console.warn(`[workspaces.create] ${materialized.warning}`); + warnings.push(materialized.warning); + } + }; + const normalizeExistingPrBranch = async () => { + try { + recordMaterializedWarning( + await normalizePrBranchTracking({ + git, + branch: resolvedBranch, + remoteName: localProject.remoteName ?? "origin", + pr: prMetadata, + }), + ); + } catch (err) { + throw new TRPCError({ + code: + err instanceof PrBranchConflictError + ? "CONFLICT" + : "INTERNAL_SERVER_ERROR", + message: + err instanceof Error + ? err.message + : "Failed to prepare existing PR branch", + }); + } + }; if (localOid !== null && !adoptLocalBranch) { const cleanupHint = existingWorktreePath @@ -616,6 +647,7 @@ export const workspacesRouter = router({ } if (adoptLocalBranch && existingWorktreePath) { + await normalizeExistingPrBranch(); worktreePath = existingWorktreePath; const result = await adoptExistingWorktree({ ctx, @@ -658,8 +690,10 @@ export const workspacesRouter = router({ ); } }; + let rollbackCreatedWorktree = rollbackWorktree; if (adoptLocalBranch) { + await normalizeExistingPrBranch(); try { await git.raw([ "worktree", @@ -679,6 +713,22 @@ export const workspacesRouter = router({ } else { let worktreeAddStarted = false; let materialized: MaterializePrBranchResult | null = null; + const rollbackPreparedPr = async () => { + await rollbackWorktree(); + if (materialized?.createdBranch) { + await deleteMaterializedPrBranchIfSafe({ + git, + branch: resolvedBranch, + expectedHeadOid: prMetadata.headRefOid, + }).catch((cleanupErr) => { + console.warn( + "[workspaces.create] failed to rollback PR branch", + { branch: resolvedBranch, err: cleanupErr }, + ); + }); + } + }; + rollbackCreatedWorktree = rollbackPreparedPr; try { materialized = await materializePrBranch({ git, @@ -686,10 +736,7 @@ export const workspacesRouter = router({ remoteName: localProject.remoteName ?? "origin", pr: prMetadata, }); - if (materialized.warning) { - console.warn(`[workspaces.create] ${materialized.warning}`); - warnings.push(materialized.warning); - } + recordMaterializedWarning(materialized); worktreeAddStarted = true; await git.raw([ "worktree", @@ -698,20 +745,8 @@ export const workspacesRouter = router({ resolvedBranch, ]); } catch (err) { - if (worktreeAddStarted) { - await rollbackWorktree(); - } - if (materialized?.createdBranch) { - await deleteMaterializedPrBranchIfSafe({ - git, - branch: resolvedBranch, - expectedHeadOid: prMetadata.headRefOid, - }).catch((cleanupErr) => { - console.warn( - "[workspaces.create] failed to rollback PR branch", - { branch: resolvedBranch, err: cleanupErr }, - ); - }); + if (worktreeAddStarted || materialized?.createdBranch) { + await rollbackPreparedPr(); } throw new TRPCError({ code: @@ -726,11 +761,16 @@ export const workspacesRouter = router({ } } - await enablePushAutoSetupRemote( - git, - worktreePath, - "[workspaces.create]", - ); + try { + await enablePushAutoSetupRemote( + git, + worktreePath, + "[workspaces.create]", + ); + } catch (err) { + await rollbackCreatedWorktree(); + throw err; + } workspaceRow = await registerCloudAndLocal({ ctx, @@ -740,7 +780,7 @@ export const workspacesRouter = router({ branch: resolvedBranch, worktreePath, taskId: input.taskId, - rollbackWorktree, + rollbackWorktree: rollbackCreatedWorktree, hostPromise, }); diff --git a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts index c63fbab6435..700243630c3 100644 --- a/packages/host-service/test/integration/workspace-create-pr.integration.test.ts +++ b/packages/host-service/test/integration/workspace-create-pr.integration.test.ts @@ -14,6 +14,7 @@ import { TRPCClientError } from "@trpc/client"; import { eq } from "drizzle-orm"; import simpleGit, { type SimpleGit } from "simple-git"; import { workspaces } from "../../src/db/schema"; +import { safeResolveWorktreePath } from "../../src/trpc/router/workspace-creation/shared/worktree-paths"; import { cloudFlows } from "../helpers/cloud-fakes"; import { createProjectScenario } from "../helpers/scenarios"; @@ -205,6 +206,208 @@ describe("workspaces.create PR checkout integration", () => { expect(lockStatus).toContain("package-lock.json"); }); + test("normalizes fork push config when adopting an existing matching local PR branch", async () => { + const prNumber = 6061; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: cloudFlows.workspaceCreateOk(), + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Adopt local fork branch", + headRefName: "feature/adopt-local", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, + isCrossRepository: true, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + const forkBare = await createBareRemote(); + let worktreePath: string | undefined; + dispose = async () => { + if (worktreePath) { + await removeWorktree(scenario.repo.git, worktreePath); + } + forkBare.dispose(); + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.raw([ + "config", + `url.${forkBare.bareRepoPath}.insteadOf`, + "https://github.com/Contributor/hello-fork.git", + ]); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch( + "contributor/feature/adopt-local", + "main", + ); + prHeadOid = await scenario.repo.commit("local branch at PR head", { + "feature.txt": "from adopted branch\n", + }); + await scenario.repo.git.raw([ + "push", + forkBare.bareRepoPath, + `${prHeadOid}:refs/heads/feature/adopt-local`, + ]); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + + const result = await scenario.host.trpc.workspaces.create.mutate({ + projectId: scenario.projectId, + name: "Adopted PR workspace", + pr: prNumber, + }); + + const expectedBranch = "contributor/feature/adopt-local"; + expect(result.workspace.branch).toBe(expectedBranch); + worktreePath = getWorkspaceRow(scenario, expectedBranch)?.worktreePath; + expect(worktreePath).toBeTruthy(); + if (!worktreePath) throw new Error("expected adopted worktree path"); + expect( + ( + await scenario.repo.git.raw([ + "config", + `branch.${expectedBranch}.remote`, + ]) + ).trim(), + ).toBe(`superset-pr-${prNumber}`); + expect( + ( + await scenario.repo.git.raw([ + "config", + `branch.${expectedBranch}.pushRemote`, + ]) + ).trim(), + ).toBe(`superset-pr-${prNumber}`); + expect( + ( + await scenario.repo.git.raw([ + "config", + `remote.superset-pr-${prNumber}.push`, + ]) + ).trim(), + ).toBe("HEAD:refs/heads/feature/adopt-local"); + expect(await simpleGit(worktreePath).raw(["push", "--dry-run"])).toEqual( + expect.any(String), + ); + }); + + test("removes a materialized PR branch when registration fails after worktree add", async () => { + const prNumber = 6062; + let prHeadOid = ""; + + const scenario = await createProjectScenario({ + hostOptions: { + apiOverrides: { + "host.ensure.mutate": () => ({ machineId: "test-machine-1" }), + "v2Workspace.create.mutate": (input: unknown) => { + const i = input as { + id?: string; + projectId: string; + branch: string; + name: string; + type?: "main"; + }; + if (i.type === "main") { + return { + id: i.id ?? randomUUID(), + projectId: i.projectId, + branch: i.branch, + name: i.name, + type: "main" as const, + }; + } + throw new Error("cloud workspace create failed"); + }, + }, + execGh: async (args) => { + if (args[0] === "pr" && args[1] === "view") { + return { + number: prNumber, + url: `https://github.com/octocat/hello/pull/${prNumber}`, + title: "Rollback PR", + headRefName: "feature/rollback", + headRefOid: prHeadOid, + baseRefName: "main", + headRepositoryOwner: { login: "Contributor" }, + headRepository: { name: "hello-fork" }, + isCrossRepository: true, + state: "OPEN", + }; + } + return {}; + }, + }, + }); + const bare = await createBareRemote(); + const expectedBranch = "contributor/feature/rollback"; + const expectedWorktreePath = safeResolveWorktreePath( + scenario.projectId, + expectedBranch, + ); + dispose = async () => { + await removeWorktree(scenario.repo.git, expectedWorktreePath); + bare.dispose(); + await scenario.dispose(); + }; + + await scenario.repo.commit("main", { "README.md": "main\n" }); + await scenario.repo.git.addRemote("origin", bare.bareRepoPath); + await scenario.repo.git.push("origin", "main", ["--set-upstream"]); + await scenario.repo.git.checkoutBranch("feature/rollback", "main"); + prHeadOid = await scenario.repo.commit("rollback PR head", { + "feature.txt": "rollback\n", + }); + await scenario.repo.git.raw([ + "push", + "origin", + `${prHeadOid}:refs/pull/${prNumber}/head`, + ]); + await scenario.repo.git.checkout("main"); + await scenario.repo.git.deleteLocalBranch("feature/rollback", true); + + const error = await scenario.host.trpc.workspaces.create + .mutate({ + projectId: scenario.projectId, + name: "Rollback PR workspace", + pr: prNumber, + }) + .catch((err: unknown) => err); + + expect(error).toBeInstanceOf(TRPCClientError); + expect(String((error as Error).message)).toContain( + "cloud workspace create failed", + ); + expect(getWorkspaceRow(scenario, expectedBranch)).toBeUndefined(); + expect(existsSync(expectedWorktreePath)).toBe(false); + const branchStillExists = await scenario.repo.git + .raw(["rev-parse", "--verify", `refs/heads/${expectedBranch}`]) + .then( + () => true, + () => false, + ); + expect(branchStillExists).toBe(false); + }); + test("reports PR head verification failures as internal errors", async () => { const prNumber = 7070; const staleHeadOid = "1111111111111111111111111111111111111111"; From 985d2b34c84511e017b586ae93a08f9148eb2502 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 17 May 2026 11:59:22 -0700 Subject: [PATCH 7/8] simplify PR branch materialization --- .../utils/pr-branch-materialize.test.ts | 20 +-- .../utils/pr-branch-materialize.ts | 18 +-- .../src/trpc/router/workspaces/workspaces.ts | 16 --- .../pr-branch-materialize.integration.test.ts | 119 ++++++++++++++++-- .../20260516-pr-worktree-checkout-flow.md | 22 ++-- 5 files changed, 142 insertions(+), 53 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index 9c692e5b548..86799e827d1 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -2,7 +2,7 @@ import { describe, expect, mock, test } from "bun:test"; import type { GitClient } from "../shared/types"; import { deleteMaterializedPrBranchIfSafe, - getSyntheticPrVerifiedRef, + getSyntheticPrFetchRef, materializePrBranch, normalizePrBranchTracking, } from "./pr-branch-materialize"; @@ -45,7 +45,7 @@ describe("materializePrBranch", () => { expect(result).toMatchObject({ createdBranch: true, sourceKind: "head-branch", - startPoint: "refs/remotes/upstream/feature/x", + startPoint: EXPECTED_HEAD_OID, trackingRemote: "upstream", trackingMergeRef: "refs/heads/feature/x", }); @@ -61,7 +61,7 @@ describe("materializePrBranch", () => { "--no-track", "--", "feature/x", - "refs/remotes/upstream/feature/x", + EXPECTED_HEAD_OID, ]); expect(raw).toHaveBeenNthCalledWith(5, [ "config", @@ -85,7 +85,7 @@ describe("materializePrBranch", () => { headRepositoryOwner: "alice", headRepositoryName: "fork", }; - const verifiedRef = getSyntheticPrVerifiedRef(pr); + const fetchRef = getSyntheticPrFetchRef(pr.number); const result = await materializePrBranch({ git, @@ -97,7 +97,7 @@ describe("materializePrBranch", () => { expect(result).toMatchObject({ createdBranch: true, sourceKind: "synthetic-pr-ref", - startPoint: verifiedRef, + startPoint: EXPECTED_HEAD_OID, trackingRemote: "superset-pr-456", trackingMergeRef: "refs/heads/feature/x", }); @@ -106,14 +106,14 @@ describe("materializePrBranch", () => { "--no-tags", "--quiet", "origin", - `+refs/pull/456/head:${verifiedRef}`, + `+refs/pull/456/head:${fetchRef}`, ]); expect(raw).toHaveBeenNthCalledWith(4, [ "branch", "--no-track", "--", "alice/feature/x", - verifiedRef, + EXPECTED_HEAD_OID, ]); expect(raw).toHaveBeenNthCalledWith(6, [ "remote", @@ -187,7 +187,7 @@ describe("materializePrBranch", () => { headRepositoryOwner: "alice", headRepositoryName: "fork", }; - const verifiedRef = getSyntheticPrVerifiedRef(pr); + const fetchRef = getSyntheticPrFetchRef(pr.number); const result = await normalizePrBranchTracking({ git, @@ -199,7 +199,7 @@ describe("materializePrBranch", () => { expect(result).toMatchObject({ createdBranch: false, sourceKind: "synthetic-pr-ref", - startPoint: verifiedRef, + startPoint: EXPECTED_HEAD_OID, trackingRemote: "superset-pr-456", trackingMergeRef: "refs/heads/feature/x", }); @@ -208,7 +208,7 @@ describe("materializePrBranch", () => { "--no-track", "--", "alice/feature/x", - verifiedRef, + fetchRef, ]); expect(raw).toHaveBeenCalledWith([ "config", diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts index ee3f5d4aaf5..d57a5ad17cb 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts @@ -55,8 +55,8 @@ export function getSyntheticPrHeadRef(prNumber: number): string { return `refs/pull/${prNumber}/head`; } -export function getSyntheticPrVerifiedRef(pr: PrBranchMetadata): string { - return `refs/superset/pr-fetch/${pr.number}/${normalizeOid(pr.headRefOid)}`; +export function getSyntheticPrFetchRef(prNumber: number): string { + return `refs/superset/pr-fetch/${prNumber}/head`; } function normalizeOid(oid: string): string { @@ -170,14 +170,14 @@ async function fetchSameRepoPrBranch(args: { err, ); } - await assertRefMatchesExpectedOid({ + const actualOid = await assertRefMatchesExpectedOid({ git: args.git, ref: remoteTrackingRef, expectedHeadOid: args.pr.headRefOid, }); return { kind: "head-branch", - startPoint: remoteTrackingRef, + startPoint: actualOid, trackingRemote: args.remoteName, mergeRef: `refs/heads/${args.pr.headRefName}`, }; @@ -190,17 +190,17 @@ async function fetchSyntheticPrBranch(args: { warning?: string; }): Promise { const syntheticRef = getSyntheticPrHeadRef(args.pr.number); - const verifiedRef = getSyntheticPrVerifiedRef(args.pr); + const fetchRef = getSyntheticPrFetchRef(args.pr.number); await args.git.raw([ "fetch", "--no-tags", "--quiet", args.remoteName, - `+${syntheticRef}:${verifiedRef}`, + `+${syntheticRef}:${fetchRef}`, ]); - await assertRefMatchesExpectedOid({ + const actualOid = await assertRefMatchesExpectedOid({ git: args.git, - ref: verifiedRef, + ref: fetchRef, expectedHeadOid: args.pr.headRefOid, }); const forkRemoteUrl = args.pr.isCrossRepository @@ -209,7 +209,7 @@ async function fetchSyntheticPrBranch(args: { const forkRemoteName = getForkRemoteName(args.pr.number); return { kind: "synthetic-pr-ref", - startPoint: verifiedRef, + startPoint: actualOid, trackingRemote: forkRemoteUrl ? forkRemoteName : args.remoteName, mergeRef: forkRemoteUrl ? `refs/heads/${args.pr.headRefName}` diff --git a/packages/host-service/src/trpc/router/workspaces/workspaces.ts b/packages/host-service/src/trpc/router/workspaces/workspaces.ts index 96f75531564..5614a4c6e08 100644 --- a/packages/host-service/src/trpc/router/workspaces/workspaces.ts +++ b/packages/host-service/src/trpc/router/workspaces/workspaces.ts @@ -663,11 +663,6 @@ export const workspacesRouter = router({ }); workspaceRow = result.workspace; alreadyExists = result.alreadyExists; - await enablePushAutoSetupRemote( - git, - worktreePath, - "[workspaces.create]", - ); } else { worktreePath = safeResolveWorktreePath( localProject.id, @@ -761,17 +756,6 @@ export const workspacesRouter = router({ } } - try { - await enablePushAutoSetupRemote( - git, - worktreePath, - "[workspaces.create]", - ); - } catch (err) { - await rollbackCreatedWorktree(); - throw err; - } - workspaceRow = await registerCloudAndLocal({ ctx, id: input.id, diff --git a/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts index 511f7d3c227..59bd7e74a08 100644 --- a/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts +++ b/packages/host-service/test/integration/pr-branch-materialize.integration.test.ts @@ -11,7 +11,7 @@ import { join } from "node:path"; import simpleGit, { type SimpleGit } from "simple-git"; import { deleteMaterializedPrBranchIfSafe, - getSyntheticPrVerifiedRef, + getSyntheticPrFetchRef, materializePrBranch, } from "../../src/trpc/router/workspace-creation/utils/pr-branch-materialize"; import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; @@ -110,14 +110,7 @@ describe("materializePrBranch (real git)", () => { }, }); expect(materialized.sourceKind).toBe("synthetic-pr-ref"); - expect(materialized.startPoint).toBe( - getSyntheticPrVerifiedRef({ - number: 5252, - headRefName: "feature/pr-lockfile", - headRefOid: scenario.prHeadOid, - isCrossRepository: true, - }), - ); + expect(materialized.startPoint).toBe(scenario.prHeadOid); const oldFlowPath = realpathSync( mkdtempSync(join(tmpdir(), "host-service-old-pr-worktree-")), @@ -184,6 +177,114 @@ describe("materializePrBranch (real git)", () => { } }); + test("synthetic PR fetch uses a stable ref while branches start from verified OIDs", async () => { + const prMetadata = { + number: 5252, + headRefName: "feature/pr-lockfile", + headRefOid: scenario.prHeadOid, + isCrossRepository: true, + }; + const stableRef = getSyntheticPrFetchRef(prMetadata.number); + + const first = await materializePrBranch({ + git: scenario.local.git, + branch: "contributor/stable-pr-lockfile-one", + remoteName: "origin", + pr: prMetadata, + }); + + expect(stableRef).toBe("refs/superset/pr-fetch/5252/head"); + expect(first.startPoint).toBe(scenario.prHeadOid); + expect( + ( + await scenario.local.git.raw([ + "rev-parse", + "--verify", + "refs/heads/contributor/stable-pr-lockfile-one", + ]) + ).trim(), + ).toBe(scenario.prHeadOid); + expect( + ( + await scenario.local.git.raw([ + "for-each-ref", + "--format=%(refname)", + "refs/superset/pr-fetch/5252", + ]) + ) + .trim() + .split("\n") + .filter(Boolean), + ).toEqual([stableRef]); + + await scenario.local.git.checkoutBranch( + "feature/pr-lockfile-force-push", + "main", + ); + const nextPrHeadOid = await scenario.local.commit("force-pushed PR head", { + "package-lock.json": "force pushed lockfile\n", + "feature.txt": "from the updated PR\n", + }); + await scenario.local.git.raw([ + "push", + "--force", + "origin", + `${nextPrHeadOid}:refs/pull/5252/head`, + ]); + await scenario.local.git.checkout("main"); + await scenario.local.git.deleteLocalBranch( + "feature/pr-lockfile-force-push", + true, + ); + + const second = await materializePrBranch({ + git: scenario.local.git, + branch: "contributor/stable-pr-lockfile-two", + remoteName: "origin", + pr: { + ...prMetadata, + headRefOid: nextPrHeadOid, + }, + }); + + expect(second.startPoint).toBe(nextPrHeadOid); + expect( + ( + await scenario.local.git.raw([ + "for-each-ref", + "--format=%(refname)", + "refs/superset/pr-fetch/5252", + ]) + ) + .trim() + .split("\n") + .filter(Boolean), + ).toEqual([stableRef]); + expect( + ( + await scenario.local.git.raw(["rev-parse", "--verify", stableRef]) + ).trim(), + ).toBe(nextPrHeadOid); + expect( + ( + await scenario.local.git.raw([ + "rev-parse", + "--verify", + "refs/heads/contributor/stable-pr-lockfile-one", + ]) + ).trim(), + ).toBe(scenario.prHeadOid); + expect( + ( + await scenario.local.git.raw([ + "rev-parse", + "--verify", + "refs/heads/contributor/stable-pr-lockfile-two", + ]) + ).trim(), + ).toBe(nextPrHeadOid); + }); + test("safe cleanup deletes only branches still at the verified PR head", async () => { await materializePrBranch({ git: scenario.local.git, diff --git a/plans/done/20260516-pr-worktree-checkout-flow.md b/plans/done/20260516-pr-worktree-checkout-flow.md index 28d9fd9edd5..a6f63be6485 100644 --- a/plans/done/20260516-pr-worktree-checkout-flow.md +++ b/plans/done/20260516-pr-worktree-checkout-flow.md @@ -221,8 +221,8 @@ For same-repo PRs: ``` 3. Verify `refs/remotes//` equals `headRefOid`. -4. Create or update the local branch to that verified commit if it does not - already exist. +4. Create the local branch from the verified commit OID if it does not already + exist. 5. Configure upstream to `/`. For cross-repo PRs or deleted head branches: @@ -230,13 +230,14 @@ For cross-repo PRs or deleted head branches: 1. Fetch GitHub's synthetic PR head ref from the base remote: ```text - git fetch --no-tags --quiet +refs/pull//head:refs/superset/pr-fetch// + git fetch --no-tags --quiet +refs/pull//head:refs/superset/pr-fetch//head ``` -2. Verify `refs/superset/pr-fetch//^{commit}` equals +2. Verify `refs/superset/pr-fetch//head^{commit}` equals `headRefOid`. This avoids `FETCH_HEAD`, which is shared across concurrent fetches in the same clone. -3. Create the local branch from the verified named ref. +3. Create the local branch from the verified commit OID, not the mutable + internal ref. 4. Configure: - `branch..remote = ` - `branch..merge = refs/pull//head` for synthetic-ref fallback, @@ -299,8 +300,9 @@ Then update only the new-branch PR path in 1. Replace `git worktree add --detach` plus `gh pr checkout` with `materializePrBranch(...)`. 2. Run `git worktree add `. -3. Keep existing workspace registration, base-branch config, `push.autoSetupRemote`, - and rollback behavior. +3. Keep existing workspace registration, base-branch config, and rollback + behavior. PR workspaces should rely on explicit branch tracking/push config + instead of `push.autoSetupRemote`. 4. Remove or narrow `recoverPrCheckoutAfterGhFailure` usage. The useful synthetic-ref fetch and OID verification logic should move into the normal materialization path. @@ -349,6 +351,10 @@ Implemented for the canonical v2 host-service path: mock GitHub CLI behavior. - Cross-repo PRs with `headRepository` metadata get a Superset-managed fork remote plus a push refspec so plain `git push` targets the contributor branch. +- Same-repo and synthetic PR fetches create branches from the verified commit + OID instead of from mutable refs. Synthetic fetches reuse + `refs/superset/pr-fetch//head` instead of accumulating one ref per + force-pushed OID. - Materialization warnings are returned from `workspaces.create` and surfaced by the desktop workspace-create flow. - Regression coverage now includes command-level helper tests, real-git @@ -358,8 +364,6 @@ Implemented for the canonical v2 host-service path: Remaining follow-ups: - Decide whether to backport the same shape to legacy v1 desktop code. -- Add a small prune/sweep for stale `refs/superset/pr-fetch/*` verified refs - from old PR force-pushes. - Add richer fork permission handling (`maintainerCanModify`, deleted forks, inaccessible forks) if we need exact GitHub CLI parity for every fork push edge case. From 457d49690e1cdd734f61726b25c1fc6363fa89a0 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sun, 17 May 2026 12:08:30 -0700 Subject: [PATCH 8/8] tighten PR materialization test assertion --- .../workspace-creation/utils/pr-branch-materialize.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts index 86799e827d1..cfd81f57de9 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts @@ -187,7 +187,6 @@ describe("materializePrBranch", () => { headRepositoryOwner: "alice", headRepositoryName: "fork", }; - const fetchRef = getSyntheticPrFetchRef(pr.number); const result = await normalizePrBranchTracking({ git, @@ -208,7 +207,7 @@ describe("materializePrBranch", () => { "--no-track", "--", "alice/feature/x", - fetchRef, + EXPECTED_HEAD_OID, ]); expect(raw).toHaveBeenCalledWith([ "config",