From dc1df7d44da93d5deb9e5f278e2c3d51c2f3c6e9 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 21:14:07 +0900 Subject: [PATCH 1/4] fix(windows): fail closed when the top-level process query fails The Windows enumeration runs under ErrorActionPreference SilentlyContinue, and the top-level Get-CimInstance Win32_Process sat outside the per-process try/catch. Only failures inside the ForEach-Object block emitted the __OCX_ENUM_INCOMPLETE__ sentinel; a failure of the query itself emitted nothing at all, which is byte-identical to a healthy machine running no Codex process. The staleness collector then reported not_running for a machine whose process list it had never actually read, and positive disk-derived v2 guidance followed from a state nobody had observed. The query now uses -ErrorAction Stop inside an outer catch that emits the same sentinel, so an unreadable process list reaches the collector as unknown. Two things surfaced while testing it. The parse loop is now parseWindowsSnapshotOutput and listWindowsSnapshots takes an optional runner, because the failure contract could not be exercised off-Windows at all - the existing coverage drives a throwing enumerator by swapping platform, which is a different path from a query that returns cleanly empty. The second is the more interesting one: collectCodexAppServerCatalogState wrapped only the default enumerator in its try, so an injected io.listSnapshots that threw would propagate instead of degrading to unknown. Every caller today passes a non-throwing double, so nothing was broken in practice - but the fail-closed contract belonged to the enumeration, not to one branch of it, and the regression test would have been asserting the safety of a path the seam does not share. Both paths now go through the same catch. Ablation: removing -ErrorAction Stop fails the new test. --- src/codex/app-server-processes.ts | 89 +++++++++++++++--------- tests/codex-app-server-processes.test.ts | 39 +++++++++++ 2 files changed, 94 insertions(+), 34 deletions(-) diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index 60c778e6b3..bb76780561 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[] { @@ -630,17 +652,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..8f4083f2ac 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -12,6 +12,7 @@ import { isWindowsCodexCandidateCommandLine, listCodexAppServerProcesses, listWindowsSnapshots, + parseWindowsSnapshotOutput, resetCodexAppServerCatalogStateCache, restartCodexAppServers, STALE_CODEX_APP_SERVER_HINT, @@ -461,6 +462,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", () => { From 497b6433803f10754ef3544ae024d13bda4422d8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 21:39:13 +0900 Subject: [PATCH 2/4] test(windows): pin every field the snapshot parser returns Extracting the parse loop dropped the owner field on the first pass and nothing failed. The two states these tests assert - unknown versus not_running - do not read it, and the ownership decisions that do read it live in other modules with their own doubles, so the loss would have travelled to Windows unnoticed. Asserting the whole row rather than a state means the next refactor of this loop cannot quietly lose a field. The fixture also covers what the loop is supposed to reject: blank lines, pid <= 1, and a row whose owner column is empty. --- tests/codex-app-server-processes.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 8f4083f2ac..749f3c426f 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -84,6 +84,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". From 535e3c256bba2729982d17cb67837972766fd0c8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 21:46:01 +0900 Subject: [PATCH 3/4] fix(windows): stop serving an unreadable process list for a full 5s The catalog-state memo used one TTL for every state, so a transient enumeration failure was cached exactly as long as a successful reading. That is the wrong trade for unknown: it is a failure to observe rather than an observation, and holding it for the full window suppresses guidance for every call in that window while the retry that would have succeeded never runs. unknown now gets 250ms. Long enough to still collapse a burst of per-turn calls into one probe, which is what the cache is for, short enough that a blip does not decide the next five seconds. The test asserts the policy rather than the gate. The memo only engages on a fully-defaulted call, so injecting a clock makes the call non-default and bypasses the cache entirely - there is no seam to drive time through, and pretending otherwise would be a test that watches itself. Extracting the policy into a named function is what makes that half checkable at all, and the comment says plainly which half is not. --- src/codex/app-server-processes.ts | 15 ++++++++++++++- tests/codex-app-server-processes.test.ts | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index bb76780561..c70c3af5c5 100644 --- a/src/codex/app-server-processes.ts +++ b/src/codex/app-server-processes.ts @@ -614,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 @@ -638,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 => { diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 749f3c426f..4a63be2f2e 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -6,6 +6,7 @@ import { setTrustedWindowsElevationExecutablesForTests } from "../src/lib/window import { afterCatalogWriteHandleAppServers, attachStaleAppServerHint, + catalogStateTtlMs, collectCodexAppServerCatalogState, formatStaleCodexAppServerWarning, isCodexAppServerCommandLine, @@ -659,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 From ed0d5af35fb4cf250fb97806d986add772ad96d1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 21:47:01 +0900 Subject: [PATCH 4/4] docs(devlog): record the WP3 outcome, including two self-inflicted defects Both were found by auditing rather than by tests, which is the part worth keeping: the extraction dropped ProcessSnapshot.owner and everything stayed green, and the collector's fail-closed catch covered only the default enumerator, so the regression test for this work-phase would have been asserting the safety of a path the injected seam does not share. Also records the gap this cannot close on macOS. The tests drive an injected PowerShell runner, so no PowerShell ever parses the emitted script - and a syntax error there is not catchable by try/catch in the same scriptblock, which would reintroduce the exact fail-open the change exists to fix. --- .../030_1876_windows_discovery.md | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) 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.