diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index f5857a33e13..e8ca5aec21a 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -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 } } diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 045dd8aba8d..7293421bdff 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -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] diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts index 3dc808e6abc..ab33ad6d616 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.test.ts @@ -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' }) }) }) diff --git a/src/renderer/src/components/terminal/running-terminal-close-guard.ts b/src/renderer/src/components/terminal/running-terminal-close-guard.ts index 73f63fc85f7..33e2cb48230 100644 --- a/src/renderer/src/components/terminal/running-terminal-close-guard.ts +++ b/src/renderer/src/components/terminal/running-terminal-close-guard.ts @@ -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, 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() diff --git a/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts b/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts index 3ad4229e233..f528cc06b1a 100644 --- a/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts +++ b/src/renderer/src/components/terminal/terminal-close-confirm-keyboard-vs-mouse.test.ts @@ -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) diff --git a/src/shared/terminal-close-liveness.test.ts b/src/shared/terminal-close-liveness.test.ts new file mode 100644 index 00000000000..23dd5982ac4 --- /dev/null +++ b/src/shared/terminal-close-liveness.test.ts @@ -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') + }) +}) diff --git a/src/shared/terminal-close-liveness.ts b/src/shared/terminal-close-liveness.ts new file mode 100644 index 00000000000..2e10d1c897b --- /dev/null +++ b/src/shared/terminal-close-liveness.ts @@ -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 +> = 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'