mirror of
https://github.com/stablyai/orca.git
synced 2026-10-16 08:03:09 +00:00
Fix automation tabs showing a shell instead of the live agent (#5099)
* Fix automation tabs showing a shell instead of the live agent
Opening a background automation's terminal tab showed a bare shell while
the agent (Claude) kept running headless — the sidebar updated but the
pane was attached to the wrong PTY.
On first mount the restored ptyId equals the tab ptyId, and
isSessionOwnedByWorktree() returns true for it, so connectPanePty routed
the still-live eagerly-spawned PTY into the daemon-reattach branch
(transport.connect({ sessionId })), which spawns a fresh shell and
orphans the live agent PTY instead of adopting it via attach()+replay.
Part A: gate the deferred reattach on the absence of a live eager buffer.
A live eager buffer means the PTY is a still-running local session to
adopt (attach + replay), not a daemon session to re-connect. Daemon
reattach and remote PTYs are unaffected (gated on the eager buffer).
Part B: publish never-mounted background automation tabs into the runtime
graph (gated on a live eager buffer) so the live agent PTY binds to its
real tab instead of surfacing as an orphan `pty:<id>` terminal — fixing
`orca terminal list`, the CLI, and automation session-reuse.
Adds a characterization test (fails on the old code, passes now) and a
runtime-graph publish test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Harden eager PTY tab adoption
Co-authored-by: Orca <help@stably.ai>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jinwoo-H <jinwoo0825@gmail.com>
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
committed by
Jinwoo-H
co-authored by
Claude Opus 4.8
Orca
Jinwoo-H
parent
72be696754
commit
2bc1a7a587
@@ -7,6 +7,7 @@ import {
|
||||
RESET_TERMINAL_CURSOR_STYLE
|
||||
} from './layout-serialization'
|
||||
import type * as UseNotificationDispatchModule from './use-notification-dispatch'
|
||||
import { getEagerPtyBufferHandle } from './pty-dispatcher'
|
||||
import { makePaneKey } from '../../../../shared/stable-pane-id'
|
||||
import type { TerminalLayoutSnapshot } from '../../../../shared/types'
|
||||
|
||||
@@ -214,6 +215,18 @@ vi.mock('./remote-runtime-pty-transport', () => ({
|
||||
)
|
||||
}))
|
||||
|
||||
// Why: the adopt-vs-reattach decision consults the eager-PTY buffer registry to
|
||||
// tell a still-live locally-spawned PTY (attach + replay) from a daemon session
|
||||
// to re-connect. Keep the real module but stub the lookup so tests can simulate
|
||||
// a live eager buffer without standing up the real IPC dispatcher.
|
||||
vi.mock('./pty-dispatcher', async (importOriginal) => {
|
||||
const actual = await importOriginal<Record<string, unknown>>()
|
||||
return {
|
||||
...actual,
|
||||
getEagerPtyBufferHandle: vi.fn(() => undefined)
|
||||
}
|
||||
})
|
||||
|
||||
function createMockTransport(initialPtyId: string | null = null): MockTransport {
|
||||
let ptyId = initialPtyId
|
||||
const transport = {
|
||||
@@ -2559,6 +2572,96 @@ describe('connectPanePty', () => {
|
||||
expect(deps.syncPanePtyLayoutBinding).toHaveBeenCalledWith(2, 'leaf-pty-2')
|
||||
})
|
||||
|
||||
it('adopts a still-live background PTY via attach instead of reattaching when an eager buffer exists', async () => {
|
||||
// Why: background automation tabs spawn the agent PTY eagerly and register an
|
||||
// eager buffer, then never mount until opened. On first mount the restored
|
||||
// ptyId equals the tab ptyId, so without this guard the pane mis-routes into
|
||||
// the daemon-reattach branch (connect) and spawns a fresh shell, orphaning the
|
||||
// live agent PTY. A live eager buffer means "attach + replay", not "reattach".
|
||||
const eagerPtyId = 'auto-eager-pty'
|
||||
vi.mocked(getEagerPtyBufferHandle).mockImplementation((ptyId: string) =>
|
||||
ptyId === eagerPtyId ? { flush: () => '', dispose: () => {} } : undefined
|
||||
)
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport()
|
||||
transportFactoryQueue.push(transport)
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: eagerPtyId }] },
|
||||
ptyIdsByTabId: { 'tab-1': [eagerPtyId] },
|
||||
terminalLayoutsByTabId: {
|
||||
'tab-1': {
|
||||
root: { type: 'leaf', leafId: LEAF_1 },
|
||||
activeLeafId: LEAF_1,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [LEAF_1]: eagerPtyId }
|
||||
}
|
||||
}
|
||||
} as StoreState
|
||||
const deps = createDeps({
|
||||
restoredLeafId: LEAF_1,
|
||||
restoredPtyIdByLeafId: { [LEAF_1]: eagerPtyId }
|
||||
})
|
||||
|
||||
connectPanePty(createPane(1) as never, createManager(1) as never, deps as never)
|
||||
await flushAsyncTicks()
|
||||
|
||||
expect(transport.attach).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ existingPtyId: eagerPtyId })
|
||||
)
|
||||
expect(transport.connect).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ sessionId: eagerPtyId })
|
||||
)
|
||||
const { hasPtySerializer } = await import('./pty-buffer-serializer')
|
||||
expect(hasPtySerializer(eagerPtyId)).toBe(true)
|
||||
})
|
||||
|
||||
it('does not adopt another tab live eager PTY from a stale restored leaf binding', async () => {
|
||||
// Why: restored leaf bindings can outlive tab ownership. A global eager
|
||||
// buffer only proves the PTY is alive; ptyIdsByTabId proves this tab owns it.
|
||||
const otherTabPtyId = 'other-tab-eager-pty'
|
||||
vi.mocked(getEagerPtyBufferHandle).mockImplementation((ptyId: string) =>
|
||||
ptyId === otherTabPtyId ? { flush: () => '', dispose: () => {} } : undefined
|
||||
)
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport()
|
||||
transport.connect.mockImplementation(async (opts: { sessionId?: string }) => {
|
||||
if (opts.sessionId) {
|
||||
return { id: opts.sessionId }
|
||||
}
|
||||
return 'fresh-pty'
|
||||
})
|
||||
transportFactoryQueue.push(transport)
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
tabsByWorktree: {
|
||||
'wt-1': [
|
||||
{ id: 'tab-1', ptyId: 'tab-pty' },
|
||||
{ id: 'tab-2', ptyId: otherTabPtyId }
|
||||
]
|
||||
},
|
||||
ptyIdsByTabId: {
|
||||
'tab-1': ['tab-pty'],
|
||||
'tab-2': [otherTabPtyId]
|
||||
}
|
||||
} as StoreState
|
||||
const deps = createDeps({
|
||||
restoredLeafId: LEAF_1,
|
||||
restoredPtyIdByLeafId: { [LEAF_1]: otherTabPtyId }
|
||||
})
|
||||
|
||||
connectPanePty(createPane(1) as never, createManager(1) as never, deps as never)
|
||||
await flushAsyncTicks()
|
||||
|
||||
expect(transport.attach).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ existingPtyId: otherTabPtyId })
|
||||
)
|
||||
expect(transport.connect).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ sessionId: otherTabPtyId })
|
||||
)
|
||||
expect(deps.updateTabPtyId).not.toHaveBeenCalledWith('tab-1', otherTabPtyId)
|
||||
})
|
||||
|
||||
it('spawns a fresh PTY when a restored daemon split session cannot reattach', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport()
|
||||
|
||||
@@ -75,6 +75,7 @@ import {
|
||||
} from './terminal-bracketed-paste'
|
||||
import { createCommandCodeOutputStatusDetector } from './command-code-output-status'
|
||||
import type { PtyDataMeta } from './pty-dispatcher'
|
||||
import { getEagerPtyBufferHandle } from './pty-dispatcher'
|
||||
import { createTerminalGitHubPRLinkDetector } from '@/lib/terminal-github-pr-link-detector'
|
||||
import { installConptyDeviceAttributesHandler } from './terminal-conpty-device-attributes'
|
||||
import {
|
||||
@@ -3031,6 +3032,24 @@ export function connectPanePty(
|
||||
restoredSessionId && restoredSessionId !== detachedLivePtyId
|
||||
? restoredSessionId
|
||||
: detachedLivePtyId
|
||||
const currentTabLivePtyIds = storeSnapshot.ptyIdsByTabId[deps.tabId] ?? []
|
||||
const candidateHasEagerBuffer = Boolean(
|
||||
candidateReattachSessionId &&
|
||||
!isRemoteRuntimePtyId(candidateReattachSessionId) &&
|
||||
getEagerPtyBufferHandle(candidateReattachSessionId)
|
||||
)
|
||||
// Why: a still-live locally-spawned PTY (e.g. a background automation agent
|
||||
// launched before its tab mounts) keeps an eager buffer until a pane adopts
|
||||
// it. Such a PTY must be adopted via attach()+replay, not re-connected as a
|
||||
// daemon session — connect({ sessionId }) on a non-session ptyId spawns a
|
||||
// fresh shell and orphans the live agent. Presence of an eager buffer plus
|
||||
// current-tab live ownership is the discriminator; route these to attach.
|
||||
const eagerLivePtyId =
|
||||
candidateReattachSessionId &&
|
||||
candidateHasEagerBuffer &&
|
||||
currentTabLivePtyIds.includes(candidateReattachSessionId)
|
||||
? candidateReattachSessionId
|
||||
: null
|
||||
// Why: daemon session IDs encode `${worktreeId}@@${uuid}`. After a daemon
|
||||
// crash + cold restore, corrupted or stale session-to-tab mappings can
|
||||
// cause a tab in workspace A to hold a ptyId from workspace B. Restoring
|
||||
@@ -3039,6 +3058,7 @@ export function connectPanePty(
|
||||
const deferredReattachSessionId =
|
||||
candidateReattachSessionId &&
|
||||
!isRemoteRuntimePtyId(candidateReattachSessionId) &&
|
||||
!candidateHasEagerBuffer &&
|
||||
isSessionOwnedByWorktree(candidateReattachSessionId, deps.worktreeId)
|
||||
? candidateReattachSessionId
|
||||
: null
|
||||
@@ -3131,11 +3151,14 @@ export function connectPanePty(
|
||||
prepareColdRestoreAgentResumeCommand()
|
||||
startFreshSpawn()
|
||||
})
|
||||
} else if (detachedRemoteLeafPtyId || detachedLivePtyId) {
|
||||
} else if (detachedRemoteLeafPtyId || detachedLivePtyId || eagerLivePtyId) {
|
||||
// Why: mirrored web terminal layouts mount one pane per host leaf.
|
||||
// Later leaves already have a pane transport, but must still attach to
|
||||
// their exact remote PTY instead of spawning replacement host tabs.
|
||||
const attachPtyId = detachedRemoteLeafPtyId ?? detachedLivePtyId!
|
||||
// eagerLivePtyId covers a still-live background PTY (e.g. an automation
|
||||
// agent) whose restored id may not equal the tab ptyId yet still has a
|
||||
// live eager buffer to adopt.
|
||||
const attachPtyId = detachedRemoteLeafPtyId ?? detachedLivePtyId ?? eagerLivePtyId!
|
||||
recordPtyConnectDiagnostic(`pane=${pane.id} -> ATTACH detached=${attachPtyId}`)
|
||||
allowInitialIdleCacheSeed = false
|
||||
// Why: surface synchronous attach failures (e.g., the PTY died between
|
||||
@@ -3161,6 +3184,9 @@ export function connectPanePty(
|
||||
deps.syncPanePtyLayoutBinding(pane.id, attachPtyId)
|
||||
deps.updateTabPtyId(deps.tabId, attachPtyId)
|
||||
agentCompletionCoordinator.startProcessTracking()
|
||||
if (attachPtyId === eagerLivePtyId) {
|
||||
registerPaneSerializerFor(attachPtyId)
|
||||
}
|
||||
} catch (err) {
|
||||
reportError(err instanceof Error ? err.message : String(err))
|
||||
deps.clearTabPtyId(deps.tabId, attachPtyId)
|
||||
|
||||
@@ -0,0 +1,124 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { RuntimeSyncWindowGraph } from '../../../shared/runtime-types'
|
||||
import type { AppState } from '../store/types'
|
||||
import type { TerminalTab } from '../../../shared/types'
|
||||
|
||||
// Why: Part B publishes never-mounted background automation tabs into the
|
||||
// runtime graph, gated on a live eager buffer. Stub the eager-buffer lookup so
|
||||
// the test can flip "still-live unmounted PTY" on and off without the real IPC
|
||||
// dispatcher, and spy on the anomaly warning to prove it stays scoped to mounted
|
||||
// tabs.
|
||||
const { warnTerminalLifecycleAnomaly } = vi.hoisted(() => ({
|
||||
warnTerminalLifecycleAnomaly: vi.fn()
|
||||
}))
|
||||
vi.mock('@/components/terminal-pane/pty-dispatcher', async (importOriginal) => {
|
||||
const actual = await importOriginal<Record<string, unknown>>()
|
||||
return { ...actual, getEagerPtyBufferHandle: vi.fn(() => undefined) }
|
||||
})
|
||||
vi.mock('@/components/terminal-pane/terminal-lifecycle-diagnostics', async (importOriginal) => {
|
||||
const actual = await importOriginal<Record<string, unknown>>()
|
||||
return { ...actual, warnTerminalLifecycleAnomaly }
|
||||
})
|
||||
|
||||
import { getEagerPtyBufferHandle } from '@/components/terminal-pane/pty-dispatcher'
|
||||
import { setRuntimeGraphStoreStateGetter, setRuntimeGraphSyncEnabled } from './sync-runtime-graph'
|
||||
|
||||
const LEAF = '11111111-1111-4111-8111-111111111111'
|
||||
const AUTO_PTY = 'auto-bg-pty'
|
||||
|
||||
function makeState(overrides: Partial<AppState> = {}): AppState {
|
||||
return {
|
||||
tabsByWorktree: {},
|
||||
terminalLayoutsByTabId: {} as AppState['terminalLayoutsByTabId'],
|
||||
runtimePaneTitlesByTabId: {} as AppState['runtimePaneTitlesByTabId'],
|
||||
groupsByWorktree: {},
|
||||
activeGroupIdByWorktree: {},
|
||||
layoutByWorktree: {},
|
||||
unifiedTabsByWorktree: {},
|
||||
tabBarOrderByWorktree: {},
|
||||
activeFileId: null,
|
||||
activeFileIdByWorktree: {},
|
||||
openFiles: [],
|
||||
editorDrafts: {},
|
||||
activeTabId: null,
|
||||
...overrides
|
||||
} as AppState
|
||||
}
|
||||
|
||||
function makeAutomationTab(): TerminalTab {
|
||||
return {
|
||||
id: 'auto-tab-1',
|
||||
ptyId: AUTO_PTY,
|
||||
worktreeId: 'wt-1',
|
||||
title: 'Generate PO review prep brief',
|
||||
customTitle: null,
|
||||
color: null,
|
||||
sortOrder: 0,
|
||||
createdAt: 1
|
||||
}
|
||||
}
|
||||
|
||||
function automationState(): AppState {
|
||||
return makeState({
|
||||
tabsByWorktree: { 'wt-1': [makeAutomationTab()] } as AppState['tabsByWorktree'],
|
||||
terminalLayoutsByTabId: {
|
||||
'auto-tab-1': {
|
||||
root: { type: 'leaf', leafId: LEAF },
|
||||
activeLeafId: LEAF,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [LEAF]: AUTO_PTY }
|
||||
}
|
||||
} as AppState['terminalLayoutsByTabId']
|
||||
})
|
||||
}
|
||||
|
||||
async function flushMicrotasks(): Promise<void> {
|
||||
await Promise.resolve()
|
||||
await Promise.resolve()
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
setRuntimeGraphSyncEnabled(false)
|
||||
setRuntimeGraphStoreStateGetter(null)
|
||||
vi.mocked(getEagerPtyBufferHandle).mockReturnValue(undefined)
|
||||
warnTerminalLifecycleAnomaly.mockClear()
|
||||
vi.unstubAllGlobals()
|
||||
})
|
||||
|
||||
async function captureGraph(): Promise<RuntimeSyncWindowGraph> {
|
||||
const syncWindowGraph = vi.fn().mockResolvedValue(undefined)
|
||||
vi.stubGlobal('window', { api: { runtime: { syncWindowGraph } } })
|
||||
vi.stubGlobal('HTMLElement', class HTMLElement {})
|
||||
setRuntimeGraphStoreStateGetter(() => automationState())
|
||||
setRuntimeGraphSyncEnabled(true)
|
||||
await flushMicrotasks()
|
||||
expect(syncWindowGraph).toHaveBeenCalledTimes(1)
|
||||
return syncWindowGraph.mock.calls[0]![0] as RuntimeSyncWindowGraph
|
||||
}
|
||||
|
||||
describe('syncRuntimeGraph background automation tabs', () => {
|
||||
it('publishes an unmounted automation tab leaf when its PTY is still live (eager buffer present)', async () => {
|
||||
vi.mocked(getEagerPtyBufferHandle).mockImplementation((ptyId: string) =>
|
||||
ptyId === AUTO_PTY ? { flush: () => '', dispose: () => {} } : undefined
|
||||
)
|
||||
|
||||
const graph = await captureGraph()
|
||||
|
||||
expect(graph.leaves).toContainEqual(
|
||||
expect.objectContaining({ tabId: 'auto-tab-1', leafId: LEAF, ptyId: AUTO_PTY })
|
||||
)
|
||||
expect(graph.tabs).toContainEqual(expect.objectContaining({ tabId: 'auto-tab-1' }))
|
||||
// Why: the no-live-transport anomaly must stay scoped to mounted tabs; an
|
||||
// unmounted background tab legitimately has no live transport yet.
|
||||
expect(warnTerminalLifecycleAnomaly).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not publish an unmounted tab whose saved PTY is no longer live (no eager buffer)', async () => {
|
||||
vi.mocked(getEagerPtyBufferHandle).mockReturnValue(undefined)
|
||||
|
||||
const graph = await captureGraph()
|
||||
|
||||
expect(graph.leaves).not.toContainEqual(expect.objectContaining({ tabId: 'auto-tab-1' }))
|
||||
expect(graph.tabs).not.toContainEqual(expect.objectContaining({ tabId: 'auto-tab-1' }))
|
||||
})
|
||||
})
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
normalizeTerminalLayoutSnapshot
|
||||
} from '@/components/terminal-pane/layout-serialization'
|
||||
import { warnTerminalLifecycleAnomaly } from '@/components/terminal-pane/terminal-lifecycle-diagnostics'
|
||||
import { getEagerPtyBufferHandle } from '@/components/terminal-pane/pty-dispatcher'
|
||||
import { createBrowserUuid } from '@/lib/browser-uuid'
|
||||
import type { PaneManager } from '@/lib/pane-manager/pane-manager'
|
||||
import { resolveLeafIdForManager } from '@/lib/pane-manager/pane-key-resolution'
|
||||
@@ -570,6 +571,55 @@ async function syncRuntimeGraph(): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
// Why: background automation tabs spawn their agent PTY eagerly and are created
|
||||
// inactive, so they never mount a TerminalPane and never enter `registeredTabs`.
|
||||
// Without this pass their leaf+ptyId is never published, so the runtime treats
|
||||
// the live agent PTY as orphaned (surfaced as a synthetic `pty:<id>` terminal)
|
||||
// and `orca terminal list` / session-reuse can't see the real tab. Publish them
|
||||
// from the persisted layout, gated on a live eager buffer so we only adopt a
|
||||
// still-running unmounted PTY (never a stale saved ptyId).
|
||||
for (const [worktreeId, tabs] of Object.entries(state.tabsByWorktree)) {
|
||||
for (const tab of tabs) {
|
||||
if (registeredTabs.has(tab.id) || isWebOnlyMirroredTerminalTab(state, tab)) {
|
||||
continue
|
||||
}
|
||||
const layout = state.terminalLayoutsByTabId[tab.id]
|
||||
const savedPtyIdsByLeafId = layout?.ptyIdsByLeafId
|
||||
if (!savedPtyIdsByLeafId) {
|
||||
continue
|
||||
}
|
||||
const liveLeaves = Object.entries(savedPtyIdsByLeafId).filter(
|
||||
([leafId, ptyId]) =>
|
||||
typeof ptyId === 'string' &&
|
||||
ptyId.length > 0 &&
|
||||
isTerminalLeafId(leafId) &&
|
||||
Boolean(getEagerPtyBufferHandle(ptyId))
|
||||
)
|
||||
if (liveLeaves.length === 0) {
|
||||
continue
|
||||
}
|
||||
const title = resolveRuntimeTerminalTitle(tab, generatedTitlesEnabled)
|
||||
graph.tabs.push({
|
||||
tabId: tab.id,
|
||||
worktreeId,
|
||||
title,
|
||||
activeLeafId: layout?.activeLeafId ?? liveLeaves[0][0],
|
||||
layout: layout?.root ?? fallbackLayoutForLeafIds(liveLeaves.map(([leafId]) => leafId))
|
||||
})
|
||||
liveLeaves.forEach(([leafId, ptyId], index) => {
|
||||
graph.leaves.push({
|
||||
tabId: tab.id,
|
||||
worktreeId,
|
||||
leafId,
|
||||
paneRuntimeId: index + 1,
|
||||
ptyId,
|
||||
paneTitle: null,
|
||||
title
|
||||
})
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const result = await window.api.runtime.syncWindowGraph(graph)
|
||||
getStoreState()?.setRuntimeAgentOrchestrationByPaneKey?.(
|
||||
|
||||
Reference in New Issue
Block a user