mirror of
https://github.com/stablyai/orca.git
synced 2026-11-02 10:08:13 +00:00
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 <noreply@anthropic.com> * refactor(agent-launch): drop the unreachable tab id mismatch guard Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
27c0275e3a
commit
e53b20095d
@@ -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()
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user