From 5a6b2994ea7cfef261cfd9b83ad1f8b59c9774cb Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 13 May 2026 16:14:50 -0700 Subject: [PATCH] fix: address review findings (#1770) --- src/main/git/push-target-validation.ts | 12 ++ src/main/git/remote.test.ts | 99 +++++++++----- src/main/git/remote.ts | 72 +++++++++-- src/main/github/client.test.ts | 70 +++++++++- src/main/github/client.ts | 86 ++++++++++++ src/main/ipc/filesystem.ts | 20 ++- src/main/ipc/worktree-logic.ts | 1 + src/main/ipc/worktree-remote.ts | 122 ++++++++++++++++++ src/main/ipc/worktrees.test.ts | 105 ++++++++++++++- src/main/ipc/worktrees.ts | 33 ++++- src/main/providers/ssh-git-provider.test.ts | 13 +- src/main/providers/ssh-git-provider.ts | 9 +- src/main/providers/types.ts | 3 +- src/preload/api-types.ts | 4 +- src/preload/index.ts | 3 +- src/relay/git-handler-push-target.ts | 58 +++++++++ src/relay/git-handler.ts | 25 ++-- .../right-sidebar/SourceControl.tsx | 28 +++- src/renderer/src/hooks/useComposerState.ts | 37 +++++- src/renderer/src/store/slices/editor.ts | 23 +++- .../src/store/slices/worktree-helpers.ts | 4 +- src/renderer/src/store/slices/worktrees.ts | 9 +- src/shared/git-push-target-validation.ts | 36 ++++++ src/shared/types.ts | 15 ++- 24 files changed, 793 insertions(+), 94 deletions(-) create mode 100644 src/main/git/push-target-validation.ts create mode 100644 src/relay/git-handler-push-target.ts create mode 100644 src/shared/git-push-target-validation.ts diff --git a/src/main/git/push-target-validation.ts b/src/main/git/push-target-validation.ts new file mode 100644 index 00000000000..f29c1bc7347 --- /dev/null +++ b/src/main/git/push-target-validation.ts @@ -0,0 +1,12 @@ +import type { GitPushTarget } from '../../shared/types' +import { assertGitPushTargetShape } from '../../shared/git-push-target-validation' +import { gitExecFileAsync } from './runner' + +export async function validateGitPushTarget( + repoPath: string, + target: unknown +): Promise { + assertGitPushTargetShape(target) + await gitExecFileAsync(['check-ref-format', '--branch', target.branchName], { cwd: repoPath }) + return target +} diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index 4d3f626f8a5..c9213fd8100 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -15,34 +15,65 @@ describe('git remote operations', () => { gitExecFileAsyncMock.mockReset() }) - it('pushes with --set-upstream regardless of publish flag', async () => { - // Why: every push uses --set-upstream so worktrees that were created - // tracking the BASE ref (origin/main) get their upstream repointed to - // origin/ on first push. Without that the local branch keeps - // tracking origin/main forever and the UI's ahead/behind read via - // @{u} measures "ahead of base" rather than "ahead of remote branch". - // Both publish=true and publish=false take the same path now; the - // parameter is preserved in the signature for IPC compatibility but - // is no longer load-bearing. + it('pushes to origin when no upstream is configured', async () => { gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('no branch'), { code: 1 })) await gitPush('/repo', true) - await gitPush('/repo', false) - expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( - 1, - ['push', '--set-upstream', 'origin', 'HEAD'], - { cwd: '/repo' } - ) - expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, + expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith( ['push', '--set-upstream', 'origin', 'HEAD'], { cwd: '/repo' } ) }) + it('pushes to the configured upstream remote and branch', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'review/pr-1738\n', stderr: '' }) + .mockResolvedValueOnce({ stdout: 'pr-prateek-orca\n', stderr: '' }) + .mockResolvedValueOnce({ + stdout: 'refs/heads/prateek/fix-sidebar-agents-toggle\n', + stderr: '' + }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await gitPush('/repo', false) + + expect(gitExecFileAsyncMock.mock.calls).toEqual([ + [['symbolic-ref', '--quiet', '--short', 'HEAD'], { cwd: '/repo' }], + [['config', '--get', 'branch.review/pr-1738.remote'], { cwd: '/repo' }], + [['config', '--get', 'branch.review/pr-1738.merge'], { cwd: '/repo' }], + [ + ['push', '--set-upstream', 'pr-prateek-orca', 'HEAD:prateek/fix-sidebar-agents-toggle'], + { cwd: '/repo' } + ] + ]) + }) + + it('uses an explicit push target even when it differs from the local branch name', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await gitPush('/repo', false, { + remoteName: 'origin', + branchName: 'contributor/fix-sidebar' + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['push', '--set-upstream', 'origin', 'HEAD:contributor/fix-sidebar'], + { cwd: '/repo' } + ) + expect(gitExecFileAsyncMock.mock.calls).toEqual([ + [['check-ref-format', '--branch', 'contributor/fix-sidebar'], { cwd: '/repo' }], + [['push', '--set-upstream', 'origin', 'HEAD:contributor/fix-sidebar'], { cwd: '/repo' }] + ]) + }) + it('maps non-fast-forward push failures to an actionable message', async () => { - gitExecFileAsyncMock.mockRejectedValueOnce(new Error('remote rejected: non-fast-forward')) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce(new Error('remote rejected: non-fast-forward')) await expect(gitPush('/repo', false)).rejects.toThrow( 'Push rejected: remote has newer commits (non-fast-forward). Please pull or sync first.' @@ -50,19 +81,23 @@ describe('git remote operations', () => { }) it('passes through clean tail line when push error does not match known patterns', async () => { - gitExecFileAsyncMock.mockRejectedValueOnce( - new Error('Command failed: git push\nfatal: something obscure happened') - ) + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce( + new Error('Command failed: git push\nfatal: something obscure happened') + ) await expect(gitPush('/repo', false)).rejects.toThrow('fatal: something obscure happened') }) it('strips embedded credentials from push error messages', async () => { - gitExecFileAsyncMock.mockRejectedValueOnce( - new Error( - 'Command failed: git push\nhttps://x-access-token:ghp_abc@github.com/foo/bar.git\nfatal: remote error' + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce( + new Error( + 'Command failed: git push\nhttps://x-access-token:ghp_abc@github.com/foo/bar.git\nfatal: remote error' + ) ) - ) let caught: Error | undefined try { @@ -77,11 +112,13 @@ describe('git remote operations', () => { }) it('strips token-only credentials (https://TOKEN@host) from push error messages', async () => { - gitExecFileAsyncMock.mockRejectedValueOnce( - new Error( - 'Command failed: git push\nhttps://ghp_onlyToken@github.com/foo/bar.git\nfatal: remote error' + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce( + new Error( + 'Command failed: git push\nhttps://ghp_onlyToken@github.com/foo/bar.git\nfatal: remote error' + ) ) - ) let caught: Error | undefined try { @@ -95,7 +132,9 @@ describe('git remote operations', () => { }) it('falls back to a generic message for non-Error rejections', async () => { - gitExecFileAsyncMock.mockRejectedValueOnce('string') + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce('string') await expect(gitPush('/repo', false)).rejects.toThrow('Git remote operation failed.') }) diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index 2d26c5c75b1..ce9f2c9a32e 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -1,26 +1,70 @@ import { normalizeGitErrorMessage } from '../../shared/git-remote-error' +import type { GitPushTarget } from '../../shared/types' +import { validateGitPushTarget } from './push-target-validation' import { gitExecFileAsync } from './runner' -export async function gitPush(worktreePath: string, _publish = false): Promise { +async function getConfiguredPushTarget( + worktreePath: string +): Promise<{ remote: string; refspec: string } | null> { try { - // Why: always pass --set-upstream so that worktrees Orca creates with - // `git worktree add --track -b ` (which initially - // track the BASE — e.g. origin/main) get their upstream repointed to - // origin/ on first push. Without this the local branch keeps - // tracking origin/main forever, so ahead/behind reads via @{u} measure - // "ahead of base" rather than "ahead of remote branch", and the primary - // button never rotates from "Push" to "Commit" after a successful push. + const { stdout: branchStdout } = await gitExecFileAsync( + ['symbolic-ref', '--quiet', '--short', 'HEAD'], + { cwd: worktreePath } + ) + const branch = branchStdout.trim() + if (!branch) { + return null + } + + const [{ stdout: remoteStdout }, { stdout: mergeStdout }] = await Promise.all([ + gitExecFileAsync(['config', '--get', `branch.${branch}.remote`], { cwd: worktreePath }), + gitExecFileAsync(['config', '--get', `branch.${branch}.merge`], { cwd: worktreePath }) + ]) + const remote = remoteStdout.trim() + const mergeRef = mergeStdout.trim() + const branchRef = mergeRef.replace(/^refs\/heads\//, '') + if (!remote || !branchRef || remote === '.' || branchRef === mergeRef) { + return null + } + if (remote === 'origin' && branchRef !== branch) { + return null + } + return { remote, refspec: `HEAD:${branchRef}` } + } catch { + return null + } +} + +function explicitPushTarget(target: GitPushTarget): { remote: string; refspec: string } { + return { remote: target.remoteName, refspec: `HEAD:${target.branchName}` } +} + +export async function gitPush( + worktreePath: string, + _publish = false, + pushTarget?: GitPushTarget +): Promise { + try { + if (pushTarget) { + await validateGitPushTarget(worktreePath, pushTarget) + } + // Why: push to the branch's configured upstream when one exists. PR-created + // worktrees can track a contributor fork remote; hardcoding origin here + // would send review commits to the upstream repository instead. // - // The `publish` flag becomes redundant under this strategy — every push - // sets upstream, including the first. We keep the parameter in the - // signature so callers don't need to change, but it's no longer - // load-bearing. On an already-published branch --set-upstream is a - // no-op for the tracking config and a regular push otherwise. + // When no upstream exists, keep the existing first-publish behavior: + // create/update origin/ and set it as upstream. // // Branch-vs-base reporting (the "Committed on Branch" section) is // unaffected because it uses branchCompare against an explicit baseRef // from worktree config, not the upstream relationship. - await gitExecFileAsync(['push', '--set-upstream', 'origin', 'HEAD'], { cwd: worktreePath }) + const target = pushTarget + ? explicitPushTarget(pushTarget) + : await getConfiguredPushTarget(worktreePath) + const args = target + ? ['push', '--set-upstream', target.remote, target.refspec] + : ['push', '--set-upstream', 'origin', 'HEAD'] + await gitExecFileAsync(args, { cwd: worktreePath }) } catch (error) { throw new Error(normalizeGitErrorMessage(error, 'push')) } diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 5ba69ebd357..4af9e9aaba6 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -5,6 +5,7 @@ const { ghExecFileAsyncMock, getOwnerRepoMock, getIssueOwnerRepoMock, + getOwnerRepoForRemoteMock, gitExecFileAsyncMock, acquireMock, releaseMock @@ -13,6 +14,7 @@ const { ghExecFileAsyncMock: vi.fn(), getOwnerRepoMock: vi.fn(), getIssueOwnerRepoMock: vi.fn(), + getOwnerRepoForRemoteMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), acquireMock: vi.fn(), releaseMock: vi.fn() @@ -23,6 +25,12 @@ vi.mock('./gh-utils', () => ({ ghExecFileAsync: ghExecFileAsyncMock, getOwnerRepo: getOwnerRepoMock, getIssueOwnerRepo: getIssueOwnerRepoMock, + getOwnerRepoForRemote: getOwnerRepoForRemoteMock, + gitExecFileAsync: gitExecFileAsyncMock, + parseGitHubOwnerRepo: (remoteUrl: string) => { + const match = remoteUrl.trim().match(/github\.com[:/]([^/]+)\/([^/]+?)(?:\.git)?$/) + return match ? { owner: match[1], repo: match[2] } : null + }, acquire: acquireMock, release: releaseMock, _resetOwnerRepoCache: vi.fn() @@ -32,7 +40,7 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) -import { getPRForBranch, _resetOwnerRepoCache } from './client' +import { getPRForBranch, getPullRequestPushTarget, _resetOwnerRepoCache } from './client' describe('getPRForBranch', () => { beforeEach(() => { @@ -40,6 +48,7 @@ describe('getPRForBranch', () => { ghExecFileAsyncMock.mockReset() getOwnerRepoMock.mockReset() getIssueOwnerRepoMock.mockReset() + getOwnerRepoForRemoteMock.mockReset() gitExecFileAsyncMock.mockReset() acquireMock.mockReset() releaseMock.mockReset() @@ -260,4 +269,63 @@ describe('getPRForBranch', () => { expect(pr).toBeNull() }) + + it('resolves fork PR push target using the origin URL protocol', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + 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: '' + }) + + const target = await getPullRequestPushTarget('/repo-root', 1738) + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith(['api', 'repos/stablyai/orca/pulls/1738'], { + cwd: '/repo-root' + }) + expect(target).toEqual({ + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + }) + }) + + it('uses origin for same-repository PR push targets', 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({ + remoteName: 'origin', + branchName: 'fix-sidebar' + }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) }) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 033998d85ac..cde6e71f3be 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -2,6 +2,7 @@ concurrency acquire/release pattern and error handling consistent across operations. */ import type { ClassifiedError, + GitPushTarget, IssueSourcePreference, ListWorkItemsResult, PRInfo, @@ -19,6 +20,7 @@ import { getPRConflictSummary } from './conflict-summary' import { execFileAsync, ghExecFileAsync, + gitExecFileAsync, acquire, release, getOwnerRepo, @@ -73,6 +75,90 @@ export async function checkOrcaStarred(): Promise { } } +function pickPushRemoteUrl(args: { + originUrl: string | null + cloneUrl: string + sshUrl: string +}): string { + const { originUrl, cloneUrl, sshUrl } = args + if (originUrl && (/^(git@|ssh:)/.test(originUrl) || originUrl.includes('ssh.github.com'))) { + return sshUrl + } + return cloneUrl +} + +function sanitizeRemoteName(owner: string, repo: string): string { + const slug = `${owner}-${repo}` + .toLowerCase() + .replace(/[^a-z0-9._-]+/g, '-') + .replace(/-+/g, '-') + .replace(/^[.-]+|[.-]+$/g, '') + return slug ? `pr-${slug}` : 'pr-head' +} + +export async function getPullRequestPushTarget( + repoPath: string, + prNumber: number +): Promise { + const ownerRepo = await getOwnerRepo(repoPath) + if (!ownerRepo) { + return null + } + + await acquire() + try { + const [{ stdout: prStdout }, origin] = await Promise.all([ + ghExecFileAsync(['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${prNumber}`], { + cwd: repoPath + }), + getOwnerRepoForRemote(repoPath, 'origin') + ]) + const pr = JSON.parse(prStdout) as { + head?: { + ref?: string + repo?: { + full_name?: string + clone_url?: string + ssh_url?: string + owner?: { login?: string } + name?: string + } | null + } + } + const headRepo = pr.head?.repo + const branchName = pr.head?.ref?.trim() + const owner = headRepo?.owner?.login?.trim() + const repo = headRepo?.name?.trim() ?? headRepo?.full_name?.split('/')[1]?.trim() + const cloneUrl = headRepo?.clone_url?.trim() + const sshUrl = headRepo?.ssh_url?.trim() + if (!owner || !repo || !branchName || !cloneUrl || !sshUrl) { + return null + } + if ( + origin && + origin.owner.toLowerCase() === owner.toLowerCase() && + origin.repo.toLowerCase() === repo.toLowerCase() + ) { + return { remoteName: 'origin', branchName } + } + + let originUrl: string | null = null + try { + const { stdout } = await gitExecFileAsync(['remote', 'get-url', 'origin'], { cwd: repoPath }) + originUrl = stdout.trim() || null + } catch { + originUrl = null + } + return { + remoteName: sanitizeRemoteName(owner, repo), + branchName, + remoteUrl: pickPushRemoteUrl({ originUrl, cloneUrl, sshUrl }) + } + } finally { + release() + } +} + /** * Star the Orca repo for the authenticated user. */ diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index cd060af12bc..09a6b53e327 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -11,6 +11,7 @@ import type { GitBranchCompareResult, GitConflictOperation, GitDiffResult, + GitPushTarget, GitUpstreamStatus, GitStatusResult, MarkdownDocument, @@ -40,6 +41,8 @@ import { } from '../git/status' import { getUpstreamStatus } from '../git/upstream' import { gitFetch, gitPull, gitPush } from '../git/remote' +import { assertGitPushTargetShape } from '../../shared/git-push-target-validation' +import { validateGitPushTarget } from '../git/push-target-validation' import { getRemoteFileUrl } from '../git/repo' import { resolveAuthorizedPath, @@ -608,21 +611,32 @@ export function registerFilesystemHandlers(store: Store): void { 'git:push', async ( _event, - args: { worktreePath: string; publish?: boolean; connectionId?: string } + args: { + worktreePath: string + publish?: boolean + connectionId?: string + pushTarget?: GitPushTarget + } ): Promise => { // Why: coerce to strict boolean at the IPC boundary so a malformed // renderer payload (e.g. string 'false') can't silently enable // --set-upstream mode. Mirrors the relay handler in src/relay/git-handler.ts. const publish = args.publish === true if (args.connectionId) { + if (args.pushTarget) { + assertGitPushTargetShape(args.pushTarget) + } const provider = getSshGitProvider(args.connectionId) if (!provider) { throw new Error(`No git provider for connection "${args.connectionId}"`) } - return provider.pushBranch(args.worktreePath, publish) + return provider.pushBranch(args.worktreePath, publish, args.pushTarget) } const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) - await gitPush(worktreePath, publish) + if (args.pushTarget) { + await validateGitPushTarget(worktreePath, args.pushTarget) + } + await gitPush(worktreePath, publish, args.pushTarget) } ) diff --git a/src/main/ipc/worktree-logic.ts b/src/main/ipc/worktree-logic.ts index c2568e12583..b950ec2897b 100644 --- a/src/main/ipc/worktree-logic.ts +++ b/src/main/ipc/worktree-logic.ts @@ -200,6 +200,7 @@ export function mergeWorktree( } : {}), ...(meta?.baseRef !== undefined ? { baseRef: meta.baseRef } : {}), + ...(meta?.pushTarget !== undefined ? { pushTarget: meta.pushTarget } : {}), // Why: diff comments are persisted on WorktreeMeta (see `WorktreeMeta` in // shared/types) and forwarded verbatim so the renderer store mirrors // on-disk state. `undefined` here means the worktree has no comments yet. diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 4c4020dfd49..91e5f2338c1 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -14,13 +14,16 @@ import type { Store } from '../persistence' import type { CreateWorktreeArgs, CreateWorktreeResult, + GitPushTarget, Repo, WorktreeMeta } from '../../shared/types' import { getPRForBranch } from '../github/client' import { listWorktrees, addWorktree, addSparseWorktree } from '../git/worktree' import { getGitUsername, getDefaultBaseRef, getBranchConflictKind } from '../git/repo' +import { validateGitPushTarget } from '../git/push-target-validation' import { gitExecFileAsync } from '../git/runner' +import { parseGitHubOwnerRepo } from '../github/gh-utils' import type { OrcaRuntimeService } from '../runtime/orca-runtime' import type { RemoteFetchResult, RemoteTrackingBase } from '../runtime/orca-runtime' import { isWslPath, parseWslPath, getWslHome } from '../wsl' @@ -49,6 +52,103 @@ async function readCommitSha(repoPath: string, ref: string): Promise { return stdout.trim() } +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 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}.`) +} + +async function prepareWorktreePushTarget( + repoPath: string, + target: GitPushTarget +): Promise { + await validateGitPushTarget(repoPath, target) + let remoteName = target.remoteName + if (target.remoteUrl) { + const existingRemote = await findRemoteForUrl(repoPath, target.remoteUrl) + if (existingRemote) { + remoteName = existingRemote + } else { + remoteName = await ensureUniqueRemoteName(repoPath, target.remoteName) + await gitExecFileAsync(['remote', 'add', remoteName, target.remoteUrl], { cwd: repoPath }) + } + } + + await gitExecFileAsync( + [ + 'fetch', + remoteName, + `+refs/heads/${target.branchName}:refs/remotes/${remoteName}/${target.branchName}` + ], + { cwd: repoPath } + ) + return { + ...target, + remoteName + } +} + +async function configureCreatedWorktreePushTarget( + worktreePath: string, + branchName: string, + target: GitPushTarget +): Promise { + await gitExecFileAsync( + ['branch', '--set-upstream-to', `${target.remoteName}/${target.branchName}`, branchName], + { cwd: worktreePath } + ) + return target +} + export function notifyWorktreesChanged(mainWindow: BrowserWindow, repoId: string): void { if (!mainWindow.isDestroyed()) { mainWindow.webContents.send('worktrees:changed', { repoId }) @@ -444,6 +544,14 @@ export async function createLocalWorktree( } emitCreateWorktreeProgress(mainWindow, 'creating') + let preparedPushTarget: GitPushTarget | undefined + if (args.pushTarget) { + // Why: validate and fetch the contributor remote before creating the + // worktree. If this fails, retrying won't hit branch/path conflicts from a + // half-created worktree. + preparedPushTarget = await prepareWorktreePushTarget(repo.path, args.pushTarget) + } + await (sparseDirectories.length > 0 ? addSparseWorktree( repo.path, @@ -461,6 +569,19 @@ export async function createLocalWorktree( settings.refreshLocalBaseRefOnWorktreeCreate )) + let configuredPushTarget: GitPushTarget | undefined + if (preparedPushTarget) { + // Why: fork-PR review worktrees should publish commits back to the PR + // author's branch. Configure the branch upstream immediately so the + // existing Push/Pull/Sync controls use the contributor remote instead of + // silently defaulting to origin. + configuredPushTarget = await configureCreatedWorktreePushTarget( + worktreePath, + branchName, + preparedPushTarget + ) + } + // Re-list to get the freshly created worktree info const gitWorktrees = await listWorktrees(repo.path) const created = gitWorktrees.find((gw) => areWorktreePathsEqual(gw.path, worktreePath)) @@ -479,6 +600,7 @@ export async function createLocalWorktree( // worktree from ambient PTY bumps in other worktrees for CREATE_GRACE_MS. createdAt: now, baseRef: baseBranch, + ...(configuredPushTarget ? { pushTarget: configuredPushTarget } : {}), ...(requestedDisplayName ? { displayName: requestedDisplayName } : shouldSetDisplayName(effectiveRequestedName, branchName, effectiveSanitizedName) diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index a4049a8866d..68a9793e584 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -10,8 +10,11 @@ const { removeWorktreeMock, getGitUsernameMock, getDefaultBaseRefMock, + getDefaultRemoteMock, getBranchConflictKindMock, getPRForBranchMock, + getWorkItemMock, + getPullRequestPushTargetMock, getEffectiveHooksMock, createIssueCommandRunnerScriptMock, createSetupRunnerScriptMock, @@ -33,8 +36,11 @@ const { removeWorktreeMock: vi.fn(), getGitUsernameMock: vi.fn(), getDefaultBaseRefMock: vi.fn(), + getDefaultRemoteMock: vi.fn(), getBranchConflictKindMock: vi.fn(), getPRForBranchMock: vi.fn(), + getWorkItemMock: vi.fn(), + getPullRequestPushTargetMock: vi.fn(), getEffectiveHooksMock: vi.fn(), createIssueCommandRunnerScriptMock: vi.fn(), createSetupRunnerScriptMock: vi.fn(), @@ -71,11 +77,14 @@ vi.mock('../git/runner', () => ({ vi.mock('../git/repo', () => ({ getGitUsername: getGitUsernameMock, getDefaultBaseRef: getDefaultBaseRefMock, + getDefaultRemote: getDefaultRemoteMock, getBranchConflictKind: getBranchConflictKindMock })) vi.mock('../github/client', () => ({ - getPRForBranch: getPRForBranchMock + getPRForBranch: getPRForBranchMock, + getWorkItem: getWorkItemMock, + getPullRequestPushTarget: getPullRequestPushTargetMock })) vi.mock('../providers/ssh-git-dispatch', () => ({ @@ -169,8 +178,11 @@ describe('registerWorktreeHandlers', () => { removeWorktreeMock, getGitUsernameMock, getDefaultBaseRefMock, + getDefaultRemoteMock, getBranchConflictKindMock, getPRForBranchMock, + getWorkItemMock, + getPullRequestPushTargetMock, getEffectiveHooksMock, createIssueCommandRunnerScriptMock, createSetupRunnerScriptMock, @@ -231,8 +243,11 @@ describe('registerWorktreeHandlers', () => { store.setWorktreeMeta.mockReturnValue({}) getGitUsernameMock.mockReturnValue('') getDefaultBaseRefMock.mockReturnValue('origin/main') + getDefaultRemoteMock.mockResolvedValue('origin') getBranchConflictKindMock.mockResolvedValue(null) getPRForBranchMock.mockResolvedValue(null) + getWorkItemMock.mockResolvedValue(null) + getPullRequestPushTargetMock.mockResolvedValue(null) // Why: createLocalWorktree can still hit legacy git fetch fallback in // narrow unit harnesses. Return a resolved promise so catch/then chains // don't trip on undefined. @@ -392,6 +407,94 @@ describe('registerWorktreeHandlers', () => { }) }) + it('configures a PR push target during local create', async () => { + listWorktreesMock.mockResolvedValue([ + { + path: '/workspace/improve-dashboard', + head: 'abc123', + branch: 'refs/heads/improve-dashboard', + isBare: false, + isMainWorktree: false + } + ]) + store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + + await handlers['worktrees:create'](null, { + repoId: 'repo-1', + name: 'improve-dashboard', + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + } + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['remote', 'add', 'pr-prateek-orca', 'git@github.com:prateek/orca.git'], + { cwd: '/workspace/repo' } + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'fetch', + 'pr-prateek-orca', + '+refs/heads/prateek/fix-sidebar-agents-toggle:refs/remotes/pr-prateek-orca/prateek/fix-sidebar-agents-toggle' + ], + { cwd: '/workspace/repo' } + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'branch', + '--set-upstream-to', + 'pr-prateek-orca/prateek/fix-sidebar-agents-toggle', + 'improve-dashboard' + ], + { cwd: '/workspace/improve-dashboard' } + ) + expect(store.setWorktreeMeta).toHaveBeenCalledWith( + 'repo-1::/workspace/improve-dashboard', + expect.objectContaining({ + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + } + }) + ) + }) + + 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' + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'rev-parse') { + return { stdout: 'abc123\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + const result = await handlers['worktrees:resolvePrBase'](null, { + repoId: 'repo-1', + prNumber: 1738, + headRefName: 'prateek/fix-sidebar-agents-toggle', + isCrossRepository: true + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['fetch', 'origin', 'refs/pull/1738/head'], { + cwd: '/workspace/repo' + }) + expect(result).toEqual({ + baseBranch: 'abc123', + pushTarget: { + remoteName: 'pr-prateek-orca', + branchName: 'prateek/fix-sidebar-agents-toggle', + remoteUrl: 'git@github.com:prateek/orca.git' + } + }) + }) + it('persists linked issue and PR metadata during remote create', async () => { const repo = { id: 'repo-ssh', diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 8257306e78f..4975ef6b0a3 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -8,6 +8,7 @@ import { deleteWorktreeHistoryDir } from '../terminal-history' import type { CreateWorktreeArgs, CreateWorktreeResult, + GitPushTarget, GitWorktreeInfo, Repo, WorktreeMeta @@ -15,7 +16,7 @@ import type { import { removeWorktree } from '../git/worktree' import { gitExecFileAsync } from '../git/runner' import { getDefaultRemote } from '../git/repo' -import { getWorkItem } from '../github/client' +import { getPullRequestPushTarget, getWorkItem } from '../github/client' import { listRepoWorktrees, createFolderWorktree } from '../repo-worktrees' import { getSshGitProvider } from '../providers/ssh-git-dispatch' import { @@ -281,7 +282,7 @@ export function registerWorktreeHandlers( headRefName?: string isCrossRepository?: boolean } - ): Promise<{ baseBranch: string } | { error: string }> => { + ): Promise<{ baseBranch: string; pushTarget?: GitPushTarget } | { error: string }> => { const repo = store.getRepo(args.repoId) if (!repo) { return { error: 'Repo not found' } @@ -297,6 +298,7 @@ export function registerWorktreeHandlers( let headRefName = args.headRefName?.trim() ?? '' let isCrossRepository = args.isCrossRepository === true + let pushTarget: CreateWorktreeArgs['pushTarget'] | undefined // Skip the gh lookup when both hints are present (picker already has them). if (!headRefName) { @@ -315,6 +317,21 @@ export function registerWorktreeHandlers( isCrossRepository = true } } + if (isCrossRepository) { + try { + pushTarget = (await getPullRequestPushTarget(repo.path, args.prNumber)) ?? undefined + } catch (error) { + return { + error: + error instanceof Error + ? error.message + : `Could not resolve PR #${args.prNumber} head push target.` + } + } + if (!pushTarget) { + return { error: `Could not resolve PR #${args.prNumber} head push target.` } + } + } let remote: string try { @@ -352,11 +369,14 @@ export function registerWorktreeHandlers( if (!sha) { return { error: `Empty SHA resolving fork PR #${args.prNumber} head.` } } - return { baseBranch: sha } + return { baseBranch: sha, ...(pushTarget ? { pushTarget } : {}) } } try { - await gitExecFileAsync(['fetch', remote, headRefName], { cwd: repo.path }) + await gitExecFileAsync( + ['fetch', remote, `+refs/heads/${headRefName}:refs/remotes/${remote}/${headRefName}`], + { cwd: repo.path } + ) } catch (error) { const message = error instanceof Error ? error.message : String(error) return { @@ -371,7 +391,10 @@ export function registerWorktreeHandlers( return { error: `Remote ref ${remoteRef} does not exist after fetch.` } } - return { baseBranch: remoteRef } + if (!pushTarget) { + pushTarget = { remoteName: remote, branchName: headRefName } + } + return { baseBranch: remoteRef, pushTarget } } ) diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 0a0c759b515..947699a591c 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -139,11 +139,18 @@ describe('SshGitProvider', () => { expect(result).toEqual(upstreamResult) }) - it('pushBranch sends git.push request and forwards publish mode', async () => { - await provider.pushBranch('/home/user/repo', true) + it('pushBranch sends git.push request and forwards publish mode and target', async () => { + await provider.pushBranch('/home/user/repo', true, { + remoteName: 'pr-fork-orca', + branchName: 'contributor/fix' + }) expect(mux.request).toHaveBeenCalledWith('git.push', { worktreePath: '/home/user/repo', - publish: true + publish: true, + pushTarget: { + remoteName: 'pr-fork-orca', + branchName: 'contributor/fix' + } }) }) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 8af00a6b408..8132a7db8b8 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -6,6 +6,7 @@ import type { GitDiffResult, GitBranchCompareResult, GitConflictOperation, + GitPushTarget, GitUpstreamStatus, GitWorktreeInfo } from '../../shared/types' @@ -90,8 +91,12 @@ export class SshGitProvider implements IGitProvider { })) as GitUpstreamStatus } - async pushBranch(worktreePath: string, publish = false): Promise { - await this.mux.request('git.push', { worktreePath, publish }) + async pushBranch( + worktreePath: string, + publish = false, + pushTarget?: GitPushTarget + ): Promise { + await this.mux.request('git.push', { worktreePath, publish, pushTarget }) } async pullBranch(worktreePath: string): Promise { diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index f79a0723542..2c16456071d 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -5,6 +5,7 @@ import type { GitDiffResult, GitBranchCompareResult, GitConflictOperation, + GitPushTarget, GitUpstreamStatus, GitWorktreeInfo, SearchOptions, @@ -148,7 +149,7 @@ export type IGitProvider = { detectConflictOperation(worktreePath: string): Promise getBranchCompare(worktreePath: string, baseRef: string): Promise getUpstreamStatus(worktreePath: string): Promise - pushBranch(worktreePath: string, publish?: boolean): Promise + pushBranch(worktreePath: string, publish?: boolean, pushTarget?: GitPushTarget): Promise pullBranch(worktreePath: string): Promise fetchRemote(worktreePath: string): Promise getBranchDiff( diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 364a699ac4c..7263505190a 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -19,6 +19,7 @@ import type { GitBranchCompareResult, GitConflictOperation, GitDiffResult, + GitPushTarget, GitStatusResult, GitUpstreamStatus, GitHubAssignableUser, @@ -459,7 +460,7 @@ export type PreloadApi = { prNumber: number headRefName?: string isCrossRepository?: boolean - }) => Promise<{ baseBranch: string } | { error: string }> + }) => Promise<{ baseBranch: string; pushTarget?: GitPushTarget } | { error: string }> remove: (args: { worktreeId: string; force?: boolean; skipArchive?: boolean }) => Promise updateMeta: (args: { worktreeId: string; updates: Partial }) => Promise persistSortOrder: (args: { orderedIds: string[] }) => Promise @@ -992,6 +993,7 @@ export type PreloadApi = { worktreePath: string publish?: boolean connectionId?: string + pushTarget?: GitPushTarget }) => Promise pull: (args: { worktreePath: string; connectionId?: string }) => Promise branchDiff: (args: { diff --git a/src/preload/index.ts b/src/preload/index.ts index f99b91a157a..9f402ee7d25 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -372,7 +372,7 @@ const api = { prNumber: number headRefName?: string isCrossRepository?: boolean - }): Promise<{ baseBranch: string } | { error: string }> => + }): Promise<{ baseBranch: string; pushTarget?: unknown } | { error: string }> => ipcRenderer.invoke('worktrees:resolvePrBase', args), remove: (args: { worktreeId: string; force?: boolean; skipArchive?: boolean }): Promise => @@ -1630,6 +1630,7 @@ const api = { worktreePath: string publish?: boolean connectionId?: string + pushTarget?: unknown }): Promise => ipcRenderer.invoke('git:push', args), pull: (args: { worktreePath: string; connectionId?: string }): Promise => ipcRenderer.invoke('git:pull', args), diff --git a/src/relay/git-handler-push-target.ts b/src/relay/git-handler-push-target.ts new file mode 100644 index 00000000000..1262837fa87 --- /dev/null +++ b/src/relay/git-handler-push-target.ts @@ -0,0 +1,58 @@ +import { assertGitPushTargetShape } from '../shared/git-push-target-validation' +import type { GitPushTarget } from '../shared/types' + +type RelayGit = (args: string[], cwd: string) => Promise<{ stdout: string; stderr: string }> + +export type ResolvedPushTarget = { + remote: string + refspec: string +} + +async function getConfiguredPushTarget( + git: RelayGit, + worktreePath: string +): Promise { + try { + const { stdout: branchStdout } = await git( + ['symbolic-ref', '--quiet', '--short', 'HEAD'], + worktreePath + ) + const branch = branchStdout.trim() + if (!branch) { + return null + } + const [{ stdout: remoteStdout }, { stdout: mergeStdout }] = await Promise.all([ + git(['config', '--get', `branch.${branch}.remote`], worktreePath), + git(['config', '--get', `branch.${branch}.merge`], worktreePath) + ]) + const remote = remoteStdout.trim() + const mergeRef = mergeStdout.trim() + const branchRef = mergeRef.replace(/^refs\/heads\//, '') + if (!remote || !branchRef || remote === '.' || branchRef === mergeRef) { + return null + } + if (remote === 'origin' && branchRef !== branch) { + return null + } + return { remote, refspec: `HEAD:${branchRef}` } + } catch { + return null + } +} + +export async function resolveRelayPushTarget( + git: RelayGit, + worktreePath: string, + pushTarget: unknown +): Promise { + if (pushTarget === undefined) { + return getConfiguredPushTarget(git, worktreePath) + } + assertGitPushTargetShape(pushTarget) + const explicitTarget: GitPushTarget = pushTarget + await git(['check-ref-format', '--branch', explicitTarget.branchName], worktreePath) + return { + remote: explicitTarget.remoteName, + refspec: `HEAD:${explicitTarget.branchName}` + } +} diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 79de7175c82..63e7be21398 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- Why: this relay handler centralizes the git RPC +protocol surface so local and SSH git behavior stay in one dispatch table. */ import { execFile } from 'child_process' import { promisify } from 'util' import { rm } from 'fs/promises' @@ -14,6 +16,7 @@ import { } from './git-handler-ops' import { commitChangesRelay, addWorktreeOp, removeWorktreeOp } from './git-handler-worktree-ops' import { detectConflictOperation, getStatusOp } from './git-handler-status-ops' +import { resolveRelayPushTarget } from './git-handler-push-target' import { normalizeGitErrorMessage, isNoUpstreamError } from '../shared/git-remote-error' const execFileAsync = promisify(execFile) @@ -246,19 +249,19 @@ export class GitHandler { private async push(params: Record) { const worktreePath = params.worktreePath as string - // Why: always pass --set-upstream (mirrors src/main/git/remote.ts). - // Orca's worktrees initially track the BASE ref (origin/main) because - // they're created via `git worktree add --track -b - // ` — without --set-upstream the local branch keeps tracking - // the base after the first push, so ahead/behind via @{u} measures - // "ahead of base" instead of "ahead of remote branch", and the UI's - // primary button never rotates from "Push" to "Commit". The `publish` - // flag is preserved in the param shape for IPC compatibility but is no - // longer load-bearing. On an already-published branch --set-upstream is - // a no-op for the tracking config. + // Why: mirror src/main/git/remote.ts. Push to a configured upstream when + // present so SSH worktrees with non-origin targets do not get repointed. void params.publish try { - await this.git(['push', '--set-upstream', 'origin', 'HEAD'], worktreePath) + const target = await resolveRelayPushTarget( + this.git.bind(this), + worktreePath, + params.pushTarget + ) + const args = target + ? ['push', '--set-upstream', target.remote, target.refspec] + : ['push', '--set-upstream', 'origin', 'HEAD'] + await this.git(args, worktreePath) } catch (error) { // Why: mirror the local gitPush normalization so SSH users see the same // "non-fast-forward / pull first" guidance instead of raw git stderr. diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 0668ce47187..542c98b7b38 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -599,11 +599,23 @@ function SourceControlInner(): React.JSX.Element { const connectionId = getConnectionId(activeWorktreeId) ?? undefined try { if (kind === 'publish') { - await pushBranch(activeWorktreeId, worktreePath, true, connectionId) + await pushBranch( + activeWorktreeId, + worktreePath, + true, + connectionId, + activeWorktree?.pushTarget + ) return } if (kind === 'push') { - await pushBranch(activeWorktreeId, worktreePath, false, connectionId) + await pushBranch( + activeWorktreeId, + worktreePath, + false, + connectionId, + activeWorktree?.pushTarget + ) return } if (kind === 'pull') { @@ -614,13 +626,21 @@ function SourceControlInner(): React.JSX.Element { await fetchBranch(activeWorktreeId, worktreePath, connectionId) return } - await syncBranch(activeWorktreeId, worktreePath, connectionId) + await syncBranch(activeWorktreeId, worktreePath, connectionId, activeWorktree?.pushTarget) } catch { // Why: remote action failures are surfaced by editor-slice actions to keep // one consistent toast path and avoid duplicate notifications in the UI. } }, - [activeWorktreeId, fetchBranch, pullBranch, pushBranch, syncBranch, worktreePath] + [ + activeWorktree?.pushTarget, + activeWorktreeId, + fetchBranch, + pullBranch, + pushBranch, + syncBranch, + worktreePath + ] ) // Why: compound actions must commit first and only run the follow-up remote diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index 4c1ce4c8e26..3907dfdc7cc 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -19,6 +19,7 @@ import { tuiAgentToAgentKind } from '@/lib/telemetry' import { isGitRepoKind } from '../../../shared/repo-kind' import type { GitHubWorkItem, + GitPushTarget, LinearIssue, OrcaHooks, SetupDecision, @@ -123,7 +124,11 @@ export type ComposerCardProps = { onBaseBranchChange: (next: string | undefined) => void /** Called when a PR is selected in the Start-from picker. Updates both * baseBranch and linkedWorkItem/linkedPR in one pass. */ - onBaseBranchPrSelect: (baseBranch: string, item: GitHubWorkItem) => void + onBaseBranchPrSelect: ( + baseBranch: string, + item: GitHubWorkItem, + pushTarget?: GitPushTarget + ) => void /** PR number selected via the Start-from picker (when applicable). Used so the * field can render "PR #N" copy. */ baseBranchLinkedPrNumber: number | null @@ -286,6 +291,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS const [baseBranch, setBaseBranch] = useState( persistDraft ? newWorkspaceDraft?.baseBranch : initialBaseBranch ) + const [pushTarget, setPushTarget] = useState(undefined) // Why: when a repo switch wipes a prior Start-from selection, surface the // reset inline (e.g. "was PR #8778") so the change is recoverable visually // instead of slipping past the user. Cleared on any subsequent selection. @@ -1077,6 +1083,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS // selection is meaningless in the new repo. Resetting to undefined // makes the field fall back to the new repo's effective base ref. setBaseBranch(undefined) + setPushTarget(undefined) setStartFromResetHint(hint) }, [baseBranch, linkedWorkItem, repoId, setRepoId] @@ -1096,12 +1103,14 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS const handleBaseBranchChange = useCallback((next: string | undefined): void => { setBaseBranch(next) + setPushTarget(undefined) setStartFromResetHint(null) }, []) const handleBaseBranchPrSelect = useCallback( - (nextBaseBranch: string, item: GitHubWorkItem): void => { + (nextBaseBranch: string, item: GitHubWorkItem, nextPushTarget?: GitPushTarget): void => { setBaseBranch(nextBaseBranch) + setPushTarget(nextPushTarget) setStartFromResetHint(null) // Why: per spec, a PR selection in the Start-from picker is also a // linkedWorkItem assignment. Reuse applyLinkedWorkItem so auto-name and @@ -1125,12 +1134,14 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS const handleSmartGitHubItemSelect = useCallback( (item: GitHubWorkItem): void => { - applyLinkedWorkItem(item) setStartFromResetHint(null) const repoForItem = eligibleRepos.find((repo) => repo.id === item.repoId) ?? selectedRepo if (item.type !== 'pr' || !repoForItem) { + setPushTarget(undefined) + applyLinkedWorkItem(item) return } + setPushTarget(undefined) void window.api.worktrees .resolvePrBase({ repoId: repoForItem.id, @@ -1142,9 +1153,17 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS }) .then((result) => { if ('error' in result) { + setBaseBranch(undefined) + setPushTarget(undefined) + toast.error(result.error) return } - handleBaseBranchPrSelect(result.baseBranch, item) + handleBaseBranchPrSelect(result.baseBranch, item, result.pushTarget) + }) + .catch((error: unknown) => { + setBaseBranch(undefined) + setPushTarget(undefined) + toast.error(error instanceof Error ? error.message : 'Failed to resolve PR base.') }) }, [applyLinkedWorkItem, eligibleRepos, handleBaseBranchPrSelect, selectedRepo] @@ -1153,6 +1172,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS const handleSmartBranchSelect = useCallback( (refName: string): void => { setBaseBranch(refName) + setPushTarget(undefined) setStartFromResetHint(null) if (!name.trim() || name === lastAutoNameRef.current) { setName(refName) @@ -1193,6 +1213,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS setLinkedPR(null) setLinkedWorkItem(null) setBaseBranch(undefined) + setPushTarget(undefined) setStartFromResetHint(null) if (name === lastAutoNameRef.current) { setName('') @@ -1297,7 +1318,8 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS telemetrySource, linkedWorkItem?.title, parsedLinkedIssueNumber ?? undefined, - effectiveLinkedPR ?? undefined + effectiveLinkedPR ?? undefined, + pushTarget ) const worktree = result.worktree @@ -1381,6 +1403,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS onCreated, parsedLinkedIssueNumber, persistDraft, + pushTarget, repoId, requiresExplicitSetupChoice, resolvedSetupDecision, @@ -1446,7 +1469,8 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS telemetrySource, linkedWorkItem?.title, parsedLinkedIssueNumber ?? undefined, - effectiveLinkedPR ?? undefined + effectiveLinkedPR ?? undefined, + pushTarget ) const worktree = result.worktree @@ -1582,6 +1606,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS onCreated, parsedLinkedIssueNumber, persistDraft, + pushTarget, repoId, requiresExplicitSetupChoice, resolvedSetupDecision, diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 5bad48e0871..6c06e042e10 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -13,6 +13,7 @@ import type { GitConflictOperation, GitConflictResolutionStatus, GitConflictStatusSource, + GitPushTarget, GitStatusEntry, GitStatusResult, GitUpstreamStatus, @@ -296,10 +297,16 @@ export type EditorSlice = { worktreeId: string, worktreePath: string, publish?: boolean, - connectionId?: string + connectionId?: string, + pushTarget?: GitPushTarget ) => Promise pullBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise - syncBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise + syncBranch: ( + worktreeId: string, + worktreePath: string, + connectionId?: string, + pushTarget?: GitPushTarget + ) => Promise fetchBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise gitBranchChangesByWorktree: Record gitBranchCompareSummaryByWorktree: Record @@ -1900,7 +1907,7 @@ export const createEditorSlice: StateCreator = (s console.error('fetchUpstreamStatus failed', error) } }, - pushBranch: async (worktreeId, worktreePath, publish = false, connectionId) => { + pushBranch: async (worktreeId, worktreePath, publish = false, connectionId, pushTarget) => { // Why: don't *await* a post-op git status / upstream refresh here. // Chaining awaited refreshes inside the mutation extends the gap before // compound flows (runCompoundCommitAction → runRemoteAction) reach the @@ -1912,7 +1919,7 @@ export const createEditorSlice: StateCreator = (s // store as soon as the IPC resolves. get().beginRemoteOperation(publish ? 'publish' : 'push') try { - await window.api.git.push({ worktreePath, publish, connectionId }) + await window.api.git.push({ worktreePath, publish, connectionId, pushTarget }) } catch (error) { toast.error(resolveRemoteOperationErrorMessage(error, { publish, isPush: true })) throw error @@ -1933,7 +1940,7 @@ export const createEditorSlice: StateCreator = (s } void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) }, - syncBranch: async (worktreeId, worktreePath, connectionId) => { + syncBranch: async (worktreeId, worktreePath, connectionId, pushTarget) => { // Why: same shape as pushBranch / pullBranch — fire-and-forget the // post-op upstream refresh after the busy flag clears so the primary // button label rotates immediately when the IPC resolves. @@ -1956,7 +1963,11 @@ export const createEditorSlice: StateCreator = (s }) if (upstreamStatus.ahead > 0) { try { - await window.api.git.push({ worktreePath, connectionId }) + await window.api.git.push({ + worktreePath, + connectionId, + pushTarget + }) } catch (error) { // Why: format under the user-facing operation (sync) rather than // the inner step (push) — the user clicked Sync and shouldn't see diff --git a/src/renderer/src/store/slices/worktree-helpers.ts b/src/renderer/src/store/slices/worktree-helpers.ts index 4d19bf13b5b..64072125e47 100644 --- a/src/renderer/src/store/slices/worktree-helpers.ts +++ b/src/renderer/src/store/slices/worktree-helpers.ts @@ -1,6 +1,7 @@ import type { CreateWorktreeResult, CreateSparseCheckoutRequest, + GitPushTarget, SetupDecision, WorkspaceCreateTelemetrySource, Worktree, @@ -71,7 +72,8 @@ export type WorktreeSlice = { telemetrySource?: WorkspaceCreateTelemetrySource, displayName?: string, linkedIssue?: number, - linkedPR?: number + linkedPR?: number, + pushTarget?: GitPushTarget ) => Promise removeWorktree: ( worktreeId: string, diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 81dde04ad98..5d5f98b7504 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -48,6 +48,9 @@ function areWorktreesEqual(current: Worktree[] | undefined, next: Worktree[]): b worktree.sortOrder === candidate.sortOrder && worktree.lastActivityAt === candidate.lastActivityAt && worktree.baseRef === candidate.baseRef && + worktree.pushTarget?.remoteName === candidate.pushTarget?.remoteName && + worktree.pushTarget?.branchName === candidate.pushTarget?.branchName && + worktree.pushTarget?.remoteUrl === candidate.pushTarget?.remoteUrl && worktree.sparseBaseRef === candidate.sparseBaseRef && arraysShallowEqual(worktree.sparseDirectories, candidate.sparseDirectories) ) @@ -226,7 +229,8 @@ export const createWorktreeSlice: StateCreator telemetrySource, displayName, linkedIssue, - linkedPR + linkedPR, + pushTarget ) => { const retryableConflictPatterns = [ /already exists locally/i, @@ -249,7 +253,8 @@ export const createWorktreeSlice: StateCreator ...(displayName ? { displayName } : {}), ...(telemetrySource ? { telemetrySource } : {}), ...(linkedIssue !== undefined ? { linkedIssue } : {}), - ...(linkedPR !== undefined ? { linkedPR } : {}) + ...(linkedPR !== undefined ? { linkedPR } : {}), + ...(pushTarget ? { pushTarget } : {}) }) // Why: a file watcher (worktrees.onChanged) can fire between the // backend creating the worktree and this callback running, causing diff --git a/src/shared/git-push-target-validation.ts b/src/shared/git-push-target-validation.ts new file mode 100644 index 00000000000..ed71eb02286 --- /dev/null +++ b/src/shared/git-push-target-validation.ts @@ -0,0 +1,36 @@ +import type { GitPushTarget } from './types' + +const SAFE_REMOTE_NAME = /^[A-Za-z0-9][A-Za-z0-9._-]{0,99}$/ +const GITHUB_CLONE_URL = /^https:\/\/github\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\.git$/ +const GITHUB_SSH_URL = /^git@github\.com:[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\.git$/ + +function assertString(value: unknown, name: string): asserts value is string { + if (typeof value !== 'string') { + throw new Error(`Invalid PR push target ${name}.`) + } +} + +export function assertGitPushTargetShape(target: unknown): asserts target is GitPushTarget { + if (typeof target !== 'object' || target === null) { + throw new Error('Invalid PR push target.') + } + const candidate = target as Record + assertString(candidate.remoteName, 'remote name') + assertString(candidate.branchName, 'branch name') + if ( + !SAFE_REMOTE_NAME.test(candidate.remoteName) || + candidate.remoteName === '.' || + candidate.remoteName === '..' + ) { + throw new Error(`Invalid git remote name: ${candidate.remoteName}`) + } + if (!candidate.branchName || candidate.branchName.startsWith('-')) { + throw new Error(`Invalid git branch name: ${candidate.branchName}`) + } + if (candidate.remoteUrl !== undefined) { + assertString(candidate.remoteUrl, 'remote URL') + if (!(GITHUB_CLONE_URL.test(candidate.remoteUrl) || GITHUB_SSH_URL.test(candidate.remoteUrl))) { + throw new Error('Invalid PR push target remote URL.') + } + } +} diff --git a/src/shared/types.ts b/src/shared/types.ts index 6a099fea49c..b382ce7df95 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -128,9 +128,17 @@ export type Worktree = { sparsePresetId?: string /** Intended create base for stale-base probes. Persisted metadata, not UI drift state. */ baseRef?: string + /** Remote/branch Orca should publish review commits to when it created this worktree. */ + pushTarget?: GitPushTarget diffComments?: DiffComment[] } & GitWorktreeInfo +export type GitPushTarget = { + remoteName: string + branchName: string + remoteUrl?: string +} + // ─── Worktree metadata (persisted user-authored fields only) ───────── export type WorktreeMeta = { displayName: string @@ -150,6 +158,8 @@ export type WorktreeMeta = { sparsePresetId?: string /** Intended create base for stale-base probes. Persisted metadata, not UI drift state. */ baseRef?: string + /** See {@link Worktree.pushTarget}. Persisted so refreshed worktree lists keep the target. */ + pushTarget?: GitPushTarget diffComments?: DiffComment[] } @@ -570,8 +580,8 @@ export type GitHubWorkItem = { branchName?: string baseRefName?: string // Why: true when a PR's head lives on a fork (headRepositoryOwner !== selected repo owner). - // The Start-from picker disables fork PRs in v1 because the create flow cannot - // safely resolve a fork head from headRefName alone. + // The Start-from picker passes this to resolvePrBase so fork heads use + // refs/pull//head for creation and a separate PR-head push target. isCrossRepository?: boolean /** Why: required because the cross-repo view merges items from every selected * repo — the table row's repo pill and the "open in browser" fallback need @@ -869,6 +879,7 @@ export type CreateWorktreeArgs = { sparseCheckout?: CreateSparseCheckoutRequest linkedIssue?: number linkedPR?: number + pushTarget?: GitPushTarget /** Telemetry-only: which UI surface initiated this create. Threaded from * the renderer entry point so main can emit `workspace_created` with the * correct `source`. `unknown` is a valid wire value — an unrecognized