mirror of
https://github.com/stablyai/orca.git
synced 2026-10-02 16:02:15 +00:00
fix: enforce shared terminal close liveness policy
This commit is contained in:
committed by
Merge Sim
parent
f7d8d7f77a
commit
6fecc7865f
@@ -149,7 +149,15 @@ import {
|
||||
useActivityTerminalPortals,
|
||||
type ActivityTerminalPortalTarget
|
||||
} from './activity/activity-terminal-portal'
|
||||
import { isRemoteRuntimePtyId } from '@/runtime/runtime-terminal-inspection'
|
||||
import {
|
||||
inspectRuntimeTerminalProcess,
|
||||
isRemoteRuntimePtyId
|
||||
} from '@/runtime/runtime-terminal-inspection'
|
||||
import { collectTabPtyIds } from './terminal/running-terminal-close-guard'
|
||||
import {
|
||||
terminalCloseDecision,
|
||||
terminalCloseLivenessFromInspection
|
||||
} from '../../../shared/terminal-close-liveness'
|
||||
import {
|
||||
activateWebRuntimeSessionTab,
|
||||
createWebRuntimeSessionBrowserTab,
|
||||
@@ -555,20 +563,27 @@ function Terminal(): React.JSX.Element | null {
|
||||
return []
|
||||
}
|
||||
return worktreeTabs
|
||||
.flatMap((tab) => state.ptyIdsByTabId[tab.id] ?? [])
|
||||
.flatMap((tab) => collectTabPtyIds(state, tab.id))
|
||||
.filter((ptyId) => !isRemoteRuntimePtyId(ptyId))
|
||||
}
|
||||
)
|
||||
if (localPtyIds.length > 0) {
|
||||
void Promise.all(localPtyIds.map((id) => window.api.pty.hasChildProcesses(id))).then(
|
||||
(results) => {
|
||||
if (results.some(Boolean)) {
|
||||
setWindowCloseDialogOpen(true)
|
||||
} else {
|
||||
confirmNativeWindowClose()
|
||||
}
|
||||
void Promise.allSettled(
|
||||
localPtyIds.map((id) => inspectRuntimeTerminalProcess(state.settings, id))
|
||||
).then((results) => {
|
||||
const hasBusyPty = results.some((result) => {
|
||||
const liveness =
|
||||
result.status === 'fulfilled'
|
||||
? terminalCloseLivenessFromInspection(result.value)
|
||||
: 'unverifiable'
|
||||
return terminalCloseDecision(liveness) === 'prompt'
|
||||
})
|
||||
if (hasBusyPty) {
|
||||
setWindowCloseDialogOpen(true)
|
||||
} else {
|
||||
confirmNativeWindowClose()
|
||||
}
|
||||
)
|
||||
})
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
@@ -58,6 +58,10 @@ import { useTerminalFontZoom } from './useTerminalFontZoom'
|
||||
import CloseTerminalDialog, { type CloseTerminalDialogCopyKind } from './CloseTerminalDialog'
|
||||
import { resolveLeafCloseCopyKind } from '../terminal/terminal-close-copy-kind'
|
||||
import { RUNNING_CLOSE_PROBE_TIMEOUT_MS } from '../terminal/running-terminal-close-guard'
|
||||
import {
|
||||
terminalCloseDecision,
|
||||
terminalCloseLivenessFromInspection
|
||||
} from '../../../../shared/terminal-close-liveness'
|
||||
import CodexRestartChip from '../CodexRestartChip'
|
||||
import { MobileDriverOverlay } from './MobileDriverOverlay'
|
||||
import { stripSshReconnectOwnedErrorLines, TerminalErrorToast } from './TerminalErrorToast'
|
||||
@@ -1249,8 +1253,9 @@ function TerminalPane(
|
||||
.then((process) => {
|
||||
clearTimeout(probeTimeout)
|
||||
decide(() => {
|
||||
const liveness = terminalCloseLivenessFromInspection(process)
|
||||
if (
|
||||
!process.hasChildProcesses ||
|
||||
terminalCloseDecision(liveness) === 'close' ||
|
||||
settings?.skipCloseTerminalWithRunningProcessConfirm
|
||||
) {
|
||||
executeClosePane(paneId)
|
||||
@@ -1259,10 +1264,17 @@ function TerminalPane(
|
||||
}
|
||||
})
|
||||
})
|
||||
// Why: if the child-process probe rejects (wedged IPC, legacy provider), close anyway — Cmd+W doing nothing is worse than closing a pane with a child.
|
||||
// A rejected probe is unverifiable, not evidence of an exited pane.
|
||||
.catch(() => {
|
||||
clearTimeout(probeTimeout)
|
||||
decide(() => executeClosePane(paneId))
|
||||
// A rejected probe is unverifiable, not evidence of an idle pane.
|
||||
decide(() => {
|
||||
if (settings?.skipCloseTerminalWithRunningProcessConfirm) {
|
||||
executeClosePane(paneId)
|
||||
} else {
|
||||
confirmClose()
|
||||
}
|
||||
})
|
||||
})
|
||||
},
|
||||
[executeClosePane, getCloseDialogCopyKind]
|
||||
|
||||
@@ -181,18 +181,18 @@ describe('guardRunningTerminalClose', () => {
|
||||
expect(onClose).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('fails open and closes when the probe rejects (wedged relay / legacy provider)', async () => {
|
||||
it('prompts when the probe rejects because its liveness is unverifiable', async () => {
|
||||
inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('rpc_timeout'))
|
||||
const onClose = vi.fn()
|
||||
|
||||
guard(onClose)
|
||||
await settleProbe()
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1)
|
||||
expect(visibleRequest()).toBeNull()
|
||||
expect(onClose).not.toHaveBeenCalled()
|
||||
expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' })
|
||||
})
|
||||
|
||||
it('fails open when a remote handle reports the inspection as unavailable', async () => {
|
||||
it('prompts when a remote handle reports the inspection as unavailable', async () => {
|
||||
inspectRuntimeTerminalProcessMock.mockResolvedValue({
|
||||
foregroundProcess: null,
|
||||
hasChildProcesses: true,
|
||||
@@ -203,8 +203,8 @@ describe('guardRunningTerminalClose', () => {
|
||||
guard(onClose)
|
||||
await settleProbe()
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1)
|
||||
expect(visibleRequest()).toBeNull()
|
||||
expect(onClose).not.toHaveBeenCalled()
|
||||
expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' })
|
||||
})
|
||||
|
||||
it('prompts once for a split tab where only the second pane is busy', async () => {
|
||||
@@ -392,10 +392,8 @@ describe('guardRunningTerminalClose', () => {
|
||||
})
|
||||
|
||||
// Why: an SSH drop zeroes ptyIdsByTabId while the layout still names the pane. The stale
|
||||
// binding is probed, that probe fails on the dead link, and the close falls open — so a
|
||||
// reconnecting tab stays closable instead of being blocked behind a prompt for a pty
|
||||
// nobody can reach. Documented so the behavior is a decision, not an accident.
|
||||
it('closes a reconnecting ssh tab whose pty ids were already zeroed', async () => {
|
||||
// binding is probed, but a dead link is unverifiable rather than evidence of an exited PTY.
|
||||
it('prompts for a reconnecting ssh tab whose pty ids were already zeroed', async () => {
|
||||
setState({ ptyIdsByTabId: { 'tab-1': [] } })
|
||||
inspectRuntimeTerminalProcessMock.mockRejectedValue(new Error('ssh_disconnected'))
|
||||
const onClose = vi.fn()
|
||||
@@ -403,7 +401,7 @@ describe('guardRunningTerminalClose', () => {
|
||||
guard(onClose)
|
||||
await settleProbe()
|
||||
|
||||
expect(onClose).toHaveBeenCalledTimes(1)
|
||||
expect(visibleRequest()).toBeNull()
|
||||
expect(onClose).not.toHaveBeenCalled()
|
||||
expect(visibleRequest()).toMatchObject({ terminalTabId: 'tab-1' })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -4,6 +4,10 @@ import { useRunningTerminalCloseConfirmStore } from '@/store/running-terminal-cl
|
||||
import type { TerminalTabCloseReason } from '@/store/slices/terminal-tab-retirement'
|
||||
import type { AppState } from '@/store/types'
|
||||
import { resolveBusyPtyCloseCopyKind } from './terminal-close-copy-kind'
|
||||
import {
|
||||
terminalCloseDecision,
|
||||
terminalCloseLivenessFromInspection
|
||||
} from '../../../../shared/terminal-close-liveness'
|
||||
|
||||
export type RunningTerminalCloseGuardOptions = {
|
||||
force?: boolean
|
||||
@@ -41,9 +45,9 @@ export function shouldConfirmRunningTerminalClose(
|
||||
/** Every PTY the tab could still own. `ptyIdsByTabId` is the liveness map the rest of the
|
||||
* app reads, but a mounting pane is bound into the layout before the map catches up, and
|
||||
* the store's own teardown collector unions both for exactly that reason — reading only
|
||||
* the map would let a close slip through the window with no prompt. A stale id costs
|
||||
* nothing: its probe fails and the guard falls open. */
|
||||
function collectTabPtyIds(
|
||||
* the map would let a close slip through the window with no prompt. A stale id is retained
|
||||
* as an unverifiable candidate so a lost host cannot look exited. */
|
||||
export function collectTabPtyIds(
|
||||
state: Pick<AppState, 'ptyIdsByTabId' | 'terminalLayoutsByTabId'>,
|
||||
terminalTabId: string
|
||||
): string[] {
|
||||
@@ -127,16 +131,15 @@ export function guardRunningTerminalClose(params: {
|
||||
if (decided) {
|
||||
return
|
||||
}
|
||||
// Why: fail open on an *answered* probe, matching the Cmd+W pane path — a rejection
|
||||
// (wedged relay, legacy provider) or a stale remote handle is not evidence of a live
|
||||
// child, and a close button that silently does nothing is worse than closing a busy tab.
|
||||
// Rejections are unverifiable, not evidence of an exited PTY; the shared table keeps
|
||||
// tab, pane, and window closes conservative when their execution host cannot answer.
|
||||
const busyPtyIds = ptyIds.filter((_, index) => {
|
||||
const result = results[index]
|
||||
return (
|
||||
result?.status === 'fulfilled' &&
|
||||
result.value.hasChildProcesses &&
|
||||
result.value.unavailable !== true
|
||||
)
|
||||
const liveness =
|
||||
result?.status === 'fulfilled'
|
||||
? terminalCloseLivenessFromInspection(result.value)
|
||||
: 'unverifiable'
|
||||
return terminalCloseDecision(liveness) === 'prompt'
|
||||
})
|
||||
if (busyPtyIds.length === 0) {
|
||||
closeNow()
|
||||
|
||||
@@ -101,6 +101,21 @@ describe('#10142 close confirmation policy is the same for keyboard and mouse',
|
||||
)
|
||||
})
|
||||
|
||||
it('uses the shared three-state close table in every destructive desktop guard', () => {
|
||||
const terminalSource = readFileSync(join(__dirname, '../Terminal.tsx'), 'utf8')
|
||||
const tabSource = readFileSync(join(__dirname, './running-terminal-close-guard.ts'), 'utf8')
|
||||
const paneSource = readFileSync(join(__dirname, '../terminal-pane/TerminalPane.tsx'), 'utf8')
|
||||
|
||||
for (const source of [terminalSource, tabSource, paneSource]) {
|
||||
expect(source).toContain('terminalCloseDecision')
|
||||
expect(source).toContain('terminalCloseLivenessFromInspection')
|
||||
}
|
||||
expect(terminalSource).toContain('collectTabPtyIds(state, tab.id)')
|
||||
expect(terminalSource).not.toContain('window.api.pty.hasChildProcesses')
|
||||
expect(paneSource).not.toContain('!process.hasChildProcesses')
|
||||
expect(tabSource).not.toContain('result.value.hasChildProcesses')
|
||||
})
|
||||
|
||||
// Control: the harness does observe a guard when one exists — pinning blocks the same mouse close.
|
||||
it('mouse close routes a pinned tab through its confirmation guard', () => {
|
||||
const state = stateWithBusyTerminalTab(closeTab)
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
TERMINAL_CLOSE_DECISION_BY_LIVENESS,
|
||||
terminalCloseDecision,
|
||||
terminalCloseLivenessFromInspection
|
||||
} from './terminal-close-liveness'
|
||||
|
||||
describe('terminal close liveness policy', () => {
|
||||
it.each([
|
||||
['live', 'prompt'],
|
||||
['unverifiable', 'prompt'],
|
||||
['exited', 'close']
|
||||
] as const)('maps %s to the shared %s decision', (liveness, expected) => {
|
||||
expect(terminalCloseDecision(liveness)).toBe(expected)
|
||||
expect(TERMINAL_CLOSE_DECISION_BY_LIVENESS[liveness]).toBe(expected)
|
||||
})
|
||||
|
||||
it('does not turn an unavailable or missing inspection into an exited verdict', () => {
|
||||
expect(terminalCloseLivenessFromInspection(undefined)).toBe('unverifiable')
|
||||
expect(
|
||||
terminalCloseLivenessFromInspection({ hasChildProcesses: false, unavailable: true })
|
||||
).toBe('unverifiable')
|
||||
})
|
||||
|
||||
it('treats a confirmed child process as live and a complete idle response as exited', () => {
|
||||
expect(terminalCloseLivenessFromInspection({ hasChildProcesses: true })).toBe('live')
|
||||
expect(terminalCloseLivenessFromInspection({ hasChildProcesses: false })).toBe('exited')
|
||||
})
|
||||
|
||||
it('lets composite host evidence poison the close even when the legacy scalar is false', () => {
|
||||
expect(
|
||||
terminalCloseLivenessFromInspection({
|
||||
hasChildProcesses: false,
|
||||
processEvidence: {
|
||||
foreground: { verdict: 'unverifiable' },
|
||||
children: { verdict: 'exited' }
|
||||
}
|
||||
})
|
||||
).toBe('unverifiable')
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,58 @@
|
||||
/**
|
||||
* The only liveness states a destructive terminal close may consume.
|
||||
*
|
||||
* Unknown inspection is deliberately represented as `unverifiable`: losing
|
||||
* contact with the execution host is not evidence that its process exited.
|
||||
*/
|
||||
export type TerminalCloseLiveness = PtyLivenessVerdict['status']
|
||||
|
||||
export type TerminalCloseDecision = 'prompt' | 'close'
|
||||
|
||||
/** One policy table shared by tab, pane, and native-window close guards. */
|
||||
export const TERMINAL_CLOSE_DECISION_BY_LIVENESS: Readonly<
|
||||
Record<TerminalCloseLiveness, TerminalCloseDecision>
|
||||
> = Object.freeze({
|
||||
live: 'prompt',
|
||||
unverifiable: 'prompt',
|
||||
exited: 'close'
|
||||
})
|
||||
|
||||
export type TerminalCloseInspection = {
|
||||
hasChildProcesses: boolean
|
||||
unavailable?: true
|
||||
/** Optional composite evidence published by newer paired hosts (#17444). */
|
||||
processEvidence?: {
|
||||
foreground?: { verdict: TerminalCloseLiveness }
|
||||
children?: { verdict: TerminalCloseLiveness }
|
||||
}
|
||||
}
|
||||
|
||||
/** Normalize one inspection response before any close guard makes a decision. */
|
||||
export function terminalCloseLivenessFromInspection(
|
||||
inspection: TerminalCloseInspection | null | undefined
|
||||
): TerminalCloseLiveness {
|
||||
if (!inspection || inspection.unavailable === true) {
|
||||
return 'unverifiable'
|
||||
}
|
||||
const evidence = inspection.processEvidence
|
||||
if (evidence) {
|
||||
const verdicts = [evidence.foreground?.verdict, evidence.children?.verdict]
|
||||
if (verdicts.includes('unverifiable')) {
|
||||
return 'unverifiable'
|
||||
}
|
||||
if (verdicts.includes('live')) {
|
||||
return 'live'
|
||||
}
|
||||
if (verdicts.every((verdict) => verdict === 'exited')) {
|
||||
return 'exited'
|
||||
}
|
||||
return 'unverifiable'
|
||||
}
|
||||
return inspection.hasChildProcesses ? 'live' : 'exited'
|
||||
}
|
||||
|
||||
/** Resolve the table entry every destructive close guard must use. */
|
||||
export function terminalCloseDecision(liveness: TerminalCloseLiveness): TerminalCloseDecision {
|
||||
return TERMINAL_CLOSE_DECISION_BY_LIVENESS[liveness]
|
||||
}
|
||||
import type { PtyLivenessVerdict } from './pty-liveness-verdict'
|
||||
Reference in New Issue
Block a user