mirror of
https://github.com/stablyai/orca.git
synced 2026-10-15 16:02:49 +00:00
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.
This commit is contained in:
@@ -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'))
|
||||
|
||||
@@ -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}
|
||||
</p>
|
||||
|
||||
@@ -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')
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user