Skip to content

fix(codex): start short-lived Codex app-servers on Windows without cmd.exe - #23830

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/fu-windows-codex-launch
Sep 30, 2026
Merged

brennanb2025 merged 3 commits into
mainfrom
brennanb2025/fu-windows-codex-launch

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

On Windows, every short background Codex process Orca starts (to index chat history, grant hook trust, read rate limits, list models) was launched through an extra cmd.exe command prompt. Orca already knows how to skip cmd.exe for npm-installed tools, but these launches wrapped Codex in cmd.exe before that skip could see it. This PR hands them Codex directly, so the skip applies.

What Changed

What broke, and who sees it. Found in the Windows validation of #22971 (Windows 11, npm-installed Codex 0.157.1). Orca's short-lived codex app-server processes launched as cmd.exe /d /s /c "…\npm\codex.cmd" …, even though Orca's process-launch chokepoint (spawnProcess) resolves npm .cmd shims to node.exe <script> precisely to keep cmd.exe out of the tree. With a large chat history this is hundreds of launches: indexing 16,000 conversations after #22971 took 323 cmd.exe → node → codex.exe starts. A burst of short-lived interpreters under one parent is one of the patterns endpoint security products score (see docs/reference/windows-edr-posture.md).

  • Affected: Windows only, npm/pnpm-installed Codex (the usual install), for these launches: the account history index passes and the state-DB pre-create from fix(codex): index a new account home before bridging history into it #22971, the default-home history indexer, the state-DB backfill recovery claimant, hook trust grants, and the rate-limit and model-catalog probes.
  • Not affected: macOS and Linux (no .cmd shims); Codex chats themselves (already spawned the bare shim); the interactive terminal; WSL and SSH Codex (launched through wsl.exe / the remote host); codex login and the rate-limit PTY fallback (out of scope, unchanged).

User-facing change. Nothing visible changes: the same probes run and return the same results. On Windows they now show up as node.exe → codex.exe with no cmd.exe parent, and a probe timeout now names the Codex path instead of C:\WINDOWS\system32\cmd.exe.

Mechanism. The five callers built their argv with getSpawnArgsForWindows, which turns a .cmd into cmd.exe /d /c <shim> … before spawnProcess runs. spawnProcess only resolves a shim it is handed, so it saw cmd.exe and spawned that. Each caller now hands the bare Codex path and its arguments to spawnProcess, the same way the long-lived chat session already does (codex-app-server-connection.ts). An unrecognised shim still falls back to cmd.exe inside spawnProcess, with its own argument encoding.

  • codex-session-index-heal.ts (buildNativeHealInvocation, used by the account index passes, the fix(codex): index a new account home before bridging history into it #22971 pre-create and the default-home indexer), codex-model-catalog-probe.ts, codex-trust-grant-host.ts: pass the CLI path instead of the pre-wrapped argv into runCodexAppServerSession, which already spawns through spawnProcess.
  • codex-state-db-backfill-recovery.ts and the rate-limit RPC probe in codex-fetcher.ts called Node's spawn directly (so they had to pre-wrap). They now spawn through spawnProcess, and both leave the node:child_process import allowlist.

Why

The chokepoint already owns this decision and is documented as the one place .cmd handling lives (docs/reference/windows-cmd-shim-resolution.md). Removing the pre-wrap makes these callers match the chat launch instead of adding a second shim resolver or a codex-specific special case. Alternatives considered: teaching getSpawnArgsForWindows to resolve shims would duplicate the chokepoint's logic in a second place; keeping cmd.exe because these arguments are fixed flags (no free text) ignores the per-launch interpreter cost on large histories.

Linked Issue

Follow-up from the Windows validation of #22971 (no separate issue).

Visual Proof

Nothing in Orca's window changes. These are Windows 11 builds from the same test run. Before (main) and after (this PR), the Codex chat opens and the model picker loads the account's model list the same way:

Before (main):
Windows, main: Codex chat open with the model picker loaded

After (this PR), model picker open:
Windows, this PR: model picker listing the account's models

The difference is in which process starts Codex. A logging shim recorded the parent of every Codex start:

Build Short-lived Codex starts (history index, model list, rate limits) Started by cmd.exe Started by Orca directly
main (2 runs) 7 7 0
this PR (2 runs) 7 0 7

Chat sessions were already started directly by Orca on both builds.

Testing

  • New codex-short-lived-app-server-spawn.test.ts: on win32 with an npm codex.cmd, the index-heal, trust-grant and model-catalog invocations hand the shim itself (not cmd.exe) to the session runner.

  • Updated the rate-limit fetcher and backfill-recovery tests to assert the bare shim reaches spawnProcess; the process-contract and exit-diagnostics tests now swap the stub in at spawnProcess instead of at getSpawnArgsForWindows.

  • Ablation: with only the source reverted, the 7 Windows-shape assertions fail; with the change they pass.

  • pnpm tc:node passes; the affected fetcher, recovery, index-heal, trust-grant and import-boundary suites pass (180 tests).

  • Windows 11 ("Windows high spec" box), npm Codex 0.157.1, Electron dev builds, isolated rig home, a guard codex.cmd (byte-for-byte npm shim) that logs each start's parent process:

    • Before, main at 4da485cac1c: the state-DB pre-create, the index pass, the model-catalog probe and the rate-limit probe were each started by cmd.exe (cmd.exe → node.exe → codex.exe). The chat's own app-server was started by electron.exe.
    • After, this PR at 11302c82f9b: across two runs, all 7 short-lived starts (and both chat starts) were started by electron.exe directly (electron.exe → node.exe → codex.exe), 0 via cmd.exe.
    • Every probe still works: the model picker lists the account's models, the history index reached 40 of 40, and the rate-limit probe ran its request and answered ("authentication required" for the rig's fabricated token, the same answer as on main). Each short-lived process exited on its own in about 1 s, and no rig process was left after the rig stopped the app.
    • Not exercised on Windows in this flow: hook trust grant (hooks off in the rig), the backfill recovery claimant and the default-home indexer (covered by unit tests only).
    • The later commits on this branch change comments (eb265fc9dcb) and a test ratchet fixture (281eb58705a) only.
  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Cross-platform: POSIX behaviour is unchanged (spawnProcess passes non-.cmd programs through as before). SSH/WSL: untouched paths. Kill switch: ORCA_DISABLE_CMD_SHIM_RESOLUTION restores the cmd.exe path for these launches too.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…d.exe

