From 2c559fa96a039b17004ce0c1b288bbe1c83b8ca1 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:11:12 -0700 Subject: [PATCH 01/16] test(child-process): make the import ratchet able to fail never grows asserted offenders.length <= ALLOWLIST.length, but the two membership assertions already force those equal, so it could not fail. The comment claimed it caught a swap -- one file migrated off child_process, one added -- which is exactly the case it let through. Pins the true count and asserts both directions, so a swap fails and a pin left stale-high after a migration also fails rather than banking ground twice. Gives the console-visibility ratchet the same test: it had no count assertion at all and the same gap. Also anchors the owner-directory exemption with a trailing slash, so a future src/shared/child-process-foo.ts is scanned rather than silently exempt. --- .../child-process-import-boundary.test.ts | 32 ++++++++++++++++--- .../windows-console-visibility.test.ts | 23 +++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/shared/child-process/child-process-import-boundary.test.ts b/src/shared/child-process/child-process-import-boundary.test.ts index 4e261de5c8a..10ca4fd8522 100644 --- a/src/shared/child-process/child-process-import-boundary.test.ts +++ b/src/shared/child-process/child-process-import-boundary.test.ts @@ -23,10 +23,19 @@ const CHILD_PROCESS_IMPORT_ALLOWLIST: readonly string[] = readFileSync( .map((line) => line.trim()) .filter((line) => line.length > 0 && !line.startsWith('#')) +/** + * The true count of files importing child_process directly. + * + * May only ever be DECREASED, and only by migrating a file off + * `node:child_process`. Raising it is never the fix. + */ +const DIRECT_IMPORTER_PIN = 160 + const IMPORT_PATTERN = /(?:from\s+['"]node:child_process['"]|from\s+['"]child_process['"]|require\(\s*['"]node:child_process['"]|require\(\s*['"]child_process['"])/ -const OWNER_DIRECTORY = 'src/shared/child-process' +// Why: trailing slash, so a sibling like src/shared/child-process-foo.ts is scanned, not exempted. +const OWNER_DIRECTORY = 'src/shared/child-process/' const SCANNED_EXTENSIONS = ['.ts', '.tsx'] const IGNORED_DIRECTORIES = new Set([ 'node_modules', @@ -110,9 +119,22 @@ describe('child_process import boundary', () => { expect(stale, 'Allowlist entry no longer imports child_process — delete the line.').toEqual([]) }) - it('never grows', () => { - // The count is asserted separately from membership so a swap (one file - // migrated, one added) still fails loudly. - expect(offenders.length).toBeLessThanOrEqual(CHILD_PROCESS_IMPORT_ALLOWLIST.length) + it('holds the offender count at the pin', () => { + // Bounding by the allowlist's own length proves nothing: the two move + // together, so a swap (one file migrated off, one new file added with its + // entry) kept the bound satisfied. The pin is a literal for that reason. + expect( + offenders.length, + `${offenders.length} files import child_process directly; the pin is ${DIRECT_IMPORTER_PIN}. ` + + 'Never raise the pin -- migrate the file to runProcess/spawnProcess from ' + + 'src/shared/child-process instead.' + ).toBeLessThanOrEqual(DIRECT_IMPORTER_PIN) + // A pin left above reality is how a ratchet rots: it re-opens room for the + // next direct import to land for free. + expect( + offenders.length, + `Only ${offenders.length} files import child_process directly. Lower DIRECT_IMPORTER_PIN to ` + + `${offenders.length} to keep the ground you just took.` + ).toBeGreaterThanOrEqual(DIRECT_IMPORTER_PIN) }) }) diff --git a/src/shared/child-process/windows-console-visibility.test.ts b/src/shared/child-process/windows-console-visibility.test.ts index e98a97cfa98..7985b0ee11a 100644 --- a/src/shared/child-process/windows-console-visibility.test.ts +++ b/src/shared/child-process/windows-console-visibility.test.ts @@ -27,6 +27,15 @@ const ALLOWLIST: readonly string[] = readAllowlist( join(__dirname, '__fixtures__', 'windows-console-visibility-allowlist.txt') ) +/** + * The true count of files spawning without `windowsHide`. + * + * May only ever be DECREASED, and only by fixing a call site. Set equality with + * the allowlist does not bound this: a swap (one file fixed and delisted, one + * new file added with its entry) satisfies both membership assertions. + */ +const UNHIDDEN_SPAWNER_PIN = 68 + const CHILD_PROCESS_IMPORT = /from\s+['"](?:node:)?child_process['"]|require\(\s*['"](?:node:)?child_process['"]/ // Includes the promisified and renamed spellings -- `execAsync`, `spawnDetached`, @@ -166,4 +175,18 @@ describe('direct child-process calls hide the Windows console', () => { // A fixed file must leave the list, or the ratchet stops ratcheting. expect(ALLOWLIST.filter((path) => !offenders.includes(path))).toEqual([]) }) + + it('holds the offender count at the pin', () => { + expect( + offenders.length, + `${offenders.length} files spawn without windowsHide; the pin is ${UNHIDDEN_SPAWNER_PIN}. ` + + 'Never raise the pin -- add the flag, or route the call through run-process.ts.' + ).toBeLessThanOrEqual(UNHIDDEN_SPAWNER_PIN) + // A pin left above reality re-opens room for the next unguarded spawn. + expect( + offenders.length, + `Only ${offenders.length} files spawn without windowsHide. Lower UNHIDDEN_SPAWNER_PIN to ` + + `${offenders.length} to keep the ground you just took.` + ).toBeGreaterThanOrEqual(UNHIDDEN_SPAWNER_PIN) + }) }) From 73fcdea23c74bab3bd49c6ee0b6441102a541bdc Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:11:13 -0700 Subject: [PATCH 02/16] fix(palette): recompute quick-action availability when runtime status changes buildQuickActionContext reads runtimeStatusByEnvironmentId transitively through getClientCreationActionPolicy, but the split dropped it from the memo deps. The store replaces the Map identity on update, so the palette held availability from a snapshot that never refreshed -- offering a browser action against a provider that had gone away, or hiding one that had come back. exhaustive-deps could not catch it: the read is behind a void statement, which the rule does not see. --- ...use-worktree-jump-palette-quick-actions.ts | 8 +- ...palette-quick-action-availability.test.tsx | 103 ++++++++++++++++++ .../lib/lazy-chunk-recovery-reload.test.ts | 29 +++++ 3 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/components/worktree-jump-palette-quick-action-availability.test.tsx diff --git a/src/renderer/src/components/use-worktree-jump-palette-quick-actions.ts b/src/renderer/src/components/use-worktree-jump-palette-quick-actions.ts index c6cf2c48d05..0783a7e5d23 100644 --- a/src/renderer/src/components/use-worktree-jump-palette-quick-actions.ts +++ b/src/renderer/src/components/use-worktree-jump-palette-quick-actions.ts @@ -58,6 +58,7 @@ export function useWorktreeJumpPaletteQuickActions({ groupsByWorktree, isLoading, settings, + runtimeStatusByEnvironmentId, deferredQuery, settingsResults }: WorktreeJumpPaletteQuickActionsInput) { @@ -117,6 +118,9 @@ export function useWorktreeJumpPaletteQuickActions({ openNewTerminalTabInActiveWorkspace ] ) + // Why: buildQuickActionContext() reads the store imperatively, so these voided values are the + // memo's real inputs — each one is read (some transitively, e.g. runtimeStatusByEnvironmentId + // via the managed-browser creation policy) while availability is computed. const availableActionResults = useMemo(() => { void activeView void activeWorktreeId @@ -127,6 +131,7 @@ export function useWorktreeJumpPaletteQuickActions({ void groupsByWorktree void isLoading void settings?.activeRuntimeEnvironmentId + void runtimeStatusByEnvironmentId const context = buildQuickActionContext() return actionResults.filter((action) => action.isAvailable(context).available) }, [ @@ -140,7 +145,8 @@ export function useWorktreeJumpPaletteQuickActions({ activeGroupIdByWorktree, groupsByWorktree, isLoading, - settings?.activeRuntimeEnvironmentId + settings?.activeRuntimeEnvironmentId, + runtimeStatusByEnvironmentId ]) const middleItems = useMemo<(SettingsPaletteItem | QuickActionPaletteItem)[]>( () => diff --git a/src/renderer/src/components/worktree-jump-palette-quick-action-availability.test.tsx b/src/renderer/src/components/worktree-jump-palette-quick-action-availability.test.tsx new file mode 100644 index 00000000000..69459901bfa --- /dev/null +++ b/src/renderer/src/components/worktree-jump-palette-quick-action-availability.test.tsx @@ -0,0 +1,103 @@ +// @vitest-environment happy-dom + +import { renderHook } from '@testing-library/react' +import { createRef } from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { BROWSER_SCREENCAST_RUNTIME_CAPABILITY } from '../../../shared/protocol-version' +import { buildCmdJActionResults } from '@/components/cmd-j/palette-results' +import { getCmdJQuickActions } from '@/components/cmd-j/quick-actions' +import { useWorktreeJumpPaletteQuickActions } from './use-worktree-jump-palette-quick-actions' + +const mocks = vi.hoisted(() => ({ state: {} as Record })) + +vi.mock('@/store', () => ({ useAppStore: { getState: () => mocks.state } })) +vi.mock('@/lib/worktree-runtime-owner', () => ({ + getRuntimeEnvironmentIdForWorktree: () => RUNTIME_ID +})) +vi.mock('@/components/sidebar/delete-worktree-flow', () => ({ runWorktreeDelete: vi.fn() })) + +const RUNTIME_ID = 'runtime-1' +const WORKTREE_ID = 'repo-1::/repo/wt' + +function runtimeStatuses(capabilities: string[]): Map { + return new Map([[RUNTIME_ID, { status: { capabilities, hostPlatform: 'darwin' } }]]) +} + +// Every input except runtimeStatusByEnvironmentId keeps a stable identity across rerenders, +// so a recomputation can only come from the runtime status dependency itself. +function buildStableProps() { + return { + openModal: vi.fn(), + openSettingsPage: vi.fn(), + openSettingsTarget: vi.fn(), + activeGroupSnapshotRef: createRef(), + openNewBrowserTabInActiveWorkspace: vi.fn(), + openNewMarkdownInActiveWorkspace: vi.fn(), + openNewTerminalTabInActiveWorkspace: vi.fn(), + actionResults: buildCmdJActionResults(getCmdJQuickActions()), + activeView: 'terminal', + activeWorktreeId: WORKTREE_ID, + worktreesByRepo: mocks.state.worktreesByRepo, + repos: mocks.state.repos, + sshConnectionStates: mocks.state.sshConnectionStates, + activeGroupIdByWorktree: mocks.state.activeGroupIdByWorktree, + groupsByWorktree: mocks.state.groupsByWorktree, + isLoading: false, + settings: mocks.state.settings, + deferredQuery: 'new browser tab', + settingsResults: [] + } +} + +function renderQuickActions(initialStatuses: Map) { + const stable = buildStableProps() + mocks.state.runtimeStatusByEnvironmentId = initialStatuses + const harness = renderHook( + (runtimeStatusByEnvironmentId: Map) => + useWorktreeJumpPaletteQuickActions({ ...stable, runtimeStatusByEnvironmentId } as never), + { initialProps: initialStatuses } + ) + return { + offersBrowserAction: (): boolean => + harness.result.current.middleItems.some((item) => item.id === 'quick-action:new-browser-tab'), + setRuntimeStatuses: (next: Map): void => { + mocks.state.runtimeStatusByEnvironmentId = next + harness.rerender(next) + } + } +} + +describe('worktree jump palette quick action availability', () => { + beforeEach(() => { + ;(globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ = true + mocks.state = { + activeView: 'terminal', + activeWorktreeId: WORKTREE_ID, + worktreesByRepo: { 'repo-1': [{ id: WORKTREE_ID, repoId: 'repo-1' }] }, + repos: [{ id: 'repo-1' }], + sshConnectionStates: new Map(), + activeGroupIdByWorktree: { [WORKTREE_ID]: 'group-1' }, + groupsByWorktree: { [WORKTREE_ID]: [{ id: 'group-1' }] }, + settings: { activeRuntimeEnvironmentId: RUNTIME_ID } + } + }) + afterEach(() => { + delete (globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ + }) + + it('drops the paired-web browser action when the runtime loses screencast capability', () => { + const palette = renderQuickActions(runtimeStatuses([BROWSER_SCREENCAST_RUNTIME_CAPABILITY])) + expect(palette.offersBrowserAction()).toBe(true) + + palette.setRuntimeStatuses(runtimeStatuses([])) + expect(palette.offersBrowserAction()).toBe(false) + }) + + it('restores the browser action when a capable runtime comes back', () => { + const palette = renderQuickActions(runtimeStatuses([])) + expect(palette.offersBrowserAction()).toBe(false) + + palette.setRuntimeStatuses(runtimeStatuses([BROWSER_SCREENCAST_RUNTIME_CAPABILITY])) + expect(palette.offersBrowserAction()).toBe(true) + }) +}) diff --git a/src/renderer/src/lib/lazy-chunk-recovery-reload.test.ts b/src/renderer/src/lib/lazy-chunk-recovery-reload.test.ts index 9cdf4fa728d..8c113363861 100644 --- a/src/renderer/src/lib/lazy-chunk-recovery-reload.test.ts +++ b/src/renderer/src/lib/lazy-chunk-recovery-reload.test.ts @@ -6,6 +6,7 @@ import { requestLazyChunkRecoveryReload } from './lazy-chunk-recovery-reload' describe('requestLazyChunkRecoveryReload', () => { afterEach(() => { vi.restoreAllMocks() + vi.unstubAllGlobals() }) it('refuses the reload when the staged checkpoint never reaches disk', async () => { @@ -36,4 +37,32 @@ describe('requestLazyChunkRecoveryReload', () => { expect(order).toEqual(['flushed', 'reload']) }) + + it('joins the preload checkpoint before navigating when no override is supplied', async () => { + const order: string[] = [] + let flush: () => void = () => undefined + const awaitBeforeUnloadCheckpoint = vi.fn( + () => + new Promise((resolve) => { + flush = () => { + order.push('flushed') + resolve() + } + }) + ) + vi.stubGlobal('api', { app: { awaitBeforeUnloadCheckpoint } }) + const reload = vi.spyOn(window.location, 'reload').mockImplementation(() => { + order.push('reload') + window.dispatchEvent(new Event(ORCA_RENDERER_UNLOAD_PREVENTED_EVENT)) + }) + + const outcome = requestLazyChunkRecoveryReload(window) + await vi.waitFor(() => expect(awaitBeforeUnloadCheckpoint).toHaveBeenCalledTimes(1)) + expect(reload).not.toHaveBeenCalled() + + flush() + + await expect(outcome).resolves.toBe('unload-vetoed') + expect(order).toEqual(['flushed', 'reload']) + }) }) From c8937936eb5ae50d8e0b9a0481617f90a6e7054a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:11:14 -0700 Subject: [PATCH 03/16] refactor(mobile): pin the terminal WebView payload and split its widest slice The payload is one concatenated string, so slice boundaries follow document order rather than responsibility -- but join is associative, so cutting a slice into consecutive slices is byte-identical by construction. Splits the widest slice, which carried fit-scale, a DECSET scanner and the write queue together with no room left under the line cap. Adds a hash guard. The behavioral tests each execute one region of the payload in a vm, so an edit to an uncovered region shipped silently; the composed output is now pinned by sha256 and length. Derives the source-file list from the composer's own imports instead of a second hardcoded list a new slice had to be added to by hand -- the same silent subject-loss shape already found twice elsewhere in this repo. --- ...rminal-webview-html-source.test-support.ts | 45 +-- mobile/src/terminal/terminal-webview-html.ts | 8 +- .../fit-scale-and-write-queue.ts | 288 ------------------ .../mouse-mode-decset-scan.ts | 52 ++++ .../terminal-fit-scale.ts | 130 ++++++++ .../terminal-webview-html/write-queue.ts | 110 +++++++ .../terminal-webview-payload-hash.test.ts | 17 ++ 7 files changed, 341 insertions(+), 309 deletions(-) delete mode 100644 mobile/src/terminal/terminal-webview-html/fit-scale-and-write-queue.ts create mode 100644 mobile/src/terminal/terminal-webview-html/mouse-mode-decset-scan.ts create mode 100644 mobile/src/terminal/terminal-webview-html/terminal-fit-scale.ts create mode 100644 mobile/src/terminal/terminal-webview-html/write-queue.ts create mode 100644 mobile/src/terminal/terminal-webview-payload-hash.test.ts diff --git a/mobile/src/terminal/terminal-webview-html-source.test-support.ts b/mobile/src/terminal/terminal-webview-html-source.test-support.ts index 25902305f41..19a9cfc07ba 100644 --- a/mobile/src/terminal/terminal-webview-html-source.test-support.ts +++ b/mobile/src/terminal/terminal-webview-html-source.test-support.ts @@ -1,24 +1,31 @@ import { readFileSync } from 'node:fs' -const SOURCE_FILES = [ - './terminal-webview-html.ts', - './terminal-webview-html/document-shell.ts', - './terminal-webview-html/runtime-state-and-text-scaling.ts', - './terminal-webview-html/fit-scale-and-write-queue.ts', - './terminal-webview-html/terminal-init-and-write.ts', - './terminal-webview-html/host-message-router.ts', - './terminal-webview-html/selection-state-and-eviction.ts', - './terminal-webview-html/term-observers-and-mode-mirroring.ts', - './terminal-webview-html/mouse-report-and-scroll-routing.ts', - './terminal-webview-html/smooth-scroll-and-cell-geometry.ts', - './terminal-webview-html/selection-overlay.ts', - './terminal-webview-html/surface-touch-gestures.ts', - './terminal-webview-html/message-bridge-and-document-close.ts' -] as const +const COMPOSER_FILE = './terminal-webview-html.ts' +const SLICE_IMPORT_RE = /^import \{[^}]*\} from '(\.\/terminal-webview-html\/[\w-]+)'$/gm +const COMPOSED_ENTRY_RE = /^ {2}TERMINAL_HTML_\w+,?$/gm -/** Reads the TypeScript source that assembles the in-WebView document. */ +function readSource(relativePath: string): string { + return readFileSync(new URL(relativePath, import.meta.url), 'utf8') +} + +/** + * Reads the TypeScript source that assembles the in-WebView document. + * + * Why: the slice list is derived from the composer's own imports rather than duplicated, so a + * new slice cannot join the emitted document while staying invisible to the tests that search + * this source. The count cross-check catches an import shape the regex cannot see. + */ export function readTerminalWebViewHtmlSource(): string { - return SOURCE_FILES.map((relativePath) => - readFileSync(new URL(relativePath, import.meta.url), 'utf8') - ).join('\n') + const composer = readSource(COMPOSER_FILE) + const slices = [...composer.matchAll(SLICE_IMPORT_RE)].map((match) => `${match[1]}.ts`) + const composedCount = [...composer.matchAll(COMPOSED_ENTRY_RE)].length + if (composedCount === 0) { + throw new Error('no composed WebView document slices found') + } + if (slices.length !== composedCount) { + throw new Error( + `WebView document slice imports (${slices.length}) do not match composed entries (${composedCount})` + ) + } + return [composer, ...slices.map(readSource)].join('\n') } diff --git a/mobile/src/terminal/terminal-webview-html.ts b/mobile/src/terminal/terminal-webview-html.ts index 40b7a2db22c..17fadd4d26c 100644 --- a/mobile/src/terminal/terminal-webview-html.ts +++ b/mobile/src/terminal/terminal-webview-html.ts @@ -1,6 +1,8 @@ import { TERMINAL_HTML_DOCUMENT_SHELL } from './terminal-webview-html/document-shell' import { TERMINAL_HTML_RUNTIME_STATE_AND_TEXT_SCALING } from './terminal-webview-html/runtime-state-and-text-scaling' -import { TERMINAL_HTML_FIT_SCALE_AND_WRITE_QUEUE } from './terminal-webview-html/fit-scale-and-write-queue' +import { TERMINAL_HTML_FIT_SCALE } from './terminal-webview-html/terminal-fit-scale' +import { TERMINAL_HTML_MOUSE_MODE_DECSET_SCAN } from './terminal-webview-html/mouse-mode-decset-scan' +import { TERMINAL_HTML_WRITE_QUEUE } from './terminal-webview-html/write-queue' import { TERMINAL_HTML_INIT_AND_WRITE } from './terminal-webview-html/terminal-init-and-write' import { TERMINAL_HTML_HOST_MESSAGE_ROUTER } from './terminal-webview-html/host-message-router' import { TERMINAL_HTML_SELECTION_STATE_AND_EVICTION } from './terminal-webview-html/selection-state-and-eviction' @@ -19,7 +21,9 @@ export { MOBILE_TERMINAL_CARET_OPTIONS } from './terminal-webview-html/theme' export const XTERM_HTML = [ TERMINAL_HTML_DOCUMENT_SHELL, TERMINAL_HTML_RUNTIME_STATE_AND_TEXT_SCALING, - TERMINAL_HTML_FIT_SCALE_AND_WRITE_QUEUE, + TERMINAL_HTML_FIT_SCALE, + TERMINAL_HTML_MOUSE_MODE_DECSET_SCAN, + TERMINAL_HTML_WRITE_QUEUE, TERMINAL_HTML_INIT_AND_WRITE, TERMINAL_HTML_HOST_MESSAGE_ROUTER, TERMINAL_HTML_SELECTION_STATE_AND_EVICTION, diff --git a/mobile/src/terminal/terminal-webview-html/fit-scale-and-write-queue.ts b/mobile/src/terminal/terminal-webview-html/fit-scale-and-write-queue.ts deleted file mode 100644 index 074185d43a5..00000000000 --- a/mobile/src/terminal/terminal-webview-html/fit-scale-and-write-queue.ts +++ /dev/null @@ -1,288 +0,0 @@ -import { TERMINAL_WEBVIEW_THEME_JS } from '../terminal-webview-theme-injected' - -// Also carries the DECSET mouse-mode scanner: emitted-document order pins it between these two concerns. -export const TERMINAL_HTML_FIT_SCALE_AND_WRITE_QUEUE = `${TERMINAL_WEBVIEW_THEME_JS} - - function getCellHeight() { - if (!term || !term._core) return 15; - var core = term._core; - if (core._renderService && core._renderService.dimensions) { - return core._renderService.dimensions.css.cell.height || 15; - } - return 15; - } - - // Why: clamp pan so the terminal content always covers the viewport - // when zoomed in. When content is smaller than viewport in a - // dimension, pin to top-left (no floating in the middle). - function clampPan() { - if (!term || !term.element) return; - var ts = getTotalScale(); - var cw = term.element.scrollWidth * ts; - var ch = term.element.scrollHeight * ts; - var vpW = window.innerWidth; - var vpH = window.innerHeight; - if (cw > vpW) { - panX = Math.min(0, Math.max(vpW - cw, panX)); - } else { - panX = 0; - } - if (ch > vpH) { - panY = Math.min(0, Math.max(vpH - ch, panY)); - } else { - panY = 0; - } - } - - // Why: intentional no-op. Mobile replays a live PTY snapshot then applies - // live cursor-relative chunks from that same PTY; resizing only the WebView - // xterm changes cursor coordinates and makes TUI repaint chunks duplicate or - // overlap. Kept as a no-op so its call sites stay legible. - function adjustRowsForViewport() {} - - // Why: cold-start fit. After init() opens xterm, the renderer needs - // several frames before cell dimensions are computed. Reading too early - // gives cellWidth=0 (renderer service not ready) or scrollWidth=0 (DOM - // not laid out), and computeFitScale returns 1 → no zoom. - // - // Gate: cellWidth × cols is the canonical "logical width" of the grid - // and reflects xterm's layout decision, independent of buffer content. - // We commit when cellWidth becomes positive (renderer ready). Fallback: - // if cellWidth never becomes available, gate on stable positive - // scrollWidth (xterm rendered something). Cap at 60 frames (~1s @60Hz) - // so a backgrounded WebView never spins forever. - var FIT_RETRY_MAX_FRAMES = 60; - var fitRetryToken = 0; - function applyFitScale(reason) { - if (!term || !term.element) return; - var token = ++fitRetryToken; - var attempts = 0; - var lastScrollWidth = -1; - function attempt() { - if (token !== fitRetryToken) return; - if (!term || !term.element) return; - attempts++; - var cellW = getCellWidth(); - if (cellW > 0 && term.cols > 0) { - commitFitScale(reason, attempts, 'cellW'); - return; - } - var w = term.element.scrollWidth; - if (w > 0 && w === lastScrollWidth) { - commitFitScale(reason, attempts, 'stableSW'); - return; - } - lastScrollWidth = w; - if (attempts >= FIT_RETRY_MAX_FRAMES) { - flog('commit-timeout', { - reason: reason, - attempts: attempts, - cellW: cellW, - scrollWidth: w, - cols: term.cols - }); - commitFitScale(reason, attempts, 'timeout'); - return; - } - requestAnimationFrame(attempt); - } - requestAnimationFrame(attempt); - } - - function commitFitScale(reason, attempts, gate) { - if (!term || !term.element) return; - var preSnapScale = computeFitScale(); - currentScale = preSnapScale; - // Why: when scale is very close to 1 (e.g. 0.97 from xterm scrollbar - // sub-pixels) snap to 1 to avoid imperceptible shrinkage that prevents - // a second applyFitScale from observing a "no-op needed" state. - if (currentScale >= 0.95) currentScale = 1; - userScale = 1; - panX = 0; - panY = 0; - smoothScrollOffsetY = 0; - updateTransform(); - adjustRowsForViewport(); - - var cellW = getCellWidth(); - var sw = term.element.scrollWidth; - var vpW = window.innerWidth; - var expectedW = cellW * term.cols; - var suspect = - currentScale === 1 && term.cols > 0 && expectedW > vpW + 1; // expected wider than viewport but no zoom - if (suspect) { - flog('commit-SUSPECT', { - reason: reason, - attempts: attempts, - gate: gate, - preSnapScale: preSnapScale, - finalScale: currentScale, - cellW: cellW, - cols: term.cols, - expectedW: expectedW, - scrollWidth: sw, - vpWidth: vpW - }); - } - repositionOverlay(); - } - - function isAltScreenActive(data) { - if (typeof data !== 'string') return false; - var on = data.lastIndexOf(ESC + '[?1049h'); - var off = data.lastIndexOf(ESC + '[?1049l'); - return on !== -1 && on > off; - } - - function normalizeInitialData(data) { - if (!isAltScreenActive(data)) return data; - var on = data.lastIndexOf(ESC + '[?1049h'); - // Why: SerializeAddon can include normal-buffer scrollback before the - // active alternate-screen snapshot. Replaying both into a fresh mobile - // xterm duplicates TUI frames and can flatten SGR attributes. - return on > 0 ? data.slice(on) : data; - } - - function updateMouseModeFromData(data) { - if (typeof data !== 'string' || data.length === 0) return; - var input = mouseModeScanTail + data; - mouseModeScanTail = extractMouseModeScanTail(input); - var re = new RegExp(ESC + 'c|' + ESC + '\\\\[\\\\?([0-9;]+)([hl])|' + C1_CSI + '\\\\?([0-9;]+)([hl])', 'g'); - var match; - while ((match = re.exec(input)) !== null) { - if (match[0] === ESC + 'c') { - trackedMouseTrackingMode = 'none'; - sgrMouseMode = false; - sgrMousePixelsMode = false; - continue; - } - var enabled = (match[2] || match[4]) === 'h'; - var params = (match[1] || match[3]).split(';'); - for (var i = 0; i < params.length; i++) { - if (params[i] === '') continue; - var param = Number(params[i]); - if (!Number.isInteger(param)) continue; - if (param === 9) trackedMouseTrackingMode = enabled ? 'x10' : 'none'; - if (param === 1000) trackedMouseTrackingMode = enabled ? 'vt200' : 'none'; - if (param === 1002) trackedMouseTrackingMode = enabled ? 'drag' : 'none'; - if (param === 1003) trackedMouseTrackingMode = enabled ? 'any' : 'none'; - if (param === 1006) { - sgrMouseMode = enabled; - sgrMousePixelsMode = false; - } - if (param === 1016) { - sgrMouseMode = false; - sgrMousePixelsMode = enabled; - } - } - } - } - - function resetWriteQueue() { - writeQueue = []; - writeQueueHead = 0; - } - - function isStatusDotPresentationSelector(value) { - return value === TEXT_PRESENTATION_SELECTOR || value === EMOJI_PRESENTATION_SELECTOR; - } - - function endsWithStatusDotPresentationSequence(data) { - var i = data.length - 1; - while (i >= 0 && isStatusDotPresentationSelector(data.charAt(i))) i--; - return i >= 0 && data.charAt(i) === CLAUDE_STATUS_DOT; - } - - // Why: iOS WebKit promotes Claude's record/status dot to a colorful emoji glyph. - function normalizeStatusDotPresentation(data) { - if (typeof data !== 'string' || data.length === 0) return data; - if (statusDotPendingSelector) { - statusDotPendingSelector = false; - var strippedPendingSelectors = false; - while (data.length > 0 && isStatusDotPresentationSelector(data.charAt(0))) data = data.slice(1); - strippedPendingSelectors = data.length === 0; - if (strippedPendingSelectors) { - statusDotPendingSelector = true; - return ''; - } - } - var normalized = data.replace(CLAUDE_STATUS_DOT_PATTERN, CLAUDE_STATUS_DOT + TEXT_PRESENTATION_SELECTOR); - statusDotPendingSelector = endsWithStatusDotPresentationSequence(data); - return normalized; - } - - function enqueueWrite(data) { - writeQueue.push(normalizeStatusDotPresentation(data)); - } - - function enqueueWriteBoundary(callback) { - writeQueue.push(callback); - } - - function nextQueuedWrite() { - if (writeQueueHead >= writeQueue.length) { - resetWriteQueue(); - return undefined; - } - var next = writeQueue[writeQueueHead]; - writeQueueHead++; - // Why: high-throughput terminals can enqueue faster than xterm parses; - // compact consumed slots so drain work stays O(1) without retaining old chunks. - if (writeQueueHead > 128 && writeQueueHead * 2 > writeQueue.length) { - writeQueue = writeQueue.slice(writeQueueHead); - writeQueueHead = 0; - } - return next; - } - - function disposeTermObservers() { - var disposables = termObserverDisposables; - termObserverDisposables = []; - for (var i = 0; i < disposables.length; i++) { - try { disposables[i] && disposables[i].dispose && disposables[i].dispose(); } catch (e) {} - } - } - - function extractMouseModeScanTail(input) { - var start = Math.max(input.lastIndexOf(ESC), input.lastIndexOf(C1_CSI)); - if (start === -1) return ''; - var tail = input.slice(start); - // Why: PTY/SSH chunks can split a long combined DECSET before the final h/l. - // Keep parser state far beyond normal mode lists while still bounding memory. - if (tail.length > PRIVATE_MODE_SCAN_TAIL_LIMIT) return ''; - if (tail === ESC || tail === ESC + '[' || tail === C1_CSI) return tail; - if (tail.indexOf(ESC + '[?') === 0) { - return /^[0-9;]*$/.test(tail.slice(3)) ? tail : ''; - } - if (tail.indexOf(C1_CSI + '?') === 0) { - return /^[0-9;]*$/.test(tail.slice(2)) ? tail : ''; - } - return ''; - } - - function pumpWrites(gen) { - if (!ready || !term || writesDraining || gen !== terminalGeneration) return; - var next = nextQueuedWrite(); - if (typeof next !== 'string') { - if (typeof next === 'function') return next(), pumpWrites(gen); - var callbacks = afterDrainCallbacks; - afterDrainCallbacks = []; - for (var i = 0; i < callbacks.length; i++) callbacks[i](); - return; - } - writesDraining = true; - // Why: xterm.write() parses asynchronously. Row adjustment/resizing must - // wait until replayed SGR attributes have landed in the buffer. - term.write(next, function() { - if (gen !== terminalGeneration) return; - writesDraining = false; - pumpWrites(gen); - }); - } - - function afterWritesDrained(callback) { - afterDrainCallbacks.push(callback); - pumpWrites(terminalGeneration); - } - -` diff --git a/mobile/src/terminal/terminal-webview-html/mouse-mode-decset-scan.ts b/mobile/src/terminal/terminal-webview-html/mouse-mode-decset-scan.ts new file mode 100644 index 00000000000..6f0685df87e --- /dev/null +++ b/mobile/src/terminal/terminal-webview-html/mouse-mode-decset-scan.ts @@ -0,0 +1,52 @@ +export const TERMINAL_HTML_MOUSE_MODE_DECSET_SCAN = ` function isAltScreenActive(data) { + if (typeof data !== 'string') return false; + var on = data.lastIndexOf(ESC + '[?1049h'); + var off = data.lastIndexOf(ESC + '[?1049l'); + return on !== -1 && on > off; + } + + function normalizeInitialData(data) { + if (!isAltScreenActive(data)) return data; + var on = data.lastIndexOf(ESC + '[?1049h'); + // Why: SerializeAddon can include normal-buffer scrollback before the + // active alternate-screen snapshot. Replaying both into a fresh mobile + // xterm duplicates TUI frames and can flatten SGR attributes. + return on > 0 ? data.slice(on) : data; + } + + function updateMouseModeFromData(data) { + if (typeof data !== 'string' || data.length === 0) return; + var input = mouseModeScanTail + data; + mouseModeScanTail = extractMouseModeScanTail(input); + var re = new RegExp(ESC + 'c|' + ESC + '\\\\[\\\\?([0-9;]+)([hl])|' + C1_CSI + '\\\\?([0-9;]+)([hl])', 'g'); + var match; + while ((match = re.exec(input)) !== null) { + if (match[0] === ESC + 'c') { + trackedMouseTrackingMode = 'none'; + sgrMouseMode = false; + sgrMousePixelsMode = false; + continue; + } + var enabled = (match[2] || match[4]) === 'h'; + var params = (match[1] || match[3]).split(';'); + for (var i = 0; i < params.length; i++) { + if (params[i] === '') continue; + var param = Number(params[i]); + if (!Number.isInteger(param)) continue; + if (param === 9) trackedMouseTrackingMode = enabled ? 'x10' : 'none'; + if (param === 1000) trackedMouseTrackingMode = enabled ? 'vt200' : 'none'; + if (param === 1002) trackedMouseTrackingMode = enabled ? 'drag' : 'none'; + if (param === 1003) trackedMouseTrackingMode = enabled ? 'any' : 'none'; + if (param === 1006) { + sgrMouseMode = enabled; + sgrMousePixelsMode = false; + } + if (param === 1016) { + sgrMouseMode = false; + sgrMousePixelsMode = enabled; + } + } + } + } + +` diff --git a/mobile/src/terminal/terminal-webview-html/terminal-fit-scale.ts b/mobile/src/terminal/terminal-webview-html/terminal-fit-scale.ts new file mode 100644 index 00000000000..b756bcb550c --- /dev/null +++ b/mobile/src/terminal/terminal-webview-html/terminal-fit-scale.ts @@ -0,0 +1,130 @@ +import { TERMINAL_WEBVIEW_THEME_JS } from '../terminal-webview-theme-injected' + +// Opens with the injected theme block: it lands at this point in the emitted document. +export const TERMINAL_HTML_FIT_SCALE = `${TERMINAL_WEBVIEW_THEME_JS} + + function getCellHeight() { + if (!term || !term._core) return 15; + var core = term._core; + if (core._renderService && core._renderService.dimensions) { + return core._renderService.dimensions.css.cell.height || 15; + } + return 15; + } + + // Why: clamp pan so the terminal content always covers the viewport + // when zoomed in. When content is smaller than viewport in a + // dimension, pin to top-left (no floating in the middle). + function clampPan() { + if (!term || !term.element) return; + var ts = getTotalScale(); + var cw = term.element.scrollWidth * ts; + var ch = term.element.scrollHeight * ts; + var vpW = window.innerWidth; + var vpH = window.innerHeight; + if (cw > vpW) { + panX = Math.min(0, Math.max(vpW - cw, panX)); + } else { + panX = 0; + } + if (ch > vpH) { + panY = Math.min(0, Math.max(vpH - ch, panY)); + } else { + panY = 0; + } + } + + // Why: intentional no-op. Mobile replays a live PTY snapshot then applies + // live cursor-relative chunks from that same PTY; resizing only the WebView + // xterm changes cursor coordinates and makes TUI repaint chunks duplicate or + // overlap. Kept as a no-op so its call sites stay legible. + function adjustRowsForViewport() {} + + // Why: cold-start fit. After init() opens xterm, the renderer needs + // several frames before cell dimensions are computed. Reading too early + // gives cellWidth=0 (renderer service not ready) or scrollWidth=0 (DOM + // not laid out), and computeFitScale returns 1 → no zoom. + // + // Gate: cellWidth × cols is the canonical "logical width" of the grid + // and reflects xterm's layout decision, independent of buffer content. + // We commit when cellWidth becomes positive (renderer ready). Fallback: + // if cellWidth never becomes available, gate on stable positive + // scrollWidth (xterm rendered something). Cap at 60 frames (~1s @60Hz) + // so a backgrounded WebView never spins forever. + var FIT_RETRY_MAX_FRAMES = 60; + var fitRetryToken = 0; + function applyFitScale(reason) { + if (!term || !term.element) return; + var token = ++fitRetryToken; + var attempts = 0; + var lastScrollWidth = -1; + function attempt() { + if (token !== fitRetryToken) return; + if (!term || !term.element) return; + attempts++; + var cellW = getCellWidth(); + if (cellW > 0 && term.cols > 0) { + commitFitScale(reason, attempts, 'cellW'); + return; + } + var w = term.element.scrollWidth; + if (w > 0 && w === lastScrollWidth) { + commitFitScale(reason, attempts, 'stableSW'); + return; + } + lastScrollWidth = w; + if (attempts >= FIT_RETRY_MAX_FRAMES) { + flog('commit-timeout', { + reason: reason, + attempts: attempts, + cellW: cellW, + scrollWidth: w, + cols: term.cols + }); + commitFitScale(reason, attempts, 'timeout'); + return; + } + requestAnimationFrame(attempt); + } + requestAnimationFrame(attempt); + } + + function commitFitScale(reason, attempts, gate) { + if (!term || !term.element) return; + var preSnapScale = computeFitScale(); + currentScale = preSnapScale; + // Why: when scale is very close to 1 (e.g. 0.97 from xterm scrollbar + // sub-pixels) snap to 1 to avoid imperceptible shrinkage that prevents + // a second applyFitScale from observing a "no-op needed" state. + if (currentScale >= 0.95) currentScale = 1; + userScale = 1; + panX = 0; + panY = 0; + smoothScrollOffsetY = 0; + updateTransform(); + adjustRowsForViewport(); + + var cellW = getCellWidth(); + var sw = term.element.scrollWidth; + var vpW = window.innerWidth; + var expectedW = cellW * term.cols; + var suspect = + currentScale === 1 && term.cols > 0 && expectedW > vpW + 1; // expected wider than viewport but no zoom + if (suspect) { + flog('commit-SUSPECT', { + reason: reason, + attempts: attempts, + gate: gate, + preSnapScale: preSnapScale, + finalScale: currentScale, + cellW: cellW, + cols: term.cols, + expectedW: expectedW, + scrollWidth: sw, + vpWidth: vpW + }); + } + repositionOverlay(); + } + +` diff --git a/mobile/src/terminal/terminal-webview-html/write-queue.ts b/mobile/src/terminal/terminal-webview-html/write-queue.ts new file mode 100644 index 00000000000..ae8ed85297f --- /dev/null +++ b/mobile/src/terminal/terminal-webview-html/write-queue.ts @@ -0,0 +1,110 @@ +// Also carries disposeTermObservers() and extractMouseModeScanTail(): both belong to +// other concerns, but emitted-document order pins them inside this queue. +export const TERMINAL_HTML_WRITE_QUEUE = ` function resetWriteQueue() { + writeQueue = []; + writeQueueHead = 0; + } + + function isStatusDotPresentationSelector(value) { + return value === TEXT_PRESENTATION_SELECTOR || value === EMOJI_PRESENTATION_SELECTOR; + } + + function endsWithStatusDotPresentationSequence(data) { + var i = data.length - 1; + while (i >= 0 && isStatusDotPresentationSelector(data.charAt(i))) i--; + return i >= 0 && data.charAt(i) === CLAUDE_STATUS_DOT; + } + + // Why: iOS WebKit promotes Claude's record/status dot to a colorful emoji glyph. + function normalizeStatusDotPresentation(data) { + if (typeof data !== 'string' || data.length === 0) return data; + if (statusDotPendingSelector) { + statusDotPendingSelector = false; + var strippedPendingSelectors = false; + while (data.length > 0 && isStatusDotPresentationSelector(data.charAt(0))) data = data.slice(1); + strippedPendingSelectors = data.length === 0; + if (strippedPendingSelectors) { + statusDotPendingSelector = true; + return ''; + } + } + var normalized = data.replace(CLAUDE_STATUS_DOT_PATTERN, CLAUDE_STATUS_DOT + TEXT_PRESENTATION_SELECTOR); + statusDotPendingSelector = endsWithStatusDotPresentationSequence(data); + return normalized; + } + + function enqueueWrite(data) { + writeQueue.push(normalizeStatusDotPresentation(data)); + } + + function enqueueWriteBoundary(callback) { + writeQueue.push(callback); + } + + function nextQueuedWrite() { + if (writeQueueHead >= writeQueue.length) { + resetWriteQueue(); + return undefined; + } + var next = writeQueue[writeQueueHead]; + writeQueueHead++; + // Why: high-throughput terminals can enqueue faster than xterm parses; + // compact consumed slots so drain work stays O(1) without retaining old chunks. + if (writeQueueHead > 128 && writeQueueHead * 2 > writeQueue.length) { + writeQueue = writeQueue.slice(writeQueueHead); + writeQueueHead = 0; + } + return next; + } + + function disposeTermObservers() { + var disposables = termObserverDisposables; + termObserverDisposables = []; + for (var i = 0; i < disposables.length; i++) { + try { disposables[i] && disposables[i].dispose && disposables[i].dispose(); } catch (e) {} + } + } + + function extractMouseModeScanTail(input) { + var start = Math.max(input.lastIndexOf(ESC), input.lastIndexOf(C1_CSI)); + if (start === -1) return ''; + var tail = input.slice(start); + // Why: PTY/SSH chunks can split a long combined DECSET before the final h/l. + // Keep parser state far beyond normal mode lists while still bounding memory. + if (tail.length > PRIVATE_MODE_SCAN_TAIL_LIMIT) return ''; + if (tail === ESC || tail === ESC + '[' || tail === C1_CSI) return tail; + if (tail.indexOf(ESC + '[?') === 0) { + return /^[0-9;]*$/.test(tail.slice(3)) ? tail : ''; + } + if (tail.indexOf(C1_CSI + '?') === 0) { + return /^[0-9;]*$/.test(tail.slice(2)) ? tail : ''; + } + return ''; + } + + function pumpWrites(gen) { + if (!ready || !term || writesDraining || gen !== terminalGeneration) return; + var next = nextQueuedWrite(); + if (typeof next !== 'string') { + if (typeof next === 'function') return next(), pumpWrites(gen); + var callbacks = afterDrainCallbacks; + afterDrainCallbacks = []; + for (var i = 0; i < callbacks.length; i++) callbacks[i](); + return; + } + writesDraining = true; + // Why: xterm.write() parses asynchronously. Row adjustment/resizing must + // wait until replayed SGR attributes have landed in the buffer. + term.write(next, function() { + if (gen !== terminalGeneration) return; + writesDraining = false; + pumpWrites(gen); + }); + } + + function afterWritesDrained(callback) { + afterDrainCallbacks.push(callback); + pumpWrites(terminalGeneration); + } + +` diff --git a/mobile/src/terminal/terminal-webview-payload-hash.test.ts b/mobile/src/terminal/terminal-webview-payload-hash.test.ts new file mode 100644 index 00000000000..f8bfa4bd134 --- /dev/null +++ b/mobile/src/terminal/terminal-webview-payload-hash.test.ts @@ -0,0 +1,17 @@ +import { createHash } from 'node:crypto' +import { describe, expect, it } from 'vitest' +import { XTERM_HTML } from './terminal-webview-html' + +// Why: every other WebView test exercises one slice of the document, so an edit to an +// uncovered region ships silently. A diff here means the emitted WebView source changed — +// update these values only when that change is deliberate, and only after checking the +// document still runs. Refactors that merely move slice boundaries must leave them alone. +const EXPECTED_SHA256 = '42cc000faddc3b58b8fd4855f848c7878f0cd6166c613f66d733645e8e1b9608' +const EXPECTED_LENGTH = 729776 + +describe('terminal WebView payload', () => { + it('composes the expected document', () => { + expect(XTERM_HTML.length).toBe(EXPECTED_LENGTH) + expect(createHash('sha256').update(XTERM_HTML, 'utf8').digest('hex')).toBe(EXPECTED_SHA256) + }) +}) From 80a52bb9b3fccd6c510bb1647c86fb7f19c8455b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:14:57 -0700 Subject: [PATCH 04/16] fix(git): recover commit ref badges on Git older than 2.43 (#17923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GIT_HISTORY_COMMIT_FORMAT asked for decorations with %(decorate:…), which Git 2.43 introduced. Older Git prints the placeholder verbatim and exits zero, so nothing raised and every commit in the Source Control panel silently lost its branch, remote and tag badges. The record now also carries %D (Git 2.10) on its own line, selected by an exact match against the unexpanded placeholder — a ref name can never contain the \x1f that Git expands inside the echoed text. %n emits the %D line on both sides of the boundary, so the message index is fixed and a missed match degrades to no badges rather than a corrupted message. The decoration separator is now bound to the field that produced the text instead of sniffed from it. A lone decoration carries no separator, so the old sniff split `refs/heads/feat,one` into two bogus refs. Verified against real Git 2.38.1 and 2.49.1. Co-authored-by: kaluli123123 <295758798+kaluli123123@users.noreply.github.com> --- docs/reference/git-compatibility.md | 11 ++++++ src/shared/git-binary-compatibility.test.ts | 26 ++++++++++++ src/shared/git-history-log-parser.ts | 33 +++++++++++----- src/shared/git-history.test.ts | 44 ++++++++++++++++++++- 4 files changed, 102 insertions(+), 12 deletions(-) diff --git a/docs/reference/git-compatibility.md b/docs/reference/git-compatibility.md index 3004b8888ab..0e8b1f257d3 100644 --- a/docs/reference/git-compatibility.md +++ b/docs/reference/git-compatibility.md @@ -42,6 +42,17 @@ authority. | `merge-tree-write-tree` | Derive real-merge conflicts and no-op tree proofs | Omit the conflict summary and keep conservative branch cleanup behavior before Git 2.38 | | `merge-tree-merge-base` | Supply the already-resolved merge base | Use the older two-commit `merge-tree --write-tree` form | +### Placeholders That Fail Open + +`GitCapabilityCache` records commands Git *rejects*. A `git log --format` +placeholder Git does not know is not rejected: Git echoes it verbatim and exits +zero, so there is no error to remember and no probe to cache. Ask for both forms +in one record and pick at parse time. + +| Placeholder | Preferred behavior | Compatibility behavior | +| ---------------- | ------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | +| `%(decorate:…)` | Git 2.43 separates commit decorations with `\x1f`, so ref names containing commas survive | The same record also carries `%D` (Git 2.10); an unexpanded `%(decorate` placeholder selects it, at the cost of comma-splitting | + ## Why Not `simple-git` `simple-git` is a process wrapper around the installed Git binary. Its custom diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index debab394649..3a8f562dff1 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -15,6 +15,7 @@ import { isUnsupportedWorktreeListZError } from './git-worktree-command-capabilities' import { gitCredentialPromptGuardEnv } from './git-credential-prompt-env' +import { GIT_HISTORY_COMMIT_FORMAT, parseGitHistoryLog } from './git-history-log-parser' import { githubPullRequestHeadLocalRef, gitlabMergeRequestHeadLocalRef, @@ -378,4 +379,29 @@ describeBinaryCompatibility('real Git binary compatibility', () => { runGit(['show', '--end-of-options', `${pinnedOid}:absent.txt`]) ).rejects.toBeDefined() }) + // Why pin this: an older Git echoes %(decorate:…) and exits zero, so only %D + // in the same record carries the badges (#15507). Asserts the echo and the recovery. + it('reads commit decorations on both sides of the %(decorate:...) boundary', async () => { + await writeFile(join(repoPath, 'decorated.txt'), 'decorated\n') + await runGit(['add', 'decorated.txt']) + await runGit(['commit', '-qm', 'decorated commit']) + await runGit(['tag', 'compat-decorated']) + const head = (await runGit(['rev-parse', 'HEAD'])).stdout.trim() + + const log = await runGit([ + 'log', + `--format=${GIT_HISTORY_COMMIT_FORMAT}`, + '-z', + '--decorate=full', + '-n1', + head + ]) + + expect(log.stdout.includes('%(decorate')).toBe(!supports(2, 43)) + + const [item] = parseGitHistoryLog(log.stdout) + expect(item?.id).toBe(head) + expect(item?.subject).toBe('decorated commit') + expect(item?.references?.map((ref) => ref.id)).toContain('refs/tags/compat-decorated') + }) }) diff --git a/src/shared/git-history-log-parser.ts b/src/shared/git-history-log-parser.ts index 8f34002f0cc..ddc354513b9 100644 --- a/src/shared/git-history-log-parser.ts +++ b/src/shared/git-history-log-parser.ts @@ -2,9 +2,15 @@ import type { GitHistoryItem, GitHistoryItemRef } from './git-history-types' import { iterateNulDelimitedFields } from './nul-delimited-fields' const GIT_HISTORY_DECORATION_SEPARATOR = '\x1f' +const GIT_HISTORY_LEGACY_DECORATION_SEPARATOR = ',' +// Why %D too: %(decorate:…) is Git 2.43+, and older Git echoes it verbatim and exits zero. +// Callers must pass --decorate=full; both fields emit short names otherwise, which parse to no refs. export const GIT_HISTORY_COMMIT_FORMAT = - '%H%n%aN%n%aE%n%at%n%ct%n%P%n%(decorate:prefix=,suffix=,separator=%x1f)%n%B' + '%H%n%aN%n%aE%n%at%n%ct%n%P%n%(decorate:prefix=,suffix=,separator=%x1f)%n%D%n%B' + +// Why exact-match: no ref name may contain the \x1f an old Git echoes here. +const UNEXPANDED_DECORATE_PLACEHOLDER = `%(decorate:prefix=,suffix=,separator=${GIT_HISTORY_DECORATION_SEPARATOR})` export function shortGitHash(hash: string): string { return hash.slice(0, 7) @@ -15,17 +21,18 @@ function commitSubject(message: string): string { return firstLine || '(no commit message)' } -function parseGitDecorationRefs(raw: string, revision: string): GitHistoryItemRef[] { +function parseGitDecorationRefs( + raw: string, + revision: string, + separator: string +): GitHistoryItemRef[] { if (!raw.trim()) { return [] } const refs: GitHistoryItemRef[] = [] - // Why: Git permits commas in ref names, so Orca's git log format uses a - // control-character separator that Git ref names cannot contain. - const parts = raw.includes(GIT_HISTORY_DECORATION_SEPARATOR) - ? raw.split(GIT_HISTORY_DECORATION_SEPARATOR) - : raw.split(',') + // Why passed in: a lone decoration carries no separator, so sniffing `raw` split `feat,one`. + const parts = raw.split(separator) for (const part of parts) { const ref = part.trim() @@ -115,8 +122,10 @@ export function parseGitHistoryLog(stdout: string): GitHistoryItem[] { const authorEmail = lines[2] ?? '' const authorDateSeconds = Number.parseInt(lines[3] ?? '', 10) const parents = (lines[5] ?? '').trim() - const decorations = lines[6] ?? '' - const message = lines.slice(7).join('\n').replace(/\n$/, '') + const decorateField = lines[6] ?? '' + const isLegacyGit = decorateField === UNEXPANDED_DECORATE_PLACEHOLDER + const decorations = isLegacyGit ? (lines[7] ?? '') : decorateField + const message = lines.slice(8).join('\n').replace(/\n$/, '') items.push({ id: hash, @@ -127,7 +136,11 @@ export function parseGitHistoryLog(stdout: string): GitHistoryItem[] { authorEmail: authorEmail || undefined, displayId: shortGitHash(hash), timestamp: Number.isFinite(authorDateSeconds) ? authorDateSeconds * 1000 : undefined, - references: parseGitDecorationRefs(decorations, hash) + references: parseGitDecorationRefs( + decorations, + hash, + isLegacyGit ? GIT_HISTORY_LEGACY_DECORATION_SEPARATOR : GIT_HISTORY_DECORATION_SEPARATOR + ) }) } return items diff --git a/src/shared/git-history.test.ts b/src/shared/git-history.test.ts index 54aac202c71..617fa33c2ef 100644 --- a/src/shared/git-history.test.ts +++ b/src/shared/git-history.test.ts @@ -16,6 +16,7 @@ function logRecord({ hash, parents = [], decorations = '', + legacyDecorations = '', message, author = 'Ada Lovelace', timestamp = 1_700_000_000 @@ -23,6 +24,7 @@ function logRecord({ hash: string parents?: string[] decorations?: string + legacyDecorations?: string message: string author?: string timestamp?: number @@ -35,6 +37,7 @@ function logRecord({ String(timestamp), parents.join(' '), decorations, + legacyDecorations, message ].join('\n')}\0` } @@ -94,8 +97,12 @@ describe('git history parsing', () => { const stdout = logRecord({ hash: HEAD_OID, parents: [BASE_OID], - decorations: - 'HEAD -> refs/heads/feature, refs/remotes/origin/HEAD -> refs/remotes/origin/feature, refs/remotes/origin/feature, tag: refs/tags/v1.0.0', + decorations: [ + 'HEAD -> refs/heads/feature', + 'refs/remotes/origin/HEAD -> refs/remotes/origin/feature', + 'refs/remotes/origin/feature', + 'tag: refs/tags/v1.0.0' + ].join(DECORATION_SEPARATOR), message: 'feat: add graph\n\nbody line' }) @@ -117,6 +124,39 @@ describe('git history parsing', () => { ]) }) + it('falls back to %D decorations when Git predates the %(decorate:…) placeholder', () => { + // Why: Git < 2.43 echoes the placeholder and exits zero (#15507). + const stdout = logRecord({ + hash: HEAD_OID, + decorations: `%(decorate:prefix=,suffix=,separator=${DECORATION_SEPARATOR})`, + legacyDecorations: 'HEAD -> refs/heads/feature, tag: refs/tags/v1.0.0', + message: 'feat: add graph' + }) + + const [item] = parseGitHistoryLog(stdout) + + expect(item?.subject).toBe('feat: add graph') + expect(item?.references?.map((ref) => [ref.id, ref.name, ref.category])).toEqual([ + ['refs/heads/feature', 'feature', 'branches'], + ['refs/tags/v1.0.0', 'v1.0.0', 'tags'] + ]) + }) + + it('keeps a comma inside a lone decoration, which carries no separator', () => { + // Why: a lone decoration carries no separator, so sniffing for \x1f split it in two. + const stdout = logRecord({ + hash: HEAD_OID, + decorations: 'HEAD -> refs/heads/feat,one', + message: 'initial' + }) + + const [item] = parseGitHistoryLog(stdout) + + expect(item?.references?.map((ref) => [ref.id, ref.name])).toEqual([ + ['refs/heads/feat,one', 'feat,one'] + ]) + }) + it('preserves commas inside branch and tag decoration names', () => { const stdout = logRecord({ hash: HEAD_OID, From 1e82f66e80c6891d5e9296dd14e7512fcfe45fbb Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:15:15 -0700 Subject: [PATCH 05/16] fix(agents): clear the unread completion marker when acknowledging agents (#17924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acknowledging is one action against two records, but only clearTerminalPaneUnread cleared unreadAgentCompletionPanes. Acking from the Activity page, the dashboard drawer or the popout bridge left the tab dot, the ⌘J row and the floating-workspace dot lit with nothing left to read; only the terminal-view auto-ack path cleared both. Cleared inside the existing set so one ack is one commit, and only the agent marker is touched — clearTerminalPaneUnread also drops unreadTerminalPanes, which would silence a BEL the user never saw. Refs #15445 (step 2 of that issue's fix; steps 1 and 3 remain open). Co-authored-by: kaluli123123 <295758798+kaluli123123@users.noreply.github.com> --- .../slices/agent-status-ack-cleanup.test.ts | 28 +++++++++++++++++++ .../src/store/slices/ui-slice-test-harness.ts | 2 ++ .../store/slices/ui/ui-slice-agent-actions.ts | 17 ++++++++++- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/store/slices/agent-status-ack-cleanup.test.ts b/src/renderer/src/store/slices/agent-status-ack-cleanup.test.ts index b763262a22d..4ca35b9bdc2 100644 --- a/src/renderer/src/store/slices/agent-status-ack-cleanup.test.ts +++ b/src/renderer/src/store/slices/agent-status-ack-cleanup.test.ts @@ -117,3 +117,31 @@ describe('acknowledgedAgentsByPaneKey cleanup on teardown', () => { expect(ackAt < newEntry.stateStartedAt).toBe(true) }) }) + +// Why: only the terminal-view path cleared unreadAgentCompletionPanes, so an +// Activity-page ack left the tab dot lit. +describe('acknowledgeAgents clears the unread agent-completion marker', () => { + it('drops the pane from unreadAgentCompletionPanes', () => { + const store = createTestStore() + store.getState().setAgentStatus('tab-1:0', { state: 'done', prompt: 'p', agentType: 'claude' }) + store.getState().markAgentCompletionPaneUnread('tab-1:0') + expect(store.getState().unreadAgentCompletionPanes['tab-1:0']).toBe(true) + + store.getState().acknowledgeAgents(['tab-1:0']) + + expect(store.getState().unreadAgentCompletionPanes['tab-1:0']).toBeUndefined() + }) + + it('leaves other panes and the terminal-bell unread map untouched', () => { + const store = createTestStore() + store.getState().markAgentCompletionPaneUnread('tab-1:0') + store.getState().markAgentCompletionPaneUnread('tab-2:0') + store.getState().markTerminalPaneUnread('tab-1:0') + + store.getState().acknowledgeAgents(['tab-1:0']) + + expect(store.getState().unreadAgentCompletionPanes['tab-2:0']).toBe(true) + // Why: a BEL is a separate signal; acking the agent must not silence it. + expect(store.getState().unreadTerminalPanes['tab-1:0']).toBe(true) + }) +}) diff --git a/src/renderer/src/store/slices/ui-slice-test-harness.ts b/src/renderer/src/store/slices/ui-slice-test-harness.ts index 318b24afef8..b8bbcf24b85 100644 --- a/src/renderer/src/store/slices/ui-slice-test-harness.ts +++ b/src/renderer/src/store/slices/ui-slice-test-harness.ts @@ -20,6 +20,8 @@ export function createUIStore(): StoreApi { combinedDiffFileTreeWidth: 256, rightSidebarTab: 'explorer', rightSidebarExplorerView: 'files', + // Why: acknowledgeAgents clears the agent-completion marker the terminal slice owns. + unreadAgentCompletionPanes: {}, ...createSettingsSearchState(args[0]), ...createWorktreeNavHistorySlice(...(args as Parameters)), ...createUISlice(...(args as Parameters)) diff --git a/src/renderer/src/store/slices/ui/ui-slice-agent-actions.ts b/src/renderer/src/store/slices/ui/ui-slice-agent-actions.ts index b1401e7d416..9fa15ffb81e 100644 --- a/src/renderer/src/store/slices/ui/ui-slice-agent-actions.ts +++ b/src/renderer/src/store/slices/ui/ui-slice-agent-actions.ts @@ -217,7 +217,16 @@ export function createUiAgentActions( const migrationUnsupported = Object.values(s.migrationUnsupportedByPtyId ?? {}) // Why: only reallocate if an ack advances; compare prev | null = null + // Why: one ack, two records — leaving the completion marker set keeps the tab dot, + // the ⌘J row and the floating-workspace dot lit with nothing left to read. + let nextUnreadCompletions: Record | null = null for (const key of paneKeys) { + if (s.unreadAgentCompletionPanes[key]) { + if (nextUnreadCompletions === null) { + nextUnreadCompletions = { ...s.unreadAgentCompletionPanes } + } + delete nextUnreadCompletions[key] + } const prev = s.acknowledgedAgentsByPaneKey[key] ?? 0 // Why not plain Date.now(): a remote/SSH execution host can stamp a turn ahead of this clock, // and every unread rule is `ackAt < turnTimestamp`. A behind-the-turn ack can never clear the @@ -258,7 +267,13 @@ export function createUiAgentActions( next[key] = stamp } } - return next ? { acknowledgedAgentsByPaneKey: next } : s + if (!next && !nextUnreadCompletions) { + return s + } + return { + ...(next ? { acknowledgedAgentsByPaneKey: next } : {}), + ...(nextUnreadCompletions ? { unreadAgentCompletionPanes: nextUnreadCompletions } : {}) + } }) const notificationIds = [...notificationIdsToDismiss] if (notificationIds.length > 0 && typeof window !== 'undefined') { From 519af49a589e2c95acfe972a844cecc4fcdaa00e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:04:44 -0700 Subject: [PATCH 06/16] fix(dev): keep the shared Electron dist writable for the dev app pn dev crashes on macOS in any worktree that adopted the shared Electron dist. publishSharedElectronDist marks the cache entry read-only, which hardlink sharing needs, but clonefile preserves mode -- so the dist lands 0555, the dev runner copies it into out/electron-dev unchanged, and the first plutil -replace on Info.plist fails with a permission error. The shipped zip has that file at 0644; on disk it is 0555, so the mode is ours, not upstream's. copyPrivateTree now restores write permission. Its contract is a private tree the caller goes on to patch, and its one production caller is the dev runner. The test that should have caught this ran the wrapper with stdio: 'ignore', so a hard crash presented as a bare 20s timeout. It now captures the wrapper's output into the failure message, and waits long enough for the two synchronous swiftc builds and a codesign --deep over ~280MB that precede the assertion. --- config/scripts/space-sharing-copy.mjs | 50 +++++++--- config/scripts/space-sharing-copy.test.ts | 35 +++++++ .../startup/run-electron-vite-dev.test.ts | 93 +++++++++++++------ 3 files changed, 139 insertions(+), 39 deletions(-) diff --git a/config/scripts/space-sharing-copy.mjs b/config/scripts/space-sharing-copy.mjs index 191bd8bffdc..01e9c8ac5ef 100644 --- a/config/scripts/space-sharing-copy.mjs +++ b/config/scripts/space-sharing-copy.mjs @@ -110,32 +110,60 @@ export function makeTreeReadOnly(targetPath, chmod = chmodSync) { chmod(targetPath, 0o755) } +/** + * Restore owner write permission across a private copy. + * + * Counterpart to `makeTreeReadOnly`: clonefile, reflink and `cpSync` all carry the source's mode + * across, so a tree copied from the write-protected shared cache lands read-only and every patch + * the caller then makes -- `plutil -replace`, `codesign` -- fails with EACCES. Only the owner bit + * comes back; group and other stay as the source left them. + */ +export function makeTreeWritable(targetPath, chmod = chmodSync) { + for (const entry of readdirSync(targetPath, { withFileTypes: true })) { + const entryPath = join(targetPath, entry.name) + if (entry.isDirectory()) { + makeTreeWritable(entryPath, chmod) + } else if (!entry.isSymbolicLink()) { + const mode = statSync(entryPath, { throwIfNoEntry: false })?.mode + chmod(entryPath, mode === undefined ? 0o644 : mode | 0o200) + } + } + chmod(targetPath, 0o755) +} + /** * Share storage when possible, otherwise copy the bytes. * * Never hardlinks: this is for trees the caller goes on to patch, where shared inodes would write - * through into the source. + * through into the source. The copy is unprotected on the way out for the same reason -- a private + * tree the caller cannot write to is useless to it. */ export function copyPrivateTree(sourcePath, destinationPath, options = {}) { const platform = options.platform ?? process.platform const copy = options.copy ?? copyTreeVerbatim + const unprotect = options.unprotect ?? makeTreeWritable const privateMechanisms = new Set(['clone', 'reflink']) + let result = { mechanism: null, copyError: null } if (getShareMechanisms(platform).some((mechanism) => privateMechanisms.has(mechanism))) { try { - const mechanism = shareTree(sourcePath, destinationPath, { - ...options, - hardlink: () => { - throw new Error('hardlinks would not be private') - } - }) - return { mechanism, copyError: null } + result = { + mechanism: shareTree(sourcePath, destinationPath, { + ...options, + hardlink: () => { + throw new Error('hardlinks would not be private') + } + }), + copyError: null + } } catch (copyError) { copy(sourcePath, destinationPath) - return { mechanism: null, copyError } + result = { mechanism: null, copyError } } + } else { + copy(sourcePath, destinationPath) } - copy(sourcePath, destinationPath) - return { mechanism: null, copyError: null } + unprotect(destinationPath) + return result } function copyTreeVerbatim(sourcePath, destinationPath) { diff --git a/config/scripts/space-sharing-copy.test.ts b/config/scripts/space-sharing-copy.test.ts index 3ce35aae25e..351f44b286e 100644 --- a/config/scripts/space-sharing-copy.test.ts +++ b/config/scripts/space-sharing-copy.test.ts @@ -19,6 +19,7 @@ import { copyPrivateTree, hardlinkTree, makeTreeReadOnly, + makeTreeWritable, shareTree } from './space-sharing-copy.mjs' @@ -170,7 +171,41 @@ describe('makeTreeReadOnly', () => { ) }) +describe('makeTreeWritable', () => { + it.runIf(process.platform !== 'win32')('undoes makeTreeReadOnly for the owner', () => { + const { source } = makeTree() + makeTreeReadOnly(source) + makeTreeWritable(source) + const file = path.join(source, 'nested', 'file') + expect(statSync(file).mode & 0o200).toBe(0o200) + expect(() => writeFileSync(file, 'mutated')).not.toThrow() + }) + + it.runIf(process.platform !== 'win32')('adds no write permission beyond the owner', () => { + const { source } = makeTree() + const executable = path.join(source, 'electron') + writeFileSync(executable, 'binary') + chmodSync(executable, 0o555) + makeTreeWritable(source) + expect(statSync(executable).mode & 0o777).toBe(0o755) + }) +}) + describe('copyPrivateTree', () => { + it.runIf(process.platform !== 'win32')( + 'hands back a tree the caller can patch, even from a write-protected source', + () => { + const { root, source } = makeTree() + const destination = path.join(root, 'private') + makeTreeReadOnly(source) + copyPrivateTree(source, destination) + // The regression this guards: the shared Electron dist is read-only, clonefile/reflink/cpSync + // all carry that across, and `pn dev` then died patching the copied bundle's Info.plist. + expect(() => writeFileSync(path.join(destination, 'nested', 'file'), 'patched')).not.toThrow() + expect(readFileSync(path.join(source, 'nested', 'file'), 'utf8')).toBe('contents') + } + ) + it('never hardlinks, because the caller patches what it gets back', () => { const { root, source } = makeTree() const destination = path.join(root, 'private') diff --git a/src/main/startup/run-electron-vite-dev.test.ts b/src/main/startup/run-electron-vite-dev.test.ts index 2146563373d..73d1bb21cdc 100644 --- a/src/main/startup/run-electron-vite-dev.test.ts +++ b/src/main/startup/run-electron-vite-dev.test.ts @@ -105,6 +105,56 @@ function devWrapperTestEnv(extra: NodeJS.ProcessEnv): NodeJS.ProcessEnv { return { ...env, ...extra } } +/** + * What the two cases below wait on: a ~280MB clone of Electron.app, two swiftc + * helper builds, and `codesign --deep` over the result. Six seconds on an idle + * machine; the swiftc builds alone pass fifteen when this file runs inside the + * full suite and every core is taken. The generous ceiling only costs time on a + * run that is already failing. + */ +const PREPARE_TIMEOUT_MS = 90_000 + +/** + * Spawns the wrapper with its output retained. + * + * Why retained: the wrapper reports its own failures on stderr, and discarding + * them turned a crash in prepare into a bare "Timed out waiting for condition" + * with nothing to act on. + */ +function spawnDevWrapper( + args: string[], + env: NodeJS.ProcessEnv +): { wrapper: ChildProcess; readOutput: () => string } { + const wrapper = spawn(process.execPath, args, { + cwd: resolve('.'), + env, + stdio: ['ignore', 'pipe', 'pipe'] + }) + let output = '' + const collect = (chunk: Buffer): void => { + output += chunk.toString() + } + wrapper.stdout?.on('data', collect) + wrapper.stderr?.on('data', collect) + return { wrapper, readOutput: () => output } +} + +async function waitForEnvFile(envFile: string, readOutput: () => string): Promise { + try { + await waitFor(() => { + try { + return readFileSync(envFile, 'utf8').trim().length > 0 + } catch { + return false + } + }, PREPARE_TIMEOUT_MS) + } catch (error) { + throw new Error( + `${(error as Error).message}: the dev wrapper never wrote ${envFile}. Wrapper output:\n${readOutput() || '(none)'}` + ) + } +} + describe('run-electron-vite-dev', () => { afterEach(async () => { for (const pid of processesToCleanUp) { @@ -351,26 +401,19 @@ describe('run-electron-vite-dev', () => { async function runWrapper(runId: string): Promise<{ electronExecPath: string }> { const pidFile = join(tempDir, `${runId}.pid`) const envFile = join(tempDir, `${runId}.json`) - const wrapper = spawn(process.execPath, [wrapperPath, '--remote-debugging-port=9448'], { - cwd: resolve('.'), - env: { + const { wrapper, readOutput } = spawnDevWrapper( + [wrapperPath, '--remote-debugging-port=9448'], + { ...baseEnv, ORCA_DEV_WRAPPER_TEST_PID_FILE: pidFile, ORCA_DEV_WRAPPER_TEST_ENV_FILE: envFile - }, - stdio: 'ignore' - }) + } + ) expect(wrapper.pid).toBeTypeOf('number') processesToCleanUp.add(wrapper.pid!) - await waitFor(() => { - try { - return readFileSync(envFile, 'utf8').trim().length > 0 - } catch { - return false - } - }, 20000) + await waitForEnvFile(envFile, readOutput) const trackedPids = trackPidFile(pidFile) @@ -409,7 +452,8 @@ describe('run-electron-vite-dev', () => { } } }, - 30000 + // Two full prepares, each budgeted at PREPARE_TIMEOUT_MS. + PREPARE_TIMEOUT_MS * 2 + 30_000 ) it.skipIf(process.platform !== 'darwin')( @@ -421,9 +465,9 @@ describe('run-electron-vite-dev', () => { const wrapperPath = resolve('config/scripts/run-electron-vite-dev.mjs') const fakeCliPath = resolve('src/main/startup/__fixtures__/fake-electron-vite-dev-cli.mjs') - const wrapper = spawn(process.execPath, [wrapperPath, '--remote-debugging-port=9448'], { - cwd: resolve('.'), - env: devWrapperTestEnv({ + const { wrapper, readOutput } = spawnDevWrapper( + [wrapperPath, '--remote-debugging-port=9448'], + devWrapperTestEnv({ ORCA_ELECTRON_VITE_CLI: fakeCliPath, ORCA_SKIP_DEV_CLI_PREPARE: '1', ORCA_SKIP_DEV_WEB_PREPARE: '1', @@ -431,20 +475,13 @@ describe('run-electron-vite-dev', () => { ORCA_DEV_WRAPPER_TEST_ENV_FILE: envFile, ORCA_DEV_BRANCH: 'feature/framework-symlinks', ORCA_DEV_WORKTREE_NAME: 'symlink-ui' - }), - stdio: 'ignore' - }) + }) + ) expect(wrapper.pid).toBeTypeOf('number') processesToCleanUp.add(wrapper.pid!) - await waitFor(() => { - try { - return readFileSync(envFile, 'utf8').trim().length > 0 - } catch { - return false - } - }, 20000) + await waitForEnvFile(envFile, readOutput) const trackedPids = trackPidFile(pidFile) @@ -464,6 +501,6 @@ describe('run-electron-vite-dev', () => { await stopWrapperAndTrackedPids(wrapper, trackedPids) }, - 30000 + PREPARE_TIMEOUT_MS + 30_000 ) }) From a2aea5d0b05db182ae1315f608dfc6d00fa487ed Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 01:03:06 -0700 Subject: [PATCH 07/16] fix(persistence): sweep rows owned by deregistered repo ids at load Deregistering a project stranded every row it owned. Each pruning path is gated on the repo still being in `state.repos`, so once an id leaves the catalogue its metadata, identity aliases, lineage and session rows became unreachable forever -- and on a paired client they rendered as phantom worktrees under an "Unknown" project. Reconcile against the repo catalogue on load instead: any repo id that owns rows but is absent from `state.repos` has its rows removed through the same path `removeProject` uses. Host-independent and session-independent, because an orphan has no owner that could object -- which is also why this reaches a client's mirror of a remote host's session partition, something no local removal can do. Only a full `::` locator seeds the orphan set; bare keys can be folder workspace ids or repo-keyed revisions, and guessing wrong there would delete live state. `retiredWorktreeNamesByRepo` is deliberately untouched so a re-added repo cannot reissue a name onto a cwd that still holds a prior occupant's agent state. Test fixtures that wrote worktree rows without registering their repo were relying on orphans surviving a reload; they now register the repo they name. Refs #17776 --- .../profile-project-worktree-identity.ts | 19 +- ...ence-cohort-and-identity-migration.test.ts | 2 + ...rsistence-cross-host-pane-identity.test.ts | 6 + ...sistence-deregistered-repo-residue.test.ts | 176 ++++++++++++++++++ ...sistence-host-partitioned-sessions.test.ts | 10 + src/main/persistence-initial-load.test.ts | 2 + ...sistence-native-chat-tab-view-mode.test.ts | 2 +- src/main/persistence-repo-lifecycle.test.ts | 6 +- src/main/persistence-settings-update.test.ts | 2 +- ...sistence-ssh-targets-and-pane-keys.test.ts | 4 +- ...tence-worktree-lineage-and-backups.test.ts | 3 + .../repo-lifecycle-operations.ts | 50 +++++ src/main/persistence/loading-store/store.ts | 10 +- .../deregistered-repo-residue.ts | 82 ++++++++ .../ssh-reattach-pane-cardinality.test.ts | 10 +- .../worktree-identity-persistence.test.ts | 26 ++- 16 files changed, 389 insertions(+), 21 deletions(-) create mode 100644 src/main/persistence-deregistered-repo-residue.test.ts create mode 100644 src/main/persistence/tracking-repos/deregistered-repo-residue.ts diff --git a/src/main/orca-profiles/profile-project-worktree-identity.ts b/src/main/orca-profiles/profile-project-worktree-identity.ts index 1586a0e0120..f4063cbb372 100644 --- a/src/main/orca-profiles/profile-project-worktree-identity.ts +++ b/src/main/orca-profiles/profile-project-worktree-identity.ts @@ -66,15 +66,18 @@ export function rekeyOwnerKey( return null } -export function ownerKeyBelongsToRepo(ownerKey: string, repoId: string): boolean { - const rawOwnerKey = isWorktreeHostIdentity(ownerKey) - ? getWorktreeIdFromHostIdentity(ownerKey) - : ownerKey - if (isRepoWorktreeId(repoId, rawOwnerKey)) { - return true +/** The worktree locator an owner key names, or null when the key is not worktree-scoped. */ +export function ownerKeyWorktreeId(ownerKey: string): string | null { + const scope = parseWorkspaceKey(ownerKey) + if (scope) { + return scope.type === 'worktree' ? scope.worktreeId : null } - const parsed = parseWorkspaceKey(ownerKey) - return parsed?.type === 'worktree' && isRepoWorktreeId(repoId, parsed.worktreeId) + return isWorktreeHostIdentity(ownerKey) ? getWorktreeIdFromHostIdentity(ownerKey) : ownerKey +} + +export function ownerKeyBelongsToRepo(ownerKey: string, repoId: string): boolean { + const worktreeId = ownerKeyWorktreeId(ownerKey) + return worktreeId !== null && isRepoWorktreeId(repoId, worktreeId) } export function removeRepoWorktreeRecord( diff --git a/src/main/persistence-cohort-and-identity-migration.test.ts b/src/main/persistence-cohort-and-identity-migration.test.ts index 4081672c821..a894b8a4c63 100644 --- a/src/main/persistence-cohort-and-identity-migration.test.ts +++ b/src/main/persistence-cohort-and-identity-migration.test.ts @@ -397,6 +397,8 @@ describe('Store.migrateWorktreeIdentity', () => { it('moves persisted mobile selections across reloads', async () => { const store = await createStore() + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + store.addRepo(makeRepo({ id: 'repo1', path: '/repo1' })) store.setMobileClientTabSelections({ 'device-a': { [OLD]: { activeTabId: 'tab-1', activeGroupId: null, activeTabIdByGroupId: {} } diff --git a/src/main/persistence-cross-host-pane-identity.test.ts b/src/main/persistence-cross-host-pane-identity.test.ts index 2b6a70da934..479d3727837 100644 --- a/src/main/persistence-cross-host-pane-identity.test.ts +++ b/src/main/persistence-cross-host-pane-identity.test.ts @@ -10,6 +10,7 @@ import { createStore, writeDataFile, readDataFile, + makeRepo, makeTerminalTab } from './persistence-test-harness' @@ -53,6 +54,11 @@ describe('cross-host pane identity migration', () => { it('refuses hostless alias and acknowledgement rewrites for a tab id two partitions share', async () => { writeDataFile({ schemaVersion: 1, + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + repos: [ + makeRepo({ id: 'repo-local', path: '/repo-local' }), + makeRepo({ id: 'repo-a', path: '/repo-a' }) + ], workspaceSession: makeLegacyPaneSession('repo-local', 'local-pty'), workspaceSessionsByHostId: { 'ssh:host-a': makeLegacyPaneSession('repo-a', 'pty-a') diff --git a/src/main/persistence-deregistered-repo-residue.test.ts b/src/main/persistence-deregistered-repo-residue.test.ts new file mode 100644 index 00000000000..f1ecbde3213 --- /dev/null +++ b/src/main/persistence-deregistered-repo-residue.test.ts @@ -0,0 +1,176 @@ +// Why this file exists: deregistering a project used to strand every row it owned. No sweeper could +// reach them -- the missing-directory prune is gated on the repo still being registered, and a +// paired client's mirror of a remote host's rows is keyed by ids that client never registers, so the +// owning host's removal never reached it (#17776). +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { rmSync, mkdtempSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { getDefaultWorkspaceSession } from '../shared/constants' +import { composeWorktreeHostIdentity } from '../shared/worktree/host-qualified-identity' +import { folderWorkspaceKey } from '../shared/workspace-scope' +import type { PersistedState } from '../shared/persisted-state-types' +import { + testState, + createStore, + writeDataFile, + readDataFile, + makeRepo, + makeTerminalTab +} from './persistence-test-harness' + +vi.mock('./ssh/ssh-config-parser', () => ({ + loadUserSshConfig: vi.fn(), + sshConfigHostsToTargets: vi.fn() +})) + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) +vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: vi.fn().mockReturnValue({}) })) + +const LIVE_REPO = 'live-repo' +const GONE_REPO = 'gone-repo' +const LIVE_WORKTREE = `${LIVE_REPO}::/workspace/live` +const GONE_WORKTREE = `${GONE_REPO}::/workspace/orphan` +const RUNTIME_HOST = 'runtime:env-a' + +const sessionFor = (worktreeId: string, tabId = 'tab-1') => ({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [worktreeId]: [makeTerminalTab({ id: tabId, worktreeId })] + }, + activeTabTypeByWorktree: { [worktreeId]: 'terminal' as const }, + lastVisitedAtByWorktreeId: { [worktreeId]: 123 }, + // The residue `profile-project-session-field-disposition` flags as leaking on repo removal. + sleepingAgentSessionsByPaneKey: { + [`${tabId}:leaf-1`]: { + paneKey: `${tabId}:leaf-1`, + tabId, + worktreeId, + agent: 'codex' as const, + providerSession: { key: 'session_id' as const, id: 'sess-1' }, + prompt: 'sleeping', + state: 'waiting' as const, + capturedAt: 1, + updatedAt: 1, + origin: 'worktree-sleep' as const + } + } +}) + +describe('deregistered repo residue', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-orphan-sweep-')) + }) + + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + it('drops metadata, identity rows and sessions owned by an unregistered repo id', async () => { + const seed = await createStore() + seed.addRepo(makeRepo({ id: LIVE_REPO, path: '/workspace/live' })) + seed.addRepo(makeRepo({ id: GONE_REPO, path: '/workspace/orphan' })) + seed.setWorktreeMetaForHost(LIVE_WORKTREE, 'local', { displayName: 'Live' }) + seed.setWorktreeMetaForHost(GONE_WORKTREE, 'local', { displayName: 'Orphan' }) + seed.setWorkspaceSession(sessionFor(GONE_WORKTREE), 'local') + seed.flush() + + // Deregister by hand: the point is that a row can outlive its repo however that happened. + const persisted = readDataFile() as PersistedState + persisted.repos = persisted.repos.filter((repo) => repo.id !== GONE_REPO) + writeDataFile(persisted) + + const reloaded = await createStore() + reloaded.flush() + const swept = readDataFile() as PersistedState + + expect(Object.keys(swept.worktreeMeta)).toEqual([LIVE_WORKTREE]) + expect(swept.worktreeIdentityAliases).not.toHaveProperty( + composeWorktreeHostIdentity('local', GONE_WORKTREE) + ) + expect(Object.keys(swept.worktreeMetaByIdentity ?? {})).toHaveLength(1) + const session = swept.workspaceSession + expect(session.tabsByWorktree).toEqual({}) + expect(session.lastVisitedAtByWorktreeId).toEqual({}) + expect(session.activeTabTypeByWorktree).toEqual({}) + expect(session.sleepingAgentSessionsByPaneKey ?? {}).toEqual({}) + }) + + it("sweeps a remote host's session partition the owning host's removal can never reach", async () => { + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: LIVE_REPO, path: '/workspace/live' })], + worktreeMeta: {}, + workspaceSessionsByHostId: { + [RUNTIME_HOST]: sessionFor(GONE_WORKTREE) + } + }) + + const store = await createStore() + store.flush() + + const partition = store.getWorkspaceSession(RUNTIME_HOST) + expect(partition.tabsByWorktree).toEqual({}) + expect(partition.activeTabTypeByWorktree).toEqual({}) + }) + + it('keeps rows for every registered repo, on any execution host', async () => { + const remoteWorktree = `${LIVE_REPO}::/home/user/remote` + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: LIVE_REPO, path: '/home/user/live', executionHostId: RUNTIME_HOST })], + worktreeMeta: { [remoteWorktree]: { hostId: RUNTIME_HOST, status: 'active' } }, + workspaceSessionsByHostId: { [RUNTIME_HOST]: sessionFor(remoteWorktree) } + }) + + const store = await createStore() + + expect(store.getWorktreeMeta(remoteWorktree)).toBeDefined() + const partition = store.getWorkspaceSession(RUNTIME_HOST) + expect(partition.tabsByWorktree[remoteWorktree]).toHaveLength(1) + // Also proves the sleeping-agent fixture is well-formed, so the sweep assertions above bite. + expect(Object.keys(partition.sleepingAgentSessionsByPaneKey ?? {})).toHaveLength(1) + }) + + it('leaves folder-workspace session rows alone: their keys name no repo', async () => { + const workspaceKey = folderWorkspaceKey('folder-1') + writeDataFile({ + schemaVersion: 1, + repos: [], + worktreeMeta: {}, + workspaceSession: { + ...getDefaultWorkspaceSession(), + lastVisitedAtByWorktreeId: { [workspaceKey]: 7 } + } + }) + + const store = await createStore() + + expect(store.getWorkspaceSession('local').lastVisitedAtByWorktreeId).toEqual({ + [workspaceKey]: 7 + }) + }) + + // Why: a sweep that dirtied every launch would rewrite the profile forever and mask real changes. + it('leaves a profile with no orphans byte-identical across reloads', async () => { + const seed = await createStore() + seed.addRepo(makeRepo({ id: LIVE_REPO, path: '/workspace/live' })) + seed.setWorktreeMetaForHost(LIVE_WORKTREE, 'local', { displayName: 'Live' }) + seed.setWorkspaceSession(sessionFor(LIVE_WORKTREE), 'local') + seed.flush() + + const canonicalizing = await createStore() + canonicalizing.flush() + const canonical = JSON.stringify(readDataFile()) + + const reloaded = await createStore() + reloaded.flush() + + expect(JSON.stringify(readDataFile())).toBe(canonical) + }) +}) diff --git a/src/main/persistence-host-partitioned-sessions.test.ts b/src/main/persistence-host-partitioned-sessions.test.ts index 023737ed386..aa2e46e52fd 100644 --- a/src/main/persistence-host-partitioned-sessions.test.ts +++ b/src/main/persistence-host-partitioned-sessions.test.ts @@ -123,6 +123,9 @@ describe('Store host-partitioned workspace sessions', () => { } }) + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + const makeRepos = (...repoIds: string[]) => repoIds.map((id) => makeRepo({ id, path: `/${id}` })) + it('migrates a legacy workspaceSession blob into the local partition', async () => { writeDataFile({ schemaVersion: 1, @@ -194,6 +197,7 @@ describe('Store host-partitioned workspace sessions', () => { writeDataFile({ schemaVersion: 1, workspaceSession: makeHostSession('local-repo'), + repos: makeRepos('repo-ssh'), workspaceSessionsByHostId: { 'ssh:ssh-1': makeLegacyPaneHostSession('repo-ssh', 'remote-pty') }, @@ -224,6 +228,7 @@ describe('Store host-partitioned workspace sessions', () => { writeDataFile({ schemaVersion: 1, workspaceSession: makeHostSession('local-repo'), + repos: makeRepos('repo-a', 'repo-b'), workspaceSessionsByHostId: { 'ssh:host-a': makeLegacyPaneHostSession('repo-a', 'pty-a'), 'ssh:host-b': makeLegacyPaneHostSession('repo-b', 'pty-b') @@ -488,6 +493,7 @@ describe('Store host-partitioned workspace sessions', () => { it('removes one orphaned worktree with a host-scoped topology fence', async () => { const store = await createStore() + store.addRepo(makeRepo({ id: 'repo-gone', path: '/repo-gone' })) const worktreeId = 'repo-gone::/workspace/stale' const session = { ...makeHostSession('repo-gone'), @@ -728,6 +734,7 @@ describe('Store host-partitioned workspace sessions', () => { const worktreeId = 'repo-1::/worktree' writeDataFile({ schemaVersion: 1, + repos: makeRepos('repo-1'), workspaceSessionsByHostId: { 'runtime:good': makeHostSession('good-repo'), // activeRepoId must be string|null; a number fails the zod parse. @@ -753,6 +760,7 @@ describe('Store host-partitioned workspace sessions', () => { const worktreeId = 'repo-1::/worktree' writeDataFile({ schemaVersion: 1, + repos: makeRepos('repo-1'), workspaceSession: { ...makeHostSession('local-repo'), // A projected/truncated write can leave a top-level field the wrong type; @@ -813,6 +821,7 @@ describe('Store host-partitioned workspace sessions', () => { const worktreeId = 'repo-1::/worktree' const profile = await canonicalize({ schemaVersion: 1, + repos: makeRepos('repo-1'), workspaceSession: { ...makeHostSession('local-repo'), tabsByWorktree: { [worktreeId]: [makeTerminalTab({ id: 'tab-keep', worktreeId })] } @@ -845,6 +854,7 @@ describe('Store host-partitioned workspace sessions', () => { const worktreeId = 'repo-1::/worktree' const profile = await canonicalize({ schemaVersion: 1, + repos: makeRepos('repo-1'), workspaceSessionsByHostId: { 'runtime:env-a': { ...makeHostSession('runtime-repo'), diff --git a/src/main/persistence-initial-load.test.ts b/src/main/persistence-initial-load.test.ts index 6d0c0f11d5e..934ab44e1cb 100644 --- a/src/main/persistence-initial-load.test.ts +++ b/src/main/persistence-initial-load.test.ts @@ -150,6 +150,8 @@ describe('Store', () => { it('does not restore a terminal tab after its durable close flush returns', async () => { const store = await createStore() + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + store.addRepo(makeRepo({ id: 'repo-1', path: '/repo-1' })) const worktreeId = 'repo-1::/tmp/worktree-1' const tabId = 'terminal-1' const session: WorkspaceSessionState = { diff --git a/src/main/persistence-native-chat-tab-view-mode.test.ts b/src/main/persistence-native-chat-tab-view-mode.test.ts index 3e3544c7495..9bcaab15494 100644 --- a/src/main/persistence-native-chat-tab-view-mode.test.ts +++ b/src/main/persistence-native-chat-tab-view-mode.test.ts @@ -58,7 +58,7 @@ describe('Store native-chat tab viewMode persistence', () => { const WORKTREE = 'repo1::/worktree' writeDataFile({ schemaVersion: 1, - repos: [makeRepo()], + repos: [makeRepo({ id: 'repo1', path: '/repo1' })], worktreeMeta: {}, settings: {}, ui: {}, diff --git a/src/main/persistence-repo-lifecycle.test.ts b/src/main/persistence-repo-lifecycle.test.ts index 1653f78297e..66f3fc2c32e 100644 --- a/src/main/persistence-repo-lifecycle.test.ts +++ b/src/main/persistence-repo-lifecycle.test.ts @@ -737,7 +737,10 @@ describe('Store', () => { it('reassignSshTargetId persists a worktree-meta-only re-point (no matching repo)', async () => { const store = await createStore() - // A meta on the old SSH host with no repo row — the re-point must still be persisted, not memory-only. + // A meta on the old SSH host with no repo row for that host — the re-point must still be + // persisted, not memory-only. The repo id stays registered so the load-time orphan sweep, + // which only reads repo ids, leaves the row alone. + store.addRepo(makeRepo({ id: 'r1', path: '/r1' })) store.setWorktreeMeta('r1::/remote/wt', { displayName: 'wt', hostId: 'ssh:ssh-old' }) const repoIds = store.reassignSshTargetId('ssh-old', 'ssh-new') @@ -787,6 +790,7 @@ describe('Store', () => { it('reassignSshTargetId re-keys a session partition stored under the old ssh host id', async () => { const store = await createStore() + store.addRepo(makeRepo({ id: 'r1', path: '/r1' })) store.setWorkspaceSession( { activeRepoId: null, diff --git a/src/main/persistence-settings-update.test.ts b/src/main/persistence-settings-update.test.ts index dc3a3c7ebb5..9af63ca7ccd 100644 --- a/src/main/persistence-settings-update.test.ts +++ b/src/main/persistence-settings-update.test.ts @@ -708,7 +708,7 @@ describe('Store', () => { } writeDataFile({ schemaVersion: 1, - repos: [makeRepo()], + repos: [makeRepo({ id: 'repo1', path: '/repo1' })], worktreeMeta: { 'repo1::/worktree-a': { status: 'active' }, 'repo1::/worktree-b': { status: 'active' } diff --git a/src/main/persistence-ssh-targets-and-pane-keys.test.ts b/src/main/persistence-ssh-targets-and-pane-keys.test.ts index 6c186ade077..c11ecbf0bc2 100644 --- a/src/main/persistence-ssh-targets-and-pane-keys.test.ts +++ b/src/main/persistence-ssh-targets-and-pane-keys.test.ts @@ -346,7 +346,7 @@ describe('Store', () => { const acknowledgedAt = 1_700_000_000_000 writeDataFile({ schemaVersion: 1, - repos: [makeRepo()], + repos: [makeRepo({ id: 'repo1', path: '/repo1' })], worktreeMeta: {}, settings: {}, ui: { @@ -408,7 +408,7 @@ describe('Store', () => { writeDataFile({ schemaVersion: 1, - repos: [makeRepo()], + repos: [makeRepo({ id: 'repo1', path: '/repo1' })], worktreeMeta: {}, settings: {}, ui: { diff --git a/src/main/persistence-worktree-lineage-and-backups.test.ts b/src/main/persistence-worktree-lineage-and-backups.test.ts index ef5e83745be..372e8b0e33b 100644 --- a/src/main/persistence-worktree-lineage-and-backups.test.ts +++ b/src/main/persistence-worktree-lineage-and-backups.test.ts @@ -166,6 +166,8 @@ describe('Store', () => { describe('mobileClientTabSelectionsByDeviceId', () => { it('persists device tab selections across reloads and drops malformed payloads', async () => { const store = await createStore() + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + store.addRepo(makeRepo({ id: 'repo-1', path: '/repo-1' })) store.setMobileClientTabSelections({ 'device-a': { 'repo-1::/tmp/wt': { activeTabId: 'tab-1', activeGroupId: 'g1', activeTabIdByGroupId: {} } @@ -188,6 +190,7 @@ describe('Store', () => { it('prunes selections for a removed repo worktree', async () => { const store = await createStore() store.addRepo(makeRepo()) + store.addRepo(makeRepo({ id: 'other-repo', path: '/other-repo' })) store.setMobileClientTabSelections({ 'device-a': { 'r1::/tmp/wt': { diff --git a/src/main/persistence/loading-store/repo-lifecycle-operations.ts b/src/main/persistence/loading-store/repo-lifecycle-operations.ts index 095090ee1ca..c7bdbd9de37 100644 --- a/src/main/persistence/loading-store/repo-lifecycle-operations.ts +++ b/src/main/persistence/loading-store/repo-lifecycle-operations.ts @@ -12,10 +12,13 @@ import { import { mergeProjectHostSetupCompatibilityState } from '../tracking-repos/project-host-compatibility' import { RepoOrderPersistenceOperations } from '../tracking-repos/repo-order-operations' import { pruneWorktreeStateForRepo as pruneWorktreeStateForRepoOperation } from '../tracking-repos/repo-worktree-pruning' +import { collectDeregisteredRepoIds } from '../tracking-repos/deregistered-repo-residue' import { hydrateRepo as hydrateRepoOperation } from '../tracking-repos/repo-hydration' import { RepoUpdatePersistenceOperations } from '../tracking-repos/repo-update-operations' import { ProjectHostSetupPersistenceOperations } from '../tracking-repos/project-host-setup-update' import { bumpLocalWorktreeScanGeneration } from '../../local-worktree-scan-generation' +import type { PersistedState } from '../../../shared/persisted-state-types' +import { getRepoIdFromWorktreeId } from '../../../shared/worktree/id' import type { StoreRuntimeState } from './store-runtime-state' import type { WriteSchedulingOperations } from './write-scheduling' @@ -129,6 +132,33 @@ export class RepoLifecycleOperations { scheduleSave(this[repoLifecycleOperationsContext].scheduling) } + /** + * Drop every persisted row owned by a repo id that is no longer registered. + * + * Runs at load because no removal path can: `removeProject` only fires while the repo is still in + * `state.repos`, and a paired client's mirror of a remote host's rows is keyed by ids that client + * never registers, so the owning host's removal never reaches it (#17776). An orphan has no owner + * that could object, so this ignores the session-ownership and local-execution-host gates the + * missing-directory sweeper needs. + */ + sweepDeregisteredRepoResidue(): string[] { + const state = this[repoLifecycleOperationsContext].runtime.state + const orphanRepoIds = collectDeregisteredRepoIds(state) + if (orphanRepoIds.size === 0) { + return [] + } + for (const repoId of orphanRepoIds) { + pruneWorktreeStateForRepo(this, repoId, null) + state.workspaceSession = removeRepoFromWorkspaceSession(state.workspaceSession, repoId) + state.workspaceSessionsByHostId = removeRepoFromHostWorkspaceSessions( + state.workspaceSessionsByHostId, + repoId + ) + } + pruneDeregisteredRepoUiResidue(state.ui, orphanRepoIds) + return [...orphanRepoIds] + } + updateRepo( id: string, updates: Partial< @@ -212,6 +242,26 @@ export function pruneMobileClientTabSelections( } } +function pruneDeregisteredRepoUiResidue( + ui: PersistedState['ui'], + orphanRepoIds: ReadonlySet +): void { + const isOrphanWorktree = (worktreeId: string): boolean => + orphanRepoIds.has(getRepoIdFromWorktreeId(worktreeId)) + if (ui.lastActiveRepoId && orphanRepoIds.has(ui.lastActiveRepoId)) { + ui.lastActiveRepoId = null + } + if (ui.lastActiveWorktreeId && isOrphanWorktree(ui.lastActiveWorktreeId)) { + ui.lastActiveWorktreeId = null + } + ui.filterRepoIds = ui.filterRepoIds?.filter((repoId) => !orphanRepoIds.has(repoId)) ?? [] + for (const worktreeId of Object.keys(ui.showDotfilesByWorktree ?? {})) { + if (isOrphanWorktree(worktreeId)) { + delete ui.showDotfilesByWorktree?.[worktreeId] + } + } +} + export function getRepoUpdateOperations( owner: RepoLifecycleOperations ): RepoUpdatePersistenceOperations { diff --git a/src/main/persistence/loading-store/store.ts b/src/main/persistence/loading-store/store.ts index 888c0092ed2..017583e8f86 100644 --- a/src/main/persistence/loading-store/store.ts +++ b/src/main/persistence/loading-store/store.ts @@ -64,6 +64,9 @@ export class Store { ) const adaptedProjectGroups = this.domains.adaptation.adaptFlatFolderScanProjectGroups() this.domains.adaptation.hydrateFolderWorkspaceDiffComments() + // Load is the only place an orphaned repo id can be swept: every removal path needs the repo to + // still be registered, so rows outlive their owner without one (#17776). + const sweptRepoIds = this.domains.repos.sweepDeregisteredRepoResidue() for (const entry of normalized.migrationUnsupportedEntries) { setMigrationUnsupportedPty(entry) } @@ -78,7 +81,12 @@ export class Store { this.state.legacyPaneKeyAliasEntries = entries scheduleSave(this.domains.scheduling) }) - if (normalized.changed || this.runtime.loadNeedsSave || adaptedProjectGroups) { + if ( + normalized.changed || + this.runtime.loadNeedsSave || + adaptedProjectGroups || + sweptRepoIds.length > 0 + ) { scheduleSave(this.domains.scheduling) } } diff --git a/src/main/persistence/tracking-repos/deregistered-repo-residue.ts b/src/main/persistence/tracking-repos/deregistered-repo-residue.ts new file mode 100644 index 00000000000..60a77a7ed10 --- /dev/null +++ b/src/main/persistence/tracking-repos/deregistered-repo-residue.ts @@ -0,0 +1,82 @@ +import type { PersistedState } from '../../../shared/persisted-state-types' +import { getWorktreeIdFromHostIdentity } from '../../../shared/worktree/host-qualified-identity' +import { splitWorktreeId } from '../../../shared/worktree/id' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { SESSION_FIELDS_PRUNED_BY_OWNER_KEY } from '../../orca-profiles/profile-project-session-field-disposition' +import { ownerKeyWorktreeId } from '../../orca-profiles/profile-project-worktree-identity' + +/** + * Repo ids that still own persisted rows but no longer appear in `state.repos`. + * + * Why nothing else finds them: every other sweeper is gated on the repo still being registered, so + * deregistering a project stranded the rows it owned permanently — including a paired client's + * mirror of a remote host's session partition, which no local repo removal can reach (#17776). + */ +export function collectDeregisteredRepoIds(state: PersistedState): Set { + const liveRepoIds = new Set(state.repos.map((repo) => repo.id)) + const orphanRepoIds = new Set() + // Only a full `::` locator seeds the set. A bare key -- a folder workspace id, a + // repo-keyed topology revision, a test-shaped locator -- cannot be told apart from a repo id, and + // guessing wrong here deletes live session state. + const addWorktreeId = (worktreeId: string | null | undefined): void => { + const repoId = worktreeId ? splitWorktreeId(worktreeId)?.repoId : undefined + if (repoId && !liveRepoIds.has(repoId)) { + orphanRepoIds.add(repoId) + } + } + const addOwnerKey = (ownerKey: string): void => { + addWorktreeId(ownerKeyWorktreeId(ownerKey)) + } + + // Deliberately not seeded from `sparsePresetsByRepo` or `retiredWorktreeNamesByRepo`: both are + // bounded, and dropping a retired-name row would let a re-added repo reissue a name onto a cwd + // that still holds a prior occupant's agent state. + for (const worktreeId of Object.keys(state.worktreeMeta)) { + addWorktreeId(worktreeId) + } + for (const alias of Object.keys(state.worktreeIdentityAliases ?? {})) { + addWorktreeId(getWorktreeIdFromHostIdentity(alias)) + } + for (const [childId, lineage] of Object.entries(state.worktreeLineageById)) { + addWorktreeId(childId) + addWorktreeId(lineage.parentWorktreeId) + } + for (const [childKey, lineage] of Object.entries(state.workspaceLineageByChildKey)) { + addOwnerKey(childKey) + addOwnerKey(lineage.parentWorkspaceKey) + } + for (const selections of Object.values(state.mobileClientTabSelectionsByDeviceId ?? {})) { + for (const worktreeId of Object.keys(selections)) { + addWorktreeId(worktreeId) + } + } + const sessions: (WorkspaceSessionState | undefined)[] = [ + state.workspaceSession, + ...Object.values(state.workspaceSessionsByHostId ?? {}) + ] + for (const session of sessions) { + if (!session) { + continue + } + for (const field of SESSION_FIELDS_PRUNED_BY_OWNER_KEY) { + for (const ownerKey of Object.keys( + (session[field] as Record | undefined) ?? {} + )) { + addOwnerKey(ownerKey) + } + } + for (const ownerKey of Object.keys(session.tabsByWorktree ?? {})) { + addOwnerKey(ownerKey) + } + for (const ownerKey of Object.keys(session.browserTabsByWorktree ?? {})) { + addOwnerKey(ownerKey) + } + for (const record of Object.values(session.sleepingAgentSessionsByPaneKey ?? {})) { + addWorktreeId(record.worktreeId) + } + for (const tombstone of Object.values(session.terminalSurfaceTombstonesByPaneKey ?? {})) { + addWorktreeId(tombstone.worktreeId) + } + } + return orphanRepoIds +} diff --git a/src/main/ssh-reattach-pane-cardinality.test.ts b/src/main/ssh-reattach-pane-cardinality.test.ts index 51cbdb26794..96362ffbfc2 100644 --- a/src/main/ssh-reattach-pane-cardinality.test.ts +++ b/src/main/ssh-reattach-pane-cardinality.test.ts @@ -2,7 +2,13 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { rmSync, mkdtempSync } from 'node:fs' import { join } from 'node:path' import { tmpdir } from 'node:os' -import { testState, createStore, makeTerminalTab, writeDataFile } from './persistence-test-harness' +import { + testState, + createStore, + makeRepo, + makeTerminalTab, + writeDataFile +} from './persistence-test-harness' import { TEST_LEAF_1, TEST_LEAF_2 } from './persistence-session-fixtures' import { getDefaultPersistedState } from '../shared/constants' @@ -196,6 +202,8 @@ describe('STA-3077: an SSH reattach binds panes without grafting them back', () it('does not clear and rebind a retired surface loaded from an older profile', async () => { const paneKey = `${TAB}:${TEST_LEAF_1}` const persisted = getDefaultPersistedState(testState.dir) + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + persisted.repos = [makeRepo({ id: 'repo1', path: '/repo1' })] persisted.workspaceSession = { ...persisted.workspaceSession, ...sessionWithPane({ tabId: TAB, leafId: TEST_LEAF_1, ptyId: 'pty-1' }), diff --git a/src/main/worktree-identity-persistence.test.ts b/src/main/worktree-identity-persistence.test.ts index c66a2d72ce8..0b6dd178e84 100644 --- a/src/main/worktree-identity-persistence.test.ts +++ b/src/main/worktree-identity-persistence.test.ts @@ -5,12 +5,26 @@ import { tmpdir } from 'node:os' import type { PersistedState } from '../shared/persisted-state-types' import { canonicalWorktreeIdentity } from '../shared/worktree/identity' import { composeWorktreeHostIdentity } from '../shared/worktree/host-qualified-identity' -import { createStore, readDataFile, testState, writeDataFile } from './persistence-test-harness' +import type { Store } from './persistence/loading-store/store' +import { + createStore, + makeRepo, + readDataFile, + testState, + writeDataFile +} from './persistence-test-harness' describe('host-qualified worktree metadata', () => { const worktreeId = 'repo-1::/workspace/feature' const ROTATED_INSTANCE_ID = '44444444-4444-4444-8444-444444444444' + // Registered on purpose: rows owned by an unregistered repo id are swept as orphans on load. + const createStoreWithRepo = (): Store => { + const store = createStore() + store.addRepo(makeRepo({ id: 'repo-1', path: '/workspace' })) + return store + } + beforeEach(() => { testState.dir = mkdtempSync(join(tmpdir(), 'orca-worktree-identity-')) }) @@ -52,7 +66,7 @@ describe('host-qualified worktree metadata', () => { }) }) it('reloads host-specific metadata without collapsing it to the legacy locator', () => { - const store = createStore() + const store = createStoreWithRepo() store.setWorktreeMetaForHost(worktreeId, 'local', { displayName: 'Local feature' }) store.setWorktreeMetaForHost(worktreeId, 'ssh:build-box', { displayName: 'Remote feature' }) store.flush() @@ -72,7 +86,7 @@ describe('host-qualified worktree metadata', () => { expect(store.getWorktreeMetaForHost(worktreeId, 'local')?.comment).toBe('after') }) it('backfills one stable instance for legacy metadata that omitted it', () => { - const seed = createStore() + const seed = createStoreWithRepo() seed.setWorktreeMeta(worktreeId, { displayName: 'Legacy feature' }) seed.flush() const legacy = readDataFile() as PersistedState @@ -97,7 +111,7 @@ describe('host-qualified worktree metadata', () => { // Fails open on purpose: an ambiguous alias used to brick reads and throw out of the worktree // listing loop, taking every workspace in the repo down with it and never self-healing. it('collapses an ambiguous locator onto its most recently active instance', () => { - const seed = createStore() + const seed = createStoreWithRepo() const first = seed.setWorktreeMetaForHost(worktreeId, 'local', { displayName: 'First' }) seed.flush() const persisted = readDataFile() as PersistedState @@ -262,7 +276,7 @@ describe('host-qualified worktree metadata', () => { it('repairs a missing canonical instance id while re-adopting an SSH target', () => { const oldHostId = 'ssh:old-target' as const const newHostId = 'ssh:new-target' as const - const seed = createStore() + const seed = createStoreWithRepo() seed.setWorktreeMetaForHost(worktreeId, oldHostId, { displayName: 'Remote feature' }) seed.flush() const persisted = readDataFile() as PersistedState @@ -318,7 +332,7 @@ describe('host-qualified worktree metadata', () => { it('deduplicates an equivalent destination during SSH target re-adoption', () => { const oldHostId = 'ssh:old-target' as const const newHostId = 'ssh:new-target' as const - const seed = createStore() + const seed = createStoreWithRepo() seed.setWorktreeMetaForHost(worktreeId, oldHostId, { displayName: 'Remote feature' }) seed.flush() const persisted = readDataFile() as PersistedState From 2f105b23d17d715bb648ae1809f8c43a3d30a0c6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:19:57 -0700 Subject: [PATCH 08/16] fix(persistence): sweep sleeping-agent-only residue and stop mis-seeding orphans Review found three holes in the load-time sweep. `sleepingAgentSessionsByPaneKey` and `terminalSurfaceTombstonesByPaneKey` are pruned by the worktreeId they name, not by their own key, but `pruneWorktreeStateForRepo` only collected owner keys from `worktreeMeta` and `lastVisitedAtByWorktreeId`. An orphan whose only residue was a sleeping agent therefore survived the sweep and re-seeded it on the next load, so the store never self-cleared and every launch scheduled another save. Collect owner keys from those records too, which fixes `removeProject` for the same shape. `ownerKeyBelongsToRepo` is restored to its original body. Reordering its two readings was not behavior-preserving as claimed: for a repo named `folder` or `worktree`, checking the workspace-key reading first flips the result. The census now uses `ownerKeyWorktreeIds`, which returns both readings, and seeds only when neither names a live repo -- seeding one reading of a key whose other reading is live would hand the removal pass a live row to delete. Seed from `activeWorktreeId`, `activeWorkspaceKey` and `activeWorktreeIdsOnShutdown`, which are pruned by bespoke rules and so were reachable by no owner-key loop, and record why `terminalTopologyRevisionByRepoId` stays excluded. Refs #17776 --- .../profile-project-worktree-identity.ts | 31 ++++++++--- ...sistence-deregistered-repo-residue.test.ts | 54 ++++++++++++++----- .../deregistered-repo-residue.ts | 30 ++++++++++- .../tracking-repos/repo-worktree-pruning.ts | 23 ++++++-- 4 files changed, 111 insertions(+), 27 deletions(-) diff --git a/src/main/orca-profiles/profile-project-worktree-identity.ts b/src/main/orca-profiles/profile-project-worktree-identity.ts index f4063cbb372..8cf58dd1bd5 100644 --- a/src/main/orca-profiles/profile-project-worktree-identity.ts +++ b/src/main/orca-profiles/profile-project-worktree-identity.ts @@ -66,18 +66,33 @@ export function rekeyOwnerKey( return null } -/** The worktree locator an owner key names, or null when the key is not worktree-scoped. */ -export function ownerKeyWorktreeId(ownerKey: string): string | null { +/** + * Every worktree locator an owner key could name. + * + * Two readings, because one key can be both: with a repo literally named `worktree`, + * `worktree::/p` is a `::` locator AND parses as a `worktree:` workspace key naming + * repo `` (empty). `ownerKeyBelongsToRepo` accepts either, so a caller that reasons about a key + * without a repo id in hand has to consider both or it will disagree with the predicate. + */ +export function ownerKeyWorktreeIds(ownerKey: string): string[] { + const rawOwnerKey = isWorktreeHostIdentity(ownerKey) + ? getWorktreeIdFromHostIdentity(ownerKey) + : ownerKey const scope = parseWorkspaceKey(ownerKey) - if (scope) { - return scope.type === 'worktree' ? scope.worktreeId : null - } - return isWorktreeHostIdentity(ownerKey) ? getWorktreeIdFromHostIdentity(ownerKey) : ownerKey + return scope?.type === 'worktree' && scope.worktreeId !== rawOwnerKey + ? [rawOwnerKey, scope.worktreeId] + : [rawOwnerKey] } export function ownerKeyBelongsToRepo(ownerKey: string, repoId: string): boolean { - const worktreeId = ownerKeyWorktreeId(ownerKey) - return worktreeId !== null && isRepoWorktreeId(repoId, worktreeId) + const rawOwnerKey = isWorktreeHostIdentity(ownerKey) + ? getWorktreeIdFromHostIdentity(ownerKey) + : ownerKey + if (isRepoWorktreeId(repoId, rawOwnerKey)) { + return true + } + const parsed = parseWorkspaceKey(ownerKey) + return parsed?.type === 'worktree' && isRepoWorktreeId(repoId, parsed.worktreeId) } export function removeRepoWorktreeRecord( diff --git a/src/main/persistence-deregistered-repo-residue.test.ts b/src/main/persistence-deregistered-repo-residue.test.ts index f1ecbde3213..62d4aab957e 100644 --- a/src/main/persistence-deregistered-repo-residue.test.ts +++ b/src/main/persistence-deregistered-repo-residue.test.ts @@ -38,6 +38,21 @@ const LIVE_WORKTREE = `${LIVE_REPO}::/workspace/live` const GONE_WORKTREE = `${GONE_REPO}::/workspace/orphan` const RUNTIME_HOST = 'runtime:env-a' +const sleepingAgentFor = (worktreeId: string, tabId = 'tab-1') => ({ + [`${tabId}:leaf-1`]: { + paneKey: `${tabId}:leaf-1`, + tabId, + worktreeId, + agent: 'codex' as const, + providerSession: { key: 'session_id' as const, id: 'sess-1' }, + prompt: 'sleeping', + state: 'waiting' as const, + capturedAt: 1, + updatedAt: 1, + origin: 'worktree-sleep' as const + } +}) + const sessionFor = (worktreeId: string, tabId = 'tab-1') => ({ ...getDefaultWorkspaceSession(), tabsByWorktree: { @@ -46,20 +61,7 @@ const sessionFor = (worktreeId: string, tabId = 'tab-1') => ({ activeTabTypeByWorktree: { [worktreeId]: 'terminal' as const }, lastVisitedAtByWorktreeId: { [worktreeId]: 123 }, // The residue `profile-project-session-field-disposition` flags as leaking on repo removal. - sleepingAgentSessionsByPaneKey: { - [`${tabId}:leaf-1`]: { - paneKey: `${tabId}:leaf-1`, - tabId, - worktreeId, - agent: 'codex' as const, - providerSession: { key: 'session_id' as const, id: 'sess-1' }, - prompt: 'sleeping', - state: 'waiting' as const, - capturedAt: 1, - updatedAt: 1, - origin: 'worktree-sleep' as const - } - } + sleepingAgentSessionsByPaneKey: sleepingAgentFor(worktreeId, tabId) }) describe('deregistered repo residue', () => { @@ -156,6 +158,30 @@ describe('deregistered repo residue', () => { }) }) + // Regression: the pane-keyed records are pruned by the worktreeId they name, not by their own key, + // so an orphan whose ONLY residue is a sleeping agent survived -- and re-seeded the sweep on every + // launch, so the store never self-cleared and every load scheduled another save. + it("drops a sleeping agent that is the orphan repo's only residue, and self-clears", async () => { + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: LIVE_REPO, path: '/workspace/live' })], + worktreeMeta: {}, + workspaceSession: { + ...getDefaultWorkspaceSession(), + sleepingAgentSessionsByPaneKey: sleepingAgentFor(GONE_WORKTREE) + } + }) + + const store = await createStore() + store.flush() + expect(store.getWorkspaceSession('local').sleepingAgentSessionsByPaneKey ?? {}).toEqual({}) + + // Self-clearing: with the residue gone nothing re-seeds the orphan id, so the next launch has + // no work. Before the fix this stayed non-empty forever and every load scheduled another save. + const reloaded = await createStore() + expect(reloaded.sweepDeregisteredRepoResidue()).toEqual([]) + }) + // Why: a sweep that dirtied every launch would rewrite the profile forever and mask real changes. it('leaves a profile with no orphans byte-identical across reloads', async () => { const seed = await createStore() diff --git a/src/main/persistence/tracking-repos/deregistered-repo-residue.ts b/src/main/persistence/tracking-repos/deregistered-repo-residue.ts index 60a77a7ed10..c52bddbb712 100644 --- a/src/main/persistence/tracking-repos/deregistered-repo-residue.ts +++ b/src/main/persistence/tracking-repos/deregistered-repo-residue.ts @@ -3,7 +3,7 @@ import { getWorktreeIdFromHostIdentity } from '../../../shared/worktree/host-qua import { splitWorktreeId } from '../../../shared/worktree/id' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { SESSION_FIELDS_PRUNED_BY_OWNER_KEY } from '../../orca-profiles/profile-project-session-field-disposition' -import { ownerKeyWorktreeId } from '../../orca-profiles/profile-project-worktree-identity' +import { ownerKeyWorktreeIds } from '../../orca-profiles/profile-project-worktree-identity' /** * Repo ids that still own persisted rows but no longer appear in `state.repos`. @@ -24,8 +24,21 @@ export function collectDeregisteredRepoIds(state: PersistedState): Set { orphanRepoIds.add(repoId) } } + /** + * Seed from an owner key, which can read as two different locators (see `ownerKeyWorktreeIds`). + * All or nothing: if either reading names a live repo the key is that repo's, and seeding the + * other reading would hand the removal pass -- which accepts either -- a live row to delete. + */ const addOwnerKey = (ownerKey: string): void => { - addWorktreeId(ownerKeyWorktreeId(ownerKey)) + const repoIds = ownerKeyWorktreeIds(ownerKey).flatMap((worktreeId) => { + const repoId = splitWorktreeId(worktreeId)?.repoId + return repoId ? [repoId] : [] + }) + if (repoIds.length > 0 && repoIds.every((repoId) => !liveRepoIds.has(repoId))) { + for (const repoId of repoIds) { + orphanRepoIds.add(repoId) + } + } } // Deliberately not seeded from `sparsePresetsByRepo` or `retiredWorktreeNamesByRepo`: both are @@ -71,6 +84,19 @@ export function collectDeregisteredRepoIds(state: PersistedState): Set { for (const ownerKey of Object.keys(session.browserTabsByWorktree ?? {})) { addOwnerKey(ownerKey) } + // Pruned by bespoke rules rather than by owner key, so the loop above never reaches them. + for (const ownerKey of [ + session.activeWorktreeId, + session.activeWorkspaceKey, + ...(session.activeWorktreeIdsOnShutdown ?? []) + ]) { + if (ownerKey) { + addOwnerKey(ownerKey) + } + } + // Not seeded from `terminalTopologyRevisionByRepoId`: its keys are bare repo ids by contract, + // and a bare key is exactly what `addWorktreeId` refuses to trust. Rows there are removed once + // any locator seeds their repo id, which every repo that ever opened a terminal has. for (const record of Object.values(session.sleepingAgentSessionsByPaneKey ?? {})) { addWorktreeId(record.worktreeId) } diff --git a/src/main/persistence/tracking-repos/repo-worktree-pruning.ts b/src/main/persistence/tracking-repos/repo-worktree-pruning.ts index f6d17b24aa2..a41f7c6319d 100644 --- a/src/main/persistence/tracking-repos/repo-worktree-pruning.ts +++ b/src/main/persistence/tracking-repos/repo-worktree-pruning.ts @@ -2,6 +2,7 @@ import type { WorkspaceKey } from '../../../shared/folder-workspace-types' import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../../shared/execution-host' import { parseWorkspaceKey } from '../../../shared/workspace-scope' import type { PersistedState } from '../../../shared/persisted-state-types' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { removeWorkspaceSessionOwners } from '../restoring-sessions/session-owner-removal' import { getExecutionHostIdFromWorktreeHostIdentity, @@ -58,10 +59,26 @@ export function pruneWorktreeStateForRepo( } } } - collectPrefixedKeys(Object.keys(state.worktreeMeta)) - collectPrefixedKeys(Object.keys(state.workspaceSession?.lastVisitedAtByWorktreeId ?? {})) - for (const session of Object.values(state.workspaceSessionsByHostId ?? {})) { + // Why the pane-keyed records contribute owner keys: they are pruned by the worktreeId they name, + // not by their own key, so a worktree with no meta and no visit row would otherwise keep its + // sleeping agents and tombstones forever -- and keep re-seeding the orphan sweep every load. + const collectScannedRecordOwners = (session: WorkspaceSessionState | undefined): void => { collectPrefixedKeys(Object.keys(session?.lastVisitedAtByWorktreeId ?? {})) + collectPrefixedKeys( + Object.values(session?.sleepingAgentSessionsByPaneKey ?? {}).map( + (record) => record.worktreeId + ) + ) + collectPrefixedKeys( + Object.values(session?.terminalSurfaceTombstonesByPaneKey ?? {}).map( + (tombstone) => tombstone.worktreeId + ) + ) + } + collectPrefixedKeys(Object.keys(state.worktreeMeta)) + collectScannedRecordOwners(state.workspaceSession) + for (const session of Object.values(state.workspaceSessionsByHostId ?? {})) { + collectScannedRecordOwners(session) } for (const key of Object.keys(state.worktreeMeta)) { From 87f3e907ddd72abd390897060d6e21d7471d9f51 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:49:04 -0700 Subject: [PATCH 09/16] test(persistence): assert the sleeping-agent cleanup reached disk The self-clearing check loaded a second store, but that constructor runs the sweep itself. If the first flush had not persisted the cleanup, the second load would have redone it in memory and the assertion would have passed without meaning anything. Read the profile back and assert the map is empty there first. Refs #17776 --- src/main/persistence-deregistered-repo-residue.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/persistence-deregistered-repo-residue.test.ts b/src/main/persistence-deregistered-repo-residue.test.ts index 62d4aab957e..03db3ed14fc 100644 --- a/src/main/persistence-deregistered-repo-residue.test.ts +++ b/src/main/persistence-deregistered-repo-residue.test.ts @@ -175,6 +175,10 @@ describe('deregistered repo residue', () => { const store = await createStore() store.flush() expect(store.getWorkspaceSession('local').sleepingAgentSessionsByPaneKey ?? {}).toEqual({}) + // On disk, not just in memory: if the flush had not persisted the cleanup, the next load would + // silently redo it and the self-clearing assertion below would pass without meaning anything. + const persisted = readDataFile() as PersistedState + expect(persisted.workspaceSession.sleepingAgentSessionsByPaneKey ?? {}).toEqual({}) // Self-clearing: with the residue gone nothing re-seeds the orphan id, so the next launch has // no work. Before the fix this stayed non-empty forever and every load scheduled another save. From 1a11f82fcc22dc613947449d285220cab0885f06 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:04:49 -0700 Subject: [PATCH 10/16] test(persistence): cover each session scalar as an orphan's only residue `activeWorktreeId`, `activeWorkspaceKey` and `activeWorktreeIdsOnShutdown` are pruned by bespoke rules rather than by owner key, so no owner-key loop reaches them and each has to be able to seed the sweep alone. The sweep already handles all three -- the census seeds from them and `removeRepoFromWorkspaceSession` clears them -- but nothing pinned it, and dropping that seeding turns all three cases red. The `activeWorkspaceKey` case uses the canonical `worktree:` form, so it also covers unwrapping the workspace key before the repo id is visible. Refs #17776 --- ...sistence-deregistered-repo-residue.test.ts | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/main/persistence-deregistered-repo-residue.test.ts b/src/main/persistence-deregistered-repo-residue.test.ts index 03db3ed14fc..3a7a3372b3c 100644 --- a/src/main/persistence-deregistered-repo-residue.test.ts +++ b/src/main/persistence-deregistered-repo-residue.test.ts @@ -8,7 +8,7 @@ import { join } from 'node:path' import { tmpdir } from 'node:os' import { getDefaultWorkspaceSession } from '../shared/constants' import { composeWorktreeHostIdentity } from '../shared/worktree/host-qualified-identity' -import { folderWorkspaceKey } from '../shared/workspace-scope' +import { folderWorkspaceKey, worktreeWorkspaceKey } from '../shared/workspace-scope' import type { PersistedState } from '../shared/persisted-state-types' import { testState, @@ -186,6 +186,40 @@ describe('deregistered repo residue', () => { expect(reloaded.sweepDeregisteredRepoResidue()).toEqual([]) }) + // The session scalars are pruned by bespoke rules, not by owner key, so no owner-key loop reaches + // them. Each has to be able to seed the sweep on its own or an orphan named only there is stuck. + it.each([ + { label: 'activeWorktreeId', session: { activeWorktreeId: GONE_WORKTREE } }, + // Canonical `worktree:` form, which needs unwrapping before the repo id is visible. + { + label: 'activeWorkspaceKey', + session: { activeWorkspaceKey: worktreeWorkspaceKey(GONE_WORKTREE) } + }, + { + label: 'activeWorktreeIdsOnShutdown', + session: { activeWorktreeIdsOnShutdown: [GONE_WORKTREE] } + } + ])("clears $label when it is the orphan repo's only residue", async ({ session }) => { + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: LIVE_REPO, path: '/workspace/live' })], + worktreeMeta: {}, + workspaceSessionsByHostId: { + [RUNTIME_HOST]: { ...getDefaultWorkspaceSession(), ...session } + } + }) + + const store = await createStore() + store.flush() + + const partition = store.getWorkspaceSession(RUNTIME_HOST) + expect(partition.activeWorktreeId ?? null).toBeNull() + expect(partition.activeWorkspaceKey ?? null).toBeNull() + expect(partition.activeWorktreeIdsOnShutdown ?? []).toEqual([]) + const reloaded = await createStore() + expect(reloaded.sweepDeregisteredRepoResidue()).toEqual([]) + }) + // Why: a sweep that dirtied every launch would rewrite the profile forever and mask real changes. it('leaves a profile with no orphans byte-identical across reloads', async () => { const seed = await createStore() From ff8b4d08ab98c8a4d8cabcadcbfe9628924b9995 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 01:11:21 -0700 Subject: [PATCH 11/16] fix(runtime): sweep missing local worktree metadata on the host that owns it `pruneMetadataMissingFromAuthoritativeLocalScan` had exactly one caller: `ipcMain.handle('worktrees:listAll')`. A headless runtime host has no renderer, so it never ran, and that host's `worktreeMeta` grew without bound even for its own local repos -- 129 of 139 rows dangling on the profile in #17776. Run it from the runtime's own detected listing instead. That is the same trigger on the same evidence: `listDetected` already prunes lineage on an authoritative scan, and a paired client refreshing a remote repo calls `worktree.detectedList`, so the host now sweeps exactly when the desktop would have. The expectation is captured before the scan, because listing can mutate metadata synchronously before its first await. WSL-routed repos are excluded for the reason the desktop listing excludes them: the listing runs in the distro and reports Linux paths while metadata can hold UNC ones, and v1 cannot prove those aliases equivalent. A runtime needing repair throws rather than resolving routing, which is likewise no basis for deleting rows. The prune's own gates still apply, so an SSH- or otherwise off-host repo is never swept from a local stat -- the execution host owns that verdict. Refs #17776 --- ...me-managed-worktree-metadata-sweep.test.ts | 117 ++++++++++++++++++ .../runtime-managed-worktree-queries.ts | 44 +++++++ src/main/runtime/runtime-store-contract.ts | 3 + 3 files changed, 164 insertions(+) create mode 100644 src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts diff --git a/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts b/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts new file mode 100644 index 00000000000..12d5d71e7e0 --- /dev/null +++ b/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts @@ -0,0 +1,117 @@ +// Why this file exists: the authoritative missing-metadata prune had exactly one caller, +// `ipcMain.handle('worktrees:listAll')`. A headless runtime host has no renderer, so it never swept +// its own repos and their `worktreeMeta` rows grew without bound (#17776). +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import type { Repo } from '../../shared/repo-types' +import { testState, createStore, makeRepo } from '../persistence-test-harness' +import type { Store } from '../persistence/loading-store/store' +import { RuntimeManagedWorktreeQueries } from './runtime-managed-worktree-queries' +import type { RuntimeStore } from './runtime-store-contract' + +vi.mock('./ssh/ssh-config-parser', () => ({ + loadUserSshConfig: vi.fn(), + sshConfigHostsToTargets: vi.fn() +})) + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) +vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: vi.fn().mockReturnValue({}) })) + +const gitWorktree = (path: string): GitWorktreeInfo => ({ + path, + branch: 'main', + head: 'abc1234', + isBare: false, + isMainWorktree: true +}) + +function queries( + store: Store, + repo: Repo, + worktrees: readonly GitWorktreeInfo[], + ok = true +): RuntimeManagedWorktreeQueries { + return new RuntimeManagedWorktreeQueries({ + getStore: () => store as unknown as RuntimeStore, + listResolved: async () => [], + resolveRepo: async () => repo, + selectRepos: () => [repo], + scanRepo: async () => ({ ok, worktrees: [...worktrees] }) + }) +} + +describe('runtime detected-worktree listing sweeps missing local metadata', () => { + let repoPath = '' + + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-runtime-sweep-')) + repoPath = join(testState.dir, 'repo') + mkdirSync(repoPath, { recursive: true }) + }) + + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + it('drops a metadata row whose directory is gone and the scan does not list', async () => { + const store = createStore() + const repo = makeRepo({ id: 'repo-1', path: repoPath }) + store.addRepo(repo) + const missingId = `${repo.id}::${join(testState.dir, 'deleted-worktree')}` + store.setWorktreeMetaForHost(missingId, 'local', { displayName: 'Gone' }) + expect(store.getWorktreeMeta(missingId)).toBeDefined() + + await queries(store, repo, [gitWorktree(repoPath)]).listDetected(repo) + + expect(store.getWorktreeMeta(missingId)).toBeUndefined() + }) + + it('keeps a row whose directory still exists', async () => { + const store = createStore() + const repo = makeRepo({ id: 'repo-1', path: repoPath }) + store.addRepo(repo) + const livePath = join(testState.dir, 'live-worktree') + mkdirSync(livePath, { recursive: true }) + const liveId = `${repo.id}::${livePath}` + store.setWorktreeMetaForHost(liveId, 'local', { displayName: 'Live' }) + + await queries(store, repo, [gitWorktree(repoPath)]).listDetected(repo) + + expect(store.getWorktreeMeta(liveId)).toBeDefined() + }) + + // A non-authoritative scan is a failed listing, which is no evidence any checkout is gone. + it('keeps every row when the scan is not authoritative', async () => { + const store = createStore() + const repo = makeRepo({ id: 'repo-1', path: repoPath }) + store.addRepo(repo) + const missingId = `${repo.id}::${join(testState.dir, 'deleted-worktree')}` + store.setWorktreeMetaForHost(missingId, 'local', { displayName: 'Gone' }) + + await queries(store, repo, [], false).listDetected(repo) + + expect(store.getWorktreeMeta(missingId)).toBeDefined() + }) + + // The execution host owns this verdict: a runtime host cannot stat an SSH checkout, so a local + // miss is not evidence of absence. See docs/reference/ssh-execution-boundary.md. + it('never sweeps a repo whose git runs off-host', async () => { + const store = createStore() + const repo = makeRepo({ id: 'repo-1', path: repoPath, connectionId: 'build-box' }) + store.addRepo(repo) + const missingId = `${repo.id}::${join(testState.dir, 'deleted-worktree')}` + store.setWorktreeMetaForHost(missingId, 'ssh:build-box', { displayName: 'Gone' }) + + await queries(store, repo, [gitWorktree(repoPath)]).listDetected(repo) + + expect(store.getWorktreeMetaForHost(missingId, 'ssh:build-box')).toBeDefined() + }) +}) diff --git a/src/main/runtime/runtime-managed-worktree-queries.ts b/src/main/runtime/runtime-managed-worktree-queries.ts index d444f024e84..b0ed2bc4a3b 100644 --- a/src/main/runtime/runtime-managed-worktree-queries.ts +++ b/src/main/runtime/runtime-managed-worktree-queries.ts @@ -20,6 +20,10 @@ import { } from '../../shared/worktree/visibility-sources' import { mergeWorktree } from '../ipc/worktree-logic' import { pruneLineageForMissingRepoWorktrees } from '../worktree-lineage-pruning' +import { pruneMetadataMissingFromAuthoritativeLocalScan } from '../ipc/worktrees/listing/authoritative-local-worktree-metadata-pruning' +import type { NativeLocalWorktreeMetadataScanExpectation } from '../persistence/tracking-repos/missing-local-worktree-metadata-pruning' +import { getLocalWorktreeScanGeneration } from '../local-worktree-scan-generation' +import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' import type { Store } from '../persistence' import type { RuntimeStore } from './runtime-store-contract' import type { RuntimeWorktreeScanResult } from './repo-worktree-resolution-scan' @@ -36,6 +40,31 @@ type Dependencies = { scanRepo(repo: Repo): Promise } +/** + * The destructive scan expectation for one repo, or undefined when this repo must not carry one. + * + * WSL-routed repos are excluded for the same reason the desktop listing excludes them: the listing + * runs in the distro and reports Linux paths while metadata can hold UNC ones, and v1 cannot prove + * those aliases equivalent. A runtime that needs repair throws rather than resolving routing, which + * is likewise no basis for deleting rows. + */ +function captureLocalMetadataPruneExpectation( + store: RuntimeStore, + repo: Repo +): NativeLocalWorktreeMetadataScanExpectation | undefined { + if (typeof store.captureNativeLocalWorktreeMetadataScanExpectation !== 'function') { + return undefined + } + try { + if (getLocalProjectWorktreeGitOptions(store as unknown as Store, repo).wslDistro) { + return undefined + } + } catch { + return undefined + } + return store.captureNativeLocalWorktreeMetadataScanExpectation(repo) +} + export class RuntimeManagedWorktreeQueries { constructor(private readonly deps: Dependencies) {} @@ -129,6 +158,10 @@ export class RuntimeManagedWorktreeQueries { worktrees: projectResolvedWorktreeLineage(detected, store.getAllWorktreeLineage?.() ?? {}) } } + // Why capture before the scan: listing can mutate metadata synchronously before its first + // await, and the prune revalidates against the rows as they stood when the scan was issued. + const metadataScanGeneration = getLocalWorktreeScanGeneration(repo.id) + const metadataPruneExpectation = captureLocalMetadataPruneExpectation(store, repo) let scan: RuntimeWorktreeScanResult try { scan = await this.deps.scanRepo(repo) @@ -136,6 +169,17 @@ export class RuntimeManagedWorktreeQueries { scan = { ok: false, worktrees: [] } } if (scan.ok) { + // Why the runtime sweeps too: the desktop listing that used to own this runs off `ipcMain`, + // so a headless host -- which has no renderer -- never pruned its own repos' rows (#17776). + if (metadataPruneExpectation) { + await pruneMetadataMissingFromAuthoritativeLocalScan({ + store: store as unknown as Store, + repo, + gitWorktrees: scan.worktrees, + scan: metadataPruneExpectation, + scanGeneration: metadataScanGeneration + }) + } pruneLineageForMissingRepoWorktrees(store as unknown as Store, repo, scan.worktrees) } const matcher = createWorktreeVisibilitySourceMatcher( diff --git a/src/main/runtime/runtime-store-contract.ts b/src/main/runtime/runtime-store-contract.ts index 692826b3c29..e160e039533 100644 --- a/src/main/runtime/runtime-store-contract.ts +++ b/src/main/runtime/runtime-store-contract.ts @@ -30,6 +30,9 @@ export type RuntimeStore = { removeProjectForHost?: Store['removeProjectForHost'] reorderRepos?: Store['reorderRepos'] getAllWorktreeMeta: Store['getAllWorktreeMeta'] + captureNativeLocalWorktreeMetadataScanExpectation?: Store['captureNativeLocalWorktreeMetadataScanExpectation'] + pruneSessionlessMissingLocalWorktreeMetadataForRepo?: Store['pruneSessionlessMissingLocalWorktreeMetadataForRepo'] + getProfileStorageDirectory?: Store['getProfileStorageDirectory'] getWorktreeMeta: Store['getWorktreeMeta'] setWorktreeMeta: Store['setWorktreeMeta'] setWorktreeMetaForHost?: Store['setWorktreeMetaForHost'] From 05a7d390588362eb7b2eff6922c62d13a3968bc7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:23:44 -0700 Subject: [PATCH 12/16] test(runtime): make the off-host sweep case a real control The row was stamped `ssh:build-box`, which `captureNativeLocalWorktreeMetadataScanExpectation` filters out before the prune runs -- so it survived whether or not any host gate existed and pinned nothing. Stamp it `local` so it is a genuine prune candidate whose directory really is missing, and make the fixture identical to the first case apart from `connectionId`. That pairing is what proves the behavior: the same fixture without a connection loses the row. Deleting any single gate would not show it, since four independent checks derive from `connectionId` on this path. Refs #17776 --- ...runtime-managed-worktree-metadata-sweep.test.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts b/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts index 12d5d71e7e0..6df19517d64 100644 --- a/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts +++ b/src/main/runtime/runtime-managed-worktree-metadata-sweep.test.ts @@ -61,6 +61,7 @@ describe('runtime detected-worktree listing sweeps missing local metadata', () = rmSync(testState.dir, { recursive: true, force: true }) }) + // Paired with the off-host case below: same fixture, no `connectionId`. it('drops a metadata row whose directory is gone and the scan does not list', async () => { const store = createStore() const repo = makeRepo({ id: 'repo-1', path: repoPath }) @@ -101,17 +102,22 @@ describe('runtime detected-worktree listing sweeps missing local metadata', () = expect(store.getWorktreeMeta(missingId)).toBeDefined() }) - // The execution host owns this verdict: a runtime host cannot stat an SSH checkout, so a local - // miss is not evidence of absence. See docs/reference/ssh-execution-boundary.md. + // The execution host owns this verdict: this host cannot stat a checkout that lives behind an SSH + // connection, so a local miss is not evidence of absence. See docs/reference/ssh-execution-boundary.md. + // + // Deliberately identical to the first case except for `connectionId`, and the row is stamped + // `local` so it is a real prune candidate. That pairing is the proof: the same fixture without a + // connection loses the row, so the connection is the only reason this one keeps it. Removing any + // single gate would not show that -- four independent checks derive from `connectionId` here. it('never sweeps a repo whose git runs off-host', async () => { const store = createStore() const repo = makeRepo({ id: 'repo-1', path: repoPath, connectionId: 'build-box' }) store.addRepo(repo) const missingId = `${repo.id}::${join(testState.dir, 'deleted-worktree')}` - store.setWorktreeMetaForHost(missingId, 'ssh:build-box', { displayName: 'Gone' }) + store.setWorktreeMetaForHost(missingId, 'local', { displayName: 'Gone' }) await queries(store, repo, [gitWorktree(repoPath)]).listDetected(repo) - expect(store.getWorktreeMetaForHost(missingId, 'ssh:build-box')).toBeDefined() + expect(store.getWorktreeMeta(missingId)).toBeDefined() }) }) From 398aeccdfea472584d976a62ea83c1d1c6f5ea2b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 01:22:04 -0700 Subject: [PATCH 13/16] fix(worktrees): retire runtime-host metadata a scan proved gone A paired client's WorktreeMeta for a runtime host is exempt from gcStaleWorktreeMeta -- that GC skips any row that is not local on both the repo and the meta's hostId -- so a scan-proven removal is the only thing that ever retires one. Both halves of that path were gated to `ssh:`, so the client kept a row for every remote worktree it had ever seen and dropped none. The renderer already computed the removals for runtime hosts and purged its own in-memory state with them; only the persisted half bailed. Widen it, and the matching main-side handler, to runtime hosts. `OffHostExecutionHostId` names the set precisely: the hosts the local-only GC skips. Also require `source === 'git'` before retiring anything. `session-fallback` reports `authoritative: true` but is the truncated, visibility-filtered `worktree.list` reply from a host too old for `worktree.detectedList`; its omissions are no evidence a checkout is gone. That guard did not matter while this only ran the in-memory purge, and does now that it deletes rows. A repo that reaches its checkouts over a connection is still never condemned under a runtime host id -- the host that executes owns that verdict. Refs #17776 --- ...orktrees-ssh-repo-owner-resolution.test.ts | 75 ++++++++++++- .../listing/register-host-catalog-handlers.ts | 13 ++- ...s-runtime-host-metadata-retirement.test.ts | 104 ++++++++++++++++++ .../authoritative-worktree-removal-memory.ts | 10 +- .../listing/fetched-worktree-merge.ts | 9 +- .../detected-worktree-provider-contract.ts | 8 +- 6 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 src/renderer/src/store/slices/worktrees-runtime-host-metadata-retirement.test.ts diff --git a/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts b/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts index 6a398e9b02d..5a4d775c2d0 100644 --- a/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts +++ b/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts @@ -1,7 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { GitWorktreeInfo, Worktree } from '../../shared/worktree/types' import type { ProviderRequestId } from '../../shared/detected-worktree-provider-contract' -import { LOCAL_EXECUTION_HOST_ID, toSshExecutionHostId } from '../../shared/execution-host' +import { + LOCAL_EXECUTION_HOST_ID, + toRuntimeExecutionHostId, + toSshExecutionHostId +} from '../../shared/execution-host' import { getSshProviderAuthority } from '../ssh/ssh-provider-authority' import { listWorktreesMock, @@ -443,7 +447,74 @@ describe('registerWorktreeHandlers', () => { expect(store.removeWorktreeMeta).not.toHaveBeenCalled() }) - it('refuses to retire metadata for non-SSH hosts and unowned repos', async () => { + // Runtime-host rows are exempt from gcStaleWorktreeMeta exactly as SSH ones are, so a paired + // client needs this path to ever drop them (#17776). + it('retires runtime-host metadata an authoritative scan proved gone', async () => { + const runtimeHostId = toRuntimeExecutionHostId('env-1') + const runtimeRepo = { + id: 'repo-1', + path: '/home/orca/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + executionHostId: runtimeHostId + } + const metaById: Record> = { + 'repo-1::/home/orca/deleted': makeWorktreeMeta({ hostId: runtimeHostId }), + 'repo-1::/home/orca/other-host': makeWorktreeMeta({ + hostId: toSshExecutionHostId('target-a') + }) + } + store.getRepos.mockReturnValue([runtimeRepo]) + store.getProjectHostSetups.mockReturnValue([]) + store.getAllWorktreeMeta.mockReturnValue(metaById) + store.removeWorktreeMeta.mockImplementation((worktreeId: string) => { + delete metaById[worktreeId] + }) + + const forgotten = await handlers['worktrees:forgetRemovedForExecutionHost'](null, { + repoId: runtimeRepo.id, + executionHostId: runtimeHostId, + worktreeIds: ['repo-1::/home/orca/deleted', 'repo-1::/home/orca/other-host'] + }) + + // The row stamped to another host needs that host's own scan, not this one's. + expect(forgotten).toEqual({ forgottenWorktreeIds: ['repo-1::/home/orca/deleted'] }) + expect(store.removeWorktreeMeta).toHaveBeenCalledExactlyOnceWith( + 'repo-1::/home/orca/deleted', + runtimeHostId + ) + }) + + // A repo that reaches its checkouts over SSH is not the runtime host's to condemn. + it('refuses to retire a connection-backed repo under a runtime host id', async () => { + const runtimeHostId = toRuntimeExecutionHostId('env-1') + store.getRepos.mockReturnValue([ + { + id: 'repo-1', + path: '/home/orca/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + executionHostId: runtimeHostId, + connectionId: 'target-a' + } + ]) + store.getAllWorktreeMeta.mockReturnValue({ + 'repo-1::/home/orca/deleted': makeWorktreeMeta({ hostId: runtimeHostId }) + }) + + expect( + await handlers['worktrees:forgetRemovedForExecutionHost'](null, { + repoId: 'repo-1', + executionHostId: runtimeHostId, + worktreeIds: ['repo-1::/home/orca/deleted'] + }) + ).toEqual({ forgottenWorktreeIds: [] }) + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + + it('refuses to retire metadata for non-executing hosts and unowned repos', async () => { const sshRepo = { id: 'repo-1', path: '/remote/repo-a', diff --git a/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts index 0d47f3638c2..56c2f282f37 100644 --- a/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts @@ -103,11 +103,20 @@ export function registerHostCatalogHandlers(context: WorktreeIpcContext): void { const requestedExecutionHostId = args?.executionHostId ?? 'ssh:' const worktreeIds = Array.isArray(args?.worktreeIds) ? args.worktreeIds : [] const parsedHost = parseExecutionHostId(requestedExecutionHostId) - if (parsedHost?.kind !== 'ssh' || worktreeIds.length === 0) { + // Runtime hosts belong here for the same reason SSH ones do: their rows are exempt from + // gcStaleWorktreeMeta, so a scan-proven removal is the only thing that ever retires them. + if ( + (parsedHost?.kind !== 'ssh' && parsedHost?.kind !== 'runtime') || + worktreeIds.length === 0 + ) { return nothingForgotten } const repo = findExactRepoOwner(store, args?.repoId ?? '', requestedExecutionHostId) - if (!repo || repo.connectionId !== parsedHost.targetId) { + // The connection must be the one the host id names, so a caller cannot retire a row belonging + // to a repo that reaches its checkouts some other way. + const connectionMatchesHost = + parsedHost.kind === 'ssh' ? repo?.connectionId === parsedHost.targetId : !repo?.connectionId + if (!repo || !connectionMatchesHost) { return nothingForgotten } // Why: a folder workspace's meta IS the workspace record, not a checkout row — gcStaleWorktreeMeta skips diff --git a/src/renderer/src/store/slices/worktrees-runtime-host-metadata-retirement.test.ts b/src/renderer/src/store/slices/worktrees-runtime-host-metadata-retirement.test.ts new file mode 100644 index 00000000000..eeaf49dd5e4 --- /dev/null +++ b/src/renderer/src/store/slices/worktrees-runtime-host-metadata-retirement.test.ts @@ -0,0 +1,104 @@ +// Why this file exists: a paired client's WorktreeMeta for a runtime host is exempt from +// gcStaleWorktreeMeta (it skips any row that is not local on both the repo and the meta's hostId), +// and `forgetPersistedWorktreeMetaForRemovals` used to bail for every non-SSH host. So the client +// kept a row per remote worktree it had ever seen and dropped none (#17776). +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { AppState } from '../types' +import { makeWorktree } from './worktrees-slice-test-fixtures' +import { makeDetectedResult } from './worktrees-detected-listing-fixtures' +import { + createTestStore, + forgetRemovedForExecutionHostMock, + resetRemoteRuntimeMocks, + resetWorktreeSliceModuleMemory, + runtimeEnvironmentCall +} from './worktrees-slice-test-harness' + +const REPO_ID = 'repo-runtime' +const HOST_ID = 'runtime:env-1' + +const worktree = (path: string) => + makeWorktree({ id: `${REPO_ID}::${path}`, repoId: REPO_ID, path, hostId: HOST_ID }) + +const live = worktree('/home/orca/live') +const deletedOnHost = worktree('/home/orca/deleted') + +function seedClientWithBothRows(): ReturnType { + const store = createTestStore() + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as never, + repos: [ + { + id: REPO_ID, + path: '/home/orca/repo', + displayName: 'Runtime Repo', + badgeColor: '#000', + addedAt: 0, + executionHostId: HOST_ID + } + ], + worktreesByRepo: { [REPO_ID]: [live, deletedOnHost] } + } as Partial) + return store +} + +beforeEach(resetWorktreeSliceModuleMemory) + +describe('runtime-host persisted metadata retirement', () => { + beforeEach(() => { + vi.clearAllMocks() + resetRemoteRuntimeMocks() + }) + + it('retires metadata for rows an authoritative runtime-host scan proved gone', async () => { + const store = seedClientWithBothRows() + runtimeEnvironmentCall.mockResolvedValue({ + id: 'rpc-detected', + ok: true, + result: makeDetectedResult(REPO_ID, [live]), + _meta: { runtimeId: 'runtime-remote' } + }) + + await store.getState().fetchWorktrees(REPO_ID, { executionHostId: HOST_ID }) + + expect(forgetRemovedForExecutionHostMock).toHaveBeenCalledExactlyOnceWith({ + repoId: REPO_ID, + executionHostId: HOST_ID, + worktreeIds: [deletedOnHost.id] + }) + }) + + // A non-authoritative reply is a failed listing, not a report that a checkout is gone. + it('retires nothing when the runtime host could not scan', async () => { + const store = seedClientWithBothRows() + runtimeEnvironmentCall.mockResolvedValue({ + id: 'rpc-detected', + ok: true, + result: makeDetectedResult(REPO_ID, [live], { + authoritative: false, + source: 'metadata-fallback' + }), + _meta: { runtimeId: 'runtime-remote' } + }) + + await store.getState().fetchWorktrees(REPO_ID, { executionHostId: HOST_ID }) + + expect(forgetRemovedForExecutionHostMock).not.toHaveBeenCalled() + }) + + // `session-fallback` claims authoritative but is the truncated, visibility-filtered `worktree.list` + // reply from a host too old for `worktree.detectedList`. Its omissions prove nothing. + it('retires nothing from a legacy session-fallback listing', async () => { + const store = seedClientWithBothRows() + runtimeEnvironmentCall.mockResolvedValue({ + id: 'rpc-detected', + ok: true, + result: makeDetectedResult(REPO_ID, [live], { source: 'session-fallback' }), + _meta: { runtimeId: 'runtime-remote' } + }) + + await store.getState().fetchWorktrees(REPO_ID, { executionHostId: HOST_ID }) + + expect(forgetRemovedForExecutionHostMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/store/slices/worktrees/listing/authoritative-worktree-removal-memory.ts b/src/renderer/src/store/slices/worktrees/listing/authoritative-worktree-removal-memory.ts index 9793adb0e16..e2fae6b9f77 100644 --- a/src/renderer/src/store/slices/worktrees/listing/authoritative-worktree-removal-memory.ts +++ b/src/renderer/src/store/slices/worktrees/listing/authoritative-worktree-removal-memory.ts @@ -50,16 +50,18 @@ export function resetAuthoritativelyRemovedWorktreeMemoryForTests(): void { authoritativelyRemovedWorktreeIdsByHost.clear() } -// Why: SSH WorktreeMeta is exempt from gcStaleWorktreeMeta (persistence.ts:407,415) and outlives the remote -// worktree, so a scan-proven removal must retire the metadata itself — otherwise the next launch's fallback -// re-lists the deleted row before the host connects, and the in-memory suppression above is already gone. +// Why: off-host WorktreeMeta is exempt from gcStaleWorktreeMeta -- it skips any row whose repo or hostId is +// not local -- and outlives the remote worktree, so a scan-proven removal must retire the metadata itself. +// Otherwise the next launch's fallback re-lists the deleted row before the host connects, and the in-memory +// suppression above is already gone. Runtime hosts were excluded until #17776, which is why a paired client +// accumulated a row per remote worktree it had ever seen and never dropped one. export function forgetPersistedWorktreeMetaForRemovals( repoId: string, hostId: ExecutionHostId, worktreeIds: readonly string[] ): void { const parsedHost = parseExecutionHostId(hostId) - if (worktreeIds.length === 0 || parsedHost?.kind !== 'ssh') { + if (worktreeIds.length === 0 || (parsedHost?.kind !== 'ssh' && parsedHost?.kind !== 'runtime')) { return } const forget = window.api.worktrees.forgetRemovedForExecutionHost diff --git a/src/renderer/src/store/slices/worktrees/listing/fetched-worktree-merge.ts b/src/renderer/src/store/slices/worktrees/listing/fetched-worktree-merge.ts index 13e6b1572c1..fde831ee2e5 100644 --- a/src/renderer/src/store/slices/worktrees/listing/fetched-worktree-merge.ts +++ b/src/renderer/src/store/slices/worktrees/listing/fetched-worktree-merge.ts @@ -254,7 +254,14 @@ export function mergeFetchedWorktrees( // Why: applied outside the updater so a repeated updater call cannot double-apply the removal memory. forgetAuthoritativelyRemovedWorktrees(args.hostId, authoritativelySeenIds) rememberAuthoritativelyRemovedWorktrees(args.hostId, authoritativelyRemovedIds) - forgetPersistedWorktreeMetaForRemovals(args.repoId, args.hostId, authoritativelyRemovedIds) + // Only a real scan retires persisted metadata. `session-fallback` also reports authoritative, + // but it is the truncated, visibility-filtered `worktree.list` reply from a host too old for + // `worktree.detectedList` -- its omissions are not evidence a checkout is gone. + forgetPersistedWorktreeMetaForRemovals( + args.repoId, + args.hostId, + args.refresh.result.source === 'git' ? authoritativelyRemovedIds : [] + ) } return admitted } diff --git a/src/shared/detected-worktree-provider-contract.ts b/src/shared/detected-worktree-provider-contract.ts index 9f146e35778..99b0f8fdc74 100644 --- a/src/shared/detected-worktree-provider-contract.ts +++ b/src/shared/detected-worktree-provider-contract.ts @@ -41,9 +41,15 @@ export type HostQualifiedKnownWorktreeResult = executionHostId: SshExecutionHostId } +/** + * Hosts whose persisted metadata a scan can retire: exactly those `gcStaleWorktreeMeta` skips, + * because it only ever condemns rows that are local on both the repo and the meta's `hostId`. + */ +export type OffHostExecutionHostId = Extract + export type ForgetRemovedWorktreesForExecutionHostArgs = { repoId: string - executionHostId: SshExecutionHostId + executionHostId: OffHostExecutionHostId /** Ids an authoritative scan of this host proved gone — the only evidence that retires persisted metadata. */ worktreeIds: readonly string[] } From f2fa4a7754e03e975b0da31efa914cf9a552e154 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:06:24 -0700 Subject: [PATCH 14/16] fix(worktrees): drop an unreachable runtime arm from the retirement gate `findExactRepoOwner` already refuses a repo carrying both a runtime `executionHostId` and a `connectionId` -- `resolveRepoOwnershipEvidence` calls that pair contradictory, and one non-owned candidate voids the whole lookup. There is also no way for a `connectionId` to yield a `runtime:` host id, since `toSshExecutionHostId` always emits `ssh:`. The runtime arm of `connectionMatchesHost` could therefore never decide anything, and the test meant to pin it was passing through the contradiction gate instead. Keep the SSH arm, which does gate, and record where the runtime refusal actually comes from. Unreachable code on a destructive path reads as a guarantee it is not making. Refs #17776 --- .../ipc/worktrees-ssh-repo-owner-resolution.test.ts | 4 +++- .../listing/register-host-catalog-handlers.ts | 11 ++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts b/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts index 5a4d775c2d0..b3231da86f6 100644 --- a/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts +++ b/src/main/ipc/worktrees-ssh-repo-owner-resolution.test.ts @@ -486,7 +486,9 @@ describe('registerWorktreeHandlers', () => { ) }) - // A repo that reaches its checkouts over SSH is not the runtime host's to condemn. + // A repo that reaches its checkouts over SSH is not the runtime host's to condemn. The refusal + // comes from `findExactRepoOwner`: a runtime `executionHostId` beside a `connectionId` is + // contradictory ownership evidence, so no owner resolves at all. it('refuses to retire a connection-backed repo under a runtime host id', async () => { const runtimeHostId = toRuntimeExecutionHostId('env-1') store.getRepos.mockReturnValue([ diff --git a/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts index 56c2f282f37..4467506e790 100644 --- a/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-host-catalog-handlers.ts @@ -111,12 +111,13 @@ export function registerHostCatalogHandlers(context: WorktreeIpcContext): void { ) { return nothingForgotten } + // No runtime arm in the check below: `findExactRepoOwner` already refuses a repo carrying both + // a runtime `executionHostId` and a `connectionId`, because `resolveRepoOwnershipEvidence` + // calls that pair contradictory and one non-owned candidate voids the whole lookup. A second + // check would be unreachable, and unreachable code on a destructive path reads as a guarantee + // it is not making. const repo = findExactRepoOwner(store, args?.repoId ?? '', requestedExecutionHostId) - // The connection must be the one the host id names, so a caller cannot retire a row belonging - // to a repo that reaches its checkouts some other way. - const connectionMatchesHost = - parsedHost.kind === 'ssh' ? repo?.connectionId === parsedHost.targetId : !repo?.connectionId - if (!repo || !connectionMatchesHost) { + if (!repo || (parsedHost.kind === 'ssh' && repo.connectionId !== parsedHost.targetId)) { return nothingForgotten } // Why: a folder workspace's meta IS the workspace record, not a checkout row — gcStaleWorktreeMeta skips From d48ab9614465f175fcaa6d39beab2d6768b89a7b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:04:45 -0700 Subject: [PATCH 15/16] test: stop two suites failing for reasons unrelated to their subject The zsh wrapper test relocated into a fixed-name directory in shared temp, so a single killed run left it behind and every later run on that machine failed with ENOTEMPTY, permanently. Makes the name unique while keeping the non-ASCII component the test exists for. The palette budget asserted a helper named percentile95 that returns sorted[floor(n * 0.95)] -- the maximum of the batch. Asserting worst-case wall-clock under a parallel runner measures scheduler preemption: the asserted quantity ranged 123-343ms across 20 saturated windows and blew the 220ms budget in 6 of them, while the fastest sample of those same batches held at 19-32ms. Asserts the fastest sample instead and adds a deterministic fan-out ceiling, so the guard counts work rather than time. Budgets are unchanged. --- ...user-config-equivalence.live-shell.test.ts | 7 +- .../lib/palette-match/palette-match-budget.ts | 25 +++-- .../palette-match-performance.test.ts | 95 ++++++++++++------- 3 files changed, 86 insertions(+), 41 deletions(-) diff --git a/src/main/zsh-startup-hook-user-config-equivalence.live-shell.test.ts b/src/main/zsh-startup-hook-user-config-equivalence.live-shell.test.ts index e5e45d2c22a..cbb7d2da7c5 100644 --- a/src/main/zsh-startup-hook-user-config-equivalence.live-shell.test.ts +++ b/src/main/zsh-startup-hook-user-config-equivalence.live-shell.test.ts @@ -16,7 +16,7 @@ */ import { existsSync, mkdirSync, mkdtempSync, renameSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' +import { basename, dirname, join } from 'node:path' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { getShellLaunchConfig } from './providers/local-pty-shell-ready' import { selectShellStartupFeatures } from './shell-startup-features' @@ -382,9 +382,12 @@ describe.skipIf(process.platform === 'win32')('the fixes the old wrapper was bui // value this wrapper cannot use degrades to $HOME, where zsh itself looks. const home = makeZshHome({ '.zshrc': 'export ORCA_TEST_FROM_ZSHRC=1\n' }) try { + // Unique per run: a fixed name here shares one path with every other run in + // the system temp dir, so a killed run leaves a stale directory behind and + // every later rename onto it fails with ENOTEMPTY. const { values } = await runFromRelocatedRoot( home, - join(dirname(userDataPath), '홍길동-wsl-view') + join(dirname(userDataPath), `홍길동-${basename(userDataPath)}`) ) expect(values.ORCA_TEST_FROM_ZSHRC).toBe('1') diff --git a/src/renderer/src/lib/palette-match/palette-match-budget.ts b/src/renderer/src/lib/palette-match/palette-match-budget.ts index c51144fb39d..864473a65e6 100644 --- a/src/renderer/src/lib/palette-match/palette-match-budget.ts +++ b/src/renderer/src/lib/palette-match/palette-match-budget.ts @@ -1,8 +1,14 @@ /** * Checked-in performance budget for the Cmd+J matcher, measured against the * synthetic corpus in `palette-match-performance.test.ts`. These are ceilings for - * catching order-of-magnitude regressions, not targets — the measured numbers on - * a developer machine sit roughly an order of magnitude under each one. + * catching order-of-magnitude regressions, not targets. + * + * The wall-clock ceilings are asserted against the *fastest* sample of a batch, + * never the slowest: a vitest worker sharing cores with the rest of the suite + * gets preempted mid-measurement, so the slowest sample measures the machine + * while the fastest still approximates the matcher. Fan-out regressions are + * caught by `fieldMatchesPerCandidate` instead, which counts work rather than + * time and so does not depend on machine speed at all. * * Raising any value requires a fresh measurement recorded in the PR. */ @@ -11,10 +17,17 @@ export const PALETTE_MATCH_BUDGET = { candidateCount: 800, /** Unique tokens in the worst supported query. */ tokenCount: 16, - /** p95 milliseconds to normalize every document once (cold open). */ - coldBuildP95Ms: 900, - /** p95 milliseconds to match the whole corpus against one prepared query. */ - warmMatchP95Ms: 220, + /** + * Ceiling on `matchPaletteField` calls per candidate for the worst query. + * Deterministic — it counts work, not time — so it catches a fan-out + * regression (re-matching every field per evidence unit, say) on any machine. + * Measured 45: 15 fields across the 3 tokens scanned before the first miss. + */ + fieldMatchesPerCandidate: 60, + /** Milliseconds to normalize every document once (cold open), fastest sample. */ + coldBuildMs: 900, + /** Milliseconds to match the whole corpus against one prepared query, fastest sample. */ + warmMatchMs: 220, /** * Megabytes of indexed text and offset tables the normalized documents retain. * Measured deterministically rather than from `heapUsed`, which is polluted by diff --git a/src/renderer/src/lib/palette-match/palette-match-performance.test.ts b/src/renderer/src/lib/palette-match/palette-match-performance.test.ts index aa1e6e12047..13fbced916f 100644 --- a/src/renderer/src/lib/palette-match/palette-match-performance.test.ts +++ b/src/renderer/src/lib/palette-match/palette-match-performance.test.ts @@ -1,8 +1,11 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { PALETTE_MATCH_BUDGET } from './palette-match-budget' import { matchPaletteDocument } from './match-document' +import * as matchFieldModule from './match-field' import { preparePaletteQuery } from './palette-query' import { buildWorktreePaletteDocuments } from '../worktree-palette-document' +import type { PaletteDocument } from './palette-document' +import type { PaletteQueryToken } from './palette-query' import type { Repo } from '../../../../shared/repo-types' import type { Worktree } from '../../../../shared/worktree/types' @@ -89,49 +92,75 @@ const WORST_QUERY = Array.from({ length: tokenCount }, (_, index) => index === 0 ? 'scan' : index === 1 ? 'daily' : `token${index}` ).join(' ') -function percentile95(samples: number[]): number { - const sorted = [...samples].sort((a, b) => a - b) - return sorted[Math.min(sorted.length - 1, Math.floor(sorted.length * 0.95))] +function prepareWorstQuery(): { tokens: readonly PaletteQueryToken[]; normalized: string } { + const prepared = preparePaletteQuery(WORST_QUERY) + if (prepared.state !== 'ready') { + throw new Error(`Expected a ready query, got ${prepared.state}`) + } + return { tokens: prepared.tokens, normalized: prepared.normalized } +} + +const preparedQuery = prepareWorstQuery() + +function matchEveryDocument(documents: ReadonlyMap): void { + for (const document of documents.values()) { + matchPaletteDocument({ + document, + tokens: preparedQuery.tokens, + normalizedQuery: preparedQuery.normalized + }) + } +} + +/** + * Why the fastest sample and not p95: this runs in a vitest worker competing for + * cores with the rest of the suite, so a slow sample records a preemption rather + * than the matcher. The fastest sample is the least contaminated estimate of + * intrinsic cost — measured stable within 1.6x on a fully saturated machine, + * while the slowest of the same batch swung by 17x. + */ +function fastestSample(samples: readonly number[]): number { + return Math.min(...samples) +} + +function timeRepeatedly(work: () => void, rounds: number): number[] { + const samples: number[] = [] + for (let round = 0; round < rounds; round += 1) { + const start = performance.now() + work() + samples.push(performance.now() - start) + } + return samples } describe('palette matcher performance budget', () => { it('normalizes a cold corpus within budget', () => { - const samples: number[] = [] - for (let run = 0; run < 5; run += 1) { - const start = performance.now() - buildWorktreePaletteDocuments(worktrees, sources) - samples.push(performance.now() - start) - } - expect(percentile95(samples)).toBeLessThan(PALETTE_MATCH_BUDGET.coldBuildP95Ms) + const samples = timeRepeatedly(() => buildWorktreePaletteDocuments(worktrees, sources), 5) + expect(fastestSample(samples)).toBeLessThan(PALETTE_MATCH_BUDGET.coldBuildMs) }) it('matches a 16-token query against warm documents within budget', () => { const documents = buildWorktreePaletteDocuments(worktrees, sources) - const prepared = preparePaletteQuery(WORST_QUERY) - expect(prepared.state).toBe('ready') - if (prepared.state !== 'ready') { - return - } - const matchAllDocuments = (): void => { - for (const document of documents.values()) { - matchPaletteDocument({ - document, - tokens: prepared.tokens, - normalizedQuery: prepared.normalized - }) - } - } + // Warm the matcher before timing so JIT compilation is not part of the samples. + matchEveryDocument(documents) - // Warm the matcher before timing so JIT compilation is not part of p95. - matchAllDocuments() - const samples: number[] = [] - for (let run = 0; run < 10; run += 1) { - const start = performance.now() - matchAllDocuments() - samples.push(performance.now() - start) + const samples = timeRepeatedly(() => matchEveryDocument(documents), 10) + expect(fastestSample(samples)).toBeLessThan(PALETTE_MATCH_BUDGET.warmMatchMs) + }) + + it('bounds field-match fan-out per candidate', () => { + const documents = buildWorktreePaletteDocuments(worktrees, sources) + const fieldMatch = vi.spyOn(matchFieldModule, 'matchPaletteField') + try { + matchEveryDocument(documents) + const perCandidate = fieldMatch.mock.calls.length / documents.size + // Guards the ceiling against going vacuous if the spy ever stops intercepting. + expect(perCandidate).toBeGreaterThan(0) + expect(perCandidate).toBeLessThan(PALETTE_MATCH_BUDGET.fieldMatchesPerCandidate) + } finally { + fieldMatch.mockRestore() } - expect(percentile95(samples)).toBeLessThan(PALETTE_MATCH_BUDGET.warmMatchP95Ms) }) it('keeps the retained document payload within budget', () => { From 4efc86a33c55948f4e3b2389adb7c99d9674c693 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:35:38 -0700 Subject: [PATCH 16/16] feat(app): open Markdown files from the OS in the floating workspace (#17906) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(app): open Markdown files from the OS in the floating workspace Registers Orca as a Markdown handler on macOS, Windows and Linux, and opens an OS-handed .md/.markdown/.mdx file as a floating-workspace editor tab — the one editor surface that needs no project. Works cold-start and when Orca is already running. Main buffers the paths and both pushes to a live renderer and answers a pull on renderer mount, mirroring SkillShareDeepLinkState. The buffer is only released once delivery is possible: the renderer's pull is what proves its ui:openMarkdownFiles listener is attached, because a push into a window whose renderer has not subscribed is dropped by Electron with no error. Both the push and the pull restore an undelivered batch, and a renderer reload clears the latch so the fresh renderer re-proves itself. Paths are stat'd and proven to be files before authorizeExternalPath sees them. Windows association is registered by hand in the NSIS include rather than through electron-builder's `fileAssociations`: app-builder-lib emits APP_ASSOCIATE, whose first line overwrites Software\Classes\.md's default value with no backup — silently taking .md from whichever editor owns it, for every existing user on their next update — and APP_UNASSOCIATE never restores it. The hand-rolled registration is additive (ProgID + OpenWithProgids + SupportedTypes) and leaves the user's default alone; verified end to end on a real Windows 11 host. Co-authored-by: Wooseong Kim Co-authored-by: Jaydev Closes #10138 * fix(os-open): register the new listener in the IPC inventory, and guard a non-array payload CI caught two things the local run did not. useIpcEvents-lifecycle.test.ts is an inventory of every App-lifetime IPC listener and the exact order they register in; ui.onOpenMarkdownFiles now appears there, positioned after the workspace-shortcut bridge's last listener, which is where it actually registers. Chasing that failure surfaced a real gap: the pending-open payload crosses the preload boundary, so a stale or mismatched preload can resolve with something that is not an array, and reading .length off it threw inside the promise chain instead of failing at the boundary. Array.isArray now gates it, with a regression test. --- config/electron-builder.config.cjs | 31 +- config/nsis/daemon-host-uninstall.nsh | 23 -- config/nsis/orca-installer-hooks.nsh | 79 +++++ ...ron-builder-markdown-associations.test.mjs | 116 +++++++ src/main/daemon/daemon-host-relocation.ts | 2 +- src/main/index.ts | 50 +++ .../startup/main-process-ipc-bootstrap.ts | 15 + src/main/startup/main-process-state.ts | 7 + src/main/startup/main-window-controller.ts | 3 + .../os-opened-markdown-delivery.test.ts | 68 ++++ .../startup/os-opened-markdown-files.test.ts | 306 ++++++++++++++++++ src/main/startup/os-opened-markdown-files.ts | 149 +++++++++ .../startup/os-opened-markdown-wiring.test.ts | 57 ++++ .../api/ui-bridge-state-and-menu-commands.ts | 9 + src/preload/api/ui-command-event-api.ts | 5 + .../use-floating-terminal-create-actions.ts | 20 +- .../ipc-events/app-lifetime-ipc-bridge.ts | 2 + .../os-markdown-file-open-bridge.test.ts | 300 +++++++++++++++++ .../os-markdown-file-open-bridge.ts | 72 +++++ .../src/hooks/useIpcEvents-lifecycle.test.ts | 2 + src/renderer/src/i18n/locales/en.json | 11 + ...pen-markdown-in-floating-workspace.test.ts | 81 +++++ .../open-markdown-in-floating-workspace.ts | 32 ++ .../src/web/preload-api/web-ui-api.ts | 3 + 24 files changed, 1399 insertions(+), 44 deletions(-) delete mode 100644 config/nsis/daemon-host-uninstall.nsh create mode 100644 config/nsis/orca-installer-hooks.nsh create mode 100644 config/scripts/electron-builder-markdown-associations.test.mjs create mode 100644 src/main/startup/os-opened-markdown-delivery.test.ts create mode 100644 src/main/startup/os-opened-markdown-files.test.ts create mode 100644 src/main/startup/os-opened-markdown-files.ts create mode 100644 src/main/startup/os-opened-markdown-wiring.test.ts create mode 100644 src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts create mode 100644 src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts create mode 100644 src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts create mode 100644 src/renderer/src/lib/open-markdown-in-floating-workspace.ts diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 1a31af9dfaf..13633ed8e01 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -105,6 +105,11 @@ const winSpeechNativeResource = { to: 'node_modules/sherpa-onnx-win-x64' } +// Why mirrored, not imported: this config is CJS loaded by electron-builder outside the TS build. +// Keep in sync with isMarkdownDocumentName() in src/main/ipc/markdown-documents.ts and with +// config/nsis/orca-installer-hooks.nsh, which registers the same set on Windows. +const MARKDOWN_FILE_EXTENSIONS = ['md', 'markdown', 'mdx'] + /** @type {import('electron-builder').Configuration} */ module.exports = { appId, @@ -376,12 +381,24 @@ module.exports = { shortcutName: '${productName}', uninstallDisplayName: '${productName}', createDesktopShortcut: 'always', - // Why: on a real uninstall, stop and remove the relocated terminal daemon - // (which lives outside the install dir under LOCALAPPDATA by design). Guarded - // by ${isUpdated} inside so it never runs during an update's uninstallOldVersion. - include: resolve(__dirname, 'nsis', 'daemon-host-uninstall.nsh') + // Why: electron-builder allows one include, so both Windows installer hooks live in it - + // the relocated-daemon uninstall sweep (guarded by ${isUpdated} so it never runs during an + // update's uninstallOldVersion) and the additive markdown "Open with" registration. + // Windows markdown association is deliberately NOT done via `fileAssociations`; see the + // header comment in that file for why that would steal the user's default .md handler. + include: resolve(__dirname, 'nsis', 'orca-installer-hooks.nsh') }, mac: { + // Why rank Alternate: Orca joins Finder's "Open With" list for Markdown without claiming + // LSHandlerRank ownership, so whichever editor the user already prefers stays the default. + // Why one entry per extension: app-builder-lib globs `*.${ext}`, which an array would break. + fileAssociations: MARKDOWN_FILE_EXTENSIONS.map((ext) => ({ + ext, + name: 'Markdown Document', + description: 'Markdown Document', + role: 'Editor', + rank: 'Alternate' + })), icon: 'resources/build/icon.icns', entitlements: 'resources/build/entitlements.mac.plist', entitlementsInherit: 'resources/build/entitlements.mac.plist', @@ -468,6 +485,12 @@ module.exports = { artifactName: 'orca-macos-${arch}.${ext}' }, linux: { + // Why mimeTypes and not fileAssociations: shared-mime-info already maps *.md/*.markdown to + // text/markdown, so reusing that type puts Orca in the Open With list without shipping a glob + // override. A desktop entry's MimeType only adds a handler - mimeapps.list still owns the + // default. .mdx is deliberately absent: Ubuntu 24.04's mime database maps it to + // application/x-genesis-32x-rom, so claiming it here would need a glob override. + mimeTypes: ['text/markdown'], // Why: Ubuntu desktop ships GNOME Orca as the `orca` package and /usr/bin/orca. // The Linux installer should not claim those system package/file names. executableName: 'orca-ide', diff --git a/config/nsis/daemon-host-uninstall.nsh b/config/nsis/daemon-host-uninstall.nsh deleted file mode 100644 index dc3a497ce67..00000000000 --- a/config/nsis/daemon-host-uninstall.nsh +++ /dev/null @@ -1,23 +0,0 @@ -; Clean up the relocated terminal daemon on a REAL uninstall. -; -; Why: the daemon host is deliberately copied to a distinct image name -; (orca-terminal-daemon.exe) under %LOCALAPPDATA%\Orca\daemon-host so that app -; UPDATES cannot kill it — that relocation is what keeps terminals alive across -; updates. The same design means a normal uninstall's process sweep and file -; removal both miss it, leaving an orphaned daemon plus its runtime copy behind. -; -; The ${isUpdated} guard is essential: electron-builder runs this uninstaller as -; part of uninstallOldVersion on EVERY update, and killing the daemon there would -; defeat the whole feature. Only clean up on a genuine uninstall. -; -; The image name and the LOCALAPPDATA folder name must stay in sync with -; DAEMON_HOST_EXE_NAME and LOCAL_HOST_ROOT_NAME in -; src/main/daemon/daemon-host-relocation.ts. -!macro customUnInstall - ${ifNot} ${isUpdated} - nsExec::Exec 'taskkill /F /IM orca-terminal-daemon.exe' - ; Give the OS a moment to release the image lock before removing the tree. - Sleep 500 - RMDir /r "$LOCALAPPDATA\Orca\daemon-host" - ${endIf} -!macroend diff --git a/config/nsis/orca-installer-hooks.nsh b/config/nsis/orca-installer-hooks.nsh new file mode 100644 index 00000000000..ca80c99fc6d --- /dev/null +++ b/config/nsis/orca-installer-hooks.nsh @@ -0,0 +1,79 @@ +; electron-builder NSIS hooks for the Orca Windows installer. +; +; electron-builder accepts exactly ONE `nsis.include` file, so every customInstall / +; customUnInstall hook Orca needs lives here. + +; --------------------------------------------------------------------------- +; Markdown "Open with Orca" (issue #10138) +; +; Why hand-rolled instead of electron-builder's `fileAssociations` on Windows: +; app-builder-lib emits !insertmacro APP_ASSOCIATE, whose first line is +; WriteRegStr SHELL_CONTEXT "Software\Classes\.md" "" "" +; That overwrites whichever editor currently owns .md, with no backup, for every +; existing user on their next UPDATE - and APP_UNASSOCIATE never restores it, so +; uninstalling Orca would leave .md pointing at a deleted ProgID. +; +; These writes are additive only. Registering a ProgID plus an OpenWithProgids +; hint and an Applications\\SupportedTypes entry puts Orca in Explorer's +; "Open with" list and in "Choose another app", while the default handler stays +; exactly where the user left it. Never add a `Software\Classes\.` default +; value here. +; +; MARKDOWN_PROGID must stay in sync with the extension list handled by +; isMarkdownDocumentName() in src/main/ipc/markdown-documents.ts. +; --------------------------------------------------------------------------- +!define MARKDOWN_PROGID "Orca.Markdown" + +!macro ORCA_REGISTER_MARKDOWN_OPEN_WITH EXT + WriteRegNone SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${MARKDOWN_PROGID}" + WriteRegStr SHELL_CONTEXT "Software\Classes\Applications\${APP_EXECUTABLE_FILENAME}\SupportedTypes" "${EXT}" "" +!macroend + +!macro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH EXT + DeleteRegValue SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${MARKDOWN_PROGID}" + DeleteRegValue SHELL_CONTEXT "Software\Classes\Applications\${APP_EXECUTABLE_FILENAME}\SupportedTypes" "${EXT}" +!macroend + +!macro customInstall + WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}" "" "Markdown Document" + WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\DefaultIcon" "" "$appExe,0" + WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\shell\open" "" "Open with ${PRODUCT_NAME}" + WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\shell\open\command" "" '"$appExe" "%1"' + !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".md" + !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".markdown" + !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".mdx" + ; Why: Explorer caches the association list until told otherwise. + System::Call "shell32::SHChangeNotify(i,i,i,i) (0x08000000, 0x1000, 0, 0)" +!macroend + +; --------------------------------------------------------------------------- +; Clean up the relocated terminal daemon on a REAL uninstall. +; +; Why: the daemon host is deliberately copied to a distinct image name +; (orca-terminal-daemon.exe) under %LOCALAPPDATA%\Orca\daemon-host so that app +; UPDATES cannot kill it — that relocation is what keeps terminals alive across +; updates. The same design means a normal uninstall's process sweep and file +; removal both miss it, leaving an orphaned daemon plus its runtime copy behind. +; +; The ${isUpdated} guard is essential: electron-builder runs this uninstaller as +; part of uninstallOldVersion on EVERY update, and killing the daemon there would +; defeat the whole feature. Only clean up on a genuine uninstall. +; +; The image name and the LOCALAPPDATA folder name must stay in sync with +; DAEMON_HOST_EXE_NAME and LOCAL_HOST_ROOT_NAME in +; src/main/daemon/daemon-host-relocation.ts. +!macro customUnInstall + ${ifNot} ${isUpdated} + nsExec::Exec 'taskkill /F /IM orca-terminal-daemon.exe' + ; Give the OS a moment to release the image lock before removing the tree. + Sleep 500 + RMDir /r "$LOCALAPPDATA\Orca\daemon-host" + ${endIf} + ; Why outside the ${isUpdated} guard: customInstall rewrites these on every update, so + ; dropping them during uninstallOldVersion is correct and keeps the pair symmetric. + DeleteRegKey SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}" + !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".md" + !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".markdown" + !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".mdx" + System::Call "shell32::SHChangeNotify(i,i,i,i) (0x08000000, 0x1000, 0, 0)" +!macroend diff --git a/config/scripts/electron-builder-markdown-associations.test.mjs b/config/scripts/electron-builder-markdown-associations.test.mjs new file mode 100644 index 00000000000..7ae3b1c9428 --- /dev/null +++ b/config/scripts/electron-builder-markdown-associations.test.mjs @@ -0,0 +1,116 @@ +import { existsSync } from 'node:fs' +import { readFile } from 'node:fs/promises' +import { createRequire } from 'node:module' +import { basename } from 'node:path' +import { describe, expect, it } from 'vitest' + +const require = createRequire(import.meta.url) +const electronBuilderConfig = require('../electron-builder.config.cjs') + +const MARKDOWN_EXTENSIONS = ['md', 'markdown', 'mdx'] + +// The exact shape app-builder-lib's APP_ASSOCIATE emits: a write to the DEFAULT ("") +// value of Software\Classes\.. Additive `WriteRegNone ...\OpenWithProgids` must not +// match, or the guard below would be unfalsifiable. +const DEFAULT_HANDLER_WRITE = /WriteRegStr\s+SHELL_CONTEXT\s+"Software\\Classes\\\.[a-z]+"\s+""/i + +// The hooks file documents the forbidden line in prose, so match executable script only. +const stripNsisCommentLines = (source) => + source + .split('\n') + .filter((line) => !/^\s*[;#]/.test(line)) + .join('\n') + +const readInstallerHooks = () => readFile(electronBuilderConfig.nsis.include, 'utf8') + +describe('electron-builder markdown file associations', () => { + // Why: any top-level (or `win.`) fileAssociations entry makes app-builder-lib's NSIS + // packager emit `!insertmacro APP_ASSOCIATE`, whose first line writes that DEFAULT value + // — silently taking .md from whichever editor owns it, for every existing user on their + // next UPDATE, with APP_UNASSOCIATE never restoring it. `rank: 'Alternate'` cannot + // prevent this; it is LSHandlerRank and applies to macOS only. So the mac block must + // stay under `mac.` — hoisting it up "to share it with Windows" is what this test blocks. + it('never claims the Windows default markdown handler', () => { + expect(electronBuilderConfig.fileAssociations).toBeUndefined() + expect(electronBuilderConfig.win?.fileAssociations).toBeUndefined() + }) + + it('joins the macOS Open With list for every markdown extension without owning it', () => { + const associations = electronBuilderConfig.mac.fileAssociations + // One entry per extension: an array `ext` would break the Linux packager's `*.${ext}` glob. + expect([...associations].map((association) => association.ext).sort()).toEqual( + [...MARKDOWN_EXTENSIONS].sort() + ) + for (const association of associations) { + expect(association).toMatchObject({ role: 'Editor', rank: 'Alternate' }) + } + }) + + // Why mimeTypes and not linux.fileAssociations: shared-mime-info already maps markdown to + // text/markdown, so the desktop entry only adds a handler and mimeapps.list keeps owning + // the default. A fileAssociations entry would ship a redundant glob override instead. + it('reuses the existing shared-mime-info markdown type on Linux', () => { + expect(electronBuilderConfig.linux.mimeTypes).toContain('text/markdown') + expect(electronBuilderConfig.linux.fileAssociations).toBeUndefined() + }) + + it('points the single NSIS include at the installer hooks file on disk', () => { + const includePath = electronBuilderConfig.nsis.include + expect(existsSync(includePath)).toBe(true) + expect(basename(includePath)).toBe('orca-installer-hooks.nsh') + }) + + // Guard for the guard: proves DEFAULT_HANDLER_WRITE really matches a takeover line, so + // the assertion below is a live check rather than a regex that can never fire. + it('recognizes an APP_ASSOCIATE-style default-handler write', () => { + for (const takeover of [ + ' WriteRegStr SHELL_CONTEXT "Software\\Classes\\.md" "" "Orca.Markdown"', + 'WriteRegStr SHELL_CONTEXT "Software\\Classes\\.markdown" "" "$0"' + ]) { + expect(takeover).toMatch(DEFAULT_HANDLER_WRITE) + } + expect( + 'WriteRegNone SHELL_CONTEXT "Software\\Classes\\.md\\OpenWithProgids" "Orca.Markdown"' + ).not.toMatch(DEFAULT_HANDLER_WRITE) + // Comment stripping must drop prose that quotes the bad line without swallowing a real + // one that happens to carry a trailing comment. + const stripped = stripNsisCommentLines( + [ + '; WriteRegStr SHELL_CONTEXT "Software\\Classes\\.md" "" ""', + ' WriteRegStr SHELL_CONTEXT "Software\\Classes\\.md" "" "$0" ; oops' + ].join('\n') + ) + expect(stripped.split('\n')).toHaveLength(1) + expect(stripped).toMatch(DEFAULT_HANDLER_WRITE) + }) + + it('registers Windows markdown Open With additively, never as the default', async () => { + const hooks = await readInstallerHooks() + + expect(stripNsisCommentLines(hooks)).not.toMatch(DEFAULT_HANDLER_WRITE) + // The additive hint that puts Orca in Explorer's "Open with" list. + expect(hooks).toMatch( + /WriteRegNone\s+SHELL_CONTEXT\s+"Software\\Classes\\\$\{EXT\}\\OpenWithProgids"/ + ) + expect(hooks).toMatch(/!macro\s+ORCA_REGISTER_MARKDOWN_OPEN_WITH\s+EXT/) + for (const ext of MARKDOWN_EXTENSIONS) { + expect(hooks).toContain(`ORCA_REGISTER_MARKDOWN_OPEN_WITH ".${ext}"`) + expect(hooks).toContain(`ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".${ext}"`) + } + expect(hooks).toMatch(/!macro\s+customInstall\b/) + expect(hooks).toMatch(/!macro\s+customUnInstall\b/) + }) + + // Why: this include was renamed from daemon-host-uninstall.nsh to carry the markdown + // hooks too. electron-builder allows only one include, so a merge that drops the daemon + // sweep would silently orphan a running orca-terminal-daemon.exe on every uninstall. + it('keeps the daemon-host uninstall sweep across the include rename', async () => { + const hooks = await readInstallerHooks() + + expect(hooks).toContain('orca-terminal-daemon.exe') + expect(hooks).toContain('$LOCALAPPDATA\\Orca\\daemon-host') + // Without this guard, uninstallOldVersion would kill the daemon on every update — + // defeating the relocation that keeps terminals alive across updates. + expect(hooks).toMatch(/\$\{ifNot\}\s+\$\{isUpdated\}/) + }) +}) diff --git a/src/main/daemon/daemon-host-relocation.ts b/src/main/daemon/daemon-host-relocation.ts index a7e8f2b6db2..6d94bea06e4 100644 --- a/src/main/daemon/daemon-host-relocation.ts +++ b/src/main/daemon/daemon-host-relocation.ts @@ -34,7 +34,7 @@ export type RelocatedDaemonHost = { const HOST_SUBDIR = 'daemon-host' const MARKER_NAME = '.materialized.json' -// LOCAL appData (not roaming) so OneDrive/roaming never syncs this ~260MB runtime. Shared with NSIS uninstall (config/nsis/daemon-host-uninstall.nsh) — keep in sync. +// LOCAL appData (not roaming) so OneDrive/roaming never syncs this ~260MB runtime. Shared with NSIS uninstall (config/nsis/orca-installer-hooks.nsh) — keep in sync. const LOCAL_HOST_ROOT_NAME = 'Orca' // Copy of Orca.exe renamed to a distinct image name so the NSIS updater's `taskkill /IM Orca.exe` can't match it. diff --git a/src/main/index.ts b/src/main/index.ts index acf913b2c7a..522e59b908f 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -12,6 +12,7 @@ import { registerMainProcessIpcHandlers } from './startup/main-process-ipc-boots import { initializeMainProcessReady } from './startup/main-process-ready' import { installMainProcessQuitHandlers } from './startup/main-process-quit' import { shouldActivateDesktopForSecondInstance } from './startup/single-instance-lock' +import { resolveOpenedMarkdownDocuments } from './startup/os-opened-markdown-files' function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {}): BrowserWindow { return openMainWindowController(options) @@ -27,6 +28,7 @@ function requestDesktopActivation(argv: readonly string[] = []): void { state.skillShareDeepLinks.capture(argv, (shareId) => { state.mainWindow?.webContents.send('ui:openSkillShare', shareId) }) + state.osOpenedMarkdownFiles.capture(argv, publishOsOpenedMarkdownFiles) // Why: a duplicate `orca serve` must not drag a headless server into opening a desktop window (#11935). if (!shouldActivateDesktopForSecondInstance(argv)) { return @@ -34,6 +36,39 @@ function requestDesktopActivation(argv: readonly string[] = []): void { state.desktopActivationGate?.requestActivation() } +/** + * Hands buffered OS-opened markdown paths to a renderer that has proven it is listening. + * + * Until that proof arrives the paths stay buffered, because `webContents.send` to a renderer + * with no listener attached is dropped silently and the queue would be gone. + */ +function publishOsOpenedMarkdownFiles(): void { + const targetWindow = state.mainWindow + if (!state.markdownFileOpenListenerReady || !targetWindow || targetWindow.isDestroyed()) { + return + } + // Why consumed before the await: a renderer pull racing this resolve must not take the same + // batch again. The restore() calls hand it back if delivery turns out to be impossible. + const filePaths = state.osOpenedMarkdownFiles.consume() + if (filePaths.length === 0) { + return + } + void resolveOpenedMarkdownDocuments(filePaths) + .then((documents) => { + if (targetWindow.isDestroyed() || targetWindow.webContents.isDestroyed()) { + state.osOpenedMarkdownFiles.restore(filePaths) + return + } + if (documents.length > 0) { + targetWindow.webContents.send('ui:openMarkdownFiles', documents) + } + }) + .catch((error) => { + state.osOpenedMarkdownFiles.restore(filePaths) + console.warn('[os-open] Failed to resolve OS-opened markdown files:', error) + }) +} + const handleMacAppActivation = createMacAppActivationHandler({ getWindow: () => state.mainWindow, requestActivation: requestDesktopActivation @@ -53,7 +88,22 @@ if (preflightReady) { event.preventDefault() requestDesktopActivation([url]) }) + // Why: macOS delivers "Open With" as open-file, often before `ready`, and only to a handler + // that claims the event. Non-markdown paths stay unclaimed so the OS default handler wins. + app.on('open-file', (event, filePath) => { + if (!state.osOpenedMarkdownFiles.captureFilePaths([filePath], publishOsOpenedMarkdownFiles)) { + return + } + event.preventDefault() + // Why gated on isReady: pre-ready the cold-start window is already on its way, and + // activating the gate here would try to open one before Electron can. + if (app.isReady()) { + requestDesktopActivation() + } + }) state.skillShareDeepLinks.capture(process.argv) + // Why no publish: nothing is listening this early, so the first renderer pulls these on mount. + state.osOpenedMarkdownFiles.capture(process.argv) registerMainProcessIpcHandlers() installMainProcessQuitHandlers() void app.whenReady().then(async () => { diff --git a/src/main/startup/main-process-ipc-bootstrap.ts b/src/main/startup/main-process-ipc-bootstrap.ts index 3f364a9335c..89be84d2119 100644 --- a/src/main/startup/main-process-ipc-bootstrap.ts +++ b/src/main/startup/main-process-ipc-bootstrap.ts @@ -2,6 +2,7 @@ import { ipcMain } from 'electron' import { recoverLegacyWorkerTerminalsForRendererStartup } from './legacy-worker-renderer-recovery' import { logStartupMilestone } from './startup-diagnostics' import { mainProcessState as state } from './main-process-state' +import { resolveOpenedMarkdownDocuments } from './os-opened-markdown-files' export function registerMainProcessIpcHandlers(): void { ipcMain.handle('app:awaitFirstWindowStartupServices', async () => { @@ -36,6 +37,20 @@ export function registerMainProcessIpcHandlers(): void { state.pendingOpenSettings.matches(event.sender.id, { consume: true }) ) ipcMain.handle('ui:consumePendingSkillShare', () => state.skillShareDeepLinks.consume()) + // Why: the renderer pulls this once its ui:openMarkdownFiles listener attaches, so a + // cold-start "Open With" queued before mount still opens. The pull doubles as the proof + // that the listener is live, which is what lets main start pushing. + ipcMain.handle('ui:consumePendingMarkdownFileOpens', async () => { + state.markdownFileOpenListenerReady = true + const filePaths = state.osOpenedMarkdownFiles.consume() + try { + return await resolveOpenedMarkdownDocuments(filePaths) + } catch (error) { + // Why restored: the renderer never received these, so a later mount must still get them. + state.osOpenedMarkdownFiles.restore(filePaths) + throw error + } + }) ipcMain.handle( 'app:startupDiagnostic', (_event, event: string, details?: Record) => { diff --git a/src/main/startup/main-process-state.ts b/src/main/startup/main-process-state.ts index ea1e0a33299..05c45d5b567 100644 --- a/src/main/startup/main-process-state.ts +++ b/src/main/startup/main-process-state.ts @@ -36,6 +36,7 @@ import type { ServeOptions } from './main-process-serve' import type { HangDetectionMarker } from '../hang-watchdog/hang-detection-marker' import { ServeReadinessPublisher } from '../server/serve-readiness' import { SkillShareDeepLinkState } from './skill-share-deep-link-state' +import { OsOpenedMarkdownFileState } from './os-opened-markdown-files' import { DEFAULT_GPU_CRASH_FALLBACK_THRESHOLD, DEFAULT_GPU_CRASH_FALLBACK_WINDOW_MS, @@ -90,6 +91,12 @@ export const mainProcessState = { // Why: a tray "Settings…" click can precede the renderer's ui:openSettings listener; it pulls this one-shot on mount. pendingOpenSettings: createWebContentsTimedFlag(), skillShareDeepLinks: new SkillShareDeepLinkState(), + // Why: a Finder/Explorer "Open With" can land before any window exists; the renderer pulls this buffer on mount. + osOpenedMarkdownFiles: new OsOpenedMarkdownFileState(), + // Why a latch and not just "a window exists": a window can be up while its renderer has not + // attached the ui:openMarkdownFiles listener yet, and a push into that gap is dropped by + // Electron with no error. Only the renderer's own pull proves the listener is live. + markdownFileOpenListenerReady: false, firstWindowStartupServicesReady: Promise.resolve(), managedWslCliReconciliationReady: Promise.resolve(), managedWslCliStartupBarrierReady: Promise.resolve(), diff --git a/src/main/startup/main-window-controller.ts b/src/main/startup/main-window-controller.ts index 57763b23ab1..0d935d5f84a 100644 --- a/src/main/startup/main-window-controller.ts +++ b/src/main/startup/main-window-controller.ts @@ -145,6 +145,9 @@ export function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {} clearExpectedRendererReload(rendererWebContentsId) recordCrashBreadcrumb('main_window_loaded') logStartupMilestone('did-finish-load') + // Why cleared here: a reload drops the old ui:openMarkdownFiles listener, and the fresh + // renderer re-attaches by pulling. Pushing into the gap between would be silently lost. + state.markdownFileOpenListenerReady = false const currentStore = state.store if (currentStore && resolveConsent(currentStore.getSettings()).effective === 'enabled') { trackAppOpenedOnce() diff --git a/src/main/startup/os-opened-markdown-delivery.test.ts b/src/main/startup/os-opened-markdown-delivery.test.ts new file mode 100644 index 00000000000..0647516e3fa --- /dev/null +++ b/src/main/startup/os-opened-markdown-delivery.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it, vi } from 'vitest' +import { OsOpenedMarkdownFileState } from './os-opened-markdown-files' + +/** + * The two ways a queued "Open With" can be lost between main and the renderer. Both are + * about ownership: main must not drop paths it has not proven the renderer received. + */ +describe('os-opened markdown delivery ownership', () => { + it('keeps the batch when resolution rejects on the pull path', async () => { + const state = new OsOpenedMarkdownFileState() + state.captureFilePaths(['/notes/a.md']) + const resolve = vi.fn().mockRejectedValue(new Error('floating root unavailable')) + + // Mirrors the ipcMain.handle('ui:consumePendingMarkdownFileOpens') body. + const pull = async (): Promise => { + const filePaths = state.consume() + try { + return await resolve(filePaths) + } catch (error) { + state.restore(filePaths) + throw error + } + } + + await expect(pull()).rejects.toThrow('floating root unavailable') + // Without the restore the file would be gone and no later mount could ever open it. + expect(state.consume()).toEqual(['/notes/a.md']) + }) + + it('holds the batch while the renderer listener is not yet attached', () => { + const state = new OsOpenedMarkdownFileState() + const send = vi.fn() + let listenerReady = false + + // Mirrors publishOsOpenedMarkdownFiles()'s guard. + const publish = (): void => { + if (!listenerReady) { + return + } + const filePaths = state.consume() + if (filePaths.length > 0) { + send(filePaths) + } + } + + // A window exists, but the renderer has not mounted its bridge yet: send() here would be + // dropped by Electron with no error, and consuming would destroy the queue. + state.captureFilePaths(['/notes/a.md'], publish) + expect(send).not.toHaveBeenCalled() + + // The renderer's pull is what proves the listener is live. + listenerReady = true + state.captureFilePaths(['/notes/b.md'], publish) + expect(send).toHaveBeenCalledExactlyOnceWith(['/notes/a.md', '/notes/b.md']) + expect(state.consume()).toEqual([]) + }) + + it('restores a batch the window could no longer receive', () => { + const state = new OsOpenedMarkdownFileState() + state.captureFilePaths(['/notes/a.md']) + const filePaths = state.consume() + + // Window died between consume and send. + state.restore(filePaths) + + expect(state.consume()).toEqual(['/notes/a.md']) + }) +}) diff --git a/src/main/startup/os-opened-markdown-files.test.ts b/src/main/startup/os-opened-markdown-files.test.ts new file mode 100644 index 00000000000..f174e10d834 --- /dev/null +++ b/src/main/startup/os-opened-markdown-files.test.ts @@ -0,0 +1,306 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve, sep } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { isMarkdownDocumentName } from '../ipc/markdown-documents' +import { + MAX_PENDING_OS_OPENED_MARKDOWN_FILES, + OsOpenedMarkdownFileState, + markdownPathsFromArguments, + resolveOpenedMarkdownDocuments +} from './os-opened-markdown-files' + +vi.mock('../ipc/filesystem-auth', () => ({ + authorizeExternalPath: vi.fn() +})) +vi.mock('../ipc/floating-workspace-directory', () => ({ + ensureDefaultFloatingWorkspacePath: vi.fn() +})) + +const { authorizeExternalPath } = await import('../ipc/filesystem-auth') +const { ensureDefaultFloatingWorkspacePath } = await import('../ipc/floating-workspace-directory') + +describe('markdownPathsFromArguments', () => { + it('keeps absolute markdown paths and drops other extensions', () => { + expect( + markdownPathsFromArguments( + [ + '/Users/dev/notes/a.md', + '/Users/dev/notes/b.markdown', + '/Users/dev/notes/c.mdx', + '/Users/dev/notes/d.txt', + '/Users/dev/src/e.tsx', + '/Users/dev/notes/README' + ], + 'darwin' + ) + ).toEqual(['/Users/dev/notes/a.md', '/Users/dev/notes/b.markdown', '/Users/dev/notes/c.mdx']) + }) + + it('drops switches, including Chromium-style ones that would otherwise look like values', () => { + expect( + markdownPathsFromArguments( + ['--serve', '-v', '--allow-file-access-from-files', '/Users/dev/notes/a.md'], + 'darwin' + ) + ).toEqual(['/Users/dev/notes/a.md']) + }) + + it('drops the executable and dev entries because none of them end in a markdown extension', () => { + const nonDocumentEntries = [ + '/Applications/Orca.app/Contents/MacOS/Orca', + '/Users/dev/orca/out/main/index.js', + '/Applications/Orca.app/Contents/Resources/app.asar' + ] + // The module documents that the extension check alone excludes these; hold it to that. + for (const entry of nonDocumentEntries) { + expect(isMarkdownDocumentName(entry), entry).toBe(false) + } + expect( + markdownPathsFromArguments([...nonDocumentEntries, '/Users/dev/notes/a.md'], 'darwin') + ).toEqual(['/Users/dev/notes/a.md']) + }) + + it('drops relative paths because a second instance has no meaningful cwd', () => { + expect( + markdownPathsFromArguments(['readme.md', './docs/a.md', '../up.md', ''], 'darwin') + ).toEqual([]) + }) + + it('accepts win32 drive-letter and UNC paths', () => { + expect( + markdownPathsFromArguments( + ['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md', 'C:\\Users\\dev\\todo.txt'], + 'win32' + ) + ).toEqual(['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md']) + }) + + it('dedupes case-insensitively on win32 and keeps the first spelling', () => { + expect(markdownPathsFromArguments(['C:\\notes\\A.md', 'c:\\notes\\a.md'], 'win32')).toEqual([ + 'C:\\notes\\A.md' + ]) + }) + + it('normalizes parent segments before deduping', () => { + expect( + markdownPathsFromArguments(['C:\\notes\\sub\\..\\a.md', 'C:\\notes\\a.md'], 'win32') + ).toEqual(['C:\\notes\\a.md']) + expect(markdownPathsFromArguments(['/docs/../notes/a.md', '/notes/a.md'], 'darwin')).toEqual([ + '/notes/a.md' + ]) + }) + + it('does not dedupe case-insensitively on posix, where casing is a different file', () => { + expect(markdownPathsFromArguments(['/a/A.md', '/a/a.md'], 'linux')).toEqual([ + '/a/A.md', + '/a/a.md' + ]) + }) + + it('accepts a file:// URI, which the desktop entry %U field code permits', () => { + // Why defensive rather than load-bearing: GLib decodes a local file:// URI to a plain + // path before spawning (measured on Ubuntu 24.04), so Linux hits the plain-path branch + // today. The %U spec still allows a URI, and a launcher that passes one literally would + // otherwise be dropped without a trace. + expect( + markdownPathsFromArguments( + ['file:///home/me/notes/a.md', 'file:///home/me/notes/b.txt'], + 'linux' + ) + ).toEqual(['/home/me/notes/a.md']) + }) + + it('percent-decodes a file:// URI so a path with spaces still opens', () => { + expect(markdownPathsFromArguments(['file:///home/me/design%20notes.md'], 'linux')).toEqual([ + '/home/me/design notes.md' + ]) + }) + + it('decodes win32 file:// URIs, including UNC authority form', () => { + expect( + markdownPathsFromArguments( + ['file:///C:/Users/me/todo.md', 'file://server/share/a.md'], + 'win32' + ) + ).toEqual(['C:\\Users\\me\\todo.md', '\\\\server\\share\\a.md']) + }) + + it('dedupes a path delivered as both a URI and a bare path', () => { + expect(markdownPathsFromArguments(['file:///home/me/a.md', '/home/me/a.md'], 'linux')).toEqual([ + '/home/me/a.md' + ]) + }) + + it('drops a malformed or non-file URL instead of throwing', () => { + expect(() => + markdownPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') + ).not.toThrow() + expect( + markdownPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') + ).toEqual([]) + }) + + it('honours the platform argument rather than the host OS', () => { + const argv = ['C:\\notes\\a.md', '/notes/b.md'] + // Same argv, two platforms: a win32 path is not absolute to posix, and posix input is + // renormalized to backslashes on win32. Neither result may depend on where the suite runs. + expect(markdownPathsFromArguments(argv, 'darwin')).toEqual(['/notes/b.md']) + expect(markdownPathsFromArguments(argv, 'win32')).toEqual(['C:\\notes\\a.md', '\\notes\\b.md']) + }) +}) + +// Why resolve(): the state uses the host platform by default, so fixture paths must already be +// spelled the way the host's path module normalizes them (`\n\a.md` and a drive on Windows). +const hostPath = (name: string): string => resolve(sep, 'notes', name) + +describe('OsOpenedMarkdownFileState', () => { + it('reports no capture and does not publish when argv carries no markdown', () => { + const state = new OsOpenedMarkdownFileState() + const publish = vi.fn() + + expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', '--serve'], publish)).toBe( + false + ) + expect(publish).not.toHaveBeenCalled() + expect(state.consume()).toEqual([]) + }) + + it('buffers and publishes when argv carries markdown', () => { + const state = new OsOpenedMarkdownFileState() + const publish = vi.fn() + const filePath = hostPath('a.md') + + expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', filePath], publish)).toBe( + true + ) + expect(publish).toHaveBeenCalledTimes(1) + expect(state.consume()).toEqual([filePath]) + }) + + it('captures a single macOS open-file path', () => { + const state = new OsOpenedMarkdownFileState() + const publish = vi.fn() + const filePath = hostPath('a.md') + + expect(state.captureFilePaths([filePath], publish)).toBe(true) + expect(state.captureFilePaths([hostPath('a.png')], publish)).toBe(false) + expect(publish).toHaveBeenCalledTimes(1) + expect(state.consume()).toEqual([filePath]) + }) + + it('does not duplicate a path captured twice', () => { + const state = new OsOpenedMarkdownFileState() + const filePath = hostPath('a.md') + + state.captureFilePaths([filePath]) + state.captureFilePaths([filePath]) + state.capture(['orca', filePath]) + + expect(state.consume()).toEqual([filePath]) + }) + + it('drains the buffer on consume', () => { + const state = new OsOpenedMarkdownFileState() + const paths = [hostPath('a.md'), hostPath('b.md')] + state.captureFilePaths(paths) + + expect(state.consume()).toEqual(paths) + expect(state.consume()).toEqual([]) + }) + + it('restores an undelivered batch at the front of the buffer', () => { + const state = new OsOpenedMarkdownFileState() + state.captureFilePaths([hostPath('later.md')]) + + state.restore([hostPath('undelivered.md')]) + + expect(state.consume()).toEqual([hostPath('undelivered.md'), hostPath('later.md')]) + }) + + it('caps the buffer when captures overflow it', () => { + const state = new OsOpenedMarkdownFileState() + const overflow = MAX_PENDING_OS_OPENED_MARKDOWN_FILES + 5 + const paths = Array.from({ length: overflow }, (_, index) => hostPath(`file-${index}.md`)) + + expect(state.captureFilePaths(paths)).toBe(true) + + expect(state.consume()).toEqual(paths.slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES)) + }) + + it('caps the buffer when a restore overflows it', () => { + const state = new OsOpenedMarkdownFileState() + state.captureFilePaths([hostPath('pending.md')]) + const restored = Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => + hostPath(`restored-${index}.md`) + ) + + state.restore(restored) + + const pending = state.consume() + expect(pending).toHaveLength(MAX_PENDING_OS_OPENED_MARKDOWN_FILES) + expect(pending).toEqual(restored) + }) +}) + +describe('resolveOpenedMarkdownDocuments', () => { + let floatingRoot: string + let fileRoot: string + + beforeEach(async () => { + vi.mocked(authorizeExternalPath).mockClear() + vi.mocked(ensureDefaultFloatingWorkspacePath).mockClear() + floatingRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-root-')) + fileRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-files-')) + vi.mocked(ensureDefaultFloatingWorkspacePath).mockResolvedValue(floatingRoot) + }) + + afterEach(async () => { + await rm(floatingRoot, { recursive: true, force: true }) + await rm(fileRoot, { recursive: true, force: true }) + }) + + it('resolves a real file outside the floating root to a basename-relative document', async () => { + const filePath = join(fileRoot, 'design notes.md') + await writeFile(filePath, '# hi\n', 'utf8') + + const documents = await resolveOpenedMarkdownDocuments([filePath]) + + expect(documents).toEqual([ + { + filePath, + relativePath: 'design notes.md', + basename: 'design notes.md', + name: 'design notes' + } + ]) + expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) + }) + + it('drops a directory that merely looks like a markdown file', async () => { + const bundlePath = join(fileRoot, 'bundle.md') + await mkdir(bundlePath) + const filePath = join(fileRoot, 'real.md') + await writeFile(filePath, '# hi\n', 'utf8') + + const documents = await resolveOpenedMarkdownDocuments([bundlePath, filePath]) + + expect(documents.map((document) => document.filePath)).toEqual([filePath]) + // Security contract: a path we never validated must never be authorized for renderer reads. + expect(authorizeExternalPath).toHaveBeenCalledTimes(1) + expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) + }) + + it('drops a path that no longer exists without authorizing it', async () => { + const missingPath = join(fileRoot, 'gone.md') + + expect(await resolveOpenedMarkdownDocuments([missingPath])).toEqual([]) + expect(authorizeExternalPath).not.toHaveBeenCalled() + }) + + it('returns nothing for an empty input without touching the filesystem', async () => { + expect(await resolveOpenedMarkdownDocuments([])).toEqual([]) + expect(ensureDefaultFloatingWorkspacePath).not.toHaveBeenCalled() + expect(authorizeExternalPath).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/startup/os-opened-markdown-files.ts b/src/main/startup/os-opened-markdown-files.ts new file mode 100644 index 00000000000..dae27fb7a78 --- /dev/null +++ b/src/main/startup/os-opened-markdown-files.ts @@ -0,0 +1,149 @@ +import { stat } from 'node:fs/promises' +import path from 'node:path' +import { fileURLToPath } from 'node:url' +import type { MarkdownDocument } from '../../shared/filesystem-entry-types' +import { authorizeExternalPath } from '../ipc/filesystem-auth' +import { ensureDefaultFloatingWorkspacePath } from '../ipc/floating-workspace-directory' +import { isMarkdownDocumentName, markdownDocumentFromFilePath } from '../ipc/markdown-documents' + +// Why: a shell can only ever hand over the files the user selected; anything past this is a +// runaway argv, and buffering it unbounded would pin the paths for the whole session. +export const MAX_PENDING_OS_OPENED_MARKDOWN_FILES = 32 + +/** + * Resolves one argv entry to a local absolute path, or null if it is not one. + * + * Why file:// is accepted defensively: electron-builder appends the `%U` field code to the + * generated Linux `Exec=` line, and `%U` is specified as "URLs". GLib turns out to decode a + * local `file://` URI back to a plain path before spawning (measured on Ubuntu 24.04, via + * the same `launch_uris` call a file manager makes), so the branch below is not what fires + * there today — but the spec permits a URI, and a launcher that honours it literally would + * otherwise be silently dropped. macOS `open-file` and the Windows shell `%1` pass paths. + */ +function localPathFromArgument(argument: string, platform: NodeJS.Platform): string | null { + const pathApi = platform === 'win32' ? path.win32 : path.posix + if (argument.startsWith('file://')) { + try { + // Why the explicit windows flag: this must decode the same way on any host so the + // behaviour is testable, and it is what turns `file://server/share` back into a UNC path. + return fileURLToPath(argument, { windows: platform === 'win32' }) + } catch { + return null + } + } + return pathApi.isAbsolute(argument) ? argument : null +} + +/** + * Absolute markdown paths an OS "Open With" put on a launch or second-instance argv. + * + * Why no executable/asar/dev-entry filtering: none of those argv entries end in a markdown + * extension, so the extension check already excludes them. Relative entries are dropped + * because the shell always passes absolute paths and `cwd` is meaningless for a second instance. + */ +export function markdownPathsFromArguments( + argv: readonly string[], + platform: NodeJS.Platform = process.platform +): string[] { + const pathApi = platform === 'win32' ? path.win32 : path.posix + const seen = new Set() + const paths: string[] = [] + for (const rawArgument of argv) { + if (!rawArgument || rawArgument.startsWith('-')) { + continue + } + const argument = localPathFromArgument(rawArgument, platform) + if (!argument || !isMarkdownDocumentName(argument)) { + continue + } + const normalized = pathApi.normalize(argument) + // Why lowercased on win32: the shell round-trips drive letters and 8.3 casing + // inconsistently, and two spellings of one path must not open two tabs. + const key = platform === 'win32' ? normalized.toLowerCase() : normalized + if (seen.has(key)) { + continue + } + seen.add(key) + paths.push(normalized) + } + return paths +} + +/** + * Buffers markdown paths the OS handed us until a renderer can receive them. + * + * Mirrors SkillShareDeepLinkState: main pushes when a window is already live, and the + * renderer pulls the same buffer when its listener attaches, so a cold-start "Open With" + * that lands before mount is not dropped. + */ +export class OsOpenedMarkdownFileState { + private pending: string[] = [] + + /** Returns true when argv carried at least one markdown path. */ + capture(argv: readonly string[], publish?: () => void): boolean { + return this.add(markdownPathsFromArguments(argv), publish) + } + + /** Returns true when at least one path was a markdown document. */ + captureFilePaths(filePaths: readonly string[], publish?: () => void): boolean { + return this.add(markdownPathsFromArguments(filePaths), publish) + } + + consume(): string[] { + const pending = this.pending + this.pending = [] + return pending + } + + /** Puts an undelivered batch back at the front so the next renderer still receives it. */ + restore(filePaths: readonly string[]): void { + this.pending = [...filePaths, ...this.pending].slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES) + } + + private add(filePaths: readonly string[], publish?: () => void): boolean { + if (filePaths.length === 0) { + return false + } + const merged = [...this.pending] + for (const filePath of filePaths) { + if (!merged.includes(filePath)) { + merged.push(filePath) + } + } + this.pending = merged.slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES) + publish?.() + return true + } +} + +/** + * Turns OS-handed paths into the same `MarkdownDocument` shape the floating workspace's own + * file picker produces, authorizing each one for the renderer's later read. + */ +export async function resolveOpenedMarkdownDocuments( + filePaths: readonly string[] +): Promise { + if (filePaths.length === 0) { + return [] + } + const floatingRoot = await ensureDefaultFloatingWorkspacePath() + const documents: MarkdownDocument[] = [] + for (const filePath of filePaths) { + try { + // Why: the shell can hand over a bundle directory named `*.md`, or a path already + // deleted by the time we resolve. Authorize only something that is really a file. + if (!(await stat(filePath)).isFile()) { + continue + } + } catch { + continue + } + authorizeExternalPath(filePath) + documents.push( + markdownDocumentFromFilePath(floatingRoot, filePath, { + outsideRootRelativePath: 'basename' + }) + ) + } + return documents +} diff --git a/src/main/startup/os-opened-markdown-wiring.test.ts b/src/main/startup/os-opened-markdown-wiring.test.ts new file mode 100644 index 00000000000..179ca0820ca --- /dev/null +++ b/src/main/startup/os-opened-markdown-wiring.test.ts @@ -0,0 +1,57 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +const read = (relativePath: string): string => + // Why source text: this wiring is module-scope side effects in the entry point, which no + // unit test can import without booting Electron. These guards pin the call shapes instead. + readFileSync(join(process.cwd(), relativePath), 'utf8').replaceAll('"', "'") + +describe('os-opened markdown wiring', () => { + const index = read('src/main/index.ts') + const bootstrap = read('src/main/startup/main-process-ipc-bootstrap.ts') + const controller = read('src/main/startup/main-window-controller.ts') + + it('captures argv before the serve-duplicate early return', () => { + const captureIndex = index.indexOf( + 'state.osOpenedMarkdownFiles.capture(argv, publishOsOpenedMarkdownFiles)' + ) + const serveGuardIndex = index.indexOf('if (!shouldActivateDesktopForSecondInstance(argv)) {') + + expect(captureIndex).toBeGreaterThanOrEqual(0) + expect(serveGuardIndex).toBeGreaterThanOrEqual(0) + // A duplicate `orca serve` returns early; capturing after that would drop the user's files. + expect(captureIndex).toBeLessThan(serveGuardIndex) + }) + + it('claims the macOS open-file event so the default handler does not win it', () => { + const handlerIndex = index.indexOf("app.on('open-file'") + expect(handlerIndex).toBeGreaterThanOrEqual(0) + + const preventDefaultIndex = index.indexOf('event.preventDefault()', handlerIndex) + const nextRegistrationIndex = index.indexOf('app.on(', handlerIndex + 1) + expect(preventDefaultIndex).toBeGreaterThan(handlerIndex) + if (nextRegistrationIndex !== -1) { + expect(preventDefaultIndex).toBeLessThan(nextRegistrationIndex) + } + }) + + it('captures the cold-start argv and lets the renderer pull it after mount', () => { + expect(index).toContain('state.osOpenedMarkdownFiles.capture(process.argv)') + expect(bootstrap).toContain("ipcMain.handle('ui:consumePendingMarkdownFileOpens'") + }) + + // Why: `webContents.send` to a renderer that has not attached the listener is dropped with no + // error, so publishing on "a window exists" alone would consume the queue into a void. + it('only pushes once the renderer has proven its listener is attached', () => { + expect(index).toContain('!state.markdownFileOpenListenerReady') + expect(bootstrap).toContain('state.markdownFileOpenListenerReady = true') + // A reload drops the listener; the fresh renderer re-proves itself by pulling again. + expect(controller).toContain('state.markdownFileOpenListenerReady = false') + }) + + it('restores an undelivered batch on both the push and the pull path', () => { + expect(index).toContain('state.osOpenedMarkdownFiles.restore(filePaths)') + expect(bootstrap).toContain('state.osOpenedMarkdownFiles.restore(filePaths)') + }) +}) diff --git a/src/preload/api/ui-bridge-state-and-menu-commands.ts b/src/preload/api/ui-bridge-state-and-menu-commands.ts index 25476165cfb..34eb83886c8 100644 --- a/src/preload/api/ui-bridge-state-and-menu-commands.ts +++ b/src/preload/api/ui-bridge-state-and-menu-commands.ts @@ -1,3 +1,4 @@ +import type { MarkdownDocument } from '../../shared/filesystem-entry-types' import { ipcRenderer } from 'electron' import type { PersistedUIState } from '../../shared/persisted-ui-state-types' import type { KeybindingActionId } from '../../shared/keybindings' @@ -26,6 +27,14 @@ export const uiStateAndMenuCommandsApi = { }, consumePendingSkillShare: (): Promise => ipcRenderer.invoke('ui:consumePendingSkillShare'), + onOpenMarkdownFiles: (callback: (documents: MarkdownDocument[]) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, documents: MarkdownDocument[]): void => + callback(documents) + ipcRenderer.on('ui:openMarkdownFiles', listener) + return () => ipcRenderer.removeListener('ui:openMarkdownFiles', listener) + }, + consumePendingMarkdownFileOpens: (): Promise => + ipcRenderer.invoke('ui:consumePendingMarkdownFileOpens'), onOpenSetupGuide: (callback: () => void): (() => void) => { const listener = (_event: Electron.IpcRendererEvent) => callback() ipcRenderer.on('ui:openSetupGuide', listener) diff --git a/src/preload/api/ui-command-event-api.ts b/src/preload/api/ui-command-event-api.ts index e63034b5233..0876104e471 100644 --- a/src/preload/api/ui-command-event-api.ts +++ b/src/preload/api/ui-command-event-api.ts @@ -1,3 +1,4 @@ +import type { MarkdownDocument } from '../../shared/filesystem-entry-types' import type { PersistedUIState } from '../../shared/persisted-ui-state-types' import type { TuiAgent } from '../../shared/tui-agent' import type { @@ -48,6 +49,10 @@ export type UiCommandEventApi = { consumePendingOpenSettings: () => Promise onOpenSkillShare: (callback: (shareId: string) => void) => () => void consumePendingSkillShare: () => Promise + /** OS "Open With" markdown paths pushed while a renderer is already listening. */ + onOpenMarkdownFiles: (callback: (documents: MarkdownDocument[]) => void) => () => void + /** Drains the "Open With" paths queued before this renderer's listener attached. */ + consumePendingMarkdownFileOpens: () => Promise onOpenSetupGuide: (callback: () => void) => () => void onOpenFeatureTour: (callback: () => void) => () => void onOpenCrashReport: (callback: () => void) => () => void diff --git a/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts b/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts index a60cdc7d93e..8ef85f7e556 100644 --- a/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts +++ b/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts @@ -4,7 +4,7 @@ import { resolveGroupTabFromVisibleId } from '@/components/tab-group/tab-group-v import { getConnectionId } from '@/lib/connection-context' import { createUntitledMarkdownFileWithTemplateSelection } from '@/lib/create-untitled-markdown' import { ensureClientCreationActionAllowed } from '@/lib/client-creation-action-error' -import { detectLanguage } from '@/lib/language-detect' +import { openMarkdownDocumentInFloatingWorkspace } from '@/lib/open-markdown-in-floating-workspace' import { extractIpcErrorMessage } from '@/lib/ipc-error' import { focusTerminalTabSurface } from '@/lib/focus-terminal-tab-surface' import { translate } from '@/i18n/i18n' @@ -123,21 +123,9 @@ export function useFloatingTerminalCreateActions({ if (!document) { return } - openFile( - { - filePath: document.filePath, - relativePath: document.relativePath, - worktreeId: FLOATING_TERMINAL_WORKTREE_ID, - language: detectLanguage(document.relativePath), - mode: 'edit', - runtimeEnvironmentId: null - }, - { - preview: false, - targetGroupId: activeGroup?.id, - suppressActiveRuntimeFallback: true - } - ) + openMarkdownDocumentInFloatingWorkspace(openFile, document, { + targetGroupId: activeGroup?.id + }) } catch (error) { toast.error(extractIpcErrorMessage(error, 'Failed to open markdown file.')) } diff --git a/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts index 7aa5dd418cf..d105db1d3e9 100644 --- a/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts @@ -12,6 +12,7 @@ import { createDirectSshBridgeRuntime } from './direct-ssh-bridge-runtime' import { registerDirectSshStateIpcBridge } from './direct-ssh-state-ipc-bridge' import { registerMobileAndTerminalCloseIpcBridge } from './mobile-terminal-close-ipc-bridge' import { registerMobileDriverIpcBridge } from './mobile-driver-ipc-bridge' +import { registerOsMarkdownFileOpenBridge } from './os-markdown-file-open-bridge' import { registerProjectCatalogIpcBridge } from './project-catalog-ipc-bridge' import { registerRateLimitIpcBridge } from './rate-limit-ipc-bridge' import { registerRemoteWorkspaceIpcBridge } from './remote-workspace-ipc-bridge' @@ -77,6 +78,7 @@ export function installAppLifetimeIpcEvents( ) registerSettingsAndSidebarIpcBridge(unsubs) registerWorkspaceShortcutIpcBridge(unsubs) + registerOsMarkdownFileOpenBridge(unsubs) unsubs.push( window.api.ui.onActivateWorktree(({ repoId, worktreeId, setup, startup, defaultTabs }) => { void worktreeRuntime diff --git a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts new file mode 100644 index 00000000000..d4825e89206 --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts @@ -0,0 +1,300 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { TOGGLE_FLOATING_TERMINAL_EVENT } from '@/lib/floating-terminal' +import type { EditorFilesSlice } from '@/store/slices/editor/types/editor-files-slice' +import type { MarkdownDocument } from '../../../../shared/filesystem-entry-types' +import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' +import { registerOsMarkdownFileOpenBridge } from './os-markdown-file-open-bridge' + +const mocks = vi.hoisted(() => ({ + openFile: vi.fn(() => 'file-1'), + updateSettings: vi.fn(async () => {}), + isFloatingWorkspacePanelVisible: vi.fn(() => false), + toastError: vi.fn() +})) + +let storeState: { + openFile: typeof mocks.openFile + updateSettings: typeof mocks.updateSettings + settings: { floatingTerminalEnabled?: boolean } | undefined +} + +vi.mock('../../store', () => ({ useAppStore: { getState: () => storeState } })) +vi.mock('@/lib/floating-workspace-terminal-actions', () => ({ + isFloatingWorkspacePanelVisible: mocks.isFloatingWorkspacePanelVisible +})) +vi.mock('sonner', () => ({ toast: { error: mocks.toastError } })) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) + +type MarkdownFileOpenListener = (documents: MarkdownDocument[]) => void + +let frames: FrameRequestCallback[] = [] +let dispatchEvent = vi.fn() +let unhandledRejections: unknown[] = [] +const recordUnhandledRejection = (reason: unknown): void => void unhandledRejections.push(reason) + +function markdownDocument(overrides: Partial = {}): MarkdownDocument { + return { + filePath: '/Users/me/notes/README.md', + relativePath: 'README.md', + basename: 'README.md', + name: 'README', + ...overrides + } +} + +function stubPreload(ui: Record): void { + dispatchEvent = vi.fn() + vi.stubGlobal('window', { api: { ui }, dispatchEvent }) +} + +/** Runs the callbacks the bridge deferred to the next frame. */ +function runFrames(): void { + const pending = frames + frames = [] + for (const frame of pending) { + frame(0) + } +} + +/** Drains microtasks and lets Node emit any unhandled rejection the bridge leaked. */ +async function settle(): Promise { + await new Promise((resolve) => setImmediate(resolve)) + await new Promise((resolve) => setImmediate(resolve)) +} + +describe('registerOsMarkdownFileOpenBridge', () => { + beforeEach(() => { + vi.clearAllMocks() + frames = [] + unhandledRejections = [] + storeState = { + openFile: mocks.openFile, + updateSettings: mocks.updateSettings, + settings: { floatingTerminalEnabled: true } + } + mocks.openFile.mockReturnValue('file-1') + mocks.updateSettings.mockResolvedValue(undefined) + mocks.isFloatingWorkspacePanelVisible.mockReturnValue(false) + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => + frames.push(callback) + ) + vi.spyOn(console, 'error').mockImplementation(() => {}) + process.on('unhandledRejection', recordUnhandledRejection) + }) + + afterEach(() => { + process.off('unhandledRejection', recordUnhandledRejection) + vi.unstubAllGlobals() + vi.restoreAllMocks() + }) + + it('opens every document main queued before the listener attached', async () => { + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => + Promise.resolve([ + markdownDocument(), + markdownDocument({ filePath: '/Users/me/notes/plan.md', relativePath: 'plan.md' }) + ]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + + expect(mocks.openFile).toHaveBeenCalledTimes(2) + expect(mocks.openFile.mock.calls.map((call) => call[0].filePath)).toEqual([ + '/Users/me/notes/README.md', + '/Users/me/notes/plan.md' + ]) + expect(mocks.openFile.mock.calls[0][0].worktreeId).toBe(FLOATING_TERMINAL_WORKTREE_ID) + }) + + it('opens documents pushed after startup and hands back the unsubscribe', async () => { + const listeners: MarkdownFileOpenListener[] = [] + const unsubscribe = vi.fn() + stubPreload({ + onOpenMarkdownFiles: (next: MarkdownFileOpenListener) => { + listeners.push(next) + return unsubscribe + }, + consumePendingMarkdownFileOpens: () => Promise.resolve([]) + }) + + const unsubs: (() => void)[] = [] + registerOsMarkdownFileOpenBridge(unsubs) + expect(unsubs).toEqual([unsubscribe]) + + listeners[0]([ + markdownDocument({ filePath: '/Users/me/notes/live.md', relativePath: 'live.md' }) + ]) + await settle() + + expect(mocks.openFile).toHaveBeenCalledTimes(1) + expect(mocks.openFile.mock.calls[0][0].filePath).toBe('/Users/me/notes/live.md') + + unsubs.forEach((teardown) => teardown()) + expect(unsubscribe).toHaveBeenCalledOnce() + }) + + it('enables the floating workspace when the setting is off', async () => { + storeState.settings = { floatingTerminalEnabled: false } + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.resolve([markdownDocument()]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + + expect(mocks.updateSettings).toHaveBeenCalledWith({ floatingTerminalEnabled: true }) + }) + + it('leaves settings alone when the floating workspace is already enabled', async () => { + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.resolve([markdownDocument()]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + + expect(mocks.updateSettings).not.toHaveBeenCalled() + }) + + it('defers the reveal a frame and toggles only while the panel is hidden', async () => { + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.resolve([markdownDocument()]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + + expect(dispatchEvent).not.toHaveBeenCalled() + runFrames() + + expect(dispatchEvent).toHaveBeenCalledTimes(1) + expect(dispatchEvent.mock.calls[0][0].type).toBe(TOGGLE_FLOATING_TERMINAL_EVENT) + }) + + it('does not toggle when the panel is already visible', async () => { + mocks.isFloatingWorkspacePanelVisible.mockReturnValue(true) + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.resolve([markdownDocument()]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + runFrames() + + expect(dispatchEvent).not.toHaveBeenCalled() + }) + + it('ignores an empty batch', async () => { + storeState.settings = { floatingTerminalEnabled: false } + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.resolve([]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + runFrames() + + expect(mocks.openFile).not.toHaveBeenCalled() + expect(mocks.updateSettings).not.toHaveBeenCalled() + expect(dispatchEvent).not.toHaveBeenCalled() + }) + + it('reports a rejected pending drain without leaking an unhandled rejection', async () => { + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => Promise.reject(new Error('ipc unavailable')) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + + expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the Markdown file.') + // Why: App.tsx awaits hydration around this registration and treats any throw as + // "session restore failed", so the bridge must swallow its own failures. + expect(unhandledRejections).toEqual([]) + }) + + it('reports a throwing openFile without leaking an unhandled rejection', async () => { + mocks.openFile.mockImplementation(() => { + throw new Error('editor slice exploded') + }) + const listeners: MarkdownFileOpenListener[] = [] + stubPreload({ + onOpenMarkdownFiles: (next: MarkdownFileOpenListener) => { + listeners.push(next) + return () => {} + }, + consumePendingMarkdownFileOpens: () => Promise.resolve([]) + }) + + registerOsMarkdownFileOpenBridge([]) + expect(() => listeners[0]([markdownDocument()])).not.toThrow() + await settle() + + expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the Markdown file.') + expect(unhandledRejections).toEqual([]) + expect(dispatchEvent).not.toHaveBeenCalled() + }) + + it('keeps opening the rest of a batch when one document fails', async () => { + mocks.openFile.mockImplementationOnce(() => { + throw new Error('first document exploded') + }) + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + consumePendingMarkdownFileOpens: () => + Promise.resolve([ + markdownDocument({ filePath: '/Users/me/notes/bad.md', relativePath: 'bad.md' }), + markdownDocument({ filePath: '/Users/me/notes/good.md', relativePath: 'good.md' }) + ]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + runFrames() + + // Why: a multi-file selection arrives as one batch; one bad file must not cost the rest. + expect(mocks.openFile).toHaveBeenCalledTimes(2) + expect(mocks.toastError).toHaveBeenCalledTimes(1) + expect(dispatchEvent).toHaveBeenCalledTimes(1) + expect(unhandledRejections).toEqual([]) + }) + + it('ignores a non-array payload from a mismatched preload', async () => { + stubPreload({ + onOpenMarkdownFiles: () => () => {}, + // Why: the payload crosses the preload boundary, so a stale preload can resolve with + // something that is not an array. Reading .length off it would throw inside the chain. + consumePendingMarkdownFileOpens: () => Promise.resolve(null as unknown as MarkdownDocument[]) + }) + + registerOsMarkdownFileOpenBridge([]) + await settle() + runFrames() + + expect(mocks.openFile).not.toHaveBeenCalled() + expect(mocks.updateSettings).not.toHaveBeenCalled() + expect(mocks.toastError).not.toHaveBeenCalled() + expect(unhandledRejections).toEqual([]) + }) + + it('tolerates a preload without the markdown open channel', async () => { + stubPreload({}) + + const unsubs: (() => void)[] = [] + expect(() => registerOsMarkdownFileOpenBridge(unsubs)).not.toThrow() + await settle() + + expect(unsubs).toEqual([]) + expect(mocks.openFile).not.toHaveBeenCalled() + expect(mocks.toastError).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts new file mode 100644 index 00000000000..3dd345da07c --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts @@ -0,0 +1,72 @@ +import { toast } from 'sonner' +import type { MarkdownDocument } from '../../../../shared/filesystem-entry-types' +import { TOGGLE_FLOATING_TERMINAL_EVENT } from '@/lib/floating-terminal' +import { isFloatingWorkspacePanelVisible } from '@/lib/floating-workspace-terminal-actions' +import { openMarkdownDocumentInFloatingWorkspace } from '@/lib/open-markdown-in-floating-workspace' +import { translate } from '@/i18n/i18n' +import { useAppStore } from '../../store' + +/** + * Opens markdown files the OS shell handed to Orca ("Open With" / double-click) in the + * floating workspace, which is the one editor surface that needs no project. + */ +async function openOsRequestedMarkdownFiles(documents: MarkdownDocument[]): Promise { + // Why the shape check: this payload crosses the preload boundary, so a stale or mismatched + // preload can hand back something that is not an array. Reading .length off that throws + // inside the promise chain rather than failing loudly at the boundary. + if (!Array.isArray(documents) || documents.length === 0) { + return + } + const store = useAppStore.getState() + let opened = 0 + for (const document of documents) { + // Why isolated: selecting several files hands us one batch, and one unopenable file + // must not cost the user the rest of the selection. + try { + openMarkdownDocumentInFloatingWorkspace(store.openFile, document) + opened += 1 + } catch (error) { + reportOsRequestedMarkdownFailure(error) + } + } + if (opened === 0) { + return + } + // Why enabled here: the user asked the OS for this file, and the tabs above are already in a + // surface a disabled floating workspace never renders. Same enable-then-reveal as the + // Settings "Edit keybindings in Orca" action. + if (store.settings?.floatingTerminalEnabled !== true) { + await store.updateSettings({ floatingTerminalEnabled: true }) + } + // Why deferred a frame: the panel only honors the toggle once the enabled flag has reached React. + requestAnimationFrame(() => { + if (!isFloatingWorkspacePanelVisible()) { + window.dispatchEvent(new CustomEvent(TOGGLE_FLOATING_TERMINAL_EVENT)) + } + }) +} + +function reportOsRequestedMarkdownFailure(error: unknown): void { + console.error('Failed to open markdown files requested by the OS:', error) + toast.error( + translate( + 'auto.hooks.ipc.events.os.markdown.file.open.bridge.1e9a1a63c4', + 'Failed to open the Markdown file.' + ) + ) +} + +export function registerOsMarkdownFileOpenBridge(unsubs: (() => void)[]): void { + const unsubscribe = window.api.ui.onOpenMarkdownFiles?.((documents) => { + void openOsRequestedMarkdownFiles(documents).catch(reportOsRequestedMarkdownFailure) + }) + if (unsubscribe) { + unsubs.push(unsubscribe) + } + + // Why: a cold-start "Open With" resolves before this listener attaches; drain what main queued. + const pending = window.api.ui.consumePendingMarkdownFileOpens?.() + if (pending && typeof pending.then === 'function') { + void pending.then(openOsRequestedMarkdownFiles).catch(reportOsRequestedMarkdownFailure) + } +} diff --git a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts index 5155aa6881f..67872949590 100644 --- a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts +++ b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts @@ -55,6 +55,7 @@ const EXPECTED_DIRECT_CALLBACK_METHODS = [ 'ui.onOpenDiffFromMobile', 'ui.onOpenFeatureTour', 'ui.onOpenFileFromMobile', + 'ui.onOpenMarkdownFiles', 'ui.onOpenNewWorkspace', 'ui.onOpenQuickOpen', 'ui.onOpenSettings', @@ -135,6 +136,7 @@ const EXPECTED_CALLBACK_REGISTRATION_SEQUENCE = [ 'ui.onJumpToTabIndex', 'ui.onWorktreeHistoryNavigate', 'ui.onToggleStatusBar', + 'ui.onOpenMarkdownFiles', 'ui.onActivateWorktree', 'ui.onCreateTerminal', 'ui.onRequestTerminalTabMount', diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index c04b2f260e9..7fc9c837ebc 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -1112,6 +1112,17 @@ "events": { "browserStateIpcBridge": { "docPreviewLinkFailed": "Could not open this link in Orca Browser." + }, + "os": { + "markdown": { + "file": { + "open": { + "bridge": { + "1e9a1a63c4": "Failed to open the Markdown file." + } + } + } + } } } } diff --git a/src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts b/src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts new file mode 100644 index 00000000000..62fc278fc7a --- /dev/null +++ b/src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it, vi } from 'vitest' +import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' +import type { MarkdownDocument } from '../../../shared/filesystem-entry-types' +import type { EditorFilesSlice } from '@/store/slices/editor/types/editor-files-slice' +import { openMarkdownDocumentInFloatingWorkspace } from './open-markdown-in-floating-workspace' + +function openFileMock(): ReturnType> { + return vi.fn(() => 'file-1') +} + +function markdownDocument(overrides: Partial = {}): MarkdownDocument { + return { + filePath: '/Users/me/notes/README.md', + relativePath: 'README.md', + basename: 'README.md', + name: 'README', + ...overrides + } +} + +describe('openMarkdownDocumentInFloatingWorkspace', () => { + it('opens the document as a permanent floating-workspace edit tab', () => { + const openFile = openFileMock() + + const fileId = openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument()) + + expect(fileId).toBe('file-1') + expect(openFile).toHaveBeenCalledTimes(1) + expect(openFile.mock.calls[0][0]).toEqual({ + filePath: '/Users/me/notes/README.md', + relativePath: 'README.md', + worktreeId: FLOATING_TERMINAL_WORKTREE_ID, + language: 'markdown', + mode: 'edit', + runtimeEnvironmentId: null + }) + expect(openFile.mock.calls[0][1]).toEqual({ + preview: false, + targetGroupId: undefined, + suppressActiveRuntimeFallback: true + }) + }) + + it('pins the open to this machine instead of the active runtime', () => { + const openFile = openFileMock() + + openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument()) + + // Why: the caller already resolved an absolute local path, so a null runtime plus the + // fallback suppression is what keeps the read off a remote SSH host the user is focused on. + // Dropping either one silently reads the file on the wrong machine. + expect(openFile.mock.calls[0][0].runtimeEnvironmentId).toBeNull() + expect(openFile.mock.calls[0][1]?.suppressActiveRuntimeFallback).toBe(true) + }) + + it('derives the language from the relative path', () => { + const openFile = openFileMock() + + openMarkdownDocumentInFloatingWorkspace( + openFile, + markdownDocument({ + filePath: '/Users/me/notes/plan.mdx', + relativePath: 'plan.mdx', + basename: 'plan.mdx', + name: 'plan' + }) + ) + + expect(openFile.mock.calls[0][0].language).toBe('markdown') + }) + + it('forwards a requested target group', () => { + const openFile = openFileMock() + + openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument(), { + targetGroupId: 'group-2' + }) + + expect(openFile.mock.calls[0][1]?.targetGroupId).toBe('group-2') + }) +}) diff --git a/src/renderer/src/lib/open-markdown-in-floating-workspace.ts b/src/renderer/src/lib/open-markdown-in-floating-workspace.ts new file mode 100644 index 00000000000..3b1bd4cd08b --- /dev/null +++ b/src/renderer/src/lib/open-markdown-in-floating-workspace.ts @@ -0,0 +1,32 @@ +import type { MarkdownDocument } from '../../../shared/filesystem-entry-types' +import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' +import type { EditorFilesSlice } from '@/store/slices/editor/types/editor-files-slice' +import { detectLanguage } from './language-detect' + +/** + * Opens a markdown file that belongs to no workspace as a floating-workspace editor tab. + * + * Why local-only: every caller resolves an absolute path on this machine (a native picker or + * the OS shell), so routing it through the active runtime would read it on the wrong host. + */ +export function openMarkdownDocumentInFloatingWorkspace( + openFile: EditorFilesSlice['openFile'], + document: MarkdownDocument, + options: { targetGroupId?: string } = {} +): string { + return openFile( + { + filePath: document.filePath, + relativePath: document.relativePath, + worktreeId: FLOATING_TERMINAL_WORKTREE_ID, + language: detectLanguage(document.relativePath), + mode: 'edit', + runtimeEnvironmentId: null + }, + { + preview: false, + targetGroupId: options.targetGroupId, + suppressActiveRuntimeFallback: true + } + ) +} diff --git a/src/renderer/src/web/preload-api/web-ui-api.ts b/src/renderer/src/web/preload-api/web-ui-api.ts index 8c6e4d73a95..ca67f5664a9 100644 --- a/src/renderer/src/web/preload-api/web-ui-api.ts +++ b/src/renderer/src/web/preload-api/web-ui-api.ts @@ -157,6 +157,9 @@ export function createWebUiApi(): NonNullable['ui']> { consumePendingOpenSettings: () => Promise.resolve(false), onOpenSkillShare: () => noopUnsubscribe, consumePendingSkillShare: () => Promise.resolve(null), + // Why: the web client has no OS shell handing it files, so there is never a queued open. + onOpenMarkdownFiles: () => noopUnsubscribe, + consumePendingMarkdownFileOpens: () => Promise.resolve([]), onOpenSetupGuide: () => noopUnsubscribe, onOpenFeatureTour: () => noopUnsubscribe, onOpenCrashReport: () => noopUnsubscribe,