fix(windows): fail closed when the top-level process query fails #1925
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -348,8 +348,34 @@ function listDarwinSnapshots(uid: number | undefined): ProcessSnapshot[] { | |
| * Exported for the Windows integration regression that exercises the real | ||
| * PowerShell enumeration. | ||
| */ | ||
| /** | ||
| * 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", | ||
| // -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 = 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); | ||
|
Comment on lines
+418
to
+425
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline src/codex/app-server-processes.ts
printf '%s\n' '--- relevant implementation ---'
sed -n '330,445p' src/codex/app-server-processes.ts
printf '%s\n' '--- related symbols and call sites ---'
rg -n -C 4 'runPowerShell|parseWindowsSnapshotOutput|collectCodexAppServerCatalogState|resolveTrustedWindowsPowerShellExe|__OCX_ENUM_INCOMPLETE__|catalogStateTtlMs' src
printf '%s\n' '--- repository metadata and test/workflow references ---'
rg -n -C 3 'real.?Windows|Windows|PowerShell|app-server-processes' .github test tests src 2>/dev/null || trueRepository: lidge-jun/opencodex Length of output: 50376 🏁 Script executed: #!/bin/bash
set -eu
file=src/codex/app-server-processes.ts
printf '%s\n' '--- file size ---'
wc -l "$file"
printf '%s\n' '--- target implementation ---'
sed -n '330,435p' "$file"
printf '%s\n' '--- exact symbol matches in target file ---'
rg -n -C 5 'runPowerShell|parseWindowsSnapshotOutput|collectCodexAppServerCatalogState|resolveTrustedWindowsPowerShellExe|__OCX_ENUM_INCOMPLETE__|catalogStateTtlMs' "$file"
printf '%s\n' '--- focused tests and workflows ---'
rg -l 'parseWindowsSnapshotOutput|collectCodexAppServerCatalogState|__OCX_ENUM_INCOMPLETE__|runPowerShell|PowerShell' tests .github 2>/dev/null | head -80Repository: lidge-jun/opencodex Length of output: 13719 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- catalog-state call path ---'
sed -n '640,725p' src/codex/app-server-processes.ts
printf '%s\n' '--- existing process-enumeration tests ---'
sed -n '1,280p' tests/codex-app-server-processes.test.ts
printf '%s\n' '--- package/runtime configuration ---'
sed -n '1,180p' package.json
rg -n -C 4 'bun|runtime|Windows integration|integration regression|real Windows|windows.*workflow|runs-on:.*windows' package.json bunfig.toml .github README.md tests src 2>/dev/null | head -240
printf '%s\n' '--- all listWindowsSnapshots references ---'
rg -n -C 5 'listWindowsSnapshots|parseWindowsSnapshotOutput' src testsRepository: lidge-jun/opencodex Length of output: 46092 🌐 Web query:
💡 Result: In Node.js, Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- focused regression tests ---'
sed -n '450,575p' tests/codex-app-server-processes.test.ts
printf '%s\n' '--- current diff summary ---'
git diff --stat
printf '%s\n' '--- current diff for target and tests ---'
git diff -- src/codex/app-server-processes.ts tests/codex-app-server-processes.test.ts | sed -n '1,260p'
printf '%s\n' '--- local runtime availability (no repository code executed) ---'
command -v node || true
node --version 2>/dev/null || true
command -v bun || true
bun --version 2>/dev/null || true
printf '%s\n' '--- execFileSync failure semantics probe ---'
node - <<'JS'
const { execFileSync } = require("node:child_process");
try {
execFileSync(process.execPath, ["-e", "process.stderr.write('diagnostic\\n'); process.exit(7)"], {
encoding: "utf8", stdio: ["ignore", "pipe", "ignore"],
});
console.log("returned");
} catch (e) {
console.log(JSON.stringify({
threw: true,
status: e.status,
stdout: String(e.stdout ?? ""),
stderr: String(e.stderr ?? ""),
}));
}
JSRepository: lidge-jun/opencodex Length of output: 5910 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- devlog references for the claimed failure ---'
rg -n -i -C 5 'parse.?time|parser failure|empty stdout|PowerShell.*diagnostic|diagnostic.*PowerShell|OCX_ENUM_INCOMPLETE' devlog src tests 2>/dev/null | head -320
printf '%s\n' '--- all process-enumerator error handling ---'
sed -n '1,120p' src/codex/app-server-processes.ts
sed -n '260,335p' src/codex/app-server-processes.ts
printf '%s\n' '--- exact tracked files mentioning this feature ---'
git ls-files | rg 'devlog|codex-app-server-processes|workflow|windows'Repository: lidge-jun/opencodex Length of output: 50378 🏁 Script executed: #!/bin/bash
set -eu
file=devlog/_plan/260817_wave5_execution/030_1876_windows_discovery.md
printf '%s\n' '--- focused plan ---'
cat -n "$file"
printf '%s\n' '--- workflow definitions and Windows runner references ---'
rg -n -C 5 'runs-on:.*windows|windows-latest|windows-2022|codex-app-server-processes' .github/workflows .github/scripts tests 2>/dev/null | head -260
printf '%s\n' '--- narrow devlog hits for parser/exit behavior ---'
rg -n -i -C 3 'parse.?error|parse.?time|exit.?code|stderr|empty.*(stdout|output)|stdout.*empty' devlog/_plan/260817_wave5_execution devlog/_fin/260817* 2>/dev/null | head -260Repository: lidge-jun/opencodex Length of output: 29063 🌐 Web query:
💡 Result: When a PowerShell script contains a syntax error (a parse error), it fails before execution begins, which often results in the PowerShell process exiting with a code of 0 and no content in stdout or stderr [1][2][3]. This occurs because parse errors prevent the script from being compiled into an executable form, meaning no part of the script—including error handling blocks like try/catch or trap—is ever invoked [1][3][4]. Key points regarding this behavior: 1. Why it happens: A parse error prevents the PowerShell engine from completing the parsing phase of the script [5]. Since the script never enters the execution phase, it cannot trigger any runtime error handling mechanisms or set specific exit codes associated with execution failures [1][4]. 2. Exit Codes: Because the process exits before execution, it does not encounter runtime conditions that would generate non-zero exit codes [3]. The process simply terminates after failing to compile the script, typically leaving the exit code at 0 [1][2]. 3. Lack of Output: The parse error is an internal engine failure occurring during the parsing phase. Depending on the environment and how PowerShell is invoked, this diagnostic information may be written to the internal error stream ($Error) rather than the standard stderr output or stdout [6]. Workarounds and Strategies: Because you cannot catch syntax errors within the failing script itself, you must validate the code before or outside of the primary execution flow [4]: - External Validation (Linter): Use a tool like PSScriptAnalyzer to check for syntax errors before running the script [4]. - Helper/Invoker Script: Wrap the execution of your target script in a "parent" or "invoker" script that validates the syntax or handles the process invocation [2][4]. - CI/CD Adjustments: In automated environments (like TeamCity or Azure DevOps), if the script runner itself does not capture these parse errors, you may need to configure the task to specifically watch the internal error streams or check for specific file content patterns that indicate completion [7][8]. You can check the automatic variable $Error in an interactive PowerShell session to view the details of the parse error that occurred, even if it did not appear in the stderr stream during automated execution [6]. Top results: [1][2][5][3][4] Citations:
🌐 Web query:
💡 Result: A parse error resulting in an exit code of 0 is typically indicative of an implementation detail or a bug in the environment where the code is being executed, rather than intended behavior [1]. In a standard command-line interface or script execution, a syntax or parse error should result in a non-zero exit status to signal failure [1]. When you encounter this specific behavior, consider the following common causes: 1. Environment-Specific Design: Certain environments, particularly those designed for interactive or REPL-like (Read-Eval-Print Loop) use, may not update the global exit status variable if a script fails during the initial parsing phase [1]. For example, in PowerShell, when running commands via stdin, a syntax error may fail to set the success status ($?), leaving the previous command's status (often 0) intact, which is then used as the final exit code [1]. 2. Wrapper Scripts or Redirects: If you are running a script through a wrapper, a pipeline, or a subshell, the process might be capturing the output or status of a successful parent process rather than the failing script itself [1]. 3. Delayed Execution or Deferred Parsing: In some dynamic languages, code might be parsed in stages. If a parse error occurs in a block that is never reached or is not evaluated by the main execution loop, the process might terminate naturally with an exit code of 0. To troubleshoot this: - Verify the environment's documentation regarding exit code behavior during initialization or parsing. - Run the script or command directly in the shell without pipes or wrappers to see if the exit code changes. - Check if the error is occurring at runtime versus parse-time [1]. If the behavior persists in a critical production system, it is often treated as a bug or a consistency issue within that specific CLI or interpreter version [1]. Citations: 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- CI Windows job details ---'
sed -n '520,590p' .github/workflows/ci.yml
printf '%s\n' '--- line-numbered target and test locations ---'
nl -ba src/codex/app-server-processes.ts | sed -n '378,428p'
nl -ba tests/codex-app-server-processes.test.ts | sed -n '480,565p'Repository: lidge-jun/opencodex Length of output: 3908 Treat PowerShell parse failures as incomplete enumeration. At 🤖 Prompt for AI AgentsSource: Path instructions |
||
| } | ||
|
|
||
| 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 < catalogStateTtlMs(catalogStateCache.status.state)) { | ||
| return catalogStateCache.status; | ||
| } | ||
| const compute = (): CodexAppServerCatalogStatus => { | ||
|
|
@@ -630,17 +665,16 @@ export function collectCodexAppServerCatalogState( | |
| }); | ||
| let snapshots: ProcessSnapshot[]; | ||
| let enumerationFailed = false; | ||
| 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<number>(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line before
## Outcome (executed).markdownlint reports MD022 because the heading is not surrounded by blank lines. Insert one blank line before Line 78.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Source: Linters/SAST tools