diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 6157af7d36d..b08ac4a67f8 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -223,8 +223,12 @@ import { type TerminalPasteSource, type TerminalPasteTextOptions } from './terminal-paste-coordinator' +import { stripIpcInvokeEnvelope } from '@/lib/ipc-error' import { appendTerminalErrorMessage } from './terminal-error-accumulation' -import { formatTerminalPasteExecutionError } from './terminal-paste-errors' +import { + formatClipboardImagePasteError, + formatTerminalPasteExecutionError +} from './terminal-paste-errors' import { resolveTerminalPasteRuntime } from './terminal-paste-runtime' import { getTerminalPasteSshRemotePlatform } from './terminal-paste-ssh-platform' import { @@ -302,11 +306,6 @@ function TerminalQuickCommandEditorDialog({ ) } -function formatClipboardImagePasteError(error: unknown): string { - const detail = error instanceof Error ? error.message : String(error) - return `Image paste failed: ${detail}` -} - function TerminalPane( { tabId, @@ -493,6 +492,11 @@ function TerminalPane( setSessionStateSaveFailureOpen(true) return } + // Why: the surface renders the reason without Electron's IPC envelope, so the wrapped form — + // which names the channel that failed — has to reach the log from here instead. + if (stripIpcInvokeEnvelope(message) !== message) { + console.warn('[terminal] pane error reached the error surface IPC-wrapped; raw:', message) + } setTerminalError((prev) => appendTerminalErrorMessage(prev, message)) }) /** Dismissal is the only signal that the user has seen the surface, so it must also release the transports' repeat-suppression memory. */ diff --git a/src/renderer/src/components/terminal-pane/terminal-error-accumulation.ts b/src/renderer/src/components/terminal-pane/terminal-error-accumulation.ts index 719d022507e..9b05a20b6de 100644 --- a/src/renderer/src/components/terminal-pane/terminal-error-accumulation.ts +++ b/src/renderer/src/components/terminal-pane/terminal-error-accumulation.ts @@ -1,3 +1,6 @@ +import { translate } from '@/i18n/i18n' +import { stripIpcInvokeEnvelope } from '@/lib/ipc-error' + // Why: the error surface aggregates every pane error into ONE newline-joined // string so TerminalErrorToast's per-line filters (isSshReconnectOwnedTerminalError, // stripSshReconnectOwnedErrorLines) keep working. That join makes line-based @@ -13,10 +16,34 @@ function containsWholeLineRun(accumulated: string, message: string): boolean { ) } +/** + * The reason a pane error carries, with Electron's IPC envelope removed. + * + * Several producers hand this surface `err.message` straight off a rejected `ipcRenderer.invoke` + * (retained-legacy attach, both deferred-reattach paths, deferred attach), so the envelope reaches + * the toast even though every direct `extractIpcErrorMessage` call site is already clean. Stripping + * at the accumulator rather than at each producer is what makes the guarantee hold for a producer + * nobody has written yet — the value cannot reach the toast without passing through here. + * + * An envelope with nothing behind it (a message-less rejection arrives as a bare class name) has no + * reason to show, so it renders copy that says so rather than the plumbing. The raw form is logged + * by the caller, which is outside React's state updater. + */ +function readableTerminalErrorMessage(message: string): string { + return ( + stripIpcInvokeEnvelope(message) ?? + translate( + 'auto.components.terminal.pane.terminalErrorAccumulation.unreadableTerminalError', + 'The terminal reported a failure, and it did not include a readable reason.' + ) + ) +} + /** Appends an error to the aggregated surface, keeping the first occurrence of an already-present message. */ export function appendTerminalErrorMessage(accumulated: string | null, message: string): string { + const readable = readableTerminalErrorMessage(message) if (!accumulated) { - return message + return readable } - return containsWholeLineRun(accumulated, message) ? accumulated : `${accumulated}\n${message}` + return containsWholeLineRun(accumulated, readable) ? accumulated : `${accumulated}\n${readable}` } diff --git a/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts b/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts new file mode 100644 index 00000000000..bb0a6d617fb --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-error-surface-census.test.ts @@ -0,0 +1,75 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * A census keyed on the surface rather than on the stripper. + * + * `ipc-error-call-site-census.test.ts` enumerates callers of `extractIpcErrorMessage`. That census + * is structurally unable to see this leak: the terminal error toast is fed through an accumulator, + * so a producer's raw `err.message` becomes one line of a newline-joined blob and never passes + * through a site that census inspects. Listing the accumulator there would have fixed one entry and + * left the shape — a value buffered before display — just as invisible. + * + * So this enumerates the doors into `terminalError`, the state the toast renders, and records how + * each one is made safe. Adding a door fails this test until its verdict is written down. That is + * what catches the next one: `formatClipboardImagePasteError` was a real envelope leak into this + * same surface that bypassed the accumulator entirely, and no stripper-keyed census could see it. + * + * What this cannot see, stated rather than implied: it is scoped to one surface. The same shape + * exists elsewhere (`use-task-page-gitlab-fetch.ts` buffers raw rejection messages into `errs` and + * renders `errs[0]`; `use-discard-confirmation.ts` renders `errors[0].message`), and nothing here + * looks at those. It also reads source text, so it sees the call, not the value. + */ +const SURFACE_DOORS: Readonly> = { + 'setTerminalError(null)': 'clears the surface', + 'setTerminalError((prev) => appendTerminalErrorMessage(prev, message))': + 'accumulator — strips the IPC envelope for every pane-error producer', + 'setTerminalError((prev) => (prev && isTerminalZeroDimensionsDiagnostic(prev) ? null : prev))': + 'derived from the value already on the surface', + 'setTerminalError(formatTerminalPasteExecutionError(execution.reason))': + 'total map from a paste-reason enum to literal copy — no rejection reaches it', + "setTerminalError('Paste failed: clipboard text is too large for a safe terminal paste.')": + 'literal copy', + 'setTerminalError(formatClipboardImagePasteError(error))': + 'strips the IPC envelope — clipboard:saveImageAsTempFile rejects through here', + "setTerminalError('Paste failed.')": 'literal copy', + 'setTerminalError(kept)': 'derived from the value already on the surface' +} + +const PANE_SOURCE = join(__dirname, 'TerminalPane.tsx') + +// Why by name and not by regex: the call is written across lines in places, so the argument text is +// normalised to one line before matching rather than the pattern being widened to swallow newlines. +function listSurfaceDoors(source: string): string[] { + const doors: string[] = [] + const marker = 'setTerminalError(' + for (let at = source.indexOf(marker); at !== -1; at = source.indexOf(marker, at + 1)) { + let depth = 0 + let end = at + marker.length - 1 + for (; end < source.length; end += 1) { + if (source[end] === '(') { + depth += 1 + } else if (source[end] === ')') { + depth -= 1 + if (depth === 0) { + break + } + } + } + doors.push(source.slice(at, end + 1).replace(/\s+/g, ' ')) + } + return doors +} + +describe('the terminal error surface', () => { + it('has exactly the enumerated doors, each with a written-down verdict', () => { + const doors = new Set(listSurfaceDoors(readFileSync(PANE_SOURCE, 'utf8'))) + + expect([...doors].sort()).toEqual(Object.keys(SURFACE_DOORS).sort()) + }) + + it('records why every door is safe', () => { + expect(Object.values(SURFACE_DOORS).filter((verdict) => verdict === '')).toEqual([]) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-error-surface-envelope.test.tsx b/src/renderer/src/components/terminal-pane/terminal-error-surface-envelope.test.tsx new file mode 100644 index 00000000000..2eff05a7bed --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-error-surface-envelope.test.tsx @@ -0,0 +1,128 @@ +// @vitest-environment happy-dom + +import { cleanup, render } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { bindAttachRetainedLegacyPty } from './pty-connection/retained-legacy-pty-attach' +import { appendTerminalErrorMessage } from './terminal-error-accumulation' +import { formatClipboardImagePasteError } from './terminal-paste-errors' +import { TerminalErrorToast } from './TerminalErrorToast' +import type { ConnectPanePtySession } from './pty-connection/connect-pane-pty-session' + +vi.mock('@/lib/client-environment-info', () => ({ + resolveClientEnvironmentFooter: vi.fn().mockResolvedValue('') +})) + +afterEach(() => { + cleanup() +}) + +const ENVELOPE_PREFIX = 'Error invoking remote method' +// The exact shape Electron rethrows in the renderer when a `pty:` handler rejects. +const WRAPPED_ATTACH_FAILURE = `Error invoking remote method 'pty:attach': Error: EACCES: permission denied, open '/srv/sessions/pty-7.sock'` +const REASON = "EACCES: permission denied, open '/srv/sessions/pty-7.sock'" + +/** Drives the real producer, then the real accumulator, then renders the real toast. */ +function renderSurfaceFor(rejection: Error): string { + const reported: string[] = [] + const session = { + authoritativeReattachGeneration: 0, + clearPaneMode2031State: () => {}, + clearHiddenOutputRestoreState: () => {}, + captureTransportOutputCallbacks: () => ({ generation: 1, callbacks: {} }), + transport: { + attach: () => { + throw rejection + }, + getPtyId: () => null + }, + bindActivePanePty: () => {}, + registerPaneSerializerFor: () => {}, + reportError: (message: string) => { + reported.push(message) + } + } as unknown as ConnectPanePtySession + + bindAttachRetainedLegacyPty(session) + expect(session.attachRetainedLegacyPty('pty-7')).toBe(false) + expect(reported).toHaveLength(1) + + const accumulated = reported.reduce( + (previous, message) => appendTerminalErrorMessage(previous, message), + null + ) + expect(accumulated).not.toBeNull() + + const { container } = render( + {}} /> + ) + return container.textContent ?? '' +} + +describe('the terminal error surface', () => { + it('renders the reason a wrapped attach failure carries, not Electron’s envelope', () => { + const rendered = renderSurfaceFor(new Error(WRAPPED_ATTACH_FAILURE)) + + expect(rendered).toContain(REASON) + expect(rendered).not.toContain(ENVELOPE_PREFIX) + expect(rendered).not.toContain("'pty:attach'") + }) + + it('renders copy rather than the envelope when the rejection carried no reason', () => { + // Absent is not empty: a message-less rejection arrives as a bare class name behind the + // envelope, so the tail is present and non-empty yet still carries no reason to show. + const rendered = renderSurfaceFor(new Error(`Error invoking remote method 'pty:attach': Error`)) + + expect(rendered).not.toContain(ENVELOPE_PREFIX) + expect(rendered).toContain('did not include a readable reason') + }) + + it('keeps the whole multi-line reason a wrapped git failure carries', () => { + // Why here: the accumulator joins with newlines, so a multi-line reason is the case its own + // dedup comment calls out — and it only reaches this surface intact now that the stripper + // no longer stops at the first newline. + const multiline = 'fatal: could not read Username\nfatal: Authentication failed' + const rendered = renderSurfaceFor( + new Error(`Error invoking remote method 'pty:attach': Error: ${multiline}`) + ) + + expect(rendered).toContain('fatal: could not read Username') + expect(rendered).toContain('fatal: Authentication failed') + expect(rendered).not.toContain(ENVELOPE_PREFIX) + }) +}) + +describe('the clipboard-image paste door into the same surface', () => { + // Why a separate door: this path calls setTerminalError directly, so the accumulator never sees + // the value. A fix that only covered the accumulator would leave this envelope on screen. + const WRAPPED_STAGING_FAILURE = new Error( + `Error invoking remote method 'clipboard:saveImageAsTempFile': Error: ENOSPC: no space left on device` + ) + + it('renders the staging reason, not Electron’s envelope', () => { + const { container } = render( + {}} + /> + ) + + expect(container.textContent).toContain('ENOSPC: no space left on device') + expect(container.textContent).not.toContain(ENVELOPE_PREFIX) + }) + + it('renders copy rather than the envelope when the staging rejection carried no reason', () => { + const message = formatClipboardImagePasteError( + new Error(`Error invoking remote method 'clipboard:saveImageAsTempFile': Error`) + ) + + expect(message).not.toContain(ENVELOPE_PREFIX) + expect(message).toContain('did not include a readable reason') + }) + + it('still shows the reason a non-Error rejection carries', () => { + // Absent is not empty: a nullish rejection has no reason, but a thrown string does. + expect(formatClipboardImagePasteError('disk is read-only')).toContain('disk is read-only') + expect(formatClipboardImagePasteError(null)).toContain('did not include a readable reason') + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-menu-paste.ts b/src/renderer/src/components/terminal-pane/terminal-pane-menu-paste.ts index 295b7e35c53..470a7b5076a 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-menu-paste.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-menu-paste.ts @@ -10,7 +10,10 @@ import { type TerminalPasteSource, type TerminalPasteTextOptions } from './terminal-paste-coordinator' -import { formatTerminalPasteExecutionError } from './terminal-paste-errors' +import { + formatClipboardImagePasteError, + formatTerminalPasteExecutionError +} from './terminal-paste-errors' import { resolveTerminalPasteRuntime } from './terminal-paste-runtime' import { getTerminalPasteSshRemotePlatform } from './terminal-paste-ssh-platform' import { isTerminalPanePasteTargetCurrent } from './terminal-paste-target-state' @@ -141,10 +144,7 @@ export const pasteTerminalPaneMenuClipboard = async ( executeTerminalPaneMenuPasteText(context, pane, source, text, options), onTextPasteError: () => onPasteError('Paste failed: clipboard text is too large for a safe terminal paste.'), - onImagePasteError: (error) => { - const detail = error instanceof Error ? error.message : String(error) - onPasteError(`Image paste failed: ${detail}`) - } + onImagePasteError: (error) => onPasteError(formatClipboardImagePasteError(error)) }) if (result.status !== 'pasted') { return diff --git a/src/renderer/src/components/terminal-pane/terminal-paste-errors.ts b/src/renderer/src/components/terminal-pane/terminal-paste-errors.ts index 8a2114cfe93..54e35f36590 100644 --- a/src/renderer/src/components/terminal-pane/terminal-paste-errors.ts +++ b/src/renderer/src/components/terminal-pane/terminal-paste-errors.ts @@ -1,3 +1,5 @@ +import { translate } from '@/i18n/i18n' +import { stripIpcInvokeEnvelopeFrom } from '@/lib/ipc-error' import type { TerminalPasteExecutionReason } from './terminal-paste-model' export function formatTerminalPasteExecutionError( @@ -20,3 +22,28 @@ export function formatTerminalPasteExecutionError( } return 'Paste failed.' } + +/** + * Copy for a clipboard image that could not be staged for the terminal. + * + * The staging step is `clipboard:saveImageAsTempFile`, so a rejection arrives wrapped in Electron's + * IPC envelope. This surface is the terminal error toast, which the pane also reaches through + * `appendTerminalErrorMessage` — but this path calls `setTerminalError` directly, so the envelope + * has to come off here as well. Nothing is discarded: the caller logs the rejection. + */ +export function formatClipboardImagePasteError(error: unknown): string { + // Why the `From` form: a non-Error rejection still carries a reason worth showing, and a nullish + // one carries none at all — absent and empty take the same branch, but neither prints the wrapper. + const detail = stripIpcInvokeEnvelopeFrom(error) + if (detail === null) { + return translate( + 'auto.components.terminal.pane.terminalPasteErrors.imagePasteUnreadable', + 'Image paste failed, and the failure did not include a readable reason.' + ) + } + return translate( + 'auto.components.terminal.pane.terminalPasteErrors.imagePasteFailed', + 'Image paste failed: {{detail}}', + { detail } + ) +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 4e170ac6fc5..a42467da811 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -3234,6 +3234,13 @@ }, "ipcPtyConnect": { "unreadableConnectError": "The terminal could not start, and the failure did not include a readable reason." + }, + "terminalErrorAccumulation": { + "unreadableTerminalError": "The terminal reported a failure, and it did not include a readable reason." + }, + "terminalPasteErrors": { + "imagePasteFailed": "Image paste failed: {{detail}}", + "imagePasteUnreadable": "Image paste failed, and the failure did not include a readable reason." } } }, diff --git a/src/renderer/src/lib/ipc-error-call-site-census.test.ts b/src/renderer/src/lib/ipc-error-call-site-census.test.ts index 263e9633693..a329c25e76b 100644 --- a/src/renderer/src/lib/ipc-error-call-site-census.test.ts +++ b/src/renderer/src/lib/ipc-error-call-site-census.test.ts @@ -109,6 +109,56 @@ function stripComments(source: string): string { // Why: the declaration reads as a call to a plain text match, so it is excluded by name rather // than by regex — a narrower pattern would also drop a real call written across two lines. const DECLARING_MODULE = 'src/renderer/src/lib/ipc-error.ts' +const ENVELOPE_MODULE = 'src/shared/ipc-invoke-envelope.ts' + +/** + * The other half of the population. + * + * A caller that must not fall back to the wrapper text branches on null instead, so it reaches for + * the stripper directly and never appears in CALL_SITES above. Counting only `extractIpcErrorMessage` + * reports 30 and reads as a total; it is not one, and the last person to freeze a number here was + * wrong for exactly this reason. Both lists together are the set of modules that open the envelope. + */ +const DIRECT_STRIPPER_SITES: Readonly> = { + 'src/renderer/src/components/LinuxPackageInstallRecoveryCard.tsx': { + calls: 1, + surface: 'recovery card' + }, + 'src/renderer/src/components/quick-open-file-list.ts': { calls: 1, surface: 'quick-open list' }, + 'src/renderer/src/components/settings/VoiceSpeechModelSection.tsx': { + calls: 1, + surface: 'settings row' + }, + 'src/renderer/src/components/settings/account-sign-in-error-copy.ts': { + calls: 2, + surface: 'settings copy+classifier' + }, + 'src/renderer/src/components/sidebar/worktree-removal-error-copy.ts': { + calls: 2, + surface: 'toast' + }, + 'src/renderer/src/components/terminal-pane/TerminalPane.tsx': { + calls: 1, + surface: 'log only — keeps the wrapped form out of the toast and in the console' + }, + 'src/renderer/src/components/terminal-pane/terminal-error-accumulation.ts': { + calls: 1, + surface: 'terminal error toast, via the accumulator every pane-error producer funnels through' + }, + 'src/renderer/src/components/terminal-pane/terminal-paste-errors.ts': { + calls: 1, + surface: 'terminal error toast' + }, + 'src/shared/ai-vault-scan-error-message.ts': { calls: 1, surface: 'scan error copy' } +} + +function countDirectStripperCalls(source: string): number { + const cleaned = stripComments(source) + return ( + (cleaned.match(/\bstripIpcInvokeEnvelope\(/g)?.length ?? 0) + + (cleaned.match(/\bstripIpcInvokeEnvelopeFrom\(/g)?.length ?? 0) + ) +} function countCalls(source: string): number { return stripComments(source).match(/\bextractIpcErrorMessage\(/g)?.length ?? 0 @@ -134,11 +184,40 @@ describe('extractIpcErrorMessage call sites', () => { expect(found).toEqual(expected) }) + it('are joined by the direct stripper sites, which this reader cannot see', () => { + const found: Record = {} + for (const file of listSourceFiles(join(REPO_ROOT, 'src'))) { + const name = relative(REPO_ROOT, file).split(sep).join('/') + if (name === DECLARING_MODULE || name === ENVELOPE_MODULE) { + continue + } + const calls = countDirectStripperCalls(readFileSync(file, 'utf8')) + if (calls > 0) { + found[name] = calls + } + } + + expect(found).toEqual( + Object.fromEntries( + Object.entries(DIRECT_STRIPPER_SITES).map(([file, { calls }]) => [file, calls]) + ) + ) + }) + + /** + * What neither list can see, stated rather than implied: both are keyed on the stripper, so they + * enumerate sites that already handle the envelope and can never enumerate one that does not. A + * leak is invisible here by construction — the terminal error toast was fed a raw envelope for as + * long as this file has existed. Surfaces that buffer error text before display need a census + * keyed on the surface; `terminal-pane/terminal-error-surface-census.test.ts` is one, for one + * surface. Others (`use-task-page-gitlab-fetch.ts`, `use-discard-confirmation.ts`) have none. + */ // Why: this is the number the freeze note got wrong, so it is asserted rather than described. it('number 30, and all of them render to a user', () => { const total = Object.values(CALL_SITES).reduce((sum, { calls }) => sum + calls, 0) expect(total).toBe(30) expect(Object.values(CALL_SITES).filter(({ surface }) => surface === '')).toEqual([]) + expect(Object.values(DIRECT_STRIPPER_SITES).filter(({ surface }) => surface === '')).toEqual([]) }) })