From b2eb055a9c4ac2438702110ce607d0ef4e1709b2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:09:35 -0700 Subject: [PATCH] refactor(ipc): make the canonical envelope stripper the only one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #17230 added stripIpcInvokeEnvelope as a canonical home for Electron's IPC wrapper. It was the sixth implementation, not the first: nine other files carried ten hand-rolled copies, and they disagreed. Measured against a shared corpus, the copies split four ways. An envelope whose tail has no "Error: " prefix — Electron builds that tail from the main side's error.toString(), so a rejected non-Error produces one — was left fully visible by quick-open. A message-less handler failure rendered the bare word "Error" in the Linux recovery card, the voice download toast and the AI Vault scan row; an empty tail rendered an empty string in all three. Every copy was anchored at ^, so an envelope a caller had prefixed with its own context stayed on screen. The two AccountsPane copies were scoped to their own channel, so a rejection from any other channel kept its wrapper. All ten now route through one stripper, moved to src/shared because ai-vault-scan-error-message is imported by main and cannot reach a renderer-only module. The canonical regex additionally covers "Error occurred in handler for", which AccountsPane stripped and the canonical one did not, and a separate stripErrorClassPrefix keeps the bare "Error: " trim that three sites had — that prefix is Error.prototype.toString(), not the envelope, and an existing Linux card test caught its loss. Nothing is swallowed. The three sites that gained a null fallback now console.warn the original rejection, which none of them logged before, and Electron still logs the handler's original error with its stack in main. extractIpcErrorMessage keeps its own fail-open regex and its 18 call sites are untouched: it returns the tail verbatim and never returns null. A census test pins the envelope to the two files that own it, so a seventh copy fails CI. It lists nine offenders against the pre-fix tree. --- .../LinuxPackageInstallRecoveryCard.test.tsx | 35 ++++++ .../LinuxPackageInstallRecoveryCard.tsx | 14 ++- .../editor/rich-markdown-image-insert.ts | 2 +- .../editor/rich-markdown-ipc-error-message.ts | 7 -- .../editor/rich-markdown-paste-image.ts | 2 +- .../components/editor/useLocalImagePick.ts | 2 +- .../components/quick-open-file-list.test.ts | 39 +++++++ .../src/components/quick-open-file-list.ts | 13 ++- .../file-explorer-inline-rename-flow.test.tsx | 1 - .../right-sidebar/useFileDuplicate.ts | 14 +-- .../useFileExplorerInlineInput.ts | 3 +- .../right-sidebar/useFileExplorerMoveDrop.ts | 9 +- .../src/components/settings/AccountsPane.tsx | 53 +-------- .../settings/VoiceSpeechModelSection.test.tsx | 51 +++++++++ .../settings/VoiceSpeechModelSection.tsx | 17 ++- .../account-sign-in-error-copy.test.ts | 106 ++++++++++++++++++ .../settings/account-sign-in-error-copy.ts | 52 +++++++++ src/renderer/src/i18n/locales/en.json | 9 +- src/renderer/src/lib/ipc-error.ts | 25 ++--- src/renderer/src/lib/rename-file.ts | 14 +-- .../ai-vault-scan-error-message.test.ts | 24 ++++ src/shared/ai-vault-scan-error-message.ts | 11 +- .../ipc-invoke-envelope-single-source.test.ts | 52 +++++++++ src/shared/ipc-invoke-envelope.test.ts | 82 ++++++++++++++ src/shared/ipc-invoke-envelope.ts | 50 +++++++++ 25 files changed, 561 insertions(+), 126 deletions(-) delete mode 100644 src/renderer/src/components/editor/rich-markdown-ipc-error-message.ts create mode 100644 src/renderer/src/components/settings/account-sign-in-error-copy.test.ts create mode 100644 src/renderer/src/components/settings/account-sign-in-error-copy.ts create mode 100644 src/shared/ipc-invoke-envelope-single-source.test.ts create mode 100644 src/shared/ipc-invoke-envelope.test.ts create mode 100644 src/shared/ipc-invoke-envelope.ts diff --git a/src/renderer/src/components/LinuxPackageInstallRecoveryCard.test.tsx b/src/renderer/src/components/LinuxPackageInstallRecoveryCard.test.tsx index 0c44cf153e5..19ae96fdff1 100644 --- a/src/renderer/src/components/LinuxPackageInstallRecoveryCard.test.tsx +++ b/src/renderer/src/components/LinuxPackageInstallRecoveryCard.test.tsx @@ -271,6 +271,41 @@ describe('LinuxPackageInstallRecoveryCard copy action', () => { expect(writeClipboardText).not.toHaveBeenCalled() }) + // Why: this envelope has no `Error: ` after the channel, and it is not anchored at the start + // of the message — both shapes used to reach the user with the wrapper still on screen. + it('strips an unprefixed and a mid-string envelope', async () => { + getInstructions.mockRejectedValue( + new Error( + "Update failed: Error invoking remote method 'updater:getLinuxPackageInstallInstructions': disk full" + ) + ) + renderCard() + + fireEvent.click(button('Copy Install Command')) + await flushActions() + + expect(footnoteText()).toBe('Update failed: disk full') + }) + + // Why: a message-less handler failure used to render the bare word "Error" as the footnote. + it('falls back to a sentence, and logs the cause, when the envelope carried no reason', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const rejection = new Error( + "Error invoking remote method 'updater:getLinuxPackageInstallInstructions': Error" + ) + getInstructions.mockRejectedValue(rejection) + renderCard() + + fireEvent.click(button('Copy Install Command')) + await flushActions() + + expect(footnoteText()).toBe( + 'That step failed, and the failure did not include a readable reason.' + ) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('unreadable'), rejection) + warn.mockRestore() + }) + it('keeps the copy path when the artifact fails revalidation', async () => { getInstructions.mockRejectedValue( new Error('Error: The downloaded package no longer matches the verified release.') diff --git a/src/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx b/src/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx index 69a0f73d627..3c998ed67b8 100644 --- a/src/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx +++ b/src/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx @@ -5,6 +5,7 @@ import type { } from '../../../shared/update-status-types' import { UpdateErrorCardContent } from './UpdateErrorCardContent' import { translate } from '@/i18n/i18n' +import { stripErrorClassPrefix, stripIpcInvokeEnvelopeFrom } from '@/lib/ipc-error' const COPY_CONFIRMATION_MS = 4_000 @@ -19,9 +20,16 @@ function copiedNote(packageFileName: string): string { } function toMessage(error: unknown): string { - const message = String((error as Error)?.message ?? error) - // Electron prefixes rejected invoke() results with the channel; keep only the user-safe tail. - return message.replace(/^Error invoking remote method '[^']*':\s*/, '').replace(/^Error:\s*/, '') + const reason = stripIpcInvokeEnvelopeFrom(error) + if (reason === null) { + // Why: the envelope carried no reason, so this log is the only surviving record of it. + console.warn('[linux-package-install] recovery action failed with an unreadable error', error) + return translate( + 'auto.components.LinuxPackageInstallRecoveryCard.unreadableActionError', + 'That step failed, and the failure did not include a readable reason.' + ) + } + return stripErrorClassPrefix(reason) } export function LinuxPackageInstallRecoveryCard({ diff --git a/src/renderer/src/components/editor/rich-markdown-image-insert.ts b/src/renderer/src/components/editor/rich-markdown-image-insert.ts index 7051502f45c..223c7f5e09d 100644 --- a/src/renderer/src/components/editor/rich-markdown-image-insert.ts +++ b/src/renderer/src/components/editor/rich-markdown-image-insert.ts @@ -9,7 +9,7 @@ import { settingsForRuntimeOwner } from '@/runtime/runtime-rpc-client' import { captureDirectSshMutationExpectation } from '@/lib/ssh-mutation-expectation' import { translate } from '@/i18n/i18n' import { parseWorkspaceKey } from '../../../../shared/workspace-scope' -import { extractIpcErrorMessage } from './rich-markdown-ipc-error-message' +import { extractIpcErrorMessage } from '@/lib/ipc-error' export type RichMarkdownImageInsertArgs = { editor: Editor diff --git a/src/renderer/src/components/editor/rich-markdown-ipc-error-message.ts b/src/renderer/src/components/editor/rich-markdown-ipc-error-message.ts deleted file mode 100644 index d6831598372..00000000000 --- a/src/renderer/src/components/editor/rich-markdown-ipc-error-message.ts +++ /dev/null @@ -1,7 +0,0 @@ -export function extractIpcErrorMessage(err: unknown, fallback: string): string { - if (!(err instanceof Error)) { - return fallback - } - const match = err.message.match(/Error invoking remote method '[^']*': (?:Error: )?(.+)/) - return match ? match[1] : err.message -} diff --git a/src/renderer/src/components/editor/rich-markdown-paste-image.ts b/src/renderer/src/components/editor/rich-markdown-paste-image.ts index c34e7ad100d..d9944f6ac4e 100644 --- a/src/renderer/src/components/editor/rich-markdown-paste-image.ts +++ b/src/renderer/src/components/editor/rich-markdown-paste-image.ts @@ -3,7 +3,7 @@ import { toast } from 'sonner' import { getConnectionId } from '@/lib/connection-context' import { useAppStore } from '@/store' import { settingsForRuntimeOwner } from '@/runtime/runtime-rpc-client' -import { extractIpcErrorMessage } from './rich-markdown-ipc-error-message' +import { extractIpcErrorMessage } from '@/lib/ipc-error' import { insertRichMarkdownImageFromPath } from './rich-markdown-image-insert' export type RichMarkdownImagePasteArgs = { diff --git a/src/renderer/src/components/editor/useLocalImagePick.ts b/src/renderer/src/components/editor/useLocalImagePick.ts index bf988ef7da2..8e278f78543 100644 --- a/src/renderer/src/components/editor/useLocalImagePick.ts +++ b/src/renderer/src/components/editor/useLocalImagePick.ts @@ -2,7 +2,7 @@ import { useCallback } from 'react' import type { Editor } from '@tiptap/react' import { toast } from 'sonner' import { insertRichMarkdownImageFromPath } from './rich-markdown-image-insert' -import { extractIpcErrorMessage } from './rich-markdown-ipc-error-message' +import { extractIpcErrorMessage } from '@/lib/ipc-error' export function useLocalImagePick( editor: Editor | null, diff --git a/src/renderer/src/components/quick-open-file-list.test.ts b/src/renderer/src/components/quick-open-file-list.test.ts index be5370b4d8c..743dc9fd7a1 100644 --- a/src/renderer/src/components/quick-open-file-list.test.ts +++ b/src/renderer/src/components/quick-open-file-list.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from 'vitest' import type { Worktree } from '../../../shared/worktree/types' import { buildExcludePathPrefixes } from '../../../shared/quick-open-filter' +import { vi } from 'vitest' import { + cleanRuntimeFileListError, getRuntimeFileListTarget, getNestedWorktreeExcludePaths, getNestedWorktreeExcludeRequest, @@ -70,3 +72,40 @@ describe('quick-open nested worktree excludes', () => { }) }) }) + +describe('cleanRuntimeFileListError', () => { + it('shows only the reason behind the IPC envelope', () => { + expect( + cleanRuntimeFileListError( + new Error("Error invoking remote method 'fs:listFiles': Error: Permission denied") + ) + ).toBe('Permission denied') + }) + + // Why: this envelope has no `Error: ` after the channel, which the previous local regex + // required — it rendered the whole wrapper to the user. + it('strips an envelope whose tail carries no Error: prefix', () => { + expect( + cleanRuntimeFileListError( + new Error("Error invoking remote method 'fs:listFiles': Permission denied") + ) + ).toBe('Permission denied') + }) + + it('falls back to a sentence, and logs the cause, when the envelope carried no reason', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const error = new Error("Error invoking remote method 'fs:listFiles': Error") + + expect(cleanRuntimeFileListError(error)).toBe( + 'Orca could not list files here, and the failure did not include a readable reason.' + ) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('unreadable'), error) + warn.mockRestore() + }) + + it('never renders the envelope, whatever the channel', () => { + expect( + cleanRuntimeFileListError(new Error("Error invoking remote method 'x:y': Error: nope")) + ).not.toContain('invoking remote method') + }) +}) diff --git a/src/renderer/src/components/quick-open-file-list.ts b/src/renderer/src/components/quick-open-file-list.ts index 04490ed7af6..19228087bb6 100644 --- a/src/renderer/src/components/quick-open-file-list.ts +++ b/src/renderer/src/components/quick-open-file-list.ts @@ -4,6 +4,8 @@ import { useShallow } from 'zustand/react/shallow' import type { Worktree } from '../../../shared/worktree/types' import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' import { createBrowserUuid } from '@/lib/browser-uuid' +import { translate } from '@/i18n/i18n' +import { stripIpcInvokeEnvelope } from '@/lib/ipc-error' import { isQuickOpenRemoteQueryTooLarge } from '@/components/quick-open-search' import { cancelRuntimeFileList, @@ -30,7 +32,16 @@ export type RuntimeFileListState = { export function cleanRuntimeFileListError(error: unknown): string { const raw = error instanceof Error ? error.message : String(error) - return raw.replace(/^Error invoking remote method '[^']+':\s*Error:\s*/, '') + const reason = stripIpcInvokeEnvelope(raw) + if (reason === null) { + // Why: the envelope carried no reason, so this log is the only surviving record of it. + console.warn('[quick-open] runtime file list failed with an unreadable error', error) + return translate( + 'auto.components.QuickOpen.unreadableListError', + 'Orca could not list files here, and the failure did not include a readable reason.' + ) + } + return reason } function debounceRuntimeFilePathSearch( diff --git a/src/renderer/src/components/right-sidebar/file-explorer-inline-rename-flow.test.tsx b/src/renderer/src/components/right-sidebar/file-explorer-inline-rename-flow.test.tsx index 479e25f9190..3fc5bbbe331 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-inline-rename-flow.test.tsx +++ b/src/renderer/src/components/right-sidebar/file-explorer-inline-rename-flow.test.tsx @@ -21,7 +21,6 @@ vi.mock('@/store', () => ({ })) vi.mock('@/lib/rename-file', () => ({ - extractIpcErrorMessage: vi.fn(), renameFileOnDisk: mocks.renameFileOnDisk })) diff --git a/src/renderer/src/components/right-sidebar/useFileDuplicate.ts b/src/renderer/src/components/right-sidebar/useFileDuplicate.ts index 1b7a19f9ca6..cab6157b175 100644 --- a/src/renderer/src/components/right-sidebar/useFileDuplicate.ts +++ b/src/renderer/src/components/right-sidebar/useFileDuplicate.ts @@ -4,19 +4,7 @@ import { basename, dirname, joinPath } from '@/lib/path' import type { TreeNode } from './file-explorer-types' import { copyRuntimePath, runtimePathExists } from '@/runtime/runtime-file-client' import { captureFileExplorerOperationGuard } from './file-explorer-operation-owner' - -/** - * Electron's ipcRenderer.invoke wraps errors as: - * "Error invoking remote method 'channel': Error: actual message" - * Strip the wrapper so users see only the meaningful part. - */ -function extractIpcErrorMessage(err: unknown, fallback: string): string { - if (!(err instanceof Error)) { - return fallback - } - const match = err.message.match(/Error invoking remote method '[^']*': (?:Error: )?(.+)/) - return match ? match[1] : err.message -} +import { extractIpcErrorMessage } from '@/lib/ipc-error' type UseFileDuplicateParams = { activeWorktreeId: string | null diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerInlineInput.ts b/src/renderer/src/components/right-sidebar/useFileExplorerInlineInput.ts index 467e21bd42d..45667c4e00c 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerInlineInput.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerInlineInput.ts @@ -4,7 +4,8 @@ import { toast } from 'sonner' import { useAppStore } from '@/store' import { detectLanguage } from '@/lib/language-detect' import { dirname, joinPath } from '@/lib/path' -import { extractIpcErrorMessage, renameFileOnDisk } from '@/lib/rename-file' +import { renameFileOnDisk } from '@/lib/rename-file' +import { extractIpcErrorMessage } from '@/lib/ipc-error' import type { InlineInput } from './file-explorer-inline-input-row' import type { TreeNode } from './file-explorer-types' import type { FileExplorerRowProjection } from './file-explorer-row-projection' diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerMoveDrop.ts b/src/renderer/src/components/right-sidebar/useFileExplorerMoveDrop.ts index b917353240b..3be69f6f8ff 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerMoveDrop.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerMoveDrop.ts @@ -5,14 +5,7 @@ import { executeOpenEditorPathMove } from '@/lib/execute-open-editor-path-move' import { commitFileExplorerOp } from './fileExplorerUndoRedo' import type { FileExplorerOperationOwner } from './file-explorer-types' import { captureFileExplorerOperationGuard } from './file-explorer-operation-owner' - -function extractIpcErrorMessage(err: unknown, fallback: string): string { - if (!(err instanceof Error)) { - return fallback - } - const match = err.message.match(/Error invoking remote method '[^']*': (?:Error: )?(.+)/) - return match ? match[1] : err.message -} +import { extractIpcErrorMessage } from '@/lib/ipc-error' type UseFileExplorerMoveDropParams = { worktreePath: string | null diff --git a/src/renderer/src/components/settings/AccountsPane.tsx b/src/renderer/src/components/settings/AccountsPane.tsx index c778a982aa0..0efc93e696f 100644 --- a/src/renderer/src/components/settings/AccountsPane.tsx +++ b/src/renderer/src/components/settings/AccountsPane.tsx @@ -85,6 +85,11 @@ import { translate } from '@/i18n/i18n' import { formatUiRelativeTime } from '@/i18n/relative-time-format' import { cn } from '@/lib/utils' import { isWebClientLocation } from '@/lib/web-client-location' +import { + getClaudeAccountErrorDescription, + getCodexAccountErrorDescription, + isClaudeAccountCancellation +} from './account-sign-in-error-copy' import { emptyClaudeAccountsState, emptyCodexAccountsState, @@ -265,54 +270,6 @@ function getClaudeAccountRuntimeLabel( return hostLabel } -function getCodexAccountErrorDescription(error: unknown): string { - const message = String((error as Error)?.message ?? error) - .replace(/^Error occurred in handler for 'codexAccounts:[^']+':\s*/i, '') - .replace(/^Error invoking remote method 'codexAccounts:[^']+':\s*/i, '') - .replace(/^Error:\s*/i, '') - .trim() - const normalizedMessage = message.toLowerCase() - - // Why: Codex account actions cross the Electron IPC boundary, and invoke() - // failures often include transport-level wrapper text that is useful in - // devtools but noisy in product UI. Normalize the handful of expected auth - // failures here so users see actionable sign-in guidance instead of IPC - // internals or raw upstream wording. - if (normalizedMessage.includes('timed out waiting for codex login to finish')) { - return 'Codex sign-in took too long to finish. Please try again.' - } - if (normalizedMessage.includes('codex sign-in took too long to finish')) { - return 'Codex sign-in took too long to finish. Please try again.' - } - if ( - normalizedMessage.includes('auth error 502') || - normalizedMessage.includes('gateway') || - normalizedMessage.includes('bad gateway') - ) { - return 'Codex sign-in is temporarily unavailable. Please try again in a minute.' - } - if (normalizedMessage.startsWith('codex login failed:')) { - const loginMessage = message.slice('Codex login failed:'.length).trim() - return loginMessage || 'Codex sign-in failed. Please try again.' - } - - return message || 'Codex sign-in failed. Please try again.' -} - -function getClaudeAccountErrorDescription(error: unknown): string { - return ( - String((error as Error)?.message ?? error) - .replace(/^Error occurred in handler for 'claudeAccounts:[^']+':\s*/i, '') - .replace(/^Error invoking remote method 'claudeAccounts:[^']+':\s*/i, '') - .replace(/^Error:\s*/i, '') - .trim() || 'Claude sign-in failed. Please try again.' - ) -} - -function isClaudeAccountCancellation(error: unknown): boolean { - return getClaudeAccountErrorDescription(error).toLowerCase() === 'claude sign-in was cancelled.' -} - type LocalAccountRuntime = { runtime: 'host' | 'wsl' wslDistro?: string | null diff --git a/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx b/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx index 4825397fbaf..bc58d4fb5d0 100644 --- a/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx +++ b/src/renderer/src/components/settings/VoiceSpeechModelSection.test.tsx @@ -226,6 +226,57 @@ describe('VoiceSpeechModelSection', () => { root.unmount() }) + it('shows only the reason behind the IPC envelope when a download fails', async () => { + const { container, root } = renderSection({ + deleteModel: () => Promise.resolve(), + downloadModel: () => + Promise.reject( + new Error( + "Error invoking remote method 'speech:downloadModel': Error: net::ERR_CONTENT_LENGTH_MISMATCH" + ) + ), + modelStates: [{ id: localModel.id, status: 'not-downloaded' }] + }) + + await act(async () => { + container + .querySelector('[role="option"]')! + .dispatchEvent(new MouseEvent('click', { bubbles: true })) + await Promise.resolve() + }) + + expect(toastErrorMock).toHaveBeenCalledWith('Failed to download model.', { + description: 'net::ERR_CONTENT_LENGTH_MISMATCH' + }) + root.unmount() + }) + + // Why: an envelope with no reason behind it used to render the bare word "Error" as the + // toast description, and the cause was not recorded anywhere. + it('falls back to a sentence, and logs the cause, when the envelope carried no reason', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const rejection = new Error("Error invoking remote method 'speech:downloadModel': Error") + const { container, root } = renderSection({ + deleteModel: () => Promise.resolve(), + downloadModel: () => Promise.reject(rejection), + modelStates: [{ id: localModel.id, status: 'not-downloaded' }] + }) + + await act(async () => { + container + .querySelector('[role="option"]')! + .dispatchEvent(new MouseEvent('click', { bubbles: true })) + await Promise.resolve() + }) + + expect(toastErrorMock).toHaveBeenCalledWith('Failed to download model.', { + description: 'The download failed, and the failure did not include a readable reason.' + }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('unreadable'), rejection) + warn.mockRestore() + root.unmount() + }) + it('keeps the model menu open when starting a local model download', async () => { const { container, root } = renderSection({ deleteModel: () => Promise.resolve(), diff --git a/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx b/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx index 31c1595a937..d585570cd86 100644 --- a/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx +++ b/src/renderer/src/components/settings/VoiceSpeechModelSection.tsx @@ -15,12 +15,21 @@ import { } from '../ui/dropdown-menu' import { Cloud, Download, Trash2, Loader2, ChevronDown, Check } from 'lucide-react' import { translate } from '@/i18n/i18n' +import { stripIpcInvokeEnvelopeFrom } from '@/lib/ipc-error' function describeSpeechModelDownloadError(error: unknown): string { - const message = error instanceof Error ? error.message : String(error) - // Why: ipcRenderer.invoke wraps main-process rejections; strip the transport - // prefix so the toast shows only the underlying download failure. - return message.replace(/^Error invoking remote method '[^']+': (?:Error: )?/, '') + // Why: the raw cause (e.g. net::ERR_CONTENT_LENGTH_MISMATCH) is the only diagnosable + // signal users can report back, so only the transport envelope is removed. + const reason = stripIpcInvokeEnvelopeFrom(error) + if (reason === null) { + // Why: the envelope carried no reason, so this log is the only surviving record of it. + console.warn('[voice] speech model download failed with an unreadable error', error) + return translate( + 'auto.components.settings.VoicePane.unreadableDownloadError', + 'The download failed, and the failure did not include a readable reason.' + ) + } + return reason } type VoiceSpeechModelSectionProps = { diff --git a/src/renderer/src/components/settings/account-sign-in-error-copy.test.ts b/src/renderer/src/components/settings/account-sign-in-error-copy.test.ts new file mode 100644 index 00000000000..7de47f48575 --- /dev/null +++ b/src/renderer/src/components/settings/account-sign-in-error-copy.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from 'vitest' +import { + getClaudeAccountErrorDescription, + getCodexAccountErrorDescription, + isClaudeAccountCancellation +} from './account-sign-in-error-copy' + +describe('getCodexAccountErrorDescription', () => { + it('shows only the reason behind the IPC envelope', () => { + expect( + getCodexAccountErrorDescription( + new Error("Error invoking remote method 'codexAccounts:login': Error: Seat limit reached") + ) + ).toBe('Seat limit reached') + }) + + // Why: the previous regex was scoped to the codexAccounts channel, so a rejection that + // reached this toast from any other channel kept its wrapper on screen. + it('strips the envelope whatever channel it names', () => { + expect( + getCodexAccountErrorDescription( + new Error("Error invoking remote method 'settings:update': Error: Seat limit reached") + ) + ).toBe('Seat limit reached') + }) + + it('still maps a known auth failure that arrived wrapped', () => { + expect( + getCodexAccountErrorDescription( + new Error("Error invoking remote method 'codexAccounts:login': Error: Auth error 502") + ) + ).toBe('Codex sign-in is temporarily unavailable. Please try again in a minute.') + }) + + it('still unwraps the codex login prefix behind the envelope', () => { + expect( + getCodexAccountErrorDescription( + new Error( + "Error invoking remote method 'codexAccounts:login': Error: Codex login failed: bad token" + ) + ) + ).toBe('bad token') + }) + + it('falls back when the envelope carried no reason', () => { + expect( + getCodexAccountErrorDescription( + new Error("Error invoking remote method 'codexAccounts:login': Error") + ) + ).toBe('Codex sign-in failed. Please try again.') + }) + + it('keeps trimming a bare Error: prefix that never crossed IPC', () => { + expect(getCodexAccountErrorDescription(new Error('Error: Seat limit reached'))).toBe( + 'Seat limit reached' + ) + }) +}) + +describe('getClaudeAccountErrorDescription', () => { + it('shows only the reason behind the IPC envelope', () => { + expect( + getClaudeAccountErrorDescription( + new Error("Error invoking remote method 'claudeAccounts:login': Error: Token expired") + ) + ).toBe('Token expired') + }) + + it('strips the envelope whatever channel it names', () => { + expect( + getClaudeAccountErrorDescription( + new Error("Error invoking remote method 'settings:update': Error: Token expired") + ) + ).toBe('Token expired') + }) + + it('falls back when the envelope carried no reason', () => { + expect( + getClaudeAccountErrorDescription( + new Error("Error invoking remote method 'claudeAccounts:login': Error") + ) + ).toBe('Claude sign-in failed. Please try again.') + }) +}) + +describe('isClaudeAccountCancellation', () => { + // Why: cancellation is matched on the stripped text, so the envelope must be gone before + // the comparison — otherwise a cancelled sign-in raises an error toast. + it('recognizes a cancellation that arrived inside the envelope', () => { + expect( + isClaudeAccountCancellation( + new Error( + "Error invoking remote method 'claudeAccounts:login': Error: Claude sign-in was cancelled." + ) + ) + ).toBe(true) + }) + + it('does not treat an unrelated failure as a cancellation', () => { + expect( + isClaudeAccountCancellation( + new Error("Error invoking remote method 'claudeAccounts:login': Error: Token expired") + ) + ).toBe(false) + }) +}) diff --git a/src/renderer/src/components/settings/account-sign-in-error-copy.ts b/src/renderer/src/components/settings/account-sign-in-error-copy.ts new file mode 100644 index 00000000000..84ee79297bb --- /dev/null +++ b/src/renderer/src/components/settings/account-sign-in-error-copy.ts @@ -0,0 +1,52 @@ +import { stripErrorClassPrefix, stripIpcInvokeEnvelopeFrom } from '@/lib/ipc-error' + +/** + * User-facing copy for a provider sign-in failure. + * + * Sign-in crosses Electron IPC, so a rejection arrives wrapped in Electron's envelope. That + * wrapper is transport, not a reason, and is removed by the canonical stripper before any of + * the wording below is matched. Nothing is discarded: the caller still surfaces the message, + * and Electron logs the handler's original error with its stack in the main process. + */ +export function getCodexAccountErrorDescription(error: unknown): string { + // Why: a bare `Error: ` prefix is not the IPC envelope — it survives a main-side error whose + // own message starts that way — so it is trimmed separately from the transport wrapper. + const message = stripErrorClassPrefix(stripIpcInvokeEnvelopeFrom(error) ?? '').trim() + const normalizedMessage = message.toLowerCase() + + // Why: Codex account actions cross the Electron IPC boundary, and invoke() + // failures often include transport-level wrapper text that is useful in + // devtools but noisy in product UI. Normalize the handful of expected auth + // failures here so users see actionable sign-in guidance instead of IPC + // internals or raw upstream wording. + if (normalizedMessage.includes('timed out waiting for codex login to finish')) { + return 'Codex sign-in took too long to finish. Please try again.' + } + if (normalizedMessage.includes('codex sign-in took too long to finish')) { + return 'Codex sign-in took too long to finish. Please try again.' + } + if ( + normalizedMessage.includes('auth error 502') || + normalizedMessage.includes('gateway') || + normalizedMessage.includes('bad gateway') + ) { + return 'Codex sign-in is temporarily unavailable. Please try again in a minute.' + } + if (normalizedMessage.startsWith('codex login failed:')) { + const loginMessage = message.slice('Codex login failed:'.length).trim() + return loginMessage || 'Codex sign-in failed. Please try again.' + } + + return message || 'Codex sign-in failed. Please try again.' +} + +export function getClaudeAccountErrorDescription(error: unknown): string { + return ( + stripErrorClassPrefix(stripIpcInvokeEnvelopeFrom(error) ?? '').trim() || + 'Claude sign-in failed. Please try again.' + ) +} + +export function isClaudeAccountCancellation(error: unknown): boolean { + return getClaudeAccountErrorDescription(error).toLowerCase() === 'claude sign-in was cancelled.' +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 64d54b3e3fb..2192ef6c2d6 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -1860,7 +1860,8 @@ "b227d88520": "{{value0}} files found", "995be8ea22": "Copy", "cf144856dc": "Copied", - "344f8a48dd": "on the host running the Quick Open scan to enable fast, gitignore-aware listing:" + "344f8a48dd": "on the host running the Quick Open scan to enable fast, gitignore-aware listing:", + "unreadableListError": "Orca could not list files here, and the failure did not include a readable reason." }, "SelectedTextCopyMenu": { "9b40d7b018": "Copy" @@ -8631,7 +8632,8 @@ "ad5d036ecc": "Could not request microphone permission. Voice dictation was not enabled.", "f9a9cf6928": "Microphone permission is required before enabling voice dictation.", "1eac933202": "Opened macOS Privacy & Security. Enable dictation again after granting access.", - "cd9fe37556": "Microphone permission granted" + "cd9fe37556": "Microphone permission granted", + "unreadableDownloadError": "The download failed, and the failure did not include a readable reason." }, "WorktreeSymlinksSection": { "1c1e35b219": "Remove {{value0}}", @@ -16275,7 +16277,8 @@ "53c4b8e148": "No usable authentication agent answered the privileged install request.", "c732bcbf8f": "Checking package...", "aa57fa4f80": "Command copied. Run it in a system terminal to install {{value0}}, then quit and reopen Orca.", - "b7e7c5bc95": "Orca checks the downloaded file against the release metadata at the moment it builds this command. The system package itself is not signature-checked, and Orca cannot vouch for the file after that point." + "b7e7c5bc95": "Orca checks the downloaded file against the release metadata at the moment it builds this command. The system package itself is not signature-checked, and Orca cannot vouch for the file after that point.", + "unreadableActionError": "That step failed, and the failure did not include a readable reason." }, "pr-check-counts": { "passingChip": "{{value0}} passing", diff --git a/src/renderer/src/lib/ipc-error.ts b/src/renderer/src/lib/ipc-error.ts index 270fb6c7e35..5bef0ba6f12 100644 --- a/src/renderer/src/lib/ipc-error.ts +++ b/src/renderer/src/lib/ipc-error.ts @@ -2,26 +2,15 @@ * Electron's ipcRenderer.invoke wraps errors as: * "Error invoking remote method 'channel': Error: actual message" * Strip the wrapper so users see only the meaningful part. + * + * The envelope itself is described once in `shared` because main-side callers strip it too. */ -// Why: the renderer builds this from the main side's `error.toString()`, so a handler that -// threw a message-less error arrives as a bare class name ("…': Error") — the tail can be -// empty even though the envelope is present. Global because a caller may have prefixed it. -const IPC_INVOKE_ENVELOPE = /Error invoking remote method '[^']*'(?::[ \t]*(?:\w*Error:[ \t]*)?)?/g -const BARE_ERROR_CLASS_RESIDUE = /^\w*Error:?$/ - -/** - * The failure behind the IPC envelope, or null when the envelope carried no readable reason. - * Callers that must never render plumbing branch on null instead of falling back to the - * wrapper text — which is what `extractIpcErrorMessage` does. - */ -export function stripIpcInvokeEnvelope(message: string): string | null { - const stripped = message.replace(IPC_INVOKE_ENVELOPE, '').trim() - if (stripped === '' || BARE_ERROR_CLASS_RESIDUE.test(stripped)) { - return null - } - return stripped -} +export { + stripErrorClassPrefix, + stripIpcInvokeEnvelope, + stripIpcInvokeEnvelopeFrom +} from '../../../shared/ipc-invoke-envelope' export function extractIpcErrorMessage(err: unknown, fallback: string): string { if (!(err instanceof Error)) { diff --git a/src/renderer/src/lib/rename-file.ts b/src/renderer/src/lib/rename-file.ts index 71161349e13..4e31f5a62cf 100644 --- a/src/renderer/src/lib/rename-file.ts +++ b/src/renderer/src/lib/rename-file.ts @@ -1,4 +1,5 @@ import { toast } from 'sonner' +import { extractIpcErrorMessage } from '@/lib/ipc-error' import { basename, dirname, joinPath } from '@/lib/path' import { commitFileExplorerOp } from '@/components/right-sidebar/fileExplorerUndoRedo' import { executeOpenEditorPathMove } from '@/lib/execute-open-editor-path-move' @@ -8,19 +9,6 @@ import { } from '@/components/right-sidebar/file-explorer-operation-owner' import type { FileExplorerOperationOwner } from '@/components/right-sidebar/file-explorer-types' -/** - * Electron's ipcRenderer.invoke wraps errors as: - * "Error invoking remote method 'channel': Error: actual message" - * Strip the wrapper so users see only the meaningful part. - */ -export function extractIpcErrorMessage(err: unknown, fallback: string): string { - if (!(err instanceof Error)) { - return fallback - } - const match = err.message.match(/Error invoking remote method '[^']*': (?:Error: )?(.+)/) - return match ? match[1] : err.message -} - type RenameFileArgs = { oldPath: string /** just the new filename (no directory) */ diff --git a/src/shared/ai-vault-scan-error-message.test.ts b/src/shared/ai-vault-scan-error-message.test.ts index 45e3a3e8bf7..368821cfbb4 100644 --- a/src/shared/ai-vault-scan-error-message.test.ts +++ b/src/shared/ai-vault-scan-error-message.test.ts @@ -87,3 +87,27 @@ describe('describeAiVaultScanError', () => { ) }) }) + +describe('describeAiVaultScanError IPC envelope handling', () => { + it('humanizes a scanner error that arrived inside the envelope', () => { + expect( + describeAiVaultScanError( + "Error invoking remote method 'aiVault:listSessions': Error: AI Vault service restart circuit is open." + ) + ).toBe('Session scanning paused after repeated failures. Refresh to try again.') + }) + + // Why: this used to surface the bare class name "Error" as the scan-issue row. + it('replaces an envelope carrying no reason with an actionable line', () => { + expect( + describeAiVaultScanError("Error invoking remote method 'aiVault:listSessions': Error") + ).toBe('The session scan failed without a readable reason. Refresh to try again.') + }) + + // Why: this used to paint an empty row. + it('replaces an empty envelope tail with an actionable line', () => { + expect(describeAiVaultScanError("Error invoking remote method 'aiVault:listSessions': ")).toBe( + 'The session scan failed without a readable reason. Refresh to try again.' + ) + }) +}) diff --git a/src/shared/ai-vault-scan-error-message.ts b/src/shared/ai-vault-scan-error-message.ts index 55244933efc..a5d2527b59a 100644 --- a/src/shared/ai-vault-scan-error-message.ts +++ b/src/shared/ai-vault-scan-error-message.ts @@ -4,10 +4,10 @@ * row and the thrown-rejection banner. Anything unrecognized passes through, so * scanner-authored messages (host name, remote path, cap) keep their own wording. */ +import { stripIpcInvokeEnvelope } from './ipc-invoke-envelope' + const RETRY = 'Refresh to try again.' -/** Electron wraps every rejected `ipcMain.handle` before the renderer sees it. */ -const IPC_INVOKE_PREFIX = /^Error invoking remote method '[^']*': (?:\w*Error: )?/ /** Relay-hosted scanner errors carry a transport-only `Relay ` prefix. */ const RELAY_PREFIX = /^Relay (?=AI Vault )/ /** Both the fork-based service and the legacy worker thread emit this family. */ @@ -45,6 +45,11 @@ function humanize(text: string): string | null { /** Returns user-facing copy, or the original text when it is already meaningful. */ export function describeAiVaultScanError(raw: string): string { - const text = raw.replace(IPC_INVOKE_PREFIX, '').replace(RELAY_PREFIX, '') + const stripped = stripIpcInvokeEnvelope(raw) + if (stripped === null) { + // Why: an envelope with no reason behind it used to surface as "Error" or an empty row. + return `The session scan failed without a readable reason. ${RETRY}` + } + const text = stripped.replace(RELAY_PREFIX, '') return humanize(text) ?? text } diff --git a/src/shared/ipc-invoke-envelope-single-source.test.ts b/src/shared/ipc-invoke-envelope-single-source.test.ts new file mode 100644 index 00000000000..36434a4d222 --- /dev/null +++ b/src/shared/ipc-invoke-envelope-single-source.test.ts @@ -0,0 +1,52 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join, relative, sep } from 'node:path' +import { describe, expect, it } from 'vitest' + +// Why: the envelope is one wire format, and every hand-rolled copy of it is a chance to +// disagree. Six independent copies had already drifted — some kept a trailing `Error:` class +// name, some scoped themselves to a single channel, and three rendered "Error" or an empty +// string when the envelope carried no reason. This census is what stops a seventh appearing. +const ENVELOPE_OWNERS = [ + // The canonical stripper. The only place the wire format is described. + 'src/shared/ipc-invoke-envelope.ts', + // `extractIpcErrorMessage` keeps its own fail-open regex on purpose: it returns the tail + // verbatim (class name included) and never returns null, which 18 call sites rely on. + 'src/renderer/src/lib/ipc-error.ts' +] + +const REPO_ROOT = join(__dirname, '..', '..') + +function listSourceFiles(dir: string): string[] { + return readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const fullPath = join(dir, entry.name) + if (entry.isDirectory()) { + return listSourceFiles(fullPath) + } + if (!/\.(ts|tsx)$/.test(entry.name) || /\.test\.(ts|tsx)$/.test(entry.name)) { + return [] + } + return [fullPath] + }) +} + +// Why: strip comments first, so a file that only *documents* the envelope (the terminal toast +// explains that its markers arrive IPC-wrapped) is not counted as a second implementation. +function stripComments(source: string): string { + return source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/(^|[^:])\/\/.*$/gm, '$1') +} + +// Why: match the bare Electron names rather than a quoted-channel shape — the canonical file +// spells them as an alternation, and a future copy could too. Prose lives in comments, which +// are stripped above, so any surviving mention is code that knows the wire format. +const ENVELOPE_MATCHER = /Error invoking remote method|Error occurred in handler for/ + +describe('IPC invoke envelope has a single source of truth', () => { + it('is described in exactly the files that own it', () => { + const offenders = listSourceFiles(join(REPO_ROOT, 'src')) + .filter((file) => ENVELOPE_MATCHER.test(stripComments(readFileSync(file, 'utf8')))) + .map((file) => relative(REPO_ROOT, file).split(sep).join('/')) + .sort() + + expect(offenders).toEqual([...ENVELOPE_OWNERS].sort()) + }) +}) diff --git a/src/shared/ipc-invoke-envelope.test.ts b/src/shared/ipc-invoke-envelope.test.ts new file mode 100644 index 00000000000..5c2ab169463 --- /dev/null +++ b/src/shared/ipc-invoke-envelope.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest' +import { stripIpcInvokeEnvelope, stripIpcInvokeEnvelopeFrom } from './ipc-invoke-envelope' + +describe('stripIpcInvokeEnvelope', () => { + it('returns the reason behind the invoke envelope', () => { + expect( + stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile': Error: Access denied") + ).toBe('Access denied') + }) + + // Why: Electron builds the tail from the main side's `error.toString()`, which has no + // `Error: ` prefix when the handler rejected with a plain value. + it('strips an envelope whose tail carries no Error: prefix', () => { + expect( + stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile': Access denied") + ).toBe('Access denied') + }) + + it('strips an Error subclass name from the tail', () => { + expect( + stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile': TypeError: boom") + ).toBe('boom') + }) + + // Why: the null branch is what stops a bare class name or an empty toast reaching a user. + it('returns null when the envelope carried no readable reason', () => { + expect(stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile': Error")).toBeNull() + expect(stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile': ")).toBeNull() + expect(stripIpcInvokeEnvelope("Error invoking remote method 'fs:readFile':")).toBeNull() + }) + + // Why: callers prefix the envelope with their own context, so anchoring at ^ would leave + // the whole wrapper on screen. The caller's prefix is kept; only the wrapper goes. + it('strips an envelope a caller has prefixed, keeping the prefix', () => { + expect( + stripIpcInvokeEnvelope( + "SSH connection failed: Error invoking remote method 'ssh:connect': Error: relay missing" + ) + ).toBe('SSH connection failed: relay missing') + }) + + // Why: Electron's `replyWithError` logs this name in main. Covered so a message that has + // crossed either boundary reads the same. + it('strips the main-side handler envelope', () => { + expect( + stripIpcInvokeEnvelope( + "Error occurred in handler for 'claudeAccounts:login': Error: Signed out" + ) + ).toBe('Signed out') + }) + + it('passes through a message that never crossed IPC', () => { + expect(stripIpcInvokeEnvelope('Access denied')).toBe('Access denied') + }) + + it('is not scoped to any channel', () => { + expect( + stripIpcInvokeEnvelope("Error invoking remote method 'codexAccounts:login': Error: nope") + ).toBe('nope') + expect( + stripIpcInvokeEnvelope("Error invoking remote method 'worktrees:remove': Error: nope") + ).toBe('nope') + }) +}) + +describe('stripIpcInvokeEnvelopeFrom', () => { + it('reads an Error message', () => { + expect( + stripIpcInvokeEnvelopeFrom(new Error("Error invoking remote method 'x': Error: boom")) + ).toBe('boom') + }) + + it('reads a rejected non-Error value', () => { + expect(stripIpcInvokeEnvelopeFrom('boom')).toBe('boom') + }) + + // Why: String(undefined) is "undefined", which is not a reason and must not reach copy. + it('returns null for a nullish rejection rather than printing "undefined"', () => { + expect(stripIpcInvokeEnvelopeFrom(undefined)).toBeNull() + expect(stripIpcInvokeEnvelopeFrom(null)).toBeNull() + }) +}) diff --git a/src/shared/ipc-invoke-envelope.ts b/src/shared/ipc-invoke-envelope.ts new file mode 100644 index 00000000000..bfa00c4bbb3 --- /dev/null +++ b/src/shared/ipc-invoke-envelope.ts @@ -0,0 +1,50 @@ +/** + * Electron names a rejected `ipcMain.handle` twice, and neither name is product copy. + * + * The renderer rethrows as "Error invoking remote method '': ", where the tail + * is the main side's `error.toString()`. Main's own console additionally carries + * "Error occurred in handler for '':" — see Electron's `replyWithError`. Both shapes + * are stripped here so a message that has crossed either boundary reads the same. + * + * Lives in `shared` rather than the renderer because main-side callers strip the same envelope. + */ + +// Why: the tail is `error.toString()`, so a handler that threw a message-less error arrives as a +// bare class name ("…': Error") — the tail can be empty even though the envelope is present. +// Global and unanchored because a caller may have prefixed the envelope with its own context. +const IPC_ENVELOPE = + /(?:Error invoking remote method|Error occurred in handler for) '[^']*'(?::[ \t]*(?:\w*Error:[ \t]*)?)?/g +const BARE_ERROR_CLASS_RESIDUE = /^\w*Error:?$/ + +/** + * The failure behind the IPC envelope, or null when the envelope carried no readable reason. + * Callers that must never render plumbing branch on null instead of falling back to the + * wrapper text — which is what `extractIpcErrorMessage` does. + */ +export function stripIpcInvokeEnvelope(message: string): string | null { + const stripped = message.replace(IPC_ENVELOPE, '').trim() + if (stripped === '' || BARE_ERROR_CLASS_RESIDUE.test(stripped)) { + return null + } + return stripped +} + +/** + * `Error.prototype.toString()` renders "Error: ", so a rejection that was stringified + * rather than read through `.message` arrives with a class prefix that is not part of the reason. + * Separate from the envelope: a message can carry this prefix without ever crossing IPC. + */ +export function stripErrorClassPrefix(text: string): string { + return text.replace(/^Error:[ \t]*/i, '') +} + +/** + * Same contract for a caller holding an unknown rejection rather than a string. A nullish + * rejection has no reason at all, so it takes the null branch rather than printing "undefined". + */ +export function stripIpcInvokeEnvelopeFrom(error: unknown): string | null { + if (error === null || error === undefined) { + return null + } + return stripIpcInvokeEnvelope(String((error as Error).message ?? error)) +}