From e53b20095d3ca98df120cd0a3ef4814e945a9779 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 14 Oct 2026 17:54:46 -0400 Subject: [PATCH] fix(terminal): reject a requested tab id that is already taken (#28841) * fix(terminal): reject a requested tab id that is already taken createTab silently minted a fresh id when the caller's requested id was already in use. The caller's PTY routes agent status by the id it asked for, so the new tab never received its agent status and the status went to the other tab instead. Now the create throws and changes nothing. Co-Authored-By: Claude Opus 5.5 * refactor(agent-launch): drop the unreachable tab id mismatch guard Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../src/lib/agent-launch-tab-publication.ts | 5 ---- .../slices/store-create-tab-id-hint.test.ts | 28 ++++++++----------- .../store/terminals/terminal-tab-creation.ts | 12 ++++---- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/renderer/src/lib/agent-launch-tab-publication.ts b/src/renderer/src/lib/agent-launch-tab-publication.ts index 20116563991..4c9959ea2d1 100644 --- a/src/renderer/src/lib/agent-launch-tab-publication.ts +++ b/src/renderer/src/lib/agent-launch-tab-publication.ts @@ -103,11 +103,6 @@ export function publishAgentLaunchTab( if (request.prompt) { rememberAgentLaunchPanePrompt(tabId, request.prompt) } - if (tab.id !== tabId) { - // createTab mints a fresh id on a collision it saw and we did not; that tab would never attach. - useAppStore.getState().closeTab(tab.id, { recordInteraction: false }) - throw new Error('agent_launch_tab_id_taken') - } if (placement.anchorTabId) { const unifiedTabId = useAppStore .getState() diff --git a/src/renderer/src/store/slices/store-create-tab-id-hint.test.ts b/src/renderer/src/store/slices/store-create-tab-id-hint.test.ts index 6dbda55207c..643b9454e97 100644 --- a/src/renderer/src/store/slices/store-create-tab-id-hint.test.ts +++ b/src/renderer/src/store/slices/store-create-tab-id-hint.test.ts @@ -47,7 +47,7 @@ describe('createTab tabId hint', () => { expect(tab.id).toBe(hintedId) }) - it('falls back to a fresh id on collision and warns', () => { + it('rejects a taken id instead of minting a fresh one', () => { const store = createTestStore() const wt = 'repo1::/path/wt-collision' const existingId = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' @@ -63,14 +63,12 @@ describe('createTab tabId hint', () => { unifiedTabsByWorktree: {} }) - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - try { - const tab = store.getState().createTab(wt, undefined, undefined, { id: existingId }) - expect(tab.id).not.toBe(existingId) - expect(warn).toHaveBeenCalledWith(expect.stringContaining(existingId)) - } finally { - warn.mockRestore() - } + const before = store.getState().tabsByWorktree + + expect(() => store.getState().createTab(wt, undefined, undefined, { id: existingId })).toThrow( + `Terminal tab ${existingId} already exists` + ) + expect(store.getState().tabsByWorktree).toBe(before) }) it('treats tab ids as global and rejects hints that collide in another worktree', () => { @@ -93,14 +91,10 @@ describe('createTab tabId hint', () => { unifiedTabsByWorktree: {} }) - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - try { - const tab = store.getState().createTab(wtA, undefined, undefined, { id: existingId }) - expect(tab.id).not.toBe(existingId) - expect(warn).toHaveBeenCalledWith(expect.stringContaining(existingId)) - } finally { - warn.mockRestore() - } + expect(() => store.getState().createTab(wtA, undefined, undefined, { id: existingId })).toThrow( + `Terminal tab ${existingId} already exists` + ) + expect(store.getState().tabsByWorktree[wtA]).toBeUndefined() }) it('ignores empty string hints instead of persisting an unusable tab id', () => { diff --git a/src/renderer/src/store/terminals/terminal-tab-creation.ts b/src/renderer/src/store/terminals/terminal-tab-creation.ts index 5e12d3b45a8..511498965d1 100644 --- a/src/renderer/src/store/terminals/terminal-tab-creation.ts +++ b/src/renderer/src/store/terminals/terminal-tab-creation.ts @@ -52,22 +52,20 @@ export function createTerminalTabCreationActions( const existing = (s.tabsByWorktree[worktreeId] ?? []).filter( (entry) => !orphanTerminalIds.has(entry.id) ) - // Why: honor a caller-supplied tab id but mint on collision — aliasing two PTYs to one id corrupts agent-status routing. See docs/cli-terminal-hook-pane-key.md. // Why: only honor a non-empty trimmed hint; useIpcEvents spreads `id` whenever tabId !== undefined, so a stray '' would break paneKey routing. const trimmedHint = typeof options?.id === 'string' ? options.id.trim() : '' const hintedId = trimmedHint.length > 0 && isValidHostTerminalTabId(trimmedHint) ? trimmedHint : undefined - const idCollides = + // Why: the caller's PTY routes agent status by this id, so a fresh id would orphan it and aliasing would corrupt another tab's. + if ( hintedId !== undefined && Object.values(s.tabsByWorktree).some((tabs) => tabs.some((entry) => entry.id === hintedId) ) - if (idCollides) { - console.warn( - `[createTab] tabId hint ${hintedId} already exists; minting a fresh id (hook attribution will degrade for this terminal)` - ) + ) { + throw new Error(`Terminal tab ${hintedId} already exists`) } - const id = hintedId !== undefined && !idCollides ? hintedId : createBrowserUuid() + const id = hintedId ?? createBrowserUuid() const requestedInitialLeafId = options?.initialLeafId && isTerminalLeafId(options.initialLeafId) ? options.initialLeafId