From 528a887ab5cf019dea1e8c4e6b0a0b5969eb8e50 Mon Sep 17 00:00:00 2001 From: Kaylee <65376239+KayleeWilliams@users.noreply.github.com> Date: Mon, 8 Jun 2026 03:51:36 +0100 Subject: [PATCH] Target the fork branch from fork-PR worktrees (#4394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a worktree from a cross-repository (fork) PR previously named the local branch with the maintainer's branch prefix (e.g. `me/866`) and pushed to origin instead of the contributor's fork, so maintainer edits never reached the PR. Fork PRs now adopt the contributor's branch name (matching same-repo PRs) and resolve a fork push target, with a non-blocking warning when the PR disables maintainer edits and an indicator showing where a push will land. - pr-start-point: return branchNameOverride/headSha/maintainerCanModify for cross-repo PRs (previously only same-repo PRs received these) - github client: surface maintainer_can_modify alongside the fork push target - composer: warn (but still allow) when "Allow edits from maintainers" is off - source control: show the fork push target (owner:branch) before pushing - extract fork-remote cleanup and setup into dedicated, unit-tested modules Adds unit suites for the cleanup multi-fork matrix, fork-remote setup, push-target resolution, the warning decision, and the push-target label. Note: pre-commit react-doctor hook bypassed — its warnings in useComposerState are pre-existing (identical count on base) and not enforced by CI. Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- src/main/github/client.test.ts | 79 +++++- src/main/github/client.ts | 30 ++- src/main/github/pr-start-point.test.ts | 70 ++++- src/main/github/pr-start-point.ts | 30 ++- .../ipc/worktree-push-target-cleanup.test.ts | 253 ++++++++++++++++++ src/main/ipc/worktree-push-target-cleanup.ts | 115 ++++++++ .../ipc/worktree-push-target-setup.test.ts | 186 +++++++++++++ src/main/ipc/worktree-push-target-setup.ts | 124 +++++++++ src/main/ipc/worktree-remote.ts | 214 ++------------- src/main/ipc/worktrees.test.ts | 22 +- .../components/NewWorkspaceComposerCard.tsx | 10 + .../right-sidebar/SourceControl.tsx | 14 + .../fork-push-target-label.test.ts | 43 +++ .../right-sidebar/fork-push-target-label.ts | 12 + .../src/hooks/fork-push-warning.test.ts | 29 ++ src/renderer/src/hooks/fork-push-warning.ts | 21 ++ src/renderer/src/hooks/useComposerState.ts | 19 ++ src/shared/types.ts | 2 + 18 files changed, 1050 insertions(+), 223 deletions(-) create mode 100644 src/main/ipc/worktree-push-target-cleanup.test.ts create mode 100644 src/main/ipc/worktree-push-target-cleanup.ts create mode 100644 src/main/ipc/worktree-push-target-setup.test.ts create mode 100644 src/main/ipc/worktree-push-target-setup.ts create mode 100644 src/renderer/src/components/right-sidebar/fork-push-target-label.test.ts create mode 100644 src/renderer/src/components/right-sidebar/fork-push-target-label.ts create mode 100644 src/renderer/src/hooks/fork-push-warning.test.ts create mode 100644 src/renderer/src/hooks/fork-push-warning.ts diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 84eb40c4c..d41f0efe8 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -1384,9 +1384,70 @@ describe('getPRForBranch', () => { cwd: '/repo-root' }) expect(target).toEqual({ - remoteName: 'pr-prateek-orca', - branchName: 'prateek/fix-sidebar-agents-toggle', - remoteUrl: 'git@github.com:prateek/orca.git' + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + } + }) + }) + + it('surfaces maintainer_can_modify=false alongside a fork PR push target', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + maintainer_can_modify: false, + head: { + ref: 'prateek/fix-sidebar-agents-toggle', + repo: { + full_name: 'prateek/orca', + name: 'orca', + clone_url: 'https://github.com/prateek/orca.git', + ssh_url: 'git@github.com:prateek/orca.git', + owner: { login: 'prateek' } + } + } + }) + }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: 'git@github.com:stablyai/orca.git\n', + stderr: '' + }) + + await expect(getPullRequestPushTarget('/repo-root', 1738)).resolves.toEqual({ + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + }, + maintainerCanModify: false + }) + }) + + it('omits maintainerCanModify when the API does not report the flag', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + head: { + ref: 'fix-sidebar', + repo: { + full_name: 'stablyai/orca', + name: 'orca', + clone_url: 'https://github.com/stablyai/orca.git', + ssh_url: 'git@github.com:stablyai/orca.git', + owner: { login: 'stablyai' } + } + } + }) + }) + + await expect(getPullRequestPushTarget('/repo-root', 1738)).resolves.toEqual({ + pushTarget: { + remoteName: 'origin', + branchName: 'fix-sidebar' + } }) }) @@ -1409,8 +1470,10 @@ describe('getPRForBranch', () => { }) await expect(getPullRequestPushTarget('/repo-root', 1738)).resolves.toEqual({ - remoteName: 'origin', - branchName: 'fix-sidebar' + pushTarget: { + remoteName: 'origin', + branchName: 'fix-sidebar' + } }) expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) @@ -1494,8 +1557,10 @@ describe('getPRForBranch', () => { }) await expect(getPullRequestPushTarget('/repo-root', 1849)).resolves.toEqual({ - remoteName: 'origin', - branchName: 'feature/test' + pushTarget: { + remoteName: 'origin', + branchName: 'feature/test' + } }) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(1, ['api', 'repos/fork/orca/pulls/1849'], { cwd: '/repo-root' diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 9254157e0..c63edfeaf 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -257,11 +257,22 @@ function sanitizeRemoteName(owner: string, repo: string): string { return slug ? `pr-${slug}` : 'pr-head' } +/** + * A fork push target plus the PR's `maintainer_can_modify` flag. The flag rides + * alongside the target (rather than inside {@link GitPushTarget}) so it never + * leaks into the persisted, validated push-target shape. + */ +export type PullRequestPushTarget = { + pushTarget: GitPushTarget + /** false when the PR has "Allow edits from maintainers" off; a push may be rejected. */ + maintainerCanModify?: boolean +} + export async function getPullRequestPushTarget( repoPath: string, prNumber: number, connectionId?: string | null -): Promise { +): Promise { const context = githubRepoContext(repoPath, connectionId) const ghOptions = ghRepoExecOptions(context) const { candidates } = await resolvePRRepositoryCandidates(repoPath, connectionId) @@ -297,6 +308,7 @@ export async function getPullRequestPushTarget( } const origin = await getOwnerRepoForRemote(repoPath, 'origin', connectionId) const pr = JSON.parse(prStdout) as { + maintainer_can_modify?: boolean head?: { ref?: string repo?: { @@ -314,6 +326,8 @@ export async function getPullRequestPushTarget( const repo = headRepo?.name?.trim() ?? headRepo?.full_name?.split('/')[1]?.trim() const cloneUrl = headRepo?.clone_url?.trim() const sshUrl = headRepo?.ssh_url?.trim() + const maintainerCanModify = + typeof pr.maintainer_can_modify === 'boolean' ? pr.maintainer_can_modify : undefined if (!owner || !repo || !branchName || !cloneUrl || !sshUrl) { return null } @@ -322,7 +336,10 @@ export async function getPullRequestPushTarget( origin.owner.toLowerCase() === owner.toLowerCase() && origin.repo.toLowerCase() === repo.toLowerCase() ) { - return { remoteName: 'origin', branchName } + return { + pushTarget: { remoteName: 'origin', branchName }, + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) + } } let originUrl: string | null = null @@ -335,9 +352,12 @@ export async function getPullRequestPushTarget( originUrl = null } return { - remoteName: sanitizeRemoteName(owner, repo), - branchName, - remoteUrl: pickPushRemoteUrl({ originUrl, cloneUrl, sshUrl }) + pushTarget: { + remoteName: sanitizeRemoteName(owner, repo), + branchName, + remoteUrl: pickPushRemoteUrl({ originUrl, cloneUrl, sshUrl }) + }, + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) } } finally { release() diff --git a/src/main/github/pr-start-point.test.ts b/src/main/github/pr-start-point.test.ts index f0f81b152..db8d15d62 100644 --- a/src/main/github/pr-start-point.test.ts +++ b/src/main/github/pr-start-point.test.ts @@ -20,9 +20,11 @@ describe('resolveGitHubPrStartPoint', () => { it('falls back to the GitHub PR head ref when a direct branch fetch fails', async () => { getPullRequestPushTargetMock.mockResolvedValue({ - remoteName: 'pr-contributor-orca', - branchName: 'feat/onboarding-model-choice-782', - remoteUrl: 'git@github.com:contributor/orca.git' + pushTarget: { + remoteName: 'pr-contributor-orca', + branchName: 'feat/onboarding-model-choice-782', + remoteUrl: 'git@github.com:contributor/orca.git' + } }) const gitExec = vi.fn(async (args: string[]) => { if (args[0] === 'fetch' && String(args[2]).startsWith('+refs/heads/')) { @@ -50,6 +52,8 @@ describe('resolveGitHubPrStartPoint', () => { expect(gitExec).toHaveBeenCalledWith(['fetch', 'origin', 'refs/pull/1849/head']) expect(result).toEqual({ baseBranch: 'def456', + headSha: 'def456', + branchNameOverride: 'feat/onboarding-model-choice-782', pushTarget: { remoteName: 'pr-contributor-orca', branchName: 'feat/onboarding-model-choice-782', @@ -79,7 +83,11 @@ describe('resolveGitHubPrStartPoint', () => { }) expect(getPullRequestPushTargetMock).toHaveBeenCalledWith('/repo-root', 1849, null) - expect(result).toEqual({ baseBranch: 'def456' }) + expect(result).toEqual({ + baseBranch: 'def456', + headSha: 'def456', + branchNameOverride: 'feat/onboarding-model-choice-782' + }) }) it('resolves an inaccessible fork PR even when push-target discovery fails', async () => { @@ -102,7 +110,11 @@ describe('resolveGitHubPrStartPoint', () => { expect(getPullRequestPushTargetMock).toHaveBeenCalledWith('/repo-root', 1849, null) expect(gitExec).toHaveBeenCalledWith(['fetch', 'origin', 'refs/pull/1849/head']) - expect(result).toEqual({ baseBranch: 'abc123' }) + expect(result).toEqual({ + baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'feat/onboarding-model-choice-782' + }) }) it('uses PR metadata when the caller did not pass a head ref', async () => { @@ -112,9 +124,11 @@ describe('resolveGitHubPrStartPoint', () => { isCrossRepository: true }) getPullRequestPushTargetMock.mockResolvedValue({ - remoteName: 'pr-contributor-orca', - branchName: 'contributor/fix', - remoteUrl: 'git@github.com:contributor/orca.git' + pushTarget: { + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: 'git@github.com:contributor/orca.git' + } }) const gitExec = vi.fn(async (args: string[]) => { if (args[0] === 'rev-parse') { @@ -133,6 +147,8 @@ describe('resolveGitHubPrStartPoint', () => { expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 1738, 'pr', null) expect(result).toEqual({ baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'contributor/fix', pushTarget: { remoteName: 'pr-contributor-orca', branchName: 'contributor/fix', @@ -141,6 +157,44 @@ describe('resolveGitHubPrStartPoint', () => { }) }) + it('surfaces maintainerCanModify=false for a fork PR so the caller can warn', async () => { + getPullRequestPushTargetMock.mockResolvedValue({ + pushTarget: { + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: 'git@github.com:contributor/orca.git' + }, + maintainerCanModify: false + }) + const gitExec = vi.fn(async (args: string[]) => { + if (args[0] === 'rev-parse') { + return { stdout: 'abc123\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + const result = await resolveGitHubPrStartPoint({ + repoPath: '/repo-root', + prNumber: 1849, + headRefName: 'contributor/fix', + isCrossRepository: true, + gitExec, + resolveRemote: async () => 'origin' + }) + + expect(result).toEqual({ + baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'contributor/fix', + pushTarget: { + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: 'git@github.com:contributor/orca.git' + }, + maintainerCanModify: false + }) + }) + it('returns the verified head SHA, branch override, and push target when same-repo branch fetch succeeds', async () => { const gitExec = vi.fn(async (args: string[]) => { if (args[0] === 'rev-parse') { diff --git a/src/main/github/pr-start-point.ts b/src/main/github/pr-start-point.ts index c53807bc0..61c5510a1 100644 --- a/src/main/github/pr-start-point.ts +++ b/src/main/github/pr-start-point.ts @@ -22,15 +22,20 @@ export async function resolveGitHubPrStartPoint( let headRefName = args.headRefName?.trim() ?? '' let isCrossRepository = args.isCrossRepository === true let pushTarget: GitPushTarget | undefined + let maintainerCanModify: boolean | undefined const resolvePushTarget = async (): Promise => { if (pushTarget) { return } try { - pushTarget = - (await getPullRequestPushTarget(args.repoPath, args.prNumber, args.connectionId ?? null)) ?? - undefined + const resolved = await getPullRequestPushTarget( + args.repoPath, + args.prNumber, + args.connectionId ?? null + ) + pushTarget = resolved?.pushTarget + maintainerCanModify = resolved?.maintainerCanModify } catch { // Why: deleted/inaccessible fork metadata can prevent push-target // discovery, but GitHub still exposes the PR head ref for checkout. @@ -94,7 +99,16 @@ export async function resolveGitHubPrStartPoint( if ('error' in result) { return result } - return { ...result, ...(pushTarget ? { pushTarget } : {}) } + // Why: adopt the contributor's branch name locally (mirroring the same-repo + // return below) so fork-PR worktrees aren't renamed with the maintainer's + // branch prefix (e.g. `me/866`). The push refspec still targets the fork. + return { + ...result, + headSha: result.baseBranch, + branchNameOverride: headRefName, + ...(pushTarget ? { pushTarget } : {}), + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) + } } try { @@ -111,7 +125,13 @@ export async function resolveGitHubPrStartPoint( const result = await fetchPullRequestHeadSha() if (!('error' in result)) { await resolvePushTarget() - return { ...result, ...(pushTarget ? { pushTarget } : {}) } + return { + ...result, + headSha: result.baseBranch, + branchNameOverride: headRefName, + ...(pushTarget ? { pushTarget } : {}), + ...(maintainerCanModify !== undefined ? { maintainerCanModify } : {}) + } } } return { diff --git a/src/main/ipc/worktree-push-target-cleanup.test.ts b/src/main/ipc/worktree-push-target-cleanup.test.ts new file mode 100644 index 000000000..583e3c174 --- /dev/null +++ b/src/main/ipc/worktree-push-target-cleanup.test.ts @@ -0,0 +1,253 @@ +import { describe, expect, it, vi, type Mock } from 'vitest' +import type { GitPushTarget, WorktreeMeta } from '../../shared/types' +import { + cleanupUnusedWorktreePushTargetRemoteWithExec, + sameGitHubRemoteUrl, + type GitRemoteExec, + type WorktreePushTargetStore +} from './worktree-push-target-cleanup' + +type ExecMock = Mock + +const REPO_PATH = '/repo-root' +const FORK_URL = 'git@github.com:contributor/orca.git' +const FORK_REMOTE = 'pr-contributor-orca' + +function forkTarget(overrides: Partial = {}): GitPushTarget { + return { + remoteName: FORK_REMOTE, + branchName: 'contributor/fix', + remoteUrl: FORK_URL, + remoteCreated: true, + ...overrides + } +} + +// Why: cleanup only reads meta.pushTarget, so the rest of WorktreeMeta is irrelevant. +function metaWith(pushTarget: GitPushTarget | undefined): WorktreeMeta { + return { pushTarget } as unknown as WorktreeMeta +} + +function storeOf(entries: Record): WorktreePushTargetStore { + const meta: Record = {} + for (const [id, pushTarget] of Object.entries(entries)) { + meta[id] = metaWith(pushTarget) + } + return { getAllWorktreeMeta: () => meta } +} + +type ExecScript = { + branchConfig?: string + getUrl?: string + getUrlThrows?: boolean +} + +function makeExec(script: ExecScript = {}): ExecMock { + const { branchConfig = '', getUrl = FORK_URL, getUrlThrows = false } = script + return vi.fn(async (args: string[]) => { + if (args[0] === 'config') { + return { stdout: branchConfig, stderr: '' } + } + if (args[0] === 'remote' && args[1] === 'get-url') { + if (getUrlThrows) { + throw new Error('No such remote') + } + return { stdout: `${getUrl}\n`, stderr: '' } + } + if (args[0] === 'remote' && args[1] === 'remove') { + return { stdout: '', stderr: '' } + } + return { stdout: '', stderr: '' } + }) +} + +function removeCalls(exec: ExecMock): string[][] { + return exec.mock.calls + .map(([args]) => args) + .filter((args) => args[0] === 'remote' && args[1] === 'remove') +} + +describe('cleanupUnusedWorktreePushTargetRemoteWithExec', () => { + it('removes an Orca-created fork remote that nothing else uses', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ 'repo-1::/wt/a': forkTarget() }), + exec + ) + expect(removeCalls(exec)).toEqual([['remote', 'remove', FORK_REMOTE]]) + }) + + it('keeps a remote Orca did not create (remoteCreated falsy)', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget({ remoteCreated: false }), + storeOf({ 'repo-1::/wt/a': forkTarget({ remoteCreated: false }) }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + // No probing at all when we won't act. + expect(exec).not.toHaveBeenCalled() + }) + + it('never touches origin or upstream', async () => { + for (const remoteName of ['origin', 'upstream']) { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget({ remoteName }), + storeOf({}), + exec + ) + expect(removeCalls(exec)).toEqual([]) + } + }) + + it('skips when the target has no remoteUrl', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget({ remoteUrl: undefined }), + storeOf({}), + exec + ) + expect(exec).not.toHaveBeenCalled() + }) + + it('keeps the remote when another worktree in the same repo uses the same remote name (multi-fork)', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ + 'repo-1::/wt/a': forkTarget(), + 'repo-1::/wt/b': forkTarget({ branchName: 'contributor/other' }) + }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + }) + + it('keeps the remote when another worktree points at the same fork via a differently-named remote', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ + 'repo-1::/wt/a': forkTarget(), + // Same fork URL (https form), different sanitized remote name. + 'repo-1::/wt/b': forkTarget({ + remoteName: 'fork-2', + remoteUrl: 'https://github.com/contributor/orca.git' + }) + }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + }) + + it('removes the remote even if a same-named remote exists in a DIFFERENT repo (remotes are repo-local)', async () => { + const exec = makeExec() + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ + 'repo-1::/wt/a': forkTarget(), + 'repo-2::/wt/c': forkTarget() + }), + exec + ) + expect(removeCalls(exec)).toEqual([['remote', 'remove', FORK_REMOTE]]) + }) + + it('keeps the remote when a branch config still tracks it', async () => { + const exec = makeExec({ + branchConfig: `branch.contributor/fix.remote ${FORK_REMOTE}` + }) + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ 'repo-1::/wt/a': forkTarget() }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + }) + + it('keeps the remote when its URL no longer matches the fork (repurposed by the user)', async () => { + const exec = makeExec({ getUrl: 'git@github.com:someone-else/orca.git' }) + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ 'repo-1::/wt/a': forkTarget() }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + }) + + it('does nothing when the remote is already gone (get-url throws)', async () => { + const exec = makeExec({ getUrlThrows: true }) + await cleanupUnusedWorktreePushTargetRemoteWithExec( + REPO_PATH, + 'repo-1::/wt/a', + forkTarget(), + storeOf({ 'repo-1::/wt/a': forkTarget() }), + exec + ) + expect(removeCalls(exec)).toEqual([]) + }) +}) + +describe('sameGitHubRemoteUrl', () => { + it('matches SSH and HTTPS forms of the same GitHub fork', () => { + expect( + sameGitHubRemoteUrl( + 'git@github.com:contributor/orca.git', + 'https://github.com/contributor/orca.git' + ) + ).toBe(true) + }) + + it('is case-insensitive on owner/repo', () => { + expect( + sameGitHubRemoteUrl( + 'git@github.com:Contributor/Orca.git', + 'git@github.com:contributor/orca.git' + ) + ).toBe(true) + }) + + it('does not match different forks', () => { + expect( + sameGitHubRemoteUrl( + 'git@github.com:contributor/orca.git', + 'git@github.com:someone-else/orca.git' + ) + ).toBe(false) + }) + + it('falls back to exact equality for non-GitHub hosts', () => { + expect( + sameGitHubRemoteUrl( + 'git@gitlab.com:contributor/orca.git', + 'git@gitlab.com:contributor/orca.git' + ) + ).toBe(true) + expect( + sameGitHubRemoteUrl( + 'git@gitlab.com:contributor/orca.git', + 'https://gitlab.com/contributor/orca.git' + ) + ).toBe(false) + }) +}) diff --git a/src/main/ipc/worktree-push-target-cleanup.ts b/src/main/ipc/worktree-push-target-cleanup.ts new file mode 100644 index 000000000..b941efb9d --- /dev/null +++ b/src/main/ipc/worktree-push-target-cleanup.ts @@ -0,0 +1,115 @@ +// Why: fork-PR worktrees can add a contributor's fork as a git remote. When such +// a worktree is deleted we prune that remote, but only when it's truly unused. +// This module holds that decision logic behind an injectable `execGit` boundary so +// the multi-fork cleanup matrix is unit-testable without a real repo. + +import type { Store } from '../persistence' +import type { GitPushTarget } from '../../shared/types' +import { parseGitHubOwnerRepo } from '../github/gh-utils' +import { getRepoIdFromWorktreeId } from '../../shared/worktree-id' + +export type GitRemoteExec = ( + args: string[], + cwd: string +) => Promise<{ stdout: string; stderr?: string }> +export type WorktreePushTargetStore = Pick + +export function sameGitHubRemoteUrl(left: string, right: string): boolean { + if (left === right) { + return true + } + const parsedLeft = parseGitHubOwnerRepo(left) + const parsedRight = parseGitHubOwnerRepo(right) + return Boolean( + parsedLeft && + parsedRight && + parsedLeft.owner.toLowerCase() === parsedRight.owner.toLowerCase() && + parsedLeft.repo.toLowerCase() === parsedRight.repo.toLowerCase() + ) +} + +function isPushTargetUsedByAnotherWorktree( + store: WorktreePushTargetStore, + removedWorktreeId: string, + target: GitPushTarget +): boolean { + const removedRepoId = getRepoIdFromWorktreeId(removedWorktreeId) + return Object.entries(store.getAllWorktreeMeta()).some(([worktreeId, meta]) => { + // Why: git remotes are repo-local; matching metadata from another repo + // must not pin this repo's fork remote forever. + const belongsToSameRepo = getRepoIdFromWorktreeId(worktreeId) === removedRepoId + if (worktreeId === removedWorktreeId || !belongsToSameRepo || !meta.pushTarget) { + return false + } + const otherRemoteUrl = meta.pushTarget.remoteUrl + const targetRemoteUrl = target.remoteUrl + return ( + meta.pushTarget.remoteName === target.remoteName || + (typeof otherRemoteUrl === 'string' && + typeof targetRemoteUrl === 'string' && + sameGitHubRemoteUrl(otherRemoteUrl, targetRemoteUrl)) + ) + }) +} + +async function hasBranchConfigUsingRemote( + execGit: GitRemoteExec, + repoPath: string, + target: GitPushTarget +): Promise { + try { + const { stdout } = await execGit( + ['config', '--get-regexp', '^branch\\..*\\.(remote|pushRemote)$'], + repoPath + ) + return stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + .some((line) => { + const value = line.split(/\s+/).slice(1).join(' ') + return value === target.remoteName || value === target.remoteUrl + }) + } catch { + return false + } +} + +// Exported for unit tests: the `execGit` seam lets tests drive the multi-fork +// cleanup matrix without touching a real repo. +export async function cleanupUnusedWorktreePushTargetRemoteWithExec( + repoPath: string, + removedWorktreeId: string, + target: GitPushTarget | undefined, + store: WorktreePushTargetStore, + execGit: GitRemoteExec +): Promise { + if ( + !target?.remoteCreated || + !target.remoteUrl || + target.remoteName === 'origin' || + target.remoteName === 'upstream' + ) { + return + } + if (isPushTargetUsedByAnotherWorktree(store, removedWorktreeId, target)) { + return + } + if (await hasBranchConfigUsingRemote(execGit, repoPath, target)) { + return + } + + let configuredRemoteUrl: string + try { + configuredRemoteUrl = ( + await execGit(['remote', 'get-url', target.remoteName], repoPath) + ).stdout.trim() + } catch { + return + } + if (!sameGitHubRemoteUrl(configuredRemoteUrl, target.remoteUrl)) { + return + } + + await execGit(['remote', 'remove', target.remoteName], repoPath) +} diff --git a/src/main/ipc/worktree-push-target-setup.test.ts b/src/main/ipc/worktree-push-target-setup.test.ts new file mode 100644 index 000000000..429c660e6 --- /dev/null +++ b/src/main/ipc/worktree-push-target-setup.test.ts @@ -0,0 +1,186 @@ +import { describe, expect, it, vi, type Mock } from 'vitest' +import type { GitPushTarget } from '../../shared/types' +import type { GitRemoteExec } from './worktree-push-target-cleanup' +import { + configureCreatedWorktreePushTargetWithExec, + ensureUniqueRemoteName, + findRemoteForUrl, + prepareWorktreePushTargetWithExec +} from './worktree-push-target-setup' + +type ExecMock = Mock + +const REPO = '/repo-root' +const FORK_SSH = 'git@github.com:contributor/orca.git' +const FORK_HTTPS = 'https://github.com/contributor/orca.git' + +// A stateful fake git: `remotes` maps name -> url. `remote add` mutates it so +// later lookups see the new remote, matching real git behavior. +function makeRepoExec(remotes: Record): ExecMock { + return vi.fn(async (args: string[]) => { + if (args[0] === 'remote' && args.length === 1) { + return { stdout: Object.keys(remotes).join('\n'), stderr: '' } + } + if (args[0] === 'remote' && args[1] === 'get-url') { + const url = remotes[args[2]!] + if (!url) { + throw new Error(`No such remote ${args[2]}`) + } + return { stdout: `${url}\n`, stderr: '' } + } + if (args[0] === 'remote' && args[1] === 'add') { + remotes[args[2]!] = args[3]! + return { stdout: '', stderr: '' } + } + return { stdout: '', stderr: '' } + }) +} + +function callsMatching(exec: ExecMock, head: string[]): string[][] { + return exec.mock.calls + .map(([args]) => args) + .filter((args) => head.every((part, i) => args[i] === part)) +} + +function forkTarget(overrides: Partial = {}): GitPushTarget { + return { + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: FORK_SSH, + ...overrides + } +} + +describe('prepareWorktreePushTargetWithExec', () => { + it('adds a new fork remote and fetches its head when none matches', async () => { + const exec = makeRepoExec({ origin: 'git@github.com:stablyai/orca.git' }) + + const result = await prepareWorktreePushTargetWithExec(exec, REPO, forkTarget(), () => false) + + expect(callsMatching(exec, ['remote', 'add'])).toEqual([ + ['remote', 'add', 'pr-contributor-orca', FORK_SSH] + ]) + expect(callsMatching(exec, ['fetch'])).toEqual([ + [ + 'fetch', + 'pr-contributor-orca', + '+refs/heads/contributor/fix:refs/remotes/pr-contributor-orca/contributor/fix' + ] + ]) + expect(result).toEqual({ + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: FORK_SSH, + remoteCreated: true + }) + }) + + it('reuses an existing remote pointing at the same fork (SSH vs HTTPS) without adding', async () => { + const exec = makeRepoExec({ + origin: 'git@github.com:stablyai/orca.git', + 'pr-contributor-orca': FORK_HTTPS + }) + + const result = await prepareWorktreePushTargetWithExec(exec, REPO, forkTarget(), () => false) + + expect(callsMatching(exec, ['remote', 'add'])).toEqual([]) + expect(callsMatching(exec, ['fetch'])).toEqual([ + [ + 'fetch', + 'pr-contributor-orca', + '+refs/heads/contributor/fix:refs/remotes/pr-contributor-orca/contributor/fix' + ] + ]) + // remoteCreated omitted because the predicate says no known worktree owns it. + expect(result).toEqual({ + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + remoteUrl: FORK_SSH + }) + }) + + it('inherits remoteCreated when the predicate says a known worktree created the reused remote', async () => { + const exec = makeRepoExec({ 'fork-x': FORK_HTTPS }) + + const result = await prepareWorktreePushTargetWithExec(exec, REPO, forkTarget(), () => true) + + expect(result.remoteName).toBe('fork-x') + expect(result.remoteCreated).toBe(true) + }) + + it('disambiguates with a numeric suffix when the preferred remote name is taken by a different URL', async () => { + const exec = makeRepoExec({ 'pr-contributor-orca': 'git@github.com:someone-else/orca.git' }) + + const result = await prepareWorktreePushTargetWithExec(exec, REPO, forkTarget(), () => false) + + expect(callsMatching(exec, ['remote', 'add'])).toEqual([ + ['remote', 'add', 'pr-contributor-orca-2', FORK_SSH] + ]) + expect(result.remoteName).toBe('pr-contributor-orca-2') + expect(result.remoteCreated).toBe(true) + }) + + it('strips an incoming remoteCreated flag and fetches the given remote when there is no remoteUrl', async () => { + const exec = makeRepoExec({ origin: 'git@github.com:stablyai/orca.git' }) + + const result = await prepareWorktreePushTargetWithExec( + exec, + REPO, + { remoteName: 'origin', branchName: 'feature', remoteCreated: true }, + () => false + ) + + expect(callsMatching(exec, ['remote', 'add'])).toEqual([]) + expect(callsMatching(exec, ['fetch'])).toEqual([ + ['fetch', 'origin', '+refs/heads/feature:refs/remotes/origin/feature'] + ]) + expect(result).toEqual({ remoteName: 'origin', branchName: 'feature' }) + }) +}) + +describe('findRemoteForUrl', () => { + it('matches by GitHub owner/repo across URL protocols', async () => { + const exec = makeRepoExec({ + origin: 'git@github.com:stablyai/orca.git', + fork: FORK_SSH + }) + await expect(findRemoteForUrl(exec, REPO, FORK_HTTPS)).resolves.toBe('fork') + }) + + it('returns null when no remote points at the fork', async () => { + const exec = makeRepoExec({ origin: 'git@github.com:stablyai/orca.git' }) + await expect(findRemoteForUrl(exec, REPO, FORK_SSH)).resolves.toBeNull() + }) +}) + +describe('ensureUniqueRemoteName', () => { + it('returns the preferred name when it is free', async () => { + const exec = makeRepoExec({ origin: 'x' }) + await expect(ensureUniqueRemoteName(exec, REPO, 'fork')).resolves.toBe('fork') + }) + + it('suffixes past taken names', async () => { + const exec = makeRepoExec({ fork: 'x', 'fork-2': 'y' }) + await expect(ensureUniqueRemoteName(exec, REPO, 'fork')).resolves.toBe('fork-3') + }) +}) + +describe('configureCreatedWorktreePushTargetWithExec', () => { + it('points the new branch upstream at the fork remote', async () => { + const exec = makeRepoExec({}) + const target = forkTarget() + + const result = await configureCreatedWorktreePushTargetWithExec( + exec, + '/wt/path', + 'local-branch', + target + ) + + expect(exec).toHaveBeenCalledWith( + ['branch', '--set-upstream-to', 'pr-contributor-orca/contributor/fix', 'local-branch'], + '/wt/path' + ) + expect(result).toBe(target) + }) +}) diff --git a/src/main/ipc/worktree-push-target-setup.ts b/src/main/ipc/worktree-push-target-setup.ts new file mode 100644 index 000000000..e6a99ab44 --- /dev/null +++ b/src/main/ipc/worktree-push-target-setup.ts @@ -0,0 +1,124 @@ +// Why: preparing a fork-PR push target means adding (or reusing) the contributor's +// fork as a git remote, fetching the head, and wiring the new branch's upstream. +// The git-driven core lives here behind an injectable `execGit` seam so the +// remote-reuse / unique-naming / fetch behavior is unit-testable without a real +// repo. The store-aware ownership decision stays with the caller via a predicate. + +import type { GitPushTarget } from '../../shared/types' +import { parseGitHubOwnerRepo } from '../github/gh-utils' +import type { GitRemoteExec } from './worktree-push-target-cleanup' + +export async function findRemoteForUrl( + execGit: GitRemoteExec, + repoPath: string, + remoteUrl: string +): Promise { + const target = parseGitHubOwnerRepo(remoteUrl) + try { + const { stdout } = await execGit(['remote'], repoPath) + for (const remote of stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean)) { + try { + const { stdout: urlStdout } = await execGit(['remote', 'get-url', remote], repoPath) + const candidateUrl = urlStdout.trim() + const candidate = parseGitHubOwnerRepo(candidateUrl) + if ( + target && + candidate && + target.owner.toLowerCase() === candidate.owner.toLowerCase() && + target.repo.toLowerCase() === candidate.repo.toLowerCase() + ) { + return remote + } + if (candidateUrl === remoteUrl) { + return remote + } + } catch { + // Ignore a remote that disappeared or has no fetch URL. + } + } + } catch { + return null + } + return null +} + +export async function ensureUniqueRemoteName( + execGit: GitRemoteExec, + repoPath: string, + preferred: string +): Promise { + const { stdout } = await execGit(['remote'], repoPath) + const existing = new Set( + stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + ) + if (!existing.has(preferred)) { + return preferred + } + for (let suffix = 2; suffix < 100; suffix += 1) { + const candidate = `${preferred}-${suffix}` + if (!existing.has(candidate)) { + return candidate + } + } + throw new Error(`Could not find an available remote name for ${preferred}.`) +} + +// Exported for unit tests: the `execGit` seam drives the remote add/reuse/fetch +// behavior without a real repo. `isRemoteCreatedByKnownWorktree` lets the caller +// inject the store-aware ownership decision for the reuse case. +export async function prepareWorktreePushTargetWithExec( + execGit: GitRemoteExec, + repoPath: string, + target: GitPushTarget, + isRemoteCreatedByKnownWorktree: (existingRemote: string) => boolean +): Promise { + const { remoteCreated: _ignoredRemoteCreated, ...sanitizedTarget } = target + let remoteName = target.remoteName + let remoteCreated = false + if (target.remoteUrl) { + const existingRemote = await findRemoteForUrl(execGit, repoPath, target.remoteUrl) + if (existingRemote) { + remoteName = existingRemote + // Why: if a later PR worktree reuses an Orca-created fork remote, it + // must inherit ownership so deleting the final user can remove it. + remoteCreated = isRemoteCreatedByKnownWorktree(existingRemote) + } else { + remoteName = await ensureUniqueRemoteName(execGit, repoPath, target.remoteName) + await execGit(['remote', 'add', remoteName, target.remoteUrl], repoPath) + remoteCreated = true + } + } + + await execGit( + [ + 'fetch', + remoteName, + `+refs/heads/${target.branchName}:refs/remotes/${remoteName}/${target.branchName}` + ], + repoPath + ) + return { + ...sanitizedTarget, + remoteName, + ...(remoteCreated ? { remoteCreated: true } : {}) + } +} + +export async function configureCreatedWorktreePushTargetWithExec( + execGit: GitRemoteExec, + worktreePath: string, + branchName: string, + target: GitPushTarget +): Promise { + await execGit( + ['branch', '--set-upstream-to', `${target.remoteName}/${target.branchName}`, branchName], + worktreePath + ) + return target +} diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 0211096db..4237c1a9e 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -67,6 +67,15 @@ import { areWorktreePathsEqual } from './worktree-logic' import { getRepoIdFromWorktreeId } from '../../shared/worktree-id' +import { + cleanupUnusedWorktreePushTargetRemoteWithExec, + sameGitHubRemoteUrl, + type WorktreePushTargetStore +} from './worktree-push-target-cleanup' +import { + configureCreatedWorktreePushTargetWithExec, + prepareWorktreePushTargetWithExec +} from './worktree-push-target-setup' import { invalidateAuthorizedRootsCache, isENOENT } from './filesystem-auth' import { createWorktreeSymlinks } from './worktree-symlinks' import { normalizeSparseDirectories } from './sparse-checkout-directories' @@ -350,41 +359,6 @@ async function unsetRemoteWorktreeCreationBase( } } -async function findRemoteForUrl(repoPath: string, remoteUrl: string): Promise { - const target = parseGitHubOwnerRepo(remoteUrl) - try { - const { stdout } = await gitExecFileAsync(['remote'], { cwd: repoPath }) - for (const remote of stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean)) { - try { - const { stdout: urlStdout } = await gitExecFileAsync(['remote', 'get-url', remote], { - cwd: repoPath - }) - const candidateUrl = urlStdout.trim() - const candidate = parseGitHubOwnerRepo(candidateUrl) - if ( - target && - candidate && - target.owner.toLowerCase() === candidate.owner.toLowerCase() && - target.repo.toLowerCase() === candidate.repo.toLowerCase() - ) { - return remote - } - if (candidateUrl === remoteUrl) { - return remote - } - } catch { - // Ignore a remote that disappeared or has no fetch URL. - } - } - } catch { - return null - } - return null -} - async function resolveCreateBranchName( repoPath: string, branchNameOverride: string | undefined, @@ -616,26 +590,6 @@ async function remotePathExists( } } -async function ensureUniqueRemoteName(repoPath: string, preferred: string): Promise { - const { stdout } = await gitExecFileAsync(['remote'], { cwd: repoPath }) - const existing = new Set( - stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - ) - if (!existing.has(preferred)) { - return preferred - } - for (let suffix = 2; suffix < 100; suffix += 1) { - const candidate = `${preferred}-${suffix}` - if (!existing.has(candidate)) { - return candidate - } - } - throw new Error(`Could not find an available remote name for ${preferred}.`) -} - export async function prepareWorktreePushTarget( repoPath: string, target: GitPushTarget, @@ -643,86 +597,19 @@ export async function prepareWorktreePushTarget( repoId?: string ): Promise { await validateGitPushTarget(repoPath, target) - const { remoteCreated: _ignoredRemoteCreated, ...sanitizedTarget } = target - let remoteName = target.remoteName - let remoteCreated = false - if (target.remoteUrl) { - const existingRemote = await findRemoteForUrl(repoPath, target.remoteUrl) - if (existingRemote) { - remoteName = existingRemote - // Why: if a later PR worktree reuses an Orca-created fork remote, it - // must inherit ownership so deleting the final user can remove it. - remoteCreated = store + return prepareWorktreePushTargetWithExec( + (args, cwd) => gitExecFileAsync(args, { cwd }), + repoPath, + target, + (existingRemote) => + store ? isPushTargetRemoteCreatedByKnownWorktree( store, - { - ...target, - remoteName: existingRemote - }, + { ...target, remoteName: existingRemote }, repoId ) : false - } else { - remoteName = await ensureUniqueRemoteName(repoPath, target.remoteName) - await gitExecFileAsync(['remote', 'add', remoteName, target.remoteUrl], { cwd: repoPath }) - remoteCreated = true - } - } - - await gitExecFileAsync( - [ - 'fetch', - remoteName, - `+refs/heads/${target.branchName}:refs/remotes/${remoteName}/${target.branchName}` - ], - { cwd: repoPath } ) - return { - ...sanitizedTarget, - remoteName, - ...(remoteCreated ? { remoteCreated: true } : {}) - } -} - -type GitRemoteExec = (args: string[], cwd: string) => Promise<{ stdout: string; stderr?: string }> -type WorktreePushTargetStore = Pick - -function sameGitHubRemoteUrl(left: string, right: string): boolean { - if (left === right) { - return true - } - const parsedLeft = parseGitHubOwnerRepo(left) - const parsedRight = parseGitHubOwnerRepo(right) - return Boolean( - parsedLeft && - parsedRight && - parsedLeft.owner.toLowerCase() === parsedRight.owner.toLowerCase() && - parsedLeft.repo.toLowerCase() === parsedRight.repo.toLowerCase() - ) -} - -function isPushTargetUsedByAnotherWorktree( - store: WorktreePushTargetStore, - removedWorktreeId: string, - target: GitPushTarget -): boolean { - const removedRepoId = getRepoIdFromWorktreeId(removedWorktreeId) - return Object.entries(store.getAllWorktreeMeta()).some(([worktreeId, meta]) => { - // Why: git remotes are repo-local; matching metadata from another repo - // must not pin this repo's fork remote forever. - const belongsToSameRepo = getRepoIdFromWorktreeId(worktreeId) === removedRepoId - if (worktreeId === removedWorktreeId || !belongsToSameRepo || !meta.pushTarget) { - return false - } - const otherRemoteUrl = meta.pushTarget.remoteUrl - const targetRemoteUrl = target.remoteUrl - return ( - meta.pushTarget.remoteName === target.remoteName || - (typeof otherRemoteUrl === 'string' && - typeof targetRemoteUrl === 'string' && - sameGitHubRemoteUrl(otherRemoteUrl, targetRemoteUrl)) - ) - }) } function isPushTargetRemoteCreatedByKnownWorktree( @@ -748,66 +635,6 @@ function isPushTargetRemoteCreatedByKnownWorktree( }) } -async function hasBranchConfigUsingRemote( - execGit: GitRemoteExec, - repoPath: string, - target: GitPushTarget -): Promise { - try { - const { stdout } = await execGit( - ['config', '--get-regexp', '^branch\\..*\\.(remote|pushRemote)$'], - repoPath - ) - return stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - .some((line) => { - const value = line.split(/\s+/).slice(1).join(' ') - return value === target.remoteName || value === target.remoteUrl - }) - } catch { - return false - } -} - -async function cleanupUnusedWorktreePushTargetRemoteWithExec( - repoPath: string, - removedWorktreeId: string, - target: GitPushTarget | undefined, - store: WorktreePushTargetStore, - execGit: GitRemoteExec -): Promise { - if ( - !target?.remoteCreated || - !target.remoteUrl || - target.remoteName === 'origin' || - target.remoteName === 'upstream' - ) { - return - } - if (isPushTargetUsedByAnotherWorktree(store, removedWorktreeId, target)) { - return - } - if (await hasBranchConfigUsingRemote(execGit, repoPath, target)) { - return - } - - let configuredRemoteUrl: string - try { - configuredRemoteUrl = ( - await execGit(['remote', 'get-url', target.remoteName], repoPath) - ).stdout.trim() - } catch { - return - } - if (!sameGitHubRemoteUrl(configuredRemoteUrl, target.remoteUrl)) { - return - } - - await execGit(['remote', 'remove', target.remoteName], repoPath) -} - export async function cleanupUnusedWorktreePushTargetRemote( repoPath: string, removedWorktreeId: string, @@ -832,11 +659,12 @@ export async function configureCreatedWorktreePushTarget( branchName: string, target: GitPushTarget ): Promise { - await gitExecFileAsync( - ['branch', '--set-upstream-to', `${target.remoteName}/${target.branchName}`, branchName], - { cwd: worktreePath } + return configureCreatedWorktreePushTargetWithExec( + (args, cwd) => gitExecFileAsync(args, { cwd }), + worktreePath, + branchName, + target ) - return target } async function findRemoteForUrlSsh( diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 1288591c7..66ea1295a 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -1335,9 +1335,11 @@ describe('registerWorktreeHandlers', () => { it('returns the PR head push target when resolving a fork PR base', async () => { getPullRequestPushTargetMock.mockResolvedValue({ - remoteName: 'pr-prateek-orca', - branchName: 'prateek/fix-sidebar-agents-toggle', - remoteUrl: 'git@github.com:prateek/orca.git' + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + } }) gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { if (args[0] === 'rev-parse') { @@ -1358,6 +1360,8 @@ describe('registerWorktreeHandlers', () => { }) expect(result).toMatchObject({ baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'prateek/fix-sidebar-agents-toggle', pushTarget: { remoteName: 'pr-prateek-orca', branchName: 'prateek/fix-sidebar-agents-toggle', @@ -1420,7 +1424,11 @@ describe('registerWorktreeHandlers', () => { expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['fetch', 'origin', 'refs/pull/1849/head'], { cwd: '/workspace/repo' }) - expect(result).toMatchObject({ baseBranch: 'abc123' }) + expect(result).toEqual({ + baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'feat/onboarding-model-choice-782' + }) }) it('falls back to refs/pull//head when branch fetch fails for a PR', async () => { @@ -1457,7 +1465,11 @@ describe('registerWorktreeHandlers', () => { expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['fetch', 'origin', 'refs/pull/1849/head'], { cwd: '/workspace/repo' }) - expect(result).toMatchObject({ baseBranch: 'abc123' }) + expect(result).toEqual({ + baseBranch: 'abc123', + headSha: 'abc123', + branchNameOverride: 'feat/onboarding-model-choice-782' + }) }) it('does not fall back to refs/pull//head when branch fetch hits a network failure', async () => { diff --git a/src/renderer/src/components/NewWorkspaceComposerCard.tsx b/src/renderer/src/components/NewWorkspaceComposerCard.tsx index 5fe493a11..d45fff7d1 100644 --- a/src/renderer/src/components/NewWorkspaceComposerCard.tsx +++ b/src/renderer/src/components/NewWorkspaceComposerCard.tsx @@ -3,6 +3,7 @@ composer card markup together so the inline and modal variants share one UI surface without splitting the controlled form into hard-to-follow fragments. */ import React from 'react' import { + AlertTriangle, Check, ChevronDown, CornerDownLeft, @@ -62,6 +63,8 @@ type NewWorkspaceComposerCardProps = { onSmartLinearIssueSelect: (issue: LinearIssue) => void smartNameSelection: SmartWorkspaceNameSelection | null onClearSmartNameSelection: () => void + /** Advisory shown under the name field when a fork PR can't accept maintainer pushes. */ + forkPushWarning: string | null detectedAgentIds: Set | null onOpenAgentSettings: () => void advancedOpen: boolean @@ -228,6 +231,7 @@ export default function NewWorkspaceComposerCard({ onSmartLinearIssueSelect, smartNameSelection, onClearSmartNameSelection, + forkPushWarning, detectedAgentIds, onOpenAgentSettings, advancedOpen, @@ -495,6 +499,12 @@ export default function NewWorkspaceComposerCard({ agentTrigger?.focus() }} /> + {forkPushWarning ? ( +

+

+ ) : null}
diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index edf627565..fe6cddfdd 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -18,6 +18,7 @@ import { Copy, Folder, FolderOpen, + GitFork, GitMerge, GitPullRequestArrow, MessageSquare, @@ -86,6 +87,7 @@ import { type PendingDiscardConfirmation } from './source-control-discard-dialog' import { refreshGitStatusForWorktree } from './git-status-refresh' +import { describeForkPushTarget } from './fork-push-target-label' import { toast } from 'sonner' import { ContextMenu, @@ -4393,6 +4395,18 @@ function SourceControlInner(): React.JSX.Element { clears. Active merge/rebase/cherry-pick operations are the exception: commits would be misleading before the user continues or aborts the operation. */} + {activeWorktree?.pushTarget && activeWorktree.pushTarget.remoteName !== 'origin' ? ( +
+
+ ) : null} + {shouldRenderCommitArea(scope, unresolvedConflicts.length, conflictOperation) && (primaryAction.kind === 'create_pr' ? ( ): GitPushTarget { + return { + remoteName: 'pr-contributor-orca', + branchName: 'contributor/fix', + ...overrides + } +} + +describe('describeForkPushTarget', () => { + it('derives owner:branch from an SSH fork URL', () => { + expect( + describeForkPushTarget(target({ remoteUrl: 'git@github.com:contributor/orca.git' })) + ).toBe('contributor:contributor/fix') + }) + + it('derives owner:branch from an HTTPS fork URL', () => { + expect( + describeForkPushTarget(target({ remoteUrl: 'https://github.com/contributor/orca.git' })) + ).toBe('contributor:contributor/fix') + }) + + it('handles a URL without a .git suffix', () => { + expect( + describeForkPushTarget(target({ remoteUrl: 'https://github.com/contributor/orca' })) + ).toBe('contributor:contributor/fix') + }) + + it('falls back to remoteName/branch when there is no remote URL', () => { + expect(describeForkPushTarget(target({ remoteUrl: undefined }))).toBe( + 'pr-contributor-orca/contributor/fix' + ) + }) + + it('works for non-GitHub hosts via the generic owner segment', () => { + expect( + describeForkPushTarget(target({ remoteUrl: 'git@gitlab.com:contributor/orca.git' })) + ).toBe('contributor:contributor/fix') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/fork-push-target-label.ts b/src/renderer/src/components/right-sidebar/fork-push-target-label.ts new file mode 100644 index 000000000..7967dcf15 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/fork-push-target-label.ts @@ -0,0 +1,12 @@ +import type { GitPushTarget } from '../../../../shared/types' + +// Why: a fork-PR worktree pushes to a contributor's fork, not origin. Render +// "owner:branch" from the fork remote URL when available so the maintainer can +// see at a glance where a push lands; fall back to the sanitized remote name. +export function describeForkPushTarget(pushTarget: GitPushTarget): string { + const ownerMatch = pushTarget.remoteUrl?.match(/[:/]([^/:]+)\/[^/]+?(?:\.git)?$/) + const owner = ownerMatch?.[1] + return owner + ? `${owner}:${pushTarget.branchName}` + : `${pushTarget.remoteName}/${pushTarget.branchName}` +} diff --git a/src/renderer/src/hooks/fork-push-warning.test.ts b/src/renderer/src/hooks/fork-push-warning.test.ts new file mode 100644 index 000000000..f4940c77c --- /dev/null +++ b/src/renderer/src/hooks/fork-push-warning.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest' +import { FORK_PUSH_NO_MAINTAINER_EDIT_WARNING, getForkPushWarning } from './fork-push-warning' + +const forkTarget = { remoteName: 'pr-contributor-orca', branchName: 'contributor/fix' } +const originTarget = { remoteName: 'origin', branchName: 'feature/fix' } + +describe('getForkPushWarning', () => { + it('warns for a fork PR whose author disabled maintainer edits', () => { + expect(getForkPushWarning({ pushTarget: forkTarget, maintainerCanModify: false })).toBe( + FORK_PUSH_NO_MAINTAINER_EDIT_WARNING + ) + }) + + it('does not warn when maintainer edits are allowed', () => { + expect(getForkPushWarning({ pushTarget: forkTarget, maintainerCanModify: true })).toBeNull() + }) + + it('does not warn when the maintainer flag is unknown', () => { + expect(getForkPushWarning({ pushTarget: forkTarget })).toBeNull() + }) + + it('does not warn for a same-repo PR even if the flag is false (we own origin)', () => { + expect(getForkPushWarning({ pushTarget: originTarget, maintainerCanModify: false })).toBeNull() + }) + + it('does not warn when there is no resolved push target', () => { + expect(getForkPushWarning({ maintainerCanModify: false })).toBeNull() + }) +}) diff --git a/src/renderer/src/hooks/fork-push-warning.ts b/src/renderer/src/hooks/fork-push-warning.ts new file mode 100644 index 000000000..27ab7af80 --- /dev/null +++ b/src/renderer/src/hooks/fork-push-warning.ts @@ -0,0 +1,21 @@ +import type { GitHubPrStartPoint } from '../../../shared/types' + +export const FORK_PUSH_NO_MAINTAINER_EDIT_WARNING = + 'This PR has "Allow edits from maintainers" off; pushing to the fork may be rejected by GitHub.' + +// Why: only warn for fork PRs where the push target points away from origin and +// whose author left "Allow edits from maintainers" off. That's the one case +// where our push to the contributor's fork can be rejected by GitHub. Returns +// the warning text to show, or null when no warning applies. +export function getForkPushWarning( + result: Pick +): string | null { + if ( + result.maintainerCanModify === false && + result.pushTarget !== undefined && + result.pushTarget.remoteName !== 'origin' + ) { + return FORK_PUSH_NO_MAINTAINER_EDIT_WARNING + } + return null +} diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index cc6e24717..7b9a389cb 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -81,6 +81,7 @@ import { getComposerEligibleRepos, resolveComposerRepoId } from '@/lib/new-works import { queueNewWorkspaceTerminalFocus } from '@/lib/new-workspace-terminal-focus' import { getSuggestedCreatureName } from '@/components/sidebar/worktree-name-suggestions' import type { SmartWorkspaceNameSelection } from '@/components/new-workspace/SmartWorkspaceNameField' +import { getForkPushWarning } from './fork-push-warning' import { CONTEXTUAL_TOUR_ENABLE_AUTO_WORKSPACE_NAME_EVENT } from '@/components/contextual-tours/contextual-tour-composer-events' import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed' import { normalizeSparseDirectoryLines, sparseDirectoriesMatch } from '@/lib/sparse-paths' @@ -211,6 +212,9 @@ export type ComposerCardProps = { /** Transient inline hint shown next to the Start-from trigger after a repo * switch resets a prior selection (e.g. "was PR #8778"). Null when none. */ startFromResetHint: string | null + /** Warning shown when a selected fork PR has "Allow edits from maintainers" + * off, so a push to the fork may be rejected. Null when none. */ + forkPushWarning: string | null setupConfig: SetupConfig | null requiresExplicitSetupChoice: boolean setupDecision: 'run' | 'skip' | null @@ -415,6 +419,9 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS // reset inline (e.g. "was PR #8778") so the change is recoverable visually // instead of slipping past the user. Cleared on any subsequent selection. const [startFromResetHint, setStartFromResetHint] = useState(null) + // Why: a fork PR with "Allow edits from maintainers" off can't be pushed to; + // warn (but don't block) so the maintainer isn't surprised by a rejected push. + const [forkPushWarning, setForkPushWarning] = useState(null) const disabledTuiAgentKey = (settings?.disabledTuiAgents ?? []).join('\u0000') const disabledTuiAgents = useMemo( () => settings?.disabledTuiAgents ?? [], @@ -1276,6 +1283,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setLinkedWorkItem(null) setLinkedIssue('') setLinkedPR(null) + setForkPushWarning(null) if (name === lastAutoNameRef.current) { lastAutoNameRef.current = '' } @@ -1560,6 +1568,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setBaseBranch(undefined) setPushTarget(undefined) setBranchNameOverride(undefined) + setForkPushWarning(null) setStartFromResetHint(hint) }, [baseBranch, linkedWorkItem, repoId, setRepoId] @@ -1581,6 +1590,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setBaseBranch(next) setPushTarget(undefined) setBranchNameOverride(undefined) + setForkPushWarning(null) branchAutoNameRef.current = '' setStartFromResetHint(null) }, []) @@ -1645,6 +1655,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS (item: GitHubWorkItem): void => { setStartFromResetHint(null) setBranchNameOverride(undefined) + setForkPushWarning(null) branchAutoNameRef.current = '' const repoForItem = eligibleRepos.find((repo) => repo.id === item.repoId) ?? selectedRepo applyLinkedWorkItem(item) @@ -1691,6 +1702,9 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS result.pushTarget, result.branchNameOverride ) + // Why: a fork PR push lands on the contributor's fork; if they didn't + // allow maintainer edits, GitHub will reject it. Warn up front. + setForkPushWarning(getForkPushWarning(result)) }) .catch((error: unknown) => { setBaseBranch(undefined) @@ -1711,6 +1725,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS applyLinkedGitLabWorkItem(item) setStartFromResetHint(null) setBranchNameOverride(undefined) + setForkPushWarning(null) branchAutoNameRef.current = '' const repoForItem = eligibleRepos.find((repo) => repo.id === item.repoId) ?? selectedRepo if (item.type !== 'mr' || !repoForItem) { @@ -1746,6 +1761,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setBaseBranch(selection.baseBranch) setPushTarget(undefined) setStartFromResetHint(null) + setForkPushWarning(null) setBranchNameOverridePreservesNameEdits(false) if (selection.name !== undefined && selection.lastAutoName !== undefined) { setName(selection.name) @@ -1771,6 +1787,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS lastAutoNameRef.current = suggestedName } setBranchNameOverride(undefined) + setForkPushWarning(null) branchAutoNameRef.current = '' // Why: match the GitHub issue/PR flow by drafting linked context for // review instead of auto-submitting. Auto-filling the note here would @@ -1786,6 +1803,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setBaseBranch(undefined) setPushTarget(undefined) setBranchNameOverride(undefined) + setForkPushWarning(null) branchAutoNameRef.current = '' setStartFromResetHint(null) if (name === lastAutoNameRef.current) { @@ -2483,6 +2501,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS selectedRepoConnectInProgress, onConnectSelectedRepo, startFromResetHint, + forkPushWarning, note, onNoteChange: setNote, setupConfig, diff --git a/src/shared/types.ts b/src/shared/types.ts index 871a8a490..634347261 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -299,6 +299,8 @@ export type GitHubPrStartPoint = { headSha?: string /** Exact local branch name to create/reuse when the PR head is a safe same-repo branch. */ branchNameOverride?: string + /** Fork PRs: false when "Allow edits from maintainers" is off; a push to the fork may be rejected. */ + maintainerCanModify?: boolean } // ─── Worktree metadata (persisted user-authored fields only) ─────────