From 268016d872e724bd996fcd585835d169db903814 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 9 Mar 2026 16:00:02 -0700 Subject: [PATCH] Fix PR thread upstream tracking for fork branches - Restore/set upstream tracking when preparing local and reused PR worktrees - Derive fork `owner/repo` from PR URL when GitHub omits `headRepositoryNameWithOwner` - Expand GitManager tests to cover local mode, URL-derived fork identity, and reused worktree upstream recovery --- apps/server/src/git/Layers/GitManager.test.ts | 227 +++++++++++++++++- apps/server/src/git/Layers/GitManager.ts | 75 +++++- 2 files changed, 284 insertions(+), 18 deletions(-) diff --git a/apps/server/src/git/Layers/GitManager.test.ts b/apps/server/src/git/Layers/GitManager.test.ts index 0245430954d3f427b076ac45e8d2d717fad354fe..ea0097e59825bd4c9f0774204789e3dcec460e4b 100644 --- a/apps/server/src/git/Layers/GitManager.test.ts +++ b/apps/server/src/git/Layers/GitManager.test.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import path from "node:path"; +import { spawnSync } from "node:child_process"; import * as NodeServices from "@effect/platform-node/NodeServices"; import { it } from "@effect/vitest"; @@ -65,6 +66,29 @@ interface FakeGitTextGeneration { type FakePullRequest = NonNullable; +function runGitSyncForFakeGh(cwd: string, args: readonly string[]): void { + const result = spawnSync("git", args, { + cwd, + encoding: "utf8", + }); + if (result.status === 0) { + return; + } + throw new GitHubCliError({ + operation: "execute", + detail: `Failed to simulate gh checkout with git ${args.join(" ")}: ${result.stderr?.trim() || "unknown error"}`, + }); +} + +function isGitHubCliError(error: unknown): error is GitHubCliError { + return ( + typeof error === "object" && + error !== null && + "_tag" in error && + (error as { _tag?: unknown })._tag === "GitHubCliError" + ); +} + function makeTempDir( prefix: string, ): Effect.Effect { @@ -252,12 +276,42 @@ function createGitHubCliWithFakeGh(scenario: FakeGhScenario = {}): { } if (args[0] === "pr" && args[1] === "checkout") { - return Effect.succeed({ - stdout: "", - stderr: "", - code: 0, - signal: null, - timedOut: false, + return Effect.try({ + try: () => { + const headBranch = scenario.pullRequest?.headRefName; + if (headBranch) { + const existingBranch = spawnSync( + "git", + ["show-ref", "--verify", "--quiet", `refs/heads/${headBranch}`], + { + cwd: input.cwd, + encoding: "utf8", + }, + ); + if (existingBranch.status === 0) { + runGitSyncForFakeGh(input.cwd, ["checkout", headBranch]); + } else { + runGitSyncForFakeGh(input.cwd, ["checkout", "-b", headBranch]); + } + } + return { + stdout: "", + stderr: "", + code: 0, + signal: null, + timedOut: false, + }; + }, + catch: (error) => + isGitHubCliError(error) + ? error + : new GitHubCliError({ + operation: "execute", + detail: + error instanceof Error + ? `Failed to simulate gh checkout: ${error.message}` + : "Failed to simulate gh checkout.", + }), }); } @@ -1210,6 +1264,111 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("preserves fork upstream tracking when preparing a local PR thread", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + const originDir = yield* createBareRemote(); + const forkDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", originDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "main"]); + yield* runGit(repoDir, ["remote", "add", "fork-seed", forkDir]); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-local-fork"]); + fs.writeFileSync(path.join(repoDir, "local-fork.txt"), "local fork\n"); + yield* runGit(repoDir, ["add", "local-fork.txt"]); + yield* runGit(repoDir, ["commit", "-m", "Local fork PR branch"]); + yield* runGit(repoDir, ["push", "-u", "fork-seed", "feature/pr-local-fork"]); + yield* runGit(repoDir, ["checkout", "main"]); + yield* runGit(repoDir, ["branch", "-D", "feature/pr-local-fork"]); + + const { manager } = yield* makeManager({ + ghScenario: { + pullRequest: { + number: 82, + title: "Local Fork PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/82", + baseRefName: "main", + headRefName: "feature/pr-local-fork", + state: "open", + isCrossRepository: true, + headRepositoryNameWithOwner: "octocat/codething-mvp", + headRepositoryOwnerLogin: "octocat", + }, + repositoryCloneUrls: { + "octocat/codething-mvp": { + url: forkDir, + sshUrl: forkDir, + }, + }, + }, + }); + + const result = yield* preparePullRequestThread(manager, { + cwd: repoDir, + reference: "82", + mode: "local", + }); + + expect(result.worktreePath).toBeNull(); + expect(result.branch).toBe("feature/pr-local-fork"); + expect( + (yield* runGit(repoDir, ["rev-parse", "--abbrev-ref", "@{upstream}"])).stdout.trim(), + ).toBe("fork-seed/feature/pr-local-fork"); + }), + ); + + it.effect("derives fork repository identity from PR URL when GitHub omits nameWithOwner", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + const originDir = yield* createBareRemote(); + const forkDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", originDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "main"]); + yield* runGit(repoDir, ["remote", "add", "binbandit-seed", forkDir]); + yield* runGit(repoDir, ["checkout", "-b", "fix/git-action-default-without-origin"]); + fs.writeFileSync(path.join(repoDir, "derived-fork.txt"), "derived fork\n"); + yield* runGit(repoDir, ["add", "derived-fork.txt"]); + yield* runGit(repoDir, ["commit", "-m", "Derived fork PR branch"]); + yield* runGit(repoDir, ["push", "-u", "binbandit-seed", "fix/git-action-default-without-origin"]); + yield* runGit(repoDir, ["checkout", "main"]); + yield* runGit(repoDir, ["branch", "-D", "fix/git-action-default-without-origin"]); + + const { manager } = yield* makeManager({ + ghScenario: { + pullRequest: { + number: 642, + title: "fix: use commit as the default git action without origin", + url: "https://github.com/pingdotgg/t3code/pull/642", + baseRefName: "main", + headRefName: "fix/git-action-default-without-origin", + state: "open", + isCrossRepository: true, + headRepositoryOwnerLogin: "binbandit", + }, + repositoryCloneUrls: { + "binbandit/t3code": { + url: forkDir, + sshUrl: forkDir, + }, + }, + }, + }); + + const result = yield* preparePullRequestThread(manager, { + cwd: repoDir, + reference: "642", + mode: "local", + }); + + expect(result.branch).toBe("fix/git-action-default-without-origin"); + expect(result.worktreePath).toBeNull(); + expect( + (yield* runGit(repoDir, ["rev-parse", "--abbrev-ref", "@{upstream}"])).stdout.trim(), + ).toBe("binbandit-seed/fix/git-action-default-without-origin"); + }), + ); + it.effect("reuses an existing dedicated worktree for the PR head branch", () => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); @@ -1248,6 +1407,62 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("reuses an existing PR worktree and restores fork upstream tracking", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + const originDir = yield* createBareRemote(); + const forkDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", originDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "main"]); + yield* runGit(repoDir, ["remote", "add", "fork-seed", forkDir]); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-reused-fork"]); + fs.writeFileSync(path.join(repoDir, "reused-fork.txt"), "reused fork\n"); + yield* runGit(repoDir, ["add", "reused-fork.txt"]); + yield* runGit(repoDir, ["commit", "-m", "Reused fork PR branch"]); + yield* runGit(repoDir, ["push", "-u", "fork-seed", "feature/pr-reused-fork"]); + yield* runGit(repoDir, ["checkout", "main"]); + const worktreePath = path.join(repoDir, "..", `pr-reused-fork-${Date.now()}`); + yield* runGit(repoDir, ["worktree", "add", worktreePath, "feature/pr-reused-fork"]); + yield* runGit(worktreePath, ["branch", "--unset-upstream"], true); + + const { manager } = yield* makeManager({ + ghScenario: { + pullRequest: { + number: 83, + title: "Reused Fork PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/83", + baseRefName: "main", + headRefName: "feature/pr-reused-fork", + state: "open", + isCrossRepository: true, + headRepositoryNameWithOwner: "octocat/codething-mvp", + headRepositoryOwnerLogin: "octocat", + }, + repositoryCloneUrls: { + "octocat/codething-mvp": { + url: forkDir, + sshUrl: forkDir, + }, + }, + }, + }); + + const result = yield* preparePullRequestThread(manager, { + cwd: repoDir, + reference: "83", + mode: "worktree", + }); + + expect(result.worktreePath && fs.realpathSync.native(result.worktreePath)).toBe( + fs.realpathSync.native(worktreePath), + ); + expect( + (yield* runGit(worktreePath, ["rev-parse", "--abbrev-ref", "@{upstream}"])).stdout.trim(), + ).toBe("fork-seed/feature/pr-reused-fork"); + }), + ); + it.effect("rejects worktree prep when the PR head branch is checked out in the main repo", () => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); diff --git a/apps/server/src/git/Layers/GitManager.ts b/apps/server/src/git/Layers/GitManager.ts index 9dc10e8fd8bf3adb04434b6a68384d39e28dd8dc..94285a0c23ff59a1435105231e4da565f7b43ff3 100644 --- a/apps/server/src/git/Layers/GitManager.ts +++ b/apps/server/src/git/Layers/GitManager.ts @@ -38,6 +38,34 @@ interface PullRequestHeadRemoteInfo { headRepositoryOwnerLogin?: string | null; } +function parseRepositoryNameFromPullRequestUrl(url: string): string | null { + const trimmed = url.trim(); + const match = /^https:\/\/github\.com\/[^/]+\/([^/]+)\/pull\/\d+(?:\/.*)?$/i.exec(trimmed); + const repositoryName = match?.[1]?.trim() ?? ""; + return repositoryName.length > 0 ? repositoryName : null; +} + +function resolveHeadRepositoryNameWithOwner( + pullRequest: ResolvedPullRequest & PullRequestHeadRemoteInfo, +): string | null { + const explicitRepository = pullRequest.headRepositoryNameWithOwner?.trim() ?? ""; + if (explicitRepository.length > 0) { + return explicitRepository; + } + + if (!pullRequest.isCrossRepository) { + return null; + } + + const ownerLogin = pullRequest.headRepositoryOwnerLogin?.trim() ?? ""; + const repositoryName = parseRepositoryNameFromPullRequestUrl(pullRequest.url); + if (ownerLogin.length === 0 || !repositoryName) { + return null; + } + + return `${ownerLogin}/${repositoryName}`; +} + function parsePullRequestList(raw: unknown): PullRequestInfo[] { if (!Array.isArray(raw)) return []; @@ -253,9 +281,10 @@ export const makeGitManager = Effect.gen(function* () { const configurePullRequestHeadUpstream = ( cwd: string, pullRequest: ResolvedPullRequest & PullRequestHeadRemoteInfo, + localBranch = pullRequest.headBranch, ) => Effect.gen(function* () { - const repositoryNameWithOwner = pullRequest.headRepositoryNameWithOwner?.trim() ?? ""; + const repositoryNameWithOwner = resolveHeadRepositoryNameWithOwner(pullRequest) ?? ""; if (repositoryNameWithOwner.length === 0) { return; } @@ -278,14 +307,14 @@ export const makeGitManager = Effect.gen(function* () { yield* gitCore.setBranchUpstream({ cwd, - branch: pullRequest.headBranch, + branch: localBranch, remoteName, remoteBranch: pullRequest.headBranch, }); }).pipe( Effect.catch((error) => Effect.logWarning( - `GitManager.configurePullRequestHeadUpstream: failed to configure upstream for ${pullRequest.headBranch} in ${cwd}: ${error.message}`, + `GitManager.configurePullRequestHeadUpstream: failed to configure upstream for ${localBranch} -> ${pullRequest.headBranch} in ${cwd}: ${error.message}`, ).pipe(Effect.asVoid), ), ); @@ -295,7 +324,7 @@ export const makeGitManager = Effect.gen(function* () { pullRequest: ResolvedPullRequest & PullRequestHeadRemoteInfo, ) => Effect.gen(function* () { - const repositoryNameWithOwner = pullRequest.headRepositoryNameWithOwner?.trim() ?? ""; + const repositoryNameWithOwner = resolveHeadRepositoryNameWithOwner(pullRequest) ?? ""; if (repositoryNameWithOwner.length === 0) { yield* gitCore.fetchPullRequestBranch({ @@ -639,11 +668,15 @@ export const makeGitManager = Effect.gen(function* () { reference: normalizedReference, force: true, }); - yield* configurePullRequestHeadUpstream(input.cwd, { - ...pullRequest, - ...toPullRequestHeadRemoteInfo(pullRequestSummary), - }); const details = yield* gitCore.statusDetails(input.cwd); + yield* configurePullRequestHeadUpstream( + input.cwd, + { + ...pullRequest, + ...toPullRequestHeadRemoteInfo(pullRequestSummary), + }, + details.branch ?? pullRequest.headBranch, + ); return { pullRequest, branch: details.branch ?? pullRequest.headBranch, @@ -651,6 +684,24 @@ export const makeGitManager = Effect.gen(function* () { }; } + const ensureExistingWorktreeUpstream = (worktreePath: string) => + Effect.gen(function* () { + const details = yield* gitCore.statusDetails(worktreePath); + yield* configurePullRequestHeadUpstream( + worktreePath, + { + ...pullRequest, + ...toPullRequestHeadRemoteInfo(pullRequestSummary), + }, + details.branch ?? pullRequest.headBranch, + ); + }); + + const pullRequestWithRemoteInfo = { + ...pullRequest, + ...toPullRequestHeadRemoteInfo(pullRequestSummary), + } as const; + const findLocalHeadBranch = (cwd: string) => gitCore.listBranches({ cwd }).pipe( Effect.map((result) => @@ -668,6 +719,7 @@ export const makeGitManager = Effect.gen(function* () { existingBranchBeforeFetch?.worktreePath && existingBranchBeforeFetchPath !== rootWorktreePath ) { + yield* ensureExistingWorktreeUpstream(existingBranchBeforeFetch.worktreePath); return { pullRequest, branch: pullRequest.headBranch, @@ -681,10 +733,7 @@ export const makeGitManager = Effect.gen(function* () { ); } - yield* materializePullRequestHeadBranch(input.cwd, { - ...pullRequest, - ...toPullRequestHeadRemoteInfo(pullRequestSummary), - }); + yield* materializePullRequestHeadBranch(input.cwd, pullRequestWithRemoteInfo); const existingBranchAfterFetch = yield* findLocalHeadBranch(input.cwd); const existingBranchAfterFetchPath = existingBranchAfterFetch?.worktreePath @@ -694,6 +743,7 @@ export const makeGitManager = Effect.gen(function* () { existingBranchAfterFetch?.worktreePath && existingBranchAfterFetchPath !== rootWorktreePath ) { + yield* ensureExistingWorktreeUpstream(existingBranchAfterFetch.worktreePath); return { pullRequest, branch: pullRequest.headBranch, @@ -712,6 +762,7 @@ export const makeGitManager = Effect.gen(function* () { branch: pullRequest.headBranch, path: null, }); + yield* ensureExistingWorktreeUpstream(worktree.worktree.path); return { pullRequest, -- 2.54.0