From cc20efcacd059fa74122b63a06a12be1750594e9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:42:32 -0700 Subject: [PATCH] fix(terminal): bind the agent PTY before the run tab is ever published MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../terminal-pane/pty-connection.test.ts | 54 ++++++ .../lib/adopt-agent-background-session-tab.ts | 129 ++++++++++++++ .../agent-background-session-test-state.ts | 20 ++- .../launch-agent-background-session.test.ts | 158 +++++++++++++----- .../lib/launch-agent-background-session.ts | 117 +++++++------ .../launch-worktree-background-terminals.ts | 4 +- .../lib/retire-unowned-background-terminal.ts | 14 +- 7 files changed, 391 insertions(+), 105 deletions(-) create mode 100644 src/renderer/src/lib/adopt-agent-background-session-tab.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 ca3b9cb777c..3d365323dac 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -6271,6 +6271,60 @@ 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. A pane with no PTY on the tab, in ptyIdsByTabId, in the layout, or in an + // eager buffer legitimately fresh-spawns a shell — that is the ordinary "user opened a + // terminal" path and stays correct here. The bug was upstream: launchAgentBackgroundSession + // published the hidden run tab before awaiting the agent spawn, so on an already-mounted + // worktree Terminal.tsx mounted a pane into exactly this state and bound a shell to the run + // tab, orphaning the agent PTY. This pins the downstream behavior the launch-site ordering + // fix depends on: publishing a PTY-less tab is unrecoverable, so it must not happen. + 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) + // The store shape launch-agent-background-session.ts used to expose between createTab and + // bindAutomationTerminal: tab exists, layout leaf exists, no PTY reachable anywhere. + 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() + + // A pending agent launch registration is NOT enough to hold the pane back. + 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/lib/adopt-agent-background-session-tab.ts b/src/renderer/src/lib/adopt-agent-background-session-tab.ts new file mode 100644 index 00000000000..debc0d56f85 --- /dev/null +++ b/src/renderer/src/lib/adopt-agent-background-session-tab.ts @@ -0,0 +1,129 @@ +import type { 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 { retireUnownedTerminal } from '@/lib/retire-unowned-background-terminal' +import type { RuntimeClientTarget } from '@/runtime/runtime-rpc-client' + +type Store = ReturnType +type RegisterArgs = Parameters + +/** + * Reserves the run tab's identity and bakes it into the spawn env before the + * PTY exists, so the tab can be created already bound once the spawn resolves. + * + * Why: createBrowserUuid, not crypto.randomUUID — the latter is undefined in + * non-secure browser contexts such as the LAN web client served over plain HTTP. + * Agent hook callbacks are keyed by pane, and background automation tabs never + * mount a TerminalPane to inject this env for us. + */ +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 + } + } +} + +/** + * Creates the hidden run tab already bound to a live PTY, or retires the PTY if + * its worktree vanished across the spawn await. + * + * Why: no await may separate tab creation from its PTY binding. A worktree the + * user has already visited is fully mounted, so Terminal.tsx renders a pane for + * the new tab in the same pass; if that pane finds no PTY on the tab, in + * ptyIdsByTabId, in the layout, or in an eager buffer, connectPanePty takes its + * FRESH SPAWN branch and binds a default shell to the run tab — the agent PTY is + * orphaned and the user sees a blank shell prompt (#2989). + */ +export async function adoptAgentBackgroundSessionTab(args: { + store: Store + worktreeId: string + reservedTabId: string + leafId: 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, leafId, ptyId, launchRegistration } = args + // Why: the run tab cannot be closed mid-spawn now that it is created after the + // await, but the worktree can still disappear across it — adopting into a gone + // worktree would strand a live PTY behind an unreachable tab. + if ( + await retireUnownedTerminal({ + owner: { worktreeId: args.worktreeId }, + ptyId, + runtimeTarget: args.runtimeTarget, + runtimeTerminalHandle: args.runtimeTerminalHandle, + onRetire: args.onRetire + }) + ) { + return null + } + const tab = store.createTab(args.worktreeId, undefined, undefined, { + id: reservedTabId, + initialPtyId: ptyId, + activate: false, + recordInteraction: false + }) + let paneKey = args.paneKey + if (tab.id !== reservedTabId) { + // Why: createTab mints a fresh id on collision, so the env-baked paneKey no + // longer routes; drop the stale registration before re-keying. + store.clearAgentLaunchConfig(paneKey) + paneKey = makePaneKey(tab.id, leafId) + launchRegistration.tabId = tab.id + } + 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-test-state.ts b/src/renderer/src/lib/agent-background-session-test-state.ts index b338493c411..37471e31b2c 100644 --- a/src/renderer/src/lib/agent-background-session-test-state.ts +++ b/src/renderer/src/lib/agent-background-session-test-state.ts @@ -35,6 +35,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 @@ -96,6 +97,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, @@ -136,6 +139,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 +152,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 +166,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 +221,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.test.ts b/src/renderer/src/lib/launch-agent-background-session.test.ts index a82996c97f7..187353e7194 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -4,6 +4,7 @@ import { toAppSshPtyId } from '../../../shared/ssh-pty-id' import { AGENT_BACKGROUND_SESSION_UUID_RE as UUID_RE, createAgentBackgroundSessionTestState, + expectReservedAgentBackgroundTabId, expectStableAgentBackgroundPaneSpawn, resetAgentBackgroundSessionTestHarness, useRemoteAgentBackgroundRuntime @@ -88,7 +89,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 +99,22 @@ describe('launchAgentBackgroundSession', () => { title: 'Nightly audit' }) + const tabId = expectReservedAgentBackgroundTabId(mockSpawn) + // Why: the run tab must never be store-visible without a PTY — Terminal.tsx + // would mount a pane for it and fresh-spawn a shell into it (#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,25 @@ describe('launchAgentBackgroundSession', () => { }) await Promise.resolve() - expect(mockCreateTab).toHaveBeenCalled() + // Why: a PTY-less tab published here is exactly #2989 — Terminal.tsx mounts a + // pane for it and connectPanePty fresh-spawns a shell into the run tab. + 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,18 +214,50 @@ 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 () => { + it('keeps a folder-workspace launch alive across the spawn await', async () => { + // Why: folder workspaces live outside worktreesByRepo, so a liveness check built on + // allWorktrees() reports them as gone and kills the agent PTY the moment it resolves. + state.getKnownWorktreeById = (worktreeId: string) => + worktreeId === 'folder:fw-1' + ? { id: 'folder:fw-1', path: '/tmp/folder-workspace' } + : undefined + state.worktreesByRepo['repo-1'] = [ + { + id: 'folder:fw-1', + repoId: 'repo-1', + projectId: 'repo-1', + path: '/tmp/folder-workspace', + displayName: 'folder-workspace' + } + ] + const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') + + const launch = launchAgentBackgroundSession({ + agent: 'claude', + worktreeId: 'folder:fw-1', + prompt: 'run in a folder workspace' + }) + // The worktree list stays empty for the whole launch; only the folder lookup resolves. + state.worktreesByRepo['repo-1'] = [] + + await expect(launch).resolves.toMatchObject({ ptyId: 'pty-1' }) + expect(mockKill).not.toHaveBeenCalled() + expect(mockCreateTab).toHaveBeenCalledOnce() + }) + + it('closes a runtime terminal when its worktree disappears before creation resolves', async () => { useRemoteAgentBackgroundRuntime(state) let resolveCreate!: (result: { ok: true @@ -234,13 +282,12 @@ describe('launchAgentBackgroundSession', () => { 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'] = [] + state.worktreesByRepo['repo-1'] = [] resolveCreate({ ok: true, result: { terminal: { handle: 'terminal-after-close', worktreeId: 'wt-1', title: null } } @@ -253,6 +300,7 @@ describe('launchAgentBackgroundSession', () => { params: { terminal: 'terminal-after-close' } }) ) + expect(mockCreateTab).not.toHaveBeenCalled() expect(mockUpdateTabPtyId).not.toHaveBeenCalled() expect(mockRuntimeEnvironmentSubscribe).not.toHaveBeenCalled() expect(mockDispatchEvent).not.toHaveBeenCalled() @@ -274,11 +322,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 +366,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 +489,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 +510,35 @@ 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('submits prompts for stdin-after-start agents in background mode', async () => { @@ -479,7 +555,7 @@ describe('launchAgentBackgroundSession', () => { ) expect(mockPasteDraftWhenAgentReady).toHaveBeenCalledWith( expect.objectContaining({ - tabId: 'tab-1', + tabId: expectReservedAgentBackgroundTabId(mockSpawn), content: 'run the automation', agent: 'aider', submit: true @@ -689,11 +765,13 @@ describe('launchAgentBackgroundSession', () => { 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( - `tab-1:${leafId}`, + `${tabId}:${leafId}`, { agentCommand: "claude '--dangerously-skip-permissions'", agentArgs: '--dangerously-skip-permissions', @@ -702,12 +780,12 @@ describe('launchAgentBackgroundSession', () => { { agentType: 'claude', launchToken: expect.stringMatching(UUID_RE), - tabId: 'tab-1', + tabId, leafId } ) expect(mockSetTabLayout).toHaveBeenCalledWith( - 'tab-1', + tabId, expect.objectContaining({ root: { type: 'leaf', leafId }, activeLeafId: leafId, @@ -723,12 +801,12 @@ describe('launchAgentBackgroundSession', () => { agent: 'claude', prompt: 'run the automation', promptDelivery: 'auto-submit', - placement: { tabId: 'tab-1', leafId }, + placement: { tabId, leafId }, presentation: 'background' }), timeoutMs: 15_000 }) - expect(mockUpdateTabPtyId).toHaveBeenCalledWith('tab-1', 'remote:env-1@@terminal-1') + expect(mockUpdateTabPtyId).toHaveBeenCalledWith(tabId, 'remote:env-1@@terminal-1') expect(mockRegisterEagerPtyBuffer).not.toHaveBeenCalled() expect(mockRuntimeEnvironmentSubscribe).toHaveBeenCalledWith( expect.objectContaining({ @@ -739,8 +817,8 @@ describe('launchAgentBackgroundSession', () => { expect.any(Object) ) expect(result).toMatchObject({ - tabId: 'tab-1', - paneKey: `tab-1:${leafId}`, + tabId, + paneKey: `${tabId}:${leafId}`, ptyId: 'remote:env-1@@terminal-1', terminalOwnership: null }) @@ -810,9 +888,13 @@ describe('launchAgentBackgroundSession', () => { 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', { + 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' }) diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index 9ab4a5e684d..d41ddbb8cd7 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -25,9 +25,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,7 +36,11 @@ 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' export async function launchAgentBackgroundSession( @@ -99,36 +101,18 @@ 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 - } + // Why: automation runs should start without revealing the workspace, and the + // hidden run tab must never be store-visible without a PTY (#2989). Reserve the + // tab id, spawn first, then create the tab already bound to the live session. + const { reservedTabId, leafId, launchToken, launchRegistration, paneEnv } = + reserveAgentBackgroundSessionIdentity({ + store, + agentType: agent, + worktreeId, + launchConfig: startupPlan.launchConfig, + env: startupPlan.env + }) + let paneKey = makePaneKey(reservedTabId, leafId) const sshConnectionId = repo?.connectionId ?? null const sshStartupDelivery = createSshBackgroundStartupDelivery({ command: sshConnectionId ? startupPlan.launchCommand : null, @@ -147,6 +131,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 +145,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) } @@ -192,7 +179,7 @@ export async function launchAgentBackgroundSession( const created = await createRuntimeAgentBackgroundTerminal({ environmentId: runtimeTarget.environmentId, worktreeId, - tabId: tab.id, + tabId: reservedTabId, leafId, agent, ...(hasPrompt && !isFollowupPath ? { prompt: trimmedPrompt } : {}), @@ -225,7 +212,7 @@ export async function launchAgentBackgroundSession( launchAgent: agent, connectionId: sshConnectionId, worktreeId, - tabId: tab.id, + tabId: reservedTabId, leafId, telemetry: { agent_kind: tuiAgentToAgentKind(agent), @@ -236,25 +223,30 @@ 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, + leafId, + 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. @@ -312,19 +304,24 @@ 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) { + // Why: a launch-failure cleanup close is not a user close — keep it out of + // the Cmd+Shift+T 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..8741e445090 100644 --- a/src/renderer/src/lib/retire-unowned-background-terminal.ts +++ b/src/renderer/src/lib/retire-unowned-background-terminal.ts @@ -3,13 +3,23 @@ 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) + : // Why: getKnownWorktreeById, not allWorktrees() — the latter reads only + // worktreesByRepo and would report every folder workspace as gone, + // killing its agent PTY the instant the spawn resolves. + state.getKnownWorktreeById(owner.worktreeId) !== undefined + if (isOwned) { return false } // Why: close can win while provider creation is in flight, before the