fix(terminal): ask before closing the window when a terminal probe could not answer

The window-close guard asked `pty:hasChildProcesses`, whose local answer is a bare boolean: `LocalPtyProvider.hasChildProcesses` reads node-pty's foreground title and returns `false` both when the native read throws (a bare `catch`) and when it silently degrades to the spawned shell name. `results.some(Boolean)` was then false, so `confirmNativeWindowClose()` fired with no dialog and the window closed on live work the user was never asked about. Route the guard through `readPtyProcessInspectionEvidence` — the same reader #17024 adopted — and block on anything that is not a positively observed `exited`. A rejected inspection now asks too; the `Promise.all` it replaced had no catch, so a rejection left the window neither closed nor prompting. The prompt (probe, decision and dialog) moves out of Terminal.tsx unchanged so the decision is only reachable through the surface the user actually sees, and so the regression test can assert on that dialog in a real DOM.
2026-10-23 16:03:49 +00:00 · 2026-08-28 11:03:08 -07:00
parent c72d2cc98e
commit 0a4b2be8fd
4 changed files with 379 additions and 101 deletions
@@ -138,17 +138,14 @@ import {
 } from './terminal-pane/terminal-parked-tab-watchers'
 import { isMainTerminalSideEffectAuthorityForPty } from './terminal-pane/terminal-side-effect-facts-handler'
 import { appendUniqueOpenFileIds } from './terminal/unsaved-close-queue'
-import {
-  runWithWindowCloseCheckpointScope,
-  setWindowCloseRequestHandler
-} from './window-close-request-coordinator'
-import { showShutdownCheckpointFailureToast } from '@/lib/shutdown-checkpoint-failure-toast'
+import { setWindowCloseRequestHandler } from './window-close-request-coordinator'
 import {
  findActivityTerminalPortal,
  useActivityTerminalPortals,
  type ActivityTerminalPortalTarget
 } from './activity/activity-terminal-portal'
 import { isRemoteRuntimePtyId } from '@/runtime/runtime-terminal-inspection'
+import { useWindowCloseRunningProcessPrompt } from './terminal/window-close-running-process-prompt'
 import {
  activateWebRuntimeSessionTab,
  createWebRuntimeSessionBrowserTab,
@@ -519,60 +516,10 @@ function Terminal(): React.JSX.Element | null {
    closeDialogDebounceTimersRef.current.add(timer)
  }, [])

-  // Window close confirmation, shown for local terminals with running children (SSH terminals detach/persist via the relay).
-  const [windowCloseDialogOpen, setWindowCloseDialogOpen] = useState(false)
-
  // Why: defer confirmWindowClose() while tabs are dirty — the beforeunload guard preventDefault()s, so an immediate confirm leaves the window open with no UI.
  const windowCloseAfterDirtyRef = useRef<{ isQuitting: boolean } | null>(null)

