mirror of
https://github.com/stablyai/orca.git
synced 2026-10-24 08:03:58 +00:00
fix(terminal): put the unreachable-pane guards in one place
Correctness round 3. The leaf-keying from round 2 came back clean, twice and independently. But the renderer produced findings for a third consecutive round, and they were all one shape: `publishUnreachablePane` is called from seven sites, each needing the same guards, and a different one was missing at each. So this stops patching sites and moves the guards into the publisher: - `disposed` — a late rejection republished a card for a numeric pane id that had already been reused, giving a fresh pane a phantom card whose actions closed over a dead session. - `connectionId` — the deferred catch is not SSH-only. A local/daemon pane could be shown an SSH ambiguity card it can never clear, and would loop: retry remounts, reattach rejects, card returns. Its provider is authoritative about its own ptys, which is exactly why the two branches in handleReattachResult already had this guard — and why the catch needed it too. Two more real defects in the banner's own actions: - "Start a new terminal" passed `null` to suppress the saved agent startup, but connect FALLS BACK to the startup its transport was constructed with whenever a per-call field is absent (pty-transport.ts). So the "fresh" shell could resume the same provider session — the duplicate transcript this pane exists to prevent, for the third time in this button. `suppressSavedStartup` makes the suppression explicit; `??` means passing null could never have worked. - It also cleared both durable bindings BEFORE the spawn, and discarded the promise. A spawn that resolves null left the pane blank, unbound, and with no way back to a shell that may still be running. Bindings are now cleared only once a replacement actually starts, and the card returns if it does not. - "Try again" voided its boolean; a declined remount left no card and no shell, strictly worse than the toast it replaced. It now republishes. The strengthened clause is the point: the old one asserted the ABSENCE of per-call startup fields on a mocked transport, which passes whether or not the production fallback fires. It now asserts the explicit suppression, and reddens when the flag is dropped. Four fixtures were under-specified — they drive SSH reattach scenarios but never seeded an SSH repo, so `connectionId` was null and they had been passing without the pane being SSH at all. Seeded, not weakened. 43,256 pass; the 7 failures are pre-existing on clean main in this environment.
This commit is contained in:
Neil
@@ -9319,6 +9319,10 @@ describe('connectPanePty', () => {
|
||||
// replacement PTY exists, whatever route created it.
|
||||
it('drops stale callbacks but delivers fresh replacement output after an old replay clear', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
const { getTerminalScrollIntentKind, markTerminalPinnedViewport } =
|
||||
await import('@/lib/pane-manager/terminal-scroll-intent')
|
||||
const transport = createMockTransport('tab-pty')
|
||||
@@ -9568,6 +9572,11 @@ describe('connectPanePty', () => {
|
||||
})
|
||||
transportFactoryQueue.push(transport)
|
||||
setReattachPaneTitle('Cursor Agent')
|
||||
// After setReattachPaneTitle: it rewrites mockStoreState and would drop this.
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
|
||||
const pane = createPane(1)
|
||||
const textarea = {} as HTMLTextAreaElement
|
||||
@@ -9669,6 +9678,12 @@ describe('connectPanePty', () => {
|
||||
})
|
||||
transportFactoryQueue.push(transport)
|
||||
setReattachPaneTitle('Cursor Agent')
|
||||
// SSH repo: the disconnected card is SSH-only, and setReattachPaneTitle rewrites
|
||||
// mockStoreState, so this must come after it.
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
|
||||
const pane = createPane(1)
|
||||
const textarea = {} as HTMLTextAreaElement
|
||||
@@ -9767,6 +9782,12 @@ describe('connectPanePty', () => {
|
||||
|
||||
it('does not respawn a pane whose restore was answered with a not-found', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
// SSH repo: these are SSH reattach scenarios, and the disconnected card is SSH-only by design.
|
||||
// Without this the fixture has no connectionId and the pane is not SSH at all.
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
const transport = createMockTransport('tab-pty')
|
||||
transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) => {
|
||||
if (sessionId) {
|
||||
@@ -9809,6 +9830,12 @@ describe('connectPanePty', () => {
|
||||
// agent process on one transcript. That is the very defect this pane exists to prevent.
|
||||
it('starts a fresh shell rather than resuming the agent when the user starts a new terminal', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
// SSH repo: these are SSH reattach scenarios, and the disconnected card is SSH-only by design.
|
||||
// Without this the fixture has no connectionId and the pane is not SSH at all.
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
const transport = createMockTransport('tab-pty')
|
||||
const spawnOptions: Record<string, unknown>[] = []
|
||||
transport.connect.mockImplementation(async (options: { sessionId?: string }) => {
|
||||
@@ -9867,6 +9894,11 @@ describe('connectPanePty', () => {
|
||||
// Nothing else retracts the card, so the action must clear it or it sits over a live shell.
|
||||
expect(onPtyRecoveryState).toHaveBeenCalledWith(pane.id, null)
|
||||
expect(deps.clearTabPtyId).toHaveBeenCalledWith('tab-1', 'tab-pty')
|
||||
// The transport falls back to the startup it was CONSTRUCTED with whenever a per-call field is
|
||||
// absent, so omitting them is not enough — a "fresh" shell would resume the saved session. This
|
||||
// asserts the explicit suppression, which is the only thing that actually closes it. Asserting
|
||||
// the absence of per-call fields cannot: on a mock it passes either way.
|
||||
expect(spawnOptions[0]).toMatchObject({ suppressSavedStartup: true })
|
||||
// The resume path claims the launch config before spawning; a fresh shell has none to claim.
|
||||
expect(mockStoreState.registerAgentLaunchConfig).not.toHaveBeenCalled()
|
||||
expect(spawnOptions).toHaveLength(1)
|
||||
|
||||
@@ -552,6 +552,10 @@ type PendingStartupCommand = {
|
||||
|
||||
type FreshSpawnOptions = {
|
||||
forceBlankRestoredViewport?: boolean
|
||||
/** Ignore the startup this transport was constructed with. Omitting the fields is not enough:
|
||||
* connect falls back to its constructor values, so a "fresh" shell would resume the saved agent
|
||||
* session — the duplicate transcript the disconnected pane exists to prevent. */
|
||||
suppressSavedStartup?: boolean
|
||||
}
|
||||
|
||||
type ColdRestoreAgentResumeStartup = PendingStartupCommand & {
|
||||
@@ -5129,6 +5133,13 @@ export function connectPanePty(
|
||||
* because nothing here knows whether the shell died.
|
||||
*/
|
||||
const publishUnreachablePane = (sessionId: string): void => {
|
||||
// Guards live HERE, not at the seven call sites. They are identical at every one, and the
|
||||
// review found three separate sites that had each forgotten a different one — a local pane
|
||||
// shown an SSH-specific card it can never clear, and a disposed pane republishing state onto
|
||||
// a reused numeric pane id. Centralising them makes the next call site correct by default.
|
||||
if (disposed || !connectionId) {
|
||||
return
|
||||
}
|
||||
// Both actions clear the banner before acting: nothing else retracts it. The app-SSH
|
||||
// transport does not publish recovery states, so a card left up would sit over a live shell
|
||||
// with armed buttons.
|
||||
@@ -5140,6 +5151,14 @@ export function connectPanePty(
|
||||
unreachablePane: {
|
||||
onRetry: () => {
|
||||
clearBanner()
|
||||
// Put the card back if the remount never happens. A stale generation, a refused remount
|
||||
// or a throw all return false without scheduling anything, and swallowing that leaves
|
||||
// the pane with no shell AND no action surface — worse than the toast this replaced.
|
||||
const republishIfDeclined = (scheduled: boolean): void => {
|
||||
if (!scheduled) {
|
||||
publishUnreachablePane(sessionId)
|
||||
}
|
||||
}
|
||||
// Why the generation and instance: a recovery declined by the cooldown only schedules
|
||||
// a deferred retry when it can identify the generation to remount. Without them a
|
||||
// second click inside the window is refused silently, which on a surface whose whole
|
||||
@@ -5150,18 +5169,29 @@ export function connectPanePty(
|
||||
reason: 'restore-blocked',
|
||||
terminalRecoveryGeneration,
|
||||
terminalRecoveryInstanceId: terminalRecoveryInstance.id
|
||||
})
|
||||
}).then(republishIfDeclined, () => republishIfDeclined(false))
|
||||
},
|
||||
onStartNewTerminal: () => {
|
||||
clearBanner()
|
||||
deps.clearExitedPanePtyLayoutBinding(pane.id, sessionId)
|
||||
deps.clearTabPtyId(deps.tabId, sessionId)
|
||||
// A NEW shell, not a resumed one. The old automatic respawn passed the cold-restore
|
||||
// startup, which carries the agent's providerSession — correct then, because it only
|
||||
// ran on proof the shell was gone. Here the shell is probably still alive, so resuming
|
||||
// would put a second agent process on one transcript: the exact defect this pane
|
||||
// exists to prevent. `null` starts fresh, which is also what the button says.
|
||||
startFreshSpawn(null, { forceBlankRestoredViewport: true })
|
||||
// A NEW shell, not a resumed one. The automatic respawn this replaced passed the
|
||||
// cold-restore startup, which carries the agent's providerSession — correct there,
|
||||
// because it only ran on proof the shell was gone. Here the shell is probably still
|
||||
// alive, so resuming would put a second agent process on one transcript. `null` alone
|
||||
// does not achieve that: connect falls back to the startup this transport was built
|
||||
// with, so the suppression has to be explicit.
|
||||
void startFreshSpawn(null, {
|
||||
forceBlankRestoredViewport: true,
|
||||
suppressSavedStartup: true
|
||||
}).then((freshPtyId) => {
|
||||
if (!freshPtyId) {
|
||||
// The replacement never started. Keep the old binding and put the card back, or the
|
||||
// pane is left blank and unbound with no way back to a shell that may still be up.
|
||||
publishUnreachablePane(sessionId)
|
||||
return
|
||||
}
|
||||
deps.clearExitedPanePtyLayoutBinding(pane.id, sessionId)
|
||||
deps.clearTabPtyId(deps.tabId, sessionId)
|
||||
})
|
||||
}
|
||||
}
|
||||
})
|
||||
@@ -5336,6 +5366,7 @@ export function connectPanePty(
|
||||
transportConnectInFlightSince = Date.now()
|
||||
const outputCallbacks = captureTransportOutputCallbacks(reportError)
|
||||
const spawnedRaw = transport.connect({
|
||||
...(options.suppressSavedStartup ? { suppressSavedStartup: true } : {}),
|
||||
url: '',
|
||||
cols,
|
||||
rows,
|
||||
|
||||
@@ -147,6 +147,10 @@ export type PtyTransport = {
|
||||
envToDelete?: string[]
|
||||
launchConfig?: SleepingAgentLaunchConfig
|
||||
resumeProviderSession?: AgentProviderSessionMetadata
|
||||
/** Start a genuinely new shell: ignore the startup this transport was constructed with.
|
||||
* Omitting the fields is not enough — connect falls back to its constructor values, so a
|
||||
* "fresh" spawn would silently resume the saved agent session. */
|
||||
suppressSavedStartup?: boolean
|
||||
launchToken?: string
|
||||
launchAgent?: TuiAgent
|
||||
startupCommandDelivery?: StartupCommandDelivery
|
||||
|
||||
@@ -818,16 +818,19 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
|
||||
...((options.envToDelete ?? envToDelete)
|
||||
? { envToDelete: options.envToDelete ?? envToDelete }
|
||||
: {}),
|
||||
command: options.command ?? command,
|
||||
...((options.launchConfig ?? launchConfig)
|
||||
command: options.suppressSavedStartup ? options.command : (options.command ?? command),
|
||||
...((options.suppressSavedStartup ? options.launchConfig : (options.launchConfig ?? launchConfig))
|
||||
? { launchConfig: options.launchConfig ?? launchConfig }
|
||||
: {}),
|
||||
...((options.resumeProviderSession ?? resumeProviderSession)
|
||||
...((options.suppressSavedStartup
|
||||
? options.resumeProviderSession
|
||||
: (options.resumeProviderSession ?? resumeProviderSession))
|
||||
? {
|
||||
resumeProviderSession: options.resumeProviderSession ?? resumeProviderSession
|
||||
resumeProviderSession:
|
||||
options.resumeProviderSession ?? resumeProviderSession
|
||||
}
|
||||
: {}),
|
||||
...((options.launchToken ?? launchToken)
|
||||
...((options.suppressSavedStartup ? options.launchToken : (options.launchToken ?? launchToken))
|
||||
? { launchToken: options.launchToken ?? launchToken }
|
||||
: {}),
|
||||
...((options.launchAgent ?? launchAgent)
|
||||
|
||||
Reference in New Issue
Block a user