Orca's short-lived Codex app-server launches (history index passes, the
state-DB recovery claimant, hook trust grants, and the rate-limit and model
catalog probes) wrapped npm's codex.cmd in `cmd.exe /d /c` before handing it
to spawnProcess. That hid the shim from spawnProcess's resolver, so every one
of these launches went through cmd.exe instead of straight to node.

Hand the bare CLI path to the spawn chokepoint like the chat session already
does. The rate-limit probe and the recovery claimant also move off direct
node:child_process imports, shrinking that allowlist by two.
… pairing comments

Short-lived Codex app-servers now reach spawnProcess as the bare shim, so the
direct child is node for a resolved npm shim and cmd.exe only as the fallback.
…ole ratchet

The probe now spawns through spawnProcess, which always hides the console.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status: ready for review. Head 281eb58705a, CI green (18 pass, 13 skipped).

What this fixes: on Windows with npm-installed Codex, Orca's short background Codex processes (history indexing, the state-DB pre-create from #22971, the model-list and rate-limit probes, hook trust grants, the recovery claimant) each ran through an extra cmd.exe. Now they start the way chat sessions already did, straight to node → codex.exe. Nothing visible changes for users.

Review:

  • One independent senior review loop came back clean. It traced every short-lived Codex launch to the spawn, and checked that the node pairing, timeout kill of the node → codex.exe tree, stdin close, cwd and stdio behave the same as before. Its only change was two stale comments (eb265fc9dcb).
  • Readiness checklist at the start and end of review: no code findings. The end pass flagged four inexact sentences in the Windows evidence, now fixed in the body.
  • CI caught one thing the local run missed: a second ratchet (windows-console-visibility) still listed the rate-limit probe file as a direct node:child_process spawner. It is delisted and the pin is lowered 61 → 60 (281eb58705a).

Verification:

  • tc:node passes.
  • Unit tests for the affected fetcher, recovery, index-heal, trust-grant and ratchet suites pass.
  • With only the source reverted, the 7 Windows-shape assertions fail.
  • Windows 11 box: on main, every short-lived launch had cmd.exe as its parent. With this PR, 0 of 7 short-lived starts went through cmd.exe, and the picker, 40/40 history index and rate-limit probe all still work (details in the body).

Not verified on Windows: the hook trust grant, the backfill recovery claimant and the default-home indexer did not occur in the test flow. They use the same changed invocation builders and are covered by unit tests. Not tested with this PR and #23831 in one build.

