From 974447175f6628e212b3323ea92fe5c56f812df7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:00:50 -0700 Subject: [PATCH 01/16] feat(terminal): add manual parking developer action (#11016) --- src/renderer/src/components/Terminal.tsx | 28 ++++-- .../sidebar/WorktreeContextMenu.tsx | 7 ++ .../sidebar/WorktreeDeveloperMenu.test.tsx | 58 +++++++++++ .../sidebar/WorktreeDeveloperMenu.tsx | 32 ++++++ .../terminal-hidden-view-parking.test.ts | 31 ++++++ .../terminal-hidden-view-parking.ts | 20 ++++ ...e-manual-terminal-worktree-parking.test.ts | 16 +++ .../use-manual-terminal-worktree-parking.ts | 99 +++++++++++++++++++ src/renderer/src/i18n/locales/en.json | 9 ++ src/renderer/src/i18n/locales/es.json | 9 ++ src/renderer/src/i18n/locales/ja.json | 9 ++ src/renderer/src/i18n/locales/ko.json | 9 ++ src/renderer/src/i18n/locales/zh.json | 9 ++ .../manual-terminal-worktree-parking.test.ts | 42 ++++++++ .../lib/manual-terminal-worktree-parking.ts | 29 ++++++ 15 files changed, 398 insertions(+), 9 deletions(-) create mode 100644 src/renderer/src/components/sidebar/WorktreeDeveloperMenu.test.tsx create mode 100644 src/renderer/src/components/sidebar/WorktreeDeveloperMenu.tsx create mode 100644 src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.test.ts create mode 100644 src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.ts create mode 100644 src/renderer/src/lib/manual-terminal-worktree-parking.test.ts create mode 100644 src/renderer/src/lib/manual-terminal-worktree-parking.ts diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 0c2982043ff..e730d4328ff 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -137,6 +137,10 @@ import { translate } from '@/i18n/i18n' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { getResolvedExecutionHostIdForWorktree } from '@/lib/resolved-worktree-execution-host' import { browserWorkspaceHasRemoteOwner } from '@/runtime/remote-browser-tab-ownership' +import { + combineTerminalWorktreeParkIds, + useManualTerminalWorktreeParking +} from './terminal-pane/use-manual-terminal-worktree-parking' const EditorPanel = lazy(() => import('./editor/EditorPanel')) @@ -738,6 +742,15 @@ function Terminal(): React.JSX.Element | null { const [parkedTerminalWorktreeIds, setParkedTerminalWorktreeIds] = useState>( () => new Set() ) + const manuallyParkedTerminalWorktreeIds = useManualTerminalWorktreeParking({ + activeView, + renderedActiveWorktreeId + }) + const effectiveParkedTerminalWorktreeIds = useMemo( + () => + combineTerminalWorktreeParkIds(parkedTerminalWorktreeIds, manuallyParkedTerminalWorktreeIds), + [manuallyParkedTerminalWorktreeIds, parkedTerminalWorktreeIds] + ) // Tab restriction for targeted background mounts (wake/resume); a worktree absent from this map mounts all its tabs. const backgroundMountTabIdsByWorktreeRef = useRef(new Map>()) // Why: only cold-activation deferral (not targeted mounts, which share the map above) creates watcher coverage for every unmounted tab. @@ -1055,9 +1068,9 @@ function Terminal(): React.JSX.Element | null { const coldWorktreePresentationTargets = useMemo( () => activeView === 'terminal' && activeTabType === 'terminal' - ? parkedTerminalWorktreeIds + ? effectiveParkedTerminalWorktreeIds : new Set(), - [activeTabType, activeView, parkedTerminalWorktreeIds] + [activeTabType, activeView, effectiveParkedTerminalWorktreeIds] ) const { presentationByScope: worktreePresentationByScope, @@ -1088,7 +1101,7 @@ function Terminal(): React.JSX.Element | null { const shouldMeasureHiddenWorktree = !isVisible && measurableBackgroundWorktreeIdsRef.current.has(workspace.id) const parked = - !isVisible && !shouldMeasureHiddenWorktree && parkedTerminalWorktreeIds.has(workspace.id) + !shouldMeasureHiddenWorktree && effectiveParkedTerminalWorktreeIds.has(workspace.id) if (parked) { for (const tab of tabs) { const activityTerminalPortal = findActivityTerminalPortal(activityTerminalPortals, { @@ -1135,7 +1148,7 @@ function Terminal(): React.JSX.Element | null { backgroundMountRevision, getEffectiveLayoutForWorktree, groupsByWorktree, - parkedTerminalWorktreeIds, + effectiveParkedTerminalWorktreeIds, pendingStartupByTabId, renderedActiveWorktreeId, tabsByWorktree, @@ -2107,9 +2120,7 @@ function Terminal(): React.JSX.Element | null { const shouldMeasureHiddenWorktree = !isVisible && measurableBackgroundWorktreeIdsRef.current.has(workspace.id) const shouldColdParkTerminalPanes = - !isVisible && - !shouldMeasureHiddenWorktree && - parkedTerminalWorktreeIds.has(workspace.id) + !shouldMeasureHiddenWorktree && effectiveParkedTerminalWorktreeIds.has(workspace.id) return ( ) : null} + {!isMultiContext ? ( + <> + + + + ) : null} void +} + +const menuItems = vi.hoisted(() => ({ values: [] as MenuItemProps[] })) +const requestManualPark = vi.hoisted(() => vi.fn()) + +vi.mock('@/components/ui/dropdown-menu', async () => { + const React_ = await import('react') + const passthrough = ({ children }: { children?: ReactNode }) => + React_.createElement(React_.Fragment, null, children) + return { + DropdownMenuSub: passthrough, + DropdownMenuSubContent: passthrough, + DropdownMenuSubTrigger: passthrough, + DropdownMenuItem: (props: MenuItemProps) => { + menuItems.values.push(props) + return React_.createElement(React_.Fragment, null, props.children) + } + } +}) + +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('@/lib/manual-terminal-worktree-parking', () => ({ + requestManualTerminalWorktreePark: requestManualPark +})) + +describe('WorktreeDeveloperMenu', () => { + beforeEach(() => { + menuItems.values = [] + requestManualPark.mockReset() + }) + + it('shows the developer submenu with the parking action', () => { + const markup = renderToStaticMarkup( + + ) + + expect(markup).toContain('Developer') + expect(markup).toContain('Park terminal') + expect(markup).toContain('lucide-code-xml') + expect(markup).toContain('lucide-square-parking') + }) + + it('requests parking for the context worktree', () => { + renderToStaticMarkup() + + menuItems.values[0]?.onSelect?.() + + expect(requestManualPark).toHaveBeenCalledWith('worktree-1') + }) +}) diff --git a/src/renderer/src/components/sidebar/WorktreeDeveloperMenu.tsx b/src/renderer/src/components/sidebar/WorktreeDeveloperMenu.tsx new file mode 100644 index 00000000000..a0cb257c105 --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeDeveloperMenu.tsx @@ -0,0 +1,32 @@ +import { + DropdownMenuItem, + DropdownMenuSub, + DropdownMenuSubContent, + DropdownMenuSubTrigger +} from '@/components/ui/dropdown-menu' +import { CodeXml, SquareParking } from 'lucide-react' +import { translate } from '@/i18n/i18n' +import { requestManualTerminalWorktreePark } from '@/lib/manual-terminal-worktree-parking' + +export function WorktreeDeveloperMenu({ + worktreeId, + disabled +}: { + worktreeId: string + disabled: boolean +}): React.JSX.Element { + return ( + + + + {translate('auto.components.sidebar.WorktreeDeveloperMenu.developer', 'Developer')} + + + requestManualTerminalWorktreePark(worktreeId)}> + + {translate('auto.components.sidebar.WorktreeDeveloperMenu.parkTerminal', 'Park terminal')} + + + + ) +} diff --git a/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.test.ts b/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.test.ts index 5c8026b74d0..cb1c698fc15 100644 --- a/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.test.ts @@ -7,6 +7,7 @@ import { TERMINAL_TAB_HOT_RETAIN_MS, TERMINAL_WORKTREE_HOT_RETAIN_MS, TERMINAL_WORKTREE_PARK_DELAY_MS, + canManuallyParkTerminalWorktreeRenderers, canParkTerminalTabRenderer, canParkTerminalWorktreeRenderers, getTerminalTabColdParkRecheckDelayMs, @@ -181,6 +182,36 @@ describe('canParkTerminalTabRenderer', () => { }) }) +describe('canManuallyParkTerminalWorktreeRenderers', () => { + const base = { + worktreeId: 'repo::/worktree', + terminalTabs: [{ id: 'tab-1', ptyId: 'repo::/worktree@@session-1' }], + pendingStartupByTabId: {}, + parkingEnabled: true + } + + it('bypasses visibility timing for snapshot-backed local terminals', () => { + expect(canManuallyParkTerminalWorktreeRenderers(base)).toBe(true) + }) + + it('preserves safety gates for empty, pending, remote, and disabled terminals', () => { + expect(canManuallyParkTerminalWorktreeRenderers({ ...base, terminalTabs: [] })).toBe(false) + expect( + canManuallyParkTerminalWorktreeRenderers({ + ...base, + pendingStartupByTabId: { 'tab-1': ['echo', 'pending'] } + }) + ).toBe(false) + expect( + canManuallyParkTerminalWorktreeRenderers({ + ...base, + terminalTabs: [{ id: 'tab-1', ptyId: 'ssh:ssh-1@@pty-1' }] + }) + ).toBe(false) + expect(canManuallyParkTerminalWorktreeRenderers({ ...base, parkingEnabled: false })).toBe(false) + }) +}) + describe('selectColdParkedTerminalWorktrees', () => { const nowMs = 500_000 diff --git a/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.ts b/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.ts index f4b434c948b..4701ff727a3 100644 --- a/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.ts +++ b/src/renderer/src/components/terminal-pane/terminal-hidden-view-parking.ts @@ -107,6 +107,26 @@ export function canParkTerminalWorktreeRenderers(args: { }) } +export function canManuallyParkTerminalWorktreeRenderers(args: { + worktreeId: string + terminalTabs: readonly ColdParkableTerminalTab[] + pendingStartupByTabId: Readonly> + parkingEnabled: boolean +}): boolean { + return ( + args.terminalTabs.length > 0 && + canParkTerminalWorktreeRenderers({ + ...args, + isVisible: false, + shouldMeasureHiddenWorktree: false, + hasActivityTerminalPortal: false, + hiddenSinceMs: 0, + nowMs: 0, + coldParkDelayMs: 0 + }) + ) +} + export function canParkTerminalTabRenderer(args: { worktreeId: string terminalTab: TerminalTabColdParkCandidate diff --git a/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.test.ts b/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.test.ts new file mode 100644 index 00000000000..d3d9b44d544 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from 'vitest' +import { combineTerminalWorktreeParkIds } from './use-manual-terminal-worktree-parking' + +describe('combineTerminalWorktreeParkIds', () => { + it('adds manually parked worktrees without dropping automatic parks', () => { + expect(combineTerminalWorktreeParkIds(new Set(['automatic']), new Set(['manual']))).toEqual( + new Set(['automatic', 'manual']) + ) + }) + + it('keeps the automatic set reference when there are no manual parks', () => { + const automatic = new Set(['automatic']) + + expect(combineTerminalWorktreeParkIds(automatic, new Set())).toBe(automatic) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.ts b/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.ts new file mode 100644 index 00000000000..2d1ed09afce --- /dev/null +++ b/src/renderer/src/components/terminal-pane/use-manual-terminal-worktree-parking.ts @@ -0,0 +1,99 @@ +import { useCallback, useEffect, useState } from 'react' +import { toast } from 'sonner' +import { useAppStore } from '@/store' +import { translate } from '@/i18n/i18n' +import { + MANUAL_TERMINAL_WORKTREE_PARK_EVENT, + takeAllPendingManualTerminalWorktreeParks, + takePendingManualTerminalWorktreePark, + type ManualTerminalWorktreeParkDetail +} from '@/lib/manual-terminal-worktree-parking' +import { canManuallyParkTerminalWorktreeRenderers } from './terminal-hidden-view-parking' +import { canWatcherCoverParkedTerminalTab } from './terminal-parked-tab-watchers' + +function addWorktreeId(current: ReadonlySet, worktreeId: string): ReadonlySet { + if (current.has(worktreeId)) { + return current + } + return new Set([...current, worktreeId]) +} + +function removeWorktreeId(current: ReadonlySet, worktreeId: string): ReadonlySet { + if (!current.has(worktreeId)) { + return current + } + const next = new Set(current) + next.delete(worktreeId) + return next +} + +export function combineTerminalWorktreeParkIds( + automaticIds: ReadonlySet, + manualIds: ReadonlySet +): ReadonlySet { + if (manualIds.size === 0) { + return automaticIds + } + return new Set([...automaticIds, ...manualIds]) +} + +export function useManualTerminalWorktreeParking(args: { + activeView: string + renderedActiveWorktreeId: string | null +}): ReadonlySet { + const [manuallyParkedWorktreeIds, setManuallyParkedWorktreeIds] = useState>( + () => new Set() + ) + + const parkWorktree = useCallback((worktreeId: string) => { + const state = useAppStore.getState() + const terminalTabs = state.tabsByWorktree[worktreeId] ?? [] + const canPark = + canManuallyParkTerminalWorktreeRenderers({ + worktreeId, + terminalTabs, + pendingStartupByTabId: state.pendingStartupByTabId, + parkingEnabled: state.settings?.terminalHiddenViewParking !== false + }) && terminalTabs.every((tab) => canWatcherCoverParkedTerminalTab(worktreeId, tab)) + if (!canPark) { + toast.warning( + translate( + 'auto.components.terminalPane.useManualTerminalWorktreeParking.cannotPark', + 'These terminals cannot be parked safely.' + ) + ) + return + } + setManuallyParkedWorktreeIds((current) => addWorktreeId(current, worktreeId)) + }, []) + + useEffect(() => { + const handleParkRequest = (event: Event): void => { + const worktreeId = (event as CustomEvent).detail?.worktreeId + if (!worktreeId) { + return + } + takePendingManualTerminalWorktreePark(worktreeId) + parkWorktree(worktreeId) + } + window.addEventListener(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, handleParkRequest as EventListener) + for (const worktreeId of takeAllPendingManualTerminalWorktreeParks()) { + parkWorktree(worktreeId) + } + return () => + window.removeEventListener( + MANUAL_TERMINAL_WORKTREE_PARK_EVENT, + handleParkRequest as EventListener + ) + }, [parkWorktree]) + + useEffect(() => { + const revealedWorktreeId = args.renderedActiveWorktreeId + if (args.activeView !== 'terminal' || !revealedWorktreeId) { + return + } + setManuallyParkedWorktreeIds((current) => removeWorktreeId(current, revealedWorktreeId)) + }, [args.activeView, args.renderedActiveWorktreeId]) + + return manuallyParkedWorktreeIds +} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 01a53e11044..ff9f7702a09 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4939,6 +4939,10 @@ "close": "Close", "image": "Image", "expand": "Expand image" + }, + "WorktreeDeveloperMenu": { + "developer": "Developer", + "parkTerminal": "Park terminal" } }, "shared": { @@ -13963,6 +13967,11 @@ "undo": "Undo", "widthOption": "{{value0}} px" } + }, + "terminalPane": { + "useManualTerminalWorktreeParking": { + "cannotPark": "These terminals cannot be parked safely." + } } }, "i18n": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index df89084b3bc..a9c4997a5d5 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -4916,6 +4916,10 @@ "close": "Cerrar", "image": "Imagen", "expand": "Ampliar imagen" + }, + "WorktreeDeveloperMenu": { + "developer": "Developer", + "parkTerminal": "Park terminal" } }, "shared": { @@ -13940,6 +13944,11 @@ "undo": "Deshacer", "widthOption": "{{value0}} px" } + }, + "terminalPane": { + "useManualTerminalWorktreeParking": { + "cannotPark": "These terminals cannot be parked safely." + } } }, "i18n": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 62fd5330a68..45a9e137069 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -4916,6 +4916,10 @@ "close": "閉じる", "image": "画像", "expand": "画像を拡大" + }, + "WorktreeDeveloperMenu": { + "developer": "Developer", + "parkTerminal": "Park terminal" } }, "shared": { @@ -13940,6 +13944,11 @@ "undo": "元に戻す", "widthOption": "{{value0}} px" } + }, + "terminalPane": { + "useManualTerminalWorktreeParking": { + "cannotPark": "These terminals cannot be parked safely." + } } }, "i18n": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index df331ec2e3c..1ba309520a2 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -4916,6 +4916,10 @@ "close": "닫기", "image": "이미지", "expand": "이미지 확대" + }, + "WorktreeDeveloperMenu": { + "developer": "Developer", + "parkTerminal": "Park terminal" } }, "shared": { @@ -13940,6 +13944,11 @@ "undo": "실행 취소", "widthOption": "{{value0}} px" } + }, + "terminalPane": { + "useManualTerminalWorktreeParking": { + "cannotPark": "These terminals cannot be parked safely." + } } }, "i18n": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index a6075fd7fb2..3d1a8f12598 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -4916,6 +4916,10 @@ "close": "关闭", "image": "图像", "expand": "展开图像" + }, + "WorktreeDeveloperMenu": { + "developer": "Developer", + "parkTerminal": "Park terminal" } }, "shared": { @@ -13940,6 +13944,11 @@ "undo": "撤销", "widthOption": "{{value0}} px" } + }, + "terminalPane": { + "useManualTerminalWorktreeParking": { + "cannotPark": "These terminals cannot be parked safely." + } } }, "i18n": { diff --git a/src/renderer/src/lib/manual-terminal-worktree-parking.test.ts b/src/renderer/src/lib/manual-terminal-worktree-parking.test.ts new file mode 100644 index 00000000000..e70ca180b01 --- /dev/null +++ b/src/renderer/src/lib/manual-terminal-worktree-parking.test.ts @@ -0,0 +1,42 @@ +// @vitest-environment happy-dom + +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + MANUAL_TERMINAL_WORKTREE_PARK_EVENT, + requestManualTerminalWorktreePark, + takeAllPendingManualTerminalWorktreeParks, + takePendingManualTerminalWorktreePark, + type ManualTerminalWorktreeParkDetail +} from './manual-terminal-worktree-parking' + +describe('manual terminal worktree parking requests', () => { + beforeEach(() => { + takeAllPendingManualTerminalWorktreeParks() + }) + + it('dispatches the target worktree and records it for a late Terminal mount', () => { + const listener = vi.fn((event: Event) => { + expect((event as CustomEvent).detail).toEqual({ + worktreeId: 'worktree-1' + }) + }) + window.addEventListener(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, listener) + + requestManualTerminalWorktreePark('worktree-1') + + expect(listener).toHaveBeenCalledOnce() + expect(takePendingManualTerminalWorktreePark('worktree-1')).toBe(true) + window.removeEventListener(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, listener) + }) + + it('ignores an empty worktree id', () => { + const listener = vi.fn() + window.addEventListener(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, listener) + + requestManualTerminalWorktreePark('') + + expect(listener).not.toHaveBeenCalled() + expect(takeAllPendingManualTerminalWorktreeParks()).toEqual([]) + window.removeEventListener(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, listener) + }) +}) diff --git a/src/renderer/src/lib/manual-terminal-worktree-parking.ts b/src/renderer/src/lib/manual-terminal-worktree-parking.ts new file mode 100644 index 00000000000..19113da5767 --- /dev/null +++ b/src/renderer/src/lib/manual-terminal-worktree-parking.ts @@ -0,0 +1,29 @@ +export const MANUAL_TERMINAL_WORKTREE_PARK_EVENT = 'orca-manual-terminal-worktree-park' + +export type ManualTerminalWorktreeParkDetail = { + worktreeId: string +} + +const pendingWorktreeIds = new Set() + +export function requestManualTerminalWorktreePark(worktreeId: string): void { + if (!worktreeId) { + return + } + pendingWorktreeIds.add(worktreeId) + window.dispatchEvent( + new CustomEvent(MANUAL_TERMINAL_WORKTREE_PARK_EVENT, { + detail: { worktreeId } + }) + ) +} + +export function takePendingManualTerminalWorktreePark(worktreeId: string): boolean { + return pendingWorktreeIds.delete(worktreeId) +} + +export function takeAllPendingManualTerminalWorktreeParks(): string[] { + const pending = [...pendingWorktreeIds] + pendingWorktreeIds.clear() + return pending +} From 1fd0f731fc0dd4f3fca25dfbd7768c107ba892c8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:03:58 -0700 Subject: [PATCH 02/16] fix(automations): bind agent terminal output before publishing, and launch SSH folder workspaces on their own host (#10818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): bind the agent PTY before the run tab is ever published Why: launchAgentBackgroundSession created the hidden run tab synchronously and only then awaited the agent spawn, so the store briefly held a tab with ptyId: null. Terminal.tsx re-renders on that write, and for an already-visited worktree the tab can neither cold-park nor defer — a TerminalPane mounts, finds no adopt candidate, and starts a fresh default shell. When the agent PTY finally resolves it is rebound in state but the mounted pane still holds the shell, so the user sees a bare prompt and the agent PTY is orphaned (#2989). Reserve the tab id before the spawn and create the tab already bound to the live PTY, with no await in between. * fix(terminal): resolve folder workspaces, and fail closed on a tab-id collision allWorktrees() reads only worktreesByRepo, so every folder workspace looked absent and its automation died at resolution. getKnownWorktreeById covers both. On a reserved-id collision, re-keying the tab could never work: ORCA_TAB_ID and ORCA_PANE_KEY are already baked into the spawned process, so routing and hook identity would permanently disagree. Retire the launch instead. Co-authored-by: Orca * test(automations): split the background-session suite so it stays under the 800-line cap Co-authored-by: Orca * fix(automations): route folder-workspace agent launches to their owning SSH host Review of the bind-before-publish fix surfaced a second defect on the path it newly makes reachable. A folder workspace has no repo row — its synthetic repoId is `folder-workspace:` — so `repos.find(...)` returns null and every repo-derived launch input silently degraded to a local default: connectionId null, platform CLIENT_PLATFORM, isRemote false. The automation then spawned on the user's machine with a cwd that only exists on the SSH host. Before this branch that path threw before reaching the spawn, so the bug was latent; making folder-workspace automations work is what exposes it. Host resolution now goes through resolveAgentBackgroundLaunchHost, which falls back to the workspace scope (the same getFolderWorkspaceConnectionId that ordinary terminal creation uses) when there is no repo. Ambiguous scopes — mixed local/remote children — still resolve to a local launch rather than guessing a host. Extracted to its own module rather than inlined: the added branch pushed launch-agent-background-session.ts over the 300-line oxlint cap, and a max-lines disable is forbidden. Tests: an SSH folder workspace must spawn with its connectionId and remote cwd; a local one must stay local. Reverting the fallback fails the first and leaves the rest green. * fix(automations): close folder dispatch and adoption races Read live state before adopting a reserved tab so a collision that lands during the spawn is retired instead of re-keyed. Route persisted folder-workspace dispatch through its ambiguity-aware owner, including SSH auth, remote trust, quiet-shell fallback, and WSL shell selection. --------- Co-authored-by: Orca --- .../terminal-pane/pty-connection.test.ts | 47 ++ .../hooks/useAutomationDispatchEvents.test.ts | 179 +++++- .../src/hooks/useAutomationDispatchEvents.ts | 80 ++- .../lib/adopt-agent-background-session-tab.ts | 113 ++++ ...ent-background-session-launch-host.test.ts | 85 +++ .../agent-background-session-launch-host.ts | 71 +++ .../agent-background-session-test-state.ts | 33 +- ...ch-agent-background-session-remote.test.ts | 511 ++++++++++++++++++ .../launch-agent-background-session.test.ts | 500 +++++------------ .../lib/launch-agent-background-session.ts | 156 +++--- .../launch-worktree-background-terminals.ts | 4 +- .../lib/retire-unowned-background-terminal.ts | 15 +- .../lib/ssh-background-startup-delivery.ts | 24 +- 13 files changed, 1329 insertions(+), 489 deletions(-) create mode 100644 src/renderer/src/lib/adopt-agent-background-session-tab.ts create mode 100644 src/renderer/src/lib/agent-background-session-launch-host.test.ts create mode 100644 src/renderer/src/lib/agent-background-session-launch-host.ts create mode 100644 src/renderer/src/lib/launch-agent-background-session-remote.test.ts diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 6011813f236..ec51ba00102 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -6389,6 +6389,53 @@ describe('connectPanePty', () => { expect(deps.updateTabPtyId).not.toHaveBeenCalledWith('tab-1', otherTabPtyId) }) + it('fresh-spawns a shell into any PTY-less tab, so agent launches must never publish one', async () => { + // Why: #2989 depends on PTY-less tabs taking this legitimate fresh-shell path. + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transport.connect.mockImplementation(async (opts: { sessionId?: string }) => { + if (opts.sessionId) { + return { id: opts.sessionId } + } + const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as + | ((ptyId: string) => void) + | undefined + onPtySpawn?.('stray-shell-pty') + return 'stray-shell-pty' + }) + transportFactoryQueue.push(transport) + // Reproduce the pre-fix gap between createTab and PTY binding. + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: null }] }, + ptyIdsByTabId: { 'tab-1': [] }, + terminalLayoutsByTabId: { + 'tab-1': { + root: { type: 'leaf', leafId: LEAF_1 }, + activeLeafId: LEAF_1, + expandedLeafId: null + } + }, + agentLaunchConfigByPaneKey: { + [`tab-1:${LEAF_1}`]: { + launchConfig: { agentCommand: 'claude', agentArgs: '', agentEnv: {} }, + identity: { agentType: 'claude' } + } + } + } as StoreState + const deps = createDeps() + + const pane = createPane(1) + connectPanePty(pane as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + // Launch registration alone cannot identify a PTY to attach. + expect(transport.connect).toHaveBeenCalledWith( + expect.objectContaining({ url: '', cols: expect.any(Number) }) + ) + expect(deps.updateTabPtyId).toHaveBeenCalledWith('tab-1', 'stray-shell-pty') + }) + it('spawns a fresh PTY when a restored daemon split session cannot reattach', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport() diff --git a/src/renderer/src/hooks/useAutomationDispatchEvents.test.ts b/src/renderer/src/hooks/useAutomationDispatchEvents.test.ts index 562bbbbf25a..ae8f70d05dd 100644 --- a/src/renderer/src/hooks/useAutomationDispatchEvents.test.ts +++ b/src/renderer/src/hooks/useAutomationDispatchEvents.test.ts @@ -12,6 +12,9 @@ const mockOnDispatchRequested = vi.fn() const mockRendererReady = vi.fn() const mockFinalizeTerminalOwnership = vi.fn() const mockReleaseTerminalOwnership = vi.fn() +const mockSshNeedsPassphrasePrompt = vi.fn() +const mockSshGetState = vi.fn() +const mockSshConnect = vi.fn() const setupLaunch = { runnerScriptPath: '/tmp/setup.sh', @@ -25,15 +28,35 @@ const createdWorktree = { path: '/repo/worktree' } type TestWorktree = typeof createdWorktree +type TestRepo = { + id: string + connectionId: string | null + executionHostId: string | null + path: string +} const state = { activeView: 'terminal' as const, activeWorktreeId: 'wt-active', activeTabId: 'tab-active', activeTabType: 'terminal' as const, - repos: [{ id: 'repo-1', connectionId: null }], + repos: [{ id: 'repo-1', connectionId: null, executionHostId: null, path: '/repo' }] as TestRepo[], + folderWorkspaces: [] as { + id: string + projectGroupId: string + folderPath: string + connectionId: string | null + }[], + projectGroups: [] as { + id: string + connectionId: string | null + executionHostId?: string | null + }[], + worktreesByRepo: {} as Record, + detectedWorktreesByRepo: {}, agentStatusByPaneKey: {}, allWorktrees: vi.fn<() => TestWorktree[]>(() => []), + getKnownWorktreeById: vi.fn<(worktreeId: string) => TestWorktree | undefined>(() => undefined), createWorktree: mockCreateWorktree, subscribe: vi.fn(() => () => {}), setActiveView: vi.fn(), @@ -146,9 +169,13 @@ describe('useAutomationDispatchEvents setup launch', () => { state.activeWorktreeId = 'wt-active' state.activeTabId = 'tab-active' state.activeTabType = 'terminal' - state.repos = [{ id: 'repo-1', connectionId: null }] + state.repos = [{ id: 'repo-1', connectionId: null, executionHostId: null, path: '/repo' }] + state.folderWorkspaces = [] + state.projectGroups = [] + state.worktreesByRepo = {} state.agentStatusByPaneKey = {} state.allWorktrees.mockReturnValue([]) + state.getKnownWorktreeById.mockReturnValue(undefined) mockCreateWorktree.mockResolvedValue({ worktree: createdWorktree, setup: setupLaunch }) mockLaunchWorktreeBackgroundTerminals.mockResolvedValue(undefined) mockLaunchAgentBackgroundSession.mockResolvedValue({ @@ -162,6 +189,9 @@ describe('useAutomationDispatchEvents setup launch', () => { } }) mockOnDispatchRequested.mockReturnValue(() => {}) + mockSshNeedsPassphrasePrompt.mockResolvedValue(false) + mockSshGetState.mockResolvedValue({ status: 'connected' }) + mockSshConnect.mockResolvedValue({ status: 'connected' }) vi.stubGlobal('window', { api: { automations: { @@ -172,9 +202,9 @@ describe('useAutomationDispatchEvents setup launch', () => { listRuns: vi.fn().mockResolvedValue([]) }, ssh: { - needsPassphrasePrompt: vi.fn().mockResolvedValue(false), - getState: vi.fn().mockResolvedValue({ status: 'connected' }), - connect: vi.fn() + needsPassphrasePrompt: mockSshNeedsPassphrasePrompt, + getState: mockSshGetState, + connect: mockSshConnect } }, dispatchEvent: vi.fn() @@ -319,6 +349,145 @@ describe('useAutomationDispatchEvents setup launch', () => { ) }) + it('dispatches an existing SSH folder workspace on its resolved host', async () => { + const folderWorkspace = { + id: 'folder:fw-1', + repoId: 'folder-workspace:group-1', + displayName: 'SSH folder', + path: '/srv/project' + } + state.repos = [ + { + id: 'repo-1', + connectionId: 'ssh-folder', + executionHostId: null, + path: '/srv/project/repo' + } + ] + state.folderWorkspaces = [ + { + id: 'fw-1', + projectGroupId: 'group-1', + folderPath: '/srv/project', + connectionId: 'ssh-folder' + } + ] + state.projectGroups = [{ id: 'group-1', connectionId: 'ssh-folder' }] + state.getKnownWorktreeById.mockReturnValue(folderWorkspace) + mockSshGetState.mockResolvedValue({ status: 'disconnected' }) + + await registerAndDispatch( + makeAutomation({ + workspaceMode: 'existing', + workspaceId: folderWorkspace.id, + setupDecision: 'skip', + runContext: { repoId: 'repo-1', hostId: 'ssh:ssh-folder' } + }) + ) + + expect(state.allWorktrees).not.toHaveBeenCalled() + expect(mockSshConnect).toHaveBeenCalledWith({ targetId: 'ssh-folder' }) + expect(mockLaunchAgentBackgroundSession).toHaveBeenCalledWith( + expect.objectContaining({ + worktreeId: folderWorkspace.id, + prompt: 'run this' + }) + ) + expect(mockMarkDispatchResult).toHaveBeenCalledWith( + expect.objectContaining({ + status: 'dispatched', + workspaceId: folderWorkspace.id, + workspaceDisplayName: folderWorkspace.displayName + }) + ) + }) + + it('dispatches a local folder workspace without SSH', async () => { + const folderWorkspace = { + id: 'folder:fw-local', + repoId: 'folder-workspace:group-local', + displayName: 'Local folder', + path: '/project' + } + state.folderWorkspaces = [ + { + id: 'fw-local', + projectGroupId: 'group-local', + folderPath: '/project', + connectionId: null + } + ] + state.projectGroups = [{ id: 'group-local', connectionId: null }] + state.getKnownWorktreeById.mockReturnValue(folderWorkspace) + + await registerAndDispatch( + makeAutomation({ + workspaceMode: 'existing', + workspaceId: folderWorkspace.id, + runContext: { repoId: 'repo-1', hostId: 'local' } + }) + ) + + expect(mockSshNeedsPassphrasePrompt).not.toHaveBeenCalled() + expect(mockLaunchAgentBackgroundSession).toHaveBeenCalledWith( + expect.objectContaining({ worktreeId: folderWorkspace.id }) + ) + }) + + it('skips a folder workspace owned by a different host', async () => { + const folderWorkspace = { + id: 'folder:fw-other', + repoId: 'folder-workspace:group-other', + displayName: 'Other host', + path: '/srv/other' + } + state.folderWorkspaces = [ + { + id: 'fw-other', + projectGroupId: 'group-other', + folderPath: '/srv/other', + connectionId: 'ssh-other' + } + ] + state.projectGroups = [{ id: 'group-other', connectionId: 'ssh-other' }] + state.getKnownWorktreeById.mockReturnValue(folderWorkspace) + + await registerAndDispatch( + makeAutomation({ + workspaceMode: 'existing', + workspaceId: folderWorkspace.id, + runContext: { repoId: 'repo-1', hostId: 'local' } + }) + ) + + expect(mockLaunchAgentBackgroundSession).not.toHaveBeenCalled() + expect(mockMarkDispatchResult).toHaveBeenCalledWith( + expect.objectContaining({ status: 'skipped_unavailable' }) + ) + }) + + it('keeps detected-only non-folder workspaces unavailable', async () => { + state.getKnownWorktreeById.mockReturnValue({ + id: 'wt-detected', + repoId: 'repo-1', + displayName: 'Detected', + path: '/repo/detected' + }) + + await registerAndDispatch( + makeAutomation({ + workspaceMode: 'existing', + workspaceId: 'wt-detected' + }) + ) + + expect(state.getKnownWorktreeById).not.toHaveBeenCalled() + expect(mockLaunchAgentBackgroundSession).not.toHaveBeenCalled() + expect(mockMarkDispatchResult).toHaveBeenCalledWith( + expect.objectContaining({ status: 'skipped_unavailable' }) + ) + }) + it('finalizes a fresh non-reuse terminal only after completed result persistence', async () => { const order: string[] = [] let launchArgs: { onAgentStatus?: (payload: { state: string }) => void } = {} diff --git a/src/renderer/src/hooks/useAutomationDispatchEvents.ts b/src/renderer/src/hooks/useAutomationDispatchEvents.ts index 1679aaf1af5..d543d639ddf 100644 --- a/src/renderer/src/hooks/useAutomationDispatchEvents.ts +++ b/src/renderer/src/hooks/useAutomationDispatchEvents.ts @@ -24,6 +24,14 @@ import { import { translate } from '@/i18n/i18n' import { createBrowserUuid } from '@/lib/browser-uuid' import type { AutomationTerminalOwnership } from '@/lib/automation-terminal-ownership' +import { getResolvedExecutionHostIdForWorktree } from '@/lib/resolved-worktree-execution-host' +import { + getRepoExecutionHostId, + parseExecutionHostId, + toSshExecutionHostId +} from '../../../shared/execution-host' +import { parseWorkspaceKey } from '../../../shared/workspace-scope' +import { getFolderWorkspaceConnectionId } from '@/lib/folder-workspace-connection' const AUTOMATIONS_CHANGED_EVENT = 'orca:automations-changed' const activeReuseDispatchTabIds = new Set() @@ -63,8 +71,11 @@ export function useAutomationDispatchEvents(): void { } const runRepoId = getAutomationRunRepoId(automation) const repo = state.repos.find((entry) => entry.id === runRepoId) + const automationWorkspaceScope = parseWorkspaceKey(automation.workspaceId ?? '') const automationWorktree = automation.workspaceId - ? state.allWorktrees().find((entry) => entry.id === automation.workspaceId) + ? automationWorkspaceScope?.type === 'folder' + ? state.getKnownWorktreeById(automation.workspaceId) + : state.allWorktrees().find((entry) => entry.id === automation.workspaceId) : null let dispatchWorkspaceId = automation.workspaceId let dispatchWorkspaceDisplayName = @@ -97,9 +108,49 @@ export function useAutomationDispatchEvents(): void { } try { - if (repo.connectionId) { + const folderWorkspaceConnectionId = + automationWorkspaceScope?.type === 'folder' + ? getFolderWorkspaceConnectionId(state, automationWorkspaceScope.folderWorkspaceId) + : null + const folderWorkspaceHostId = + automationWorkspaceScope?.type === 'folder' && automationWorktree + ? folderWorkspaceConnectionId === undefined + ? null + : folderWorkspaceConnectionId + ? toSshExecutionHostId(folderWorkspaceConnectionId) + : getResolvedExecutionHostIdForWorktree(state, automationWorktree.id) + : null + const runHostId = + parseExecutionHostId(automation.runContext?.hostId)?.id ?? getRepoExecutionHostId(repo) + const workspaceMatchesRunTarget = + automationWorkspaceScope?.type === 'folder' + ? folderWorkspaceHostId !== null && folderWorkspaceHostId === runHostId + : !automation.runContext?.repoId || + automationWorktree?.repoId === automation.runContext.repoId + if ( + automation.workspaceMode === 'existing' && + automationWorktree && + !workspaceMatchesRunTarget + ) { + await markDispatchResult({ + runId: run.id, + status: 'skipped_unavailable', + workspaceId: automation.workspaceId, + workspaceDisplayName: dispatchWorkspaceDisplayName, + error: translate( + 'auto.hooks.useAutomationDispatchEvents.3ad7d77f57', + 'The target workspace is on a different host than this automation run target.' + ) + }) + return + } + const sshTargetId = + automationWorkspaceScope?.type === 'folder' + ? (folderWorkspaceConnectionId ?? null) + : (repo.connectionId ?? null) + if (sshTargetId) { const needsPrompt = await window.api.ssh.needsPassphrasePrompt({ - targetId: repo.connectionId + targetId: sshTargetId }) if (needsPrompt) { await markDispatchResult({ @@ -114,10 +165,10 @@ export function useAutomationDispatchEvents(): void { }) return } - const sshState = await window.api.ssh.getState({ targetId: repo.connectionId }) + const sshState = await window.api.ssh.getState({ targetId: sshTargetId }) if (sshState?.status !== 'connected') { try { - const connected = await window.api.ssh.connect({ targetId: repo.connectionId }) + const connected = await window.api.ssh.connect({ targetId: sshTargetId }) if (connected?.status !== 'connected') { throw new Error('SSH target is unavailable.') } @@ -134,25 +185,6 @@ export function useAutomationDispatchEvents(): void { } } - if ( - automation.workspaceMode === 'existing' && - automationWorktree && - automation.runContext?.repoId && - automationWorktree.repoId !== automation.runContext.repoId - ) { - await markDispatchResult({ - runId: run.id, - status: 'skipped_unavailable', - workspaceId: automation.workspaceId, - workspaceDisplayName: dispatchWorkspaceDisplayName, - error: translate( - 'auto.hooks.useAutomationDispatchEvents.3ad7d77f57', - 'The target workspace is on a different host than this automation run target.' - ) - }) - return - } - if (automation.workspaceMode === 'existing' && !automationWorktree) { await markDispatchResult({ runId: run.id, diff --git a/src/renderer/src/lib/adopt-agent-background-session-tab.ts b/src/renderer/src/lib/adopt-agent-background-session-tab.ts new file mode 100644 index 00000000000..ec604137645 --- /dev/null +++ b/src/renderer/src/lib/adopt-agent-background-session-tab.ts @@ -0,0 +1,113 @@ +import { useAppStore } from '@/store' +import { makePaneKey, type PaneKey } from '../../../shared/stable-pane-id' +import type { AgentType } from '../../../shared/agent-status-types' +import { bindAutomationTerminal } from '@/lib/automation-terminal-ownership' +import { createBrowserUuid } from '@/lib/browser-uuid' +import { retireProvider, retireUnownedTerminal } from '@/lib/retire-unowned-background-terminal' +import { isTerminalTabPresent } from '@/store/slices/terminal-tab-retirement' +import type { RuntimeClientTarget } from '@/runtime/runtime-rpc-client' + +type Store = ReturnType +type RegisterArgs = Parameters + +/** Reserves env-stable tab and pane identities before spawning the PTY. */ +export function reserveAgentBackgroundSessionIdentity(args: { + store: Store + agentType: AgentType + worktreeId: string + launchConfig: RegisterArgs[1] + env: Record | undefined +}): { + reservedTabId: string + leafId: string + paneKey: PaneKey + launchToken: string + launchRegistration: NonNullable + paneEnv: Record +} { + const reservedTabId = createBrowserUuid() + const leafId = createBrowserUuid() + const paneKey = makePaneKey(reservedTabId, leafId) + const launchToken = createBrowserUuid() + const launchRegistration = { + agentType: args.agentType, + launchToken, + tabId: reservedTabId, + leafId + } + args.store.registerAgentLaunchConfig(paneKey, args.launchConfig, launchRegistration) + return { + reservedTabId, + leafId, + paneKey, + launchToken, + launchRegistration, + paneEnv: { + ...args.env, + ORCA_PANE_KEY: paneKey, + ORCA_TAB_ID: reservedTabId, + ORCA_WORKTREE_ID: args.worktreeId, + ORCA_AGENT_LAUNCH_TOKEN: launchToken + } + } +} + +/** Publishes a hidden run tab already bound to its live PTY (#2989). */ +export async function adoptAgentBackgroundSessionTab(args: { + store: Store + worktreeId: string + reservedTabId: string + ptyId: string + paneKey: PaneKey + launchConfig: RegisterArgs[1] + launchRegistration: NonNullable + runtimeTarget: RuntimeClientTarget + runtimeTerminalHandle: string | null + onRetire: () => void + title?: string +}): Promise<{ + tab: ReturnType + paneKey: PaneKey + terminalOwnership: ReturnType +} | null> { + const { store, reservedTabId, ptyId, launchRegistration } = args + // The worktree can disappear while its PTY spawn is pending. + if ( + await retireUnownedTerminal({ + owner: { worktreeId: args.worktreeId }, + ptyId, + runtimeTarget: args.runtimeTarget, + runtimeTerminalHandle: args.runtimeTerminalHandle, + onRetire: args.onRetire + }) + ) { + return null + } + // Re-keying would desynchronize env-baked routing identities; fail closed. + if (isTerminalTabPresent(useAppStore.getState(), reservedTabId)) { + store.clearAgentLaunchConfig(args.paneKey) + args.onRetire() + await retireProvider({ + ptyId, + runtimeTarget: args.runtimeTarget, + runtimeTerminalHandle: args.runtimeTerminalHandle + }) + return null + } + const tab = store.createTab(args.worktreeId, undefined, undefined, { + id: reservedTabId, + initialPtyId: ptyId, + activate: false, + recordInteraction: false + }) + const paneKey = args.paneKey + store.registerAgentLaunchConfig(paneKey, args.launchConfig, launchRegistration) + const terminalOwnership = bindAutomationTerminal( + tab, + paneKey, + ptyId, + args.runtimeTarget.kind, + args.title + ) + return { tab, paneKey, terminalOwnership } +} diff --git a/src/renderer/src/lib/agent-background-session-launch-host.test.ts b/src/renderer/src/lib/agent-background-session-launch-host.test.ts new file mode 100644 index 00000000000..86fd8577fae --- /dev/null +++ b/src/renderer/src/lib/agent-background-session-launch-host.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from 'vitest' +import { resolveAgentBackgroundLaunchHost } from './agent-background-session-launch-host' + +function makeFolderHostState(args: { + connectionId: string | null + folderPath: string + repos?: { + id: string + connectionId: string | null + path: string + projectGroupId: string + }[] +}) { + return { + folderWorkspaces: [ + { + id: 'folder-1', + projectGroupId: 'group-1', + folderPath: args.folderPath, + connectionId: args.connectionId + } + ], + projectGroups: [ + { + id: 'group-1', + parentGroupId: null, + connectionId: args.connectionId + } + ], + repos: args.repos ?? [] + } +} + +describe('resolveAgentBackgroundLaunchHost', () => { + it('keeps an authoritative local folder owner local', () => { + const host = resolveAgentBackgroundLaunchHost({ + store: makeFolderHostState({ connectionId: null, folderPath: '/project' }) as never, + worktreeId: 'folder:folder-1', + worktreePath: '/project', + repo: null + }) + + expect(host).toMatchObject({ + connectionId: null, + isRemote: false, + expectedConnectionId: null + }) + }) + + it('fails closed when folder ownership is ambiguous', () => { + const store = makeFolderHostState({ + connectionId: 'ssh-1', + folderPath: '/project', + repos: [ + { + id: 'repo-local', + connectionId: null, + path: '/project/repo', + projectGroupId: 'group-1' + } + ] + }) + + expect(() => + resolveAgentBackgroundLaunchHost({ + store: store as never, + worktreeId: 'folder:folder-1', + worktreePath: '/project', + repo: null + }) + ).toThrow('unavailable or ambiguous') + }) + + it('uses Linux startup quoting for a local WSL folder', () => { + const folderPath = '\\\\wsl.localhost\\Ubuntu\\home\\me\\project' + const host = resolveAgentBackgroundLaunchHost({ + store: makeFolderHostState({ connectionId: null, folderPath }) as never, + worktreeId: 'folder:folder-1', + worktreePath: folderPath, + repo: null + }) + + expect(host.platform).toBe('linux') + }) +}) diff --git a/src/renderer/src/lib/agent-background-session-launch-host.ts b/src/renderer/src/lib/agent-background-session-launch-host.ts new file mode 100644 index 00000000000..7919f9f3af5 --- /dev/null +++ b/src/renderer/src/lib/agent-background-session-launch-host.ts @@ -0,0 +1,71 @@ +import type { useAppStore } from '@/store' +import { getAgentLaunchPlatformForRepo } from '@/lib/agent-launch-platform' +import { CLIENT_PLATFORM } from '@/lib/new-workspace' +import { getLocalProjectExecutionRuntimeContext } from '@/lib/local-preflight-context' +import { getFolderWorkspaceConnectionId } from '@/lib/folder-workspace-connection' +import { parseWorkspaceKey } from '../../../shared/workspace-scope' +import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path' +import { repoIsRemote } from '../../../shared/agent-launch-remote' +import { isWslUncPath } from '../../../shared/wsl-paths' + +type LaunchStore = ReturnType +type LaunchRepo = LaunchStore['repos'][number] + +export type AgentBackgroundLaunchHost = { + /** SSH connection to spawn on, or null for a local launch. */ + connectionId: string | null + /** Platform whose shell quoting and CLI naming the startup plan must target. */ + platform: NodeJS.Platform + isRemote: boolean + /** Accepted status connection; undefined preserves unknown-owner behavior. */ + expectedConnectionId: string | null | undefined +} + +function resolveFolderWorkspaceConnectionIdForLaunch( + store: LaunchStore, + worktreeId: string +): string | null | undefined { + const parsed = parseWorkspaceKey(worktreeId) + if (parsed?.type !== 'folder') { + return undefined + } + return getFolderWorkspaceConnectionId(store, parsed.folderWorkspaceId) +} + +/** Resolves folder launch ownership from workspace scope when no repo row exists. */ +export function resolveAgentBackgroundLaunchHost(args: { + store: LaunchStore + worktreeId: string + worktreePath: string | undefined + repo: LaunchRepo | null | undefined +}): AgentBackgroundLaunchHost { + const { store, worktreeId, worktreePath, repo } = args + if (repo) { + return { + connectionId: repo.connectionId ?? null, + platform: getAgentLaunchPlatformForRepo( + repo, + repo.connectionId ? undefined : getLocalProjectExecutionRuntimeContext(store, worktreeId) + ), + isRemote: repoIsRemote(repo), + expectedConnectionId: repo.connectionId ?? null + } + } + const folderWorkspaceConnectionId = resolveFolderWorkspaceConnectionIdForLaunch(store, worktreeId) + const isFolderWorkspace = parseWorkspaceKey(worktreeId)?.type === 'folder' + if (isFolderWorkspace && folderWorkspaceConnectionId === undefined) { + throw new Error('The target folder workspace host is unavailable or ambiguous.') + } + return { + connectionId: folderWorkspaceConnectionId ?? null, + platform: folderWorkspaceConnectionId + ? isWindowsAbsolutePathLike(worktreePath ?? '') + ? 'win32' + : 'linux' + : isWslUncPath(worktreePath ?? '') + ? 'linux' + : CLIENT_PLATFORM, + isRemote: Boolean(folderWorkspaceConnectionId), + expectedConnectionId: isFolderWorkspace ? (folderWorkspaceConnectionId ?? null) : undefined + } +} diff --git a/src/renderer/src/lib/agent-background-session-test-state.ts b/src/renderer/src/lib/agent-background-session-test-state.ts index b338493c411..85def0ffadb 100644 --- a/src/renderer/src/lib/agent-background-session-test-state.ts +++ b/src/renderer/src/lib/agent-background-session-test-state.ts @@ -24,7 +24,14 @@ export type AgentBackgroundSessionTestState = { | { kind: 'windows-host' } | { kind: 'wsl'; distro: string | null } }[] - repos: { id: string; connectionId: string | null; path: string }[] + repos: { id: string; connectionId: string | null; path: string; projectGroupId?: string | null }[] + folderWorkspaces: { + id: string + projectGroupId: string + folderPath: string + connectionId?: string | null + }[] + projectGroups: { id: string; parentGroupId?: string | null; connectionId?: string | null }[] worktreesByRepo: Record< string, { id: string; repoId: string; projectId: string; path: string; displayName: string }[] @@ -35,6 +42,7 @@ export type AgentBackgroundSessionTestState = { sshConnectionStates: Map transientClearedAgentStatusConnectionIds: Record allWorktrees: () => { id: string; repoId: string; path: string }[] + getKnownWorktreeById: (worktreeId: string) => { id: string; path: string } | undefined createTab: TestMock setTabCustomTitle: TestMock updateTabPtyId: TestMock @@ -87,6 +95,8 @@ export function createAgentBackgroundSessionTestState(mocks: { } ] }, + folderWorkspaces: [] as AgentBackgroundSessionTestState['folderWorkspaces'], + projectGroups: [] as AgentBackgroundSessionTestState['projectGroups'], tabsByWorktree: { 'wt-1': [] as { id: string; title: string }[] }, terminalLayoutsByTabId: {} as Record< string, @@ -96,6 +106,8 @@ export function createAgentBackgroundSessionTestState(mocks: { sshConnectionStates: new Map(), transientClearedAgentStatusConnectionIds: {} as Record, allWorktrees: () => state.worktreesByRepo['repo-1'], + getKnownWorktreeById: (worktreeId: string) => + state.worktreesByRepo['repo-1']?.find((worktree) => worktree.id === worktreeId), createTab: mocks.createTab, setTabCustomTitle: mocks.setTabCustomTitle, updateTabPtyId: mocks.updateTabPtyId, @@ -120,6 +132,8 @@ export function resetAgentBackgroundSessionTestState(state: AgentBackgroundSessi } state.projects = [{ id: 'repo-1', localWindowsRuntimePreference: { kind: 'inherit-global' } }] state.repos = [{ id: 'repo-1', connectionId: null, path: '/repo' }] + state.folderWorkspaces = [] + state.projectGroups = [] state.worktreesByRepo = { 'repo-1': [ { @@ -136,6 +150,9 @@ export function resetAgentBackgroundSessionTestState(state: AgentBackgroundSessi state.ptyIdsByTabId = {} state.sshConnectionStates = new Map() state.transientClearedAgentStatusConnectionIds = {} + // Why: restored here so a test that stubs folder-workspace lookup cannot leak it forward. + state.getKnownWorktreeById = (worktreeId: string) => + state.worktreesByRepo['repo-1']?.find((worktree) => worktree.id === worktreeId) } export function useRemoteAgentBackgroundRuntime(state: AgentBackgroundSessionTestState): void { @@ -146,6 +163,13 @@ export function useRemoteAgentBackgroundRuntime(state: AgentBackgroundSessionTes } } +/** The tab id reserved before the spawn; the run tab adopts it once the PTY is live. */ +export function expectReservedAgentBackgroundTabId(spawn: TestMock): string { + const tabId = spawn.mock.calls[0]?.[0]?.tabId + expect(tabId).toMatch(AGENT_BACKGROUND_SESSION_UUID_RE) + return tabId +} + export function expectStableAgentBackgroundPaneSpawn(spawn: TestMock): string { const spawnArgs = spawn.mock.calls[0]?.[0] const paneKey = spawnArgs?.env?.ORCA_PANE_KEY @@ -153,7 +177,7 @@ export function expectStableAgentBackgroundPaneSpawn(spawn: TestMock): string { expect(typeof paneKey).toBe('string') expect(typeof leafId).toBe('string') expect(leafId).toMatch(AGENT_BACKGROUND_SESSION_UUID_RE) - expect(paneKey).toBe(`tab-1:${leafId}`) + expect(paneKey).toBe(`${expectReservedAgentBackgroundTabId(spawn)}:${leafId}`) return paneKey } @@ -208,8 +232,9 @@ export function resetAgentBackgroundSessionTestHarness(args: { (args.runtimeCall as unknown as (value: unknown) => unknown)(request) ) resetAgentBackgroundSessionTestState(args.state) - args.createTab.mockImplementation(() => { - const tab = { id: 'tab-1', title: 'Terminal 1' } + // Why: production reserves the tab id before the spawn; honoring options.id mirrors createTab's adoption contract. + args.createTab.mockImplementation((_worktreeId, _groupId, _shellOverride, options) => { + const tab = { id: options?.id ?? 'tab-1', title: 'Terminal 1' } args.state.tabsByWorktree['wt-1'].push(tab) return tab }) diff --git a/src/renderer/src/lib/launch-agent-background-session-remote.test.ts b/src/renderer/src/lib/launch-agent-background-session-remote.test.ts new file mode 100644 index 00000000000..6ae4071ba55 --- /dev/null +++ b/src/renderer/src/lib/launch-agent-background-session-remote.test.ts @@ -0,0 +1,511 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + AGENT_BACKGROUND_SESSION_UUID_RE as UUID_RE, + createAgentBackgroundSessionTestState, + resetAgentBackgroundSessionTestHarness, + useRemoteAgentBackgroundRuntime +} from '@/lib/agent-background-session-test-state' + +const mockSpawn = vi.fn() +const mockKill = vi.fn() +const mockWrite = vi.fn() +const mockRuntimeEnvironmentCall = vi.fn() +const mockRuntimeEnvironmentTransportCall = vi.fn() +const mockRuntimeEnvironmentSubscribe = vi.fn() +const mockCreateTab = vi.fn() +const mockSetTabCustomTitle = vi.fn() +const mockUpdateTabPtyId = vi.fn() +const mockCloseTab = vi.fn() +const mockSetTabLayout = vi.fn() +const mockRegisterAgentLaunchConfig = vi.fn() +const mockRegisterEagerPtyBuffer = vi.fn() +const mockSubscribeToPtyData = vi.fn() +const mockSubscribeToPtyExit = vi.fn() +const mockPasteDraftWhenAgentReady = vi.fn() +const mockMarkTrusted = vi.fn() +const mockDispatchEvent = vi.fn() +const mockGetAgentLaunchPlatformForRepo = vi.fn<() => NodeJS.Platform>() +const state = createAgentBackgroundSessionTestState({ + createTab: mockCreateTab, + setTabCustomTitle: mockSetTabCustomTitle, + updateTabPtyId: mockUpdateTabPtyId, + closeTab: mockCloseTab, + setTabLayout: mockSetTabLayout, + registerAgentLaunchConfig: mockRegisterAgentLaunchConfig +}) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => state, + subscribe: vi.fn(() => () => {}) + } +})) + +vi.mock('@/lib/telemetry', () => ({ + track: vi.fn(), + tuiAgentToAgentKind: (agent: string) => agent +})) + +vi.mock('@/lib/agent-paste-draft', () => ({ + pasteDraftWhenAgentReady: mockPasteDraftWhenAgentReady +})) + +vi.mock('@/lib/agent-launch-platform', () => ({ + getAgentLaunchPlatformForRepo: mockGetAgentLaunchPlatformForRepo +})) + +vi.mock('@/components/terminal-pane/pty-dispatcher', () => ({ + registerEagerPtyBuffer: mockRegisterEagerPtyBuffer, + subscribeToPtyExit: mockSubscribeToPtyExit +})) + +vi.mock('@/components/terminal-pane/pty-data-sidecar-subscriptions', () => ({ + subscribeToPtyData: mockSubscribeToPtyData +})) + +describe('launchAgentBackgroundSession remote runtime and SSH startup delivery', () => { + beforeEach(() => { + resetAgentBackgroundSessionTestHarness({ + state, + createTab: mockCreateTab, + closeTab: mockCloseTab, + getLaunchPlatform: mockGetAgentLaunchPlatformForRepo, + runtimeCall: mockRuntimeEnvironmentCall, + runtimeTransportCall: mockRuntimeEnvironmentTransportCall, + runtimeSubscribe: mockRuntimeEnvironmentSubscribe, + subscribeToData: mockSubscribeToPtyData, + subscribeToExit: mockSubscribeToPtyExit, + setTabLayout: mockSetTabLayout, + updateTabPtyId: mockUpdateTabPtyId, + dispatchEvent: mockDispatchEvent, + kill: mockKill, + markTrusted: mockMarkTrusted, + spawn: mockSpawn, + write: mockWrite + }) + }) + + it('closes a runtime terminal when its worktree disappears before creation resolves', async () => { + useRemoteAgentBackgroundRuntime(state) + let resolveCreate!: (result: { + ok: true + result: { terminal: { handle: string; worktreeId: string; title: null } } + }) => void + const createResult = new Promise<{ + ok: true + result: { terminal: { handle: string; worktreeId: string; title: null } } + }>((resolve) => { + resolveCreate = resolve + }) + mockRuntimeEnvironmentCall.mockImplementation((args: { method: string }) => { + if (args.method === 'terminal.createAgentSession') { + return createResult + } + return Promise.resolve({ ok: true, result: {} }) + }) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + const launch = launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run remotely' + }) + await vi.waitFor(() => + expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.createAgentSession' }) + ) + ) + state.worktreesByRepo['repo-1'] = [] + resolveCreate({ + ok: true, + result: { terminal: { handle: 'terminal-after-close', worktreeId: 'wt-1', title: null } } + }) + + await expect(launch).resolves.toBeNull() + expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'terminal.close', + params: { terminal: 'terminal-after-close' } + }) + ) + expect(mockCreateTab).not.toHaveBeenCalled() + expect(mockUpdateTabPtyId).not.toHaveBeenCalled() + expect(mockRuntimeEnvironmentSubscribe).not.toHaveBeenCalled() + expect(mockDispatchEvent).not.toHaveBeenCalled() + }) + + it('forwards Hermes startup queries through SSH command transport', async () => { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'hermes', + worktreeId: 'wt-1', + prompt: 'remote automation prompt' + }) + + expect(mockSpawn).toHaveBeenCalledWith( + expect.objectContaining({ + command: expect.stringContaining('ORCA_HERMES_STARTUP_QUERY'), + connectionId: 'ssh-1', + env: expect.objectContaining({ ORCA_HERMES_STARTUP_QUERY: 'remote automation prompt' }) + }) + ) + }) + + it('injects fast startup commands into SSH background sessions after shell output arrives', async () => { + vi.useFakeTimers() + try { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run the automation', + title: 'Nightly audit' + }) + + expect(mockSpawn.mock.calls[0]?.[0]?.command).toBe( + "claude '--dangerously-skip-permissions' 'run the automation'" + ) + expect(mockSpawn.mock.calls[0]?.[0]?.startupCommandDelivery).toBeUndefined() + const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void + dataSidecar('user@remote repo % ') + vi.advanceTimersByTime(50) + + expect(mockWrite).toHaveBeenCalledWith( + 'pty-1', + "claude '--dangerously-skip-permissions' 'run the automation'\r" + ) + } finally { + vi.useRealTimers() + } + }) + + it('waits for shell-ready before injecting payload-bearing SSH background commands', async () => { + vi.useFakeTimers() + try { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'codex', + worktreeId: 'wt-1', + prompt: 'run the automation', + title: 'Nightly audit' + }) + + expect(mockSpawn.mock.calls[0]?.[0]).toEqual( + expect.objectContaining({ + command: "codex '--dangerously-bypass-approvals-and-sandbox' 'run the automation'", + startupCommandDelivery: 'shell-ready' + }) + ) + const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void + dataSidecar('user@remote repo % ') + vi.advanceTimersByTime(50) + expect(mockWrite).not.toHaveBeenCalled() + + dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') + vi.advanceTimersByTime(50) + + expect(mockWrite).toHaveBeenCalledWith( + 'pty-1', + "codex '--dangerously-bypass-approvals-and-sandbox' 'run the automation'\r" + ) + } finally { + vi.useRealTimers() + } + }) + + it('falls back when an SSH shell produces no observable startup data', async () => { + vi.useFakeTimers() + try { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'codex', + worktreeId: 'wt-1', + prompt: 'run the automation' + }) + vi.advanceTimersByTime(1_550) + + expect(mockWrite).toHaveBeenCalledWith( + 'pty-1', + "codex '--dangerously-bypass-approvals-and-sandbox' 'run the automation'\r" + ) + } finally { + vi.useRealTimers() + } + }) + + it('waits for shell-ready for SSH background Codex native prefill commands without a hint', async () => { + vi.useFakeTimers() + try { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + state.settings = { + agentCmdOverrides: { codex: "codex --prefill 'draft from override'" }, + activeRuntimeEnvironmentId: null, + terminalMainSideEffectAuthority: undefined + } + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'codex', + worktreeId: 'wt-1', + title: 'Nightly audit' + }) + + expect(mockSpawn.mock.calls[0]?.[0]).toEqual( + expect.objectContaining({ + command: + "codex --prefill 'draft from override' '--dangerously-bypass-approvals-and-sandbox'" + }) + ) + expect(mockSpawn.mock.calls[0]?.[0]).not.toHaveProperty('startupCommandDelivery') + const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void + dataSidecar('user@remote repo % ') + vi.advanceTimersByTime(50) + expect(mockWrite).not.toHaveBeenCalled() + + dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') + vi.advanceTimersByTime(50) + + expect(mockWrite).toHaveBeenCalledWith( + 'pty-1', + "codex --prefill 'draft from override' '--dangerously-bypass-approvals-and-sandbox'\r" + ) + } finally { + vi.useRealTimers() + } + }) + + it('does not rearm SSH background startup delivery after exit cleanup', async () => { + vi.useFakeTimers() + try { + state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'codex', + worktreeId: 'wt-1', + prompt: 'run the automation', + title: 'Nightly audit' + }) + + const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void + const exitSidecar = mockSubscribeToPtyExit.mock.calls[0]?.[1] as (code: number) => void + exitSidecar(0) + + dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') + vi.advanceTimersByTime(50) + + expect(mockWrite).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('creates background sessions on the active runtime environment', async () => { + useRemoteAgentBackgroundRuntime(state) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + const result = await launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run the automation' + }) + + expect(mockSpawn).not.toHaveBeenCalled() + const params = mockRuntimeEnvironmentCall.mock.calls[0]?.[0]?.params + const leafId = params?.placement?.leafId + const tabId = params?.placement?.tabId + expect(leafId).toMatch(UUID_RE) + expect(tabId).toMatch(UUID_RE) + // Why: background launches have no explicit recipe override, so remote host settings win. + expect(params).not.toHaveProperty('agentArgs') + expect(mockRegisterAgentLaunchConfig).toHaveBeenCalledWith( + `${tabId}:${leafId}`, + { + agentCommand: "claude '--dangerously-skip-permissions'", + agentArgs: '--dangerously-skip-permissions', + agentEnv: {} + }, + { + agentType: 'claude', + launchToken: expect.stringMatching(UUID_RE), + tabId, + leafId + } + ) + expect(mockSetTabLayout).toHaveBeenCalledWith( + tabId, + expect.objectContaining({ + root: { type: 'leaf', leafId }, + activeLeafId: leafId, + ptyIdsByLeafId: { [leafId]: 'remote:env-1@@terminal-1' } + }) + ) + expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'terminal.createAgentSession', + params: expect.objectContaining({ + clientOperationId: expect.stringMatching(/^\d{13}-[0-9a-f]{32}$/), + worktree: 'id:wt-1', + agent: 'claude', + prompt: 'run the automation', + promptDelivery: 'auto-submit', + placement: { tabId, leafId }, + presentation: 'background' + }), + timeoutMs: 15_000 + }) + expect(mockUpdateTabPtyId).toHaveBeenCalledWith(tabId, 'remote:env-1@@terminal-1') + expect(mockRegisterEagerPtyBuffer).not.toHaveBeenCalled() + expect(mockRuntimeEnvironmentSubscribe).toHaveBeenCalledWith( + expect.objectContaining({ + selector: 'env-1', + method: 'terminal.multiplex', + params: {} + }), + expect.any(Object) + ) + expect(result).toMatchObject({ + tabId, + paneKey: `${tabId}:${leafId}`, + ptyId: 'remote:env-1@@terminal-1', + terminalOwnership: null + }) + }) + + it('preserves the legacy background spawn on an old remote host', async () => { + useRemoteAgentBackgroundRuntime(state) + mockRuntimeEnvironmentTransportCall.mockImplementation((request: { method: string }) => { + if (request.method === 'status.get') { + return Promise.resolve({ + id: 'status', + ok: true, + result: { + runtimeId: 'old-runtime', + graphStatus: 'ready', + runtimeProtocolVersion: 3, + minCompatibleRuntimeClientVersion: 2, + capabilities: [] + } + }) + } + return Promise.resolve({ + id: 'create', + ok: true, + result: { terminal: { handle: 'legacy-terminal-1' } } + }) + }) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await expect( + launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run remotely' + }) + ).resolves.toMatchObject({ ptyId: 'remote:env-1@@legacy-terminal-1' }) + + expect(mockRuntimeEnvironmentTransportCall).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'terminal.create', + params: expect.objectContaining({ + worktree: 'id:wt-1', + command: "claude '--dangerously-skip-permissions' 'run remotely'", + launchAgent: 'claude', + presentation: 'background' + }) + }) + ) + }) + + it('closes a created runtime terminal when its data subscription fails', async () => { + useRemoteAgentBackgroundRuntime(state) + mockRuntimeEnvironmentSubscribe.mockRejectedValueOnce(new Error('subscription failed')) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await expect( + launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run the automation' + }) + ).rejects.toThrow('subscription failed') + + expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'terminal.close', + params: { terminal: 'terminal-1' }, + timeoutMs: undefined + }) + const tabId = mockRuntimeEnvironmentCall.mock.calls[0]?.[0]?.params?.placement?.tabId + expect(tabId).toMatch(UUID_RE) + expect(state.clearTabPtyId).toHaveBeenCalledWith(tabId, 'remote:env-1@@terminal-1') + expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith( + expect.stringMatching(new RegExp(`^${tabId}:`)) + ) + expect(mockCloseTab).toHaveBeenCalledWith(tabId, { + recordInteraction: false, + reason: 'cleanup' + }) + expect(mockDispatchEvent).not.toHaveBeenCalled() + }) + + it('spawns an SSH folder-workspace automation on the owning host, not locally', async () => { + // Folder workspaces have no repo row, so launch ownership comes from their scope. + state.repos = [ + { id: 'repo-1', connectionId: 'ssh-1', path: '/srv/proj/api', projectGroupId: 'grp-1' } + ] + state.projectGroups = [{ id: 'grp-1', parentGroupId: null, connectionId: 'ssh-1' }] + state.folderWorkspaces = [ + { id: 'fw-1', projectGroupId: 'grp-1', folderPath: '/srv/proj', connectionId: 'ssh-1' } + ] + state.getKnownWorktreeById = (worktreeId: string) => + worktreeId === 'folder:fw-1' ? { id: 'folder:fw-1', path: '/srv/proj' } : undefined + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'codex', + worktreeId: 'folder:fw-1', + prompt: 'run the automation' + }) + + expect(mockMarkTrusted).toHaveBeenCalledWith({ + preset: 'codex', + workspacePath: '/srv/proj', + connectionId: 'ssh-1' + }) + expect(mockSpawn).toHaveBeenCalledWith( + expect.objectContaining({ connectionId: 'ssh-1', cwd: '/srv/proj' }) + ) + }) + + it('keeps a local folder workspace on the local host', async () => { + state.repos = [ + { id: 'repo-1', connectionId: null, path: '/home/me/proj/api', projectGroupId: 'grp-1' } + ] + state.projectGroups = [{ id: 'grp-1', parentGroupId: null, connectionId: null }] + state.folderWorkspaces = [ + { id: 'fw-1', projectGroupId: 'grp-1', folderPath: '/home/me/proj', connectionId: null } + ] + state.getKnownWorktreeById = (worktreeId: string) => + worktreeId === 'folder:fw-1' ? { id: 'folder:fw-1', path: '/home/me/proj' } : undefined + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'folder:fw-1', + prompt: 'run the automation' + }) + + expect(mockSpawn).toHaveBeenCalledWith( + expect.objectContaining({ connectionId: null, cwd: '/home/me/proj' }) + ) + }) +}) diff --git a/src/renderer/src/lib/launch-agent-background-session.test.ts b/src/renderer/src/lib/launch-agent-background-session.test.ts index a82996c97f7..78fd3a91f6e 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -4,9 +4,9 @@ import { toAppSshPtyId } from '../../../shared/ssh-pty-id' import { AGENT_BACKGROUND_SESSION_UUID_RE as UUID_RE, createAgentBackgroundSessionTestState, + expectReservedAgentBackgroundTabId, expectStableAgentBackgroundPaneSpawn, - resetAgentBackgroundSessionTestHarness, - useRemoteAgentBackgroundRuntime + resetAgentBackgroundSessionTestHarness } from '@/lib/agent-background-session-test-state' const mockSpawn = vi.fn() @@ -36,10 +36,11 @@ const state = createAgentBackgroundSessionTestState({ setTabLayout: mockSetTabLayout, registerAgentLaunchConfig: mockRegisterAgentLaunchConfig }) +let currentStoreState = state vi.mock('@/store', () => ({ useAppStore: { - getState: () => state, + getState: () => currentStoreState, subscribe: vi.fn(() => () => {}) } })) @@ -68,6 +69,7 @@ vi.mock('@/components/terminal-pane/pty-data-sidecar-subscriptions', () => ({ describe('launchAgentBackgroundSession', () => { beforeEach(() => { + currentStoreState = state resetAgentBackgroundSessionTestHarness({ state, createTab: mockCreateTab, @@ -88,7 +90,7 @@ describe('launchAgentBackgroundSession', () => { }) }) - it('spawns a PTY immediately and adopts it in an inactive tab', async () => { + it('spawns a PTY first and creates the inactive tab already bound to it', async () => { const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') const result = await launchAgentBackgroundSession({ @@ -98,14 +100,21 @@ describe('launchAgentBackgroundSession', () => { title: 'Nightly audit' }) + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + // A store-visible PTY-less run tab fresh-spawns a shell (#2989). + expect(mockSpawn.mock.invocationCallOrder[0]).toBeLessThan( + mockCreateTab.mock.invocationCallOrder[0] ?? 0 + ) expect(mockCreateTab).toHaveBeenCalledWith('wt-1', undefined, undefined, { + id: tabId, + initialPtyId: 'pty-1', activate: false, recordInteraction: false }) expect(mockDispatchEvent).toHaveBeenCalledWith( expect.objectContaining({ type: BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT, - detail: { worktreeId: 'wt-1', tabIds: ['tab-1'] } + detail: { worktreeId: 'wt-1', tabIds: [tabId] } }) ) expect(mockUpdateTabPtyId.mock.invocationCallOrder[0]).toBeLessThan( @@ -116,18 +125,18 @@ describe('launchAgentBackgroundSession', () => { cwd: '/repo/worktree', command: "claude '--dangerously-skip-permissions' 'run the automation'", env: expect.objectContaining({ - ORCA_TAB_ID: 'tab-1', + ORCA_TAB_ID: tabId, ORCA_WORKTREE_ID: 'wt-1' }), connectionId: null, worktreeId: 'wt-1', - tabId: 'tab-1' + tabId }) ) const paneKey = expectStableAgentBackgroundPaneSpawn(mockSpawn) - const leafId = paneKey.slice('tab-1:'.length) + const leafId = paneKey.slice(`${tabId}:`.length) expect(mockSetTabLayout).toHaveBeenCalledWith( - 'tab-1', + tabId, expect.objectContaining({ root: { type: 'leaf', leafId }, activeLeafId: leafId, @@ -147,17 +156,17 @@ describe('launchAgentBackgroundSession', () => { expect(mockSpawn.mock.calls[0]?.[0].launchToken).toBe( mockSpawn.mock.calls[0]?.[0].env.ORCA_AGENT_LAUNCH_TOKEN ) - expect(mockSetTabCustomTitle).toHaveBeenCalledWith('tab-1', 'Nightly audit', { + expect(mockSetTabCustomTitle).toHaveBeenCalledWith(tabId, 'Nightly audit', { recordInteraction: false }) - expect(mockUpdateTabPtyId).toHaveBeenCalledWith('tab-1', 'pty-1') + expect(mockUpdateTabPtyId).toHaveBeenCalledWith(tabId, 'pty-1') expect(mockRegisterEagerPtyBuffer).toHaveBeenCalledWith('pty-1', expect.any(Function)) expect(mockSubscribeToPtyData).toHaveBeenCalledWith('pty-1', expect.any(Function)) expect(mockSubscribeToPtyExit).toHaveBeenCalledWith('pty-1', expect.any(Function)) - expect(result).toMatchObject({ tabId: 'tab-1', paneKey, ptyId: 'pty-1' }) + expect(result).toMatchObject({ tabId, paneKey, ptyId: 'pty-1' }) }) - it('does not mount the tab while the explicit PTY spawn is unresolved', async () => { + it('does not create or mount the tab while the explicit PTY spawn is unresolved', async () => { let resolveSpawn!: (result: { id: string }) => void mockSpawn.mockReturnValueOnce( new Promise<{ id: string }>((resolve) => { @@ -173,18 +182,24 @@ describe('launchAgentBackgroundSession', () => { }) await Promise.resolve() - expect(mockCreateTab).toHaveBeenCalled() + // Publishing a PTY-less tab here reproduces #2989. + expect(mockCreateTab).not.toHaveBeenCalled() expect(mockDispatchEvent).not.toHaveBeenCalled() resolveSpawn({ id: 'pty-slow' }) await expect(launch).resolves.toMatchObject({ ptyId: 'pty-slow' }) - expect(mockUpdateTabPtyId).toHaveBeenCalledWith('tab-1', 'pty-slow') + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + expect(mockCreateTab.mock.calls[0]?.[3]).toMatchObject({ + id: tabId, + initialPtyId: 'pty-slow' + }) + expect(mockUpdateTabPtyId).toHaveBeenCalledWith(tabId, 'pty-slow') expect(mockDispatchEvent).toHaveBeenCalledWith( - expect.objectContaining({ detail: { worktreeId: 'wt-1', tabIds: ['tab-1'] } }) + expect.objectContaining({ detail: { worktreeId: 'wt-1', tabIds: [tabId] } }) ) }) - it('kills a local PTY when its tab closes before spawn resolves', async () => { + it('kills a local PTY when its worktree disappears before spawn resolves', async () => { let resolveSpawn!: (result: { id: string }) => void mockSpawn.mockReturnValueOnce( new Promise<{ id: string }>((resolve) => { @@ -198,64 +213,66 @@ describe('launchAgentBackgroundSession', () => { worktreeId: 'wt-1', prompt: 'run slowly' }) - await vi.waitFor(() => expect(mockCreateTab).toHaveBeenCalledOnce()) - state.tabsByWorktree['wt-1'] = [] + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledOnce()) + state.worktreesByRepo['repo-1'] = [] resolveSpawn({ id: 'pty-after-close' }) await expect(launch).resolves.toBeNull() expect(mockKill).toHaveBeenCalledWith('pty-after-close') + expect(mockCreateTab).not.toHaveBeenCalled() expect(mockUpdateTabPtyId).not.toHaveBeenCalled() expect(mockSubscribeToPtyData).not.toHaveBeenCalled() expect(mockDispatchEvent).not.toHaveBeenCalled() }) - it('closes a runtime terminal when its tab closes before creation resolves', async () => { - useRemoteAgentBackgroundRuntime(state) - let resolveCreate!: (result: { - ok: true - result: { terminal: { handle: string; worktreeId: string; title: null } } - }) => void - const createResult = new Promise<{ - ok: true - result: { terminal: { handle: string; worktreeId: string; title: null } } - }>((resolve) => { - resolveCreate = resolve - }) - mockRuntimeEnvironmentCall.mockImplementation((args: { method: string }) => { - if (args.method === 'terminal.createAgentSession') { - return createResult - } - return Promise.resolve({ ok: true, result: {} }) - }) + it('launches into a folder workspace that is absent from worktreesByRepo throughout', async () => { + // Folder workspaces never appear in worktreesByRepo. + state.worktreesByRepo['repo-1'] = [] + state.folderWorkspaces = [ + { id: 'fw-1', projectGroupId: 'grp-1', folderPath: '/tmp/folder-workspace' } + ] + state.projectGroups = [{ id: 'grp-1', connectionId: null }] + state.getKnownWorktreeById = (worktreeId: string) => + worktreeId === 'folder:fw-1' + ? { id: 'folder:fw-1', path: '/tmp/folder-workspace' } + : undefined const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') const launch = launchAgentBackgroundSession({ agent: 'claude', - worktreeId: 'wt-1', - prompt: 'run remotely' - }) - await vi.waitFor(() => expect(mockCreateTab).toHaveBeenCalledOnce()) - await vi.waitFor(() => - expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith( - expect.objectContaining({ method: 'terminal.createAgentSession' }) - ) - ) - state.tabsByWorktree['wt-1'] = [] - resolveCreate({ - ok: true, - result: { terminal: { handle: 'terminal-after-close', worktreeId: 'wt-1', title: null } } + worktreeId: 'folder:fw-1', + prompt: 'run in a folder workspace' }) - await expect(launch).resolves.toBeNull() - expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith( + await expect(launch).resolves.toMatchObject({ ptyId: 'pty-1' }) + expect(mockKill).not.toHaveBeenCalled() + expect(mockCreateTab).toHaveBeenCalledOnce() + }) + + it('launches a local WSL folder through wsl.exe', async () => { + const folderPath = '\\\\wsl.localhost\\Ubuntu\\home\\me\\project' + state.worktreesByRepo['repo-1'] = [] + state.folderWorkspaces = [ + { id: 'fw-wsl', projectGroupId: 'grp-wsl', folderPath, connectionId: null } + ] + state.projectGroups = [{ id: 'grp-wsl', connectionId: null }] + state.getKnownWorktreeById = (worktreeId: string) => + worktreeId === 'folder:fw-wsl' ? { id: worktreeId, path: folderPath } : undefined + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'folder:fw-wsl', + prompt: 'run the automation' + }) + + expect(mockSpawn).toHaveBeenCalledWith( expect.objectContaining({ - method: 'terminal.close', - params: { terminal: 'terminal-after-close' } + cwd: folderPath, + shellOverride: 'wsl.exe', + command: "claude '--dangerously-skip-permissions' 'run the automation'" }) ) - expect(mockUpdateTabPtyId).not.toHaveBeenCalled() - expect(mockRuntimeEnvironmentSubscribe).not.toHaveBeenCalled() - expect(mockDispatchEvent).not.toHaveBeenCalled() }) it('records effective launch config returned by local PTY spawn', async () => { @@ -274,11 +291,12 @@ describe('launchAgentBackgroundSession', () => { }) const paneKey = expectStableAgentBackgroundPaneSpawn(mockSpawn) - const leafId = paneKey.slice('tab-1:'.length) + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + const leafId = paneKey.slice(`${tabId}:`.length) expect(mockRegisterAgentLaunchConfig).toHaveBeenLastCalledWith(paneKey, effectiveLaunchConfig, { agentType: 'claude', launchToken: mockSpawn.mock.calls[0]?.[0].env.ORCA_AGENT_LAUNCH_TOKEN, - tabId: 'tab-1', + tabId, leafId }) }) @@ -317,7 +335,7 @@ describe('launchAgentBackgroundSession', () => { command: "claude '--dangerously-skip-permissions' 'don'\\''t use powershell quoting'", connectionId: null, worktreeId: 'wt-1', - tabId: 'tab-1' + tabId: expect.stringMatching(UUID_RE) }) ) }) @@ -440,13 +458,16 @@ describe('launchAgentBackgroundSession', () => { const sidecar = mockSubscribeToPtyExit.mock.calls[0]?.[1] as (code: number) => void sidecar(0) - expect(state.clearTabPtyId).toHaveBeenCalledWith('tab-1', 'pty-1') - expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith(expect.stringMatching(/^tab-1:/)) + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + expect(state.clearTabPtyId).toHaveBeenCalledWith(tabId, 'pty-1') + expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith( + expect.stringMatching(new RegExp(`^${tabId}:`)) + ) expect(onExit).toHaveBeenCalledWith('pty-1', 0) expect(unsubscribe).toHaveBeenCalled() }) - it('removes the inactive tab if PTY spawn fails', async () => { + it('leaves no tab behind if PTY spawn fails', async () => { mockSpawn.mockRejectedValueOnce(new Error('spawn failed')) const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') @@ -458,11 +479,63 @@ describe('launchAgentBackgroundSession', () => { }) ).rejects.toThrow('spawn failed') - expect(mockCloseTab).toHaveBeenCalledWith('tab-1', { + // Why: the tab is only created once a PTY is live, so a failed spawn has nothing to close. + expect(mockCreateTab).not.toHaveBeenCalled() + expect(mockCloseTab).not.toHaveBeenCalled() + expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith( + expect.stringMatching(new RegExp(`^${expectReservedAgentBackgroundTabId(mockSpawn)}:`)) + ) + expect(mockUpdateTabPtyId).not.toHaveBeenCalled() + }) + + it('closes the adopted tab if binding fails after the PTY is live', async () => { + mockSubscribeToPtyData.mockImplementationOnce(() => { + throw new Error('subscribe failed') + }) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await expect( + launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run the automation' + }) + ).rejects.toThrow('subscribe failed') + + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + expect(mockCloseTab).toHaveBeenCalledWith(tabId, { recordInteraction: false, reason: 'cleanup' }) - expect(mockUpdateTabPtyId).not.toHaveBeenCalled() + expect(mockKill).toHaveBeenCalledWith('pty-1') + }) + + it('retires the launch instead of adopting a colliding tab id', async () => { + mockSpawn.mockImplementationOnce((args: { tabId: string }) => { + currentStoreState = { + ...state, + tabsByWorktree: { + ...state.tabsByWorktree, + 'wt-1': [{ id: args.tabId, title: 'Squatter' }] + } + } + return Promise.resolve({ id: 'pty-1' }) + }) + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + await expect( + launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'wt-1', + prompt: 'run the automation' + }) + ).resolves.toBeNull() + + expect(mockCreateTab).not.toHaveBeenCalled() + expect(mockKill).toHaveBeenCalledWith('pty-1') + expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith( + expectStableAgentBackgroundPaneSpawn(mockSpawn) + ) }) it('submits prompts for stdin-after-start agents in background mode', async () => { @@ -479,7 +552,7 @@ describe('launchAgentBackgroundSession', () => { ) expect(mockPasteDraftWhenAgentReady).toHaveBeenCalledWith( expect.objectContaining({ - tabId: 'tab-1', + tabId: expectReservedAgentBackgroundTabId(mockSpawn), content: 'run the automation', agent: 'aider', submit: true @@ -523,299 +596,4 @@ describe('launchAgentBackgroundSession', () => { }) ) }) - - it('forwards Hermes startup queries through SSH command transport', async () => { - state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await launchAgentBackgroundSession({ - agent: 'hermes', - worktreeId: 'wt-1', - prompt: 'remote automation prompt' - }) - - expect(mockSpawn).toHaveBeenCalledWith( - expect.objectContaining({ - command: expect.stringContaining('ORCA_HERMES_STARTUP_QUERY'), - connectionId: 'ssh-1', - env: expect.objectContaining({ ORCA_HERMES_STARTUP_QUERY: 'remote automation prompt' }) - }) - ) - }) - - it('injects fast startup commands into SSH background sessions after shell output arrives', async () => { - vi.useFakeTimers() - try { - state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await launchAgentBackgroundSession({ - agent: 'claude', - worktreeId: 'wt-1', - prompt: 'run the automation', - title: 'Nightly audit' - }) - - expect(mockSpawn.mock.calls[0]?.[0]?.command).toBe( - "claude '--dangerously-skip-permissions' 'run the automation'" - ) - expect(mockSpawn.mock.calls[0]?.[0]?.startupCommandDelivery).toBeUndefined() - const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void - dataSidecar('user@remote repo % ') - vi.advanceTimersByTime(50) - - expect(mockWrite).toHaveBeenCalledWith( - 'pty-1', - "claude '--dangerously-skip-permissions' 'run the automation'\r" - ) - } finally { - vi.useRealTimers() - } - }) - - it('waits for shell-ready before injecting payload-bearing SSH background commands', async () => { - vi.useFakeTimers() - try { - state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await launchAgentBackgroundSession({ - agent: 'codex', - worktreeId: 'wt-1', - prompt: 'run the automation', - title: 'Nightly audit' - }) - - expect(mockSpawn.mock.calls[0]?.[0]).toEqual( - expect.objectContaining({ - command: "codex '--dangerously-bypass-approvals-and-sandbox' 'run the automation'", - startupCommandDelivery: 'shell-ready' - }) - ) - const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void - dataSidecar('user@remote repo % ') - vi.advanceTimersByTime(50) - expect(mockWrite).not.toHaveBeenCalled() - - dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') - vi.advanceTimersByTime(50) - - expect(mockWrite).toHaveBeenCalledWith( - 'pty-1', - "codex '--dangerously-bypass-approvals-and-sandbox' 'run the automation'\r" - ) - } finally { - vi.useRealTimers() - } - }) - - it('waits for shell-ready for SSH background Codex native prefill commands without a hint', async () => { - vi.useFakeTimers() - try { - state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] - state.settings = { - agentCmdOverrides: { codex: "codex --prefill 'draft from override'" }, - activeRuntimeEnvironmentId: null, - terminalMainSideEffectAuthority: undefined - } - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await launchAgentBackgroundSession({ - agent: 'codex', - worktreeId: 'wt-1', - title: 'Nightly audit' - }) - - expect(mockSpawn.mock.calls[0]?.[0]).toEqual( - expect.objectContaining({ - command: - "codex --prefill 'draft from override' '--dangerously-bypass-approvals-and-sandbox'" - }) - ) - expect(mockSpawn.mock.calls[0]?.[0]).not.toHaveProperty('startupCommandDelivery') - const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void - dataSidecar('user@remote repo % ') - vi.advanceTimersByTime(50) - expect(mockWrite).not.toHaveBeenCalled() - - dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') - vi.advanceTimersByTime(50) - - expect(mockWrite).toHaveBeenCalledWith( - 'pty-1', - "codex --prefill 'draft from override' '--dangerously-bypass-approvals-and-sandbox'\r" - ) - } finally { - vi.useRealTimers() - } - }) - - it('does not rearm SSH background startup delivery after exit cleanup', async () => { - vi.useFakeTimers() - try { - state.repos = [{ id: 'repo-1', connectionId: 'ssh-1', path: '/repo' }] - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await launchAgentBackgroundSession({ - agent: 'codex', - worktreeId: 'wt-1', - prompt: 'run the automation', - title: 'Nightly audit' - }) - - const dataSidecar = mockSubscribeToPtyData.mock.calls[0]?.[1] as (data: string) => void - const exitSidecar = mockSubscribeToPtyExit.mock.calls[0]?.[1] as (code: number) => void - exitSidecar(0) - - dataSidecar('\x1b]777;orca-shell-ready\x07user@remote repo % ') - vi.advanceTimersByTime(50) - - expect(mockWrite).not.toHaveBeenCalled() - } finally { - vi.useRealTimers() - } - }) - - it('creates background sessions on the active runtime environment', async () => { - useRemoteAgentBackgroundRuntime(state) - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - const result = await launchAgentBackgroundSession({ - agent: 'claude', - worktreeId: 'wt-1', - prompt: 'run the automation' - }) - - expect(mockSpawn).not.toHaveBeenCalled() - const params = mockRuntimeEnvironmentCall.mock.calls[0]?.[0]?.params - const leafId = params?.placement?.leafId - expect(leafId).toMatch(UUID_RE) - // Why: background launches have no explicit recipe override, so remote host settings win. - expect(params).not.toHaveProperty('agentArgs') - expect(mockRegisterAgentLaunchConfig).toHaveBeenCalledWith( - `tab-1:${leafId}`, - { - agentCommand: "claude '--dangerously-skip-permissions'", - agentArgs: '--dangerously-skip-permissions', - agentEnv: {} - }, - { - agentType: 'claude', - launchToken: expect.stringMatching(UUID_RE), - tabId: 'tab-1', - leafId - } - ) - expect(mockSetTabLayout).toHaveBeenCalledWith( - 'tab-1', - expect.objectContaining({ - root: { type: 'leaf', leafId }, - activeLeafId: leafId, - ptyIdsByLeafId: { [leafId]: 'remote:env-1@@terminal-1' } - }) - ) - expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith({ - selector: 'env-1', - method: 'terminal.createAgentSession', - params: expect.objectContaining({ - clientOperationId: expect.stringMatching(/^\d{13}-[0-9a-f]{32}$/), - worktree: 'id:wt-1', - agent: 'claude', - prompt: 'run the automation', - promptDelivery: 'auto-submit', - placement: { tabId: 'tab-1', leafId }, - presentation: 'background' - }), - timeoutMs: 15_000 - }) - expect(mockUpdateTabPtyId).toHaveBeenCalledWith('tab-1', 'remote:env-1@@terminal-1') - expect(mockRegisterEagerPtyBuffer).not.toHaveBeenCalled() - expect(mockRuntimeEnvironmentSubscribe).toHaveBeenCalledWith( - expect.objectContaining({ - selector: 'env-1', - method: 'terminal.multiplex', - params: {} - }), - expect.any(Object) - ) - expect(result).toMatchObject({ - tabId: 'tab-1', - paneKey: `tab-1:${leafId}`, - ptyId: 'remote:env-1@@terminal-1', - terminalOwnership: null - }) - }) - - it('preserves the legacy background spawn on an old remote host', async () => { - useRemoteAgentBackgroundRuntime(state) - mockRuntimeEnvironmentTransportCall.mockImplementation((request: { method: string }) => { - if (request.method === 'status.get') { - return Promise.resolve({ - id: 'status', - ok: true, - result: { - runtimeId: 'old-runtime', - graphStatus: 'ready', - runtimeProtocolVersion: 3, - minCompatibleRuntimeClientVersion: 2, - capabilities: [] - } - }) - } - return Promise.resolve({ - id: 'create', - ok: true, - result: { terminal: { handle: 'legacy-terminal-1' } } - }) - }) - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await expect( - launchAgentBackgroundSession({ - agent: 'claude', - worktreeId: 'wt-1', - prompt: 'run remotely' - }) - ).resolves.toMatchObject({ ptyId: 'remote:env-1@@legacy-terminal-1' }) - - expect(mockRuntimeEnvironmentTransportCall).toHaveBeenCalledWith( - expect.objectContaining({ - method: 'terminal.create', - params: expect.objectContaining({ - worktree: 'id:wt-1', - command: "claude '--dangerously-skip-permissions' 'run remotely'", - launchAgent: 'claude', - presentation: 'background' - }) - }) - ) - }) - - it('closes a created runtime terminal when its data subscription fails', async () => { - useRemoteAgentBackgroundRuntime(state) - mockRuntimeEnvironmentSubscribe.mockRejectedValueOnce(new Error('subscription failed')) - const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') - - await expect( - launchAgentBackgroundSession({ - agent: 'claude', - worktreeId: 'wt-1', - prompt: 'run the automation' - }) - ).rejects.toThrow('subscription failed') - - expect(mockRuntimeEnvironmentCall).toHaveBeenCalledWith({ - selector: 'env-1', - method: 'terminal.close', - params: { terminal: 'terminal-1' }, - timeoutMs: undefined - }) - expect(state.clearTabPtyId).toHaveBeenCalledWith('tab-1', 'remote:env-1@@terminal-1') - expect(state.clearAgentLaunchConfig).toHaveBeenCalledWith(expect.stringMatching(/^tab-1:/)) - expect(mockCloseTab).toHaveBeenCalledWith('tab-1', { - recordInteraction: false, - reason: 'cleanup' - }) - expect(mockDispatchEvent).not.toHaveBeenCalled() - }) }) diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index 9ab4a5e684d..f65204d6558 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -4,18 +4,15 @@ import type { LaunchAgentBackgroundSessionArgs, LaunchAgentBackgroundSessionResult } from '@/lib/agent-background-session-contract' -import { getAgentLaunchPlatformForRepo } from '@/lib/agent-launch-platform' -import { CLIENT_PLATFORM } from '@/lib/new-workspace' import { tuiAgentToAgentKind } from '@/lib/telemetry' import { scheduleAgentBackgroundDraft } from '@/lib/agent-background-draft-delivery' -import { getLocalProjectExecutionRuntimeContext } from '@/lib/local-preflight-context' import { requestBackgroundTerminalWorktreeMount } from '@/components/terminal/background-terminal-worktree-mount' import { resolveTuiAgentLaunchArgs, resolveTuiAgentLaunchEnv } from '../../../shared/tui-agent-launch-defaults' import { TUI_AGENT_CONFIG } from '../../../shared/tui-agent-config' -import { repoIsRemote } from '../../../shared/agent-launch-remote' +import { resolveAgentBackgroundLaunchHost } from '@/lib/agent-background-session-launch-host' import { makePaneKey } from '../../../shared/stable-pane-id' import { registerEagerPtyBuffer, @@ -25,9 +22,7 @@ import { import { subscribeToPtyData } from '@/components/terminal-pane/pty-data-sidecar-subscriptions' import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { getSettingsForWorktreeRuntimeOwner } from '@/lib/worktree-runtime-owner' -import { singlePaneLayoutSnapshot } from '@/store/slices/terminal-helpers' -import { retireProvider, retireUnownedTerminal } from '@/lib/retire-unowned-background-terminal' -import { createBrowserUuid } from '@/lib/browser-uuid' +import { retireProvider } from '@/lib/retire-unowned-background-terminal' import { createRuntimeAgentBackgroundTerminal } from '@/lib/runtime-agent-background-create' import { subscribeToRuntimeTerminalData, @@ -38,42 +33,48 @@ import { shouldUseShellReadyStartupDelivery } from '../../../shared/codex-startu import { isMainTerminalSideEffectAuthorityForPty } from '@/components/terminal-pane/terminal-side-effect-facts-handler' import { resolveLocalWindowsAgentStartupShell } from '../../../shared/windows-terminal-shell' import { runBestEffortAgentBackgroundCleanups } from '@/lib/agent-background-session-cleanup' -import { bindAutomationTerminal } from '@/lib/automation-terminal-ownership' +import type { bindAutomationTerminal } from '@/lib/automation-terminal-ownership' +import { + adoptAgentBackgroundSessionTab, + reserveAgentBackgroundSessionIdentity +} from '@/lib/adopt-agent-background-session-tab' import { createBackgroundAgentStatusConsumer } from '@/lib/background-agent-status-consumer' +import { isWslUncPath } from '../../../shared/wsl-paths' export async function launchAgentBackgroundSession( args: LaunchAgentBackgroundSessionArgs ): Promise { const { agent, worktreeId, prompt, launchSource, title, onData, onExit, onAgentStatus } = args const store = useAppStore.getState() - const worktree = store.allWorktrees().find((entry) => entry.id === worktreeId) + // Folder workspaces exist only in getKnownWorktreeById (#2989). + const worktree = store.getKnownWorktreeById(worktreeId) const repo = worktree ? store.repos.find((entry) => entry.id === worktree.repoId) : null if (!worktree) { throw new Error('The target workspace is no longer available.') } + const cmdOverrides = store.settings?.agentCmdOverrides ?? {} + const agentArgs = resolveTuiAgentLaunchArgs(agent, store.settings?.agentDefaultArgs) + const agentEnv = resolveTuiAgentLaunchEnv(agent, store.settings?.agentDefaultEnv) + // Folder launch ownership cannot be derived from a repo row (#2989). + const launchHost = resolveAgentBackgroundLaunchHost({ + store, + worktreeId, + worktreePath: worktree.path, + repo + }) const preflight = TUI_AGENT_CONFIG[agent].preflightTrust if (preflight && worktree.path && window.api.agentTrust?.markTrusted) { try { await window.api.agentTrust.markTrusted({ preset: preflight, - workspacePath: worktree.path + workspacePath: worktree.path, + ...(launchHost.connectionId ? { connectionId: launchHost.connectionId } : {}) }) } catch { - // Best-effort: continue with launch. The user can still accept the trust menu. + // Best-effort: the user can still accept the trust prompt. } } - const cmdOverrides = store.settings?.agentCmdOverrides ?? {} - const agentArgs = resolveTuiAgentLaunchArgs(agent, store.settings?.agentDefaultArgs) - const agentEnv = resolveTuiAgentLaunchEnv(agent, store.settings?.agentDefaultEnv) - const launchPlatform = repo - ? getAgentLaunchPlatformForRepo( - repo, - repo.connectionId ? undefined : getLocalProjectExecutionRuntimeContext(store, worktreeId) - ) - : CLIENT_PLATFORM - // Why: SSH remotes deploy the CLI shim as plain `orca`, so the Linux-only - // `orca-ide` rename must not be applied for remote launches. - const isRemote = repo ? repoIsRemote(repo) : false + const { platform: launchPlatform, isRemote } = launchHost const startupShell = resolveLocalWindowsAgentStartupShell({ platform: launchPlatform, isRemote, @@ -99,37 +100,17 @@ export async function launchAgentBackgroundSession( return null } - // Why: automation runs should start without revealing the workspace. - // Spawn the PTY immediately, then attach an inactive tab to the live session. - const tab = store.createTab(worktreeId, undefined, undefined, { - activate: false, - recordInteraction: false - }) - // Why: agent hook callbacks are keyed by pane, and background automation - // tabs never mount a TerminalPane to inject this env for us. createBrowserUuid - // (not crypto.randomUUID) because the latter is undefined in non-secure - // browser contexts — the LAN web client served over plain HTTP. - const leafId = createBrowserUuid() - const paneKey = makePaneKey(tab.id, leafId) - const launchToken = createBrowserUuid() - const launchRegistration = { - agentType: agent, - launchToken, - tabId: tab.id, - leafId - } - store.registerAgentLaunchConfig(paneKey, startupPlan.launchConfig, launchRegistration) - // Why: `title` labels the tab/worktree entry. Pane titles render as an - // in-terminal title row, so background sessions must not persist it there. - store.setTabLayout(tab.id, singlePaneLayoutSnapshot(leafId)) - const paneEnv = { - ...startupPlan.env, - ORCA_PANE_KEY: paneKey, - ORCA_TAB_ID: tab.id, - ORCA_WORKTREE_ID: worktreeId, - ORCA_AGENT_LAUNCH_TOKEN: launchToken - } - const sshConnectionId = repo?.connectionId ?? null + // A hidden run tab must never be store-visible without its PTY (#2989). + const { reservedTabId, leafId, launchToken, launchRegistration, paneEnv } = + reserveAgentBackgroundSessionIdentity({ + store, + agentType: agent, + worktreeId, + launchConfig: startupPlan.launchConfig, + env: startupPlan.env + }) + let paneKey = makePaneKey(reservedTabId, leafId) + const sshConnectionId = launchHost.connectionId const sshStartupDelivery = createSshBackgroundStartupDelivery({ command: sshConnectionId ? startupPlan.launchCommand : null, waitForShellReady: @@ -147,6 +128,7 @@ export async function launchAgentBackgroundSession( let ptyId = '', runtimeTerminalHandle: string | null = null let returnedLaunchConfig: typeof startupPlan.launchConfig | undefined + let tab: ReturnType | null = null let exitHandled = false, eagerPtyBuffer: EagerPtyHandle | null = null let terminalOwnership: ReturnType = null @@ -160,7 +142,9 @@ export async function launchAgentBackgroundSession( unsubscribeExit() unsubscribeData() sshStartupDelivery.clear() - useAppStore.getState().clearTabPtyId(tab.id, exitPtyId) + if (tab) { + useAppStore.getState().clearTabPtyId(tab.id, exitPtyId) + } useAppStore.getState().clearAgentLaunchConfig(paneKey) onExit?.(exitPtyId, code) } @@ -174,7 +158,7 @@ export async function launchAgentBackgroundSession( paneKey, launchToken, mainOwnsAgentStatusWrites, - expectedConnectionId: repo ? (repo.connectionId ?? null) : undefined, + expectedConnectionId: launchHost.expectedConnectionId, runtimeEnvironmentId: runtimeTarget.kind === 'environment' ? runtimeTarget.environmentId : null, getPtyId: () => ptyId, onAgentStatus @@ -192,7 +176,7 @@ export async function launchAgentBackgroundSession( const created = await createRuntimeAgentBackgroundTerminal({ environmentId: runtimeTarget.environmentId, worktreeId, - tabId: tab.id, + tabId: reservedTabId, leafId, agent, ...(hasPrompt && !isFollowupPath ? { prompt: trimmedPrompt } : {}), @@ -216,6 +200,7 @@ export async function launchAgentBackgroundSession( rows: 40, cwd: worktree.path, command: startupPlan.launchCommand, + ...(!sshConnectionId && isWslUncPath(worktree.path) ? { shellOverride: 'wsl.exe' } : {}), ...(!startupPlan.startupCommandDelivery ? {} : { startupCommandDelivery: startupPlan.startupCommandDelivery }), @@ -225,7 +210,7 @@ export async function launchAgentBackgroundSession( launchAgent: agent, connectionId: sshConnectionId, worktreeId, - tabId: tab.id, + tabId: reservedTabId, leafId, telemetry: { agent_kind: tuiAgentToAgentKind(agent), @@ -236,25 +221,29 @@ export async function launchAgentBackgroundSession( ptyId = result.id returnedLaunchConfig = result.launchConfig } - if ( - await retireUnownedTerminal({ - tabId: tab.id, - ptyId, - runtimeTarget, - runtimeTerminalHandle, - onRetire: () => { - exitHandled = true - sshStartupDelivery.clear() - store.clearAgentLaunchConfig(paneKey) - } - }) - ) { + const adopted = await adoptAgentBackgroundSessionTab({ + store, + worktreeId, + reservedTabId, + ptyId, + paneKey, + launchConfig: returnedLaunchConfig ?? startupPlan.launchConfig, + launchRegistration, + runtimeTarget, + runtimeTerminalHandle, + onRetire: () => { + exitHandled = true + sshStartupDelivery.clear() + store.clearAgentLaunchConfig(paneKey) + }, + ...(title ? { title } : {}) + }) + if (!adopted) { return null } - if (returnedLaunchConfig) { - store.registerAgentLaunchConfig(paneKey, returnedLaunchConfig, launchRegistration) - } - terminalOwnership = bindAutomationTerminal(tab, paneKey, ptyId, runtimeTarget.kind, title) + tab = adopted.tab + paneKey = adopted.paneKey + terminalOwnership = adopted.terminalOwnership if (agent === 'command-code' && hasPrompt && !isFollowupPath) { // Why: Command Code does not expose a prompt-start hook; seed working for // hidden prompt launches so sidebar/activity surfaces do not stay idle. @@ -297,6 +286,7 @@ export async function launchAgentBackgroundSession( // alive regardless of whether the tab is hidden or mounted. unsubscribeExit = subscribeToPtyExit(ptyId, (code) => handleExit(ptyId, code)) } + sshStartupDelivery.armFallback(ptyId) // Why: bind the explicit PTY and ownership before mount; an earlier mount // can double-spawn, while later tracking can miss user takeover. @@ -312,19 +302,23 @@ export async function launchAgentBackgroundSession( // A failure between them must not strand an invisible runtime terminal. exitHandled = true terminalOwnership?.release() + const createdTab = tab runBestEffortAgentBackgroundCleanups(unsubscribeExit, unsubscribeData) runBestEffortAgentBackgroundCleanups(() => eagerPtyBuffer?.dispose()) runBestEffortAgentBackgroundCleanups(() => sshStartupDelivery.clear()) - runBestEffortAgentBackgroundCleanups(() => store.clearTabPtyId(tab.id, ptyId)) + if (createdTab) { + runBestEffortAgentBackgroundCleanups(() => store.clearTabPtyId(createdTab.id, ptyId)) + } runBestEffortAgentBackgroundCleanups(() => store.clearAgentLaunchConfig(paneKey)) if (ptyId) { await retireProvider({ ptyId, runtimeTarget, runtimeTerminalHandle }) } - // Why: a launch-failure cleanup close is not a user close — keep it out of - // the Cmd+Shift+T reopen stack. - runBestEffortAgentBackgroundCleanups(() => - store.closeTab(tab.id, { recordInteraction: false, reason: 'cleanup' }) - ) + if (createdTab) { + // Cleanup closes must not enter the reopen stack. + runBestEffortAgentBackgroundCleanups(() => + store.closeTab(createdTab.id, { recordInteraction: false, reason: 'cleanup' }) + ) + } throw error } } diff --git a/src/renderer/src/lib/launch-worktree-background-terminals.ts b/src/renderer/src/lib/launch-worktree-background-terminals.ts index 85a02598484..4c4c1603d0b 100644 --- a/src/renderer/src/lib/launch-worktree-background-terminals.ts +++ b/src/renderer/src/lib/launch-worktree-background-terminals.ts @@ -183,7 +183,7 @@ async function createBackgroundTab(args: { } if ( await retireUnownedTerminal({ - tabId: tab.id, + owner: { tabId: tab.id }, ptyId, runtimeTarget: { kind: 'local' } }) @@ -215,7 +215,7 @@ async function addSetupSplit(args: { }) if ( await retireUnownedTerminal({ - tabId: args.tab.tabId, + owner: { tabId: args.tab.tabId }, ptyId: setupPtyId, runtimeTarget: { kind: 'local' } }) diff --git a/src/renderer/src/lib/retire-unowned-background-terminal.ts b/src/renderer/src/lib/retire-unowned-background-terminal.ts index 7a7f128125a..40a0b0afb60 100644 --- a/src/renderer/src/lib/retire-unowned-background-terminal.ts +++ b/src/renderer/src/lib/retire-unowned-background-terminal.ts @@ -3,17 +3,24 @@ import { callRuntimeRpc, type RuntimeClientTarget } from '@/runtime/runtime-rpc- import { isTerminalTabPresent } from '@/store/slices/terminal-tab-retirement' export async function retireUnownedTerminal(args: { - tabId: string + /** Present tab id, or `{ worktreeId }` for a launch whose tab is created after the spawn. */ + owner: { tabId: string } | { worktreeId: string } ptyId: string runtimeTarget: RuntimeClientTarget runtimeTerminalHandle?: string | null onRetire?: () => void }): Promise { - if (isTerminalTabPresent(useAppStore.getState(), args.tabId)) { + const state = useAppStore.getState() + const owner = args.owner + const isOwned = + 'tabId' in owner + ? isTerminalTabPresent(state, owner.tabId) + : // Folder workspaces exist only in getKnownWorktreeById. + state.getKnownWorktreeById(owner.worktreeId) !== undefined + if (isOwned) { return false } - // Why: close can win while provider creation is in flight, before the - // returned handle is bindable to store state or visible to tab retirement. + // Close can win before the provider is bindable to store state. args.onRetire?.() await retireProvider(args) return true diff --git a/src/renderer/src/lib/ssh-background-startup-delivery.ts b/src/renderer/src/lib/ssh-background-startup-delivery.ts index eb178fd5964..f117ca447b7 100644 --- a/src/renderer/src/lib/ssh-background-startup-delivery.ts +++ b/src/renderer/src/lib/ssh-background-startup-delivery.ts @@ -14,6 +14,7 @@ type SshBackgroundStartupDeliveryOptions = { export type SshBackgroundStartupDelivery = { handleData(data: string): string + armFallback(ptyId: string): void schedule(ptyId: string): void clear(): void } @@ -51,22 +52,28 @@ export function createSshBackgroundStartupDelivery( } } + const armFallback = (ptyId: string): void => { + lastPtyId = ptyId + if (!pendingCommand || fallbackTimer !== null) { + return + } + fallbackTimer = setTimeout(() => { + fallbackTimer = null + startupShellReady = true + schedule(ptyId) + }, SSH_SHELL_READY_STARTUP_FALLBACK_MS) + } + const schedule = (ptyId: string): void => { lastPtyId = ptyId if (!pendingCommand) { return } if (!startupShellReady) { - if (fallbackTimer === null) { - // Why: hidden SSH sessions can use shells that cannot emit Orca's - // marker. Prefer readiness, but never drop the startup command forever. - fallbackTimer = setTimeout(() => { - fallbackTimer = null - markShellReady() - }, SSH_SHELL_READY_STARTUP_FALLBACK_MS) - } + armFallback(ptyId) return } + clearFallbackTimer() clearInjectTimer() injectTimer = setTimeout(() => { injectTimer = null @@ -102,6 +109,7 @@ export function createSshBackgroundStartupDelivery( } return scanned.output }, + armFallback, schedule, clear() { clearInjectTimer() From f48cb78646804bf8aa74b93f2f4fd097cf014606 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:09:31 -0700 Subject: [PATCH 03/16] perf(store): index worktrees and tabs once per hydration loop (#10891) * perf(store): index worktrees and tabs once per hydration loop Four sites in hydrateWorkspaceSession and reconnectPersistedTerminals rebuilt a flattened array inside a loop and linearly searched it: Object.values(worktreesByRepo).flat().find((e) => e.id === worktreeId) That is O(rows x ids) for O(rows + ids) distinct work. Build one first-wins index per loop instead. Neither loop sets state or awaits, so a single index over the store snapshot is valid for every iteration. 54.5x at a real 10-repo / 423-worktree session with 188 pending reconnects; 2.1x on a one-repo session. This runs synchronously on renderer cold start and gates workspaceSessionReady, which blocks terminal pane mounting. First-wins matters: Array.prototype.find returns the first match, so an index that overwrote on collision would resolve a different repo for a duplicated worktree id. Both the tests and the benchmark fixture carry a deliberate cross-repo duplicate so that difference is observable. Co-authored-by: Orca * test(store): count the generated worktree rows instead of multiplying Review feedback: the table printed repoCount * worktreesPerRepo, which misses the duplicate id makeStore injects for multi-repo cases (that duplicate is what makes first-wins observable). Count the generated map, and say plainly that the fixtures are synthetic at real-world scale rather than a replay of a real session. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../hydrate-worktree-lookup-benchmark.mjs | 178 ++++++++++++++++++ src/renderer/src/store/slices/terminals.ts | 44 ++--- .../store/slices/worktree-by-id-index.test.ts | 85 +++++++++ .../src/store/slices/worktree-by-id-index.ts | 34 ++++ 4 files changed, 319 insertions(+), 22 deletions(-) create mode 100644 config/scripts/hydrate-worktree-lookup-benchmark.mjs create mode 100644 src/renderer/src/store/slices/worktree-by-id-index.test.ts create mode 100644 src/renderer/src/store/slices/worktree-by-id-index.ts diff --git a/config/scripts/hydrate-worktree-lookup-benchmark.mjs b/config/scripts/hydrate-worktree-lookup-benchmark.mjs new file mode 100644 index 00000000000..76b737501e3 --- /dev/null +++ b/config/scripts/hydrate-worktree-lookup-benchmark.mjs @@ -0,0 +1,178 @@ +#!/usr/bin/env node +// Benchmark: the per-id worktree/tab lookups in session hydration and terminal reconnect. +// +// Four sites in store/slices/terminals.ts re-flattened worktreesByRepo (or tabsByWorktree) +// and linearly searched it once per loop iteration -- O(rows x ids) for O(rows + ids) +// distinct work. The fix builds one first-wins index per loop. +// +// This runs on the renderer's synchronous cold-start path and gates workspaceSessionReady, +// which blocks terminal pane mounting, so the cost is paid before the first frame. +// +// Both arms produce the resolved rows and are compared for equality before timing, so an +// index that resolved differently could not be reported as a win. +// +// Run with: node config/scripts/hydrate-worktree-lookup-benchmark.mjs +import { performance } from 'node:perf_hooks' + +const ITERATIONS = Number(process.env.ORCA_HYDRATE_BENCH_ITERATIONS ?? '60') +const WARMUP = Number(process.env.ORCA_HYDRATE_BENCH_WARMUP ?? '10') +const ROUNDS = 6 + +for (const [name, value] of [ + ['ORCA_HYDRATE_BENCH_ITERATIONS', ITERATIONS], + ['ORCA_HYDRATE_BENCH_WARMUP', WARMUP] +]) { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer, received ${value}`) + } +} + +// Pre-fix: re-flatten and linear-search per id. +function resolveByFlatten(worktreesByRepo, ids) { + const resolved = [] + for (const id of ids) { + const worktree = Object.values(worktreesByRepo) + .flat() + .find((entry) => entry.id === id) + resolved.push(worktree ? worktree.repoId : null) + } + return resolved +} + +// Post-fix: mirrors buildWorktreeByIdIndex in store/slices/worktree-by-id-index.ts. +function resolveByIndex(worktreesByRepo, ids) { + const index = new Map() + for (const worktrees of Object.values(worktreesByRepo)) { + for (const worktree of worktrees) { + if (!index.has(worktree.id)) { + index.set(worktree.id, worktree) + } + } + } + const resolved = [] + for (const id of ids) { + const worktree = index.get(id) + resolved.push(worktree ? worktree.repoId : null) + } + return resolved +} + +function makeStore(repoCount, worktreesPerRepo) { + const worktreesByRepo = {} + for (let repo = 0; repo < repoCount; repo += 1) { + const repoId = `repo-${repo}` + worktreesByRepo[repoId] = Array.from({ length: worktreesPerRepo }, (_value, index) => ({ + id: `${repoId}/wt-${index}`, + repoId, + path: `/Users/dev/worktrees/${repoId}/wt-${index}`, + branch: `feature/branch-${index}` + })) + } + // Why a deliberate duplicate: `.find()` is first-wins, so an index that overwrote on + // collision would resolve a different repo. Without a collision in the fixture that + // difference is unobservable and the equality check below would pass a broken index. + if (repoCount > 1) { + const [firstRepo, secondRepo] = Object.keys(worktreesByRepo) + worktreesByRepo[secondRepo] = [ + { ...worktreesByRepo[firstRepo][0], repoId: secondRepo }, + ...worktreesByRepo[secondRepo] + ] + } + return worktreesByRepo +} + +// Why a miss fraction: SSH worktrees are absent from worktreesByRepo at cold start, so +// the real workload includes ids that scan the whole list without matching -- the worst +// case for the linear arm, and the one the code comments call out explicitly. +function makeIds(worktreesByRepo, count) { + const all = Object.values(worktreesByRepo).flat() + const ids = Array.from({ length: count }, (_value, index) => + index % 7 === 0 ? `absent/wt-${index}` : all[(index * 31) % all.length].id + ) + // Always look up the duplicated id, so first-wins is exercised, not just present. + ids[1] = all[0].id + return ids +} + +function timeArm(resolve, worktreesByRepo, ids) { + let sink = 0 + const start = performance.now() + for (let index = 0; index < ITERATIONS; index += 1) { + // Consume the result so V8 cannot drop the call as dead. + sink += resolve(worktreesByRepo, ids).length + } + const elapsed = (performance.now() - start) / ITERATIONS + if (sink === -1) { + throw new Error('unreachable') + } + return elapsed +} + +function median(samples) { + const sorted = [...samples].sort((a, b) => a - b) + const mid = sorted.length / 2 + return (sorted[mid - 1] + sorted[mid]) / 2 +} + +// Arms alternate which one leads so within-round drift cannot favour either. +function measure(worktreesByRepo, ids) { + for (let index = 0; index < WARMUP; index += 1) { + resolveByFlatten(worktreesByRepo, ids) + resolveByIndex(worktreesByRepo, ids) + } + const flattenSamples = [] + const indexSamples = [] + for (let round = 0; round < ROUNDS; round += 1) { + if (round % 2 === 0) { + flattenSamples.push(timeArm(resolveByFlatten, worktreesByRepo, ids)) + indexSamples.push(timeArm(resolveByIndex, worktreesByRepo, ids)) + } else { + indexSamples.push(timeArm(resolveByIndex, worktreesByRepo, ids)) + flattenSamples.push(timeArm(resolveByFlatten, worktreesByRepo, ids)) + } + } + return { flattenMs: median(flattenSamples), indexMs: median(indexSamples) } +} + +const pad = (value, width) => String(value).padStart(width) +console.log('Session-hydration worktree lookup, per cold start. Lower is better.') +console.log(`iterations=${ITERATIONS} warmup=${WARMUP} rounds=${ROUNDS} (per-arm medians)`) +console.log( + `${pad('repos', 6)} ${pad('worktrees', 10)} ${pad('ids', 5)} ${pad('flatten', 11)} ${pad('indexed', 11)} ${pad('speedup', 9)}` +) + +// Row shapes are synthetic, sized against a real orca-data.json on a heavy machine +// (10 repos / 423 worktrees / 188 pending reconnect). They are not that dataset: the +// generator spreads worktrees evenly and injects one duplicate id, so treat the counts +// as "about this scale", not a replay. +for (const [repoCount, worktreesPerRepo, idCount] of [ + [1, 5, 5], + [3, 20, 20], + [10, 42, 188], + [10, 100, 400] +]) { + const worktreesByRepo = makeStore(repoCount, worktreesPerRepo) + const ids = makeIds(worktreesByRepo, idCount) + const flattenResult = resolveByFlatten(worktreesByRepo, ids) + const indexResult = resolveByIndex(worktreesByRepo, ids) + if (JSON.stringify(flattenResult) !== JSON.stringify(indexResult)) { + throw new Error(`resolution differs at ${repoCount} repos x ${worktreesPerRepo} worktrees`) + } + if (!flattenResult.some((value) => value !== null)) { + throw new Error(`fixture resolved nothing at ${repoCount} repos`) + } + if (!flattenResult.some((value) => value === null)) { + throw new Error(`fixture had no absent ids at ${repoCount} repos`) + } + // Count the generated rows rather than multiplying: makeStore injects a duplicate + // id for multi-repo cases, so the product would misreport the fixture by one. + const worktreeCount = Object.values(worktreesByRepo).reduce((sum, rows) => sum + rows.length, 0) + const { flattenMs, indexMs } = measure(worktreesByRepo, ids) + console.log( + `${pad(repoCount, 6)} ${pad(worktreeCount, 10)} ${pad(idCount, 5)} ${pad(`${flattenMs.toFixed(4)} ms`, 11)} ${pad(`${indexMs.toFixed(4)} ms`, 11)} ${pad(`${(flattenMs / indexMs).toFixed(1)}x`, 9)}` + ) +} + +console.log( + '\nFixtures are synthetic at real-world scale, not a replay of a real session.\nThis times one of the four lookup sites. A one-repo session sees almost nothing;\nthe win scales with worktrees x pending ids, and lands on the cold-start path that\ngates terminal pane mounting.' +) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 5c880618505..a0cfcea982f 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -34,6 +34,7 @@ import { parsePaneKey } from '../../../../shared/stable-pane-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../../../shared/terminal-tab-id' +import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' import { getRepoIdFromWorktreeId, splitWorktreeIdForFilesystem @@ -3210,13 +3211,13 @@ export const createTerminalSlice: StateCreator // Why: preserve each tab's prior ptyId so reconnect passes it as sessionId to the daemon's createOrAttach, triggering reattach instead of a fresh spawn. const pendingReconnectPtyIdByTabId: Record = {} + const placeholderWorktreeById = buildWorktreeByIdIndex( + runtimeSessionPlaceholders.worktreesByRepo + ) + const placeholderRepoById = buildByIdIndex(runtimeSessionPlaceholders.repos) for (const worktreeId of pendingReconnectWorktreeIds) { - const worktree = Object.values(runtimeSessionPlaceholders.worktreesByRepo) - .flat() - .find((entry) => entry.id === worktreeId) - const repo = worktree - ? runtimeSessionPlaceholders.repos.find((entry) => entry.id === worktree.repoId) - : null + const worktree = placeholderWorktreeById.get(worktreeId) + const repo = worktree ? placeholderRepoById.get(worktree.repoId) : null if (repo?.connectionId) { continue } @@ -3304,6 +3305,11 @@ export const createTerminalSlice: StateCreator nextEverActivated.add(activeWorktreeId) } + // Why indexed: the layout map below looks up a tab per persisted layout, and + // re-flattening tabsByWorktree per entry is O(tabs x layouts). + const allTabs = Object.values(tabsByWorktree).flat() + const tabById = buildByIdIndex(allTabs) + return { activeRepoId, activeWorktreeId, @@ -3328,11 +3334,7 @@ export const createTerminalSlice: StateCreator // Why: seed nav history with the hydrated active worktree so the first activation has a Back target; hydration bypasses recordWorktreeVisit, so otherwise Back stays disabled until a second click. worktreeNavHistory: activeWorktreeId ? [activeWorktreeId] : [], worktreeNavHistoryIndex: activeWorktreeId ? 0 : -1, - ptyIdsByTabId: Object.fromEntries( - Object.values(tabsByWorktree) - .flat() - .map((tab) => [tab.id, []] as const) - ), + ptyIdsByTabId: Object.fromEntries(allTabs.map((tab) => [tab.id, []] as const)), // Why: daemon ptyIds survive app restart; preserve ptyIdsByLeafId so reconnect can reattach each split-pane leaf to its own session, not just the tab-level ptyId. terminalLayoutsByTabId: Object.fromEntries( Object.entries(session.terminalLayoutsByTabId) @@ -3340,9 +3342,7 @@ export const createTerminalSlice: StateCreator .map(([tabId, layout]) => { // Why: old sessions can contain renderer-local pane:1-style leaf ids; normalize before runtime/mobile surfaces read them. const normalized = normalizeTerminalLayoutSnapshot(layout).snapshot - const tab = Object.values(tabsByWorktree) - .flat() - .find((entry) => entry.id === tabId) + const tab = tabById.get(tabId) const sanitized = tab ? sanitizeTerminalLayoutPaneTitles(normalized, tab) : normalized const activeLeafId = sanitized.root ? resolvePtyBoundActiveLeafId({ @@ -3382,12 +3382,14 @@ export const createTerminalSlice: StateCreator // Why: defer daemon createOrAttach to connectPanePty (real fitAddon dims) instead of eager-spawning at 80×24 and garbling on flush; this loop only records the session IDs to reattach. let reconnectedTabsByWorktree: Record | null = null let reconnectedPtyIdsByTabId: Record | null = null + // Why indexed: the loop neither sets state nor awaits, so one index over the + // whole store snapshot serves every iteration. + const worktreeById = buildWorktreeByIdIndex(get().worktreesByRepo) + const repoById = buildByIdIndex(get().repos) for (const worktreeId of ids) { const tabs = tabsByWorktree[worktreeId] ?? [] - const worktree = Object.values(get().worktreesByRepo) - .flat() - .find((entry) => entry.id === worktreeId) - const repo = worktree ? get().repos.find((entry) => entry.id === worktree.repoId) : null + const worktree = worktreeById.get(worktreeId) + const repo = worktree ? (repoById.get(worktree.repoId) ?? null) : null // Why: only allow deferred reattach when the SSH connection is active; reattaching to a not-yet-connected relay (deferred/passphrase targets) would fail. const sshState = repo?.connectionId ? get().sshConnectionStates.get(repo.connectionId) : null const sshConnected = repo?.connectionId != null && sshState?.status === 'connected' @@ -3441,12 +3443,10 @@ export const createTerminalSlice: StateCreator // Why: deferred SSH targets haven't connected yet, so their ptyIds weren't restored above; stash session IDs in a map that survives cleanup for pty-connection.ts's deferred reconnect. const deferredSshSessionIdsByTabId: Record = {} for (const worktreeId of ids) { - const worktree = Object.values(get().worktreesByRepo) - .flat() - .find((entry) => entry.id === worktreeId) + const worktree = worktreeById.get(worktreeId) // Why: SSH worktrees aren't in worktreesByRepo at cold start; fall back to the repo id in the composite worktree id so sessions still reach the deferred map. const repoId = worktree?.repoId ?? getRepoIdFromWorktreeId(worktreeId) - const repo = repoId ? get().repos.find((entry) => entry.id === repoId) : null + const repo = repoId ? (repoById.get(repoId) ?? null) : null if (!repo?.connectionId) { continue } diff --git a/src/renderer/src/store/slices/worktree-by-id-index.test.ts b/src/renderer/src/store/slices/worktree-by-id-index.test.ts new file mode 100644 index 00000000000..1c5d28e6295 --- /dev/null +++ b/src/renderer/src/store/slices/worktree-by-id-index.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from 'vitest' +import type { Worktree } from '../../../../shared/types' +import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' + +function worktree(id: string, repoId: string): Worktree { + return { id, repoId, path: `/repos/${repoId}/${id}`, branch: id } as Worktree +} + +// Reference: the pre-change lookup these indexes replace. +function findByFlatten(worktreesByRepo: Record, id: string): Worktree | null { + return ( + Object.values(worktreesByRepo) + .flat() + .find((entry) => entry.id === id) ?? null + ) +} + +describe('buildWorktreeByIdIndex', () => { + const worktreesByRepo = { + repoA: [worktree('wt-1', 'repoA'), worktree('wt-2', 'repoA')], + repoB: [worktree('wt-3', 'repoB')], + repoC: [] + } + + // Why: the index replaces a per-iteration flatten+find, so the contract that + // matters is that it resolves exactly what that find resolved. + it('resolves every id identically to flatten-and-find', () => { + for (const id of ['wt-1', 'wt-2', 'wt-3', 'missing', '']) { + expect(buildWorktreeByIdIndex(worktreesByRepo).get(id) ?? null).toEqual( + findByFlatten(worktreesByRepo, id) + ) + } + }) + + it('returns undefined for an id in no repo', () => { + expect(buildWorktreeByIdIndex(worktreesByRepo).get('nope')).toBeUndefined() + }) + + it('handles an empty map', () => { + expect(buildWorktreeByIdIndex({}).size).toBe(0) + }) + + it('skips repos with no worktrees', () => { + expect(buildWorktreeByIdIndex(worktreesByRepo).size).toBe(3) + }) + + // Why first-wins: Array.prototype.find returns the first match, and repo order is + // Object.values order. A duplicate id across repos must resolve the same way. + it('keeps the first entry when an id appears in two repos', () => { + const duplicated = { + repoA: [worktree('shared', 'repoA')], + repoB: [worktree('shared', 'repoB')] + } + expect(buildWorktreeByIdIndex(duplicated).get('shared')?.repoId).toBe('repoA') + expect(buildWorktreeByIdIndex(duplicated).get('shared')).toEqual( + findByFlatten(duplicated, 'shared') + ) + }) +}) + +describe('buildByIdIndex', () => { + it('resolves every id identically to find', () => { + const rows = [ + { id: 'a', n: 1 }, + { id: 'b', n: 2 }, + { id: 'c', n: 3 } + ] + for (const id of ['a', 'b', 'c', 'zzz']) { + expect(buildByIdIndex(rows).get(id)).toEqual(rows.find((row) => row.id === id)) + } + }) + + it('keeps the first row when ids repeat', () => { + const rows = [ + { id: 'dup', n: 1 }, + { id: 'dup', n: 2 } + ] + expect(buildByIdIndex(rows).get('dup')).toEqual(rows.find((row) => row.id === 'dup')) + expect(buildByIdIndex(rows).get('dup')?.n).toBe(1) + }) + + it('handles an empty list', () => { + expect(buildByIdIndex([]).size).toBe(0) + }) +}) diff --git a/src/renderer/src/store/slices/worktree-by-id-index.ts b/src/renderer/src/store/slices/worktree-by-id-index.ts new file mode 100644 index 00000000000..a7eaf17239e --- /dev/null +++ b/src/renderer/src/store/slices/worktree-by-id-index.ts @@ -0,0 +1,34 @@ +import type { Worktree } from '../../../../shared/types' + +/** + * Flattens worktreesByRepo into an id lookup. + * + * Why: session hydration and terminal reconnect loop over pending worktree ids and + * previously re-flattened + linear-searched the whole map per iteration, which is + * O(worktrees x ids) for O(worktrees + ids) distinct work. First entry wins, matching + * the `.find()` it replaces. + */ +export function buildWorktreeByIdIndex( + worktreesByRepo: Record +): Map { + const index = new Map() + for (const worktrees of Object.values(worktreesByRepo)) { + for (const worktree of worktrees) { + if (!index.has(worktree.id)) { + index.set(worktree.id, worktree) + } + } + } + return index +} + +/** Same first-wins contract as `buildWorktreeByIdIndex`, for any id-bearing row. */ +export function buildByIdIndex(rows: readonly T[]): Map { + const index = new Map() + for (const row of rows) { + if (!index.has(row.id)) { + index.set(row.id, row) + } + } + return index +} From 6943638053861ae8288673e8c90c9f2d9f75ac11 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:10:45 -0700 Subject: [PATCH 04/16] perf(terminal): scan output frames by code unit, not per code point (#10915) * perf(terminal): scan output frames by code unit, not per code point iterateTerminalOutputFrameChunks walked `for (const part of data)`, materializing a 1-2 character string per code point and calling terminalStreamByteLength on each, while accumulating the frame text with `chunk += part`. The accumulator was never needed: `chunk` only ever reconstructs the contiguous substring data[chunkStart..end), and `startSeq + chunkStartOffset + chunk.length` collapses algebraically to `startSeq + end`. Track two integer offsets and emit data.slice(chunkStart, end) instead, computing UTF-8 width inline from charCodeAt. Also short-circuits the cap gate on UTF-16 length before measuring UTF-8 bytes, which is sound because UTF-8 length is never below UTF-16 length. 2.4-6.1x across payload shapes, stable across reruns. This runs per terminal output batch and per snapshot chunk. Extracted to its own module along a real seam (the chunk-emission concern plus its two types and cap gate); methods/terminal.ts shrinks by 85 lines. No max-lines suppression added and the baseline is untouched. Co-authored-by: Orca * fix(terminal): preserve chunk sequence rounding Co-authored-by: Orca --------- Co-authored-by: Orca --- .../terminal-output-frame-chunk-benchmark.mjs | 347 +++++++++++++ src/main/runtime/rpc/methods/terminal.ts | 97 +--- ...al-output-frame-chunks-equivalence.test.ts | 477 ++++++++++++++++++ .../rpc/terminal-output-frame-chunks.ts | 113 +++++ 4 files changed, 943 insertions(+), 91 deletions(-) create mode 100644 config/scripts/terminal-output-frame-chunk-benchmark.mjs create mode 100644 src/main/runtime/rpc/terminal-output-frame-chunks-equivalence.test.ts create mode 100644 src/main/runtime/rpc/terminal-output-frame-chunks.ts diff --git a/config/scripts/terminal-output-frame-chunk-benchmark.mjs b/config/scripts/terminal-output-frame-chunk-benchmark.mjs new file mode 100644 index 00000000000..6dd6364524c --- /dev/null +++ b/config/scripts/terminal-output-frame-chunk-benchmark.mjs @@ -0,0 +1,347 @@ +#!/usr/bin/env node +// Benchmark: iterateTerminalOutputFrameChunks, which every byte of remote terminal +// output passes through on its way to a mobile/remote-desktop multiplex stream. +// +// The pre-fix loop was `for (const part of data)`: V8 materializes a fresh 1-2 code +// unit string per code point, then measureClipboardTextByteLength re-walked that +// string through codePointAt to get its UTF-8 width, and the chunk was rebuilt with +// `chunk += part`. The gate in front of it (terminalStreamByteLengthExceeds) ran the +// same per-code-point walk over the whole payload a second time. +// +// The fix: one charCodeAt scan computing the UTF-8 width inline with no per-code-point +// string, chunk text taken as data.slice(chunkStart, end), and a gate of +// `data.length > CHUNK || Buffer.byteLength(data, 'utf8') > CHUNK` -- sound because a +// string's UTF-8 length is never below its UTF-16 length. +// +// BOTH arms run the complete production path, encodeTerminalStreamText included, and +// their emitted frames (base64 + seq + opcode) are compared before any timing, so an +// arm that split differently or renumbered a seq cannot be reported as a win. +import { spawnSync } from 'node:child_process' +import { existsSync, readFileSync } from 'node:fs' +import nodeModule from 'node:module' +import { performance } from 'node:perf_hooks' +import { fileURLToPath } from 'node:url' + +// terminal-stream-protocol.ts declares a TS enum, which Node's default strip-only +// loader rejects; re-exec once with type transformation rather than re-modelling +// the opcodes here (a hand copy could drift from the wire contract). +if (!process.execArgv.includes('--experimental-transform-types')) { + const result = spawnSync( + process.execPath, + ['--experimental-transform-types', '--no-warnings', import.meta.filename], + { stdio: 'inherit' } + ) + process.exit(result.status ?? 1) +} + +// The app's TS sources import siblings without an extension; Node's ESM resolver needs it. +nodeModule.registerHooks({ + resolve(specifier, context, nextResolve) { + if (specifier.startsWith('.') && !/\.[cm]?[jt]s$/.test(specifier) && context.parentURL) { + const candidate = new URL(`${specifier}.ts`, context.parentURL) + if (existsSync(fileURLToPath(candidate))) { + return { url: candidate.href, shortCircuit: true } + } + } + return nextResolve(specifier, context) + } +}) + +const ITERATIONS = Number(process.env.ORCA_FRAME_CHUNK_BENCH_ITERATIONS ?? '40') +const WARMUP = Number(process.env.ORCA_FRAME_CHUNK_BENCH_WARMUP ?? '8') +const ROUNDS = Number(process.env.ORCA_FRAME_CHUNK_BENCH_ROUNDS ?? '6') + +for (const [name, value] of [ + ['ORCA_FRAME_CHUNK_BENCH_ITERATIONS', ITERATIONS], + ['ORCA_FRAME_CHUNK_BENCH_WARMUP', WARMUP], + ['ORCA_FRAME_CHUNK_BENCH_ROUNDS', ROUNDS] +]) { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer, received ${value}`) + } +} +if (ROUNDS % 2 !== 0) { + throw new Error(`ORCA_FRAME_CHUNK_BENCH_ROUNDS must be even so each arm leads equally`) +} + +const CHUNK_SOURCE = readFileSync( + new URL('../../src/main/runtime/rpc/terminal-output-frame-chunks.ts', import.meta.url), + 'utf8' +) +const CLIPBOARD_SOURCE = readFileSync( + new URL('../../src/shared/clipboard-text.ts', import.meta.url), + 'utf8' +) + +// Why re-read the sources: this benchmark's claim is that the OLD arm paid a +// measureClipboardTextByteLength call per code point and the NEW one pays a single +// Buffer.byteLength gate. Match the CALL form, not a bare word -- a comment naming +// the function would satisfy a substring check. +for (const [source, label, marker] of [ + [CHUNK_SOURCE, 'terminal-output-frame-chunks.ts', "Buffer.byteLength(data, 'utf8')"], + [CHUNK_SOURCE, 'terminal-output-frame-chunks.ts', 'const text = data.slice(chunkStart, end)'], + [CHUNK_SOURCE, 'terminal-output-frame-chunks.ts', 'data.charCodeAt(index + 1)'], + [CLIPBOARD_SOURCE, 'clipboard-text.ts', 'export function measureClipboardTextByteLength('], + [CLIPBOARD_SOURCE, 'clipboard-text.ts', 'text.codePointAt(index)'] +]) { + if (!source.includes(marker)) { + throw new Error(`${label} no longer contains \`${marker}\`; this benchmark is stale`) + } +} +if (CHUNK_SOURCE.includes('for (const part of data)')) { + throw new Error( + 'terminal-output-frame-chunks.ts still iterates code points as strings; this benchmark is stale' + ) +} + +const { TERMINAL_STREAM_CHUNK_BYTES } = await import( + new URL('../../src/shared/terminal-multiplex-flow-control.ts', import.meta.url).href +) +const { measureClipboardTextByteLength } = await import( + new URL('../../src/shared/clipboard-text.ts', import.meta.url).href +) +const { TerminalStreamOpcode, encodeTerminalStreamJson, encodeTerminalStreamText } = await import( + new URL('../../src/shared/terminal-stream-protocol.ts', import.meta.url).href +) +const { iterateTerminalOutputFrameChunks } = await import( + new URL('../../src/main/runtime/rpc/terminal-output-frame-chunks.ts', import.meta.url).href +) + +// Pre-fix arm: the exact code that shipped, including the second full walk in the gate. +function* iterateBefore(data, meta) { + const rawLength = meta?.rawLength ?? data.length + if (meta?.transformed || rawLength !== data.length) { + yield { + opcode: TerminalStreamOpcode.OutputSpan, + bytes: encodeTerminalStreamJson({ data, rawLength, transformed: true }), + seq: meta?.seq + } + return + } + if ( + !measureClipboardTextByteLength(data, { stopAfterBytes: TERMINAL_STREAM_CHUNK_BYTES }) + .exceededLimit + ) { + yield { bytes: encodeTerminalStreamText(data), seq: meta?.seq } + return + } + const canPreserveChunkSeq = typeof meta?.seq === 'number' && rawLength === data.length + const shouldDelayFinalSeq = !canPreserveChunkSeq && typeof meta?.seq === 'number' + const startSeq = canPreserveChunkSeq ? meta.seq - rawLength : undefined + let chunk = '' + let chunkBytes = 0 + let chunkStartOffset = 0 + let offset = 0 + let delayedChunk = null + + const takeChunk = () => { + if (!chunk) { + return null + } + const chunkSeq = canPreserveChunkSeq ? startSeq + chunkStartOffset + chunk.length : undefined + const current = { text: chunk, seq: chunkSeq } + chunk = '' + chunkBytes = 0 + chunkStartOffset = offset + return current + } + + for (const part of data) { + const partBytes = measureClipboardTextByteLength(part).byteLength + if (chunkBytes > 0 && chunkBytes + partBytes > TERMINAL_STREAM_CHUNK_BYTES) { + const nextChunk = takeChunk() + if (nextChunk) { + if (shouldDelayFinalSeq) { + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + delayedChunk = nextChunk + } else { + yield { bytes: encodeTerminalStreamText(nextChunk.text), seq: nextChunk.seq } + } + } + } + chunk += part + chunkBytes += partBytes + offset += part.length + } + const finalChunk = takeChunk() + if (shouldDelayFinalSeq) { + if (finalChunk) { + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + delayedChunk = finalChunk + } + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text), seq: meta.seq } + } + return + } + if (finalChunk) { + yield { bytes: encodeTerminalStreamText(finalChunk.text), seq: finalChunk.seq } + } +} + +// The multiplex stream consumes every frame's bytes/seq/opcode; charge both arms for it. +let frameChecksum = 0 +function drain(iterate, data, meta) { + let frames = 0 + let bytes = 0 + let seqSum = 0 + for (const frame of iterate(data, meta)) { + frames += 1 + bytes += frame.bytes.byteLength + seqSum += frame.seq ?? 0 + } + frameChecksum = Math.imul(frameChecksum ^ (frames + bytes + seqSum), 16777619) >>> 0 + return frames +} + +function describeFrames(iterate, data, meta) { + const shapes = [] + for (const frame of iterate(data, meta)) { + shapes.push( + `${Buffer.from(frame.bytes).toString('base64')}|${frame.seq ?? 'u'}|${frame.opcode ?? 'u'}` + ) + } + return shapes.join('\n') +} + +const SURROGATE_PAIR = '\u{1f600}' +const LONE_HIGH = '\ud83d' + +function repeatTo(unit, codeUnits) { + let out = '' + while (out.length < codeUnits) { + out += unit + } + return out.slice(0, out.length - (out.length % unit.length)) +} + +// A realistic agent-TUI line: SGR runs, a wide glyph, a currency sign, an emoji. +const TUI_LINE = + '\u001b[35m\u273b Thinking\u001b[0m about the \u20ac plan \u{1f600} 42 passed, 0 failed\r\n' + +const fixtures = [ + { + label: 'ascii 4KiB (typical batch)', + data: 'x'.repeat(4 * 1024), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + label: `ascii ${TERMINAL_STREAM_CHUNK_BYTES}B (at cap)`, + data: 'x'.repeat(TERMINAL_STREAM_CHUNK_BYTES), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + label: 'ascii 64KiB (batch cap, 2 chunks)', + data: 'x'.repeat(64 * 1024), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + label: 'mixed TUI 64KiB (2 chunks)', + data: repeatTo(TUI_LINE, 64 * 1024), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + // seq without an explicit rawLength: the batcher's ordinary shape. + label: 'mixed TUI 64KiB (implicit raw)', + data: repeatTo(TUI_LINE, 64 * 1024), + meta: () => ({ seq: 5_000_000 }) + }, + { + label: 'emoji 64KiB (4-byte, 2 chunks)', + data: repeatTo(SURROGATE_PAIR, 32 * 1024), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + label: 'lone surrogates 64KiB', + data: repeatTo(LONE_HIGH, 64 * 1024), + meta: (data) => ({ seq: 5_000_000, rawLength: data.length }) + }, + { + label: 'ascii 512KiB (snapshot chunking)', + data: 'x'.repeat(512 * 1024), + meta: () => undefined + } +] + +function median(samples) { + const sorted = [...samples].sort((a, b) => a - b) + const middle = Math.floor(sorted.length / 2) + return sorted.length % 2 === 0 ? (sorted[middle - 1] + sorted[middle]) / 2 : sorted[middle] +} + +// Why interleaved with an alternating lead: running one arm's whole batch first lets +// CPU frequency drift correlate with whichever arm is being measured. Elsewhere in this +// effort that alone reported 23.3x for a real 6.7x. +function measureInterleaved(data, meta) { + for (let index = 0; index < WARMUP; index += 1) { + drain(iterateBefore, data, meta) + drain(iterateTerminalOutputFrameChunks, data, meta) + } + const beforeSamples = [] + const afterSamples = [] + for (let round = 0; round < ROUNDS; round += 1) { + const runBefore = () => { + const start = performance.now() + for (let index = 0; index < ITERATIONS; index += 1) { + drain(iterateBefore, data, meta) + } + beforeSamples.push((performance.now() - start) / ITERATIONS) + } + const runAfter = () => { + const start = performance.now() + for (let index = 0; index < ITERATIONS; index += 1) { + drain(iterateTerminalOutputFrameChunks, data, meta) + } + afterSamples.push((performance.now() - start) / ITERATIONS) + } + if (round % 2 === 0) { + runBefore() + runAfter() + } else { + runAfter() + runBefore() + } + } + return { beforeMs: median(beforeSamples), afterMs: median(afterSamples) } +} + +const pad = (value, width) => String(value).padStart(width) +console.log('iterateTerminalOutputFrameChunks, per flushed batch. Lower is better.') +console.log( + `iterations=${ITERATIONS} warmup=${WARMUP} rounds=${ROUNDS} (alternating lead, per-arm median)` +) +console.log( + `${pad('fixture', 34)} ${pad('frames', 7)} ${pad('per-part', 11)} ${pad('scanned', 11)} ${pad('speedup', 9)}` +) + +let comparedFixtures = 0 +for (const fixture of fixtures) { + const meta = fixture.meta(fixture.data) + const before = describeFrames(iterateBefore, fixture.data, meta) + const after = describeFrames(iterateTerminalOutputFrameChunks, fixture.data, meta) + if (before !== after) { + throw new Error(`frames differ for ${fixture.label}`) + } + const frames = before.split('\n').length + // Guard against a fixture that never reaches the chunking loop; then both arms would + // just be measuring the gate and the comparison above would prove nothing about it. + if (fixture.data.length > TERMINAL_STREAM_CHUNK_BYTES && frames < 2) { + throw new Error(`${fixture.label} never split; fixture does not exercise the chunk loop`) + } + comparedFixtures += 1 + const { beforeMs, afterMs } = measureInterleaved(fixture.data, meta) + console.log( + `${pad(fixture.label, 34)} ${pad(frames, 7)} ${pad(`${beforeMs.toFixed(3)} ms`, 11)} ${pad(`${afterMs.toFixed(3)} ms`, 11)} ${pad(`${(beforeMs / afterMs).toFixed(1)}x`, 9)}` + ) +} + +console.log( + `\nvalidated=${comparedFixtures} fixtures frame-identical before timing, checksum=${frameChecksum >>> 0}` +) +console.log( + 'Every byte of remote terminal output crosses this function; the batcher flushes at\nTERMINAL_OUTPUT_BATCH_MAX_BYTES (64 KiB) or every 5 ms, so a busy remote agent pane\nruns it tens of times a second per subscribed stream.' +) diff --git a/src/main/runtime/rpc/methods/terminal.ts b/src/main/runtime/rpc/methods/terminal.ts index 1261a2de4cf..11848a2f0db 100644 --- a/src/main/runtime/rpc/methods/terminal.ts +++ b/src/main/runtime/rpc/methods/terminal.ts @@ -17,6 +17,11 @@ import { encodeTerminalStreamText, type TerminalStreamFrame } from '../../../../shared/terminal-stream-protocol' +import { + iterateTerminalOutputFrameChunks, + type TerminalOutputFrameChunk, + type TerminalOutputMeta +} from '../terminal-output-frame-chunks' import { TERMINAL_PANE_SPLIT_SOURCES } from '../../../../shared/feature-education-telemetry' import type { TerminalOscLinkRange } from '../../../../shared/terminal-osc-link-ranges' import { @@ -49,8 +54,7 @@ import { TERMINAL_MULTIPLEX_ACK_TOTAL_MAX_WINDOW_BYTES, TERMINAL_MULTIPLEX_MAX_STREAMS_PER_CONNECTION, TERMINAL_MULTIPLEX_PENDING_MAX_BYTES, - TERMINAL_OUTPUT_BATCH_MAX_BYTES, - TERMINAL_STREAM_CHUNK_BYTES + TERMINAL_OUTPUT_BATCH_MAX_BYTES } from '../../../../shared/terminal-multiplex-flow-control' import { drainTerminalMultiplexRoundRobin } from '../terminal-multiplex-round-robin' @@ -139,19 +143,6 @@ type TerminalOutputChunk = { meta?: TerminalOutputMeta } -type TerminalOutputMeta = { - seq?: number - rawLength?: number - transformed?: boolean - cwd?: string -} - -type TerminalOutputFrameChunk = { - bytes: Uint8Array - seq?: number - opcode?: TerminalStreamOpcode -} - function createTerminalOutputBatcher(onFlush: (data: string, meta?: TerminalOutputMeta) => void): { push: (data: string, meta?: TerminalOutputMeta) => void flush: () => void @@ -240,82 +231,6 @@ function createTerminalOutputBatcher(onFlush: (data: string, meta?: TerminalOutp } } -function* iterateTerminalOutputFrameChunks( - data: string, - meta?: TerminalOutputMeta -): Generator { - const rawLength = meta?.rawLength ?? data.length - if (meta?.transformed || rawLength !== data.length) { - yield { - opcode: TerminalStreamOpcode.OutputSpan, - bytes: encodeTerminalStreamJson({ data, rawLength, transformed: true }), - seq: meta?.seq - } - return - } - if (!terminalStreamByteLengthExceeds(data, TERMINAL_STREAM_CHUNK_BYTES)) { - yield { bytes: encodeTerminalStreamText(data), seq: meta?.seq } - return - } - const canPreserveChunkSeq = typeof meta?.seq === 'number' && rawLength === data.length - const shouldDelayFinalSeq = !canPreserveChunkSeq && typeof meta?.seq === 'number' - const startSeq = canPreserveChunkSeq ? meta.seq! - rawLength : undefined - let chunk = '' - let chunkBytes = 0 - let chunkStartOffset = 0 - let offset = 0 - let delayedChunk: { text: string; seq?: number } | null = null - - const takeChunk = (): { text: string; seq?: number } | null => { - if (!chunk) { - return null - } - const chunkSeq = canPreserveChunkSeq ? startSeq! + chunkStartOffset + chunk.length : undefined - const current = { text: chunk, seq: chunkSeq } - chunk = '' - chunkBytes = 0 - chunkStartOffset = offset - return current - } - - for (const part of data) { - const partBytes = terminalStreamByteLength(part) - if (chunkBytes > 0 && chunkBytes + partBytes > TERMINAL_STREAM_CHUNK_BYTES) { - const nextChunk = takeChunk() - if (nextChunk) { - if (shouldDelayFinalSeq) { - if (delayedChunk) { - yield { bytes: encodeTerminalStreamText(delayedChunk.text) } - } - delayedChunk = nextChunk - } else { - yield { bytes: encodeTerminalStreamText(nextChunk.text), seq: nextChunk.seq } - } - } - } - chunk += part - chunkBytes += partBytes - offset += part.length - } - const finalChunk = takeChunk() - if (shouldDelayFinalSeq) { - // Why: only the final frame can safely carry the high-water mark when rawLength can't map back to UTF-16 offsets. - if (finalChunk) { - if (delayedChunk) { - yield { bytes: encodeTerminalStreamText(delayedChunk.text) } - } - delayedChunk = finalChunk - } - if (delayedChunk) { - yield { bytes: encodeTerminalStreamText(delayedChunk.text), seq: meta.seq } - } - return - } - if (finalChunk) { - yield { bytes: encodeTerminalStreamText(finalChunk.text), seq: finalChunk.seq } - } -} - function isTerminalInputLockedForClient( runtime: OrcaRuntimeService, ptyId: string, diff --git a/src/main/runtime/rpc/terminal-output-frame-chunks-equivalence.test.ts b/src/main/runtime/rpc/terminal-output-frame-chunks-equivalence.test.ts new file mode 100644 index 00000000000..f43f7cb93fa --- /dev/null +++ b/src/main/runtime/rpc/terminal-output-frame-chunks-equivalence.test.ts @@ -0,0 +1,477 @@ +import { describe, expect, it } from 'vitest' +import { + TerminalStreamOpcode, + encodeTerminalStreamJson, + encodeTerminalStreamText +} from '../../../shared/terminal-stream-protocol' +import { TERMINAL_STREAM_CHUNK_BYTES } from '../../../shared/terminal-multiplex-flow-control' +import { measureClipboardTextByteLength } from '../../../shared/clipboard-text' +import { + iterateTerminalOutputFrameChunks, + type TerminalOutputFrameChunk, + type TerminalOutputMeta +} from './terminal-output-frame-chunks' + +// Byte-for-byte reference: the pre-optimization implementation, copied verbatim. +// It accumulates `chunk += part` over `for (const part of data)` and measures each +// code point through the shared clipboard measurer. +function legacyByteLength(data: string): number { + return measureClipboardTextByteLength(data).byteLength +} + +function legacyByteLengthExceeds(data: string, maxBytes: number): boolean { + return measureClipboardTextByteLength(data, { stopAfterBytes: maxBytes }).exceededLimit +} + +function* legacyIterateTerminalOutputFrameChunks( + data: string, + meta?: TerminalOutputMeta +): Generator { + const rawLength = meta?.rawLength ?? data.length + if (meta?.transformed || rawLength !== data.length) { + yield { + opcode: TerminalStreamOpcode.OutputSpan, + bytes: encodeTerminalStreamJson({ data, rawLength, transformed: true }), + seq: meta?.seq + } + return + } + if (!legacyByteLengthExceeds(data, TERMINAL_STREAM_CHUNK_BYTES)) { + yield { bytes: encodeTerminalStreamText(data), seq: meta?.seq } + return + } + const canPreserveChunkSeq = typeof meta?.seq === 'number' && rawLength === data.length + const shouldDelayFinalSeq = !canPreserveChunkSeq && typeof meta?.seq === 'number' + const startSeq = canPreserveChunkSeq ? meta.seq! - rawLength : undefined + let chunk = '' + let chunkBytes = 0 + let chunkStartOffset = 0 + let offset = 0 + let delayedChunk: { text: string; seq?: number } | null = null + + const takeChunk = (): { text: string; seq?: number } | null => { + if (!chunk) { + return null + } + const chunkSeq = canPreserveChunkSeq ? startSeq! + chunkStartOffset + chunk.length : undefined + const current = { text: chunk, seq: chunkSeq } + chunk = '' + chunkBytes = 0 + chunkStartOffset = offset + return current + } + + for (const part of data) { + const partBytes = legacyByteLength(part) + if (chunkBytes > 0 && chunkBytes + partBytes > TERMINAL_STREAM_CHUNK_BYTES) { + const nextChunk = takeChunk() + if (nextChunk) { + if (shouldDelayFinalSeq) { + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + delayedChunk = nextChunk + } else { + yield { bytes: encodeTerminalStreamText(nextChunk.text), seq: nextChunk.seq } + } + } + } + chunk += part + chunkBytes += partBytes + offset += part.length + } + const finalChunk = takeChunk() + if (shouldDelayFinalSeq) { + if (finalChunk) { + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + delayedChunk = finalChunk + } + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text), seq: meta.seq } + } + return + } + if (finalChunk) { + yield { bytes: encodeTerminalStreamText(finalChunk.text), seq: finalChunk.seq } + } +} + +type FrameShape = { base64: string; seq: number | 'undefined'; opcode: number | 'undefined' } + +function describeFrames(frames: Iterable): FrameShape[] { + const out: FrameShape[] = [] + for (const frame of frames) { + out.push({ + base64: Buffer.from(frame.bytes).toString('base64'), + seq: frame.seq ?? 'undefined', + opcode: frame.opcode ?? 'undefined' + }) + } + return out +} + +function expectEquivalent(data: string, meta: TerminalOutputMeta | undefined, label: string): void { + const legacy = describeFrames(legacyIterateTerminalOutputFrameChunks(data, meta)) + const next = describeFrames(iterateTerminalOutputFrameChunks(data, meta)) + expect(next, label).toEqual(legacy) +} + +const SURROGATE_PAIR = '\u{1f600}' +const LONE_HIGH = '\ud83d' +const LONE_LOW = '\ude00' +// Extremes of both surrogate ranges: U+10000 (D800 DC00) and U+10FFFF (DBFF DFFF). +const FIRST_ASTRAL = '\u{10000}' +const LAST_ASTRAL = '\u{10ffff}' +const SURROGATE_EDGES = [ + '\ud800', + '\udbff', + '\udc00', + '\udfff', + FIRST_ASTRAL, + LAST_ASTRAL, + '\udbff\udbff', + '\ud800\udbff', + '\udfff\udc00' +] + +// Meta shapes exercised against every fixture: no meta, seq-preserved (rawLength === +// data.length), the delayed-final-seq path (rawLength !== data.length -> OutputSpan), +// transformed, and cwd-only. +function metaShapesFor(data: string): { label: string; meta: TerminalOutputMeta | undefined }[] { + return [ + { label: 'no-meta', meta: undefined }, + { label: 'seq-only', meta: { seq: 5_000_000 } }, + { label: 'seq+rawLength=len', meta: { seq: 9_000, rawLength: data.length } }, + { label: 'seq+rawLength!=len', meta: { seq: 9_000, rawLength: data.length + 7 } }, + { label: 'rawLength!=len only', meta: { rawLength: data.length + 3 } }, + { label: 'transformed', meta: { seq: 42, rawLength: data.length, transformed: true } }, + { label: 'cwd-only', meta: { cwd: '/home/dev/orca' } }, + { label: 'seq=0', meta: { seq: 0, rawLength: data.length } } + ] +} + +function sweepAll(data: string, label: string): void { + for (const shape of metaShapesFor(data)) { + expectEquivalent(data, shape.meta, `${label} [${shape.label}]`) + } +} + +// A fixture whose seq path is only observable when rawLength maps 1:1 to UTF-16 +// offsets; forcing that shape is what makes the algebraic collapse testable. +function seqPreservingMeta(data: string, seq: number): TerminalOutputMeta { + return { seq, rawLength: data.length } +} + +function escapeUnits(value: string): string { + const units: string[] = [] + for (let index = 0; index < value.length; index += 1) { + units.push(`U+${value.charCodeAt(index).toString(16).toUpperCase()}`) + } + return units.join(' ') +} + +function repeatToLength(unit: string, codeUnits: number): string { + let out = '' + while (out.length < codeUnits) { + out += unit + } + return out.slice(0, out.length - (out.length % unit.length)) +} + +// Deterministic PRNG so a fuzz failure is reproducible from the seed alone. +function makeRandom(seed: number): () => number { + let state = seed >>> 0 || 1 + return () => { + state ^= state << 13 + state >>>= 0 + state ^= state >>> 17 + state ^= state << 5 + state >>>= 0 + return state / 0x1_0000_0000 + } +} + +const FUZZ_ALPHABET = [ + 'a', + 'z', + '\r', + '\n', + '\u00e9', + '\u20ac', + SURROGATE_PAIR, + LONE_HIGH, + LONE_LOW, + '\u0000', + '\u001b' +] + +function randomText(random: () => number, parts: number): string { + let out = '' + for (let index = 0; index < parts; index += 1) { + out += FUZZ_ALPHABET[Math.floor(random() * FUZZ_ALPHABET.length)] + } + return out +} + +describe('iterateTerminalOutputFrameChunks equivalence with the pre-optimization loop', () => { + it('matches on the small/no-chunking sizes', () => { + for (const size of [0, 1, 2, 3, 7, 64, 1024, TERMINAL_STREAM_CHUNK_BYTES - 1]) { + sweepAll('a'.repeat(size), `ascii ${size}`) + } + }) + + it('matches across 1B..200KiB ASCII payloads', () => { + for (const size of [ + 1, + 100, + TERMINAL_STREAM_CHUNK_BYTES, + TERMINAL_STREAM_CHUNK_BYTES + 1, + TERMINAL_STREAM_CHUNK_BYTES * 2, + TERMINAL_STREAM_CHUNK_BYTES * 3 + 17, + 100 * 1024, + 200 * 1024 + ]) { + sweepAll('x'.repeat(size), `ascii ${size}`) + } + }) + + it('preserves legacy addition rounding for unsafe sequence values', () => { + const data = 'x'.repeat(TERMINAL_STREAM_CHUNK_BYTES + 1) + const seq = 9_007_199_254_740_994 + const meta = seqPreservingMeta(data, seq) + + expectEquivalent(data, meta, 'unsafe seq rounding') + const frames = [...iterateTerminalOutputFrameChunks(data, meta)] + expect(frames.at(-1)?.seq).toBe(9_007_199_254_740_992) + }) + + it('matches on multi-byte-only payloads at every UTF-8 width', () => { + // UTF-8 width boundaries: 1|2 at U+0080, 2|3 at U+0800, 3|4 at U+10000. + for (const unit of [ + '\u00e9', + '\u20ac', + SURROGATE_PAIR, + '\u0080', + '\u07ff', + '\u0800', + '\uffff' + ]) { + for (const codeUnits of [ + TERMINAL_STREAM_CHUNK_BYTES / 2, + TERMINAL_STREAM_CHUNK_BYTES, + TERMINAL_STREAM_CHUNK_BYTES + 64, + 90 * 1024 + ]) { + const data = repeatToLength(unit, codeUnits) + sweepAll(data, `unit=${JSON.stringify(unit)} codeUnits=${codeUnits}`) + } + } + }) + + it('matches with lone surrogates, including a trailing lone high surrogate', () => { + const filler = 'a'.repeat(TERMINAL_STREAM_CHUNK_BYTES + 5) + for (const data of [ + LONE_HIGH, + LONE_LOW, + LONE_HIGH + LONE_HIGH, + LONE_LOW + LONE_HIGH, + filler + LONE_HIGH, + filler + LONE_LOW, + LONE_HIGH + filler, + LONE_LOW + filler, + filler + LONE_HIGH + filler, + // Reversed pair: never a valid pair, so both halves must stay 3-byte replacements. + filler + LONE_LOW + LONE_HIGH + filler, + repeatToLength(LONE_HIGH, 60 * 1024), + repeatToLength(LONE_LOW + LONE_HIGH, 60 * 1024) + ]) { + sweepAll(data, `lone-surrogate len=${data.length}`) + } + }) + + it('matches at both ends of both surrogate ranges (U+D800..U+DBFF, U+DC00..U+DFFF)', () => { + const filler = 'a'.repeat(TERMINAL_STREAM_CHUNK_BYTES + 5) + for (const edge of SURROGATE_EDGES) { + for (const data of [ + edge, + filler + edge, + edge + filler, + filler + edge + filler, + repeatToLength(edge, 40 * 1024) + ]) { + sweepAll(data, `surrogate-edge ${escapeUnits(edge)} len=${data.length}`) + } + // Land the split inside the edge sequence itself. + for (let offset = -4; offset <= 4; offset += 1) { + const data = `${'a'.repeat(TERMINAL_STREAM_CHUNK_BYTES + offset)}${edge}${'b'.repeat(8)}` + expectEquivalent(data, undefined, `surrogate-edge ${escapeUnits(edge)} offset=${offset}`) + expectEquivalent( + data, + seqPreservingMeta(data, 500_000 + data.length), + `surrogate-edge ${escapeUnits(edge)} offset=${offset} seq` + ) + } + } + }) + + it('sweeps a chunk boundary across a surrogate pair at CHUNK-4..CHUNK+4', () => { + for (let offset = -4; offset <= 4; offset += 1) { + const prefixBytes = TERMINAL_STREAM_CHUNK_BYTES + offset + const data = `${'a'.repeat(prefixBytes)}${SURROGATE_PAIR}${'b'.repeat(64)}` + sweepAll(data, `pair boundary offset=${offset}`) + expectEquivalent( + data, + seqPreservingMeta(data, 1_000_000 + data.length), + `pair boundary offset=${offset} seq-preserved` + ) + } + }) + + it('sweeps a chunk boundary across a multi-byte run at CHUNK-4..CHUNK+4', () => { + for (let offset = -4; offset <= 4; offset += 1) { + const prefixBytes = TERMINAL_STREAM_CHUNK_BYTES + offset + // 3-byte run straddling the cap: the split point cannot land mid-code-point. + const data = `${'a'.repeat(prefixBytes)}${'\u20ac'.repeat(32)}${'b'.repeat(16)}` + sweepAll(data, `multibyte boundary offset=${offset}`) + } + }) + + it('sweeps a chunk boundary across a lone surrogate at CHUNK-4..CHUNK+4', () => { + for (let offset = -4; offset <= 4; offset += 1) { + const prefixBytes = TERMINAL_STREAM_CHUNK_BYTES + offset + for (const lone of [LONE_HIGH, LONE_LOW]) { + const data = `${'a'.repeat(prefixBytes)}${lone}${'b'.repeat(64)}` + sweepAll(data, `lone ${lone === LONE_HIGH ? 'high' : 'low'} boundary offset=${offset}`) + } + // Lone high surrogate as the very last code unit of the payload. + const trailing = `${'a'.repeat(prefixBytes)}${LONE_HIGH}` + sweepAll(trailing, `trailing lone high offset=${offset}`) + } + }) + + it('sweeps 2-byte and 4-byte code points across a 1-code-unit window at the cap', () => { + for (const unit of ['\u00e9', SURROGATE_PAIR]) { + for ( + let pad = TERMINAL_STREAM_CHUNK_BYTES - 6; + pad <= TERMINAL_STREAM_CHUNK_BYTES + 2; + pad += 1 + ) { + const data = `${'a'.repeat(pad)}${unit.repeat(8)}${'z'.repeat(8)}` + expectEquivalent(data, undefined, `window unit=${unit} pad=${pad}`) + expectEquivalent( + data, + seqPreservingMeta(data, 777 + data.length), + `window unit=${unit} pad=${pad} seq` + ) + expectEquivalent( + data, + { seq: 777, rawLength: data.length + 1 }, + `window unit=${unit} pad=${pad} delayed` + ) + } + } + }) + + it('matches on the delayed-final-seq path across many chunk counts', () => { + // rawLength !== data.length routes to OutputSpan; force the multi-chunk delayed + // path by keeping rawLength === data.length but seq present with transformed=false, + // then separately assert the true delayed shape (canPreserveChunkSeq=false). + for (const chunkCount of [1, 2, 3, 5, 9]) { + const data = `${'m'.repeat(TERMINAL_STREAM_CHUNK_BYTES * chunkCount)}${SURROGATE_PAIR}tail` + expectEquivalent(data, { seq: 4242 }, `delayed chunks=${chunkCount} seq-only`) + expectEquivalent(data, { seq: 4242, rawLength: 1 }, `delayed chunks=${chunkCount} raw=1`) + expectEquivalent(data, undefined, `delayed chunks=${chunkCount} no-meta`) + } + }) + + it('matches on realistic mixed terminal output', () => { + const line = '\u001b[35m\u273b Thinking\u001b[0m about the \u20ac plan \u{1f600} 42 passed\r\n' + for (const repeats of [1, 200, 2000, 6000]) { + const data = line.repeat(repeats) + sweepAll(data, `mixed repeats=${repeats}`) + } + }) + + it('fuzzes 4000 short random payloads over the surrogate/control alphabet', () => { + const random = makeRandom(0x5eed_1234) + for (let trial = 0; trial < 4000; trial += 1) { + const data = randomText(random, Math.floor(random() * 40)) + expectEquivalent(data, undefined, `fuzz-small trial=${trial}`) + expectEquivalent( + data, + { seq: 31337, rawLength: data.length }, + `fuzz-small seq trial=${trial}` + ) + } + }) + + it('fuzzes 800 near-cap payloads whose split point lands in the random region', () => { + const random = makeRandom(0x1234_5eed) + for (let trial = 0; trial < 800; trial += 1) { + const fillerLength = TERMINAL_STREAM_CHUNK_BYTES - 6 + Math.floor(random() * 12) + const data = 'q'.repeat(fillerLength) + randomText(random, 1 + Math.floor(random() * 24)) + expectEquivalent(data, undefined, `fuzz-cap trial=${trial}`) + expectEquivalent(data, { seq: 88_888, rawLength: data.length }, `fuzz-cap seq trial=${trial}`) + expectEquivalent(data, { seq: 88_888 }, `fuzz-cap delayed trial=${trial}`) + } + }) + + it('keeps every emitted frame within the wire cap and reassembles to the input', () => { + const data = `${'a'.repeat(200 * 1024)}${SURROGATE_PAIR.repeat(4096)}${LONE_HIGH}` + const frames = [...iterateTerminalOutputFrameChunks(data, seqPreservingMeta(data, 999_999))] + expect(frames.length).toBeGreaterThan(4) + for (const frame of frames) { + expect(frame.bytes.byteLength).toBeLessThanOrEqual(TERMINAL_STREAM_CHUNK_BYTES) + } + expect(Buffer.concat(frames.map((frame) => Buffer.from(frame.bytes))).toString('utf8')).toBe( + Buffer.from(new TextEncoder().encode(data)).toString('utf8') + ) + // Seqs must be strictly increasing and end at the meta high-water mark. + const seqs = frames.map((frame) => frame.seq!) + expect(seqs.every((seq, index) => index === 0 || seq > seqs[index - 1]!)).toBe(true) + expect(seqs.at(-1)).toBe(999_999) + }) + + it('emits exactly one frame when the payload fits the cap in bytes but not naively', () => { + // 3-byte code points: 16384 code units = 49152 bytes = exactly the cap. + const exact = '\u20ac'.repeat(TERMINAL_STREAM_CHUNK_BYTES / 3) + expect(Buffer.byteLength(exact, 'utf8')).toBe(TERMINAL_STREAM_CHUNK_BYTES) + expect([...iterateTerminalOutputFrameChunks(exact)]).toHaveLength(1) + expect([...legacyIterateTerminalOutputFrameChunks(exact)]).toHaveLength(1) + const overByOne = `${exact}a` + expect([...iterateTerminalOutputFrameChunks(overByOne)].length).toBeGreaterThan(1) + expect([...legacyIterateTerminalOutputFrameChunks(overByOne)].length).toBeGreaterThan(1) + }) + + // The chunking loop is only reached when rawLength === data.length and transformed + // is falsy, which makes canPreserveChunkSeq === (typeof meta.seq === 'number') and + // therefore shouldDelayFinalSeq unconditionally false. The branch survives here + // (it also survived in the pre-optimization code) purely as defence in depth. + it('never reaches the delayed-final-seq branch for any meta shape', () => { + for (const data of ['', 'a', 'abc', 'x'.repeat(200)]) { + for (const seq of [undefined, 0, 5] as (number | undefined)[]) { + for (const rawDelta of [undefined, 0, 1, -1] as (number | undefined)[]) { + for (const transformed of [undefined, false, true] as (boolean | undefined)[]) { + const meta: TerminalOutputMeta = {} + if (seq !== undefined) { + meta.seq = seq + } + if (rawDelta !== undefined) { + meta.rawLength = data.length + rawDelta + } + if (transformed !== undefined) { + meta.transformed = transformed + } + const rawLength = meta.rawLength ?? data.length + const reachesChunkLoop = !meta.transformed && rawLength === data.length + const canPreserveChunkSeq = typeof meta.seq === 'number' && rawLength === data.length + const shouldDelayFinalSeq = !canPreserveChunkSeq && typeof meta.seq === 'number' + expect(reachesChunkLoop && shouldDelayFinalSeq, JSON.stringify(meta)).toBe(false) + } + } + } + } + }) +}) diff --git a/src/main/runtime/rpc/terminal-output-frame-chunks.ts b/src/main/runtime/rpc/terminal-output-frame-chunks.ts new file mode 100644 index 00000000000..f040934a565 --- /dev/null +++ b/src/main/runtime/rpc/terminal-output-frame-chunks.ts @@ -0,0 +1,113 @@ +import { + TerminalStreamOpcode, + encodeTerminalStreamJson, + encodeTerminalStreamText +} from '../../../shared/terminal-stream-protocol' +import { TERMINAL_STREAM_CHUNK_BYTES } from '../../../shared/terminal-multiplex-flow-control' + +export type TerminalOutputMeta = { + seq?: number + rawLength?: number + transformed?: boolean + cwd?: string +} + +export type TerminalOutputFrameChunk = { + bytes: Uint8Array + seq?: number + opcode?: TerminalStreamOpcode +} + +// Why: UTF-8 length is never below UTF-16 length, so a code-unit count over the cap already proves the byte count is. +function exceedsTerminalStreamChunkBytes(data: string): boolean { + return ( + data.length > TERMINAL_STREAM_CHUNK_BYTES || + Buffer.byteLength(data, 'utf8') > TERMINAL_STREAM_CHUNK_BYTES + ) +} + +export function* iterateTerminalOutputFrameChunks( + data: string, + meta?: TerminalOutputMeta +): Generator { + const rawLength = meta?.rawLength ?? data.length + if (meta?.transformed || rawLength !== data.length) { + yield { + opcode: TerminalStreamOpcode.OutputSpan, + bytes: encodeTerminalStreamJson({ data, rawLength, transformed: true }), + seq: meta?.seq + } + return + } + if (!exceedsTerminalStreamChunkBytes(data)) { + yield { bytes: encodeTerminalStreamText(data), seq: meta?.seq } + return + } + const canPreserveChunkSeq = typeof meta?.seq === 'number' && rawLength === data.length + // Unreachable past the OutputSpan branch above (rawLength === data.length there), but kept + // as defence in depth: it is the only shape that can carry a seq the chunk offsets can't map. + const shouldDelayFinalSeq = !canPreserveChunkSeq && typeof meta?.seq === 'number' + const startSeq = canPreserveChunkSeq ? meta.seq! - rawLength : undefined + let chunkStart = 0 + let chunkBytes = 0 + let delayedChunk: { text: string; seq?: number } | null = null + + // Why two offsets instead of an accumulator: the emitted text is always the contiguous + // substring from chunkStart to end. Keep the legacy addition order for unsafe sequence values. + const takeChunk = (end: number): { text: string; seq?: number } => { + const text = data.slice(chunkStart, end) + const current = { + text, + seq: canPreserveChunkSeq ? startSeq! + chunkStart + text.length : undefined + } + chunkStart = end + chunkBytes = 0 + return current + } + + let index = 0 + while (index < data.length) { + const code = data.charCodeAt(index) + let width = 1 + let partBytes = 3 + if (code < 0x80) { + partBytes = 1 + } else if (code < 0x800) { + partBytes = 2 + } else if ( + code >= 0xd800 && + code <= 0xdbff && + // Past the end charCodeAt is NaN, which masks to 0 — a trailing lone surrogate stays 3 bytes. + (data.charCodeAt(index + 1) & 0xfc00) === 0xdc00 + ) { + partBytes = 4 + width = 2 + } + // Why chunkBytes > 0: keeps a code point wider than the whole cap in its own frame + // instead of splitting an empty one forever. + if (chunkBytes > 0 && chunkBytes + partBytes > TERMINAL_STREAM_CHUNK_BYTES) { + const nextChunk = takeChunk(index) + if (shouldDelayFinalSeq) { + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + delayedChunk = nextChunk + } else { + yield { bytes: encodeTerminalStreamText(nextChunk.text), seq: nextChunk.seq } + } + } + chunkBytes += partBytes + index += width + } + // A split always advances past its boundary, so the tail after the loop is never empty. + const finalChunk = takeChunk(data.length) + if (shouldDelayFinalSeq) { + // Why: only the final frame can safely carry the high-water mark when rawLength can't map back to UTF-16 offsets. + if (delayedChunk) { + yield { bytes: encodeTerminalStreamText(delayedChunk.text) } + } + yield { bytes: encodeTerminalStreamText(finalChunk.text), seq: meta.seq } + return + } + yield { bytes: encodeTerminalStreamText(finalChunk.text), seq: finalChunk.seq } +} From a72068015f75446e299251ca3f7d0d4e2ba4fc40 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:13:00 -0700 Subject: [PATCH 05/16] fix(panes): stop a stray touch hijacking a mouse divider drag (#11013) Each pointer type has its own primary pointer, so a finger on a touchscreen arrives with isPrimary true while a mouse drag is in flight and satisfies the WSLg fallback in isActiveDragPointer. Exclude touch from that fallback; the pen path WSLg needs is unchanged and a touch-started drag still matches by pointerId. --- .../src/lib/pane-manager/pane-divider-drag.ts | 7 +- .../pane-divider-stray-touch.test.ts | 208 ++++++++++++++++++ 2 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 src/renderer/src/lib/pane-manager/pane-divider-stray-touch.test.ts diff --git a/src/renderer/src/lib/pane-manager/pane-divider-drag.ts b/src/renderer/src/lib/pane-manager/pane-divider-drag.ts index a01a49de988..073cc29e874 100644 --- a/src/renderer/src/lib/pane-manager/pane-divider-drag.ts +++ b/src/renderer/src/lib/pane-manager/pane-divider-drag.ts @@ -220,9 +220,12 @@ export function attachDividerDrag( // Why: WSLg's RDP input path reports press/release as a `mouse` pointer but // streams motion as a `pen` pointer with a different pointerId, so a strict - // pointerId match drops every move. Any primary pointer continues the drag. + // pointerId match drops every move. Any primary pointer continues the drag, + // except touch: each pointer type has its own primary, so a stray finger + // would otherwise hijack an in-flight mouse/pen drag. A touch-started drag + // still matches by pointerId. const isActiveDragPointer = (e: PointerEvent): boolean => - e.pointerId === activePointerId || e.isPrimary + e.pointerId === activePointerId || (e.isPrimary && e.pointerType !== 'touch') const onPointerMove = (e: PointerEvent): void => { if (!dragging || !isActiveDragPointer(e) || !prevEl || !nextEl) { diff --git a/src/renderer/src/lib/pane-manager/pane-divider-stray-touch.test.ts b/src/renderer/src/lib/pane-manager/pane-divider-stray-touch.test.ts new file mode 100644 index 00000000000..0a5d0c967e0 --- /dev/null +++ b/src/renderer/src/lib/pane-manager/pane-divider-stray-touch.test.ts @@ -0,0 +1,208 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { createDivider } from './pane-divider' + +type PaneElement = HTMLElement & { style: Record } + +type DividerDragHarness = { + divider: HTMLElement + dividerListeners: Map + windowListeners: Map + previousPane: PaneElement + nextPane: PaneElement + onLayoutChanged: ReturnType + flushAnimationFrames: () => void +} + +function createPaneElement(width: number): PaneElement { + return { + style: {}, + classList: { contains: vi.fn(() => false) }, + dispatchEvent: vi.fn(() => true), + getBoundingClientRect: vi.fn(() => ({ + left: 0, + top: 0, + right: width, + bottom: 200, + width, + height: 200 + })), + querySelectorAll: vi.fn(() => []) + } as unknown as PaneElement +} + +function createPointerEvent(args: Partial): PointerEvent { + return { + preventDefault: vi.fn(), + pointerId: 1, + pointerType: 'mouse', + isPrimary: true, + clientX: 0, + clientY: 0, + ...args + } as unknown as PointerEvent +} + +function createDividerDragHarness(): DividerDragHarness { + const dividerListeners = new Map() + const windowListeners = new Map() + const capturedPointerIds = new Set() + const animationFrames = new Map() + const previousPane = createPaneElement(100) + const nextPane = createPaneElement(300) + const divider = { + style: { setProperty: vi.fn() }, + classList: { add: vi.fn(), remove: vi.fn() }, + addEventListener: vi.fn((event: string, listener: EventListener) => { + dividerListeners.set(event, listener) + }), + removeEventListener: vi.fn((event: string, listener: EventListener) => { + if (dividerListeners.get(event) === listener) { + dividerListeners.delete(event) + } + }), + setPointerCapture: vi.fn((pointerId: number) => capturedPointerIds.add(pointerId)), + hasPointerCapture: vi.fn((pointerId: number) => capturedPointerIds.has(pointerId)), + releasePointerCapture: vi.fn((pointerId: number) => capturedPointerIds.delete(pointerId)), + previousElementSibling: previousPane, + nextElementSibling: nextPane + } as unknown as HTMLElement + vi.stubGlobal('document', { createElement: vi.fn(() => divider) }) + vi.stubGlobal('window', { + addEventListener: vi.fn((event: string, listener: EventListener) => { + windowListeners.set(event, listener) + }), + removeEventListener: vi.fn((event: string, listener: EventListener) => { + if (windowListeners.get(event) === listener) { + windowListeners.delete(event) + } + }) + }) + let nextFrameId = 0 + vi.stubGlobal( + 'requestAnimationFrame', + vi.fn((callback: FrameRequestCallback) => { + nextFrameId += 1 + animationFrames.set(nextFrameId, callback) + return nextFrameId + }) + ) + vi.stubGlobal( + 'cancelAnimationFrame', + vi.fn((frameId: number) => animationFrames.delete(frameId)) + ) + const onLayoutChanged = vi.fn() + createDivider(true, {}, { refitPanesUnder: vi.fn(), onLayoutChanged }) + + return { + divider, + dividerListeners, + windowListeners, + previousPane, + nextPane, + onLayoutChanged, + flushAnimationFrames: () => { + for (const [frameId, callback] of animationFrames) { + animationFrames.delete(frameId) + callback(16) + } + } + } +} + +// Each pointer type has its own primary pointer, so a finger on a touchscreen +// arrives with isPrimary true while a mouse drag is already in flight. +const STRAY_TOUCH = { pointerId: 42, pointerType: 'touch', isPrimary: true } as const + +function startMouseDrag(harness: DividerDragHarness): void { + harness.dividerListeners.get('pointerdown')?.( + createPointerEvent({ pointerId: 9, pointerType: 'mouse', isPrimary: true, clientX: 100 }) + ) + harness.windowListeners.get('pointermove')?.( + createPointerEvent({ pointerId: 9, pointerType: 'mouse', isPrimary: true, clientX: 180 }) + ) + harness.flushAnimationFrames() +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('divider drag pointer-type isolation', () => { + it('ignores stray touch motion during an active mouse drag', () => { + const harness = createDividerDragHarness() + startMouseDrag(harness) + expect(harness.previousPane.style.flex).toBe('180 1 0%') + + harness.windowListeners.get('pointermove')?.( + createPointerEvent({ ...STRAY_TOUCH, clientX: 320 }) + ) + harness.flushAnimationFrames() + + expect(harness.previousPane.style.flex).toBe('180 1 0%') + expect(harness.nextPane.style.flex).toBe('220 1 0%') + }) + + it('does not commit the layout when a stray touch lifts mid mouse drag', () => { + const harness = createDividerDragHarness() + startMouseDrag(harness) + + harness.windowListeners.get('pointerup')?.(createPointerEvent({ ...STRAY_TOUCH, clientX: 320 })) + + expect(harness.onLayoutChanged).not.toHaveBeenCalled() + expect(harness.divider.classList.remove).not.toHaveBeenCalledWith('is-dragging') + expect(harness.windowListeners.has('pointermove')).toBe(true) + + harness.windowListeners.get('pointerup')?.( + createPointerEvent({ pointerId: 9, pointerType: 'mouse', isPrimary: true, clientX: 180 }) + ) + + expect(harness.previousPane.style.flex).toBe('180 1 0%') + expect(harness.onLayoutChanged).toHaveBeenCalledTimes(1) + }) + + it('does not revert the layout when a stray touch is cancelled mid mouse drag', () => { + const harness = createDividerDragHarness() + harness.previousPane.style.flex = '2 1 0%' + harness.nextPane.style.flex = '3 1 0%' + startMouseDrag(harness) + + harness.windowListeners.get('pointercancel')?.( + createPointerEvent({ ...STRAY_TOUCH, clientX: 320 }) + ) + + expect(harness.previousPane.style.flex).toBe('180 1 0%') + expect(harness.windowListeners.has('pointermove')).toBe(true) + }) + + it('still continues a mouse drag from a primary pen pointer (WSLg relay)', () => { + const harness = createDividerDragHarness() + harness.dividerListeners.get('pointerdown')?.( + createPointerEvent({ pointerId: 1, pointerType: 'mouse', isPrimary: true, clientX: 100 }) + ) + harness.windowListeners.get('pointermove')?.( + createPointerEvent({ pointerId: 19, pointerType: 'pen', isPrimary: true, clientX: 180 }) + ) + harness.windowListeners.get('pointerup')?.( + createPointerEvent({ pointerId: 1, pointerType: 'mouse', isPrimary: true, clientX: 180 }) + ) + + expect(harness.previousPane.style.flex).toBe('180 1 0%') + expect(harness.nextPane.style.flex).toBe('220 1 0%') + expect(harness.onLayoutChanged).toHaveBeenCalledTimes(1) + }) + + it('drives a touch-started drag from its own pointerId', () => { + const harness = createDividerDragHarness() + const touchDrag = { pointerId: 7, pointerType: 'touch', isPrimary: true } as const + harness.dividerListeners.get('pointerdown')?.( + createPointerEvent({ ...touchDrag, clientX: 100 }) + ) + harness.windowListeners.get('pointermove')?.(createPointerEvent({ ...touchDrag, clientX: 180 })) + harness.flushAnimationFrames() + harness.windowListeners.get('pointerup')?.(createPointerEvent({ ...touchDrag, clientX: 180 })) + + expect(harness.previousPane.style.flex).toBe('180 1 0%') + expect(harness.nextPane.style.flex).toBe('220 1 0%') + expect(harness.onLayoutChanged).toHaveBeenCalledTimes(1) + }) +}) From 025c242f0c2476882516f4ff07cc1ba5ef4a7f89 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:13:23 -0700 Subject: [PATCH 06/16] feat(dashboard): tint agent cards by state, show the project as an icon, name the chat (#11012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(dashboard): tint agent cards by state, show the project as an icon, name the chat Three glanceability changes to the agent board: - The "Needs You" signal moves from the column border onto the cards themselves, and done agents get the same treatment in green. Idle cards that simply aren't running stay neutral, so a tint always means "this one wants you". - The repo is now its own icon with the name in a tooltip, instead of a mono label that truncated and competed with the worktree name. Icons ride the snapshot keyed by repoId — image icons are data URLs, and the snapshot republishes several times a second. - The user-message line is labelled with the tab's conversation name rather than "You", resolved through the same getAgentRowConversationName the sidebar's agent rows use. Status-only titles still fall back to "You". * refactor(dashboard): head the card with the session name, move the worktree beside the project The conversation name now sits next to the agent icon as the card's heading rather than prefixing the user-message line, and the worktree drops to the footer beside the project icon. The message line reads "You" again — the name moved up, so keeping it there said the same thing twice. Cards without a resolvable session name keep the worktree as the heading, and the footer omits it rather than repeating it. * fix(dashboard): thread settings into every snapshot builder caller Adding `settings` to DashboardSnapshotState left three callers constructing it without one. The in-window drawer's was a real defect, not just a type error: useLiveDashboardSnapshot derives its own snapshot rather than receiving the relayed one, so a dropped slice silently blanks generated conversation names in the drawer while the pop-out shows them. Bucket counts pass null deliberately — they never render a conversation name, so the sidebar stays unsubscribed from settings. Covers the drawer's wiring with a test, since `settings: null` type-checks and would blank names again without failing loudly. * test(dashboard): complete the terminal layout fixture TerminalLayoutSnapshot requires expandedLeafId; the neighbouring builder test hides this behind an `as unknown as` cast on the whole state object. --- .../ipc/dashboard-payload-validation.test.ts | 50 ++++++ src/main/ipc/dashboard-payload-validation.ts | 28 ++- .../AgentKanbanBoard.test.tsx | 35 +++- .../dashboard-popout/AgentKanbanBoard.tsx | 109 ++++++------ .../dashboard-popout/AgentKanbanCard.test.tsx | 165 ++++++++++++++++-- .../dashboard-popout/AgentKanbanCard.tsx | 83 +++++++-- .../build-dashboard-snapshot.test.ts | 67 +++++++ .../dashboard/build-dashboard-snapshot.ts | 30 +++- .../dashboard/useAgentBucketCounts.ts | 5 +- .../useDashboardPopoutBridge.test.tsx | 1 + .../dashboard/useDashboardPopoutBridge.ts | 2 + .../useLiveDashboardSnapshot.test.ts | 126 +++++++++++++ .../dashboard/useLiveDashboardSnapshot.ts | 6 +- src/shared/dashboard-snapshot.ts | 15 +- 14 files changed, 635 insertions(+), 87 deletions(-) create mode 100644 src/renderer/src/components/dashboard/useLiveDashboardSnapshot.test.ts diff --git a/src/main/ipc/dashboard-payload-validation.test.ts b/src/main/ipc/dashboard-payload-validation.test.ts index 3ae3e85fdf1..afdf4b3661c 100644 --- a/src/main/ipc/dashboard-payload-validation.test.ts +++ b/src/main/ipc/dashboard-payload-validation.test.ts @@ -50,6 +50,56 @@ describe('dashboard payload validation', () => { ).toBe(false) }) + it('accepts repo icons a pop-out can safely render, and rejects the rest', () => { + expect( + isDashboardSnapshot({ + ...SNAPSHOT, + repoIconsByRepoId: { + 'repo-1': { type: 'lucide', name: 'Rocket' }, + 'repo-2': null, + 'repo-3': { + type: 'image', + src: 'https://github.com/anthropics.png?size=64', + source: 'github' + } + } + }) + ).toBe(true) + // Absent entirely: a pop-out on older code still gets its snapshot. + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: undefined })).toBe(true) + + expect( + isDashboardSnapshot({ + ...SNAPSHOT, + repoIconsByRepoId: { + 'repo-1': { type: 'image', src: 'javascript:alert(1)', source: 'file' } + } + }) + ).toBe(false) + expect( + isDashboardSnapshot({ + ...SNAPSHOT, + repoIconsByRepoId: { 'repo-1': { type: 'nonsense' } } + }) + ).toBe(false) + expect(isDashboardSnapshot({ ...SNAPSHOT, repoIconsByRepoId: [] })).toBe(false) + }) + + it('bounds the conversation name', () => { + expect( + isDashboardSnapshot({ + ...SNAPSHOT, + cards: [{ ...SNAPSHOT.cards[0], conversationName: 'Sparse-checkout parser' }] + }) + ).toBe(true) + expect( + isDashboardSnapshot({ + ...SNAPSHOT, + cards: [{ ...SNAPSHOT.cards[0], conversationName: 'x'.repeat(1_025) }] + }) + ).toBe(false) + }) + it('requires complete bounded reveal routing', () => { expect( isDashboardRevealAgentArgs({ diff --git a/src/main/ipc/dashboard-payload-validation.ts b/src/main/ipc/dashboard-payload-validation.ts index 54168d1904a..69abed2c930 100644 --- a/src/main/ipc/dashboard-payload-validation.ts +++ b/src/main/ipc/dashboard-payload-validation.ts @@ -1,4 +1,5 @@ import type { DashboardRevealAgentArgs, DashboardSnapshot } from '../../shared/dashboard-snapshot' +import { sanitizeRepoIcon } from '../../shared/repo-icon' import { AGENT_STATUS_ASSISTANT_MESSAGE_MAX_LENGTH, AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH, @@ -7,6 +8,7 @@ import { } from '../../shared/agent-status-types' const MAX_DASHBOARD_CARDS = 1_000 +const MAX_DASHBOARD_REPO_ICONS = 500 const MAX_ID_LENGTH = 4_096 const MAX_LABEL_LENGTH = 1_024 const DASHBOARD_BUCKETS = new Set(['attention', 'working', 'idle']) @@ -50,7 +52,28 @@ export function isDashboardSnapshot(value: unknown): value is DashboardSnapshot isFiniteNumber(snapshot.generatedAt) && Array.isArray(snapshot.cards) && snapshot.cards.length <= MAX_DASHBOARD_CARDS && - snapshot.cards.every(isDashboardCard) + snapshot.cards.every(isDashboardCard) && + isDashboardRepoIcons(snapshot.repoIconsByRepoId) + ) +} + +/** Repo icons reach the pop-out's ``, so each one must survive the + * same sanitizer the settings picker writes through. */ +function isDashboardRepoIcons(value: unknown): boolean { + if (value === undefined) { + return true + } + if (!value || typeof value !== 'object' || Array.isArray(value)) { + return false + } + const entries = Object.entries(value as Record) + return ( + entries.length <= MAX_DASHBOARD_REPO_ICONS && + entries.every( + ([repoId, icon]) => + isBoundedString(repoId, MAX_ID_LENGTH) && + (icon === null || sanitizeRepoIcon(icon) !== undefined) + ) ) } @@ -80,6 +103,7 @@ function isDashboardCard(value: unknown): boolean { (card.finishedAt === null || isFiniteNumber(card.finishedAt)) && isFiniteNumber(card.stateChangedAt) && typeof card.unseen === 'boolean' && - isOptionalBoundedString(card.askSummary, AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH) + isOptionalBoundedString(card.askSummary, AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH) && + isOptionalBoundedString(card.conversationName, MAX_LABEL_LENGTH) ) } diff --git a/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsx b/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsx index 9a5eed15216..941d3cfec03 100644 --- a/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsx +++ b/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsx @@ -4,6 +4,7 @@ import '@testing-library/jest-dom/vitest' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { act, cleanup, fireEvent, render, screen, within } from '@testing-library/react' import type { DashboardCard, DashboardSnapshot } from '../../../../shared/dashboard-snapshot' +import type { RepoIcon } from '../../../../shared/repo-icon' import { AgentKanbanBoard } from './AgentKanbanBoard' // Stub the card and dialog so the board test stays free of xterm / Radix @@ -11,10 +12,12 @@ import { AgentKanbanBoard } from './AgentKanbanBoard' vi.mock('./AgentKanbanCard', () => ({ AgentKanbanCard: ({ card, + repoIcon, now, onOpenTerminal }: { card: DashboardCard + repoIcon?: RepoIcon | null now: number onOpenTerminal: (card: DashboardCard) => void }) => ( @@ -23,6 +26,7 @@ vi.mock('./AgentKanbanCard', () => ({ data-bucket={card.bucket} data-unseen={card.unseen} data-now={now} + data-repo-icon={repoIcon === null ? 'none' : JSON.stringify(repoIcon)} onClick={() => onOpenTerminal(card)} > {card.worktreeName} @@ -70,8 +74,11 @@ function card(overrides: Partial): DashboardCard { } } -function renderBoard(cards: DashboardCard[]): void { - const snapshot: DashboardSnapshot = { generatedAt: 1, cards } +function renderBoard( + cards: DashboardCard[], + repoIconsByRepoId?: Record +): void { + const snapshot: DashboardSnapshot = { generatedAt: 1, cards, repoIconsByRepoId } render() } @@ -108,6 +115,30 @@ describe('AgentKanbanBoard', () => { expect(screen.getByText('3 total')).toBeTruthy() }) + it('leaves every column border neutral now that cards carry the state color', () => { + renderBoard([card({ bucket: 'attention' })]) + for (const column of document.querySelectorAll('section')) { + expect(column.className).toContain('border-border/60') + expect(column.className).not.toContain('amber') + } + }) + + it('routes each card its own repo icon', () => { + renderBoard( + [ + card({ repoId: 'r1', worktreeName: 'from-r1' }), + card({ repoId: 'r2', worktreeName: 'from-r2' }), + card({ repoId: 'r3', worktreeName: 'from-r3' }) + ], + { r1: { type: 'lucide', name: 'Rocket' }, r2: null } + ) + + expect(screen.getByText('from-r1').dataset.repoIcon).toBe('{"type":"lucide","name":"Rocket"}') + expect(screen.getByText('from-r2').dataset.repoIcon).toBe('none') + // Unknown repo → the card's own default glyph, never another repo's icon. + expect(screen.getByText('from-r3').dataset.repoIcon).toBe('none') + }) + it('shows "None" for empty columns', () => { renderBoard([card({ bucket: 'working' })]) // attention and idle are empty → two "None" placeholders. diff --git a/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx b/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx index 01c46072c9d..b43fc971efb 100644 --- a/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx +++ b/src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx @@ -6,7 +6,9 @@ import { type DashboardCard, type DashboardSnapshot } from '../../../../shared/dashboard-snapshot' +import type { RepoIcon } from '../../../../shared/repo-icon' import { cn } from '@/lib/utils' +import { TooltipProvider } from '@/components/ui/tooltip' import { installWindowVisibilityInterval } from '@/lib/window-visibility-interval' import { AgentKanbanCard } from './AgentKanbanCard' import { AgentTerminalDialog, type AgentRevealArgs } from './AgentTerminalDialog' @@ -58,22 +60,20 @@ function groupByBucket(cards: DashboardCard[]): Record | undefined now: number onOpenTerminal: (card: DashboardCard) => void }): React.JSX.Element { - const highlight = bucket === 'attention' && cards.length > 0 return ( -
+ // Why: attention no longer tints the whole column — the cards inside carry + // their own state color, so a column border would double-signal it. +
{bucketLabel(bucket)} @@ -92,6 +92,7 @@ function KanbanColumn({ @@ -192,54 +193,60 @@ export function AgentKanbanBoard({ }, [dialogCard?.unseen, dialogCard?.paneKey, onAckAgent]) return ( -
-
-

- {translate('dashboardPopout.title', 'Agents')} -

- - {translate('dashboardPopout.total', '{{count}} total', { - count: snapshot.cards.length - })} - - {headerActions || onClose ? ( -
- {headerActions} - {onClose ? ( - - ) : null} -
- ) : null} -
-
- {/* Why: columns share the window width up to a readable cap; mx-auto + // Why: the pop-out is its own React root with no app-level provider, and the + // card's repo tooltip needs one in both hosts. Nesting inside the main + // window's provider is harmless. + +
+
+

+ {translate('dashboardPopout.title', 'Agents')} +

+ + {translate('dashboardPopout.total', '{{count}} total', { + count: snapshot.cards.length + })} + + {headerActions || onClose ? ( +
+ {headerActions} + {onClose ? ( + + ) : null} +
+ ) : null} +
+
+ {/* Why: columns share the window width up to a readable cap; mx-auto centers the capped board so leftover space splits evenly instead of pooling on the right. In overflow the auto margins collapse to 0, keeping the left edge reachable while scrolling. */} -
- {DASHBOARD_BUCKET_ORDER.map((bucket) => ( - - ))} +
+ {DASHBOARD_BUCKET_ORDER.map((bucket) => ( + + ))} +
+
- -
+ ) } diff --git a/src/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsx b/src/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsx index ebcfee3d26b..16739e15d99 100644 --- a/src/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsx +++ b/src/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsx @@ -4,7 +4,9 @@ import '@testing-library/jest-dom/vitest' import { act, cleanup, render, screen } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { DashboardCard } from '../../../../shared/dashboard-snapshot' +import type { RepoIcon } from '../../../../shared/repo-icon' import { i18n } from '@/i18n/i18n' +import { TooltipProvider } from '@/components/ui/tooltip' import { AgentKanbanCard } from './AgentKanbanCard' const agentIconRender = vi.fn() @@ -42,6 +44,24 @@ function card(overrides: Partial = {}): DashboardCard { } } +function renderCard(props: { + card: DashboardCard + now: number + repoIcon?: RepoIcon | null + onOpenTerminal?: () => void +}): ReturnType { + return render( + + + + ) +} + describe('AgentKanbanCard', () => { beforeEach(async () => { await i18n.changeLanguage('en') @@ -53,9 +73,7 @@ describe('AgentKanbanCard', () => { }) it('does not render an invented age when the start time is unknown', () => { - render( - - ) + renderCard({ card: card({ startedAt: 0 }), now: 2_000_000_000 }) expect(screen.queryByText(/\d+d/)).not.toBeInTheDocument() }) @@ -66,47 +84,160 @@ describe('AgentKanbanCard', () => { dotState: 'waiting', askSummary: 'Approve deploy?' }) - const { container, rerender } = render( - - ) + const { container, rerender } = renderCard({ card: attentionCard, now: 2_000 }) expect(screen.queryByTestId('state-dot')).not.toBeInTheDocument() expect(container.querySelectorAll('.lucide-message-circle-question-mark')).toHaveLength(1) rerender( - + + + ) expect(screen.getByTestId('state-dot')).toBeInTheDocument() expect(container.querySelector('.lucide-message-circle-question-mark')).toBeNull() }) + it('tints attention amber and done green, leaving every other state neutral', () => { + const { container: attention } = renderCard({ + card: card({ bucket: 'attention', dotState: 'waiting' }), + now: 2_000 + }) + expect(attention.querySelector('button')?.className).toContain('border-amber-500/40') + + cleanup() + const { container: done } = renderCard({ + card: card({ bucket: 'idle', dotState: 'done' }), + now: 2_000 + }) + expect(done.querySelector('button')?.className).toContain('border-emerald-500/40') + + cleanup() + const { container: idle } = renderCard({ + card: card({ bucket: 'idle', dotState: 'idle' }), + now: 2_000 + }) + const idleClassName = idle.querySelector('button')?.className ?? '' + expect(idleClassName).toContain('border-border/60') + expect(idleClassName).not.toContain('emerald') + expect(idleClassName).not.toContain('amber') + }) + + it('heads the card with the conversation name and drops the worktree to the footer', () => { + const { container } = renderCard({ + card: card({ lastUserMessage: 'ship it', conversationName: 'Sparse-checkout parser' }), + now: 2_000 + }) + + const [header, footer] = [ + container.querySelector('button')!.firstElementChild!, + container.querySelector('button')!.lastElementChild! + ] + expect(header).toHaveTextContent('Sparse-checkout parser') + expect(header).not.toHaveTextContent('dashboard-review') + expect(footer).toHaveTextContent('dashboard-review') + // The message line is attributed to the user again — the name moved up. + expect(screen.getByText('You')).toBeInTheDocument() + }) + + it('heads the card with the worktree when no name resolves, without repeating it', () => { + const { container } = renderCard({ card: card({ lastUserMessage: 'ship it' }), now: 2_000 }) + + expect(screen.getAllByText('dashboard-review')).toHaveLength(1) + expect(container.querySelector('button')!.firstElementChild).toHaveTextContent( + 'dashboard-review' + ) + }) + + it('shows the repo as an icon labelled with its name instead of inline text', () => { + renderCard({ + card: card(), + now: 2_000, + repoIcon: { type: 'emoji', emoji: '🐳' } + }) + + expect(screen.getByLabelText('Orca')).toBeInTheDocument() + expect(screen.getByText('🐳')).toBeInTheDocument() + }) + it('skips structured-clone rerenders until visible card data or its age changes', () => { const onOpenTerminal = vi.fn() const initial = card({ startedAt: 1_000 }) + const repoIcon: RepoIcon = { type: 'lucide', name: 'Rocket' } const { rerender } = render( - + + + ) expect(agentIconRender).toHaveBeenCalledTimes(1) expect(screen.getByText('1m')).toBeInTheDocument() - rerender() + // A fresh structured clone of identical data — including the repo icon. + rerender( + + + + ) expect(agentIconRender).toHaveBeenCalledTimes(1) rerender( - + + + ) expect(agentIconRender).toHaveBeenCalledTimes(2) expect(screen.getByText('2m')).toBeInTheDocument() }) - it('updates the relative age when the UI language changes', async () => { - render( - + it('rerenders when the repo icon changes', () => { + const onOpenTerminal = vi.fn() + const initial = card({ startedAt: 1_000 }) + const { rerender } = render( + + + ) + expect(agentIconRender).toHaveBeenCalledTimes(1) + + rerender( + + + + ) + expect(agentIconRender).toHaveBeenCalledTimes(2) + }) + + it('updates the relative age when the UI language changes', async () => { + renderCard({ card: card({ startedAt: 1_000 }), now: 121_500 }) expect(screen.getByText('2m')).toBeInTheDocument() await act(async () => { diff --git a/src/renderer/src/components/dashboard-popout/AgentKanbanCard.tsx b/src/renderer/src/components/dashboard-popout/AgentKanbanCard.tsx index e8d08b525d6..9262077e17d 100644 --- a/src/renderer/src/components/dashboard-popout/AgentKanbanCard.tsx +++ b/src/renderer/src/components/dashboard-popout/AgentKanbanCard.tsx @@ -4,8 +4,11 @@ import { MessageCircleQuestion } from 'lucide-react' import { AgentIcon } from '@/lib/agent-catalog' import { agentTypeToIconAgent, formatAgentTypeLabel } from '@/lib/agent-status' import { AgentStateDot } from '@/components/AgentStateDot' +import { RepoIconGlyph } from '@/components/repo/repo-icon' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { cn } from '@/lib/utils' import type { DashboardCard } from '../../../../shared/dashboard-snapshot' +import type { RepoIcon } from '../../../../shared/repo-icon' import { translate } from '@/i18n/i18n' /** Compact "started N ago" (the card is glanceable — coarse units are fine). */ @@ -53,12 +56,34 @@ function sameCard(a: DashboardCard, b: DashboardCard): boolean { a.finishedAt === b.finishedAt && a.stateChangedAt === b.stateChangedAt && a.unseen === b.unseen && - a.askSummary === b.askSummary + a.askSummary === b.askSummary && + a.conversationName === b.conversationName ) } +/** Structural — the icon arrives inside a fresh structured clone each publish, + * so identity alone would re-render every card several times a second. */ +function sameRepoIcon(a: RepoIcon | null | undefined, b: RepoIcon | null | undefined): boolean { + if (a === b) { + return true + } + if (!a || !b || a.type !== b.type) { + return false + } + if (a.type === 'lucide') { + return a.name === (b as typeof a).name + } + if (a.type === 'emoji') { + return a.emoji === (b as typeof a).emoji + } + const image = b as typeof a + return a.src === image.src && a.source === image.source && a.label === image.label +} + type AgentKanbanCardProps = { card: DashboardCard + /** The card repo's icon. null renders the default folder glyph. */ + repoIcon?: RepoIcon | null now: number /** Opens the board-level terminal dialog. The dialog is NOT owned by the * card: bucket moves remount the card, and an embedded dialog would close @@ -68,8 +93,23 @@ type AgentKanbanCardProps = { /** One agent on the kanban board. Clicking opens the board's live terminal dialog. */ export const AgentKanbanCard = memo( - function AgentKanbanCard({ card, now, onOpenTerminal }: AgentKanbanCardProps): React.JSX.Element { + function AgentKanbanCard({ + card, + repoIcon = null, + now, + onOpenTerminal + }: AgentKanbanCardProps): React.JSX.Element { useTranslation() + // Why: the two outcomes worth scanning for get a tinted card — amber for + // "answer me", green for "finished, look at it". Everything else stays + // neutral so the tint keeps meaning something. + const needsYou = card.bucket === 'attention' + const isDone = card.dotState === 'done' + // Why: the session's own name heads the card. Without one the worktree is + // the best heading left — and then the footer drops it rather than say it + // twice. + const heading = card.conversationName ?? card.worktreeName + const worktreeInFooter = card.conversationName !== undefined return (