diff --git a/devlog/_plan/260817_wave5_execution/030_1876_windows_discovery.md b/devlog/_plan/260817_wave5_execution/030_1876_windows_discovery.md index 5e2a2b69ae..c7764d9894 100644 --- a/devlog/_plan/260817_wave5_execution/030_1876_windows_discovery.md +++ b/devlog/_plan/260817_wave5_execution/030_1876_windows_discovery.md @@ -75,3 +75,38 @@ than implying platform coverage. #1876 merges after the top-level fix; #1852 closes citing the merge SHA plus the top-level-failure regression test. +## Outcome (executed) + +DONE with one open evidence gap. Three commits: + +| Commit | Change | +|--------|--------| +| `dc1df7d44` | `-ErrorAction Stop` + outer catch on the top-level query; parse loop extracted to `parseWindowsSnapshotOutput`; `listWindowsSnapshots` takes an optional runner; the collector's fail-closed catch now covers the injected seam too | +| `497b64338` | full-row fixture pinning every parsed field | +| `535e3c256` | `unknown` cached for 250ms instead of the uniform 5s (accept criterion 3) | + +**Two defects found in my own work, both by auditing rather than by tests.** + +The extraction silently dropped `ProcessSnapshot.owner` and every test still +passed — the two states these tests assert never read it, and the ownership +decisions that do live in other modules with their own doubles. It was caught by +diffing against `4d9738f43`, and the full-row fixture exists so the next refactor +cannot repeat it. + +`collectCodexAppServerCatalogState` wrapped only the *default* enumerator in its +try, so an injected `listSnapshots` that threw would propagate instead of +degrading to `unknown`. No caller was broken in practice, but the regression test +for this work-phase would have been asserting the safety of a path the seam does +not share. Both paths now go through one catch — the shape +`src/codex/log-guard/processes.ts` already had. + +**Open gap, recorded rather than implied.** There is no real-Windows evidence. +`platform-windows` is `workflow_dispatch`-only and the aggregate accepts it as +skipped. The reviewer's sharpest point stands: the tests drive an injected +`runPowerShell`, so no PowerShell ever parses the emitted script, and a *syntax* +error is not catchable by `try/catch` in the same scriptblock — it fails at parse +time, writes to stderr (which is `stdio: "ignore"`), and leaves stdout empty, +reintroducing precisely the fail-open this fixes. A second, milder risk: +`-ErrorAction Stop` promotes non-terminating CIM errors to terminating, so a benign +per-instance error could turn a mostly-complete read into a persistent `unknown`. +Both need a maintainer-triggered dispatch on the merged head. diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index 60c778e6b3..c70c3af5c5 100644 --- a/src/codex/app-server-processes.ts +++ b/src/codex/app-server-processes.ts @@ -348,8 +348,34 @@ function listDarwinSnapshots(uid: number | undefined): ProcessSnapshot[] { * Exported for the Windows integration regression that exercises the real * PowerShell enumeration. */ -export function listWindowsSnapshots(): ProcessSnapshot[] { +/** + * Turn one PowerShell enumeration's stdout into snapshots. + * + * Split out from the spawn so the failure contract is testable off-Windows: the + * sentinel path is the difference between "no Codex process is running" and "we could + * not read the process list", and only one of those is safe to act on. + */ +export function parseWindowsSnapshotOutput(output: string): ProcessSnapshot[] { const out: ProcessSnapshot[] = []; + for (const line of output.split(/\r?\n/)) { + // A candidate whose owner could not be verified — or a top-level query that + // failed outright — makes the whole enumeration incomplete. The staleness + // collector must not read the partial result as "nothing running". + if (line.trim() === "__OCX_ENUM_INCOMPLETE__") throw new Error("windows_enum_incomplete"); + const tab = line.indexOf("\t"); + if (tab <= 0) continue; + const tab2 = line.indexOf("\t", tab + 1); + if (tab2 <= tab) continue; + const pid = Number(line.slice(0, tab)); + const commandLine = line.slice(tab + 1, tab2).trim(); + const owner = line.slice(tab2 + 1).trim(); + if (!Number.isSafeInteger(pid) || pid <= 1 || !commandLine || !owner) continue; + out.push({ pid, commandLine, owner }); + } + return out; +} + +export function listWindowsSnapshots(runPowerShell?: (psCommand: string) => string): ProcessSnapshot[] { // Newlines keep -Command as a real script (space-joined statements need ';'). // Double-quoted format string so `t expands to a real tab. // Codex candidates only: basename token codex / codex.exe / codex.cmd / @@ -361,7 +387,15 @@ export function listWindowsSnapshots(): ProcessSnapshot[] { const psCommand = [ "$ErrorActionPreference='SilentlyContinue'", "$me=[System.Security.Principal.WindowsIdentity]::GetCurrent().Name", - "Get-CimInstance Win32_Process | Where-Object {", + // -ErrorAction Stop plus the outer try is what makes a TOP-LEVEL query failure + // observable. Under SilentlyContinue alone, a failing Get-CimInstance emits nothing + // and the enumeration is indistinguishable from "no Codex process is running" — + // the parse loop finds no rows, no sentinel is produced, and the staleness collector + // reports not_running for a machine whose process list it never actually read. + // The per-process catch below cannot cover this: it only runs once the pipeline has + // objects to iterate. + "try {", + "Get-CimInstance Win32_Process -ErrorAction Stop | Where-Object {", " -not [string]::IsNullOrWhiteSpace($_.CommandLine) -and (", ` $_.CommandLine -match ${basenameMatch} -or`, ` $_.CommandLine -match ${codeModeMatch}`, @@ -376,31 +410,19 @@ export function listWindowsSnapshots(): ProcessSnapshot[] { " \"{0}`t{1}`t{2}\" -f $_.ProcessId, $cmd, $owner", " } catch { \"__OCX_ENUM_INCOMPLETE__\" }", "}", + "} catch { \"__OCX_ENUM_INCOMPLETE__\" }", ].join("\n"); // Top-level exec failure propagates (see listDarwinSnapshots note). The // executable resolves from the trusted System32 directory (never PATH), and // windowsHide keeps the enumeration console-less on desktop sessions (#1278). - const output = execFileSync(resolveTrustedWindowsPowerShellExe(), [ - "-NoProfile", "-NoLogo", "-NonInteractive", - "-Command", - psCommand, - ], { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 8_000, windowsHide: true }); - for (const line of output.split(/\r?\n/)) { - // A candidate whose owner could not be verified makes the whole - // enumeration incomplete — the staleness collector must not read the - // partial result as "nothing running". - if (line.trim() === "__OCX_ENUM_INCOMPLETE__") throw new Error("windows_enum_incomplete"); - const tab = line.indexOf("\t"); - if (tab <= 0) continue; - const tab2 = line.indexOf("\t", tab + 1); - if (tab2 <= tab) continue; - const pid = Number(line.slice(0, tab)); - const commandLine = line.slice(tab + 1, tab2).trim(); - const owner = line.slice(tab2 + 1).trim(); - if (!Number.isSafeInteger(pid) || pid <= 1 || !commandLine || !owner) continue; - out.push({ pid, commandLine, owner }); - } - return out; + const output = runPowerShell + ? runPowerShell(psCommand) + : execFileSync(resolveTrustedWindowsPowerShellExe(), [ + "-NoProfile", "-NoLogo", "-NonInteractive", + "-Command", + psCommand, + ], { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 8_000, windowsHide: true }); + return parseWindowsSnapshotOutput(output); } function defaultListSnapshots(platform: NodeJS.Platform, getuid: () => number | undefined): ProcessSnapshot[] { @@ -592,6 +614,18 @@ function defaultCatalogMtimeMs(): number | null { // guidance calls (#857). let catalogStateCache: { atMs: number; status: CodexAppServerCatalogStatus } | null = null; const CATALOG_STATE_TTL_MS = 5_000; +/** + * `unknown` is a failure to observe, not an observation, so it gets a much shorter + * window than a real reading. At the full 5s a single transient enumeration failure + * suppresses guidance for every call in that window, and the retry that would have + * succeeded never runs. Keeping a brief window still collapses a burst of per-turn + * calls into one probe, which is what the cache is for. + */ +const CATALOG_STATE_UNKNOWN_TTL_MS = 250; + +export function catalogStateTtlMs(state: CodexAppServerCatalogState): number { + return state === "unknown" ? CATALOG_STATE_UNKNOWN_TTL_MS : CATALOG_STATE_TTL_MS; +} /** * Compare the on-disk catalog mtime against the start time of running Codex @@ -616,7 +650,8 @@ export function collectCodexAppServerCatalogState( const fullyDefault = !io.listSnapshots && !io.readStartMs && !io.catalogMtimeMs && !io.platform && !io.getuid && !io.now; if (fullyDefault - && catalogStateCache && now - catalogStateCache.atMs < CATALOG_STATE_TTL_MS) { + && catalogStateCache + && now - catalogStateCache.atMs < catalogStateTtlMs(catalogStateCache.status.state)) { return catalogStateCache.status; } const compute = (): CodexAppServerCatalogStatus => { @@ -630,17 +665,16 @@ export function collectCodexAppServerCatalogState( }); let snapshots: ProcessSnapshot[]; let enumerationFailed = false; - if (io.listSnapshots) { - snapshots = io.listSnapshots(); - } else { - try { - snapshots = defaultListSnapshots(platform, getuid); - } catch { - // Enumeration failure must never read as "nothing running" — that - // would let positive model guidance through on guesswork (#857). - snapshots = []; - enumerationFailed = true; - } + const enumerate = io.listSnapshots ?? (() => defaultListSnapshots(platform, getuid)); + try { + snapshots = enumerate(); + } catch { + // Enumeration failure must never read as "nothing running" — that would let + // positive model guidance through on guesswork (#857). The injected seam gets + // the same contract as the default path: whoever enumerates, a failure to read + // the process list is unknown, not an empty machine. + snapshots = []; + enumerationFailed = true; } const processes: CodexAppServerProcess[] = []; const seen = new Set(); diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 7d89663ec3..4a63be2f2e 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -6,12 +6,14 @@ import { setTrustedWindowsElevationExecutablesForTests } from "../src/lib/window import { afterCatalogWriteHandleAppServers, attachStaleAppServerHint, + catalogStateTtlMs, collectCodexAppServerCatalogState, formatStaleCodexAppServerWarning, isCodexAppServerCommandLine, isWindowsCodexCandidateCommandLine, listCodexAppServerProcesses, listWindowsSnapshots, + parseWindowsSnapshotOutput, resetCodexAppServerCatalogStateCache, restartCodexAppServers, STALE_CODEX_APP_SERVER_HINT, @@ -83,6 +85,22 @@ describe("collectCodexAppServerCatalogState (#857)", () => { expect(status.state).toBe("not_running"); }); + // The extraction that made the sentinel testable also silently dropped `owner` on + // its first pass, and nothing failed — the field feeds ownership decisions elsewhere, + // not the two states these tests assert. Pin the whole parsed row so a refactor of the + // parse loop cannot quietly lose a field again. + test("parsed rows keep every field the enumeration reports", () => { + const rows = parseWindowsSnapshotOutput([ + "4321\tC:\\Program Files\\codex\\codex.exe app-server\tCONTOSO\\jun", + "", + "1\tinit\tCONTOSO\\jun", + "9999\tcodex app-server\t", + ].join("\r\n")); + expect(rows).toEqual([ + { pid: 4321, commandLine: "C:\\Program Files\\codex\\codex.exe app-server", owner: "CONTOSO\\jun" }, + ]); + }); + test("enumeration failure reports unknown, never not_running", () => { // On macOS the win32 enumeration path has no powershell.exe → it throws, // which must surface as unknown rather than "nothing is running". @@ -461,6 +479,44 @@ describe("Windows Win32_Process owner enumeration (#476)", () => { expect(WINDOWS_CODEX_BASENAME_CANDIDATE_RE.source).toContain("['\"]?"); }); + // The top-level Get-CimInstance sits under `$ErrorActionPreference='SilentlyContinue'`. + // If it fails without `-ErrorAction Stop` and an outer catch, it emits nothing at all — + // which is byte-identical to a healthy machine running no Codex process. The existing + // coverage drives a *throwing* enumerator (by swapping `platform` so the real one fails + // on a missing binary); the path below is the one that returns cleanly empty, and it is + // the one that used to launder "we could not look" into "nothing is running". + test("a top-level CIM failure emits the sentinel, so an empty read is never not_running", () => { + const psCommand = { value: "" }; + expect(() => listWindowsSnapshots((command) => { + psCommand.value = command; + // What PowerShell actually prints when the outer catch fires. + return "__OCX_ENUM_INCOMPLETE__\n"; + })).toThrow("windows_enum_incomplete"); + + // The guard has to be on the top-level query itself, not only per-process. + expect(psCommand.value).toContain("Get-CimInstance Win32_Process -ErrorAction Stop"); + expect(psCommand.value).toContain("} catch { \"__OCX_ENUM_INCOMPLETE__\" }"); + + // And the collector must turn that throw into unknown, never not_running. + const status = collectCodexAppServerCatalogState({ + listSnapshots: () => listWindowsSnapshots(() => "__OCX_ENUM_INCOMPLETE__\n"), + catalogMtimeMs: () => 1_000, + }); + expect(status.state).toBe("unknown"); + }); + + test("a clean empty read still means not_running", () => { + // The other half of the contract: no sentinel, no rows, nothing wrong — the + // sentinel must not make every quiet machine look unreadable. + expect(listWindowsSnapshots(() => "")).toEqual([]); + expect(parseWindowsSnapshotOutput("")).toEqual([]); + const status = collectCodexAppServerCatalogState({ + listSnapshots: () => listWindowsSnapshots(() => ""), + catalogMtimeMs: () => 1_000, + }); + expect(status.state).toBe("not_running"); + }); + test.skipIf(process.platform !== "win32")( "listWindowsSnapshots returns a current-user Codex-shaped process via real PowerShell enumeration", () => { @@ -604,6 +660,24 @@ describe("warnIfStaleCodexAppServersAfterStartupWrite (#1046)", () => { expect(collectCodexAppServerCatalogState()).not.toBe(first); }); + /* + * An `unknown` reading is a failure to observe, not an observation. Serving it for + * the full window means one transient enumeration failure suppresses guidance for + * every call in that window and the retry that would have succeeded never runs. + * + * Scope: this asserts the POLICY the cache gate consults. The gate itself only + * engages on a fully-defaulted call — injecting `now` would make the call + * non-default and bypass the memo entirely — so there is no seam to drive a clock + * through, and no test here proves the gate reads this function. That is why it is + * one function rather than an inline ternary. + */ + test("an unknown reading is cached far more briefly than a real one", () => { + expect(catalogStateTtlMs("unknown")).toBeLessThan(catalogStateTtlMs("fresh")); + expect(catalogStateTtlMs("unknown")).toBeLessThan(catalogStateTtlMs("not_running")); + expect(catalogStateTtlMs("fresh")).toBe(catalogStateTtlMs("stale")); + expect(catalogStateTtlMs("unknown")).toBeGreaterThan(0); + }); + /* * The assertion that would catch a future refactor pointing startup at * `afterCatalogWriteHandleAppServers({ restart: true })`, which SIGTERMs matching