From 9d6b3ca7b8a558e8b7d7c7e1928ea52a947d3a12 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 20 Aug 2026 12:15:08 -0700 Subject: [PATCH] fix(terminal): keep a quick command queued until its own spawn takes it (#15630) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Increase shell readiness timeout to match daemon barrier Slow interactive rc files can take longer than 1.5s to initialize. Raise the startup command readiness timeout from 1.5s to 15s to match the daemon barrier and prevent queued commands from executing mid-startup. * fix(terminal): keep a quick command queued until its own spawn takes it (STA-4876) Triggering a quick command opened a terminal tab titled with the command's label, the shell started and drew its prompt, and the command never ran. TerminalPane snapshots `pendingStartupByTabId[tabId]` in a useState lazy initializer, and a mount effect deleted the entry immediately. The pane's key is `${tab.id}-${tab.generation ?? 0}`, so anything that bumps generation before the command reaches a shell — the stall-recovery remount fired from `requestTerminalPaneRecovery`, or the allDead activation regeneration — mounted a second pane that re-read an emptied slot and spawned with no command at all. The loss was permanent, which is why every scope failed alike: repo, global and agent-prompt all funnel through the same queue-then-snapshot sequence. Spend the entry at `onPtySpawn` instead, which is the one point that proves this pane's own fresh spawn exists. A pane retired mid-connect never reaches it, so the command stays queued for the next mount; reattach skips `onPtySpawn`, so it cannot spend a command it never delivers. Three details are load-bearing: - Ownership is reference identity (`paneOwnsQueuedStartup`). Setup and issue splits borrow the same `deps.startup` field for their own one-shot payload, and that payload can be structurally identical to the queued command, so a truthiness test would let a split pane spend a command it never runs. - The consume runs after `bindActivePanePty`. While the tab still has no ptyId, the queued entry is the only thing holding its worktree out of the retention-budget force-park, so dropping it first unmounts the pane mid-spawn. - The callback is one-shot. `onPtySpawn` fires on every fresh spawn a pane makes, including hibernation wake and the respawn ladder, and a command queued after the first launch belongs to that later launch. Known residual, documented at the call site: the consume tracks "a pty exists", not "the command ran". Windows embeds short commands in the shell argv, so they execute before the spawn resolves and a pane retired in that window re-delivers on remount; on POSIX the write waits for shell-ready, so a pty that dies in that window loses a command already spent. Closing either needs a delivery signal from main rather than this callback. Both windows are narrow, and both are strictly better than losing the command unconditionally. * fix(terminal): guard the queued-startup wiring the review found untested Follow-ups from the final review pass on this branch. - Collapse the ownership + one-shot decision into `createQueuedStartupConsumer` so the call site is a single call rather than inline logic no test could reach. Two mutants survived the whole suite before this: relaxing ownership to a truthiness check, and dropping the one-shot guard. Both now fail. - Rewrite the throwing-consume test. It asserted `updateTabPtyId` had been called, which runs *before* the callback, so it passed with the try/catch deleted. It now asserts the throw does not escape into the connect promise, which is the invariant the try/catch actually provides. - Correct the `onQueuedStartupSpawned` docblock. It claimed the callback is "the first moment the command is guaranteed to reach a shell"; the diff's own caveat says otherwise, since Windows runs an argv-embedded command before this fires and a POSIX shell can die before the shell-ready write. It marks a live shell, not delivery. No behavior change: the consumer is the same predicate and the same one-shot, moved behind one exported seam. * fix(terminal): roll shell wrapper isolation into a fresh daemon * Revert "Increase shell readiness timeout to match daemon barrier" This reverts commit 6ab273ef362be4056c16c2caec715fe794532fbf. * Condense queued startup spawn comment Simplify the multi-paragraph explanation into a concise summary that captures the key points: spawn must wait until after the pane is bound to preserve the worktree from force-parking, and the behavior differs between POSIX and Windows for delivery timing. * fix(terminal): prevent consuming replaced queued startup commands When a queued startup is replaced before the pane's first spawn, the one-shot guard alone still allows consuming the replacement. Add isStillQueued callback to verify the slot still holds the originally captured command (STA-4876). --------- Co-authored-by: Jinwoo-H (cherry picked from commit 6e25a90085970498c772490d0659bf258ed18c17) --- ...n-foreground-confirmation-protocol.test.ts | 3 +- .../daemon/daemon-protocol-version.test.ts | 6 +- src/main/daemon/daemon-protocol-version.ts | 7 +- .../components/terminal-pane/TerminalPane.tsx | 7 - ...-connection-queued-startup-consume.test.ts | 206 ++++++++++++++++++ .../terminal-pane/pty-connection-types.ts | 5 + .../terminal-pane/pty-connection.ts | 10 + .../use-terminal-pane-lifecycle.test.ts | 124 +++++++++++ .../use-terminal-pane-lifecycle.ts | 59 +++++ ...terminal-startup-command-retention.test.ts | 78 +++++++ .../local-build-compatibility-contract.json | 4 +- .../local-build-compatibility-contract.ts | 4 +- 12 files changed, 496 insertions(+), 17 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-connection-queued-startup-consume.test.ts create mode 100644 src/renderer/src/store/slices/terminal-startup-command-retention.test.ts diff --git a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts index f27331ece4e..142d1706f9c 100644 --- a/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts +++ b/src/main/daemon/daemon-foreground-confirmation-protocol.test.ts @@ -3,7 +3,7 @@ import { PREVIOUS_DAEMON_PROTOCOL_VERSIONS, PROTOCOL_VERSION } from './types' describe('foreground-confirmation daemon protocol', () => { it('rejects daemons from before the fresh-confirmation RPC', () => { - expect(PROTOCOL_VERSION).toBe(35) + expect(PROTOCOL_VERSION).toBe(36) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(19) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(22) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(23) @@ -18,5 +18,6 @@ describe('foreground-confirmation daemon protocol', () => { expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(32) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(33) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(34) + expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toContain(35) }) }) diff --git a/src/main/daemon/daemon-protocol-version.test.ts b/src/main/daemon/daemon-protocol-version.test.ts index 063eb645580..dfe84947ff5 100644 --- a/src/main/daemon/daemon-protocol-version.test.ts +++ b/src/main/daemon/daemon-protocol-version.test.ts @@ -5,6 +5,7 @@ import { ASYNC_CWD_VALIDATION_DAEMON_PROTOCOL_VERSION, CODEX_SHELL_LAUNCH_PREFLIGHT_DAEMON_PROTOCOL_VERSION, COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION, + CONTENT_ADDRESSED_SHELL_WRAPPER_DAEMON_PROTOCOL_VERSION, GET_FOREGROUND_PROCESS_PROTOCOL_VERSION, HISTORY_SEED_TRANSFER_PROTOCOL_VERSION, MODE_2031_UNSUBSCRIBE_FACT_PROTOCOL_VERSION, @@ -18,7 +19,8 @@ import { describe('daemon protocol version', () => { it('ships bounded history transfer after the 2031-unsubscribe fact', () => { - expect(PROTOCOL_VERSION).toBe(35) + expect(PROTOCOL_VERSION).toBe(36) + expect(CONTENT_ADDRESSED_SHELL_WRAPPER_DAEMON_PROTOCOL_VERSION).toBe(36) expect(ASYNC_CWD_VALIDATION_DAEMON_PROTOCOL_VERSION).toBe(35) expect(CODEX_SHELL_LAUNCH_PREFLIGHT_DAEMON_PROTOCOL_VERSION).toBe(34) expect(WSL_POSIX_CWD_DAEMON_PROTOCOL_VERSION).toBe(33) @@ -31,7 +33,7 @@ describe('daemon protocol version', () => { expect(AGENT_SESSION_CLAIM_DAEMON_PROTOCOL_VERSION).toBe(26) expect(AGENT_SESSION_CREATE_OPERATION_DAEMON_PROTOCOL_VERSION).toBe(26) expect(PREVIOUS_DAEMON_PROTOCOL_VERSIONS).toEqual( - Array.from({ length: 34 }, (_, index) => index + 1) + Array.from({ length: 35 }, (_, index) => index + 1) ) }) diff --git a/src/main/daemon/daemon-protocol-version.ts b/src/main/daemon/daemon-protocol-version.ts index dcccaf2ac16..bae2ae972dc 100644 --- a/src/main/daemon/daemon-protocol-version.ts +++ b/src/main/daemon/daemon-protocol-version.ts @@ -1,6 +1,7 @@ // Why: daemons survive app updates, so wire behavior must be version-gated. -// v35 makes cwd validation async with request-scoped spawn cancellation; older owners stay attachable. -export const PROTOCOL_VERSION = 35 +// v36 launches shells from content-addressed wrapper trees; older owners stay attachable. +export const PROTOCOL_VERSION = 36 +export const CONTENT_ADDRESSED_SHELL_WRAPPER_DAEMON_PROTOCOL_VERSION = 36 export const ASYNC_CWD_VALIDATION_DAEMON_PROTOCOL_VERSION = 35 export const CODEX_SHELL_LAUNCH_PREFLIGHT_DAEMON_PROTOCOL_VERSION = 34 export const WSL_POSIX_CWD_DAEMON_PROTOCOL_VERSION = 33 @@ -29,7 +30,7 @@ export const CLEAN_DISCONNECT_PROTOCOL_VERSION = 24 export const MODE_2031_UNSUBSCRIBE_FACT_PROTOCOL_VERSION = 29 export const PREVIOUS_DAEMON_PROTOCOL_VERSIONS = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, - 28, 29, 30, 31, 32, 33, 34 + 28, 29, 30, 31, 32, 33, 34, 35 ] as const export function supportsPtyStartupIngress(protocolVersion: number): boolean { diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index e611e090321..3d66ead0734 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -731,19 +731,12 @@ function TerminalPane( const [sessionRestoredBannerPaneIds, setSessionRestoredBannerPaneIds] = useState< Map >(() => new Map()) - const consumeTabStartupCommand = useAppStore((store) => store.consumeTabStartupCommand) const [setupSplit] = useState(() => useAppStore.getState().pendingSetupSplitByTabId[tabId]) const consumeTabSetupSplit = useAppStore((store) => store.consumeTabSetupSplit) const [issueCommandSplit] = useState( () => useAppStore.getState().pendingIssueCommandSplitByTabId[tabId] ) const consumeTabIssueCommandSplit = useAppStore((store) => store.consumeTabIssueCommandSplit) - useEffect(() => { - if (startup) { - consumeTabStartupCommand(tabId) - } - }, [startup, tabId, consumeTabStartupCommand]) - useLayoutEffect(() => { if (isVisible && shouldMeasureHiddenStartup) { // Why: hidden startup measurement is first-launch only; keeping it past first visibility would let inactive tabs refit and SIGWINCH. diff --git a/src/renderer/src/components/terminal-pane/pty-connection-queued-startup-consume.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-queued-startup-consume.test.ts new file mode 100644 index 00000000000..a09e1584567 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection-queued-startup-consume.test.ts @@ -0,0 +1,206 @@ +import type * as React from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { flushAsyncTicks } from './pty-connection-test-async' +import { + createMockTransport, + createPane, + createManager, + type MockTransport +} from './pty-connection-test-pane-fixtures' +import { buildPaneConnectionDeps } from './pty-connection-test-deps' +import { createInitialStoreState } from './pty-connection-test-store-fixtures' +import type { StoreState } from './pty-connection-test-store-state' +import { + installTerminalTestGlobals, + restoreTerminalTestGlobals +} from './pty-connection-test-environment' + +const { + resetAndRefreshAllTerminalWebglAtlases, + scheduleTerminalWebglAtlasRecovery, + scheduleRuntimeGraphSync, + shouldSeedCacheTimerOnInitialTitle, + toastInfo, + notifyCodexPaneBoundForStaleSweep +} = vi.hoisted(() => ({ + resetAndRefreshAllTerminalWebglAtlases: vi.fn(), + scheduleTerminalWebglAtlasRecovery: vi.fn(), + scheduleRuntimeGraphSync: vi.fn(), + shouldSeedCacheTimerOnInitialTitle: vi.fn(() => false), + toastInfo: vi.fn(), + notifyCodexPaneBoundForStaleSweep: vi.fn() +})) + +let mockStoreState: StoreState +let transportFactoryQueue: MockTransport[] = [] +let createdTransportOptions: Record[] = [] +let storeSubscribers: ((state: StoreState) => void)[] = [] + +vi.mock('@/runtime/sync-runtime-graph', () => ({ scheduleRuntimeGraphSync })) + +vi.mock('@/lib/pane-manager/pane-manager-registry', async (importOriginal) => ({ + ...(await importOriginal>()), + resetAndRefreshAllTerminalWebglAtlases +})) + +vi.mock('./terminal-webgl-atlas-recovery', () => ({ scheduleTerminalWebglAtlasRecovery })) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => mockStoreState, + subscribe: (listener: (state: StoreState) => void) => { + storeSubscribers.push(listener) + return () => { + storeSubscribers = storeSubscribers.filter((candidate) => candidate !== listener) + } + } + } +})) + +vi.mock('@/lib/agent-status', async (importOriginal) => { + const { buildAgentStatusModuleMock } = await import('./pty-connection-test-environment') + return buildAgentStatusModuleMock(await importOriginal>()) +}) + +vi.mock('./cache-timer-seeding', () => ({ shouldSeedCacheTimerOnInitialTitle })) + +vi.mock('sonner', () => ({ toast: { info: toastInfo } })) + +vi.mock('@/lib/codex-stale-pane-sweep', () => ({ notifyCodexPaneBoundForStaleSweep })) + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useCallback: unknown>(fn: T): T => fn + } +}) + +vi.mock('./pty-transport', () => ({ + createIpcPtyTransport: vi.fn((options: Record) => { + createdTransportOptions.push(options) + const nextTransport = transportFactoryQueue.shift() + if (!nextTransport) { + throw new Error('No mock transport queued') + } + return nextTransport + }) +})) + +vi.mock('./pty-dispatcher', async (importOriginal) => { + const actual = await importOriginal>() + return { ...actual, getEagerPtyBufferHandle: vi.fn(() => undefined) } +}) + +function spawnOnConnect(transport: MockTransport, ptyId: string): void { + transport.connect.mockImplementation(async () => { + const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as ((id: string) => void) | undefined + onPtySpawn?.(ptyId) + return ptyId + }) +} + +// Why this suite exists: the queued startup command is spent through onQueuedStartupSpawned, and +// both *when* it fires and *how often* are load-bearing. Spending it before the pty is bound +// unmounts the pane mid-spawn (the entry is what holds its worktree out of the retention-budget +// force-park); spending it on a later respawn drops a command queued after the first launch +// (STA-4876). +describe('connectPanePty queued startup consume', () => { + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + transportFactoryQueue = [] + createdTransportOptions = [] + storeSubscribers = [] + mockStoreState = createInitialStoreState(() => mockStoreState) + installTerminalTestGlobals() + }) + + afterEach(async () => { + await restoreTerminalTestGlobals() + }) + + it('spends the queued startup only after the pty is bound to the tab', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + spawnOnConnect(transport, 'fresh-pty') + transportFactoryQueue.push(transport) + + const callOrder: string[] = [] + const deps = buildPaneConnectionDeps(() => mockStoreState, { + startup: { command: 'echo queued' }, + updateTabPtyId: vi.fn(() => { + callOrder.push('updateTabPtyId') + }), + onQueuedStartupSpawned: vi.fn(() => { + callOrder.push('onQueuedStartupSpawned') + }) + }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks(20) + + expect(callOrder).toContain('onQueuedStartupSpawned') + expect(callOrder.indexOf('updateTabPtyId')).toBeGreaterThanOrEqual(0) + expect(callOrder.indexOf('updateTabPtyId')).toBeLessThan( + callOrder.indexOf('onQueuedStartupSpawned') + ) + }) + + it('leaves the queued startup unspent when the spawn never reaches onPtySpawn', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + // A retired/reattached spawn resolves without ever announcing a fresh pty. + transport.connect.mockImplementation(async () => undefined) + transportFactoryQueue.push(transport) + + const onQueuedStartupSpawned = vi.fn() + const deps = buildPaneConnectionDeps(() => mockStoreState, { + startup: { command: 'echo queued' }, + onQueuedStartupSpawned + }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks(20) + + // The next mount must still find the command queued, or it is lost for good. + expect(onQueuedStartupSpawned).not.toHaveBeenCalled() + }) + + it('keeps a throwing consume from escaping into the spawn', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + // Mirror the transport contract: onPtySpawn is invoked from inside the connect promise, so + // anything it throws rejects that promise, strands the pane with no pty, and is far worse + // than the stale queued entry the callback exists to clear. + let escapedIntoSpawn = false + transport.connect.mockImplementation(async () => { + const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as + | ((id: string) => void) + | undefined + try { + onPtySpawn?.('fresh-pty') + } catch { + escapedIntoSpawn = true + return undefined + } + return 'fresh-pty' + }) + transportFactoryQueue.push(transport) + + const updateTabPtyId = vi.fn() + const deps = buildPaneConnectionDeps(() => mockStoreState, { + startup: { command: 'echo queued' }, + updateTabPtyId, + onQueuedStartupSpawned: vi.fn(() => { + throw new Error('store blew up') + }) + }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks(20) + + expect(escapedIntoSpawn).toBe(false) + expect(updateTabPtyId).toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-connection-types.ts b/src/renderer/src/components/terminal-pane/pty-connection-types.ts index 9fef3423cbf..f502360ebea 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-types.ts @@ -70,6 +70,11 @@ export type PtyConnectionDeps = { (paneId: number, state: PtyTransportRecoveryState | null) => void > clearTabPtyId: (tabId: string, ptyId: string) => void + /** Set only on the pane carrying the tab's queued startup command; called once its own fresh + * spawn exists, which is the first moment a live shell exists to receive it. Not proof of + * delivery: Windows runs an argv-embedded command before this, and a POSIX shell can die + * before the shell-ready write. */ + onQueuedStartupSpawned?: () => void consumeSuppressedPtyExit: (ptyId: string) => boolean isPtyShutdownPending: (ptyId: string) => boolean updateTabTitle: (tabId: string, title: string) => void diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index abcebbdc417..cbe42205377 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -3102,6 +3102,16 @@ export function connectPanePty( // Why: Command Code has no prompt-start hook. Seed the visible working row // once the PTY exists, then let real hook events refine or complete it. bindActivePanePty(ptyId, { seedInitialAgentStatus: true }) + // Spend the queued command only after this pane has a bound PTY. Retired panes never reach + // this point, and the pending entry keeps the worktree from being force-parked during spawn. + // This marks spawn, not delivery: Windows may run argv-embedded commands earlier, while POSIX + // delivery can still be waiting for shell-ready. + try { + deps.onQueuedStartupSpawned?.() + } catch { + // Why swallowed: this callback runs inside the connect promise, so a throw would reject the + // spawn and strand the pane with no pty at all — a far worse failure than a stale entry. + } } const onPtyRebind = (ptyId: string, replacedPtyId: string): void => { if (!canAdoptCapturedDirectSshRetryPty(ptyId)) { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts index 8e18ece6f2e..053a22405d5 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts @@ -3,10 +3,12 @@ import { applyTerminalPaneCloseRequest, applyTerminalScrollbackRowsToMountedPanes, clearQueuedInitialCwdAfterFirstPane, + createQueuedStartupConsumer, getPreviousVisibleForTerminalPane, isTerminalPaneVisibilityResume, isTouchIOSUserAgent, mapRestoredPaneTitlesByPaneId, + paneOwnsQueuedStartup, resolvePaneLinkCwd, resolvePaneSeedCwd, resolveQueuedInitialCwd, @@ -177,6 +179,128 @@ describe('resetTerminalKeyboardProtocolAfterInterrupt', () => { }) }) +// Why: onPaneCreated uses paneOwnsQueuedStartup to decide whether a pane may spend the tab's queued +// startup command. Setup/issue splits borrow the same deps.startup field for their own one-shot +// payload, so a looser test would let a split pane spend a command it never runs — re-breaking +// STA-4876 for split-setup worktrees. +describe('paneOwnsQueuedStartup', () => { + it('grants ownership only to the pane still holding the queued object', () => { + const queuedStartup = { command: 'echo queued' } + const deps: { startup?: { command: string; env?: Record } | null } = { + startup: queuedStartup + } + const ownershipAtConnect: boolean[] = [] + const observeConnect = (): void => { + ownershipAtConnect.push(paneOwnsQueuedStartup(deps.startup, queuedStartup)) + } + + // Primary pane connects first and owns the queued command. + observeConnect() + // connectPanePty took it; the lifecycle nulls the outer slot so splits cannot replay it. + deps.startup = null + splitPaneWithOneShotStartup(deps, { command: 'orca setup' }, () => { + observeConnect() + return { id: 2 } + }) + splitPaneWithOneShotStartup(deps, { command: 'orca issue' }, () => { + observeConnect() + return { id: 3 } + }) + + expect(ownershipAtConnect).toEqual([true, false, false]) + }) + + // Why this case matters: a truthiness regression ("has a startup") passes the test above, because + // the split payload is non-null there too. Only a structurally-identical payload separates them. + it('denies ownership to a split payload structurally identical to the queued command', () => { + const queuedStartup = { command: 'orca setup' } + + expect(paneOwnsQueuedStartup({ command: 'orca setup' }, queuedStartup)).toBe(false) + expect(paneOwnsQueuedStartup(queuedStartup, queuedStartup)).toBe(true) + }) + + it('denies ownership when the tab queued nothing, so an unrelated pane cannot spend a slot', () => { + expect(paneOwnsQueuedStartup(null, null)).toBe(false) + expect(paneOwnsQueuedStartup(undefined, undefined)).toBe(false) + expect(paneOwnsQueuedStartup({ command: 'orca setup' }, null)).toBe(false) + }) +}) + +describe('createQueuedStartupConsumer', () => { + it('withholds the consumer from a pane that does not own the queued command', () => { + const queuedStartup = { command: 'echo queued' } + const consume = vi.fn() + + // A setup split's borrowed payload, structurally identical to the queued command. + expect( + createQueuedStartupConsumer({ command: 'echo queued' }, queuedStartup, consume, () => true) + ).toBeUndefined() + expect(createQueuedStartupConsumer(null, queuedStartup, consume, () => true)).toBeUndefined() + expect(consume).not.toHaveBeenCalled() + }) + + // Why: onPtySpawn fires again on hibernation wake and the respawn ladder. Spending the slot on a + // later spawn would drop a command queued after the first launch, without ever delivering it. + it('spends the queued command at most once across repeated spawns', () => { + const queuedStartup = { command: 'echo queued' } + const consume = vi.fn() + + const consumer = createQueuedStartupConsumer(queuedStartup, queuedStartup, consume, () => true) + expect(consumer).toBeTypeOf('function') + + consumer?.() + consumer?.() + consumer?.() + + expect(consume).toHaveBeenCalledTimes(1) + }) + + // Why: a replacement can land before this pane's own spawn, so the one-shot guard alone still lets + // the callback delete a command it never launched (STA-4876). + it('leaves a command that replaced the captured one queued for its own launch', () => { + const capturedStartup = { command: 'echo captured' } + let pending: object | null = capturedStartup + const consume = vi.fn(() => { + pending = null + }) + + const consumer = createQueuedStartupConsumer( + capturedStartup, + capturedStartup, + consume, + () => pending === capturedStartup + ) + const replacement = { command: 'echo replacement' } + pending = replacement + + consumer?.() + + expect(consume).not.toHaveBeenCalled() + expect(pending).toBe(replacement) + }) + + // Why: the replacement belongs to the launch that queued it, so this pane's respawn ladder must not + // reach for it after skipping its own spent chance. + it('does not spend a replacement on a later spawn of the same pane', () => { + const capturedStartup = { command: 'echo captured' } + let pending: object | null = { command: 'echo replacement' } + const consume = vi.fn() + + const consumer = createQueuedStartupConsumer( + capturedStartup, + capturedStartup, + consume, + () => pending === capturedStartup + ) + + consumer?.() + pending = capturedStartup + consumer?.() + + expect(consume).not.toHaveBeenCalled() + }) +}) + describe('splitPaneWithOneShotStartup', () => { it('only exposes startup to the intentional split and clears it afterwards', () => { const deps: { startup?: { command: string; env?: Record } | null } = { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts index 4f3254fc281..91c98a4285f 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts @@ -478,6 +478,55 @@ function resolveTerminalHomePathFromEnv(env: Record | undefined) return homeDrive && homePath ? `${homeDrive}${homePath}` : null } +/** + * Whether this pane's startup is the tab's queued command rather than a payload a split borrowed. + * + * Why reference identity and not truthiness: setup/issue splits assign their own one-shot object to + * the same `deps.startup` field, so "has a startup" would let a split pane spend a command it never + * runs — and a split's payload can be structurally identical to the queued one (STA-4876). + */ +export function paneOwnsQueuedStartup( + paneStartup: object | null | undefined, + queuedStartup: object | null | undefined +): boolean { + return queuedStartup != null && paneStartup === queuedStartup +} + +/** + * The callback that spends the tab's queued startup command, or `undefined` when this pane does + * not own it. + * + * Why one-shot: `onPtySpawn` fires on every fresh spawn a pane makes — hibernation wake, the + * respawn ladder — but only the first carried the queued command. A command queued onto the tab + * afterwards belongs to that later launch, and spending it here would drop it undelivered. + * + * Why `isStillQueued` on top of that guard: the replacement can also arrive before this pane's very + * first spawn, so the slot is only spent while it still holds the command this pane launched. + */ +export function createQueuedStartupConsumer( + paneStartup: object | null | undefined, + queuedStartup: object | null | undefined, + consume: () => void, + isStillQueued: () => boolean +): (() => void) | undefined { + if (!paneOwnsQueuedStartup(paneStartup, queuedStartup)) { + return undefined + } + let spent = false + return () => { + if (spent) { + return + } + // Why spent regardless: this pane's launch is its one chance at the slot; a later spawn of the + // same pane must not spend whatever command took its place. + spent = true + if (!isStillQueued()) { + return + } + consume() + } +} + /** Scopes `deps.startup` to a single call of `splitPane()`, clearing it in `finally` so later splits do not replay the payload. */ export function splitPaneWithOneShotStartup( deps: SplitWithStartupDeps, @@ -1295,8 +1344,18 @@ export function useTerminalPaneLifecycle({ } } applyAppearance(manager) + const onQueuedStartupSpawned = createQueuedStartupConsumer( + ptyDeps.startup, + startupWithSetupSplitWait, + () => useAppStore.getState().consumeTabStartupCommand(tabId), + // Why `startup` and not startupWithSetupSplitWait: setup-split hands the pane a copy, so only + // the raw prop still matches the object the store holds. Read and consume run in one + // synchronous step, so nothing can queue in between. + () => useAppStore.getState().pendingStartupByTabId[tabId] === startup + ) const panePtyBinding = connectPanePty(pane, manager, { ...ptyDeps, + ...(onQueuedStartupSpawned ? { onQueuedStartupSpawned } : {}), // Why: spread order matters — spawnHints.cwd (source pane) must override ptyDeps.cwd (worktree root) so splits boot in the live cwd. ...(spawnHints?.cwd ? { cwd: spawnHints.cwd } : {}), restoredPtyIdByLeafId: spawnHints?.ptyId diff --git a/src/renderer/src/store/slices/terminal-startup-command-retention.test.ts b/src/renderer/src/store/slices/terminal-startup-command-retention.test.ts new file mode 100644 index 00000000000..ea369e54f6a --- /dev/null +++ b/src/renderer/src/store/slices/terminal-startup-command-retention.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from 'vitest' +import { createTestStore, makeWorktree, seedStore } from './store-test-helpers' + +const WORKTREE_ID = 'repo1::/path/wt1' + +function seedWorktreeWithTab(store: ReturnType): string { + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1', path: '/path/wt1' })] + } + }) + return store.getState().createTab(WORKTREE_ID).id +} + +// Why this suite exists: TerminalPane snapshots pendingStartupByTabId in a useState initializer, +// so whatever is missing there at mount is missing for good. The command must therefore survive +// every store event that can precede its owning pane's fresh spawn, or a quick command leaves the +// user a titled tab sitting at a bare prompt (STA-4876). +describe('queued startup command retention', () => { + it('survives a recovery remount so the next mount can still deliver it', () => { + const store = createTestStore() + const tabId = seedWorktreeWithTab(store) + store.getState().queueTabStartupCommand(tabId, { command: 'echo hi' }) + + store.getState().remountTerminalTabForRecovery(tabId) + + expect(store.getState().pendingStartupByTabId[tabId]).toEqual({ command: 'echo hi' }) + }) + + // Why: binding is pane-scoped but this map is tab-scoped. A tab routinely runs several panes + // (setup/issue splits land on the same tabId), and the sibling often binds first — if a bind + // spent the command, the pane that actually owns it would find an empty slot on remount. + it('is not spent by a sibling pane binding a pty to the same tab', () => { + const store = createTestStore() + const tabId = seedWorktreeWithTab(store) + store.getState().queueTabStartupCommand(tabId, { command: 'echo hi' }) + + store.getState().updateTabPtyId(tabId, 'pty-sibling') + + expect(store.getState().pendingStartupByTabId[tabId]).toEqual({ command: 'echo hi' }) + }) + + // Only the owning pane spends it, via this action, once its own fresh spawn exists. + it('is spent through consumeTabStartupCommand and does not replay afterwards', () => { + const store = createTestStore() + const tabId = seedWorktreeWithTab(store) + store.getState().queueTabStartupCommand(tabId, { command: 'echo hi' }) + + expect(store.getState().consumeTabStartupCommand(tabId)).toEqual({ command: 'echo hi' }) + + store.getState().remountTerminalTabForRecovery(tabId) + expect(store.getState().pendingStartupByTabId[tabId]).toBeUndefined() + expect(store.getState().consumeTabStartupCommand(tabId)).toBeNull() + }) + + // Why: retention is bounded by tab lifetime, so a spawn that never binds cannot leak forever. + it('is dropped when the tab closes without ever spawning', () => { + const store = createTestStore() + const tabId = seedWorktreeWithTab(store) + store.getState().queueTabStartupCommand(tabId, { command: 'echo hi' }) + + store.getState().closeTab(tabId) + + expect(store.getState().pendingStartupByTabId[tabId]).toBeUndefined() + }) + + it('leaves a sibling tab’s queued command alone', () => { + const store = createTestStore() + const tabId = seedWorktreeWithTab(store) + const siblingId = store.getState().createTab(WORKTREE_ID).id + store.getState().queueTabStartupCommand(tabId, { command: 'echo mine' }) + store.getState().queueTabStartupCommand(siblingId, { command: 'echo theirs' }) + + store.getState().consumeTabStartupCommand(tabId) + + expect(store.getState().pendingStartupByTabId[siblingId]).toEqual({ command: 'echo theirs' }) + }) +}) diff --git a/src/shared/local-build-compatibility-contract.json b/src/shared/local-build-compatibility-contract.json index aa2cc016f4b..7c918c73a0d 100644 --- a/src/shared/local-build-compatibility-contract.json +++ b/src/shared/local-build-compatibility-contract.json @@ -3,9 +3,9 @@ "appId": "com.stablyai.orca", "stateSchemaVersion": 1, "readableStateSchemaVersions": [1], - "daemonProtocolVersion": 35, + "daemonProtocolVersion": 36, "attachableDaemonProtocolVersions": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, - 27, 28, 29, 30, 31, 32, 33, 34, 35 + 27, 28, 29, 30, 31, 32, 33, 34, 35, 36 ] } diff --git a/src/shared/local-build-compatibility-contract.ts b/src/shared/local-build-compatibility-contract.ts index 4ff84aa6dfa..7e61c309180 100644 --- a/src/shared/local-build-compatibility-contract.ts +++ b/src/shared/local-build-compatibility-contract.ts @@ -3,9 +3,9 @@ export const LOCAL_BUILD_COMPATIBILITY_CONTRACT = { appId: 'com.stablyai.orca', stateSchemaVersion: 1, readableStateSchemaVersions: [1], - daemonProtocolVersion: 35, + daemonProtocolVersion: 36, attachableDaemonProtocolVersions: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, - 27, 28, 29, 30, 31, 32, 33, 34, 35 + 27, 28, 29, 30, 31, 32, 33, 34, 35, 36 ] } as const