Out of scope, possible follow-ups: commit-message generation (source-control-agent-launch.ts) still pre-wraps its CLI in cmd.exe before spawnProcess, the same defect in another feature. codex login and the rate-limit terminal fallback also still use cmd.exe.

Not merged; marking ready for a human to decide.

@brennanb2025
brennanb2025 marked this pull request as ready for review September 29, 2026 10:12
@coderabbitai

coderabbitai Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 52 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: c4d94084-d397-4de8-8c06-d66bddc06cb7

📥 Commits

Reviewing files that changed from the base of the PR and between c49388c and 281eb58.

📒 Files selected for processing (17)
  • src/main/codex/codex-app-server-process-tree-kill.ts
  • src/main/codex/codex-app-server-session.ts
  • src/main/codex/codex-model-catalog-probe.ts
  • src/main/codex/codex-session-index-heal.ts
  • src/main/codex/codex-short-lived-app-server-spawn.test.ts
  • src/main/codex/codex-state-db-backfill-recovery.test.ts
  • src/main/codex/codex-state-db-backfill-recovery.ts
  • src/main/codex/codex-trust-grant-host.ts
  • src/main/rate-limits/codex-fetcher-process-contract.test.ts
  • src/main/rate-limits/codex-fetcher-rpc-exit-diagnostics.test.ts
  • src/main/rate-limits/codex-fetcher-runtime-pairing.test.ts
  • src/main/rate-limits/codex-fetcher.test.ts
  • src/main/rate-limits/codex-fetcher.ts
  • src/main/rate-limits/codex-probe-termination.ts
  • src/shared/child-process/__fixtures__/child-process-import-allowlist.txt
  • src/shared/child-process/__fixtures__/windows-console-visibility-allowlist.txt
  • src/shared/child-process/windows-console-visibility.test.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Refactors how child processes spawn on Windows, removing cmd.exe wrapping.

The PR appears safe to merge; no actionable regression was identified.

Summary

The PR routes short-lived Codex app-server launches through the shared spawn chokepoint so recognized Windows npm/pnpm shims can launch without an intermediate cmd.exe. It also updates the affected tests and process-import allowlists.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Short-lived Codex caller] --> B[spawnProcess]
  B --> C{Recognized Windows shim?}
  C -->|Yes| D[node.exe and Codex script]
  C -->|No, batch target| E[cmd.exe fallback]
  C -->|Other program| F[Direct spawn]
Loading

Reviews (1) · Last reviewed commit: "test(child-process): delist the rate-lim..."

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Bare CLI handed to the spawn chokepoint. The five short-lived Codex callers (codex-session-index-heal.ts buildNativeHealInvocation, codex-model-catalog-probe.ts, codex-trust-grant-host.ts, codex-state-db-backfill-recovery.ts, codex-fetcher.ts fetchViaRpc) no longer pre-wrap the CLI with getSpawnArgsForWindows; they pass the resolved path (the npm codex.cmd shim) to spawnProcess / runCodexAppServerSession, so resolveSpawn resolves it to node.exe <script> and drops cmd.exe from the tree.
  • Two direct spawn call sites migrated. codex-state-db-backfill-recovery.ts and codex-fetcher.ts now route through the chokepoint, replacing Node's spawn; both files leave the child-process import allowlist, and codex-fetcher.ts leaves the Windows console-visibility allowlist (pin lowered 61 → 60).
  • Ratchet + test rewiring. The fetcher tests now mock spawnProcess instead of getSpawnArgsForWindows, the recovery test asserts the shim reaches the chokepoint, and codex-short-lived-app-server-spawn.test.ts pins the index-heal, trust-grant, and model-catalog invocations to the shim on win32.
  • Comment corrections. The cmd.exe-wrapper premises in the process-tree-kill, probe-termination, and CLI-pairing comments were updated to the launcher-agnostic wording.

Verification this run. All affected suites pass (98 + 61 tests), pnpm tc:node passes, and an ablation reverting codex-trust-grant-host.ts to the pre-wrap makes the new test fail with cmd.exe vs the shim — the new coverage is real, not vacuous. ORCA_DISABLE_CMD_SHIM_RESOLUTION still restores the cmd.exe fallback for these launches. One pre-existing, unamplified observation only: runCodexHookTrustGrantSession in codex-app-server-client.ts carries its own inline spawnProcess({ program, args, ...options }) adapter duplicating spawnCodexAppServerProcess; not introduced by this PR.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 merged commit 7f8ca49 into main Sep 30, 2026
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant