diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts index 9d51cc26f84..fd880a4ab2e 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts @@ -192,9 +192,12 @@ describe("buildPrCheckoutPayload", () => { url: "https://github.com/o/r/pull/42", title: "Fix typo", branch: "fix/typo", + headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseBranch: "main", headRepositoryOwner: "kietho", + headRepositoryName: "r", isCrossRepository: true, + isDraft: false, state: "open", body: "body text", }; @@ -217,9 +220,12 @@ describe("buildPrCheckoutPayload", () => { url: "https://github.com/o/r/pull/42", title: "Fix typo", headRefName: "fix/typo", + headRefOid: "c4ecea7dec8c6d09cf54fe0ad2f9edb8a24fd45a", baseRefName: "main", headRepositoryOwner: "kietho", + headRepositoryName: "r", isCrossRepository: true, + isDraft: false, state: "open", }); expect(payload.branch).toBeUndefined(); @@ -285,15 +291,15 @@ describe("buildPrCheckoutPayload", () => { expect(payload.pr?.state).toBe("open"); }); - test("throws clear error for cross-repo PR with deleted fork (null owner)", () => { + test("allows cross-repo PR with deleted fork so host-service can recover from refs/pull", () => { const pending = makePending({ intent: "pr-checkout" }); - expect(() => - buildPrCheckoutPayload("pid", pending, { - ...prContent, - headRepositoryOwner: null, - isCrossRepository: true, - }), - ).toThrow("head fork repository has been deleted"); + const payload = buildPrCheckoutPayload("pid", pending, { + ...prContent, + headRepositoryOwner: null, + isCrossRepository: true, + }); + expect(payload.pr?.headRepositoryOwner).toBe(""); + expect(payload.pr?.isCrossRepository).toBe(true); }); test("same-repo PR with null owner is fine (owner not needed)", () => { diff --git a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts index 81c2f57df71..d38fe856543 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts @@ -97,22 +97,15 @@ export function buildPrCheckoutPayload( url: string; title: string; branch: string; // headRefName + headRefOid: string; baseBranch: string; // baseRefName headRepositoryOwner: string | null; + headRepositoryName?: string | null; isCrossRepository: boolean; + isDraft?: boolean; state: string; }, ): CheckoutWorkspaceInput { - // Null owner on a cross-repo PR means the head fork repo has been - // deleted. We can't derive `/` without it, and - // `gh pr checkout` wouldn't have a fork to configure push against. - // Fail early with a clear error rather than a cryptic server-side - // "headRepositoryOwner is required". - if (prContent.isCrossRepository && !prContent.headRepositoryOwner) { - throw new Error( - `Cannot check out PR #${prContent.number}: the head fork repository has been deleted.`, - ); - } const linked = mapLinkedContextFromPending(pending); const normalizedState: "open" | "closed" | "merged" = prContent.state === "closed" @@ -130,11 +123,14 @@ export function buildPrCheckoutPayload( url: prContent.url, title: prContent.title, headRefName: prContent.branch, + headRefOid: prContent.headRefOid, baseRefName: prContent.baseBranch, // Same-repo PRs don't need an owner for branch derivation; pass an // empty string rather than leaking null into the server input. headRepositoryOwner: prContent.headRepositoryOwner ?? "", + headRepositoryName: prContent.headRepositoryName ?? null, isCrossRepository: prContent.isCrossRepository, + isDraft: prContent.isDraft ?? false, state: normalizedState, }, composer: { diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts index f0718e1f921..60cf63f1ffe 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts @@ -20,9 +20,12 @@ export interface CheckoutWorkspaceInput { url: string; title: string; headRefName: string; + headRefOid: string; baseRefName: string; headRepositoryOwner: string; + headRepositoryName?: string | null; isCrossRepository: boolean; + isDraft?: boolean; state: "open" | "closed" | "merged"; }; composer: { diff --git a/packages/host-service/src/runtime/pull-requests/index.ts b/packages/host-service/src/runtime/pull-requests/index.ts index 4ea1fd133a5..648909b52cb 100644 --- a/packages/host-service/src/runtime/pull-requests/index.ts +++ b/packages/host-service/src/runtime/pull-requests/index.ts @@ -1,4 +1,5 @@ export { + type CheckoutPullRequestMetadata, PullRequestRuntimeManager, type PullRequestRuntimeManagerOptions, type PullRequestStateSnapshot, diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts new file mode 100644 index 00000000000..e74050c70e0 --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -0,0 +1,241 @@ +import { describe, expect, test } from "bun:test"; +import { pullRequests, workspaces } from "../../db/schema"; +import { PullRequestRuntimeManager } from "./pull-requests"; + +const PROJECT_ID = "project-1"; +const WORKSPACE_ID = "workspace-1"; + +interface FakeProject { + id: string; + repoPath: string; + repoProvider: "github"; + repoOwner: string; + repoName: string; + repoUrl: string; + remoteName: string; +} + +interface FakeWorkspace { + id: string; + projectId: string; + worktreePath: string; + branch: string; + headSha: string | null; + upstreamOwner: string | null; + upstreamRepo: string | null; + upstreamBranch: string | null; + pullRequestId: string | null; +} + +interface FakePullRequest { + id: string; + projectId: string; + repoProvider: "github"; + repoOwner: string; + repoName: string; + prNumber: number; + url: string; + title: string; + state: string; + isDraft: boolean; + headBranch: string; + headSha: string; + reviewDecision: string | null; + checksStatus: string; + checksJson: string; + lastFetchedAt: number | null; + error: string | null; + createdAt: number; + updatedAt: number; +} + +interface FakeState { + project: FakeProject; + workspace: FakeWorkspace; + pullRequest: FakePullRequest | undefined; +} + +function makeState(branch: string): FakeState { + return { + project: { + id: PROJECT_ID, + repoPath: "/repo", + repoProvider: "github", + repoOwner: "base-owner", + repoName: "base-repo", + repoUrl: "https://github.com/base-owner/base-repo.git", + remoteName: "origin", + }, + workspace: { + id: WORKSPACE_ID, + projectId: PROJECT_ID, + worktreePath: `/repo/.worktrees/${branch}`, + branch, + headSha: null, + upstreamOwner: null, + upstreamRepo: null, + upstreamBranch: null, + pullRequestId: null, + }, + pullRequest: undefined, + }; +} + +function createFakeDb(state: FakeState) { + return { + query: { + projects: { + findFirst: () => ({ sync: () => state.project }), + }, + pullRequests: { + findFirst: () => ({ sync: () => state.pullRequest }), + }, + }, + insert: (table: unknown) => ({ + values: (values: FakePullRequest) => ({ + run: () => { + if (table === pullRequests) { + state.pullRequest = values; + } + }, + }), + }), + update: (table: unknown) => ({ + set: (values: Partial | Partial) => ({ + where: () => ({ + run: () => { + if (table === workspaces) { + state.workspace = { + ...state.workspace, + ...(values as Partial), + }; + } + if (table === pullRequests && state.pullRequest) { + state.pullRequest = { + ...state.pullRequest, + ...(values as Partial), + }; + } + }, + }), + }), + }), + select: (shape?: unknown) => ({ + from: (table: unknown) => ({ + where: () => ({ + all: () => { + if (table !== workspaces) return []; + if (shape) return [{ projectId: state.workspace.projectId }]; + return [state.workspace]; + }, + }), + all: () => { + if (table !== workspaces) return []; + if (shape) return [{ projectId: state.workspace.projectId }]; + return [state.workspace]; + }, + }), + }), + }; +} + +function createManager(state: FakeState) { + return new PullRequestRuntimeManager({ + db: createFakeDb(state) as never, + git: async () => { + throw new Error("git should not be used when project metadata is set"); + }, + github: async () => { + throw new Error("github should not be used for direct PR linking"); + }, + }); +} + +describe("PullRequestRuntimeManager direct checkout PR linking", () => { + test("links a fork PR workspace to the selected PR and records fork upstream", async () => { + const state = makeState("fork-owner/fix-typo"); + const manager = createManager(state); + + const prId = await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Fix typo", + state: "open", + isDraft: false, + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: "fork-owner", + headRepositoryName: "fork-repo", + isCrossRepository: true, + }, + }); + + expect(state.workspace.pullRequestId).toBe(prId); + expect(state.workspace.upstreamOwner).toBe("fork-owner"); + expect(state.workspace.upstreamRepo).toBe("fork-repo"); + expect(state.workspace.upstreamBranch).toBe("fix-typo"); + expect(state.pullRequest?.prNumber).toBe(42); + expect(state.pullRequest?.repoOwner).toBe("base-owner"); + expect(state.pullRequest?.repoName).toBe("base-repo"); + expect(state.pullRequest?.headBranch).toBe("fix-typo"); + }); + + test("keeps a deleted-fork PR link when no upstream can be recorded", async () => { + const state = makeState("pr/42"); + const manager = createManager(state); + + const prId = await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Deleted fork", + state: "merged", + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: null, + headRepositoryName: null, + isCrossRepository: true, + }, + }); + + expect(state.workspace.pullRequestId).toBe(prId); + expect(state.workspace.upstreamOwner).toBeNull(); + expect(state.workspace.upstreamRepo).toBeNull(); + expect(state.workspace.upstreamBranch).toBeNull(); + + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + + expect(state.workspace.pullRequestId).toBe(prId); + }); + + test("clears a no-upstream PR link when workspace HEAD no longer matches the PR", async () => { + const state = makeState("pr/42"); + const manager = createManager(state); + + await manager.linkWorkspaceToCheckoutPullRequest({ + workspaceId: WORKSPACE_ID, + projectId: PROJECT_ID, + pullRequest: { + number: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Deleted fork", + state: "merged", + headRefName: "fix-typo", + headRefOid: "abc123", + headRepositoryOwner: null, + headRepositoryName: null, + isCrossRepository: true, + }, + }); + state.workspace.headSha = "def456"; + + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + + expect(state.workspace.pullRequestId).toBeNull(); + }); +}); diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index cb279e6b371..728227f845e 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -197,6 +197,45 @@ interface NormalizedRepoIdentity { remoteName: string; } +type PullRequestRow = typeof pullRequests.$inferSelect; + +export interface CheckoutPullRequestMetadata { + number: number; + url: string; + title: string; + state: "open" | "closed" | "merged"; + isDraft?: boolean; + headRefName: string; + headRefOid: string; + headRepositoryOwner?: string | null; + headRepositoryName?: string | null; + isCrossRepository: boolean; +} + +function mapCheckoutPullRequestState( + state: CheckoutPullRequestMetadata["state"], + isDraft: boolean, +): PullRequestState { + if (state === "merged") return "merged"; + if (state === "closed") return "closed"; + if (isDraft) return "draft"; + return "open"; +} + +function deriveCheckoutPullRequestUpstream( + repo: NormalizedRepoIdentity, + pr: CheckoutPullRequestMetadata, +): { owner: string; name: string; branch: string } | null { + if (!pr.isCrossRepository) { + return { owner: repo.owner, name: repo.name, branch: pr.headRefName }; + } + + const owner = pr.headRepositoryOwner?.trim(); + const name = pr.headRepositoryName?.trim(); + if (!owner || !name) return null; + return { owner, name, branch: pr.headRefName }; +} + export class PullRequestRuntimeManager { private readonly db: HostDb; private readonly git: GitFactory; @@ -303,6 +342,63 @@ export class PullRequestRuntimeManager { ); } + async linkWorkspaceToCheckoutPullRequest({ + workspaceId, + projectId, + pullRequest, + }: { + workspaceId: string; + projectId: string; + pullRequest: CheckoutPullRequestMetadata; + }): Promise { + const repo = await this.getProjectRepository(projectId); + if (!repo) { + console.warn( + "[host-service:pull-request-runtime] linkWorkspaceToCheckoutPullRequest: skipping; project repo metadata unavailable", + { projectId, workspaceId, prNumber: pullRequest.number }, + ); + return null; + } + + const existing = this.findPullRequestRow(repo, pullRequest.number); + const existingChecks = parseChecksJson(existing?.checksJson ?? null); + const now = Date.now(); + const isDraft = pullRequest.isDraft ?? false; + const rowId = this.upsertPullRequestRow({ + existing, + projectId, + repo, + prNumber: pullRequest.number, + url: pullRequest.url, + title: pullRequest.title, + state: mapCheckoutPullRequestState(pullRequest.state, isDraft), + isDraft, + headBranch: pullRequest.headRefName, + headSha: pullRequest.headRefOid, + reviewDecision: coerceReviewDecision(existing?.reviewDecision ?? null), + checksStatus: coerceChecksStatus(existing?.checksStatus ?? null), + checksJson: JSON.stringify(existingChecks), + lastFetchedAt: existing?.lastFetchedAt ?? now, + error: null, + now, + }); + + const upstream = deriveCheckoutPullRequestUpstream(repo, pullRequest); + this.db + .update(workspaces) + .set({ + pullRequestId: rowId, + headSha: pullRequest.headRefOid, + upstreamOwner: upstream?.owner ?? null, + upstreamRepo: upstream?.name ?? null, + upstreamBranch: upstream?.branch ?? null, + }) + .where(eq(workspaces.id, workspaceId)) + .run(); + + return rowId; + } + private async syncWorkspaceBranches(): Promise { const allWorkspaces = this.db.select().from(workspaces).all(); const changedProjectIds = new Set(); @@ -319,13 +415,19 @@ export class PullRequestRuntimeManager { const upstreamOwner = upstream?.owner ?? null; const upstreamRepo = upstream?.name ?? null; const upstreamBranch = upstream?.branch ?? null; + const pullRequestId = + upstream || + this.pullRequestHeadMatches(workspace.pullRequestId, headSha) + ? workspace.pullRequestId + : null; if ( branch === workspace.branch && headSha === workspace.headSha && upstreamOwner === workspace.upstreamOwner && upstreamRepo === workspace.upstreamRepo && - upstreamBranch === workspace.upstreamBranch + upstreamBranch === workspace.upstreamBranch && + pullRequestId === workspace.pullRequestId ) { continue; } @@ -338,6 +440,7 @@ export class PullRequestRuntimeManager { upstreamOwner, upstreamRepo, upstreamBranch, + pullRequestId, }) .where(eq(workspaces.id, workspace.id)) .run(); @@ -442,7 +545,28 @@ export class PullRequestRuntimeManager { workspace.upstreamRepo, workspace.upstreamBranch ?? workspace.branch, ); - const match = key ? keyToPullRequest.get(key) : undefined; + if (!key) { + // PR checkouts recovered from GitHub's archived refs intentionally + // have no upstream. Keep the explicit PR link only while the + // workspace HEAD still matches the selected PR head. + if ( + this.pullRequestHeadMatches( + workspace.pullRequestId, + workspace.headSha, + ) + ) { + continue; + } + if (workspace.pullRequestId) { + this.db + .update(workspaces) + .set({ pullRequestId: null }) + .where(eq(workspaces.id, workspace.id)) + .run(); + } + continue; + } + const match = keyToPullRequest.get(key); this.db .update(workspaces) .set({ pullRequestId: match?.id ?? null }) @@ -509,6 +633,113 @@ export class PullRequestRuntimeManager { }; } + private findPullRequestRow( + repo: NormalizedRepoIdentity, + prNumber: number, + ): PullRequestRow | undefined { + return this.db.query.pullRequests + .findFirst({ + where: and( + eq(pullRequests.repoProvider, repo.provider), + eq(pullRequests.repoOwner, repo.owner), + eq(pullRequests.repoName, repo.name), + eq(pullRequests.prNumber, prNumber), + ), + }) + .sync(); + } + + private findPullRequestRowById(id: string): PullRequestRow | undefined { + return this.db.query.pullRequests + .findFirst({ where: eq(pullRequests.id, id) }) + .sync(); + } + + private pullRequestHeadMatches( + pullRequestId: string | null, + headSha: string | null, + ): boolean { + if (!pullRequestId || !headSha) return false; + const pr = this.findPullRequestRowById(pullRequestId); + return pr?.headSha.toLowerCase() === headSha.trim().toLowerCase(); + } + + private upsertPullRequestRow({ + existing, + projectId, + repo, + prNumber, + url, + title, + state, + isDraft, + headBranch, + headSha, + reviewDecision, + checksStatus, + checksJson, + lastFetchedAt, + error, + now, + }: { + existing: PullRequestRow | undefined; + projectId: string; + repo: NormalizedRepoIdentity; + prNumber: number; + url: string; + title: string; + state: PullRequestState; + isDraft: boolean; + headBranch: string; + headSha: string; + reviewDecision: ReviewDecision; + checksStatus: ChecksStatus; + checksJson: string; + lastFetchedAt: number | null; + error: string | null; + now: number; + }): string { + const rowId = existing?.id ?? randomUUID(); + const data = { + projectId, + repoProvider: repo.provider, + repoOwner: repo.owner, + repoName: repo.name, + prNumber, + url, + title, + state, + isDraft, + headBranch, + headSha, + reviewDecision, + checksStatus, + checksJson, + lastFetchedAt, + error, + updatedAt: now, + }; + + if (existing) { + this.db + .update(pullRequests) + .set(data) + .where(eq(pullRequests.id, rowId)) + .run(); + } else { + this.db + .insert(pullRequests) + .values({ + id: rowId, + createdAt: now, + ...data, + }) + .run(); + } + + return rowId; + } + private async getCachedRepoPullRequests( repo: NormalizedRepoIdentity, options: { bypassCache?: boolean } = {}, @@ -578,27 +809,15 @@ export class PullRequestRuntimeManager { const now = Date.now(); for (const [key, node] of latestByKey) { - const existing = this.db.query.pullRequests - .findFirst({ - where: and( - eq(pullRequests.repoProvider, repo.provider), - eq(pullRequests.repoOwner, repo.owner), - eq(pullRequests.repoName, repo.name), - eq(pullRequests.prNumber, node.number), - ), - }) - .sync(); - - const rowId = existing?.id ?? randomUUID(); + const existing = this.findPullRequestRow(repo, node.number); const checks = parseCheckContexts( node.statusCheckRollup?.contexts?.nodes ?? [], ); - const data = { + const rowId = this.upsertPullRequestRow({ + existing, projectId, - repoProvider: repo.provider, - repoOwner: repo.owner, - repoName: repo.name, prNumber: node.number, + repo, url: node.url, title: node.title, state: mapPullRequestState(node.state, node.isDraft), @@ -610,25 +829,8 @@ export class PullRequestRuntimeManager { checksJson: JSON.stringify(checks), lastFetchedAt: now, error: null, - updatedAt: now, - }; - - if (existing) { - this.db - .update(pullRequests) - .set(data) - .where(eq(pullRequests.id, rowId)) - .run(); - } else { - this.db - .insert(pullRequests) - .values({ - id: rowId, - createdAt: now, - ...data, - }) - .run(); - } + now, + }); keyToRow.set(key, { id: rowId }); } diff --git a/packages/host-service/src/terminal/daemon-client-singleton.ts b/packages/host-service/src/terminal/daemon-client-singleton.ts index d169d993619..92d8ee6adfd 100644 --- a/packages/host-service/src/terminal/daemon-client-singleton.ts +++ b/packages/host-service/src/terminal/daemon-client-singleton.ts @@ -10,9 +10,22 @@ // failure model. import { getSupervisor, waitForDaemonReady } from "../daemon/index.ts"; -import { env } from "../env.ts"; import { DaemonClient } from "./DaemonClient/index.ts"; +// Read org id directly from process.env rather than importing the validated +// `env` module — this singleton is eagerly loaded by the trpc terminal +// router, so importing `env` here makes every test that boots the router +// crash at import time when the production env vars aren't set. +function getOrganizationId(): string { + const id = process.env.ORGANIZATION_ID; + if (!id) { + throw new Error( + "ORGANIZATION_ID is not set; pty-daemon cannot be addressed.", + ); + } + return id; +} + let cached: DaemonClient | null = null; let connecting: Promise | null = null; @@ -38,8 +51,8 @@ async function ptyDaemonSocketPath(): Promise { const testOverride = process.env.SUPERSET_PTY_DAEMON_SOCKET; if (testOverride) return testOverride; - await waitForDaemonReady(env.ORGANIZATION_ID); - const sockPath = getSupervisor().getSocketPath(env.ORGANIZATION_ID); + await waitForDaemonReady(getOrganizationId()); + const sockPath = getSupervisor().getSocketPath(getOrganizationId()); if (!sockPath) { throw new Error( "pty-daemon is not available: supervisor returned no socket path. " + diff --git a/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts b/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts index 87bfa337240..1420ee7bc3c 100644 --- a/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts +++ b/packages/host-service/src/trpc/router/terminal/terminal.daemon.test.ts @@ -24,10 +24,13 @@ process.env.SUPERSET_API_URL = "https://cloud.example.com"; const { appRouter } = await import("../router.ts"); +const TEST_ORG_ID = "00000000-0000-4000-8000-000000000000"; + function makeCaller(authenticated = true) { // Cast to whatever; we only invoke procedures that don't touch db/git/etc. return appRouter.createCaller({ isAuthenticated: authenticated, + organizationId: TEST_ORG_ID, } as unknown as Parameters[0]); } diff --git a/packages/host-service/src/trpc/router/terminal/terminal.ts b/packages/host-service/src/trpc/router/terminal/terminal.ts index dd97a8c840e..216b8b7ae17 100644 --- a/packages/host-service/src/trpc/router/terminal/terminal.ts +++ b/packages/host-service/src/trpc/router/terminal/terminal.ts @@ -3,7 +3,6 @@ import { eq } from "drizzle-orm"; import { z } from "zod"; import { getSupervisor, waitForDaemonReady } from "../../../daemon"; import { terminalSessions, workspaces } from "../../../db/schema"; -import { env } from "../../../env"; import { createTerminalSessionInternal, disposeSession, @@ -13,23 +12,25 @@ import { import { protectedProcedure, router } from "../../index"; // Daemon control surface — sibling to the per-workspace terminal ops above. -// Org-scoped (one daemon per host-service); reads org id from env. +// Org-scoped (one daemon per host-service); org id comes from request ctx +// rather than env so this module can be imported in tests where env vars +// aren't set. // Supervisor lives in this same process so calls go through the in-process // singleton, not over the wire. const daemonRouter = router({ - getUpdateStatus: protectedProcedure.query(() => - getSupervisor().getUpdateStatus(env.ORGANIZATION_ID), + getUpdateStatus: protectedProcedure.query(({ ctx }) => + getSupervisor().getUpdateStatus(ctx.organizationId), ), - listSessions: protectedProcedure.query(async () => { + listSessions: protectedProcedure.query(async ({ ctx }) => { // Wait for the bootstrap so the supervisor has a socket path. - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().listSessions(env.ORGANIZATION_ID); + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().listSessions(ctx.organizationId); }), - restart: protectedProcedure.mutation(async () => { - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().restart(env.ORGANIZATION_ID); + restart: protectedProcedure.mutation(async ({ ctx }) => { + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().restart(ctx.organizationId); }), /** @@ -40,9 +41,9 @@ const daemonRouter = router({ * "Update" path (vs `restart` which kills sessions). On failure, the * UI offers force-restart as a fallback. */ - update: protectedProcedure.mutation(async () => { - await waitForDaemonReady(env.ORGANIZATION_ID); - return getSupervisor().update(env.ORGANIZATION_ID); + update: protectedProcedure.mutation(async ({ ctx }) => { + await waitForDaemonReady(ctx.organizationId); + return getSupervisor().update(ctx.organizationId); }), }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts index 3ca06bdc60f..c886b78b920 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts @@ -14,6 +14,10 @@ import { clearProgress, setProgress } from "../shared/progress-store"; import { safeResolveWorktreePath } from "../shared/worktree-paths"; import { execGh } from "../utils/exec-gh"; import { derivePrLocalBranchName } from "../utils/pr-branch-name"; +import { + getErrorMessage, + recoverPrCheckoutAfterGhFailure, +} from "../utils/pr-checkout-recovery"; export const checkout = protectedProcedure .input(checkoutInputSchema) @@ -45,11 +49,27 @@ export const checkout = protectedProcedure }) .sync(); if (existing) { + const warnings: string[] = []; + try { + await ctx.runtime.pullRequests.linkWorkspaceToCheckoutPullRequest({ + workspaceId: existing.id, + projectId: input.projectId, + pullRequest: input.pr, + }); + } catch (err) { + console.warn( + "[workspaceCreation.checkout] failed to link existing workspace PR metadata", + { workspaceId: existing.id, err }, + ); + warnings.push( + "Existing workspace found, but Superset could not link pull request status automatically.", + ); + } clearProgress(input.pendingId); return { workspace: { id: existing.id }, terminals: [], - warnings: [], + warnings, alreadyExists: true as const, }; } @@ -107,6 +127,7 @@ export const checkout = protectedProcedure }); } + let prCheckoutRecoveryWarning: string | null = null; try { await execGh( [ @@ -120,21 +141,57 @@ export const checkout = protectedProcedure { cwd: worktreePath, timeout: 120_000 }, ); } catch (err) { - await git - .raw(["worktree", "remove", "--force", worktreePath]) - .catch((rollbackErr) => { - console.warn( - "[workspaceCreation.checkout] failed to rollback PR worktree", - { worktreePath, err: rollbackErr }, - ); + // Distinguish recovery declined (recoveryError null, + // prCheckoutRecoveryWarning null) from recovery threw + // (recoveryError set) so the surfaced message includes the + // secondary failure only when it adds information. + let recoveryError: unknown = null; + try { + const recovery = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath, + branch, + prNumber: input.pr.number, + remoteName: localProject.remoteName ?? "origin", + expectedHeadOid: input.pr.headRefOid, + error: err, }); - clearProgress(input.pendingId); - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `gh pr checkout failed: ${ - err instanceof Error ? err.message : String(err) - }`, - }); + if (recovery.recovered) { + prCheckoutRecoveryWarning = recovery.warning; + } + } catch (e) { + recoveryError = e; + } + + if (!prCheckoutRecoveryWarning) { + await git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch((rollbackErr) => { + console.warn( + "[workspaceCreation.checkout] failed to rollback PR worktree", + { worktreePath, err: rollbackErr }, + ); + }); + clearProgress(input.pendingId); + const recoveryMessage = recoveryError + ? ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryError)}` + : ""; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, + }); + } + } + + if (prCheckoutRecoveryWarning) { + console.warn( + "[workspaceCreation.checkout] recovered failed gh pr checkout", + { + prNumber: input.pr.number, + branch, + warning: prCheckoutRecoveryWarning, + }, + ); } // Push ergonomics. `gh pr checkout` sets per-branch push config @@ -147,6 +204,9 @@ export const checkout = protectedProcedure ); const extraWarnings: string[] = []; + if (prCheckoutRecoveryWarning) { + extraWarnings.push(prCheckoutRecoveryWarning); + } if (input.pr.state !== "open") { extraWarnings.push( `PR is ${input.pr.state} — commits are included, but the PR may not merge.`, @@ -168,6 +228,7 @@ export const checkout = protectedProcedure runSetupScript: input.composer.runSetupScript ?? false, git, extraWarnings, + pullRequest: input.pr, }); } diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts index 30b16121933..82d28187849 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts @@ -14,8 +14,10 @@ type PullRequestContent = { url: string; state: string; branch: string; + headRefOid: string; baseBranch: string; headRepositoryOwner: string | null; + headRepositoryName: string | null; isCrossRepository: boolean; author: string | null; isDraft: boolean; @@ -55,7 +57,7 @@ export const getGitHubPullRequestContent = protectedProcedure "--repo", `${repo.owner}/${repo.name}`, "--json", - "number,title,body,url,state,author,headRefName,baseRefName,headRepositoryOwner,isCrossRepository,isDraft,createdAt,updatedAt", + "number,title,body,url,state,author,headRefName,headRefOid,baseRefName,headRepositoryOwner,headRepository,isCrossRepository,isDraft,createdAt,updatedAt", ]); const data = pullRequestContentSchema.parse(raw); return { @@ -65,8 +67,10 @@ export const getGitHubPullRequestContent = protectedProcedure url: data.url, state: data.state.toLowerCase(), branch: data.headRefName, + headRefOid: data.headRefOid, baseBranch: data.baseRefName, headRepositoryOwner: data.headRepositoryOwner?.login ?? null, + headRepositoryName: data.headRepository?.name ?? null, isCrossRepository: data.isCrossRepository, author: data.author?.login ?? null, isDraft: data.isDraft, diff --git a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts index 7c98376f7bf..46829e366ff 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/schemas.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/schemas.ts @@ -65,9 +65,12 @@ const checkoutPrSchema = z.object({ url: z.string().url(), title: z.string(), headRefName: z.string(), + headRefOid: z.string().min(1), baseRefName: z.string(), headRepositoryOwner: z.string(), + headRepositoryName: z.string().nullable().optional(), isCrossRepository: z.boolean(), + isDraft: z.boolean().optional(), state: z.enum(["open", "closed", "merged"]), }); @@ -145,12 +148,14 @@ export const pullRequestContentSchema = z.object({ url: z.string(), state: z.string(), headRefName: z.string(), + headRefOid: z.string().min(1), baseRefName: z.string(), // `gh pr view` returns null when the PR's head fork repository has been // deleted. Nullable so the schema parse doesn't fail; consumers decide // how to handle a missing owner (client surfaces a clear error for // cross-repo PRs — same-repo PRs shouldn't see null in practice). headRepositoryOwner: z.object({ login: z.string() }).nullable(), + headRepository: z.object({ name: z.string() }).nullable(), isCrossRepository: z.boolean(), isDraft: z.boolean(), author: z.object({ login: z.string() }).optional(), diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts index e982e2e479f..a5fed75f266 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts @@ -1,6 +1,7 @@ import { getHostId, getHostName } from "@superset/shared/host-info"; import { TRPCError } from "@trpc/server"; import { workspaces } from "../../../../db/schema"; +import type { CheckoutPullRequestMetadata } from "../../../../runtime/pull-requests"; import type { HostServiceContext } from "../../../../types"; import { clearProgress, setProgress } from "./progress-store"; import { startSetupTerminalIfPresent } from "./setup-terminal"; @@ -27,6 +28,7 @@ export async function finishCheckout( runSetupScript: boolean; git: GitClient; extraWarnings: string[]; + pullRequest?: CheckoutPullRequestMetadata; }, ): Promise { setProgress(args.pendingId, "registering"); @@ -142,6 +144,24 @@ export async function finishCheckout( const terminals: TerminalDescriptor[] = []; const warnings: string[] = [...args.extraWarnings]; + if (args.pullRequest) { + try { + await ctx.runtime.pullRequests.linkWorkspaceToCheckoutPullRequest({ + workspaceId: cloudRow.id, + projectId: args.projectId, + pullRequest: args.pullRequest, + }); + } catch (err) { + console.warn( + "[workspaceCreation.checkout] failed to link checkout PR metadata", + { workspaceId: cloudRow.id, err }, + ); + warnings.push( + "Workspace was created, but Superset could not link pull request status automatically.", + ); + } + } + if (args.runSetupScript) { const { terminal, warning } = await startSetupTerminalIfPresent({ ctx, diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts index b1547db43a5..c38e564c375 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts @@ -111,4 +111,15 @@ describe("derivePrLocalBranchName", () => { }), ).toThrow("headRepositoryOwner is required"); }); + + test("cross-repo with empty owner falls back to pr number when available", () => { + expect( + derivePrLocalBranchName({ + number: 3711, + headRefName: "foo", + headRepositoryOwner: "", + isCrossRepository: true, + }), + ).toBe("pr/3711"); + }); }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts index a6d702c91bf..87d2cc36388 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts @@ -4,10 +4,13 @@ * Same-repo PRs use the head ref as-is. Cross-repo (fork) PRs get the * fork owner as a lowercase prefix — avoids collisions with local/upstream * branches of the same name and namespaces by author. + * If GitHub no longer reports the fork owner, fall back to `pr/` so + * the checkout can still recover from GitHub's synthetic PR ref. * * Mirrors v1 (`apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:1630`). */ export function derivePrLocalBranchName(pr: { + number?: number; headRefName: string; headRepositoryOwner: string; isCrossRepository: boolean; @@ -19,6 +22,9 @@ export function derivePrLocalBranchName(pr: { if (pr.isCrossRepository) { const owner = pr.headRepositoryOwner.trim().toLowerCase(); if (!owner) { + if (pr.number) { + return `pr/${pr.number}`; + } throw new Error( "derivePrLocalBranchName: headRepositoryOwner is required for cross-repo PRs", ); diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts new file mode 100644 index 00000000000..dc373399d85 --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts @@ -0,0 +1,232 @@ +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 new file mode 100644 index 00000000000..2534d08488e --- /dev/null +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts @@ -0,0 +1,181 @@ +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/test/integration/pr-checkout-recovery.integration.test.ts b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts new file mode 100644 index 00000000000..ed0e86410b6 --- /dev/null +++ b/packages/host-service/test/integration/pr-checkout-recovery.integration.test.ts @@ -0,0 +1,256 @@ +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); + }); +}); diff --git a/packages/host-service/test/pull-requests.test.ts b/packages/host-service/test/pull-requests.test.ts index da65d076890..164c9acb714 100644 --- a/packages/host-service/test/pull-requests.test.ts +++ b/packages/host-service/test/pull-requests.test.ts @@ -67,6 +67,7 @@ describe("PullRequestRuntimeManager branch sync", () => { upstreamOwner: null, upstreamRepo: null, upstreamBranch: null, + pullRequestId: null, }); expect(refreshProjectMock).toHaveBeenCalledWith("project-1"); });