From 4e7017fe3f3e35018c26eff7a2158faa91ba8292 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:15:10 -0700 Subject: [PATCH] Route relay process probes through runProcess and type processEvidence on PtyApi --- src/preload/api/pty-api.ts | 4 + src/relay/pty-process-probes.test.ts | 116 ++++++++++++------ src/relay/pty-process-probes.ts | 76 ++++++------ ...t-completion-unverifiable-evidence.test.ts | 26 ++++ ...ction-unverifiable-completion.unit.test.ts | 87 ++++++++----- 5 files changed, 200 insertions(+), 109 deletions(-) diff --git a/src/preload/api/pty-api.ts b/src/preload/api/pty-api.ts index 7eee16ea522..4278f10175a 100644 --- a/src/preload/api/pty-api.ts +++ b/src/preload/api/pty-api.ts @@ -6,6 +6,7 @@ import type { StartupCommandDelivery } from '../../shared/codex-startup-delivery import type { ProjectExecutionRuntimeResolution } from '../../shared/project-execution-runtime' import type { PtyListedSession } from '../../shared/pty-listed-session' import type { PtyMainDeliveryDiagnostics } from '../../shared/pty-delivery-diagnostics' +import type { PtyProcessInspectionEvidence } from '../../shared/pty-process-inspection-evidence' import type { PtyModelRestoreNeededEvent } from '../../shared/pty-model-restore-marker' import type { PtyRendererDeliveryHealthReply, @@ -113,6 +114,9 @@ export type PtyApi = { foregroundProcess: string | null hasChildProcesses: boolean unavailable?: true + /** Per-probe live/unverifiable/exited evidence; absent from older hosts, + * whose legacy fields above keep the pre-evidence collapse. */ + processEvidence?: PtyProcessInspectionEvidence }> confirmForegroundProcess: (id: string) => Promise getCwd: (id: string) => Promise diff --git a/src/relay/pty-process-probes.test.ts b/src/relay/pty-process-probes.test.ts index 43ab1844d5f..2be8b2d2709 100644 --- a/src/relay/pty-process-probes.test.ts +++ b/src/relay/pty-process-probes.test.ts @@ -1,16 +1,24 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { execFileMock, execFileSyncMock, getAllProcessesMock } = vi.hoisted(() => ({ +const { execFileMock, execFileSyncMock, getAllProcessesMock, runProcessMock } = vi.hoisted(() => ({ execFileMock: vi.fn(), getAllProcessesMock: vi.fn(), - execFileSyncMock: vi.fn() + execFileSyncMock: vi.fn(), + runProcessMock: vi.fn() })) +// The process-table snapshot (`ps -axo`) is an allowlisted direct execFile +// caller; the probes themselves go through runProcess. vi.mock('child_process', () => ({ execFile: execFileMock, execFileSync: execFileSyncMock })) +vi.mock('../shared/child-process/run-process', async (importOriginal) => ({ + ...(await importOriginal()), + runProcess: runProcessMock +})) + import { resetWindowsProcessRowsSnapshotForTests } from '../main/providers/windows-foreground-process-rows' import { __setWindowsProcessTreeLoaderForTests } from '../main/windows/windows-process-table' import { resetProcessTableSnapshotForTests } from '../shared/process-table-snapshot' @@ -38,6 +46,45 @@ function mockExecFile( ) } +type ProbeProcessResult = { + code: number | null + stdout?: string + signal?: NodeJS.Signals | null + timedOut?: boolean +} + +/** + * Feed the probes' runProcess calls. Return an Error to reject the way + * runProcess does when the probe binary could not be spawned at all. + */ +function mockRunProcess( + implementation: (program: string, args: string[]) => ProbeProcessResult | Error +): void { + runProcessMock.mockImplementation(async (spec: { program: string; args?: readonly string[] }) => { + const result = implementation(spec.program, [...(spec.args ?? [])]) + if (result instanceof Error) { + throw result + } + return { + code: result.code, + signal: result.signal ?? null, + stdout: result.stdout ?? '', + stderr: '', + timedOut: result.timedOut ?? false + } + }) +} + +function spawnFailure(message: string, code: string): Error { + const error = new Error(message) as NodeJS.ErrnoException + error.code = code + return error +} + +function timedOutProbe(): ProbeProcessResult { + return { code: null, signal: 'SIGTERM', timedOut: true } +} + /** * Feed the native Windows snapshot. A real snapshot always contains the * querying process, and the reader rejects a table without it. @@ -72,6 +119,7 @@ beforeEach(() => { vi.resetModules() execFileMock.mockReset() execFileSyncMock.mockReset() + runProcessMock.mockReset() resetProcessTableSnapshotForTests() resetWindowsProcessRowsSnapshotForTests() __setWindowsProcessTreeLoaderForTests() @@ -116,6 +164,7 @@ describe('getForegroundProcessName', () => { await expect(getForegroundProcessName(100, 'vim')).resolves.toBe('vim') expect(execFileMock).not.toHaveBeenCalled() + expect(runProcessMock).not.toHaveBeenCalled() }) it('recognizes SSH relay node-wrapped agents from descendant command lines', async () => { @@ -240,9 +289,11 @@ describe('getForegroundProcessName', () => { it('returns an outer omp fallback without a process-table scan', async () => { mockExecFile(() => new Error('unexpected process-table scan')) + mockRunProcess(() => new Error('unexpected probe subprocess')) await expect(getForegroundProcessName(100, 'omp')).resolves.toBe('omp') expect(execFileMock).not.toHaveBeenCalled() + expect(runProcessMock).not.toHaveBeenCalled() }) it('rescans a Windows pi fallback for its outer omp wrapper', async () => { @@ -271,60 +322,41 @@ describe('getForegroundProcessName', () => { }) it('falls back to the root process command when descendant inspection fails', async () => { - mockExecFile((_command, args) => { - if (args[0] === '-axo') { - return new Error('ps table unavailable') - } - return { stdout: 'bash\n' } - }) + mockExecFile(() => new Error('ps table unavailable')) + mockRunProcess(() => ({ code: 0, stdout: 'bash\n' })) await expect(getForegroundProcessName(100)).resolves.toBe('bash') }) }) -function subprocessExitError( - message: string, - overrides: { code?: number | string; killed?: boolean; signal?: string | null } = {} -): Error { - const error = new Error(message) as Error & { - code?: number | string - killed?: boolean - signal?: string | null - } - error.code = overrides.code - error.killed = overrides.killed ?? false - error.signal = overrides.signal ?? null - return error -} - describe('probeProcessChildren', () => { it('reports live children from pgrep output', async () => { - mockExecFile(() => ({ stdout: '4242\n' })) + mockRunProcess(() => ({ code: 0, stdout: '4242\n' })) await expect(probeProcessChildren(100)).resolves.toEqual({ verdict: 'live' }) }) it('treats pgrep exit 1 as positive absence', async () => { // pgrep exits 1 with no output when it ran and matched nothing. - mockExecFile(() => subprocessExitError('no match', { code: 1 })) + mockRunProcess(() => ({ code: 1 })) await expect(probeProcessChildren(100)).resolves.toEqual({ verdict: 'exited' }) }) it('keeps a missing pgrep binary unverifiable, never exited', async () => { - mockExecFile(() => subprocessExitError('spawn pgrep ENOENT', { code: 'ENOENT' })) + mockRunProcess(() => spawnFailure('spawn pgrep ENOENT', 'ENOENT')) await expect(probeProcessChildren(100)).resolves.toMatchObject({ verdict: 'unverifiable' }) }) it('keeps a timed-out pgrep unverifiable, never exited', async () => { - mockExecFile(() => subprocessExitError('killed', { killed: true, signal: 'SIGTERM' })) + mockRunProcess(() => timedOutProbe()) await expect(probeProcessChildren(100)).resolves.toMatchObject({ verdict: 'unverifiable' }) }) it('keeps a pgrep fatal error (exit 3) unverifiable', async () => { - mockExecFile(() => subprocessExitError('fatal', { code: 3 })) + mockRunProcess(() => ({ code: 3 })) await expect(probeProcessChildren(100)).resolves.toMatchObject({ verdict: 'unverifiable' }) }) @@ -357,7 +389,13 @@ describe('probeProcessChildren', () => { }) it('collapses every probe failure to false for the legacy boolean', async () => { - mockExecFile(() => subprocessExitError('spawn pgrep ENOENT', { code: 'ENOENT' })) + mockRunProcess(() => spawnFailure('spawn pgrep ENOENT', 'ENOENT')) + + await expect(processHasChildren(100)).resolves.toBe(false) + }) + + it('collapses a non-zero pgrep exit to false for the legacy boolean', async () => { + mockRunProcess(() => ({ code: 1 })) await expect(processHasChildren(100)).resolves.toBe(false) }) @@ -369,8 +407,9 @@ describe('observeForegroundProcess', () => { if (args[0] === '-axo') { return { stdout: '' } } - return { stdout: 'vim\n' } + return new Error('unexpected direct subprocess') }) + mockRunProcess(() => ({ code: 0, stdout: 'vim\n' })) await expect(observeForegroundProcess(100)).resolves.toEqual({ verdict: 'observed', @@ -379,12 +418,8 @@ describe('observeForegroundProcess', () => { }) it('reports ps exit 1 as an observed absence of the process', async () => { - mockExecFile((_command, args) => { - if (args[0] === '-axo') { - return new Error('ps table unavailable') - } - return subprocessExitError('no such pid', { code: 1 }) - }) + mockExecFile(() => new Error('ps table unavailable')) + mockRunProcess(() => ({ code: 1 })) await expect(observeForegroundProcess(100)).resolves.toEqual({ verdict: 'observed', @@ -393,7 +428,8 @@ describe('observeForegroundProcess', () => { }) it('keeps a timed-out ps read unverifiable', async () => { - mockExecFile(() => subprocessExitError('killed', { killed: true, signal: 'SIGTERM' })) + mockExecFile(() => new Error('ps table unavailable')) + mockRunProcess(() => timedOutProbe()) await expect(observeForegroundProcess(100)).resolves.toMatchObject({ verdict: 'unverifiable' @@ -401,7 +437,8 @@ describe('observeForegroundProcess', () => { }) it('keeps the node-pty fallback observed when descendant enrichment fails', async () => { - mockExecFile(() => subprocessExitError('killed', { killed: true, signal: 'SIGTERM' })) + mockExecFile(() => new Error('ps table unavailable')) + mockRunProcess(() => timedOutProbe()) await expect(observeForegroundProcess(100, 'node')).resolves.toEqual({ verdict: 'observed', @@ -410,7 +447,8 @@ describe('observeForegroundProcess', () => { }) it('collapses an unverifiable observation to null for the legacy reader', async () => { - mockExecFile(() => subprocessExitError('killed', { killed: true, signal: 'SIGTERM' })) + mockExecFile(() => new Error('ps table unavailable')) + mockRunProcess(() => timedOutProbe()) await expect(getForegroundProcessName(100)).resolves.toBeNull() }) diff --git a/src/relay/pty-process-probes.ts b/src/relay/pty-process-probes.ts index 5d15ea3cd24..969f6a72eaa 100644 --- a/src/relay/pty-process-probes.ts +++ b/src/relay/pty-process-probes.ts @@ -1,5 +1,3 @@ -import { execFile as execFileCb } from 'node:child_process' -import { promisify } from 'node:util' import { isAgentForegroundWrapperProcess, isExpectedAgentProcess, @@ -22,8 +20,9 @@ import { shouldInspectWindowsAgentForeground } from '../main/providers/windows-agent-foreground-process' import { queryWindowsPaneProcessInventory } from '../main/providers/windows-foreground-process-rows' +import { runProcess, type ProcessResult } from '../shared/child-process/run-process' -const execFile = promisify(execFileCb) +const PROBE_TIMEOUT_MS = 3000 /** * Check whether a process has child processes (via pgrep). @@ -34,37 +33,34 @@ const execFile = promisify(execFileCb) */ export async function processHasChildren(pid: number): Promise { try { - const { stdout } = await execFile('pgrep', ['-P', String(pid)], { - encoding: 'utf-8', - timeout: 3000 - }) - return stdout.trim().length > 0 + const result = await runProbe('pgrep', ['-P', String(pid)]) + return result.code === 0 && result.stdout.trim().length > 0 } catch { return false } } -// Why not NodeJS.ErrnoException: execFile stamps the numeric exit status into -// `code`, which the lib types declare as string-only. -type SubprocessProbeError = { - code?: number | string - killed?: boolean - signal?: NodeJS.Signals | null - message?: string +// Rejects only when the probe binary could not be spawned at all (ENOENT, +// fork pressure); a probe that ran resolves with its exit status as data. +function runProbe(program: string, args: string[]): Promise { + return runProcess({ program, args, timeoutMs: PROBE_TIMEOUT_MS }) } // Exit code 1 without a kill signal: pgrep/ps RAN and matched nothing — that // is positive absence, not a failed probe. -function subprocessRanAndMatchedNothing(error: unknown): boolean { - const failure = error as SubprocessProbeError - return failure?.code === 1 && failure.killed !== true && !failure.signal +function subprocessRanAndMatchedNothing(result: ProcessResult): boolean { + return result.code === 1 && !result.timedOut && !result.signal } -function describeProcessProbeFailure(command: string, error: unknown): string { - const failure = error as SubprocessProbeError - if (failure?.killed === true || failure?.signal) { +function describeProcessProbeFailure(command: string, result: ProcessResult): string { + if (result.timedOut || result.signal) { return `${command} did not answer before its deadline` } + return `${command} could not run: exit ${result.code ?? 'unknown'}` +} + +function describeProcessSpawnFailure(command: string, error: unknown): string { + const failure = error as NodeJS.ErrnoException | undefined return `${command} could not run: ${failure?.code ?? failure?.message ?? 'unknown failure'}` } @@ -86,18 +82,19 @@ export async function probeProcessChildren(pid: number): Promise 0 ? { verdict: 'live' } : { verdict: 'exited' } } + let result: ProcessResult try { - const { stdout } = await execFile('pgrep', ['-P', String(pid)], { - encoding: 'utf-8', - timeout: 3000 - }) - return stdout.trim().length > 0 ? { verdict: 'live' } : { verdict: 'exited' } + result = await runProbe('pgrep', ['-P', String(pid)]) } catch (error) { - if (subprocessRanAndMatchedNothing(error)) { - return { verdict: 'exited' } - } - return { verdict: 'unverifiable', reason: describeProcessProbeFailure('pgrep', error) } + return { verdict: 'unverifiable', reason: describeProcessSpawnFailure('pgrep', error) } } + if (result.code === 0) { + return result.stdout.trim().length > 0 ? { verdict: 'live' } : { verdict: 'exited' } + } + if (subprocessRanAndMatchedNothing(result)) { + return { verdict: 'exited' } + } + return { verdict: 'unverifiable', reason: describeProcessProbeFailure('pgrep', result) } } // Why: signal 0 probes existence without delivering a signal. Only ESRCH ("no @@ -265,16 +262,17 @@ export async function observeForegroundProcess( if (fallbackProcess) { return observedForeground(fallbackProcess) } + let result: ProcessResult try { - const { stdout } = await execFile('ps', ['-o', 'comm=', '-p', String(pid)], { - encoding: 'utf-8', - timeout: 3000 - }) - return observedForeground(stdout.trim() || null) + result = await runProbe('ps', ['-o', 'comm=', '-p', String(pid)]) } catch (error) { - if (subprocessRanAndMatchedNothing(error)) { - return observedForeground(null) - } - return { verdict: 'unverifiable', reason: describeProcessProbeFailure('ps', error) } + return { verdict: 'unverifiable', reason: describeProcessSpawnFailure('ps', error) } } + if (result.code === 0) { + return observedForeground(result.stdout.trim() || null) + } + if (subprocessRanAndMatchedNothing(result)) { + return observedForeground(null) + } + return { verdict: 'unverifiable', reason: describeProcessProbeFailure('ps', result) } } diff --git a/src/renderer/src/components/terminal-pane/agent-completion-unverifiable-evidence.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-unverifiable-evidence.test.ts index 220ba3dcfef..a297ebd05a3 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-unverifiable-evidence.test.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-unverifiable-evidence.test.ts @@ -5,6 +5,10 @@ import { useAgentCompletionCoordinatorLifecycle } from './agent-completion-coordinator-test-harness' import type { RuntimeTerminalProcessInspection } from '@/runtime/runtime-terminal-inspection' +import type { PtyApi } from '../../../../preload/api/pty-api' + +// The renderer-visible type of the local `window.api.pty.inspectProcess` leg. +type LocalInspectProcessResult = Awaited> function unverifiableChildren(foregroundProcess: string | null): RuntimeTerminalProcessInspection { return { @@ -69,6 +73,28 @@ describe('agent completion with inspection evidence', () => { expect(dispatchCompletion).not.toHaveBeenCalled() }) + it('carries unverifiable evidence through the preload-typed local inspection result', async () => { + // Pins the preload contract: `processEvidence` must exist on the renderer- + // visible type of the local IPC leg, not only on the runtime-RPC shape — + // otherwise a typed consumer cannot see the evidence the host published. + let result: LocalInspectProcessResult = processResult('codex') + const { dispatchCompletion } = startCoordinator(() => result) + + await vi.advanceTimersByTimeAsync(2_000) + + result = { + foregroundProcess: 'zsh', + hasChildProcesses: false, + processEvidence: { + foreground: { verdict: 'observed', processName: 'zsh' }, + children: { verdict: 'unverifiable', reason: 'pgrep did not answer before its deadline' } + } + } + await vi.advanceTimersByTimeAsync(60_000) + + expect(dispatchCompletion).not.toHaveBeenCalled() + }) + it('never turns unverifiable foreground evidence into a process-exit completion', async () => { let result: RuntimeTerminalProcessInspection = processResult('codex') const { dispatchCompletion } = startCoordinator(() => result) diff --git a/tests/e2e/relay-completion-evidence/relay-inspection-unverifiable-completion.unit.test.ts b/tests/e2e/relay-completion-evidence/relay-inspection-unverifiable-completion.unit.test.ts index 0540a33ba9e..6de1366d483 100644 --- a/tests/e2e/relay-completion-evidence/relay-inspection-unverifiable-completion.unit.test.ts +++ b/tests/e2e/relay-completion-evidence/relay-inspection-unverifiable-completion.unit.test.ts @@ -2,30 +2,44 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Mock } from 'vitest' import * as ptyProcessProbes from '../../../src/relay/pty-process-probes' -const { execFileMock, execFileSyncMock, mockPtySpawn, mockPtyInstance, mockReadinessProbe } = - vi.hoisted(() => ({ - execFileMock: vi.fn(), - execFileSyncMock: vi.fn(), - mockPtySpawn: vi.fn(), - mockReadinessProbe: vi.fn(), - mockPtyInstance: { - pid: process.pid, - onData: vi.fn(), - onExit: vi.fn(), - write: vi.fn(), - resize: vi.fn(), - kill: vi.fn(), - clear: vi.fn(), - pause: vi.fn(), - resume: vi.fn() - } - })) +const { + execFileMock, + execFileSyncMock, + runProcessMock, + mockPtySpawn, + mockPtyInstance, + mockReadinessProbe +} = vi.hoisted(() => ({ + execFileMock: vi.fn(), + execFileSyncMock: vi.fn(), + runProcessMock: vi.fn(), + mockPtySpawn: vi.fn(), + mockReadinessProbe: vi.fn(), + mockPtyInstance: { + pid: process.pid, + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + clear: vi.fn(), + pause: vi.fn(), + resume: vi.fn() + } +})) +// The process-table snapshot (`ps -axo`) reads through execFile; the +// pgrep/ps probes go through the runProcess chokepoint. vi.mock('child_process', () => ({ execFile: execFileMock, execFileSync: execFileSyncMock })) +vi.mock('../../../src/shared/child-process/run-process', async (importOriginal) => ({ + ...(await importOriginal()), + runProcess: runProcessMock +})) + vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) @@ -72,7 +86,7 @@ describe('relay inspection under probe failure', () => { const { spawnPty } = createPtyRequestHelpers(() => dispatcher) // Timeout-killed subprocess: what execFile yields when the host is too - // loaded to answer `ps`/`pgrep` inside the 3s probe deadline. + // loaded to answer the `ps -axo` table read inside the probe deadline. function timeoutKilledError(command: string): Error { const error = new Error(`spawn ${command} ETIMEDOUT`) as Error & { killed: boolean @@ -109,19 +123,31 @@ describe('relay inspection under probe failure', () => { }) return } - if (command === 'pgrep') { - callback(null, { stdout: '99999\n', stderr: '' }) - return - } callback(new Error(`unexpected command ${command}`), { stdout: '', stderr: '' }) } ) } + // The pgrep/ps probes reach the host through runProcess, which resolves + // exit status as data and reports a probe deadline as `timedOut`. + function installRunProcess(): void { + runProcessMock.mockImplementation(async (spec: { program: string }) => { + if (execBehavior === 'probe-failure') { + return { code: null, signal: 'SIGTERM', stdout: '', stderr: '', timedOut: true } + } + if (spec.program === 'pgrep') { + return { code: 0, signal: null, stdout: '99999\n', stderr: '', timedOut: false } + } + throw new Error(`unexpected command ${spec.program}`) + }) + } + beforeEach(() => { execBehavior = 'healthy' execFileMock.mockReset() + runProcessMock.mockReset() installExecFile() + installRunProcess() resetProcessTableSnapshotForTests() ;({ dispatcher, handler, originalPlatform } = beginPtyHandlerTest({ mockPtySpawn, @@ -248,17 +274,16 @@ describe('relay inspection under probe failure', () => { callback(null, { stdout: `${process.pid} 1 Ss+ bash -l`, stderr: '' }) return } - if (command === 'pgrep') { - // pgrep ran and matched nothing: exits 1 with empty output. - const noMatch = new Error('pgrep exited 1') as Error & { code: number; killed: boolean } - noMatch.code = 1 - noMatch.killed = false - callback(noMatch, { stdout: '', stderr: '' }) - return - } callback(new Error(`unexpected command ${command}`), { stdout: '', stderr: '' }) } ) + runProcessMock.mockImplementation(async (spec: { program: string }) => { + if (spec.program === 'pgrep') { + // pgrep ran and matched nothing: exits 1 with empty output. + return { code: 1, signal: null, stdout: '', stderr: '', timedOut: false } + } + throw new Error(`unexpected command ${spec.program}`) + }) await vi.advanceTimersByTimeAsync(30_000)