From 8d313ea57974f64236fa15b0a397cc866ca12f9d Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Fri, 23 Oct 2026 07:33:54 -0700 Subject: [PATCH] chore: shorten process-host comments (#32262) Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../process-host/scripts/build-dist.mjs | 5 +- .../__fixtures__/windows-argument-corpus.ts | 22 +- .../__fixtures__/windows-cmd-shim-bodies.ts | 31 +-- .../process-host/src/bounded-output-sink.ts | 11 +- src/packages/process-host/src/fork-process.ts | 19 +- .../process-host/src/growing-byte-buffer.ts | 1 - .../process-host/src/posix-lsof-probe.test.ts | 19 +- .../process-host/src/posix-lsof-probe.ts | 10 +- src/packages/process-host/src/process-spec.ts | 14 +- .../src/process-tree-kill-gate.ts | 21 +- .../src/process-tree-termination.test.ts | 8 +- .../src/process-tree-termination.ts | 40 +--- .../src/run-process-windows-tree-kill.test.ts | 6 +- .../process-host/src/run-process.test.ts | 24 +-- src/packages/process-host/src/run-process.ts | 95 ++------- .../process-host/src/spawn-observer.ts | 10 +- .../process-host/src/spawn-resolution.ts | 38 +--- .../src/windows-cmd-shim-resolution.ts | 188 ++---------------- .../src/windows-command-line.test.ts | 10 +- .../process-host/src/windows-command-line.ts | 84 +------- .../process-host/src/windows-system-binary.ts | 17 +- .../src/windows-tree-kill.test.ts | 2 - .../process-host/src/windows-tree-kill.ts | 37 +--- 23 files changed, 101 insertions(+), 611 deletions(-) diff --git a/src/packages/process-host/scripts/build-dist.mjs b/src/packages/process-host/scripts/build-dist.mjs index a0574559e2c..8fbbb6b4c87 100644 --- a/src/packages/process-host/scripts/build-dist.mjs +++ b/src/packages/process-host/scripts/build-dist.mjs @@ -14,8 +14,6 @@ import { dirname, join, relative, resolve } from 'node:path' import { runProcess } from '@orca/process-host' import lockfile from 'proper-lockfile' -// Source resolution bootstraps compilation without requiring an existing dist. - const STAGING_PREFIX = '.dist-staging-' const LOCK_PATH = '.dist-build.lock' const LOCK_STALE_MS = 30_000 @@ -99,7 +97,7 @@ async function syncCompiledOutput(stagingDir, distDir, assertLockHeld) { await renameReplacing(source, target, assertLockHeld) result.written += 1 } - // Outputs of renamed or deleted sources would otherwise ship and stay importable. + // Remove obsolete outputs so renamed or deleted modules cannot remain importable. for (const file of existing) { if (!stagedSet.has(file)) { assertLockHeld() @@ -152,7 +150,6 @@ export async function buildPackageDist(packageDir) { compromised = error } }) - // Reject before publishing after proper-lockfile observes a lost lock. const assertLockHeld = () => { if (compromised) { throw new Error( diff --git a/src/packages/process-host/src/__fixtures__/windows-argument-corpus.ts b/src/packages/process-host/src/__fixtures__/windows-argument-corpus.ts index 033bb4d9e1d..913c5545896 100644 --- a/src/packages/process-host/src/__fixtures__/windows-argument-corpus.ts +++ b/src/packages/process-host/src/__fixtures__/windows-argument-corpus.ts @@ -1,15 +1,4 @@ -/** - * Adversarial argument corpus for the Windows command-line encoding. - * - * Every entry round-trips through a real `.cmd` shim on Windows 11 with the - * encoding in windows-command-line.ts. They are shared between the pure unit - * test (which asserts the encoded bytes) and the Windows integration test - * (which asserts what the child process actually receives), so a fix that only - * satisfies one parser cannot pass. - * - * The `%`, `&` and `"` cases are the ones that shipped as bugs: an argument - * containing `&` was truncated AND its remainder executed as a command. - */ +// Shared with real Windows .cmd round-trip tests so both parsers must preserve every argument. export const WINDOWS_ARGUMENT_CORPUS: readonly { name: string; value: string }[] = [ { name: 'plain', value: 'hello' }, { name: 'empty', value: '' }, @@ -22,9 +11,7 @@ export const WINDOWS_ARGUMENT_CORPUS: readonly { name: string; value: string }[] { name: 'pipe', value: 'a|b' }, { name: 'redirect', value: 'ac' }, { name: 'percent-pair', value: 'e%F%g' }, - // The shape that broke: a backslash immediately before a percent. Quoting - // inserts a quote there, and a quote after a single backslash is an escaped - // quote to CommandLineToArgvW. + // Inserted quotes after backslashes must not become CommandLineToArgvW escapes. { name: 'percent-after-backslash', value: 'C:\\Users\\%F%\\x' }, { name: 'percent-after-two-backslashes', value: 'C:\\\\%F%' }, { name: 'percent-lone', value: '100% done' }, @@ -40,10 +27,7 @@ export const WINDOWS_ARGUMENT_CORPUS: readonly { name: string; value: string }[] { name: 'agent-prompt', value: 'Fix "src/a b.ts" & run tests 100% !now! ^ (v2)' } ] -/** - * Environment the Windows integration test must run with, so the `%VAR%` cases - * fail loudly instead of passing by accident against an undefined variable. - */ +// Defined variables expose accidental percent expansion instead of letting it pass silently. export const WINDOWS_ARGUMENT_CORPUS_ENV: Readonly> = { F: 'EXPANDED_F', VAR: 'EXPANDED_VAR' diff --git a/src/packages/process-host/src/__fixtures__/windows-cmd-shim-bodies.ts b/src/packages/process-host/src/__fixtures__/windows-cmd-shim-bodies.ts index 956f92f5ac0..476d6ef88bc 100644 --- a/src/packages/process-host/src/__fixtures__/windows-cmd-shim-bodies.ts +++ b/src/packages/process-host/src/__fixtures__/windows-cmd-shim-bodies.ts @@ -1,25 +1,13 @@ -/** - * Shim bodies transcribed verbatim from a real Windows 11 install, plus - * builders that emit the same shapes around a caller-chosen target. - * - * The verbatim copies are what pins the shapes to reality: the resolver is only - * allowed to recognise files these generators actually write, and a copy here - * is how a reviewer checks that without a Windows box. - * - * Sources: - * %APPDATA%\npm\codex.cmd — npm cmd-shim, node script - * %APPDATA%\npm\agent-browser.cmd — npm cmd-shim, bundled .exe - * %APPDATA%\npm\pnx.cmd — npm cmd-shim, extensionless target - * \node_modules\.bin\vitest.cmd— pnpm @zkochan/cmd-shim, node script - * %LOCALAPPDATA%\pnpm\bin\pnpm.CMD — pnpm, bundled .exe - * %LOCALAPPDATA%\pnpm\bin\pn.CMD — pnpm alias, bare PATH command +/** Verbatim Windows 11 shims from: + * npm: %APPDATA%\npm\{codex,agent-browser,pnx}.cmd + * pnpm: \node_modules\.bin\vitest.cmd; %LOCALAPPDATA%\pnpm\bin\{pnpm,pn}.CMD */ function crlf(lines: readonly string[]): string { return `${lines.join('\r\n')}\r\n` } -/** npm's `cmd-shim` for a node script — the shape MDE flagged as `codex.cmd`. */ +// npm node-script shim. export const REAL_CODEX_CMD = crlf([ '@ECHO off', 'GOTO start', @@ -40,14 +28,12 @@ export const REAL_CODEX_CMD = crlf([ 'endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\\node_modules\\@openai\\codex\\bin\\codex.js" %*' ]) -/** npm's `cmd-shim` for a package that ships its own executable. */ export const REAL_AGENT_BROWSER_CMD = crlf([ '@ECHO off', '"%~dp0node_modules\\agent-browser\\bin\\agent-browser-win32-x64.exe" %*' ]) -/** npm's `cmd-shim` for an extensionless target — cmd resolves it via PATHEXT, - * so it must NOT resolve. */ +// Extensionless targets need cmd PATHEXT resolution and must not resolve directly. export const REAL_PNX_CMD = crlf([ '@ECHO off', 'GOTO start', @@ -66,7 +52,6 @@ const VITEST_NODE_PATH = [ 'C:\\Users\\neil\\orca\\orca\\node_modules\\.pnpm\\node_modules' ].join(';') -/** pnpm's `.bin` shim: two interpreter branches plus the NODE_PATH prepend. */ export const REAL_VITEST_CMD = crlf([ '@SETLOCAL', '@IF NOT DEFINED NODE_PATH (', @@ -84,21 +69,18 @@ export const REAL_VITEST_CMD = crlf([ export const REAL_VITEST_NODE_PATH = VITEST_NODE_PATH -/** pnpm's global shim for a bundled executable. */ export const REAL_PNPM_CMD = crlf([ '@SETLOCAL', '@"%~dp0\\..\\global\\v11\\27d0-19f7df4c136-1fab7163f1a52461\\node_modules\\@pnpm\\exe\\pnpm.exe" %*' ]) -/** pnpm's `pn` alias: a bare PATH command, with nothing to resolve. */ +// A bare PATH alias has no shim-relative target to resolve. export const REAL_PN_CMD = crlf(['@echo off', 'pnpm %*']) -/** The npm `%_prog%` shape around an arbitrary script. */ export function npmProgNodeShim(scriptRelative: string): string { return REAL_CODEX_CMD.replace('node_modules\\@openai\\codex\\bin\\codex.js', scriptRelative) } -/** The pnpm two-branch shape, with the NODE_PATH block only when asked for. */ export function pnpmBranchedNodeShim(scriptRelative: string, nodePathPrefix?: string): string { return crlf([ '@SETLOCAL', @@ -120,7 +102,6 @@ export function pnpmBranchedNodeShim(scriptRelative: string, nodePathPrefix?: st ]) } -/** The npm one-line shape around an arbitrary target. */ export function npmDirectShim(targetRelative: string): string { return crlf(['@ECHO off', `"%~dp0${targetRelative}" %*`]) } diff --git a/src/packages/process-host/src/bounded-output-sink.ts b/src/packages/process-host/src/bounded-output-sink.ts index c6b225106ab..b63a852f711 100644 --- a/src/packages/process-host/src/bounded-output-sink.ts +++ b/src/packages/process-host/src/bounded-output-sink.ts @@ -1,13 +1,7 @@ import { Buffer } from 'node:buffer' import { GrowingByteBuffer } from './growing-byte-buffer' -/** - * Collects output up to a cap, so a chatty child cannot grow the heap. - * - * Accepts strings as well as buffers: a stream someone called `setEncoding` on - * emits strings, and concatenating those as buffers throws inside a `data` - * handler, where the rejection has nowhere to go and the caller just hangs. - */ +// setEncoding streams emit strings; accepting them avoids exceptions in data handlers. export function createOutputSink( maxBytes: number, outputCapture: 'head' | 'tail' = 'head' @@ -44,8 +38,7 @@ export function createOutputSink( chunks.push(chunk.length > remaining ? chunk.subarray(0, remaining) : chunk) }, text: () => tail?.toString() ?? buffer().toString('utf8'), - // Why: callers that parse the output need to tell a short answer from a - // clipped one -- truncated JSON or JSONL parses as a smaller valid result. + // Truncated JSONL can still parse as a valid, incomplete result. truncated: () => bytes > maxBytes } } diff --git a/src/packages/process-host/src/fork-process.ts b/src/packages/process-host/src/fork-process.ts index f718f2a3038..c4fbcab9902 100644 --- a/src/packages/process-host/src/fork-process.ts +++ b/src/packages/process-host/src/fork-process.ts @@ -1,6 +1,4 @@ -// `fork` arm of Orca's child-process chokepoint. Kept beside `run-process.ts` for the same -// reason that file exists: callers outside this directory must not import `node:child_process`, -// and a guard test enforces that against a shrinking allowlist. +// All host process creation goes through this package; callers cannot import child_process directly. import { fork as nodeFork, type ForkOptions, @@ -8,17 +6,8 @@ import { } from 'node:child_process' import type { SpawnedProcess } from './process-spec' -/** - * What `spawnProcess` cannot express: a Node child with an IPC channel, started from a module - * path rather than a program, optionally under a different Node/Electron binary than this - * process's own. - * - * `fork` is the right primitive for that and there is no spawn-based substitute — the existing - * launch paths and their tests are written against `fork`'s contract (module path resolution, - * `execPath` override, inherited `execArgv`), and a spawn rewrite would change all three. - */ +// Preserve fork semantics for module resolution, execPath overrides, and inherited execArgv. export type ForkSpec = { - /** Node module to run as the child's entry point. */ modulePath: string args?: readonly string[] cwd?: string @@ -27,7 +16,6 @@ export type ForkSpec = { detached?: boolean /** Must keep an `'ipc'` slot; without one the child gets no channel and cannot `send`. */ stdio?: NodeSpawnOptions['stdio'] - /** Run a different Node/Electron binary — e.g. a relocated executable image. */ execPath?: string } @@ -37,8 +25,7 @@ export function forkProcess(spec: ForkSpec): SpawnedProcess { env: spec.env, detached: spec.detached, stdio: spec.stdio, - // Why conditional rather than `execPath: spec.execPath`: an explicit `undefined` is not the - // same as absent to Node, which reads the key to decide whether to override its own binary. + // Node treats an explicit undefined execPath differently from an absent key. ...(spec.execPath ? { execPath: spec.execPath } : {}) } // Node forwards this undocumented fork option to spawn, preventing console flashes on Windows. diff --git a/src/packages/process-host/src/growing-byte-buffer.ts b/src/packages/process-host/src/growing-byte-buffer.ts index 1837d6fabb6..c384d228bea 100644 --- a/src/packages/process-host/src/growing-byte-buffer.ts +++ b/src/packages/process-host/src/growing-byte-buffer.ts @@ -110,7 +110,6 @@ export class GrowingByteBuffer { } private reclaimHeadRoom(required: number): void { - // Slide the live bytes down while they fit in half the storage; only grow when they do not. if (required * 2 <= this.storage.byteLength) { this.storage.copy(this.storage, 0, this.start, this.start + this.length) } else { diff --git a/src/packages/process-host/src/posix-lsof-probe.test.ts b/src/packages/process-host/src/posix-lsof-probe.test.ts index db7c66e60c9..9a1fe81ff7e 100644 --- a/src/packages/process-host/src/posix-lsof-probe.test.ts +++ b/src/packages/process-host/src/posix-lsof-probe.test.ts @@ -49,7 +49,6 @@ async function runProbe(options: { signal?: string census?: string censusKillDenied?: boolean - /** Replaces the whole stub `lsof` body, for cases about what lsof answered. */ lsof?: string sockPath?: string }) { @@ -166,17 +165,7 @@ describe.skipIf(process.platform === 'win32')('lsof supervisor lifecycle', () => }) }) -/** - * When the probe runs as a uid that does not own the socket's holder, `lsof -t -a -U ` - * exits 1 with no stdout and no stderr. Measured on Debian 12: a live relay owned by root, - * probed as `nobody`, produced `exit=1 stdout=[] stderr=[]` — byte-identical to a genuinely - * stale socket, so no amount of inspecting lsof's answer can separate the two. That answer - * reaches `verdict: exited / evidence: no-holder`, which the host census reads as an idle - * endpoint a live relay still holds. - * - * The stub reproduces that exact shape rather than switching uid, which needs root and two - * accounts. What separates the cases is /proc/net/unix, which is world-readable. - */ +// Other-UID lsof misses look identical to stale sockets; stub that response without requiring root. const linuxOnly = describe.skipIf(process.platform !== 'linux') linuxOnly('lsof blindness to another uid', () => { @@ -204,8 +193,6 @@ linuxOnly('lsof blindness to another uid', () => { }) it('still reports lsof for an empty answer about a path nothing has bound', async () => { - // The control that keeps this from becoming a universal accumulation bug: on a healthy - // host a genuinely stale socket has no /proc/net/unix entry and must stay reapable. const dir = mkdtempSync(join(tmpdir(), 'orca-lsof-stale-')) try { const result = await runProbe({ lsof: BLIND_LSOF, sockPath: join(dir, 'never-bound.sock') }) @@ -222,9 +209,7 @@ linuxOnly('lsof blindness to another uid', () => { expect(result.stdout).toBe('lsof\n123\n') }) - // Both neighbours of the bound path, because a substring test would pass one and fail the - // other: `` is a prefix of the bound path, and the bound path is a prefix of - // `.other`. Only exact equality answers correctly for both. + // Test both prefix directions to reject substring-based socket matches. it.each([ ['a prefix of the bound path', (sockPath: string) => sockPath.slice(0, -1)], ['a path the bound one prefixes', (sockPath: string) => `${sockPath}.other`] diff --git a/src/packages/process-host/src/posix-lsof-probe.ts b/src/packages/process-host/src/posix-lsof-probe.ts index 664ed492456..faaa95bf50c 100644 --- a/src/packages/process-host/src/posix-lsof-probe.ts +++ b/src/packages/process-host/src/posix-lsof-probe.ts @@ -20,19 +20,14 @@ function groupExists(pid) { try { process.kill(-pid, 0); return true; } catch (error) { return error.code !== 'ESRCH'; } } -// An empty lsof answer means "nobody holds it" only if lsof could see. Running as a uid that -// does not own the holder, lsof exits 1 with no stdout and no stderr -- byte-identical to a -// genuinely stale socket. /proc/net/unix is world-readable and lists every bound unix socket -// regardless of owner, so an entry for this path alongside no pid proves lsof was blind. -// Absent off Linux, where this returns false and the marker stays whatever it already was. +// lsof cannot distinguish other-UID holders from stale sockets; corroborate with /proc/net/unix. function pathStillBound(target) { var text; try { text = require('fs').readFileSync('/proc/net/unix', 'utf8'); } catch (error) { return false; } var lines = text.split('\n'); for (var i = 0; i < lines.length; i++) { - // Num: RefCount Protocol Flags Type St Inode Path -- the path is the line remainder, so - // one containing spaces survives intact. + // The path is the line remainder and may contain spaces. var match = lines[i].match(/^\S+:(?:\s+\S+){5}\s+\d+ (.*)$/); if (match && match[1] === target) return true; } @@ -54,7 +49,6 @@ function finish(unconfirmed) { unavailable = true; return false; }); - // Only an otherwise-clean empty answer needs corroborating; a reported pid stands on its own. if (!unconfirmed && !unavailable && !stderrSeen && !pids.length && pathStillBound(process.argv[1])) { unavailable = true; } diff --git a/src/packages/process-host/src/process-spec.ts b/src/packages/process-host/src/process-spec.ts index 20c9a3838d5..e9df215559c 100644 --- a/src/packages/process-host/src/process-spec.ts +++ b/src/packages/process-host/src/process-spec.ts @@ -9,18 +9,12 @@ export type ChildProcessHandle = ChildProcess export type SpawnedProcess = ChildProcess -/** Centralizes host execution settings so argument encoding and tree ownership stay consistent. */ export type ProcessSpec = { - /** - * Program to run. On Windows this should already be an absolute path — - * spawning by bare name depends on the child's PATH, which under Group Policy - * or a stripped Electron environment can resolve to nothing. - */ + /** Prefer an absolute Windows path; stripped Electron PATHs may not resolve bare names. */ program: string args?: readonly string[] cwd?: string env?: NodeJS.ProcessEnv - /** Kill the process (and, on Windows, its console) after this long. */ timeoutMs?: number | null /** Written to stdin then closed. Omit to leave stdin empty and closed. */ input?: string @@ -28,15 +22,13 @@ export type ProcessSpec = { maxOutputBytes?: number /** Capture stdout as bytes without decoding; stdout stays empty in this mode. */ captureStdoutAsBytes?: boolean - /** Stop a parser command as soon as captured output exceeds its cap. */ killOnOutputLimit?: boolean - /** Kills the process when aborted; the result still reports the exit. */ + /** Aborting still resolves with the process exit result. */ signal?: AbortSignal /** Keep the child in its own POSIX process group for tree termination. */ detached?: boolean /** Preserve a caller-owned Windows command line such as a cmd.exe invocation. */ windowsVerbatimArguments?: boolean - /** Streaming callers may suppress child output for auxiliary processes. */ stdio?: NodeSpawnOptions['stdio'] /** Bun and Node require JSON for an IPC channel shared between the two runtimes. */ serialization?: NodeSpawnOptions['serialization'] @@ -68,9 +60,7 @@ export type ProcessResult = { stdout: string stdoutBytes?: Buffer stderr: string - /** True when the process was killed by `timeoutMs` rather than exiting. */ timedOut: boolean - /** True when stdout or stderr exceeded `maxOutputBytes` and was clipped. */ outputTruncated?: boolean } diff --git a/src/packages/process-host/src/process-tree-kill-gate.ts b/src/packages/process-host/src/process-tree-kill-gate.ts index 420fbfeda9b..d6555a8a967 100644 --- a/src/packages/process-host/src/process-tree-kill-gate.ts +++ b/src/packages/process-host/src/process-tree-kill-gate.ts @@ -1,19 +1,6 @@ -/** - * Seam that lets the main process decide, and record, the tree-kills issued - * from code it does not own. - * - * Why a seam and not a direct call: `signalProcessTree` is the choke point every - * `runProcess` termination funnels through, and the codex app-server and - * ephemeral-VM kills are shared with the CLI — all of them live outside - * `src/main` and cannot import the own-Chromium guard or the crash breadcrumb - * store. Main registers the guard at startup; everywhere else this admits every - * kill and records nothing. - */ +// Main installs its Chromium kill guard here; daemon, relay, and CLI cannot import main code. -/** Blast radius, not mechanism: `win-taskkill-tree` is addressed by pid and walks - * whatever tree that pid has *now*, so it can land on a recycled pid that is - * since one of Orca's own Chromium processes. A process group can only contain - * processes Orca itself put there. */ +/** Windows tree walks can target reused PIDs; POSIX groups contain only Orca-assigned processes. */ export type ProcessTreeKillScope = 'win-taskkill-tree' | 'posix-process-group' export type ProcessTreeKill = { @@ -22,9 +9,7 @@ export type ProcessTreeKill = { scope: ProcessTreeKillScope } -/** False means the caller must not walk that pid's tree — main is accounting for - * it. Killing the root through its own child handle stays correct and required: - * a handle cannot land on the recycled pid the refusal is about. */ +/** Refusal forbids the PID-based tree walk; callers must still kill the root through its handle. */ type ProcessTreeKillGate = (kill: ProcessTreeKill) => boolean let gate: ProcessTreeKillGate | null = null diff --git a/src/packages/process-host/src/process-tree-termination.test.ts b/src/packages/process-host/src/process-tree-termination.test.ts index 798bbe3a1af..9b9872c5072 100644 --- a/src/packages/process-host/src/process-tree-termination.test.ts +++ b/src/packages/process-host/src/process-tree-termination.test.ts @@ -138,11 +138,9 @@ describe('forceTerminateProcessTree', () => { await vi.advanceTimersByTimeAsync(5_000) await expect(pending).resolves.toBe(false) - // Killing either now would orphan the descendants taskkill has not reached. expect(taskkill.kill).not.toHaveBeenCalled() expect(child.kill).not.toHaveBeenCalled() - // A retry joins the same taskkill and sees its late success. const retry = forceTerminateProcessTree(child) taskkill.emit('close', 0) await expect(retry).resolves.toBe(true) @@ -242,14 +240,10 @@ describe('process-tree-kill breadcrumb seam', () => { it('never taskkills a pid the child already gave back to Windows', async () => { await withWindows(async () => { - // A reaped pid is Windows' to reissue, and this host may be the daemon or - // relay, where the main-process own-Chromium guard cannot run. const child = mockProcess(1234) child.exitCode = 0 - // `false`, not `true`: a taskkill against a reaped pid already resolved to - // `false`, and reporting verified termination here would release the git - // admission grant on root exit instead of on `close`. + // Root exit alone cannot release the barrier while descendants still hold the pipes. await expect(signalProcessTree(child, 'SIGKILL')).resolves.toBe(false) expect(spawnMock).not.toHaveBeenCalled() diff --git a/src/packages/process-host/src/process-tree-termination.ts b/src/packages/process-host/src/process-tree-termination.ts index 49e4445173c..55f76d6c67c 100644 --- a/src/packages/process-host/src/process-tree-termination.ts +++ b/src/packages/process-host/src/process-tree-termination.ts @@ -10,49 +10,19 @@ const PROBE_INTERVAL_MS = 25 const SUBPROCESS_TIMEOUT_MS = 2_000 const MAX_PS_OUTPUT_BYTES = 8 * 1024 * 1024 -/** - * Signal the child's whole tree. - * - * POSIX precondition: the child must have been spawned `detached`. The signal - * goes to the process group `-child.pid`, so a child that is not its own group - * leader would hand it to whatever group it inherited instead. - * - * Runs in every host — Electron main, the daemon, the relay, the CLI — so the - * own-Chromium guard arrives through `process-tree-kill-gate`, which main - * installs and every other host leaves admitting. The exit check below is what - * keeps the Windows branch off a pid that is no longer ours on those hosts. - * - * Both arms ask before walking the tree, so a refused pid never gets a - * pid-addressed kill; that also means the recorded crumb says "about to kill", - * not "killed". The root is still killed through its handle, which cannot reach - * the recycled pid the refusal was about, so a refusal is never a leak. Main's - * gate only ever refuses the `win-taskkill-tree` scope — a POSIX group holds - * only what Orca put in it — so the POSIX refusal arm is the seam's contract, - * not something any installed gate exercises today. - */ +/** POSIX children must be detached: signaling -pid targets the process group. */ export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): Promise { if (!child.pid) { killRoot(child, signal) return Promise.resolve(true) } if (process.platform === 'win32') { - // Why first: a root that the in-flight taskkill already killed must join - // that kill's verdict, not read as a reaped pid. + // A tracked taskkill may have killed the root already; join its verdict before checking exit. const tracked = trackedWindowsTreeKill(child) if (tracked) { return waitForWindowsTreeKill(tracked) } - // Why the exit check: once the child is reaped its pid is Windows' to - // reissue, and `taskkill /t /f` walks whatever tree owns it *now* — a - // recycled pid can be one of Orca's own Chromium processes (#10680). The - // POSIX branch below cannot hit this: killing a reaped group is ESRCH. - // - // Why `false`: this is exactly what a taskkill against a reaped pid already - // resolved to (non-zero exit -> killRoot + `false`), so skipping the unsafe - // spawn must not also flip the termination barrier to "verified". Reporting - // `true` here would release the git admission grant on root exit instead of - // on `close`, admitting the next git command while a descendant that - // inherited the pipes still holds the repo. + // A reaped Windows PID can be reused; root exit alone does not prove descendants are gone. if (childProcessHasExited(child)) { killRoot(child, signal) return Promise.resolve(false) @@ -66,8 +36,7 @@ export function signalProcessTree(child: ChildProcess, signal?: NodeJS.Signals): scope: 'posix-process-group' }) ) { - // Same shape as the reaped-pid skip above: refuse the group, still kill the - // root by handle, and report unverified. + // Refuse the group kill but still terminate the root through its handle. killRoot(child, signal) return Promise.resolve(false) } @@ -100,7 +69,6 @@ function taskkillTree( rootPid: number, signal?: NodeJS.Signals ): Promise { - // Asked before the spawn, not after: a refusal has to prevent the taskkill. if ( !admitProcessTreeKill({ pid: rootPid, site: 'run-process-tree', scope: 'win-taskkill-tree' }) ) { diff --git a/src/packages/process-host/src/run-process-windows-tree-kill.test.ts b/src/packages/process-host/src/run-process-windows-tree-kill.test.ts index caad7799bca..692286ccbc9 100644 --- a/src/packages/process-host/src/run-process-windows-tree-kill.test.ts +++ b/src/packages/process-host/src/run-process-windows-tree-kill.test.ts @@ -60,7 +60,7 @@ describe('runProcess barrier with a slow Windows taskkill', () => { settled = true }) - // Past the caller's 5 s taskkill wait and the 2 s exit grace. + // Exceed both the taskkill wait and root-exit grace. await vi.advanceTimersByTimeAsync(8_000) expect(settled).toBe(false) expect(root.kill).not.toHaveBeenCalled() @@ -85,10 +85,10 @@ describe('runProcess barrier with a slow Windows taskkill', () => { }) await vi.advanceTimersByTimeAsync(1_000) - // taskkill reached the root; its pipes close while descendants may still be dying. + // The root can exit before taskkill finishes its descendants. root.emit('exit', 1, null) root.emit('close', 1, null) - // The git admission slot is a concurrency budget, not a repo lock: it never outlives the root. + // The admission budget follows root lifetime, independently of tree verification. expect(onChildTerminated).toHaveBeenCalledOnce() let settled = false void pending.then(() => { diff --git a/src/packages/process-host/src/run-process.test.ts b/src/packages/process-host/src/run-process.test.ts index a466353e7ad..153fc5675fb 100644 --- a/src/packages/process-host/src/run-process.test.ts +++ b/src/packages/process-host/src/run-process.test.ts @@ -36,8 +36,6 @@ describe('resolveSpawn', () => { }) it('spawns a non-cmd program directly, letting Node do the argv quoting', () => { - // Node's own Windows quoting is already CommandLineToArgvW-correct for real - // executables; re-implementing it here would add risk for no gain. const resolved = resolveSpawn({ program: 'C:\\bin\\agent.exe', args: ['a b'] }, 'win32') expect(resolved.file).toBe('C:\\bin\\agent.exe') expect(resolved.args).toEqual(['a b']) @@ -54,8 +52,6 @@ describe('resolveSpawn', () => { }) it('treats a .cmd path as an ordinary program off Windows', () => { - // A POSIX host running a file that happens to end in .cmd must not gain a - // cmd.exe hop; the extension carries no meaning there. const resolved = resolveSpawn(SPEC, 'linux') expect(resolved.file).toBe(SPEC.program) expect(resolved.args).toEqual(SPEC.args) @@ -65,8 +61,7 @@ describe('resolveSpawn', () => { for (const { name, value } of WINDOWS_ARGUMENT_CORPUS) { const line = resolveSpawn({ program: 'C:\\a.cmd', args: [value] }, 'win32').args[0]! const body = line.slice('/d /v:off /s /c "'.length, -1) - // Every `&`/`|`/`<`/`>` must sit inside a quoted run. Walking the parity - // is the same thing cmd does, so this is the property that matters. + // cmd uses quote parity to decide whether metacharacters are operators. let quoted = false for (const char of body) { if (char === '"') { @@ -116,17 +111,12 @@ describe('runProcessSync', () => { }) it('does not report a deliberately terminated child as timedOut', () => { - // Both cases exit on SIGTERM, so reading the signal alone conflates them — - // and a caller that retries on timeout would then retry a process someone - // stopped on purpose. const result = runProcessSync({ program: process.execPath, args: ['-e', 'process.kill(process.pid, "SIGTERM")'], timeoutMs: 30_000 }) - // Why not assert the signal: Windows has no signals, so the same deliberate - // kill reports an exit code there and a signal on POSIX. What must hold on - // both is that neither shape reads as a timeout. + // Windows reports an exit code for deliberate termination; POSIX reports a signal. expect(result.timedOut).toBe(false) expect(result.code === 0 && result.signal === null).toBe(false) }) @@ -178,9 +168,6 @@ describe('stderr streaming', () => { describe('unkillable children', () => { it('settles after the grace period rather than outliving its own deadline', async () => { - // `close` only fires once the child is gone, so a child that ignores the - // kill would otherwise hold the promise forever — and callers that cache an - // in-flight probe would hand every later caller the same dead promise. const result = await runProcess({ program: process.execPath, args: [ @@ -196,8 +183,6 @@ describe('unkillable children', () => { describe('abort', () => { it('settles after the grace period when an aborted child ignores the signal', async () => { - // An aborted caller has stopped waiting; an unkillable child must not keep - // the promise alive on their behalf either. const controller = new AbortController() let stderr = '' const pending = runProcess({ @@ -263,8 +248,7 @@ describe('abort', () => { describe('stdin delivery failures', () => { it('survives a child that exits without reading its input', async () => { - // The queued write fails with EPIPE, and an unhandled error on a stream is - // an uncaught exception — it would take the whole main process down. + // A queued stdin write may fail with EPIPE after the child exits. const result = await runProcess({ program: process.execPath, args: ['-e', 'process.exit(7)'], @@ -278,8 +262,6 @@ describe('stdin delivery failures', () => { describe('a signal that is already aborted', () => { it('stops the child instead of running it to the timeout', async () => { - // addEventListener never fires for an abort that already happened, so the - // caller would wait out the full deadline having already given up. const controller = new AbortController() controller.abort() const onChildTerminated = vi.fn() diff --git a/src/packages/process-host/src/run-process.ts b/src/packages/process-host/src/run-process.ts index 9e6028fa17a..aba08ff047d 100644 --- a/src/packages/process-host/src/run-process.ts +++ b/src/packages/process-host/src/run-process.ts @@ -20,39 +20,17 @@ import type { ProcessResult } from './process-spec' import { DEFAULT_PROCESS_TIMEOUT_MS, DEFAULT_MAX_OUTPUT_BYTES } from './process-spec' -/** - * Grace between the timeout kill and giving up on the child's exit. - * - * Why give up at all: `close` only fires once the child is actually gone, and a - * child that ignores the kill never emits it -- so the promise would outlive - * its own deadline forever. Callers that cache an in-flight probe (the pwsh - * availability cache, the process-table reader) would then hand every later - * caller the same dead promise. - */ +// Bound waits for children that ignore termination and never emit close. const PROCESS_EXIT_GRACE_MS = 2_000 -/** - * Last resort for a barrier caller once tree termination could not be verified. - * - * Why bounded: waiting for the root's exit is what stops an unverified caller - * mutating shared state under a live child, but a tree that neither dies nor - * reports would otherwise leave the promise pending for the app's lifetime. - */ +// An unverifiable, silent tree must not leave barrier callers pending forever. const BARRIER_UNVERIFIED_EXIT_GRACE_MS = 10_000 -/** - * Start a child process. Use for long-lived or streaming children. - * - * The caller owns the returned streams, including their `error` events — an - * unhandled one is an uncaught exception that takes the main process down. - * `runProcess` handles that for you; here it cannot, because a blanket handler - * would also defeat callers that track and remove their own listeners. - */ +/** Callers own stream error handling; unhandled stream errors can crash the host process. */ export function spawnProcess(spec: PipedProcessSpec): PipedChildProcess export function spawnProcess(spec: ProcessSpec): ChildProcess export function spawnProcess(spec: ProcessSpec): ChildProcess { const resolved = resolveSpawn(spec, process.platform) - // Diagnostics only: uv_spawn runs synchronously on the calling thread, so this - // brackets the main-thread block for every child started through the wrapper. + // uv_spawn blocks the calling thread; measure that block for diagnostics. const spawnStartedAt = hasSpawnObserver() ? performance.now() : null const child = nodeSpawn(resolved.file, [...resolved.args], resolved.options) if (spawnStartedAt !== null) { @@ -61,13 +39,7 @@ export function spawnProcess(spec: ProcessSpec): ChildProcess { return child } -/** - * Run a child process to completion and capture its output. - * - * Never rejects on a non-zero exit — the exit code is data. Rejects only when - * the process could not be started at all. - * Tail capture keeps final diagnostics without changing termination policy. - */ +/** Nonzero exits resolve; spawn errors reject. Tail capture preserves final diagnostics. */ export function runProcess( spec: ProcessSpec, outputCapture: 'head' | 'tail' = 'head' @@ -130,12 +102,7 @@ export function runProcess( spec.terminationBarrier.observeStderr?.(chunk) } }) - // Why listeners that do nothing: an unhandled `error` on a stream is an - // uncaught exception, and that takes the whole main process down. A child - // that exits without reading makes the queued stdin write fail with EPIPE, - // and a broken pipe can surface on the read side too. The child's own - // `error` listener does not cover its streams. Losing output is not worth a - // crash, and the exit code still reaches the caller. + // Stream errors (including stdin EPIPE) are otherwise uncaught; the exit still reaches callers. for (const stream of [child.stdin, child.stdout, child.stderr]) { stream?.on('error', () => {}) } @@ -181,8 +148,7 @@ export function runProcess( terminationReporter.report() resolveBarrierIfSafe() } - // A root killed before taskkill reads the tree orphans the descendants it - // has not reached, so an in-flight taskkill owns that fallback. + // Let taskkill finish reading the tree before killing the root, or descendants may be orphaned. const killRootUnlessTreeKillInFlight = (): void => { if (!inFlightWindowsTreeKill(child)) { terminate(child, 'SIGKILL') @@ -198,20 +164,12 @@ export function runProcess( if (!barrierAttemptComplete) { return } - // Why a second deadline: the tree survived every attempt and the root has - // gone silent, so nothing else will ever settle this promise. + // Nothing else can settle a silent tree after all termination attempts fail. barrierDeadlineTimer ??= setTimeout(settleBarrierOutcome, BARRIER_UNVERIFIED_EXIT_GRACE_MS) barrierDeadlineTimer.unref?.() } - /** - * Stop the child, then settle. - * - * Without a barrier, settle whether or not the child complies. With one, wait - * for verified tree termination or a root exit first — a descendant can keep - * `close` pending after the root exits, but failed verification must not let - * callers mutate shared state — then settle on the deadline regardless. - */ + // Barrier callers wait for termination evidence, bounded by a final deadline. const stopAndSettle = (): void => { if (spec.terminationBarrier) { barrierStopping = true @@ -239,8 +197,7 @@ export function runProcess( barrierTerminationVerified = initialTerminated || forceTerminated terminationReporter.reportIf(barrierTerminationVerified) if (!barrierTerminationVerified) { - // The barrier never confirmed the tree died, so the root - // would otherwise outlive the abort or timeout. + // Unverified termination must not leave the root running beyond abort/timeout. killRootUnlessTreeKillInFlight() } resolveBarrierIfSafe() @@ -287,13 +244,9 @@ export function runProcess( }, spec.timeoutMs ?? DEFAULT_PROCESS_TIMEOUT_MS) timer?.unref?.() - // Why the same escalation: an aborted caller has stopped waiting, so an - // unkillable child must not keep the promise alive on their behalf either. const onAbort = (): void => stopAndSettle() spec.signal?.addEventListener('abort', onAbort, { once: true }) - // Why check after subscribing: a signal that was already aborted never - // fires the event, so the child would otherwise run to its full timeout on - // behalf of a caller who had already given up. + // Already-aborted signals emit no event; recheck after subscribing to close the race. if (spec.signal?.aborted) { onAbort() } @@ -329,14 +282,12 @@ export function runProcess( resolveFromClose(code, signal) }) - // Why close rather than leave open: a child that reads stdin (a hook - // draining its payload, a CLI probing for a TTY) otherwise blocks until the - // timeout instead of seeing EOF immediately. + // Send EOF so stdin readers do not wait until timeout. child.stdin?.end(spec.input) }) } -/** Best-effort root termination, or whole-tree termination for barrier callers. */ +// Best-effort root termination; tree termination is handled separately. function terminate(child: ChildProcess, signal?: NodeJS.Signals): void { try { child.kill(signal) @@ -345,18 +296,10 @@ function terminate(child: ChildProcess, signal?: NodeJS.Signals): void { } } -/** - * Synchronous variant, for the call sites that genuinely cannot await — CLI - * entry points and teardown paths that run while the event loop is stopping. - * - * Prefer `runProcess`. This exists so those callers still get the Windows - * invariants (hidden console, correct `.cmd` argv) instead of reaching for - * `execFileSync` and re-deciding them. - */ +/** Prefer runProcess; reserve blocking execution for synchronous entry points and teardown. */ export function runProcessSync(spec: ProcessSpec): ProcessResult { const resolved = resolveSpawn(spec, process.platform) - // Diagnostics only: spawnSync holds the calling thread until the child exits, - // so the whole call is the block, reported even when it throws. + // spawnSync blocks until exit; report that duration even if it throws. const spawnStartedAt = hasSpawnObserver() ? performance.now() : null try { return runResolvedProcessSync(spec, resolved) @@ -388,13 +331,9 @@ function runResolvedProcessSync(spec: ProcessSpec, resolved: ResolvedSpawn): Pro stdout: spec.captureStdoutAsBytes ? '' : (result.stdout?.toString('utf8') ?? ''), ...(spec.captureStdoutAsBytes ? { stdoutBytes: result.stdout ?? Buffer.alloc(0) } : {}), stderr: result.stderr?.toString('utf8') ?? '', - // Why always false: spawnSync reports an overrun as an ENOBUFS error, and - // the guard above rethrows it, so no truncated result reaches this point. + // ENOBUFS is rethrown above, so truncated output never reaches this result. outputTruncated: false, - // Why ETIMEDOUT and not the signal: a timeout kills with SIGTERM, but so - // does anything else that terminates the child, and only a timeout also - // sets this error. Reading the signal alone reports a deliberately - // stopped process as having timed out, which callers retry. + // SIGTERM can be deliberate; only ETIMEDOUT proves a timeout. timedOut } } diff --git a/src/packages/process-host/src/spawn-observer.ts b/src/packages/process-host/src/spawn-observer.ts index 086d46a7af8..05974385254 100644 --- a/src/packages/process-host/src/spawn-observer.ts +++ b/src/packages/process-host/src/spawn-observer.ts @@ -1,12 +1,4 @@ -/** - * Diagnostic seam for counting every child process started through this - * package. `src/main/diagnostics/main-thread-churn-probe` registers itself here - * under ORCA_MAIN_THREAD_DIAGNOSTICS=1; with nothing registered every spawn - * pays one undefined check, so production behaviour is unchanged. - * - * Why a seam rather than a direct import: run-process.ts also runs in the - * terminal daemon, the relay and the CLI, none of which may depend on main. - */ +// Main installs diagnostics here; daemon, relay, and CLI cannot depend on main code. export type SpawnObserver = (command: string, args: readonly string[], blockMs: number) => void let observer: SpawnObserver | null = null diff --git a/src/packages/process-host/src/spawn-resolution.ts b/src/packages/process-host/src/spawn-resolution.ts index 242500a1376..c0665fcb0e9 100644 --- a/src/packages/process-host/src/spawn-resolution.ts +++ b/src/packages/process-host/src/spawn-resolution.ts @@ -9,18 +9,7 @@ export type ResolvedSpawn = { options: NodeSpawnOptions } -/** - * Translate a spec into the exact `child_process.spawn` call to make. - * - * Exported so the Windows branch is testable from macOS/Linux: the decisions - * below are the whole point of the spawn chokepoint, and they must not be - * observable only on the platform that breaks. - * - * Pure except on the win32 `.cmd` branch, where shim resolution does a `stat` - * and (on a cache miss) one bounded read of the shim itself. Both are inside - * try/catch and any failure falls back to the cmd.exe path, so the function - * still cannot throw or reach anything but the program path it was handed. - */ +/** Host-independent for Windows tests; recognized shim I/O failures fall back to cmd.exe. */ export function resolveSpawn(spec: ProcessSpec, platform: NodeJS.Platform): ResolvedSpawn { const args = spec.args ?? [] const base: NodeSpawnOptions = { @@ -28,15 +17,11 @@ export function resolveSpawn(spec: ProcessSpec, platform: NodeJS.Platform): Reso env: spec.env, stdio: spec.stdio ?? ['pipe', 'pipe', 'pipe'], serialization: spec.serialization, - // Why unconditional: Orca's main process is GUI-subsystem and owns no - // console, so every console-subsystem child it starts gets a fresh visible - // conhost that takes foreground — keystrokes typed into an Orca terminal at - // that moment land in the black box instead. + // GUI-subsystem parents otherwise spawn visible consoles that steal foreground input. windowsHide: true, detached: spec.detached, windowsVerbatimArguments: spec.windowsVerbatimArguments, - // Why never `shell: true`: it concatenates arguments without escaping (Node - // itself warns DEP0190) and it silently makes windowsHide a no-op. + // shell:true loses argument escaping and disables windowsHide. shell: false, ...(spec.terminationBarrier && platform !== 'win32' ? { detached: true } : {}) } @@ -45,12 +30,7 @@ export function resolveSpawn(spec: ProcessSpec, platform: NodeJS.Platform): Reso return { file: spec.program, args, options: base } } - // An npm/pnpm shim is a generated file that only locates node and runs a - // script, so reading it lets us spawn that program directly. That drops - // cmd.exe from the tree — which is what Defender scores as obfuscation once - // the caret-escaped payload is agent prompt text — and lifts cmd's ban on - // arguments containing a line break. Unrecognised shims resolve to null and - // keep the cmd.exe path below. + // Direct shim resolution avoids cmd escaping, EDR obfuscation flags, and multiline-argument limits. const shim = resolveWindowsCmdShim(spec.program, spec.env ?? process.env) if (shim) { return { @@ -59,19 +39,13 @@ export function resolveSpawn(spec: ProcessSpec, platform: NodeJS.Platform): Reso options: { ...base, ...(shim.env ? { env: shim.env } : {}), - // Why cleared rather than inherited: the flag exists for callers that - // hand us a whole pre-built command line, and there is no such line - // here — Node would join `[script, ...args]` unquoted and shred any - // argument containing a space. + // Node must quote argv normally after replacing the caller-owned cmd line. windowsVerbatimArguments: undefined } } } - // Node refuses to spawn `.cmd`/`.bat` without a shell (EINVAL, the - // CVE-2024-27980 mitigation), so cmd.exe has to be the program. Building the - // line ourselves — rather than handing Node `shell: true` — is what keeps the - // arguments intact and the console hidden. + // Node rejects direct .cmd/.bat spawns; build the cmd line ourselves to preserve quoting and hiding. const comSpec = spec.env?.ComSpec ?? process.env.ComSpec ?? 'cmd.exe' return { file: comSpec, diff --git a/src/packages/process-host/src/windows-cmd-shim-resolution.ts b/src/packages/process-host/src/windows-cmd-shim-resolution.ts index c75da83ea81..e18853f614f 100644 --- a/src/packages/process-host/src/windows-cmd-shim-resolution.ts +++ b/src/packages/process-host/src/windows-cmd-shim-resolution.ts @@ -1,107 +1,57 @@ -/** - * Resolve an npm/pnpm-generated `.cmd` shim to the program it would have run. - * - * Why: a `.cmd` target forces the spawn through `cmd.exe /c` with every - * argument caret-escaped (see windows-command-line.ts for that encoding). A - * long `cmd.exe /c` line whose caret-escaped payload is natural-language agent - * prompt text is what Microsoft Defender for Endpoint's command-line model - * scores as obfuscation, and `codex.cmd` sits in the spawn cluster of a real - * MDE incident against Orca. These shims are generated files whose entire body - * is "find node, run this script", so reading one and spawning - * `node.exe