fix(pty): preserve child-process inspection uncertainty (#20756)

* fix(pty): preserve unverifiable local child reads

* fix(pty): make child-process inspection synchronous

Separate foreground and child-process sampling. Sample child processes
synchronously after confirming foreground availability, returning
unverifiable verdicts when pty reads fail. Handle both transport loss
and local read failures uniformly in the completion coordinator.

* fix(pty): handle retired masters and pane instance swaps

Detect when node-pty retires the master fd (fd == -1) and return
unverifiable instead of misreading the spawn file as an idle shell.
Guard inspectProcess against PTY replacement mid-read to avoid pairing
old foreground with replacement's children.

* fix test

* fix tests

src/main/providers/local-pty-child-process-verdict.test.ts

@@ -1,4 +1,4 @@
-import type * as pty from 'node-pty'
+import * as pty from 'node-pty'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 const { resolveForegroundMock } = vi.hoisted(() => ({ resolveForegroundMock: vi.fn() }))
@@ -7,6 +7,7 @@ vi.mock('./agent-foreground-process', () => ({
   resolveAgentForegroundProcessWithAvailability: resolveForegroundMock,
   confirmShellForegroundProcess: vi.fn()
 }))
+import { isRetiredPtyMaster } from '../pty/node-pty-master-fd-retirement'
 import {
   hasLocalPtyChildProcesses,
   inspectLocalPtyChildProcesses
@@ -15,6 +16,8 @@ import { LocalPtyProvider } from './local-pty-provider'
 import { ptyProcesses, ptyShellName } from './local-pty-provider-state'
 import { inspectPtyProviderProcess } from './pty-process-inspection'
 
+const POSIX_SHELL = '/bin/sh'
+
 function registerPane(id: string, foreground: string | (() => string), shell?: string): void {
   const pane: pty.IPty = {
     pid: 4242,
@@ -39,6 +42,32 @@ function registerPane(id: string, foreground: string | (() => string), shell?: s
   }
 }
 
+/**
+ * A real node-pty whose master has been given up. The getter does not throw here -- it answers
+ * `POSIX_SHELL`, which is exactly the recorded shell name, so only the descriptor distinguishes
+ * this pane from an idle one.
+ */
+async function registerRetiredPane(id: string): Promise<pty.IPty> {
+  const term = pty.spawn(POSIX_SHELL, ['-c', 'exit 0'], {
+    name: 'xterm-256color',
+    cols: 80,
+    rows: 24,
+    cwd: process.cwd(),
+    env: { ...process.env }
+  })
+  await new Promise<void>((resolve) => {
+    term.onExit(() => resolve())
+  })
+  // `onExit` runs before node-pty's `_close()`, which is where the patch retires `_fd`.
+  await vi.waitFor(() => expect(isRetiredPtyMaster(term)).toBe(true), {
+    timeout: 10000,
+    interval: 10
+  })
+  ptyProcesses.set(id, term)
+  ptyShellName.set(id, POSIX_SHELL)
+  return term
+}
+
 beforeEach(() => {
   resolveForegroundMock.mockReset()
   resolveForegroundMock.mockResolvedValue({ available: true, processName: '/bin/zsh' })
@@ -49,6 +78,9 @@ afterEach(() => {
   ptyShellName.clear()
 })
 
+// Windows has no master fd to retire, and `WindowsTerminal.process` answers from the spawn name.
+const describeOnPosix = process.platform === 'win32' ? describe.skip : describe
+
 describe('inspectLocalPtyChildProcesses', () => {
   it('reports unverifiable when the pty fd cannot be read', () => {
     registerPane(
@@ -58,8 +90,6 @@ describe('inspectLocalPtyChildProcesses', () => {
       },
       '/bin/zsh'
     )
-
-    // Not `no-children`: the close guard reads that as "nothing is running here" and kills the pane.
     expect(inspectLocalPtyChildProcesses('pty-closed')).toBe('unverifiable')
   })
 
@@ -78,17 +108,37 @@ describe('inspectLocalPtyChildProcesses', () => {
   })
 
   it('collapses uncertainty to false only in the boolean adapter', async () => {
+    let reads = 0
     registerPane(
       'pty-closed',
       () => {
+        reads += 1
         throw new Error('EBADF: bad file descriptor')
       },
       '/bin/zsh'
     )
+    await expect(hasLocalPtyChildProcesses('pty-closed')).resolves.toBe(false)
+    // The `false` has to come from the failed read, not from an earlier short-circuit.
+    expect(reads).toBe(1)
+  })
+})
+
+describeOnPosix('inspectLocalPtyChildProcesses on a retired master', () => {
+  it('reports unverifiable rather than reading the spawn file as an idle shell', async () => {
+    const term = await registerRetiredPane('pty-retired')
+
+    // The mechanism is silent: this is the same string an idle pane reports.
+    expect(term.process).toBe(POSIX_SHELL)
+    // Not `no-children`: the close guard reads that as "nothing is running here" and kills the pane.
+    expect(inspectLocalPtyChildProcesses('pty-retired')).toBe('unverifiable')
+  }, 15000)
+
+  it('collapses uncertainty to false only in the boolean adapter', async () => {
+    await registerRetiredPane('pty-retired')
 
     // The adapter exists for `IPtyProvider.hasChildProcesses`, which has no third slot.
-    await expect(hasLocalPtyChildProcesses('pty-closed')).resolves.toBe(false)
-  })
+    await expect(hasLocalPtyChildProcesses('pty-retired')).resolves.toBe(false)
+  }, 15000)
 })
 
 describe('inspectPtyProviderProcess child-process evidence', () => {
@@ -107,7 +157,6 @@ describe('inspectPtyProviderProcess child-process evidence', () => {
       },
       '/bin/zsh'
     )
-
     await expect(inspectPtyProviderProcess(provider, 'pty-closing')).resolves.toEqual({
       foregroundProcess: '/bin/zsh',
       hasChildProcesses: false,
@@ -139,4 +188,31 @@ describe('inspectPtyProviderProcess child-process evidence', () => {
     expect(inspection.hasChildProcesses).toBe(true)
     expect(inspection.childProcessEvidence).toBe('children')
   })
+
+  it('refuses to pair one panes foreground with its replacements children', async () => {
+    registerPane('pty-swapped', '/bin/zsh', '/bin/zsh')
+    resolveForegroundMock.mockImplementation(async () => {
+      // Cleanup plus reactivation lands a different IPty under the same id mid-read.
+      registerPane('pty-swapped', 'vim', '/bin/zsh')
+      return { available: true, processName: '/bin/zsh' }
+    })
+
+    await expect(inspectPtyProviderProcess(provider, 'pty-swapped')).resolves.toEqual({
+      foregroundProcess: null,
+      hasChildProcesses: false,
+      childProcessEvidence: 'unverifiable'
+    })
+  })
+})
+
+describeOnPosix('inspectPtyProviderProcess on a retired master', () => {
+  const provider = new LocalPtyProvider()
+
+  it('carries unverifiable child evidence beside the foreground it could still read', async () => {
+    await registerRetiredPane('pty-retired')
+
+    const inspection = await inspectPtyProviderProcess(provider, 'pty-retired')
+    expect(inspection.hasChildProcesses).toBe(false)
+    expect(inspection.childProcessEvidence).toBe('unverifiable')
+  }, 15000)
 })

src/main/providers/local-pty-foreground-inspection.ts

@@ -7,6 +7,7 @@ import {
   resolveAgentForegroundProcessWithAvailability
 } from './agent-foreground-process'
 import { buildPaneProcessFingerprint } from './posix-pane-foreground-fingerprint'
+import { isRetiredPtyMaster } from '../pty/node-pty-master-fd-retirement'
 import { resolveForegroundFallbackProcess } from './local-pty-launch-helpers'
 import {
   ptyAgentForegroundContextPaths,
@@ -22,11 +23,19 @@ import {
 import { readWindowsConsoleAttachedProcessIds } from './windows-console-attached-processes'
 import { isWindowsPtyJobReadable, readWindowsPtyJobProcessIds } from './windows-pty-job-membership'
 
+/**
+ * A retired master does not fail loudly: the `process` getter answers with the spawn file, which
+ * equals the recorded shell and would otherwise read as a real "nothing is running here". Ask the
+ * descriptor before the name, because an unreadable PTY is not evidence that its children exited.
+ */
 export function inspectLocalPtyChildProcesses(id: string): PtyChildProcessVerdict {
   const proc = ptyProcesses.get(id)
   if (!proc) {
     return 'no-children'
   }
+  if (isRetiredPtyMaster(proc)) {
+    return 'unverifiable'
+  }
   try {
     const foreground = proc.process
     const shell = ptyShellName.get(id)

src/main/providers/local-pty-provider.ts

@@ -130,7 +130,18 @@ export class LocalPtyProvider implements IPtyProvider {
   }
 
   async inspectProcess(id: string): Promise<PtyProcessInspection> {
+    const proc = ptyProcesses.get(id)
     const foregroundProcess = await getLocalPtyForegroundProcess(id)
+    // Both fields have to describe one PTY: cleanup plus reactivation across the await above would
+    // otherwise pair the old pane's identity with the replacement's children. The child read below
+    // is synchronous, so this recheck is the last point either answer can drift.
+    if (ptyProcesses.get(id) !== proc) {
+      return {
+        foregroundProcess: null,
+        hasChildProcesses: false,
+        childProcessEvidence: 'unverifiable'
+      }
+    }
     const childProcessEvidence = inspectLocalPtyChildProcesses(id)
     return {
       foregroundProcess,

src/main/pty/node-pty-master-fd-retirement.ts

@@ -0,0 +1,15 @@
+/**
+ * node-pty hands the master fd to libuv, and Orca's patch sets it to -1 in the same block that
+ * gives up the handle (config/patches/node-pty@1.1.0.patch). Past that point every fd-addressed
+ * answer is a stand-in rather than an error: the `process` getter names the spawn file instead of
+ * whatever `tcgetpgrp` would have reported, so callers that need a real observation have to ask
+ * about the descriptor first. Windows exposes no master fd, so it never reads as retired; an
+ * unpatched (relay-installed) node-pty never retires the number at all.
+ */
+export function isRetiredPtyMaster(proc: unknown): boolean {
+  if (typeof proc !== 'object' || proc === null || !('fd' in proc)) {
+    return false
+  }
+  const fd: unknown = proc.fd
+  return typeof fd === 'number' && fd < 0
+}