From 203eea1ce9d16af192c4af3e151b26a45e16c385 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 26 Jun 2026 01:31:43 -0700 Subject: [PATCH] Clarify push-rejected errors with remote target diagnostics Provide detailed context on non-fast-forward push rejections by extracting and displaying the target remote URL, branch, and active Git configuration options. This helps developers identify when they are pushing to unexpected destinations (such as a fork) that differ from origin. --- src/main/git/configured-push-remote.ts | 101 ++++++++++++ src/main/git/push-target-diagnostic.ts | 52 ++++++ src/main/git/remote.test.ts | 70 +++++++- src/main/git/remote.ts | 156 +++++++----------- src/relay/git-handler-push-target.test.ts | 23 ++- src/relay/git-handler-push-target.ts | 85 +++++++++- src/relay/git-handler.ts | 10 +- .../lib/source-control-remote-error.test.ts | 12 ++ .../src/lib/source-control-remote-error.ts | 16 ++ src/shared/git-remote-error.test.ts | 44 +++++ src/shared/git-remote-error.ts | 86 +++++++++- 11 files changed, 534 insertions(+), 121 deletions(-) create mode 100644 src/main/git/configured-push-remote.ts create mode 100644 src/main/git/push-target-diagnostic.ts diff --git a/src/main/git/configured-push-remote.ts b/src/main/git/configured-push-remote.ts new file mode 100644 index 00000000000..b358f7682d6 --- /dev/null +++ b/src/main/git/configured-push-remote.ts @@ -0,0 +1,101 @@ +import type { GitRuntimeOptions } from './git-runtime-options' +import { gitOptionsForWorktree } from './git-runtime-options' +import { gitExecFileAsync } from './runner' + +export type ConfiguredPushRemote = { + remote: string + branchRemote: string | null + branchPushRemote: string | null + branchRemoteValue: string | null + remotePushDefault: string | null +} + +export async function getGitConfigValue( + worktreePath: string, + key: string, + options: GitRuntimeOptions = {} +): Promise { + try { + const { stdout } = await gitExecFileAsync( + ['config', '--get', key], + gitOptionsForWorktree(worktreePath, options) + ) + const value = stdout.trim() + return value || null + } catch { + return null + } +} + +function isUrlValuedRemote(remote: string): boolean { + return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) +} + +async function findRemoteNameForUrl( + worktreePath: string, + remoteUrl: string, + options: GitRuntimeOptions = {} +): Promise { + try { + const { stdout } = await gitExecFileAsync( + ['remote'], + gitOptionsForWorktree(worktreePath, options) + ) + const remotes = stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + for (const remoteName of remotes) { + try { + const { stdout: urlStdout } = await gitExecFileAsync( + ['remote', 'get-url', remoteName], + gitOptionsForWorktree(worktreePath, options) + ) + if (urlStdout.trim() === remoteUrl) { + return remoteName + } + } catch { + // Ignore a remote that disappeared or has no fetch URL. + } + } + } catch { + return null + } + return null +} + +async function normalizePushRemote( + worktreePath: string, + remote: string, + options: GitRuntimeOptions = {} +): Promise { + if (!isUrlValuedRemote(remote)) { + return remote + } + return (await findRemoteNameForUrl(worktreePath, remote, options)) ?? remote +} + +export async function getConfiguredPushRemote( + worktreePath: string, + branch: string, + options: GitRuntimeOptions = {} +): Promise { + const [branchRemote, branchPushRemote, remotePushDefault] = await Promise.all([ + getGitConfigValue(worktreePath, `branch.${branch}.remote`, options), + getGitConfigValue(worktreePath, `branch.${branch}.pushRemote`, options), + getGitConfigValue(worktreePath, 'remote.pushDefault', options) + ]) + const remote = branchPushRemote ?? remotePushDefault ?? branchRemote + if (!remote) { + return null + } + return { + remote: await normalizePushRemote(worktreePath, remote, options), + branchRemote: branchRemote + ? await normalizePushRemote(worktreePath, branchRemote, options) + : null, + branchPushRemote, + branchRemoteValue: branchRemote, + remotePushDefault + } +} diff --git a/src/main/git/push-target-diagnostic.ts b/src/main/git/push-target-diagnostic.ts new file mode 100644 index 00000000000..79ff15d3740 --- /dev/null +++ b/src/main/git/push-target-diagnostic.ts @@ -0,0 +1,52 @@ +import type { GitPushTargetDiagnostic } from '../../shared/git-remote-error' +import type { GitRuntimeOptions } from './git-runtime-options' +import { gitOptionsForWorktree } from './git-runtime-options' +import { gitExecFileAsync } from './runner' + +function isUrlValuedRemote(remote: string): boolean { + return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) +} + +async function getRemoteUrl( + worktreePath: string, + remote: string, + options: GitRuntimeOptions = {} +): Promise { + if (isUrlValuedRemote(remote)) { + return remote + } + try { + const { stdout } = await gitExecFileAsync( + ['remote', 'get-url', remote], + gitOptionsForWorktree(worktreePath, options) + ) + const value = stdout.trim() + return value || null + } catch { + return null + } +} + +export async function createLocalPushTargetDiagnostic( + worktreePath: string, + target: Omit, + options: GitRuntimeOptions = {} +): Promise { + if (target.remote === 'origin') { + const originUrl = await getRemoteUrl(worktreePath, 'origin', options) + return { + ...target, + remoteUrl: originUrl, + originUrl + } + } + const [remoteUrl, originUrl] = await Promise.all([ + getRemoteUrl(worktreePath, target.remote, options), + getRemoteUrl(worktreePath, 'origin', options) + ]) + return { + ...target, + remoteUrl, + originUrl + } +} diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index 4e7b27639f0..76324b96753 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -214,16 +214,30 @@ describe('git remote operations', () => { ) expect(gitExecFileAsyncMock.mock.calls).toEqual([ [['check-ref-format', '--branch', 'contributor/fix-sidebar'], { cwd: '/repo' }], + [['remote', 'get-url', 'origin'], { cwd: '/repo' }], [['push', '--set-upstream', 'origin', 'HEAD:contributor/fix-sidebar'], { cwd: '/repo' }] ]) }) it('passes --force-with-lease when requested', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'feature\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: 'origin\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: 'refs/heads/feature\n', stderr: '' }) - .mockResolvedValueOnce({ stdout: '', stderr: '' }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref') { + return { stdout: 'feature\n', stderr: '' } + } + if (args[0] === 'config' && args.includes('branch.feature.remote')) { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'config' && args.includes('branch.feature.pushRemote')) { + throw new Error('missing pushRemote') + } + if (args[0] === 'config' && args.includes('remote.pushDefault')) { + throw new Error('missing pushDefault') + } + if (args[0] === 'config' && args.includes('branch.feature.merge')) { + return { stdout: 'refs/heads/feature\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) await gitPush('/repo', false, undefined, { forceWithLease: true }) @@ -243,6 +257,52 @@ describe('git remote operations', () => { ) }) + it('includes branch-specific URL pushRemote details in non-fast-forward failures', async () => { + const branch = 'fix/cold-start-hides-local-repos' + const forkUrl = 'https://github.com/omarshahine/orca.git' + const originUrl = 'https://github.com/stablyai/orca.git' + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref') { + return { stdout: `${branch}\n`, stderr: '' } + } + if (args[0] === 'config' && args.includes(`branch.${branch}.pushRemote`)) { + return { stdout: `${forkUrl}\n`, stderr: '' } + } + if (args[0] === 'config' && args.includes(`branch.${branch}.remote`)) { + return { stdout: `${forkUrl}\n`, stderr: '' } + } + if (args[0] === 'config' && args.includes('remote.pushDefault')) { + throw new Error('missing pushDefault') + } + if (args[0] === 'config' && args.includes(`branch.${branch}.merge`)) { + return { stdout: `refs/heads/${branch}\n`, stderr: '' } + } + if (args[0] === 'config' && args.includes(`branch.${branch}.base`)) { + throw new Error('missing branch base') + } + if (args[0] === 'remote' && args[1] === 'get-url' && args[2] === 'origin') { + return { stdout: `${originUrl}\n`, stderr: '' } + } + if (args[0] === 'remote' && args.length === 1) { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'push') { + throw new Error( + 'Command failed: git push\n' + ` ! [rejected] HEAD -> ${branch} (fetch first)\n` + ) + } + return { stdout: '', stderr: '' } + }) + + await expect(gitPush('/repo', false)).rejects.toThrow( + 'Push rejected: remote has newer commits (non-fast-forward). ' + + `Target: ${forkUrl} -> ${branch}. ` + + `Branch config: branch.${branch}.pushRemote=${forkUrl}; branch.${branch}.remote=${forkUrl}. ` + + `This differs from origin (${originUrl}). ` + + 'Pull or sync only if this is the intended target; otherwise change the branch remote/pushRemote or publish to the intended remote, then try again.' + ) + }) + it('maps recursive submodule push failures to submodule-specific guidance', async () => { gitExecFileAsyncMock .mockRejectedValueOnce(new Error('no branch')) diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index d2b1c0b5dc4..f1dd6bf9d14 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -1,17 +1,33 @@ -import { normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { + normalizeGitErrorMessage, + type GitPushTargetDiagnostic +} from '../../shared/git-remote-error' import { resolveEffectiveGitUpstream } from '../../shared/git-effective-upstream' import { gitRefTargetsBranchOnRemote } from '../../shared/git-remote-branch-name' import { resolveGitRemoteRebaseSource } from '../../shared/git-rebase-source' import type { GitPushTarget } from '../../shared/types' +import { + getConfiguredPushRemote, + getGitConfigValue, + type ConfiguredPushRemote +} from './configured-push-remote' import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' +import { createLocalPushTargetDiagnostic } from './push-target-diagnostic' import { validateGitPushTarget } from './push-target-validation' import { gitExecFileAsync } from './runner' +type ResolvedPushTarget = { + remote: string + refspec: string + branchName: string | null + diagnostic: GitPushTargetDiagnostic +} + async function getConfiguredPushTarget( worktreePath: string, options: GitRuntimeOptions = {} -): Promise<{ remote: string; refspec: string } | null> { +): Promise { try { const { stdout: branchStdout } = await gitExecFileAsync( ['symbolic-ref', '--quiet', '--short', 'HEAD'], @@ -41,101 +57,26 @@ async function getConfiguredPushTarget( if (!canPushConfiguredMergeBranch(pushRemote, branch, branchRef)) { return null } - return { remote, refspec: `HEAD:${branchRef}` } - } catch { - return null - } -} - -async function getConfigValue( - worktreePath: string, - key: string, - options: GitRuntimeOptions = {} -): Promise { - try { - const { stdout } = await gitExecFileAsync( - ['config', '--get', key], - gitOptionsForWorktree(worktreePath, options) - ) - const value = stdout.trim() - return value || null - } catch { - return null - } -} - -function isUrlValuedRemote(remote: string): boolean { - return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) -} - -type ConfiguredPushRemote = { - remote: string - branchRemote: string | null -} - -async function findRemoteNameForUrl( - worktreePath: string, - remoteUrl: string, - options: GitRuntimeOptions = {} -): Promise { - try { - const { stdout } = await gitExecFileAsync( - ['remote'], - gitOptionsForWorktree(worktreePath, options) - ) - const remotes = stdout - .split(/\r?\n/) - .map((line) => line.trim()) - .filter(Boolean) - for (const remoteName of remotes) { - try { - const { stdout: urlStdout } = await gitExecFileAsync( - ['remote', 'get-url', remoteName], - gitOptionsForWorktree(worktreePath, options) - ) - if (urlStdout.trim() === remoteUrl) { - return remoteName - } - } catch { - // Ignore a remote that disappeared or has no fetch URL. - } + return { + remote, + refspec: `HEAD:${branchRef}`, + branchName: branchRef, + diagnostic: await createLocalPushTargetDiagnostic( + worktreePath, + { + remote, + branchName: branchRef, + currentBranch: branch, + branchPushRemote: pushRemote.branchPushRemote, + branchRemote: pushRemote.branchRemoteValue, + remotePushDefault: pushRemote.remotePushDefault + }, + options + ) } } catch { return null } - return null -} - -async function normalizePushRemote( - worktreePath: string, - remote: string, - options: GitRuntimeOptions = {} -): Promise { - if (!isUrlValuedRemote(remote)) { - return remote - } - return (await findRemoteNameForUrl(worktreePath, remote, options)) ?? remote -} - -async function getConfiguredPushRemote( - worktreePath: string, - branch: string, - options: GitRuntimeOptions = {} -): Promise { - const branchRemote = await getConfigValue(worktreePath, `branch.${branch}.remote`, options) - const remote = - (await getConfigValue(worktreePath, `branch.${branch}.pushRemote`, options)) ?? - (await getConfigValue(worktreePath, 'remote.pushDefault', options)) ?? - branchRemote - if (!remote) { - return null - } - return { - remote: await normalizePushRemote(worktreePath, remote, options), - branchRemote: branchRemote - ? await normalizePushRemote(worktreePath, branchRemote, options) - : null - } } async function branchMergeTargetsConfiguredBase( @@ -146,7 +87,7 @@ async function branchMergeTargetsConfiguredBase( options: GitRuntimeOptions = {} ): Promise { return gitRefTargetsBranchOnRemote( - await getConfigValue(worktreePath, `branch.${branch}.base`, options), + await getGitConfigValue(worktreePath, `branch.${branch}.base`, options), remote, branchRef ) @@ -168,8 +109,25 @@ function canPushConfiguredMergeBranch( return pushRemote.remote !== 'origin' && pushRemote.branchRemote === pushRemote.remote } -function explicitPushTarget(target: GitPushTarget): { remote: string; refspec: string } { - return { remote: target.remoteName, refspec: `HEAD:${target.branchName}` } +async function explicitPushTarget( + worktreePath: string, + target: GitPushTarget, + options: GitRuntimeOptions = {} +): Promise { + return { + remote: target.remoteName, + refspec: `HEAD:${target.branchName}`, + branchName: target.branchName, + diagnostic: await createLocalPushTargetDiagnostic( + worktreePath, + { + remote: target.remoteName, + branchName: target.branchName, + explicit: true + }, + options + ) + } } export async function gitPush( @@ -178,6 +136,7 @@ export async function gitPush( pushTarget?: GitPushTarget, options: { forceWithLease?: boolean } & GitRuntimeOptions = {} ): Promise { + let targetDiagnostic: GitPushTargetDiagnostic | undefined try { if (pushTarget) { await validateGitPushTarget(worktreePath, pushTarget, options) @@ -193,8 +152,9 @@ export async function gitPush( // unaffected because it uses branchCompare against an explicit baseRef // from worktree config, not the upstream relationship. const target = pushTarget - ? explicitPushTarget(pushTarget) + ? await explicitPushTarget(worktreePath, pushTarget, options) : await getConfiguredPushTarget(worktreePath, options) + targetDiagnostic = target?.diagnostic const args = [ 'push', ...(options.forceWithLease ? ['--force-with-lease'] : []), @@ -203,7 +163,7 @@ export async function gitPush( ] await gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)) } catch (error) { - throw new Error(normalizeGitErrorMessage(error, 'push')) + throw new Error(normalizeGitErrorMessage(error, 'push', targetDiagnostic)) } } diff --git a/src/relay/git-handler-push-target.test.ts b/src/relay/git-handler-push-target.test.ts index c040a645a16..b51d7bbb7de 100644 --- a/src/relay/git-handler-push-target.test.ts +++ b/src/relay/git-handler-push-target.test.ts @@ -68,7 +68,7 @@ describe('resolveRelayPushTarget', () => { merge: 'refs/heads/contributor/fix' }) - await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ + await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toMatchObject({ remote: 'fork', refspec: 'HEAD:contributor/fix' }) @@ -96,7 +96,7 @@ describe('resolveRelayPushTarget', () => { base: 'refs/remotes/origin/main' }) - await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ + await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toMatchObject({ remote: 'fork', refspec: 'HEAD:main' }) @@ -109,7 +109,7 @@ describe('resolveRelayPushTarget', () => { branchRemote: 'origin' }) - await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ + await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toMatchObject({ remote: 'fork', refspec: 'HEAD:feature/fix' }) @@ -128,7 +128,7 @@ describe('resolveRelayPushTarget', () => { } }) - await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ + await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toMatchObject({ remote: 'pr-contributor-orca', refspec: 'HEAD:feature/fix' }) @@ -145,10 +145,21 @@ describe('resolveRelayPushTarget', () => { } }) - await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ + const result = await resolveRelayPushTarget(git, '/repo', undefined) + + expect(result).toMatchObject({ remote: forkUrl, refspec: 'HEAD:feature/fix' }) + expect(result?.diagnostic).toMatchObject({ + remote: forkUrl, + branchName: 'feature/fix', + remoteUrl: forkUrl, + currentBranch: 'feature/fix', + branchPushRemote: forkUrl, + branchRemote: forkUrl, + originUrl: 'git@github.com:stablyai/orca.git' + }) }) it('uses an explicit push target without reading branch config', async () => { @@ -159,7 +170,7 @@ describe('resolveRelayPushTarget', () => { remoteName: 'fork', branchName: 'feature/head' }) - ).resolves.toEqual({ + ).resolves.toMatchObject({ remote: 'fork', refspec: 'HEAD:feature/head' }) diff --git a/src/relay/git-handler-push-target.ts b/src/relay/git-handler-push-target.ts index dfbed1a9186..306db716d13 100644 --- a/src/relay/git-handler-push-target.ts +++ b/src/relay/git-handler-push-target.ts @@ -1,5 +1,6 @@ import { assertGitPushTargetShape } from '../shared/git-push-target-validation' import { gitRefTargetsBranchOnRemote } from '../shared/git-remote-branch-name' +import type { GitPushTargetDiagnostic } from '../shared/git-remote-error' import type { GitPushTarget } from '../shared/types' type RelayGit = (args: string[], cwd: string) => Promise<{ stdout: string; stderr: string }> @@ -7,6 +8,8 @@ type RelayGit = (args: string[], cwd: string) => Promise<{ stdout: string; stder export type ResolvedPushTarget = { remote: string refspec: string + branchName: string | null + diagnostic: GitPushTargetDiagnostic } async function getConfiguredPushTarget( @@ -38,7 +41,19 @@ async function getConfiguredPushTarget( if (!canPushConfiguredMergeBranch(pushRemote, branch, branchRef)) { return null } - return { remote, refspec: `HEAD:${branchRef}` } + return { + remote, + refspec: `HEAD:${branchRef}`, + branchName: branchRef, + diagnostic: await createPushTargetDiagnostic(git, worktreePath, { + remote, + branchName: branchRef, + currentBranch: branch, + branchPushRemote: pushRemote.branchPushRemote, + branchRemote: pushRemote.branchRemoteValue, + remotePushDefault: pushRemote.remotePushDefault + }) + } } catch { return null } @@ -65,6 +80,9 @@ function isUrlValuedRemote(remote: string): boolean { type ConfiguredPushRemote = { remote: string branchRemote: string | null + branchPushRemote: string | null + branchRemoteValue: string | null + remotePushDefault: string | null } async function findRemoteNameForUrl( @@ -112,17 +130,21 @@ async function getConfiguredPushRemote( ): Promise { // Why: mirror the local gitPush resolver so SSH worktrees do not drift to a // different target when branch.pushRemote or remote.pushDefault is present. - const branchRemote = await getConfigValue(git, worktreePath, `branch.${branch}.remote`) - const remote = - (await getConfigValue(git, worktreePath, `branch.${branch}.pushRemote`)) ?? - (await getConfigValue(git, worktreePath, 'remote.pushDefault')) ?? - branchRemote + const [branchRemote, branchPushRemote, remotePushDefault] = await Promise.all([ + getConfigValue(git, worktreePath, `branch.${branch}.remote`), + getConfigValue(git, worktreePath, `branch.${branch}.pushRemote`), + getConfigValue(git, worktreePath, 'remote.pushDefault') + ]) + const remote = branchPushRemote ?? remotePushDefault ?? branchRemote if (!remote) { return null } return { remote: await normalizePushRemote(git, worktreePath, remote), - branchRemote: branchRemote ? await normalizePushRemote(git, worktreePath, branchRemote) : null + branchRemote: branchRemote ? await normalizePushRemote(git, worktreePath, branchRemote) : null, + branchPushRemote, + branchRemoteValue: branchRemote, + remotePushDefault } } @@ -169,6 +191,53 @@ export async function resolveRelayPushTarget( await git(['check-ref-format', '--branch', explicitTarget.branchName], worktreePath) return { remote: explicitTarget.remoteName, - refspec: `HEAD:${explicitTarget.branchName}` + refspec: `HEAD:${explicitTarget.branchName}`, + branchName: explicitTarget.branchName, + diagnostic: await createPushTargetDiagnostic(git, worktreePath, { + remote: explicitTarget.remoteName, + branchName: explicitTarget.branchName, + explicit: true + }) + } +} + +async function getRemoteUrl( + git: RelayGit, + worktreePath: string, + remote: string +): Promise { + if (isUrlValuedRemote(remote)) { + return remote + } + try { + const { stdout } = await git(['remote', 'get-url', remote], worktreePath) + const value = stdout.trim() + return value || null + } catch { + return null + } +} + +async function createPushTargetDiagnostic( + git: RelayGit, + worktreePath: string, + target: Omit +): Promise { + if (target.remote === 'origin') { + const originUrl = await getRemoteUrl(git, worktreePath, 'origin') + return { + ...target, + remoteUrl: originUrl, + originUrl + } + } + const [remoteUrl, originUrl] = await Promise.all([ + getRemoteUrl(git, worktreePath, target.remote), + getRemoteUrl(git, worktreePath, 'origin') + ]) + return { + ...target, + remoteUrl, + originUrl } } diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 46dd750f2a0..b8081b802be 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -28,7 +28,11 @@ import { import { refreshLocalBaseRefForWorktreeCreateOp } from './git-handler-local-base-ref-refresh' import { checkIgnoredPathsOp, detectConflictOperation, getStatusOp } from './git-handler-status-ops' import { resolveRelayPushTarget } from './git-handler-push-target' -import { isNoUpstreamError, normalizeGitErrorMessage } from '../shared/git-remote-error' +import { + isNoUpstreamError, + normalizeGitErrorMessage, + type GitPushTargetDiagnostic +} from '../shared/git-remote-error' import { upstreamOnlyCommitsArePatchEquivalent } from '../shared/git-upstream-status' import { assertGitPushTargetShape } from '../shared/git-push-target-validation' import { getPublishTargetStatus, type GitCommandRunner } from '../shared/git-publish-target-status' @@ -647,12 +651,14 @@ export class GitHandler { // 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 + let targetDiagnostic: GitPushTargetDiagnostic | undefined try { const target = await resolveRelayPushTarget( this.git.bind(this), worktreePath, params.pushTarget ) + targetDiagnostic = target?.diagnostic const args = [ 'push', ...(params.forceWithLease === true ? ['--force-with-lease'] : []), @@ -663,7 +669,7 @@ export class GitHandler { } 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. - throw new Error(normalizeGitErrorMessage(error, 'push')) + throw new Error(normalizeGitErrorMessage(error, 'push', targetDiagnostic)) } } diff --git a/src/renderer/src/lib/source-control-remote-error.test.ts b/src/renderer/src/lib/source-control-remote-error.test.ts index 8977168bf56..824c2c94816 100644 --- a/src/renderer/src/lib/source-control-remote-error.test.ts +++ b/src/renderer/src/lib/source-control-remote-error.test.ts @@ -38,4 +38,16 @@ describe('source-control remote error formatting', () => { expect(usedLineSplit).toBe(false) expect(usedCrlfReplace).toBe(false) }) + + it('preserves detailed push target context for publish non-fast-forward errors', () => { + const detailed = + 'Push rejected: remote has newer commits (non-fast-forward). ' + + 'Target: fork (https://gitlab.example.com/team/project.git) -> feature/fix. ' + + 'Branch config: branch.feature/fix.pushRemote=fork; branch.feature/fix.remote=fork. ' + + 'This differs from origin (https://gitlab.example.com/upstream/project.git). ' + + 'Pull or sync only if this is the intended target; otherwise change the branch remote/pushRemote or publish to the intended remote, then try again.' + const error = new Error(`Error invoking remote method 'git:push': Error: ${detailed}`) + + expect(resolveRemoteOperationErrorMessage(error, { publish: true })).toBe(detailed) + }) }) diff --git a/src/renderer/src/lib/source-control-remote-error.ts b/src/renderer/src/lib/source-control-remote-error.ts index 54b3ecebea6..6bd9ac0bacf 100644 --- a/src/renderer/src/lib/source-control-remote-error.ts +++ b/src/renderer/src/lib/source-control-remote-error.ts @@ -67,6 +67,17 @@ function resolveSubmodulePushFailureMessage( return detail ? `${operationLabel} failed. ${truncateDetail(detail)}` : null } +function extractDetailedPushRejection(message: string): string | null { + const cleaned = stripCredentialsFromMessage(message) + const match = cleaned.match( + /Push rejected: remote has newer commits \(non-fast-forward\)\. Target:/i + ) + if (!match?.index && match?.index !== 0) { + return null + } + return cleaned.slice(match.index).trim() +} + function isNonFastForwardRemoteError(error: unknown): boolean { if (!(error instanceof Error)) { return false @@ -163,6 +174,11 @@ export function resolveRemoteOperationErrorMessage( return 'Force push rejected — remote changed since last fetch. Fetch first, then try again.' } + const detailedPushRejection = extractDetailedPushRejection(error.message) + if (detailedPushRejection) { + return detailedPushRejection + } + // Why: non-fast-forward/rejected detection is shared across publish and push so // both paths surface the same actionable toast regardless of operation type. if (/non-fast-forward|fetch first|updates were rejected/i.test(error.message)) { diff --git a/src/shared/git-remote-error.test.ts b/src/shared/git-remote-error.test.ts index 387dbb4aacb..e609c3552d2 100644 --- a/src/shared/git-remote-error.test.ts +++ b/src/shared/git-remote-error.test.ts @@ -53,6 +53,50 @@ describe('normalizeGitErrorMessage', () => { ) expect(usedLineSplit).toBe(false) }) + + it('includes the rejected target and branch-specific remote config for push rejections', () => { + const error = new Error( + 'Command failed: git push\n' + + ' ! [rejected] HEAD -> fix/cold-start-hides-local-repos (fetch first)\n' + ) + + expect( + normalizeGitErrorMessage(error, 'push', { + remote: 'https://github.com/omarshahine/orca.git', + branchName: 'fix/cold-start-hides-local-repos', + remoteUrl: 'https://github.com/omarshahine/orca.git', + currentBranch: 'fix/cold-start-hides-local-repos', + branchPushRemote: 'https://github.com/omarshahine/orca.git', + branchRemote: 'https://github.com/omarshahine/orca.git', + originUrl: 'https://github.com/stablyai/orca.git' + }) + ).toBe( + 'Push rejected: remote has newer commits (non-fast-forward). ' + + 'Target: https://github.com/omarshahine/orca.git -> fix/cold-start-hides-local-repos. ' + + 'Branch config: branch.fix/cold-start-hides-local-repos.pushRemote=https://github.com/omarshahine/orca.git; ' + + 'branch.fix/cold-start-hides-local-repos.remote=https://github.com/omarshahine/orca.git. ' + + 'This differs from origin (https://github.com/stablyai/orca.git). ' + + 'Pull or sync only if this is the intended target; otherwise change the branch remote/pushRemote or publish to the intended remote, then try again.' + ) + }) + + it('scrubs credentials from push target diagnostics', () => { + const error = new Error('Command failed: git push\n ! [rejected] HEAD -> feature (fetch first)') + + const message = normalizeGitErrorMessage(error, 'push', { + remote: 'origin', + branchName: 'feature', + remoteUrl: 'https://token:secret@example.com/repo.git', + currentBranch: 'feature', + branchPushRemote: 'https://token:secret@example.com/repo.git', + originUrl: 'https://user:password@example.com/base.git' + }) + + expect(message).toContain('https://example.com/repo.git') + expect(message).toContain('https://example.com/base.git') + expect(message).not.toContain('secret') + expect(message).not.toContain('password') + }) }) describe('formatSubmodulePushFailureDetail', () => { diff --git a/src/shared/git-remote-error.ts b/src/shared/git-remote-error.ts index 57a256ddd01..add683df0e0 100644 --- a/src/shared/git-remote-error.ts +++ b/src/shared/git-remote-error.ts @@ -81,7 +81,89 @@ function* iterateLinesFromEnd(value: string): Generator { export type GitRemoteOperation = 'push' | 'pull' | 'fetch' | 'upstream' -export function normalizeGitErrorMessage(error: unknown, operation?: GitRemoteOperation): string { +export type GitPushTargetDiagnostic = { + remote: string + branchName: string | null + remoteUrl?: string | null + currentBranch?: string | null + branchPushRemote?: string | null + branchRemote?: string | null + remotePushDefault?: string | null + originUrl?: string | null + explicit?: boolean +} + +function cleanDiagnosticValue(value: string | null | undefined): string | null { + const cleaned = stripCredentialsFromMessage(value ?? '').trim() + return cleaned || null +} + +function formatTargetRemote(diagnostic: GitPushTargetDiagnostic): string { + const remote = cleanDiagnosticValue(diagnostic.remote) ?? 'unknown remote' + const remoteUrl = cleanDiagnosticValue(diagnostic.remoteUrl) + if (!remoteUrl || remoteUrl === remote) { + return remote + } + return `${remote} (${remoteUrl})` +} + +function formatBranchConfigDiagnostic(diagnostic: GitPushTargetDiagnostic): string | null { + const currentBranch = cleanDiagnosticValue(diagnostic.currentBranch) + if (!currentBranch) { + return null + } + const entries: string[] = [] + const branchPushRemote = cleanDiagnosticValue(diagnostic.branchPushRemote) + const branchRemote = cleanDiagnosticValue(diagnostic.branchRemote) + const remotePushDefault = cleanDiagnosticValue(diagnostic.remotePushDefault) + if (branchPushRemote) { + entries.push(`branch.${currentBranch}.pushRemote=${branchPushRemote}`) + } + if (branchRemote) { + entries.push(`branch.${currentBranch}.remote=${branchRemote}`) + } + if (remotePushDefault && !branchPushRemote) { + entries.push(`remote.pushDefault=${remotePushDefault}`) + } + if (entries.length === 0) { + return null + } + + const originUrl = cleanDiagnosticValue(diagnostic.originUrl) + const remoteUrl = cleanDiagnosticValue(diagnostic.remoteUrl) + const differsFromOrigin = + originUrl && + ((remoteUrl && remoteUrl !== originUrl) || + (!remoteUrl && cleanDiagnosticValue(diagnostic.remote) !== 'origin')) + const originDetail = differsFromOrigin ? ` This differs from origin (${originUrl}).` : '' + return `Branch config: ${entries.join('; ')}.${originDetail}` +} + +export function formatPushRejectedRemoteTargetMessage( + diagnostic?: GitPushTargetDiagnostic +): string { + const summary = 'Push rejected: remote has newer commits (non-fast-forward).' + if (!diagnostic) { + return `${summary} Please pull or sync first.` + } + + const targetBranch = cleanDiagnosticValue(diagnostic.branchName) ?? 'HEAD' + const details = [summary, `Target: ${formatTargetRemote(diagnostic)} -> ${targetBranch}.`] + const branchConfig = formatBranchConfigDiagnostic(diagnostic) + if (branchConfig) { + details.push(branchConfig) + } + details.push( + 'Pull or sync only if this is the intended target; otherwise change the branch remote/pushRemote or publish to the intended remote, then try again.' + ) + return details.join(' ') +} + +export function normalizeGitErrorMessage( + error: unknown, + operation?: GitRemoteOperation, + pushTargetDiagnostic?: GitPushTargetDiagnostic +): string { if (!(error instanceof Error)) { return 'Git remote operation failed.' } @@ -107,7 +189,7 @@ export function normalizeGitErrorMessage(error: unknown, operation?: GitRemoteOp (operation === 'push' || operation === undefined) && (raw.includes('non-fast-forward') || raw.includes('fetch first')) ) { - return 'Push rejected: remote has newer commits (non-fast-forward). Please pull or sync first.' + return formatPushRejectedRemoteTargetMessage(pushTargetDiagnostic) } if (raw.includes('could not read Username') || raw.includes('Authentication failed')) {