From e4b0d548daa1fdeb5f351f02e5718d59083bad06 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:52:35 -0700 Subject: [PATCH] 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 --- .../lib/adopt-agent-background-session-tab.ts | 31 ++++++++----- .../launch-agent-background-session.test.ts | 46 +++++++++++++------ .../lib/launch-agent-background-session.ts | 5 +- 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/src/renderer/src/lib/adopt-agent-background-session-tab.ts b/src/renderer/src/lib/adopt-agent-background-session-tab.ts index debc0d56f85..2f661d86979 100644 --- a/src/renderer/src/lib/adopt-agent-background-session-tab.ts +++ b/src/renderer/src/lib/adopt-agent-background-session-tab.ts @@ -3,7 +3,8 @@ 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 { 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 @@ -74,7 +75,6 @@ export async function adoptAgentBackgroundSessionTab(args: { store: Store worktreeId: string reservedTabId: string - leafId: string ptyId: string paneKey: PaneKey launchConfig: RegisterArgs[1] @@ -88,7 +88,7 @@ export async function adoptAgentBackgroundSessionTab(args: { paneKey: PaneKey terminalOwnership: ReturnType } | null> { - const { store, reservedTabId, leafId, ptyId, launchRegistration } = args + const { store, reservedTabId, 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. @@ -103,20 +103,29 @@ export async function adoptAgentBackgroundSessionTab(args: { ) { return null } + // Why fail instead of re-key: ORCA_TAB_ID / ORCA_PANE_KEY are already baked into + // the running process's env and cannot be rewritten. On collision createTab mints + // a different id, so renderer routing and the agent's own hook identity would + // permanently disagree — status and completion attribution silently go to the + // colliding tab. A dead launch the user can retry beats a mis-attributed live one. + // (A v4 UUID collision is vanishingly unlikely; this is a fail-closed guard.) + if (isTerminalTabPresent(store, 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 }) - 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 - } + const paneKey = args.paneKey store.registerAgentLaunchConfig(paneKey, args.launchConfig, launchRegistration) const terminalOwnership = bindAutomationTerminal( 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 187353e7194..d7ab616aa76 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -226,22 +226,16 @@ describe('launchAgentBackgroundSession', () => { expect(mockDispatchEvent).not.toHaveBeenCalled() }) - 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. + it('launches into a folder workspace that is absent from worktreesByRepo throughout', async () => { + // Why: a real folder workspace is NEVER in worktreesByRepo — it lives in + // folderWorkspaces and only getKnownWorktreeById resolves it. Seeding the repo + // list here (even briefly) would fake the one condition under test, so both the + // initial resolution and the post-spawn retire check are exercised for real (#2989). + state.worktreesByRepo['repo-1'] = [] 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({ @@ -249,8 +243,6 @@ describe('launchAgentBackgroundSession', () => { 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() @@ -541,6 +533,32 @@ describe('launchAgentBackgroundSession', () => { expect(mockKill).toHaveBeenCalledWith('pty-1') }) + it('retires the launch instead of adopting a colliding tab id', async () => { + // Why: ORCA_TAB_ID / ORCA_PANE_KEY are already baked into the spawned process, so a + // re-keyed tab would route bytes to one identity and hooks to another. Fail closed. + mockSpawn.mockImplementationOnce((args: { tabId: string }) => { + // Land the collision between the reservation and the adoption, exactly as a + // concurrently-created tab would. + state.tabsByWorktree['wt-1'].push({ 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 () => { const { launchAgentBackgroundSession } = await import('./launch-agent-background-session') diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index d41ddbb8cd7..a9c5bab276c 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -48,7 +48,9 @@ export async function launchAgentBackgroundSession( ): Promise { const { agent, worktreeId, prompt, launchSource, title, onData, onExit, onAgentStatus } = args const store = useAppStore.getState() - const worktree = store.allWorktrees().find((entry) => entry.id === worktreeId) + // Why: allWorktrees() reads only worktreesByRepo, so every folder workspace looks + // absent and its automation dies here. getKnownWorktreeById covers both (#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.') @@ -227,7 +229,6 @@ export async function launchAgentBackgroundSession( store, worktreeId, reservedTabId, - leafId, ptyId, paneKey, launchConfig: returnedLaunchConfig ?? startupPlan.launchConfig,