mirror of
https://github.com/stablyai/orca.git
synced 2026-10-25 16:03:53 +00:00
fix(ipc): strip the invoke envelope from the discard and GitLab list errors
Two surfaces still rendered Electron's `Error invoking remote method '<channel>': ...` wrapper verbatim, both because they buffer a rejection before displaying it rather than reading it at the call site — the shape the stripper-keyed census cannot see. `use-discard-confirmation.ts` retains every discard-all failure in `errors[]` and shows `errors[0].message` as the toast description; `git:bulkUnstage` and `git:discard` reject through IPC, so the wrapper was the description. It now branches on the canonical stripper's null: the title already names the failure, so an envelope with nothing behind it drops the description instead of printing plumbing. `use-task-page-gitlab-fetch.ts` buffers rejections into `errs` and banners `errs[0]`; `gitlab:listIssues` throws out of `assertRegisteredRepo`, so "Access denied: unknown repository path" reached the user wrapped. It now reads through `extractIpcErrorMessage` with readable fallback copy, and still pushes one entry either way — `errs.length` is the pager's `errorCount`, so dropping an unreadable one would retreat the page. Both sites keep logging the original in full. Both censuses are updated, and the note in each now records what they cannot see: enumerating by render sink instead of by stripper finds 118 renderer modules that still hand a raw rejection to a user surface with an IPC-capable producer. That is not closable per call site — the leaking shape is the ordinary idiom. The narrow fix is the boundary: 730 `ipcRenderer.invoke(` calls live in two preload files.
This commit is contained in:
committed by
Merge Sim
parent
ea908de900
commit
fc60163b35
+107
@@ -0,0 +1,107 @@
|
||||
// @vitest-environment happy-dom
|
||||
|
||||
import { act, renderHook } from '@testing-library/react'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { DiscardAllArea } from './discard-all-sequence'
|
||||
import type { SourceControlEntryGroups } from '../listing/section-order'
|
||||
import { useSourceControlDiscardConfirmation } from './use-discard-confirmation'
|
||||
|
||||
const toastError = vi.fn()
|
||||
const bulkUnstage = vi.fn()
|
||||
|
||||
vi.mock('sonner', () => ({ toast: { error: (...args: unknown[]) => toastError(...args) } }))
|
||||
vi.mock('@/i18n/i18n', () => ({
|
||||
translate: (_key: string, fallback: string, vars?: Record<string, unknown>) =>
|
||||
fallback.replace(/\{\{(\w+)\}\}/g, (_m, name: string) => String(vars?.[name] ?? ''))
|
||||
}))
|
||||
vi.mock('@/lib/connection-context', () => ({ getConnectionId: () => null }))
|
||||
vi.mock('@/runtime/runtime-git-client', () => ({
|
||||
bulkUnstageRuntimeGitPaths: (...args: unknown[]) => bulkUnstage(...args)
|
||||
}))
|
||||
|
||||
// Electron rethrows a rejected `ipcMain.handle` as `Error invoking remote method '<channel>': <tail>`,
|
||||
// and `window.api.git.bulkUnstage` / `git.discard` are exactly that. Both discard-all toasts read the
|
||||
// first retained rejection, so the wrapper reached the description verbatim.
|
||||
const ENVELOPED =
|
||||
"Error invoking remote method 'git:bulkUnstage': Error: fatal: pathspec 'src/app.ts' did not match any file(s) known to git"
|
||||
const REASON = "fatal: pathspec 'src/app.ts' did not match any file(s) known to git"
|
||||
const ENVELOPE_ONLY = "Error invoking remote method 'git:bulkUnstage': Error"
|
||||
|
||||
function renderDiscard(overrides: {
|
||||
discardMany?: (paths: string[]) => Promise<void>
|
||||
discardSingle?: (path: string) => Promise<void>
|
||||
}) {
|
||||
return renderHook(() =>
|
||||
useSourceControlDiscardConfirmation({
|
||||
activeRepoSettings: {} as never,
|
||||
activeWorktreeId: 'repo::/w',
|
||||
worktreePath: '/w',
|
||||
grouped: {} as SourceControlEntryGroups,
|
||||
isExecutingBulk: false,
|
||||
setIsExecutingBulk: () => {},
|
||||
clearSelection: () => {},
|
||||
discardMany: overrides.discardMany ?? (async () => {}),
|
||||
discardSingle: overrides.discardSingle ?? (async () => {}),
|
||||
refreshActiveGitStatusAfterMutation: async () => {}
|
||||
})
|
||||
)
|
||||
}
|
||||
|
||||
// Why: the hook exposes the confirmation dialog, not the handler — this is the real user path.
|
||||
async function confirmDiscardAll(
|
||||
result: { current: ReturnType<typeof useSourceControlDiscardConfirmation> },
|
||||
area: DiscardAllArea,
|
||||
paths: readonly string[]
|
||||
): Promise<void> {
|
||||
await act(async () => {
|
||||
result.current.requestDiscardPaths(area, paths)
|
||||
})
|
||||
await act(async () => {
|
||||
result.current.confirmPendingDiscard()
|
||||
await Promise.resolve()
|
||||
})
|
||||
}
|
||||
|
||||
describe('the discard-all failure toast', () => {
|
||||
beforeEach(() => {
|
||||
toastError.mockReset()
|
||||
bulkUnstage.mockReset()
|
||||
})
|
||||
|
||||
it('shows the reason, not the IPC envelope, when the unstage pre-step rejects', async () => {
|
||||
bulkUnstage.mockRejectedValue(new Error(ENVELOPED))
|
||||
const { result } = renderDiscard({})
|
||||
|
||||
await confirmDiscardAll(result, 'staged', ['src/app.ts'])
|
||||
|
||||
const description = toastError.mock.calls[0][1].description
|
||||
expect(description).toBe(REASON)
|
||||
expect(description).not.toContain('Error invoking remote method')
|
||||
})
|
||||
|
||||
it('drops the description when the envelope carried no reason', async () => {
|
||||
bulkUnstage.mockRejectedValue(new Error(ENVELOPE_ONLY))
|
||||
const { result } = renderDiscard({})
|
||||
|
||||
await confirmDiscardAll(result, 'staged', ['src/app.ts'])
|
||||
|
||||
expect(toastError.mock.calls[0][1].description).toBeUndefined()
|
||||
})
|
||||
|
||||
it('shows the reason alongside the failed paths when a per-file discard rejects', async () => {
|
||||
const { result } = renderDiscard({
|
||||
discardMany: async () => {
|
||||
throw new Error('no bulk discard on this relay')
|
||||
},
|
||||
discardSingle: async () => {
|
||||
throw new Error(ENVELOPED)
|
||||
}
|
||||
})
|
||||
|
||||
await confirmDiscardAll(result, 'unstaged', ['src/app.ts'])
|
||||
|
||||
const description = toastError.mock.calls[0][1].description
|
||||
expect(description).toBe(`${REASON} (e.g. src/app.ts)`)
|
||||
expect(description).not.toContain('Error invoking remote method')
|
||||
})
|
||||
})
|
||||
+8
-2
@@ -1,6 +1,7 @@
|
||||
import { useCallback, useState } from 'react'
|
||||
import { toast } from 'sonner'
|
||||
import { getConnectionId } from '@/lib/connection-context'
|
||||
import { stripIpcInvokeEnvelope } from '@/lib/ipc-error'
|
||||
import { bulkUnstageRuntimeGitPaths, type RuntimeGitContext } from '@/runtime/runtime-git-client'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import type { GitStatusEntry } from '../../../../../../shared/git-status-types'
|
||||
@@ -90,17 +91,22 @@ export function useSourceControlDiscardConfirmation({
|
||||
console.error('[SourceControl] discard-all failure', error)
|
||||
}
|
||||
})
|
||||
// Why: onError retains every rejection in full above; the title already names the operation,
|
||||
// so an envelope with nothing behind it drops the description rather than printing plumbing.
|
||||
const firstMsg =
|
||||
errors[0] instanceof Error
|
||||
? (stripIpcInvokeEnvelope(errors[0].message) ?? undefined)
|
||||
: undefined
|
||||
if (result.aborted) {
|
||||
toast.error(
|
||||
translate(
|
||||
'auto.components.right.sidebar.SourceControl.a5e5a11090',
|
||||
'Discard all failed — unable to unstage files before discard'
|
||||
),
|
||||
{ description: errors[0] instanceof Error ? errors[0].message : undefined }
|
||||
{ description: firstMsg }
|
||||
)
|
||||
} else if (result.failed.length > 0) {
|
||||
// Why: show only the first error + a sample of failed paths to avoid a huge toast body on bulk failures.
|
||||
const firstMsg = errors[0] instanceof Error ? errors[0].message : undefined
|
||||
const sample = result.failed.slice(0, 3).join(', ')
|
||||
const more = result.failed.length > 3 ? `, +${result.failed.length - 3} more` : ''
|
||||
toast.error(
|
||||
|
||||
@@ -0,0 +1,93 @@
|
||||
// @vitest-environment happy-dom
|
||||
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { renderHook, waitFor } from '@testing-library/react'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { useTaskPageGitLabFetch } from './use-task-page-gitlab-fetch'
|
||||
import type { Repo } from '../../../../../shared/repo-types'
|
||||
|
||||
vi.mock('@/i18n/i18n', () => ({
|
||||
translate: (_key: string, fallback: string) => fallback
|
||||
}))
|
||||
vi.mock('@/components/task-page/source/repo-source-context', () => ({
|
||||
getTaskPageRepoSourceContext: () => null
|
||||
}))
|
||||
|
||||
// `gitlab:listIssues` throws before it returns a payload — `assertRegisteredRepo` rejects with
|
||||
// "Access denied: unknown repository path" — so the renderer sees Electron's invoke envelope, and
|
||||
// this banner renders errs[0] verbatim.
|
||||
const ENVELOPED =
|
||||
"Error invoking remote method 'gitlab:listIssues': Error: Access denied: unknown repository path"
|
||||
const REASON = 'Access denied: unknown repository path'
|
||||
const ENVELOPE_ONLY = "Error invoking remote method 'gitlab:listIssues': Error"
|
||||
const UNREADABLE_COPY =
|
||||
'Could not load GitLab items, and the failure did not include a readable reason.'
|
||||
|
||||
const repo = { id: 'repo-1', path: '/w/project' } as Repo
|
||||
|
||||
function renderFetch(rejection: unknown) {
|
||||
const listIssues = vi.fn().mockRejectedValue(rejection)
|
||||
;(globalThis as unknown as { window: Window }).window.api = {
|
||||
gl: { listIssues, listMRs: vi.fn() }
|
||||
} as never
|
||||
|
||||
const setGitlabError = vi.fn()
|
||||
renderHook(() =>
|
||||
useTaskPageGitLabFetch({
|
||||
taskSource: 'gitlab',
|
||||
gitlabView: 'issues',
|
||||
activeGitlabFilter: 'assigned-to-me',
|
||||
gitlabRefreshNonce: 0,
|
||||
selectedRepos: [repo],
|
||||
selectedReposKey: 'repo-1',
|
||||
primaryRepo: repo,
|
||||
gitlabIssuePage: 0,
|
||||
setGitlabItems: vi.fn(),
|
||||
setGitlabLoading: vi.fn(),
|
||||
setGitlabError,
|
||||
setGitlabIssuePage: vi.fn(),
|
||||
setGitlabIssueTotalPages: vi.fn(),
|
||||
setGitlabIssueLoadingTargetPage: vi.fn(),
|
||||
setGitlabTodos: vi.fn(),
|
||||
setGitlabTodosLoading: vi.fn()
|
||||
})
|
||||
)
|
||||
return setGitlabError
|
||||
}
|
||||
|
||||
// Why: the effect clears the banner with setGitlabError(null) before fetching, so the asserted
|
||||
// value is the last call, not the first.
|
||||
async function bannerText(setGitlabError: ReturnType<typeof vi.fn>): Promise<unknown> {
|
||||
await waitFor(() => expect(setGitlabError.mock.calls.length).toBeGreaterThan(1))
|
||||
return setGitlabError.mock.calls.at(-1)?.[0]
|
||||
}
|
||||
|
||||
describe('the GitLab task-list error banner', () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
// Why: `translate` is mocked to its fallback above, and the real one prefers the catalog whenever
|
||||
// the key resolves — so without this the asserted copy is a string no user ever sees.
|
||||
it('reads that copy from the catalog, not just the inline fallback', () => {
|
||||
const catalog = JSON.parse(
|
||||
readFileSync(join(__dirname, '..', '..', '..', 'i18n', 'locales', 'en.json'), 'utf8')
|
||||
)
|
||||
|
||||
expect(catalog.auto.components.TaskPage.unreadableGitlabListError).toBe(UNREADABLE_COPY)
|
||||
})
|
||||
|
||||
it('shows the reason, not the IPC envelope, when a list handler rejects', async () => {
|
||||
const banner = await bannerText(renderFetch(new Error(ENVELOPED)))
|
||||
|
||||
expect(banner).toBe(REASON)
|
||||
expect(banner).not.toContain('Error invoking remote method')
|
||||
})
|
||||
|
||||
it('shows readable copy when the envelope carried no reason', async () => {
|
||||
const banner = await bannerText(renderFetch(new Error(ENVELOPE_ONLY)))
|
||||
|
||||
expect(banner).toBe(UNREADABLE_COPY)
|
||||
})
|
||||
})
|
||||
@@ -7,6 +7,8 @@ import {
|
||||
import { resolveGitLabIssuePageState } from '@/components/task-page/gitlab/gitlab-issue-pages'
|
||||
import { getTaskPageRepoSourceContext } from '@/components/task-page/source/repo-source-context'
|
||||
import { withGitLabIpcTimeout } from '@/runtime/gitlab-ipc-timeout'
|
||||
import { extractIpcErrorMessage } from '@/lib/ipc-error'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import type { GitLabIssueFilter, GitLabTaskFilter } from '@/components/task-page-localized-options'
|
||||
import type { GitLabTodo, GitLabWorkItem } from '../../../../../shared/gitlab-types'
|
||||
import type { Repo } from '../../../../../shared/repo-types'
|
||||
@@ -136,7 +138,19 @@ export function useTaskPageGitLabFetch({
|
||||
const settled: { items: readonly GitLabWorkItem[]; totalPages?: number }[] = []
|
||||
for (const r of results) {
|
||||
if (r.status !== 'fulfilled') {
|
||||
errs.push(r.reason instanceof Error ? r.reason.message : String(r.reason))
|
||||
// Why: a rejected `gl:` handler arrives wrapped in Electron's invoke envelope, and this
|
||||
// string is rendered verbatim in the list banner. Kept in errs either way — errs.length
|
||||
// is the pager's errorCount, so dropping an unreadable one would retreat the page.
|
||||
console.error('[TaskPage] GitLab list request rejected', r.reason)
|
||||
errs.push(
|
||||
extractIpcErrorMessage(
|
||||
r.reason,
|
||||
translate(
|
||||
'auto.components.TaskPage.unreadableGitlabListError',
|
||||
'Could not load GitLab items, and the failure did not include a readable reason.'
|
||||
)
|
||||
)
|
||||
)
|
||||
continue
|
||||
}
|
||||
settled.push(r.value)
|
||||
|
||||
@@ -16,10 +16,15 @@ import { describe, expect, it } from 'vitest'
|
||||
* 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.
|
||||
* What this cannot see, stated rather than implied: it is scoped to one surface, and it reads source
|
||||
* text, so it sees the call, not the value. The two siblings it used to name — `use-task-page-gitlab-fetch.ts`
|
||||
* and `use-discard-confirmation.ts` — are fixed and now enumerated in `lib/ipc-error-call-site-census.test.ts`.
|
||||
*
|
||||
* That does not make the population closed, and the note there says why: enumerating by render sink
|
||||
* rather than by stripper finds 118 renderer modules that still hand a raw rejection to a user
|
||||
* surface with an IPC-capable producer behind it. Per-surface censuses like this one do not scale to
|
||||
* that, and a lint rule on the idiom would need suppressions. The proposal on the table is to strip
|
||||
* once at the preload boundary instead.
|
||||
*/
|
||||
const SURFACE_DOORS: Readonly<Record<string, string>> = {
|
||||
'setTerminalError(null)': 'clears the surface',
|
||||
|
||||
@@ -2211,7 +2211,8 @@
|
||||
"linearModeHasWorktreeTooltip": "Linear tickets linked to an Orca workspace",
|
||||
"linearEmptyHasWorktree": "No Linear tickets are linked to an Orca workspace yet. Start work from a Linear issue to see it here.",
|
||||
"linearOpenAttachedWorkspace": "Open workspace attached to {{value0}}",
|
||||
"linearWorktreesColumn": "Workspaces"
|
||||
"linearWorktreesColumn": "Workspaces",
|
||||
"unreadableGitlabListError": "Could not load GitLab items, and the failure did not include a readable reason."
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "Close",
|
||||
|
||||
@@ -4,7 +4,7 @@ import { describe, expect, it } from 'vitest'
|
||||
|
||||
/**
|
||||
* Why an enumeration and not a count: the previous note on this reader put its population at 18
|
||||
* and said the sites depended on its fail-open behaviour. The real number is 30, and every one
|
||||
* and said the sites depended on its fail-open behaviour. The real number is 31, and every one
|
||||
* of them renders the result to a user, so "these can keep the looser regex" was never a choice
|
||||
* anybody had made. A count is easy to be wrong about; this produces the list.
|
||||
*
|
||||
@@ -65,6 +65,10 @@ const CALL_SITES: Readonly<Record<string, { calls: number; surface: string }>> =
|
||||
surface: 'dialog error'
|
||||
},
|
||||
'src/renderer/src/components/sidebar/useCreateRepo.ts': { calls: 1, surface: 'dialog error' },
|
||||
'src/renderer/src/components/task-page/hooks/use-task-page-gitlab-fetch.ts': {
|
||||
calls: 1,
|
||||
surface: 'task list banner, via the errs buffer whose first entry the banner renders'
|
||||
},
|
||||
'src/renderer/src/components/terminal-pane/ipc-pty-connect.ts': {
|
||||
calls: 1,
|
||||
surface: 'terminal error+classifier'
|
||||
@@ -116,7 +120,7 @@ const ENVELOPE_MODULE = 'src/shared/ipc-invoke-envelope.ts'
|
||||
*
|
||||
* 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
|
||||
* reports 31 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<Record<string, { calls: number; surface: string }>> = {
|
||||
@@ -125,6 +129,11 @@ const DIRECT_STRIPPER_SITES: Readonly<Record<string, { calls: number; surface: s
|
||||
surface: 'recovery card'
|
||||
},
|
||||
'src/renderer/src/components/quick-open-file-list.ts': { calls: 1, surface: 'quick-open list' },
|
||||
'src/renderer/src/components/right-sidebar/source-control/commit/use-discard-confirmation.ts': {
|
||||
calls: 1,
|
||||
surface:
|
||||
'discard-all toast description — null drops the description, the title still names the failure'
|
||||
},
|
||||
'src/renderer/src/components/settings/VoiceSpeechModelSection.tsx': {
|
||||
calls: 1,
|
||||
surface: 'settings row'
|
||||
@@ -208,15 +217,34 @@ describe('extractIpcErrorMessage call sites', () => {
|
||||
* 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.
|
||||
* long as this file has existed.
|
||||
*
|
||||
* So the honest scope of this file: it is a change-detector for the sites that already strip, not
|
||||
* evidence that the leaking population is empty. It is not. Enumerating by surface instead — every
|
||||
* renderer expression that reads free text off a rejection and passes it to a render sink (a
|
||||
* `toast.*` argument or a `set*` state setter whose value is rendered) — finds 239 such
|
||||
* expressions across 162 modules, of which 118 reach a producer that can cross `ipcRenderer.invoke`
|
||||
* and do not strip. Four sampled at random were all real leaks of this exact shape, including
|
||||
* `source-control/commit/use-bulk-actions.ts`, which sits beside the file this PR just fixed, and
|
||||
* `settings/SshPane.tsx`, which renders the caught message as the whole toast title.
|
||||
*
|
||||
* No per-call-site gate can close that, because the leaking shape *is* the ordinary idiom: the
|
||||
* discriminator is whether the value crossed IPC, which is not visible where it is rendered. A
|
||||
* lint rule keyed on the idiom would fire on hundreds of correct sites and need suppressions.
|
||||
*
|
||||
* The narrow fix is the boundary, not the call site. Every envelope in the app is created in one
|
||||
* place — 730 `ipcRenderer.invoke(` calls live in exactly two files under `src/preload`. A preload
|
||||
* `invoke` wrapper that rejects with the stripped reason would fix all 118 at once and make this
|
||||
* census unnecessary, enforced by a ratchet test banning bare `ipcRenderer.invoke` outside it —
|
||||
* the same shape as the existing `child_process` ratchet. That is a separate change; the cost to
|
||||
* weigh first is that `TerminalPane.tsx` deliberately keeps the wrapped form in the console, so
|
||||
* the boundary must strip for display while the log keeps the original.
|
||||
*/
|
||||
// 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', () => {
|
||||
it('number 31, 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(total).toBe(31)
|
||||
expect(Object.values(CALL_SITES).filter(({ surface }) => surface === '')).toEqual([])
|
||||
expect(Object.values(DIRECT_STRIPPER_SITES).filter(({ surface }) => surface === '')).toEqual([])
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user