chore: shorten process-host comments (#32262)

Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>

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(

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: 'a<b>c' },
   { 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<Record<string, string>> = {
   F: 'EXPANDED_F',
   VAR: 'EXPANDED_VAR'

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
- *   <repo>\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: <repo>\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}" %*`])
 }

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
   }
 }

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.

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 {

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 <path>`
- * 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: `<sock>` is a prefix of the bound path, and the bound path is a prefix of
-  // `<sock>.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`]

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;
   }

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
 }
 

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

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()

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<boolean> {
   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<boolean> {
-  // 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' })
   ) {

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(() => {

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()

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
   }
 }

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

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,

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 <script> <args…>` removes cmd.exe — and with it the escaping —
- * from the process tree.
- *
- * It also removes a real limitation: cmd's parser ends the command at a raw
- * CR/LF whatever the quote state, so a multi-line agent prompt through a `.cmd`
- * shim has to be rejected outright. The resolved path has no such problem.
- *
- * Everything here is deliberately all-or-nothing. A file that does not match a
- * known shape exactly, or whose resolved target cannot be confirmed on disk,
- * returns null and the caller keeps today's `cmd.exe /c` behaviour. A
- * mis-resolution silently runs the wrong program or drops arguments, which is
- * far worse than an EDR alert.
- */
+// Resolve only exact known shims with verified targets; otherwise preserve cmd.exe fallback.
 import { readFileSync, statSync, type Stats } from 'node:fs'
 import { win32 } from 'node:path'
 
-/** Escape hatch if resolution ever picks the wrong target in the field. */
 const DISABLE_FLAG = 'ORCA_DISABLE_CMD_SHIM_RESOLUTION'
 
-/** Real shims are under 2KB; anything larger is not one of these generators. */
+// Bound reads of unrecognized or malformed shim files.
 const MAX_SHIM_BYTES = 64 * 1024
 
-/** Both spellings of the shim's own directory. Each already ends in `\`, so the
- * separator the shim writes after it is optional and inert. */
+// Both dp0 spellings end in a separator; tolerate a redundant trailing one.
 const DP0 = String.raw`(?:%~dp0|%dp0%)\\?`
 const DP0_NODE_EXE = `"${DP0}node\\.exe"`
 const dp0Path = (group: string): string => `"${DP0}(?<${group}>[^"\\r\\n]+)"`
 
 const ECHO_OFF = String.raw`@echo off\n`
-/** npm's `cmd-shim` captures its own directory through a subroutine. */
 const FIND_DP0 = String.raw`GOTO start\n:find_dp0\nSET dp0=%~dp0\nEXIT /b\n:start\nSETLOCAL\nCALL :find_dp0\n`
-/** pnpm prepends its virtual-store directories so the script can resolve deps. */
 const NODE_PATH_BLOCK = String.raw`(?:@IF NOT DEFINED NODE_PATH \(\n@SET "NODE_PATH=(?<nodePath>[^"\r\n]*)"\n\) ELSE \(\n@SET "NODE_PATH=(?<nodePathElse>[^"\r\n]*)"\n\)\n)?`
 const PATHEXT_STRIP = String.raw`SET PATHEXT=%PATHEXT:;\.JS;=;%`
 
-/**
- * Current `cmd-shim`: picks the interpreter into `%_prog%`, then runs it from a
- * single trailing line. This is the shape of `codex.cmd`.
- */
+// Modern npm cmd-shim selects the interpreter through %_prog%.
 const NPM_PROG_NODE_SHIM = new RegExp(
   String.raw`^${ECHO_OFF}${FIND_DP0}IF EXIST ${DP0_NODE_EXE} \(\nSET "_prog=${DP0}node\.exe"\n\) ELSE \(\nSET "_prog=node"\n${PATHEXT_STRIP}\n\)\nendLocal & goto #_undefined_# 2>NUL \|\| title %COMSPEC% & "%_prog%" +${dp0Path('script')} +%\*$`,
   'i'
 )
 
-/**
- * Legacy `cmd-shim` and pnpm's `@zkochan/cmd-shim`: the same script spelled once
- * per interpreter branch.
- */
+// Legacy npm and pnpm repeat the script in both interpreter branches.
 const BRANCHED_NODE_SHIM = new RegExp(
   String.raw`^(?:@SETLOCAL\n)?${NODE_PATH_BLOCK}@?IF EXIST ${DP0_NODE_EXE} \(\n${DP0_NODE_EXE} +${dp0Path('script')} +%\*\n\) ELSE \(\n(?:@?SETLOCAL\n)?@?${PATHEXT_STRIP}\nnode +${dp0Path('scriptElse')} +%\*\n\)$`,
   'i'
 )
 
-/** `cmd-shim` for a target that needs no interpreter (a bundled `.exe`). */
+// npm shims for native executables need no interpreter.
 const NPM_DIRECT_SHIM = new RegExp(
   String.raw`^${ECHO_OFF}(?:${FIND_DP0})?${dp0Path('target')} +%\*$`,
   'i'
 )
 
-/** pnpm's equivalent, which omits `@echo off` and keeps only `@SETLOCAL`. */
+// pnpm native-executable shims may omit echo-off.
 const PNPM_DIRECT_SHIM = new RegExp(String.raw`^(?:@SETLOCAL\n)?@?${dp0Path('target')} +%\*$`, 'i')
 
-// `:` matters as much as the operators: `win32.isAbsolute('D:evil.js')` is
-// false, but `win32.resolve` reads the drive letter and lands on `D:\evil.js`,
-// outside the shim directory entirely.
-//
-// Rejecting every `:` is provably free rather than merely untested: Windows
-// reserves the character in a path segment, so a relative path cannot contain
-// one at all. The only spellings that can are drive-qualified (`D:x`), an
-// alternate data stream (`a.js:zone`), or a `\\?\` device path — and the last
-// is already refused as absolute. No generator can emit a shim-relative path
-// this rule would wrongly refuse.
+// Reject drive-relative paths and alternate streams: isAbsolute alone permits D:evil.js and a.js:zone.
 const UNSAFE_SHIM_PATH = /[%^&|<>":\r\n]/
 
-/** A target with no interpreter must be something CreateProcess can start on
- * its own. Extensionless or `.cmd` targets need cmd's own PATHEXT search, which
- * is exactly the hop being removed. */
+// Extensionless and batch targets need cmd PATHEXT lookup, so cannot bypass cmd.exe.
 const DIRECT_TARGET_EXTENSIONS = ['.exe', '.com']
 
 export type ParsedWindowsCmdShim =
   | { kind: 'node'; script: string; nodePathPrefix?: string }
   | { kind: 'direct'; target: string }
 
-/**
- * A captured path must be relative to the shim's own directory and free of the
- * characters that mean we misread the file — `%` is an unexpanded variable, the
- * rest are cmd operators we are not emulating.
- */
 function isPlainRelativePath(spelled: string): boolean {
   return !UNSAFE_SHIM_PATH.test(spelled) && !win32.isAbsolute(spelled)
 }
 
-/** Collapse a shim to comparable text: shims differ only in line endings,
- * indentation and blank lines between generators and versions. */
 function canonicalize(contents: string): string {
   return contents
     .replace(/^\uFEFF/, '')
@@ -111,11 +61,7 @@ function canonicalize(contents: string): string {
     .join('\n')
 }
 
-/**
- * Recognise a generated shim. Pure, so the shapes are testable off Windows.
- *
- * Returns paths exactly as the shim spells them, relative to its own directory.
- */
+/** Returns paths as spelled in the shim, relative to its directory. */
 export function parseWindowsCmdShim(contents: string): ParsedWindowsCmdShim | null {
   const canonical = canonicalize(contents)
 
@@ -126,8 +72,7 @@ export function parseWindowsCmdShim(contents: string): ParsedWindowsCmdShim | nu
 
   const branched = BRANCHED_NODE_SHIM.exec(canonical)?.groups
   if (branched?.script) {
-    // Both branches must name the same script; if they differ we matched a file
-    // that only looks like a shim.
+    // Divergent branches are not a shim we can safely replace.
     if (branched.script !== branched.scriptElse || !isPlainRelativePath(branched.script)) {
       return null
     }
@@ -135,8 +80,7 @@ export function parseWindowsCmdShim(contents: string): ParsedWindowsCmdShim | nu
     if (nodePath === undefined) {
       return { kind: 'node', script: branched.script }
     }
-    // The else branch must be exactly "prefix, then whatever was there", or the
-    // prepend we would reproduce is not the one the shim performs.
+    // Only reproduce an exact NODE_PATH prepend, without variable expansion.
     if (nodePath.includes('%') || branched.nodePathElse !== `${nodePath};%NODE_PATH%`) {
       return null
     }
@@ -158,39 +102,11 @@ type ParseCacheEntry = {
   parsed: ParsedWindowsCmdShim | null
 }
 
-/** Shim bodies do not change between spawns, but an upgrade rewrites them —
- * hence the mtime/size half of the key. */
+// mtime and size invalidate parsed bodies after package upgrades.
 const parseCache = new Map<string, ParseCacheEntry>()
 const PARSE_CACHE_LIMIT = 256
 
-/**
- * The interpreter each shim directory resolves to, keyed by that directory and
- * the PATH that was searched.
- *
- * Why cached at all: the parse cache spares the shim read but not this walk, so
- * a 30-entry PATH cost 30 `stat`s on every spawn of an already-parsed shim —
- * synchronous I/O on `resolveSpawn`, where one dead network mount in PATH
- * blocks the calling thread each time.
- *
- * Held for the process's life, and the two stale directions are treated
- * differently on purpose:
- *
- * - A stale `null` is left alone. A `node.exe` installed after the first probe
- *   is not picked up until restart, which only means the working cmd.exe
- *   fallback stays in use — and re-probing to catch that is the whole cost this
- *   cache exists to avoid.
- * - A stale path is revalidated on every hit, because it is the direction that
- *   breaks. `resolveSpawn` would otherwise keep returning an interpreter that
- *   has since been uninstalled, failing the spawn with ENOENT where an uncached
- *   process falls back to cmd.exe and succeeds. Verified by execution: without
- *   the check, deleting the cached `node.exe` still yielded its path.
- *
- * Known gap: PATH is in the key, so a caller that rebuilds `env` per spawn with
- * a varying PATH — a per-worktree `.bin` prepended, say — misses every time and
- * pays the full walk. Correct, just no faster than before. The 256 cap and the
- * wholesale clear mean those misses cost memory nothing, so this is a bound on
- * who benefits rather than a leak.
- */
+// Cache interpreter lookup to avoid a synchronous PATH walk on every spawn.
 const nodeCache = new Map<string, string | null>()
 const NODE_CACHE_LIMIT = 256
 
@@ -204,12 +120,7 @@ function statFile(path: string): Stats | null {
 }
 
 function readParsedShim(program: string): ParsedWindowsCmdShim | null {
-  // This `stat` puts synchronous I/O on the spawn path, where `resolveSpawn`
-  // previously had none: a shim on an unresponsive network share now blocks the
-  // caller's thread until the filesystem gives up. Judged acceptable because a
-  // `.cmd` on such a share was already about to be spawned from it. It is the
-  // only such cost paid when nothing resolves — the interpreter lookup runs
-  // only for a shim that already parsed, and is itself cached.
+  // A synchronous stat can block on network shares; interpreter probes run only for recognized shims.
   const stats = statFile(program)
   if (!stats || stats.size > MAX_SHIM_BYTES) {
     return null
@@ -225,10 +136,6 @@ function readParsedShim(program: string): ParsedWindowsCmdShim | null {
     return null
   }
   const parsed = parseWindowsCmdShim(contents)
-  // Clearing wholesale drops hot entries with cold ones, where an LRU would
-  // not. Left as is because the cap is per-process and one entry per distinct
-  // `.cmd` path Orca ever spawns; reaching it means a re-read, not a wrong
-  // answer.
   if (parseCache.size >= PARSE_CACHE_LIMIT) {
     parseCache.clear()
   }
@@ -242,57 +149,22 @@ function firstEnvKey(env: NodeJS.ProcessEnv, name: string): string | undefined {
   return Object.keys(env).find((key) => key.toLowerCase() === lower && env[key] !== undefined)
 }
 
-/** cmd's own default when PATHEXT is unset or empty. The order is the point:
- * `.COM` outranks `.EXE`, so a `node.com` is what cmd runs even with a
- * `node.exe` sitting beside it. */
+// Preserve cmd PATHEXT order: node.com outranks node.exe.
 const DEFAULT_PATHEXT = '.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC'
 
-/**
- * The interpreter the shim itself would pick: a `node.exe` beside it, else
- * whatever a bare `node` resolves to on PATH.
- *
- * The sibling test is exact, matching the shim's own literal
- * `IF EXIST "%~dp0\node.exe"`. The PATH scan follows cmd's rule instead: the
- * first directory holding ANY PATHEXT spelling of `node` wins, and inside that
- * directory PATHEXT order decides. So a `node.com` early on PATH beats a
- * `node.exe` later, and only the `.exe` outcome is one we can start directly.
- * Every other winning spelling returns null and keeps the cmd.exe path, which a
- * `.bat`/`.cmd` node would have needed anyway.
- *
- * Scanning past a non-`.exe` winner to a `node.exe` further down PATH is the
- * tempting shortcut and the one bug this module must never have: it silently
- * runs a different binary than the shim does. Every decision here stays a
- * strict subset of what cmd.exe would pick, or gives up.
- *
- * Not searched: the working directory, which cmd would consult first for a bare
- * name. Preferring a `node.exe` that happens to sit in the cwd over the
- * installed one is a Windows footgun, not a behaviour worth reproducing.
- */
+/** Prefer sibling node.exe, then cmd PATH/PATHEXT order; deliberately exclude cwd lookup. */
 function resolveShimNode(directory: string, env: NodeJS.ProcessEnv): string | null {
   const pathKey = firstEnvKey(env, 'PATH')
   const pathValue = (pathKey ? env[pathKey] : undefined) ?? ''
   const pathExtKey = firstEnvKey(env, 'PATHEXT')
   const pathExtValue = (pathExtKey ? env[pathExtKey] : undefined) || DEFAULT_PATHEXT
-  // All three inputs are in the key because all three decide the answer: the
-  // shim prefers its own directory, falls back to PATH, and PATHEXT orders the
-  // spellings tried within each PATH entry. An edit to any of them therefore
-  // misses rather than serving the previous interpreter. Newlines separate them
-  // because Windows allows one in none of the three.
   const key = `${directory}\n${pathValue}\n${pathExtValue}`
   const cached = nodeCache.get(key)
-  // A hit is confirmed still on disk before it is used, because the two stale
-  // directions are not symmetric. A stale `null` is safe and stays uncorrected:
-  // it keeps the cmd.exe fallback, which works. A stale path is not — handing
-  // `resolveSpawn` a `node.exe` that has since been uninstalled (or dropped
-  // from PATH by a version manager) fails the spawn with ENOENT, where an
-  // uncached process would have fallen back and succeeded. One `stat` instead
-  // of one per PATH entry, so the walk this cache exists to skip is still skipped.
+  // Revalidate cached paths after uninstall; cached null safely retains the cmd.exe fallback.
   if (cached !== undefined && (cached === null || statFile(cached))) {
     return cached
   }
   const resolved = probeShimNode(directory, pathValue, pathExtValue)
-  // Same wholesale eviction as the parse cache, for the same reason: the cap is
-  // per-process and one entry per distinct shim directory Orca ever spawns from.
   if (nodeCache.size >= NODE_CACHE_LIMIT) {
     nodeCache.clear()
   }
@@ -311,17 +183,11 @@ function probeShimNode(directory: string, pathValue: string, pathExtValue: strin
     .filter((extension) => extension.startsWith('.'))
   for (const entry of pathValue.split(';')) {
     const trimmed = entry.trim().replace(/^"(.*)"$/, '$1')
-    // A relative PATH entry resolves against the child's working directory, so
-    // we cannot answer it here.
+    // Relative PATH entries depend on the child cwd, which is unknown here.
     if (!trimmed || !win32.isAbsolute(trimmed)) {
       continue
     }
-    // Why every spelling and not just `.exe`: cmd stops at the first directory
-    // holding any of them, so passing over a `node.com` here and matching a
-    // `node.exe` further down PATH would run a binary the shim never would.
-    // Costs one `stat` per PATHEXT entry per node-less directory, paid once per
-    // process because of the cache above -- where the per-spawn walk it
-    // replaced paid its own stats on every single spawn.
+    // Stop at any PATHEXT winner; skipping node.com for a later node.exe would run the wrong binary.
     for (const extension of extensions) {
       const candidate = win32.join(trimmed, `node${extension}`)
       if (!statFile(candidate)) {
@@ -341,21 +207,13 @@ function withNodePath(env: NodeJS.ProcessEnv, prefix: string): NodeJS.ProcessEnv
 }
 
 export type WindowsCmdShimResolution = {
-  /** Executable to spawn in place of the `.cmd`. */
   program: string
-  /** Arguments the shim inserts ahead of the caller's own argv. */
   prefixArgs: readonly string[]
   /** Set only when the shim itself mutates the environment (pnpm's NODE_PATH). */
   env?: NodeJS.ProcessEnv
 }
 
-/**
- * Resolve `program` to the executable its shim body would have launched, or
- * null to keep the `cmd.exe /c` path.
- *
- * `env` is the environment the child will actually receive, because both the
- * PATH lookup for `node` and the NODE_PATH prepend depend on it.
- */
+/** Pass the actual child environment for PATH lookup and NODE_PATH prepending; null keeps cmd.exe. */
 export function resolveWindowsCmdShim(
   program: string,
   env: NodeJS.ProcessEnv
@@ -364,8 +222,7 @@ export function resolveWindowsCmdShim(
   if (disableKey && env[disableKey]) {
     return null
   }
-  // A relative program is resolved against the child's working directory, which
-  // is the caller's to decide, not ours to guess.
+  // Relative programs depend on the child cwd, which is unknown here.
   if (!win32.isAbsolute(program)) {
     return null
   }
@@ -375,7 +232,6 @@ export function resolveWindowsCmdShim(
   }
   const directory = win32.dirname(program)
 
-  // `resolve` collapses the `..` hops and the doubled separator `%dp0%\` leaves.
   if (parsed.kind === 'direct') {
     const target = win32.resolve(directory, parsed.target)
     const lower = target.toLowerCase()

src/packages/process-host/src/windows-command-line.test.ts

@@ -27,9 +27,6 @@ describe('quoteWindowsArgument', () => {
   })
 
   it('writes an embedded quote as "" so cmd quote parity stays even', () => {
-    // Why this exact assertion: `\"` also decodes to a literal quote, but it
-    // leaves cmd one quote short, and every later `&`/`|` on the line then
-    // parses as an operator instead of data.
     expect(quoteWindowsArgument('c"d')).toBe('"c""d"')
     expect(quoteParity(quoteWindowsArgument('c"d'))).toBe(0)
   })
@@ -48,7 +45,6 @@ describe('buildWindowsCmdShimCommandLine', () => {
   })
 
   it('breaks every percent out of the quoted run', () => {
-    // `%VAR%` expands even inside quotes, so the pair must not survive intact.
     const line = buildWindowsCmdShimCommandLine('C:\\bin\\agent.cmd', ['e%F%g'])
     expect(line).toContain('"^%"')
     expect(line).not.toMatch(/%F%/)
@@ -56,8 +52,6 @@ describe('buildWindowsCmdShimCommandLine', () => {
 
   it('pins the cmd flags that make the encoding safe', () => {
     const line = buildWindowsCmdShimCommandLine('C:\\bin\\agent.cmd', ['x'])
-    // /d: no registry AutoRun. /v:off: `!` stays literal regardless of the
-    // Command Processor default. /s: strip exactly the outer quote pair.
     expect(line.startsWith('/d /v:off /s /c "')).toBe(true)
     expect(line.endsWith('"')).toBe(true)
   })
@@ -87,9 +81,7 @@ describe('line breaks', () => {
     ['carriage return in an argument', 'agent', ['fix\rthis']],
     ['newline in the program path', 'C:\\a\nb.cmd', []]
   ])('rejects %s rather than truncating it', (_case, program, args) => {
-    // cmd ends the command at a raw CR/LF whatever the quote state, so an
-    // encoded newline silently truncates the argument and can leave the
-    // remainder to run as a further command.
+    // cmd terminates at raw CR/LF even inside quotes; the remainder could become another command.
     expect(() => buildWindowsCmdShimCommandLine(program, args)).toThrow(/line break/)
   })
 })

src/packages/process-host/src/windows-command-line.ts

@@ -1,36 +1,6 @@
-/**
- * Windows command-line construction.
- *
- * Two parsers read the line Orca hands to CreateProcess, and they disagree:
- *
- * - The target program's own startup code (`CommandLineToArgvW`, which the CRT and
- *   Node both use) understands `\"` as a literal quote and `\\` as a literal
- *   backslash.
- * - `cmd.exe` — unavoidable for a `.cmd`/`.bat` target — does NOT. It counts `"`
- *   naively to track quote state, and inside that state it still expands `%VAR%`.
- *
- * So a `\"` written for the first parser silently flips cmd's quote parity, and
- * every later `&` `|` `<` `>` on the line stops being data and becomes an
- * operator. Measured on Windows 11 with a real `.cmd` shim: `["a b", 'c"d',
- * "e%F%g", "h&i", "j^k"]` came back as `["a b", 'c"d', "e^%F^%g", "h"]` — the
- * `&` truncated the argument AND ran the remainder as a command.
- *
- * The encoding below satisfies both parsers at once and round-trips all of the
- * adversarial cases; see windows-command-line.test.ts for the corpus.
- */
+// cmd counts quotes naively and expands %VAR%; quoting must also survive CommandLineToArgvW.
 
-/**
- * Quote one argument for `CommandLineToArgvW`, writing an embedded `"` as `""`
- * rather than `\"`.
- *
- * Why `""` and not `\"`: both spellings decode to a literal quote, but only `""`
- * leaves cmd's naive quote count even. Since the same encoding has to survive a
- * cmd hop for `.cmd` targets, we pay the (identical-length) `""` spelling
- * everywhere rather than keep two dialects.
- *
- * Backslashes are still doubled before a quote and at the end of a quoted run,
- * because that part `CommandLineToArgvW` does interpret.
- */
+// Doubled quotes decode literally while preserving cmd quote parity; backslashes still need doubling.
 function quoteWindows(value: string, escapePercent: boolean): string {
   if (!(escapePercent ? /[\\"%]/ : /[\\"]/).test(value)) {
     return `"${value}"`
@@ -43,17 +13,12 @@ function quoteWindows(value: string, escapePercent: boolean): string {
       continue
     }
     if (char === '"') {
-      // The backslash run is literal, so double it; the quote itself becomes `""`.
       quoted += `${'\\'.repeat(backslashes * 2)}""`
       backslashes = 0
       continue
     }
     if (escapePercent && char === '%') {
-      // `%VAR%` expands even inside a quoted token, so the pair has to be
-      // broken: close the quote, escape the percent, reopen. The backslash run
-      // must be DOUBLED first -- a quote straight after a single backslash is
-      // an escaped quote to CommandLineToArgvW, which silently corrupts every
-      // `C:\Users\%USERNAME%\...` path.
+      // Break %VAR% expansion outside quotes; double backslashes before the inserted closing quote.
       quoted += `${'\\'.repeat(backslashes * 2)}"^%"`
       backslashes = 0
       continue
@@ -61,32 +26,21 @@ function quoteWindows(value: string, escapePercent: boolean): string {
     quoted += `${'\\'.repeat(backslashes)}${char}`
     backslashes = 0
   }
-  // Trailing backslashes precede the closing quote, so they need doubling too —
-  // otherwise `C:\dir\` ends the argument with an escaped quote and swallows it.
+  // Double trailing backslashes so the closing quote cannot become escaped.
   return `${quoted}${'\\'.repeat(backslashes * 2)}"`
 }
 
-/** Quote one argument for `CommandLineToArgvW`. */
+/** Uses CommandLineToArgvW quoting. */
 export function quoteWindowsArgument(value: string): string {
   return quoteWindows(value, false)
 }
 
-/**
- * Quote one argument that has to survive a cmd hop as well.
- *
- * Separate from `quoteWindowsArgument` rather than a boolean parameter: the
- * two differ only in whether `%` is neutralised, and a flag there is easy to
- * pass by accident — `values.map(quoteWindowsArgument)` hands `map`'s index in
- * as the flag, which is exactly how this was first written.
- */
+// A separate function prevents Array.map indices from accidentally enabling percent escaping.
 export function quoteWindowsCmdArgument(value: string): string {
   return quoteWindows(value, true)
 }
 
-/**
- * Decode a command line the way `CommandLineToArgvW` does, e.g. one read back from
- * the process table. `""` inside a quoted run yields a literal quote.
- */
+/** Parses CommandLineToArgvW rules, including doubled quotes inside quoted runs. */
 export function parseWindowsCommandLine(line: string): string[] {
   const argv: string[] = []
   let current = ''
@@ -143,8 +97,7 @@ export function parseWindowsCommandLine(line: string): string[] {
   return argv
 }
 
-/** Thrown before spawn when a `.cmd`/`.bat` target would receive a line break. Typed so
- * callers can tell the user the batch file is the limit, not the agent. */
+/** Distinguishes cmd.exe line-break limits from agent failures. */
 export class WindowsCmdLineBreakError extends Error {
   constructor() {
     super('cmd.exe cannot receive an argument containing a line break')
@@ -161,32 +114,17 @@ export function validateWindowsCmdArguments(values: readonly string[]): void {
   }
 }
 
-/**
- * Build the argv Node should spawn to run `program` with `args` through
- * `cmd.exe`, for targets cmd must interpret (`.cmd`, `.bat`).
- *
- * Callers must pass the result with `windowsVerbatimArguments: true` so Node
- * hands the line through untouched, and must NOT set `shell: true` — that
- * concatenates arguments without escaping (Node warns DEP0190) and silently
- * disables `windowsHide`.
- *
- * `/d` skips AutoRun commands from the registry, which would otherwise run
- * arbitrary user configuration before our command. `/v:off` pins delayed
- * expansion off so a `!` in an argument stays literal even where the Command
- * Processor registry default turns it on. `/s` makes cmd strip exactly the
- * outer quote pair and treat the rest verbatim.
- */
+/** Requires windowsVerbatimArguments:true and shell:false to preserve quoting and windowsHide. */
 export function buildWindowsCmdShimCommandLine(program: string, args: readonly string[]): string {
   validateWindowsCmdArguments([program, ...args])
-  // The program path needs the same treatment as the arguments: it is just as
-  // likely to contain `%USERNAME%`, and cmd expands it just the same.
+  // Program paths can contain percent variables too.
   const inner = [program, ...args].map(quoteWindowsCmdArgument).join(' ')
+  // Disable AutoRun and delayed expansion; /s strips only the outer quote pair.
   return `/d /v:off /s /c "${inner}"`
 }
 
 const CMD_INTERPRETED_EXTENSIONS = ['.cmd', '.bat']
 
-/** Whether `program` is a target Windows can only start through `cmd.exe`. */
 export function isCmdInterpretedProgram(program: string): boolean {
   const lower = program.toLowerCase()
   return CMD_INTERPRETED_EXTENSIONS.some((extension) => lower.endsWith(extension))

src/packages/process-host/src/windows-system-binary.ts

@@ -1,20 +1,7 @@
-// Why win32 and not the host `join`: these are Windows paths and are only ever spawned on Windows,
-// but they are also built off-platform (tests, and any code that plans a Windows command from a
-// POSIX host), where the host separator produces the mixed `C:\Windows/System32/whoami.exe`.
+// Use Windows separators even when planning commands or testing from a POSIX host.
 import { win32 as pathWin32 } from 'node:path'
 
-/**
- * Absolute paths for the Windows system binaries Orca shells out to.
- *
- * Why not a bare name: a bare `powershell.exe` is resolved against the child's
- * PATH, and Orca's PATH under Electron is not the user's. Where a policy has
- * pruned or shadowed the System32 entry the spawn simply fails, and the caller
- * reports the wrong thing — the font picker silently fell back to five
- * hardcoded families rather than saying it could not enumerate (#11771).
- *
- * `SystemRoot` is set on every Windows session; the literal is only a last
- * resort for a stripped environment.
- */
+// Absolute system paths avoid stripped or shadowed Electron PATH entries.
 function systemRoot(env: NodeJS.ProcessEnv = process.env): string {
   return env.SystemRoot ?? env.SYSTEMROOT ?? env.windir ?? 'C:\\Windows'
 }

src/packages/process-host/src/windows-tree-kill.test.ts

@@ -82,7 +82,6 @@ describe('windows tree kill tracker', () => {
     launched[0].emit('close', 0)
     await expect(secondWait).resolves.toBe(true)
     expect(inFlightWindowsTreeKill(child)).toBeUndefined()
-    // A proven kill stays proven: no second taskkill against a pid Windows may reissue.
     expect(startWindowsTreeKill(child, 1234, vi.fn())).toBe(first)
     expect(launch).toHaveBeenCalledOnce()
   })
@@ -133,7 +132,6 @@ describe('windows tree kill tracker', () => {
     await expect(kill.settled).resolves.toBe('unverifiable')
     expect(launched[0].kill).toHaveBeenCalledOnce()
     expect(killRoot).toHaveBeenCalledOnce()
-    // A late success after abandonment does not rewrite the verdict.
     launched[0].emit('close', 0)
     expect(kill.verdict()).toBe('unverifiable')
   })

src/packages/process-host/src/windows-tree-kill.ts

@@ -2,30 +2,14 @@ import { spawn as nodeSpawn, type ChildProcess } from 'node:child_process'
 import { constants, getPriority, setPriority } from 'node:os'
 import { windowsSystem32Binary } from './windows-system-binary'
 
-/**
- * `live` while taskkill is still walking the tree, `exited` once taskkill
- * reported the whole tree terminated, `unverifiable` when it failed or ran out
- * of time. Nothing short of a successful taskkill is reported as `exited`.
- */
+/** Only a successful taskkill proves the entire tree exited. */
 export type WindowsTreeKillVerdict = 'live' | 'unverifiable' | 'exited'
 export type SettledWindowsTreeKillVerdict = Exclude<WindowsTreeKillVerdict, 'live'>
 
-/** How long one caller waits before treating the tree as unverifiable. */
 export const WINDOWS_TASKKILL_WAIT_MS = 5_000
-/**
- * How long the tracked taskkill may keep walking after its caller stops waiting.
- * Why past the wait: loaded windows-2022 runners measured 4.6-18 s, and killing
- * the root before taskkill reads it orphans the descendants it was about to
- * find. Bounded because a live root keeps its git admission slot until it exits.
- */
+// Outlive the caller wait: killing the root before taskkill reads it can orphan descendants.
 export const WINDOWS_TASKKILL_DEADLINE_MS = 30_000
-/**
- * How long taskkill may keep running once the root has exited.
- * Why short: Node then releases the root's handle, so Windows may reissue the
- * pid (#10680) to a process a late-starting taskkill would walk instead. Within
- * this window a taskkill that itself killed the root still reports success;
- * past it the kill is abandoned as unverifiable. No longer than the old cap.
- */
+// Bound taskkill after root exit: Windows may reuse the PID once Node releases its handle.
 export const WINDOWS_TASKKILL_AFTER_ROOT_EXIT_MS = 2_000
 
 export type WindowsTreeKill = {
@@ -47,13 +31,11 @@ let launchTaskkill: TaskkillLauncher = launchSystemTaskkill
 // Keyed by handle, not pid: a reused pid must never join another tree's kill.
 const treeKills = new WeakMap<ChildProcess, WindowsTreeKill>()
 
-/** The tree kill already in flight or proven for this child, if any. */
 export function trackedWindowsTreeKill(child: ChildProcess): WindowsTreeKill | undefined {
   const kill = treeKills.get(child)
   return kill && kill.verdict() !== 'unverifiable' ? kill : undefined
 }
 
-/** Settles when this child's in-flight tree kill does; undefined when none is running. */
 export function inFlightWindowsTreeKill(
   child: ChildProcess
 ): Promise<SettledWindowsTreeKillVerdict> | undefined {
@@ -61,14 +43,7 @@ export function inFlightWindowsTreeKill(
   return kill?.verdict() === 'live' ? kill.settled : undefined
 }
 
-/**
- * Start one tracked `taskkill /t /f` for this child's tree.
- *
- * The taskkill outlives every caller's wait and is abandoned only at the
- * deadline or shortly after the root exits; the root is killed by handle only
- * after taskkill has failed or been abandoned, never while it may still be
- * reading the tree.
- */
+/** Delay the root fallback until taskkill fails or its deadline expires. */
 export function startWindowsTreeKill(
   child: ChildProcess,
   rootPid: number,
@@ -140,7 +115,7 @@ export function startWindowsTreeKill(
   return kill
 }
 
-/** True when the tree is proven dead within this caller's wait; the kill keeps running either way. */
+/** The tracked kill continues even if this caller stops waiting. */
 export function waitForWindowsTreeKill(
   kill: WindowsTreeKill,
   waitMs = WINDOWS_TASKKILL_WAIT_MS
@@ -162,7 +137,7 @@ function childProcessExited(child: ChildProcess): boolean {
   return (child.exitCode ?? null) !== null || (child.signalCode ?? null) !== null
 }
 
-/** Test seam: swap the taskkill launch, e.g. for one that starts late like a loaded host's. */
+/** Simulates taskkill delays seen on loaded Windows hosts. */
 export function _setWindowsTaskkillLauncherForTests(launcher: TaskkillLauncher | null): void {
   launchTaskkill = launcher ?? launchSystemTaskkill
 }