From 064b1ac8a2437d45d367124aa330b21f613dffa0 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 11 Jun 2026 17:19:21 -0700 Subject: [PATCH] Improve error messaging for git submodule push failures Detect when git push operations fail due to un-pushed or out-of-sync submodules, and extract the specific submodule name to provide clear, actionable guidance to the user. Additionally, update the Source Control error UI to allow better text wrapping and word-breaking for detailed error alerts. --- src/main/git/remote.test.ts | 17 ++++++ .../right-sidebar/SourceControl.tsx | 2 +- src/renderer/src/store/slices/editor.test.ts | 58 +++++++++++++++++++ src/renderer/src/store/slices/editor.ts | 48 ++++++++++++++- src/shared/git-remote-error.test.ts | 44 +++++++++++++- src/shared/git-remote-error.ts | 33 +++++++++++ 6 files changed, 197 insertions(+), 5 deletions(-) diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index e6ebc146533..879fd986165 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -95,6 +95,23 @@ describe('git remote operations', () => { ) }) + it('maps recursive submodule push failures to submodule-specific guidance', async () => { + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('no branch')) + .mockRejectedValueOnce( + new Error( + "Command failed: git push\nPushing submodule 'find-cmux-followers'\n" + + ' ! [rejected] master -> master (fetch first)\n' + + "Unable to push submodule 'find-cmux-followers'\n" + + 'fatal: failed to push all needed submodules' + ) + ) + + await expect(gitPush('/repo', false)).rejects.toThrow( + "Submodule 'find-cmux-followers' has remote changes. Pull inside the submodule, then try again." + ) + }) + it('passes through clean tail line when push error does not match known patterns', async () => { gitExecFileAsyncMock .mockRejectedValueOnce(new Error('no branch')) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 94858bb03ab..5f86a1b45a3 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -5219,7 +5219,7 @@ export function CommitArea({ id="commit-area-remote-error" role="alert" aria-live="polite" - className="mt-1 text-[11px] text-destructive" + className="mt-1 min-w-0 text-[11px] leading-4 break-words text-destructive [overflow-wrap:anywhere]" > {remoteActionError}

diff --git a/src/renderer/src/store/slices/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts index 2d5f96a1391..11d38269717 100644 --- a/src/renderer/src/store/slices/editor.test.ts +++ b/src/renderer/src/store/slices/editor.test.ts @@ -2418,6 +2418,64 @@ describe('createEditorSlice remote branch actions', () => { expect(store.getState().isRemoteOperationActive).toBe(false) }) + it('surfaces submodule push failures with the submodule name', async () => { + const store = createEditorStore() + const pushError = new Error( + "Command failed: git push\nPushing submodule 'find-cmux-followers'\n" + + ' ! [rejected] master -> master (fetch first)\n' + + "Unable to push submodule 'find-cmux-followers'\n" + + 'fatal: failed to push all needed submodules' + ) + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toThrow( + pushError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + "Push failed. Submodule 'find-cmux-followers' has remote changes. Pull inside the submodule, then try again." + ) + await flushAsyncRemoteRefresh() + + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitFetchMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(gitUpstreamStatusMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('surfaces transport-prefixed normalized submodule push failures', async () => { + const store = createEditorStore() + const pushError = new Error( + "Error invoking remote method 'git:push': Error: Submodule 'find-cmux-followers' has remote changes. Pull inside the submodule, then try again." + ) + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toThrow( + pushError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + "Push failed. Submodule 'find-cmux-followers' has remote changes. Pull inside the submodule, then try again." + ) + await flushAsyncRemoteRefresh() + + expect(gitFetchMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(gitUpstreamStatusMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + it('uses a fallback message for generic push errors', async () => { const store = createEditorStore() const pushError = new Error('network timeout') diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 5e6532f4c63..ed5b75804b8 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -29,7 +29,10 @@ import type { WorkspaceSessionState, WorkspaceVisibleTabType } from '../../../../shared/types' -import { stripCredentialsFromMessage } from '../../../../shared/git-remote-error' +import { + formatSubmodulePushFailureDetail, + stripCredentialsFromMessage +} from '../../../../shared/git-remote-error' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' import { folderWorkspaceKey } from '../../../../shared/workspace-scope' import type { RemoteOpKind } from '@/components/right-sidebar/source-control-primary-action' @@ -1112,10 +1115,21 @@ function extractPublishFailureDetail(message: string): string | null { return null } +function resolveSubmodulePushFailureMessage( + message: string, + operationLabel: string +): string | null { + const detail = formatSubmodulePushFailureDetail(message) + return detail ? `${operationLabel} failed. ${truncateDetail(detail)}` : null +} + function isNonFastForwardRemoteError(error: unknown): boolean { + if (!(error instanceof Error)) { + return false + } return ( - error instanceof Error && - /non-fast-forward|fetch first|updates were rejected|stale info/i.test(error.message) + /non-fast-forward|fetch first|updates were rejected|stale info/i.test(error.message) || + formatSubmodulePushFailureDetail(error.message)?.includes('has remote changes') === true ) } @@ -1153,6 +1167,34 @@ export function resolveRemoteOperationErrorMessage( : 'Pull stopped with merge conflicts. Resolve them in Source Control, then commit the merge.' } + if (options?.publish) { + const submoduleMessage = resolveSubmodulePushFailureMessage(error.message, 'Publish Branch') + if (submoduleMessage) { + return submoduleMessage + } + } + + if (options?.isSync) { + const submoduleMessage = resolveSubmodulePushFailureMessage(error.message, 'Sync') + if (submoduleMessage) { + return submoduleMessage + } + } + + if (options?.isForcePush) { + const submoduleMessage = resolveSubmodulePushFailureMessage(error.message, 'Force Push') + if (submoduleMessage) { + return submoduleMessage + } + } + + if (options?.isPush) { + const submoduleMessage = resolveSubmodulePushFailureMessage(error.message, 'Push') + if (submoduleMessage) { + return submoduleMessage + } + } + // Why: under sync, the inner push runs *after* a successful pull, so a // non-fast-forward at that point means the remote raced ahead between // fetch and push — not "user forgot to pull". Saying "Pull first" would diff --git a/src/shared/git-remote-error.test.ts b/src/shared/git-remote-error.test.ts index 7a069e6f117..8afdad1d5c1 100644 --- a/src/shared/git-remote-error.test.ts +++ b/src/shared/git-remote-error.test.ts @@ -1,5 +1,47 @@ import { describe, expect, it } from 'vitest' -import { isNoUpstreamError } from './git-remote-error' +import { + formatSubmodulePushFailureDetail, + isNoUpstreamError, + normalizeGitErrorMessage +} from './git-remote-error' + +describe('normalizeGitErrorMessage', () => { + it('keeps the submodule name when a recursive push is rejected', () => { + const error = new Error( + "Command failed: git push\nPushing submodule 'find-cmux-followers'\n" + + 'To https://github.com/stablyai/orca-internal\n' + + ' ! [rejected] master -> master (fetch first)\n' + + "Unable to push submodule 'find-cmux-followers'\n" + + 'fatal: failed to push all needed submodules' + ) + + expect(normalizeGitErrorMessage(error, 'push')).toBe( + "Submodule 'find-cmux-followers' has remote changes. Pull inside the submodule, then try again." + ) + }) +}) + +describe('formatSubmodulePushFailureDetail', () => { + it('keeps normalized guidance when transport layers prefix the error', () => { + expect( + formatSubmodulePushFailureDetail( + "Error invoking remote method 'git:push': Error: Submodule 'vendor/tools' has remote changes. Pull inside the submodule, then try again." + ) + ).toBe( + "Submodule 'vendor/tools' has remote changes. Pull inside the submodule, then try again." + ) + }) + + it('falls back to submodule-specific guidance when git omits the nested reason', () => { + expect( + formatSubmodulePushFailureDetail( + "Unable to push submodule 'vendor/tools'\nfatal: failed to push all needed submodules" + ) + ).toBe( + "Submodule 'vendor/tools' could not be pushed. Resolve the submodule push error, then try again." + ) + }) +}) describe('isNoUpstreamError', () => { it('treats a missing HEAD@{u} tracking ref as no upstream', () => { diff --git a/src/shared/git-remote-error.ts b/src/shared/git-remote-error.ts index 07bf656c92d..b130013efd1 100644 --- a/src/shared/git-remote-error.ts +++ b/src/shared/git-remote-error.ts @@ -9,11 +9,39 @@ // scrubbing passwords on any scheme and HTTPS token-only forms. const USERPASS_URL_PATTERN = /([a-z][a-z0-9+.-]*:\/\/)[^\s/@:]+:[^\s/@]+@/gi const HTTPS_TOKEN_URL_PATTERN = /(https?:\/\/)[^\s/@:]+@/gi +const SUBMODULE_PUSH_FAILURE_PATTERN = /Unable to push submodule ['"](.+?)['"]/i +const SUBMODULE_PUSH_FAILURE_SENTINEL_PATTERN = + /failed to push all needed submodules|Unable to push submodule/i +const SUBMODULE_REMOTE_CHANGED_PATTERN = + /non-fast-forward|fetch first|updates were rejected|remote contains work that you do not have/i +const NORMALIZED_SUBMODULE_PUSH_FAILURE_PATTERN = + /(?:^|:\s)((?:Submodule '[^'\n]+'|A submodule) (?:has remote changes\. Pull inside the submodule, then try again\.|could not be pushed\. Resolve the submodule push error, then try again\.))(?:$|\s)/i export function stripCredentialsFromMessage(message: string): string { return message.replace(USERPASS_URL_PATTERN, '$1').replace(HTTPS_TOKEN_URL_PATTERN, '$1') } +export function formatSubmodulePushFailureDetail(message: string): string | null { + const raw = stripCredentialsFromMessage(message) + const normalized = raw.replace(/\r\n/g, '\n').trim() + const normalizedMatch = normalized.match(NORMALIZED_SUBMODULE_PUSH_FAILURE_PATTERN) + if (normalizedMatch) { + return normalizedMatch[1] + } + if (!SUBMODULE_PUSH_FAILURE_SENTINEL_PATTERN.test(normalized)) { + return null + } + + // Why: recursive push can hide the actionable nested rejection behind a + // top-level "failed to push all needed submodules" fatal line. + const submoduleName = normalized.match(SUBMODULE_PUSH_FAILURE_PATTERN)?.[1]?.trim() + const subject = submoduleName ? `Submodule '${submoduleName}'` : 'A submodule' + if (SUBMODULE_REMOTE_CHANGED_PATTERN.test(normalized)) { + return `${subject} has remote changes. Pull inside the submodule, then try again.` + } + return `${subject} could not be pushed. Resolve the submodule push error, then try again.` +} + function extractTailLine(message: string): string { // Why: execFile rejections prefix the message with "Command failed: git ..." // followed by the full stderr. The meaningful diagnostic is typically the @@ -39,6 +67,11 @@ export function normalizeGitErrorMessage(error: unknown, operation?: GitRemoteOp // literals today, but this hardens against accidental leakage later. const raw = stripCredentialsFromMessage(error.message) + const submodulePushFailureDetail = formatSubmodulePushFailureDetail(raw) + if ((operation === 'push' || operation === undefined) && submodulePushFailureDetail) { + return submodulePushFailureDetail + } + // Why: `non-fast-forward` / `fetch first` can appear on fetch (after a // remote force-push updating a tracking ref) and on pull (with // `pull.ff=only`), so the "pull or sync first" guidance only makes sense