-  const confirmNativeWindowClose = useCallback(() => {
-    // Why: capture only after every close guard has committed. A canceled child-
-    // process prompt must not consume App's synthetic/native unload guard.
-    const accepted = runWithWindowCloseCheckpointScope(() =>
-      window.dispatchEvent(new Event('beforeunload', { cancelable: true }))
-    )
-    if (!accepted) {
-      // Why: a checkpoint-vetoed quit used to die here with no dialog and no log,
-      // leaving SIGKILL as the only exit (#15352). The dirty-file veto publishes
-      // no reason — its deferred dialog flow already gives the user a surface.
-      showShutdownCheckpointFailureToast()
-      return
-    }
-    window.api.ui.confirmWindowClose()
-  }, [])
-
-  const proceedToNativeWindowClose = useCallback(
-    (isQuitting: boolean) => {
-      if (!isQuitting) {
-        const state = useAppStore.getState()
-        const localPtyIds = Object.entries(state.tabsByWorktree).flatMap(
-          ([worktreeId, worktreeTabs]) => {
-            const connectionId = getConnectionId(worktreeId)
-            if (connectionId !== null) {
-              return []
-            }
-            return worktreeTabs
-              .flatMap((tab) => state.ptyIdsByTabId[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()
-              }
-            }
-          )
-          return
-        }
-      }
-      confirmNativeWindowClose()
-    },
-    [confirmNativeWindowClose]
-  )
+  const { proceedToNativeWindowClose, windowCloseDialog } = useWindowCloseRunningProcessPrompt()

  const waitForFileClosed = useCallback((fileId: string, timeoutMs: number): Promise<boolean> => {
    if (!useAppStore.getState().openFiles.some((f) => f.id === fileId)) {
@@ -2800,51 +2747,7 @@ function Terminal(): React.JSX.Element | null {
        </DialogContent>
      </Dialog>

-      {/* Window close confirmation dialog */}
-      <Dialog
-        open={windowCloseDialogOpen}
-        onOpenChange={(open) => {
-          if (!open) {
-            setWindowCloseDialogOpen(false)
-          }
-        }}
-      >
-        <DialogContent className="max-w-sm" showCloseButton={false}>
-          <DialogHeader>
-            <DialogTitle className="text-sm">
-              {translate('auto.components.Terminal.2fa9c69ff3', 'Close Window?')}
-            </DialogTitle>
-            <DialogDescription className="text-xs">
-              {translate(
-                'auto.components.Terminal.7958465754',
-                'There are local terminals with running processes. Close the window anyway?'
-              )}
-            </DialogDescription>
-          </DialogHeader>
-          <DialogFooter className="gap-2">
-            <Button
-              type="button"
-              variant="outline"
-              size="sm"
-              onClick={() => setWindowCloseDialogOpen(false)}
-            >
-              {translate('auto.components.Terminal.f82e9f02df', 'Cancel')}
-            </Button>
-            <Button
-              type="button"
-              variant="destructive"
-              size="sm"
-              autoFocus
-              onClick={() => {
-                setWindowCloseDialogOpen(false)
-                confirmNativeWindowClose()
-              }}
-            >
-              {translate('auto.components.Terminal.73768427cf', 'Close')}
-            </Button>
-          </DialogFooter>
-        </DialogContent>
-      </Dialog>
+      {windowCloseDialog}
    </div>
  )
 }
@@ -0,0 +1,31 @@
+import type { GlobalSettings } from '../../../../shared/global-settings-types'
+import { readPtyProcessInspectionEvidence } from '../../../../shared/pty-process-inspection-evidence'
+import { inspectRuntimeTerminalProcess } from '@/runtime/runtime-terminal-inspection'
+
+/**
+ * Whether any local PTY must stop the window from closing silently.
+ *
+ * `exited` is the only verdict that closes with no prompt. A probe that could
+ * not answer is `unverifiable` (docs/reference/ssh-execution-boundary.md) and
+ * must never read as an idle shell: the degraded local read publishes the
+ * legacy collapse — the stable-cache shell name and `hasChildProcesses: false` —
+ * which is byte-identical to a genuinely idle pane unless the evidence is read.
+ * Unlike every other consumer of this evidence, closing here acts for the user
+ * and destroys the work the warning would have let them save, so unknown asks.
+ */
+export async function anyLocalPtyBlocksWindowClose(
+  settings: Pick<GlobalSettings, 'activeRuntimeEnvironmentId'> | null | undefined,
+  ptyIds: readonly string[]
+): Promise<boolean> {
+  const results = await Promise.allSettled(
+    ptyIds.map((ptyId) => inspectRuntimeTerminalProcess(settings, ptyId))
+  )
+  return results.some((result) =>
+    // Why rejected counts as blocking: a raised inspection answered nothing, and
+    // the Promise.all this replaced had no catch — a rejection left the window
+    // neither closed nor prompting.
+    result.status === 'rejected'
+      ? true
+      : readPtyProcessInspectionEvidence(result.value).children.verdict !== 'exited'
+  )
+}
@@ -0,0 +1,215 @@
+// @vitest-environment happy-dom
+import { act } from 'react'
+import { createRoot, type Root } from 'react-dom/client'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import {
+  buildPtyProcessInspectionWireResult,
+  type PtyProcessInspectionEvidence
+} from '../../../../shared/pty-process-inspection-evidence'
+
+/**
+ * The window-close guard is the one consumer of this evidence that acts FOR the
+ * user: a degraded probe read as "nothing is running" skips the prompt entirely
+ * and the window closes on live work. So every case is asserted on the dialog
+ * text rendered into a real DOM and on whether confirmWindowClose() fired —
+ * never on an intermediate boolean.
+ */
+
+const { getStateMock } = vi.hoisted(() => ({ getStateMock: vi.fn() }))
+
+vi.mock('@/store', () => ({ useAppStore: { getState: getStateMock } }))
+vi.mock('@/lib/connection-context', () => ({ getConnectionId: () => null }))
+vi.mock('@/lib/shutdown-checkpoint-failure-toast', () => ({
+  showShutdownCheckpointFailureToast: vi.fn()
+}))
+
+import { useWindowCloseRunningProcessPrompt } from './window-close-running-process-prompt'
+
+const PTY_ID = 'pty-local-1'
+const SHELL = 'zsh'
+const WARNING = 'There are local terminals with running processes.'
+
+/**
+ * What `LocalPtyProvider.inspectProcess` really publishes when the foreground
+ * scan is degraded (`classifyLocalPtyChildProcesses`, "pty title matches the
+ * shell while the foreground scan is degraded"): the legacy fields are the
+ * stable-cache shell name and `hasChildProcesses: false`, byte-identical to a
+ * genuinely idle shell. Composed through the real collapse builder so the
+ * fixture cannot drift from the producer.
+ */
+type InspectionShape = {
+  foregroundProcess: string | null
+  hasChildProcesses: boolean
+  unavailable?: true
+  processEvidence?: PtyProcessInspectionEvidence
+}
+
+function degradedLocalInspection(): InspectionShape {
+  return {
+    ...buildPtyProcessInspectionWireResult(
+      { verdict: 'unverifiable', reason: 'process table scan degraded' },
+      {
+        verdict: 'unverifiable',
+        reason: 'pty title matches the shell while the foreground scan is degraded'
+      }
+    ),
+    // The provider publishes its stable-cache legacy value, which for a pane
+    // that never ran an agent is the shell name.
+    foregroundProcess: SHELL
+  }
+}
+
+function observedIdleInspection(): InspectionShape {
+  return buildPtyProcessInspectionWireResult(
+    { verdict: 'observed', processName: SHELL },
+    { verdict: 'exited' }
+  )
+}
+
+function observedLiveInspection(): InspectionShape {
+  return buildPtyProcessInspectionWireResult(
+    { verdict: 'observed', processName: 'codex' },
+    { verdict: 'live' }
+  )
+}
+
+let root: Root | null = null
+let container: HTMLDivElement | null = null
+let confirmWindowClose: ReturnType<typeof vi.fn>
+let proceed: ((isQuitting: boolean) => void) | null = null
+
+function Harness(): React.ReactNode {
+  const prompt = useWindowCloseRunningProcessPrompt()
+  proceed = prompt.proceedToNativeWindowClose
+  return prompt.windowCloseDialog
+}
+
+/**
+ * Installs a PTY inspection that RESOLVES with a degraded answer — the shape the
+ * bug actually has. A throwing controller would be a different boundary; the
+ * local host answers, its probes just could not see.
+ */
+function installInspectProcess(inspectProcess: () => Promise<InspectionShape>): void {
+  ;(window as unknown as { api: unknown }).api = {
+    pty: {
+      inspectProcess: vi.fn(inspectProcess),
+      // Why the double publishes both: `pty:hasChildProcesses` is the legacy
+      // boolean route this guard used to take, and it is derived from the SAME
+      // host answer. A double that omitted it would let the pre-fix predicate
+      // look untestable instead of wrong.
+      hasChildProcesses: vi.fn(async () => (await inspectProcess()).hasChildProcesses === true)
+    },
+    ui: { confirmWindowClose }
+  }
+}
+
+async function runWindowClose(): Promise<void> {
+  await act(async () => {
+    proceed!(false)
+    await Promise.resolve()
+    await Promise.resolve()
+  })
+}
+
+function warningIsVisible(): boolean {
+  return document.body.textContent?.includes(WARNING) === true
+}
+
+beforeEach(() => {
+  confirmWindowClose = vi.fn()
+  getStateMock.mockReturnValue({
+    settings: { activeRuntimeEnvironmentId: null },
+    tabsByWorktree: { 'wt-1': [{ id: 'tab-1' }] },
+    ptyIdsByTabId: { 'tab-1': [PTY_ID] }
+  })
+  container = document.createElement('div')
+  document.body.appendChild(container)
+  root = createRoot(container)
+  act(() => {
+    root!.render(<Harness />)
+  })
+})
+
+afterEach(() => {
+  act(() => {
+    root?.unmount()
+  })
+  container?.remove()
+  root = null
+  container = null
+  proceed = null
+  document.body.innerHTML = ''
+  vi.unstubAllGlobals()
+})
+
+describe('window close with a degraded local process read', () => {
+  it('shows the running-processes warning instead of closing the window', async () => {
+    installInspectProcess(async () => degradedLocalInspection())
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(true)
+    expect(confirmWindowClose).not.toHaveBeenCalled()
+  })
+
+  it('still closes silently when the probe positively observed an idle shell', async () => {
+    installInspectProcess(async () => observedIdleInspection())
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(false)
+    expect(confirmWindowClose).toHaveBeenCalledTimes(1)
+  })
+
+  it('shows the warning for an observed live process', async () => {
+    installInspectProcess(async () => observedLiveInspection())
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(true)
+    expect(confirmWindowClose).not.toHaveBeenCalled()
+  })
+
+  it('leaves loss of contact unchanged: an unroutable handle publishes no verdict and closes', async () => {
+    // `pty:inspectProcess` answers this for an id it cannot route, and the relay
+    // path answers it on terminal_gone. No processEvidence rides along, so the
+    // legacy fields are the only answer the host can give.
+    installInspectProcess(async () => ({
+      foregroundProcess: null,
+      hasChildProcesses: false,
+      unavailable: true as const
+    }))
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(false)
+    expect(confirmWindowClose).toHaveBeenCalledTimes(1)
+  })
+
+  it('asks when the inspection raises instead of leaving the window stuck', async () => {
+    installInspectProcess(async () => {
+      throw new Error('inspect failed')
+    })
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(true)
+    expect(confirmWindowClose).not.toHaveBeenCalled()
+  })
+
+  it('reads a malformed foreign evidence payload as unverifiable, not as idle', async () => {
+    installInspectProcess(async () => ({
+      foregroundProcess: SHELL,
+      hasChildProcesses: false,
+      processEvidence: {
+        foreground: { verdict: 'bogus' },
+        children: { verdict: 'bogus' }
+      } as unknown as PtyProcessInspectionEvidence
+    }))
+
+    await runWindowClose()
+
+    expect(warningIsVisible()).toBe(true)
+    expect(confirmWindowClose).not.toHaveBeenCalled()
+  })
+})
@@ -0,0 +1,129 @@
+import { useCallback, useState, type ReactElement } from 'react'
+import { Button } from '@/components/ui/button'
+import {
+  Dialog,
+  DialogContent,
+  DialogDescription,
+  DialogFooter,
+  DialogHeader,
+  DialogTitle
+} from '@/components/ui/dialog'
+import { translate } from '@/i18n/i18n'
+import { getConnectionId } from '@/lib/connection-context'
+import { showShutdownCheckpointFailureToast } from '@/lib/shutdown-checkpoint-failure-toast'
+import { isRemoteRuntimePtyId } from '@/runtime/runtime-terminal-inspection'
+import { useAppStore } from '@/store'
+import { runWithWindowCloseCheckpointScope } from '../window-close-request-coordinator'
+import { anyLocalPtyBlocksWindowClose } from './window-close-running-process-evidence'
+
+export type WindowCloseRunningProcessPrompt = {
+  /** Probes the window's local PTYs, then either raises the confirmation or closes. */
+  proceedToNativeWindowClose: (isQuitting: boolean) => void
+  windowCloseDialog: ReactElement
+}
+
+/**
+ * The window-close confirmation, shown for local terminals with running children
+ * (SSH terminals detach/persist via the relay). Owns the probe, the decision and
+ * the dialog together so the decision is only ever reachable through the surface
+ * the user actually sees.
+ */
+export function useWindowCloseRunningProcessPrompt(): WindowCloseRunningProcessPrompt {
+  const [windowCloseDialogOpen, setWindowCloseDialogOpen] = useState(false)
+
+  const confirmNativeWindowClose = useCallback(() => {
+    // Why: capture only after every close guard has committed. A canceled child-
+    // process prompt must not consume App's synthetic/native unload guard.
+    const accepted = runWithWindowCloseCheckpointScope(() =>
+      window.dispatchEvent(new Event('beforeunload', { cancelable: true }))
+    )
+    if (!accepted) {
+      // Why: a checkpoint-vetoed quit used to die here with no dialog and no log,
+      // leaving SIGKILL as the only exit (#15352). The dirty-file veto publishes
+      // no reason — its deferred dialog flow already gives the user a surface.
+      showShutdownCheckpointFailureToast()
+      return
+    }
+    window.api.ui.confirmWindowClose()
+  }, [])
+
+  const proceedToNativeWindowClose = useCallback(
+    (isQuitting: boolean) => {
+      if (!isQuitting) {
+        const state = useAppStore.getState()
+        const localPtyIds = Object.entries(state.tabsByWorktree).flatMap(
+          ([worktreeId, worktreeTabs]) => {
+            const connectionId = getConnectionId(worktreeId)
+            if (connectionId !== null) {
+              return []
+            }
+            return worktreeTabs
+              .flatMap((tab) => state.ptyIdsByTabId[tab.id] ?? [])
+              .filter((ptyId) => !isRemoteRuntimePtyId(ptyId))
+          }
+        )
+        if (localPtyIds.length > 0) {
+          void anyLocalPtyBlocksWindowClose(state.settings, localPtyIds).then((blocked) => {
+            if (blocked) {
+              setWindowCloseDialogOpen(true)
+            } else {
+              confirmNativeWindowClose()
+            }
+          })
+          return
+        }
+      }
+      confirmNativeWindowClose()
+    },
+    [confirmNativeWindowClose]
+  )
+
+  const windowCloseDialog = (
+    <Dialog
+      open={windowCloseDialogOpen}
+      onOpenChange={(open) => {
+        if (!open) {
+          setWindowCloseDialogOpen(false)
+        }
+      }}
+    >
+      <DialogContent className="max-w-sm" showCloseButton={false}>
+        <DialogHeader>
+          <DialogTitle className="text-sm">
+            {translate('auto.components.Terminal.2fa9c69ff3', 'Close Window?')}
+          </DialogTitle>
+          <DialogDescription className="text-xs">
+            {translate(
+              'auto.components.Terminal.7958465754',
+              'There are local terminals with running processes. Close the window anyway?'
+            )}
+          </DialogDescription>
+        </DialogHeader>
+        <DialogFooter className="gap-2">
+          <Button
+            type="button"
+            variant="outline"
+            size="sm"
+            onClick={() => setWindowCloseDialogOpen(false)}
+          >
+            {translate('auto.components.Terminal.f82e9f02df', 'Cancel')}
+          </Button>
+          <Button
+            type="button"
+            variant="destructive"
+            size="sm"
+            autoFocus
+            onClick={() => {
+              setWindowCloseDialogOpen(false)
+              confirmNativeWindowClose()
+            }}
+          >
+            {translate('auto.components.Terminal.73768427cf', 'Close')}
+          </Button>
+        </DialogFooter>
+      </DialogContent>
+    </Dialog>
+  )
+
+  return { proceedToNativeWindowClose, windowCloseDialog }
+}